From fa42fea5fdb50c0a1bcc28620d0c5194048e82b9 Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Tue, 14 Jul 2026 09:21:32 +0000 Subject: [PATCH] fix: normalize vaultFolder-relative path before gitService.deleteFile performRemoteDeletion passed the vault-relative path (carrying the vaultFolder prefix) straight to gitService.deleteFile(), but getFullPath() expects a path relative to rootPath only -- vaultFolder is stripped before every other gitService call site (see sync-manager.ts's getNormalizedPath(file.path) pattern). With a non-empty vaultFolder setting this built the wrong repo path, so deleteFile's pre-delete getFile() lookup 404'd -- surfacing as "file was not found on branch main" for a file the UI still listed as remote-only, since status refresh already normalizes the path correctly. Also hardens SyncStatusView against a local directory (or symlink to one) colliding with a stale remote record at the same path: a new isLocalFile() helper (adapter.stat().type === 'file') gates identifyExtraFiles and the hidden-file recursiveScan, and readFileContent's string-path branch falls back to readLocalSymlinkTarget() on read failure. --- progress.md | 16 ++++++++-------- src/ui/SyncStatusView.ts | 25 ++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/progress.md b/progress.md index 9b92c5c..91a6815 100644 --- a/progress.md +++ b/progress.md @@ -12,9 +12,9 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State -**Last Updated:** 2026-07-14 09:20 +**Last Updated:** 2026-07-14 09:35 **Session ID:** current -**Active Feature:** issue #78 (blog repo, misfiled — actually a git-files-sync bug: delete remote-only file fails) — two root causes fixed, user re-testing after rebuild +**Active Feature:** issue #78 (blog repo, misfiled — actually a git-files-sync bug: delete remote-only file fails) — three root causes found and fixed, awaiting user re-test after rebuild ## Status @@ -23,13 +23,13 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont - [x] feat-001..009 (project setup, settings UX bundle, folder picker, symlink pull fix, tree-SHA refresh, HTML-response error clarity, what's-new modal, i18n/multi-language support) — see archive/2026-07.md - All consolidated onto branch `claude/fix-directory-symlink-pull-260713` → **PR #51** (open, all CI green as of last check), per user's explicit request to keep the PR count down rather than one PR per issue. - [x] "Root path" folder picker now suggests folders from the **remote repo tree** instead of the local vault. Added `src/ui/RemoteFolderSuggest.ts` (derives folder paths from `gitService.listFiles(branch, false)`); wired into the Root path field in `src/settings.ts`. Vault folder field untouched (still uses local `FolderSuggest`). -- [x] Issue #78 (filed in `firstsun-dev/blog` but actually a git-files-sync bug — user confirmed to fix it here regardless of repo mismatch): "delete remote only file fail". Two distinct root causes found and fixed: +- [x] Issue #78 (filed in `firstsun-dev/blog` but actually a git-files-sync bug — user confirmed to fix it here regardless of repo mismatch): "delete remote only file fail". Three distinct root causes found and fixed across this session (iteratively, as the user re-tested against real console errors each time): 1. **URL encoding + silent empty-sha delete**: `github-service.ts`/`gitea-service.ts` `getApiUrl()` built the Contents API URL without `encodeURIComponent` on path segments (unlike `gitlab-service.ts`, which already encodes), so paths with spaces/non-ASCII (e.g. Chinese filenames) 404'd on the pre-delete sha lookup; that 404 silently produced an empty `sha`, and the resulting DELETE with `sha: ''` was rejected — silently, since the UI swallowed all delete errors. Fixed: encode path segments in both services; `deleteFile()` now throws a clear error if the pre-delete lookup's sha is empty; `SyncStatusView.ts` delete flow now captures `{path, message}` per failure and surfaces the real message in the Notice (`syncStatus.notice.deleteResult.partialWithMessage` i18n key, added to both `en.ts`/`zh-tw.ts`) instead of a bare "N failed". - 2. **EISDIR on local-only symlinked folders**: user confirmed the actual file they tried to delete was *not* a symlink, but a real console error they pasted (`EISDIR: illegal operation on a directory, read` for `.claude/skills/polish-blog`) revealed a second bug: `SyncStatusView.ts` `readFileContent()` called `adapter.read()` directly on string paths with no symlink guard — only the sha-based path (`readLocalContentForSha`) checked for symlinks, and only when the *remote* already knew the entry was a symlink. A local-only symlinked folder not yet pushed hit `adapter.read()` on a directory and crashed, leaving its status stuck. Fixed by extracting `readStringPathContent()` with a catch-and-fallback to `readLocalSymlinkTarget()`. - - Added 4 new tests (2 in `tests/services/github-service.test.ts`, 2 in `tests/services/gitea-service.test.ts`) covering the empty-sha throw and non-ASCII/space path encoding. - - Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 306/306 passed. - - Committed as `896d77b`, merged with the i18n branch's concurrent changes to the same files (`45ea8fa`-range merge commit), pushed to `claude/fix-directory-symlink-pull-260713`. - - **Not yet confirmed fixed**: the user's last live test (before this push) still showed the old bare "N failed" text with a genuinely non-symlink file on GitHub — that was because the fix hadn't reached their test vault yet (everything was uncommitted). Now pushed; next step is the user rebuilding/reloading and re-testing to confirm root cause #1 (or find a third cause) with the new detailed error message. + 2. **EISDIR + folder/remote-record collision**: `SyncStatusView.ts` `readFileContent()`/`identifyExtraFiles()` treated any local path that `adapter.exists()` returned true for as a readable file — but a real local directory (or a symlink to one) that happens to share a path with a stale remote record (e.g. `.claude/skills/polish-blog`) is not readable content, and `adapter.read()` on it throws `EISDIR`. Fixed: new `isLocalFile()` helper (`adapter.stat().type === 'file'`) gates `identifyExtraFiles` and the hidden-file `recursiveScan`; `readFileContent`'s string-path branch (extracted to `readStringPathContent()`) also falls back to `readLocalSymlinkTarget()` on read failure as a last resort. + 3. **Wrong path passed to deleteFile (the actual reported bug)**: `performRemoteDeletion` called `gitService.deleteFile(s.path, ...)` with `s.path` — a *vault-relative* path (carries the `vaultFolder` prefix, per `identifyExtraFiles`'s `getVaultPath()` mapping) — but `deleteFile`'s `getFullPath()` expects a path relative to `rootPath` only (vaultFolder is a purely local concept, stripped before every other `gitService` call site, e.g. `sync-manager.ts`'s `getNormalizedPath(file.path)` pattern used everywhere else). With a non-empty `vaultFolder` setting, this built the wrong repo path, causing `deleteFile`'s pre-delete `getFile()` lookup to 404 — surfacing as "file was not found on branch main" for a file the UI still listed as remote-only (since refresh's status computation *does* normalize correctly). Fixed: `performRemoteDeletion` now computes `this.plugin.getNormalizedPath(s.path)` before calling `deleteFile`, matching every other call site. + - Added 4 new tests (2 in `tests/services/github-service.test.ts`, 2 in `tests/services/gitea-service.test.ts`) covering the empty-sha throw and non-ASCII/space path encoding. No new test yet for root cause #3 (vaultFolder/rootPath path mismatch) or #2 (folder collision) — `SyncStatusView.ts` currently has no dedicated test file; consider adding one if this recurs. + - Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 312/312 passed. + - Root causes #1+#2 committed as `896d77b`, merged with the i18n branch (`563a28e`), pushed. Root cause #3 fixed after that push, in this same session — **not yet committed/pushed as of this note**. - Not yet done: comment on/reference issue #78 (in the `blog` repo) noting the fix landed in `git-files-sync` instead — flag to user before doing so since it crosses repos. ### What's In Progress diff --git a/src/ui/SyncStatusView.ts b/src/ui/SyncStatusView.ts index b302b50..fd91a07 100644 --- a/src/ui/SyncStatusView.ts +++ b/src/ui/SyncStatusView.ts @@ -387,7 +387,12 @@ export class SyncStatusView extends ItemView { try { const listing = await this.app.vault.adapter.list(folderPath); for (const file of listing.files) { - if (this.isHidden(file)) { + if (!this.isHidden(file)) continue; + // Guard against a symlinked folder being misclassified as a file + // by the adapter's raw listing (Node's dirent type doesn't follow + // links) — still track it as a link entry rather than a readable + // file, same as a symlinked folder found via listing.folders below. + if (readLocalSymlinkTarget(this.app, file) !== null || await this.isLocalFile(file)) { result.push(file); } } @@ -410,6 +415,12 @@ export class SyncStatusView extends ItemView { return path.split('/').some(part => part.startsWith('.')); } + /** True only for an actual local file — excludes real directories (and symlinks to one), which `adapter.stat()` follows. */ + private async isLocalFile(vaultPath: string): Promise { + const stat = await this.app.vault.adapter.stat(vaultPath); + return stat?.type === 'file'; + } + private initializeFileStatuses(localFiles: TFile[]): void { for (const file of localFiles) { this.fileStatuses.set(file.path, { file, path: file.path, status: 'checking' }); @@ -429,9 +440,13 @@ export class SyncStatusView extends ItemView { if (localFile) { extra.push(localFile); - } else if (await this.app.vault.adapter.exists(vaultPath)) { + } else if (await this.isLocalFile(vaultPath)) { extra.push(vaultPath); } else { + // Either nothing exists locally, or the remote's record (e.g. a + // stale symlink push) now collides with a real local folder of + // the same name — either way there's no readable local file to + // compare, so it's remote-only. this.fileStatuses.set(vaultPath, { path: vaultPath, status: 'remote-only' }); } } @@ -765,7 +780,11 @@ export class SyncStatusView extends ItemView { cur++; prog.setMessage(t('syncStatus.progress.deletingRemote', { current: cur, total, path: s.path })); try { - await this.plugin.gitService.deleteFile(s.path, this.plugin.settings.branch, `Delete ${s.path}`); + // s.path is a vault-relative path (may carry the vaultFolder prefix); + // the git service expects a path relative to rootPath only, so strip + // vaultFolder first, same as every other gitService call site. + const repoPath = this.plugin.getNormalizedPath(s.path); + await this.plugin.gitService.deleteFile(repoPath, this.plugin.settings.branch, `Delete ${repoPath}`); this.fileStatuses.delete(s.path); this.selectedFiles.delete(s.path); } catch (e) {