Three reviewer findings: an axis that shrank, a horizon that was short, and focus left on a hidden node - #184
Merged
Conversation
…t, and focus left on a hidden node Three CodeRabbit findings from #155 and #161 that were recorded as follow-ups and not yet done. A fourth from the same batch — the handoff spec's unguarded cleanup — was done as #183 after it turned main red twice on commits that changed no code, which is the argument for clearing the rest of the list now rather than later. ── 1. `yMax` replaced the scale instead of raising its floor ─────────────── BarChart.tsx:77 and LineAreaChart.tsx:68 both read: niceAxis(yMaxProp ?? rawMax, …) while the comment immediately above each says `yMax` "raises the floor of the axis; it never becomes the scale on its own" and "does not become the scale unnoticed". `??` makes both sentences false: a caller passing a yMax BELOW the data maximum replaces the scale with it, and every mark above it renders past `plotH` / above `padTop` — outside the plot area. Now `Math.max(yMaxProp ?? 0, rawMax)`, which is what the comments already described. LATENT, NOT LIVE, and said so plainly: `git grep yMax` finds no caller passing one today. Fixed because the prop is public surface and the next caller would have found it the hard way. ── 2. The forward bucketer's horizon was short by however much of today had already passed ─────────────────────────────────────────────────────── `bucketByWeekForward` documents "Bucket 0 is the next seven days" and computed `start = startOfDay(now)`, so `end = start + weeks*WEEK` sat a fraction of a day BEFORE the horizon it promised. With the suite's own noon clock that is TWELVE HOURS: anything scheduled in that gap failed `t < end` and was dropped from the series entirely. This one is live — `dashboard/page.tsx:189` feeds it the real event trend. And it fails quietly, which is what makes it worth fixing: a bucketer returns a number either way, and an under-count is indistinguishable from a quiet calendar. `start = now.getTime()`. All four pre-existing cases still hold (days(1) and days(2) -> bucket 0, days(9) -> 1, days(20) -> 2, laterToday -> 0), because none of them sat near a boundary — which is exactly why none of them caught it. Two tests added at the boundaries that were unguarded: an event at now+6.75d must be in bucket 0 (the seventh day the doc promises, which the midnight anchor pushed into bucket 1), and an event one hour before the end of the stated horizon must be counted at all. CONTROL: restoring `startOfDay(now)` fails EXACTLY those two and leaves the eleven pre-existing tests passing. ── 3. Escape left focus on the element it had just hidden ────────────────── `SideNav.tsx` moves focus INTO the drawer when it opens (`panel.current?.focus()`) and Escape called `closeDrawer()`, which hides that element — leaving focus on a hidden node, so the next Tab resumed from the top of the document rather than from the control the person was using. Focus now returns to the button that owns the drawer, found by `[aria-controls="app-sidenav"]` — the relationship WAI-ARIA already defines, verified present at ShellHeader.tsx:52 — rather than by an id, so nothing silently stops matching if the markup moves. ── A local artifact worth recording ──────────────────────────────────────── `tsc` initially reported four errors in `src/lib/service-notice/*` after #180 merged. Not a defect on main: the new `ServiceNotice` model needs `prisma generate`, which CI runs and a stale local node_modules had not. Clean after regenerating. Worth knowing before someone reports main as broken. Verified: tsc clean, 4067 passed / 1 pre-existing skip, and re-verified after merging #179's DonutChart changes (charts: 90 passed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
satvikOS has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe changes prevent chart clipping, correct forward weekly forecast boundaries, and restore focus to the mobile navigation trigger after Escape closes the drawer. ChangesChart axis scaling
Forward weekly bucketing
Mobile navigation focus
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The changes correct chart scaling, include the promised forward time horizon, and return focus to the drawer control after Escape; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
satvikOS
pushed a commit
that referenced
this pull request
Aug 23, 2026
Three conflicts, each resolved as a union rather than by taking a side. layout.tsx — #161 landed ShellNavProvider around the header and the side nav; this branch inserts SkipLink at the same point. Both kept. SkipLink is FIRST (its whole feature is being the first Tab stop) and sits outside the provider: it is a bare server-rendered <a> with no drawer state, so nesting it would pull it across the client boundary for nothing. dashboard/page.tsx — two hunks, both unions. · the timeseries import now carries bucketByDay (this branch) AND forwardDelta (#184). · #184 changed the event spark's delta from trendDelta to forwardDelta: eventSpark is bucketed FORWARD, so bucket 0 is the next seven days and reading the LAST two buckets compared the far end of the horizon with itself. That is kept, alongside this branch's move of the activity bucketing to the server. bucketByWeekForward's `now` anchor is untouched. settings/page.tsx — #156 converted every <form action={...}> here to <ReportingForm>, and #167 replaced the "club member" fallback with "—". The conflict was only the email form's opening tag. Resolved to #156's <ReportingForm> with this branch's gap-3, so the three-state radio group ships inside the wrapper that lets a refusal reach the person instead of throwing. One seam the merge creates rather than inherits: readEmailMode refused an unoffered mode by throwing a bare Error. Under #156 that is not a refusal — it is caught as an unexpected fault, replaced with "Something went wrong on our side", and logged as `[admin] action failed`. So the sentence written for the person reached nobody, and a rejected radio value was reported as a server fault. It throws a Refusal now; the existing assertion (toThrow(/Choose one of/)) still holds, because Refusal extends Error. Verified nothing was dropped: the merged tree touches exactly the 19 files this PR declares, and every line it removes relative to main is one of the five defects being fixed — the two-state checkbox write, the old receipt label, the client-side timestamp payload. No ReportingForm reverted to <form>, and "—" is still the no-institution-role fallback. Also checked, and NOT redundant: the monogram fix. #180 reworked InstitutionMark and the brand slots, but initials() lives in ui/Avatar.tsx, main never touched it, and InstitutionMark carries no initials logic at all. Twelve call sites still read it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Three CodeRabbit findings from #155 and #161, recorded as follow-ups and not yet done. A fourth from the same batch — the handoff spec's unguarded cleanup — became #183 after it turned main red twice on commits that changed no code. That is the argument for clearing the rest of the list now rather than later.
1.
yMaxreplaced the scale instead of raising its floorBarChart.tsx:77andLineAreaChart.tsx:68both readniceAxis(yMaxProp ?? rawMax, …)— while the comment immediately above each saysyMax"raises the floor of the axis; it never becomes the scale on its own."??makes both sentences false. A caller passing ayMaxbelow the data maximum replaces the scale with it, and every mark above it renders pastplotH/ abovepadTop— outside the plot area.Now
Math.max(yMaxProp ?? 0, rawMax), which is what the comments already described.Latent, not live —
git grep yMaxfinds no caller passing one today. Fixed because the prop is public surface and the next caller would have found it the hard way.2. The forward bucketer's horizon was short by however much of today had passed
bucketByWeekForwarddocuments "Bucket 0 is the next seven days" and computedstart = startOfDay(now), soend = start + weeks*WEEKsat a fraction of a day before the horizon it promised. With the suite's own noon clock that is twelve hours — anything scheduled in that gap failedt < endand vanished from the series.This one is live:
dashboard/page.tsx:189feeds it the real event trend. And it fails quietly, which is what makes it worth fixing — a bucketer returns a number either way, and an under-count is indistinguishable from a quiet calendar.All four pre-existing cases still hold, because none of them sat near a boundary — which is exactly why none of them caught it. Two boundary tests added: an event at
now+6.75dmust be in bucket 0 (the seventh day the doc promises, which the midnight anchor pushed into bucket 1), and an event one hour before the end of the stated horizon must be counted at all.Control: restoring
startOfDay(now)fails exactly those two, and leaves the eleven pre-existing tests passing.3. Escape left focus on the element it had just hidden
SideNav.tsxmoves focus into the drawer when it opens, and Escape calledcloseDrawer(), which hides that element — leaving focus on a hidden node, so the next Tab resumed from the top of the document rather than from the control the person was using.Focus now returns to the button that owns the drawer via
[aria-controls="app-sidenav"]— the relationship WAI-ARIA already defines, verified present atShellHeader.tsx:52— rather than by an id, so nothing silently stops matching if the markup moves.A local artifact worth recording
tscinitially reported four errors insrc/lib/service-notice/*after #180 merged. Not a defect on main: the newServiceNoticemodel needsprisma generate, which CI runs and a stale localnode_moduleshad not. Clean after regenerating — worth knowing before someone reports main as broken.Verified:
tscclean, 4067 passed / 1 pre-existing skip, re-verified after merging #179's DonutChart changes (charts: 90 passed).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests