feat(editor): window controls and detachable results panel#369
Conversation
Replace the single collapse chevron in the results panel header with the usual window-style controls — minimize, maximize, close — and add a detach button that pops the results into their own window. Manual drag-resize of the panel is unchanged. - minimize / close collapse the panel (no data loss); the "Show Results" button brings it back - maximize hides the editor so results take the full height; the same button restores it - detach opens a separate window for the active tab's results; the in-app panel shows a "results detached" placeholder with a re-attach button Each detached window is bound to the tab it was opened from, so it keeps showing that tab's results even when you switch tabs, and you can detach several tabs into several windows at once. The main window stays the single source of truth: it pushes result state to each window over Tauri events keyed by tab id, and the windows forward user actions (pagination, page change, entry select/close/rename, load count) back to be applied to the right tab. Closing a tab closes its detached window.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return tab && tab.results ? tab : null; | ||
| }; | ||
| return { | ||
| onRunQueryPage: (query, page) => runQuery(query, page, tabId), |
There was a problem hiding this comment.
WARNING: Detached-window query runs mutate the main window's global results-panel state
onRunQueryPage forwards page changes from the detached window to runQuery(query, page, tabId). Inside runQuery, setIsResultsCollapsed(false) is toggled unconditionally (line ~698). When the detached tab is not the active tab in the main window, this still expands the main window's results panel for whatever tab is currently active.
Scope the collapse state change to the target tab, or skip it when the action originates from a non-active detached tab.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| onRunQueryPage: (query, page) => runQuery(query, page, tabId), | ||
| onPageChange: (entryId, page) => runResultEntryPage(entryId, page, tabId), | ||
| onRerunEntry: (entryId) => runResultEntryPage(entryId, 1, tabId), | ||
| onLoadCount: () => loadCount(tabId), |
There was a problem hiding this comment.
WARNING: Detached-window row-count loading affects the active tab's UI
onLoadCount calls loadCount(tabId), which sets the global isCountLoading flag and later clears it in a finally block. If the detached tab is not the active tab in the main window, the spinner and disabled state are shown on the active tab's load-count button instead of the detached tab's.
Use a tab-scoped loading indicator for count operations triggered from detached windows.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All 3 previous review warnings have been addressed in the latest commits. Files Reviewed (1 file)
Previous Review Summaries (3 snapshots, latest commit 28ce2e9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 28ce2e9)Status: No Issues Found | Recommendation: Merge All 3 previous review warnings have been addressed in the latest commits. Files Reviewed (1 file)
Previous review (commit 11470e1)Status: No Issues Found | Recommendation: Merge All 3 previous review warnings have been addressed in the latest commits. Files Reviewed (1 file)
Previous review (commit e9196af)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (16 files)
Reviewed by laguna-xs.2-20260421:free · Input: 534.4K · Output: 6.8K · Cached: 145.8K |
There was a problem hiding this comment.
Reviewed locally — really nice feature, and cleanly built: the action handlers are properly tab-keyed, the main window stays the source of truth, and no query/DB logic is duplicated. Unit tests pass (11/11), tsc clean, i18n complete across all 8 locales, and the new Tauri commands are registered. 🙏
I can confirm all three of @kilo-code-bot's warnings are real — they're shared main-window UI state leaking into actions a detached window triggers, so no data loss, but worth tidying:
Editor.tsx:698—runQuerycallssetIsResultsCollapsed(false)(and would popsetQueryParamsModalfor parameterised queries), so a re-run/page-change from a detached window re-expands the main panel — which slightly defeats the point of detaching. This is the one I'd fix before merge.Editor.tsx:1095—loadCountflips the globalsetIsCountLoading, so a detached count-load shows a spinner on the active tab. Cosmetic.Editor.tsx:1292—applyActionhas nodetachedTabIds.has(tabId)guard; low risk since events only come from app-owned windows, but cheap defense-in-depth.
#2 and #3 are polish and fine as follow-ups. Approving — just flagging #1 as worth a look first.
|
Thanks for the thorough review! Fixed all three — they boiled down to detached-window actions leaking into main-window-only UI state, so I gated each on whether the target tab is actually detached. To do that without re-creating the callbacks or reading stale closures, I added a
Build, |
# Conflicts: # src/pages/Editor.tsx
NewtTheWolf
force-pushed
the
feat/detachable-query-results
branch
from
28ce2e9 to
3ed7ab6
Compare
|
Re-reviewed locally after merging
Want me to push fixes for any of these, or merge and follow up separately? |
…race - Orphan-cleanup effect no longer prunes detachedTabIds synchronously; closing the window emits results-window:closed and the listener owns pruning, keeping the effect side-effect-only. - Register the ready/action/closed listeners unconditionally instead of gating behind detachedTabIds.size > 0, so a freshly opened window's ready handshake can't be missed (was leaving the window stuck on "Loading…"). Each handler self-guards, and detachedTabIds is dropped from the deps to avoid needless listener re-registration.
- Remember window bounds per tab (HashMap keyed by tab_id) instead of a single global slot, so multiple simultaneously-detached windows no longer all reopen stacked on top of the last-closed one. - Save inner_size() instead of outer_size() on close; bounds are restored via .inner_size(...), so saving the outer size grew the window by the decoration height on every detach->close->reopen cycle. - Test the pure window_label() helper and per-tab bounds storage.
|
Thanks for the thorough re-review! I've addressed all five points in two commits (
|
2 participants
What this does
The query results panel had a single chevron to collapse it and not much else. This PR turns the panel header into the familiar set of window controls and adds the ability to pop results out into their own window.
Manual drag-to-resize of the panel is untouched.
Window controls (right side of the results bar)
Detachable results
Detach pops the active tab's results into their own OS window. While detached, the in-app panel shows a "results detached" placeholder with a Re-attach button.
Each window is bound to the tab it was opened from, so:
The main window stays the single source of truth. It pushes result state to each detached window over Tauri events keyed by tab id, and the windows send user actions back (pagination, page change, entry select / close / rename, load count) to be applied to the right tab. No query or DB logic is duplicated in the detached window — it renders the existing
MultiResultPanel/ResultEntryContentand forwards everything else.How it's built
src-tauri/src/results_window.rs—open_results_window/close_results_windowcommands; one window per tab (labelresults-window-{tabId}), remembered bounds, emitsresults-window:closedon close.src/utils/resultsWindowSync.ts— the event protocol (sync payload, action envelope) plus pure helpers (buildSyncPayload,applyAction,singleResultToEntry), with unit tests.src/pages/ResultsWindowPage.tsx— the detached window, reuses the existing result components.src/pages/Editor.tsx— the header controls and the sync/action wiring.Scope / not in this PR
In-place row editing (insert / delete / pending changes) is intentionally not available from inside the detached window for now — that machinery is tightly coupled to the main editor. In-grid scroll, sort, cell copy and pagination all work.
Testing
npx tsc --noEmit— cleannpm run build— cleannpm test(resultsWindowSyncplus the full suite) — greencargo checkandcargo test results_window— greenContext
Framed as part of the UI consistency effort in #195. This isn't a design-system change in itself, but it brings the results panel in line with conventional window affordances and adds a feature that was awkward to do before. Relates to #195.