feat(frontend): read a chart day over its bar, and expand the zone rail on hover - #2441
Conversation
The value appeared in the caption under the strip. That is a change in the middle of a chart, which is exactly where a reader looking at one bar is not looking: they hover, the number moves somewhere in their periphery, and they never learn it was there. It now sits over the bar the pointer is on, and the caption stops changing at all — what is down there is constant again. One positioned element rather than a tooltip per day: a month is thirty-one triggers, and thirty-one floating cards is both heavy and the wrong shape. It sits above the bars so it never covers the neighbours being compared against, and its position is clamped so the first and last days do not push it outside the card and get it clipped. Refs constructorfabric#2408 Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…product Four things make it work, and each is there because leaving it out broke something. The rail keeps its 56px slot and the labels open OVER what is beside them: widening the element itself shoves the pane sideways every time a pointer crosses the rail on its way elsewhere. The buttons widen only while it is open — a label you can read but not click is a trap, because the pointer leaves the narrow column on its way to the word and the rail shuts before it arrives. People aim at what they can read. The buttons sit inside the hover target, so a pointer resting on one keeps it open rather than fighting what opened it. And a click collapses it until the pointer leaves: a click navigates, the pointer has not moved, and without this the rail reopens on top of the pane the click was aimed at. Two things could not be copied. The lite product keeps the post-click flag in sessionStorage because its click reloads the page; here navigation is client-side, so plain state carries it. And the open state is held in React rather than expressed as CSS variants — it is the product of two facts, one of which no selector can see. The variant version silently never matched, and nothing said so; hence the tests. The panel takes pointer events while open. With it inert the gaps between buttons belong to whatever is underneath, so a pointer moving from an icon towards its label crossed bare panel, the rail counted that as having been left, and it slammed shut under the hand reaching for it. It deliberately does not cover the pane beside it. What the panel does not cover is faded rather than dimmed: the rows do not object to being darkened, they object to being CUT, and a hard edge through the middle of a word reads as a rendering fault whatever its brightness. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
📝 WalkthroughWalkthroughLensRail now supports delayed hover expansion, keyboard and pointer interactions, responsive reset behavior, and expanded zone labels. LensRail tests cover these interactions. ChangesPortal and metric interactions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PointerUser
participant LensRail
participant ZoneItem
participant ZoneNavigation
PointerUser->>LensRail: pointer enters rail
LensRail->>LensRail: start delayed expansion timer
LensRail->>ZoneItem: render expanded labels
PointerUser->>ZoneItem: click zone
ZoneItem->>ZoneNavigation: select zone
LensRail->>LensRail: dismiss after click
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…g its header Review found ten things. The two that mattered both showed the previous design claiming, in its own comments, to prevent something it did not. The panel became clickable the moment `open` flipped while its opacity waited 200ms, because `pointer-events` is not animatable and carried no delay. For that window it was an invisible 136px-wide sink over the pane: clicks aimed at a row landed on it and vanished. Worse, a pointer crossing the rail entered the invisible panel, which counted as still being inside, so the rail opened after the delay anyway — over the row being reached for. The comment said the delay stopped exactly that. So the wait is now a timer and `open` means "the wait is over". Open and visible are the same fact, and there is no window where one is true and the other is not. The `dismissed` guard was set by any click, including the one Enter produces on a focused button — and it was only ever cleared by a pointer leaving. A keyboard user pressing Enter left the rail dead to the mouse until they moved onto it and off again. It is now set only for pointer-driven clicks, and blur clears it too. Three more ways the state was stranded: a width change unmounting the rail under the pointer, focus never opening it at all (eight identical icons and the label at zero opacity for anyone not using a pointer), and no reset on unmount. The width case is handled during render rather than in an effect, because an effect sets state after painting the stale frame. `[&>div]:overflow-visible` had beaten `SidebarContent`'s own `overflow-auto` on specificity and left the zone list unable to scroll at all. The escape now applies to that one box and only while the labels are showing — a box cannot clip one axis and release the other, so scrolling is the half worth losing, and only for as long as something is being read. The fade is `absolute` rather than `fixed` (it worked only because the rail happens to sit at the viewport edge), starts from the panel's own token rather than the page background (they differ, most visibly in dark theme, putting a step at the seam), mirrors in RTL (a logical inset with a physical gradient put the opaque end at the wrong side and reinstated the hard cut it exists to remove), and renders only when the pane is actually beside the rail rather than collapsed off-canvas. On the chart: the readout stood 26px above a strip 8px below its header, so it covered the comparisons a reader hovers in order to reason against. It now has headroom. And it is anchored to its bar's edge and grown inwards rather than centred and clamped — centring needs the rendered width to stay inside, and a constant guarding the ends is a guess that longer strings walk past. The tests mocked a layout value the app never produces, so the middle width tier — where the pane is collapsed — went untested. Four cases added for the orderings above; two of them fail on the previous commit. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Pushed a review round. The body above has been updated to describe what the code now does rather than what the first attempt did — the two differed materially after this, and a description of a design that is no longer there is worse than none. The mechanism changed. The rail opened via a CSS delay on the fade; it now opens on a timer. Three ways the state could be stranded, none of which types or the original four tests would show:
Each is covered now; two of the new tests fail on the previous commit. One fix broke another feature and had to be reconciled. Removing the blanket child-overflow escape restored the zone list's scrolling and immediately clipped the labels — that escape was what let the widened buttons paint outside the rail. A box cannot clip one axis and release the other, so the two cannot both hold at once. Scrolling is the half worth losing, and only while the labels are showing. Also: the fade is On the chart, the readout stood 26px above a strip 8px below its header, so it covered the two comparisons a reader hovers in order to reason against. It has headroom now, and it anchors to its bar's edge and grows inwards instead of centring — centring needs the rendered width to stay inside, and the constant guarding the ends was a guess that longer strings walked past. The tests also mocked a layout value the app never produces, so the middle width tier — where the pane is collapsed and the fade has no work to do — was untested. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/frontend/src/components/portal/lens-rail.test.tsx (2)
121-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
afterEachnext tobeforeEach.
afterEachsits at module scope between the twodescribeblocks. Vitest applies module-scope hooks to every test in the file, so this hook also runs for the firstdescribe. The placement suggests it applies only to the seconddescribe.Move it directly after the
beforeEachblock at Line 69 so the scope is obvious.🤖 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/frontend/src/components/portal/lens-rail.test.tsx` around lines 121 - 123, Move the module-scope afterEach hook using vi.useRealTimers() to immediately follow the beforeEach block near the start of the second describe, leaving its behavior unchanged and making its intended scope clear.
71-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the three untested branches in
LensRail.The suite covers hover, click dismissal, keyboard, phone layout, and layout reset. Three changed branches in
lens-rail.tsxhave no coverage:
onPointerEnterearly return whene.pointerType === "touch"(Line 152). A regression here would flash the labels on every tap.onBlurCaptureclosing the rail when focus moves outside (Lines 167-172). This is the only path that closes a keyboard-opened rail.- The
paneIsBesideguard on the fade element (Lines 186-209). The mockedSidebarProviderstate drives this, so both states are reachable in jsdom.As per coding guidelines: "Ensure new and changed lines achieve at least 80% test coverage."
Do you want me to draft these three test cases?
🤖 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/frontend/src/components/portal/lens-rail.test.tsx` around lines 71 - 183, Add tests in the LensRail suites covering the three untested branches: verify touch pointer entry does not reveal labels, verify a keyboard-opened rail closes when focus moves outside via onBlurCapture, and verify the fade element behavior for both paneIsBeside states using the mocked SidebarProvider layout state. Keep existing interaction coverage intact and exercise each branch directly.Source: Coding guidelines
src/frontend/src/components/portal/lens-rail.tsx (1)
201-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Tailwind CSS 4 gradient utilities
Replace
bg-gradient-to-randrtl:bg-gradient-to-lwithbg-linear-to-randrtl:bg-linear-to-l.tailwind-merge3.5.0 preserves both utilities.🤖 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/frontend/src/components/portal/lens-rail.tsx` at line 201, Update the gradient class in the lens rail component to use Tailwind CSS 4 utilities: replace bg-gradient-to-r with bg-linear-to-r and rtl:bg-gradient-to-l with rtl:bg-linear-to-l, preserving the existing gradient directions and other classes.src/frontend/src/components/widgets/metric-views/metric-activity.tsx (1)
322-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for the hover readout.
The current tests do not trigger
onPointerEnteror assert the overlay. Add left-half and right-half cases that check the rendereddayTitletext,leftanchor, andtranslateX(-100%)behavior.🤖 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/frontend/src/components/widgets/metric-views/metric-activity.tsx` around lines 322 - 340, Add direct tests for the hover readout rendered by the metric activity component: trigger onPointerEnter for days in both the left and right halves, assert the rendered dayTitle text and percentage-based left style, and verify translateX(-100%) is absent on the left case but applied on the right case.Source: Coding guidelines
🤖 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/frontend/src/components/portal/lens-rail.test.tsx`:
- Around line 55-56: Update the beforeEach fake-timer setup to call
vi.useFakeTimers() without shouldAdvanceTime. Preserve the explicit
advanceTimers callback so the 200 ms opening timer remains under deterministic
test control.
In `@src/frontend/src/components/portal/lens-rail.tsx`:
- Around line 156-172: Update the onPointerLeave handler to receive the pointer
event and only call close() and setDismissed(false) when focus is not contained
within the current target, matching the containment guard used by onBlurCapture;
preserve the existing dismissal behavior when the rail has no focused
descendant.
In `@src/frontend/src/components/widgets/metric-views/metric-activity.tsx`:
- Around line 327-335: Update the readout positioning logic near the hovered-bar
style so the translated case uses the hovered bar’s right boundary, `(hovered +
1) / days.length * 100`, while preserving the existing left-boundary position
and no-transform behavior for left-side readouts.
- Around line 364-369: Update the constantDenominator display in the metric
activity view to use the denominator’s API-provided unit metadata instead of
hardcoding “per day”; support units such as chat_active_day,
total_chat_messages, and pr_created, and omit the unit when metadata is
unavailable while preserving the existing silent-reading fallback.
---
Nitpick comments:
In `@src/frontend/src/components/portal/lens-rail.test.tsx`:
- Around line 121-123: Move the module-scope afterEach hook using
vi.useRealTimers() to immediately follow the beforeEach block near the start of
the second describe, leaving its behavior unchanged and making its intended
scope clear.
- Around line 71-183: Add tests in the LensRail suites covering the three
untested branches: verify touch pointer entry does not reveal labels, verify a
keyboard-opened rail closes when focus moves outside via onBlurCapture, and
verify the fade element behavior for both paneIsBeside states using the mocked
SidebarProvider layout state. Keep existing interaction coverage intact and
exercise each branch directly.
In `@src/frontend/src/components/portal/lens-rail.tsx`:
- Line 201: Update the gradient class in the lens rail component to use Tailwind
CSS 4 utilities: replace bg-gradient-to-r with bg-linear-to-r and
rtl:bg-gradient-to-l with rtl:bg-linear-to-l, preserving the existing gradient
directions and other classes.
In `@src/frontend/src/components/widgets/metric-views/metric-activity.tsx`:
- Around line 322-340: Add direct tests for the hover readout rendered by the
metric activity component: trigger onPointerEnter for days in both the left and
right halves, assert the rendered dayTitle text and percentage-based left style,
and verify translateX(-100%) is absent on the left case but applied on the right
case.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37b1acc6-58c0-419e-b5a2-07f1f4650d2a
📒 Files selected for processing (3)
src/frontend/src/components/portal/lens-rail.test.tsxsrc/frontend/src/components/portal/lens-rail.tsxsrc/frontend/src/components/widgets/metric-views/metric-activity.tsx
| beforeEach(() => { | ||
| vi.useFakeTimers({ shouldAdvanceTime: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Vitest useFakeTimers shouldAdvanceTime with user-event advanceTimers option conflict
💡 Result:
When using Vitest fake timers with the Testing Library user-event package, historical compatibility issues often lead to tests hanging or timing out [1][2]. These issues typically arise because user-event expects a specific timer advancement interface, historically modeled after Jest, which Vitest does not provide by default in the global scope [3][2]. There is no conflict between vi.useFakeTimers({ shouldAdvanceTime: true }) and the user-event advanceTimers option in terms of logical contradictions, but they serve different purposes and can be confusing when used together [4][5]. 1. Understanding the Options: - vi.useFakeTimers({ shouldAdvanceTime: true }): This tells Vitest to automatically increment the mocked clock based on the real system time passage [4]. It is rarely the recommended solution for standard component interaction tests, as it can introduce flakiness by tying your test execution speed to the real system clock [6]. - userEvent.setup({ advanceTimers: ... }): This allows user-event to trigger time advancement explicitly when it needs to wait for interaction delays [7][5]. 2. Resolution and Best Practices: - Modern versions of @testing-library/user-event (v14.4.0+) have built-in support to auto-detect Vitest's vi global [3]. This eliminates the need for manual configuration or global stubs in most cases [3]. - If you encounter issues, ensure you are using the latest version of user-event [3]. - If you must configure it manually, explicitly pass Vitest's advancement function [5]: const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime, }); - Avoid shouldAdvanceTime: true unless you have a specific requirement to simulate real-time passage within a fake timer environment, as it often defeats the purpose of deterministic testing [6]. If you still experience timeouts, verify that you are awaiting the user-event calls (e.g., await user.click(...)) and that your test setup does not inadvertently leak timers by failing to call vi.useRealTimers() or vi.restoreAllMocks() in an afterEach block [8].
Citations:
- 1: Bug Report: userEvent click causing test timeout in Vitest but works in Jest vitest-dev/vitest#6179
- 2:
userEvent.click()fails when used withvi.useFakeTimers(), all available solutions are not working testing-library/user-event#1115 - 3: feat: automatically advance fake timers in Vitest testing-library/user-event#1304
- 4: https://vitest.dev/config/faketimers
- 5: https://qaskills.sh/blog/vitest-testing-library-user-event-fake-timers
- 6: Update to v14 breaks @testing-library/user-event on Vitest testing-library/react-testing-library#1197
- 7: https://testing-library.com/docs/user-event/options
- 8: https://testing-library.com/docs/using-fake-timers/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'src/frontend/src/components/portal/lens-rail.test.tsx' '*package.json' '*vitest*' '*vite.config*' | sed -n '1,120p'
printf '%s\n' '--- test structure and timer usage ---'
sed -n '1,230p' src/frontend/src/components/portal/lens-rail.test.tsx
printf '%s\n' '--- relevant dependency versions and configuration ---'
rg -n --glob 'package.json' --glob '*lock*' --glob '*vitest*' --glob '*vite.config*' \
'(`@testing-library/user-event`|vitest|fakeTimers|shouldAdvanceTime|advanceTimers)' . | sed -n '1,220p'Repository: constructorfabric/insight
Length of output: 19980
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("src/frontend/src/components/portal/lens-rail.test.tsx")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "shouldAdvanceTime" in line or "advanceTimers" in line or "stays shut" in line or "hover(" in line or "unhover(" in line or "200" in line:
print(f"{i}: {line}")
PYRepository: constructorfabric/insight
Length of output: 1335
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- LensRail timer implementation ---'
rg -n -C 12 'setTimeout|clearTimeout|hover|pointer|open|close|isOpen' \
src/frontend/src/components/portal/lens-rail.tsx src/frontend/src/components/portal/lens-rail.test.tsx
printf '%s\n' '--- fake-timer configuration ---'
sed -n '1,190p' src/frontend/vitest.config.ts
printf '%s\n' '--- user-event package metadata from lockfile ---'
sed -n '1565,1595p' src/frontend/pnpm-lock.yaml
sed -n '5500,5525p' src/frontend/pnpm-lock.yamlRepository: constructorfabric/insight
Length of output: 50381
🌐 Web query:
site:vitest.dev/config/faketimers shouldAdvanceTime advanceTimersByTime real time
💡 Result:
shouldAdvanceTime: truemakes Vitest advance fake time automatically according to real elapsed time. By default, it advances in20 msincrements; configure this withadvanceTimeDelta. [1]vi.advanceTimersByTime(ms)is manual: it immediately advances mocked time by exactlyms, without waiting for real time. [2]
vi.useFakeTimers({
shouldAdvanceTime: true,
advanceTimeDelta: 50,
})
// Real 50 ms passing advances fake time by about 50 ms.So, use shouldAdvanceTime when timers should track wall-clock time; use advanceTimersByTime for deterministic, instant test control. [1][2]
🌐 Web query:
site:testing-library.com/docs/user-event/options advanceTimers fake timers
💡 Result:
In testing environments, particularly when using testing-library's user-event, advanceTimers is a configuration option that allows the library to coordinate with your test runner's fake timers [1][2]. When user-event simulates interactions (such as typing or clicking), it often introduces artificial delays between events to mimic real user behavior [1]. When tests use fake timers (a feature provided by libraries like Jest or Vitest to mock time), these internal delays can conflict with the test runner, often causing tests to hang or time out if the fake clock is not advanced correctly [1][2]. To resolve this, you pass the test runner's time advancement function (e.g., jest.advanceTimersByTime or vitest.advanceTimersByTime) to the advanceTimers option in the userEvent.setup configuration [1][2]. This enables user-event to programmatically advance the fake clock by the necessary delay duration, allowing the simulated interactions to proceed without stalling the test [1]. Example usage with Jest: const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); This approach is recommended over setting delay: null, as the latter can lead to unexpected behavior during test execution [1].
Citations:
- 1: https://testing-library.com/docs/user-event/options
- 2: https://testing-library.com/docs/user-event/options/
🌐 Web query:
site:github.com/testing-library/user-event 14.6.1 advanceTimers pointer hover unhover delay
💡 Result:
In @testing-library/user-event v14.6.1, the advanceTimers option and the delay configuration are used to manage asynchronous timing, particularly when working with fake timers in testing frameworks like Jest [1][2]. The advanceTimers option allows you to integrate your test runner's timer-advancing function (e.g., jest.advanceTimersByTime) into the user-event workflow [1][2]. This ensures that internal delays used by the library—which are intended to move events to the next macro task and allow asynchronous code to process—do not cause tests to hang or timeout when fake timers are active [3][1][2]. Usage typically involves configuring it during the setup phase: const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime, }); Regarding pointer interactions (hover/unhover) and delays: 1. The delay option dictates the pause duration between sequential user-event actions [1]. 2. By default, user-event introduces these small delays to better simulate real-user behavior and prevent race conditions with asynchronous state updates [1][2]. 3. If these internal delays conflict with your test environment (especially when using fake timers), you must either provide the advanceTimers callback to "fast-forward" through these delays [1] or, in specific scenarios where the overhead is not desired, set delay to null [1][2]. Version 14.6.1 included a fix to correct the documentation/description for the delay option [4], ensuring developers better understand how this configuration affects event execution timing.
Citations:
- 1: User events causes timeout when Jest fake timers are active testing-library/user-event#959
- 2: userEvent not working properly with jest.useFakeTimers testing-library/user-event#1034
- 3: feat: add
advanceTimersoption testing-library/user-event#907 - 4: https://github.com/testing-library/user-event/releases
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.request import urlopen
urls = {
"user-event setup": "https://unpkg.com/@testing-library/user-event@14.6.1/dist/esm/setup/setup.js",
"user-event wait": "https://unpkg.com/@testing-library/user-event@14.6.1/dist/esm/utils/misc/wait.js",
"user-event pointer": "https://unpkg.com/@testing-library/user-event@14.6.1/dist/esm/system/pointer/index.js",
}
for name, url in urls.items():
print(f"--- {name}: {url} ---")
try:
text = urlopen(url, timeout=10).read().decode()
except Exception as exc:
print(f"FETCH_ERROR: {exc}")
continue
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in ("advanceTimers", "delay", "setTimeout", "async pointer", "wait(")):
print(f"{i}: {line}")
PY
printf '%s\n' '--- repository timing invariant ---'
python3 - <<'PY'
# Read-only timing model for the test's relevant sequence.
OPEN_AFTER_MS = 200
manual_user_event_advances = [0, 0] # default user-event delay for hover/unhover
explicit_settle = 400
for real_elapsed in (0, 199, 200, 250):
fake_before_settle = real_elapsed + sum(manual_user_event_advances)
opened_before_settle = fake_before_settle >= OPEN_AFTER_MS
print({
"real_elapsed_between_hover_and_unhover_ms": real_elapsed,
"fake_time_before_settle_ms": fake_before_settle,
"open_timer_fired_before_unhover": opened_before_settle,
"final_fake_time_after_settle_ms": fake_before_settle + explicit_settle,
})
PYRepository: constructorfabric/insight
Length of output: 1632
Remove shouldAdvanceTime from the fake-timer setup.
Real elapsed time can advance the mocked clock past the 200 ms opening timer before unhover. Keep vi.useFakeTimers() and the explicit advanceTimers callback for deterministic control.
🤖 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/frontend/src/components/portal/lens-rail.test.tsx` around lines 55 - 56,
Update the beforeEach fake-timer setup to call vi.useFakeTimers() without
shouldAdvanceTime. Preserve the explicit advanceTimers callback so the 200 ms
opening timer remains under deterministic test control.
| onPointerLeave={() => { | ||
| close(); | ||
| setDismissed(false); | ||
| }} | ||
| // Keyboard gets the labels too, and immediately: a sighted keyboard user | ||
| // was tabbing through eight identical icons with the text at zero | ||
| // opacity, and `title` does not surface on focus in any browser. | ||
| onFocusCapture={() => { | ||
| cancel(); | ||
| setOpen(true); | ||
| }} | ||
| onBlurCapture={(e) => { | ||
| if (!e.currentTarget.contains(e.relatedTarget as Node | null)) { | ||
| close(); | ||
| setDismissed(false); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
onPointerLeave collapses the rail while focus is still inside it.
onFocusCapture opens the rail for keyboard users. onPointerLeave then calls close() unconditionally. If a keyboard user has focus on a zone button and any pointer crosses and leaves the rail, the labels collapse while focus remains inside. The focused button then has a zero-opacity label again.
Guard the close on focus containment, the same way onBlurCapture does.
♿ Proposed fix
onPointerLeave={() => {
- close();
setDismissed(false);
+ // Focus outranks the pointer: a keyboard user's labels must not
+ // vanish because an unrelated pointer crossed the rail.
+ if (e.currentTarget.contains(document.activeElement)) {
+ cancel();
+ return;
+ }
+ close();
}}Change the handler signature to onPointerLeave={(e) => { ... }} for this to compile.
📝 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.
| onPointerLeave={() => { | |
| close(); | |
| setDismissed(false); | |
| }} | |
| // Keyboard gets the labels too, and immediately: a sighted keyboard user | |
| // was tabbing through eight identical icons with the text at zero | |
| // opacity, and `title` does not surface on focus in any browser. | |
| onFocusCapture={() => { | |
| cancel(); | |
| setOpen(true); | |
| }} | |
| onBlurCapture={(e) => { | |
| if (!e.currentTarget.contains(e.relatedTarget as Node | null)) { | |
| close(); | |
| setDismissed(false); | |
| } | |
| }} | |
| onPointerLeave={(e) => { | |
| setDismissed(false); | |
| // Focus outranks the pointer: a keyboard user's labels must not | |
| // vanish because an unrelated pointer crossed the rail. | |
| if (e.currentTarget.contains(document.activeElement)) { | |
| cancel(); | |
| return; | |
| } | |
| close(); | |
| }} | |
| // Keyboard gets the labels too, and immediately: a sighted keyboard user | |
| // was tabbing through eight identical icons with the text at zero | |
| // opacity, and `title` does not surface on focus in any browser. | |
| onFocusCapture={() => { | |
| cancel(); | |
| setOpen(true); | |
| }} | |
| onBlurCapture={(e) => { | |
| if (!e.currentTarget.contains(e.relatedTarget as Node | null)) { | |
| close(); | |
| setDismissed(false); | |
| } | |
| }} |
🤖 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/frontend/src/components/portal/lens-rail.tsx` around lines 156 - 172,
Update the onPointerLeave handler to receive the pointer event and only call
close() and setDismissed(false) when focus is not contained within the current
target, matching the containment guard used by onBlurCapture; preserve the
existing dismissal behavior when the rail has no focused descendant.
| // Anchored to the bar's own edge and grown inwards, rather than | ||
| // centred and clamped. Centring needs to know the readout's | ||
| // width to keep it inside, and it does not: the text varies from | ||
| // "3 messages" to "3.5 hours of 8", so any constant guarding the | ||
| // ends is a guess that the longer strings walk straight past. | ||
| // Growing inwards cannot overflow whatever the text says. | ||
| left: `${(hovered / days.length) * 100}%`, | ||
| transform: | ||
| hovered < days.length / 2 ? undefined : "translateX(-100%)", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Anchor right-side readouts to the bar's right edge.
When hovered >= days.length / 2, Line 335 applies translateX(-100%). Line 333 still uses the hovered bar's left boundary. The readout therefore ends at the left edge instead of starting at the right edge. The last bar is translated from (days.length - 1) / days.length, not from 100%.
Use the right boundary for the translated case:
Proposed fix
- left: `${(hovered / days.length) * 100}%`,
+ left: `${((hovered + (hovered < days.length / 2 ? 0 : 1)) / days.length) * 100}%`,📝 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.
| // Anchored to the bar's own edge and grown inwards, rather than | |
| // centred and clamped. Centring needs to know the readout's | |
| // width to keep it inside, and it does not: the text varies from | |
| // "3 messages" to "3.5 hours of 8", so any constant guarding the | |
| // ends is a guess that the longer strings walk straight past. | |
| // Growing inwards cannot overflow whatever the text says. | |
| left: `${(hovered / days.length) * 100}%`, | |
| transform: | |
| hovered < days.length / 2 ? undefined : "translateX(-100%)", | |
| // Anchored to the bar's own edge and grown inwards, rather than | |
| // centred and clamped. Centring needs to know the readout's | |
| // width to keep it inside, and it does not: the text varies from | |
| // "3 messages" to "3.5 hours of 8", so any constant guarding the | |
| // ends is a guess that the longer strings walk straight past. | |
| // Growing inwards cannot overflow whatever the text says. | |
| left: `${((hovered + (hovered < days.length / 2 ? 0 : 1)) / days.length) * 100}%`, | |
| transform: | |
| hovered < days.length / 2 ? undefined : "translateX(-100%)", |
🤖 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/frontend/src/components/widgets/metric-views/metric-activity.tsx` around
lines 327 - 335, Update the readout positioning logic near the hovered-bar style
so the translated case uses the hovered bar’s right boundary, `(hovered + 1) /
days.length * 100`, while preserving the existing left-boundary position and
no-transform behavior for left-side readouts.
| <span className="text-center"> | ||
| {constantDenominator != null | ||
| ? `measured against ${constantDenominator} per day` | ||
| : silent > 0 | ||
| ? `${silent} ${silent === 1 ? "day" : "days"} with no reading` | ||
| : null} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 'constantDenominator|denominator|metric\.unit|description|per day' \
src/frontend/src || trueRepository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- metric-activity outline ---'
ast-grep outline src/frontend/src/components/widgets/metric-views/metric-activity.tsx || true
printf '%s\n' '--- relevant component sections ---'
sed -n '1,120p' src/frontend/src/components/widgets/metric-views/metric-activity.tsx
sed -n '260,390p' src/frontend/src/components/widgets/metric-views/metric-activity.tsx
printf '%s\n' '--- constantDenominator definitions and uses ---'
rg -n -C 8 'constantDenominator' src/frontend/src src/frontend --glob '*.{ts,tsx}' || true
printf '%s\n' '--- metric activity tests ---'
fd -i 'metric-activity' src/frontend
rg -n -C 6 'Activity|activity|no reading|measured against|per day|denominator' \
src/frontend/src/components src/frontend/src/lib src/frontend/src/queries \
--glob '*.{test,spec}.{ts,tsx}' || trueRepository: constructorfabric/insight
Length of output: 50381
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- metric activity test ---'
cat -n src/frontend/src/components/widgets/metric-views/metric-activity.test.tsx
printf '%s\n' '--- grain implementation and types ---'
cat -n src/frontend/src/lib/insight/day-strip.ts
cat -n src/frontend/src/lib/insight/metric-grain.ts
printf '%s\n' '--- API result and drilldown types ---'
sed -n '70,150p' src/frontend/src/api/metric-results-client.ts
cat -n src/frontend/src/api/metric-drilldown-client.ts | sed -n '1,180p'
printf '%s\n' '--- denominator references outside frontend ---'
python3 - <<'PY'
from pathlib import Path
for p in Path(".").rglob("*"):
if not p.is_file() or any(part in {"node_modules", "dist", "build", ".git"} for part in p.parts):
continue
try:
text = p.read_text(errors="ignore")
except OSError:
continue
hits = [(i, line.strip()) for i, line in enumerate(text.splitlines(), 1)
if "denominator" in line.lower()]
if hits:
print(f"\n{p}")
for i, line in hits[:30]:
print(f"{i}:{line}")
PYRepository: constructorfabric/insight
Length of output: 44503
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- metric design contract ---'
sed -n '205,305p' docs/domain/metrics/specs/DESIGN.md
printf '%s\n' '--- drilldown DTO and presentation ---'
sed -n '145,180p' src/backend/services/analytics/src/domain/metric_drilldown/dto.rs
sed -n '80,155p' src/backend/services/analytics/src/domain/metric_drilldown/presentation.rs
sed -n '180,215p' src/backend/services/analytics/src/domain/metric_drilldown/presentation.rs
printf '%s\n' '--- concrete denominator measures in the metric registry ---'
sed -n '250,310p' src/backend/services/analytics/src/domain/metric_definitions/registry.yaml
sed -n '480,550p' src/backend/services/analytics/src/domain/metric_definitions/registry.yaml
sed -n '640,695p' src/backend/services/analytics/src/domain/metric_definitions/registry.yaml
printf '%s\n' '--- API schema for drilldown denominator ---'
sed -n '880,920p' docs/components/backend/analytics/openapi.jsonRepository: constructorfabric/insight
Length of output: 17148
Use the denominator's declared unit. Denominators represent different measures, such as chat_active_day, total_chat_messages, and pr_created; per day is not universal. Use API-provided denominator metadata or omit the unit until the API supplies it.
🤖 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/frontend/src/components/widgets/metric-views/metric-activity.tsx` around
lines 364 - 369, Update the constantDenominator display in the metric activity
view to use the denominator’s API-provided unit metadata instead of hardcoding
“per day”; support units such as chat_active_day, total_chat_messages, and
pr_created, and omit the unit when metadata is unavailable while preserving the
existing silent-reading fallback.
Source: Coding guidelines
2 participants
Two interaction changes on the person surface. Both came from watching someone use it rather than from reading the code.
Reading a day out of a chart
The value appeared in the caption under the strip. That is a change in the middle of a chart, which is exactly where a reader looking at one bar is not looking: they hover, the number moves somewhere in their periphery, and they never learn it was there.
It now sits over the bar the pointer is on, and the caption stops changing at all — what is down there is constant again.
One positioned element rather than a tooltip per day: a month is thirty-one triggers, and thirty-one floating cards is both heavy and the wrong shape. It sits above the bars, with headroom reserved, so it covers neither the neighbours being compared against nor the header's own comparisons — which are exactly what a reader is hovering in order to reason against.
It anchors to its bar's edge and grows inwards rather than centring. Centring needs the rendered width in order to stay inside the strip, and it does not have it: the text runs from "3 messages" to "3.5 hours of 8", so any constant guarding the ends is a guess that the longer strings walk straight past. Growing inwards cannot overflow whatever the text says.
The zone rail expands on hover
Ported from the lite product's rail. Four things make it work, and each is there because leaving it out broke something.
It opens on a timer, and that is the whole guard against opening over something a pointer was only passing. The first version delayed the fade in CSS instead and let the panel become clickable at once — which achieved the opposite, and is described under "what the first attempt got wrong" below.
The rail keeps its slot in the layout and the labels open OVER what is beside them: widening the element itself shoves the pane sideways every time a pointer crosses the rail on its way elsewhere. The buttons widen only while it is open — a label you can read but not click is a trap, because the pointer leaves the narrow column on its way to the word and the rail shuts before it arrives. People aim at what they can read. The buttons sit inside the hover target, so a pointer resting on one keeps it open rather than fighting what opened it. And a click collapses it until the pointer leaves: a click navigates, the pointer has not moved, and without this the rail reopens on top of the pane the click was aimed at.
Two things could not be copied over. The lite product keeps the post-click flag in
sessionStoragebecause its click reloads the page; here navigation is client-side, so plain state carries it. And the open state is held in React rather than expressed as CSS variants — it is the product of two facts, one of which no selector can see. The variant version silently never matched and nothing said so, which is why the behaviour is now covered by tests rather than by eye.What the first attempt got wrong
The panel takes pointer events while open. With it inert, the gaps between buttons belong to whatever is underneath, so a pointer moving from an icon towards its label crossed bare panel, the rail counted that as having been left, and it slammed shut under the hand reaching for it.
Fixing that introduced the opposite fault, which review caught.
pointer-eventsis not animatable, so it flipped the instant the state did while the opacity waited out its delay: for that window the panel was an invisible, fully clickable band over the pane. Clicks aimed at a row landed on it and vanished, and a pointer merely crossing the rail entered it, counted as still being inside, and got the rail opened over the row it was heading for — the exact thing the delay was written to prevent. Hence the timer: with it, open and visible are the same fact and no such window exists.Three further ways the state could be stranded, all found in review: a keyboard activation set the "stay shut" flag that only a departing pointer could clear, so Enter left the rail dead to the mouse; a width change unmounted the rail under the pointer without a leave event, so it returned already open; and focus never opened it at all, leaving a sighted keyboard user with eight identical icons. Each is covered by a test now, and two of those tests fail on the commit before the fix.
It deliberately does not cover the pane beside it, and what it does not cover is faded rather than dimmed. Dimming was tried first and did nothing: the rows do not object to being darkened, they object to being CUT, and a hard edge through the middle of a word reads as a rendering fault whatever its brightness.
Checks
tsc -bclean, eslint clean, 1081 unit tests pass, including eight covering the rail's open state. Verified in a browser: the click-then-still-hovering case, both ends of the chart strip measured against the strip and the card, and the labels measured against the box that used to clip them.Summary by CodeRabbit