feat: command palette for actions and database objects - #545
Conversation
verbaux
changed the title
a8ef2b7 to
a8c3ac6
Compare
|
Hi @verbaux , first off, sorry for taking so long to get back to you. This PR deserved a quicker reply, especially since you left such clear questions. And thank you for the work here, the navigation contract cleanup alone is something we've needed for a while. Going through your points: Scope registration. Keep the store. The palette is a single modal at the root while the scopes live in the panes, so a context-based approach would force the palette inside the active pane's provider, which doesn't really work with one modal and N panes. The store with Route state as the contract. Fine by me. You clear the state right after consuming it and dedupe via the navigation key, so refresh and back don't re-run anything. And since split panes execute the intent directly without going through the router, route state stays what it should be: the cross-route contract to the main editor. No rework needed. Mode indication. Let's do the cheap fix in this PR, a visible mode label in the header. The placeholder disappearing as soon as you type isn't enough of a cue. On the bigger question, I wouldn't block this PR on it. The shared SpotlightPalette shell makes merging the two surfaces into one palette cheap later, so keep the two shortcuts for now and we can revisit once there are more commands to justify a mode switcher. Initial command set. A few that should fit the current CommandScope without extending anything, since they already exist as object actions: new SQL console at connection level (not just "open current table in console"), inspect / show schema for the current table, generate SQL, count rows. Plus "open connection manager" as pure navigation, same shape as open settings. I agree switching connections is the right first candidate for extending the scope, but I'd leave that for a follow-up rather than grow this diff. On the known limitation with Happy to take this out of draft whenever you feel ready, I'll do a proper pass with real data on the split view scenarios then. Thanks again for the thorough writeup, it made this much easier to review. |
|
@debba Thanks for the detailed pass. Went through your points in order. Scope registrationKeeping the store. On your question about the split view — the same connection cannot end up in two panes, and not by convention: the connection id already is the pane identity.
So a pane id would be a second name for the value we already use. If opening one connection in two panes ever becomes a feature, the layout tree has to grow a pane identity first, and the scope id follows it there — a small change in getActiveCommandScopeId once that exists. Mode indicationAdded a visible badge in the header carrying the dialog's accessible name — COMMAND PALETTE / DATABASE OBJECTS. It is aria-hidden, since the dialog's own aria-label already announces the mode. Command setAdded all five, each reusing an action that already exists on database objects, with CommandScope untouched:
Inspect and generate SQL open modals that CommandPaletteModal owns rather than navigating, so they arrive as a separate argument to createBuiltInCommandItems instead of going through CommandScope.runtime — that is what keeps the scope as it was. Left switching connections out, as you suggested. configuredDatabasesFiled as #591, linking both PRs. It turned out to be a bit worse than "the list can go stale": in all-databases mode the palette lists nothing at all, because the configured list is empty by design there and getNavigatorItems builds its groups exclusively from it. Taking it out of draft. What I exercised by hand against a live PostgreSQL connection: the badge in both modes, the four table commands (the inspect modal renders over the closing palette, and count rows opens a console that runs and returns), and the two navigation commands. I have not driven the split-view scenarios with two panes — that part is still only covered by tests, so it is worth your pass with real data. |
|
Found one platform-specific issue while testing on Linux, plus a follow-up question. 1. Monaco's "Toggle Block Comment" is The repo already has the right pattern for this: 2. On the two shortcuts. We agreed to keep |
Overlay clicks are ignored unless a caller opts in with closeOnBackdrop, so existing modals keep their current behaviour.
Routes that own the whole pane (root, connections, settings) never show a split view, so the decision lives next to the layout model instead of the components that render it.
Both palettes and the explorer sidebar used to build their own editor navigation, each with a slightly different idea of what "open this object" means. Route state now carries a typed EditorNavigationRequest, and the sidebar, the palettes and the schema/SQL modals all go through it. - Extract database object actions and editor navigation into utilities, exposed to components through useDatabaseObjectNavigation. - Drop the separate actions/objects palette providers in favour of item hooks over a single palette context. - Move the schema and generate-SQL modals onto an explicit TableTarget, so they no longer read the active connection behind the caller's back.
The locale landed upstream while this branch was in flight, so it was the only one missing the palette keys.
The placeholder was the only cue for which palette had opened, and it disappears as soon as the user types. Render the dialog's accessible name as a visible badge instead, hidden from screen readers so the mode is not announced twice.
Rounds out the initial command set with actions that already exist on database objects: open the connection manager, open a console on the current connection, and inspect, generate SQL for, or count the rows of the table in scope. Inspect and generate SQL open modals that CommandPaletteModal owns, so they arrive as a separate argument rather than through CommandScope, which stays as it is. Their labels reuse the object palette's, so only the connection category and the connection manager needed translating.
One comment described how the superseded quick navigator treated views; the other repeated what the docblock four lines above already said.
The palette resolves its scope from the focused connection. Behind a split layout MainLayout renders SplitPaneLayout instead of <Outlet/>, so the routed Editor is unmounted and anything reaching it through the router is dropped. - GenerateSQLModal takes an optional openEditor, supplied from the active scope, and keeps router navigation as the fallback for the explorer - a missing connection dialect now surfaces inline instead of guessing a dialect or firing an alert behind the modal - failed object loads are reported in the palette and the report is dropped again once the schema or database finally loads - the palette header names the open mode, and Ctrl+Shift+A reaches it from inside Monaco on Linux without losing toggle block comment Tests cover the scope routing end to end, the load-failure recovery, and the keyboard and focus behaviour of the shared palette shell.
Point the agent rules at the current gitnexus tool names and drop the stale runner instructions. Ignore the scratch directories local tooling leaves in the working tree so they cannot be committed by accident.
The banner arrived with the rebase and reads the database context, which this suite does not provide — it mounts MainLayout with its neighbours stubbed to assert the palette host alone.
Opening the palette under a resting cursor highlighted the row underneath it. Two separate causes stacked up: the row fired mouse enter without the user moving the mouse, and its CSS hover painted the selected background on top of the row activeIndex already held, so two rows looked active at once. Selection now has a single source. The pointer routes through activeIndex via mouse move, which needs real cursor travel, and the hover styling is gone.
8a875d9 to
b6d19ca
Compare
|
@debba Both done, plus a few things from testing since. Linux On the threshold. I don't think it's a command count, and I'd rather not go the VS Code route — typing > to pick a mode is the thing I don't want people thinking about. What I keep coming back to is JetBrains: direct shortcuts for when you know what you want (⌘P, ⌘O, ⌘⇧A), plus Search Everywhere with tabs on top for when you don't. Which reframes your question — it's not "when do we collapse the two surfaces". Under that model ⌘P and ⌘⇧A stay exactly as they are and become the direct entries; the merged palette is a third thing added over them, with an All tab for when you're not sure what you're looking for. So my answer: nothing to collapse, and the next step isn't a mode switcher. It's making the per-object actions we already attach — inspect, new console, generate SQL, count — reachable from the keyboard, since today they're click-or-Tab only. Three things testing turned up, all fixed here:
Tested by hand: split view with two Postgres connections — the object palette resolves to the focused pane, Generate SQL reads that pane's connection, and "Run in console" lands in that pane. Not exercised: SQLite/MySQL without schemas, and the read-only routine/trigger definitions. One thing to know before your pass: in split view the palette follows whichever pane has explorer focus, and there's no keyboard way to move it — explorerConnectionId only changes on a click, and ⌘⇧1…9 switches the active connection without touching it. So it's "click the pane, then open the palette". I'd fix that in a follow-up rather than grow this diff: show the target connection in the header, add a pane-focus shortcut. Can file it. Rebased onto current main, 29 commits, force-pushed. That dropped your Merge branch 'main' (8a875d9) — the main it brought in is an ancestor of what I rebased onto, and the merge had no conflict resolutions of its own, so nothing was lost. |
2 participants
Summary
Replaces
QuickNavigatorModalwith a Spotlight-style palette that has two (for now) modes: object search (tables, views, routines, triggers) and an action palette onCmd/Ctrl+Shift+A.The part worth reviewing is not the UI. Both palettes and the explorer sidebar each used to build their own "open this in the editor" logic — assembling SQL, guessing the tab type, and reading the active connection regardless of which pane the user clicked in. This PR gives them one contract to share.
What changed
Navigation contract
src/utils/editorNavigation.ts— router state now carries a typedEditorNavigationRequest;Editor.tsxparses it and opens the tab.src/utils/databaseObjectActions.ts+src/hooks/useDatabaseObjectNavigation.ts— one definition of what "open / count / show definition" means per object type. The sidebar's 16 call sites and both palettes go through it.Palette
src/utils/commandScopeStore.ts+src/hooks/useCommandPaletteScope.ts— each pane registers a command scope; the palette resolves against the active one. This is what makes split view target the right connection.src/components/ui/SpotlightPalette.tsx— shared shell (focus trap, arrow navigation, ARIA combobox/listbox), used by both modes.src/utils/paletteItems.ts+objectPaletteItems.ts— single item pipeline for search, grouping and ranking. Uses Fuse.js, already a dependency.Modals no longer read the active connection
SchemaModalandGenerateSQLModaltake an explicitTableTarget { connectionId, tableName, schema }. Before this, opening either from a non-active split pane inspected the wrong connection.Shortcut
command_palette_actions—Cmd+Shift+A/Ctrl+Shift+A, overridable in settings.Test plan
pnpm typecheckcleanpnpm lintcleanpnpm test— 3658/3658 pass, 216 filesKnown limitations
The object palette builds its multi-database list from the saved
connection.params.database(src/hooks/useCommandPaletteObjectItems.ts:66), while #524/#530 reconcile dropped databases intoselectedDatabasesonly. A database dropped on the server can still show up in the palette until the connection list reloads.Not introduced here —
QuickNavigatorModalread the same source, andquickNavigator.tsiteratedconfiguredDatabasesonmaintoo. Left as-is because the fix is a one-line source swap but the multi-database path has no test fixture yet.Open questions
Draft because I'd rather hear about the approach before polishing:
location.state.⌘Pobjects,⌘⇧Aactions) or become one palette with switchable modes, DataGrip-style.Open settingsandOpen current table in SQL console— enough to exercise the registry, the scoping and the navigation contract without padding the diff. Adding a third is onePaletteItemincreateBuiltInCommandItems(src/utils/builtInCommands.ts) plus an i18n key. Anything that needs more thanconnectionId/driver/tableextendsCommandScopefirst — switching connections is the obvious next one and the first case that would need that. Happy to take a list of what you'd want in the initial set.