fix(web): localize dates, harden ExportModal, clear dead code + cover untested logic#180
Merged
Conversation
… untested logic - date/time now render in the viewer's locale + timezone (DateTime component); fixes US-format/UTC frozen into SSR HTML - ExportModal: Escape + backdrop-click close, role=dialog + focus return, defer blob URL revoke (Firefox download cancel) - drop unused export (RecoverySummary) and unused prop (filter-bar value) - add tests: feed clear-all/error-retry paths, getBackgroundForPath routing (clears CRAP 272 + CRAP 90 measurement gaps)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughIntroduces a shared ChangesDateTime, ExportModal a11y, and cleanups
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/components/feed/filter-bar.tsx (1)
25-25: ⚡ Quick win
valueis still part ofContentTypeTab’s prop contract.Line 25 still types props with
ContentTypeTabData, sovalueremains in the component surface (and is still passed via spread later). If the goal is to remove the unused prop, narrow the child props to onlylabel/countplus control props.Proposed refactor
interface ContentTypeTabData { label: string; value: string; count: number; } -function ContentTypeTab({ label, count, active, onClick }: ContentTypeTabData & { active: boolean; onClick: () => void }) { +type ContentTypeTabProps = Pick<ContentTypeTabData, "label" | "count"> & { + active: boolean; + onClick: () => void; +}; + +function ContentTypeTab({ label, count, active, onClick }: ContentTypeTabProps) {{tabs.map((tab) => ( <ContentTypeTab key={tab.value} - {...tab} + label={tab.label} + count={tab.count} active={ctFilter === tab.value} onClick={() => setCtFilter(tab.value)} /> ))}🤖 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 `@web/components/feed/filter-bar.tsx` at line 25, The ContentTypeTab function is still typed with ContentTypeTabData in its prop contract, which includes the unused value property. Remove ContentTypeTabData from the type annotation and explicitly define only the props that ContentTypeTab actually uses: label, count, active, and onClick. Replace the current prop type on the ContentTypeTab function signature to only include these specific properties without spreading ContentTypeTabData.
🤖 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 `@web/components/date-time.tsx`:
- Around line 8-13: The issue is that the component initializes state with a
locale-formatted string which can cause hydration mismatch issues in React 18 if
the server and client locales differ. When suppressHydrationWarning is used, the
recovery re-render is skipped. Fix this by initializing the text state in the
useState hook with the raw iso string value instead of new
Date(iso).toLocaleString(), so that the useEffect will always trigger a state
update with the client locale version, ensuring the mismatch is corrected. Keep
the useEffect the same so it continues to format the date string with the client
locale.
In `@web/components/ExportModal.tsx`:
- Around line 46-58: Implement a focus trap within the modal dialog to prevent
keyboard users from tabbing to background controls. In the useEffect hook where
you handle Escape key behavior and focus management with closeButtonRef, add a
keydown event listener that intercepts Tab key presses. When Tab is pressed
while focus is on the last focusable element in the modal, redirect focus to the
first focusable element, and when Shift+Tab is pressed on the first focusable
element, redirect focus to the last. This ensures focus cycles within the modal
and maintains proper accessibility semantics.
---
Nitpick comments:
In `@web/components/feed/filter-bar.tsx`:
- Line 25: The ContentTypeTab function is still typed with ContentTypeTabData in
its prop contract, which includes the unused value property. Remove
ContentTypeTabData from the type annotation and explicitly define only the props
that ContentTypeTab actually uses: label, count, active, and onClick. Replace
the current prop type on the ContentTypeTab function signature to only include
these specific properties without spreading ContentTypeTabData.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e644b86d-e492-425b-adb0-17ec8f08ce15
📒 Files selected for processing (9)
web/app/(dashboard)/jobs/[id]/page.test.tsxweb/app/(dashboard)/page.test.tsxweb/components/ExportModal.tsxweb/components/date-time.tsxweb/components/feed/filter-bar.tsxweb/components/feed/preview-card.tsxweb/components/job-card.tsxweb/components/page-background.test.tsxweb/lib/hooks/useRecovery.ts
…p iteration 1) - separate mount-only focus capture/restore from the Escape listener so an inline onClose identity change can't round-trip focus on parent re-render (P1) - trap Tab/Shift+Tab within the dialog per the APG pattern (P2)
…ly applies (greploop iteration 2) suppressHydrationWarning skips React's recovery re-render, so initializing state with the client-locale value made the effect a no-op (same value → bail out) and the server format stuck. Init with a deterministic UTC string instead; the effect's locale value now differs and re-renders. Caught by CodeRabbit.
Owner
Author
|
@greptile review |
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
1 participant
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.
What
Web dashboard quality pass — three related fixes plus test coverage.
Date/time localization
DateTimecomponent renders timestamps in the viewer's locale + timezone. PreviouslytoLocaleString()ran during SSR (server = en-US/UTC) and froze US format into the HTML; now it reformats on mount. Applied tojob-cardandpreview-card(every date in the app).ExportModal hardening (a11y)
role="dialog"+aria-modal+ focus moves in on open and returns to the trigger on close.URL.revokeObjectURLso Firefox doesn't cancel the download.Dead code + coverage
RecoverySummaryexport and the unusedvalueprop inContentTypeTab(fallow dead-code gate).clearAll/ error-retry feed paths andgetBackgroundForPathrouting (clears CRAP 272 + CRAP 90 measurement gaps).jobs/[id]/page.test.tsxfixture (missingsummary/transcript/key_phrases) that leftmainred ontsc.Verification
tsc --noEmitclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
This PR delivers a focused quality pass across three areas: SSR-safe date localization, ExportModal accessibility hardening, and dead-code removal with new test coverage.
en-US/UTCstring so server and client hydration match, then reformats to the viewer's locale on mount. Applied to bothjob-cardandpreview-card.role="dialog"+aria-modal, separates the focus-capture effect (mount-only) from the Escape-key listener (onClosedep), implements a full Tab trap viatrapTab, and defersURL.revokeObjectURLto fix the Firefox download cancellation.RecoverySummaryinterface, removes the unusedvalueprop fromContentTypeTab, adds tests forclearAll/error-retry feed paths andgetBackgroundForPathrouting, and fixes thetsc-breaking fixture in the jobs detail test.Confidence Score: 5/5
All changes are additive accessibility and correctness fixes; no data paths, API contracts, or authentication boundaries are touched.
The two prior review findings (focus round-trip on parent re-render, missing Tab trap) are both resolved cleanly. The DateTime localization approach is correct: deterministic initial state prevents hydration mismatch, the effect always differs for non-en-US/non-UTC viewers and triggers the re-render. Dead-code removal and test additions are low-risk. No logic regressions are visible across the changed files.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Server as SSR Next.js participant React as React Hydration participant Effect as useEffect client participant DOM as time element Server->>DOM: "render en-US/UTC string" React->>DOM: "hydrate — state matches, no warning" Effect->>DOM: "toLocaleString() — viewer locale+tz" DOM-->>Effect: "displays locale-correct timestamp"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Server as SSR Next.js participant React as React Hydration participant Effect as useEffect client participant DOM as time element Server->>DOM: "render en-US/UTC string" React->>DOM: "hydrate — state matches, no warning" Effect->>DOM: "toLocaleString() — viewer locale+tz" DOM-->>Effect: "displays locale-correct timestamp"Reviews (4): Last reviewed commit: "fix(web/DateTime): deterministic UTC SSR..." | Re-trigger Greptile