Conversation
Opening a large vault showed an empty workspace for as long as the full note listing took: init() and every vault-switch flow awaited refreshVaultIndexes() before restoreWorkspaceForVault(), and the restore validated every saved tab against the complete notes index, so the tab strip could not appear until the whole vault had been walked. The workspace snapshot is tiny next to the index, so the order is now reversed by an openVaultWorkspace orchestrator shared by all eight call sites: restore first, trusting the snapshot (sanitizeWorkspaceLayout shape-checks paths but no longer requires them to exist), then kick off the index refresh. Little new reconciliation is needed: refreshNotes has pruned tabs whose notes vanished since #384, active tabs are read from disk during restore anyway (a failed read still drops the tab), and tab titles fall back to the filename until metadata arrives. Two pieces deliberately wait for the index instead: folder rows join the startup-collapsed set only once folders exist (Quick Notes and Inbox are decided at restore time and left alone, in case the user toggled them in the gap), and the background-tab content warm-up runs after the listing can vouch for the paths, instead of being computed from an index that is always empty at restore time. Verified over CDP against the built app with a seeded 3000-note vault and a saved workspace containing a deleted note's tab: before, tabs painted 100ms after the listing applied; after, they paint 313ms before it, the stale tab is pruned on arrival, and subfolders come up collapsed. The lead grows with vault size, which is the point.
With auto-pair brackets on (the default), typing {{ in the template
editor produces {{}} with the caret in the middle. The variable
completion then replaced only the text from the opening braces to the
caret with the full {{variable}}, so the auto-paired closers survived
after it and every accepted variable ended in four braces:
{{cursor}}}}. The same leftover appeared without auto-pair when
completing inside an existing {{...}} pair, e.g. editing {{date}} down
to {{ti}} and accepting {{time}}.
The completion's inserted text carries its own closers, so applying it
now swallows up to two } characters already sitting at the caret. Two
is the cap on purpose: it is exactly what the insert provides, so a
cell of extra braces beyond the pair is left alone. The transaction
builder is exported pure (templateVariableApplySpec) in the
cm-auto-pairs style, with unit tests for the auto-pair leftover, the
existing-pair edit, a single stray closer, plain no-closer insertion,
unrelated trailing text, and the never-consume-more-than-two cap.
Verified over CDP against the built app both ways: Settings, New
template, type {{cur, accept the completion. Before: {{cursor}}}}.
After: {{cursor}}.
In visual mode with vim on, `f m` opened the editor context menu instead of extending the selection to the next m. Worse, the orphaned f motion stayed pending behind the menu: after Escape, the next key typed was swallowed as the f target and the selection jumped somewhere unasked. The context-menu shortcut lives in VimNav's window-level keydown listener, registered with capture: it fires before CodeMirror sees the event and stops propagation, so vim never got the chance to consume the key as an operand. The guard is a new `vimAwaitsNextKey(view)` helper (cm-vim-pending-input.ts) reading the vim plugin's own pending state: `expectLiteralNext` (f/t/r awaiting their character) or a non-empty input key buffer (a count like 2, a g prefix, a register). Both VimNav menu sites and the editor-level m handler in EditorPane now yield when it reports pending, so `v f m` finds the m and a plain `v m` still opens the menu. The native context-menu key is not a character and is deliberately left alone. The helper is pure enough to test against a real vim editor in jsdom: four tests cover normal/visual baseline, f-then-target (asserting the motion actually consumed the key), a buffered count, and r. Verified over CDP against the built app both ways with vim enabled: before, `v f m` opened the menu and the f stayed pending; after, `v f m d` deletes "alpha m" out of "alpha m bravo" and `v m` alone still opens the menu.
:wa and :wall were registered as straight aliases of :qall, on the theory that autosave leaves nothing to flush so the write half of the command is a no-op. That reading got the vim contract backwards. In vim, :wa is a save command: write all changed buffers, close nothing. Every vim user's muscle memory types it reflexively after an edit, and here it silently demolished the whole tab layout instead (#569, reported by @simonlearnscoding). The data was never lost, closing flushes dirty buffers, but the workspace was. :wa / :wall now run flushDirtyNotes: every dirty note is written to disk (task-mutation queues drained first, workspace persisted), no tab is touched. The quit-and-write family keeps closing, exactly as vim defines it: :xa / :xall stay, and :wqa / :wqall is newly registered so the muscle-memory spelling of "save everything and quit" works too. :w was already write-only and :q / :wq still close just the active note. No toggle needed; vim semantics are the only behavior now. The in-app help and the website docs table both listed :wa in the close-everything row; both now document :wa / :wall as "write every unsaved note" with :wqa in the closing row. Verified over CDP against the built app both ways: three tabs open, dirty the active note with a normal-mode x, run :wa from the real ex prompt. Before: every tab closes. After: all three tabs stay and the file on disk has the edit; :wqa then closes all three.
gt and gT could only cycle, so reaching a specific tab in a full strip meant hammering the same key and watching every stop in between (#497, requested by @uNyanda, who asked for exactly these two halves). The vim half follows vim's own contract: {count}gt is absolute (2gt is tab 2, 5gt is tab 5, a count past the end lands on the last tab, plain gt still cycles) while {count}gT stays relative and walks that many tabs back. Both ride the existing nextBuffer/previousBuffer actions, which now read the count CM-Vim already collects (repeatIsExplicit distinguishes 1gt from a bare gt). Tab numbers count across panes in the same order gt cycles through, one shared openTabOrder helper, so cycling and direct selection can never disagree about which tab is number three. The non-vim half is nine rebindable shortcuts, tabs.select1 through 9, dispatched from the app-level handler so they work with vim off and regardless of focus. Alt+digit cross-platform; on macOS the default is Ctrl+digit, because Option+digit types characters on many layouts (the #514 trap) and Cmd+1/2/4/5/6 already mean sidebar, connections, and the pane modes (the same escape Ctrl+Tab uses). Direct selection targets open tabs only: no recent-notes fallback, "tab 3" is an open tab or nothing. Docs updated in all the mirrored places: in-app help (vim card and shortcut table), the website docs rows, and the shared keymaps catalog (whose mirror test caught the omission). Verified over CDP against the built app: 2gt lands on tab 2, plain gt cycles on, 9gt clamps to the last of three, 2gT walks two back, and Ctrl+3 / Ctrl+1 (the mac defaults) jump directly. Unit tests cover the select-order math, clamping, the no-fallback rule, and multi-step gT wrapping.
The help has promised Obsidian-style size hints "same syntax as images" since Excalidraw embeds got them, but only drawings ever honored them. ![[assets/pic.jpg|100x50]] rendered full-width with "100x50" leaking into the alt text, and the markdown spelling  did the same, in the editor's live preview and the reading view alike (#570, reported by @corpserot with both spellings tried). The hint parser already existed for drawings; images now share it. A new splitEmbedLabel helper separates a label's caption from a trailing size segment (hint-only labels, caption|hint, pipes inside captions all covered), and three surfaces consume it: - The reading view gets one remarkImageSizeHints pass over image nodes. Both spellings arrive there with the hint in the alt (the wikilink branch routes through an image node), so a single pass strips the hint and emits real width/height attributes. Attachment chips ride the image node type with their labels as display names, so only image-classified and remote urls participate. - The live-preview widget parses the hint for both spellings and sizes its img. - Both surfaces apply the size as inline style as well as attributes, because the embed class stretches images to width: 100% and presentational attributes lose to any CSS rule. The first fix attempt set attributes only; the pixels on screen did not move, and only measuring getBoundingClientRect in the running app caught it. The class's max-width: 100% still caps hints wider than the pane. Verified over CDP against the built app both ways with the reporter's exact two lines: before, both images render 854px wide with the hint in the alt; after, both render exactly 100x50 with clean captions, in Edit and Preview. Seven new unit tests cover the label splitting and the rendered attributes.
adibhanna
left a comment
adibhanna
left a comment
There was a problem hiding this comment.
Multi-agent review of this branch at xhigh effort: 31 candidate findings, 15 survived adversarial verification (14 confirmed, 1 plausible), 2 refuted. 13 are anchored as inline comments; two sit on pre-existing code outside the diff hunks, so they live here:
correctness / CONFIRMED: VimNav.tsx:612: #497 gave {count}gt absolute-jump semantics, but count-awareness only exists in the editor-focused codemirror-vim path; VimNav's global gt/gT fallback (preview mode, non-editor focus) drops the typed count and cycles one tab instead of jumping to tab N. With 5 tabs open and focus in the rendered preview (the #321 fallback path), typing 2gt moves to the NEXT tab instead of tab 2. The new help.ts entry promises the vim contract without qualifying that it only works while the editor owns focus.
correctness / CONFIRMED: Editor.tsx:773: the documented ex-line equivalents :tabnext/:tabprevious (and :bn/:bp) still discard any count or argument, so the two spellings of the same vim command now disagree. :tabnext 3 (or :3tabnext), which in vim jumps to tab 3, ignores its params entirely and calls navigateActiveBuffer(state, 1), silently advancing one tab; the same dropped-count inconsistency as the VimNav fallback, but on the ex-command entry point.
The findings cluster in three areas: the #564 workspace-restore rework (phantom/un-prunable tabs), the #497 tab shortcuts (modal bleed-through, override shadowing, AZERTY and Alt-code and Mission Control conflicts, dropped counts), and #570 size-hint parsing (numeric captions consumed, zero dimensions mishandled). Plus one template-completion change that can delete literal braces from note content.
| if (typeof value !== 'string') return null | ||
| return existingPaths.has(value) || isWorkspaceVirtualTabPath(value) ? value : null | ||
| if (typeof value !== 'string' || !value) return null | ||
| return value |
There was a problem hiding this comment.
correctness / CONFIRMED: #564: sanitizeWorkspaceLayout no longer verifies tab paths, and refreshNotes' selectedPath exemption then keeps a never-verified ghost tab alive, so a stale snapshot restores to a phantom active tab with a blank editor instead of being pruned. (Same root cause surfaces at store.ts:4290 and store.ts:4340.)
Vault synced between two machines; the notes open in the saved workspace were renamed or deleted on the other machine. On launch, the active tab's eager read fails and rewritePathsInTree prunes it, promoting a sibling stale tab to activeTab (nextActive = nextTabs[0]) whose content was never loaded and whose note does not exist. When the index lands, refreshNotes' keep() retains that path because it equals s.selectedPath (the don't-blank-mid-save exemption), so the user lands on a tab titled after a nonexistent note showing an empty editor that survives subsequent refreshes until manually closed. Pre-2.27 restore ran after refreshVaultIndexes and checked every tab against existingPaths, so the phantom never appeared.
| if (typeof value !== 'string') return null | ||
| return existingPaths.has(value) || isWorkspaceVirtualTabPath(value) ? value : null | ||
| if (typeof value !== 'string' || !value) return null | ||
| return value |
There was a problem hiding this comment.
correctness / CONFIRMED: With path validation removed here, an all-stale snapshot whose active tab is virtual trips the #384 preserve guard, so every dead note tab is preserved on every refresh for the whole session.
A restored snapshot has a virtual tab (e.g. Tasks or a diagram tab) active plus background note tabs synced from another machine that do not exist locally. The eager read skips background tabs, so they all survive restore. On refreshNotes the prune would leave zero note tabs, so preserveLayoutIfPruneEmptiesNoteTabs (pane-layout.ts:292) returns the pre-prune layout, and it does so again on every subsequent refresh: the phantom tabs persist all session, and clicking any of them yields a failed note load instead of the tab quietly disappearing as the pre-PR existingPaths filter guaranteed.
| insert: string | ||
| ): TransactionSpec { | ||
| const after = state.doc.sliceString(to, to + 2) | ||
| const consumed = after.startsWith('}}') ? 2 : after.startsWith('}') ? 1 : 0 |
There was a problem hiding this comment.
correctness / CONFIRMED: Completion apply no longer replaces exactly the typed range: it unconditionally swallows up to two closing braces after the caret even when they are literal note content.
The old apply dispatched changes for exactly [from, to]; the new templateVariableApplySpec always extends the replacement over one or two } characters sitting after the caret, without checking they were produced by auto-pair or belong to this variable. In a note documenting escaped template syntax, e.g. the user types {{da immediately before an existing literal }} that closes a different construct (a Handlebars/Jinja snippet written as prose), accepting the completion deletes those two literal brace characters from the note body, corrupting the surrounding text instead of only inserting the variable.
| image.alt = alt | ||
| const data = (image.data ??= {}) as { hProperties?: Record<string, unknown> } | ||
| const hProperties = (data.hProperties ??= {}) | ||
| if (size.width) hProperties.width = size.width |
There was a problem hiding this comment.
correctness / CONFIRMED: #570: falsy-zero width/height checks run after splitEmbedLabel has already consumed the label, so a |0 or |0xH hint destroys the alt/caption without applying the corresponding dimension (and |0x300 applies height only, distorting the image).
A note containing ![[diagram.png|0x300]] renders in reading view with height="300" but no width attribute (parseEmbedSizeHint's ^(\d+) accepts 0, splitEmbedLabel returns {width: 0, height: 300}, then if (size.width) skips the 0), stretching the image, and the label is gone (alt=""). ![[diagram.png|0]] loses its label entirely with no resize at all. The same falsy-zero pattern repeats in cm-live-preview.ts LocalImageWidget (if (this.width)) and local-assets.ts (hintWidth > 0), so live preview mirrors the distortion.
| const raw = (label ?? '').trim() | ||
| if (!raw) return { alt: '', size: null } | ||
| const wholeSize = parseEmbedSizeHint(raw) | ||
| if (wholeSize) return { alt: '', size: wholeSize } |
There was a problem hiding this comment.
correctness / CONFIRMED: #570: splitEmbedLabel treats a purely numeric markdown alt text as a whole-label size hint, so an intentional numeric caption is consumed: the image is resized and its caption/alt is silently erased.
A note with , where '2024' is the caption (a year), now renders the chart with width="2024" (stretched to the pane's max-width cap) and alt=""; the reading-view figcaption falls back to the filename and screen readers lose the alt text. Before 2.27 the caption displayed '2024' and the image kept its natural size. This may be intended Obsidian parity for the wikilink form ![[img|600]], but it also rewrites the semantics of pre-existing standard-markdown alt text in users' notes; worth a deliberate verdict.
| group: "global", | ||
| title: `Go to tab ${n}`, | ||
| description: `Jump straight to tab ${n}, counted across panes in the same order gt cycles.`, | ||
| defaultBinding: `Alt+${n}`, |
There was a problem hiding this comment.
correctness / CONFIRMED: Alt+digit default bindings match on event.key, so on keyboard layouts where digits are shifted (French/Belgian AZERTY, Czech) Alt+1..9 produce keys like & or é and the new tab shortcuts never fire.
A Windows or Linux user on a French AZERTY layout presses Alt+1 to jump to tab 1 (as documented in the in-app help): resolveKeyFromEvent takes the typed-character fast path and builds "Alt+&", which never equals the stored binding "Alt+1", so nothing happens and the entire #497 non-vim feature is dead by default on those layouts. The codebase already documents this exact trap class (#514, the defaultBindingMac comment about Option+printable chords) but only shielded macOS; the cross-platform Alt+digit defaults introduced here are the first digit-based defaults and hit the same key-vs-layout pitfall unshielded.
| title: `Go to tab ${n}`, | ||
| description: `Jump straight to tab ${n}, counted across panes in the same order gt cycles.`, | ||
| defaultBinding: `Alt+${n}`, | ||
| defaultBindingMac: `Ctrl+${n}`, |
There was a problem hiding this comment.
correctness / PLAUSIBLE: The macOS default Ctrl+1..9 collides with Mission Control's 'Switch to Desktop N' shortcuts, which macOS auto-enables once a user has multiple Spaces, so the OS consumes the key before the app sees it.
A Mac user with two or more Spaces presses Ctrl+2 as the in-app help ('Ctrl+1 ... Ctrl+9 on macOS') advertises: macOS switches to Desktop 2 instead of tab 2, and the app never receives the keydown. The advertised default shortcut silently does nothing in-app for these users; only rebinding or {count}gt works, and nothing in the UI explains why.
| // presentational attributes lose to any CSS rule, so the hint also goes | ||
| // on as inline style. The class's max-width: 100% still caps a hint | ||
| // wider than the pane. | ||
| if (this.width) { |
There was a problem hiding this comment.
correctness / CONFIRMED: LocalImageWidget.toDOM applies #570 size hints only when truthy onto a cached img element whose cache key (url|mtime) does not include the hint, so a reused element keeps stale width/height attributes and inline styles after the hint is removed or shrunk.
Type ![[pic.png|300x200]]; the image renders and, once loaded, the element is admitted to the #472 element cache keyed by url|mtime. Put the cursor on the line (widget destroyed, element disconnected but still cached), edit the hint to |300 or delete it entirely, and move the cursor off: eq() correctly rebuilds the widget, but takeCachedImage returns the same element still carrying width=300/height=200 attributes and inline styles, and since this.width/this.height are now undefined nothing clears them, so the image stays at the old size (or renders 300px wide with a stale 200px height, distorting the aspect ratio) until the file's mtime changes or the cache evicts. The same leak fires when one note embeds ![[pic.png|100]] and another embeds ![[pic.png]] plain: whichever renders second inherits the 100px style.
| * closed (#568). | ||
| */ | ||
| export function vimAwaitsNextKey(view: EditorView | null | undefined): boolean { | ||
| const vim = view ? getCM(view)?.state.vim : null |
There was a problem hiding this comment.
simplification / CONFIRMED: The new vimAwaitsNextKey helper (plus its test file) duplicates the pre-existing exported isVimAwaitingArgument in lib/vim-nav.ts:190 (#147), which implements the identical predicate (expectLiteralNext || keyBuffer.length > 0); VimNav.tsx now imports and calls both names for the same vim-pending state in the same handler.
No runtime failure today, but the two copies are one behavioral tweak away from diverging: a future fix to pending-key detection (e.g. also treating inputState.operator as pending) applied to one helper leaves the leader-key guard at VimNav.tsx:1018 and the #568 context-menu guards at VimNav.tsx:1073/1150 plus EditorPane disagreeing about whether vim owns the next key, reintroducing the #147/#568 class of stolen-operand bugs in whichever path kept the stale copy. The #568 change could have imported isVimAwaitingArgument (already covered by vim-nav.test.ts) instead of adding a second module, test file, and name for the same logic.
| state.setWordWrap(!state.wordWrap) | ||
| return | ||
| } | ||
| // Alt+1..9 (⌃1..9 on macOS) — jump straight to tab N (#497). Position |
There was a problem hiding this comment.
style / CONFIRMED: New comment uses an em dash, violating the house writing rule (never use an em dash in code comments, commits, docs, or UI copy).
The newly added comment // Alt+1..9 (⌃1..9 on macOS) — jump straight to tab N (#497) contains an em dash; restructure with a comma, colon, or parentheses.
…571) The release review confirmed twelve defects and one plausible trap across the branch's changes. This lands all of them in one follow-up commit so each release commit keeps telling its own story. Restore (#564): trusting the snapshot before the index exists was the point, but the strict check the old restore order gave for free never came back. A snapshot synced from another machine could promote a never-verified tab to active after the eager read pruned the ghost, and refreshNotes' selectedPath exemption then kept it alive forever; a virtual-active snapshot full of stale note tabs tripped the #384 anti-wipe guard on every refresh instead. openVaultWorkspace now runs a one-time strict validation once the first listing lands: every restored tab must exist, be virtual, or hold unsaved edits. An empty listing skips the pass, because it is indistinguishable from a failed one (the #384 lesson). The refreshNotes exemption also requires the note to be loaded or dirty, so nothing that never loaded can ride it. Templates (#566): the completion swallowed up to two closing braces without asking whose they were. It now counts the line's earlier {{ against }}: if an earlier construct is still open, the braces after the caret are its closers, literal note content, and they stay. Size hints (#570): zero is not a size. parseEmbedSizeHint rejects 0-dimension hints at the source, so |0x300 stays a caption instead of eating the label and applying half a resize. splitEmbedLabel now takes its source: a bare numeric alt in standard markdown () stays the author's caption; only the wikilink form keeps Obsidian's whole-label hint. And the live-preview widget clears width/height on a cached element it reuses, since the #472 cache key is url|mtime and knows nothing about hints. Tab shortcuts (#497): the new dispatch loop bails while a modal, palette, menu, or Settings is open (the house rule), and steps aside when the pressed combination is one the user explicitly rebound to a later-checked action: an explicit rebind outranks a shipped default. Two keyboard realities got shields: Alt+numpad digits never resolve on Windows (that is Alt-code character entry, the OS owns it), and digit bindings fall back to the physical Digit1-9 position so Alt+1 fires on AZERTY, where the digit row types punctuation (the #514 trap class, now cross-platform). The Mission Control collision on macOS Ctrl+digit stays the default deliberately (Option types characters and the Cmd digits are taken), but the caveat now ships in the keymap description and the help row, so an affected user learns to rebind instead of concluding the feature is broken. Cleanup: vimAwaitsNextKey duplicated isVimAwaitingArgument one PR after #147 shipped it; the module and its name are gone, VimNav and EditorPane call the one helper, and the real-editor test moved to vim-nav-awaiting-argument.test.ts because vim-nav.test.ts mocks getCM. One comment lost its em dash. Verified over CDP against the built app (scratchpad verify-571-fixes.mjs): a seeded stale snapshot's ghost tab disappears once the index lands, Ctrl+2 jumps to tab 2, Ctrl+1 is inert under the command palette and works again after Escape. Full app-core suite and typecheck green.
1 participant
Release branch for 2.27.0: one feature and five fixes, all issues closed. Per the usual flow, main fast-forwards from this branch at release time rather than merging through the button.
Feature
Fixes
![[assets/pic.jpg|100x50]]andnow render at the hinted size in live preview and the reading view, instead of full-width with the hint leaking into the alt text.fMotion Triggers Context Menu in Visual Mode #568).Each change was verified over CDP against the built app, and docs are updated in the mirrored surfaces (in-app help, shared keymaps catalog, website rows).