Refactor components to eliminate unnecessary useEffect hooks and repl…#9
Merged
Conversation
…ace with render-time state adjustments; introduce useIsMounted hook for SSR-safe mounted state management.
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR systematically replaces ChangesuseEffect to render-time synchronization refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ 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.
Pull request overview
This PR removes temporary ESLint rule overrides added during the Next.js 16 upgrade by refactoring ~20 hook sites: effect-only derived state is moved to render-time guarded updates, mounted flags are replaced with a new useIsMounted() hook based on useSyncExternalStore, and the remaining legitimate external-store sync effects keep narrowly-scoped ESLint disables.
Changes:
- Add
useIsMounted()(SSR/hydration-safe) and migrate mounted-guard patterns to it. - Refactor multiple components to remove “sync derived state in useEffect” patterns (using guarded render-time updates, refs, or memoized derivations).
- Remove global ESLint rule overrides and keep only scoped disables where effects are truly required.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/use-is-mounted.ts | New useSyncExternalStore-based mounted hook for SSR/hydration safety. |
| src/components/welcome-dialog-wrapper.tsx | Opens dialog based on showHelp via guarded render-time update; keeps effect for URL cleanup. |
| src/components/ui/searchable-mission-selector.tsx | Converts retry handlers to use latest-callback refs; scopes effect lint disables for data fetching. |
| src/components/theme-selector.tsx | Replaces mounted state/effect with useIsMounted(). |
| src/components/radar-view-dialog.tsx | Avoids immediate effect-body state set by generating noise pattern on first interval tick. |
| src/components/quest-board-client.tsx | Documents and scopes ESLint disable for intentional external-store synchronization effect. |
| src/components/new-quest-dialog.tsx | Replaces form init and default-deadline effects with guarded render-time updates. |
| src/components/live-time.tsx | Replaces mounted + timezone effects with useIsMounted() + useSyncExternalStore snapshot. |
| src/components/focus-mode/InterruptCapture.tsx | Moves modal-open reset logic to guarded render-time update; effect handles focus + timer with cleanup. |
| src/components/focus-mode/FocusTimer.tsx | Refactors timer resets and guards to avoid effect-only derived state; uses refs for write-only guard. |
| src/components/edit-mission-dialog.tsx | Moves mission->form synchronization to guarded render-time update. |
| src/components/archive/filters/quest-filters.tsx | Moves external reset sync to guarded render-time update. |
| src/components/archive/date-range-selector.tsx | Derives selected preset via memo; changes range->tempRange syncing logic. |
| src/components/archive-mission-dialog.tsx | Moves open/close reset logic to guarded render-time update. |
| eslint.config.mjs | Removes global disables for react-hooks/set-state-in-effect and react-hooks/immutability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+60
to
+61
| const loadMissionsRef = useRef<() => void>(undefined); | ||
| const performSearchRef = useRef<(query: string) => void>(undefined); |
Comment on lines
+104
to
108
| // Sync tempRange when the incoming range changes (render-time). | ||
| const [prevRange, setPrevRange] = useState(range); | ||
| if (range !== prevRange) { | ||
| setPrevRange(range); | ||
| setTempRange(range); |
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
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.
Removes eslint rule overrides added during the Next.js 16 upgrade and fixes the ~20 flagged sites: effects that only synced derived state are converted to render-time derived-state guards (prevX pattern), mounted flags replaced with useSyncExternalStore via a new useIsMounted hook, write-only guard flags converted to refs, and two genuine external-store sync effects kept with scoped eslint-disable comments.
Summary by CodeRabbit
Refactor
Chores