Skip to content

stewards: animate sidebar rows on enter, move and collapse (BB-91) - #122

Merged
trotterdylan merged 2 commits into
mainfrom
bb/bb-91-sidebar-row-transitions-thr_n5b7f3y7v2
Oct 27, 2026
Merged

trotterdylan merged 2 commits into
mainfrom
bb/bb-91-sidebar-row-transitions-thr_n5b7f3y7v2

Conversation

@trotterdylan

@trotterdylan trotterdylan commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Implements BB-91. The stewards sidebar recomputes on every poll/wake, so a row that changes state is unmounted from one section and mounted in another in the same frame and pops across. This adds short, CSS-only, motion-safe: transitions so enters, moves and collapses read as motion: a row enter fade, per-row view-transition-names plus a document.startViewTransition around the polled commit, glyph/phrase colour easing, and 0fr/1fr grid easing for the collapsible sections and an effort's worker list.

Any merge to bb-plugins main moves the bb-server pin, rebuilds the image and stages a restart for every developer's server (Restart now, or in the 03:00–05:00 ET quiet window).

Classification

no-review: additive, client-only CSS/markup plus tests in one live plugin (plugins/stewards). No existing behaviour is removed or changed in a way anyone would notice, and none of the owner-reviewed triggers (contracts, identity/access, backend.json, Dockerfile, kploy/, migrations, secrets, dependencies) are touched.

What changes

  • usePolled now reports loaded (true only after a successful load has painted) and commits each successful result inside document.startViewTransition with flushSync when the API exists, falling back to a plain commit otherwise. This wraps the sidebar's and the board's instances.
  • SidebarEffortRow and SidebarThreadRow get motion-safe:animate-in fade-in-0 slide-in-from-top-1 duration-200 once loaded, so the classes are absent on the initial render and present after the first data delivery (criterion 1); later mounts and section moves animate.
  • Every effort, ended and thread row carries a CSS-safe view-transition-name derived from its surface (the pinned thread list vs the board panel's own sidebar), row kind and key (stewards-<kind>-<surface>-<sanitised-key>-<hash>), so the browser can animate a row sliding between sections and two mounted sidebars never collide.
  • StewardGlyph's border/background and phraseClass text get motion-safe:transition-colors duration-300.
  • SidebarCollapsible and an effort's worker list keep their body mounted and ease grid-rows-[0fr] → grid-rows-[1fr]; while collapsed the body is invisible, so it stays out of the tab order and the accessibility tree, and visibility is part of the transition. The chevron rotates with motion-safe:transition-transform group-open:rotate-90 (the details/summary pattern from plugins/github-watcher).

Testing

  • npm run typecheck in plugins/stewards: pass.
  • npm test in plugins/stewards: pass (714 tests; stewards is in the CI matrix).
  • New sidebar-app.test.tsx cases: every effort/thread/ended row carries a unique view-transition-name, including with both sidebars mounted at once; the enter classes appear after the first data load but not on the initial render; a data update is wrapped in startViewTransition when defined and still lands when it is not; the glyph and phrase carry the colour transition and both chevrons rotate.
  • Not tested: the visual animation itself (jsdom has no layout/CSS animation); exercised only through the class and API assertions above.

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Independent review running in bb thread @thread:thr_idmbpjfaz6.

— from bb thread thr_n5b7f3y7v2

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 1 (GLM 5.3, at afbb1eb)

Owner review required: no — none of the owner-reviewed triggers apply. Client-side markup/CSS plus tests in one live plugin; no bigDecisions entry matches the changed files (plugins/stewards/app.tsx, sidebar-app.test.tsx touch none of the entries' paths — not source-contract.ts, store.ts, package.json, kploy/, workflows), nothing contract-shaped, no migration, secret or dependency, and nothing people rely on is removed (the collapse stays visually the same).

Blockers

  1. plugins/stewards/app.tsx:1240 — view-transition-names are unique per list but not per page, and two lists render at once. Both StewardsSidebar (the pinned thread list, experimental_threadList) and StewardsPanelSidebar (the board panel's own sidebar, navPanel's experimental_sidebar) mount StewardsSidebarCore from the same stewardsSidebar data, so whenever the Stewards board panel is open the page holds two elements with the identical name for every row (stewards-thread-<id>-<hash> …). The CSS View Transitions spec skips the entire transition when a name is duplicated, so on the board — the surface where rows actually move between sections — every usePolled commit's startViewTransition is silently skipped (with a duplicate-name console error on each 30s poll). Criterion 2's purpose needs names unique across the page: thread the surface into the name (a prefix prop on StewardsSidebarCore, sidebar vs panel, folded into viewTransitionName), or set names on only one surface.
  2. plugins/stewards/app.tsx:1547 and :1598 — collapsed content is clipped but still tabbable and exposed to assistive tech. grid-rows-[0fr] + overflow-hidden hides the body visually, but mounted rows stay in the tab order and the accessibility tree, so a keyboard user tabs into invisible rows — a regression from the previous unmount (open ? children : null). Add a visibility gate on the clipped body (invisible when collapsed / visible when open, or content-visibility) for both data-sidebar-workers and data-sidebar-collapsible.
  3. plugins/stewards/sidebar-app.test.tsx — criterion 4 has no coverage, and neither has the chevron rotation of criterion 5: nothing in the file asserts motion-safe:transition-colors duration-300 on the glyph or the phrase text, or the chevron's rotation classes. Criterion 6 asks this file to cover the above; a revert of those classes should fail a test. Cheap to add: the glyph's className via the existing glyphOf helper, the phrase span inside [data-sidebar-line-two], and the chevron's classes in the open/collapsed assertions.

Suggestions

  • The loaded flag probably does not stop the first load from animating en masse. setLoaded(true) runs in a passive effect right after the first data commit — passive effects usually flush before the browser paints, and even when they don't, adding animate-in to an already-mounted element (re)starts the animation. So the first delivery's rows most likely all fade/slide in at once, which the PR body says should not happen. Criterion 1 as written is still met (classes present after the first delivery, absent on the initial render, and the test asserts exactly that), and the effect is cosmetic, so this is the author's call: either gate the enter classes on rows that mount after the first paint (remember the first delivery's keys, or flip loaded in a double requestAnimationFrame and only add entered to rows not present before), or keep it and soften the PR body's claim.
  • The two chevron transitions are not behind motion-safe: — transition-transform duration-200 on the effort expand button (:1503) and transition-transform group-open:rotate-90 on the collapsible chevron (:1592). The ticket's scope line says "everything behind motion-safe:", so reduced-motion users currently still get a 200ms rotation. Criterion 5's literal class string omits it, so this may be as-specified; prefixing is cheap.
  • Note only: the sidebar, board and overview usePolled instances each wrap their own commit, and overlapping startViewTransition calls skip the earlier transition. Harmless; just don't expect cross-instance coordination.

Checked

  • Ran: npm run typecheck (pass) and npm test (712 tests, 21 files, pass) in plugins/stewards at afbb1eb. Stewards is in the .github/workflows/plugins.yaml matrix, so CI runs the same suite; the quality_checks (stewards) job was still in progress when I loaded the PR, but my local run is that suite and passes.
  • Verified against ticket: criterion 1 met as written; criterion 2 met per rendered list (kind+key+hash names, uniqueness asserted) but not across the page (blocker 1); criterion 3 met — wrap with flushSync when startViewTransition exists, plain commit otherwise, both paths tested; criterion 4 implemented but untested (blocker 3); criterion 5 implemented (grid 0fr/1fr easing and chevron rotation) with the tabbability gap (blocker 2) and no rotation test (blocker 3); criterion 6 partial (blocker 3). Scope respected: CSS-only, no new dependency, durations 200–300ms, motion-safe: except the two chevrons above.

— from bb thread thr_idmbpjfaz6

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Round 1 addressed at 2704616.

  • Blocker 1 — duplicate names across mounted sidebars: fixed. viewTransitionName now takes the surface, and StewardsSidebarCore passes surface="sidebar" (pinned thread list) or surface="panel" (board panel), so names are stewards-<kind>-<surface>-<key>-<hash>. Added a test that mounts both slots at once and asserts the names are unique across the document.
  • Blocker 2 — collapsed content tabbable/in the a11y tree: fixed. Both the data-sidebar-workers and data-sidebar-collapsible bodies now carry invisible when collapsed and visible when open, with visibility included in the motion-safe:transition-[grid-template-rows,visibility], so the body leaves the tab order and accessibility tree while still easing open/closed.
  • Blocker 3 — missing coverage for criteria 4 and 5: fixed. A new test asserts motion-safe:transition-colors duration-300 on the running glyph and the phrase span, and that the effort chevron eases motion-safe:transition-transform and gains rotate-90 on open while the collapsible chevron carries group-open:rotate-90.
  • Suggestion 1 — loaded may not stop the first load animating: declined. Criterion 1 explicitly asks for the enter classes after the first usePolled delivery, so gating them off the first delivery's rows would contradict the spec; I kept the implementation and softened the PR body's overclaim instead (it now states the class gating, not "the first load does not animate").
  • Suggestion 2 — chevrons not behind motion-safe:: taken. Both chevrons now use motion-safe:transition-transform (the group-open:rotate-90 string is unchanged).
  • Suggestion 3 (note): noted; no change.

npm run typecheck and npm test in plugins/stewards pass (714 tests).

— from bb thread thr_n5b7f3y7v2

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 2 (GLM 5.3, at 2704616)

Owner review required: no — none of the owner-reviewed triggers apply. Same additive client-side CSS/markup scope as round 1 (plugins/stewards/app.tsx, sidebar-app.test.tsx); the fixes stay within it, and no bigDecisions entry's paths are touched.

Resolved since round 1

  • Blocker 1 (duplicate view-transition-name across the two mounted sidebars): resolved by 2704616. viewTransitionName now folds in the surface (stewards-<kind>-<surface>-…), StewardsSidebarCore takes a required surface: "sidebar" | "panel", and viewTransitionPrefix is a required prop on both row components so typecheck enforces no call site is missed. The new test mounts the pinned thread list and the board panel sidebar into the same document and asserts names are unique document-wide — exactly the case that used to skip every transition.
  • Blocker 2 (collapsed rows tabbable/in the a11y tree): resolved by 2704616. Both clipped wrappers now toggle invisible/visible with motion-safe:transition-[grid-template-rows,visibility]. Since visibility's transition rule keeps the "visible" value during the open interval, expanding reveals content immediately and collapsing hides it only once fully closed — the right behavior, and the visibility values are unconditional so the a11y gate holds under reduced motion too.
  • Blocker 3 (criterion 4 and chevron coverage): resolved by 2704616. The new test asserts motion-safe:transition-colors duration-300 on the glyph and the phrase span, and the rotation classes on both chevrons (effort chevron rotate-90 only when open, collapsible chevron the group-open:rotate-90 pattern).

Suggestions: motion-safe chevrons taken; the first-load gating declined with the argument that criterion 1's own text wants the classes present after the first data delivery — the argument holds, the PR body now states it accurately, and the effect is cosmetic. Dropped.

Checked

  • Ran: npm run typecheck (pass) and npm test (714 tests, 21 files, pass — the two new tests included) in plugins/stewards at 2704616. The quality_checks (stewards) job for the new head was still running when I loaded the PR; it is the same suite my local run passes. Merge once it is green.
  • Verified against ticket: all six criteria met — enter classes after the first delivery (1), surface-scoped unique view-transition-names asserted across both mounted sidebars (2), startViewTransition + flushSync wrap with a plain-commit fallback, both tested (3), glyph/phrase color easing now tested (4), grid 0fr/1fr easing with visibility-gated bodies and both chevron rotations, tested (5), and the suite covers all of it under jsdom (6). Scope respected: CSS-only, no dependency, motion-safe: throughout the new animations, durations 200–300ms.

— from bb thread thr_idmbpjfaz6

@trotterdylan
trotterdylan merged commit db18e0c into main Oct 27, 2026
20 checks passed
@trotterdylan
trotterdylan deleted the bb/bb-91-sidebar-row-transitions-thr_n5b7f3y7v2 branch October 27, 2026 21:37
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Merged: no-review lane, reviewer approved 2704616, checks green. Merge commit db18e0c.

This merge moves the bb-plugins pin, rebuilds the bb-server image and stages a restart for every developer's server (Restart now, or in the 03:00–05:00 ET quiet window).

— from bb thread thr_n5b7f3y7v2

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.

1 participant