refactor: code quality cleanup — dead code, boilerplate, types, constants, AI comments#77
Merged
Merged
Conversation
…ants, AI comments - Remove dead code: resumeTimer (no-op), onSwipeGesture (ignores callback), themePreset from useAppStore (duplicated), prevNotesRef (unused) - Consolidate boilerplate: booleanSetter helper in useAppStore, onEvent helper in api.ts, data-driven KeybindsModal, removed 11 redundant setters - Fix Rust: clippy needless_borrows_for_generic_args, unexpected_cfgs lint - Improve types: GraphControls interface, typed openAIChat response - Extract ~25 magic numbers to named constants - Remove ~15 pedagogical AI-style comments
Contributor
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR replaces inline magic numbers with named constants across Rust ( ChangesRefactor and Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useReminders.ts (1)
44-50: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake reminder scheduling last-write-wins before calling Electron.
Line 49 checks the token only after Line 47 has already sent
pendingtoscheduleReminders, so an older in-flight call can still overwrite newer reminder schedules when it resolves later.Possible fix: serialize scheduling and skip superseded payloads
let scheduleToken = 0 +let scheduleQueue: Promise<void> = Promise.resolve()const pending = collectFutureReminders(notes) const token = ++scheduleToken - window.electronAPI - .scheduleReminders(pending) - .then(() => { - if (token !== scheduleToken) return - }) - // eslint-disable-next-line no-console - .catch((e) => console.error('Failed to schedule reminders', e)) + scheduleQueue = scheduleQueue + .catch(() => undefined) + .then(async () => { + if (token !== scheduleToken) return + await window.electronAPI.scheduleReminders(pending) + }) + // eslint-disable-next-line no-console + .catch((e) => { + if (token === scheduleToken) console.error('Failed to schedule reminders', e) + }) }, [notes])🤖 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/hooks/useReminders.ts` around lines 44 - 50, In useReminders, the token check currently happens only after window.electronAPI.scheduleReminders(pending) has already been invoked, so stale payloads can still be sent and later overwrite newer schedules. Move the last-write-wins guard so it skips superseded pending data before calling scheduleReminders, and ensure only the latest scheduling request is allowed to reach Electron. Use the existing scheduleToken logic in useReminders to serialize or gate requests so older in-flight updates cannot apply after a newer one.src/App.tsx (1)
123-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
searchInputRefis never attached, so this focus path is a no-op.
NoteSearchis rendered without a forwarded ref here, sosearchInputRef.currentstaysnulland this branch can never focus the search box. Either pass the ref intoNoteSearchor remove this effect if the child owns focus now.🤖 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/App.tsx` around lines 123 - 127, The focus effect in App.tsx is a no-op because searchInputRef is never connected to the rendered NoteSearch input. Either forward and attach searchInputRef through NoteSearch so the useEffect can focus the actual search box, or remove the App-level focus logic if NoteSearch now manages its own focus. Use the existing useEffect, searchInputRef, and NoteSearch symbols to wire the ref correctly or delete the dead path.
🧹 Nitpick comments (4)
src/components/KeybindsModal.tsx (1)
123-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the global shortcut action/key mapping into
ShortcutConfig.This loop is still hardcoded to exactly two global shortcuts. Adding another
section: 'global'entry will silently reuse the'new-note'action/path.♻️ Proposed refactor
interface ShortcutConfig { key: string label: string storageKey: string defaultKey: string section: 'global' | 'app' + globalAction?: 'toggle' | 'new-note' + legacyStorageKey?: string } const shortcuts: ShortcutConfig[] = [ { key: 'shortcutToggle', label: 'Toggle App Visibility', storageKey: SETTINGS_KEYS.SHORTCUT_TOGGLE, defaultKey: `${defaultMod}+Shift+C`, section: 'global', + globalAction: 'toggle', + legacyStorageKey: SETTINGS_KEYS.SHORTCUT_TOGGLE, }, { key: 'shortcutNewNote', label: 'New Note (Global)', storageKey: SETTINGS_KEYS.SHORTCUT_NEWNOTE, defaultKey: `${defaultMod}+Shift+N`, section: 'global', + globalAction: 'new-note', + legacyStorageKey: SETTINGS_KEYS.SHORTCUT_NEWNOTE, }, const handleSave = () => { for (const sc of shortcuts) { - if (sc.section === 'global') { - const oldShortcutKey = - sc.key === 'shortcutToggle' ? 'papercache-shortcut-toggle' : 'papercache-shortcut-newnote' - const oldShortcut = localStorage.getItem(oldShortcutKey) || sc.defaultKey - const action = sc.key === 'shortcutToggle' ? 'toggle' : 'new-note' + if (sc.section === 'global' && sc.globalAction) { + const oldShortcut = + localStorage.getItem(sc.legacyStorageKey ?? sc.storageKey) || sc.defaultKey if (window.electronAPI.updateGlobalShortcut) { - window.electronAPI.updateGlobalShortcut(action, oldShortcut, values[sc.key]) + window.electronAPI.updateGlobalShortcut(sc.globalAction, oldShortcut, values[sc.key]) } - localStorage.setItem(oldShortcutKey, values[sc.key]) + if (sc.legacyStorageKey) { + localStorage.setItem(sc.legacyStorageKey, values[sc.key]) + } } localStorage.setItem(sc.storageKey, values[sc.key]) }🤖 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/KeybindsModal.tsx` around lines 123 - 130, The global shortcut loop in KeybindsModal.tsx is still hardcoded to two cases, so add the action and storage key mapping to ShortcutConfig and use that metadata here instead of branching on sc.key. Update the code that builds oldShortcutKey and action inside the shortcuts loop to read from each ShortcutConfig entry, so any new section: 'global' shortcut can be handled without silently falling back to 'new-note'. Keep the updateGlobalShortcut call using the resolved action, stored shortcut value, and values[sc.key] from the same config-driven source.src-tauri/Cargo.toml (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the
unexpected_cfgssuppression more narrowly.Line 37 disables typo checking for every
#[cfg]in this crate, not just the legacyobjcmacro case called out in the PR notes. Prefer whitelisting the specific cfgs or localizing the allow so real platform-gating mistakes still surface.🤖 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-tauri/Cargo.toml` around lines 36 - 37, The current lints.rust setting for unexpected_cfgs is too broad and suppresses cfg typo checks across the entire crate. Narrow the suppression by updating the Cargo.toml lint configuration to only whitelist the specific legacy objc-related cfg case or move the allow closer to the affected code path, using the existing lints.rust section as the place to adjust. Keep real #[cfg] validation enabled everywhere else so platform-gating mistakes still surface.src/api.ts (1)
58-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExpose these bridge methods as
Promise<void>.src/types.d.tsand the test mock still declarepauseShortcuts/resumeShortcutsasvoid, so callers can’t await theinvoke(...)result or handle failures. Update the contract and mocks, and drop the cast insrc/api.ts.🤖 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/api.ts` around lines 58 - 59, The bridge methods pauseShortcuts and resumeShortcuts are typed inconsistently with their async invoke behavior, so update the contract to return Promise<void> instead of void. Change the declarations in src/types.d.ts and the test mock to match, then remove the unnecessary cast in src/api.ts so the functions directly return the invoke(...) Promise. Ensure the API surface and mock names stay aligned for pauseShortcuts and resumeShortcuts.src/components/TimersPage.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
TICK_INTERVAL_MSfor the initial schedule too.The follow-up tick now uses the constant, but the first
setTimeoutin this loop still hard-codes250. That leaves the countdown with split sources of truth.♻️ Proposed fix
- timeoutRef.current = setTimeout(tick, 250) + timeoutRef.current = setTimeout(tick, TICK_INTERVAL_MS)Also applies to: 40-40
🤖 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/TimersPage.tsx` at line 1, The countdown loop in TimersPage has split sources of truth because the first schedule still hard-codes 250 while the follow-up uses TICK_INTERVAL_MS. Update the initial setTimeout in the timer loop to use TICK_INTERVAL_MS as well, and keep the change aligned with the timer scheduling logic in TimersPage so both the first and subsequent ticks share the same constant.
🤖 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 `@src/api.ts`:
- Around line 5-14: The onEvent helper in src/api.ts can leak a listener if
cleanup happens before listen(...).then(...) assigns unlisten. Update onEvent to
track a disposed flag alongside unlisten, so the returned cleanup marks the
callback as disposed and, when the listen promise resolves, immediately invokes
the resolved unlisten if disposal already happened; keep the fix centered on
onEvent and its listen/unlisten flow.
In `@src/lib/editor/extensions.ts`:
- Around line 174-178: The completion handling in window.electronAPI.openAIChat
assumes any non-empty choices array is valid, but the new response shape allows
message.content to be missing. Update the response validation in the editor
extension so it checks the selected choice’s message.content actually exists and
is non-empty before treating the call as success; otherwise fall back to the
error path or an explicit empty-state handling. Use the openAIChat call site and
the completion/choices handling logic to locate the fix.
---
Outside diff comments:
In `@src/App.tsx`:
- Around line 123-127: The focus effect in App.tsx is a no-op because
searchInputRef is never connected to the rendered NoteSearch input. Either
forward and attach searchInputRef through NoteSearch so the useEffect can focus
the actual search box, or remove the App-level focus logic if NoteSearch now
manages its own focus. Use the existing useEffect, searchInputRef, and
NoteSearch symbols to wire the ref correctly or delete the dead path.
In `@src/hooks/useReminders.ts`:
- Around line 44-50: In useReminders, the token check currently happens only
after window.electronAPI.scheduleReminders(pending) has already been invoked, so
stale payloads can still be sent and later overwrite newer schedules. Move the
last-write-wins guard so it skips superseded pending data before calling
scheduleReminders, and ensure only the latest scheduling request is allowed to
reach Electron. Use the existing scheduleToken logic in useReminders to
serialize or gate requests so older in-flight updates cannot apply after a newer
one.
---
Nitpick comments:
In `@src-tauri/Cargo.toml`:
- Around line 36-37: The current lints.rust setting for unexpected_cfgs is too
broad and suppresses cfg typo checks across the entire crate. Narrow the
suppression by updating the Cargo.toml lint configuration to only whitelist the
specific legacy objc-related cfg case or move the allow closer to the affected
code path, using the existing lints.rust section as the place to adjust. Keep
real #[cfg] validation enabled everywhere else so platform-gating mistakes still
surface.
In `@src/api.ts`:
- Around line 58-59: The bridge methods pauseShortcuts and resumeShortcuts are
typed inconsistently with their async invoke behavior, so update the contract to
return Promise<void> instead of void. Change the declarations in src/types.d.ts
and the test mock to match, then remove the unnecessary cast in src/api.ts so
the functions directly return the invoke(...) Promise. Ensure the API surface
and mock names stay aligned for pauseShortcuts and resumeShortcuts.
In `@src/components/KeybindsModal.tsx`:
- Around line 123-130: The global shortcut loop in KeybindsModal.tsx is still
hardcoded to two cases, so add the action and storage key mapping to
ShortcutConfig and use that metadata here instead of branching on sc.key. Update
the code that builds oldShortcutKey and action inside the shortcuts loop to read
from each ShortcutConfig entry, so any new section: 'global' shortcut can be
handled without silently falling back to 'new-note'. Keep the
updateGlobalShortcut call using the resolved action, stored shortcut value, and
values[sc.key] from the same config-driven source.
In `@src/components/TimersPage.tsx`:
- Line 1: The countdown loop in TimersPage has split sources of truth because
the first schedule still hard-codes 250 while the follow-up uses
TICK_INTERVAL_MS. Update the initial setTimeout in the timer loop to use
TICK_INTERVAL_MS as well, and keep the change aligned with the timer scheduling
logic in TimersPage so both the first and subsequent ticks share the same
constant.
🪄 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: e2f3606a-885c-42c6-99b4-5c0ad001ce11
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
src-tauri/Cargo.tomlsrc-tauri/src/commands/notifications.rssrc-tauri/src/lib.rssrc/App.tsxsrc/GraphView.tsxsrc/api.tssrc/components/Editor.tsxsrc/components/KeybindsModal.tsxsrc/components/TimersPage.tsxsrc/hooks/useReminders.tssrc/lib/editor/MathEvaluator.tssrc/lib/editor/VariableScope.tssrc/lib/editor/extensions.tssrc/setupTests.tssrc/store/useAppStore.test.tssrc/store/useAppStore.tssrc/store/useSettingsStore.tssrc/store/useTimerStore.tssrc/types.d.ts
💤 Files with no reviewable changes (1)
- src/store/useAppStore.test.ts
… ref, stale guard, cfg scope, shortcut loop, timer constant
…de quality cleanup and keybinds customization
This was referenced Jun 29, 2026
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.
Summary
Comprehensive code quality cleanup across the entire codebase:
Dead Code Removal
resumeTimer(no-op stub in useTimerStore)onSwipeGesture(ignores callback in api.ts)themePreset/setThemePreset(duplicated in useAppStore — all consumers use useSettingsStore)prevNotesRef(assigned but never read in useReminders)Boilerplate Consolidation
useAppStore.ts: 9 setters reduced viabooleanSetter/simpleSetterhelpersapi.ts: 5 identical listener patterns → singleonEventhelper (63→40 lines)KeybindsModal.tsx: 9 paralleluseState/getShortcutcalls → data-driven config array (223→140 lines)useSettingsStore.ts: 11 redundant individual setters removed — usesetSettingsinsteadRust Fixes
needless_borrows_for_generic_argsin notifications.rs[lints.rust] unexpected_cfgs = "allow"for oldobjccrate macrosType Safety
any→ typedGraphControlsinterface in GraphViewPromise<unknown>→ properly typed response foropenAIChatascasts replaced with wrapper functionsMagic Numbers → Named Constants
~25 magic numbers extracted: toast timeouts, z-indices, force parameters, debounce intervals, canvas dimensions, etc.
Comment Cleanup
Removed ~15 pedagogical/AI-generated comments that explain the obvious
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Chores