feat: new hook useModalFocus - #186
Merged
Merged
Conversation
…ents. - Introduced `sync-ai-instructions` skill with a script to copy AI instruction files from `.github/instructions/` to `.agents/rules/`. - Added `sync-ai-skills` skill to copy local repository skills from `.github/skills` to AI skill target directories. - Created scripts for both skills to handle file copying and verification. - Updated `.gitignore` to exclude `.agents` and `.agent` directories. - Added a new hook `useModalFocus` for managing focus within modal components. - Refactored modal components to utilize the new focus management hook. - Updated tests for modal components to ensure focus behavior is correct. - Removed deprecated `sync-codex-skills` skill and its associated files. - Updated `biome.json` schema version.
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
Hidden review stack artifact:WalkthroughThis PR centralizes modal focus/keyboard handling into a shared ChangesModal Focus Refactoring
Skill Scripts Infrastructure
Localization and Date Formatting
Configuration Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
components/DeviationPill.tsx (1)
4-4: ⚡ Quick winUse
useFormatter()instead ofuseLocale()+ baretoLocaleDateStringto prevent hydration mismatches.
Date formatting in a user's locale which doesn't match the serveris a documented cause of Next.js hydration errors.useFormatter()from next-intl is specifically designed to avoid hydration mismatches by ensuring thatlocale,timeZone, andnoware shared across the entire app — server and client alike. Using rawtoLocaleDateString(locale)bypasses that infrastructure.
VersionHistory.tsxin this same PR already uses the correct pattern. AlignDeviationPill.tsxto match:♻️ Proposed fix
-import { useLocale, useTranslations } from 'next-intl' +import { useFormatter, useTranslations } from 'next-intl'- const locale = useLocale() + const format = useFormatter()- <span> - {new Date(deviation.createdAt).toLocaleDateString(locale)} - </span> + <span> + {format.dateTime(new Date(deviation.createdAt), { dateStyle: 'short' })} + </span>- <span> - {new Date(deviation.decidedAt).toLocaleDateString(locale)} - </span> + <span> + {format.dateTime(new Date(deviation.decidedAt), { dateStyle: 'short' })} + </span>Also applies to: 31-31, 100-103, 129-132
🤖 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 `@components/DeviationPill.tsx` at line 4, The component DeviationPill currently imports and uses useLocale and calls toLocaleDateString(locale), which can cause hydration mismatches; replace useLocale with next-intl's useFormatter() and use its formatDate (e.g., const { formatDate } = useFormatter()) to format dates consistently across server and client, updating every place in DeviationPill that calls date.toLocaleDateString(locale) (and similar bare toLocale* usages) to call formatDate(date, options) instead; adjust the imports (remove useLocale, import useFormatter) and ensure the values passed are Date objects or ISO strings acceptable to formatDate so the formatting matches VersionHistory.tsx's pattern.
🤖 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 @.github/skills/sync-ai-skills/scripts/sync_ai_skills.sh:
- Line 86: The parameter expansion ${source_file#$source_skill_dir/} can treat
$source_skill_dir as a glob; update the pattern operand to quote the inner
variable so the shell treats the directory literally (i.e., modify the expansion
that sets relative_path to use a quoted $source_skill_dir inside the removal
pattern), keeping the assignment to relative_path and using the same symbols
source_file and source_skill_dir.
In `@hooks/useModalFocus.ts`:
- Around line 50-63: The effect in useModalFocus schedules focus via
requestAnimationFrame but never stores or cancels the RAF handle, so a pending
callback can run after the modal begins exit and steal focus; fix by storing the
requestAnimationFrame id (e.g., rafIdRef) when calling requestAnimationFrame in
the open branch and cancel it in the cleanup (and on unmount) using
cancelAnimationFrame; update the useEffect so that initialFocusRef scheduling
assigns the id and the returned cleanup always cancels rafIdRef.current before
restoring previousFocusRef.current.
- Around line 25-26: The FOCUSABLE selector constant is including disabled
controls and missing native links, causing the focus-trap in useModalFocus to
treat disabled buttons as tabbable; update the FOCUSABLE string used by the hook
(FOCUSABLE) to include a[href] and to exclude disabled elements (e.g., 'a[href],
input:not([disabled]), textarea:not([disabled]), button:not([disabled]),
select:not([disabled]), [tabindex]:not([tabindex="-1"]):not([disabled])') so
querySelectorAll returns only keyboard-reachable elements and the focus-wrap
logic (used by the hook's focus handling) correctly detects first/last enabled
elements.
In `@tests/unit/use-modal-focus.test.tsx`:
- Around line 62-68: The test file stubs globals in beforeEach
(vi.stubGlobal('requestAnimationFrame', ...) and
vi.stubGlobal('cancelAnimationFrame', ...)) but never restores them; add an
afterEach that unstubs those globals (either
vi.unstubGlobal('requestAnimationFrame') and
vi.unstubGlobal('cancelAnimationFrame') or vi.unstubAllGlobals()) so the
synchronous RAF stub does not leak into other tests; place the afterEach
alongside the existing beforeEach to ensure restoration after each test.
In `@tests/unit/version-history.test.tsx`:
- Around line 96-98: The three hardcoded expects
(expect(screen.getByText('3/3/26')) etc.) are timezone-sensitive; instead
compute the expected strings from the same Date objects used in the test by
creating new Date('2026-03-03'), new Date('2026-03-02'), new Date('2026-03-01')
and calling toLocaleDateString('en', { dateStyle: 'short' }) and assert those
computed strings with getByText; apply the same dynamic derivation pattern to
the other date assertions mentioned (lines 360–361) so all date expectations use
the same locale/dateStyle formatting as the component under test.
---
Nitpick comments:
In `@components/DeviationPill.tsx`:
- Line 4: The component DeviationPill currently imports and uses useLocale and
calls toLocaleDateString(locale), which can cause hydration mismatches; replace
useLocale with next-intl's useFormatter() and use its formatDate (e.g., const {
formatDate } = useFormatter()) to format dates consistently across server and
client, updating every place in DeviationPill that calls
date.toLocaleDateString(locale) (and similar bare toLocale* usages) to call
formatDate(date, options) instead; adjust the imports (remove useLocale, import
useFormatter) and ensure the values passed are Date objects or ISO strings
acceptable to formatDate so the formatting matches VersionHistory.tsx's pattern.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f54b3984-0ac5-422b-adda-4456620f17cd
📒 Files selected for processing (21)
.github/skills/sync-ai-instructions/SKILL.md.github/skills/sync-ai-instructions/agents/openai.yaml.github/skills/sync-ai-instructions/scripts/sync_ai_instructions.sh.github/skills/sync-ai-skills/SKILL.md.github/skills/sync-ai-skills/agents/openai.yaml.github/skills/sync-ai-skills/scripts/sync_ai_skills.sh.github/skills/sync-codex-skills/SKILL.md.github/skills/sync-codex-skills/agents/openai.yaml.gitignorebiome.jsoncomponents/ConfirmModal.tsxcomponents/DeviationDecisionModal.tsxcomponents/DeviationFormModal.tsxcomponents/DeviationPill.tsxcomponents/SuggestionFormModal.tsxcomponents/SuggestionResolutionModal.tsxcomponents/VersionHistory.tsxhooks/useModalFocus.tstests/unit/confirm-modal.test.tsxtests/unit/use-modal-focus.test.tsxtests/unit/version-history.test.tsx
💤 Files with no reviewable changes (2)
- .github/skills/sync-codex-skills/agents/openai.yaml
- .github/skills/sync-codex-skills/SKILL.md
…ng in DeviationPill
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
==========================================
+ Coverage 60.38% 60.48% +0.09%
==========================================
Files 293 293
Lines 17294 17239 -55
Branches 6653 6635 -18
==========================================
- Hits 10443 10427 -16
+ Misses 6710 6671 -39
Partials 141 141
🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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.
Description
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is