feat: open notes in temporary tab#304
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements a preview tab feature allowing users to open notes temporarily. Notes open in preview mode by single-clicking (or via single-click in lists), automatically close when another preview tab opens, and promote to permanent tabs when edited or double-clicked. The Redux store is extended with ChangesPreview Tab Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ 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: 5
🤖 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/features/NoteEditor/useTogglePreviewTabToRegularOnChange.ts`:
- Around line 13-33: The hook useTogglePreviewTabToRegularOnChange currently
triggers promotion whenever any item in deps changes, allowing non-user updates
to promote preview tabs; change its signature to accept an explicit boolean flag
(e.g., shouldPromote) that callers set only from editor input handlers, then add
a guard at the top of the effect to return unless shouldPromote is true and
previewTabId === noteId, and keep deps limited to [shouldPromote, previewTabId,
noteId, workspaceData] so only user-originated edits (when callers pass
shouldPromote=true) can trigger
dispatch(workspacesApi.togglePreviewTabToRegular({ ...workspaceData })).
In `@packages/app/src/features/NotesContainer/OpenedNotesPanel.tsx`:
- Line 124: The double-click handler currently always calls onPick(note.id, {
preview: false }) which promotes every tab; change the handler in
OpenedNotesPanel to first check isPreviewTab (for the note/tab) and only call
onPick(note.id, { preview: false }) when isPreviewTab is true, otherwise do
nothing (or keep existing behavior for regular tabs). Update the onDoubleClick
invocation to conditionally call onPick based on isPreviewTab to avoid
unnecessary state updates and telemetry for non-preview tabs.
In `@packages/app/src/hooks/notes/useNoteActions.ts`:
- Around line 39-50: The async open flow can apply stale preview intent when
multiple rapid clicks resolve out of order; update the handler around
notesRegistry.getById([noteId]) to track the latest intent per noteId (e.g.,
store a pendingPromises map or a sequence token keyed by noteId) so that when
the promise resolves you verify the resolved result matches the most recent
requested intent before calling openNote(note, { preview }); also add rejection
handling for getById() (log or surface the error) so failures don’t silently
drop; reference notesRegistry.getById, noteId, preview, and openNote when
implementing the map/token and error handling.
In `@packages/app/src/state/redux/vaults/vaults.ts`:
- Around line 374-388: When replacing a preview tab in the vault reducer, ensure
workspace.activeNote doesn't end up pointing to a removed note: in the block
handling isPreview (where workspace.previewTabId and workspace.openedNotes are
updated), after removing the previous preview note check if
workspace.previewTabId === workspace.activeNote and, if so, set
workspace.activeNote to the new note.id (or clear it if you prefer) so
activeNote remains valid regardless of the isActive flag; update the logic
around workspace.previewTabId, workspace.openedNotes, and workspace.activeNote
to perform this fix.
- Around line 537-557: The reducer handling the setOpenedNotesState payload can
crash when notes is empty because it does notes[0].id; update the logic in the
setOpenedNotesState handler (the function that processes the PayloadAction with
WorkspaceScoped<{ notes: INote[]; activeNoteId: NoteId; previewTabId: NoteId |
null }>) to first check if notes.length === 0 and, in that case, set
workspace.openedNotes = [] and set workspace.activeNote (and
workspace.previewTab if applicable) to a safe value (null/undefined or leave
unchanged per your type constraints), then return early; otherwise proceed with
the existing Set lookup and fallback to notes[0].id as before.
🪄 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: f6ea3c1e-93bc-4bec-aa3b-8d62e2e20025
📒 Files selected for processing (16)
packages/app/src/features/App/Workspace/WorkspaceProvider.tsxpackages/app/src/features/App/Workspace/WorkspaceStateInitializer.tsxpackages/app/src/features/App/Workspace/index.tsxpackages/app/src/features/App/Workspace/services/workspaceState.tspackages/app/src/features/MainScreen/NotesListPanel/NotesList.tsxpackages/app/src/features/NoteEditor/index.tsxpackages/app/src/features/NoteEditor/useTogglePreviewTabToRegularOnChange.tspackages/app/src/features/NotesContainer/OpenedNotesPanel.tsxpackages/app/src/features/NotesContainer/index.tsxpackages/app/src/hooks/notes/useCreateNote.tspackages/app/src/hooks/notes/useNoteActions.tspackages/app/src/hooks/notes/useNotesShortcutActions.tspackages/app/src/state/redux/vaults/__tests__/redux.vault.activeNote.test.tspackages/app/src/state/redux/vaults/selectors/notes.tspackages/app/src/state/redux/vaults/selectors/selectWorkspaceState.tspackages/app/src/state/redux/vaults/vaults.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/state/redux/vaults/__tests__/redux.vault.activeNote.test.ts (1)
112-144: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winIncomplete test coverage: missing previewTabId state verification.
The test sets
previewTabId: '3'in the initial state and dispatchesaddOpenedNotewithisPreview: true, but does not verify thatpreviewTabIdis correctly updated to'6'after the action. Since the test explicitly exercises preview tab behavior, it should verify the complete preview state.🧪 Proposed fix to add preview state verification
First, import the selector:
import { selectActiveNoteId, selectOpenedNotes, + selectPreviewTabId, selectRecentlyClosedNotes, workspacesApi, } from '../vaults';Then add an assertion after line 143:
expect( selectOpenedNotes(selectors.workspace()).map((note) => note.id), ).toStrictEqual(['1', '2', '4', '5', '6']); + expect(selectPreviewTabId(selectors.workspace())).toStrictEqual('6'); });🤖 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/state/redux/vaults/__tests__/redux.vault.activeNote.test.ts` around lines 112 - 144, The test "Active note must be updated by close preview tab" is missing verification that the preview tab state is updated when adding a preview note; import and use the selector selectPreviewTabId and add an assertion after the existing expectations to assert selectPreviewTabId(selectors.workspace()) === '6' (this complements the call to workspacesApi.addOpenedNote with isPreview: true and ensures previewTabId is updated).
♻️ Duplicate comments (1)
packages/app/src/state/redux/vaults/vaults.ts (1)
539-568: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winImprove type safety: allow
activeNoteIdto be nullable.The past review concern about
notes[0].idcrashing has been fixed with optional chaining at Line 562. However, the payload type declaresactiveNoteId: NoteId(non-nullable), but the implementation treats it as potentially invalid by checkingnoteIdSet.has(activeNoteId)and falling back tonotes[0]?.id ?? null. This mismatch can mislead callers about the contract.📝 Proposed type fix
setOpenedNotesState: ( state, { payload: { vaultId, workspaceId, notes, activeNoteId, previewTabId }, }: PayloadAction< WorkspaceScoped<{ notes: INote[]; - activeNoteId: NoteId; + activeNoteId: NoteId | null; previewTabId: NoteId | null; }> >, ) => { const workspace = selectWorkspaceObject(state, { vaultId, workspaceId }); if (!workspace) return; workspace.openedNotes = notes; // Use Set for O(1) lookups const noteIdSet = new Set(notes.map(({ id }) => id)); // Fall back to first note if active note not in list - workspace.activeNote = noteIdSet.has(activeNoteId) + workspace.activeNote = activeNoteId !== null && noteIdSet.has(activeNoteId) ? activeNoteId : (notes[0]?.id ?? null); workspace.previewTabId = previewTabId !== null && noteIdSet.has(previewTabId) ? previewTabId : null; },🤖 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/state/redux/vaults/vaults.ts` around lines 539 - 568, The payload type for setOpenedNotesState is wrong: it declares activeNoteId as non-nullable but the reducer treats it as possibly missing/invalid; update the PayloadAction generic so activeNoteId is NoteId | null (and keep previewTabId as NoteId | null), i.e., change the WorkspaceScoped payload shape referenced in setOpenedNotesState to allow activeNoteId to be nullable (adjust any related types like WorkspaceScoped and consumers if necessary) so the runtime fallback logic using notes[0]?.id ?? null matches the TypeScript contract for setOpenedNotesState, activeNoteId, and previewTabId.
🤖 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/state/redux/vaults/__tests__/redux.vault.activeNote.test.ts`:
- Around line 112-144: The test "Active note must be updated by close preview
tab" is missing verification that the preview tab state is updated when adding a
preview note; import and use the selector selectPreviewTabId and add an
assertion after the existing expectations to assert
selectPreviewTabId(selectors.workspace()) === '6' (this complements the call to
workspacesApi.addOpenedNote with isPreview: true and ensures previewTabId is
updated).
---
Duplicate comments:
In `@packages/app/src/state/redux/vaults/vaults.ts`:
- Around line 539-568: The payload type for setOpenedNotesState is wrong: it
declares activeNoteId as non-nullable but the reducer treats it as possibly
missing/invalid; update the PayloadAction generic so activeNoteId is NoteId |
null (and keep previewTabId as NoteId | null), i.e., change the WorkspaceScoped
payload shape referenced in setOpenedNotesState to allow activeNoteId to be
nullable (adjust any related types like WorkspaceScoped and consumers if
necessary) so the runtime fallback logic using notes[0]?.id ?? null matches the
TypeScript contract for setOpenedNotesState, activeNoteId, and previewTabId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1819ffcf-a967-4d0a-a7a2-26da4d5552bf
📒 Files selected for processing (3)
packages/app/src/features/NotesContainer/OpenedNotesPanel.tsxpackages/app/src/state/redux/vaults/__tests__/redux.vault.activeNote.test.tspackages/app/src/state/redux/vaults/vaults.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app/src/features/NotesContainer/OpenedNotesPanel.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
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.
Closes #251
Related to #269
In this PR I just to polish the code.
Summary by CodeRabbit
New Features
Behavior Changes