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 |
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. |
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.
b6d19ca to
ffae683
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }); | ||
| setExplorerConnectionId(prev => (prev === connectionId ? null : prev)); | ||
| }, []); | ||
| const next = splitView |
There was a problem hiding this comment.
CRITICAL: removeConnectionFromSplit reads splitView from the closure instead of using a functional state update. If two panel-close gestures are batched in the same tick, the second callback reads the stale splitView from the original closure and computes the removal against the already-stale layout. The developer's own comment acknowledges this. This can leave an incorrect split layout after rapid multi-panel removal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Acknowledged and intentional — the tradeoff is documented at useConnectionLayout.ts:74-77. The next layout is needed outside the updater to settle visibility and focus in the same pass.
Both call sites are single user gestures: SplitPaneLayout.tsx:75 (close button, and only reached with ≥3 panels — 2 panels take the deactivateSplit() branch at :69-72) and ConnectionGroupItem.tsx:99 (context menu item). React flushes discrete events separately, so two removals cannot land in one tick from the UI, and there is no programmatic multi-remove in the codebase. If one ever appears, the fix is the reducer noted in the comment.
| @@ -15,29 +15,27 @@ interface TableColumn { | |||
| interface SchemaModalProps { | |||
There was a problem hiding this comment.
WARNING: Breaking API change. The prop interface changed from tableName: string; schema?: string | null to target: TableTarget. All internal call sites in this PR were updated, but any external consumer, plugin, or test that renders <SchemaModal tableName="..." schema={...} /> will now fail at compile time or runtime.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
There are no external consumers.
| import type { CommandRuntime } from "../../types/commands"; | ||
| import { openEditor as navigateToEditor } from "../../utils/editorNavigation"; | ||
|
|
||
| interface GenerateSQLModalProps { |
There was a problem hiding this comment.
WARNING: Breaking API change. The prop interface changed from tableName: string to target: TableTarget. Same concern as SchemaModal.tsx: external callers using the old prop shape will break.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Same as SchemaModal — private package, no published surface.
| const connectionId = scope?.connectionId ?? null; | ||
| const requestDatabaseData = useEffectEvent(loadDatabaseData); | ||
| const requestSchemaData = useEffectEvent(loadSchemaData); | ||
| const loadAttemptsRef = useRef(new Map<string, number>()); |
There was a problem hiding this comment.
WARNING: loadAttemptsRef is a useRef(new Map<string, number>()) that is never cleared when connectionId changes. The keys are namespaced by connection, so they do not collide, but the map grows unbounded across connection switches. Over a long session with many connections, this leaks memory with stale attempt counters that are never evicted.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
No unbounded growth here. Entries are deleted on successful load (:112, :134), keys are namespaced per connection (:110, :132), and the hook only lives while the objects palette is open — ObjectPalette mounts conditionally at CommandPaletteModal.tsx:90-95, so the ref is recreated on every open. There is no long-lived instance for the map to accumulate in.
| ? "sidebar.failGetRoutineDefinition" | ||
| : "sidebar.failGetTriggerDefinition", | ||
| ) + String(error), | ||
| { kind: "error" }, |
There was a problem hiding this comment.
WARNING: showDefinitionError concatenates the translated label with String(error). For Error objects this includes the full stack trace, producing very long alert messages that expose internal file paths, line numbers, and framework internals to the user. The console.error(error) call already logs the raw error for debugging.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
This is carry-over of the existing pattern (ExplorerSidebar.tsx:2277, and ExplorerSidebar.tsx:461 on main), not new behavior
| window.dispatchEvent(new CustomEvent("tabularis:paste-import")); | ||
| return; | ||
| } | ||
| if (matchesShortcut(e, "quick_navigator")) { |
There was a problem hiding this comment.
WARNING: The quick_navigator shortcut handler now calls togglePalette("objects") instead of dispatching the tabularis:open-quick-navigator custom event. The old custom event is removed entirely. Any external code, plugins, or tests listening for tabularis:open-quick-navigator will silently stop working.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
tabularis:open-quick-navigator has zero references left in the repository.
| const navigate = useNavigate(); | ||
| const { matchesShortcut, isMac } = useKeybindings(); | ||
| const { openConnections, handleSwitch } = useConnectionManager(); | ||
| const { togglePalette } = useCommandPaletteDispatch(); |
There was a problem hiding this comment.
WARNING: useCommandPaletteDispatch() is now called inside useGlobalShortcuts, and this hook throws if not wrapped in CommandPaletteProvider. Previously, useGlobalShortcuts only required KeybindingsProvider and BrowserRouter. This is a new implicit dependency. While the sole caller (MainLayout.tsx) is wrapped in CommandPaletteProvider, any future consumer of useGlobalShortcuts outside that provider will crash the app.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fair catch on the documentation. The dependency itself is fine — useCommandPaletteDispatch throws loudly (useCommandPalette.ts:20-23) rather than failing silently, and the only caller is wrapped (MainLayout.tsx:47-51). But the JSDoc at :15-17 still lists only KeybindingsProvider and BrowserRouter. Updating it to include CommandPaletteProvider.
Code Review SummaryStatus: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit c100325)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c100325)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit ffae683)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (7 files)
Reviewed by glm-5.2 · Input: 45.7K · Output: 18.4K · Cached: 573.2K |
useGlobalShortcuts now calls useCommandPaletteDispatch, which throws outside CommandPaletteProvider. The JSDoc still listed only KeybindingsProvider and BrowserRouter.
|
Hi @verbaux, Sorry for the delay, I’ve been busy with work and some other stuff in Tabularis. I’ll do a final review today and merge it if everything is ready on your side. I saw you’re arguing with Kilobot, it’s not worth it. |
|
Hi @debba My replies to Kilobot for anyone reviewing this pull request, in case they have any doubts. |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
I love this feature and how it's implemented. The navigation contract is the right call: I verified locally that every editor navigation in the codebase now goes through the typed request, the split-pane scoping works exactly as described, and removing the silent ?? "sqlite" dialect fallback in GenerateSQLModal is a real fix on top. Typecheck, lint and the full suite (3707 tests) pass on the branch, and i18n is complete across all 11 locales.
From my side this is ready to merge. Just two things that look like local tooling that slipped into the branch, please check them:
- AGENTS.md: the GitNexus managed block got regenerated by a different local setup. It drops the
explainsecurity-review line and thedetect_changes({scope: "compare", base_ref: "main"})guidance, and replaces thenode .gitnexus/run.cjs analyzeinstructions. Please restore the block as it is on main. - .gitignore: the new entries (
.foglamp/,.grove/,.groverc.json,.pnpm-store/,skills-lock.json,tasks/,*.local.md) are your local tools, not project artifacts. Broad patterns liketasks/could silently hide real project files later. Please drop them (a global gitignore on your machine covers these better).
Once those two files are back to main's version, happy to merge immediately. Thanks for the excellent work!
The gitnexus block was regenerated by a different local setup and the ignore entries were personal tooling, not project artifacts. Both are back to main's version per review on TabularisDB#545; the local patterns moved to .git/info/exclude.
|
@debba my bad, with a large number of changes, it’s easy to miss these unwanted changes. I rolled them back to their original state. |
|
No worry. |
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.