✨ feat(command palette): Add "Show All" and "Hide All" table visibility shortcuts and options in CommandPalette #4006
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Finished running flow.
|
||||||||||||
WalkthroughAdds "Show All Tables" and "Hide All Tables" commands to the Command Palette (with Shift+A and Shift+H shortcuts). Introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document as Document (keydown)
participant SubHook as useSubscribeTableVisibility
participant Visibility as useTableVisibility
participant Adapter as NuqsTestingAdapter
User->>Document: Press Shift+A or Shift+H
Document->>SubHook: keydown event
SubHook->>Visibility: call showAllNodes / hideAllNodes
Visibility->>Adapter: update node visibility
Adapter->>Adapter: invoke onUrlUpdate with new queryString
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tnyo43
changed the base branch from
main
to
refactor/visibility-control-hook
This comment was marked as outdated.
This comment was marked as outdated.
tnyo43
requested review from
MH4GF,
NoritakaIkeda,
junkisai and
sasamuku
and removed request for
a team
| commandName: string | ||
| } | ||
|
|
||
| // TODO: set gif or image for "Show All" and "Hide All" commands |
There was a problem hiding this comment.
You're right, No video is needed for now.
| <span className={styles.keyIcon}>⇧</span> | ||
| <span className={styles.keyIcon}>H</span> | ||
| </Command.Item> | ||
| )} |
There was a problem hiding this comment.
I noticed that Show All and Show All Fields were confusing. In the Left Pane, there was a Show All button within the section listing tables, so it seemed natural to omit the word "Tables".
I've thought about it and would like to request that you change the labels to Show All Tables and Hide All Tables.
There was a problem hiding this comment.
You're right, thank you for pointing that out. I fixed the literals in a435dd6
… confusion with 'Show All Fields' command
tnyo43
force-pushed
the
feat/show-and-hide-command
branch
from
7c716e2 to
a435dd6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsx (1)
3-7: CorrectonUrlUpdatemock typing and consider a default formockDefaultNodes
As in the hook test,
onUrlUpdateis declared asvi.fn<() => [UrlUpdateEvent]>()but is passed intoNuqsTestingAdapterand conceptually represents a handler receiving a singleUrlUpdateEvent. Typing it as something like:const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>()will better match both the adapter’s expected callback type and any future assertions you add around its arguments.
mockDefaultNodesis a nice way to control visibility scenarios. To avoid ever passingundefinedintoReactFlowProviderin tests that don’t set it explicitly, you might want to set a default once:mockDefaultNodes.mockReturnValue([])or use a fallback in the wrapper:
<ReactFlowProvider defaultNodes={mockDefaultNodes() ?? []}>That keeps the test harness types and runtime behavior a bit more explicit.
Please confirm the
onUrlUpdatecallback type against thenuqs/adapters/testingdefinitions to ensure this mock signature matches the actual API.Also applies to: 12-19, 52-136
🧹 Nitpick comments (1)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsx (1)
8-8: Align TODO wording with actual command labelsThe commands are labeled “Show All Tables” / “Hide All Tables”, so updating the TODO text to match (plural “Tables”) will keep searches and future edits consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/breezy-yaks-arrive.md(1 hunks)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsx(2 hunks)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsx(4 hunks)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsx(1 hunks)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.ts(2 hunks)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx(1 hunks)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.tsfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.tsfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts
frontend/packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Shared libraries and tools live under frontend/packages
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.tsfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.tsfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase and use TSX (e.g., App.tsx)
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests with filenames ending in .test.ts or .test.tsx colocated near source
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx
frontend/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow principles outlined in @docs/test-principles.md for tests
Files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-23T03:36:21.590Z
Learnt from: tnyo43
Repo: liam-hq/liam PR: 3129
File: frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/TableOptions.tsx:11-15
Timestamp: 2025-08-23T03:36:21.590Z
Learning: In the liam ERD codebase, CommandPalette.tsx has a 'use client' directive, ensuring that all CommandPalette child components (CommandPaletteContent, TableOptions, etc.) run only on the client side, making window object access safe without additional typeof checks.
Applied to files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.tsfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsx.changeset/breezy-yaks-arrive.mdfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts
📚 Learning: 2025-08-23T03:36:21.590Z
Learnt from: tnyo43
Repo: liam-hq/liam PR: 3129
File: frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/TableOptions.tsx:11-15
Timestamp: 2025-08-23T03:36:21.590Z
Learning: The CommandPalette and its child components (including CommandPaletteContent and TableOptions) in the ERD renderer are only rendered on the client side, so SSR safety checks for window object access are not necessary in this context.
Applied to files:
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.test.tsxfrontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPalettePreview/CommandPreview.tsx
🧬 Code graph analysis (3)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.ts (1)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts (1)
useSubscribeTableVisibility(4-30)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx (2)
frontend/packages/erd-core/src/stores/userEditing/Provider.tsx (1)
UserEditingProvider(47-250)frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts (1)
useSubscribeTableVisibility(4-30)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts (1)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/hooks/useTableVisibility/useTableVisibility.ts (1)
useTableVisibility(10-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: frontend-ci
🔇 Additional comments (3)
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeCommands.ts (1)
3-3: New visibility subscription is wired in correctlyImporting and invoking
useSubscribeTableVisibilityalongside the other subscription hooks keeps the “one hook to subscribe them all” pattern consistent and localized.Also applies to: 12-12
.changeset/breezy-yaks-arrive.md (1)
1-7: Changeset accurately documents the new commands and shortcutsThe summary clearly calls out the new “Show All Tables” / “Hide All Tables” commands and their ⇧A / ⇧H shortcuts, matching the UI and hook implementations.
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/CommandPaletteOptions/CommandOptions.tsx (1)
3-5: Show/Hide All Tables commands are correctly integratedUsing
visibilityStatusto conditionally render “Show All Tables” and “Hide All Tables”, and wiring each toshowAllNodes/hideAllNodesplussetOpen(false), cleanly mirrors the existing command patterns and avoids redundant options in each visibility state.Also applies to: 14-15, 25-25, 110-145
| const onUrlUpdate = vi.fn<() => [UrlUpdateEvent]>() | ||
|
|
||
| const wrapper = ({ children }: { children: ReactNode }) => ( | ||
| <NuqsTestingAdapter onUrlUpdate={onUrlUpdate}> | ||
| <ReactFlowProvider |
There was a problem hiding this comment.
🧩 Analysis chain
Fix onUrlUpdate mock typing and align comments with expectations
-
The mock is declared as
vi.fn<() => [UrlUpdateEvent]>(), but you pass it toNuqsTestingAdapterand assert it’s called with a singleUrlUpdateEventobject:expect(onUrlUpdate).toHaveBeenCalledWith( expect.objectContaining({ queryString: '...' }), )
This strongly suggests the signature should be something like:
const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>()
so TypeScript matches the adapter’s callback type and your usage.
-
In the second test (
shows all table nodes by ⇧A), the query string is'?hidden=', but the comment still says “hidden query parameter should be added with all node ids”. Consider updating that comment to reflect that the hidden list is cleared instead.
Please double‑check the NuqsTestingAdapter onUrlUpdate type in its docs or .d.ts to confirm the callback signature matches the suggested typing.
Also applies to: 48-55, 64-71
I'll verify the review comment by examining the test file and understanding the NuqsTestingAdapter type requirements.
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the test file to see current state
cat "frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx"
Length of output: 2109
🏁 Script executed:
#!/bin/bash
# Search for NuqsTestingAdapter type definitions in the codebase
rg -n "NuqsTestingAdapter" --type=ts --type=tsx -A 5 -B 2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for onUrlUpdate type definitions or imports
rg -n "onUrlUpdate" --type=ts --type=tsx -C 3Length of output: 27
I can see the test file and the typing issue. Let me search for the NuqsTestingAdapter type definition and verify the expected callback signature:
🏁 Script executed:
#!/bin/bash
# Search for NuqsTestingAdapter and onUrlUpdate with corrected rg syntax
rg -n "NuqsTestingAdapter|onUrlUpdate" -A 5 -B 2Length of output: 48526
🏁 Script executed:
#!/bin/bash
# Check if nuqs types are available in node_modules or package.json
find . -name "package.json" -type f | head -20 | xargs grep -l "nuqs"Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for type definitions or d.ts files related to nuqs
fd "nuqs" --type f -e d.ts -e ts | grep -E "(nuqs|testing)" | head -20Length of output: 0
🌐 Web query:
nuqs NuqsTestingAdapter onUrlUpdate callback signature type definition
💡 Result:
Signature (in TypeScript):
type UrlUpdateEvent = {
searchParams: URLSearchParams;
queryString: string;
options: {
history?: 'push' | 'replace';
shallow?: boolean;
scroll?: boolean;
clearOnDefault?: boolean;
// other per-update options supported by nuqs (hook/adapter-level options)
};
};
type OnUrlUpdateFunction = (event: UrlUpdateEvent) => void;
Explanation: the testing adapter calls onUrlUpdate with an object containing the new URLSearchParams, the rendered query string, and the options used for the update (e.g. options.history is 'push' in examples). Sources: [1], [2].
Sources:
[1] nuqs — Testing docs (shows UrlUpdateEvent properties and example).
[2] nuqs — Options docs (shows per-update options such as history/shallow/scroll).
Fix onUrlUpdate mock callback signature and correct misleading comment
The mock is typed as vi.fn<() => [UrlUpdateEvent]>() (no parameters, returns array), but the actual callback signature expected by NuqsTestingAdapter is (event: UrlUpdateEvent) => void. Update line 11 and all occurrences:
const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>()Additionally, the comment at line 64 contradicts the test expectation. The second test shows the hidden list being cleared (queryString: '?hidden='), not nodes being added. Update the comment to reflect this behavior.
Same typing issue appears in:
CommandPaletteOptions/CommandOptions.test.tsx(line 13)ERDRenderer/hooks/useTableVisibility/useTableVisibility.test.tsx(line 11)
🤖 Prompt for AI Agents
In
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.test.tsx
around lines 11–15, the onUrlUpdate mock is incorrectly typed as a no-arg
function returning an array; change it to the actual signature vi.fn<(event:
UrlUpdateEvent) => void>() and update any other occurrences in this file; also
update the misleading comment at line 64 to state that the hidden list is
cleared (queryString: '?hidden=') instead of nodes being added; apply the same
typing fix (vi.fn<(event: UrlUpdateEvent) => void>()) to the other files noted:
CommandPaletteOptions/CommandOptions.test.tsx (line 13) and
ERDRenderer/hooks/useTableVisibility/useTableVisibility.test.tsx (line 11).
| import { useEffect } from 'react' | ||
| import { useTableVisibility } from '../../../hooks' | ||
|
|
||
| export const useSubscribeTableVisibility = () => { | ||
| const { showAllNodes, hideAllNodes } = useTableVisibility() | ||
|
|
||
| // Show all nodes when ⇧A is pressed | ||
| useEffect(() => { | ||
| const down = async (event: KeyboardEvent) => { | ||
| if (event.code === 'KeyA' && event.shiftKey) { | ||
| showAllNodes() | ||
| } | ||
| } | ||
|
|
||
| document.addEventListener('keydown', down) | ||
| return () => document.removeEventListener('keydown', down) | ||
| }, [showAllNodes]) | ||
|
|
||
| // Hide all nodes when ⇧H is pressed | ||
| useEffect(() => { | ||
| const down = async (event: KeyboardEvent) => { | ||
| if (event.code === 'KeyH' && event.shiftKey) { | ||
| hideAllNodes() | ||
| } | ||
| } | ||
|
|
||
| document.addEventListener('keydown', down) | ||
| return () => document.removeEventListener('keydown', down) | ||
| }, [hideAllNodes]) | ||
| } |
There was a problem hiding this comment.
Guard global hotkeys against editable targets and simplify handlers
The global keydown listeners correctly wire ⇧A / ⇧H to showAllNodes / hideAllNodes, but they currently fire for any Shift+A/H while the ERD page is mounted — including when the user is typing in an <input>, <textarea>, or content‑editable field. That can be pretty surprising UX (e.g., hiding all tables while entering uppercase “A” or “H”).
Consider:
-
Early‑returning when the event originates from an editable element:
const isEditableTarget = (event: KeyboardEvent) => { const target = event.target as HTMLElement | null return ( target?.closest('input, textarea, [contenteditable=""], [contenteditable="true"]') != null ) } // inside the listener if (isEditableTarget(event)) return
-
Dropping the unnecessary
asynckeyword on the listeners since nothing is awaited. -
Optionally, combining the two effects into one
keydownhandler that switches onevent.codeto avoid registering two separate listeners ondocument.
Based on learnings, direct document access here is fine since the CommandPalette runs only on the client.
🤖 Prompt for AI Agents
In
frontend/packages/erd-core/src/features/erd/components/ERDRenderer/CommandPalette/hooks/useSubscribeCommands/useSubscribeTableVisibility.ts
lines 1-30, the two document keydown handlers fire while the user is typing in
inputs/textarea/contenteditable and both listeners are marked async
unnecessarily; update by replacing the two effects with a single keydown
listener that first guards editable targets (use event.target as HTMLElement and
return early when closest('input, textarea, [contenteditable=""],
[contenteditable="true"]') is non-null), drop the async keyword from the
handler, switch on event.code ('KeyA' and 'KeyH') and event.shiftKey to call
showAllNodes/hideAllNodes, and ensure a single
addEventListener/removeEventListener pair is used in the effect cleanup.
Issue
Why is this change needed?
Be aware with the merge target of this branch. Please review #4007 before reviewing this PR.
This PR will introduce "Show All Tables" and "Hide All Tables" command options in the
CommandPalette. Also, it will set shortcuts for these actions.It will subscribe shortcut commands as well, ⇧A for "Show All Tables" and ⇧H for "Hide All Tables".
Screen.Recording.0007-11-17.at.9.20.45.mov
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.