refactor: improve performance for common use cases#307
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates app build/runtime wiring, migrates theme and component APIs to Chakra v3 patterns, and rewires notes, tags, settings, and renderer storage/telemetry flows. ChangesChakra v3 migration and runtime updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/features/NoteEditor/NoteVersions.tsx (1)
61-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the shadowed
noteIdcheck in the history event listener.The callback parameter shadows the component prop, so
if (noteId !== noteId)is always false and the versions list refreshes for unrelated notes.Proposed fix
useEffect(() => { - return eventBus.listen(WorkspaceEvents.NOTE_HISTORY_UPDATED, (noteId) => { - if (noteId !== noteId) return; + return eventBus.listen( + WorkspaceEvents.NOTE_HISTORY_UPDATED, + (updatedNoteId) => { + if (updatedNoteId !== noteId) return; updateVersionsList(); - }); + }, + ); }, [eventBus, updateVersionsList]);🤖 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 `@packages/app/src/features/NoteEditor/NoteVersions.tsx` around lines 61 - 63, The listener passed to eventBus.listen for WorkspaceEvents.NOTE_HISTORY_UPDATED shadows the component prop noteId (so if (noteId !== noteId) is wrong); rename the callback parameter (e.g., updatedNoteId or incomingNoteId) and compare that to the component prop noteId, e.g., if (updatedNoteId !== noteId) return; then call updateVersionsList(); update the listener closure in NoteVersions.tsx (the eventBus.listen callback) to use the new parameter name in the comparison.
🤖 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.
Outside diff comments:
In `@packages/app/src/features/NoteEditor/NoteVersions.tsx`:
- Around line 61-63: The listener passed to eventBus.listen for
WorkspaceEvents.NOTE_HISTORY_UPDATED shadows the component prop noteId (so if
(noteId !== noteId) is wrong); rename the callback parameter (e.g.,
updatedNoteId or incomingNoteId) and compare that to the component prop noteId,
e.g., if (updatedNoteId !== noteId) return; then call updateVersionsList();
update the listener closure in NoteVersions.tsx (the eventBus.listen callback)
to use the new parameter name in the comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bfeed80-576a-4b2f-ac6f-f6956935a709
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
packages/app/package.jsonpackages/app/src/components/ListBox/ListBox.theme.tspackages/app/src/components/ListBox/ListBox.tsxpackages/app/src/components/NestedList/index.tsxpackages/app/src/components/PropertiesForm.tsxpackages/app/src/components/TagEditor/index.tsxpackages/app/src/components/theme/base.tspackages/app/src/features/App/ChooseVaultScreen.tsxpackages/app/src/features/App/Settings/sections/WorkspaceSettings.tsxpackages/app/src/features/App/VaultCreator/index.tsxpackages/app/src/features/App/VaultLoginForm/index.tsxpackages/app/src/features/App/WelcomeScreen.tsxpackages/app/src/features/MainScreen/NotesListPanel/index.tsxpackages/app/src/features/NoteEditor/NoteVersions.tsx
💤 Files with no reviewable changes (1)
- packages/app/src/components/NestedList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/app/src/components/PropertiesForm.tsx
- packages/app/src/features/App/Settings/sections/WorkspaceSettings.tsx
- packages/app/src/components/TagEditor/index.tsx
- packages/app/src/features/MainScreen/NotesListPanel/index.tsx
- packages/app/src/features/App/VaultLoginForm/index.tsx
- packages/app/src/features/App/VaultCreator/index.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/features/NoteEditor/NoteMenu.tsx (1)
68-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDisable or remove non-functional menu items.
These menu items have no
onSelecthandler, making them appear interactive but do nothing when clicked. This creates a confusing user experience.Consider one of these approaches:
- Add
disabledprop if these features are planned but not yet implemented- Remove the items entirely until they're ready
- Add code comments explaining they're placeholders
♻️ Example: disabling placeholder items
- <Menu.Item value="remindMe"> + <Menu.Item value="remindMe" disabled> <HStack> <FaBell /> <Text>{t('note.menu.remindMe')}</Text> </HStack> </Menu.Item>Apply the same pattern to backLinks, readonlyMode, spellcheck, passwordProtection, and disableSync.
Also applies to: 90-95, 96-101, 102-107, 121-128, 129-134
🤖 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 `@packages/app/src/features/NoteEditor/NoteMenu.tsx` around lines 68 - 73, Several Menu.Item entries (e.g., value="remindMe" and the other items referenced as backLinks, readonlyMode, spellcheck, passwordProtection, disableSync) are interactive but lack onSelect handlers; mark these placeholder items as non-interactive by adding the disabled prop to each Menu.Item (or remove them if you prefer), and add a short inline comment indicating they are placeholders until implemented; update Menu.Item elements for remindMe, backLinks, readonlyMode, spellcheck, passwordProtection, and disableSync accordingly so the UI doesn’t present non-functional clickable options.
🤖 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.
Outside diff comments:
In `@packages/app/src/features/NoteEditor/NoteMenu.tsx`:
- Around line 68-73: Several Menu.Item entries (e.g., value="remindMe" and the
other items referenced as backLinks, readonlyMode, spellcheck,
passwordProtection, disableSync) are interactive but lack onSelect handlers;
mark these placeholder items as non-interactive by adding the disabled prop to
each Menu.Item (or remove them if you prefer), and add a short inline comment
indicating they are placeholders until implemented; update Menu.Item elements
for remindMe, backLinks, readonlyMode, spellcheck, passwordProtection, and
disableSync accordingly so the UI doesn’t present non-functional clickable
options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68d44e76-4dfe-44d6-b000-fa4b75bf3dac
📒 Files selected for processing (11)
packages/app/src/components/theme/base.tspackages/app/src/components/ui/toaster.tsxpackages/app/src/features/App/VaultLoginForm/index.tsxpackages/app/src/features/App/useGetAppUpdates.tsxpackages/app/src/features/MainScreen/NotesListPanel/index.tsxpackages/app/src/features/MainScreen/TagsPanel/TagsTree.tsxpackages/app/src/features/MainScreen/TagsPanel/VirtualTagsList.tsxpackages/app/src/features/NoteEditor/EditorPanel/buttons/HeaderPicker.tsxpackages/app/src/features/NoteEditor/NoteMenu.tsxpackages/app/src/utils/dev.tspackages/app/src/windows/main/renderer.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/app/src/utils/dev.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/app/src/features/App/VaultLoginForm/index.tsx
- packages/app/src/components/ui/toaster.tsx
- packages/app/src/features/MainScreen/TagsPanel/VirtualTagsList.tsx
- packages/app/src/features/App/useGetAppUpdates.tsx
- packages/app/src/windows/main/renderer.tsx
- packages/app/src/features/NoteEditor/EditorPanel/buttons/HeaderPicker.tsx
- packages/app/src/features/MainScreen/TagsPanel/TagsTree.tsx
- packages/app/src/features/MainScreen/NotesListPanel/index.tsx
- packages/app/src/components/theme/base.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/features/MainScreen/TagsPanel/TagsTree.tsx (1)
47-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop modifier-based multi-select until this tree is actually controlled as multi-select.
activeTagmakes this component single-select, andsetSelectedItemsimmediately collapses any selection update back to one id/null. The ctrl/meta branch still callsitem.toggleSelect(), so modifier-click asks headless-tree for multi-selection and then truncates it in the callback. That produces incorrect selection behavior in the tag list.💡 Minimal fix
onClick: (e: MouseEvent) => { - if (e.ctrlKey || e.metaKey) { - item.toggleSelect(); - } else { - tree.setSelectedItems([item.getItemMeta().itemId]); - } + tree.setSelectedItems([item.getItemMeta().itemId]); item.setFocused(); },Also applies to: 85-97
🤖 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 `@packages/app/src/features/MainScreen/TagsPanel/TagsTree.tsx` around lines 47 - 52, The click handler currently tries to perform modifier-based multi-select by calling item.toggleSelect() when e.ctrlKey/e.metaKey is set while the tree is single-select via activeTag and tree.setSelectedItems(), causing selection conflicts; remove the modifier-branch so the onClick always calls tree.setSelectedItems([item.getItemMeta().itemId]) (and likewise remove or change any identical logic around the other onClick at the 85-97 range) so selection is consistently controlled by activeTag and multi-select toggling is not attempted client-side.
🤖 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.
Outside diff comments:
In `@packages/app/src/features/MainScreen/TagsPanel/TagsTree.tsx`:
- Around line 47-52: The click handler currently tries to perform modifier-based
multi-select by calling item.toggleSelect() when e.ctrlKey/e.metaKey is set
while the tree is single-select via activeTag and tree.setSelectedItems(),
causing selection conflicts; remove the modifier-branch so the onClick always
calls tree.setSelectedItems([item.getItemMeta().itemId]) (and likewise remove or
change any identical logic around the other onClick at the 85-97 range) so
selection is consistently controlled by activeTag and multi-select toggling is
not attempted client-side.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9ee50db-3380-4603-a0e7-0b8cee0794fe
📒 Files selected for processing (2)
packages/app/src/features/MainScreen/TagsPanel/TagsTree.tsxpackages/app/src/state/redux/vaults/selectors/tags/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/features/NotesContainer/NoteContextMenu/ElectronContextMenu.ts (1)
23-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancellation and selection events are mixed incorrectly.
nullcancellation still triggersonClicked, and successful selection never triggersonClosed, which leaves close watchers uncleared in the caller path.Suggested fix
}).then((action) => { if (action === null) { this.onClosed(); + return; } - - this.onClicked(action as T); + this.onClicked(action as T); + this.onClosed(); });🤖 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 `@packages/app/src/features/NotesContainer/NoteContextMenu/ElectronContextMenu.ts` around lines 23 - 29, The promise handler currently calls onClicked even when action is null and never calls onClosed after a successful selection; change the then((action) => { ... }) logic so that if action === null you call this.onClosed() and return immediately, and if action !== null you first call this.onClicked(action as T) and then call this.onClosed() to ensure close watchers are cleared; update the handler surrounding the Promise returned in ElectronContextMenu so both paths are handled explicitly.packages/app/src/hooks/useShowNoteContextMenu.ts (1)
17-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubscribe before
opento avoid dropping immediate events.Calling
openfirst can missonClose/onClickif the menu implementation emits synchronously (which the DOM path currently does).Suggested fix
- contextMenu.open(point); - const unsubscribeOnClick = contextMenu.onClick((action) => { callback({ action, id }); unsubscribeOnClick(); }); const unsubscribeOnClose = contextMenu.onClose(() => { unsubscribeOnClick(); unsubscribeOnClose(); }); + + contextMenu.open(point);🤖 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 `@packages/app/src/hooks/useShowNoteContextMenu.ts` around lines 17 - 27, The current code calls contextMenu.open(point) before subscribing, which can drop synchronous events; move the subscriptions for contextMenu.onClick and contextMenu.onClose so they are created before calling contextMenu.open(point), capture their unsubscribe functions (unsubscribeOnClick, unsubscribeOnClose), then call contextMenu.open(point); ensure the onClick handler still invokes callback({ action, id }) and calls unsubscribeOnClick(), and the onClose handler calls both unsubscribeOnClick() and unsubscribeOnClose() to clean up.
♻️ Duplicate comments (1)
packages/app/src/features/NotesContainer/NoteContextMenu/DOMContextMenu.ts (1)
17-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
DOMContextMenu.opencurrently disables all menu actions.
opencloses immediately and never emitsonClicked, so note context-menu commands cannot run in non-Electron runtime.🤖 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 `@packages/app/src/features/NotesContainer/NoteContextMenu/DOMContextMenu.ts` around lines 17 - 25, DOMContextMenu.open currently calls this.onClosed() immediately which prevents the menu from rendering and stops any menu item click events (onClicked) from firing; update DOMContextMenu.open to render the menu DOM using this.menu and position it at {x,y} instead of closing, attach click handlers to each menu item that call this.onClicked(item) and then call this.onClosed() after selection or when clicking outside, and ensure existing methods (onClosed, onClicked) are used for lifecycle rather than immediately invoking onClosed on open so context-menu commands work in non-Electron runtimes.
🤖 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 `@packages/app/src/state/redux/vaults/hooks.ts`:
- Around line 17-20: useWorkspaceData currently returns a new plain object each
render which makes consumers like useWorkspaceActions (which depends on
[workspaceData]) re-create wrappers every time; update useWorkspaceData to
return a stable/memoized reference (e.g., useMemo) based on
useWorkspaceContext()'s vaultId and workspaceId so the returned object identity
only changes when those values change, ensuring useWorkspaceActions (and any
other consumers) do not rebuild unnecessarily.
---
Outside diff comments:
In
`@packages/app/src/features/NotesContainer/NoteContextMenu/ElectronContextMenu.ts`:
- Around line 23-29: The promise handler currently calls onClicked even when
action is null and never calls onClosed after a successful selection; change the
then((action) => { ... }) logic so that if action === null you call
this.onClosed() and return immediately, and if action !== null you first call
this.onClicked(action as T) and then call this.onClosed() to ensure close
watchers are cleared; update the handler surrounding the Promise returned in
ElectronContextMenu so both paths are handled explicitly.
In `@packages/app/src/hooks/useShowNoteContextMenu.ts`:
- Around line 17-27: The current code calls contextMenu.open(point) before
subscribing, which can drop synchronous events; move the subscriptions for
contextMenu.onClick and contextMenu.onClose so they are created before calling
contextMenu.open(point), capture their unsubscribe functions
(unsubscribeOnClick, unsubscribeOnClose), then call contextMenu.open(point);
ensure the onClick handler still invokes callback({ action, id }) and calls
unsubscribeOnClick(), and the onClose handler calls both unsubscribeOnClick()
and unsubscribeOnClose() to clean up.
---
Duplicate comments:
In `@packages/app/src/features/NotesContainer/NoteContextMenu/DOMContextMenu.ts`:
- Around line 17-25: DOMContextMenu.open currently calls this.onClosed()
immediately which prevents the menu from rendering and stops any menu item click
events (onClicked) from firing; update DOMContextMenu.open to render the menu
DOM using this.menu and position it at {x,y} instead of closing, attach click
handlers to each menu item that call this.onClicked(item) and then call
this.onClosed() after selection or when clicking outside, and ensure existing
methods (onClosed, onClicked) are used for lifecycle rather than immediately
invoking onClosed on open so context-menu commands work in non-Electron
runtimes.
🪄 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 Plus
Run ID: 8101e2cd-b488-42c0-8816-0307ff74bf2a
📒 Files selected for processing (19)
packages/app/src/components/AppToast.tsxpackages/app/src/components/CalmButton.tsxpackages/app/src/components/Features/Header/FeaturesHeader.tsxpackages/app/src/components/ListBox/ListBox.theme.tspackages/app/src/components/WithFocusRedirect.tsxpackages/app/src/features/App/Settings/sections/ImportAndExport.tsxpackages/app/src/features/MainScreen/ActivityBar.tsxpackages/app/src/features/MainScreen/NotesListPanel/index.tsxpackages/app/src/features/MainScreen/TagsPanel/TagsTree.tsxpackages/app/src/features/MainScreen/TagsPanel/VirtualTagsList.tsxpackages/app/src/features/MainScreen/TagsPanel/useMemoizedCallback.tsxpackages/app/src/features/NoteEditor/index.tsxpackages/app/src/features/NotesContainer/NoteContextMenu/DOMContextMenu.tspackages/app/src/features/NotesContainer/NoteContextMenu/ElectronContextMenu.tspackages/app/src/hooks/useContextMenu.tspackages/app/src/hooks/useShowNoteContextMenu.tspackages/app/src/state/redux/vaults/hooks.tspackages/app/src/state/redux/vaults/selectors/tags/index.tspackages/app/src/windows/main/renderer.tsx
💤 Files with no reviewable changes (5)
- packages/app/src/components/CalmButton.tsx
- packages/app/src/components/WithFocusRedirect.tsx
- packages/app/src/components/ListBox/ListBox.theme.ts
- packages/app/src/components/AppToast.tsx
- packages/app/src/features/MainScreen/ActivityBar.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/app/src/features/MainScreen/TagsPanel/VirtualTagsList.tsx
- packages/app/src/hooks/useContextMenu.ts
- packages/app/src/features/App/Settings/sections/ImportAndExport.tsx
- packages/app/src/components/Features/Header/FeaturesHeader.tsx
- packages/app/src/windows/main/renderer.tsx
- packages/app/src/features/MainScreen/TagsPanel/TagsTree.tsx
- packages/app/src/features/MainScreen/NotesListPanel/index.tsx
- packages/app/src/state/redux/vaults/selectors/tags/index.ts
- packages/app/src/features/NoteEditor/index.tsx
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.
Closes #306, closes #297
Changes
Side changes
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
Performance