Add tabs to the desktop editor (#240) - #265
Conversation
Several notes can be open at once as tabs, with one visible at a time. Each tab keeps its own back/forward trail and its own scroll position. A tab records where a note is, not what it holds: its path and its trail. The open document stays singular — one file loaded, one editor, one watcher — and activating a tab saves the current note and loads the tab's note, which is what clicking a sidebar row already did. Per-tab document buffers were considered and rejected: the single-document assumption is load-bearing in reads, not only writes, so savePathContent, the auto-title liveness check, and updateMovedLinks would all quietly come to mean "the focused document" while still compiling. Opening a note explicitly, from the sidebar or the palette, gives it a tab or focuses the one already showing it. Following a link stays in the current tab, which is what its trail is for. Leaving a note now goes through one place that saves it, records where the user was, and refuses when the file has changed on disk. That also fixes a standing bug: typing without pausing and immediately opening another note dropped the last edit, because the editor's unmount flush ran after the store had moved on. Tabs are session state and are not persisted, so relaunch restores the last opened note as a single tab. Not included, and left for follow-ups: binding a terminal session to a tab, split view, preview tabs, drag to reorder, and preserving undo across a tab switch.
|
@MonisMS is attempting to deploy a commit to the bholmesdev's projects Team on Vercel. A member of the Team first needs to authorize it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Adds desktop editor tabs as path+trail session state (no per-tab buffers), with save-before-leave, conflict refusal on every exit, per-tab history, path-keyed scroll memory, TabStrip UI, and store/component tests. Core architecture matches the stated single-document design and the PR description.
Concerns
- PRODUCT.md drifts from the code and from TECH/PR intent on strip visibility, what creates a new tab vs replaces the active one, and several shortcuts/open gestures.
closeTabcan navigate after a stalewasActivesnapshot if the active tab changes duringleaveCurrentDocument's await.- TabStrip drops all tab stops when
activeTabIdis null (changelog overlay), so the strip is not keyboard-reachable in that state. - Active-tab
scrollIntoViewon overflow is specified but not implemented. - At least one test comment narrates branch process ("step 4") against simplify comment rules.
Verdict
Found: 0 critical, 3 important, 2 suggestions
Request changes
Reviewed by a Warp Factory agent.
| 1. With a Workspace Folder or Plain Folder open, the tab strip appears above the editor whenever more than one Markdown File is open. | ||
| 2. Opening a file — sidebar, wiki link, relative link, `CmdOrCtrl+P`, file picker, or creating a new file — replaces the Active Tab, exactly as today. |
There was a problem hiding this comment.
Behavior 1/19 say the strip appears only with >1 note and is hidden for single-note use. Code always shows the strip from the first note (TabStrip + tests).
Behavior 2 says sidebar / CmdOrCtrl+P / file picker replace the Active Tab. Code routes sidebar and palette through openTabForPath, which mints or focuses a tab; only in-note navigation stays on loadPath.
Behavior 3's CmdOrCtrl+T, Cmd-click / middle-click background open, and Behavior 6's CmdOrCtrl+1–9 are unspecified in the shipped commands.
Update PRODUCT to the intentional model (or implement the missing behaviors) so the checked-in spec matches the PR.
| const wasActive = tabs.activeTabId === id; | ||
| const next = wasActive ? nextActiveTabId(tabs, id) : null; | ||
|
|
||
| if (wasActive && !(await leaveCurrentDocument())) return; | ||
| dropHistory(id); | ||
| appStore.set((state) => ({ ...state, tabs: withClosedTab(state.tabs, id) })); | ||
|
|
||
| if (!wasActive) return; | ||
| const nextPath = next ? tabsStore.get().byId[next]?.path : null; | ||
| if (nextPath && next) { | ||
| await loadPath(nextPath, { | ||
| history: "none", | ||
| launchExternal: false, | ||
| tab: next, | ||
| }); |
There was a problem hiding this comment.
wasActive / next are snapshotted before await leaveCurrentDocument(). If the user (or another close/activate) changes the Active Tab during that save, this path still treats the close as active-tab teardown and may loadPath a neighbour over the tab the user already switched to.
After the await, re-read tabs: only run the active-close navigation when id is still the active tab (or still owns currentPath). If the tab was closed elsewhere, return. If it is now background, close without loading.
| aria-selected={active} | ||
| // Roving focus: only the open note is a tab stop, so the strip | ||
| // costs one Tab press rather than one per note. | ||
| tabIndex={active ? 0 : -1} |
There was a problem hiding this comment.
activeTabId is null (changelog covers the editor), every tab gets tabIndex={-1}, so the strip has no tab stop and arrow/Delete handling bails (at < 0).
Roving tabindex needs a fallback focus target when nothing is selected, e.g. first tab or last-focused id:
| tabIndex={active ? 0 : -1} | |
| tabIndex={active || (activeTabId == null && tabs[0]?.id === tab.id) ? 0 : -1} |
| if (target !== null) { | ||
| event.preventDefault(); | ||
| onActivate(tabs[target].id); |
There was a problem hiding this comment.
💡 [SUGGESTION] PRODUCT Behavior 18: when the strip overflows, keep the Active Tab scrolled into view. overflow-x-auto alone does not. After activate (click or keyboard), scrollIntoView on the active tab button.
| // Stand in for the second tab that step 4's actions will create: the | ||
| // trail follows the Active Tab, so a fresh tab starts with no history. |
There was a problem hiding this comment.
🧹 [NIT] Narrates branch process ("step 4"). Per simplify: delete conversation/history comments; say the constraint only.
| // Stand in for the second tab that step 4's actions will create: the | |
| // trail follows the Active Tab, so a fresh tab starts with no history. | |
| // Trail follows the Active Tab; a fresh tab starts with no history. |
There was a problem hiding this comment.
Overview
Follow-up on desktop tabs (#240): prior blockers on closeTab stale snapshot, TabStrip focus when changelog covers the editor, active-tab scrollIntoView, PRODUCT open/switch model, and the narrating test comment are fixed. Architecture (single document, path+trail tabs, save-before-leave) still matches the PR intent.
Prior findings
- Addressed:
closeTabre-reads tabs afterleaveCurrentDocument; TabStrip anchors focus whenactiveTabIdis null;scrollIntoViewon activate; PRODUCT strip/open/shortcut model mostly rewritten; test comment no longer says "step 4". - Still open: PRODUCT Behavior 2 still lists file picker and new-file as tab-minting opens; those call sites still use plain
loadPathand replace the Active Tab. Behavior 14 still says undo reopens "as a single tab"; code restores the pre-delete strip when unchanged.
Concerns
- Spec still over-promises deliberate-open surfaces and under-describes delete undo.
- Untouched (not in this PR's code edits):
openFilePickerandcreateEmptyFileInFolderstillloadPathwithouttab: "new"— either wire them throughopenTabForPath/{ tab: "new" }or drop them from Behavior 2.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Reviewed by a Warp Factory agent.
| ### Opening | ||
|
|
||
| 1. With a Workspace Folder or Plain Folder open, the tab strip appears above the editor as soon as one Markdown File is open, and stays for every note after it. | ||
| 2. Choosing a file deliberately gives it a tab: the sidebar, `CmdOrCtrl+P`, the file picker, and creating a new file. The tab becomes active. |
There was a problem hiding this comment.
This lists the file picker and creating a new file as deliberate opens that mint/focus a tab. Only sidebar + CmdOrCtrl+P were switched to openTabForPath. openFilePicker and createEmptyFileInFolder still call plain loadPath, so they replace the Active Tab.
Either route those through openTabForPath / { tab: "new" }, or drop file picker and new-file from this behavior so the checked-in spec matches the PR.
| ### Files changing underneath | ||
|
|
||
| 13. Renaming or moving a Markdown File open in any tab updates that tab in place; the tab's back/forward trail is rewritten as it already is today. | ||
| 14. Deleting a Markdown File closes every tab showing it. Undoing that delete reopens it as a single tab. |
There was a problem hiding this comment.
Delete snapshots the whole tabs slice and restores tabsBefore when the strip is unchanged, so a background delete undo puts the closed tab back at its old index beside the survivors — not a lone tab.
Rewrite Behavior 14 to the restore-strip model (or change undo to the single-tab rule).
There was a problem hiding this comment.
Overview
Follow-up on desktop tabs (#240). Latest delta only refactors TabStrip (tabAt helper, scroll/focus via anchor). Prior code fixes for closeTab, focus when changelog covers the editor, and scrollIntoView stay in place. Architecture still matches the single-document / path+trail design.
Prior findings
- Addressed:
closeTabre-reads after leave; TabStrip focus fallback; active-tabscrollIntoView; narrating test comment; PRODUCT strip/open/shortcut model mostly aligned. - Still open: PRODUCT Behavior 2 still lists file picker + new-file as tab-minting opens while those call sites stay on plain
loadPath. Behavior 14 still says undo reopens "as a single tab" while code restores the pre-delete strip.
Concerns
- Spec still over-promises deliberate-open surfaces and misstates delete undo.
- Untouched (not in this PR's code edits):
openFilePickerandcreateEmptyFileInFolderstillloadPathwithouttab: "new"— wire them or drop them from Behavior 2.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Reviewed by a Warp Factory agent.
| ### Opening | ||
|
|
||
| 1. With a Workspace Folder or Plain Folder open, the tab strip appears above the editor as soon as one Markdown File is open, and stays for every note after it. | ||
| 2. Choosing a file deliberately gives it a tab: the sidebar, `CmdOrCtrl+P`, the file picker, and creating a new file. The tab becomes active. |
There was a problem hiding this comment.
This lists the file picker and creating a new file as deliberate opens that mint/focus a tab. Only sidebar + CmdOrCtrl+P were switched to openTabForPath. openFilePicker and createEmptyFileInFolder still call plain loadPath, so they replace the Active Tab.
Either route those through openTabForPath / { tab: "new" }, or drop file picker and new-file from this behavior so the checked-in spec matches the PR.
| ### Files changing underneath | ||
|
|
||
| 13. Renaming or moving a Markdown File open in any tab updates that tab in place; the tab's back/forward trail is rewritten as it already is today. | ||
| 14. Deleting a Markdown File closes every tab showing it. Undoing that delete reopens it as a single tab. |
There was a problem hiding this comment.
Delete snapshots the whole tabs slice and restores tabsBefore when the strip is unchanged, so a background delete undo puts the closed tab back at its old index beside the survivors — not a lone tab.
Rewrite Behavior 14 to the restore-strip model (or change undo to the single-tab rule).
There was a problem hiding this comment.
Overview
Follow-up on desktop tabs (#240). Prior blockers are addressed: PRODUCT Behavior 2/14 wording for strip restore and deliberate opens, closeTab post-await re-read, TabStrip focus when changelog covers the editor, and scrollIntoView. Architecture (single document, path+trail tabs, save-before-leave) still matches the PR intent.
Prior findings
- Addressed: PRODUCT Behavior 2 no longer claims file picker / new-file mint tabs; Behavior 14 describes restore-strip when unchanged; earlier code fixes remain.
- Still open / new: Behavior 14's "otherwise" branch is still unimplemented. Single-file / folder delete paths close tabs but do not load a survivor the way multi-delete does.
Concerns
- PRODUCT Behavior 14 promises that when the strip changed since delete, undo reopens the file as one tab beside the current set. Code only restores on reference equality with
tabsAfterand never mints that fallback tab. deleteMarkdownFile/deleteFolder(sidebar single delete, command palette) prune matching tabs and empty the document when the open note is deleted, but neverloadPaththe new Active Tab. Multi-selectdeleteSidebarItemsdoes. Deleting the visible note with other tabs open leaves a live strip and an empty editor.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Reviewed by a Warp Factory agent.
| ### Files changing underneath | ||
|
|
||
| 13. Renaming or moving a Markdown File open in any tab updates that tab in place; the tab's back/forward trail is rewritten as it already is today. | ||
| 14. Deleting a Markdown File closes every tab showing it. Undoing that delete puts the strip back as it was, each reopened tab at the position it held, provided no tab has opened or closed since; otherwise the file reopens as one tab beside whatever is now open. |
There was a problem hiding this comment.
When the strip has changed since delete, this says undo reopens the file as one tab beside whatever is now open. undoDelete only restores tabsBefore when tabsStore.get() === deletion.tabsAfter, and never mints a fallback tab for the deleted path.
Implement that mint (openTabForPath / { tab: "new" } when the strip moved), or drop the otherwise clause so the checked-in spec matches the PR.
| ), | ||
| ), | ||
| }, | ||
| tabs: withoutTabsMatching(state.tabs, (tabPath) => tabPath === path), |
There was a problem hiding this comment.
deleteSidebarItems closes matching tabs and, when the visible note was deleted, loads the new Active Tab so the editor follows the strip. deleteMarkdownFile (sidebar single-file delete, command palette) and deleteFolder only run withoutTabsMatching + emptyDoc.
Deleting the open note while other tabs remain leaves a live strip and an empty editor. After closing tabs here, if currentPath was cleared, load the survivor the same way runDelete does (activeTabId → loadPath(..., { history: "none", launchExternal: false, tab: survivor })).
2 participants
Description
Refs #240.Opening as a draft: the issue is still labelled
needs-discussionand @bholmesdev hasn't weighed in, so this is here to look at rather than to merge. @nyashkn confirmed the shape on the issue: save-then-load per switch, no per-document buffers, dirty dot dropped, undo and view-mode reset accepted.Several notes can be open at once as tabs, one visible at a time, each with its own back/forward trail and scroll position.
A tab records where a note is, not what it holds: its path and its trail. The open document stays singular, with one file loaded, one editor and one watcher. Activating a tab saves the current note and loads the tab's note, which is what clicking a sidebar row already did.
Per-tab document buffers were considered and rejected. The single-document assumption is load-bearing in reads, not only writes.
savePathContentNow, the auto-title liveness check intitleManagement.ts, andupdateMovedLinkswould all quietly come to mean "the focused document" while still compiling.Opening a note explicitly, from the sidebar or
Cmd/Ctrl+P, gives it a tab or focuses the one already showing it. Following a wiki link stays in the current tab, which is what its trail is for.New module
apps/desktop/src/store/tabs.tsholds the tab state shape and the pure functions over it.state.tsimports from it rather than the reverse, so the logic is testable without standing up the store.Two behaviour changes outside tabs, worth flagging
Leaving a note now saves it first. This fixes a standing bug: typing without pausing and immediately opening another note dropped the last edit, because the editor's unmount flush ran after the store had moved on.
Leaving a note with a disk conflict is refused, with a toast. Back already refused, but a sidebar click didn't. Now every exit goes through one place. It matters more once the note is saved on the way out, since saving a conflicted note would overwrite the disk copy.
Not included
Terminal-per-tab is agreed as a separate PR, so this issue stays open after this lands. Also out of scope: split view, preview tabs, drag to reorder, persisting tabs across relaunch, and preserving undo across a switch.
Changelog
Entry added under
[Unreleased],Added:Type of Change
Testing
Existing tests pass
Added new tests for changes
Tested manually (describe below)
Tested manually (describe below)
pnpm build:desktopclean. 247 desktop tests, 152 ui, 115 editor. React Compiler audit: 0 failed.Store-level tests cover tab targeting, per-tab history isolation, rename rewriting a background tab, delete closing tabs and undo restoring them, and conflict refusal on each exit path. The pure helpers in
tabs.tsand the scroll memory are tested directly, without standing up the store.TabStriphas component tests under the existing happy-dom precedent, including keyboard navigation.Manual Testing Details:
Driven against the running Electron app: the strip appears with the first note, sidebar opens accumulate tabs, reopening a note focuses its tab instead of duplicating it, arrow keys move between notes, the strip costs one tab stop rather than one per note, clicking the open tab is a no-op, following a link stays in the same tab, and closing one tab leaves the rest.
Checklist