feat(#429): unify Dashboard creation and add the metadata rename pencil (phase 3) - #495
Merged
Merged
Conversation
…il (phase 3) Absorbs #481. `openNameDialog`/`openDialogShell` move out of file-menu.ts into a shared src/ui/dialog-shell.ts (generalized returnFocusTo + a single-slot staleness-guarded teardown) so the Dashboard tree's new rename pencil can reuse them. File > New dashboard... and the empty-workspace placeholder now both prompt and commit through appendDashboard, never the compatibility slot. A new renameDashboard planner + commitDashboardRename wrapper back a two-field title/description dialog on each Dashboard row's pencil. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
This was referenced Jul 27, 2026
Open
This was referenced Jul 27, 2026
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
… create (Altinity#495 review) Post-merge review of PR Altinity#495 raised four defects; all four are fixed here, and three of them are foundations Altinity#494 (phase 4) builds three more row buttons and two more dialogs on. 1. Enter on a nested action button ran the ROW's command. The tree's keydown handler is on the list and its Enter arm calls preventDefault() + runs the focused row's single command, so Enter on the pencil opened the Dashboard — and the preventDefault() could swallow the button's own activation on the way out. The `⋯` and the orphan-variable trash shared the bug. Fixed in two independent layers: `isolateActivationKeys` stops Enter/Space propagating from each control (without preventing the default, so native activation still fires exactly once), and `handleTreeKeydown` ignores an Enter that originated on a button. Arrow/Home/End still reach the tree from a nested control, which is what keeps the row's composite tab stop navigable. 2. Rename failures closed the dialog and discarded the outcome. `commit()` now awaits `commitDashboardRename` and keeps the card open with the typed values on every unsuccessful outcome, showing one targeted diagnostic inline (`role="alert"`): a distinct sentence for a Dashboard that no longer resolves vs. the aggregate's own rejection diagnostic. Both actions are disabled while a write is in flight, so the same mutation cannot be submitted twice, and a late answer for a force-closed dialog is dropped rather than written into a detached card. 3. Dashboard creation had two commands with divergent failure behaviour. `application/dashboard-create.ts` is now the single one: it mints, appends against dequeue-time truth (falling back to a caller-supplied baseline for a workspace with no persisted aggregate), and `dashboardCreateMessage` normalizes the report. Both entry points call it; each keeps its own reveal policy, which is genuinely different. The placeholder previously said NOTHING on a rejected commit. 4. The modal had no dialog semantics. `openDialogShell` gives the card `role="dialog"`, `aria-modal="true"` and `aria-labelledby` pointing at a per-dialog title id. The two-field metadata dialog moves from `ui/dashboard-tree.ts` to `ui/dialog-shell.ts` as `openMetadataDialog` — Altinity#494's panel pencil is the second consumer hard rule 5 asks for before extracting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
…he CHANGELOG entry Real-browser coverage for what happy-dom cannot see: Enter and Space on a hover-revealed pencil (the Altinity#495 review defect, reached by tabbing rather than by `.click()`), the dialog's `role="dialog"` accessible name, the panel pencil's committed rename, the panel trash's confirmation and cascade, and — sabotage-checked against a label pinned to `flex: 0 0 auto` — that a long title still ellipsizes with both controls revealed instead of pushing them out of the pane. The action buttons deliberately keep the user agent's own focus ring: the chevron owns the `2px solid` outline channel, and Altinity#472 requires the row's targets to stay visibly distinguishable — the existing Tab-walk test catches a collision, which an earlier custom ring here caused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
…nd confirmations Two independent review passes over the branch found one regression and one contract that no test could fail on. Both are fixed here. **Focus was lost to `<body>` after a successful metadata commit.** Altinity#495's review-2 fix made the dialog close only once the write ANSWERS — but that write repaints the tree first, so the trigger button the dialog captured at open time is detached by the time focus is handed back, and `focus()` on a detached node is a silent no-op. `renderDashboardTree`'s own restore deliberately declines to help, because focus was inside the body-mounted dialog rather than in the list. `returnFocusTo` now also accepts a resolver called at CLOSE time: the trigger while it is still on screen (Cancel/Escape, where nothing repainted), else the row, re-resolved by key. Sabotage-checked. **Deleting a Dashboard stranded focus the same way** — only the panel delete placed it. Both now do, and when the last Dashboard goes there is no row left to stand on, so focus lands on the tree's own search box. **A destructive confirmation opened with the destructive item focused.** `openMenu` autofocuses its first row, which for a confirmation means an Enter pressed out of momentum deletes a Dashboard. It takes an `initialFocus` hint now, and confirmations pass `'last'` — Cancel. **An UNAVAILABLE delete stopped looking like one**: `destructive` was derived from `act.confirm`, which the model nulls for an action it will never ask about, so a malformed-ownership trash lost its destructive styling and announced `aria-haspopup="dialog"`. It comes from the action's KIND now. **Cancel stays operable while a write is in flight.** Only the confirm is barred (that is what prevents a double submit); Escape and the backdrop were never gated, so a visibly dead Cancel just read as a wedged dialog. Tests that could not fail, now able to: the same-row `cancelFor` (all three existing tests only exercised the other-row half, so deleting the call left the suite green), the pencil/trash glyph mapping (swapping the icons passed everything), two `not.toBe('')` assertions that also pass on a missing attribute, and the e2e post-delete focus check, which asserted row text rather than `document.activeElement`. A keyboard-driven delete now proves the whole path in a real browser, including that the landing row is `:focus-visible`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Part of #429 — phase 3, absorbing #481: Dashboard-document metadata gets
exactly one create path and its first rename affordance.
One Dashboard-creation path (#481)
File ▸ New dashboard… and the empty-workspace placeholder's Create button
disagreed: the File menu prompted for a name and committed through
appendDashboard; the placeholder appliedcreateEmptyDashboard's defaulttitle silently and wrote through
withCompatibilityDashboard/dashboards[0].Both now prompt through the same
openNameDialog(seeded with the sameDEFAULT_DASHBOARD_TITLE) and commit throughappendDashboard— no callerreaches the compatibility slot to create any more. The placeholder's dead
"collection already non-empty" guard is deleted rather than ported: once the
write is unconditionally additive, checking it first serves no purpose.
Dashboard-level metadata pencil
Each Dashboard row's new pencil (revealed on hover/
:focus-within, beside theexisting
⋯) opens a two-field dialog — title and description — prefilledfrom the current committed document. Cancel/Escape commit nothing; a blank
title disables Save; a real commit bumps only that Dashboard's revision and
refreshes the tree/any open Dashboard surface. There is still no Dashboard
trash action (non-goal, unchanged).
Implementation notes
openDialogShell/openNameDialogare promoted out offile-menu.tsinto anew
src/ui/dialog-shell.ts, so the Dashboard tree's pencil can reuse themwith no behavior change for File-menu's own dialogs. The hardcoded
app.dom.fileBtn!.focus()restore becomes an explicitreturnFocusToparam (File-menu's own wrapper still supplies
fileBtn); thedialogClosersWeakMap +
app.dom.fileDialogbookkeeping (now-dead field removed fromAppDom) becomes a single module-local "current open dialog" slot with itsown staleness guard, so a dialog opened from the tree — which, unlike
File-menu's own overlays, persists across Query/Dashboard surface switches —
is force-closed on a surface transition too.
renameDashboard(pure planner,workspace-dashboards.ts, modeled onwithVariableConfig) trims the title and refuses (commits nothing) on ablank-after-trim result — defense in depth alongside the dialog's own
disabled-confirm, since the schema itself carries no
minLengthfloor on aDashboard's
title.description: undefined(omitted) leaves the storedvalue untouched; an explicit string trims and omits the field once empty,
matching
renameSaved's convention.commitDashboardRename(
src/application/dashboard-title.ts) wraps it exactly likecommitVariableConfigwrapswithVariableConfig.display: noneexcept on hover/:focus-within, like the⋯it sits beside.By the time its own dialog closes, the pointer has typically moved onto the
dialog's controls, so neither still holds on the row — and
focus()on analready-hidden element is a silent no-op in a real browser (happy-dom
enforces no CSS layout at all, so the equivalent unit test passed regardless).
Fixed the same way
buildMenuButtonalready does it: the trigger's ownaria-expandedstays"true"for the dialog's whole lifetime, reveals itthrough the
openDialogShell'sreturnFocusTo.focus()call, and only resetsto
"false"afterward — by which point:focus-withincovers itindependently.
dialog-shell.tsgained anonClosehook for this.⋯(Library-row icon-order convention), gated bya new
DashboardTreeRow.renamablefield (true for the Dashboard row only) —keeping the row at two trailing controls, not three, so phase 5's "third icon
on a narrow row" concern isn't triggered here.
Acceptance criteria claimed
empty-Dashboard placeholder prompt for its name and append (Reconcile the two Dashboard-creation paths behind one action #481).
Deferred by design: panel-row edit/delete (phases 4/5), the repair planner
decision (phase 6), and every Dashboard-trash/deletion path (non-goal,
unchanged).
Verification
npm test— 179 files / 5877 tests passed, per-file coverage gate held;tsc --noEmitclean;npm run buildclean.npm run test:e2e(dashboard-tree.spec.js) — full spec green onchromium, webkit and firefox; the real-browser focus-restore bug above
was caught here first and is now pinned by a dedicated e2e test plus a unit
test on the
aria-expandedtoggle.test:e2esuite green on chromium (152/152); webkit clean; a handfulof unrelated Firefox specs (dashboard-kpi-move, dashboard-membership,
tile-open-workbench, variable-tab — none touched by this change) flaked
under full parallel load and passed individually on retry, matching known
sandbox Firefox flakiness.
staleness guard,
renameDashboard/commitDashboardRenamecorrectness,boundary rules, coverage-gate honesty, scope) came back with no defects.
ch-26.6): created a Dashboard via File ▸ New dashboard…, renamed it via the
pencil (title + description), confirmed the tree label updates, the toast
fires, and no console errors — including moving the mouse away right after
Save, to exercise the focus-restore fix above.
Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated if behavior or the deployed surface changed🤖 Generated with Claude Code
https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU