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/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/cloud-sync-filesystem.test.ts b/apps/desktop/src/main/cloud-sync-filesystem.test.ts index f0f15dd0..7dc8486b 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,143 @@ 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' + }) + 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"]}' + ) + + // 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..16d5a101 100644 --- a/apps/desktop/src/main/cloud-sync-filesystem.ts +++ b/apps/desktop/src/main/cloud-sync-filesystem.ts @@ -1,8 +1,16 @@ 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, + CLOUD_SYNC_VAULT_SETTINGS_PATH, + cloudSyncConflictCopyPath, + isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, shouldSyncVaultPath, shouldTraverseCloudSyncDirectory @@ -80,7 +88,16 @@ export class DesktopCloudSyncRepository implements CloudSyncRepository { return items.sort((left, right) => left.path.localeCompare(right.path)) } - async apply(change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined): Promise { + 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 + ): Promise { const affectedPaths = [change.path, change.previous_path, previous?.path].filter( (path): path is string => typeof path === 'string' ) @@ -88,13 +105,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 +136,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 +184,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 +341,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 eab1f32c..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, @@ -182,6 +185,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"; @@ -2629,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()), ); @@ -4759,6 +4774,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 diff --git a/apps/desktop/src/main/vault.test.ts b/apps/desktop/src/main/vault.test.ts index d524ec33..70c146b1 100644 --- a/apps/desktop/src/main/vault.test.ts +++ b/apps/desktop/src/main/vault.test.ts @@ -36,6 +36,8 @@ import { setVaultSettings, unarchiveNote, vaultChangeAffectsSettings, + isAtomicWriteTempPath, + renameWithRetry, writeNote } from './vault' @@ -1168,3 +1170,80 @@ 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('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) + 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 914957a7..5aa40e80 100644 --- a/apps/desktop/src/main/vault.ts +++ b/apps/desktop/src/main/vault.ts @@ -655,16 +655,98 @@ 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)) +} + +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 + +/** 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 +758,8 @@ export async function writeFileAtomic(absPath: string, data: 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..549f9f01 100644 --- a/apps/desktop/src/main/watcher.ts +++ b/apps/desktop/src/main/watcher.ts @@ -2,7 +2,7 @@ import path from 'node:path' import chokidar, { FSWatcher } from 'chokidar' import type { NoteFolder, VaultChangeEvent, VaultChangeKind, VaultSettings } from '@shared/ipc' import { databaseCsvPathFor } from '@shared/databases' -import { folderForRelativePath, getVaultSettings } from './vault' +import { folderForRelativePath, getVaultSettings, isAtomicWriteTempPath } from './vault' const ATTACHMENTS_DIRS = new Set(['assets', 'attachements', '_assets']) const INTERNAL_VAULT_DIR = '.zennotes' @@ -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) + // 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/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/server/internal/vault/atomicwrite.go b/apps/server/internal/vault/atomicwrite.go new file mode 100644 index 00000000..d1ee5a5e --- /dev/null +++ b/apps/server/internal/vault/atomicwrite.go @@ -0,0 +1,171 @@ +package vault + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "runtime" + "syscall" + "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 := renameWithRetry(temp, target, os.Rename, time.Sleep); err != nil { + _ = os.Remove(temp) + return err + } + return nil +} + +const atomicRenameAttempts = 20 +const windowsSharingViolation syscall.Errno = 32 + +func transientRenameError(err error) bool { + if errors.Is(err, fs.ErrPermission) { + return true + } + var errno syscall.Errno + return runtime.GOOS == "windows" && errors.As(err, &errno) && errno == windowsSharingViolation +} + +// Windows refuses a replace while any reader has the destination open without +// delete sharing. Watchers, indexers, and antivirus scanners all create that +// short-lived condition, so wait for the handle instead of failing the save. +func renameWithRetry( + from, to string, + rename func(string, string) error, + sleep func(time.Duration), +) error { + delay := time.Millisecond + for attempt := 1; ; attempt++ { + err := rename(from, to) + if err == nil { + return nil + } + if attempt >= 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 + } + // 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..570afd1f --- /dev/null +++ b/apps/server/internal/vault/atomicwrite_test.go @@ -0,0 +1,240 @@ +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 +// 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: + } +} + +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. +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/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/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/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/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 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/EditorPane.tsx b/packages/app-core/src/components/EditorPane.tsx index 53915bda..80474045 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 { @@ -74,7 +75,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 +106,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' @@ -120,6 +122,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' @@ -325,6 +328,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: () => { @@ -404,6 +412,7 @@ function markdownEditingExtensions(showHeadingLevelLabels = false): Extension[] vimAwareMarkdownKeymap, markdownListIndentPlugin, frontmatterStyle, + frontmatterTagExtension, orderedListRenumber, forwardOnCheckboxArrow, headingFolding({ showLevelLabels: showHeadingLevelLabels }), @@ -1779,7 +1788,9 @@ export function EditorPane({ pane }: { pane: PaneLeaf }): JSX.Element { slashCommandSource, calloutTypeSource, dateShortcutSource, + latexCommandSource, atNoteSource, + frontmatterTagSource, hashtagSource, wikilinkSource, wikilinkHeadingSource 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/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/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 397ced93..7e5442f4 100644 --- a/packages/app-core/src/components/PromptModal.tsx +++ b/packages/app-core/src/components/PromptModal.tsx @@ -4,6 +4,27 @@ 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) +} + +/** + * 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 @@ -107,6 +128,7 @@ export function PromptModal({ }, [options.initialValue, options.title]) useEffect(() => { + if (!shouldAutofocusPrompt(isCoarsePointer(), options.suggestions?.length ?? 0)) return const t = setTimeout(() => { inputRef.current?.focus() inputRef.current?.select() @@ -213,7 +235,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 +255,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(' ')} > 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/cloud-auto-sync.test.ts b/packages/app-core/src/lib/cloud-auto-sync.test.ts index 297bfab4..3d1f9dbd 100644 --- a/packages/app-core/src/lib/cloud-auto-sync.test.ts +++ b/packages/app-core/src/lib/cloud-auto-sync.test.ts @@ -36,7 +36,7 @@ function setup( pulled: 0, pushed: 0, conflicts: [], - bootstrap_conflicts: [], + bootstrap_conflicts: [], local_conflicts: [], })); const logoutCloudAccount = vi.fn(async (): Promise => { 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/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-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..3807e82e --- /dev/null +++ b/packages/app-core/src/lib/cm-frontmatter-tag-complete.ts @@ -0,0 +1,106 @@ +/** + * 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 { 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 + * 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 = frontmatterTagsValue(text) + if (inline) { + if (col < inline.offset) return null + return tagTokenAt(state, line.from, inline.offset, 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..3110b257 --- /dev/null +++ b/packages/app-core/src/lib/cm-frontmatter-tag.test.ts @@ -0,0 +1,112 @@ +// @vitest-environment jsdom + +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() + +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') + }) +}) + +// 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 9321a1b4..3fbbdb5d 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,121 @@ export const frontmatterStyle = ViewPlugin.fromClass( }, { decorations: (v) => v.decorations } ) + +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) + 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 = frontmatterTagsValue(text) + if (inline) { + addTagTokens(inline.value, line.from + inline.offset, 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/lib/cm-latex-completions.test.ts b/packages/app-core/src/lib/cm-latex-completions.test.ts new file mode 100644 index 00000000..c0b034e1 --- /dev/null +++ b/packages/app-core/src/lib/cm-latex-completions.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, it } from 'vitest' +import { EditorState } from '@codemirror/state' +import { markdown } from '@codemirror/lang-markdown' +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()] }) +} + +/** 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('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) + + 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() + }) +}) + +// 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 new file mode 100644 index 00000000..f0f2958d --- /dev/null +++ b/packages/app-core/src/lib/cm-latex-completions.ts @@ -0,0 +1,314 @@ +/** + * 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' +import { mathRendererOf } from './cm-math-render' + +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. */ +/** 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 blockFences = countDelimiters( + state, + 0, + state.doc.sliceString(0, pos), + /(? + ({ + 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 { + // These are LaTeX commands. A note set to the Typst typesetter takes + // different syntax, so offering `\frac{}{}` there would only ever be wrong. + if (mathRendererOf(context.state) !== 'katex') return 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]*$/ + } +} + +/** KaTeX output for one preview, kept between popups: a bare `\` opens the + * whole table at once, and re-typesetting every row each time it opens is the + * one visible cost this feature has. */ +const previewCache = new Map() + +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. */ +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' + icon.innerHTML = renderPreview(_preview ?? completion.label) + + 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-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 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') 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 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/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```"; 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.' }, diff --git a/packages/app-core/src/store-note-integrity.test.ts b/packages/app-core/src/store-note-integrity.test.ts index a50c149b..c6b398fb 100644 --- a/packages/app-core/src/store-note-integrity.test.ts +++ b/packages/app-core/src/store-note-integrity.test.ts @@ -191,3 +191,145 @@ 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') + }) + + 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 + // 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 5c00e7e9..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 /** @@ -6236,7 +6240,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 @@ -6248,6 +6257,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 { @@ -6307,41 +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) => { - const dirty = { ...cur.noteDirty, [path]: false } - 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) } }, 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 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/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/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-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..fbfa22b8 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') } @@ -149,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 6564c49d..92043583 100644 --- a/packages/shared-domain/src/cloud-sync-coordinator.ts +++ b/packages/shared-domain/src/cloud-sync-coordinator.ts @@ -2,12 +2,13 @@ import type { CloudSyncChange, CloudSyncBootstrapConflict, CloudSyncConflict, + CloudSyncLocalConflict, CloudSyncManifestItem, CloudSyncManifestResponse, CloudSyncMutationRequest, CloudSyncMutationResponse } from '@zennotes/bridge-contract/cloud-sync' -import { cloudSyncPathKey } from './cloud-sync' +import { cloudSyncPathKey, isCloudSyncVaultSettingsPath } from './cloud-sync' import { emptyCloudSyncState, planCloudSyncMutations, @@ -39,7 +40,17 @@ export interface CloudSyncRemote { export interface CloudSyncRepository { scan(): Promise - apply(change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined): 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. */ + apply( + change: CloudSyncChange, + previous: CloudSyncTrackedItem | undefined + ): Promise } export interface CloudSyncStateStore { @@ -53,6 +64,7 @@ export interface CloudSyncRunResult { pushed: number conflicts: CloudSyncConflict[] bootstrapConflicts: CloudSyncBootstrapConflict[] + localConflicts: CloudSyncLocalConflict[] } /** @@ -88,17 +100,38 @@ 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 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 @@ -122,17 +155,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 +175,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,26 +189,35 @@ 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) { 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, @@ -185,7 +230,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 +239,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..21236bc9 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,91 @@ 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('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) + + 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..a57d1f83 100644 --- a/packages/shared-domain/src/cloud-sync-portable-filesystem.ts +++ b/packages/shared-domain/src/cloud-sync-portable-filesystem.ts @@ -1,8 +1,13 @@ import type { CloudSyncChange, - CloudSyncContent + CloudSyncContent, + CloudSyncLocalConflict } from '@zennotes/bridge-contract/cloud-sync' import { + CLOUD_SYNC_SETTINGS_CONFLICT_PATH, + CLOUD_SYNC_VAULT_SETTINGS_PATH, + cloudSyncConflictCopyPath, + isCloudSyncVaultSettingsPath, normalizeCloudSyncPath, shouldSyncVaultPath, shouldTraverseCloudSyncDirectory @@ -81,6 +86,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 +108,16 @@ export class PortableCloudSyncRepository implements CloudSyncRepository { return items.sort((left, right) => left.path.localeCompare(right.path)) } - async apply(change: CloudSyncChange, previous: CloudSyncTrackedItem | undefined): Promise { + 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 + ): Promise { const affectedPaths = [change.path, change.previous_path, previous?.path].filter( (path): path is string => typeof path === 'string' ) @@ -101,7 +127,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 +145,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 +169,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 +242,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 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"