feat(web): clarify the Project shell and card hierarchy#27
Merged
Conversation
johannesjo
force-pushed
the
task/beautify-03-member-navigation
branch
from
July 21, 2026 11:16
2c8135a to
46b3a5f
Compare
Add meaningful loading, error, retry, and terms states; preserve modal focus ownership; expose card headings and reading order; and reduce unused Scratchpad height. Exercise the real reactive store with typed API fixtures.
Replace the float-based content layout with a grid: an explicit flexible second row keeps Recent activity pinned under the task list when the Scratchpad column outgrows it, and `align-content: start` stops grid from padding the stacked cards apart when the shell is taller than its content. Both behaviours are browser-measured; the columns now resolve to the exact 1.35fr / 1fr split rather than the floats' percentage approximation. Restore a usable Scratchpad editor on engines without `field-sizing` (Baseline only since June 2026) -- with `resize: none` and a 64px floor those users had a three-line scroller and no way out. Drop the boolean `handleDeleteItem` returned: both consumers type the callback as `void` and discard it, so nothing could observe it. The two tests that asserted it now assert the API call and the toasts instead. Give ListCard the section heading and `aria-controls` the other cards gained, take the terms gate back off `<main>` (Dialog renders through a Portal, so that landmark was always empty), and drop a `tabindex` and an `aria-live` that were dead.
Project's retry flow focuses `[data-testid="project-name"]` after a successful reload, but Project.test.tsx mocks Header with a hand-written h1 -- so dropping the real tabindex would break the flow with every test still green. Assert the contract where Header itself is rendered.
ListCard gained the section heading and aria-controls the other cards already had, but it has no unit test file -- so cover it where the real card is rendered, alongside the existing collapse assertions.
PR #26 removed this tabindex on the grounds that nothing focused the Space title. That was true at the time; this PR makes it false -- the project load's retry effect focuses `[data-testid="project-name"]` once a reload succeeds. focus() on a plain h1 is a silent no-op, and Project.test mocks Header with its own focusable h1, so without this the retry flow would break with the whole suite still green. The Header contract test added in this PR catches exactly that, and comment names the reason so the attribute does not get tidied away again.
johannesjo
force-pushed
the
task/beautify-04-project-shell
branch
from
July 21, 2026 12:52
cf37a85 to
f09aec4
Compare
Multi-agent review turned up three defects and one duplication.
The retry flow released focus too early: setProjectData clears `loading`
as soon as the project lands, but the load is still fetching activity, so
a rejection there swapped in the error screen after focus had already
committed to the Space title -- leaving it on an unmounted heading. Gate
the final move on a new `loadSettled` signal that only the current load
may set, and cover it with a test that fails without the gate.
Batch handleRetry's three writes. Unbatched, the focus effect first ran
against a torn state and only survived because queueMicrotask deferred it
until the stale node had gone; with a coherent state the microtask hop is
unnecessary, so the flag lifecycle no longer depends on a selector miss.
The Scratchpad's `field-sizing` fallback raised only the editor, not the
display button it swaps with, so engines without `field-sizing` jumped
~156px on click. Both now read one `--pad-min-h`. Drop `rows`, which the
spec ignores under `field-sizing` and which the fallback floor outranks
otherwise, and stop the test asserting it.
PanelCard's h2 wrapper is `display: flex` again: the rename input is
`flex: 1`, which a block parent silently ignored, dropping it from 403px
to its intrinsic 295px and overflowing narrow cards. Both card headings
also get ListCard's `font: inherit`, without which global.css's
`button { font: inherit }` pulls the h2's bold into a collapsed card's
"N" count.
Finally, use the shared useDocumentTitle helper instead of a bespoke
effect -- every other route already does.
The previous fix inferred "the load has finished" from a third signal (`loadSettled`) and a 25-line effect that re-derived which screen was up from `loading`/`error`/`project`/`termsRequired` -- a second source of truth for the routing the <Show> tree already owns. The load itself knows when it settles, so call focus from there: one `retryFocusPending` boolean (no longer a signal -- nothing renders from it), a two-line focus helper, and a `settleRetryFocus()` invoked from the load's existing guarded `finally`. Behaviour is unchanged and the same regression test still fails when focus settles before the activity fetch. Dropping the effect also removes the need for `batch()` and the explicit `setLoading(true)`: with no effect observing the intermediate state, the load effect's own `resetState()` renders the loading screen before `focus()` looks for it. Net -19 lines, one fewer signal, one fewer effect.
The no-identity branch returns before any `await`, so the load IIFE's `finally` runs synchronously while the effect body is still on the stack with a tracking Listener. `settleRetryFocus()` reads `state.error` and `termsRequired()`, which therefore subscribed the *load* effect to them: with a retry pending, the next `setError` re-ran the whole teardown -- abort, disconnectSSE, resetState and a second navigate. Wrap the call in `untrack`. Covered by a test that fails without it. Also correct both heading comments, which named the wrong mechanism. Measured on the built app: the reset matters because a *block* h2 gives its line box a 1.5em/bold strut -- ScratchpadCard's header is 27px with the reset and 36px without. PanelCard is immune to that anyway now that its heading is a flex container, so there the reset only keeps the UA font off the toggle. Neither card passes `count` today, so the bold "N" the old comments cited was latent, not the actual reason.
Three cards were pairing `createCollapsed(id)` with a separate `createUniqueId()` and then threading the result into both `CollapseToggle`'s `controls` and `CollapseBody`'s `id` -- the kind of two-step a card can silently half-wire, which is why the e2e assertion guards it. `createCollapsed` now returns `bodyId` alongside `collapsed`/`toggle`, so the toggle and the body it folds can only come from the same source. DisclosureSection keeps its own `createUniqueId`: it is the per-visit show/hide primitive and does not use `createCollapsed` at all. Also drop `.emptyState` from Project.module.css (no TSX has referenced it for some time) and record that the shared scratchpad floor depends on the global border-box reset for the display/editor heights to match exactly.
johannesjo
marked this pull request as ready for review
July 21, 2026 15:07
johannesjo
added a commit
that referenced
this pull request
Jul 21, 2026
ListCard's onDeleteItem became Promise<boolean> so a failed delete can keep focus on the row. ChecklistCard's twin handler already returned it; the hero list's copy in Project.tsx still returned void, which only surfaced once the branch rebased onto main and 'npm run check' actually ran. Also drops a duplicate .titleGroup comment: main's merged #27 added the same heading reset, so this branch no longer needs to touch that file.
johannesjo
added a commit
that referenced
this pull request
Jul 21, 2026
* feat(web): improve task controls and keyboard reordering * fix(web): preserve task focus until deletion succeeds (#24) * fix(web): keep reorder actions mounted while a move is saving Gating onMoveUp/onMoveDown on reorderPending() dropped .reorderAvailable from the ⋯ trigger mid-flight, which desktop CSS display:none's — pulling the element out from under the focus Popover had just returned to it, so a keyboard reorder ended with focus on <body>. Offer the actions by position only and let commitReorder's guard refuse the second commit. A refused commit now toasts instead of no-oping: a drag dropped during a pending reorder had its DOM already reverted, so the row snapped back with no explanation. The toast is role="status", so it covers the keyboard path too. Also hover/focus-reveal the ⋯ on desktop like the other row actions (it was permanently visible), and neutralize the new <h2>'s UA type so the collapsed "· N" count stops inheriting bold. * refactor(web): name the task checkbox after the task role="checkbox" + aria-checked already announce "checkbox, not checked", so "Mark X complete" duplicated the state and flipped the accessible name as the row toggled. Name it after the task instead, per the APG checkbox pattern. Also flatten captureFocusFromRow's restore closure: a four-level nested ternary and a duplicated row lookup become two named helpers (checkboxIn / sourceControlIn) and a rowById lookup. No behaviour change. * chore: patch high-severity dev advisories and unblock CI checks npm audit --audit-level=high has been failing on this branch, and it ran BEFORE npm run check — so typecheck, lint and format:check never executed. Any lint or formatting regression could land unnoticed. Bump the two high-severity dev-only transitives (js-yaml 4.2.0 -> 4.3.0 via eslint, shell-quote 1.8.4 -> 1.9.0 via concurrently 9.2.4); lockfile-only, no majors. Then move the audit after npm run check with if:always(), so a newly published advisory can no longer mask the code checks on an unrelated run. The remaining 4 moderate esbuild advisories (via drizzle-kit) sit below the high threshold and would need a drizzle-kit major. * test(e2e): cover keyboard reorder focus retention The keyboard reorder path had no e2e coverage, and the regression it guards is invisible to jsdom: the focus loss came from a CSS rule (.moreButton:not(.reorderAvailable) { display: none }) unrendering the focused ⋯ trigger, and unit tests apply no CSS. The PATCH is delayed via page.route so the in-flight window survives a style recalc. NOT YET RUN — the sandbox cannot reach docker-published host ports, so no local database was available. Typecheck and format pass; CI's e2e job is the first real execution. * fix(web): return the hero list delete result to the row ListCard's onDeleteItem became Promise<boolean> so a failed delete can keep focus on the row. ChecklistCard's twin handler already returned it; the hero list's copy in Project.tsx still returned void, which only surfaced once the branch rebased onto main and 'npm run check' actually ran. Also drops a duplicate .titleGroup comment: main's merged #27 added the same heading reset, so this branch no longer needs to touch that file. * fix(web): keep focus on the row through a keyboard reorder The previous fix kept the reorder actions mounted so the desktop rule '.moreButton:not(.reorderAvailable) { display: none }' could not unrender the menu trigger under focus. That was necessary but not sufficient, and the invariant it relied on was not actually holdable: - The optimistic store write reorders the row synchronously, and <For> re-inserts it. Re-parenting a node blurs its focused descendant, so focus hit <body> on every keyboard move, long before the PATCH resolved. - .reorderAvailable is derived from props.items, so a collaborator's SSE echo could drop it mid-interaction and display:none the trigger anyway. Drop the display:none rule instead of defending it: .moreButton already sits in the opacity reveal group, whose comment states the rule this violated -- buttons stay in layout, hit-testable, and focusable. That removes canReorder(), the .reorderAvailable class, and the whole failure mode. Focus is then handed back to the trigger after the synchronous write. Keep the trigger visible while its own menu is open, since the portal puts focus outside :focus-within. The existing tests mocked moveItem to a no-op, so no row ever moved and focus was never at risk; the new test reorders for real and fails without the fix. Rework the e2e test onto a released gate rather than a 600ms timer, drive it from the keyboard, and assert focus after the response lands. * test(web): make the store mocks reorder for real Every reorder test mocked moveItem/setItemPosition as no-ops, so no row ever moved. That is how a focus bug that broke the feature's central promise on every keyboard move survived the whole suite: with a static list, focus was never at risk, and the assertions could only ever check that the API was called with the right arguments. Have renderCard drive the card from a signal and give the mocks a real reposition-and-resort, mutating items in place so identity survives the way produce() does in store.ts. <For> then re-inserts rows exactly as it does in the app, and any future reorder test exercises the real DOM movement by default rather than opting in. Three tests needed adjusting because they had been asserting against a list that never changed: - the refusal test picked Move up on a row that is first once the optimistic move lands; it now moves the row that still has somewhere to go - two announcement assertions raced the extra render and now wait for it - the focus test waits for its commit to settle, because reorderPending is module scope and an in-flight move leaks into the next test * fix(web): announce a reorder result once, and document the ⋯ trigger Toast is role="status", so raising a toast AND writing the live region read every reorder failure to a screen reader twice. Let the toast own failures and the live region own success. That also collapses commitReorder's boolean | null tri-state: refused and failed are now both plain false, since the only caller treats them identically -- both have already been toasted. Refresh /_styleguide, which still described the ⋯ menu as touch-only. It is on pointer devices too now, because keyboard reorder has no inline equivalent. The demo rows passed no move handlers, so the menu could not show Move up / Move down at all; give them positional availability the way ListCard computes it, folding four near-identical blocks into a For. Use if: ${{ !cancelled() }} for the audit step -- it expresses the actual intent (never let the checks above short-circuit the audit) without also burning a step on cancelled runs. * fix(web): restore focus after a failed reorder rolls the row back The focus restore ran once, after the optimistic write. A failed move rolls the position back -- a second synchronous store write, so <For> re-inserts the row a second time and blurs the trigger again, with nothing to catch it. Every focus assertion sat on the success path, so the suite missed it. Restore on both edges, and take the trigger from the argument Menu already hands to onSelect instead of sniffing document.activeElement: the node is then known rather than inferred from Popover's focus-return ordering. The <body> guard stays, so a user who deliberately focused elsewhere during the request keeps it. Also from review: - the styleguide demo gave move actions to its two checked rows, which ListCard excludes from reordering, while claiming to mirror it - reorderIndex's comment justified the position-only rule with a scenario that cannot happen (Menu closes before onSelect runs); state the real reason - drop an assertion that could not fail, rename a describe that still called the menu mobile-only, and assert the e2e menu focus instead of setting it * fix(web): close the remaining focus and live-region gaps From an accessibility/correctness/simplicity review round. Two of these were reproduced with probes before being fixed, and both are the same shape as the earlier bugs: a path handled on one branch and not its sibling. - justArrived was only dropped when the outro was still running, so a toggle whose PATCH fails after the outro finishes orphans the marker and the row replays its entrance animation on the next remount. Drop it unconditionally. - moveByKeyboard held a raw trigger node, so a concurrent remote edit that remounts the row mid-save left focus stranded on <body>. Re-query by item id, the same way ListItem#captureFocusFromRow already does. - Tab out of an open menu walked past the portal into nothing: focus on <body>, menu still open, its backdrop swallowing every click. Close on Tab and keep the items out of the tab sequence (APG). Menu had no tests; it has three now. - Toasts each mounted their own role="status" together with its text, which is the unreliably-announced pattern — and they are the only channel for reorder failures. Move the live region to the always-mounted host. Also: two comments still called the ⋯ trigger mobile-only, one described the fade-out as keyboard-specific when the scrim makes it happen for pointers too, the opacity-not-display rationale was stated in four places (the CSS is where re-adding display:none would be tempting, so it keeps it), and sourceControlIn's last fallback was unreachable — every focusable control in a row carries a data-testid. * chore(deps): patch fast-uri and hono advisories npm audit --audit-level=high went red on a fresh advisory, with no code change on our side: fast-uri <3.1.2 host confusion via failed IDN canonicalization (GHSA-4c8g-83qw-93j6). Both bumps are patch-level and within the declared ranges, so this is what a fresh install would resolve to anyway; hono 4.12.31 also clears three moderate advisories and supersedes dependabot #6. Only the dev-only esbuild/drizzle-kit chain remains, at moderate, with no non-breaking fix — below the gate's threshold. This is exactly the case the audit step's comment describes, and the reordering earned itself: npm run check ran and reported green on the same CI run instead of being short-circuited by the audit. * fix(web): keep the PWA update toast announced Moving role="status" off Toast and onto the Project toast host silenced this one, which renders a Toast outside that host. Its own host was inside the <Show>, so putting the role there would recreate the problem the move was meant to fix — a live region inserted in the same tick as its content is unreliably announced. Hoist the host out instead, so the region is always mounted and only its content changes. * fix: make task reordering atomic and race-safe * fix(web): handle interactions on stable task rows Prevent Enter's default action from reopening the reminder picker after focus returns to its trigger. Update the assignee regression to assert the identity-preserving row behavior directly.
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
1 participant
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.
Summary
Why
The Project shell's visual order, DOM order, and transient states were not consistently aligned, and card headings lacked a clear semantic hierarchy.
Impact
Project states are easier to understand and recover from, while cards scan and collapse more predictably across screen sizes.
Validation
Stack 4 of 6. Base:
task/beautify-03-member-navigation. Review and merge this stack from the bottom upward.