Skip to content

Query viewer: BaseNodeViewer-style title, gated Kanban, Default view stays Default - #3274

Merged
mstomar125 merged 2 commits into
mainfrom
issue-3261-query-viewer-ux
Sep 29, 2026
Merged

mstomar125 merged 2 commits into
mainfrom
issue-3261-query-viewer-ux

Conversation

@mstomar125

Copy link
Copy Markdown
Contributor

Closes #3261

  • Shared HeaderInput (borderless 2rem/500, always-present input) used by BaseNodeViewer and the query viewer; commit on blur/Enter, Escape cancels.
  • Removed the N-items pill, Edit Query button, edit-mode state/panel/CSS, and the now-unused QueryEditor component, model and test.
  • Kanban tab disabled with tooltip when eligibleGroupByFields is empty; a saved/stored kanban view on such a type opens in List.
  • Default view's view and group-by persist per type in localStorage; view/group-by switches no longer create a query node. Only a rename materializes; the title stays Default until then.

Tests: rewrote the materialize-race tests, added Kanban gating, no-pill/no-Edit-Query, default-view-prefs and HeaderInput tests.

🤖 Generated with Claude Code

@mstomar125

Copy link
Copy Markdown
Contributor Author

Code Review Summary (PR #3274 / issue #3261, reviewed at 46ecfbbe)

Verdict: APPROVE, with two suggested improvements that don't block merge. The change covers all seven acceptance criteria of #3261 and leaves the code cleaner (about 900 lines of dead editor code deleted). I ran the four affected test files locally: 17/17 pass. No CI checks are reported on the branch, so run the local gate before merging.

Acceptance criteria

  • Title: HeaderInput is shared by BaseNodeViewer and the query viewer, with the styles moved into the component (borderless, 2rem/500), so the two can't drift apart.
  • "N items" pill and its CSS removed.
  • Edit Query button, isEditMode, the edit panel and its CSS removed, along with QueryEditor, its model and its test. The removal is complete.
  • Kanban tab is disabled with a tooltip when eligibleGroupByFields is empty. A saved or stored lastView: kanban opens in List (resolveEffectiveView), and nothing is written back, which is correct.
  • A view or group-by switch on Default writes per-type localStorage and never calls materializeQuery.
  • The title stays "Default" until a rename.
  • Tests for Kanban gating and for "a view switch doesn't materialize" are present.

HeaderInput extraction (BaseNodeViewer): behavior is preserved. id, value expression, oninput gate, onfocus/onblur, readonly={hasTitleTemplate}, the placeholder and the aria-label all carry over unchanged. The .header-input--readonly class now comes from the readonly prop, so the title-template styling and the readonly attribute match the old behavior. No other file relied on the old scoped .header-input CSS.

Findings

Critical Issues

None.

Suggested Improvements

  1. query-node-viewer.svelte handleTitleFocus: focusing the Default title puts the text "Default" into the draft.
    titleDraft = displayTitle means a click on "Default" leaves the caret inside the word. Typing "Sprint" produces "DefaultSprint", and on blur that becomes a materialized query node with that name. The old startEditTitle deliberately started empty for the Default view ("The default's placeholder 'Default' is not a real name — start from empty"), and that intent was lost here. The issue asks for "click and type", and BaseNodeViewer handles an unnamed node by showing an empty value with a placeholder. Suggestion: in default mode render value='' with placeholder="Default" (you already pass DEFAULT_QUERY_TITLE as the placeholder). If you want to keep the current text look, select all on focus instead. Either way, the first keystroke replaces "Default" rather than appending to it.

  2. The Escape and Enter paths on the new always-present input have no tests.
    Escape changed from flipping isEditingTitle to titleEditCancelled = true followed by blur(), so correctness now depends on the blur-then-guard ordering. The rewrite deleted the test that reached the old path, and nothing replaced it. If titleEditCancelled = true is removed, every test still passes, but a cancelled edit would materialize a node. Add one viewer test: focus the title, type, press Escape, then assert that mockCreateNode was not called and the value is back to "Default". Add a second: type, press Enter, then assert the node is created with the typed name. header-input.test.ts also doesn't cover forwarding of onkeydown.

Nitpicks

  • Nit: commitTitle sets isEditingTitle = false before await materializeQuery(...), so the input briefly shows "Default" before the reroute. If materialize fails, the typed name is lost; only the error banner remains. You could clear the editing state after the await, or keep it on failure.
  • Nit: dead code left behind:
    • materializeQuery's definition and viewConfig options, and its ?? MATERIALIZED_QUERY_TITLE fallback, have no remaining callers. The only call site passes content.
    • backendAdapter.countQuery (all three adapters plus adapter-core) has no frontend caller now that handleQueryPreview is gone. Keep it if the backend route is wanted for later GUI editing, but note that it's intentionally dormant.
    • kanban-view.svelte's noEnumField hint and empty-state branch can't be reached from this viewer any more. It's fine as a defensive fallback.
  • Nit: saveDefaultViewPrefs(targetType, …) can run while targetType === '' if a tab is clicked before the load resolves (mode also starts as 'default' for a saved query that is still loading). The result is a harmless junk key, nodespace:default-view:. Fix: if (!targetType) return;.
  • Nit: the localStorage key is scoped by type id only, so two databases (ADR-053 multi-DB) that share a type id, including built-ins, share the Default-view preference. Consider adding the active database id to the key.
  • Nit: the disabled Kanban reason is only in title on the wrapper <span>, which is correct for hover because disabled buttons don't fire mouse events. Screen readers get no reason, though. Adding aria-describedby, or the same title on the button, would fix that cheaply.

Test quality: good overall.

  • The "view/group-by switch creates no node" test would fail if the fix were reverted, because the old handleViewChange called materializeQuery, which calls createNode. It also checks the per-type restore on remount.
  • The Kanban-disabled and saved-kanban-falls-back-to-List tests are direct and would fail on revert.
  • The store-eviction test now uses the pinReachableNodes spy instead of the removed pill text. That's a reasonable proxy, since the viewer pins exactly its loaded ids, and it still has its positive control.

@mstomar125

Copy link
Copy Markdown
Contributor Author

Addressed review (ebf2c87):

  • Default title now starts empty on focus (placeholder 'Default'), so typing no longer appends; saved query starts from its name. Tested.
  • Added Escape (no node created; verified the test fails if the cancel flag is removed) and Enter (materializes with typed name) tests.
  • Guarded view/group-by prefs writes against an unloaded type id; removed unused materializeQuery options/fallback.
  • Not changed: storage key not scoped per database (type ids are stable names; acceptable), disabled-Kanban reason is tooltip-only, materialize-failure title flicker, countQuery adapter method left in place (backend API surface). No re-review needed: changes are small and covered by tests.

mstomar125 and others added 2 commits September 29, 2026 08:37
…e on view switch (closes #3261)

- Extract HeaderInput shared by BaseNodeViewer and QueryNodeViewer
- Remove the item-count pill, Edit Query button, edit mode and QueryEditor
- Disable the Kanban tab with an explanation when no enum field exists;
  a stored kanban view on such a type opens in List
- Persist the Default view's view/group-by per type in browser storage
  instead of materializing a query node; only a rename materializes

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, trim materialize options, cover Escape/Enter

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mstomar125
mstomar125 force-pushed the issue-3261-query-viewer-ux branch from ebf2c87 to cbcdf71 Compare September 29, 2026 06:37
@mstomar125
mstomar125 merged commit a6cf60a into main Sep 29, 2026
@mstomar125
mstomar125 deleted the issue-3261-query-viewer-ux branch September 29, 2026 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query/type viewer: BaseNodeViewer-style title, drop count pill and Edit Query, gated Kanban tab, keep 'Default' on view switch

1 participant