feat: Remember opened notes#217
Merged
vitonsky merged 201 commits intoApr 10, 2026
Merged
Conversation
katsyuta
force-pushed
the
173-remember-opened-notes
branch
from
089986e to
af0e8e9
Compare
katsyuta
force-pushed
the
173-remember-opened-notes
branch
from
af0e8e9 to
7b031ec
Compare
katsyuta
changed the title
katsyuta
force-pushed
the
173-remember-opened-notes
branch
4 times, most recently
from
c3da850 to
9442acb
Compare
katsyuta
force-pushed
the
173-remember-opened-notes
branch
2 times, most recently
from
7e8f3a4 to
0425dc5
Compare
Collaborator
Author
|
@vitonsky, this pull request is ready! |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors vault/workspace initialization and persistence: adds vault- and workspace-level error providers, workspace state/config sync services, loading-status tracking, screen-based vault selection, and UI for workspace errors and recovery. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User / UI
participant Profile as Profile component
participant Vault as VaultStorage / Files
participant Redux as Redux (profiles/workspaces)
participant WorkspaceSvc as WorkspaceServices / Sync hooks
UI->>Profile: choose/open vault
Profile->>Vault: open profile (read config.json)
Vault-->>Profile: vault config
Profile->>Redux: dispatch workspaces list (from workspacesManager)
Profile->>WorkspaceSvc: mount WorkspaceServices for each workspace
WorkspaceSvc->>Vault: create/open workspace state/config files
WorkspaceSvc->>Redux: watch selectors (workspace state/config)
Redux-->>WorkspaceSvc: state updates
WorkspaceSvc->>Vault: write state/config files
alt any error
component->>Profile: report error via useVaultError / useWorkspaceError
Profile->>UI: render Vault/Workspace Error Screen
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🧹 Nitpick comments (2)
src/features/App/Workspace/useWorkspaceState.tsx (1)
60-66: Consider adding error handling for state persistence.The
workspaceState.set(state)call is fire-and-forget. If persistence fails, the error is silently swallowed. Consider logging errors to aid debugging:🔧 Suggested improvement
onChange(state) { if (!state) return; - workspaceState.set(state); + workspaceState.set(state).catch((error) => { + console.error('Failed to save workspace state', error); + }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/useWorkspaceState.tsx` around lines 60 - 66, The onChange callback in useWorkspaceState.tsx calls workspaceState.set(state) fire-and-forget; wrap that call in a try/catch inside the onChange handler to catch persistence errors and log them (e.g., using the existing logger or console.error) and optionally surface or rethrow if needed; update the onChange function (the callback that currently does "if (!state) return; workspaceState.set(state);") to handle and log exceptions from workspaceState.set(state) so failures are not silently swallowed.src/features/App/useRecentProfile.ts (1)
10-13: Misleading variable name:profileIdholds the entireRecentProfileobject.The state variable is named
profileIdbut it stores aRecentProfileobject with bothisLoadedandprofileIdfields. This causes confusion—e.g.,setProfileId({ isLoaded: true, profileId: activeProfile })reads oddly.Consider renaming to better reflect the actual content:
🔧 Suggested rename
- const [profileId, setProfileId] = useState<RecentProfile>({ + const [recentProfile, setRecentProfile] = useState<RecentProfile>({ isLoaded: false, profileId: null, });And update the setter call at line 22-25:
- setProfileId({ + setRecentProfile({ isLoaded: true, profileId: activeProfile, });And the return at line 29:
- return profileId; + return recentProfile;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useRecentProfile.ts` around lines 10 - 13, The state variable profileId actually stores a RecentProfile object; rename both the variable and its setter to reflect that (for example recentProfile and setRecentProfile) and update all usages: initialization (useState<RecentProfile>), setter calls like setProfileId({ isLoaded: true, profileId: activeProfile }) -> setRecentProfile(...), and the returned value from the hook so the exported name matches (replace references to profileId and setProfileId with recentProfile and setRecentProfile throughout useRecentProfile.ts and any callers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/App/useVaultOpener.tsx`:
- Around line 37-60: The isProfileOpening flag isn't cleared on all paths;
ensure setIsProfileOpening(false) runs no matter how the async opener exits by
wrapping the whole operation in a try/finally (or moving the early-return
branches to set the flag false before returning). Concretely, in the async
opener function that calls profiles.openProfile (the anonymous async (profile:
ProfileObject, password?: string) => { ... }), make sure any early return for
the no-encryption branch and the password-missing branch and the try/catch for
passworded profiles all ultimately execute setIsProfileOpening(false) in a
finally block (or call setIsProfileOpening(false) immediately before each
return) so the loading state is always reset.
In `@src/features/App/VaultScreenManager.tsx`:
- Around line 63-68: The create-profile flow currently calls
profiles.createProfile(...).then(newProfile => onOpenProfile(newProfile,
profile.password ?? undefined).then(console.warn)) which logs every result and
ignores the returned status; update the onCreateProfile handler to await (or
return) the Promise from onOpenProfile and handle success/error explicitly
instead of using console.warn so failures are surfaced to the caller/UI;
specifically change the chain that uses profiles.createProfile and onOpenProfile
(and the similar block around onOpenProfile usage at the other occurrence) to
propagate the resolved status or throw/return an error so the caller can react
(e.g., show an error toast or set error state) rather than swallowing the
result.
In `@src/features/App/Workspace/services/useWorkspaceConfigSync.tsx`:
- Around line 36-41: The onChange handler in useWorkspaceConfigSync calls
workspaceConfig.set(config) but only attaches .then(...), leaving rejections
unhandled; update the onChange implementation to handle failures by adding a
.catch(...) on workspaceConfig.set (or convert to async/await with try/catch) to
log the error and surface/report the failure (e.g., processLogger/errorToast)
and ensure any necessary rollback or retry behavior; reference the onChange
handler in useWorkspaceConfigSync and the workspaceConfig.set call when making
the change.
In `@src/features/App/Workspace/useWorkspaceInitialization.tsx`:
- Around line 76-83: The persisted variable view (string | null) is being
unsafely cast to NOTES_VIEW before dispatching workspacesApi.setView; validate
the persisted view against the NOTES_VIEW enum first (e.g., const valid =
Object.values(NOTES_VIEW).includes(view as NOTES_VIEW)) and only cast when
valid, otherwise use a safe fallback (default view or omit view from
workspaceData) so update the block around useWorkspaceInitialization/useEffect
to compute a validatedView and pass that into workspacesApi.setView rather than
directly casting view to NOTES_VIEW.
---
Nitpick comments:
In `@src/features/App/useRecentProfile.ts`:
- Around line 10-13: The state variable profileId actually stores a
RecentProfile object; rename both the variable and its setter to reflect that
(for example recentProfile and setRecentProfile) and update all usages:
initialization (useState<RecentProfile>), setter calls like setProfileId({
isLoaded: true, profileId: activeProfile }) -> setRecentProfile(...), and the
returned value from the hook so the exported name matches (replace references to
profileId and setProfileId with recentProfile and setRecentProfile throughout
useRecentProfile.ts and any callers).
In `@src/features/App/Workspace/useWorkspaceState.tsx`:
- Around line 60-66: The onChange callback in useWorkspaceState.tsx calls
workspaceState.set(state) fire-and-forget; wrap that call in a try/catch inside
the onChange handler to catch persistence errors and log them (e.g., using the
existing logger or console.error) and optionally surface or rethrow if needed;
update the onChange function (the callback that currently does "if (!state)
return; workspaceState.set(state);") to handle and log exceptions from
workspaceState.set(state) so failures are not silently swallowed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/core/features/notes/controller/NotesController.test.tssrc/features/App/Profile/index.tsxsrc/features/App/Profile/services/useVaultConfigSync.tsxsrc/features/App/ProfileCreator/index.tsxsrc/features/App/ProfileLoginForm/index.tsxsrc/features/App/ProfilesForm.tsxsrc/features/App/VaultScreenManager.tsxsrc/features/App/Workspace/index.tsxsrc/features/App/Workspace/services/WorkspaceServices.tsxsrc/features/App/Workspace/services/useWorkspaceConfigSync.tsxsrc/features/App/Workspace/useWorkspaceInitialization.tsxsrc/features/App/Workspace/useWorkspaceState.tsxsrc/features/App/WorkspaceManager/index.tsxsrc/features/App/index.tsxsrc/features/App/useProfilesList.tssrc/features/App/useRecentProfile.tssrc/features/App/useVaultOpener.tsxsrc/state/redux/profiles/profiles.ts
💤 Files with no reviewable changes (1)
- src/features/App/WorkspaceManager/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/App/useVaultOpener.tsx`:
- Around line 51-55: In the catch block inside useVaultOpener (the function that
currently logs err and returns { status: 'error', message: 'Invalid password'
}), replace the hard-coded password-specific message with a generic open failure
message (e.g., 'Failed to open profile') and include optional error details for
diagnostics (for example combine a short user-facing message with err?.message
or pass error info to telemetry/logging). Ensure process remains:
console.error(err) (or processLogger) still logs the full error and the returned
object uses the generic message instead of 'Invalid password'.
- Around line 63-83: The effect in useVaultOpener reads profilesList.profiles
and recentProfile.profileId but only depends on the loaded flags, causing stale
closures; update the effect to include profilesList.profiles and
recentProfile.profileId in its dependency array and add a ref-based guard (e.g.,
hasAutoOpenedRef) inside the effect to preserve the run-once behavior;
specifically, in the useEffect that currently calls setCurrentProfileId and
onOpenProfile, check and set hasAutoOpenedRef.current to ensure the auto-open
logic runs only once while allowing the effect to re-evaluate when
profilesList.profiles or recentProfile.profileId change.
In `@src/features/App/Workspace/useWorkspaceInitialization.tsx`:
- Around line 152-155: The tags change listener installed via
workspace.tagsRegistry.onChange currently calls workspace.tagsRegistry.getTags()
without handling rejections; update the listener to add a .catch(...) on
getTags() to handle errors (and avoid unhandled promise rejections) and, if the
component can unmount/dispose, guard the subsequent
dispatch(workspacesApi.setTags({ ...workspaceData, tags })) so it is skipped
when disposed; reference workspace.tagsRegistry.onChange,
workspace.tagsRegistry.getTags, dispatch, workspacesApi.setTags, and
workspaceData when making the change.
- Around line 58-135: initWorkspace is started fire-and-forget and its promises
can resolve after the component has switched workspaces, causing stale updates
and uncaught rejections; wrap the async flow in try/catch to handle errors from
getWorkspaceState, workspace.tagsRegistry.getTags, and
workspace.notesRegistry.getById (log or dispatch an error), and prevent stale
state writes by gating all dispatches (including
workspacesApi.setIsWorkspaceReady in the finally) with a run token or mounted
check: generate a unique runId (or use an AbortController/ref) when starting
initWorkspace, store it in a local ref, and before each dispatch/assertion
verify the current runId matches so only the latest run updates state (ensure
you reference initWorkspace, getWorkspaceState, workspace.tagsRegistry.getTags,
workspace.notesRegistry.getById, dispatch, and workspacesApi.setIsWorkspaceReady
in your changes).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/App/Workspace/useWorkspaceInitialization.tsxsrc/features/App/useVaultOpener.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/features/App/useVaultOpener.tsx (3)
56-60:⚠️ Potential issue | 🟡 MinorUse a generic open-failure message in
catch.Line 59 always returns
'Invalid password', but failures can also be storage/runtime/open errors. This is misleading for users and diagnostics.Suggested fix
- return { status: 'error', message: 'Invalid password' }; + return { status: 'error', message: 'Failed to open profile' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useVaultOpener.tsx` around lines 56 - 60, The catch block inside useVaultOpener (the try/catch around the vault opening logic) currently always returns message 'Invalid password'; change it to return a generic failure like 'Failed to open vault' and avoid asserting the cause; log the actual error (err) for diagnostics via console.error or processLogger.error so developers see the real exception but users see the generic message; ensure the returned object remains { status: 'error', message: 'Failed to open vault' } from the catch in useVaultOpener.
68-87:⚠️ Potential issue | 🟠 MajorRemove stale-closure risk in auto-open effect.
Lines 73-77 read
recentProfile.profileIdandprofilesList.profiles, but they are not in the dependency array (Line 87), and exhaustive-deps is suppressed (Line 86). This can miss updates after initial load.Suggested refactor (preserve run-once behavior)
-import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; @@ const [isProfileOpening, setIsProfileOpening] = useState(false); + const didAutoOpenRef = useRef(false); @@ - useEffect( - () => { - if (!profilesList.isProfilesLoaded || !recentProfile.isLoaded) return; + useEffect(() => { + if (didAutoOpenRef.current) return; + if (!profilesList.isProfilesLoaded || !recentProfile.isLoaded) return; - // Restore profile id - setCurrentProfileId(recentProfile.profileId); + // Restore profile id + setCurrentProfileId(recentProfile.profileId); - const profile = profilesList.profiles.find( - (profile) => profile.id === recentProfile.profileId, - ); + const profile = profilesList.profiles.find( + (profile) => profile.id === recentProfile.profileId, + ); - if (!profile || profile.encryption) { - return; - } + didAutoOpenRef.current = true; + if (!profile || profile.encryption) return; - onOpenProfile(profile); - }, - // Depends only of loading status and run only once - // eslint-disable-next-line react-hooks/exhaustive-deps - [profilesList.isProfilesLoaded, recentProfile.isLoaded], - ); + void onOpenProfile(profile); + }, [ + profilesList.isProfilesLoaded, + profilesList.profiles, + recentProfile.isLoaded, + recentProfile.profileId, + setCurrentProfileId, + onOpenProfile, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useVaultOpener.tsx` around lines 68 - 87, The useEffect in useVaultOpener has a stale-closure risk because it reads recentProfile.profileId and profilesList.profiles but suppresses exhaustive-deps; update it to avoid missing later updates by capturing the necessary values before the run-once effect or by using refs/callbacks: compute and store the resolved profileId and profile object (from profilesList.profiles) in local refs or state (or read them into a stable value via useMemo) and then call setCurrentProfileId and onOpenProfile from the effect using those stable references; specifically adjust the logic in the effect that references recentProfile.profileId, profilesList.profiles, setCurrentProfileId and onOpenProfile so the effect does not depend on changing closures while preserving run-once behavior.
36-46:⚠️ Potential issue | 🔴 CriticalWrap all open paths in one
try/finallyto prevent stuck loading state.At Line 42,
profiles.openProfile({ profile }, true)is outside thetry/catch/finally. If it throws,setIsProfileOpening(false)is skipped and the effect-triggered call can reject unhandled.Suggested fix
const onOpenProfile: OnPickProfile = useCallback( async (profile: ProfileObject, password?: string) => { - setIsProfileOpening(true); - - // Profiles with no password - if (!profile.encryption) { - await profiles.openProfile({ profile }, true); - - setIsProfileOpening(false); - return { status: 'ok' }; - } - - try { - // Profiles with password - if (profile.encryption && password === undefined) - return { status: 'error', message: 'Enter password' }; - - await profiles.openProfile({ profile, password }, true); + if (profile.encryption && password === undefined) { + return { status: 'error', message: 'Enter password' }; + } + + setIsProfileOpening(true); + try { + if (!profile.encryption) { + await profiles.openProfile({ profile }, true); + } else { + await profiles.openProfile({ profile, password }, true); + } return { status: 'ok' }; } catch (err) { console.error(err); return { status: 'error', message: 'Invalid password' }; } finally { setIsProfileOpening(false); } }, [profiles], );Also applies to: 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useVaultOpener.tsx` around lines 36 - 46, The onOpenProfile handler currently calls profiles.openProfile({ profile }, true) (and the alternate branch around lines 60-62 for the password case) outside of a try/finally so setIsProfileOpening(false) can be skipped if openProfile throws; wrap the entire async body that performs profiles.openProfile (both the no-password and password branches) in a try/finally and move setIsProfileOpening(false) into the finally block inside the useCallback so the loading flag is always cleared even on exceptions, preserving existing return paths or rethrowing the error as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/features/App/useVaultOpener.tsx`:
- Around line 56-60: The catch block inside useVaultOpener (the try/catch around
the vault opening logic) currently always returns message 'Invalid password';
change it to return a generic failure like 'Failed to open vault' and avoid
asserting the cause; log the actual error (err) for diagnostics via
console.error or processLogger.error so developers see the real exception but
users see the generic message; ensure the returned object remains { status:
'error', message: 'Failed to open vault' } from the catch in useVaultOpener.
- Around line 68-87: The useEffect in useVaultOpener has a stale-closure risk
because it reads recentProfile.profileId and profilesList.profiles but
suppresses exhaustive-deps; update it to avoid missing later updates by
capturing the necessary values before the run-once effect or by using
refs/callbacks: compute and store the resolved profileId and profile object
(from profilesList.profiles) in local refs or state (or read them into a stable
value via useMemo) and then call setCurrentProfileId and onOpenProfile from the
effect using those stable references; specifically adjust the logic in the
effect that references recentProfile.profileId, profilesList.profiles,
setCurrentProfileId and onOpenProfile so the effect does not depend on changing
closures while preserving run-once behavior.
- Around line 36-46: The onOpenProfile handler currently calls
profiles.openProfile({ profile }, true) (and the alternate branch around lines
60-62 for the password case) outside of a try/finally so
setIsProfileOpening(false) can be skipped if openProfile throws; wrap the entire
async body that performs profiles.openProfile (both the no-password and password
branches) in a try/finally and move setIsProfileOpening(false) into the finally
block inside the useCallback so the loading flag is always cleared even on
exceptions, preserving existing return paths or rethrowing the error as
appropriate.
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 (1)
src/features/App/Profile/services/useVaultConfigSync.tsx (1)
33-35:⚠️ Potential issue | 🟠 MajorHandle vault config write rejections.
Line 33 lacks rejection handling, so write failures can fail silently and leave sync inconsistent.
Proposed fix
- vaultConfig.set(config).then(() => { - console.debug('Vault config is saved'); - }); + void vaultConfig + .set(config) + .then(() => { + console.debug('Vault config is saved'); + }) + .catch((error) => { + console.error('Failed to save vault config', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Profile/services/useVaultConfigSync.tsx` around lines 33 - 35, The call to vaultConfig.set(config) in useVaultConfigSync is missing rejection handling; update the write to handle failures (either await inside try/catch or append .then(...).catch(...)) so errors are logged and sync state updated accordingly; specifically modify the vaultConfig.set(config) usage in useVaultConfigSync to catch rejections, call process or component logger (e.g., console.error or an injected logger) with the error, and set any local error/sync flags or retry behavior so failed writes don't fail silently.
♻️ Duplicate comments (6)
src/features/App/Workspace/useWorkspaceInitialization.tsx (2)
58-59:⚠️ Potential issue | 🟠 MajorHandle initialization failures before setting workspace as ready.
initWorkspace()still has nocatch, and it always reachessetIsWorkspaceReady(..., true)infinally. Failures are unhandled and readiness can be misleading.Proposed fix
const initWorkspace = async () => { + let isSuccessful = false; try { const [state, tags] = await Promise.all([ getWorkspaceState(), workspace.tagsRegistry.getTags(), ]); @@ dispatch( workspacesApi.setActiveNote({ ...workspaceData, noteId: activeNote.id, }), ); + isSuccessful = true; + } catch (error) { + console.error('[useWorkspaceInitialization] Failed to initialize workspace', error); } finally { dispatch( workspacesApi.setIsWorkspaceReady({ ...workspaceData, - isReady: true, + isReady: isSuccessful, }), ); } }; - initWorkspace(); + void initWorkspace();Also applies to: 125-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/useWorkspaceInitialization.tsx` around lines 58 - 59, initWorkspace() currently lacks error handling and always calls setIsWorkspaceReady(..., true) in the finally block, which can mark the workspace ready even on failures; update initWorkspace (and the other similar initializer around the 125-135 block) to wrap the async initialization in try/catch, log or surface the caught error, setIsWorkspaceReady(false) on failure (and only setIsWorkspaceReady(true) on successful completion), and avoid unconditionally using finally to mark readiness—use finally only for cleanup and ensure the ready flag is driven by success/failure paths.
152-155:⚠️ Potential issue | 🟡 MinorHandle
tagsRegistry.getTags()errors in change listener.Lines 152-155 still omit rejection handling, which can produce unhandled promise rejections during tag sync.
Proposed fix
return workspace.tagsRegistry.onChange(() => { - workspace.tagsRegistry.getTags().then((tags) => { - dispatch(workspacesApi.setTags({ ...workspaceData, tags })); - }); + void workspace.tagsRegistry + .getTags() + .then((tags) => { + dispatch(workspacesApi.setTags({ ...workspaceData, tags })); + }) + .catch((error) => { + console.error('[useWorkspaceInitialization] Failed to sync tags', error); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/useWorkspaceInitialization.tsx` around lines 152 - 155, The onChange listener registered via workspace.tagsRegistry.onChange currently calls workspace.tagsRegistry.getTags() without handling rejections; update the listener in useWorkspaceInitialization.tsx to await or attach a .catch to getTags() and handle errors (e.g., log via processLogger or console.error and avoid dispatch on failure) before calling dispatch(workspacesApi.setTags({ ...workspaceData, tags })); ensure you reference workspace.tagsRegistry.onChange, workspace.tagsRegistry.getTags, and the dispatch/workspacesApi.setTags call so the fix surrounds the promise resolution and error path.src/features/App/Workspace/services/useWorkspaceConfigSync.tsx (1)
39-41:⚠️ Potential issue | 🟠 MajorHandle failed workspace config writes explicitly.
Line 39 only handles success. A failed
workspaceConfig.set(config)becomes an unhandled rejection and can silently break sync.Proposed fix
- workspaceConfig.set(config).then(() => { - console.debug('Workspace config is saved'); - }); + void workspaceConfig + .set(config) + .then(() => { + console.debug('Workspace config is saved'); + }) + .catch((error) => { + console.error('Failed to save workspace config', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/services/useWorkspaceConfigSync.tsx` around lines 39 - 41, The call to workspaceConfig.set(config) in useWorkspaceConfigSync only handles success and leaves rejections unhandled; wrap the write in explicit error handling by either awaiting it inside a try/catch or appending a .catch(...) to workspaceConfig.set(config). In the catch, log the error (e.g., console.error or processLogger.error) with contextual text and, if applicable, update local state or surface an error to the caller (e.g., setSyncError) so failed writes are not silently dropped.src/features/App/useVaultOpener.tsx (2)
37-46:⚠️ Potential issue | 🔴 Critical
isProfileOpeningcan get stuck on unencrypted open errors.If
profiles.openProfile({ profile }, true)throws in the unencrypted branch,setIsProfileOpening(false)is never reached. This can permanently block the UI loading state.💡 Proposed fix
const onOpenProfile: OnPickProfile = useCallback( async (profile: ProfileObject, password?: string) => { - setIsProfileOpening(true); - - // Profiles with no password - if (!profile.encryption) { - await profiles.openProfile({ profile }, true); - - setIsProfileOpening(false); - return { status: 'ok' }; - } - - try { - // Profiles with password - if (password === undefined) - return { status: 'error', message: 'Enter password' }; - - await profiles.openProfile({ profile, password }, true); + if (profile.encryption && password === undefined) { + return { status: 'error', message: 'Enter password' }; + } + + setIsProfileOpening(true); + try { + if (!profile.encryption) { + await profiles.openProfile({ profile }, true); + } else { + await profiles.openProfile({ profile, password }, true); + } return { status: 'ok' }; } catch (err) { console.error(err); return { status: 'error', message: 'Invalid password' }; } finally { setIsProfileOpening(false); } }, [profiles], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useVaultOpener.tsx` around lines 37 - 46, The unencrypted branch can throw from profiles.openProfile({ profile }, true) and never clear the loading flag; wrap that call in a try/catch (or try/finally) inside the async handler in useVaultOpener.tsx so setIsProfileOpening(false) is always executed (use finally to always call setIsProfileOpening(false), and in the catch log or return an error status), referencing the async opener function, profiles.openProfile, and the setIsProfileOpening state setter.
68-87:⚠️ Potential issue | 🟠 MajorAuto-open effect uses stale captured values due incomplete dependencies.
The effect reads
profilesList.profilesandrecentProfile.profileIdbut excludes them from dependencies, which can miss profile/recent-id updates after initial load.💡 Suggested refactor (preserve run-once semantics safely)
-import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; @@ const [isProfileOpening, setIsProfileOpening] = useState(false); + const didAutoOpenRef = useRef(false); @@ - useEffect( - () => { - if (!profilesList.isProfilesLoaded || !recentProfile.isLoaded) return; + useEffect(() => { + if (didAutoOpenRef.current) return; + if (!profilesList.isProfilesLoaded || !recentProfile.isLoaded) return; // Restore profile id setCurrentProfileId(recentProfile.profileId); @@ - if (!profile || profile.encryption) { - return; - } + didAutoOpenRef.current = true; + if (!profile || profile.encryption) return; - onOpenProfile(profile); - }, - // Depends only of loading status and run only once - // eslint-disable-next-line react-hooks/exhaustive-deps - [profilesList.isProfilesLoaded, recentProfile.isLoaded], - ); + void onOpenProfile(profile); + }, + [ + profilesList.isProfilesLoaded, + profilesList.profiles, + recentProfile.isLoaded, + recentProfile.profileId, + setCurrentProfileId, + onOpenProfile, + ], + );#!/bin/bash # Verify that the effect body references values omitted from deps. rg -n "profilesList\\.profiles|recentProfile\\.profileId|setCurrentProfileId|onOpenProfile|eslint-disable-next-line react-hooks/exhaustive-deps" src/features/App/useVaultOpener.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useVaultOpener.tsx` around lines 68 - 87, The effect currently captures stale values because it omits profilesList.profiles and recentProfile.profileId (and the mutators) from its dependency list; update the effect so it lists all referenced values (profilesList.profiles, recentProfile.profileId, setCurrentProfileId, onOpenProfile, profilesList.isProfilesLoaded, recentProfile.isLoaded) and prevent repeated runs by introducing a run-once guard ref (e.g., hasOpenedRef) checked and set inside the effect; keep the existing logic (find profile, check profile.encryption, call setCurrentProfileId and onOpenProfile) but rely on the guard to preserve run-once semantics while using correct dependencies.src/features/App/VaultScreenManager.tsx (1)
63-68:⚠️ Potential issue | 🟠 MajorOpen result is swallowed in create/select paths.
onOpenProfilestatus is ignored (and in one path logged viaconsole.warn), so failures are not surfaced to UX state.💡 Proposed fix
- onCreateProfile={(profile) => - profiles.createProfile(profile).then((newProfile) => { - onOpenProfile(newProfile, profile.password ?? undefined).then( - console.warn, - ); - }) - } + onCreateProfile={async (profile) => { + const newProfile = await profiles.createProfile(profile); + const result = await onOpenProfile( + newProfile, + profile.password ?? undefined, + ); + if (result.status === 'error') { + onChooseProfile(newProfile.id); + } + }} @@ if (profile.encryption === null) { - onOpenProfile(profile); + void onOpenProfile(profile).then((result) => { + if (result.status === 'error') { + onChooseProfile(profile.id); + } + }); }Also applies to: 116-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/VaultScreenManager.tsx` around lines 63 - 68, The onOpenProfile result is being ignored (and even passed to console.warn) in the create/select flows; update the handlers that call profiles.createProfile(...).then(...) and the similar select path to return and propagate the promise from onOpenProfile so failures surface to callers and UX state instead of being swallowed—specifically change the inline arrow handlers around profiles.createProfile and profiles.selectProfile to return onOpenProfile(newProfile, profile.password ?? undefined) and attach proper .catch handling (or rethrow) rather than calling console.warn; locate the anonymous onCreateProfile/onSelectProfile callbacks that call profiles.createProfile and profiles.selectProfile and replace the dangling .then(console.warn) pattern with returned promise propagation and explicit error handling.
🧹 Nitpick comments (1)
src/core/features/notes/controller/NotesController.test.ts (1)
406-409: Strengthen this test to verify returned identities, not only count.On Line 408, checking only
toHaveLength(10)can still pass if wrong notes are returned. Assert the returned IDs exactly matchnotesId(in order) and thatnotExistingNoteis absent.Proposed test assertion update
- await expect( - registry.getById([...notesId, notExistingNote]), - ).resolves.toHaveLength(10); + const result = await registry.getById([...notesId, notExistingNote]); + expect(result.map((note) => note.id)).toEqual(notesId); + expect(result.some((note) => note.id === notExistingNote)).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/notes/controller/NotesController.test.ts` around lines 406 - 409, The test currently only asserts the result length for registry.getById([...notesId, notExistingNote]) which can hide wrong items; update the assertion to verify the actual returned IDs and order: call registry.getById with the combined array and assert the resolved result strictly equals the expected notesId array (in order) and does not include notExistingNote (e.g., use toEqual/strict equality on the IDs array and an explicit check that notExistingNote is absent). Target the registry.getById call and the notesId / notExistingNote variables when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/App/Workspace/useWorkspaceInitialization.tsx`:
- Around line 46-47: The call to initConfig() inside the effect can yield an
unhandled rejection; wrap the async operation so failures are caught — e.g.,
make the effect call an async IIFE or call initConfig().catch(...) and log or
dispatch an error. Update the effect that references initConfig() (in
useWorkspaceInitialization / the useEffect with dependency
[controls.profile.files, dispatch, workspaceData]) to await the result inside
try/catch or attach a .catch handler so any thrown error is handled and reported
via process logger or dispatching an error action.
---
Outside diff comments:
In `@src/features/App/Profile/services/useVaultConfigSync.tsx`:
- Around line 33-35: The call to vaultConfig.set(config) in useVaultConfigSync
is missing rejection handling; update the write to handle failures (either await
inside try/catch or append .then(...).catch(...)) so errors are logged and sync
state updated accordingly; specifically modify the vaultConfig.set(config) usage
in useVaultConfigSync to catch rejections, call process or component logger
(e.g., console.error or an injected logger) with the error, and set any local
error/sync flags or retry behavior so failed writes don't fail silently.
---
Duplicate comments:
In `@src/features/App/useVaultOpener.tsx`:
- Around line 37-46: The unencrypted branch can throw from
profiles.openProfile({ profile }, true) and never clear the loading flag; wrap
that call in a try/catch (or try/finally) inside the async handler in
useVaultOpener.tsx so setIsProfileOpening(false) is always executed (use finally
to always call setIsProfileOpening(false), and in the catch log or return an
error status), referencing the async opener function, profiles.openProfile, and
the setIsProfileOpening state setter.
- Around line 68-87: The effect currently captures stale values because it omits
profilesList.profiles and recentProfile.profileId (and the mutators) from its
dependency list; update the effect so it lists all referenced values
(profilesList.profiles, recentProfile.profileId, setCurrentProfileId,
onOpenProfile, profilesList.isProfilesLoaded, recentProfile.isLoaded) and
prevent repeated runs by introducing a run-once guard ref (e.g., hasOpenedRef)
checked and set inside the effect; keep the existing logic (find profile, check
profile.encryption, call setCurrentProfileId and onOpenProfile) but rely on the
guard to preserve run-once semantics while using correct dependencies.
In `@src/features/App/VaultScreenManager.tsx`:
- Around line 63-68: The onOpenProfile result is being ignored (and even passed
to console.warn) in the create/select flows; update the handlers that call
profiles.createProfile(...).then(...) and the similar select path to return and
propagate the promise from onOpenProfile so failures surface to callers and UX
state instead of being swallowed—specifically change the inline arrow handlers
around profiles.createProfile and profiles.selectProfile to return
onOpenProfile(newProfile, profile.password ?? undefined) and attach proper
.catch handling (or rethrow) rather than calling console.warn; locate the
anonymous onCreateProfile/onSelectProfile callbacks that call
profiles.createProfile and profiles.selectProfile and replace the dangling
.then(console.warn) pattern with returned promise propagation and explicit error
handling.
In `@src/features/App/Workspace/services/useWorkspaceConfigSync.tsx`:
- Around line 39-41: The call to workspaceConfig.set(config) in
useWorkspaceConfigSync only handles success and leaves rejections unhandled;
wrap the write in explicit error handling by either awaiting it inside a
try/catch or appending a .catch(...) to workspaceConfig.set(config). In the
catch, log the error (e.g., console.error or processLogger.error) with
contextual text and, if applicable, update local state or surface an error to
the caller (e.g., setSyncError) so failed writes are not silently dropped.
In `@src/features/App/Workspace/useWorkspaceInitialization.tsx`:
- Around line 58-59: initWorkspace() currently lacks error handling and always
calls setIsWorkspaceReady(..., true) in the finally block, which can mark the
workspace ready even on failures; update initWorkspace (and the other similar
initializer around the 125-135 block) to wrap the async initialization in
try/catch, log or surface the caught error, setIsWorkspaceReady(false) on
failure (and only setIsWorkspaceReady(true) on successful completion), and avoid
unconditionally using finally to mark readiness—use finally only for cleanup and
ensure the ready flag is driven by success/failure paths.
- Around line 152-155: The onChange listener registered via
workspace.tagsRegistry.onChange currently calls workspace.tagsRegistry.getTags()
without handling rejections; update the listener in
useWorkspaceInitialization.tsx to await or attach a .catch to getTags() and
handle errors (e.g., log via processLogger or console.error and avoid dispatch
on failure) before calling dispatch(workspacesApi.setTags({ ...workspaceData,
tags })); ensure you reference workspace.tagsRegistry.onChange,
workspace.tagsRegistry.getTags, and the dispatch/workspacesApi.setTags call so
the fix surrounds the promise resolution and error path.
---
Nitpick comments:
In `@src/core/features/notes/controller/NotesController.test.ts`:
- Around line 406-409: The test currently only asserts the result length for
registry.getById([...notesId, notExistingNote]) which can hide wrong items;
update the assertion to verify the actual returned IDs and order: call
registry.getById with the combined array and assert the resolved result strictly
equals the expected notesId array (in order) and does not include
notExistingNote (e.g., use toEqual/strict equality on the IDs array and an
explicit check that notExistingNote is absent). Target the registry.getById call
and the notesId / notExistingNote variables when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea7a3b78-cf9d-4d36-bbee-395c37d2ad12
📒 Files selected for processing (18)
src/core/features/notes/controller/NotesController.test.tssrc/features/App/Profile/index.tsxsrc/features/App/Profile/services/useVaultConfigSync.tsxsrc/features/App/ProfileCreator/index.tsxsrc/features/App/ProfileLoginForm/index.tsxsrc/features/App/ProfilesForm.tsxsrc/features/App/VaultScreenManager.tsxsrc/features/App/Workspace/index.tsxsrc/features/App/Workspace/services/WorkspaceServices.tsxsrc/features/App/Workspace/services/useWorkspaceConfigSync.tsxsrc/features/App/Workspace/useWorkspaceInitialization.tsxsrc/features/App/Workspace/useWorkspaceState.tsxsrc/features/App/WorkspaceManager/index.tsxsrc/features/App/index.tsxsrc/features/App/useProfilesList.tssrc/features/App/useRecentProfile.tssrc/features/App/useVaultOpener.tsxsrc/state/redux/profiles/profiles.ts
💤 Files with no reviewable changes (1)
- src/features/App/WorkspaceManager/index.tsx
vitonsky
reviewed
Mar 4, 2026
Member
Why we hide the content? In case user should not to see a content, we should not to render it. Rendering with a "display: none" may leat us to bugs where some logic of components is in run, but initializes incorrect due to visibility problems. Also we may have bugs where some logic is in run in assertion that user interacts with UI, but they don't. |
katsyuta
force-pushed
the
173-remember-opened-notes
branch
from
642921a to
98b62eb
Compare
vitonsky
requested changes
Mar 8, 2026
katsyuta
force-pushed
the
173-remember-opened-notes
branch
from
a4583f1 to
fe75242
Compare
Collaborator
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
katsyuta
force-pushed
the
173-remember-opened-notes
branch
from
148f9b0 to
854e0e7
Compare
Collaborator
Author
|
@coderabbitai full review |
katsyuta
force-pushed
the
173-remember-opened-notes
branch
from
54e7770 to
b5c25a1
Compare
vitonsky
reviewed
Apr 10, 2026
vitonsky
approved these changes
Apr 10, 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
2 participants
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.
Closed #173
Changes:
App Restore Lifecycle
While the vault is opening, show Splash.
If an error occurs while opening the vault:
After the vault opens, show Splash and restore the vault state (workspace list, active workspace, vault config).
If an error occurs while restoring vault state:
Once vault state is restored, restore the workspace — the recently used one, or the default if none was recently used.
Splash still visible.
If an error occurs while restoring the workspace:
If workspace restore succeeds:
error-screen-remember.mp4
Workspace error screen
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements