From c804038fe147e71becf10c2e68b7a62a47fbfa6b Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 08:18:26 -0500 Subject: [PATCH 01/18] Fix(cloud): store the cloud credential on desktops Chromium does not recognize On Linux, Chromium picks the safeStorage keyring backend from XDG_CURRENT_DESKTOP instead of probing the bus for a Secret Service. On compositors it does not recognize (Niri, Hyprland, Sway) it falls back to the plaintext basic_text backend, safeStorage reports encryption as unavailable, and cloud sign-in dies with "could not store the cloud credential securely" even though gnome-keyring is running and healthy. Reported on Discord by a Niri user whose secret-tool round trip worked, which is the confusing part: nothing the user can check locally is consulted by Chromium's detection. The fix appends --password-store=gnome-libsecret before app ready, only when the session variables identify no desktop Chromium recognizes and the user did not pass the switch themselves. Safe by construction: when the Secret Service is genuinely absent, Chromium falls back to basic_text, the same outcome as today. On recognized desktops, including XFCE and LXQt where Chromium chose plaintext deliberately, ZenNotes defers to Chromium's own choice. The storage failure dialog and the remote-workspace console warning now point at the switch instead of leaving a dead end. --- apps/desktop/src/main/cloud-auth.ts | 9 +++- apps/desktop/src/main/index.ts | 13 +++++ .../src/main/linux-password-store.test.ts | 42 +++++++++++++++++ apps/desktop/src/main/linux-password-store.ts | 47 +++++++++++++++++++ apps/desktop/src/main/secret-store.ts | 5 +- 5 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/src/main/linux-password-store.test.ts create mode 100644 apps/desktop/src/main/linux-password-store.ts diff --git a/apps/desktop/src/main/cloud-auth.ts b/apps/desktop/src/main/cloud-auth.ts index 6560a467..c9876bcf 100644 --- a/apps/desktop/src/main/cloud-auth.ts +++ b/apps/desktop/src/main/cloud-auth.ts @@ -170,8 +170,15 @@ export class CloudAuthManager { throw new Error("ZenNotes Cloud returned an invalid sign-in response."); } if (!(await this.dependencies.setSecret(pending.base_url, payload.token))) { + // On Linux this almost always means Chromium settled on its plaintext + // key store because it did not recognize the desktop environment, so + // point at the override instead of leaving a dead end. throw new Error( - "ZenNotes could not store the cloud credential securely on this device.", + process.platform === "linux" + ? "ZenNotes could not store the cloud credential securely on this device. " + + "If a Secret Service keyring (such as gnome-keyring) is running, " + + "launch ZenNotes with --password-store=gnome-libsecret and sign in again." + : "ZenNotes could not store the cloud credential securely on this device.", ); } diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index eab1f32c..b780ec4b 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -182,6 +182,7 @@ import { setRemoteWorkspaceSecret, } from "./secret-store"; import { CloudAuthManager, resolveCloudBaseUrl } from "./cloud-auth"; +import { shouldForceGnomeLibsecret } from "./linux-password-store"; import { CloudAuthLoopbackServer } from "./cloud-auth-loopback"; import { createCloudSyncClient } from "./cloud-sync-client"; import { DesktopCloudSyncService } from "./cloud-sync-service"; @@ -4759,6 +4760,18 @@ if (process.platform === "linux") { // through xdg-desktop-portal, but Electron only wires that path when this // Chromium feature is enabled before app.whenReady(). app.commandLine.appendSwitch("enable-features", "GlobalShortcutsPortal"); + // Chromium picks the safeStorage keyring backend from desktop detection, + // not by probing the bus, so on compositors it does not recognize (Niri, + // Hyprland, Sway) it falls back to plaintext and cloud sign-in cannot store + // its credential even with a healthy gnome-keyring running. Point Chromium + // at libsecret on those sessions; rationale and the safety argument live in + // linux-password-store.ts. A user-supplied --password-store always wins. + if ( + !app.commandLine.hasSwitch("password-store") && + shouldForceGnomeLibsecret(process.env) + ) { + app.commandLine.appendSwitch("password-store", "gnome-libsecret"); + } } app.whenReady().then(async () => { diff --git a/apps/desktop/src/main/linux-password-store.test.ts b/apps/desktop/src/main/linux-password-store.test.ts new file mode 100644 index 00000000..c8cba042 --- /dev/null +++ b/apps/desktop/src/main/linux-password-store.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' +import { shouldForceGnomeLibsecret } from './linux-password-store' + +describe('shouldForceGnomeLibsecret', () => { + // The report that motivated this: Niri with a healthy gnome-keyring, where + // Chromium's desktop detection falls back to plaintext and cloud sign-in + // cannot store its credential. + it('forces libsecret on compositors Chromium does not recognize', () => { + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'niri' })).toBe(true) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'Hyprland' })).toBe(true) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'sway' })).toBe(true) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'river' })).toBe(true) + }) + + // Harmless on headless sessions: libsecret init fails without a bus and + // Chromium falls back to plaintext, the same outcome as without the switch. + it('forces libsecret when no desktop is declared at all', () => { + expect(shouldForceGnomeLibsecret({})).toBe(true) + }) + + it('defers to Chromium on desktops it recognizes', () => { + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'GNOME' })).toBe(false) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'ubuntu:GNOME' })).toBe(false) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'KDE' })).toBe(false) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'X-Cinnamon' })).toBe(false) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'XFCE' })).toBe(false) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'LXQt' })).toBe(false) + expect(shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'Unity:Unity7:ubuntu' })).toBe(false) + }) + + it('bails out when the fallback session variables identify a desktop', () => { + expect(shouldForceGnomeLibsecret({ DESKTOP_SESSION: 'kde-plasma' })).toBe(false) + expect(shouldForceGnomeLibsecret({ DESKTOP_SESSION: 'mate' })).toBe(false) + expect(shouldForceGnomeLibsecret({ DESKTOP_SESSION: 'xubuntu' })).toBe(false) + expect( + shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'weird', GNOME_DESKTOP_SESSION_ID: 'this-is-deprecated' }) + ).toBe(false) + expect( + shouldForceGnomeLibsecret({ XDG_CURRENT_DESKTOP: 'weird', KDE_FULL_SESSION: 'true' }) + ).toBe(false) + }) +}) diff --git a/apps/desktop/src/main/linux-password-store.ts b/apps/desktop/src/main/linux-password-store.ts new file mode 100644 index 00000000..c6e27502 --- /dev/null +++ b/apps/desktop/src/main/linux-password-store.ts @@ -0,0 +1,47 @@ +/** + * Chromium picks the safeStorage keyring backend from desktop-environment + * detection (XDG_CURRENT_DESKTOP and friends, base/nix/xdg_util.cc), never by + * probing the session bus for a Secret Service. A session it does not + * recognize (Niri, Hyprland, Sway, and other niche compositors) lands on the + * plaintext basic_text backend, safeStorage then reports encryption as + * unavailable, and ZenNotes refuses to persist cloud and remote-workspace + * credentials even though a healthy gnome-keyring is sitting on the bus. + * + * The escape hatch is Chromium's `--password-store=gnome-libsecret` switch, + * which skips detection and talks to the Secret Service directly. Forcing it + * on unrecognized sessions is safe: when the Secret Service is genuinely + * absent, Chromium falls back to basic_text, which is exactly the behavior + * without the switch. + */ + +/** + * Tokens Chromium's desktop detection recognizes. When one is present, + * Chromium either wires a real keyring on its own (the GNOME family via + * libsecret, KDE via KWallet) or deliberately chose plaintext for that + * desktop (XFCE, LXQt); ZenNotes defers to that choice either way. Matching + * is a case-insensitive substring test over the combined session variables, + * which over-approximates on purpose: a false bail-out keeps the stock + * behavior, a false positive would override a working KWallet. + */ +const RECOGNIZED_DESKTOP_TOKENS = [ + 'cinnamon', + 'deepin', + 'gnome', + 'kde', + 'lxqt', + 'mate', + 'pantheon', + 'plasma', + 'ukui', + 'unity', + 'xfce', + 'xubuntu' +] + +export function shouldForceGnomeLibsecret(env: Record): boolean { + const session = `${env.XDG_CURRENT_DESKTOP ?? ''}:${env.DESKTOP_SESSION ?? ''}`.toLowerCase() + if (RECOGNIZED_DESKTOP_TOKENS.some((token) => session.includes(token))) return false + // Chromium's last-resort detection reads these legacy session markers. + if (env.GNOME_DESKTOP_SESSION_ID || env.KDE_FULL_SESSION) return false + return true +} diff --git a/apps/desktop/src/main/secret-store.ts b/apps/desktop/src/main/secret-store.ts index 789e2db2..5f66a9f4 100644 --- a/apps/desktop/src/main/secret-store.ts +++ b/apps/desktop/src/main/secret-store.ts @@ -69,7 +69,10 @@ function encodeSecret(secret: string): string | null { if (!warnedAboutMissingSecureStorage) { warnedAboutMissingSecureStorage = true console.warn( - 'ZenNotes could not persist a remote workspace token securely because no OS secret store is available.' + 'ZenNotes could not persist a remote workspace token securely because no OS secret store is available.' + + (process.platform === 'linux' + ? ' If a Secret Service keyring is running, launch ZenNotes with --password-store=gnome-libsecret.' + : '') ) } return null From 376245e44fea72841a81d1b7ae9d7c2bb4d4f69c Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 08:47:36 -0500 Subject: [PATCH 02/18] Fix(editor): a note being edited can no longer be wiped by its own save echo (#585) Typing a multi-line mermaid block (or editing bullet lists) could erase the entire note, with undo unable to restore it. Four small defects had to line up, and all four are fixed here. writeNote was truncate-then-write, so for a moment every save leaves an empty file on disk. The watcher's awaitWriteFinish delays the echo of one save into exactly that moment of the next, and the echoed readNote comes back empty. The store's live change path then pushed that read over the open buffer without checking the dirty flag (the resync path has the check, with a comment explaining why; the live path did not). The editor applies external content as a non-undoable doc swap (#247), which is why undo could not bring anything back. And persistNote cleared the dirty flag even when keystrokes landed during the write, so the follow-up save that would have healed the disk bailed on its dirty check, making the wipe permanent. Mermaid and bullet lists are only amplifiers: per-line save cycles plus renderer stalls from the first mermaid render push the echo into the truncate window. Note saves now go through writeFileAtomic so no reader can observe a half-written note; the watcher ignores the atomic-write scratch files (which also stops every database save from firing an asset refresh); a dirty buffer is never replaced by a watcher read, the pending save reconciles disk instead; and the dirty flag survives a save whenever the buffer has moved past what hit disk. Verified by driving the built app over CDP: forcing the empty read mid-edit wiped the editor and disk with undo dead before the change, and leaves buffer, disk, and undo history intact after it. Regression tests cover both store defects in store-note-integrity.test.ts. The Go server's WriteNote shares the truncate-then-write shape; the renderer guard already protects web buffers, and the Go write gets its own follow-up once its watcher's rename semantics are verified. --- apps/desktop/src/main/vault.ts | 7 +- apps/desktop/src/main/watcher.ts | 4 ++ .../app-core/src/store-note-integrity.test.ts | 65 +++++++++++++++++++ packages/app-core/src/store.ts | 14 +++- 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/main/vault.ts b/apps/desktop/src/main/vault.ts index 914957a7..9bff9dfb 100644 --- a/apps/desktop/src/main/vault.ts +++ b/apps/desktop/src/main/vault.ts @@ -3050,7 +3050,12 @@ export async function readNote(root: string, rel: string): Promise export async function writeNote(root: string, rel: string, body: string): Promise { const abs = resolveSafe(root, rel) await fs.mkdir(path.dirname(abs), { recursive: true }) - await fs.writeFile(abs, body, 'utf8') + // Atomic on purpose (#585): a plain writeFile truncates first, and the + // watcher echo of the PREVIOUS save can read the file inside that window. + // The renderer then sees an empty "external change" and replaces the open + // buffer with it, wiping the note. With temp-file + rename, no reader can + // ever observe a half-written note. + await writeFileAtomic(abs, body) invalidateNoteMetaCache(root, rel) invalidateVaultTextSearchCache(root) const folder = await folderOf(root, abs) diff --git a/apps/desktop/src/main/watcher.ts b/apps/desktop/src/main/watcher.ts index 23e85572..dcf0b00b 100644 --- a/apps/desktop/src/main/watcher.ts +++ b/apps/desktop/src/main/watcher.ts @@ -92,6 +92,10 @@ export class VaultWatcher { if (this.root && isVaultSettingsPath(this.root, p)) return false if (this.root && relativeVaultPath(this.root, p) === INTERNAL_VAULT_DIR) return false const base = path.basename(p) + // `...tmp` is writeFileAtomic's scratch file + // (note saves, database saves). Its add/unlink pair is not a vault + // change; without this filter every save also fired an asset refresh. + if (/\.\d+\.\d+\.tmp$/.test(base)) return true return base.startsWith('.') || base === 'node_modules' }, awaitWriteFinish: { diff --git a/packages/app-core/src/store-note-integrity.test.ts b/packages/app-core/src/store-note-integrity.test.ts index a50c149b..c9ae6b4b 100644 --- a/packages/app-core/src/store-note-integrity.test.ts +++ b/packages/app-core/src/store-note-integrity.test.ts @@ -191,3 +191,68 @@ describe('#202 — store keeps each note its own content during navigation', () expect(writeCalls).toEqual([]) }) }) + +// #585 ("ZenNotes clears all text from a note while editing"): the watcher +// echo of one save could read the file while the next non-atomic save had it +// truncated. applyChange pushed that empty read over the DIRTY buffer, the +// editor applied it as a non-undoable doc swap, and persistNote had already +// cleared the dirty flag so the follow-up save bailed instead of healing disk. +describe('#585 — dirty buffers survive watcher change events', () => { + it('a change event delivering a truncated read never clobbers unsaved edits', async () => { + const { useStore } = await loadStore() + seedRootVault(useStore) + const paneId = useStore.getState().activePaneId + const target = 'index.md' + await useStore.getState().openNoteInPane(paneId, target) + await flush() + + useStore.getState().updateNoteBody(target, 'INDEX_BODY plus unsaved edits') + // What the reporter hit: the file reads back empty mid-save-cycle. + vault.set(target, '') + await useStore + .getState() + .applyChange({ kind: 'change', path: target, folder: 'inbox', scope: 'content' }) + await flush() + + expect(useStore.getState().noteContents[target]?.body).toBe('INDEX_BODY plus unsaved edits') + expect(useStore.getState().noteDirty[target]).toBe(true) + + // The still-pending save reconciles disk with the buffer, not vice versa. + await useStore.getState().persistNote(target) + expect(vault.get(target)).toBe('INDEX_BODY plus unsaved edits') + }) + + it('typing during a slow write keeps the note dirty so the follow-up save lands', async () => { + const { useStore } = await loadStore() + seedRootVault(useStore) + const paneId = useStore.getState().activePaneId + const target = 'index.md' + await useStore.getState().openNoteInPane(paneId, target) + await flush() + + // Hold the first write open, as a real IPC round-trip can be. + let release!: () => void + const gate = new Promise((r) => { + release = r + }) + const zen = window.zen as unknown as { + writeNote: (p: string, b: string) => Promise + } + const realWrite = zen.writeNote + zen.writeNote = async (p: string, b: string) => { + await gate + return realWrite(p, b) + } + + useStore.getState().updateNoteBody(target, 'FIRST') + const persisting = useStore.getState().persistNote(target) + useStore.getState().updateNoteBody(target, 'FIRST AND SECOND') // typed mid-write + release() + await persisting + + // The buffer is ahead of disk, so the flag must survive the completion. + expect(useStore.getState().noteDirty[target]).toBe(true) + await useStore.getState().persistNote(target) + expect(vault.get(target)).toBe('FIRST AND SECOND') + }) +}) diff --git a/packages/app-core/src/store.ts b/packages/app-core/src/store.ts index 5c00e7e9..c855c4c4 100644 --- a/packages/app-core/src/store.ts +++ b/packages/app-core/src/store.ts @@ -6248,6 +6248,12 @@ export const useStore = create((set, get) => { const existing = s.noteContents[ev.path] // Ignore noise — only push when disk differs from our buffer. if (existing && existing.body === content.body) return s + // Never replace a dirty buffer: it holds edits the user has not + // saved, and the editor applies this push as a non-undoable doc + // swap (#247), so a stale or truncated read here destroyed work + // with no way back (#585). Same policy as the resync path above; + // the pending save will reconcile disk with the buffer instead. + if (s.noteDirty[ev.path]) return s const contents = { ...s.noteContents, [ev.path]: content } const dirty = { ...s.noteDirty, [ev.path]: false } return { @@ -6333,7 +6339,13 @@ export const useStore = create((set, get) => { void get().refreshTypstPreambles() } set((cur) => { - const dirty = { ...cur.noteDirty, [path]: false } + // Keystrokes that landed while the write was in flight leave the + // buffer ahead of disk. Clearing the flag then made the already + // scheduled follow-up save bail on its dirty check and stranded + // those edits unsaved (#585) — the flag only clears when the buffer + // still holds exactly what hit disk. + const stillCurrent = cur.noteContents[path]?.body === writtenBody + const dirty = stillCurrent ? { ...cur.noteDirty, [path]: false } : cur.noteDirty return { noteDirty: dirty, notes: cur.notes.map((n) => (n.path === meta.path ? { ...n, ...meta } : n)), From ff63df1070b75aa1d3376e7cdd01c0c7c76e50ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian-S=C3=A9bastien=20Vautier?= Date: Fri, 14 Aug 2026 17:48:09 +0400 Subject: [PATCH 03/18] editor: LaTeX command completion in math regions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typing `\su` inside `$…$`, `$$…$$`, or a ```math fence pops KaTeX commands with the rendered symbol in the completion row's icon slot. ~250 curated commands (greek, operators, accents, fonts, relations, arrows, functions, delimiters, environments); argument-taking commands insert as snippets — accepting `\frac` lands the cursor in the numerator and Tab moves to the denominator — and big operators scaffold their usual bounds (`\sum` → `\sum_{i=1}^{n}` with each bound selectable in turn). Math detection counts unmatched `$`/`$$` delimiters rather than closed pairs, so a formula still being typed — the moment completion matters — already counts as math. Escaped dollars and non-math code regions are excluded; a fence with the `math` info string is a math region in its own right, matching how remark-math renders it. One more source in the editor's existing autocompletion stack; no new dependencies (katex and @codemirror/autocomplete are already there). Co-Authored-By: Claude Fable 5 --- .../app-core/src/components/EditorPane.tsx | 2 + .../src/lib/cm-latex-completions.test.ts | 79 +++++ .../app-core/src/lib/cm-latex-completions.ts | 279 ++++++++++++++++++ .../app-core/src/lib/cm-slash-commands.ts | 3 + 4 files changed, 363 insertions(+) create mode 100644 packages/app-core/src/lib/cm-latex-completions.test.ts create mode 100644 packages/app-core/src/lib/cm-latex-completions.ts diff --git a/packages/app-core/src/components/EditorPane.tsx b/packages/app-core/src/components/EditorPane.tsx index 53915bda..43fe336f 100644 --- a/packages/app-core/src/components/EditorPane.tsx +++ b/packages/app-core/src/components/EditorPane.tsx @@ -120,6 +120,7 @@ import { mathBlockArrowKeymap } from '../lib/cm-math-nav' import { slashCommandSource, slashCommandRender } from '../lib/cm-slash-commands' import { calloutTypeSource } from '../lib/cm-callouts' import { dateShortcutSource } from '../lib/cm-date-shortcuts' +import { latexCommandSource } from '../lib/cm-latex-completions' import { wikilinkSource, wikilinkHeadingSource, atNoteSource } from '../lib/cm-wikilinks' import { linkRangeAtCursor, markdownLinkAt } from '../lib/internal-links' import { setBlockType, toggleWrap, wrapLink } from '../lib/cm-format' @@ -1779,6 +1780,7 @@ export function EditorPane({ pane }: { pane: PaneLeaf }): JSX.Element { slashCommandSource, calloutTypeSource, dateShortcutSource, + latexCommandSource, atNoteSource, hashtagSource, wikilinkSource, diff --git a/packages/app-core/src/lib/cm-latex-completions.test.ts b/packages/app-core/src/lib/cm-latex-completions.test.ts new file mode 100644 index 00000000..b3287bc5 --- /dev/null +++ b/packages/app-core/src/lib/cm-latex-completions.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from 'vitest' +import { EditorState } from '@codemirror/state' +import { markdown } from '@codemirror/lang-markdown' +import { isInMathContext, latexTokenBefore } from './cm-latex-completions' + +function state(doc: string): EditorState { + return EditorState.create({ doc, extensions: [markdown()] }) +} + +/** Position right after the given marker's first occurrence. */ +function after(doc: string, marker: string): number { + const idx = doc.indexOf(marker) + if (idx === -1) throw new Error(`marker ${marker} not found`) + return idx + marker.length +} + +describe('isInMathContext', () => { + it('detects inline math, including a formula still being typed', () => { + const closed = 'before $a + b$ after' + expect(isInMathContext(state(closed), after(closed, '$a + '))).toBe(true) + expect(isInMathContext(state(closed), after(closed, 'after'))).toBe(false) + expect(isInMathContext(state(closed), after(closed, 'before'))).toBe(false) + + const open = 'text $\\su' + expect(isInMathContext(state(open), open.length)).toBe(true) + }) + + it('detects block math across lines, closed or not', () => { + const closed = 'a\n$$\nx = y\n$$\nb' + expect(isInMathContext(state(closed), after(closed, 'x ='))).toBe(true) + expect(isInMathContext(state(closed), closed.length)).toBe(false) + + const open = 'a\n$$\nx =' + expect(isInMathContext(state(open), open.length)).toBe(true) + }) + + it('treats ```math fences as math, other fences as code', () => { + const mathFence = 'a\n```math\n\\su\n```\nb' + expect(isInMathContext(state(mathFence), after(mathFence, '\\su'))).toBe(true) + + const jsFence = 'a\n```js\nconst x = 1\n```\nb' + expect(isInMathContext(state(jsFence), after(jsFence, 'const x'))).toBe(false) + + const bareFence = 'a\n```\n\\su\n```\nb' + expect(isInMathContext(state(bareFence), after(bareFence, '\\su'))).toBe(false) + }) + + it('ignores escaped dollars and code regions', () => { + const escaped = 'price \\$5 and \\$6 end' + expect(isInMathContext(state(escaped), escaped.length)).toBe(false) + + const fenced = '```\n$a + b$\n```\ntext' + expect(isInMathContext(state(fenced), after(fenced, '$a + '))).toBe(false) + + const inlineCode = 'use `$HOME` now' + expect(isInMathContext(state(inlineCode), after(inlineCode, '`$HO'))).toBe(false) + }) +}) + +describe('latexTokenBefore', () => { + it('matches a backslash command prefix ending at the cursor', () => { + const doc = '$\\sum' + const token = latexTokenBefore(state(doc), doc.length) + expect(token).not.toBeNull() + expect(token!.query).toBe('sum') + expect(token!.from).toBe(1) + }) + + it('matches a bare backslash and rejects non-command contexts', () => { + const bare = '$x + \\' + expect(latexTokenBefore(state(bare), bare.length)!.query).toBe('') + + const rowBreak = '$a \\\\' + expect(latexTokenBefore(state(rowBreak), rowBreak.length)).toBeNull() + + const plain = '$x + y' + expect(latexTokenBefore(state(plain), plain.length)).toBeNull() + }) +}) diff --git a/packages/app-core/src/lib/cm-latex-completions.ts b/packages/app-core/src/lib/cm-latex-completions.ts new file mode 100644 index 00000000..86f355b9 --- /dev/null +++ b/packages/app-core/src/lib/cm-latex-completions.ts @@ -0,0 +1,279 @@ +/** + * LaTeX command completion for math regions: typing `\su` inside `$…$` or + * `$$…$$` pops KaTeX commands (`\sum`, `\sqrt`, …) with a rendered preview. + * Commands that take arguments insert as snippets, so accepting `\frac` + * lands the cursor in the numerator and Tab moves to the denominator. + * + * Math-region detection mirrors cm-math-render's delimiters, but stays a + * cheap unmatched-delimiter scan: while the user is mid-formula the closing + * `$` usually does not exist yet, which is exactly when completion matters. + */ +import type { Completion, CompletionContext, CompletionResult } from '@codemirror/autocomplete' +import { snippet } from '@codemirror/autocomplete' +import { syntaxTree } from '@codemirror/language' +import type { EditorState } from '@codemirror/state' +import katex from 'katex' + +interface LatexCommand { + /** Command as typed, with the backslash: `\sum`. */ + label: string + detail: string + /** Snippet template when the command takes arguments. */ + template?: string + /** LaTeX rendered in the popup preview; defaults to the label. */ + preview?: string + /** Ranking bump for everyday commands. */ + boost?: number +} + +const g = (name: string): LatexCommand => ({ label: `\\${name}`, detail: 'greek' }) +const rel = (name: string): LatexCommand => ({ label: `\\${name}`, detail: 'relation' }) +const arr = (name: string): LatexCommand => ({ label: `\\${name}`, detail: 'arrow' }) +const bin = (name: string): LatexCommand => ({ label: `\\${name}`, detail: 'operator' }) +const fnc = (name: string): LatexCommand => ({ label: `\\${name}`, detail: 'function', preview: `\\${name} x` }) +const sym = (name: string, detail = 'symbol'): LatexCommand => ({ label: `\\${name}`, detail }) +const env = (name: string, inner: string): LatexCommand => ({ + label: `\\${name}`, + detail: 'environment', + template: `\\begin{${name}}\n\t\${}\n\\end{${name}}`, + preview: `\\begin{${name}}${inner}\\end{${name}}` +}) + +const LATEX_COMMANDS: LatexCommand[] = [ + // Everyday constructs, boosted to the top. + { label: '\\frac', detail: 'fraction', template: '\\frac{${}}{${}}', preview: '\\frac{a}{b}', boost: 99 }, + { label: '\\sqrt', detail: 'square root', template: '\\sqrt{${}}', preview: '\\sqrt{x}', boost: 98 }, + { label: '\\sum', detail: 'sum', template: '\\sum_{${i=1}}^{${n}}', preview: '\\sum_{i=1}^{n}', boost: 97 }, + { label: '\\int', detail: 'integral', template: '\\int_{${a}}^{${b}}', preview: '\\int_{a}^{b}', boost: 96 }, + { label: '\\lim', detail: 'limit', template: '\\lim_{${x \\to 0}}', preview: '\\lim_{x \\to 0}', boost: 95 }, + { label: '\\prod', detail: 'product', template: '\\prod_{${i=1}}^{${n}}', preview: '\\prod_{i=1}^{n}', boost: 90 }, + { label: '\\infty', detail: 'infinity', boost: 90 }, + { label: '\\sqrt[n]', detail: 'nth root', template: '\\sqrt[${}]{${}}', preview: '\\sqrt[n]{x}' }, + { label: '\\dfrac', detail: 'display fraction', template: '\\dfrac{${}}{${}}', preview: '\\dfrac{a}{b}' }, + { label: '\\binom', detail: 'binomial', template: '\\binom{${}}{${}}', preview: '\\binom{n}{k}' }, + + // Greek. + ...[ + 'alpha', 'beta', 'gamma', 'delta', 'epsilon', 'varepsilon', 'zeta', 'eta', 'theta', 'vartheta', + 'iota', 'kappa', 'lambda', 'mu', 'nu', 'xi', 'pi', 'rho', 'sigma', 'varsigma', 'tau', 'upsilon', + 'phi', 'varphi', 'chi', 'psi', 'omega', + 'Gamma', 'Delta', 'Theta', 'Lambda', 'Xi', 'Pi', 'Sigma', 'Upsilon', 'Phi', 'Psi', 'Omega' + ].map(g), + + // Big operators, scaffolded with their usual bounds like `\sum` above. + { label: '\\coprod', detail: 'big operator', template: '\\coprod_{${i=1}}^{${n}}', preview: '\\coprod_{i=1}^{n}' }, + { label: '\\iint', detail: 'big operator', template: '\\iint_{${D}}', preview: '\\iint_{D}' }, + { label: '\\iiint', detail: 'big operator', template: '\\iiint_{${V}}', preview: '\\iiint_{V}' }, + { label: '\\oint', detail: 'big operator', template: '\\oint_{${C}}', preview: '\\oint_{C}' }, + { label: '\\limsup', detail: 'big operator', template: '\\limsup_{${n \\to \\infty}}', preview: '\\limsup_{n \\to \\infty}' }, + { label: '\\liminf', detail: 'big operator', template: '\\liminf_{${n \\to \\infty}}', preview: '\\liminf_{n \\to \\infty}' }, + ...['bigcup', 'bigcap', 'bigoplus', 'bigotimes', 'bigsqcup', 'bigvee', 'bigwedge'].map( + (name): LatexCommand => ({ + label: `\\${name}`, + detail: 'big operator', + template: `\\${name}_{\${i}}`, + preview: `\\${name}_{i}` + }) + ), + + // Accents and decorations. + ...[ + ['hat', '\\hat{x}'], ['bar', '\\bar{x}'], ['vec', '\\vec{x}'], ['dot', '\\dot{x}'], ['ddot', '\\ddot{x}'], + ['tilde', '\\tilde{x}'], ['widehat', '\\widehat{xy}'], ['widetilde', '\\widetilde{xy}'], + ['overline', '\\overline{xy}'], ['underline', '\\underline{xy}'], + ['overbrace', '\\overbrace{xy}'], ['underbrace', '\\underbrace{xy}'], ['boxed', '\\boxed{x}'], + ['cancel', '\\cancel{x}'] + ].map(([name, preview]): LatexCommand => ({ + label: `\\${name}`, + detail: 'accent', + template: `\\${name}{\${}}`, + preview + })), + + // Fonts and text. + ...[ + ['text', '\\text{if}'], ['mathrm', '\\mathrm{d}'], ['mathbb', '\\mathbb{R}'], ['mathcal', '\\mathcal{L}'], + ['mathfrak', '\\mathfrak{g}'], ['mathbf', '\\mathbf{v}'], ['mathit', '\\mathit{x}'], + ['mathsf', '\\mathsf{A}'], ['mathtt', '\\mathtt{x}'], ['operatorname', '\\operatorname{op}'] + ].map(([name, preview]): LatexCommand => ({ + label: `\\${name}`, + detail: 'font', + template: `\\${name}{\${}}`, + preview + })), + + // Stacked constructs. + { label: '\\overset', detail: 'stack above', template: '\\overset{${}}{${}}', preview: '\\overset{!}{=}' }, + { label: '\\underset', detail: 'stack below', template: '\\underset{${}}{${}}', preview: '\\underset{n}{\\max}' }, + { label: '\\stackrel', detail: 'stack relation', template: '\\stackrel{${}}{${}}', preview: '\\stackrel{def}{=}' }, + { label: '\\substack', detail: 'stacked subscript', template: '\\substack{${}}', preview: '\\sum_{\\substack{i0 \\\\ b & x\\le 0'), + env('aligned', 'a &= b \\\\ &= c'), + env('gathered', 'a=b \\\\ c=d') +].flat() + +type CodeContext = { kind: 'inline' } | { kind: 'fenced'; lang: string } | null + +function codeContext(state: EditorState, pos: number): CodeContext { + let node = syntaxTree(state).resolveInner(pos, 1) + for (;;) { + const n = node.name + if (n === 'InlineCode') return { kind: 'inline' } + if (n === 'FencedCode' || n === 'CodeBlock') { + const info = node.getChild('CodeInfo') + const lang = info ? state.doc.sliceString(info.from, info.to).trim().toLowerCase() : '' + return { kind: 'fenced', lang } + } + if (!node.parent) return null + node = node.parent + } +} + +/** Inside `$…$`, `$$…$$`, or a ```math fence at `pos`? Counts unmatched + * dollar delimiters so a formula still being typed (no closing `$` yet) + * already counts as math. */ +export function isInMathContext(state: EditorState, pos: number): boolean { + const code = codeContext(state, pos) + // A ```math fence is a math region in its own right (remark-math renders + // it as display math); every other code region shuts completion off. + if (code) return code.kind === 'fenced' && code.lang === 'math' + const before = state.doc.sliceString(0, pos) + const blockFences = before.match(/(? + ({ + label: cmd.label, + detail: cmd.detail, + type: 'keyword', + boost: cmd.boost ?? 0, + _kind: 'latex', + _preview: cmd.preview ?? cmd.label, + apply: cmd.template ? snippet(cmd.template) : undefined + }) as Completion & { _kind: string; _preview: string } + ) + return cachedOptions +} + +export function latexCommandSource(context: CompletionContext): CompletionResult | null { + const token = latexTokenBefore(context.state, context.pos) + if (!token) return null + if (!isInMathContext(context.state, token.from)) return null + return { + from: token.from, + options: buildOptions(), + validFor: /^\\[a-zA-Z]*$/ + } +} + +/** Full option row for a LaTeX completion — the KaTeX-rendered symbol sits in + * the icon slot, then label and detail reuse the slash-command layout. Called + * first from the shared `renderCompletion`; null for every other kind. */ +export function renderLatexCompletion(completion: Completion): HTMLElement | null { + const { _kind, _preview } = completion as Completion & { _kind?: string; _preview?: string } + if (_kind !== 'latex') return null + + const el = document.createElement('div') + el.className = 'slash-cmd-item' + + const icon = document.createElement('span') + icon.className = 'slash-cmd-icon latex-cmd-icon' + icon.style.fontSize = '0.72em' + icon.style.lineHeight = '1' + icon.style.display = 'inline-flex' + icon.style.alignItems = 'center' + icon.style.justifyContent = 'center' + try { + icon.innerHTML = katex.renderToString(_preview ?? completion.label, { throwOnError: false }) + } catch { + icon.textContent = '' + } + + const label = document.createElement('span') + label.className = 'slash-cmd-label' + label.textContent = completion.label + + const detail = document.createElement('span') + detail.className = 'slash-cmd-detail' + detail.textContent = completion.detail ?? '' + + el.appendChild(icon) + el.appendChild(label) + el.appendChild(detail) + return el +} diff --git a/packages/app-core/src/lib/cm-slash-commands.ts b/packages/app-core/src/lib/cm-slash-commands.ts index d387975e..53bdcccf 100644 --- a/packages/app-core/src/lib/cm-slash-commands.ts +++ b/packages/app-core/src/lib/cm-slash-commands.ts @@ -1,6 +1,7 @@ import type { CompletionContext, CompletionResult, Completion } from '@codemirror/autocomplete' import type { EditorView } from '@codemirror/view' import { useStore } from '../store' +import { renderLatexCompletion } from './cm-latex-completions' interface SlashCmd { label: string @@ -67,6 +68,8 @@ const COMMANDS: SlashCmd[] = [ /** Render a custom completion item matching the app theme. */ function renderCompletion(completion: Completion): HTMLElement { + const latex = renderLatexCompletion(completion) + if (latex) return latex const decorated = completion as DecoratedCompletion if (decorated._kind === 'callout') { const el = document.createElement('div') From 4fb67b373d3b40748a73000f6464b45c94976081 Mon Sep 17 00:00:00 2001 From: Junerey Date: Fri, 14 Aug 2026 21:04:54 +0700 Subject: [PATCH 04/18] feat(editor): frontmatter tag autocomplete and clickable tags --- .../app-core/src/components/EditorPane.tsx | 5 +- .../src/components/PinnedReferencePane.tsx | 5 +- .../lib/cm-frontmatter-tag-complete.test.ts | 132 ++++++++++++++++ .../src/lib/cm-frontmatter-tag-complete.ts | 107 +++++++++++++ .../src/lib/cm-frontmatter-tag.test.ts | 78 +++++++++ packages/app-core/src/lib/cm-frontmatter.ts | 149 ++++++++++++++++-- .../src/lib/cm-hashtag-complete.test.ts | 5 + .../app-core/src/lib/cm-hashtag-complete.ts | 35 ++-- packages/app-core/src/styles/index.css | 13 ++ 9 files changed, 499 insertions(+), 30 deletions(-) create mode 100644 packages/app-core/src/lib/cm-frontmatter-tag-complete.test.ts create mode 100644 packages/app-core/src/lib/cm-frontmatter-tag-complete.ts create mode 100644 packages/app-core/src/lib/cm-frontmatter-tag.test.ts diff --git a/packages/app-core/src/components/EditorPane.tsx b/packages/app-core/src/components/EditorPane.tsx index 53915bda..338482fe 100644 --- a/packages/app-core/src/components/EditorPane.tsx +++ b/packages/app-core/src/components/EditorPane.tsx @@ -74,7 +74,7 @@ import { } from '../lib/cm-vim-clipboard' import { wireYankHighlight, yankHighlightExtension } from '../lib/cm-yank-highlight' import { vimVisualHighlightExtension } from '../lib/cm-vim-visual-highlight' -import { frontmatterStyle } from '../lib/cm-frontmatter' +import { frontmatterStyle, frontmatterTagExtension } from '../lib/cm-frontmatter' import { codeBlockFontPlugin } from '../lib/cm-code-block-font' import { orderedListRenumber, @@ -105,6 +105,7 @@ import { hashtagExtension } from '../lib/cm-hashtags' import { taskMetadataExtension } from '../lib/cm-task-metadata' import { taskRollupExtension } from '../lib/cm-task-rollup' import { hashtagSource } from '../lib/cm-hashtag-complete' +import { frontmatterTagSource } from '../lib/cm-frontmatter-tag-complete' import { applyHighlight, HIGHLIGHT_COLORS, highlightExtension } from '../lib/cm-highlight' import { wikilinkRenderExtension } from '../lib/cm-wikilink-render' import { mathRenderExtension } from '../lib/cm-math-render' @@ -404,6 +405,7 @@ function markdownEditingExtensions(showHeadingLevelLabels = false): Extension[] vimAwareMarkdownKeymap, markdownListIndentPlugin, frontmatterStyle, + frontmatterTagExtension, orderedListRenumber, forwardOnCheckboxArrow, headingFolding({ showLevelLabels: showHeadingLevelLabels }), @@ -1780,6 +1782,7 @@ export function EditorPane({ pane }: { pane: PaneLeaf }): JSX.Element { calloutTypeSource, dateShortcutSource, atNoteSource, + frontmatterTagSource, hashtagSource, wikilinkSource, wikilinkHeadingSource diff --git a/packages/app-core/src/components/PinnedReferencePane.tsx b/packages/app-core/src/components/PinnedReferencePane.tsx index 941737bf..6c2e4cd3 100644 --- a/packages/app-core/src/components/PinnedReferencePane.tsx +++ b/packages/app-core/src/components/PinnedReferencePane.tsx @@ -44,13 +44,14 @@ import { autocompletion } from '@codemirror/autocomplete' import { useStore } from '../store' import type { LineNumberMode } from '../store' import { livePreviewPlugin } from '../lib/cm-live-preview' -import { frontmatterStyle } from '../lib/cm-frontmatter' +import { frontmatterStyle, frontmatterTagExtension } from '../lib/cm-frontmatter' import { headingFolding } from '../lib/cm-heading-fold' import { slashCommandSource, slashCommandRender } from '../lib/cm-slash-commands' import { calloutTypeSource } from '../lib/cm-callouts' import { dateShortcutSource } from '../lib/cm-date-shortcuts' import { wikilinkSource, wikilinkHeadingSource } from '../lib/cm-wikilinks' import { hashtagSource } from '../lib/cm-hashtag-complete' +import { frontmatterTagSource } from '../lib/cm-frontmatter-tag-complete' import { completionKeymapForEditor, completionNavKeymap } from '../lib/cm-completion-nav' import { classifyLocalAssetHref, hrefFragment, type LocalAssetKind } from '../lib/local-assets' import { LazyPreview as Preview } from './LazyPreview' @@ -224,6 +225,7 @@ export function PinnedReferencePane(): JSX.Element | null { vimAwareMarkdownKeymap, markdownListIndentPlugin, frontmatterStyle, + frontmatterTagExtension, headingCompartment.of( headingFolding({ showLevelLabels: s0.showHeadingLevelLabels }) ), @@ -240,6 +242,7 @@ export function PinnedReferencePane(): JSX.Element | null { slashCommandSource, calloutTypeSource, dateShortcutSource, + frontmatterTagSource, hashtagSource, wikilinkSource, wikilinkHeadingSource diff --git a/packages/app-core/src/lib/cm-frontmatter-tag-complete.test.ts b/packages/app-core/src/lib/cm-frontmatter-tag-complete.test.ts new file mode 100644 index 00000000..dce23508 --- /dev/null +++ b/packages/app-core/src/lib/cm-frontmatter-tag-complete.test.ts @@ -0,0 +1,132 @@ +// @vitest-environment jsdom + +import { CompletionContext } from '@codemirror/autocomplete' +import { EditorState } from '@codemirror/state' +import { EditorView } from '@codemirror/view' +import { describe, expect, it, vi } from 'vitest' +import { frontmatterTagSource } from './cm-frontmatter-tag-complete' + +const meta = (path: string, folder: 'inbox' | 'trash', tags: string[]) => ({ + path, + title: path.split('/').pop()!.replace(/\.md$/, ''), + folder, + siblingOrder: 0, + createdAt: 0, + updatedAt: 0, + size: 0, + tags, + wikilinks: [], + hasAttachments: false, + excerpt: '' +}) + +const storeState = vi.hoisted(() => ({ + activeNote: { path: 'inbox/Active.md', title: 'Active', folder: 'inbox' as const, body: '' } +})) as { + activeNote: { path: string; title: string; folder: 'inbox'; body: string } + notes: ReturnType[] +} +storeState.notes = [ + meta('inbox/A.md', 'inbox', ['project', 'idea', 'work/deep']), + meta('inbox/B.md', 'inbox', ['project', 'projectplan', 'todo']), + meta('trash/Old.md', 'trash', ['project', 'projecttrash']) +] + +vi.mock('../store', () => { + const useStore = Object.assign(() => null, { getState: () => storeState }) + return { useStore } +}) + +function result(doc: string, pos: number) { + const state = EditorState.create({ doc }) + return frontmatterTagSource(new CompletionContext(state, pos, true)) +} + +describe('frontmatterTagSource', () => { + it('suggests tags inside an inline list', () => { + const doc = '---\ntags: [pro\n---\n' + const pos = doc.indexOf('\n---\n') // end of the tags line, before the closing fence + const r = result(doc, pos) + expect(r?.options.map((o) => o.label)).toEqual(['project', 'projectplan']) + }) + + it('suggests tags inside a scalar value', () => { + const doc = '---\ntags: pro\n---\n' + const pos = doc.indexOf('\n---\n') + const r = result(doc, pos) + expect(r?.options.map((o) => o.label)).toEqual(['project', 'projectplan']) + }) + + it('suggests tags inside a block list under a bare tags key', () => { + const doc = '---\ntags:\n - pro\n---\n' + const pos = doc.indexOf('\n---\n') + const r = result(doc, pos) + expect(r?.options.map((o) => o.label)).toEqual(['project', 'projectplan']) + }) + + it('does not suggest outside frontmatter', () => { + expect(result('tags: pro', 'tags: pro'.length)).toBeNull() + expect(result('---\nbody\n---\ntags: pro', '---\nbody\n---\ntags: pro'.length)).toBeNull() + }) + + it('does not suggest on other frontmatter keys', () => { + expect(result('---\ntitle: pro\n---\n', '---\ntitle: pro'.length)).toBeNull() + }) + + it('does not suggest before the list marker', () => { + const doc = '---\ntags:\n - \n---\n' + const pos = '---\ntags:\n - '.length + expect(result(doc, pos)).toBeNull() + }) + + it('does not suggest while the cursor is still in the key', () => { + const doc = '---\ntags: pro\n---\n' + const pos = '---\ntags'.length + expect(result(doc, pos)).toBeNull() + }) + + it('inserts the tag without a leading hash', () => { + const parent = document.createElement('div') + document.body.append(parent) + const doc = '---\ntags: [pro]\n---\n' + const view = new EditorView({ parent, state: EditorState.create({ doc }) }) + const pos = doc.indexOf(']') + const r = frontmatterTagSource(new CompletionContext(view.state, pos, true)) + const option = r?.options.find((o) => o.label === 'project') + if (typeof option?.apply !== 'function') throw new Error('expected apply function') + option.apply(view, option, r!.from, pos) + expect(view.state.doc.toString()).toBe('---\ntags: [project]\n---\n') + view.destroy() + parent.remove() + }) + + it('keeps surrounding quotes intact', () => { + const parent = document.createElement('div') + document.body.append(parent) + const doc = '---\ntags: ["pro"]\n---\n' + const view = new EditorView({ parent, state: EditorState.create({ doc }) }) + const pos = doc.indexOf('"]') + const r = frontmatterTagSource(new CompletionContext(view.state, pos, true)) + const option = r?.options.find((o) => o.label === 'project') + if (typeof option?.apply !== 'function') throw new Error('expected apply function') + option.apply(view, option, r!.from, pos) + expect(view.state.doc.toString()).toBe('---\ntags: ["project"]\n---\n') + view.destroy() + parent.remove() + }) + + it('consumes a stray leading # when completing', () => { + const parent = document.createElement('div') + document.body.append(parent) + const doc = '---\ntags: [#pro]\n---\n' + const view = new EditorView({ parent, state: EditorState.create({ doc }) }) + const pos = doc.indexOf(']') + const r = frontmatterTagSource(new CompletionContext(view.state, pos, true)) + const option = r?.options.find((o) => o.label === 'project') + if (typeof option?.apply !== 'function') throw new Error('expected apply function') + option.apply(view, option, r!.from, pos) + expect(view.state.doc.toString()).toBe('---\ntags: [project]\n---\n') + view.destroy() + parent.remove() + }) +}) diff --git a/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts b/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts new file mode 100644 index 00000000..3a55c18b --- /dev/null +++ b/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts @@ -0,0 +1,107 @@ +/** + * Frontmatter `tags:` autocomplete. Typing inside the value of a frontmatter + * `tags:` field (inline list, scalar, or block-list form) surfaces the same + * existing-tag suggestions as inline `#tags`, but without the leading `#`. + */ +import type { Completion, CompletionContext, CompletionResult } from '@codemirror/autocomplete' +import type { EditorState } from '@codemirror/state' +import { collectTagCounts, rankTagCompletions } from './cm-hashtag-complete' +import { isInsideFrontmatter } from './cm-frontmatter' + +/** Characters that terminate a tag token when scanning forward or backward + * for the *body* of the token. A leading `#` is intentionally not a start + * delimiter: if someone types `#pro` in a frontmatter value, the `#` is + * consumed and replaced with the selected tag. */ +const TOKEN_BODY_DELIMITERS = /[\s,\[\]"'#]/ +const TOKEN_START_DELIMITERS = /[\s,\[\]"']/ + +function tokenEndAt(state: EditorState, pos: number): number { + const line = state.doc.lineAt(pos) + const text = line.text + const col = pos - line.from + let i = col + while (i < text.length && !TOKEN_BODY_DELIMITERS.test(text[i] as string)) i++ + return line.from + i +} + +function tagTokenAt( + state: EditorState, + lineStart: number, + valueStart: number, + pos: number +): { from: number; query: string } | null { + const line = state.doc.lineAt(lineStart) + const text = line.text + const cursor = pos - line.from + if (cursor < valueStart) return null + let i = cursor - 1 + while (i >= valueStart && !TOKEN_START_DELIMITERS.test(text[i] as string)) i-- + const tokenStart = i + 1 + const token = text.slice(tokenStart, cursor) + if (token.length < 1) return null + return { from: line.from + tokenStart, query: token.replace(/^#/, '') } +} + +function isUnderTagsKey(state: EditorState, lineNo: number): boolean { + for (let i = lineNo - 1; i >= 2; i--) { + const text = state.doc.line(i).text + const trimmed = text.trim() + if (!trimmed || trimmed.startsWith('#')) continue + const key = text.match(/^([A-Za-z0-9_][\w-]*)\s*:\s*(.*)$/) + if (key) { + return key[1].toLowerCase() === 'tags' && key[2].trim() === '' + } + if (/^\s*-\s+/.test(text)) continue + return false + } + return false +} + +function frontmatterTagMatch(context: CompletionContext): { from: number; query: string } | null { + const { state, pos } = context + if (!isInsideFrontmatter(state, pos)) return null + const line = state.doc.lineAt(pos) + const text = line.text + const col = pos - line.from + + const inline = text.match(/^(\s*)tags\s*:\s*(.*)$/) + if (inline) { + const valueStart = inline[0].length - (inline[2] as string).length + if (col < valueStart) return null + return tagTokenAt(state, line.from, valueStart, pos) + } + + const item = text.match(/^(\s*)-\s+(.*)$/) + if (item) { + if (!isUnderTagsKey(state, line.number)) return null + const valueStart = item[0].length - (item[2] as string).length + if (col < valueStart) return null + return tagTokenAt(state, line.from, valueStart, pos) + } + + return null +} + +export function frontmatterTagSource(context: CompletionContext): CompletionResult | null { + const match = frontmatterTagMatch(context) + if (!match || match.query.length < 1) return null + + const ranked = rankTagCompletions(match.query, collectTagCounts()) + if (ranked.length === 0) return null + + const options: Completion[] = ranked.map(({ tag, count }) => ({ + label: tag, + displayLabel: tag, + detail: count > 1 ? `${count}` : '', + _icon: '#', + apply: (view, _completion, _from, to) => { + const end = tokenEndAt(view.state, to) + view.dispatch({ + changes: { from: match.from, to: end, insert: tag }, + selection: { anchor: match.from + tag.length } + }) + } + })) + + return { from: match.from, options, filter: false } +} diff --git a/packages/app-core/src/lib/cm-frontmatter-tag.test.ts b/packages/app-core/src/lib/cm-frontmatter-tag.test.ts new file mode 100644 index 00000000..b5913812 --- /dev/null +++ b/packages/app-core/src/lib/cm-frontmatter-tag.test.ts @@ -0,0 +1,78 @@ +// @vitest-environment jsdom + +import { EditorState } from '@codemirror/state' +import { EditorView } from '@codemirror/view' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { frontmatterTagExtension } from './cm-frontmatter' + +const openTagView = vi.fn() + +vi.mock('../store', () => { + const useStore = Object.assign(() => null, { + getState: () => ({ openTagView }) + }) + return { useStore } +}) + +const views: EditorView[] = [] +function mount(doc: string): EditorView { + const parent = document.createElement('div') + document.body.append(parent) + const view = new EditorView({ + parent, + state: EditorState.create({ doc, extensions: [frontmatterTagExtension] }) + }) + views.push(view) + return view +} + +afterEach(() => { + openTagView.mockClear() + while (views.length) views.pop()!.destroy() +}) + +function tagsIn(view: EditorView): string[] { + return Array.from(view.dom.querySelectorAll('.cm-frontmatter-tag')).map( + (el) => (el as HTMLElement).dataset.tag ?? '' + ) +} + +describe('frontmatterTagExtension', () => { + it('marks inline list tags and strips quotes', () => { + const view = mount(['---', 'tags: [idea, "work/deep", \'project\']', '---', ''].join('\n')) + expect(tagsIn(view)).toEqual(['idea', 'work/deep', 'project']) + }) + + it('marks scalar tags split by comma or whitespace', () => { + const view = mount(['---', 'tags: daily, work', '---', ''].join('\n')) + expect(tagsIn(view)).toEqual(['daily', 'work']) + }) + + it('marks block list tags under a bare tags key', () => { + const view = mount(['---', 'tags:', ' - idea', ' - "project"', '---', ''].join('\n')) + expect(tagsIn(view)).toEqual(['idea', 'project']) + }) + + it('strips a stray leading # from frontmatter tags', () => { + const view = mount(['---', 'tags: [#idea]', '---', ''].join('\n')) + expect(tagsIn(view)).toEqual(['idea']) + }) + + it('does not mark tags on other frontmatter keys', () => { + const view = mount(['---', 'title: idea', '---', ''].join('\n')) + expect(tagsIn(view)).toEqual([]) + }) + + it('does not mark tags outside frontmatter', () => { + const view = mount(['---', 'title: x', '---', '', 'tags: idea'].join('\n')) + expect(tagsIn(view)).toEqual([]) + }) + + it('opens the tag view when a frontmatter tag is clicked', () => { + const view = mount(['---', 'tags: [idea]', '---', ''].join('\n')) + const el = view.dom.querySelector('.cm-frontmatter-tag') as HTMLElement | null + expect(el).not.toBeNull() + el!.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, cancelable: true })) + expect(openTagView).toHaveBeenCalledWith('idea') + }) +}) diff --git a/packages/app-core/src/lib/cm-frontmatter.ts b/packages/app-core/src/lib/cm-frontmatter.ts index 9321a1b4..fc3e628c 100644 --- a/packages/app-core/src/lib/cm-frontmatter.ts +++ b/packages/app-core/src/lib/cm-frontmatter.ts @@ -5,7 +5,7 @@ * database "record page" notes (whose properties live in frontmatter) read like * a property list rather than a wall of big text. */ -import { RangeSetBuilder } from '@codemirror/state' +import { type EditorState, RangeSetBuilder } from '@codemirror/state' import { Decoration, type DecorationSet, @@ -13,6 +13,27 @@ import { ViewPlugin, type ViewUpdate } from '@codemirror/view' +import { useStore } from '../store' + +/** Range of a closed leading `---` … `---` frontmatter block, or null if the + * document does not start with one. Used by autocomplete to avoid offering + * inline `#tags` inside frontmatter and to offer tags inside frontmatter + * `tags:` fields. */ +export function frontmatterRange(state: EditorState): { from: number; to: number } | null { + const doc = state.doc + if (doc.lines < 2 || doc.line(1).text.trim() !== '---') return null + for (let i = 2; i <= doc.lines; i++) { + if (doc.line(i).text.trim() === '---') { + return { from: doc.line(1).from, to: doc.line(i).to } + } + } + return null +} + +export function isInsideFrontmatter(state: EditorState, pos: number): boolean { + const range = frontmatterRange(state) + return range != null && pos >= range.from && pos <= range.to +} const FRONTMATTER_LINE = Decoration.line({ class: 'cm-frontmatter-line' }) const FRONTMATTER_TOP = Decoration.line({ class: 'cm-frontmatter-line cm-frontmatter-top' }) @@ -21,27 +42,21 @@ const FRONTMATTER_KEY = Decoration.mark({ class: 'cm-frontmatter-key' }) function buildFrontmatterDeco(view: EditorView): DecorationSet { const builder = new RangeSetBuilder() + const range = frontmatterRange(view.state) + if (!range) return builder.finish() const doc = view.state.doc - // Frontmatter must start on line 1 with an exact `---` fence. - if (doc.lines < 2 || doc.line(1).text.trim() !== '---') return builder.finish() - let endLine = -1 - for (let i = 2; i <= doc.lines; i++) { - if (doc.line(i).text.trim() === '---') { - endLine = i - break - } - } - if (endLine === -1) return builder.finish() - for (let i = 1; i <= endLine; i++) { + const startLine = doc.lineAt(range.from).number + const endLine = doc.lineAt(range.to).number + for (let i = startLine; i <= endLine; i++) { const line = doc.line(i) // Line decoration first (its start side sorts before any mark at the same // offset), then the key mark for property lines. builder.add( line.from, line.from, - i === 1 ? FRONTMATTER_TOP : i === endLine ? FRONTMATTER_BOTTOM : FRONTMATTER_LINE + i === startLine ? FRONTMATTER_TOP : i === endLine ? FRONTMATTER_BOTTOM : FRONTMATTER_LINE ) - if (i !== 1 && i !== endLine) { + if (i !== startLine && i !== endLine) { // Mark the key (text before the first `:`) so it reads as a muted label // next to its value — a metadata panel, not a wall of text. const colon = line.text.indexOf(':') @@ -63,3 +78,109 @@ export const frontmatterStyle = ViewPlugin.fromClass( }, { decorations: (v) => v.decorations } ) + +const TAG_TOKEN_RE = /[^,\s\[\]"'#]+/g + +/** Which frontmatter lines are `- item` entries under a bare `tags:` key. */ +function tagsBlockLineNumbers(state: EditorState): Set { + const range = frontmatterRange(state) + if (!range) return new Set() + const doc = state.doc + const startLine = doc.lineAt(range.from).number + const endLine = doc.lineAt(range.to).number + const lines = new Set() + let inTags = false + for (let n = startLine + 1; n < endLine; n++) { + const text = doc.line(n).text + const trimmed = text.trim() + if (!trimmed || trimmed.startsWith('#')) continue + const key = text.match(/^([A-Za-z0-9_][\w-]*)\s*:\s*(.*)$/) + if (key) { + inTags = key[1].toLowerCase() === 'tags' && key[2].trim() === '' + continue + } + if (inTags && /^\s*-\s+/.test(text)) { + lines.add(n) + continue + } + if (!/^\s/.test(text)) inTags = false + } + return lines +} + +function addTagTokens(value: string, valueStartAbs: number, builder: RangeSetBuilder): void { + TAG_TOKEN_RE.lastIndex = 0 + let m: RegExpExecArray | null + while ((m = TAG_TOKEN_RE.exec(value)) !== null) { + const token = m[0] + const tag = token.replace(/^#/, '') + if (!tag) continue + const from = valueStartAbs + m.index + (token.length - tag.length) + const to = from + tag.length + builder.add( + from, + to, + Decoration.mark({ class: 'cm-frontmatter-tag', attributes: { 'data-tag': tag } }) + ) + } +} + +function buildFrontmatterTagDeco(view: EditorView): DecorationSet { + const builder = new RangeSetBuilder() + const range = frontmatterRange(view.state) + if (!range) return builder.finish() + const doc = view.state.doc + const startLine = doc.lineAt(range.from).number + const endLine = doc.lineAt(range.to).number + const blockLines = tagsBlockLineNumbers(view.state) + for (let n = startLine + 1; n < endLine; n++) { + const line = doc.line(n) + const text = line.text + const trimmed = text.trim() + if (!trimmed || trimmed.startsWith('#')) continue + const inline = text.match(/^(\s*)tags\s*:\s*(.*)$/) + if (inline) { + const value = inline[2] as string + const valueStart = line.from + inline[0].length - value.length + addTagTokens(value, valueStart, builder) + continue + } + if (blockLines.has(n)) { + const item = text.match(/^(\s*)-\s+(.*)$/) + if (item) { + const value = item[2] as string + const valueStart = line.from + item[0].length - value.length + addTagTokens(value, valueStart, builder) + } + } + } + return builder.finish() +} + +const frontmatterTagPlugin = ViewPlugin.fromClass( + class { + decorations: DecorationSet + constructor(view: EditorView) { + this.decorations = buildFrontmatterTagDeco(view) + } + update(update: ViewUpdate): void { + if (update.docChanged) this.decorations = buildFrontmatterTagDeco(update.view) + } + }, + { decorations: (v) => v.decorations } +) + +// Clicking a frontmatter tag opens the tag view, mirroring inline hashtags. +const frontmatterTagClick = EditorView.domEventHandlers({ + mousedown: (event) => { + const target = event.target as HTMLElement | null + const el = target?.closest('.cm-frontmatter-tag') + const tag = el?.dataset.tag + if (!tag) return false + event.preventDefault() + void useStore.getState().openTagView(tag) + return true + } +}) + +export const frontmatterTagExtension = [frontmatterTagPlugin, frontmatterTagClick] diff --git a/packages/app-core/src/lib/cm-hashtag-complete.test.ts b/packages/app-core/src/lib/cm-hashtag-complete.test.ts index 6b688209..9dad87b9 100644 --- a/packages/app-core/src/lib/cm-hashtag-complete.test.ts +++ b/packages/app-core/src/lib/cm-hashtag-complete.test.ts @@ -96,6 +96,11 @@ describe('hashtagSource (#410 — hashtag autocomplete)', () => { expect(result('#project')?.options.map((o) => o.label)).toEqual(['projectplan']) }) + it('does not suggest inside frontmatter', () => { + const doc = '---\ntags: #pro\n---\n' + expect(result(doc)).toBeNull() + }) + it('does not suggest inside a fenced code block', () => { const parent = document.createElement('div') document.body.append(parent) diff --git a/packages/app-core/src/lib/cm-hashtag-complete.ts b/packages/app-core/src/lib/cm-hashtag-complete.ts index db7557d6..fc78df73 100644 --- a/packages/app-core/src/lib/cm-hashtag-complete.ts +++ b/packages/app-core/src/lib/cm-hashtag-complete.ts @@ -16,6 +16,7 @@ import { useStore } from '../store' import { noteTagsForCount } from './tags' import { resolveTypstPreambleFolder } from './typst-preamble' import { isTagSkippedContext } from './cm-hashtags' +import { isInsideFrontmatter } from './cm-frontmatter' /** Completion carrying the `_icon` the shared slash renderer reads. */ type HashtagCompletion = Completion & { _icon?: string } @@ -43,7 +44,7 @@ function hashtagMatch(context: CompletionContext): { from: number; query: string * them. The active note is read live from its buffer so a tag just typed in the * same note is offered too. Mirrors the aggregation in `TagView`. */ -function collectTagCounts(): Map { +export function collectTagCounts(): Map { const state = useStore.getState() const activePath = state.activeNote?.path ?? null const activeBody = state.activeNote?.body ?? null @@ -61,28 +62,34 @@ function collectTagCounts(): Map { return counter } -export function hashtagSource(context: CompletionContext): CompletionResult | null { - const match = hashtagMatch(context) - // Require at least one character after `#` so a bare `#` (headings, an empty - // token) doesn't flash the menu; suggestions appear once a tag is being typed. - if (!match || match.query.length < 1) return null - // Don't suggest where a `#` isn't a tag (code spans/blocks, headings). - if (isTagSkippedContext(context.state, context.pos)) return null +export interface RankedTag { tag: string; count: number } - const q = match.query.toLowerCase() - const ranked = [...collectTagCounts().entries()] +/** Rank vault tags for `query` so prefix matches beat substring matches, and + * more-used tags beat less-used ones. Excludes the exact tag already typed. */ +export function rankTagCompletions(query: string, counts: Map): RankedTag[] { + const q = query.toLowerCase() + return [...counts.entries()] .map(([tag, count]) => { const lower = tag.toLowerCase() - // Prefix matches rank above substring matches; then by usage, then name. const rank = lower.startsWith(q) ? 0 : lower.includes(q) ? 1 : 2 return { tag, lower, count, rank } }) - // Drop non-matches and the exact tag already typed — completing to what's - // on screen is a no-op, and the live buffer read would otherwise suggest the - // in-progress tag back to itself. .filter((t) => t.rank < 2 && t.lower !== q) .sort((a, b) => a.rank - b.rank || b.count - a.count || a.tag.localeCompare(b.tag)) .slice(0, MAX_SUGGESTIONS) + .map(({ tag, count }) => ({ tag, count })) +} + +export function hashtagSource(context: CompletionContext): CompletionResult | null { + const match = hashtagMatch(context) + // Require at least one character after `#` so a bare `#` (headings, an empty + // token) doesn't flash the menu; suggestions appear once a tag is being typed. + if (!match || match.query.length < 1) return null + // Don't suggest where a `#` isn't a tag (code spans/blocks, headings, frontmatter). + if (isTagSkippedContext(context.state, context.pos)) return null + if (isInsideFrontmatter(context.state, context.pos)) return null + + const ranked = rankTagCompletions(match.query, collectTagCounts()) if (ranked.length === 0) return null const options: Completion[] = ranked.map( diff --git a/packages/app-core/src/styles/index.css b/packages/app-core/src/styles/index.css index 70a4c4b9..3a334f52 100644 --- a/packages/app-core/src/styles/index.css +++ b/packages/app-core/src/styles/index.css @@ -5372,6 +5372,19 @@ html[data-completed-task-style="gray-strikethrough"] .prose-zen li.task-list-ite background-color: rgb(var(--z-accent) / 0.2); } +/* Frontmatter `tags:` values are also clickable chips, styled a little smaller + than inline hashtags so they sit comfortably in the compact metadata block. */ +.cm-wysiwyg .cm-editor .cm-frontmatter-tag { + color: rgb(var(--z-accent)); + cursor: pointer; + border-radius: 0.35em; + padding: 0.05em 0.35em; + background-color: rgb(var(--z-accent) / 0.08); +} +.cm-wysiwyg .cm-editor .cm-frontmatter-tag:hover { + background-color: rgb(var(--z-accent) / 0.15); +} + /* Task metadata on task lines (#454): priorities, due dates, and @fields get the same at-a-glance cues as the Tasks view, so they stand out from the task text. All three are chips — a tinted background plus the coloured text — and From 8926e34c796ca03de8f53cf03f79f83341828e43 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 09:30:22 -0500 Subject: [PATCH 05/18] Fix(vault): saving a note is atomic everywhere, and never replaces a symlink (#585) The desktop stopped wiping notes in 376245e by writing them atomically, but the self-hosted server still wrote truncate-then-write, so a browser or a second device could read a note in the moment its own save had emptied it. A Go test that reads the file while WriteNote runs proves it: against the old implementation a reader sees 0 bytes, the same empty read that erased notes on the desktop. The server writes atomically now. Verifying that turned up three things a rename does that a plain write did not, two of them regressions the desktop change had already shipped. A rename replaces the directory entry, not the file. Pointed at a symlinked note it leaves a regular file where the link was, so a note kept in one place and linked into the vault silently becomes two files. The same call writes config.toml, so a stow or chezmoi managed ~/.config/zennotes/config.toml was being replaced by a regular file on startup with the dotfiles copy never seeing another change, and that one predates this cycle. Both writers now resolve the link and do the atomic dance at the target, which is what workflow-apply.ts had already worked out for workflow edits. A rename also carries the temp file's permissions, so a note chmod'ed to 0600 came back at the default: an existing file's mode is now reproduced exactly, while files these calls create are left umasked exactly as they were. The event kinds changed too, and clients read them. Linux maps a rename into place to IN_MOVED_TO, which fsnotify folds into Create, so an atomic save reaches clients as "add" rather than "change", and the renderer ignored "add" and would have gone on showing content that no longer existed on disk. It now treats "add" for a note it holds open as content to read, which also repairs edits made by git, rsync, Syncthing or vim, none of which ever refreshed an open tab. macOS reports the same rename as a delete followed by a create, and a client told its open note was deleted closes the tab, so the watcher stats the path and never calls an existing file unlinked. Both watchers drop the scratch file itself, which otherwise made every keystroke-driven save re-list the whole asset tree. Verified past the unit tests: the real server binary driven over its change feed the way a web client subscribes (no scratch events, symlink intact and written through, 0600 kept), the full Go suite and an inotify probe inside a Linux container, and the #585 reproductions replayed against the rebuilt desktop app. --- apps/desktop/src/main/vault.test.ts | 57 +++++ apps/desktop/src/main/vault.ts | 71 +++++- apps/desktop/src/main/watcher.ts | 10 +- apps/desktop/src/main/workflow-apply.ts | 10 +- apps/server/internal/vault/atomicwrite.go | 136 +++++++++++ .../server/internal/vault/atomicwrite_test.go | 211 ++++++++++++++++++ apps/server/internal/vault/vault.go | 5 +- apps/server/internal/watcher/watcher.go | 24 +- apps/server/internal/watcher/watcher_test.go | 87 ++++++++ .../app-core/src/store-note-integrity.test.ts | 40 ++++ packages/app-core/src/store.ts | 7 +- 11 files changed, 633 insertions(+), 25 deletions(-) create mode 100644 apps/server/internal/vault/atomicwrite.go create mode 100644 apps/server/internal/vault/atomicwrite_test.go diff --git a/apps/desktop/src/main/vault.test.ts b/apps/desktop/src/main/vault.test.ts index d524ec33..ebddd60b 100644 --- a/apps/desktop/src/main/vault.test.ts +++ b/apps/desktop/src/main/vault.test.ts @@ -36,6 +36,7 @@ import { setVaultSettings, unarchiveNote, vaultChangeAffectsSettings, + isAtomicWriteTempPath, writeNote } from './vault' @@ -1168,3 +1169,59 @@ describe('per-vault view settings round-trip (#292)', () => { expect((await getVaultSettings(root)).view).toBeUndefined() }) }) + +// #585 made note saves atomic (temp file + rename) so no reader can ever see a +// half-written note. A rename replaces the directory entry, so these are the +// properties the plain fs.writeFile gave for free and that the atomic write has +// to put back deliberately. +describe('writeNote atomic-save fidelity (#585)', () => { + it('writes THROUGH a symlinked note instead of replacing the link', async () => { + const root = await makeTempDir('zennotes-atomic-symlink-') + await ensureVaultLayout(root) + const srcDir = await makeTempDir('zennotes-atomic-symlink-src-') + const external = path.join(srcDir, 'External.md') + await writeFile(external, '# External\n\noriginal\n', 'utf8') + + const link = path.join(root, 'inbox', 'Linked.md') + try { + await symlink(external, link) + } catch { + // Creating symlinks can require privileges (e.g. Windows); skip there. + return + } + + await writeNote(root, 'inbox/Linked.md', '# External\n\nedited through the link\n') + + expect((await fsPromises.lstat(link)).isSymbolicLink()).toBe(true) + expect(await readFile(external, 'utf8')).toBe('# External\n\nedited through the link\n') + }) + + it('leaves an existing note its own permissions', async () => { + if (process.platform === 'win32') return + const root = await makeTempDir('zennotes-atomic-mode-') + await ensureVaultLayout(root) + const abs = path.join(root, 'inbox', 'Private.md') + await writeFile(abs, '# Private\n', 'utf8') + await chmod(abs, 0o600) + + await writeNote(root, 'inbox/Private.md', '# Private\n\nsecond draft\n') + + expect((await stat(abs)).mode & 0o777).toBe(0o600) + }) + + it('leaves no scratch file behind', async () => { + const root = await makeTempDir('zennotes-atomic-scratch-') + await ensureVaultLayout(root) + await writeNote(root, 'inbox/Note.md', 'one') + await writeNote(root, 'inbox/Note.md', 'two') + + const entries = await fsPromises.readdir(path.join(root, 'inbox')) + expect(entries.filter((name) => name.endsWith('.tmp'))).toEqual([]) + }) + + it('recognizes its own scratch files without swallowing user files', () => { + expect(isAtomicWriteTempPath('inbox/Note.md.4123.1786714355519000.tmp')).toBe(true) + expect(isAtomicWriteTempPath('inbox/Note.md')).toBe(false) + expect(isAtomicWriteTempPath('inbox/report.2024.01.tmp')).toBe(false) + }) +}) diff --git a/apps/desktop/src/main/vault.ts b/apps/desktop/src/main/vault.ts index 9bff9dfb..b85d6c29 100644 --- a/apps/desktop/src/main/vault.ts +++ b/apps/desktop/src/main/vault.ts @@ -655,16 +655,72 @@ export async function saveConfig(cfg: PersistedConfig): Promise { } } +/** The scratch file `writeFileAtomic` renames from: `...tmp`. + * The Go server writes the same shape and both watchers filter on it, so the + * two must stay recognizable to each other. Requiring an epoch-length stamp is + * what keeps a file the user named `notes.2024.01.tmp` out of the filter: + * events dropped here are events no window ever hears about. */ +const ATOMIC_WRITE_TEMP_PATTERN = /\.\d+\.\d{13,}\.tmp$/ + +export function isAtomicWriteTempPath(p: string): boolean { + return ATOMIC_WRITE_TEMP_PATTERN.test(path.basename(p)) +} + +/** Same millisecond, same path, two writers: the stamp alone would name one + * temp file for both and let them interleave into it. */ +let atomicWriteSequence = 0 + +/** Follow a symlink to the file it points at, so an atomic write lands on the + * target instead of replacing the link. A dangling link resolves to the path + * it names, which is where a plain write would have created the file. */ +async function atomicWriteTarget(absPath: string): Promise { + let stats + try { + stats = await fs.lstat(absPath) + } catch { + return absPath + } + if (!stats.isSymbolicLink()) return absPath + try { + return await fs.realpath(absPath) + } catch { + return path.resolve(path.dirname(absPath), await fs.readlink(absPath)) + } +} + /** * Atomically write a file: temp file + fsync + rename. The rename is atomic, so - * readers never see a half-written file. Exposed for the databases feature - * (CSV + sidecar). No `.bak` is left behind — those files live next to the - * user's data and are just clutter. + * readers never see a half-written file, which is what stops a note save from + * being read back as an empty note by the watcher echo (#585). Exposed for the + * databases feature (CSV + sidecar). No `.bak` is left behind — those files live + * next to the user's data and are just clutter. + * + * A rename replaces the DIRECTORY ENTRY, so two things a plain `fs.writeFile` + * gave for free have to be put back deliberately: + * + * - A symlink is written THROUGH, not over. Pointed straight at one, the rename + * would leave a regular file where the link was and detach it from its target + * for good: a note the user sees in two places becomes two files, and a + * `config.toml` managed by stow or chezmoi quietly stops being managed. + * - An existing file keeps its own permissions. `fs.writeFile` only applies a + * mode when it creates the file, so a note someone chmod'ed to 0600 must not + * come back 0644 after an edit. Files this call creates are left to the + * default, exactly as before. */ export async function writeFileAtomic(absPath: string, data: string): Promise { - const tmp = `${absPath}.${process.pid}.${Date.now()}.tmp` - await fs.mkdir(path.dirname(absPath), { recursive: true }) - const handle = await fs.open(tmp, 'w') + const target = await atomicWriteTarget(absPath) + atomicWriteSequence = (atomicWriteSequence + 1) % 1000 + const stamp = `${Date.now()}${String(atomicWriteSequence).padStart(3, '0')}` + const tmp = `${target}.${process.pid}.${stamp}.tmp` + await fs.mkdir(path.dirname(target), { recursive: true }) + const existingMode = await fs + .stat(target) + .then((s) => s.mode & 0o777) + .catch(() => null) + // 'wx' rather than 'w': a temp file that somehow already exists means another + // writer is mid-flight, and failing the save (the note stays dirty and the + // next save retries) beats two writers sharing one temp file. + const handle = await fs.open(tmp, 'wx') try { await handle.writeFile(data, 'utf8') try { @@ -676,7 +732,8 @@ export async function writeFileAtomic(absPath: string, data: string): Promise...tmp` is writeFileAtomic's scratch file - // (note saves, database saves). Its add/unlink pair is not a vault - // change; without this filter every save also fired an asset refresh. - if (/\.\d+\.\d+\.tmp$/.test(base)) return true + // writeFileAtomic's scratch file (note saves, database saves). Its + // add/unlink pair is not a vault change; without this filter every save + // also fired an asset refresh. + if (isAtomicWriteTempPath(p)) return true return base.startsWith('.') || base === 'node_modules' }, awaitWriteFinish: { diff --git a/apps/desktop/src/main/workflow-apply.ts b/apps/desktop/src/main/workflow-apply.ts index 39386a8d..06ec58a7 100644 --- a/apps/desktop/src/main/workflow-apply.ts +++ b/apps/desktop/src/main/workflow-apply.ts @@ -303,10 +303,12 @@ async function linkTargetOf(abs: string): Promise { * regular file where the link was and detach the link from its target for good, * so the note the user sees in two places would silently become two files, and * an undo afterwards would write a plain file over the link as well. `vault.ts` - * saves with `fs.writeFile`, which follows the link, so a workflow editing a - * note must do the same. Resolving the link and doing the atomic dance at the - * target keeps both properties: the link survives and no reader ever sees a - * half-written file. + * saves through the link, so a workflow editing a note must do the same. + * Resolving the link and doing the atomic dance at the target keeps both + * properties: the link survives and no reader ever sees a half-written file. + * `writeFileAtomic` resolves links itself now, so this is belt and braces; the + * resolved path is still wanted here, because undo removes the file the run + * created and that file is the target, never the link. * * The target may sit outside the vault. That is what following a link means, * and it is the same reach every other save in the app has; see diff --git a/apps/server/internal/vault/atomicwrite.go b/apps/server/internal/vault/atomicwrite.go new file mode 100644 index 00000000..7981a57e --- /dev/null +++ b/apps/server/internal/vault/atomicwrite.go @@ -0,0 +1,136 @@ +package vault + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "time" +) + +// The scratch file writeFileAtomic renames from: `...tmp`. +// The shape is shared with the desktop app's writeFileAtomic, and both watchers +// filter on it, so the two must stay recognizable to each other. The trailing +// number is an epoch stamp (millis on the desktop, nanos here), and requiring +// its length is what keeps a file the user actually named `notes.2024.01.tmp` +// out of the filter: events we drop here are events no client ever hears about. +var atomicWriteTempPattern = regexp.MustCompile(`\.\d+\.\d{13,}\.tmp$`) + +// IsAtomicWriteTempPath reports whether p is one of those scratch files. The +// watcher drops them: a temp file appearing and vanishing is not a vault +// change, and a client that heard about it would rebuild its asset list on +// every keystroke-driven note save. +func IsAtomicWriteTempPath(p string) bool { + return atomicWriteTempPattern.MatchString(filepath.Base(p)) +} + +// writeFileAtomic writes data to abs by way of a temp file in the same +// directory, fsynced, then renamed over the target. The rename is atomic, so no +// reader can ever observe a truncated or half-written file. That is what keeps +// a save from erasing the note it is saving: the file watcher echoes each save +// back to every client, and with a truncate-then-write the echo of one save +// could read the file inside the next save's empty window and hand clients an +// empty note (#585). +// +// A rename replaces the DIRECTORY ENTRY, which would silently take away two +// properties the plain os.WriteFile this replaced had for free: +// +// - A symlinked note gets written THROUGH, not over. Pointed straight at a +// link, the rename would leave a regular file where the link was and detach +// it from its target for good. SafeJoin has already proved the target +// resolves inside the vault. +// - An existing file keeps its own permissions. os.WriteFile only applies its +// mode when it creates the file, so a note the operator chmod'ed stays as +// they left it; fileMode applies only to files this call creates. +func writeFileAtomic(abs string, data []byte, fileMode, dirMode fs.FileMode) error { + target, err := resolveLinkTarget(abs) + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(target), dirMode); err != nil { + return err + } + + mode := fileMode + replacing := false + if info, statErr := os.Stat(target); statErr == nil { + mode = info.Mode().Perm() + replacing = true + } else if !errors.Is(statErr, os.ErrNotExist) { + return statErr + } + + temp := fmt.Sprintf("%s.%d.%d.tmp", target, os.Getpid(), time.Now().UnixNano()) + f, err := os.OpenFile(temp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode) + if err != nil { + return err + } + if err := writeAndSync(f, data); err != nil { + _ = f.Close() + _ = os.Remove(temp) + return err + } + if err := f.Close(); err != nil { + _ = os.Remove(temp) + return err + } + // O_CREATE runs the mode through the process umask, so reproducing the mode + // of a file we are replacing takes an explicit chmod (0664 under umask 022, + // say). A file this call creates is deliberately left umasked, which is what + // os.WriteFile did with fileMode. On Windows chmod touches nothing but the + // read-only bit, which is the most it can mean there. + if replacing { + if err := os.Chmod(temp, mode); err != nil { + _ = os.Remove(temp) + return err + } + } + if err := os.Rename(temp, target); err != nil { + _ = os.Remove(temp) + return err + } + return nil +} + +func writeAndSync(f *os.File, data []byte) error { + if _, err := f.Write(data); err != nil { + return err + } + // The bytes have to reach the disk before the rename publishes them, or a + // crash can leave the entry pointing at a file with nothing in it. + return f.Sync() +} + +// resolveLinkTarget follows a symlink at abs to the file it points at, so the +// atomic write lands on the target rather than replacing the link. A dangling +// link resolves to the path it names, which is where a plain write would have +// created the file. +func resolveLinkTarget(abs string) (string, error) { + info, err := os.Lstat(abs) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return abs, nil + } + return "", err + } + if info.Mode()&os.ModeSymlink == 0 { + return abs, nil + } + resolved, err := filepath.EvalSymlinks(abs) + if err == nil { + return resolved, nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + dest, err := os.Readlink(abs) + if err != nil { + return "", err + } + if filepath.IsAbs(dest) { + return dest, nil + } + return filepath.Join(filepath.Dir(abs), dest), nil +} diff --git a/apps/server/internal/vault/atomicwrite_test.go b/apps/server/internal/vault/atomicwrite_test.go new file mode 100644 index 00000000..7878ef49 --- /dev/null +++ b/apps/server/internal/vault/atomicwrite_test.go @@ -0,0 +1,211 @@ +package vault + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" +) + +// The #585 property, and the whole reason WriteNote is atomic: the watcher +// echoes every save to every client, and a client that reads the file inside a +// truncate-then-write window gets an empty note and shows it as the truth. No +// reader may ever observe anything but a complete body. +func TestWriteNoteNeverExposesAPartialFile(t *testing.T) { + root := t.TempDir() + v, err := New(root, Options{}) + if err != nil { + t.Fatal(err) + } + const rel = "inbox/race.md" + // Big enough that the write is not a single instantaneous syscall. + bodyA := strings.Repeat("A", 96*1024) + bodyB := strings.Repeat("B", 96*1024) + if _, err := v.WriteNote(rel, bodyA); err != nil { + t.Fatal(err) + } + abs := filepath.Join(v.Root(), "inbox", "race.md") + + stop := make(chan struct{}) + bad := make(chan string, 1) + var readers sync.WaitGroup + readers.Add(1) + go func() { + defer readers.Done() + for { + select { + case <-stop: + return + default: + } + data, err := os.ReadFile(abs) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + select { + case bad <- "the note vanished mid-save": + default: + } + return + } + continue + } + if body := string(data); body != bodyA && body != bodyB { + select { + case bad <- fmt.Sprintf("a reader saw %d bytes, neither the old body nor the new one", len(body)): + default: + } + return + } + } + }() + + for i := range 200 { + body := bodyA + if i%2 == 1 { + body = bodyB + } + if _, err := v.WriteNote(rel, body); err != nil { + t.Fatal(err) + } + } + close(stop) + readers.Wait() + + select { + case msg := <-bad: + t.Fatal(msg) + default: + } +} + +// A rename replaces the directory entry, so an atomic write aimed straight at a +// symlinked note would leave a regular file where the link was and detach it +// from its target for good. +func TestWriteNoteFollowsSymlinkInsteadOfReplacingIt(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink semantics differ on windows") + } + root := t.TempDir() + v, err := New(root, Options{}) + if err != nil { + t.Fatal(err) + } + realAbs := filepath.Join(v.Root(), "inbox", "real.md") + if err := os.WriteFile(realAbs, []byte("original"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(v.Root(), "inbox", "link.md") + // Target inside the vault, which is what SafeJoin permits. + if err := os.Symlink(realAbs, link); err != nil { + t.Fatal(err) + } + + if _, err := v.WriteNote("inbox/link.md", "written through the link"); err != nil { + t.Fatal(err) + } + + info, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatal("the symlink was replaced by a regular file") + } + got, err := os.ReadFile(realAbs) + if err != nil { + t.Fatal(err) + } + if string(got) != "written through the link" { + t.Fatalf("link target holds %q, want the written body", got) + } +} + +// os.WriteFile only applied its mode when it created the file, so replacing it +// with temp-plus-rename must not quietly re-permission notes the operator (or +// another tool) left with a mode of their own. +func TestWriteNotePreservesModeOfAnExistingNote(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file modes are not meaningful on windows") + } + root := t.TempDir() + v, err := New(root, Options{}) + if err != nil { + t.Fatal(err) + } + shared := filepath.Join(v.Root(), "inbox", "shared.md") + if err := os.WriteFile(shared, []byte("x"), 0o640); err != nil { + t.Fatal(err) + } + if err := os.Chmod(shared, 0o640); err != nil { // defeat the process umask + t.Fatal(err) + } + + if _, err := v.WriteNote("inbox/shared.md", "updated"); err != nil { + t.Fatal(err) + } + info, err := os.Stat(shared) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o640 { + t.Fatalf("mode after save = %v, want 0640", perm) + } + + // A note this call creates still gets the vault's configured mode. + if _, err := v.WriteNote("inbox/fresh.md", "new"); err != nil { + t.Fatal(err) + } + fresh, err := os.Stat(filepath.Join(v.Root(), "inbox", "fresh.md")) + if err != nil { + t.Fatal(err) + } + if perm := fresh.Mode().Perm(); perm != 0o600 { + t.Fatalf("new note mode = %v, want the vault's 0600", perm) + } +} + +func TestWriteNoteLeavesNoScratchFiles(t *testing.T) { + root := t.TempDir() + v, err := New(root, Options{}) + if err != nil { + t.Fatal(err) + } + for range 3 { + if _, err := v.WriteNote("inbox/note.md", "body"); err != nil { + t.Fatal(err) + } + } + entries, err := os.ReadDir(filepath.Join(v.Root(), "inbox")) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasSuffix(entry.Name(), ".tmp") { + t.Fatalf("a scratch file survived the save: %s", entry.Name()) + } + } +} + +func TestIsAtomicWriteTempPath(t *testing.T) { + cases := []struct { + path string + want bool + }{ + {"inbox/note.md.4123.1786714355519.tmp", true}, // desktop, millis + {"inbox/note.md.4123.1786714355519123456.tmp", true}, // server, nanos + {"inbox/note.md", false}, + {"inbox/note.tmp", false}, + // A file the user named themselves keeps its live updates: the trailing + // group is too short to be an epoch stamp. + {"inbox/report.2024.01.tmp", false}, + } + for _, c := range cases { + if got := IsAtomicWriteTempPath(c.path); got != c.want { + t.Errorf("IsAtomicWriteTempPath(%q) = %v, want %v", c.path, got, c.want) + } + } +} diff --git a/apps/server/internal/vault/vault.go b/apps/server/internal/vault/vault.go index a42884f5..6a59e14d 100644 --- a/apps/server/internal/vault/vault.go +++ b/apps/server/internal/vault/vault.go @@ -1511,10 +1511,7 @@ func (v *Vault) WriteNote(rel, body string) (NoteMeta, error) { if err != nil { return NoteMeta{}, err } - if err := os.MkdirAll(filepath.Dir(abs), v.dirMode); err != nil { - return NoteMeta{}, err - } - if err := os.WriteFile(abs, []byte(body), v.fileMode); err != nil { + if err := writeFileAtomic(abs, []byte(body), v.fileMode, v.dirMode); err != nil { return NoteMeta{}, err } v.invalidateTextSearchCache() diff --git a/apps/server/internal/watcher/watcher.go b/apps/server/internal/watcher/watcher.go index 011599ca..4b4ed1e2 100644 --- a/apps/server/internal/watcher/watcher.go +++ b/apps/server/internal/watcher/watcher.go @@ -244,6 +244,12 @@ func (w *Watcher) commentsNotePath(absPath string) (string, bool) { func (w *Watcher) handle(ev fsnotify.Event) { base := filepath.Base(ev.Name) + // The scratch file every atomic write renames from. Its create/write/rename + // burst is not a vault change, and since the name does not end in .md a + // client would answer each one by re-listing the whole asset tree. + if vault.IsAtomicWriteTempPath(ev.Name) { + return + } if strings.HasPrefix(base, ".") && !w.isVaultSettingsPath(ev.Name) && base != internalVaultDir { return } @@ -275,7 +281,7 @@ func (w *Watcher) handle(ev fsnotify.Event) { } if relPosix == vaultSettingsFilePath { w.reloadFolderPaths() - kind := eventKind(ev) + kind := eventKind(ev, statErr == nil) if kind == "" { return } @@ -288,7 +294,7 @@ func (w *Watcher) handle(ev fsnotify.Event) { return } if notePath, ok := w.commentsNotePath(ev.Name); ok { - kind := eventKind(ev) + kind := eventKind(ev, statErr == nil) if kind == "" { return } @@ -321,7 +327,7 @@ func (w *Watcher) handle(ev fsnotify.Event) { } } - kind := eventKind(ev) + kind := eventKind(ev, statErr == nil) if kind == "" { return } @@ -335,13 +341,23 @@ func (w *Watcher) handle(ev fsnotify.Event) { w.broadcast(change) } -func eventKind(ev fsnotify.Event) string { +// exists says whether the path was still on disk when the event was handled, +// which is what separates a deleted note from a replaced one. +func eventKind(ev fsnotify.Event, exists bool) string { switch { case ev.Op&fsnotify.Create != 0: return "add" case ev.Op&fsnotify.Write != 0: return "change" case ev.Op&fsnotify.Remove != 0, ev.Op&fsnotify.Rename != 0: + // A rename into place, which is what every atomic save is, drops the + // old directory entry while the replacement is already sitting there. + // The kqueue backend (a server hosted on macOS) reports that as a + // delete of the note itself, and a client told its open note was + // deleted closes the tab. A path that still exists was replaced. + if exists { + return "add" + } return "unlink" default: return "" diff --git a/apps/server/internal/watcher/watcher_test.go b/apps/server/internal/watcher/watcher_test.go index fc5f58c2..7f9f415f 100644 --- a/apps/server/internal/watcher/watcher_test.go +++ b/apps/server/internal/watcher/watcher_test.go @@ -228,3 +228,90 @@ func TestActiveDistinguishesRealFromDisabledWatcher(t *testing.T) { t.Fatal("nil watcher reports Active") } } + +// Every atomic note save creates a scratch file next to the note and renames it +// into place. The scratch file is not a vault change, and because its name does +// not end in .md a client that heard about it would answer by re-listing the +// whole asset tree, on every save. +func TestWatcherIgnoresAtomicWriteScratchFiles(t *testing.T) { + root := t.TempDir() + w := newTestWatcher(t, root) + ch, unsub := w.Subscribe() + defer unsub() + + scratch := filepath.Join(root, "inbox", "note.md.4123.1786714355519123456.tmp") + for _, op := range []fsnotify.Op{fsnotify.Create, fsnotify.Write, fsnotify.Rename} { + w.handle(fsnotify.Event{Name: scratch, Op: op}) + } + + select { + case ev := <-ch: + t.Fatalf("a scratch file reached clients: %+v", ev) + case <-time.After(100 * time.Millisecond): + } + + // The note the scratch file was renamed onto still reports normally. + w.handle(fsnotify.Event{Name: filepath.Join(root, "inbox", "note.md"), Op: fsnotify.Create}) + if ev := recvChange(t, ch); ev.Path != "inbox/note.md" { + t.Fatalf("note event = %+v, want inbox/note.md", ev) + } +} + +// inotify reports a rename-into-place as IN_MOVED_TO, which fsnotify folds into +// Create, so an atomic write (ours, or git/rsync/vim/Syncthing doing the same +// dance) surfaces as "add" rather than "change". Clients therefore have to treat +// an "add" for a note they hold open as new content to read, and this test is +// what pins that contract down on the server side. +func TestWatcherReportsRenameIntoPlaceAsAdd(t *testing.T) { + root := t.TempDir() + w := newTestWatcher(t, root) + ch, unsub := w.Subscribe() + defer unsub() + + note := filepath.Join(root, "inbox", "note.md") + if err := os.MkdirAll(filepath.Dir(note), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(note, []byte("replaced by rename"), 0o600); err != nil { + t.Fatal(err) + } + + w.handle(fsnotify.Event{Name: note, Op: fsnotify.Create}) + ev := recvChange(t, ch) + if ev.Kind != "add" || ev.Path != "inbox/note.md" || ev.Scope != "" { + t.Fatalf("rename-into-place event = %+v, want {add inbox/note.md}", ev) + } +} + +// The kqueue backend (a server hosted on macOS) reports the rename half of an +// atomic save as a delete of the note itself, arriving just before the add. A +// client that believes it closes the tab of the note being saved, so a path +// that still exists must never be reported as gone. +func TestWatcherDoesNotReportAReplacedNoteAsDeleted(t *testing.T) { + root := t.TempDir() + w := newTestWatcher(t, root) + ch, unsub := w.Subscribe() + defer unsub() + + note := filepath.Join(root, "inbox", "note.md") + if err := os.MkdirAll(filepath.Dir(note), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(note, []byte("the replacement is already here"), 0o600); err != nil { + t.Fatal(err) + } + + w.handle(fsnotify.Event{Name: note, Op: fsnotify.Remove}) + if ev := recvChange(t, ch); ev.Kind == "unlink" { + t.Fatalf("a replaced note was reported as deleted: %+v", ev) + } + + // A note that really is gone still reports as gone. + if err := os.Remove(note); err != nil { + t.Fatal(err) + } + w.handle(fsnotify.Event{Name: note, Op: fsnotify.Remove}) + if ev := recvChange(t, ch); ev.Kind != "unlink" { + t.Fatalf("deleted note event = %+v, want unlink", ev) + } +} diff --git a/packages/app-core/src/store-note-integrity.test.ts b/packages/app-core/src/store-note-integrity.test.ts index c9ae6b4b..45ebe37b 100644 --- a/packages/app-core/src/store-note-integrity.test.ts +++ b/packages/app-core/src/store-note-integrity.test.ts @@ -255,4 +255,44 @@ describe('#585 — dirty buffers survive watcher change events', () => { await useStore.getState().persistNote(target) expect(vault.get(target)).toBe('FIRST AND SECOND') }) + + // Saves are atomic now (temp file renamed into place), and on Linux a rename + // arrives as IN_MOVED_TO, which the server's watcher reports as 'add'. Any + // other tool that writes by renaming (git, rsync, Syncthing, vim) looks the + // same, so an 'add' for an open note carries content that must be read. + it('refreshes an open note when a writer renames a new file into place', async () => { + const { useStore } = await loadStore() + seedRootVault(useStore) + const paneId = useStore.getState().activePaneId + const target = 'index.md' + await useStore.getState().openNoteInPane(paneId, target) + await flush() + + vault.set(target, 'REPLACED BY RENAME') + await useStore + .getState() + .applyChange({ kind: 'add', path: target, folder: 'inbox', scope: 'content' }) + await flush() + + expect(useStore.getState().noteContents[target]?.body).toBe('REPLACED BY RENAME') + expect(writeCalls).toEqual([]) + }) + + it('still refuses to let an add event overwrite unsaved edits', async () => { + const { useStore } = await loadStore() + seedRootVault(useStore) + const paneId = useStore.getState().activePaneId + const target = 'index.md' + await useStore.getState().openNoteInPane(paneId, target) + await flush() + + useStore.getState().updateNoteBody(target, 'INDEX_BODY with unsaved edits') + vault.set(target, 'REPLACED BY RENAME') + await useStore + .getState() + .applyChange({ kind: 'add', path: target, folder: 'inbox', scope: 'content' }) + await flush() + + expect(useStore.getState().noteContents[target]?.body).toBe('INDEX_BODY with unsaved edits') + }) }) diff --git a/packages/app-core/src/store.ts b/packages/app-core/src/store.ts index c855c4c4..8155c548 100644 --- a/packages/app-core/src/store.ts +++ b/packages/app-core/src/store.ts @@ -6236,7 +6236,12 @@ export const useStore = create((set, get) => { return } - if (ev.kind === 'change') { + // 'add' counts as new content for a note we already hold open. A writer + // that renames a file into place (ZenNotes saving atomically, but equally + // git, rsync, Syncthing or vim) shows up on Linux as IN_MOVED_TO, which the + // server's watcher reports as 'add' rather than 'change'; treating it as + // noise left the buffer showing content that no longer existed on disk. + if (ev.kind === 'change' || ev.kind === 'add') { try { const content = await window.zen.readNote(ev.path) // Drop the watcher echo of our own writes. Without this, an From 3c6b5f0f28ce82c8b081615d90f3ba1d5c798657 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 09:32:41 -0500 Subject: [PATCH 06/18] Fix(prompt): pickers with a list are tappable one-handed on touch devices A prompt autofocused its input as soon as it opened, which on a phone summons the soft keyboard over the very list the user is about to tap. The folder picker was the worst of it: the suggestions it exists to offer were hidden behind the keyboard the moment it appeared. On a coarse pointer, a prompt that actually has suggestions now opens tap-first: no autofocus, no hint line about arrow keys and Ctrl+J that a phone has no way to press, and taller rows to aim at. Typing is still one tap away in the input itself, and a prompt with nothing to tap keeps its autofocus. Nothing changes for a mouse or a trackpad. --- .../app-core/src/components/PromptModal.tsx | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/app-core/src/components/PromptModal.tsx b/packages/app-core/src/components/PromptModal.tsx index 397ced93..aa8ecd65 100644 --- a/packages/app-core/src/components/PromptModal.tsx +++ b/packages/app-core/src/components/PromptModal.tsx @@ -4,6 +4,17 @@ import { isPaletteNextKey, isPalettePreviousKey } from '../lib/palette-nav' import { Modal } from './ui/Modal' import { Button } from './ui/Button' +/** + * Touch devices get a tap-first prompt when suggestions exist: no input + * autofocus (which summons the soft keyboard over the very list the user is + * about to tap — the folder picker was unusable one-handed on phones), no + * keyboard-shortcut hint line, and taller suggestion rows. Typing is still one + * tap away via the input itself. + */ +function isCoarsePointer(): boolean { + return typeof window !== 'undefined' && (window.matchMedia?.('(pointer: coarse)').matches ?? false) +} + export interface PromptSuggestion { value: string label?: string @@ -107,6 +118,8 @@ export function PromptModal({ }, [options.initialValue, options.title]) useEffect(() => { + // Tap-first on touch when there is a list to tap (see isCoarsePointer). + if (isCoarsePointer() && (options.suggestions?.length ?? 0) > 0) return const t = setTimeout(() => { inputRef.current?.focus() inputRef.current?.select() @@ -213,7 +226,7 @@ export function PromptModal({ }} className="w-full rounded-md border border-paper-300 bg-paper-50 px-2.5 py-1.5 text-sm text-ink-900 outline-none focus:border-accent" /> - {options.suggestionsHint && ( + {options.suggestionsHint && !isCoarsePointer() && (
{options.suggestionsHint}
)} {showSuggestions && ( @@ -233,7 +246,8 @@ export function PromptModal({ onMouseEnter={() => setActiveSuggestion(index)} onClick={() => chooseSuggestion(suggestion)} className={[ - 'flex w-full items-center justify-between gap-3 px-3 py-2 text-left transition-colors', + 'flex w-full items-center justify-between gap-3 px-3 text-left transition-colors', + isCoarsePointer() ? 'py-3' : 'py-2', active ? 'bg-paper-200' : 'hover:bg-paper-200/70' ].join(' ')} > From c164601343758a730681ced12326cfb67059270d Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 09:36:16 -0500 Subject: [PATCH 07/18] test(prompt): cover the touch autofocus rule Extracts the decision from 3c6b5f0 into shouldAutofocusPrompt() so it can be asserted the way activeSuggestionAfterInput() is, and pins the three cases that matter: touch with a list opts out, touch with nothing to tap keeps its keyboard, and a fine pointer is never affected. Behavior is unchanged. --- .../src/components/PromptModal-touch.test.ts | 22 +++++++++++++++++++ .../app-core/src/components/PromptModal.tsx | 13 +++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 packages/app-core/src/components/PromptModal-touch.test.ts diff --git a/packages/app-core/src/components/PromptModal-touch.test.ts b/packages/app-core/src/components/PromptModal-touch.test.ts new file mode 100644 index 00000000..67eb5724 --- /dev/null +++ b/packages/app-core/src/components/PromptModal-touch.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' +import { shouldAutofocusPrompt } from './PromptModal' + +// Focusing the input pops the soft keyboard, which on a phone covers the very +// suggestion list the prompt is asking the user to choose from (the folder +// picker in Move to… was unusable one-handed). Touch + a list = tap-first. +describe('shouldAutofocusPrompt', () => { + it('does not autofocus a touch prompt that has suggestions (folder pickers)', () => { + expect(shouldAutofocusPrompt(true, 1)).toBe(false) + expect(shouldAutofocusPrompt(true, 12)).toBe(false) + }) + + it('autofocuses a touch prompt with no list — those are pure typing', () => { + // Rename note / New folder: nothing to tap, so the keyboard is the point. + expect(shouldAutofocusPrompt(true, 0)).toBe(true) + }) + + it('always autofocuses with a fine pointer, so desktop is unchanged', () => { + expect(shouldAutofocusPrompt(false, 0)).toBe(true) + expect(shouldAutofocusPrompt(false, 8)).toBe(true) + }) +}) diff --git a/packages/app-core/src/components/PromptModal.tsx b/packages/app-core/src/components/PromptModal.tsx index aa8ecd65..7e5442f4 100644 --- a/packages/app-core/src/components/PromptModal.tsx +++ b/packages/app-core/src/components/PromptModal.tsx @@ -15,6 +15,16 @@ function isCoarsePointer(): boolean { return typeof window !== 'undefined' && (window.matchMedia?.('(pointer: coarse)').matches ?? false) } +/** + * Whether to focus (and so pop the soft keyboard for) the prompt input on open. + * Only a touch device with a list to tap opts out — a mouse never does, and a + * prompt with no suggestions (Rename, New folder) is pure typing, so it keeps + * the focus it has always had. + */ +export function shouldAutofocusPrompt(coarsePointer: boolean, suggestionCount: number): boolean { + return !(coarsePointer && suggestionCount > 0) +} + export interface PromptSuggestion { value: string label?: string @@ -118,8 +128,7 @@ export function PromptModal({ }, [options.initialValue, options.title]) useEffect(() => { - // Tap-first on touch when there is a list to tap (see isCoarsePointer). - if (isCoarsePointer() && (options.suggestions?.length ?? 0) > 0) return + if (!shouldAutofocusPrompt(isCoarsePointer(), options.suggestions?.length ?? 0)) return const t = setTimeout(() => { inputRef.current?.focus() inputRef.current?.select() From 46152f785d9159084d967a5b2f2b6c7d382d7d3d Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 10:17:21 -0500 Subject: [PATCH 08/18] Fix(cloud): one file can no longer stop sync forever, and settings ask first A vault could reach a state where every sync ended with "Cloud sync stopped because .zennotes/vault.json has unsynced local edits", naming a file that had not been touched in days and whose hash never changed. Reported on Discord with the diagnostics that made it findable: no write events on the file, the same error after every restart, and a stack pointing at assertUnchanged. Sync refuses to write over a file it cannot vouch for, which is the right instinct, but it never asked whether the file it was refusing to overwrite was already the same file. When the change feed carried an item this device had never tracked and the path existed on disk, it threw without comparing content, and throwing stopped the run before the cursor was saved, so the next run fetched the same change and stopped in the same place, forever. The file it names is not special: the scan sorts by path and a dot sorts first, so .zennotes/vault.json simply got there ahead of every note. Three rules replace the refusal. A file already holding exactly what the change carries is adopted, because both sides agree and there is nothing to resolve. A file that differs is kept where it is while the incoming version lands beside it as "Note (cloud conflict).md", numbered when that name is taken. A delete or a move whose file cannot be vouched for keeps the local file instead, since nothing arrives with those to park and the local file is the version being preserved. Each of those reports a conflict and lets the run continue, so one file can never wedge sync again. Vault settings are answered rather than merged. A conflict copy inside a hidden folder is not something anyone can act on, so the cloud's settings wait at .zennotes/vault.cloud-conflict.json, never synced themselves and replaced rather than numbered when a newer one arrives, while Cloud settings asks which side to keep. This device's settings stay in use until the question is answered, so doing nothing keeps what already works, and taking the cloud's writes them through the vault's own normalizer, which refuses a file it cannot read instead of applying half of it. Two smaller wedges went with it: a remote delete for a file already deleted locally used to fail the run with a raw ENOENT, and so did a move whose source had gone. Both repositories change, the desktop one and the shared portable one behind mobile and web, since they are a deliberately synced pair. The portable one already adopted identical content, which is why this only ever bit desktop. --- .../src/main/cloud-sync-filesystem.test.ts | 173 +++++++++++++++--- .../desktop/src/main/cloud-sync-filesystem.ts | 98 ++++++++-- .../src/main/cloud-sync-service.test.ts | 46 ++++- apps/desktop/src/main/cloud-sync-service.ts | 51 +++++- apps/desktop/src/main/index.ts | 16 +- apps/desktop/src/preload/index.ts | 6 + apps/web/src/bridge/http-bridge.ts | 2 + .../src/components/CloudSettings.test.ts | 54 +++++- .../app-core/src/components/CloudSettings.tsx | 96 +++++++++- .../app-core/src/lib/cloud-auto-sync.test.ts | 6 +- packages/bridge-contract/src/bridge.ts | 4 + packages/bridge-contract/src/cloud-sync.ts | 25 +++ packages/bridge-contract/src/ipc.ts | 2 + .../shared-domain/src/cloud-backup.test.ts | 2 +- .../src/cloud-sync-coordinator.test.ts | 68 +++++++ .../src/cloud-sync-coordinator.ts | 35 +++- .../src/cloud-sync-host-service.test.ts | 2 +- .../src/cloud-sync-host-service.ts | 3 +- .../cloud-sync-portable-filesystem.test.ts | 75 +++++--- .../src/cloud-sync-portable-filesystem.ts | 77 ++++++-- packages/shared-domain/src/cloud-sync.test.ts | 25 +++ packages/shared-domain/src/cloud-sync.ts | 44 +++++ 22 files changed, 805 insertions(+), 105 deletions(-) diff --git a/apps/desktop/src/main/cloud-sync-filesystem.test.ts b/apps/desktop/src/main/cloud-sync-filesystem.test.ts index f0f15dd0..201ec51f 100644 --- a/apps/desktop/src/main/cloud-sync-filesystem.test.ts +++ b/apps/desktop/src/main/cloud-sync-filesystem.test.ts @@ -1,13 +1,13 @@ import { afterEach, describe, expect, it } from 'vitest' -import { mkdtemp, readFile, rm, writeFile, mkdir } from 'node:fs/promises' +import { mkdtemp, readdir, readFile, rm, writeFile, mkdir } from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import { createHash } from 'node:crypto' import { - CloudSyncLocalEditConflictError, DesktopCloudSyncRepository, DesktopCloudSyncStateStore } from './cloud-sync-filesystem' +import type { CloudSyncChange } from '@zennotes/bridge-contract/cloud-sync' import type { CloudSyncTrackedItem } from '@zennotes/shared-domain/cloud-sync-engine' const roots: string[] = [] @@ -26,6 +26,24 @@ function hash(contents: string): string { return createHash('sha256').update(contents).digest('hex') } +function upsert(path: string, contents: string): CloudSyncChange { + return { + sequence: 2, + item_id: 'item-remote', + type: 'upsert', + path, + previous_path: null, + revision: 2, + content: { + encoding: 'utf8', + data: contents, + sha256: hash(contents), + byte_length: Buffer.byteLength(contents), + media_type: 'text/markdown' + } + } +} + function tracked(path: string, contents: string): CloudSyncTrackedItem { return { item_id: 'item-1', @@ -140,33 +158,142 @@ describe('DesktopCloudSyncRepository', () => { }) }) - it('does not overwrite a local edit while pulling remote changes', async () => { + // The local file is never overwritten, and the incoming version is never + // thrown away: it lands beside it. Sync used to throw here instead, which + // stopped the whole run and, because the cursor never advanced, stopped + // every run after it too (#585 follow-up, reported on Discord). + it('keeps both versions when a remote change meets a local edit', async () => { + const root = await temporaryRoot() + await writeFile(path.join(root, 'note.md'), 'local edit') + const repository = new DesktopCloudSyncRepository(root) + + const conflict = await repository.apply( + upsert('note.md', 'remote edit'), + tracked('note.md', 'old contents') + ) + + expect(conflict).toEqual({ + code: 'LOCAL_EDIT_CONFLICT', + path: 'note.md', + conflict_copy_path: 'note (cloud conflict).md' + }) + expect(await readFile(path.join(root, 'note.md'), 'utf8')).toBe('local edit') + expect(await readFile(path.join(root, 'note (cloud conflict).md'), 'utf8')).toBe('remote edit') + }) + + // What wedged the reporter: the change feed carried a file this device had + // never tracked, so sync refused it without ever noticing that the bytes on + // disk were already exactly what was being delivered. + it('adopts a file that already matches the incoming change', async () => { + const root = await temporaryRoot() + await mkdir(path.join(root, '.zennotes'), { recursive: true }) + await writeFile(path.join(root, '.zennotes', 'vault.json'), '{"favorites":[]}') + const repository = new DesktopCloudSyncRepository(root) + + const conflict = await repository.apply( + upsert('.zennotes/vault.json', '{"favorites":[]}'), + undefined + ) + + expect(conflict).toBeUndefined() + expect(await readFile(path.join(root, '.zennotes', 'vault.json'), 'utf8')).toBe( + '{"favorites":[]}' + ) + expect(await readdir(path.join(root, '.zennotes'))).toEqual(['vault.json']) + }) + + it('numbers conflict copies instead of overwriting an earlier one', async () => { + const root = await temporaryRoot() + await writeFile(path.join(root, 'note.md'), 'local edit') + await writeFile(path.join(root, 'note (cloud conflict).md'), 'an earlier conflict') + const repository = new DesktopCloudSyncRepository(root) + + const conflict = await repository.apply(upsert('note.md', 'remote edit'), undefined) + + expect(conflict?.conflict_copy_path).toBe('note (cloud conflict 2).md') + expect(await readFile(path.join(root, 'note (cloud conflict).md'), 'utf8')).toBe( + 'an earlier conflict' + ) + expect(await readFile(path.join(root, 'note (cloud conflict 2).md'), 'utf8')).toBe('remote edit') + }) + + // Settings are a question, not a merge: a numbered copy inside a hidden + // folder is not something anyone can act on, so the cloud version waits at + // one fixed path and the app asks which side to keep. + it('parks conflicting vault settings at one fixed path for the user to answer', async () => { + const root = await temporaryRoot() + await mkdir(path.join(root, '.zennotes'), { recursive: true }) + await writeFile(path.join(root, '.zennotes', 'vault.json'), '{"favorites":["a"]}') + const repository = new DesktopCloudSyncRepository(root) + + const first = await repository.apply( + upsert('.zennotes/vault.json', '{"favorites":["b"]}'), + undefined + ) + expect(first).toEqual({ + code: 'SETTINGS_CONFLICT', + path: '.zennotes/vault.json', + conflict_copy_path: '.zennotes/vault.cloud-conflict.json' + }) + // The settings in use are still this device's. + expect(await readFile(path.join(root, '.zennotes', 'vault.json'), 'utf8')).toBe( + '{"favorites":["a"]}' + ) + + // A newer cloud version replaces the pending one instead of piling up. + await repository.apply(upsert('.zennotes/vault.json', '{"favorites":["c"]}'), undefined) + expect( + await readFile(path.join(root, '.zennotes', 'vault.cloud-conflict.json'), 'utf8') + ).toBe('{"favorites":["c"]}') + expect((await readdir(path.join(root, '.zennotes'))).sort()).toEqual([ + 'vault.cloud-conflict.json', + 'vault.json' + ]) + }) + + it('keeps a locally edited file that the remote deleted', async () => { const root = await temporaryRoot() await writeFile(path.join(root, 'note.md'), 'local edit') const repository = new DesktopCloudSyncRepository(root) - await expect( - repository.apply( - { - sequence: 2, - item_id: 'item-1', - type: 'upsert', - path: 'note.md', - previous_path: null, - revision: 2, - content: { - encoding: 'utf8', - data: 'remote edit', - sha256: hash('remote edit'), - byte_length: 11, - media_type: 'text/markdown' - } - }, - tracked('note.md', 'old contents') - ) - ).rejects.toBeInstanceOf(CloudSyncLocalEditConflictError) + const conflict = await repository.apply( + { + sequence: 3, + item_id: 'item-1', + type: 'delete', + path: 'note.md', + previous_path: null, + revision: 3 + }, + tracked('note.md', 'old contents') + ) + + expect(conflict).toEqual({ + code: 'LOCAL_EDIT_CONFLICT', + path: 'note.md', + conflict_copy_path: null + }) expect(await readFile(path.join(root, 'note.md'), 'utf8')).toBe('local edit') }) + + it('accepts a delete for a file that is already gone locally', async () => { + const root = await temporaryRoot() + const repository = new DesktopCloudSyncRepository(root) + + const conflict = await repository.apply( + { + sequence: 4, + item_id: 'item-1', + type: 'delete', + path: 'note.md', + previous_path: null, + revision: 4 + }, + tracked('note.md', 'old contents') + ) + + expect(conflict).toBeUndefined() + }) }) describe('DesktopCloudSyncStateStore', () => { diff --git a/apps/desktop/src/main/cloud-sync-filesystem.ts b/apps/desktop/src/main/cloud-sync-filesystem.ts index a395788b..f8cd7618 100644 --- a/apps/desktop/src/main/cloud-sync-filesystem.ts +++ b/apps/desktop/src/main/cloud-sync-filesystem.ts @@ -1,8 +1,15 @@ import { createHash, randomUUID } from 'node:crypto' import { constants as fsConstants, promises as fs } from 'node:fs' import path from 'node:path' -import type { CloudSyncChange, CloudSyncContent } from '@zennotes/bridge-contract/cloud-sync' +import type { + CloudSyncChange, + CloudSyncContent, + CloudSyncLocalConflict +} from '@zennotes/bridge-contract/cloud-sync' import { + CLOUD_SYNC_SETTINGS_CONFLICT_PATH, + cloudSyncConflictCopyPath, + isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, shouldSyncVaultPath, shouldTraverseCloudSyncDirectory @@ -80,7 +87,10 @@ export class DesktopCloudSyncRepository implements CloudSyncRepository { return items.sort((left, right) => left.path.localeCompare(right.path)) } - async apply(change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined): Promise { + async apply( + change: CloudSyncChange, + previous: CloudSyncTrackedItem | undefined + ): Promise { const affectedPaths = [change.path, change.previous_path, previous?.path].filter( (path): path is string => typeof path === 'string' ) @@ -88,13 +98,28 @@ export class DesktopCloudSyncRepository implements CloudSyncRepository { if (change.type === 'upsert') { if (!change.content) throw new Error(`Upsert change ${change.sequence} did not include content`) - await this.assertUnchanged(previous?.path ?? change.path, previous) + const atTarget = await this.readIfExists(change.path) + // Already byte-for-byte what the change carries. There is nothing to + // write and nothing to conflict over, so adopt the file and move on. + // Without this, a file both sides already agree on stopped sync dead. + if (atTarget && sha256(atTarget) === change.content.sha256) return + + const guardPath = previous?.path ?? change.path + const unvouched = await this.firstUnvouchedPath( + guardPath === change.path ? [change.path] : [guardPath, change.path], + previous + ) + if (unvouched) return await this.keepBoth(change.path, decodeContent(change.content)) + await this.write(change.path, decodeContent(change.content)) return } const previousPath = previous?.path ?? change.previous_path ?? change.path - await this.assertUnchanged(previousPath, previous) + const unvouched = await this.firstUnvouchedPath([previousPath], previous) + // A delete or a move carries no content to park, so keeping the local file + // where it is IS the preserved version. The next push re-uploads it. + if (unvouched) return localConflict(unvouched, null) if (change.type === 'delete') { await fs.rm(this.resolve(previousPath), { force: true }) @@ -104,9 +129,14 @@ export class DesktopCloudSyncRepository implements CloudSyncRepository { const source = this.resolve(previousPath) const destination = this.resolve(change.path) if (source === destination) return + if (!(await exists(source))) { + // Nothing here to move. Either the move already landed, or the file is + // gone locally and the next scan reconciles it. + return + } await fs.mkdir(path.dirname(destination), { recursive: true }) - if (await exists(destination)) throw new CloudSyncLocalEditConflictError(change.path) + if (await exists(destination)) return localConflict(change.path, null) await fs.rename(source, destination) } @@ -147,23 +177,55 @@ export class DesktopCloudSyncRepository implements CloudSyncRepository { return absolutePath } - private async assertUnchanged( - relPath: string, - previous: CloudSyncTrackedItem | undefined - ): Promise { - const absolutePath = this.resolve(relPath) - + private async readIfExists(relPath: string): Promise { try { - const bytes = await fs.readFile(absolutePath) - if (!previous || sha256(bytes) !== previous.sha256) { - throw new CloudSyncLocalEditConflictError(relPath) - } + return await fs.readFile(this.resolve(relPath)) } catch (error) { - if (isMissingFileError(error) && !previous) return + if (isMissingFileError(error)) return null throw error } } + /** + * The first of these paths holding a file sync cannot vouch for, meaning it + * is not the exact bytes we last agreed on with the server. A file that is + * absent is fine: there is nothing there to lose. + */ + private async firstUnvouchedPath( + relPaths: readonly string[], + previous: CloudSyncTrackedItem | undefined + ): Promise { + for (const relPath of relPaths) { + const bytes = await this.readIfExists(relPath) + if (!bytes) continue + if (!previous || sha256(bytes) !== previous.sha256) return relPath + } + return null + } + + /** Park the incoming version beside the local file rather than over it. */ + private async keepBoth(relPath: string, bytes: Buffer): Promise { + // Settings are answered, not merged: the newest cloud version replaces any + // older pending one at a fixed path, and the app asks which side to keep. + if (isCloudSyncVaultSettingsPath(relPath)) { + await this.write(CLOUD_SYNC_SETTINGS_CONFLICT_PATH, bytes) + return { + code: 'SETTINGS_CONFLICT', + path: relPath, + conflict_copy_path: CLOUD_SYNC_SETTINGS_CONFLICT_PATH + } + } + for (let attempt = 1; attempt <= 100; attempt++) { + const candidate = cloudSyncConflictCopyPath(relPath, attempt) + if (await exists(this.resolve(candidate))) continue + await this.write(candidate, bytes) + return localConflict(relPath, candidate) + } + // A hundred conflict copies of one file means something is looping. Keep + // the local file and report it rather than filling the vault. + return localConflict(relPath, null) + } + private async write(relPath: string, bytes: Buffer): Promise { const destination = this.resolve(relPath) const temporaryPath = `${destination}.${process.pid}.${randomUUID()}.tmp` @@ -272,6 +334,10 @@ function mediaType(relPath: string, text: boolean): string { (text ? 'text/plain' : 'application/octet-stream') } +function localConflict(path: string, conflictCopyPath: string | null): CloudSyncLocalConflict { + return { code: 'LOCAL_EDIT_CONFLICT', path, conflict_copy_path: conflictCopyPath } +} + function sha256(bytes: Buffer): string { return createHash('sha256').update(bytes).digest('hex') } diff --git a/apps/desktop/src/main/cloud-sync-service.test.ts b/apps/desktop/src/main/cloud-sync-service.test.ts index bce75578..76eca845 100644 --- a/apps/desktop/src/main/cloud-sync-service.test.ts +++ b/apps/desktop/src/main/cloud-sync-service.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import type { @@ -162,6 +162,48 @@ async function setup( } describe('DesktopCloudSyncService', () => { + // Settings differ between devices, so sync asks instead of picking. Doing + // nothing keeps this device's settings, which are already in use. + it('answers the settings question either way', async () => { + const { service, localRoot } = await setup([]) + const settingsPath = path.join(localRoot, '.zennotes', 'vault.json') + const parkedPath = path.join(localRoot, '.zennotes', 'vault.cloud-conflict.json') + await mkdir(path.join(localRoot, '.zennotes'), { recursive: true }) + await writeFile(settingsPath, JSON.stringify({ favorites: ['local.md'] })) + + expect(await service.settingsConflict(localRoot)).toBeNull() + + await writeFile(parkedPath, JSON.stringify({ favorites: ['cloud.md'] })) + expect(await service.settingsConflict(localRoot)).toEqual({ + path: '.zennotes/vault.json', + cloud_path: '.zennotes/vault.cloud-conflict.json' + }) + + // Keeping this device's settings drops the pending copy and changes nothing. + await service.resolveSettingsConflict(localRoot, 'local') + expect(await service.settingsConflict(localRoot)).toBeNull() + expect(JSON.parse(await readFile(settingsPath, 'utf8')).favorites).toEqual(['local.md']) + + // Taking the cloud's writes them through the vault's own normalizer. + await writeFile(parkedPath, JSON.stringify({ favorites: ['cloud.md'] })) + await service.resolveSettingsConflict(localRoot, 'cloud') + expect(await service.settingsConflict(localRoot)).toBeNull() + expect(JSON.parse(await readFile(settingsPath, 'utf8')).favorites).toEqual(['cloud.md']) + }) + + it('refuses to apply cloud settings that are not readable', async () => { + const { service, localRoot } = await setup([]) + await mkdir(path.join(localRoot, '.zennotes'), { recursive: true }) + await writeFile(path.join(localRoot, '.zennotes', 'vault.json'), JSON.stringify({})) + await writeFile(path.join(localRoot, '.zennotes', 'vault.cloud-conflict.json'), 'not json') + + await expect(service.resolveSettingsConflict(localRoot, 'cloud')).rejects.toThrow( + 'could not be read' + ) + // The question stays open rather than resolving itself badly. + expect(await service.settingsConflict(localRoot)).not.toBeNull() + }) + it('links only a vault owned by the connected account', async () => { const remoteVault: CloudSyncVault = { id: 'vault-1', @@ -335,7 +377,7 @@ describe('DesktopCloudSyncService', () => { local_sha256: 'a'.repeat(64), remote_sha256: 'b'.repeat(64) } - ] + ], local_conflicts: [] }) await expect(service.createBackup(localRoot)).rejects.toThrow('Resolve sync conflicts') diff --git a/apps/desktop/src/main/cloud-sync-service.ts b/apps/desktop/src/main/cloud-sync-service.ts index 925a591c..2a194c50 100644 --- a/apps/desktop/src/main/cloud-sync-service.ts +++ b/apps/desktop/src/main/cloud-sync-service.ts @@ -13,10 +13,17 @@ import type { CloudPublishNoteInput, CloudServiceAccount, CloudSyncRunSummary, + CloudSyncSettingsChoice, + CloudSyncSettingsConflict, CloudSyncVault, CloudVaultLink } from '@zennotes/bridge-contract/cloud-sync' import { restoreCloudBackup } from '@zennotes/shared-domain/cloud-backup' +import { + CLOUD_SYNC_SETTINGS_CONFLICT_PATH, + CLOUD_SYNC_VAULT_SETTINGS_PATH +} from '@zennotes/shared-domain/cloud-sync' +import { setVaultSettings } from './vault' import type { CloudSyncApiClient } from '@zennotes/shared-domain/cloud-sync-api' import { createDesktopCloudSyncCoordinator } from './cloud-sync-filesystem' @@ -296,10 +303,52 @@ export class DesktopCloudSyncService { pulled: result.pulled, pushed: result.pushed, conflicts: result.conflicts, - bootstrap_conflicts: result.bootstrapConflicts + bootstrap_conflicts: result.bootstrapConflicts, + local_conflicts: result.localConflicts } } + /** The pending settings question, if sync parked a cloud version. It lives + * in the vault rather than in memory, so closing the app does not answer + * it by accident. */ + async settingsConflict(localRoot: string): Promise { + const parked = path.join(localRoot, ...CLOUD_SYNC_SETTINGS_CONFLICT_PATH.split('/')) + try { + await fs.access(parked) + } catch { + return null + } + return { + path: CLOUD_SYNC_VAULT_SETTINGS_PATH, + cloud_path: CLOUD_SYNC_SETTINGS_CONFLICT_PATH + } + } + + /** Answer it. Keeping this device's settings just drops the parked copy; + * the next sync pushes the local ones up. Taking the cloud's writes them + * through the vault's own normalizer, so a hand-edited or older-format + * file cannot land as broken settings. */ + async resolveSettingsConflict( + localRoot: string, + choice: CloudSyncSettingsChoice + ): Promise { + const parked = path.join(localRoot, ...CLOUD_SYNC_SETTINGS_CONFLICT_PATH.split('/')) + if (choice === 'cloud') { + const raw = await fs.readFile(parked, 'utf8') + let parsed: unknown + try { + parsed = JSON.parse(raw) + } catch { + throw new Error('The settings from the cloud could not be read, so nothing was changed.') + } + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + throw new Error('The settings from the cloud could not be read, so nothing was changed.') + } + await setVaultSettings(localRoot, parsed as Parameters[1]) + } + await fs.rm(parked, { force: true }) + } + private async connection(): Promise<{ account: NonNullable client: SyncClient diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index b780ec4b..8982ff15 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -23,7 +23,10 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { createRequire } from "node:module"; import { IPC } from "@shared/ipc"; -import type { CloudPublishNoteInput } from "@zennotes/bridge-contract/cloud-sync"; +import type { + CloudPublishNoteInput, + CloudSyncSettingsChoice, +} from "@zennotes/bridge-contract/cloud-sync"; import type { NoteMeta, NoteCommentInput, @@ -2630,6 +2633,17 @@ function registerIpc(): void { handle(IPC.CLOUD_VAULT_SYNC, () => getCloudSyncService().sync(requireLocalCloudVaultRoot()), ); + handle(IPC.CLOUD_VAULT_SETTINGS_CONFLICT_GET, () => + getCloudSyncService().settingsConflict(requireLocalCloudVaultRoot()), + ); + handle( + IPC.CLOUD_VAULT_SETTINGS_CONFLICT_RESOLVE, + (_event, choice: CloudSyncSettingsChoice) => + getCloudSyncService().resolveSettingsConflict( + requireLocalCloudVaultRoot(), + choice === "cloud" ? "cloud" : "local", + ), + ); handle(IPC.CLOUD_BACKUPS_LIST, () => getCloudSyncService().listBackups(requireLocalCloudVaultRoot()), ); diff --git a/apps/desktop/src/preload/index.ts b/apps/desktop/src/preload/index.ts index b645f60e..d10b77dc 100644 --- a/apps/desktop/src/preload/index.ts +++ b/apps/desktop/src/preload/index.ts @@ -23,6 +23,8 @@ import type { CloudPublishNoteInput, CloudServiceAccount, CloudSyncRunSummary, + CloudSyncSettingsChoice, + CloudSyncSettingsConflict, CloudSyncVault, CloudVaultLink } from '@zennotes/bridge-contract/cloud-sync' @@ -245,6 +247,10 @@ const api: ZenBridge = { ipcRenderer.invoke(IPC.CLOUD_VAULT_LINK_CREATE, name), unlinkCloudVault: (): Promise => ipcRenderer.invoke(IPC.CLOUD_VAULT_LINK_DELETE), syncCloudVault: (): Promise => ipcRenderer.invoke(IPC.CLOUD_VAULT_SYNC), + getCloudSettingsConflict: (): Promise => + ipcRenderer.invoke(IPC.CLOUD_VAULT_SETTINGS_CONFLICT_GET), + resolveCloudSettingsConflict: (choice: CloudSyncSettingsChoice): Promise => + ipcRenderer.invoke(IPC.CLOUD_VAULT_SETTINGS_CONFLICT_RESOLVE, choice), listCloudBackups: (): Promise => ipcRenderer.invoke(IPC.CLOUD_BACKUPS_LIST), getCloudBackupSchedule: (): Promise => diff --git a/apps/web/src/bridge/http-bridge.ts b/apps/web/src/bridge/http-bridge.ts index e39b17ed..6fa596d2 100644 --- a/apps/web/src/bridge/http-bridge.ts +++ b/apps/web/src/bridge/http-bridge.ts @@ -1371,6 +1371,8 @@ export const httpBridge: ZenBridge = { createAndLinkCloudVault: async () => notImplemented('createAndLinkCloudVault'), unlinkCloudVault: async () => notImplemented('unlinkCloudVault'), syncCloudVault: async () => notImplemented('syncCloudVault'), + getCloudSettingsConflict: async () => null, + resolveCloudSettingsConflict: async () => notImplemented('resolveCloudSettingsConflict'), listCloudBackups: async () => notImplemented('listCloudBackups'), getCloudBackupSchedule: async () => notImplemented('getCloudBackupSchedule'), updateCloudBackupSchedule: async () => notImplemented('updateCloudBackupSchedule'), diff --git a/packages/app-core/src/components/CloudSettings.test.ts b/packages/app-core/src/components/CloudSettings.test.ts index dc525200..7980f96c 100644 --- a/packages/app-core/src/components/CloudSettings.test.ts +++ b/packages/app-core/src/components/CloudSettings.test.ts @@ -26,6 +26,8 @@ const mocks = vi.hoisted(() => ({ createAndLinkCloudVault: vi.fn(), unlinkCloudVault: vi.fn(), syncCloudVault: vi.fn(), + getCloudSettingsConflict: vi.fn(), + resolveCloudSettingsConflict: vi.fn(), listCloudBackups: vi.fn(), getCloudBackupSchedule: vi.fn(), updateCloudBackupSchedule: vi.fn(), @@ -283,7 +285,7 @@ describe("CloudSettings", () => { pulled: 2, pushed: 3, conflicts: [], - bootstrap_conflicts: [], + bootstrap_conflicts: [], local_conflicts: [], }; mocks.syncCloudVault.mockResolvedValue(summary); @@ -324,6 +326,52 @@ describe("CloudSettings", () => { expect(host.textContent).not.toContain("Cursor 7"); }); + // Settings that differ between devices are a question, not a silent merge. + // Doing nothing keeps this device's settings, so the local choice leads. + it("asks which vault settings to keep and applies the answer", async () => { + mocks.getCloudAccountStatus.mockResolvedValue(connected); + mocks.getCloudServiceAccount.mockResolvedValue(serviceAccount); + mocks.getCloudVaultLink.mockResolvedValue({ + base_url: "https://zennotes.org", + vault_id: "vault-1", + vault_name: "Cloud Notes", + linked_at: "2026-08-10T12:00:00.000Z", + }); + mocks.listCloudVaults.mockResolvedValue([]); + mocks.getCloudSettingsConflict.mockResolvedValue({ + path: ".zennotes/vault.json", + cloud_path: ".zennotes/vault.cloud-conflict.json", + }); + + await act(async () => + root.render( + createElement(CloudSettings, { + localVaultAvailable: true, + localVaultName: "Notes", + }), + ), + ); + + expect(host.textContent).toContain("Vault settings differ from the cloud"); + expect(host.textContent).toContain("This device’s settings are the ones in use."); + + const keepLocal = [...host.querySelectorAll("button")].find( + (button) => button.textContent?.trim() === "Keep this device's", + ); + const useCloud = [...host.querySelectorAll("button")].find( + (button) => button.textContent?.trim() === "Use the cloud's", + ); + expect(keepLocal).toBeTruthy(); + expect(useCloud).toBeTruthy(); + + mocks.getCloudSettingsConflict.mockResolvedValue(null); + await act(async () => useCloud!.click()); + + expect(mocks.resolveCloudSettingsConflict).toHaveBeenCalledWith("cloud"); + // Answered, so the question stops being asked. + expect(host.textContent).not.toContain("Vault settings differ from the cloud"); + }); + it("does not request vault data when sync is not included", async () => { mocks.getCloudAccountStatus.mockResolvedValue(connected); mocks.getCloudServiceAccount.mockResolvedValue({ @@ -526,7 +574,7 @@ describe("CloudSettings", () => { pulled: 10, pushed: 0, conflicts: [], - bootstrap_conflicts: [], + bootstrap_conflicts: [], local_conflicts: [], }, }); mocks.updateCloudBackupSchedule.mockResolvedValue({ @@ -573,7 +621,7 @@ describe("CloudSettings", () => { pulled: 1, pushed: 0, conflicts: [], - bootstrap_conflicts: [], + bootstrap_conflicts: [], local_conflicts: [], }, }); diff --git a/packages/app-core/src/components/CloudSettings.tsx b/packages/app-core/src/components/CloudSettings.tsx index f6bd72d3..b7efb488 100644 --- a/packages/app-core/src/components/CloudSettings.tsx +++ b/packages/app-core/src/components/CloudSettings.tsx @@ -9,6 +9,8 @@ import type { CloudPublishedNote, CloudServiceAccount, CloudSyncRunSummary, + CloudSyncSettingsChoice, + CloudSyncSettingsConflict, CloudSyncVault, CloudUsage, CloudVaultLink, @@ -39,6 +41,8 @@ type CloudAction = | "backup-refresh" | "publish-refresh" | "publish-delete" + | "settings-local" + | "settings-cloud" | null; export function CloudSettings({ @@ -57,6 +61,8 @@ export function CloudSettings({ const [selectedVaultId, setSelectedVaultId] = useState(""); const [newVaultName, setNewVaultName] = useState(localVaultName); const [summary, setSummary] = useState(null); + const [settingsConflict, setSettingsConflict] = + useState(null); const [backups, setBackups] = useState([]); const [backupSchedule, setBackupSchedule] = useState(null); @@ -283,11 +289,36 @@ export function CloudSettings({ setRestoreResult(null); }); + const loadSettingsConflict = useCallback(async (): Promise => { + try { + setSettingsConflict(await bridge.getCloudSettingsConflict()); + } catch { + // A host without the question (the web client) simply has none to ask. + setSettingsConflict(null); + } + }, [bridge]); + + useEffect(() => { + void loadSettingsConflict(); + }, [loadSettingsConflict]); + const syncVault = (): Promise => runAction("sync", async () => { setSummary(await syncCloudVaultWithStatus(bridge, link?.vault_name)); + await loadSettingsConflict(); }); + const resolveSettingsConflict = ( + choice: CloudSyncSettingsChoice, + ): Promise => + runAction( + choice === "cloud" ? "settings-cloud" : "settings-local", + async () => { + await bridge.resolveCloudSettingsConflict(choice); + await loadSettingsConflict(); + }, + ); + const createBackup = (): Promise => runAction("backup-create", async () => { const label = backupLabel.trim() || undefined; @@ -497,6 +528,10 @@ export function CloudSettings({ onSync={() => void syncVault()} onUnlink={() => void unlinkVault()} onUseAnotherAccount={() => void logout()} + settingsConflict={settingsConflict} + onResolveSettingsConflict={(choice) => + void resolveSettingsConflict(choice) + } syncIncluded={serviceAccount.features.sync.active} /> void; onLink: () => void; onNewVaultNameChange: (value: string) => void; + onResolveSettingsConflict: (choice: CloudSyncSettingsChoice) => void; onSelectedVaultChange: (value: string) => void; onSync: () => void; onUnlink: () => void; @@ -1014,6 +1053,12 @@ function CloudVaultPanel({ + {settingsConflict && ( + + )} {summary && } ) : ( @@ -1696,13 +1741,62 @@ function numericLimit( : null; } +/** + * Vault settings that differ between this device and the cloud. Notes get a + * conflict copy to compare side by side, but settings are a single answer, and + * a copy of them inside a hidden folder is not something anyone can act on. + * This device's settings stay in use until the question is answered, so doing + * nothing keeps what is already working. + */ +function CloudSettingsConflictCard({ + action, + onResolve, +}: { + action: CloudAction; + onResolve: (choice: CloudSyncSettingsChoice) => void; +}): JSX.Element { + return ( +
+
Vault settings differ from the cloud
+
+ Another device saved different settings for this vault: favorites, + folder icons and colors, and where the built-in folders live. This + device’s settings are the ones in use. +
+
+ + +
+
+ ); +} + function CloudSyncSummary({ summary, }: { summary: CloudSyncRunSummary; }): JSX.Element { + // A host on an older build sends no local_conflicts at all. const conflictCount = - summary.conflicts.length + summary.bootstrap_conflicts.length; + summary.conflicts.length + + summary.bootstrap_conflicts.length + + (summary.local_conflicts?.length ?? 0); return (
=> { const disconnected: CloudAccountStatus = { @@ -199,7 +199,7 @@ describe("cloud auto sync host wiring", () => { pulled: 1, pushed: 0, conflicts: [], - bootstrap_conflicts: [], + bootstrap_conflicts: [], local_conflicts: [], }); }), ); @@ -267,7 +267,7 @@ describe("cloud auto sync host wiring", () => { pulled: 0, pushed: 0, conflicts: [], - bootstrap_conflicts: [], + bootstrap_conflicts: [], local_conflicts: [], }; }); const runtime = startCloudAutoSync(host.bridge, host.environment, { diff --git a/packages/bridge-contract/src/bridge.ts b/packages/bridge-contract/src/bridge.ts index 36a88d34..c6e230d1 100644 --- a/packages/bridge-contract/src/bridge.ts +++ b/packages/bridge-contract/src/bridge.ts @@ -48,6 +48,8 @@ import type { CloudPublishNoteInput, CloudServiceAccount, CloudSyncRunSummary, + CloudSyncSettingsChoice, + CloudSyncSettingsConflict, CloudSyncVault, CloudVaultLink } from './cloud-sync' @@ -139,6 +141,8 @@ export interface ZenBridge { createAndLinkCloudVault(name: string): Promise unlinkCloudVault(): Promise syncCloudVault(): Promise + getCloudSettingsConflict(): Promise + resolveCloudSettingsConflict(choice: CloudSyncSettingsChoice): Promise listCloudBackups(): Promise getCloudBackupSchedule(): Promise updateCloudBackupSchedule(enabled: boolean): Promise diff --git a/packages/bridge-contract/src/cloud-sync.ts b/packages/bridge-contract/src/cloud-sync.ts index 450e452f..067e25f9 100644 --- a/packages/bridge-contract/src/cloud-sync.ts +++ b/packages/bridge-contract/src/cloud-sync.ts @@ -363,10 +363,35 @@ export interface CloudSyncBootstrapConflict { remote_sha256: string; } +/** + * A remote change that could not be applied because the local file was not + * what sync last agreed on. The local file is always kept; `conflict_copy_path` + * is where the incoming version was parked, or null when the change was a + * delete or a move and there was no incoming content to keep. + */ +export interface CloudSyncLocalConflict { + code: "LOCAL_EDIT_CONFLICT" | "SETTINGS_CONFLICT"; + path: string; + conflict_copy_path: string | null; +} + +/** + * Vault settings that differ between this device and the cloud. The local + * settings stay in use; this is the pending question, and it survives + * restarts because the cloud's copy is parked in the vault until answered. + */ +export interface CloudSyncSettingsConflict { + path: string; + cloud_path: string; +} + +export type CloudSyncSettingsChoice = "local" | "cloud"; + export interface CloudSyncRunSummary { cursor: number; pulled: number; pushed: number; conflicts: CloudSyncConflict[]; bootstrap_conflicts: CloudSyncBootstrapConflict[]; + local_conflicts: CloudSyncLocalConflict[]; } diff --git a/packages/bridge-contract/src/ipc.ts b/packages/bridge-contract/src/ipc.ts index 1195bbe7..9384ca8e 100644 --- a/packages/bridge-contract/src/ipc.ts +++ b/packages/bridge-contract/src/ipc.ts @@ -123,6 +123,8 @@ export const IPC = { CLOUD_VAULT_LINK_CREATE: 'cloud-vault-link:create', CLOUD_VAULT_LINK_DELETE: 'cloud-vault-link:delete', CLOUD_VAULT_SYNC: 'cloud-vault:sync', + CLOUD_VAULT_SETTINGS_CONFLICT_GET: 'cloud-vault-settings-conflict:get', + CLOUD_VAULT_SETTINGS_CONFLICT_RESOLVE: 'cloud-vault-settings-conflict:resolve', CLOUD_BACKUPS_LIST: 'cloud-backups:list', CLOUD_BACKUP_SCHEDULE_GET: 'cloud-backup-schedule:get', CLOUD_BACKUP_SCHEDULE_UPDATE: 'cloud-backup-schedule:update', diff --git a/packages/shared-domain/src/cloud-backup.test.ts b/packages/shared-domain/src/cloud-backup.test.ts index 57f4ac00..2578a61f 100644 --- a/packages/shared-domain/src/cloud-backup.test.ts +++ b/packages/shared-domain/src/cloud-backup.test.ts @@ -42,7 +42,7 @@ describe('restoreCloudBackup', () => { pulled: 4, pushed: 0, conflicts: [], - bootstrap_conflicts: [] + bootstrap_conflicts: [], local_conflicts: [] })) await expect( diff --git a/packages/shared-domain/src/cloud-sync-coordinator.test.ts b/packages/shared-domain/src/cloud-sync-coordinator.test.ts index 86a48ad8..52348a9a 100644 --- a/packages/shared-domain/src/cloud-sync-coordinator.test.ts +++ b/packages/shared-domain/src/cloud-sync-coordinator.test.ts @@ -108,6 +108,74 @@ function remote(options: { } describe('CloudSyncCoordinator', () => { + // The Discord report behind this: a change for a file the device had never + // tracked threw, the run stopped before saving the cursor, and every later + // run replayed the same change and stopped at the same place. A repository + // that reports a conflict instead of throwing has to leave the run able to + // finish, or sync is wedged for good. + it('finishes the run and advances the cursor when a file reports a conflict', async () => { + const repository: CloudSyncRepository = { + async scan() { + return [] + }, + async apply(change) { + return { + code: 'LOCAL_EDIT_CONFLICT', + path: change.path, + conflict_copy_path: `${change.path} (cloud conflict)` + } + } + } + const states = memoryState({ + version: 1, + vault_id: 'vault-1', + cursor: 7, + items: {} + }) + const server = remote({ + changes: [ + { + sequence: 8, + item_id: 'item-untracked', + type: 'upsert', + path: '.zennotes/vault.json', + previous_path: null, + revision: 3, + content: content('{}') + } + ], + mutate: () => ({ acknowledged: [], conflicts: [], cursor: 8 }) + }) + + const first = await new CloudSyncCoordinator( + 'vault-1', + server, + repository, + states, + ids() + ).sync() + + expect(first.localConflicts).toEqual([ + { + code: 'LOCAL_EDIT_CONFLICT', + path: '.zennotes/vault.json', + conflict_copy_path: '.zennotes/vault.json (cloud conflict)' + } + ]) + expect(states.current?.cursor).toBe(8) + + // The next run is past it rather than replaying the same change forever. + const second = await new CloudSyncCoordinator( + 'vault-1', + server, + repository, + states, + ids() + ).sync() + expect(second.localConflicts).toEqual([]) + expect(states.current?.cursor).toBe(8) + }) + it('merges remote and local files on first sync without deleting either side', async () => { const repository = memoryRepository([ { path: 'local.md', kind: 'text', content: content('local') } diff --git a/packages/shared-domain/src/cloud-sync-coordinator.ts b/packages/shared-domain/src/cloud-sync-coordinator.ts index 6564c49d..7ef1168a 100644 --- a/packages/shared-domain/src/cloud-sync-coordinator.ts +++ b/packages/shared-domain/src/cloud-sync-coordinator.ts @@ -2,6 +2,7 @@ import type { CloudSyncChange, CloudSyncBootstrapConflict, CloudSyncConflict, + CloudSyncLocalConflict, CloudSyncManifestItem, CloudSyncManifestResponse, CloudSyncMutationRequest, @@ -39,7 +40,13 @@ export interface CloudSyncRemote { export interface CloudSyncRepository { scan(): Promise - apply(change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined): Promise + /** Returns a conflict when the local file was kept instead of being + * replaced, so one unapplied change reports itself rather than stopping + * the run. Sync must always be able to move past a single file. */ + apply( + change: CloudSyncChange, + previous: CloudSyncTrackedItem | undefined + ): Promise } export interface CloudSyncStateStore { @@ -53,6 +60,7 @@ export interface CloudSyncRunResult { pushed: number conflicts: CloudSyncConflict[] bootstrapConflicts: CloudSyncBootstrapConflict[] + localConflicts: CloudSyncLocalConflict[] } /** @@ -88,15 +96,18 @@ export class CloudSyncCoordinator { pulled: bootstrap.pulled, pushed: 0, conflicts: [], - bootstrapConflicts: bootstrap.conflicts + bootstrapConflicts: bootstrap.conflicts, + localConflicts: bootstrap.localConflicts } } let state = bootstrap.state let pulled = bootstrap.pulled + const localConflicts = [...bootstrap.localConflicts] const initialPull = await this.pullChanges(state) state = initialPull.state pulled += initialPull.pulled + localConflicts.push(...initialPull.localConflicts) const plan = planCloudSyncMutations(state, await this.repository.scan(), this.ids) const conflicts: CloudSyncConflict[] = [] @@ -122,17 +133,19 @@ export class CloudSyncCoordinator { const finalPull = await this.pullChanges(state, acknowledgedSequences) state = finalPull.state pulled += finalPull.pulled + localConflicts.push(...finalPull.localConflicts) } - return { state, pulled, pushed, conflicts, bootstrapConflicts: [] } + return { state, pulled, pushed, conflicts, bootstrapConflicts: [], localConflicts } } private async pullChanges( initialState: CloudSyncState, acknowledgedSequences: ReadonlySet = new Set() - ): Promise<{ state: CloudSyncState; pulled: number }> { + ): Promise<{ state: CloudSyncState; pulled: number; localConflicts: CloudSyncLocalConflict[] }> { let state = initialState let pulled = 0 + const localConflicts: CloudSyncLocalConflict[] = [] for (;;) { const response = await this.remote.changes(this.vaultId, state.cursor, CHANGE_PAGE_SIZE) @@ -140,7 +153,8 @@ export class CloudSyncCoordinator { for (const change of response.data) { if (!acknowledgedSequences.has(change.sequence)) { const previous = state.items[change.item_id] - await this.repository.apply(change, previous) + const conflict = await this.repository.apply(change, previous) + if (conflict) localConflicts.push(conflict) pulled++ } state = reduceCloudSyncChange(state, change) @@ -153,21 +167,23 @@ export class CloudSyncCoordinator { } } - return { state, pulled } + return { state, pulled, localConflicts } } private async loadOrBootstrap(): Promise<{ state: CloudSyncState pulled: number conflicts: CloudSyncBootstrapConflict[] + localConflicts: CloudSyncLocalConflict[] }> { const existing = await this.states.load(this.vaultId) - if (existing) return { state: existing, pulled: 0, conflicts: [] } + if (existing) return { state: existing, pulled: 0, conflicts: [], localConflicts: [] } const manifest = await this.stableManifest() const localItems = await this.repository.scan() const localByPath = new Map(localItems.map((item) => [cloudSyncPathKey(item.path), item])) const conflicts: CloudSyncBootstrapConflict[] = [] + const localConflicts: CloudSyncLocalConflict[] = [] let pulled = 0 for (const item of manifest.items) { @@ -185,7 +201,8 @@ export class CloudSyncCoordinator { if (!local) { if (!item.content) throw new Error(`Manifest item ${item.item_id} did not include content`) - await this.repository.apply(manifestUpsert(item), undefined) + const conflict = await this.repository.apply(manifestUpsert(item), undefined) + if (conflict) localConflicts.push(conflict) pulled++ } } @@ -193,7 +210,7 @@ export class CloudSyncCoordinator { const state = manifestState(this.vaultId, manifest.cursor, manifest.items) if (conflicts.length === 0) await this.states.save(state) - return { state, pulled, conflicts } + return { state, pulled, conflicts, localConflicts } } private async stableManifest(): Promise<{ diff --git a/packages/shared-domain/src/cloud-sync-host-service.test.ts b/packages/shared-domain/src/cloud-sync-host-service.test.ts index 4c7e4d06..def3fc09 100644 --- a/packages/shared-domain/src/cloud-sync-host-service.test.ts +++ b/packages/shared-domain/src/cloud-sync-host-service.test.ts @@ -289,7 +289,7 @@ describe('CloudSyncHostService', () => { current_path: 'Note.md' } ], - bootstrap_conflicts: [] + bootstrap_conflicts: [], local_conflicts: [] }) await expect(service.createBackup(hostVault)).rejects.toThrow('Resolve sync conflicts') diff --git a/packages/shared-domain/src/cloud-sync-host-service.ts b/packages/shared-domain/src/cloud-sync-host-service.ts index 7ae18de3..83eed768 100644 --- a/packages/shared-domain/src/cloud-sync-host-service.ts +++ b/packages/shared-domain/src/cloud-sync-host-service.ts @@ -229,7 +229,8 @@ export class CloudSyncHostService { pulled: result.pulled, pushed: result.pushed, conflicts: result.conflicts, - bootstrap_conflicts: result.bootstrapConflicts + bootstrap_conflicts: result.bootstrapConflicts, + local_conflicts: result.localConflicts } } finally { await vault.refresh() diff --git a/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts b/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts index cbbb71ec..aa50c914 100644 --- a/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts +++ b/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts @@ -171,33 +171,62 @@ describe('PortableCloudSyncRepository', () => { expect(fs.text('archive/New.md')).toBeNull() }) - it('stops before overwriting unsynced local edits', async () => { + // Neither version is thrown away: the local file stays put and the incoming + // one lands beside it. Throwing here used to stop the run before the cursor + // was saved, so every later run replayed the same change and stopped too. + it('keeps both versions instead of overwriting unsynced local edits', async () => { const fs = new MemoryFileSystem({ 'inbox/Plan.md': 'local edit' }) const repository = new PortableCloudSyncRepository(fs) - await expect( - repository.apply( - { - sequence: 2, - item_id: 'item-1', - type: 'upsert', - path: 'inbox/Plan.md', - previous_path: 'inbox/Plan.md', - revision: 2, - content: await textContent('remote edit') - }, - { - item_id: 'item-1', - path: 'inbox/Plan.md', - kind: 'text', - revision: 1, - sha256: (await textContent('old synced value')).sha256, - byte_length: 16, - media_type: 'text/markdown' - } - ) - ).rejects.toBeInstanceOf(CloudSyncLocalEditConflictError) + const conflict = await repository.apply( + { + sequence: 2, + item_id: 'item-1', + type: 'upsert', + path: 'inbox/Plan.md', + previous_path: 'inbox/Plan.md', + revision: 2, + content: await textContent('remote edit') + }, + { + item_id: 'item-1', + path: 'inbox/Plan.md', + kind: 'text', + revision: 1, + sha256: (await textContent('old synced value')).sha256, + byte_length: 16, + media_type: 'text/markdown' + } + ) + + expect(conflict).toEqual({ + code: 'LOCAL_EDIT_CONFLICT', + path: 'inbox/Plan.md', + conflict_copy_path: 'inbox/Plan (cloud conflict).md' + }) expect(fs.text('inbox/Plan.md')).toBe('local edit') + expect(fs.text('inbox/Plan (cloud conflict).md')).toBe('remote edit') + }) + + it('adopts a file that already matches the incoming change', async () => { + const fs = new MemoryFileSystem({ '.zennotes/vault.json': '{"favorites":[]}' }) + const repository = new PortableCloudSyncRepository(fs) + + const conflict = await repository.apply( + { + sequence: 8, + item_id: 'item-untracked', + type: 'upsert', + path: '.zennotes/vault.json', + previous_path: null, + revision: 3, + content: await textContent('{"favorites":[]}') + }, + undefined + ) + + expect(conflict).toBeUndefined() + expect(fs.text('.zennotes/vault.json')).toBe('{"favorites":[]}') }) it('keeps device-local workspace state out of scans and ignores remote workspace mutations', async () => { diff --git a/packages/shared-domain/src/cloud-sync-portable-filesystem.ts b/packages/shared-domain/src/cloud-sync-portable-filesystem.ts index c678e964..3781628d 100644 --- a/packages/shared-domain/src/cloud-sync-portable-filesystem.ts +++ b/packages/shared-domain/src/cloud-sync-portable-filesystem.ts @@ -1,8 +1,12 @@ import type { CloudSyncChange, - CloudSyncContent + CloudSyncContent, + CloudSyncLocalConflict } from '@zennotes/bridge-contract/cloud-sync' import { + CLOUD_SYNC_SETTINGS_CONFLICT_PATH, + cloudSyncConflictCopyPath, + isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, shouldSyncVaultPath, shouldTraverseCloudSyncDirectory @@ -81,6 +85,18 @@ export class CloudSyncLocalEditConflictError extends Error { } } +/** Whether a local file is the exact bytes sync last agreed on with the server. */ +function vouchedFor( + current: CloudSyncLocalItem, + previous: CloudSyncTrackedItem | undefined +): boolean { + return Boolean(previous) && current.content.sha256 === previous?.sha256 +} + +function localConflict(path: string, conflictCopyPath: string | null): CloudSyncLocalConflict { + return { code: 'LOCAL_EDIT_CONFLICT', path, conflict_copy_path: conflictCopyPath } +} + /** Web-API implementation shared by iOS and Android Capacitor filesystems. */ export class PortableCloudSyncRepository implements CloudSyncRepository { constructor(private readonly fs: PortableCloudSyncFileSystem) {} @@ -91,7 +107,10 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { return items.sort((left, right) => left.path.localeCompare(right.path)) } - async apply(change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined): Promise { + async apply( + change: CloudSyncChange, + previous: CloudSyncTrackedItem | undefined + ): Promise { const affectedPaths = [change.path, change.previous_path, previous?.path].filter( (path): path is string => typeof path === 'string' ) @@ -101,7 +120,9 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { const previousPath = this.path(previous?.path ?? change.previous_path ?? change.path) const current = await this.readItemOrNull(previousPath) if (!current) return - this.assertTracked(previousPath, current, previous) + // Nothing arrives with a delete to keep beside it, so the local file + // itself is the version being preserved. The next push re-uploads it. + if (!vouchedFor(current, previous)) return localConflict(previousPath, null) await this.fs.deleteFile(previousPath) return } @@ -117,10 +138,11 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { ]) if (!source) { if (destination && previous && destination.content.sha256 === previous.sha256) return - throw new CloudSyncLocalEditConflictError(previousPath) + // Nothing here to move; the next scan reconciles it. + return } - this.assertTracked(previousPath, source, previous) - if (destination) throw new CloudSyncLocalEditConflictError(nextPath) + if (!vouchedFor(source, previous)) return localConflict(previousPath, null) + if (destination) return localConflict(nextPath, null) await this.fs.rename(previousPath, nextPath) return } @@ -140,18 +162,44 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { if (currentAtTarget?.content.sha256 === change.content.sha256) { if (previousPath !== nextPath && source) { - this.assertTracked(previousPath, source, previous) + if (!vouchedFor(source, previous)) return localConflict(previousPath, null) await this.fs.deleteFile(previousPath) } return } - this.assertTracked(previousPath, source, previous) - if (destination) throw new CloudSyncLocalEditConflictError(nextPath) + if (source && !vouchedFor(source, previous)) return await this.keepBoth(nextPath, change.content) + if (destination) return await this.keepBoth(nextPath, change.content) await this.write(nextPath, change.content) if (previousPath !== nextPath && source) await this.fs.deleteFile(previousPath) } + /** Park the incoming version beside the local file rather than over it. */ + private async keepBoth( + relPath: string, + content: CloudSyncContent + ): Promise { + // Settings are answered, not merged: the newest cloud version replaces any + // older pending one at a fixed path, and the app asks which side to keep. + if (isCloudSyncVaultSettingsPath(relPath)) { + await this.write(CLOUD_SYNC_SETTINGS_CONFLICT_PATH, content) + return { + code: 'SETTINGS_CONFLICT', + path: relPath, + conflict_copy_path: CLOUD_SYNC_SETTINGS_CONFLICT_PATH + } + } + for (let attempt = 1; attempt <= 100; attempt++) { + const candidate = cloudSyncConflictCopyPath(relPath, attempt) + if ((await this.fs.stat(candidate)) !== null) continue + await this.write(candidate, content) + return localConflict(relPath, candidate) + } + // A hundred conflict copies of one file means something is looping. Keep + // the local file and report it rather than filling the vault. + return localConflict(relPath, null) + } + private async walk(directory: string, items: CloudSyncLocalItem[]): Promise { const entries = await this.fs.readdir(directory) for (const entry of entries) { @@ -187,17 +235,6 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { } } - private assertTracked( - path: string, - current: CloudSyncLocalItem | null, - previous: CloudSyncTrackedItem | undefined - ): void { - if (!previous && !current) return - if (!previous || !current || current.content.sha256 !== previous.sha256) { - throw new CloudSyncLocalEditConflictError(path) - } - } - private async write(path: string, content: CloudSyncContent): Promise { if (content.encoding === 'utf8') { await this.fs.writeText(path, content.data) diff --git a/packages/shared-domain/src/cloud-sync.test.ts b/packages/shared-domain/src/cloud-sync.test.ts index 53e73aed..9894da50 100644 --- a/packages/shared-domain/src/cloud-sync.test.ts +++ b/packages/shared-domain/src/cloud-sync.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from 'vitest' import { + cloudSyncConflictCopyPath, cloudSyncPathKey, + isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, shouldTraverseCloudSyncDirectory, shouldSyncVaultPath @@ -25,6 +27,26 @@ describe('cloudSyncPathKey', () => { }) }) +describe('cloudSyncConflictCopyPath', () => { + it('keeps the extension so the copy opens like the original', () => { + expect(cloudSyncConflictCopyPath('inbox/Note.md', 1)).toBe('inbox/Note (cloud conflict).md') + expect(cloudSyncConflictCopyPath('inbox/Note.md', 3)).toBe('inbox/Note (cloud conflict 3).md') + expect(cloudSyncConflictCopyPath('Note.md', 1)).toBe('Note (cloud conflict).md') + }) + + it('treats a leading dot as part of the name, not an extension', () => { + expect(cloudSyncConflictCopyPath('.gitignore', 1)).toBe('.gitignore (cloud conflict)') + }) +}) + +describe('isCloudSyncVaultSettingsPath', () => { + it('matches only the vault settings file', () => { + expect(isCloudSyncVaultSettingsPath('.zennotes/vault.json')).toBe(true) + expect(isCloudSyncVaultSettingsPath('.zennotes/vault.cloud-conflict.json')).toBe(false) + expect(isCloudSyncVaultSettingsPath('inbox/vault.json')).toBe(false) + }) +}) + describe('shouldSyncVaultPath', () => { it.each([ 'inbox/Note.md', @@ -49,6 +71,9 @@ describe('shouldSyncVaultPath', () => { '.zennotes/deleted-assets/token/file.png', '.zennotes/sync/device-state.json', '.zennotes/unknown-runtime-cache.json', + // The cloud's settings waiting for an answer are this device's business, + // and uploading them would hand the question to every other device too. + '.zennotes/vault.cloud-conflict.json', '.git/config', 'vendor/project/.svn/entries', 'node_modules/package/index.js' diff --git a/packages/shared-domain/src/cloud-sync.ts b/packages/shared-domain/src/cloud-sync.ts index f928a566..96e796b7 100644 --- a/packages/shared-domain/src/cloud-sync.ts +++ b/packages/shared-domain/src/cloud-sync.ts @@ -86,6 +86,50 @@ export function shouldSyncVaultPath(path: string): boolean { ) } +/** The one file under `.zennotes` that carries user-authored vault settings. */ +export const CLOUD_SYNC_VAULT_SETTINGS_PATH = '.zennotes/vault.json' + +/** + * Where the cloud's settings wait while the user decides which side to keep. + * + * Settings are not a note: a numbered pile of conflict copies inside a hidden + * folder is not something anyone can act on, so the newest remote version + * lands at one fixed path and the app asks. The local settings stay in use + * until the user says otherwise. + */ +export const CLOUD_SYNC_SETTINGS_CONFLICT_PATH = '.zennotes/vault.cloud-conflict.json' + +export function isCloudSyncVaultSettingsPath(path: string): boolean { + try { + return normalizeCloudSyncPath(path).toLowerCase() === CLOUD_SYNC_VAULT_SETTINGS_PATH + } catch { + return false + } +} + +/** + * Where a remote version is parked when it cannot replace the local file. + * + * Sync refuses to overwrite a file it cannot vouch for, and refusing used to + * stop the whole run: the cursor never advanced, so every later sync retried + * the same change and failed the same way, forever. Keeping both versions ends + * that. The local file stays exactly where it is and the incoming one lands + * beside it, which is the outcome every other sync tool converged on because + * it cannot lose either side. + */ +export function cloudSyncConflictCopyPath(path: string, attempt: number): string { + const normalized = normalizeCloudSyncPath(path) + const slash = normalized.lastIndexOf('/') + const directory = slash === -1 ? '' : normalized.slice(0, slash + 1) + const name = normalized.slice(slash + 1) + // A leading dot is part of the name, not an extension: `.gitignore` keeps it. + const dot = name.lastIndexOf('.') + const stem = dot > 0 ? name.slice(0, dot) : name + const extension = dot > 0 ? name.slice(dot) : '' + const suffix = attempt > 1 ? `(cloud conflict ${attempt})` : '(cloud conflict)' + return `${directory}${stem} ${suffix}${extension}` +} + /** Skip large device-local trees before reading their contents. */ export function shouldTraverseCloudSyncDirectory(path: string): boolean { let normalized: string From d1f8fd269632a4f632e92ca662bbd541a8075003 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 10:38:46 -0500 Subject: [PATCH 09/18] Fix(editor): Home and End reach the real edge of a wrapped display row (#591) On a soft-wrapped line, Home and End could land short of the row edge or on a neighboring row, exactly as a bare $ did before a16300f. The reporter spotted that it was the same bug wearing different keys, and they were right: Home and End were never our bindings at all. They fell through to CodeMirror's cursorLineBoundaryBackward/Forward, which locate the row edge by hit-testing an x coordinate at the editor's edge, the same resolution that #575 removed from $ because it walks sub-pixel glyph rects under fractional display scaling. ZenNotes gives that hit-test further to travel than plain CodeMirror does. It probes view.dom, the whole editor element, while the text column is centered inside it, so the probed x sits well outside the text: 87 pixels past it on the machine this was measured on. codemirror-vim maps neither key, so Vim users were on the same CodeMirror path rather than a motion. Both keys are now bound ahead of defaultKeymap to commands that compute the boundary from row geometry through displayRowEdge, the helper $, g0, A and I already use, so no x coordinate is resolved anywhere. Shift extends the selection the same way, and Mod-Home/Mod-End still fall through to document start and end. A caret resting exactly on a wrap point belongs to two rows at once, so, like CodeMirror's own boundary motion, the character before a backward-associated caret is what gets measured; without that a second End press walked on to the next row. displayRowEdge moves out of cm-vim-display-line.ts into cm-display-row.ts, since it is now shared by the Vim motions and by keys that have nothing to do with Vim, and this repo would rather move a function than keep a second copy of it. The mislanding does not reproduce on a pixel-accurate display, as in #575, and emulating fractional device scale factors did not provoke it either, so the verification is structural: unit tests assert exact landings under jittered row coordinates and that posAtCoords is never called at all, and in the running app the true wrap points were measured from DOM text rects, with Home and End landing exactly on every display row in both modes. --- .../app-core/src/components/EditorPane.tsx | 6 + .../app-core/src/lib/cm-display-row.test.ts | 108 ++++++++++++ packages/app-core/src/lib/cm-display-row.ts | 155 ++++++++++++++++++ .../app-core/src/lib/cm-vim-display-line.ts | 68 +------- 4 files changed, 270 insertions(+), 67 deletions(-) create mode 100644 packages/app-core/src/lib/cm-display-row.test.ts create mode 100644 packages/app-core/src/lib/cm-display-row.ts diff --git a/packages/app-core/src/components/EditorPane.tsx b/packages/app-core/src/components/EditorPane.tsx index 53915bda..2c2b8e1a 100644 --- a/packages/app-core/src/components/EditorPane.tsx +++ b/packages/app-core/src/components/EditorPane.tsx @@ -50,6 +50,7 @@ import { } from '@codemirror/commands' import { markdown, markdownLanguage } from '@codemirror/lang-markdown' import { isImeComposing } from '../lib/ime' +import { displayRowBoundaryKeymap } from '../lib/cm-display-row' import { resolveCodeLanguage } from '../lib/cm-code-languages' import { customCodeFenceHighlightExtension } from '../lib/cm-custom-code-languages' import { @@ -325,6 +326,11 @@ function pointerOverRange( function buildEditorKeymap(vimMode: boolean, overrides: KeymapOverrides): Extension { return keymap.of([ + // Home/End on the display row the user can see. Listed before + // defaultKeymap, whose versions hit-test an x coordinate at the editor's + // edge and misland on wrapped lines under fractional display scaling + // (#591, the same resolution #575 removed from `$`). + ...displayRowBoundaryKeymap, { key: 'Mod-f', run: () => { diff --git a/packages/app-core/src/lib/cm-display-row.test.ts b/packages/app-core/src/lib/cm-display-row.test.ts new file mode 100644 index 00000000..715eca1a --- /dev/null +++ b/packages/app-core/src/lib/cm-display-row.test.ts @@ -0,0 +1,108 @@ +import { describe, expect, it, vi } from 'vitest' +import { EditorSelection, EditorState } from '@codemirror/state' +import type { EditorView } from '@codemirror/view' +import { + cursorDisplayRowEnd, + cursorDisplayRowStart, + selectDisplayRowEnd +} from './cm-display-row' + +// Simulated layout: line 2 (offsets 6..106) holds 100 characters wrapping into +// rows of 30, so the wrap points sit at 36, 66 and 96. `jitter` adds sub-pixel +// noise like the fractional-scaling environments where an x hit-test mislands. +const DOC = `alpha\n${'x'.repeat(100)}\nomega` + +function fakeView(jitter = false) { + const state = EditorState.create({ + doc: DOC, + selection: EditorSelection.cursor(50) + }) + const posAtCoords = vi.fn(() => 0) + const view = { + state, + posAtCoords, + coordsAtPos: (offset: number) => { + if (offset < 6 || offset > 106) return null + const row = Math.min(3, Math.floor((offset - 6) / 30)) + const noise = jitter ? ((offset * 7) % 5) - 2 : 0 + const top = 100 + row * 20 + noise + return { left: 0, right: 0, top, bottom: top + 18 } + }, + dispatch: vi.fn((spec: { selection?: EditorSelection }) => { + if (spec.selection) view.state = state.update({ selection: spec.selection }).state + }) + } + return view as unknown as EditorView & { + posAtCoords: ReturnType + dispatch: ReturnType + } +} + +function cursorAfter(view: ReturnType): { head: number; assoc: number } { + const spec = view.dispatch.mock.calls.at(-1)?.[0] as { selection: EditorSelection } + const range = spec.selection.main + return { head: range.head, assoc: range.assoc } +} + +// #591: Home and End were CodeMirror's own bindings, which find the row edge by +// hit-testing an x coordinate at the editor's edge. That is the resolution #575 +// removed from `$` because it lands short of the wrap point, or on a +// neighboring row, under fractional display scaling. +describe('Home/End on a wrapped display row (#591)', () => { + it('End lands on the end of the row the cursor is on, never past it', () => { + const view = fakeView() + expect(cursorDisplayRowEnd(view)).toBe(true) + // Offset 50 sits in the second row (36..65), which ends at 66. + expect(cursorAfter(view)).toEqual({ head: 66, assoc: -1 }) + }) + + it('Home lands on the start of that same row', () => { + const view = fakeView() + expect(cursorDisplayRowStart(view)).toBe(true) + expect(cursorAfter(view)).toEqual({ head: 36, assoc: 1 }) + }) + + it('never resolves an x coordinate, which is what mislands', () => { + const view = fakeView() + cursorDisplayRowEnd(view) + cursorDisplayRowStart(view) + expect(view.posAtCoords).not.toHaveBeenCalled() + }) + + it('sub-pixel jitter in the row coordinates changes nothing', () => { + const view = fakeView(true) + cursorDisplayRowEnd(view) + expect(cursorAfter(view).head).toBe(66) + }) + + it('Shift+End extends the selection to the row end instead of moving the caret', () => { + const view = fakeView() + expect(selectDisplayRowEnd(view)).toBe(true) + const spec = view.dispatch.mock.calls.at(-1)?.[0] as { selection: EditorSelection } + expect(spec.selection.main.anchor).toBe(50) + expect(spec.selection.main.head).toBe(66) + }) + + it('reports the key as handled at the boundary so the old bindings never run', () => { + const view = fakeView() + cursorDisplayRowEnd(view) + view.dispatch.mockClear() + // A second press has nowhere to go, but handing the key back to + // CodeMirror would reintroduce the hit-testing this replaces. + expect(cursorDisplayRowEnd(view)).toBe(true) + expect(view.dispatch).not.toHaveBeenCalled() + }) + + it('falls back to the logical line boundary when coordinates are unavailable', () => { + const state = EditorState.create({ doc: DOC, selection: EditorSelection.cursor(50) }) + const view = { + state, + posAtCoords: vi.fn(), + coordsAtPos: () => null, + dispatch: vi.fn() + } as unknown as EditorView & { dispatch: ReturnType } + cursorDisplayRowEnd(view) + const spec = view.dispatch.mock.calls.at(-1)?.[0] as { selection: EditorSelection } + expect(spec.selection.main.head).toBe(106) + }) +}) diff --git a/packages/app-core/src/lib/cm-display-row.ts b/packages/app-core/src/lib/cm-display-row.ts new file mode 100644 index 00000000..9d5509a8 --- /dev/null +++ b/packages/app-core/src/lib/cm-display-row.ts @@ -0,0 +1,155 @@ +import { EditorSelection } from '@codemirror/state' +import type { Command, EditorView, KeyBinding } from '@codemirror/view' + +/** + * The wrap point ending the display row that contains `pos` (forward), or the + * offset starting that row (backward). Forward returns `line.to` when the + * cursor sits on the line's last row. + * + * Found by binary-searching `coordsAtPos` rows instead of hit-testing an x + * coordinate at the viewport edge, which is what `goLineRight` does and what + * #575 broke: under fractional display scaling the x resolution walks + * sub-pixel glyph rects and lands several characters short of the wrap point, + * or on a neighboring row entirely. Two positions count as the same row when + * their vertical ranges overlap, not when their midpoints sit close: an + * inline widget on the row (a rendered wikilink chip, say) can be taller + * than the text beside it, and a midpoint tolerance misread that skew as a + * wrap, which sent `A` and `$` short of a line-ending link (#582). Returns + * null when coordinates are unavailable (unrendered or widget-only spans); + * callers fall back structurally. + * + * Shared by the Vim display-row motions (`$`, `g0`, `A`, `I`) and the Home/End + * keys, which are not Vim-specific and had the same mislanding (#591). + */ +export function displayRowEdge(view: EditorView, pos: number, forward: boolean): number | null { + const line = view.state.doc.lineAt(pos) + const rowCoords = (offset: number) => { + const side: 1 | -1 = offset >= line.to ? -1 : 1 + const other: 1 | -1 = side === 1 ? -1 : 1 + return view.coordsAtPos(offset, side) ?? view.coordsAtPos(offset, other) + } + const anchorCoords = rowCoords(pos) + if (!anchorCoords) return null + const sameRow = (offset: number): boolean | null => { + const coords = rowCoords(offset) + if (!coords) return null + const overlap = + Math.min(coords.bottom, anchorCoords.bottom) - Math.max(coords.top, anchorCoords.top) + const shortest = Math.min( + coords.bottom - coords.top, + anchorCoords.bottom - anchorCoords.top + ) + return overlap > Math.max(1, shortest / 4) + } + if (forward) { + let lo = pos + let hi = line.to + const atEnd = sameRow(hi) + if (atEnd == null) return null + if (atEnd) return line.to + while (lo + 1 < hi) { + const mid = (lo + hi) >> 1 + const same = sameRow(mid) + if (same == null) return null + if (same) lo = mid + else hi = mid + } + return hi + } + let lo = line.from + let hi = pos + const atStart = sameRow(lo) + if (atStart == null) return null + if (atStart) return line.from + while (lo + 1 < hi) { + const mid = (lo + hi) >> 1 + const same = sameRow(mid) + if (same == null) return null + if (same) hi = mid + else lo = mid + } + return hi +} + +/** The row boundary, or the logical line's when coordinates are unavailable. */ +function rowBoundary(view: EditorView, head: number, assoc: number, forward: boolean): number { + const line = view.state.doc.lineAt(head) + // An offset sitting exactly on a wrap point belongs to two rows at once: it + // ends one and starts the next. A caret that arrived there moving forward + // carries assoc -1 and renders at the end of the row it came along, so + // measure the character before it, which is what CodeMirror's own boundary + // motion does. Without this a second End press walks on to the next row. + const probe = assoc < 0 && head > line.from ? head - 1 : head + let edge: number | null = null + try { + edge = displayRowEdge(view, probe, forward) + } catch { + edge = null + } + return edge ?? (forward ? line.to : line.from) +} + +/** + * Home/End on the display row the user can actually see. + * + * CodeMirror's own `cursorLineBoundaryForward`/`Backward` find the row edge by + * hit-testing an x coordinate at the editor's left or right edge + * (`moveToLineBoundary` in @codemirror/view). That is the same resolution that + * sent `$` several characters short of the wrap point, or onto a neighboring + * row, under fractional display scaling (#575), and Home/End inherited it + * unchanged (#591). ZenNotes gives the hit-test even further to travel: the + * editor column is centered inside a much wider editor element, so the probed + * x sits well outside the text. + * + * These bindings compute the boundary from row geometry instead, so no x + * coordinate is resolved at all. The returned cursor keeps CodeMirror's own + * association (`-1` forward, `1` backward) so a caret landing exactly on a + * wrap point renders at the end of the row it moved along rather than at the + * start of the next one. + */ +function displayRowBoundaryCommand(forward: boolean, extend: boolean): Command { + return (view) => { + const { selection } = view.state + const next = EditorSelection.create( + selection.ranges.map((range) => { + const target = rowBoundary(view, range.head, range.assoc, forward) + return extend + ? EditorSelection.range(range.anchor, target) + : EditorSelection.cursor(target, forward ? -1 : 1) + }), + selection.mainIndex + ) + // Always report the key as handled, even when the cursor was already on the + // boundary. Returning false would hand Home/End back to CodeMirror's + // hit-testing commands, which is the behavior these replace. + if (!next.eq(selection)) { + view.dispatch({ selection: next, scrollIntoView: true, userEvent: 'select' }) + } + return true + } +} + +export const cursorDisplayRowStart = displayRowBoundaryCommand(false, false) +export const cursorDisplayRowEnd = displayRowBoundaryCommand(true, false) +export const selectDisplayRowStart = displayRowBoundaryCommand(false, true) +export const selectDisplayRowEnd = displayRowBoundaryCommand(true, true) + +/** + * Listed ahead of `defaultKeymap`, whose Home/End bindings these replace. + * `Mod-Home`/`Mod-End` (document start/end) carry a modifier and so still fall + * through to it. + */ +export const displayRowBoundaryKeymap: readonly KeyBinding[] = [ + { + key: 'Home', + run: cursorDisplayRowStart, + shift: selectDisplayRowStart, + preventDefault: true + }, + { + key: 'End', + run: cursorDisplayRowEnd, + shift: selectDisplayRowEnd, + preventDefault: true + } +] diff --git a/packages/app-core/src/lib/cm-vim-display-line.ts b/packages/app-core/src/lib/cm-vim-display-line.ts index 80390164..5cc144e2 100644 --- a/packages/app-core/src/lib/cm-vim-display-line.ts +++ b/packages/app-core/src/lib/cm-vim-display-line.ts @@ -1,5 +1,6 @@ import { CodeMirror, Vim } from '@replit/codemirror-vim' import type { EditorView } from '@codemirror/view' +import { displayRowEdge } from './cm-display-row' import { mathBlockLineRanges } from './cm-math-render' import { embedBlockLineRanges } from './cm-embed-render' import { mermaidBlockLineRanges } from './cm-mermaid-render' @@ -111,73 +112,6 @@ function isWrapPoint(view: EditorView, offset: number): boolean { return after.top - before.top > (before.bottom - before.top) / 2 } -/** - * The wrap point ending the display row that contains `pos` (forward), or the - * offset starting that row (backward). Forward returns `line.to` when the - * cursor sits on the line's last row. - * - * Found by binary-searching `coordsAtPos` rows instead of hit-testing an x - * coordinate at the viewport edge, which is what `goLineRight` does and what - * #575 broke: under fractional display scaling the x resolution walks - * sub-pixel glyph rects and lands several characters short of the wrap point, - * or on a neighboring row entirely. Two positions count as the same row when - * their vertical ranges overlap, not when their midpoints sit close: an - * inline widget on the row (a rendered wikilink chip, say) can be taller - * than the text beside it, and a midpoint tolerance misread that skew as a - * wrap, which sent `A` and `$` short of a line-ending link (#582). Returns - * null when coordinates are unavailable (unrendered or widget-only spans); - * callers fall back structurally. - */ -function displayRowEdge(view: EditorView, pos: number, forward: boolean): number | null { - const line = view.state.doc.lineAt(pos) - const rowCoords = (offset: number) => { - const side: 1 | -1 = offset >= line.to ? -1 : 1 - const other: 1 | -1 = side === 1 ? -1 : 1 - return view.coordsAtPos(offset, side) ?? view.coordsAtPos(offset, other) - } - const anchorCoords = rowCoords(pos) - if (!anchorCoords) return null - const sameRow = (offset: number): boolean | null => { - const coords = rowCoords(offset) - if (!coords) return null - const overlap = - Math.min(coords.bottom, anchorCoords.bottom) - Math.max(coords.top, anchorCoords.top) - const shortest = Math.min( - coords.bottom - coords.top, - anchorCoords.bottom - anchorCoords.top - ) - return overlap > Math.max(1, shortest / 4) - } - if (forward) { - let lo = pos - let hi = line.to - const atEnd = sameRow(hi) - if (atEnd == null) return null - if (atEnd) return line.to - while (lo + 1 < hi) { - const mid = (lo + hi) >> 1 - const same = sameRow(mid) - if (same == null) return null - if (same) lo = mid - else hi = mid - } - return hi - } - let lo = line.from - let hi = pos - const atStart = sameRow(lo) - if (atStart == null) return null - if (atStart) return line.from - while (lo + 1 < hi) { - const mid = (lo + hi) >> 1 - const same = sameRow(mid) - if (same == null) return null - if (same) hi = mid - else lo = mid - } - return hi -} - /** * `j`/`k` motion that moves by *visual* (display) line through soft-wrapped * content instead of skipping to the next logical line (#290). With wrapping on From d7acdc6c7fb906286eae9f4b1f2768df16ce4dbf Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 11:18:07 -0500 Subject: [PATCH 10/18] Feat(vim): ]] and [[ jump to the next and previous heading (#578) Requested after Zed, which maps the same keys, and the keys Vim itself uses to move between sections. In a note the sections are the headings, so `]]` goes to the next one and `[[` to the one before. It is a motion rather than a command, which is what makes it worth having: `d]]` deletes to the next heading, `v]]` selects to it, `3]]` skips three, and `Ctrl+O` comes back, none of which needs writing. The headings come from the parser the outline panel and `Space p` already use, so a `# comment` inside a code fence or a `#` line in frontmatter is never a destination, and what the motion stops on is exactly what the outline lists. With no heading left that way the cursor runs to the end or start of the note, like Vim's section motions, so the key never sits there doing nothing. The keys turned out to be unreachable before they could work. VimNav carries `[b`/`]b` buffer switching as a global fallback for when focus is outside the editor, and it consumed the first `[` or `]` to arm that sequence, with preventDefault and stopImmediatePropagation, so codemirror-vim never saw either press. A bare `]]` did nothing while `2]]` worked, because a pending count already had an exception carved out for it: the same problem was found once before for `f[` and patched narrowly. That fallback now stands down for the whole focused editor, which is where it never belonged: codemirror-vim has `[b`, `]b`, `gt` and `gT` mapped itself. Any future `[x` or `]x` motion would have hit the same wall. Buffer and tab keys are mapped in visual context too, so nothing that used to reach the fallback from a standing selection loses its binding. Two suites cover it: the motion directly, and the keys pressed for real through codemirror-vim, which is the one that matters because `]` is a built-in Vim motion that `]]` also matches, and only a real keypress proves which mapping wins. `d]]` leaves a blank line behind, which is Vim's own rule for an exclusive motion ending in column one, so the test asserts that rather than pretending otherwise. --- packages/app-core/src/components/Editor.tsx | 38 ++++-- .../src/components/FloatingNoteApp.tsx | 2 + .../src/components/QuickCaptureApp.tsx | 2 + packages/app-core/src/components/VimNav.tsx | 22 ++-- .../lib/cm-vim-heading-motion-keys.test.ts | 100 +++++++++++++++ .../src/lib/cm-vim-heading-motion.test.ts | 116 ++++++++++++++++++ .../app-core/src/lib/cm-vim-heading-motion.ts | 99 +++++++++++++++ packages/app-core/src/lib/help.ts | 1 + 8 files changed, 355 insertions(+), 25 deletions(-) create mode 100644 packages/app-core/src/lib/cm-vim-heading-motion-keys.test.ts create mode 100644 packages/app-core/src/lib/cm-vim-heading-motion.test.ts create mode 100644 packages/app-core/src/lib/cm-vim-heading-motion.ts diff --git a/packages/app-core/src/components/Editor.tsx b/packages/app-core/src/components/Editor.tsx index 418dece0..969fea27 100644 --- a/packages/app-core/src/components/Editor.tsx +++ b/packages/app-core/src/components/Editor.tsx @@ -11,6 +11,7 @@ import { useCallback, useEffect, useMemo, useRef } from "react"; import type { EditorView } from "@codemirror/view"; import { Vim, getCM } from "@replit/codemirror-vim"; import { registerDisplayLineMotion } from "../lib/cm-vim-display-line"; +import { registerHeadingMotion } from "../lib/cm-vim-heading-motion"; import { moveLineDown, moveLineUp } from "@codemirror/commands"; import { foldAll, unfoldAll, foldCode, unfoldCode } from "@codemirror/language"; import { isTagsViewActive, isTasksViewActive, useStore } from "../store"; @@ -156,8 +157,15 @@ function editorHalfPage(view: EditorView | undefined, forward: boolean): void { } function syncVimKeymaps(overrides: KeymapOverrides): void { - const mappings: Array<{ id: KeymapId; action: string; bindings: string[] }> = - [ + const mappings: Array<{ + id: KeymapId; + action: string; + bindings: string[]; + // VimNav's global fallback stands down while the editor has focus (#578), + // so anything that used to reach it from a standing selection has to be + // mapped in visual context here as well. + contexts?: Array<"normal" | "visual">; + }> = [ { id: "vim.goToDefinition", action: "goToDefinition", @@ -187,6 +195,7 @@ function syncVimKeymaps(overrides: KeymapOverrides): void { }, { id: "vim.bufferPrevious", + contexts: ["normal", "visual"], action: "previousBuffer", bindings: [ toVimSequence(getKeymapBinding(overrides, "vim.bufferPrevious")), @@ -194,6 +203,7 @@ function syncVimKeymaps(overrides: KeymapOverrides): void { }, { id: "vim.bufferNext", + contexts: ["normal", "visual"], action: "nextBuffer", bindings: [ toVimSequence(getKeymapBinding(overrides, "vim.bufferNext")), @@ -201,6 +211,7 @@ function syncVimKeymaps(overrides: KeymapOverrides): void { }, { id: "vim.tabPrevious", + contexts: ["normal", "visual"], action: "previousBuffer", bindings: [ toVimSequence(getKeymapBinding(overrides, "vim.tabPrevious")), @@ -208,6 +219,7 @@ function syncVimKeymaps(overrides: KeymapOverrides): void { }, { id: "vim.tabNext", + contexts: ["normal", "visual"], action: "nextBuffer", bindings: [ toVimSequence(getKeymapBinding(overrides, "vim.tabNext")), @@ -258,21 +270,20 @@ function syncVimKeymaps(overrides: KeymapOverrides): void { ]; for (const mapping of mappings) { + const contexts = mapping.contexts ?? ["normal"]; for (const binding of syncedVimBindings[mapping.id] ?? []) { - try { - Vim.unmap(binding, "normal"); - } catch { - /* ignore */ + for (const context of contexts) { + try { + Vim.unmap(binding, context); + } catch { + /* ignore */ + } } } for (const binding of mapping.bindings) { - Vim.mapCommand( - binding, - "action", - mapping.action, - {}, - { context: "normal" }, - ); + for (const context of contexts) { + Vim.mapCommand(binding, "action", mapping.action, {}, { context }); + } } syncedVimBindings[mapping.id] = mapping.bindings; } @@ -423,6 +434,7 @@ function registerVimCommands(): void { // #290/#312: make j/k move by display line through soft-wrapped content. // Shared with the Quick Note window (QuickCaptureApp) via the same helper. registerDisplayLineMotion(); + registerHeadingMotion(); Vim.defineEx("write", "w", () => { void useStore.getState().persistActive(); diff --git a/packages/app-core/src/components/FloatingNoteApp.tsx b/packages/app-core/src/components/FloatingNoteApp.tsx index 57330834..23f793a9 100644 --- a/packages/app-core/src/components/FloatingNoteApp.tsx +++ b/packages/app-core/src/components/FloatingNoteApp.tsx @@ -34,6 +34,7 @@ import { resolveCodeLanguage } from '../lib/cm-code-languages' import { customCodeFenceHighlightExtension } from '../lib/cm-custom-code-languages' import { applyVimInsertEscape } from '../lib/vim-insert-escape' import { registerDisplayLineMotion } from '../lib/cm-vim-display-line' +import { registerHeadingMotion } from '../lib/cm-vim-heading-motion' import { markdownListIndentPlugin } from '../lib/cm-markdown-list-indent' import { appMarkdownSnippetExtension } from '../lib/markdown-snippets-config' import { syntaxHighlighting, HighlightStyle, defaultHighlightStyle } from '@codemirror/language' @@ -550,6 +551,7 @@ function registerFloatingVimCommands(): void { floatingVimRegistered = true registerDisplayLineMotion() + registerHeadingMotion() Vim.defineEx('write', 'w', () => { void floatingHandlers.persist?.() diff --git a/packages/app-core/src/components/QuickCaptureApp.tsx b/packages/app-core/src/components/QuickCaptureApp.tsx index 36df1a65..22110d54 100644 --- a/packages/app-core/src/components/QuickCaptureApp.tsx +++ b/packages/app-core/src/components/QuickCaptureApp.tsx @@ -44,6 +44,7 @@ import { history, historyKeymap, indentWithTab } from '@codemirror/commands' import { vimAwareDefaultKeymap, vimAwareMarkdownKeymap } from '../lib/cm-vim-default-keymap' import { vimVisualHighlightExtension } from '../lib/cm-vim-visual-highlight' import { registerDisplayLineMotion } from '../lib/cm-vim-display-line' +import { registerHeadingMotion } from '../lib/cm-vim-heading-motion' import { toggleWrap, wrapLink } from '../lib/cm-format' import { markdown, markdownLanguage } from '@codemirror/lang-markdown' import { resolveCodeLanguage } from '../lib/cm-code-languages' @@ -209,6 +210,7 @@ function registerCaptureVimCommands(): void { // #312: this window is a separate Electron renderer with its own Vim, so it // needs its own registration to get the main editor's j/k display-line motion. registerDisplayLineMotion() + registerHeadingMotion() Vim.defineEx('write', 'w', () => { setTimeout(() => { diff --git a/packages/app-core/src/components/VimNav.tsx b/packages/app-core/src/components/VimNav.tsx index a6f5a53a..d13ddec5 100644 --- a/packages/app-core/src/components/VimNav.tsx +++ b/packages/app-core/src/components/VimNav.tsx @@ -550,18 +550,16 @@ export function VimNav(): JSX.Element | null { } } - if ( - !leaderPending.current && - !( - isEditorFocused(state.editorViewRef) && - (isEditorInsertMode(state.editorViewRef, state.vimMode) || - // While Vim is mid-command awaiting an argument (after f/F/t/T/r, an - // operator, or a count), the next key is that command's literal - // target — e.g. `f[` finds `[`. Don't let the `[b`/`]b` buffer-nav - // or `gt`/`gT` prefixes swallow it; let it reach codemirror-vim. - isVimAwaitingArgument(state.editorViewRef)) - ) - ) { + // Buffer and tab sequences as a GLOBAL fallback: they exist for when + // focus sits anywhere but the editor (#321). A focused editor has + // codemirror-vim, which carries `[b`/`]b` and `gt`/`gT` of its own, so + // this layer must not touch its keys. Consuming the first key here meant + // no Vim sequence beginning with `[` or `]` could ever run: `]]` and + // `[[` were swallowed before Vim saw either press (#578). The same + // problem was already visible for a pending argument (`f[` finding a + // bracket) and patched narrowly then; standing down for the whole + // focused editor is the rule that covers both. + if (!leaderPending.current && !isEditorFocused(state.editorViewRef)) { const consumeBufferKey = (): void => { e.preventDefault() e.stopImmediatePropagation() diff --git a/packages/app-core/src/lib/cm-vim-heading-motion-keys.test.ts b/packages/app-core/src/lib/cm-vim-heading-motion-keys.test.ts new file mode 100644 index 00000000..8cc5003d --- /dev/null +++ b/packages/app-core/src/lib/cm-vim-heading-motion-keys.test.ts @@ -0,0 +1,100 @@ +// @vitest-environment jsdom +// +// The `]]` / `[[` bindings driven through a real codemirror-vim, rather than +// by calling the motion directly. `]` is a built-in Vim motion, so +// `]]` matches it too, and only pressing the keys for real proves which of the +// two wins (#578). +import { afterEach, describe, expect, it } from 'vitest' +import { EditorState } from '@codemirror/state' +import { EditorView } from '@codemirror/view' +import { vim } from '@replit/codemirror-vim' +import { registerHeadingMotion } from './cm-vim-heading-motion' + +const DOC = [ + '# Title', // 1 + '', // 2 + 'intro', // 3 + '## Section one', // 4 + 'body', // 5 + '```md', // 6 + '# not a heading', // 7 + '```', // 8 + '## Section two', // 9 + 'tail' // 10 +].join('\n') + +let view: EditorView | null = null + +afterEach(() => { + view?.destroy() + view = null +}) + +function mount(): EditorView { + registerHeadingMotion() + view = new EditorView({ + state: EditorState.create({ doc: DOC, extensions: [vim()] }), + parent: document.body + }) + return view +} + +function press(target: EditorView, ...keys: string[]): void { + for (const key of keys) { + target.contentDOM.dispatchEvent( + new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }) + ) + } +} + +function line(target: EditorView): number { + return target.state.doc.lineAt(target.state.selection.main.head).number +} + +describe(']] and [[ pressed for real (#578)', () => { + it('walks forward to each heading, and skips one inside a code fence', () => { + const v = mount() + press(v, ']', ']') + expect(line(v)).toBe(4) + press(v, ']', ']') + // Line 7 is `# not a heading` inside the fence; the next stop is line 9. + expect(line(v)).toBe(9) + }) + + it('walks back the same way', () => { + const v = mount() + press(v, ']', ']', ']', ']') + expect(line(v)).toBe(9) + press(v, '[', '[') + expect(line(v)).toBe(4) + press(v, '[', '[') + expect(line(v)).toBe(1) + }) + + it('takes a count', () => { + const v = mount() + press(v, '2', ']', ']') + expect(line(v)).toBe(9) + }) + + it('composes with an operator, so d]] deletes up to the next heading', () => { + const v = mount() + press(v, ']', ']') // on `## Section one` + press(v, 'd', ']', ']') + // That heading and its body are gone, fenced block included. The blank + // line left behind is Vim's own rule for an exclusive motion that ends in + // column one: the end backs up to the end of the previous line, so the + // newline closing the deleted section survives. Real Vim's `d]]` leaves + // the same gap. + expect(v.state.doc.toString()).toBe( + ['# Title', '', 'intro', '', '## Section two', 'tail'].join('\n') + ) + }) + + it('extends a visual selection', () => { + const v = mount() + press(v, 'v', ']', ']') + expect(v.state.selection.main.empty).toBe(false) + expect(line(v)).toBe(4) + }) +}) diff --git a/packages/app-core/src/lib/cm-vim-heading-motion.test.ts b/packages/app-core/src/lib/cm-vim-heading-motion.test.ts new file mode 100644 index 00000000..635cc6bd --- /dev/null +++ b/packages/app-core/src/lib/cm-vim-heading-motion.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from 'vitest' +import { EditorState } from '@codemirror/state' +import type { EditorView } from '@codemirror/view' +import { zenMoveToHeading } from './cm-vim-heading-motion' + +const DOC = [ + '---', // 1 + 'title: Front matter', // 2 a `#` in here is not a heading + 'tags: [a]', // 3 + '---', // 4 + 'intro paragraph', // 5 + '# One', // 6 + 'body', // 7 + '```python', // 8 + '# not a heading, this is a comment', // 9 + '```', // 10 + '## Two', // 11 + 'body', // 12 + 'Setext heading', // 13 + '==============', // 14 + 'tail' // 15 +].join('\n') + +function cm(doc = DOC) { + const state = EditorState.create({ doc }) + return { + firstLine: () => 0, + lastLine: () => state.doc.lines - 1, + cm6: { state } as unknown as EditorView + } +} + +// #578: `]]` / `[[` move between markdown headings, the way Vim's section +// motions move between sections and the way Zed maps the same keys. +describe('heading motion (#578)', () => { + it(']] walks forward through the headings', () => { + const view = cm() + // From the intro (line 5, 0-based 4) to `# One` on line 6. + expect(zenMoveToHeading(view, { line: 4, ch: 3 }, { forward: true })).toEqual({ + line: 5, + ch: 0 + }) + // From `# One` to `## Two`, stepping over the fenced block between them. + expect(zenMoveToHeading(view, { line: 5, ch: 0 }, { forward: true })).toEqual({ + line: 10, + ch: 0 + }) + }) + + it('[[ walks back the same way', () => { + const view = cm() + expect(zenMoveToHeading(view, { line: 11, ch: 2 }, { forward: false })).toEqual({ + line: 10, + ch: 0 + }) + expect(zenMoveToHeading(view, { line: 10, ch: 0 }, { forward: false })).toEqual({ + line: 5, + ch: 0 + }) + }) + + it('never stops on a `#` line inside a code fence or in frontmatter', () => { + const view = cm() + // Line 9 is `# not a heading…` inside the fence: jumping forward from the + // intro skips it, and nothing lands before `# One` going backward. + expect(zenMoveToHeading(view, { line: 6, ch: 0 }, { forward: true })).toEqual({ + line: 10, + ch: 0 + }) + expect(zenMoveToHeading(view, { line: 5, ch: 0 }, { forward: false })).toEqual({ + line: 0, + ch: 0 + }) + }) + + it('finds a setext heading by its text line, not its underline', () => { + const view = cm() + expect(zenMoveToHeading(view, { line: 10, ch: 0 }, { forward: true })).toEqual({ + line: 12, + ch: 0 + }) + }) + + it('takes a count, and stops at the furthest heading rather than overshooting', () => { + const view = cm() + expect(zenMoveToHeading(view, { line: 4, ch: 0 }, { forward: true, repeat: 2 })).toEqual({ + line: 10, + ch: 0 + }) + expect(zenMoveToHeading(view, { line: 4, ch: 0 }, { forward: true, repeat: 99 })).toEqual({ + line: 12, + ch: 0 + }) + }) + + it('runs to the end or start of the note when no heading is left that way', () => { + const view = cm() + // Past the last heading, like Vim's section motions. + expect(zenMoveToHeading(view, { line: 13, ch: 0 }, { forward: true })).toEqual({ + line: 14, + ch: 0 + }) + expect(zenMoveToHeading(view, { line: 4, ch: 0 }, { forward: false })).toEqual({ + line: 0, + ch: 0 + }) + }) + + it('leaves the cursor alone when there is no view to measure', () => { + const detached = { firstLine: () => 0, lastLine: () => 5 } + expect(zenMoveToHeading(detached, { line: 2, ch: 4 }, { forward: true })).toEqual({ + line: 2, + ch: 4 + }) + }) +}) diff --git a/packages/app-core/src/lib/cm-vim-heading-motion.ts b/packages/app-core/src/lib/cm-vim-heading-motion.ts new file mode 100644 index 00000000..08f564bf --- /dev/null +++ b/packages/app-core/src/lib/cm-vim-heading-motion.ts @@ -0,0 +1,99 @@ +import { CodeMirror, Vim } from '@replit/codemirror-vim' +import type { EditorView } from '@codemirror/view' +import { parseOutline, type OutlineItem } from './outline' + +// Minimal shape of the CodeMirror-Vim adapter this motion touches. +type VimHeadingCm = { + firstLine: () => number + lastLine: () => number + /** The underlying CodeMirror 6 view (set by the codemirror-vim adapter). */ + cm6?: EditorView +} + +/** + * Headings for a document, keyed by the doc itself. CodeMirror's `Text` is + * immutable, so an edit produces a new key and the entry for the old one is + * collected: repeated presses on an unchanged note reuse one scan, and the + * cache can never go stale. + */ +const headingCache = new WeakMap() + +function headingsOf(view: EditorView): OutlineItem[] { + const doc = view.state.doc + const cached = headingCache.get(doc) + if (cached) return cached + // The same parser the outline panel and `Space p` use, so a heading the + // outline lists is exactly a heading this motion stops on: fences are + // tracked by their own marker run and frontmatter is skipped, which keeps + // a `# comment` inside a code block from being a destination (#249). + const items = parseOutline(doc.toString()) + headingCache.set(doc, items) + return items +} + +/** + * `]]` / `[[`: jump to the next or previous markdown heading (#578). + * + * Vim's own `]]` and `[[` move between sections, which in a C file means a + * brace in column one and in a note means a heading; Zed maps them the same + * way, which is where the request came from. Being a motion rather than a + * command means it composes: `d]]` deletes to the next heading, `v]]` selects + * to it, `3]]` skips three, and `Ctrl+O` comes back, all for free. + * + * With no heading left in that direction the cursor goes to the end or start + * of the note, like Vim's section motions do, so the key always moves rather + * than silently doing nothing. + */ +export function zenMoveToHeading( + cm: VimHeadingCm, + head: { line: number; ch: number }, + motionArgs: { forward?: boolean; repeat?: number } +): { line: number; ch: number } { + const view = cm.cm6 + const forward = !!motionArgs.forward + const repeat = Math.max(1, motionArgs.repeat || 1) + if (!view) return new CodeMirror.Pos(head.line, head.ch) + + // codemirror-vim counts lines from 0; the outline counts from 1. + const current = head.line + 1 + const headings = headingsOf(view) + const ahead = forward + ? headings.filter((item) => item.line > current) + : headings.filter((item) => item.line < current).reverse() + + const target = ahead[Math.min(repeat, ahead.length) - 1] + if (target) return new CodeMirror.Pos(target.line - 1, 0) + return new CodeMirror.Pos(forward ? cm.lastLine() : cm.firstLine(), 0) +} + +let headingMotionRegistered = false + +/** + * Register `]]` / `[[` on the (per-window) global Vim. Like the display-line + * motions, every renderer with an editor has its own Vim singleton, so each + * one calls this. Idempotent, so it is safe on HMR. + */ +export function registerHeadingMotion(): void { + if (headingMotionRegistered) return + headingMotionRegistered = true + Vim.defineMotion( + 'zenMoveToHeading', + zenMoveToHeading as unknown as Parameters[1] + ) + for (const context of ['normal', 'visual', 'operatorPending'] as const) { + Vim.mapCommand( + ']]', + 'motion', + 'zenMoveToHeading', + { forward: true, toJumplist: true }, + { context } + ) + Vim.mapCommand( + '[[', + 'motion', + 'zenMoveToHeading', + { forward: false, toJumplist: true }, + { context } + ) + } +} diff --git a/packages/app-core/src/lib/help.ts b/packages/app-core/src/lib/help.ts index 99a3d787..03c3efc0 100644 --- a/packages/app-core/src/lib/help.ts +++ b/packages/app-core/src/lib/help.ts @@ -506,6 +506,7 @@ export const HELP_SHORTCUT_SECTIONS: HelpShortcutSection[] = [ { keys: 'Space f', action: 'Search notes', detail: 'Open the vault-wide note search palette.' }, { keys: 'Space s t', action: 'Search vault text', detail: 'Fuzzy-search matching text lines across notes in Inbox, Quick Notes, and Archive.' }, { keys: 'Space e', action: 'Toggle left sidebar', detail: 'Show or hide the folder/tag sidebar without touching the mouse.' }, + { keys: ']] / [[', action: 'Next / previous heading', detail: 'Jump the cursor to the next or previous markdown heading in the note, the way Vim’s section motions move between sections. It is a motion, so it composes: `d]]` deletes to the next heading, `v]]` selects to it, `3]]` skips three, and `Ctrl+O` jumps back. Headings inside code fences and frontmatter are skipped, matching the outline. With no heading left that way, the cursor goes to the end or start of the note.' }, { keys: 'Space p', action: 'Note outline', detail: 'Jump to any heading in the active note via a searchable overlay.' }, { keys: 'Space v', action: 'Switch vault', detail: 'Open the command palette directly to the local vault switcher.' }, { keys: 'Space a', action: 'Open workflows', detail: 'Open the Workflows view, where saved pipelines over your notes are built and run. Workflows are off by default; turn them on under Settings → Workflows first.' }, From 10e13453d9bd9c4c02909610c78852bfad957b6f Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 11:31:29 -0500 Subject: [PATCH 11/18] chore(release): 2.28.2 --- apps/desktop/package.json | 2 +- apps/server/package.json | 2 +- apps/web/package.json | 2 +- package-lock.json | 18 +++++++++--------- package.json | 2 +- packages/app-core/package.json | 2 +- packages/bridge-contract/package.json | 2 +- packages/shared-domain/package.json | 2 +- packages/shared-ui/package.json | 2 +- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index e7176f89..f7ebfeb4 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/desktop", "productName": "ZenNotes", - "version": "2.28.1", + "version": "2.28.2", "description": "ZenNotes desktop shell", "private": true, "main": "./out/main/index.js", diff --git a/apps/server/package.json b/apps/server/package.json index 9bf4da71..2806e085 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/server", "private": true, - "version": "2.28.1", + "version": "2.28.2", "scripts": { "dev": "node ../../tooling/scripts/run-go-server-dev.mjs", "prepare-web": "node ../../tooling/scripts/prepare-server-web-dist.mjs", diff --git a/apps/web/package.json b/apps/web/package.json index 0b6c73c2..d64fc061 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/web", "private": true, - "version": "2.28.1", + "version": "2.28.2", "type": "module", "description": "ZenNotes web client for self-hosted and hosted deployments", "homepage": "https://zennotes.org", diff --git a/package-lock.json b/package-lock.json index 66213394..350f5d3d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "zennotes-monorepo", - "version": "2.28.1", + "version": "2.28.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "zennotes-monorepo", - "version": "2.28.1", + "version": "2.28.2", "workspaces": [ "apps/*", "packages/*" @@ -20,7 +20,7 @@ }, "apps/desktop": { "name": "@zennotes/desktop", - "version": "2.28.1", + "version": "2.28.2", "license": "MIT", "dependencies": { "@codemirror/autocomplete": "^6.18.3", @@ -99,11 +99,11 @@ }, "apps/server": { "name": "@zennotes/server", - "version": "2.28.1" + "version": "2.28.2" }, "apps/web": { "name": "@zennotes/web", - "version": "2.28.1", + "version": "2.28.2", "dependencies": { "@codemirror/autocomplete": "^6.18.3", "@codemirror/commands": "^6.7.1", @@ -17123,7 +17123,7 @@ }, "packages/app-core": { "name": "@zennotes/app-core", - "version": "2.28.1", + "version": "2.28.2", "dependencies": { "@codemirror/autocomplete": "^6.18.3", "@codemirror/commands": "^6.7.1", @@ -17187,11 +17187,11 @@ }, "packages/bridge-contract": { "name": "@zennotes/bridge-contract", - "version": "2.28.1" + "version": "2.28.2" }, "packages/shared-domain": { "name": "@zennotes/shared-domain", - "version": "2.28.1", + "version": "2.28.2", "dependencies": { "@zennotes/bridge-contract": "*", "lz-string": "^1.5.0" @@ -17199,7 +17199,7 @@ }, "packages/shared-ui": { "name": "@zennotes/shared-ui", - "version": "2.28.1" + "version": "2.28.2" } } } diff --git a/package.json b/package.json index 0b468999..f9086e92 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "zennotes-monorepo", "private": true, - "version": "2.28.1", + "version": "2.28.2", "description": "ZenNotes monorepo for desktop, web, and self-hosted server builds", "packageManager": "npm@10.9.2", "engines": { diff --git a/packages/app-core/package.json b/packages/app-core/package.json index bb769deb..e19a5984 100644 --- a/packages/app-core/package.json +++ b/packages/app-core/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/app-core", "private": true, - "version": "2.28.1", + "version": "2.28.2", "type": "module", "exports": { "./main": "./src/main.tsx" diff --git a/packages/bridge-contract/package.json b/packages/bridge-contract/package.json index a4687ce1..882f420a 100644 --- a/packages/bridge-contract/package.json +++ b/packages/bridge-contract/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/bridge-contract", "private": true, - "version": "2.28.1", + "version": "2.28.2", "type": "module", "exports": { "./bridge": "./src/bridge.ts", diff --git a/packages/shared-domain/package.json b/packages/shared-domain/package.json index 7e074c28..a6b8dace 100644 --- a/packages/shared-domain/package.json +++ b/packages/shared-domain/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/shared-domain", "private": true, - "version": "2.28.1", + "version": "2.28.2", "type": "module", "exports": { "./*": "./src/*.ts" diff --git a/packages/shared-ui/package.json b/packages/shared-ui/package.json index dcfb50d3..f16dfbd2 100644 --- a/packages/shared-ui/package.json +++ b/packages/shared-ui/package.json @@ -1,7 +1,7 @@ { "name": "@zennotes/shared-ui", "private": true, - "version": "2.28.1", + "version": "2.28.2", "type": "module", "exports": { ".": "./src/index.ts" From 5eb6d5c5c764c5f95bb369086d88757f893ccd35 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 11:59:19 -0500 Subject: [PATCH 12/18] Fix(editor): LaTeX completion asks which typesetter, and reads dollars in context Review follow-up to #594, kept out of the contributor's commit. A note set to the Typst typesetter takes different syntax entirely, so offering `\frac{}{}` in one was wrong every time. The completion source now reads the renderer the pane is configured with and stays silent unless it is KaTeX. The facet already carried the answer; it just had no accessor. Dollars inside code were being counted as delimiters. A note with `echo $$` in a shell block flipped the parity for everything below it, so `\` popped LaTeX commands in plain prose from there on, and a `$5` in inline code did the same for the rest of its line. Both counts now skip anything the syntax tree says is code, which is the same question the cursor position was already being asked. The block scan also ran from the start of the document on every `\` typed, allocating the whole prefix as a string. It now looks back a bounded window: a display block opened further up than that is not a formula anyone is still typing. The rendered previews are cached between popups. A bare `\` opens the entire table at once, and typesetting every visible row through KaTeX on each open was the one cost this feature could be felt through. --- .../src/lib/cm-latex-completions.test.ts | 47 +++++++++++++- .../app-core/src/lib/cm-latex-completions.ts | 63 +++++++++++++++---- packages/app-core/src/lib/cm-math-render.ts | 6 ++ 3 files changed, 104 insertions(+), 12 deletions(-) diff --git a/packages/app-core/src/lib/cm-latex-completions.test.ts b/packages/app-core/src/lib/cm-latex-completions.test.ts index b3287bc5..c01a0dec 100644 --- a/packages/app-core/src/lib/cm-latex-completions.test.ts +++ b/packages/app-core/src/lib/cm-latex-completions.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from 'vitest' import { EditorState } from '@codemirror/state' import { markdown } from '@codemirror/lang-markdown' -import { isInMathContext, latexTokenBefore } from './cm-latex-completions' +import { CompletionContext } from '@codemirror/autocomplete' +import { isInMathContext, latexCommandSource, latexTokenBefore } from './cm-latex-completions' +import { mathRenderExtension } from './cm-math-render' function state(doc: string): EditorState { return EditorState.create({ doc, extensions: [markdown()] }) @@ -77,3 +79,46 @@ describe('latexTokenBefore', () => { expect(latexTokenBefore(state(plain), plain.length)).toBeNull() }) }) + +// Review follow-ups to #594. +describe('math context, delimiters that are not delimiters', () => { + it('ignores dollars inside a code block, which would otherwise flip parity', () => { + // `$$` is the shell PID, and a note that mentions it used to leave every + // later line reading as display math. + const doc = 'intro\n\n```bash\necho $$\n```\n\nplain prose here\n' + expect(isInMathContext(state(doc), after(doc, 'plain prose'))).toBe(false) + }) + + it('ignores a dollar inside inline code on the same line', () => { + const doc = 'costs `$5` and then more prose' + expect(isInMathContext(state(doc), after(doc, 'more prose'))).toBe(false) + }) + + it('still sees real math after a code block that mentions dollars', () => { + const doc = '```bash\necho $$\n```\n\n$x + ' + expect(isInMathContext(state(doc), doc.length)).toBe(true) + }) +}) + +describe('the typesetter the note is set to', () => { + const DOC = 'text $\\su' + + function sourceFor(renderer: 'katex' | 'typst') { + const editorState = EditorState.create({ + doc: DOC, + extensions: [markdown(), mathRenderExtension(renderer)] + }) + return latexCommandSource(new CompletionContext(editorState, DOC.length, false)) + } + + it('offers LaTeX commands with KaTeX selected', () => { + const result = sourceFor('katex') + expect(result?.options.length ?? 0).toBeGreaterThan(0) + }) + + it('stays out of the way when the note compiles as Typst', () => { + // Typst is a different language: `\\frac{}{}` is not what it takes, so + // suggesting it would be wrong every time. + expect(sourceFor('typst')).toBeNull() + }) +}) diff --git a/packages/app-core/src/lib/cm-latex-completions.ts b/packages/app-core/src/lib/cm-latex-completions.ts index 86f355b9..ba9a00a4 100644 --- a/packages/app-core/src/lib/cm-latex-completions.ts +++ b/packages/app-core/src/lib/cm-latex-completions.ts @@ -13,6 +13,7 @@ import { snippet } from '@codemirror/autocomplete' import { syntaxTree } from '@codemirror/language' import type { EditorState } from '@codemirror/state' import katex from 'katex' +import { mathRendererOf } from './cm-math-render' interface LatexCommand { /** Command as typed, with the backslash: `\sum`. */ @@ -183,19 +184,42 @@ function codeContext(state: EditorState, pos: number): CodeContext { /** Inside `$…$`, `$$…$$`, or a ```math fence at `pos`? Counts unmatched * dollar delimiters so a formula still being typed (no closing `$` yet) * already counts as math. */ +/** How far back an unclosed `$$` is looked for. A display block open further + * above than this is not a formula anyone is still typing, and the bound keeps + * the scan off the whole document on every `\` in a long note. */ +const BLOCK_SCAN_WINDOW = 20_000 + +/** Dollars inside code are not delimiters: `echo $$` in a shell block would + * otherwise flip the parity and make the rest of the note read as math. */ +function countDelimiters(state: EditorState, from: number, text: string, re: RegExp): number { + let count = 0 + for (const match of text.matchAll(re)) { + if (match.index === undefined) continue + if (codeContext(state, from + match.index + 1)) continue + count++ + } + return count +} + export function isInMathContext(state: EditorState, pos: number): boolean { const code = codeContext(state, pos) // A ```math fence is a math region in its own right (remark-math renders // it as display math); every other code region shuts completion off. if (code) return code.kind === 'fenced' && code.lang === 'math' - const before = state.doc.sliceString(0, pos) - const blockFences = before.match(/(?() + +function renderPreview(latex: string): string { + const cached = previewCache.get(latex) + if (cached !== undefined) return cached + let html = '' + try { + html = katex.renderToString(latex, { throwOnError: false }) + } catch { + html = '' + } + previewCache.set(latex, html) + return html +} + /** Full option row for a LaTeX completion — the KaTeX-rendered symbol sits in * the icon slot, then label and detail reuse the slash-command layout. Called * first from the shared `renderCompletion`; null for every other kind. */ @@ -258,11 +303,7 @@ export function renderLatexCompletion(completion: Completion): HTMLElement | nul icon.style.display = 'inline-flex' icon.style.alignItems = 'center' icon.style.justifyContent = 'center' - try { - icon.innerHTML = katex.renderToString(_preview ?? completion.label, { throwOnError: false }) - } catch { - icon.textContent = '' - } + icon.innerHTML = renderPreview(_preview ?? completion.label) const label = document.createElement('span') label.className = 'slash-cmd-label' diff --git a/packages/app-core/src/lib/cm-math-render.ts b/packages/app-core/src/lib/cm-math-render.ts index 4a86f69c..27cabc91 100644 --- a/packages/app-core/src/lib/cm-math-render.ts +++ b/packages/app-core/src/lib/cm-math-render.ts @@ -30,6 +30,12 @@ const mathRendererFacet = Facet.define({ combine: (values) => (values.length ? values[values.length - 1] : 'katex') }) +/** The typesetter this editor is configured for. Anything offering LaTeX help + * has to ask: a note written for Typst takes different syntax entirely. */ +export function mathRendererOf(state: EditorState): MathRenderer { + return state.facet(mathRendererFacet) +} + /** Tag-driven Typst definitions for the note in this editor, prepended to every * formula it compiles. Rides a facet like the renderer, so changing a note's * tags reconfigures the pane and re-renders its math. Empty for KaTeX and for From a342da2f9c2e3b65c2923d402a1cf527ca7cc3c7 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 12:13:50 -0500 Subject: [PATCH 13/18] Fix(editor): a frontmatter Tags: field is the tags field in the editor too Review follow-up to #595, kept out of the contributor's commit. `parseFrontmatterFields` lowercases keys, so `Tags:` and `TAGS:` are the tags field everywhere the vault is indexed: those notes show up under their tags in the Tags view, in search, in the CLI. The editor read the key case-sensitively for an inline `tags: a, b` line, so a note written with a capital T had tags the rest of the app knew about and the editor would neither chip nor complete. Its own block-list branch already lowercased, which is how the two halves of one feature came to disagree. Both places now ask one question, `frontmatterTagsValue`, phrased the way the shared parser phrases it. The deeper risk is that two parsers now decide what a frontmatter tag is: the shared one, which is what gets indexed, and the editor's own scan, which exists because the shared one returns tags without positions and decorations need offsets. They cannot be collapsed, so a test pins them together instead: a matrix of frontmatter shapes (inline list, comma scalar, space scalar, block list, quoted, `#`-prefixed, capitalized key, and two fields that only look like tags) asserts the chips a note renders are exactly the tags `frontmatterTags` returns for it. Whichever side moves next, that test fails. --- .../src/lib/cm-frontmatter-tag-complete.ts | 9 +++-- .../src/lib/cm-frontmatter-tag.test.ts | 34 +++++++++++++++++++ packages/app-core/src/lib/cm-frontmatter.ts | 20 ++++++++--- 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts b/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts index 3a55c18b..3807e82e 100644 --- a/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts +++ b/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts @@ -6,7 +6,7 @@ import type { Completion, CompletionContext, CompletionResult } from '@codemirror/autocomplete' import type { EditorState } from '@codemirror/state' import { collectTagCounts, rankTagCompletions } from './cm-hashtag-complete' -import { isInsideFrontmatter } from './cm-frontmatter' +import { frontmatterTagsValue, isInsideFrontmatter } from './cm-frontmatter' /** Characters that terminate a tag token when scanning forward or backward * for the *body* of the token. A leading `#` is intentionally not a start @@ -64,11 +64,10 @@ function frontmatterTagMatch(context: CompletionContext): { from: number; query: const text = line.text const col = pos - line.from - const inline = text.match(/^(\s*)tags\s*:\s*(.*)$/) + const inline = frontmatterTagsValue(text) if (inline) { - const valueStart = inline[0].length - (inline[2] as string).length - if (col < valueStart) return null - return tagTokenAt(state, line.from, valueStart, pos) + if (col < inline.offset) return null + return tagTokenAt(state, line.from, inline.offset, pos) } const item = text.match(/^(\s*)-\s+(.*)$/) diff --git a/packages/app-core/src/lib/cm-frontmatter-tag.test.ts b/packages/app-core/src/lib/cm-frontmatter-tag.test.ts index b5913812..3110b257 100644 --- a/packages/app-core/src/lib/cm-frontmatter-tag.test.ts +++ b/packages/app-core/src/lib/cm-frontmatter-tag.test.ts @@ -3,6 +3,7 @@ import { EditorState } from '@codemirror/state' import { EditorView } from '@codemirror/view' import { afterEach, describe, expect, it, vi } from 'vitest' +import { frontmatterTags } from '@shared/frontmatter' import { frontmatterTagExtension } from './cm-frontmatter' const openTagView = vi.fn() @@ -76,3 +77,36 @@ describe('frontmatterTagExtension', () => { expect(openTagView).toHaveBeenCalledWith('idea') }) }) + +// Review follow-up to #595. Two places now decide what a frontmatter tag is: +// `frontmatterTags` in shared-domain, which is what the vault indexes and what +// the Tags view lists, and the editor's own scan, which needs positions the +// shared parser does not return. They must not drift: a chip the index has +// never heard of goes nowhere, and a tag with no chip looks broken next to its +// neighbours. +describe('chips agree with the tags the vault indexes', () => { + const cases = [ + '---\ntags: [draft, research]\n---\n', + '---\ntags: draft research\n---\n', + '---\ntags: draft, research\n---\n', + '---\ntags:\n - draft\n - research\n---\n', + '---\ntags: "#draft"\n---\n', + // parseFrontmatterFields lowercases keys, so a capital T is still the + // tags field: the index counts it and the editor has to as well. + '---\nTags: draft\n---\n', + '---\nTAGS:\n - draft\n---\n', + // Fields that merely look adjacent must stay plain text. + '---\nkeywords: draft\n---\n', + '---\ntitle: tags: not a list\n---\n' + ] + + for (const doc of cases) { + it(`matches frontmatterTags for ${JSON.stringify(doc.split('\n')[1])}`, () => { + const view = mount(doc) + const chips = Array.from(view.dom.querySelectorAll('.cm-frontmatter-tag')).map( + (el) => el.textContent + ) + expect(chips).toEqual(frontmatterTags(doc)) + }) + } +}) diff --git a/packages/app-core/src/lib/cm-frontmatter.ts b/packages/app-core/src/lib/cm-frontmatter.ts index fc3e628c..3fbbdb5d 100644 --- a/packages/app-core/src/lib/cm-frontmatter.ts +++ b/packages/app-core/src/lib/cm-frontmatter.ts @@ -81,6 +81,20 @@ export const frontmatterStyle = ViewPlugin.fromClass( const TAG_TOKEN_RE = /[^,\s\[\]"'#]+/g +/** A frontmatter `key: value` line, split into its key and value. + * `parseFrontmatterFields` (shared-domain) lowercases keys, so `Tags:` is the + * tags field as far as the vault index is concerned; anything reading the + * same field in the editor has to agree, or a note written with a capital T + * gets tags the Tags view lists and the editor refuses to show. */ +const FRONTMATTER_KEY_RE = /^(\s*)([A-Za-z0-9_][\w-]*)\s*:\s*(.*)$/ + +export function frontmatterTagsValue(lineText: string): { value: string; offset: number } | null { + const match = lineText.match(FRONTMATTER_KEY_RE) + if (!match || match[2].toLowerCase() !== 'tags') return null + const value = match[3] ?? '' + return { value, offset: match[0].length - value.length } +} + /** Which frontmatter lines are `- item` entries under a bare `tags:` key. */ function tagsBlockLineNumbers(state: EditorState): Set { const range = frontmatterRange(state) @@ -138,11 +152,9 @@ function buildFrontmatterTagDeco(view: EditorView): DecorationSet { const text = line.text const trimmed = text.trim() if (!trimmed || trimmed.startsWith('#')) continue - const inline = text.match(/^(\s*)tags\s*:\s*(.*)$/) + const inline = frontmatterTagsValue(text) if (inline) { - const value = inline[2] as string - const valueStart = line.from + inline[0].length - value.length - addTagTokens(value, valueStart, builder) + addTagTokens(inline.value, line.from + inline.offset, builder) continue } if (blockLines.has(n)) { From 6b6daec896ed04ede8a457ab82001e563f3f0656 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 12:34:23 -0500 Subject: [PATCH 14/18] Fix(editor): newer note saves always finish last Two saves for one note could overlap, and the filesystem was free to finish the older body after the newer one. The newer completion had already marked the buffer clean, leaving memory ahead of disk with no retry scheduled. Queue writes per path while keeping different notes independent. Each queued turn snapshots the latest dirty buffer only after the prior write finishes, so disk order now matches edit order. --- .../app-core/src/store-note-integrity.test.ts | 37 ++++++++ packages/app-core/src/store.ts | 84 +++++++++++-------- 2 files changed, 86 insertions(+), 35 deletions(-) diff --git a/packages/app-core/src/store-note-integrity.test.ts b/packages/app-core/src/store-note-integrity.test.ts index 45ebe37b..c6b398fb 100644 --- a/packages/app-core/src/store-note-integrity.test.ts +++ b/packages/app-core/src/store-note-integrity.test.ts @@ -256,6 +256,43 @@ describe('#585 — dirty buffers survive watcher change events', () => { expect(vault.get(target)).toBe('FIRST AND SECOND') }) + it('never lets an older overlapping save finish after the newest body', async () => { + const { useStore } = await loadStore() + seedRootVault(useStore) + const paneId = useStore.getState().activePaneId + const target = 'index.md' + await useStore.getState().openNoteInPane(paneId, target) + await flush() + + let releaseFirst!: () => void + const firstGate = new Promise((resolve) => { + releaseFirst = resolve + }) + const zen = window.zen as unknown as { + writeNote: (path: string, body: string) => Promise> + } + zen.writeNote = async (path, body) => { + if (body === 'FIRST') await firstGate + vault.set(path, body) + return meta(path, body) + } + + useStore.getState().updateNoteBody(target, 'FIRST') + const firstSave = useStore.getState().persistNote(target) + useStore.getState().updateNoteBody(target, 'SECOND') + const secondSave = useStore.getState().persistNote(target) + + // Without per-note serialization, SECOND reaches disk now and the older + // blocked write replaces it as soon as this gate opens. + await flush() + releaseFirst() + await Promise.all([firstSave, secondSave]) + + expect(vault.get(target)).toBe('SECOND') + expect(useStore.getState().noteContents[target]?.body).toBe('SECOND') + expect(useStore.getState().noteDirty[target]).toBe(false) + }) + // Saves are atomic now (temp file renamed into place), and on Linux a rename // arrives as IN_MOVED_TO, which the server's watcher reports as 'add'. Any // other tool that writes by renaming (git, rsync, Syncthing, vim) looks the diff --git a/packages/app-core/src/store.ts b/packages/app-core/src/store.ts index 8155c548..58f25854 100644 --- a/packages/app-core/src/store.ts +++ b/packages/app-core/src/store.ts @@ -3450,6 +3450,10 @@ interface Store { /** Debounced per-path save timers. Module-scoped so they survive re-renders. */ const pathSaveTimers = new Map>() +/** Per-path write tails. Filesystems and remote workspaces do not promise that + * two concurrent writes finish in call order, so a newer body must not race an + * older one to the final rename. */ +const pathSaveQueues = new Map>() const PATH_SAVE_DEBOUNCE_MS = 350 /** @@ -6318,47 +6322,57 @@ export const useStore = create((set, get) => { }, persistNote: async (path) => { - const s = get() - const content = s.noteContents[path] - if (!content || !s.noteDirty[path]) return const pending = pathSaveTimers.get(path) if (pending) { clearTimeout(pending) pathSaveTimers.delete(path) } - try { - // Snapshot the body BEFORE the await so we know what hit disk - // even if the user keeps typing while the write resolves. - const writtenBody = content.body - lastWrittenByPath.set(path, writtenBody) - const meta = await window.zen.writeNote(path, writtenBody) - // Saving a Typst preamble note changes the definitions every note tagged - // for it compiles against — reload so open panes repaint. (#486) - if ( - get().typstTagPreambles && - isTypstPreamblePath( - path, - resolveTypstPreambleFolder(get().vaultSettings?.typstPreambles?.folder) - ) - ) { - void get().refreshTypstPreambles() - } - set((cur) => { - // Keystrokes that landed while the write was in flight leave the - // buffer ahead of disk. Clearing the flag then made the already - // scheduled follow-up save bail on its dirty check and stranded - // those edits unsaved (#585) — the flag only clears when the buffer - // still holds exactly what hit disk. - const stillCurrent = cur.noteContents[path]?.body === writtenBody - const dirty = stillCurrent ? { ...cur.noteDirty, [path]: false } : cur.noteDirty - return { - noteDirty: dirty, - notes: cur.notes.map((n) => (n.path === meta.path ? { ...n, ...meta } : n)), - ...activeFieldsFrom(cur.paneLayout, cur.activePaneId, cur.noteContents, dirty) + const performWrite = async (): Promise => { + const s = get() + const content = s.noteContents[path] + if (!content || !s.noteDirty[path]) return + try { + // Snapshot only after earlier writes finish. A second caller sees the + // newest buffer here, then becomes the last writer by construction. + const writtenBody = content.body + lastWrittenByPath.set(path, writtenBody) + const meta = await window.zen.writeNote(path, writtenBody) + // Saving a Typst preamble note changes the definitions every note tagged + // for it compiles against, so reload and repaint open panes. (#486) + if ( + get().typstTagPreambles && + isTypstPreamblePath( + path, + resolveTypstPreambleFolder(get().vaultSettings?.typstPreambles?.folder) + ) + ) { + void get().refreshTypstPreambles() } - }) - } catch (err) { - console.error('writeNote failed', err) + set((cur) => { + // Keystrokes that landed while the write was in flight leave the + // buffer ahead of disk. The queued caller will persist them next. + const stillCurrent = cur.noteContents[path]?.body === writtenBody + const dirty = stillCurrent ? { ...cur.noteDirty, [path]: false } : cur.noteDirty + return { + noteDirty: dirty, + notes: cur.notes.map((n) => (n.path === meta.path ? { ...n, ...meta } : n)), + ...activeFieldsFrom(cur.paneLayout, cur.activePaneId, cur.noteContents, dirty) + } + }) + } catch (err) { + console.error('writeNote failed', err) + } + } + const previous = pathSaveQueues.get(path) + // Start the first write synchronously through its first await, preserving + // the body visible to this call. Later callers wait for that promise and + // snapshot the newest buffer only when their turn begins. + const run = previous ? previous.catch(() => {}).then(performWrite) : performWrite() + pathSaveQueues.set(path, run) + try { + await run + } finally { + if (pathSaveQueues.get(path) === run) pathSaveQueues.delete(path) } }, From ca83e98d4782f513e24dcb21cb22367de2cfbf4a Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 12:36:38 -0500 Subject: [PATCH 15/18] Fix(cloud): settings wait for the user's answer A first sync classified differing vault settings as an ordinary bootstrap conflict, so it never parked the cloud copy and repeated forever. Established syncs had the opposite problem: they parked the copy, then immediately uploaded local settings before showing the question. Treat settings specially during bootstrap and expose durable pending paths from each filesystem repository. Mutation planning now leaves those paths untouched while continuing to sync every unrelated file. --- .../src/main/cloud-sync-filesystem.test.ts | 1 + .../desktop/src/main/cloud-sync-filesystem.ts | 7 ++ .../src/cloud-sync-coordinator.test.ts | 117 ++++++++++++++++++ .../src/cloud-sync-coordinator.ts | 33 ++++- .../cloud-sync-portable-filesystem.test.ts | 29 +++++ .../src/cloud-sync-portable-filesystem.ts | 7 ++ 6 files changed, 192 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/main/cloud-sync-filesystem.test.ts b/apps/desktop/src/main/cloud-sync-filesystem.test.ts index 201ec51f..7dc8486b 100644 --- a/apps/desktop/src/main/cloud-sync-filesystem.test.ts +++ b/apps/desktop/src/main/cloud-sync-filesystem.test.ts @@ -235,6 +235,7 @@ describe('DesktopCloudSyncRepository', () => { path: '.zennotes/vault.json', conflict_copy_path: '.zennotes/vault.cloud-conflict.json' }) + expect(await repository.pendingConflictPaths()).toEqual(['.zennotes/vault.json']) // The settings in use are still this device's. expect(await readFile(path.join(root, '.zennotes', 'vault.json'), 'utf8')).toBe( '{"favorites":["a"]}' diff --git a/apps/desktop/src/main/cloud-sync-filesystem.ts b/apps/desktop/src/main/cloud-sync-filesystem.ts index f8cd7618..16d5a101 100644 --- a/apps/desktop/src/main/cloud-sync-filesystem.ts +++ b/apps/desktop/src/main/cloud-sync-filesystem.ts @@ -8,6 +8,7 @@ import type { } from '@zennotes/bridge-contract/cloud-sync' import { CLOUD_SYNC_SETTINGS_CONFLICT_PATH, + CLOUD_SYNC_VAULT_SETTINGS_PATH, cloudSyncConflictCopyPath, isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, @@ -87,6 +88,12 @@ export class DesktopCloudSyncRepository implements CloudSyncRepository { return items.sort((left, right) => left.path.localeCompare(right.path)) } + async pendingConflictPaths(): Promise { + return (await exists(this.resolve(CLOUD_SYNC_SETTINGS_CONFLICT_PATH))) + ? [CLOUD_SYNC_VAULT_SETTINGS_PATH] + : [] + } + async apply( change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined diff --git a/packages/shared-domain/src/cloud-sync-coordinator.test.ts b/packages/shared-domain/src/cloud-sync-coordinator.test.ts index 52348a9a..fbfa22b8 100644 --- a/packages/shared-domain/src/cloud-sync-coordinator.test.ts +++ b/packages/shared-domain/src/cloud-sync-coordinator.test.ts @@ -217,6 +217,123 @@ describe('CloudSyncCoordinator', () => { expect(states.current?.cursor).toBe(4) }) + it('parks differing settings on first sync while continuing with other files', async () => { + const localSettings = { + path: '.zennotes/vault.json', + kind: 'text' as const, + content: content('{"favorites":["local.md"]}') + } + const localNote = { path: 'local.md', kind: 'text' as const, content: content('local') } + const repository: CloudSyncRepository & { + pendingConflictPaths(): Promise + } = { + async scan() { + return [localSettings, localNote] + }, + async apply(change) { + if (change.path !== '.zennotes/vault.json') return + return { + code: 'SETTINGS_CONFLICT', + path: change.path, + conflict_copy_path: '.zennotes/vault.cloud-conflict.json' + } + }, + async pendingConflictPaths() { + return ['.zennotes/vault.json'] + } + } + const states = memoryState() + const server = remote({ + manifest: { + data: [ + { + item_id: 'settings-remote', + path: '.zennotes/vault.json', + kind: 'text', + revision: 2, + sha256: 'hash:{"favorites":["cloud.md"]}', + byte_length: 26, + media_type: 'application/json', + content: content('{"favorites":["cloud.md"]}') + } + ], + cursor: 4, + next_page: null + } + }) + + const result = await new CloudSyncCoordinator( + 'vault-1', + server, + repository, + states, + ids() + ).sync() + + expect(result.bootstrapConflicts).toEqual([]) + expect(result.localConflicts).toEqual([ + expect.objectContaining({ code: 'SETTINGS_CONFLICT', path: '.zennotes/vault.json' }) + ]) + expect(server.mutations).toHaveLength(1) + expect(server.mutations[0]?.mutations).toEqual([ + expect.objectContaining({ type: 'upsert', path: 'local.md' }) + ]) + expect(states.current?.items['settings-remote']?.sha256).toBe( + 'hash:{"favorites":["cloud.md"]}' + ) + }) + + it('does not upload local settings while their cloud choice is still pending', async () => { + const repository: CloudSyncRepository & { + pendingConflictPaths(): Promise + } = { + async scan() { + return [ + { + path: '.zennotes/vault.json', + kind: 'text', + content: content('{"favorites":["local.md"]}') + } + ] + }, + async apply() {}, + async pendingConflictPaths() { + return ['.zennotes/vault.json'] + } + } + const states = memoryState({ + version: 1, + vault_id: 'vault-1', + cursor: 9, + items: { + 'settings-remote': { + item_id: 'settings-remote', + path: '.zennotes/vault.json', + kind: 'text', + revision: 3, + sha256: 'hash:{"favorites":["cloud.md"]}', + byte_length: 26, + media_type: 'application/json' + } + } + }) + const server = remote({}) + + const result = await new CloudSyncCoordinator( + 'vault-1', + server, + repository, + states, + ids() + ).sync() + + expect(result.pushed).toBe(0) + expect(server.mutations).toEqual([]) + expect(states.current?.items['settings-remote']?.sha256).toBe( + 'hash:{"favorites":["cloud.md"]}' + ) + }) + it('pulls contiguous remote changes before planning local mutations', async () => { const states = memoryState({ version: 1, diff --git a/packages/shared-domain/src/cloud-sync-coordinator.ts b/packages/shared-domain/src/cloud-sync-coordinator.ts index 7ef1168a..92043583 100644 --- a/packages/shared-domain/src/cloud-sync-coordinator.ts +++ b/packages/shared-domain/src/cloud-sync-coordinator.ts @@ -8,7 +8,7 @@ import type { CloudSyncMutationRequest, CloudSyncMutationResponse } from '@zennotes/bridge-contract/cloud-sync' -import { cloudSyncPathKey } from './cloud-sync' +import { cloudSyncPathKey, isCloudSyncVaultSettingsPath } from './cloud-sync' import { emptyCloudSyncState, planCloudSyncMutations, @@ -40,6 +40,10 @@ export interface CloudSyncRemote { export interface CloudSyncRepository { scan(): Promise + /** Paths with a durable user decision still pending. The coordinator leaves + * both their tracked and local versions out of mutation planning until the + * host removes the pending marker. */ + pendingConflictPaths?(): Promise /** Returns a conflict when the local file was kept instead of being * replaced, so one unapplied change reports itself rather than stopping * the run. Sync must always be able to move past a single file. */ @@ -109,7 +113,25 @@ export class CloudSyncCoordinator { pulled += initialPull.pulled localConflicts.push(...initialPull.localConflicts) - const plan = planCloudSyncMutations(state, await this.repository.scan(), this.ids) + const localItems = await this.repository.scan() + const pendingPathKeys = new Set( + (await this.repository.pendingConflictPaths?.() ?? []).map(cloudSyncPathKey) + ) + const mutationState = + pendingPathKeys.size === 0 + ? state + : { + ...state, + items: Object.fromEntries( + Object.entries(state.items).filter( + ([, item]) => !pendingPathKeys.has(cloudSyncPathKey(item.path)) + ) + ) + } + const mutationItems = localItems.filter( + (item) => !pendingPathKeys.has(cloudSyncPathKey(item.path)) + ) + const plan = planCloudSyncMutations(mutationState, mutationItems, this.ids) const conflicts: CloudSyncConflict[] = [] const acknowledgedSequences = new Set() let mutationCursor = state.cursor @@ -189,6 +211,13 @@ export class CloudSyncCoordinator { for (const item of manifest.items) { const local = localByPath.get(cloudSyncPathKey(item.path)) if (local && local.content.sha256 !== item.sha256) { + if (isCloudSyncVaultSettingsPath(item.path)) { + if (!item.content) throw new Error(`Manifest item ${item.item_id} did not include content`) + const conflict = await this.repository.apply(manifestUpsert(item), undefined) + if (conflict) localConflicts.push(conflict) + pulled++ + continue + } conflicts.push({ code: 'BOOTSTRAP_CONTENT_CONFLICT', item_id: item.item_id, diff --git a/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts b/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts index aa50c914..21236bc9 100644 --- a/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts +++ b/packages/shared-domain/src/cloud-sync-portable-filesystem.test.ts @@ -208,6 +208,35 @@ describe('PortableCloudSyncRepository', () => { expect(fs.text('inbox/Plan (cloud conflict).md')).toBe('remote edit') }) + it('reports a parked settings choice until its cloud copy is removed', async () => { + const fs = new MemoryFileSystem({ + '.zennotes/vault.json': '{"favorites":["local.md"]}' + }) + const repository = new PortableCloudSyncRepository(fs) + + const conflict = await repository.apply( + { + sequence: 2, + item_id: 'settings-1', + type: 'upsert', + path: '.zennotes/vault.json', + previous_path: '.zennotes/vault.json', + revision: 2, + content: await textContent('{"favorites":["cloud.md"]}') + }, + undefined + ) + + expect(conflict).toEqual({ + code: 'SETTINGS_CONFLICT', + path: '.zennotes/vault.json', + conflict_copy_path: '.zennotes/vault.cloud-conflict.json' + }) + expect(await repository.pendingConflictPaths()).toEqual(['.zennotes/vault.json']) + await fs.deleteFile('.zennotes/vault.cloud-conflict.json') + expect(await repository.pendingConflictPaths()).toEqual([]) + }) + it('adopts a file that already matches the incoming change', async () => { const fs = new MemoryFileSystem({ '.zennotes/vault.json': '{"favorites":[]}' }) const repository = new PortableCloudSyncRepository(fs) diff --git a/packages/shared-domain/src/cloud-sync-portable-filesystem.ts b/packages/shared-domain/src/cloud-sync-portable-filesystem.ts index 3781628d..a57d1f83 100644 --- a/packages/shared-domain/src/cloud-sync-portable-filesystem.ts +++ b/packages/shared-domain/src/cloud-sync-portable-filesystem.ts @@ -5,6 +5,7 @@ import type { } from '@zennotes/bridge-contract/cloud-sync' import { CLOUD_SYNC_SETTINGS_CONFLICT_PATH, + CLOUD_SYNC_VAULT_SETTINGS_PATH, cloudSyncConflictCopyPath, isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, @@ -107,6 +108,12 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { return items.sort((left, right) => left.path.localeCompare(right.path)) } + async pendingConflictPaths(): Promise { + return (await this.fs.stat(CLOUD_SYNC_SETTINGS_CONFLICT_PATH)) === 'file' + ? [CLOUD_SYNC_VAULT_SETTINGS_PATH] + : [] + } + async apply( change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined From f1fbe3aa3a9d7c7cd9cc7c8aaa8029b2e6aaf6f0 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 12:39:28 -0500 Subject: [PATCH 16/18] Fix(vault): atomic saves wait out Windows readers Windows can deny a destination rename while a watcher, indexer, or antivirus scanner has the file open. The atomic writer treated that transient sharing window as a permanent save failure, and the new concurrent-reader test exposed it in CI. Retry only permission and sharing failures with a short bounded backoff. Both the Go server and Electron writer keep the old file visible until a complete replacement can land, and all other errors still fail immediately. --- apps/desktop/src/main/vault.test.ts | 22 +++++++++++ apps/desktop/src/main/vault.ts | 28 +++++++++++++- apps/server/internal/vault/atomicwrite.go | 37 ++++++++++++++++++- .../server/internal/vault/atomicwrite_test.go | 29 +++++++++++++++ 4 files changed, 114 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/main/vault.test.ts b/apps/desktop/src/main/vault.test.ts index ebddd60b..70c146b1 100644 --- a/apps/desktop/src/main/vault.test.ts +++ b/apps/desktop/src/main/vault.test.ts @@ -37,6 +37,7 @@ import { unarchiveNote, vaultChangeAffectsSettings, isAtomicWriteTempPath, + renameWithRetry, writeNote } from './vault' @@ -1219,6 +1220,27 @@ describe('writeNote atomic-save fidelity (#585)', () => { expect(entries.filter((name) => name.endsWith('.tmp'))).toEqual([]) }) + it('retries a replace while another process temporarily denies it', async () => { + let calls = 0 + const delays: number[] = [] + await renameWithRetry( + 'Note.md.tmp', + 'Note.md', + async () => { + calls++ + if (calls < 3) { + throw Object.assign(new Error('sharing violation'), { code: 'EACCES' }) + } + }, + async (delay) => { + delays.push(delay) + } + ) + + expect(calls).toBe(3) + expect(delays).toEqual([1, 2]) + }) + it('recognizes its own scratch files without swallowing user files', () => { expect(isAtomicWriteTempPath('inbox/Note.md.4123.1786714355519000.tmp')).toBe(true) expect(isAtomicWriteTempPath('inbox/Note.md')).toBe(false) diff --git a/apps/desktop/src/main/vault.ts b/apps/desktop/src/main/vault.ts index b85d6c29..5aa40e80 100644 --- a/apps/desktop/src/main/vault.ts +++ b/apps/desktop/src/main/vault.ts @@ -666,6 +666,32 @@ export function isAtomicWriteTempPath(p: string): boolean { return ATOMIC_WRITE_TEMP_PATTERN.test(path.basename(p)) } +const ATOMIC_RENAME_ATTEMPTS = 20 + +function transientRenameError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | null)?.code + return code === 'EACCES' || code === 'EPERM' || code === 'EBUSY' +} + +/** Wait out a reader that temporarily denies replacing the destination. */ +export async function renameWithRetry( + from: string, + to: string, + rename: (from: string, to: string) => Promise = fs.rename, + pause: (delayMs: number) => Promise = (delayMs) => + new Promise((resolve) => setTimeout(resolve, delayMs)) +): Promise { + for (let attempt = 1; ; attempt++) { + try { + await rename(from, to) + return + } catch (error) { + if (attempt >= ATOMIC_RENAME_ATTEMPTS || !transientRenameError(error)) throw error + await pause(Math.min(2 ** (attempt - 1), 25)) + } + } +} + /** Same millisecond, same path, two writers: the stamp alone would name one * temp file for both and let them interleave into it. */ let atomicWriteSequence = 0 @@ -733,7 +759,7 @@ export async function writeFileAtomic(absPath: string, data: string): Promise= atomicRenameAttempts || !transientRenameError(err) { + return err + } + sleep(delay) + delay = min(delay*2, 25*time.Millisecond) + } +} + func writeAndSync(f *os.File, data []byte) error { if _, err := f.Write(data); err != nil { return err diff --git a/apps/server/internal/vault/atomicwrite_test.go b/apps/server/internal/vault/atomicwrite_test.go index 7878ef49..570afd1f 100644 --- a/apps/server/internal/vault/atomicwrite_test.go +++ b/apps/server/internal/vault/atomicwrite_test.go @@ -3,12 +3,14 @@ package vault import ( "errors" "fmt" + "io/fs" "os" "path/filepath" "runtime" "strings" "sync" "testing" + "time" ) // The #585 property, and the whole reason WriteNote is atomic: the watcher @@ -82,6 +84,33 @@ func TestWriteNoteNeverExposesAPartialFile(t *testing.T) { } } +func TestRenameWithRetryWaitsOutTransientPermissionErrors(t *testing.T) { + calls := 0 + var delays []time.Duration + err := renameWithRetry( + "note.tmp", + "note.md", + func(_, _ string) error { + calls++ + if calls < 3 { + return fs.ErrPermission + } + return nil + }, + func(delay time.Duration) { delays = append(delays, delay) }, + ) + + if err != nil { + t.Fatal(err) + } + if calls != 3 { + t.Fatalf("rename calls = %d, want 3", calls) + } + if len(delays) != 2 || delays[0] <= 0 || delays[1] <= delays[0] { + t.Fatalf("retry delays = %v, want two increasing delays", delays) + } +} + // A rename replaces the directory entry, so an atomic write aimed straight at a // symlinked note would leave a regular file where the link was and detach it // from its target for good. From bf97e4802d22efeb7d2d725e56b655ace5e2f805 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Fri, 14 Aug 2026 12:40:20 -0500 Subject: [PATCH 17/18] Fix(editor): LaTeX completion keeps dollar context The display-math scan started at an arbitrary 20,000-character boundary. If that boundary split a long formula from its opening delimiter, completion either disappeared inside the formula or appeared in prose after its closing delimiter. Count display delimiters from the stable start of the document. Long closed and still-open formulas now preserve the same parity as short ones, while code-region filtering remains unchanged. --- packages/app-core/src/lib/cm-latex-completions.test.ts | 9 +++++++++ packages/app-core/src/lib/cm-latex-completions.ts | 10 ++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/app-core/src/lib/cm-latex-completions.test.ts b/packages/app-core/src/lib/cm-latex-completions.test.ts index c01a0dec..c0b034e1 100644 --- a/packages/app-core/src/lib/cm-latex-completions.test.ts +++ b/packages/app-core/src/lib/cm-latex-completions.test.ts @@ -36,6 +36,15 @@ describe('isInMathContext', () => { expect(isInMathContext(state(open), open.length)).toBe(true) }) + it('keeps delimiter parity across very long display blocks', () => { + const longBody = 'x'.repeat(20_100) + const outside = `$$\n${longBody}\n$$\nplain \\su` + expect(isInMathContext(state(outside), outside.length)).toBe(false) + + const inside = `$$\n${longBody}\n\\su` + expect(isInMathContext(state(inside), inside.length)).toBe(true) + }) + it('treats ```math fences as math, other fences as code', () => { const mathFence = 'a\n```math\n\\su\n```\nb' expect(isInMathContext(state(mathFence), after(mathFence, '\\su'))).toBe(true) diff --git a/packages/app-core/src/lib/cm-latex-completions.ts b/packages/app-core/src/lib/cm-latex-completions.ts index ba9a00a4..f0f2958d 100644 --- a/packages/app-core/src/lib/cm-latex-completions.ts +++ b/packages/app-core/src/lib/cm-latex-completions.ts @@ -184,11 +184,6 @@ function codeContext(state: EditorState, pos: number): CodeContext { /** Inside `$…$`, `$$…$$`, or a ```math fence at `pos`? Counts unmatched * dollar delimiters so a formula still being typed (no closing `$` yet) * already counts as math. */ -/** How far back an unclosed `$$` is looked for. A display block open further - * above than this is not a formula anyone is still typing, and the bound keeps - * the scan off the whole document on every `\` in a long note. */ -const BLOCK_SCAN_WINDOW = 20_000 - /** Dollars inside code are not delimiters: `echo $$` in a shell block would * otherwise flip the parity and make the rest of the note read as math. */ function countDelimiters(state: EditorState, from: number, text: string, re: RegExp): number { @@ -206,11 +201,10 @@ export function isInMathContext(state: EditorState, pos: number): boolean { // A ```math fence is a math region in its own right (remark-math renders // it as display math); every other code region shuts completion off. if (code) return code.kind === 'fenced' && code.lang === 'math' - const blockFrom = Math.max(0, pos - BLOCK_SCAN_WINDOW) const blockFences = countDelimiters( state, - blockFrom, - state.doc.sliceString(blockFrom, pos), + 0, + state.doc.sliceString(0, pos), /(? Date: Fri, 14 Aug 2026 12:50:20 -0500 Subject: [PATCH 18/18] Fix(editor): cold grammars get their first scan vscode-textmate includes one-time scanner compilation in its per-line deadline. Slower Windows runners could therefore mark a tiny valid preview as stopped before any token was emitted. Retry only a stopped first line once, after compilation has completed, while charging both attempts to the existing wall-clock caps. A deterministic cold-start regression now covers the behavior. --- .../src/lib/custom-code-language-engine.ts | 19 +++++++++++++--- .../src/lib/custom-code-languages.test.ts | 22 +++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/app-core/src/lib/custom-code-language-engine.ts b/packages/app-core/src/lib/custom-code-language-engine.ts index aa1eba19..bf563f70 100644 --- a/packages/app-core/src/lib/custom-code-language-engine.ts +++ b/packages/app-core/src/lib/custom-code-language-engine.ts @@ -162,14 +162,27 @@ export function tokenizeWithGrammar( const lines = source.split("\n"); for (let lineIndex = 0; lineIndex < lines.length; lineIndex++) { const line = lines[lineIndex]; - const started = performance.now(); - const result = grammar.tokenizeLine(line, state, LINE_TIME_LIMIT_MS); - const elapsed = performance.now() - started; + let started = performance.now(); + let result = grammar.tokenizeLine(line, state, LINE_TIME_LIMIT_MS); + let elapsed = performance.now() - started; spent += elapsed; if (elapsed > LINE_BUDGET_MS || spent > FENCE_BUDGET_MS) { quarantine(definition.id, definition.name); return []; } + // TextMate charges one-time scanner compilation against the first line's + // limit. A busy Windows host can exhaust that limit before scanning even a + // tiny line. Retry that first line once now that the scanner is compiled. + if (lineIndex === 0 && result.stoppedEarly) { + started = performance.now(); + result = grammar.tokenizeLine(line, state, LINE_TIME_LIMIT_MS); + elapsed = performance.now() - started; + spent += elapsed; + if (elapsed > LINE_BUDGET_MS || spent > FENCE_BUDGET_MS) { + quarantine(definition.id, definition.name); + return []; + } + } if (result.stoppedEarly) return []; state = result.ruleStack; for (const token of result.tokens) { diff --git a/packages/app-core/src/lib/custom-code-languages.test.ts b/packages/app-core/src/lib/custom-code-languages.test.ts index 17c646ec..32a4cbcf 100644 --- a/packages/app-core/src/lib/custom-code-languages.test.ts +++ b/packages/app-core/src/lib/custom-code-languages.test.ts @@ -63,6 +63,28 @@ describe("custom code language runtime", () => { expect(highlighted).toContainEqual({ text: "42", kind: "number" }); }); + it("retries a cold grammar when scanner compilation uses the first line budget", async () => { + await customCodeLanguageRegistry.replace([gleam]); + const engine = await import("./custom-code-language-engine"); + const loaded = customCodeLanguageRegistry.resolve("gleam"); + if (!loaded) throw new Error("gleam should be registered"); + + const realTokenizeLine = loaded.grammar.tokenizeLine.bind(loaded.grammar); + let calls = 0; + loaded.grammar.tokenizeLine = ((line, state, limit) => { + const result = realTokenizeLine(line, state, limit); + calls++; + return calls === 1 ? { ...result, stoppedEarly: true } : result; + }) as typeof loaded.grammar.tokenizeLine; + + const source = "fn main"; + const tokens = engine.tokenizeWithGrammar(loaded, source); + expect(tokens.map((token) => source.slice(token.from, token.to))).toContain( + "fn", + ); + expect(calls).toBe(2); + }); + it("uses one registry for rendered Markdown and CodeMirror decorations", async () => { await customCodeLanguageRegistry.replace([gleam]); const source = "```gleam\nfn main {\n let answer = 42\n}\n```";