Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/__tests__/attachmentSyncTimeoutRetry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,33 @@ describe('syncToGitHub — attachment push survives a stalled IndexedDB read', (
expect(outcome2.result.attachmentSyncSkipped).toBeFalsy()
})

// Attachments now stream in AFTER the sync (fillAttachmentsInBackground), so
// a push can fire while a fill is half done. Neither half may confuse the
// push: a banked image has localSha === the remote blob sha it came from, so
// 3b skips it, and one still in flight simply isn't in IDB yet (absence is
// never a delete — only an explicit tombstone is).
test('a background-fetched attachment is not re-uploaded, while a genuinely local one still is', async () => {
mockAttachmentState.paths = ['Files/fetched.png', 'Files/mine.png']
// Banked by the background fill: its sha is the remote blob's sha.
mockAttachmentState.shaByPath.set('Files/fetched.png', 'remote-blob-sha')
mockAttachmentState.blobByPath.set('Files/fetched.png', new Blob([new Uint8Array([1])], { type: 'image/png' }))
// Created locally by the user: not on the remote at all.
mockAttachmentState.shaByPath.set('Files/mine.png', 'local-only-sha')
mockAttachmentState.blobByPath.set('Files/mine.png', new Blob([new Uint8Array([2])], { type: 'image/png' }))
mockGetTreeMap.mockResolvedValue(new Map([['Files/fetched.png', 'remote-blob-sha']]))

const real = note({ id: 'n1', title: 'Real note', content: 'hello\n' })
await syncToGitHub({ provider: new GitHubProvider('tok'), repo: REPO, notes: [real], folders: [] })

const paths = postedTreeEntries().map(e => e.path)
expect(paths).not.toContain('Files/fetched.png')
expect(paths).toContain('Files/mine.png')
// An image the fill has NOT reached yet is absent from IDB — it must not
// be pushed as a deletion.
expect(paths).not.toContain('Files/notyet.png')
expect(mockCreateBlobBinary).toHaveBeenCalledTimes(1)
})

test('tombstones are also left unconsumed this cycle (3c skipped alongside 3b)', async () => {
mockAttachmentState.listTimesOut = true
const real = note({ id: 'n1', title: 'Real note', content: 'hello\n' })
Expand Down
99 changes: 99 additions & 0 deletions src/__tests__/syncApply.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,105 @@ test('applyAttachmentClassifications: a single failed fetch is counted, not thro
spy.mockRestore()
})

// Regression (83 MiB / 175-image vault, 08/09/2026): the apply used to await
// the ENTIRE fetch batch before writing anything to IDB, so a watchdog abort
// mid-batch banked zero attachments and every retry re-classified all 175 as
// `attachmentCreated` and restarted from nothing — a permanent "Syncing…".
// Each blob must now be persisted as it lands, and a caller abort must reject
// the batch instead of being swallowed as N per-file failures.
test('applyAttachmentClassifications: an AbortError mid-batch keeps the already-fetched blobs and propagates', async () => {
useGitHubStore.setState({ token: 'tok', syncRepo: REPO })
mockGetBlobBytes.mockImplementation(async (..._a: unknown[]) => {
const sha = _a[3] as string
if (sha === 'sha-b') {
// Macrotask tick: every pending microtask (i.e. a.png's IDB write on the
// fixed code) has drained by the time this rejects.
await new Promise(r => setTimeout(r, 0))
throw Object.assign(new Error('The operation was aborted'), { name: 'AbortError' })
}
return new Uint8Array([1, 2, 3])
})

const classifications: PullClassification[] = [
{ kind: 'attachmentCreated', path: 'attachments/a.png', remoteSha: 'sha-a', mime: 'image/png' },
{ kind: 'attachmentCreated', path: 'attachments/b.png', remoteSha: 'sha-b', mime: 'image/png' },
{ kind: 'attachmentCreated', path: 'attachments/c.png', remoteSha: 'sha-c', mime: 'image/png' },
]

await expect(applyAttachmentClassifications(classifications)).rejects.toMatchObject({
name: 'AbortError',
})
// The first blob is banked, so the next pull classifies it as present.
expect(mockPutAttachmentAtPath.mock.calls.map(call => call[0])).toContain('attachments/a.png')
// ...and the aborted one never is.
expect(mockPutAttachmentAtPath.mock.calls.map(call => call[0])).not.toContain('attachments/b.png')

mockGetBlobBytes.mockReset()
mockPutAttachmentAtPath.mockReset().mockResolvedValue(undefined)
})

// The fill runs in the background now, so it must be cancellable: a newer pull
// (or the page going away) aborts the controller, and the batch has to stop
// fetching while KEEPING what it already banked — that banked set is exactly
// what shrinks the next pull's `attachmentCreated` list.
test('applyAttachmentClassifications: an aborted signal stops the batch and keeps what was banked', async () => {
useGitHubStore.setState({ token: 'tok', syncRepo: REPO })
const controller = new AbortController()
// Cancel the moment the first image is banked.
mockPutAttachmentAtPath.mockImplementation(async (path: string) => {
if (path === 'attachments/a.png') controller.abort()
})
mockGetBlobBytes.mockImplementation(async (..._a: unknown[]) => {
const sha = _a[3] as string
// The signal must actually reach getBlobBytes — that is the wiring this
// asserts. Real githubFetch throws exactly this shape on a caller abort.
const signal = _a[4] as AbortSignal | undefined
if (signal?.aborted) throw Object.assign(new Error('The operation was aborted'), { name: 'AbortError' })
if (sha !== 'sha-a') {
// Park on a macrotask so the abort (a microtask behind a.png's write)
// has landed by the time these resume.
await new Promise(r => setTimeout(r, 0))
if (signal?.aborted) throw Object.assign(new Error('The operation was aborted'), { name: 'AbortError' })
}
return new Uint8Array([1, 2, 3])
})

const classifications: PullClassification[] = [
{ kind: 'attachmentCreated', path: 'attachments/a.png', remoteSha: 'sha-a', mime: 'image/png' },
{ kind: 'attachmentCreated', path: 'attachments/b.png', remoteSha: 'sha-b', mime: 'image/png' },
{ kind: 'attachmentCreated', path: 'attachments/c.png', remoteSha: 'sha-c', mime: 'image/png' },
]

await expect(
applyAttachmentClassifications(classifications, { signal: controller.signal }),
).rejects.toMatchObject({ name: 'AbortError' })

const written = mockPutAttachmentAtPath.mock.calls.map(call => call[0])
expect(written).toEqual(['attachments/a.png'])
// Every getBlobBytes call carried the signal through.
for (const call of mockGetBlobBytes.mock.calls) expect(call[4]).toBe(controller.signal)

mockGetBlobBytes.mockReset()
mockPutAttachmentAtPath.mockReset().mockResolvedValue(undefined)
})

test('applyAttachmentClassifications: reports progress as it banks each image', async () => {
useGitHubStore.setState({ token: 'tok', syncRepo: REPO })
mockGetBlobBytes.mockResolvedValue(new Uint8Array([1]))
const phases: string[] = []

await applyAttachmentClassifications(
[
{ kind: 'attachmentCreated', path: 'attachments/1.png', remoteSha: 's1', mime: 'image/png' },
{ kind: 'attachmentCreated', path: 'attachments/2.png', remoteSha: 's2', mime: 'image/png' },
],
{ onPhase: (m) => phases.push(m) },
)

expect(phases[0]).toBe('Downloading images… (0 / 2)')
expect(phases[phases.length - 1]).toBe('Downloading images… (2 / 2)')
})

test('applyAttachmentClassifications: no attachment classifications → all-zero counts, no fetch', async () => {
const counts = await applyAttachmentClassifications([
{ kind: 'remoteCreated', path: 'X.md', remoteSha: 's', remoteContent: 'x\n', tags: [], body: 'x\n' },
Expand Down
34 changes: 34 additions & 0 deletions src/__tests__/useGitHubSync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,40 @@ beforeEach(() => {
})

describe('useGitHubSync — runPullOnly', () => {
// The 83 MiB / 175-image regression, at the hook level: applying attachments
// used to be awaited INSIDE the watchdog-wrapped sync, so a slow image batch
// timed out the whole sync — notes included. The attachment fill is now fire
// and forget, so the sync must reach its terminal state while the images are
// still downloading. A never-resolving fetch stands in for "83 MiB over a
// slow link"; before the change this test would hit the watchdog instead.
test('resolves to a terminal sync state while the attachment fill is still running', async () => {
let fillStarted = false
applyAttachmentClassificationsMock.mockImplementation(() => {
fillStarted = true
return new Promise(() => {}) // never settles
})
pullFromGitHubMock.mockResolvedValue({
classifications: [
{ kind: 'remoteCreated', path: 'a.md', remoteSha: 'sha1', remoteContent: '', tags: [], body: 'hi' },
{ kind: 'attachmentCreated', path: 'Files/big.png', remoteSha: 'img1', mime: 'image/png' },
],
latestCommitSha: 'commit-sha',
})
applyNonConflictsMock.mockReturnValue({ created: 1, updated: 0, deleted: 0, autoMerged: 0 })

const { result } = renderHook(() => useGitHubSync())
await act(async () => {
await result.current.runPullOnly()
})

expect(fillStarted).toBe(true)
expect(result.current.syncState.kind).toBe('ok')
if (result.current.syncState.kind === 'ok') {
// Queued, not finished — the count comes from the classifications.
expect(result.current.syncState.message).toMatch(/1 image/)
}
})

test('pulls and applies, never pushes', async () => {
pullFromGitHubMock.mockResolvedValue({
classifications: [
Expand Down
68 changes: 39 additions & 29 deletions src/hooks/useGitHubSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ import { syncToGitHub, pullFromGitHub } from '@/utils/githubSync'
import type { PullClassification, SyncResult, GitPathUpdate } from '@/utils/githubSync'
import { makeGitHostProvider } from '@/utils/gitHost'
import { getValidGitHubToken, withTokenRefresh, ReconnectRequiredError } from '@/utils/tokenRefresh'
import { applyNonConflicts, applyAttachmentClassifications } from '@/utils/syncApply'
import { fillShellsInBackground } from '@/utils/backgroundFill'
import { applyNonConflicts } from '@/utils/syncApply'
import { fillShellsInBackground, fillAttachmentsInBackground } from '@/utils/backgroundFill'
import { pendingStoreHydration } from '@/utils/ensureStoresHydrated'
import { switchVault } from '@/utils/switchVault'
import { notesKey } from '@/utils/repoStorage'
import type { ApplyCounts, AttachmentApplyCounts } from '@/utils/syncApply'
import type { ApplyCounts } from '@/utils/syncApply'
import type { ConflictTabData } from '@/stores/workspaceStore'
import type { SyncRepo } from '@/types'
import {
Expand Down Expand Up @@ -207,10 +207,19 @@ async function runPull(
// are skipped here — the caller opens them in the merge UI instead.
async function runApply(
classifications: PullClassification[],
): Promise<{ notes: ApplyCounts; attachments: AttachmentApplyCounts }> {
onPhase?: (msg: string) => void,
): Promise<{ notes: ApplyCounts; attachmentsQueued: number }> {
const notes = await applyNonConflicts(classifications)
const attachments = await applyAttachmentClassifications(classifications)
return { notes, attachments }
// Attachments are NOT awaited: the binary fetch is the one part of an apply
// that can outlast SYNC_WATCHDOG_MS on a big vault, and awaiting it here used
// to time out the entire sync (notes and push included). Kicked off from
// runApply rather than from each of the four call sites so no path can
// forget it. See fillAttachmentsInBackground for the resume story.
void fillAttachmentsInBackground(classifications, onPhase)
const attachmentsQueued = classifications.filter(
c => c.kind === 'attachmentCreated' || c.kind === 'attachmentUpdated',
).length
return { notes, attachmentsQueued }
}

// ── Step 3: PUSH ────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -258,20 +267,20 @@ async function runPush(
// Compose the human-readable status line shown in the sidebar's sync button.
function formatSyncMessage(
pulled: ApplyCounts,
attached: AttachmentApplyCounts,
attachmentsQueued: number,
pushed: SyncResult,
): string {
const totalPulled =
pulled.created + pulled.updated + pulled.deleted +
attached.created + attached.updated
pulled.created + pulled.updated + pulled.deleted + attachmentsQueued
if (pushed.unchanged && totalPulled === 0) return 'Up to date'

const parts: string[] = []
if (pulled.created) parts.push(`↓${pulled.created} new`)
if (pulled.updated) parts.push(`↓${pulled.updated} updated`)
if (pulled.deleted) parts.push(`↓${pulled.deleted} removed`)
const attachTotal = attached.created + attached.updated
if (attachTotal) parts.push(`↓${attachTotal} image${attachTotal === 1 ? '' : 's'}`)
// Queued, not finished: the images stream in behind the sync and report
// their own "Downloading images… (n / m)" progress line.
if (attachmentsQueued) parts.push(`↓${attachmentsQueued} image${attachmentsQueued === 1 ? '' : 's'}`)
if (pushed.created) parts.push(`↑${pushed.created} new`)
if (pushed.updated) parts.push(`↑${pushed.updated} updated`)
if (pushed.deleted) parts.push(`↑${pushed.deleted} deleted`)
Expand Down Expand Up @@ -299,19 +308,17 @@ function addSyncToast(toast: Omit<Toast, 'id' | 'source'>): void {
// pretending we uploaded anything.
function formatPullMessage(
pulled: ApplyCounts,
attached: AttachmentApplyCounts,
attachmentsQueued: number,
): string {
const totalPulled =
pulled.created + pulled.updated + pulled.deleted +
attached.created + attached.updated
pulled.created + pulled.updated + pulled.deleted + attachmentsQueued
if (totalPulled === 0) return 'Up to date'

const parts: string[] = []
if (pulled.created) parts.push(`↓${pulled.created} new`)
if (pulled.updated) parts.push(`↓${pulled.updated} updated`)
if (pulled.deleted) parts.push(`↓${pulled.deleted} removed`)
const attachTotal = attached.created + attached.updated
if (attachTotal) parts.push(`↓${attachTotal} image${attachTotal === 1 ? '' : 's'}`)
if (attachmentsQueued) parts.push(`↓${attachmentsQueued} image${attachmentsQueued === 1 ? '' : 's'}`)
if (pulled.autoMerged) parts.push(`auto-merged ${pulled.autoMerged}`)
return `Pulled ${parts.join(' · ')}`
}
Expand All @@ -334,6 +341,14 @@ export function useGitHubSync(): UseGitHubSyncResult {

const [syncState, setSyncState] = useState<SyncState>({ kind: 'idle' })

// Progress line for the two fire-and-forget fills (note bodies, attachments).
// Only surfaces when nothing more important is showing, so a background fill
// can never overwrite a real sync status or an error — the status bar reads
// "Synced" for the notes while the images are still coming down.
const backgroundPhase = useCallback((msg: string) => {
setSyncState((prev) => (prev.kind === 'idle' ? { kind: 'running', message: msg } : prev))
}, [])

// Defensive: clear any leftover `isSyncing: true` from a sync that never
// reached its finally block (e.g. tab crash mid-pull, unmount during
// setState). Without this, a wedged flag would silently kill every
Expand Down Expand Up @@ -401,7 +416,7 @@ export function useGitHubSync(): UseGitHubSyncResult {
// Apply everything that isn't in conflict; leave push for the user
// to retry after they resolve the merge tabs.
setSyncState({ kind: 'running', message: 'Applying changes…' })
await runApply(classifications)
await runApply(classifications, backgroundPhase)
if (conflicts.length >= BATCH_THRESHOLD) {
openMergeBatch(conflicts)
} else {
Expand All @@ -414,18 +429,15 @@ export function useGitHubSync(): UseGitHubSyncResult {
}

setSyncState({ kind: 'running', message: 'Applying changes…' })
const { notes: pullCounts, attachments: attachCounts } = await runApply(classifications)
const { notes: pullCounts, attachmentsQueued } = await runApply(classifications, backgroundPhase)

// progressive-clone: stream shell bodies in the background. Fire AND
// FORGET — we don't await, so the push below and the success toast
// happen immediately while bodies fill in. The push excludes shells
// (syncToGitHub drops contentLoaded===false), so an in-flight fill can
// never race the push into an empty-body overwrite. Resumes on reload
// via the startup kick-off in useAutoSync.
void fillShellsInBackground((msg) => {
// Only surface fill progress when nothing more important is showing.
setSyncState((prev) => (prev.kind === 'idle' ? { kind: 'running', message: msg } : prev))
})
void fillShellsInBackground(backgroundPhase)

// AI commit messages: when the user has opted in AND didn't
// pass a custom message via the SCM input, ask the model to
Expand Down Expand Up @@ -471,7 +483,7 @@ export function useGitHubSync(): UseGitHubSyncResult {
}
recordSync(result.commitSha)

const okMessage = formatSyncMessage(pullCounts, attachCounts, result)
const okMessage = formatSyncMessage(pullCounts, attachmentsQueued, result)
setSyncState({
kind: 'ok',
message: okMessage,
Expand Down Expand Up @@ -587,7 +599,7 @@ export function useGitHubSync(): UseGitHubSyncResult {
// isn't in conflict, open merge tabs (batch view above
// BATCH_THRESHOLD) for the user to resolve.
setSyncState({ kind: 'running', message: 'Applying changes…' })
await runApply(classifications)
await runApply(classifications, backgroundPhase)
if (conflicts.length >= BATCH_THRESHOLD) {
openMergeBatch(conflicts)
} else {
Expand All @@ -600,14 +612,12 @@ export function useGitHubSync(): UseGitHubSyncResult {
}

setSyncState({ kind: 'running', message: 'Applying changes…' })
const { notes: pullCounts, attachments: attachCounts } = await runApply(classifications)
const { notes: pullCounts, attachmentsQueued } = await runApply(classifications, backgroundPhase)

// progressive-clone: stream shell bodies in the background (fire and
// forget). See runSync for the full rationale — pull-only never pushes,
// so there's no race to worry about here at all.
void fillShellsInBackground((msg) => {
setSyncState((prev) => (prev.kind === 'idle' ? { kind: 'running', message: msg } : prev))
})
void fillShellsInBackground(backgroundPhase)

// Record the pulled HEAD as the new baseline so lastCommitSha tracks
// the remote after a pull-only too. Previously only runSync called
Expand All @@ -616,7 +626,7 @@ export function useGitHubSync(): UseGitHubSyncResult {
// refetch both key off it.
recordSync(latestCommitSha)

const okMessage = formatPullMessage(pullCounts, attachCounts)
const okMessage = formatPullMessage(pullCounts, attachmentsQueued)
setSyncState({
kind: 'ok',
message: okMessage,
Expand Down
Loading
Loading