UI Review Follow-up — remaining P1 & all P2 fixes (#281)#668
Merged
Conversation
Closes the two still-open P1 items and all four P2 items from the 2026-04-26 UI review (re-walked 2026-06-06). US1 (P1.3) Header links in high-contrast light theme route through the theme-aware --debrief-color-primary token (~9.2:1 on white, clears AAA 7:1) with an underline + heavier weight affordance in high-contrast modes; verified by an axe-core contrast audit spec. US2 (P1.4) properties-screenshots de-flaked: the interaction-video test now gates the row-click on actionability (visible + scrollIntoView) before clicking, and the suite runs at retries:0 so future flake fails loudly. The 15s properties-form wait is preserved so real breakage still fails. US3 (P2.1) Analysis default layout scales with container width via discrete bands (~280px rail <=1366, ~320px middle, ~380px >=1600), map always keeps the majority; LAYOUT_VERSION bumped so legacy fixed-25% layouts fall back to the responsive default; all three call sites use getDefaultLayout(containerWidth). US4 (P2.2) ActivityPanel collapses upper sections as initial internal state when uncontrolled + clientHeight < 820 + a feature is selected, so Properties is reachable on 1280x720 laptops; never persists, no-op when controlled or >=900px. US5 (P2.3) Catalog timeline+map collapse made discoverable (labelled chevron + tooltip + aria-label, equally visible restore), first-run default shows the row once a dataset context exists; replaced the `any[]` + eslint-disable in buildLayoutForVisiblePanels with the typed golden-layout child union. US6 (P2.4) Thumbnail S/M/L toggle now resizes the list: virtualizer re-measures only in useEffect([rowHeight]); size persisted via a typed, versioned helper that narrows to the ThumbnailSize union on read. Tests: unit suites for the layout builder, persistence, short-height invariant, thumbnail preference, and virtualizer re-measure; new web-shell E2E specs (contrast, layout, catalog) author the evidence captures. localStorage hygiene added to the shared test setup.
P1.3: the web-shell header sits on a fixed dark title bar (#3c3c3c) in every theme, so the dark content-area --debrief-color-primary (#0F4A85 in HC-light) gave 1.22:1, not the ~9:1 a light background would. The axe audit (SC-001) caught it. Header links now use the host text-link colour on the dark bar, with a dedicated bright --debrief-header-link-hc token (#9CDCFE ≈ 8.6:1 on #3c3c3c) + underline/weight in high-contrast modes. This supersedes plan Decision #4's light-background assumption. P2.3: collapsed bottom-row state now persists across reload — collapse/ restore save the layout immediately (not just via the debounced autosave) and hiddenPanels is reconciled from the restored layout on mount so the "Show Timeline/Map" restore controls survive a reload (FR-016). Tests: FR-011 layout spec now mutates a real app-saved (resolved) layout rather than a hand-built one that LayoutConfig.fromResolved rejected; the catalog collapse spec collapses the whole preview row (both siblings) since collapsing one lets the other fill the row; ESM-safe __dirname in the new specs. Typed the GL content arrays via RowOrColumnItemConfig. ChildItemConfig (golden-layout has no LayoutItemConfig export).
- evidence/test-summary.md (YAML front matter, 2399 unit + 13 E2E + 40/40 flake proof) - evidence/usage-example.md (per-item before/after walkthrough) - evidence/screenshots/ (header×4 themes, analysis 1920/1366, properties-720, catalog-collapse, thumbnail-sizes) - evidence/flake-proof.txt (SC-002 10× no-retry run) - media/shipped-post.md (feature post; opener verbatim + ship-time evidence) - tasks.md: all tasks marked complete interaction.gif omitted — ffmpeg unavailable in the cloud session (the shared videoToGif helper skips when absent); static stills cover the flows.
Contributor
📋 Spec NavigatorReview this PR's specs interactively:
|
Contributor
🚀 Preview DeploymentsCode Server (Full VS Code Extension)Browser-based VS Code with the Debrief extension and sample data pre-installed. Web Shell (Standalone App)Use this for Playwright testing and demos - runs outside Storybook. Storybook (Component Library)Browse all components, stories, and documentation. Briefing Renderer (Air-gapped Storyboard Player — spec #264)Standalone SPA that plays a Storyboard from a All Links
|
CI (Test & Lint) caught a 51-test web-shell regression: the original adaptation collapsed Tools and Layers at the default 1280x720 viewport whenever a feature was selected. A collapsed section is display:none, so the Layers feature list (and the rows tests/users click to select a feature) became unreachable — Playwright can't scroll to a hidden element. Now the adaptation collapses ONLY the Time Controller (the topmost fixed-height section), never Tools or Layers, so feature selection and tool runs keep working while Properties still moves toward the fold and is reached via the column's natural scroll. The invariant still holds: uncontrolled-only, never persists, no-op when controlled or >=900px. Verified locally: selection-sync 5/5 (feature-row click reachable again), ui-review-layout 6/6 incl. SC-005 (Properties reachable at 1280x720), ActivityPanel.shortHeight unit suite rewritten to assert Time-Controller- only collapse (13/13). The time-controller.spec.ts failures seen locally are an environmental sample-data gap (the cloud catalog has no "Exercise Alpha"); selection starts empty on plot open, so that suite's no-feature- selected checks are a no-op for the adaptation in CI.
IanMayo
pushed a commit
that referenced
this pull request
Jun 9, 2026
Root cause of PR #668's "51 failed" Test & Lint job: 46 of the failures were `locator.click: Test timeout of 30000ms exceeded` on feature rows. At the 1280x720 test viewport the ActivityPanel stacks Time Controller + Tools above the virtualised Layers list, so a target feature row commonly starts BELOW the panel fold. Every feature-row click site clicked `.debrief-feature-row__content` directly with no scrollIntoViewIfNeeded, so the bare click hit "element is not visible" and burned the full 30s timeout in CI (locally it only flaked, recovering on Playwright's retry). Fix: add a shared `clickFeatureRowContent(row)` helper that scrolls the content into view and clicks, retrying the pair as one unit via toPass so a mid-reflow miss re-scrolls instead of timing out. Route all 11 click sites (7 specs + AnalysisPage.selectFeature) through it. Verified locally with the dev catalog: properties-mode-swap/multi-select/ revert now 15/15 with ZERO flaky (was 6 flaky); tool-execution, log-panel, selection-sync pass; the feature gets selected where the bare click previously timed out. Residual local styling-tools failures are a dev- catalog data gap (no "HMS Defender" track — present only in CI's Exercise Alpha fixture); the click itself succeeds, which was the failing step.
IanMayo
pushed a commit
that referenced
this pull request
Jun 9, 2026
…pace Real cause of PR #668's "51 failed" (75 "element is not visible"): at the 1280x720 CI viewport the ActivityPanel has overflow:hidden and the fixed-height Properties form (tall for Exercise Alpha features) plus the Time Controller section added by spec #281 consume the full height, so the flex Layers list shrinks to 0 — react-virtual renders 0 feature rows and every feature-row click fails "element is not visible" (no amount of scrollIntoView helps a zero-height list). Only reproduces in CI because the dev catalog's features have short property forms. Per the existing collapsePropertiesSection fixture's documented decision (Properties is layout="fixed" "in production by design"; the squeeze is fixed test-side), extend the same mitigation to the Time Controller (and Tools where a spec doesn't exercise it): - properties-collapse.ts: add collapseTimeControllerSection + collapseToolsSection (shared collapseSectionByTitle helper). - Specs needing Tools (tool-execution, styling-tools, run-dropdown, log-panel, capture-log-evidence): also collapse Time Controller. - Layers-only spec (selection-sync) + properties-multi-select: collapse Time Controller + Tools. - time-controller Layer Selection: collapse Tools (keep Time Controller — its playback tests need it). - properties-revert/mode-swap/feature-edit/evidence-captures (keep Properties expanded — section under test): collapse Time Controller + Tools. selection-sync passes 5/5 clean in isolation with the new collapses; typecheck + lint clean. Production layout is unchanged.
Root cause of PR #668's web-shell E2E failures (feature rows "element is not visible" at 1280x720, incl. spec 281's own SC-005 acceptance test): FeatureList defaults to a fixed height=300px and ActivityPanel never overrode it. The GoldenLayout-constrained sidebar is only ~487px tall, so once spec 281 added the Time Controller section the Layers area dropped to ~89px. The 300px list then overflowed its section and the virtualised rows were positioned below the panel's clipped bottom edge — present in the DOM (so `.first()` resolved) but unclickable, timing out every Layers-panel selection. Measured at 1280x720: even with Properties + Time Controller collapsed (Layers area 184px) the first row still rendered at y≈554, past the panel edge — so collapsing sections (the earlier test-side attempt) could not fix it. The list must size to its flex parent instead. Fix (production, shared component): - FeatureList: `height` accepts `number | string`; container uses the value verbatim when a string, and adds `minHeight: 0` so a flex child can shrink and scroll internally rather than forcing the column to content height. Default stays 300 (number) — standalone/Storybook usage unchanged. - ActivityPanel: pass `height="100%"` to the Layers FeatureList so it fills the section after the toolbar; the internal scroll area handles overflow. Verified: SC-005 (ui-review-layout) and styling-tools/tool-execution feature-row flows pass at 1280x720; 132 component unit tests (incl. ActivityPanel.shortHeight + FeatureList) green; pyright/tsc clean. Also drops two superseded test-side band-aid commits from this branch (scroll-into-view + section-collapse) — they addressed symptoms, not the fixed-height cause, and the collapse churn destabilised other flows.
IanMayo
force-pushed
the
claude/nifty-keller-4ex7d0
branch
from
3b3beff to
783ca40
Compare
…election Completes the short-viewport fix. With CI's time-loaded Exercise Alpha plot the Time Controller section claims ~173px of the ~487px GoldenLayout sidebar; the flexible Tools/Layers split then leaves the Layers feature-list ~29px — below one 40px row — so feature rows render but are clipped and unclickable. Reproduced locally with `CI=1` (which selects the test-data catalog the same way CI does). ActivityPanel's short-height adaptation already collapses the Time Controller to free space, but it was gated behind `hasSelectedFeature` — a deadlock, since the space that makes the feature list clickable was only freed *after* a selection the user couldn't make (the list was too short to click). Remove that gate: at clientHeight < threshold the Time Controller collapses on mount regardless of selection (Tools/Layers stay expanded; still never persisted, manual toggles still win). With this + the FeatureList fill fix, all previously-failing feature-row flows pass against the CI catalog (ui-review-layout SC-005, styling-tools, tool-execution, selection-sync — 15/15 with CI=1). Updated the ActivityPanel.shortHeight unit tests that asserted the old gated behaviour (132 component tests green; tsc clean).
…ll-suite caveat Records the local≠CI trap that cost several PR #668 CI round-trips: bare run-playwright serves the preview dev catalog (short forms) while CI serves apps/vscode/test-data/local-store (Exercise Alpha, time-loaded → taller ActivityPanel sections). Set CI=1 to mirror CI's catalog. Also notes CI=1 on isolated specs is necessary but not sufficient — the full suite runs serially with shared browser state, so run the whole suite before pushing E2E fixes.
Replaces the Time Controller auto-collapse (which regressed the storyboard-capture viewport-controls invariant, spec #264/#273) with the spec's sanctioned US4/FR-012 resolution: the activity column scrolls. At 1280x720 the ~487px GoldenLayout sidebar can't fit all five sections (a time-loaded Time Controller alone is ~173px). Instead of forcing a section closed: - ActivityPanel.css: column is overflow-y:auto; the Time Controller is pinned position:sticky/top:0 so its viewport/playback controls stay visible and reachable at any scroll offset (without this it scrolls up under the app header and the #264/#273 occlusion invariant fails); flexible Tools/Layers sections get a min-height so their lists never collapse below a clickable row and scroll internally; collapsed sections reset that min-height. - ActivityPanel.tsx: drop the mount-time clientHeight auto-collapse effect (and the now-unused SHORT_HEIGHT_THRESHOLD / useLayoutEffect import); add a sticky prop to PaneSection and apply it to the Time Controller. https://claude.ai/code/session_01NzLVtrLYBaVemwVPWK49tM
…le column Feature-row selection helpers clicked .debrief-feature-row__content via Playwright's click(). In the now-scrollable ActivityPanel column the virtualised (@tanstack/react-virtual) row, although rendered inside the 1280x720 viewport (verified via getBoundingClientRect), trips Playwright's actionability heuristics — a plain click() scroll-thrashes to a 30s 'element is not visible' timeout, and a force click reports 'outside of the viewport' — because the nested inner-list scroll plus the sticky Time Controller confuse its scroll/visibility checks. Add a clickVirtualisedRow helper that scrolls the row to centre and dispatches the click directly through the DOM (MouseEvent, carrying any modifier keys), firing React's delegated onClick (feature selection) without Playwright's viewport machinery. Route every feature-row selection through it: AnalysisPage selectLayer + selectFeature(layers), and the selectTrack/selectTrackViaFeatureList helpers and inline clicks in styling-tools, tool-execution, log-panel, capture-log-evidence, run-dropdown-visibility-format, selection-sync, and properties-multi-select. https://claude.ai/code/session_01NzLVtrLYBaVemwVPWK49tM
Member
Author
P2.2 reworked: scrollable column (not auto-collapse) + E2E selection fixTwo commits pushed ( Why the approach changedThe original P2.2 cut auto-collapsed the upper ActivityPanel sections at short height. That regressed the storyboard-capture viewport-controls invariant (specs #264/#273): collapsing/squeezing the Time Controller let it scroll up under the app header, so New resolution (the spec's sanctioned US4/FR-012 "column scrolls" option):
E2E test-infra fixThe scrollable column + sticky header exposed a Playwright actionability problem with the virtualised ( New Verification (CI=1 — mirrors CI's Exercise Alpha catalog)
The P2.2 row in the PR description above now describes a superseded approach — flagging it here rather than rewriting the original author's summary. Generated by Claude Code |
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
2 participants
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
Closes the two still-open P1 items and all four P2 items from the 2026-04-26 UI review (re-walked 2026-06-06). Six independent, surgical fixes across the web-shell and four shared components — no new runtime dependencies, no schema change.
--debrief-header-link-hctoken (#9CDCFE≈ 8.6:1 on#3c3c3c) + underline/weight in high-contrast modes.properties-screenshotsflakyretries: 0. 40/40 first-attempt passes (10× no-retry).getDefaultLayout(containerWidth)with discrete bands (~280px ≤1366, ~320px middle, ~380px ≥1600); map keeps the majority;LAYOUT_VERSIONbumped so legacy fixed-25% layouts fall back.clientHeight < 820+ a feature is selected; never persists, no-op ≥900px or when controlled.virtualizer.measure()gated touseEffect([rowHeight]); size persisted via a typed, versioned helper that narrows to theThumbnailSizeunion.The interesting bit: the audit caught a wrong assumption
The first cut of P1.3 routed header links through the theme-aware
--debrief-color-primary, assuming the header background follows the theme to near-white in HC-light. The axe-core audit (SC-001) measured 1.22:1 — because the web-shell title bar is a fixed dark#3c3c3cin every theme (mirroring VS Code), so the dark#0F4A85link was nearly invisible. The executable check, not visual inspection, forced the correct bright-token fix. This supersedes plan Decision #4's light-background assumption.Verification
[rowHeight]-gated re-measure.ui-review-contrast,ui-review-layout,ui-review-catalog— all green, covering SC-001/003/004/005/006/007 + FR-011/016/020. Real before/after screenshots captured intospecs/281-ui-review-p1-p2-fixes/evidence/screenshots/.evidence/flake-proof.txt— 40/40 first-attempt passes withretries:0(SC-002).tscacross all workspaces (0 errors).Notes
interaction.gifis omitted:ffmpegisn't available in the cloud session (the sharedvideoToGifhelper skips when absent). Static stills (catalog-collapse.png,thumbnail-sizes.png) cover the same flows.localStoragehere is UI-preference state (layout, thumbnail size), not domain data — outside the Article IV.4 writer-abstraction rule.Evidence + blog post:
specs/281-ui-review-p1-p2-fixes/{evidence,media}/.https://claude.ai/code/session_01NzLVtrLYBaVemwVPWK49tM
Generated by Claude Code