Data-viz: event-density strip + workspace swimlane (#546) — epic #500 completes - #547
Conversation
The #533 lesson applied BEFORE building instead of after. The ledger claimed the two remaining #500 charts were "thin until runs carry more telemetry" — measured, that was wrong about the telemetry and right only about the fixtures: real runs write dense events.jsonl through the observe hooks on every harness (tool_call bursts are already a first-class feed concept), but every canonical fixture carried 5-6 events across 3 hours. Sparse enough that a chart could pass its tests having never drawn anything meaningful. fixtureDenseRun writes ~50 events with real shape: ten >=3/min tool bursts, a failed attempt, a guardrail block with an approved-then-consumed override, a retry that passes, and a stage still in progress — the open-ended span the swimlane must render honestly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A time-bucketed activity strip above the stream; selecting a bucket filters the visible list to that window. One source of truth by construction: buckets are computed from the SAME served feed array the list renders — never a second read of raw events, or the strip and the list disagree the first time a cap or a burst rule changes. An assertion pins that every served item lands in exactly one bucket and that filtering by a bucket window yields exactly its count (the final bucket closes on the right, since the newest item defines the span end and was counted into it). A quiet minute is a REAL zero — events.jsonl is complete for the window it covers — so empty buckets render as gaps, and gaps are not clickable: you cannot select silence. Occupied buckets are BUTTONS, not rects, so keyboard and focus come free (the #526 axe lesson), each named for a screen reader. The selection is a client-local viewing aid, never persisted; a snapshot that reshapes the feed under an active filter drops the filter rather than showing a stale-window empty list. Unparseable timestamps are dropped, never coerced — Date.parse NaN treated as 0 would file items under 1970. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) One lane per stage the timeline actually names (first-seen order, never the canonical 15 with twelve empty rows), spans from task_started -> task_validated pairs, amber markers where a guardrail or gate fired. Honest geometry throughout. An attempt that never validated stays OPEN — dashed edge, reaching the last observed moment of the timeline, never an invented completion; the abandoned middle attempt of a retried task renders exactly that way. The axis is linear because a run that sat blocked for a day IS mostly gap; compressing it would hide the thing a manager looks for. A validated event with no matching start draws nothing — there is no span, and inventing one would claim a start nobody observed. Reads the SAME #496 section the event list renders (persisted or events-fallback, capped, source-labeled), so the two views cannot disagree. The source label survives verbatim — it is a truth label. Stated deviation from the epic text (recorded on #546): the child says the swimlane replaces the flat list. The list is the only place event-level detail is readable, and deleting detail to add a summary reduces information — so the swimlane renders above the list, which stays. Lane ids are external data: a null-prototype map, so a task named __proto__ cannot poison the lookup. Labels escaped in every position. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#546) Found by the new browser suite on its first run against fixtureDenseRun: the swimlane painted 07-code's FAILED first attempt green. Two faults stacked. The #496 events-fallback mapping in the projection copied ts/type/task_id/stage_id and silently discarded `status` — the one field that says how an attempt ended. And the swimlane model then defaulted a missing verdict to PASS, which turned "the projection lost a field" into "every failed attempt reads as a success". The projection now carries the verdict, and the model treats a verdict-less validation as UNKNOWN — grey, neither claimed nor denied — because a green bar must mean an observed PASS and nothing weaker. Mutation-checked with both halves reverted together: two unit failures and the browser geometry journey. Regression pinned at both layers — the projection test asserts the verdict survives, the model test asserts UNKNOWN never renders as pass. This is what the dense fixture exists for: the sparse fixtures could never have exposed it, because their timeline never reached the fallback with a FAIL in it under a browser assertion on span colour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#546) The screenshot of the filtered strip showed the SELECTED bucket's bar vanishing instead of turning accent-coloured. An undefined custom property does not fall back: it invalidates the whole declaration at computed-value time, so background: var(--accent) computed to transparent — and the one bucket the reader chose became the one bucket they could not see. var(--accent) had four consumers and no definition: this strip's selection, plus three focus-visible outlines from the #537 batch that were silently unset (their tests asserted focus, not ring visibility, so they passed). A new styles-wide guard pins the class: every fallback-less var() reference must be defined somewhere the scan can see (the stylesheet or a page module writing inline custom properties). Its first run found FOUR more, all pre-existing: --mono (two font-family rules silently inheriting the body font) and the Evidence Center trio --blue-soft/--danger/--success (pressed filter lost its tint; verdict borders fell to currentColor). All defined against the existing palette. var(--x, fallback) is exempt — a fallback is the author handling absence. The browser test now also asserts the selected fill is painted and reads differently from an unselected bar — sampled after the background transition settles, because reading at the click returns the mid-interpolation base colour (the #541 sampling lesson, hit again). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe dashboard adds an event-density strip to Live Feed and a swimlane timeline to Run Workspace. It preserves validation statuses in event-derived timelines and adds dense-run fixtures, browser coverage, unit tests, styling, filtering, markers, and accessibility checks. ChangesDashboard data visualization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Live Feed event-density strip and Run Workspace swimlane; fix verdict + CSS tokens
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/observability/dashboard/ui/styles.js (3)
1188-1191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDistinguish the selected bucket by more than colour.
Line 1190 paints the selected fill
--accent(#1570ef). Line 1188 paints the hovered fill--blue(#1d4ed8). The two colours are nearly the same hue and lightness, so a hovered unselected bar reads like the selected bar. The selected fill also differs from the base fill#b9c4d8by colour alone, at a contrast ratio near 2.2:1, which is below the 3:1 that WCAG 1.4.11 asks for a non-text state indicator.
aria-pressedalready carries the state for assistive technology. Add a visual cue that does not depend on colour.♻️ Proposed cue for the selected bucket
.density-bucket:hover .density-fill { background: var(--blue); } .density-bucket:focus-visible { outline: 2px solid var(--accent); outline-offset: 1px; } .density-bucket[aria-pressed="true"] .density-fill { background: var(--accent); } +/* Selection must not rest on hue alone: the hover colour is a neighbouring + blue, so the pressed bucket also carries a mark the eye reads as state. */ +.density-bucket[aria-pressed="true"] { box-shadow: inset 0 2px 0 var(--ink); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/observability/dashboard/ui/styles.js` around lines 1188 - 1191, Update the .density-bucket[aria-pressed="true"] .density-fill rule to add a non-color visual cue for selected buckets, such as a distinct pattern or border, while retaining the existing accent color. Ensure the cue remains distinguishable from both hovered and unselected fills and does not rely on color alone.
1186-1187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the bucket tap target on a phone.
Line 1187 sets
.density-buckettoflex: 1 1 0; min-width: 4px. The model builds 24 buckets. Inside a 390px panel each bucket is about 12px wide, and a bucket next to gaps can reach the 4px minimum. WCAG 2.5.8 asks for a 24x24 CSS pixel target. The strip is 44px tall, so only the width falls short.The
.path-copyrule at lines 1141-1142 solves the same problem with an expanded::afterhit area, but that approach does not transfer here: the buckets sit 2px apart, so expanded hit areas would overlap and steal each other's taps. Reducing the bucket count on a narrow viewport is the honest fix.The browser suite at
tests/browser/dashboard-546-dataviz.test.jslines 135-146 measures overflow at 390px but does not measure bucket width.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/observability/dashboard/ui/styles.js` around lines 1186 - 1187, Update the responsive density-strip styles around .density-bucket to reduce the rendered bucket count on narrow phone viewports so each remaining bucket maintains a 24px minimum tap width, rather than expanding overlapping hit areas. Preserve the existing 24-bucket layout at wider widths and ensure the strip remains within the 390px panel without overflow; extend the browser coverage to assert the bucket width at the narrow viewport.
1207-1209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTie the open-span gradient to the accent token.
Line 1209 hardcodes
rgba(21,112,239,.5)andrgba(21,112,239,.2). That colour is--accent(#1570ef) written as literal channels. This PR added--accentto stop exactly this drift, so the literal will fall out of step the next time the accent changes.
rgba()cannot take a hex custom property, so add a channel token next to--accent.♻️ Proposed token for the accent channels
Add next to the
--accentdefinition:--accent: `#1570ef`; + /* Channel form of --accent, for the rgba() ramps that cannot read a hex. */ + --accent-rgb: 21, 112, 239;Then read it in the gradient:
-.swimlane-span.open { background: repeating-linear-gradient(90deg, rgba(21,112,239,.5) 0 6px, rgba(21,112,239,.2) 6px 12px); border-right: 2px dashed var(--blue); } +.swimlane-span.open { background: repeating-linear-gradient(90deg, rgba(var(--accent-rgb),.5) 0 6px, rgba(var(--accent-rgb),.2) 6px 12px); border-right: 2px dashed var(--blue); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/observability/dashboard/ui/styles.js` around lines 1207 - 1209, Add an accent RGB-channel custom property alongside the existing --accent definition, then update the .swimlane-span.open repeating-linear-gradient to use that channel token for both rgba stops instead of hardcoded 21,112,239 values; preserve the existing opacities and gradient behavior.tests/browser/dashboard-546-dataviz.test.js (2)
216-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis test does not exercise the sparse-run browser journey it names.
The body calls
window.timelineSwimlaneModelwith a hand-written array on line 221. That is the same input and the same assertion as the unit test attests/hub-dataviz-546.test.jslines 219-224. It runs in a browser, but it never renders anything.Two consequences:
- The custom seed on line 224 writes
fixtureBlockedRun, and nothing in the body reads that run. The seed is dead setup.- The named journey — open the sparse run's workspace, see no swimlane, see no page error — stays unverified. A swimlane that throws while rendering a sparse run would not be caught here.
Navigate to the blocked run's timeline tab and assert that
.swimlaneis absent while the event list still renders.As per coding guidelines: "Browser behavior must be verified with real-browser regression tests for critical journeys, including accessibility, responsive behavior, no-data honesty, navigation interaction, and page-error detection."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/browser/dashboard-546-dataviz.test.js` around lines 216 - 224, Update the browser test around openLiveFeed to exercise the seeded fixtureBlockedRun journey instead of directly calling window.timelineSwimlaneModel. Navigate to the blocked run’s timeline tab, assert that the event list renders, assert that .swimlane is absent, and retain page-error detection; keep only seed setup required for this journey and remove the redundant hand-written model assertion.Source: Coding guidelines
125-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not depend on a synchronous render inside
page.evaluate.Line 128 calls
button.click()and line 129 reads the row count in the same expression. This passes only whilefeedDensityToggle→renderLiveFeed→setHTMLstays synchronous. IfsetHTMLever defers the write torequestAnimationFrameor a microtask batch, line 129 reads the pre-click rows. The assertion on line 131 would then compare the bucket count against the unfiltered list and could pass or fail for the wrong reason.The sibling test at lines 87-92 already uses the safer shape:
page.click, then a separatepage.evaluate.💚 Proposed restructure of the agreement test
await openLiveFeed(page, server); - const agreement = await page.evaluate(() => { - const button = document.querySelector('`#live-feed-density` .density-bucket'); - const claimed = Number((button.getAttribute('aria-label').match(/(\d+) event/) || [])[1]); - button.click(); - return { claimed, listed: document.querySelectorAll('`#live-feed-list` .feed-row').length }; - }); - assert.equal(agreement.listed, agreement.claimed, + const claimed = await page.evaluate(() => { + const button = document.querySelector('`#live-feed-density` .density-bucket'); + return Number((button.getAttribute('aria-label').match(/(\d+) event/) || [])[1]); + }); + await page.click('`#live-feed-density` .density-bucket'); + await page.waitForFunction( + () => document.querySelectorAll('`#live-feed-density` [aria-pressed="true"]').length === 1, + null, { timeout: STATE_LOAD_TIMEOUT_MS }); + const listed = await page.evaluate( + () => document.querySelectorAll('`#live-feed-list` .feed-row').length); + assert.equal(listed, claimed, 'the bucket announces N events and the filtered list shows exactly N — one source of truth');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/browser/dashboard-546-dataviz.test.js` around lines 125 - 132, Restructure the agreement check so the click and row-count read are separate browser actions: use page.click on the density bucket first, then use page.evaluate to read the filtered row count and retain the previously captured claimed count. Follow the established pattern in the sibling test around the existing agreement test, ensuring the assertion observes the post-render state rather than relying on synchronous updates inside one evaluate callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/observability/dashboard/ui/pages/live-feed.js`:
- Around line 153-154: Update the density bucket label construction near `when`
to use the same local-time formatting as `timeHtml(item.ts)` instead of slicing
`toISOString()`. Keep the bucket’s ISO value separate if it is used elsewhere,
and preserve the existing count and singular/plural label formatting.
- Around line 183-192: Update the empty-state selection in the live-feed
rendering flow after the FEED_DENSITY_SELECTED filtering block so an empty
selected window uses copy indicating no events match the selected window, rather
than “No events yet.” Preserve the existing “No events yet” message when no
density window is selected or the feed itself is empty, and keep the filtered
count unchanged.
In `@tests/hub-dataviz-546.test.js`:
- Around line 122-134: Add regression coverage in the existing feed-density
window test suite that selects the final bucket from feedDensityModel, calls
feedDensityWindow, and verifies the half-open window includes the item at the
span end and matches that bucket’s count. Ensure the test fails if the +1ms
extension in feedDensityWindow is removed, while preserving the existing
first-bucket assertions.
---
Nitpick comments:
In `@src/observability/dashboard/ui/styles.js`:
- Around line 1188-1191: Update the .density-bucket[aria-pressed="true"]
.density-fill rule to add a non-color visual cue for selected buckets, such as a
distinct pattern or border, while retaining the existing accent color. Ensure
the cue remains distinguishable from both hovered and unselected fills and does
not rely on color alone.
- Around line 1186-1187: Update the responsive density-strip styles around
.density-bucket to reduce the rendered bucket count on narrow phone viewports so
each remaining bucket maintains a 24px minimum tap width, rather than expanding
overlapping hit areas. Preserve the existing 24-bucket layout at wider widths
and ensure the strip remains within the 390px panel without overflow; extend the
browser coverage to assert the bucket width at the narrow viewport.
- Around line 1207-1209: Add an accent RGB-channel custom property alongside the
existing --accent definition, then update the .swimlane-span.open
repeating-linear-gradient to use that channel token for both rgba stops instead
of hardcoded 21,112,239 values; preserve the existing opacities and gradient
behavior.
In `@tests/browser/dashboard-546-dataviz.test.js`:
- Around line 216-224: Update the browser test around openLiveFeed to exercise
the seeded fixtureBlockedRun journey instead of directly calling
window.timelineSwimlaneModel. Navigate to the blocked run’s timeline tab, assert
that the event list renders, assert that .swimlane is absent, and retain
page-error detection; keep only seed setup required for this journey and remove
the redundant hand-written model assertion.
- Around line 125-132: Restructure the agreement check so the click and
row-count read are separate browser actions: use page.click on the density
bucket first, then use page.evaluate to read the filtered row count and retain
the previously captured claimed count. Follow the established pattern in the
sibling test around the existing agreement test, ensuring the assertion observes
the post-render state rather than relying on synchronous updates inside one
evaluate callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a528af8b-d942-4e74-ab3d-776491c8f41a
📒 Files selected for processing (9)
src/observability/dashboard/state/run-workspace.jssrc/observability/dashboard/ui/pages/index.jssrc/observability/dashboard/ui/pages/live-feed.jssrc/observability/dashboard/ui/pages/run-workspace.jssrc/observability/dashboard/ui/styles.jstests/browser/dashboard-546-dataviz.test.jstests/helpers/dashboard-fixtures.jstests/hub-dataviz-546.test.jstests/run-workspace-timeline-496.test.js
| test('the filter window is the selected bucket, half-open (#546)', () => { | ||
| const api = feedRenderer(); | ||
| const model = api.feedDensityModel(feedItems(), 12); | ||
| const occupied = model.buckets.findIndex((bucket) => bucket.count > 0); | ||
| const window = api.feedDensityWindow(model, occupied); | ||
| assert.ok(window.startMs < window.endMs); | ||
| const inWindow = feedItems().filter((item) => { | ||
| const ms = Date.parse(item.ts); | ||
| return ms >= window.startMs && ms < window.endMs; | ||
| }); | ||
| assert.equal(inWindow.length, model.buckets[occupied].count, | ||
| 'filtering by the window yields exactly the bucket count — strip and list agree'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover the final bucket, where the window closes on the right.
This test picks the first occupied bucket, which is index 0. The special rule in feedDensityWindow applies only to the last bucket: line 143 of src/observability/dashboard/ui/pages/live-feed.js extends endMs by 1 ms so the item at exactly the span end stays in the window. The bucket-assignment clamp on line 129 counted that item into the last bucket, so the two must agree.
No test here selects the last bucket. Delete the + 1 and every assertion in this file still passes, while the strip announces N events and the list shows N-1.
As per coding guidelines: "Every new fix must include regression coverage, and fixes—especially review findings—should be mutation-tested by reverting the fix and proving the test fails."
💚 Proposed regression test for the closing bucket
+test('the last bucket closes on the right so the newest event is not dropped (`#546`)', () => {
+ const api = feedRenderer();
+ const items = feedItems();
+ const model = api.feedDensityModel(items, 12);
+ const last = model.buckets.length - 1;
+ const window = api.feedDensityWindow(model, last);
+ const inWindow = items.filter((item) => {
+ const ms = Date.parse(item.ts);
+ return ms >= window.startMs && ms < window.endMs;
+ });
+ assert.equal(inWindow.length, model.buckets[last].count,
+ 'the newest item was counted into the last bucket, so the filter must include it too');
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('the filter window is the selected bucket, half-open (#546)', () => { | |
| const api = feedRenderer(); | |
| const model = api.feedDensityModel(feedItems(), 12); | |
| const occupied = model.buckets.findIndex((bucket) => bucket.count > 0); | |
| const window = api.feedDensityWindow(model, occupied); | |
| assert.ok(window.startMs < window.endMs); | |
| const inWindow = feedItems().filter((item) => { | |
| const ms = Date.parse(item.ts); | |
| return ms >= window.startMs && ms < window.endMs; | |
| }); | |
| assert.equal(inWindow.length, model.buckets[occupied].count, | |
| 'filtering by the window yields exactly the bucket count — strip and list agree'); | |
| }); | |
| test('the filter window is the selected bucket, half-open (`#546`)', () => { | |
| const api = feedRenderer(); | |
| const model = api.feedDensityModel(feedItems(), 12); | |
| const occupied = model.buckets.findIndex((bucket) => bucket.count > 0); | |
| const window = api.feedDensityWindow(model, occupied); | |
| assert.ok(window.startMs < window.endMs); | |
| const inWindow = feedItems().filter((item) => { | |
| const ms = Date.parse(item.ts); | |
| return ms >= window.startMs && ms < window.endMs; | |
| }); | |
| assert.equal(inWindow.length, model.buckets[occupied].count, | |
| 'filtering by the window yields exactly the bucket count — strip and list agree'); | |
| }); | |
| test('the last bucket closes on the right so the newest event is not dropped (`#546`)', () => { | |
| const api = feedRenderer(); | |
| const items = feedItems(); | |
| const model = api.feedDensityModel(items, 12); | |
| const last = model.buckets.length - 1; | |
| const window = api.feedDensityWindow(model, last); | |
| const inWindow = items.filter((item) => { | |
| const ms = Date.parse(item.ts); | |
| return ms >= window.startMs && ms < window.endMs; | |
| }); | |
| assert.equal(inWindow.length, model.buckets[last].count, | |
| 'the newest item was counted into the last bucket, so the filter must include it too'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/hub-dataviz-546.test.js` around lines 122 - 134, Add regression
coverage in the existing feed-density window test suite that selects the final
bucket from feedDensityModel, calls feedDensityWindow, and verifies the
half-open window includes the item at the span end and matches that bucket’s
count. Ensure the test fails if the +1ms extension in feedDensityWindow is
removed, while preserving the existing first-bucket assertions.
Source: Coding guidelines
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| .density-strip { display: flex; align-items: flex-end; gap: 2px; height: 44px; padding: 6px 2px 4px; margin-bottom: 10px; border-bottom: 1px solid var(--line); } | ||
| .density-bucket { flex: 1 1 0; min-width: 4px; height: 100%; display: flex; align-items: flex-end; padding: 0; border: 0; background: none; cursor: pointer; border-radius: 3px 3px 0 0; } | ||
| .density-bucket:hover .density-fill { background: var(--blue); } |
There was a problem hiding this comment.
1. Density buckets under 44px 📜 Skill insight ☑ Accessibility
The new event-density strip renders each bucket as a clickable button, but the CSS allows bucket widths far below 44px (e.g., min-width: 4px), which violates the minimum touch target size requirement. On mobile and even many desktop widths, these buttons will be difficult to tap accurately.
Agent Prompt
## Issue description
The density-strip buckets are interactive `<button>` elements but are styled to be much smaller than the required 44px minimum touch target size.
## Issue Context
`.density-bucket` currently uses `flex: 1 1 0` with `min-width: 4px`, so when there are many buckets (e.g., 24) the computed width per button will often be far under 44px.
## Fix Focus Areas
- src/observability/dashboard/ui/styles.js[1186-1188]
## Suggested implementation direction
- Make the density strip responsive so each bucket button is at least 44px wide (e.g., compute bucket count based on available width, or reduce bucket count on small viewports).
- Alternatively, allow horizontal scrolling with `min-width: 44px` per bucket (but ensure this aligns with the product’s no-overflow requirements/tests).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
CodeRabbit, Major — no test covered the final bucket, and it named the
surviving mutation exactly: delete the +1 in feedDensityWindow and every
assertion still passed while the strip announced N events and the list
showed N-1. My half-open test had picked the FIRST occupied bucket; the
right-closed rule only exists for the LAST. New test selects the last
bucket and asserts the item at the exact span end is inside the window —
mutation-checked with precisely the deletion CodeRabbit described.
CodeRabbit — bucket labels were UTC (an ISO slice) while feed rows render
local time, so a non-UTC reader saw a bucket labelled 09:15 holding rows
that read 14:45. Labels now go through the same Intl local-time path,
asserted by equality with the formatter so the test holds in any timezone.
CodeRabbit — an empty FILTERED list fell back to "No events yet", flatly
contradicting the count line ("0 of 41 events"). A filtered empty now says
what is true: the stream holds N events, the filter hides them, click the
highlighted bucket to clear.
Qodo — 24 fixed buckets at a 390px viewport rendered ~12px slivers, below
any usable touch target. The bucket count now adapts to the measured strip
width (viewport-estimated when the page is hidden at render time), keeping
every button at the WCAG 2.5.8 24px minimum across the full 44px strip
height; fewer, wider buckets carry the same story. Browser test measures
button widths at 390px.
Qodo — timelineSwimlaneModel exceeded the 50-line rule; the per-event
application is extracted into swimlaneApplyEvent (33 + 24 lines), tests
unchanged and green as the behaviour proof.
Qodo — fixtureDenseRun clobbered a composed budget, the exact class it
caught on #533: projectShell rewrites budget.json, so dense after spend
replaced the deliberate $10 cap with the $25 default. Dense now passes
keepExistingBudget (it does not care about the budget, so it must not vote
on it); regression test pins the composed cap.
1998/1998 core, 67/67 browser, lint 0 errors, typecheck clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All six findings fixed in
|
2 participants
Closes #546. The final two children of epic #500 — the epic completes at 5/5 with this.
Correcting my own claim first
The ledger said these two were "thin until runs carry more telemetry." Measured: wrong about the telemetry, right only about the fixtures. Real runs write dense
events.jsonlthrough the observe hooks on every harness; the canonical fixtures carried 5–6 events. So this PR's first commit isfixtureDenseRun(~50 events with real shape: ten tool bursts, a failed attempt, a guardrail block with an approved-then-consumed override, a passing retry, an in-progress stage) — the #533 lesson applied before building instead of after.That fixture promptly earned its keep twice (below).
1. Event-density strip (Live Feed)
A time-bucketed activity strip above the stream; clicking a bucket filters the list to that window, clicking again clears.
2. Run Workspace swimlane (stages × time)
One lane per stage the timeline actually names, spans from
task_started → task_validatedpairs, amber markers where a guardrail or gate fired. Reads the same #496 section the event list renders; the source label survives verbatim. Stated deviation: the epic said the swimlane replaces the flat list — it renders above the list instead, because the list is the only place event-level detail is readable, and deleting detail to add a summary reduces information. One-line change if the list should truly go.Three real bugs found while building — one of them governance-grade
The events-fallback timeline drops the validation verdict. The browser suite's first run against the dense fixture showed 07-code's FAILED first attempt painted green. Two faults stacked: the #496 fallback mapping in
state/run-workspace.jscopied ts/type/task_id/stage_id and silently discardedstatus; and the swimlane model defaulted a missing verdict to PASS. The projection now carries the verdict, and a verdict-less validation closes as UNKNOWN (grey) — a green bar must mean an observed PASS and nothing weaker. Regression pinned at both layers; mutation-checked (2 unit failures + 1 browser failure with both halves reverted).An abandoned attempt ran underneath its own retry. Found by screenshot, invisible to every numeric assertion: the model extended every open span to the timeline end, so 07-code's abandoned second attempt stretched 8 hours, its dashes running through the retry's green bar. A superseded open span now ends at its successor's start; only a trailing open span reaches the last observed moment. Mutation-checked.
var(--accent)was never defined. The screenshot of the filtered strip showed the selected bucket vanishing — an undefined custom property invalidates the whole declaration, so the selection background computed to transparent. Four consumers, no definition (including three #537-era focus rings, silently unset — their tests asserted focus, not ring visibility). A new styles-wide guard pins the class: every fallback-lessvar()reference must resolve. Its first run found four more pre-existing ones —--mono(two font-family rules inheriting the body font) and the Evidence Center trio--blue-soft/--danger/--success(pressed filter lost its tint, verdict borders fell to currentColor). All defined against the existing palette;var(--x, fallback)exempt.Verification
Not built
Drag-range selection (click-to-select shipped; recorded on #546 for later if click proves insufficient). Any new server projection.
Summary by CodeRabbit
New Features
Bug Fixes