fix: resolve architecture rule violations from AGENTS.md#12
Conversation
📝 WalkthroughWalkthroughThree IPC listener registration methods in the Electron preload ( ChangesIPC Listener Unsubscribe
Zustand Selector Migration and Timer Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@electron/preload.ts`:
- Around line 25-33: The preload.ts file correctly implements the disposer
pattern by having onTriggerNewNote and onTriggerTasks return cleanup functions
that call ipcRenderer.removeListener. However, this unsubscribe contract is only
effective if callers actually invoke the returned disposer functions. While the
implementation in this file is correct, ensure the disposer pattern is being
properly consumed by verifying that callers in useGlobalHotkey.ts are capturing
the return values from onTriggerNewNote and onTriggerTasks and invoking them in
useEffect cleanup functions to prevent listener accumulation.
In `@src/components/RemindersPage.tsx`:
- Around line 71-79: The recursive scheduling in the scheduleNext function can
create dangling timers after unmount if the callback fires while teardown is in
progress. Add a boolean flag initialized to true (like isMounted) to track
component mount state, set it to false in the cleanup function return statement,
and check this flag at the beginning of the setTimeout callback before calling
scheduleNext() again. This prevents scheduling new timeouts after the component
has unmounted.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffa96f59-e8ea-4897-b794-da396326de98
📒 Files selected for processing (9)
electron/preload.tssrc/App.tsxsrc/components/MainActionMenu.tsxsrc/components/NoteSearch.tsxsrc/components/NoteTitleBar.tsxsrc/components/RemindersPage.tsxsrc/hooks/useGlobalHotkey.tssrc/hooks/useNoteStorage.tssrc/types.d.ts
| onTriggerNewNote: (callback: () => void) => { | ||
| ipcRenderer.on('trigger-new-note', () => callback()) | ||
| const handler = () => callback() | ||
| ipcRenderer.on('trigger-new-note', handler) | ||
| return () => ipcRenderer.removeListener('trigger-new-note', handler) | ||
| }, | ||
| onTriggerTasks: (callback: () => void) => { | ||
| ipcRenderer.on('trigger-tasks', () => callback()) | ||
| const handler = () => callback() | ||
| ipcRenderer.on('trigger-tasks', handler) | ||
| return () => ipcRenderer.removeListener('trigger-tasks', handler) |
There was a problem hiding this comment.
Unsubscribe contract is implemented but not consumed by current renderer callers.
onTriggerNewNote/onTriggerTasks now return disposers, but src/hooks/useGlobalHotkey.ts (Lines 124-138 in provided context) does not capture or invoke them in useEffect cleanup, so listeners can still accumulate.
Suggested caller-side fix (outside this file)
// src/hooks/useGlobalHotkey.ts (inside the relevant useEffect)
+ let offNewNote: (() => void) | undefined
+ let offTasks: (() => void) | undefined
if (window.electronAPI.onTriggerNewNote) {
- window.electronAPI.onTriggerNewNote(() => {
+ offNewNote = window.electronAPI.onTriggerNewNote(() => {
// ...
})
}
if (window.electronAPI.onTriggerTasks) {
- window.electronAPI.onTriggerTasks(() => {
+ offTasks = window.electronAPI.onTriggerTasks(() => {
// ...
})
}
+ return () => {
+ offNewNote?.()
+ offTasks?.()
+ }As per coding guidelines, trace context across related files to ensure cross-file contracts are actually enforced at usage sites.
🤖 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 `@electron/preload.ts` around lines 25 - 33, The preload.ts file correctly
implements the disposer pattern by having onTriggerNewNote and onTriggerTasks
return cleanup functions that call ipcRenderer.removeListener. However, this
unsubscribe contract is only effective if callers actually invoke the returned
disposer functions. While the implementation in this file is correct, ensure the
disposer pattern is being properly consumed by verifying that callers in
useGlobalHotkey.ts are capturing the return values from onTriggerNewNote and
onTriggerTasks and invoking them in useEffect cleanup functions to prevent
listener accumulation.
| let timeoutId: ReturnType<typeof setTimeout> | ||
| const scheduleNext = () => { | ||
| timeoutId = setTimeout(() => { | ||
| setNow(Date.now()) | ||
| scheduleNext() | ||
| }, 10000) // update every 10s for better responsiveness | ||
| } | ||
| scheduleNext() | ||
| return () => clearTimeout(timeoutId) |
There was a problem hiding this comment.
Prevent post-unmount re-scheduling in the recursive timer.
Line 73 can still schedule the next timeout after unmount if teardown happens while the callback is in flight, which leaves a dangling timer. Add a cancellation flag and nullable timeout guard in cleanup.
Suggested patch
React.useEffect(() => {
- let timeoutId: ReturnType<typeof setTimeout>
+ let timeoutId: ReturnType<typeof setTimeout> | null = null
+ let cancelled = false
const scheduleNext = () => {
+ if (cancelled) return
timeoutId = setTimeout(() => {
+ if (cancelled) return
setNow(Date.now())
scheduleNext()
}, 10000) // update every 10s for better responsiveness
}
scheduleNext()
- return () => clearTimeout(timeoutId)
+ return () => {
+ cancelled = true
+ if (timeoutId) clearTimeout(timeoutId)
+ }
}, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let timeoutId: ReturnType<typeof setTimeout> | |
| const scheduleNext = () => { | |
| timeoutId = setTimeout(() => { | |
| setNow(Date.now()) | |
| scheduleNext() | |
| }, 10000) // update every 10s for better responsiveness | |
| } | |
| scheduleNext() | |
| return () => clearTimeout(timeoutId) | |
| let timeoutId: ReturnType<typeof setTimeout> | null = null | |
| let cancelled = false | |
| const scheduleNext = () => { | |
| if (cancelled) return | |
| timeoutId = setTimeout(() => { | |
| if (cancelled) return | |
| setNow(Date.now()) | |
| scheduleNext() | |
| }, 10000) // update every 10s for better responsiveness | |
| } | |
| scheduleNext() | |
| return () => { | |
| cancelled = true | |
| if (timeoutId) clearTimeout(timeoutId) | |
| } |
🤖 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 `@src/components/RemindersPage.tsx` around lines 71 - 79, The recursive
scheduling in the scheduleNext function can create dangling timers after unmount
if the callback fires while teardown is in progress. Add a boolean flag
initialized to true (like isMounted) to track component mount state, set it to
false in the cleanup function return statement, and check this flag at the
beginning of the setTimeout callback before calling scheduleNext() again. This
prevents scheduling new timeouts after the component has unmounted.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
1 participant
This PR resolves several architectural rule violations identified in
AGENTS.md.Fixes:
useAppStore()whole-store subscriptions into optimized slice subscriptions to prevent unnecessary re-renders across the React tree.preload.tsevent handlers (onSwipeGesture,onTriggerNewNote,onTriggerTasks) to return an unsubscribe cleanup function, preventing memory leaks and conforming to IPC isolation standards. Also updatedsrc/types.d.tsappropriately.setIntervalpolling inRemindersPage.tsxwith a recursivesetTimeoutchain to comply with background execution/efficiency guidelines.Performance Reporting
Vite build output post-changes:
Summary by CodeRabbit
New Features
Performance Improvements