From 77612a995c0bd958dd78147194f6766873a26e5c Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 22 Oct 2026 17:22:24 -0500 Subject: [PATCH] fix(mobile): New note and Task stop re-reading the whole vault The Android half of this fix (ZenNotes/zennotesandroid#128, f6a9bfb there) began with New note and Quick taking 12 to 15 seconds on a 368 MB picked folder. The cause sits in code this app shares with Android line for line. app-core's refresh asks for the notes and the folders together, and each walked the whole tree on its own. A new note starts two of those refreshes at once (createAndOpen's own and the one its 'add' event starts), and returning to the app starts a rescan plus the resync it emits. The task scan read every note in the vault on every call. On Android's emulator that was 678 directory listings for one New note and 1,613 note reads for one Task. A listing here is cheaper than a picked folder's document-provider query there, but an iCloud or picked-folder vault of the same size made just as many. MobileVault's tree walks (notes, folders, assets) now share their directory listings: a walk reuses a listing another walk is fetching or fetched, while any walk runs and for one second after the last one ends, which covers the resync a rescan emits. A listing never outlives a change this vault makes: NativeFs moves a write generation before and after every mutation, and a walk that sees a new generation starts from fresh listings. That counts writes, folder creation, renames, copies and deletes, Cloud sync's verified copies, downloads and decryption, and the temp file an encrypted upload is sealed into (Android counts that one through the placeholder it writes first; here native creates the file, so the native call itself is counted). Changes from outside the app (iCloud, the Files app, another device) are found by the foreground rescan and pull to refresh, as before; a walk within that one second may miss one, the next one will not. The task scan keeps each note's parsed tasks while the note's listed mtime and size hold, the test the meta cache already uses, and drops a note's entry whenever the vault writes, moves or deletes it. What this does not do: app-core still awaits a full refresh before it opens a new note and a full task scan before "Task added". That half is in core (ZenNotes/zennotes, branch fix/create-open-before-refresh-128) and reaches this app when it adopts the next core. The vault-fs change is Android's, applied as is apart from two comments. vault-walks.test.ts is Android's test: three of its five tests fail on the old code (the inbox folder listed five times instead of once, a change from outside visible inside the window, 16 note reads instead of 11), and ignoring the write generation fails a fourth. 283 tests, typecheck and boundary check pass. The app builds for the simulator, and on an iPhone 17 simulator it creates a vault and opens the welcome note. Not yet timed on a device. --- src/bridge/native-fs.ts | 53 +++++++--- src/bridge/vault-fs.ts | 171 ++++++++++++++++++++++++++------- src/bridge/vault-walks.test.ts | 158 ++++++++++++++++++++++++++++++ 3 files changed, 333 insertions(+), 49 deletions(-) create mode 100644 src/bridge/vault-walks.test.ts diff --git a/src/bridge/native-fs.ts b/src/bridge/native-fs.ts index 3b0e8d1..62962ce 100644 --- a/src/bridge/native-fs.ts +++ b/src/bridge/native-fs.ts @@ -115,6 +115,14 @@ export class NativeFs { readonly cloudRootUri: string | null /** Native URI of the vault root, resolved at open. */ rootUri: string | null = null + /** + * Moves before and after every change this vault makes on disk, so a + * listing taken at one generation is known to predate nothing the app wrote + * since (MobileVault shares listings between overlapping tree walks on that + * promise). Writers outside the app are not counted; the foreground rescan + * is what finds those. + */ + writeGeneration = 0 constructor(vaultName: string, cloudRootUri: string | null = null) { this.rootPath = `${VAULTS_DIR}/${vaultName}` @@ -125,6 +133,15 @@ export class NativeFs { return this.cloudRootUri !== null } + private async changing(change: () => Promise): Promise { + this.writeGeneration += 1 + try { + return await change() + } finally { + this.writeGeneration += 1 + } + } + private loc(relPath: string): FsLocation { if (this.cloudRootUri) { return { @@ -232,6 +249,10 @@ export class NativeFs { /** Verified native copy into a new staging path. */ async copyForSync(fromRel: string, toRel: string, byteLength: number): Promise { + await this.changing(() => this.copyForSyncOnce(fromRel, toRel, byteLength)) + } + + private async copyForSyncOnce(fromRel: string, toRel: string, byteLength: number): Promise { if (fromRel === toRel || !Number.isSafeInteger(byteLength) || byteLength < 0) throw new Error('Invalid Cloud file copy.') if (await this.statVerified(toRel) !== null) throw new Error('Cloud copy destination already exists.') const source = await this.readForSync(fromRel, false) @@ -245,6 +266,10 @@ export class NativeFs { /** Stream a signed Cloud revision to a staging file; native verifies before resolving. */ async download(options: { url: string; headers: Record; to: string; byteLength: number; sha256: string }): Promise { + await this.changing(() => this.downloadOnce(options)) + } + + private async downloadOnce(options: { url: string; headers: Record; to: string; byteLength: number; sha256: string }): Promise { if (await this.statVerified(options.to) !== null) throw new Error('Cloud download destination already exists.') const parent = options.to.includes('/') ? options.to.slice(0, options.to.lastIndexOf('/')) : '' if (parent) await this.mkdir(parent) @@ -266,7 +291,7 @@ export class NativeFs { const destination = this.fileUri(options.to) if (!source || !destination) throw new Error('Vault root is not resolved.') const { header, objectKey, aad } = options - return CloudFiles.decryptFile({ source, destination, header, objectKey, aad }) + return await this.changing(() => CloudFiles.decryptFile({ source, destination, header, objectKey, aad })) } /** @@ -280,7 +305,7 @@ export class NativeFs { if (!destination) throw new Error('Vault root is not resolved.') const discard = () => this.deleteFile(temporary).catch(() => {}) try { - const sealed = await CloudFiles.encryptFile({ source: sourceUri, destination, ...keys }) + const sealed = await this.changing(() => CloudFiles.encryptFile({ source: sourceUri, destination, ...keys })) return { ...sealed, uri: destination, discard } } catch (error) { await discard() @@ -294,21 +319,21 @@ export class NativeFs { * a note folder surfaces in listAssets and the sidebar as a stray file. */ async writeText(relPath: string, body: string): Promise { - await Filesystem.writeFile({ + await this.changing(() => Filesystem.writeFile({ ...this.loc(relPath), data: body, encoding: Encoding.UTF8, recursive: true - }) + })) } /** Write binary data from a base64 string (Capacitor's native transport). */ async writeBase64(relPath: string, base64Data: string): Promise { - await Filesystem.writeFile({ + await this.changing(() => Filesystem.writeFile({ ...this.loc(relPath), data: base64Data, recursive: true - }) + })) } async statOrNull(relPath: string): Promise { @@ -367,40 +392,40 @@ export class NativeFs { } async mkdir(relPath: string): Promise { - await Filesystem.mkdir({ ...this.loc(relPath), recursive: true }).catch((err) => { + await this.changing(() => Filesystem.mkdir({ ...this.loc(relPath), recursive: true }).catch((err) => { const msg = String((err as Error)?.message ?? err) if (!/exist/i.test(msg)) throw err - }) + })) } async rename(fromRel: string, toRel: string): Promise { const from = this.loc(fromRel) const to = this.loc(toRel) - await Filesystem.rename({ + await this.changing(() => Filesystem.rename({ from: from.path, to: to.path, directory: from.directory, toDirectory: to.directory - }) + })) } async copy(fromRel: string, toRel: string): Promise { const from = this.loc(fromRel) const to = this.loc(toRel) - await Filesystem.copy({ + await this.changing(() => Filesystem.copy({ from: from.path, to: to.path, directory: from.directory, toDirectory: to.directory - }) + })) } async deleteFile(relPath: string): Promise { - await Filesystem.deleteFile(this.loc(relPath)) + await this.changing(() => Filesystem.deleteFile(this.loc(relPath))) } async rmdir(relPath: string): Promise { - await Filesystem.rmdir({ ...this.loc(relPath), recursive: true }) + await this.changing(() => Filesystem.rmdir({ ...this.loc(relPath), recursive: true })) } async exists(relPath: string): Promise { diff --git a/src/bridge/vault-fs.ts b/src/bridge/vault-fs.ts index 0006d4e..0225de3 100644 --- a/src/bridge/vault-fs.ts +++ b/src/bridge/vault-fs.ts @@ -103,6 +103,19 @@ interface CachedMeta { meta: NoteMeta } +interface CachedTasks { + mtime: number + size: number + title: string + folder: NoteFolder + tasks: VaultTask[] +} + +/** How long the listings of a finished walk stay shareable. Long enough for + * the resync a rescan emits to reuse the rescan's walk, short enough that no + * external change waits on it in a way a person could notice. */ +const SHARED_LISTING_GRACE_MS = 1000 + interface SearchCandidate { path: string title: string @@ -176,6 +189,25 @@ export class MobileVault { private metaCacheDirty = false private metaCachePersistTimer: ReturnType | null = null private settingsCache: VaultSettings | null = null + /** + * Directory listings the tree walks (notes, folders, assets) share. app-core + * asks for notes and folders together on every refresh, and a new note or a + * return to the app starts two refreshes at once, so the vault was listed + * four times over before a new note could open. On Android, where a picked + * folder answers each listing with a document-provider query, that was 12 + * to 15 seconds on a budget phone (zennotesandroid#128); the walks here are + * the same code and multiplied the same way on a large vault. + * A listing is shared only with walks that overlap it or start + * within SHARED_LISTING_GRACE_MS of the last one ending (the resync a rescan + * emits), and never across a write this vault makes. + */ + private sharedListings: { generation: number; byDir: Map> } | null = null + private walksInFlight = 0 + private sharedListingsTimer: ReturnType | null = null + /** Each note's parsed tasks, reused while its listed mtime and size hold, + * the test the meta cache uses. Without it every task scan read every note + * in the vault, and adding one task runs a full scan (zennotesandroid#128). */ + private taskCache = new Map() constructor(name: string, cloudRootUri: string | null = null) { this.name = name @@ -418,6 +450,7 @@ export class MobileVault { private invalidateMeta(relPath: string): void { this.metaCache.delete(relPath) + this.taskCache.delete(relPath) this.metaCacheDirty = true } @@ -495,20 +528,56 @@ export class MobileVault { // Listing // ------------------------------------------------------------------- - async listNotes(): Promise { - const out: NoteMeta[] = [] - const settings = await this.getVaultSettings() - const hidden = hiddenPrimaryRootNames(settings.systemFolderPaths) - for (const folder of FOLDERS) { - const topRel = await this.folderRootRel(folder) - if (folder !== 'inbox' || topRel !== '') { - const stat = await this.fs.statOrNull(topRel) - if (!stat || stat.type !== 'directory') continue + /** Run one tree walk with listings shared between overlapping walks. */ + private async sharingListings(walk: () => Promise): Promise { + this.walksInFlight += 1 + if (this.sharedListingsTimer) { + clearTimeout(this.sharedListingsTimer) + this.sharedListingsTimer = null + } + try { + return await walk() + } finally { + this.walksInFlight -= 1 + if (this.walksInFlight === 0) { + this.sharedListingsTimer = setTimeout(() => { + this.sharedListingsTimer = null + if (this.walksInFlight === 0) this.sharedListings = null + }, SHARED_LISTING_GRACE_MS) } - await this.walkNotes(topRel, folder, topRel === '' || topRel === undefined, hidden, out) } - this.scheduleMetaCachePersist() - return out + } + + private walkReaddir(dirRel: string): Promise { + const generation = this.fs.writeGeneration + if (this.sharedListings?.generation !== generation) { + this.sharedListings = { generation, byDir: new Map() } + } + const byDir = this.sharedListings.byDir + let listing = byDir.get(dirRel) + if (!listing) { + listing = this.fs.readdir(dirRel) + byDir.set(dirRel, listing) + } + return listing + } + + async listNotes(): Promise { + return await this.sharingListings(async () => { + const out: NoteMeta[] = [] + const settings = await this.getVaultSettings() + const hidden = hiddenPrimaryRootNames(settings.systemFolderPaths) + for (const folder of FOLDERS) { + const topRel = await this.folderRootRel(folder) + if (folder !== 'inbox' || topRel !== '') { + const stat = await this.fs.statOrNull(topRel) + if (!stat || stat.type !== 'directory') continue + } + await this.walkNotes(topRel, folder, topRel === '' || topRel === undefined, hidden, out) + } + this.scheduleMetaCachePersist() + return out + }) } private async walkNotes( @@ -518,7 +587,7 @@ export class MobileVault { hidden: Set, out: NoteMeta[] ): Promise { - const entries = await this.fs.readdir(dirRel) + const entries = await this.walkReaddir(dirRel) for (const [index, entry] of entries.entries()) { const childRel = dirRel ? `${dirRel}/${entry.name}` : entry.name if (entry.type === 'directory') { @@ -540,19 +609,21 @@ export class MobileVault { } async listFolders(): Promise { - const out: FolderEntry[] = [] - const settings = await this.getVaultSettings() - const hidden = hiddenPrimaryRootNames(settings.systemFolderPaths) - for (const folder of FOLDERS) { - const topRel = await this.folderRootRel(folder) - const isPrimaryRoot = folder === 'inbox' && topRel === '' - if (!isPrimaryRoot) { - const stat = await this.fs.statOrNull(topRel) - if (!stat || stat.type !== 'directory') continue + return await this.sharingListings(async () => { + const out: FolderEntry[] = [] + const settings = await this.getVaultSettings() + const hidden = hiddenPrimaryRootNames(settings.systemFolderPaths) + for (const folder of FOLDERS) { + const topRel = await this.folderRootRel(folder) + const isPrimaryRoot = folder === 'inbox' && topRel === '' + if (!isPrimaryRoot) { + const stat = await this.fs.statOrNull(topRel) + if (!stat || stat.type !== 'directory') continue + } + await this.walkFolders(topRel, folder, '', isPrimaryRoot, hidden, out) } - await this.walkFolders(topRel, folder, '', isPrimaryRoot, hidden, out) - } - return out + return out + }) } private async walkFolders( @@ -563,7 +634,7 @@ export class MobileVault { hidden: Set, out: FolderEntry[] ): Promise { - const entries = await this.fs.readdir(dirRel) + const entries = await this.walkReaddir(dirRel) for (const [index, entry] of entries.entries()) { if (entry.type !== 'directory') continue if (entry.name.startsWith('.')) continue @@ -577,14 +648,16 @@ export class MobileVault { } async listAssets(): Promise { - const out: AssetMeta[] = [] - await this.walkAssets('', true, out) - out.sort((a, b) => b.updatedAt - a.updatedAt || a.name.localeCompare(b.name)) - return out + return await this.sharingListings(async () => { + const out: AssetMeta[] = [] + await this.walkAssets('', true, out) + out.sort((a, b) => b.updatedAt - a.updatedAt || a.name.localeCompare(b.name)) + return out + }) } private async walkAssets(dirRel: string, isRoot: boolean, out: AssetMeta[]): Promise { - const entries = await this.fs.readdir(dirRel) + const entries = await this.walkReaddir(dirRel) for (const [index, entry] of entries.entries()) { if (entry.name.startsWith('.')) continue const childRel = dirRel ? `${dirRel}/${entry.name}` : entry.name @@ -1252,15 +1325,43 @@ export class MobileVault { const { excluded, included } = await this.tasksFolderLists() const notes = await this.listNotes() const out: VaultTask[] = [] + const scanned = new Set() for (const note of notes) { if (note.folder === 'trash') continue if (isPathExcludedFromTasks(note.path, excluded, included)) continue if (!isMarkdownPath(note.path)) continue + scanned.add(note.path) + const cached = this.taskCache.get(note.path) + if ( + cached && + cached.mtime === note.updatedAt && + cached.size === note.size && + cached.title === note.title && + cached.folder === note.folder + ) { + out.push(...cached.tasks) + continue + } const body = await this.fs.readTextOrNull(note.path) - if (body === null) continue - out.push( - ...this.parseAllTasks(body, { path: note.path, title: note.title, folder: note.folder }) - ) + if (body === null) { + this.taskCache.delete(note.path) + continue + } + const tasks = this.parseAllTasks(body, { path: note.path, title: note.title, folder: note.folder }) + // A listing without a usable stamp cannot vouch for the next read. + if (Number.isFinite(note.updatedAt) && Number.isFinite(note.size)) { + this.taskCache.set(note.path, { + mtime: note.updatedAt, + size: note.size, + title: note.title, + folder: note.folder, + tasks + }) + } + out.push(...tasks) + } + for (const path of this.taskCache.keys()) { + if (!scanned.has(path)) this.taskCache.delete(path) } return out } diff --git a/src/bridge/vault-walks.test.ts b/src/bridge/vault-walks.test.ts new file mode 100644 index 0000000..54d5830 --- /dev/null +++ b/src/bridge/vault-walks.test.ts @@ -0,0 +1,158 @@ +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { loadMobileModule } from '../../tooling/load-mobile-module.ts' + +// app-core asks for notes and folders together on every refresh, with two +// refreshes at once when a note is created or the app comes back, so one New +// note listed the tree four times over. On Android, where a picked folder +// answers every listing with a document-provider query, that took 12 to 15 +// seconds on a budget phone (zennotesandroid#128), and these walks are the +// same code. Overlapping walks now share their listings, a write the vault +// makes never lets a listing taken before it be reused, and the task scan +// reads only notes whose listing changed. + +Object.defineProperty(globalThis, 'localStorage', { configurable: true, value: { getItem: () => null } }) +const { MobileVault } = await loadMobileModule(['./src/bridge/vault-fs']) + +interface MemoryFile { body: string; mtime: number } + +/** The NativeFs surface the walks use, counting listings and reads. Writes + * move the write generation the way NativeFs does. */ +class MemoryFs { + files = new Map() + listings = new Map() + reads = new Map() + writeGeneration = 0 + clock = 1_000 + + put(path: string, body: string): void { + this.files.set(path, { body, mtime: (this.clock += 1) }) + } + private isDirectory(path: string): boolean { + return path === '' || [...this.files.keys()].some((key) => key.startsWith(`${path}/`)) + } + async readdir(path: string) { + this.listings.set(path, (this.listings.get(path) ?? 0) + 1) + const prefix = path ? `${path}/` : '' + const entries = new Map() + for (const [key, file] of this.files) { + if (!key.startsWith(prefix)) continue + const [name, ...nested] = key.slice(prefix.length).split('/') + entries.set(name!, nested.length + ? { name: name!, type: 'directory', size: 0, mtime: 0 } + : { name: name!, type: 'file', size: file.body.length, mtime: file.mtime }) + } + return [...entries.values()] + } + async statOrNull(path: string) { + const file = this.files.get(path) + if (file) return { type: 'file', size: file.body.length, mtime: file.mtime, ctime: null, uri: path } + return this.isDirectory(path) ? { type: 'directory', size: 0, mtime: 0, ctime: null, uri: path } : null + } + async readTextOrNull(path: string) { + this.reads.set(path, (this.reads.get(path) ?? 0) + 1) + return this.files.get(path)?.body ?? null + } + async writeText(path: string, body: string) { + this.writeGeneration += 1 + this.put(path, body) + this.writeGeneration += 1 + } + listingsOf(path: string): number { + return this.listings.get(path) ?? 0 + } + totalReads(): number { + // Notes only: vault.json and the meta cache are read on their own terms. + return [...this.reads.entries()].filter(([path]) => path.endsWith('.md')).reduce((n, [, count]) => n + count, 0) + } +} + +function fixture() { + const fs = new MemoryFs() + fs.put('.zennotes/vault.json', JSON.stringify({ primaryNotesLocation: 'inbox' })) + fs.put('inbox/Plan.md', '# Plan\n\n- [ ] write the plan\n') + fs.put('inbox/Projects/Site.md', '# Site\n\n- [ ] ship the site\n') + fs.put('inbox/Projects/Notes.md', '# Notes\n\nno tasks here\n') + fs.put('quick/Quick Note.md', '# Quick Note\n\n- [ ] call back\n') + fs.put('archive/Old.md', '# Old\n\n- [x] done long ago\n') + fs.put('trash/Gone.md', '# Gone\n\n- [ ] in the trash\n') + fs.put('attachements/photo.png', 'png') + const vault = new MobileVault('Fixture') + Object.assign(vault, { fs }) + return { fs, vault } +} + +const paths = (notes: { path: string }[]): string[] => notes.map((note) => note.path).sort() + +test('a refresh lists each folder once while notes, folders and assets are walked together', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }) + const { fs, vault } = fixture() + const [notes, folders, assets] = await Promise.all([ + vault.listNotes(), vault.listFolders(), vault.listAssets(), + // The second refresh a new note starts, overlapping the first. + vault.listNotes(), vault.listFolders() + ]) + assert.deepEqual(paths(notes), [ + 'archive/Old.md', 'inbox/Plan.md', 'inbox/Projects/Notes.md', 'inbox/Projects/Site.md', + 'quick/Quick Note.md', 'trash/Gone.md' + ]) + assert.deepEqual(folders.map((f: { subpath: string }) => f.subpath), ['Projects']) + assert.deepEqual(assets.map((a: { path: string }) => a.path), ['attachements/photo.png']) + for (const dir of ['inbox', 'inbox/Projects', 'quick', 'archive', 'trash']) { + assert.equal(fs.listingsOf(dir), 1, `${dir} listed once`) + } +}) + +test('a write the vault makes is never hidden by a listing taken before it', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }) + const { fs, vault } = fixture() + await vault.listNotes() + await vault.writeNote('inbox/Fresh.md', '# Fresh\n') + // Within the grace window, right after the write: the note is listed. + assert.ok(paths(await vault.listNotes()).includes('inbox/Fresh.md')) + assert.equal(fs.listingsOf('inbox/Projects'), 2) +}) + +test('a walk after the grace window lists afresh, so a rescan sees outside changes', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }) + const { fs, vault } = fixture() + await vault.listNotes() + // Syncthing drops a note in; the vault made no write. + fs.put('inbox/From Desktop.md', '# From Desktop\n') + assert.equal(paths(await vault.listNotes()).includes('inbox/From Desktop.md'), false) + t.mock.timers.tick(1000) + assert.equal(paths(await vault.listNotes()).includes('inbox/From Desktop.md'), true) +}) + +test('the task scan reads a note again only when its listing changed', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }) + const { fs, vault } = fixture() + const titles = async (): Promise => + (await vault.scanTasks()).map((task: { content: string }) => task.content).sort() + + assert.deepEqual(await titles(), ['call back', 'done long ago', 'ship the site', 'write the plan']) + const firstReads = fs.totalReads() + + t.mock.timers.tick(1000) + assert.deepEqual(await titles(), ['call back', 'done long ago', 'ship the site', 'write the plan']) + assert.equal(fs.totalReads(), firstReads, 'an unchanged vault is not read again') + + // Edited elsewhere: new mtime and size in the listing. + fs.put('inbox/Projects/Site.md', '# Site\n\n- [ ] ship it today\n') + const readsBefore = new Map(fs.reads) + t.mock.timers.tick(1000) + assert.deepEqual(await titles(), ['call back', 'done long ago', 'ship it today', 'write the plan']) + const readAgain = [...fs.reads].filter(([path, n]) => path.endsWith('.md') && n !== readsBefore.get(path)) + assert.deepEqual(readAgain.map(([path]) => path), ['inbox/Projects/Site.md']) +}) + +test('a note the app writes is read again by the next task scan', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }) + const { fs, vault } = fixture() + await vault.scanTasks() + const before = fs.reads.get('inbox/Plan.md') ?? 0 + await vault.writeNote('inbox/Plan.md', '# Plan\n\n- [x] write the plan\n') + const plan = (await vault.scanTasks()).filter((task: { sourcePath: string }) => task.sourcePath === 'inbox/Plan.md') + assert.deepEqual(plan.map((task: { checked: boolean }) => task.checked), [true]) + assert.equal((fs.reads.get('inbox/Plan.md') ?? 0) > before, true) +})