feat: open the note in temporary tab - #269
Closed
katsyuta wants to merge 114 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a temporary tab system for notes. Notes now open as temporary tabs by default, which close automatically when another temporary note opens. Double-clicking a temporary tab or editing a note makes it permanent. The API now accepts an optional Changes
Sequence DiagramsequenceDiagram
actor User
participant NotesList
participant NoteEditor
participant Redux as Redux Store
participant OpenedNotesPanel
Note over User,OpenedNotesPanel: Single-click: Open as Temporary
User->>NotesList: Single-click note
NotesList->>Redux: noteActions.click(id, { isTemporary: true })
Redux->>Redux: Add to openedNotes, set openedNotesMeta[id]={isTemporary: true}
Redux->>OpenedNotesPanel: Update state
OpenedNotesPanel->>OpenedNotesPanel: Render tab (italic style)
Note over User,OpenedNotesPanel: Double-click or Edit: Convert to Permanent
User->>NoteEditor: Edit note content
NoteEditor->>Redux: workspaceActions.markNoteAsPermanent(noteId)
Redux->>Redux: Set openedNotesMeta[id]={isTemporary: false}
Redux->>OpenedNotesPanel: Update state
OpenedNotesPanel->>OpenedNotesPanel: Render tab (bold style)
Note over User,OpenedNotesPanel: New Temporary Note: Close Previous Temporary
User->>NotesList: Single-click different note
NotesList->>Redux: noteActions.click(id2, { isTemporary: true })
Redux->>Redux: Mark id2 as temporary, remove previous temporary
Redux->>Redux: Clean up openedNotesMeta for closed temporary note
Redux->>OpenedNotesPanel: Update opened notes list
OpenedNotesPanel->>OpenedNotesPanel: Close previous tab, show new tab
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested Reviewers
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 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 |
katsyuta
force-pushed
the
251-feat-open-note-in-temporary-tab
branch
from
March 24, 2026 21:27
9f0ed16 to
c55c204
Compare
katsyuta
marked this pull request as ready for review
March 25, 2026 15:33
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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 (2)
src/hooks/notes/useNoteActions.ts (1)
47-51:⚠️ Potential issue | 🟠 MajorBug:
temporaryparameter is not forwarded toopenNotewhen opening a closed note.When
click()is called with{ temporary: false }on a note that is not yet open, line 49 callsopenNote(note)without passing thetemporaryflag. TheopenNotefunction defaults totemporary = true, so the note will incorrectly open as temporary despite the caller requesting persistent mode.This breaks the double-click-to-open-persistently flow for notes not already in tabs.
Proposed fix
} else { notesRegistry.getById([id]).then(([note]) => { - if (note) openNote(note); + if (note) openNote(note, true, temporary); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/notes/useNoteActions.ts` around lines 47 - 51, The callback handling closed notes in useNoteActions.click is not forwarding the temporary flag to openNote, so calls like click(id, { temporary: false }) end up opening the note as temporary; update the notesRegistry.getById([id]).then(([note]) => { if (note) openNote(note); }); path to pass the original temporary argument into openNote (e.g., openNote(note, temporary)) so openNote receives the correct persistence flag; ensure you reference the temporary parameter from the enclosing click scope and adjust the call site accordingly.src/features/App/Workspace/index.tsx (1)
98-119:⚠️ Potential issue | 🟠 Major
temporaryparameter not forwarded fromuseNoteActions.click()when opening a closed note.The
openNotecallback accepts atemporaryparameter, butuseNoteActions.tsline 49 callsopenNote(note)without forwarding it. When double-clicking a closed note (which passes{ temporary: false }), the parameter is lost and the note opens as temporary by default.Additionally, the
NotesApiinterface inWorkspaceProvider.tsxdeclaresopenNote: (note: INote, focus?: boolean) => voidbut the implementation adds atemporaryparameter. The interface needs updating to match.Proposed fix in `src/hooks/notes/useNoteActions.ts`
} else { notesRegistry.getById([id]).then(([note]) => { - if (note) openNote(note); + if (note) openNote(note, true, temporary); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/index.tsx` around lines 98 - 119, The openNote handler in Workspace (openNote: (note: INote, focus = true, temporary = true) => { ... }) accepts a temporary flag but useNoteActions.click() currently calls openNote(note) losing that flag; update useNoteActions.click (and any other callers) to call openNote(note, focus, temporary) so the temporary=false case is preserved when double-clicking a closed note, and also update the NotesApi interface declaration in WorkspaceProvider.tsx to match the implementation signature (openNote: (note: INote, focus?: boolean, temporary?: boolean) => void) so the types align with the concrete openNote function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/features/App/Workspace/index.tsx`:
- Around line 98-119: The openNote handler in Workspace (openNote: (note: INote,
focus = true, temporary = true) => { ... }) accepts a temporary flag but
useNoteActions.click() currently calls openNote(note) losing that flag; update
useNoteActions.click (and any other callers) to call openNote(note, focus,
temporary) so the temporary=false case is preserved when double-clicking a
closed note, and also update the NotesApi interface declaration in
WorkspaceProvider.tsx to match the implementation signature (openNote: (note:
INote, focus?: boolean, temporary?: boolean) => void) so the types align with
the concrete openNote function.
In `@src/hooks/notes/useNoteActions.ts`:
- Around line 47-51: The callback handling closed notes in useNoteActions.click
is not forwarding the temporary flag to openNote, so calls like click(id, {
temporary: false }) end up opening the note as temporary; update the
notesRegistry.getById([id]).then(([note]) => { if (note) openNote(note); });
path to pass the original temporary argument into openNote (e.g., openNote(note,
temporary)) so openNote receives the correct persistence flag; ensure you
reference the temporary parameter from the enclosing click scope and adjust the
call site accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f57835e-4be7-401b-9546-a07bfe379da8
📒 Files selected for processing (7)
src/features/App/Workspace/index.tsxsrc/features/MainScreen/NotesListPanel/NotesList.tsxsrc/features/NotesContainer/OpenedNotesPanel.tsxsrc/features/NotesContainer/index.tsxsrc/hooks/notes/useNoteActions.tssrc/state/redux/profiles/profiles.tssrc/state/redux/profiles/selectors/notes.ts
katsyuta
marked this pull request as draft
March 25, 2026 15:43
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/hooks/notes/useNoteActions.ts (1)
32-50:⚠️ Potential issue | 🟠 MajorForward
temporaryparameter through the closed-note path.The
elsebranch at line 49 callsopenNote(note)without passing thetemporaryparameter from theclickfunction. This meansclick(id, { temporary: false })silently falls back to the default and opens the tab as temporary whenever the note is not yet open—breaking the new API contract.Suggested fix
Update the type signature in
src/features/App/Workspace/WorkspaceProvider.tsx:export type NotesApi = { openNote: (note: INote, focus?: boolean, temporary?: boolean) => void; noteUpdated: (note: INote) => void; noteClosed: (noteId: string) => void; };Update the call site in
src/hooks/notes/useNoteActions.ts:49:} else { notesRegistry.getById([id]).then(([note]) => { - if (note) openNote(note); + if (note) openNote(note, true, temporary); }); }Test case (PoC):
it('opens a closed note persistently when requested', async () => { const openNote = vi.fn(); mockUseNotesContext({ openNote, noteClosed: vi.fn() }); mockNotesRegistry({ getById: vi.fn().mockResolvedValue([note]), }); const { result } = renderHook(() => useNoteActions(), { wrapper }); await act(async () => { result.current.click(note.id, { temporary: false }); await Promise.resolve(); }); expect(openNote).toHaveBeenCalledWith(note, true, false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/notes/useNoteActions.ts` around lines 32 - 50, The click handler in useNoteActions.ts currently calls openNote(note) in the closed-note path without forwarding the temporary flag; update the NotesApi openNote signature to accept temporary (e.g., openNote(note: INote, focus?: boolean, temporary?: boolean)) and modify the call in useNoteActions.ts to pass the temporary parameter (e.g., openNote(note, true, temporary)) so click(id, { temporary: false }) opens closed notes persistently; ensure the workspace-related dispatchs in the opened-note path remain unchanged.
🤖 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/state/redux/profiles/profiles.ts`:
- Around line 313-329: The code in setTemporaryNote currently evicts the old
temporary tab whenever a non-null noteId is set, even if it's the same id;
change the condition so the old temporary note is removed from
workspace.openedNotes only when workspace.temporaryNoteId exists, the new noteId
is non-null, and the ids differ (i.e., workspace.temporaryNoteId !== noteId).
Update the if guard around the openedNotes filter in the setTemporaryNote
reducer to compare workspace.temporaryNoteId and noteId before filtering.
---
Outside diff comments:
In `@src/hooks/notes/useNoteActions.ts`:
- Around line 32-50: The click handler in useNoteActions.ts currently calls
openNote(note) in the closed-note path without forwarding the temporary flag;
update the NotesApi openNote signature to accept temporary (e.g., openNote(note:
INote, focus?: boolean, temporary?: boolean)) and modify the call in
useNoteActions.ts to pass the temporary parameter (e.g., openNote(note, true,
temporary)) so click(id, { temporary: false }) opens closed notes persistently;
ensure the workspace-related dispatchs in the opened-note path remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10ed4e25-9d56-482d-9f6f-5f017d99cb0a
📒 Files selected for processing (7)
src/features/App/Workspace/index.tsxsrc/features/MainScreen/NotesListPanel/NotesList.tsxsrc/features/NotesContainer/OpenedNotesPanel.tsxsrc/features/NotesContainer/index.tsxsrc/hooks/notes/useNoteActions.tssrc/state/redux/profiles/profiles.tssrc/state/redux/profiles/selectors/notes.ts
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
katsyuta
marked this pull request as ready for review
March 25, 2026 20:55
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/hooks/notes/useNoteActions.ts`:
- Around line 39-45: When handling a click with temporary === false, only clear
the workspace temporary note if the clicked note is the workspace's current
temporary note; change the dispatch block that calls
workspacesApi.setTemporaryNote({...workspaceData, noteId: null}) so it first
checks that workspaceData.noteId === workspace.temporaryNoteId (or the
equivalent variable holding the workspace's temporaryNoteId) before dispatching;
i.e., only call workspacesApi.setTemporaryNote to clear noteId when the clicked
note id matches the workspace temporaryNoteId.
- Around line 48-49: The code calls notesRegistry.getById(...).then(([note]) =>
{ if (note) openNote(note); } ) but decides whether the note should be
persistent before the async resolution, so a second (persistent) click can race
and be lost; inside the .then callback re-check current open state and the
intended persistence flag (read from the same store/dispatch used by the click
handler) and then either convert an already-open temporary note to persistent
(e.g. dispatch the conversion action) or call openNote(note, { temporary }) with
the correct temporary value; update the logic in the click handler /
notesRegistry.getById.then block that references getById, openNote, and any
dispatch/workspacesApi.setTemporaryNote calls to consult current state before
acting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b23f3dfd-0301-42a3-ad1a-6fc14f640141
📒 Files selected for processing (8)
src/features/App/Workspace/WorkspaceProvider.tsxsrc/features/App/Workspace/index.tsxsrc/features/MainScreen/NotesListPanel/NotesList.tsxsrc/features/NotesContainer/OpenedNotesPanel.tsxsrc/features/NotesContainer/index.tsxsrc/hooks/notes/useNoteActions.tssrc/state/redux/profiles/profiles.tssrc/state/redux/profiles/selectors/notes.ts
katsyuta
changed the title
katsyuta
marked this pull request as draft
March 25, 2026 22:50
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)
src/features/App/Workspace/index.tsx (1)
97-124:⚠️ Potential issue | 🔴 Critical
temporary: falseis dropped for unopened notes.it('opens an unopened note persistently when requested', async () => { await click(note.id, { temporary: false }); expect(openNote).toHaveBeenCalledWith(note, { temporary: false }); });
src/hooks/notes/useNoteActions.ts:31-54still callsopenNote(note)in the unopened-note branch. With the new default here (temporary = true), that path opens a temporary tab instead of a persistent one, so the double-click/open-persistently flow still breaks for notes that are not already open. The unopened branch should forward the flag:openNote(note, { temporary }).Based on learnings: When identifying a potential bug in TypeScript/TSX files, include a minimal reproducing unit test (PoC) before the explanation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/index.tsx` around lines 97 - 124, The openNote handler now defaults temporary=true, but the unopened-note branch in useNoteActions still calls openNote(note) so requests like openNote(note, { temporary: false }) get dropped; update the unopened-note branch in src/hooks/notes/useNoteActions (the code that currently calls openNote(note) when the note isn't open) to forward the flag by calling openNote(note, { temporary }) (and include focus if that branch tracks it) so the temporary=false intent is preserved when opening unopened notes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/features/App/Workspace/index.tsx`:
- Around line 97-124: The openNote handler now defaults temporary=true, but the
unopened-note branch in useNoteActions still calls openNote(note) so requests
like openNote(note, { temporary: false }) get dropped; update the unopened-note
branch in src/hooks/notes/useNoteActions (the code that currently calls
openNote(note) when the note isn't open) to forward the flag by calling
openNote(note, { temporary }) (and include focus if that branch tracks it) so
the temporary=false intent is preserved when opening unopened notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b2ecb1c-25de-4f31-82e9-d8efca8c9aef
📒 Files selected for processing (2)
src/features/App/Workspace/WorkspaceProvider.tsxsrc/features/App/Workspace/index.tsx
Member
|
@katsyuta plese describe the specification how this feature does work. I want to know when transition between temporary and persistent tab does toggle. |
katsyuta
force-pushed
the
251-feat-open-note-in-temporary-tab
branch
from
April 14, 2026 15:02
348c398 to
6e48400
Compare
Collaborator
Author
Ok! i described it on pull request definition |
katsyuta
marked this pull request as ready for review
April 15, 2026 20:38
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/notes/useNoteActions.ts (1)
47-51:⚠️ Potential issue | 🟠 MajorRace condition remains when rapid clicks cause overlapping async fetches.
As flagged in the previous review, when a note is not yet opened, the "closed" path doesn't re-check state after
getByIdresolves. If user single-clicks (temporary) then quickly double-clicks (persistent):
- First click →
getByIdstarts withisTemporary=true- Second click →
getByIdstarts withisTemporary=false- Whichever resolves last wins, potentially leaving the note in the wrong state
The fix from the past review comment (re-checking open state inside
.then()) would address this.🐛 Proposed fix
} else { notesRegistry.getById([id]).then(([note]) => { - if (note) openNote(note, { isTemporary }); + if (!note) return; + + // Re-check state after async fetch + const currentWorkspace = selectWorkspace(workspaceData)(store.getState()); + const isOpenedNow = selectIsNoteOpened(id)(currentWorkspace); + + if (isOpenedNow) { + // Note was opened by a concurrent click - just focus it + dispatch(workspaceActions.setActiveNote({ noteId: id })); + // Convert to persistent if this was a persistent-open intent + if (!isTemporary && currentWorkspace?.temporaryNoteId === id) { + dispatch(workspaceActions.replaceTemporaryNote({ noteId: null })); + } + return; + } + + openNote(note, { isTemporary }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/notes/useNoteActions.ts` around lines 47 - 51, Re-check the current open state before calling openNote inside the notesRegistry.getById promise to avoid the race: after notesRegistry.getById([id]).then(([note]) => { ... }), verify the note is still not open (or that the desired isTemporary state is still applicable) using the same state check you use for the synchronous path, and only call openNote(note, { isTemporary }) if that re-check passes; use the existing identifiers notesRegistry.getById, openNote, id and isTemporary to locate and implement the guard inside the .then() callback.
🧹 Nitpick comments (1)
src/features/App/Workspace/WorkspaceStateInitializer.tsx (1)
89-95: Consider validating that the temporary note exists in opened notes.The restoration dispatches
replaceTemporaryNotewithout verifying thatstate.temporaryNodeIdcorresponds to a note inopenedNoteList. If the persisted state is inconsistent (e.g.,temporaryNodeIdreferences a deleted note), a stale ID would be set.💡 Optional defensive check
// Restore the temporarily opened note + const temporaryNoteExists = openedNoteList.some( + (n) => n.id === state.temporaryNodeId, + ); dispatch( workspaceActions.replaceTemporaryNote({ - noteId: state.temporaryNodeId, + noteId: temporaryNoteExists ? state.temporaryNodeId : null, }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/Workspace/WorkspaceStateInitializer.tsx` around lines 89 - 95, Before calling dispatch(workspaceActions.replaceTemporaryNote({ noteId: state.temporaryNodeId })), validate that state.temporaryNodeId exists in the openedNoteList: check openedNoteList.some(n => n.id === state.temporaryNodeId) and only dispatch replaceTemporaryNote when true; otherwise either skip dispatch or clear the temporaryNodeId via the appropriate action. Update the logic inside WorkspaceStateInitializer (where replaceTemporaryNote is invoked) to perform this defensive existence check and handle the stale ID path.
🤖 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/services/workspaceState.ts`:
- Line 7: The schema field temporaryNodeId in workspaceState.ts is a typo and
should be temporaryNoteId to match the rest of the codebase; rename the field in
the schema (temporaryNodeId → temporaryNoteId) and update all usages/mappings
(notably in selectWorkspaceState.ts where the mapping occurs and in
WorkspaceStateInitializer.tsx around the referenced initialization) so the
property name is consistent across WorkspaceState, selectors, and initializers;
ensure any tests or type imports referencing temporaryNodeId are updated to
temporaryNoteId as well.
---
Duplicate comments:
In `@src/hooks/notes/useNoteActions.ts`:
- Around line 47-51: Re-check the current open state before calling openNote
inside the notesRegistry.getById promise to avoid the race: after
notesRegistry.getById([id]).then(([note]) => { ... }), verify the note is still
not open (or that the desired isTemporary state is still applicable) using the
same state check you use for the synchronous path, and only call openNote(note,
{ isTemporary }) if that re-check passes; use the existing identifiers
notesRegistry.getById, openNote, id and isTemporary to locate and implement the
guard inside the .then() callback.
---
Nitpick comments:
In `@src/features/App/Workspace/WorkspaceStateInitializer.tsx`:
- Around line 89-95: Before calling
dispatch(workspaceActions.replaceTemporaryNote({ noteId: state.temporaryNodeId
})), validate that state.temporaryNodeId exists in the openedNoteList: check
openedNoteList.some(n => n.id === state.temporaryNodeId) and only dispatch
replaceTemporaryNote when true; otherwise either skip dispatch or clear the
temporaryNodeId via the appropriate action. Update the logic inside
WorkspaceStateInitializer (where replaceTemporaryNote is invoked) to perform
this defensive existence check and handle the stale ID path.
🪄 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: 5b20aa79-48ea-4032-9e4b-c48878de007f
📒 Files selected for processing (11)
src/features/App/Workspace/WorkspaceProvider.tsxsrc/features/App/Workspace/WorkspaceStateInitializer.tsxsrc/features/App/Workspace/index.tsxsrc/features/App/Workspace/services/workspaceState.tssrc/features/MainScreen/NotesListPanel/NotesList.tsxsrc/features/NotesContainer/OpenedNotesPanel.tsxsrc/features/NotesContainer/index.tsxsrc/hooks/notes/useNoteActions.tssrc/state/redux/profiles/profiles.tssrc/state/redux/profiles/selectors/notes.tssrc/state/redux/profiles/selectors/selectWorkspaceState.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/features/MainScreen/NotesListPanel/NotesList.tsx
- src/features/App/Workspace/WorkspaceProvider.tsx
- src/features/NotesContainer/index.tsx
- src/state/redux/profiles/profiles.ts
vitonsky
reviewed
Apr 16, 2026
katsyuta
force-pushed
the
251-feat-open-note-in-temporary-tab
branch
from
April 16, 2026 14:24
f2e4a57 to
b4c28b0
Compare
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 (1)
src/features/MainScreen/NotesListPanel/NotesList.tsx (1)
151-170:⚠️ Potential issue | 🟠 MajorAvoid opening a temporary tab before the double-click persistent open.
PoC:
it('opens persistently on double click without first dispatching a temporary open', () => { const click = vi.fn(); render(<NotePreview onClick={(event) => { if (event.detail === 1) click('note-b'); else click('note-b', { isTemporary: false }); }} />); fireEvent.click(screen.getByRole('button'), { detail: 1 }); fireEvent.click(screen.getByRole('button'), { detail: 2 }); expect(click).toHaveBeenCalledTimes(1); expect(click).toHaveBeenCalledWith('note-b', { isTemporary: false }); });This fails with the current handler because a double-click runs the
detail === 1branch first. A double-click fires two separate click events—first withdetail === 1, then withdetail === 2—so both the temporary and persistent opens execute, and telemetry double-counts the event.🐛 Proposed direction
+const singleClickTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + +useEffect(() => { + return () => { + if (singleClickTimerRef.current) { + clearTimeout(singleClickTimerRef.current); + } + }; +}, []); + ... - onClick={(e) => { - const isSingleClick = e.detail === 1; - - if (isSingleClick) { - // Single click - open note temporarily - noteActions.click(note.id); - } else { - // Double click - open note persistently - noteActions.click(note.id, { - isTemporary: false, - }); - } - - telemetry.track( - TELEMETRY_EVENT_NAME.NOTE_OPENED, - { - context: 'notes list', - }, - ); - }} + onClick={() => { + singleClickTimerRef.current = setTimeout(() => { + noteActions.click(note.id); + telemetry.track(TELEMETRY_EVENT_NAME.NOTE_OPENED, { + context: 'notes list', + }); + singleClickTimerRef.current = null; + }, 200); + }} + onDoubleClick={() => { + if (singleClickTimerRef.current) { + clearTimeout(singleClickTimerRef.current); + singleClickTimerRef.current = null; + } + + noteActions.click(note.id, { + isTemporary: false, + }); + telemetry.track(TELEMETRY_EVENT_NAME.NOTE_OPENED, { + context: 'notes list', + }); + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/MainScreen/NotesListPanel/NotesList.tsx` around lines 151 - 170, The click handler currently fires a temporary open on the first click event and then a persistent open on the second, causing both opens and duplicate telemetry; change to separate single- and double-click handling: implement an onDoubleClick handler that immediately calls noteActions.click(note.id, { isTemporary: false }) and tracks TELEMETRY_EVENT_NAME.NOTE_OPENED with context 'notes list', and make the onClick handler delay the single-click action (noteActions.click(note.id) + telemetry) using a short timeout so it can be cancelled if onDoubleClick runs; ensure the timeout is cleared/cancelled when double-click occurs so only the persistent open and a single telemetry event are emitted (update the component in NotesList.tsx where the current onClick, noteActions.click, and telemetry.track are used).
♻️ Duplicate comments (1)
src/hooks/notes/useNoteActions.ts (1)
55-56:⚠️ Potential issue | 🟠 MajorRe-check open state after the async note lookup resolves.
it('keeps a double-clicked note persistent when the first temporary lookup resolves last', async () => { const note = { id: 'n1' } as INote; let resolveTemporary!: (value: [INote]) => void; let resolvePersistent!: (value: [INote]) => void; notesRegistry.getById .mockReturnValueOnce(new Promise((resolve) => { resolveTemporary = resolve as typeof resolveTemporary; })) .mockReturnValueOnce(new Promise((resolve) => { resolvePersistent = resolve as typeof resolvePersistent; })); result.current.click(note.id); result.current.click(note.id, { isTemporary: false }); resolvePersistent([note]); await flushPromises(); resolveTemporary([note]); await flushPromises(); expect(selectIsNoteTemporary(note.id)(selectWorkspace(scope)(store.getState()))).toBe(false); });The
.then()callback can run after a newer persistent-open intent. If the older temporary request resolves last,openNote(note, { isTemporary: true })can downgrade the already-open persistent tab back to temporary.🐛 Proposed fix
} else { notesRegistry.getById([id]).then(([note]) => { - if (note) openNote(note, { isTemporary }); + if (!note) return; + + const nextWorkspace = selectWorkspace(workspaceData)(store.getState()); + const isNoteOpenedNow = selectIsNoteOpened(id)(nextWorkspace); + + if (isNoteOpenedNow) { + dispatch(workspaceActions.setActiveNote({ noteId: id })); + + if (!isTemporary && selectIsNoteTemporary(id)(nextWorkspace)) { + dispatch( + workspaceActions.setNoteTemporaryState({ + noteId: id, + isTemporary: false, + }), + ); + } + return; + } + + openNote(note, { isTemporary }); }); }Based on learnings, TypeScript/TSX bug reports in this repo should start with a minimal reproducing unit test before any explanation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/notes/useNoteActions.ts` around lines 55 - 56, The async callback from notesRegistry.getById currently calls openNote(note, { isTemporary }) unconditionally, which allows an older temporary lookup to overwrite a newer persistent intent; update the then() handler to re-check the current desired open state before calling openNote — e.g., query the same source the click/open flow uses (the open-intent or workspace selector such as selectIsNoteTemporary or the in-memory open state) and only call openNote if the resolved isTemporary matches the current intent/state for that note id (or skip if the note is already open as persistent); references: notesRegistry.getById, openNote, and the click/open intent handling code.
🤖 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/services/workspaceState.ts`:
- Line 7: The WorkspaceState schema requires the new temporaryNotesId field
which breaks parsing older persisted states; update the schema entry for
temporaryNotesId (in the object that defines WorkspaceStateScheme) to provide a
default empty array so missing values parse correctly, e.g. change
temporaryNotesId: z.array(z.string()) to use a Zod default
(z.array(z.string()).default([]) or .optional().default([])) so
WorkspaceStateScheme.parse(previousState) yields [] for older state files.
In `@src/state/redux/profiles/profiles.ts`:
- Around line 394-410: When evicting previous temporary notes inside the
setNoteTemporaryState handling (the block that builds previousTemporaryIds from
workspace.openedNotesMeta), ensure workspace.activeNote doesn't remain pointing
at an evicted tab: after you remove previousTemporaryIds from
workspace.openedNotes, check if workspace.activeNote is included in
previousTemporaryIds and, if so, set workspace.activeNote = noteId (the id being
made temporary) so the active note moves to the newly temporary note; if you
prefer a fallback when noteId isn't present in openedNotes, set activeNote to
the first remaining openedNotes[0]?.id or null. This change touches the
previousTemporaryIds variable, workspace.openedNotes, and workspace.activeNote
in the setNoteTemporaryState logic.
---
Outside diff comments:
In `@src/features/MainScreen/NotesListPanel/NotesList.tsx`:
- Around line 151-170: The click handler currently fires a temporary open on the
first click event and then a persistent open on the second, causing both opens
and duplicate telemetry; change to separate single- and double-click handling:
implement an onDoubleClick handler that immediately calls
noteActions.click(note.id, { isTemporary: false }) and tracks
TELEMETRY_EVENT_NAME.NOTE_OPENED with context 'notes list', and make the onClick
handler delay the single-click action (noteActions.click(note.id) + telemetry)
using a short timeout so it can be cancelled if onDoubleClick runs; ensure the
timeout is cleared/cancelled when double-click occurs so only the persistent
open and a single telemetry event are emitted (update the component in
NotesList.tsx where the current onClick, noteActions.click, and telemetry.track
are used).
---
Duplicate comments:
In `@src/hooks/notes/useNoteActions.ts`:
- Around line 55-56: The async callback from notesRegistry.getById currently
calls openNote(note, { isTemporary }) unconditionally, which allows an older
temporary lookup to overwrite a newer persistent intent; update the then()
handler to re-check the current desired open state before calling openNote —
e.g., query the same source the click/open flow uses (the open-intent or
workspace selector such as selectIsNoteTemporary or the in-memory open state)
and only call openNote if the resolved isTemporary matches the current
intent/state for that note id (or skip if the note is already open as
persistent); references: notesRegistry.getById, openNote, and the click/open
intent handling code.
🪄 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: 27f4b785-9038-4f38-8133-aacac84e44d0
📒 Files selected for processing (13)
src/features/App/Workspace/WorkspaceProvider.tsxsrc/features/App/Workspace/WorkspaceStateInitializer.tsxsrc/features/App/Workspace/index.tsxsrc/features/App/Workspace/services/workspaceState.tssrc/features/MainScreen/NotesListPanel/NotesList.tsxsrc/features/NoteEditor/index.tsxsrc/features/NotesContainer/OpenedNotesPanel.tsxsrc/features/NotesContainer/index.tsxsrc/hooks/notes/useNoteActions.tssrc/hooks/notes/useNotesShortcutActions.tssrc/state/redux/profiles/profiles.tssrc/state/redux/profiles/selectors/notes.tssrc/state/redux/profiles/selectors/selectWorkspaceState.ts
vitonsky
requested changes
May 15, 2026
vitonsky
requested changes
May 17, 2026
| const [title, setTitle] = useState(note.content.title); | ||
| const [text, setText] = useState(note.content.text); | ||
|
|
||
| useMakePreviewTabRegular(note.id, [text, title]); |
Member
There was a problem hiding this comment.
Improve hook name. I would use something like useTogglePreviewTabToRegularOnChange.
Also it is important to understand here how this code will work when we will implement synchronization and the external changes will occurs.
In case any external changes applies to the note, it must not to make tab regular.
What protection is provided?
Collaborator
Author
There was a problem hiding this comment.
In the future we can add a ref flag that indicates whether the current change is programmatic (e.g. from synchronization). Pass this ref to the hook useTogglePreviewTabToRegularOnChange and do not toggle the note to regular in this case.
vitonsky
requested changes
May 18, 2026
| [dispatch, notesRegistry, openNote, store, workspaceData], | ||
| ); | ||
|
|
||
| const previewTabId = useWorkspaceSelector(selectPreviewTabId); |
Member
There was a problem hiding this comment.
Selector value can be stale on moment of event handling.
That's why we inline the selectors in hook above
| ); | ||
|
|
||
| const previewTabId = useWorkspaceSelector(selectPreviewTabId); | ||
| const doubleClick = useCallback( |
Member
There was a problem hiding this comment.
Why dedicated callback is needed?
| // Ignore if the note is not preview | ||
| if (previewTabId !== id) return; | ||
|
|
||
| dispatch(workspacesApi.togglePreviewTabToRegular({ ...workspaceData })); |
Member
There was a problem hiding this comment.
In case a note is not opened yet - we will do nothing here
Member
|
I've done that PR in #304 so now I close that version |
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
To implement the requirement "When a user opens another temporary tab, the existing temporary tab must be closed" from issue, a structure with a separate field
temporaryNoteId: NoteId | nullis used.This explicitly encodes the that there can be at most one temporary tab, eliminates unnecessary filtering, and makes the intent of the state easier to understand when reading the store.
Transition between temporary and persistent modes:
tem-note.mp4
Summary by CodeRabbit
Release Notes