fix: prevent infinite loop in NotificationPreferences locale updates #107
Conversation
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
- Change useEffect dependency from defaultPreferences to i18n.locale - Prevents potential infinite loop from frequent _ function reference changes - Add i18n to useLingui destructuring to access locale - Add 2 new tests to verify locale change behavior and render count - Update comments to explain why locale dependency prevents infinite loop Fixes #103 Tests: - All 134 tests passing (2 new tests added for translation updates) - TypeScript: 0 errors - ESLint: 0 warnings - Prettier: compliant - REUSE: compliant (3.3)
💡 Tip: Consider Using Draft PRsBenefits of opening PRs as drafts initially:
How to convert:
This is just a friendly reminder - feel free to continue as is! 😊 |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a potential infinite loop issue in the NotificationPreferences component by changing the useEffect dependency from relying on defaultPreferences (which depends on the frequently-changing _ function reference) to i18n.locale (which only changes when the locale actually changes).
- Extracts
i18nfromuseLingui()hook to access the stablelocaleproperty - Updates useEffect dependencies to use
i18n.localeinstead of relying solely ondefaultPreferences - Adds test coverage for translation updates and render behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/NotificationPreferences.tsx | Adds i18n extraction, updates useEffect dependencies to include i18n.locale, and adds explanatory comments |
| src/components/NotificationPreferences.test.tsx | Adds test suite for translation updates to verify no excessive re-renders and state preservation |
Addresses Copilot review comment - removed defaultPreferences from dependency array and compute translations directly using i18n._ and msg to avoid dependency on the _ function which changes frequently. This fully prevents the infinite loop issue by only depending on the stable i18n object.
…e dependency Addresses Copilot review comments: - Extract getTranslationsForCategory helper to avoid duplicating translation strings - Use i18n.locale instead of i18n object in useEffect dependency - Simplify useEffect logic using helper function
- Update comment to reflect current implementation (no longer using _ function) - Remove redundant i18n from dependency array, only keep i18n.locale - Add eslint-disable comment with explanation for intentional dependency choice
💡 Tip: Consider Using Draft PRsBenefits of opening PRs as drafts initially:
How to convert:
This is just a friendly reminder - feel free to continue as is! 😊 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🐛 Bug Fix
Fixes #103
Problem
The
NotificationPreferencescomponent had a potential infinite loop risk due touseEffectdepending ondefaultPreferences, which depends on the Lingui_function. Since_function reference can change frequently during re-renders, this could trigger excessive re-renders.Solution
Changed
useEffectdependency fromdefaultPreferencestoi18n.locale. This ensures translations only update when the locale actually changes, not on every_function reference change.Changes
i18ntouseLingui()destructuring to accesslocaleuseEffectdependency array:[i18n.locale, defaultPreferences]Testing
Self-Review Checklist
Phase 1: Functional ✅
Phase 2: Pattern Review ✅
Phase 3: Cleanup ✅
Phase 4: Consistency ✅
Related