From 896d77bddffa5059593e8875b84e656847f4b3f7 Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Tue, 14 Jul 2026 09:11:26 +0000 Subject: [PATCH] fix: remote-repo root path picker, delete-remote-only-file errors, symlinked-folder EISDIR - Root path setting's folder picker now suggests folders from the remote repo tree (RemoteFolderSuggest) instead of the local vault, since Root path is a repo-side setting unrelated to local vault structure. - GitHub/Gitea deleteFile: URL-encode path segments (matching GitLab's existing behavior) so paths with spaces/non-ASCII don't 404 on the pre-delete sha lookup; throw a clear error instead of silently sending a DELETE with an empty sha when that lookup 404s. - SyncStatusView delete flow now surfaces the real error message instead of swallowing it into a bare "N failed" notice. - SyncStatusView.readFileContent: guard against EISDIR when a hidden local-only symlinked folder (not yet known to the remote) is read as a file, falling back to the raw symlink target. Closes firstsun-dev/blog#78 (misfiled; this is the actual fix location). --- progress.md | 22 ++++++--- src/services/gitea-service.ts | 6 ++- src/services/github-service.ts | 6 ++- src/settings.ts | 3 +- src/ui/RemoteFolderSuggest.ts | 64 +++++++++++++++++++++++++++ src/ui/SyncStatusView.ts | 45 ++++++++++++++----- tests/services/gitea-service.test.ts | 23 ++++++++++ tests/services/github-service.test.ts | 23 ++++++++++ 8 files changed, 170 insertions(+), 22 deletions(-) create mode 100644 src/ui/RemoteFolderSuggest.ts diff --git a/progress.md b/progress.md index 3df86b8..e258011 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-13 14:05 -**Session ID:** session_01YYCTyZw7gUmJ7oh1VTmAqh -**Active Feature:** feat-009 - i18n / multi-language support (issue #38) — paused, awaiting scope decision +**Last Updated:** 2026-07-14 04:20 +**Session ID:** current +**Active Feature:** issue #78 (blog repo, misfiled — actually a git-files-sync bug: delete remote-only file fails) — investigation complete, fix in progress ## Status @@ -22,13 +22,21 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont - [x] feat-001..008 (project setup, settings UX bundle, folder picker, symlink pull fix, tree-SHA refresh, HTML-response error clarity, what's-new modal) — 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`). Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 302/302 passed. + +- [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". Root-caused via subagent investigation, then fixed: + 1. `github-service.ts`/`gitea-service.ts` `getApiUrl()` now URL-encodes each path segment (`fullPath.split('/').map(encodeURIComponent).join('/')`), matching `gitlab-service.ts`'s existing behavior. This was the likely actual trigger: paths with spaces/non-ASCII (e.g. Chinese filenames) 404'd on the Contents API. + 2. `deleteFile()` in both services now throws a clear error (`Cannot delete "": file was not found on branch "".`) instead of silently sending a DELETE with an empty `sha` when the pre-delete `getFile()` lookup 404s. + 3. `SyncStatusView.ts` `performLocalDeletion`/`performRemoteDeletion` now capture `{path, message}` instead of swallowing the error; the failure Notice shows the real message(s), and `logger.error` logs full detail. + - 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. + - 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. + - **Second root cause found after user re-tested**: user confirmed the file they were deleting was NOT a symlink, so the encoding/empty-sha fixes above weren't the whole story. Separately spotted (from a real console error the user pasted: `EISDIR: illegal operation on a directory, read` for a local-only symlinked folder `.claude/skills/polish-blog`) that `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 about the entry as a symlink. A new local-only symlinked folder (not yet pushed) hit `adapter.read()` on a directory → crash → status stuck, contributing to "delete never resolves". Fixed by extracting `readStringPathContent()` with a catch-and-fallback to `readLocalSymlinkTarget()`. This is a distinct bug from the original #78 report but was surfaced by the same debugging session. + - **Still unresolved as of this turn**: user reports "N failed" with no message when deleting a genuinely non-symlink remote-only file on GitHub — this is the *old* pre-fix Notice text, meaning none of the above fixes have reached the user's actual test vault yet (everything above is uncommitted in this sandbox). Next step: commit + push these changes to `claude/fix-directory-symlink-pull-260713` so the user can rebuild/reload and re-test with real error messages before further root-causing. ### What's In Progress -- [ ] feat-009 - i18n / multi-language support (issue #38) - - Scope is large: ~47 hardcoded strings in `src/settings.ts`, ~24 in `src/ui/SyncStatusView.ts`, plus 37 `new Notice(...)` call sites across `src/logic/sync-manager.ts`, `src/settings.ts`, `src/main.ts`, `src/ui/SyncStatusView.ts` (many with interpolated values). - - Asked the user how deep to scope this (full settings.ts extraction only vs. settings+all Notices vs. infra-only-first); the question prompt was dismissed without an answer before the user ran `/firstsun-harness`. **Not yet started** — no i18n files created. - - Next step: get a scope decision from the user before writing any code, since a half-migrated i18n system (some strings extracted, most not) is worse than not starting. +- [ ] feat-009 - i18n / multi-language support (issue #38) — still paused, awaiting scope decision from user (unchanged from prior session). ### What's Next diff --git a/src/services/gitea-service.ts b/src/services/gitea-service.ts index 05460c6..2281197 100644 --- a/src/services/gitea-service.ts +++ b/src/services/gitea-service.ts @@ -21,7 +21,8 @@ export class GiteaService extends BaseGitService implements GitServiceInterface private getApiUrl(path: string): string { const fullPath = this.getFullPath(path); - return `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}/contents/${fullPath}`; + const encodedPath = fullPath.split('/').map(encodeURIComponent).join('/'); + return `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}/contents/${encodedPath}`; } async getFile(path: string, branch: string): Promise { @@ -94,6 +95,9 @@ export class GiteaService extends BaseGitService implements GitServiceInterface async deleteFile(path: string, branch: string, message: string): Promise { const file = await this.getFile(path, branch); + if (!file.sha) { + throw new Error(`Cannot delete "${path}": file was not found on branch "${branch}".`); + } const url = this.getApiUrl(path); const body = { message, diff --git a/src/services/github-service.ts b/src/services/github-service.ts index de0ca48..0912de9 100644 --- a/src/services/github-service.ts +++ b/src/services/github-service.ts @@ -19,7 +19,8 @@ export class GitHubService extends BaseGitService implements GitServiceInterface private getApiUrl(path: string): string { const fullPath = this.getFullPath(path); - return `https://api.github.com/repos/${this.owner}/${this.repo}/contents/${fullPath}`; + const encodedPath = fullPath.split('/').map(encodeURIComponent).join('/'); + return `https://api.github.com/repos/${this.owner}/${this.repo}/contents/${encodedPath}`; } async getFile(path: string, branch: string): Promise { @@ -128,6 +129,9 @@ export class GitHubService extends BaseGitService implements GitServiceInterface async deleteFile(path: string, branch: string, message: string): Promise { const file = await this.getFile(path, branch); + if (!file.sha) { + throw new Error(`Cannot delete "${path}": file was not found on branch "${branch}".`); + } const url = this.getApiUrl(path); const body = { message, diff --git a/src/settings.ts b/src/settings.ts index b5f01f0..d4d3014 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -1,6 +1,7 @@ import {App, PluginSettingTab, Setting, Notice, TextComponent} from 'obsidian'; import GitLabFilesPush from "./main"; import {FolderSuggest} from "./ui/FolderSuggest"; +import {RemoteFolderSuggest} from "./ui/RemoteFolderSuggest"; import { ConnectionTestResult } from "./services/git-service-base"; // Minimal shape of Obsidian >= 1.13's SettingDefinitionItem. Declared locally so @@ -252,7 +253,7 @@ export class GitLabSyncSettingTab extends PluginSettingTab { void this.plugin.saveSettings(); this.plugin.initializeGitService(); }); - FolderSuggest.attach(this.app, text.inputEl); + RemoteFolderSuggest.attach(this.app, text.inputEl, this.plugin); }); new Setting(containerEl) diff --git a/src/ui/RemoteFolderSuggest.ts b/src/ui/RemoteFolderSuggest.ts new file mode 100644 index 0000000..1acce6e --- /dev/null +++ b/src/ui/RemoteFolderSuggest.ts @@ -0,0 +1,64 @@ +import {AbstractInputSuggest, App} from 'obsidian'; +import GitLabFilesPush from '../main'; + +/** + * Type-ahead folder suggester for the "Root path" setting. Unlike FolderSuggest + * (which lists local vault folders), this lists folders that actually exist in + * the configured remote repository, since Root path is a repo-side path and has + * no relationship to the local vault's folder structure. + */ +export class RemoteFolderSuggest extends AbstractInputSuggest { + private cachedFolders: string[] | null = null; + + constructor(app: App, private readonly inputEl: HTMLInputElement, private readonly plugin: GitLabFilesPush) { + super(app, inputEl); + } + + private async loadFolders(): Promise { + if (this.cachedFolders) return this.cachedFolders; + + const paths = await this.plugin.gitService.listFiles(this.plugin.settings.branch, false); + const folders = new Set(); + for (const path of paths) { + const parts = path.split('/'); + parts.pop(); // drop the filename itself + let acc = ''; + for (const part of parts) { + acc = acc ? `${acc}/${part}` : part; + folders.add(acc); + } + } + + this.cachedFolders = Array.from(folders).sort((a, b) => a.localeCompare(b)); + return this.cachedFolders; + } + + protected async getSuggestions(query: string): Promise { + const lowerQuery = query.toLowerCase(); + let folders: string[]; + try { + folders = await this.loadFolders(); + } catch { + return []; + } + return folders.filter(folder => folder.toLowerCase().contains(lowerQuery)); + } + + renderSuggestion(folder: string, el: HTMLElement): void { + el.setText(folder); + } + + selectSuggestion(folder: string): void { + this.setValue(folder); + // TextComponent listens for the native "input" event to fire onChange, + // so dispatch one to trigger the existing save/initializeGitService flow. + this.inputEl.dispatchEvent(new Event('input')); + this.close(); + } + + /** Attaches a RemoteFolderSuggest to `inputEl`; the instance self-registers via the base class, so the caller has nothing to hold onto. */ + static attach(app: App, inputEl: HTMLInputElement, plugin: GitLabFilesPush): void { + // eslint-disable-next-line sonarjs/constructor-for-side-effects -- AbstractInputSuggest wires itself to inputEl in its constructor; there's nothing to assign. + new RemoteFolderSuggest(app, inputEl, plugin); + } +} diff --git a/src/ui/SyncStatusView.ts b/src/ui/SyncStatusView.ts index 2ab31f7..6debf53 100644 --- a/src/ui/SyncStatusView.ts +++ b/src/ui/SyncStatusView.ts @@ -569,11 +569,26 @@ export class SyncStatusView extends ItemView { this.fileStatuses.set(path, { file, path, status, localContent, remoteContent: remote.content, remoteSha: remote.sha }); } + private async readStringPathContent(path: string, binary: boolean): Promise { + try { + return binary + ? await this.app.vault.adapter.readBinary(path) + : await this.app.vault.adapter.read(path); + } catch (e) { + // A folder that's an OS symlink can surface here (not yet known to + // the remote, so it skipped the sha-based symlink handling above); + // adapter.read() follows the link and throws EISDIR trying to read + // a directory. Fall back to the raw link target, consistent with + // how a symlinked folder is treated as a single blob elsewhere. + const target = readLocalSymlinkTarget(this.app, path); + if (target !== null) return target; + throw e; + } + } + private async readFileContent(fileOrPath: TFile | string, binary: boolean, isStr: boolean): Promise { if (isStr) { - return binary - ? await this.app.vault.adapter.readBinary(fileOrPath as string) - : await this.app.vault.adapter.read(fileOrPath as string); + return this.readStringPathContent(fileOrPath as string, binary); } if (fileOrPath instanceof TFile) { try { @@ -660,16 +675,18 @@ export class SyncStatusView extends ItemView { const total = local.length + remote.length; const prog = new Notice(`Deleting 0/${total} files…`, 0); - const errors: string[] = []; + const errors: { path: string, message: string }[] = []; await this.performLocalDeletion(local, total, prog, errors); await this.performRemoteDeletion(remote, total, local.length, prog, errors); prog.hide(); - new Notice(errors.length > 0 - ? `Deleted ${total - errors.length}/${total}. ${errors.length} failed.` - : `Deleted ${total} files` - ); + if (errors.length > 0) { + logger.error('Delete errors:', errors); + new Notice(`Deleted ${total - errors.length}/${total}. ${errors.length} failed: ${errors.map(e => e.message).join('; ')}`); + } else { + new Notice(`Deleted ${total} files`); + } this.renderView(); } @@ -704,7 +721,7 @@ export class SyncStatusView extends ItemView { return this.showConfirmDialog(msg); } - private async performLocalDeletion(local: FileStatus[], total: number, prog: Notice, errors: string[]): Promise { + private async performLocalDeletion(local: FileStatus[], total: number, prog: Notice, errors: { path: string, message: string }[]): Promise { let cur = 0; for (const s of local) { cur++; @@ -715,11 +732,13 @@ export class SyncStatusView extends ItemView { await this.plugin.sync.clearMetadata(s.path); this.fileStatuses.delete(s.path); this.selectedFiles.delete(s.path); - } catch { errors.push(s.path); } + } catch (e) { + errors.push({ path: s.path, message: e instanceof Error ? e.message : String(e) }); + } } } - private async performRemoteDeletion(remote: FileStatus[], total: number, localCount: number, prog: Notice, errors: string[]): Promise { + private async performRemoteDeletion(remote: FileStatus[], total: number, localCount: number, prog: Notice, errors: { path: string, message: string }[]): Promise { let cur = localCount; for (const s of remote) { cur++; @@ -728,7 +747,9 @@ export class SyncStatusView extends ItemView { await this.plugin.gitService.deleteFile(s.path, this.plugin.settings.branch, `Delete ${s.path}`); this.fileStatuses.delete(s.path); this.selectedFiles.delete(s.path); - } catch { errors.push(s.path); } + } catch (e) { + errors.push({ path: s.path, message: e instanceof Error ? e.message : String(e) }); + } } } diff --git a/tests/services/gitea-service.test.ts b/tests/services/gitea-service.test.ts index 829a43e..907fa89 100644 --- a/tests/services/gitea-service.test.ts +++ b/tests/services/gitea-service.test.ts @@ -241,6 +241,29 @@ describe('GiteaService', () => { const deleteCall = calls[1]?.[0] as RequestUrlParam; expect(deleteCall.url).toBe(`${baseUrl}/api/v1/repos/${owner}/${repo}/contents/notes/test.md`); }); + + it('should throw instead of sending an empty sha when the pre-delete lookup 404s', async () => { + mockRequest({ status: 404 }); + + await expect(service.deleteFile('missing.md', 'main', 'delete missing.md')).rejects.toThrow('missing.md'); + + const calls = vi.mocked(requestUrl).mock.calls; + expect(calls).toHaveLength(1); // no DELETE request was sent + }); + + it('should URL-encode path segments with spaces or non-ASCII characters', async () => { + vi.mocked(requestUrl) + .mockResolvedValueOnce({ status: 200, json: { content: btoa('content'), sha: 'file-sha' } } as unknown as RequestUrlResponse) + .mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse); + + await service.deleteFile('folder/我的 筆記.md', 'main', 'delete note'); + + const calls = vi.mocked(requestUrl).mock.calls; + const getCall = calls[0]?.[0] as RequestUrlParam; + expect(getCall.url).toContain('/contents/folder/'); + expect(getCall.url).not.toContain(' '); + expect(getCall.url).not.toContain('我的'); + }); }); describe('testConnection', () => { diff --git a/tests/services/github-service.test.ts b/tests/services/github-service.test.ts index 1154052..2d5a184 100644 --- a/tests/services/github-service.test.ts +++ b/tests/services/github-service.test.ts @@ -239,6 +239,29 @@ describe('GitHubService', () => { expect(deleteCall.method).toBe('DELETE'); expect(deleteCall.body).toContain('"sha":"file-sha"'); }); + + it('should throw instead of sending an empty sha when the pre-delete lookup 404s', async () => { + mockRequest({ status: 404 }); + + await expect(service.deleteFile('missing.md', 'main', 'delete missing.md')).rejects.toThrow('missing.md'); + + const calls = vi.mocked(requestUrl).mock.calls; + expect(calls).toHaveLength(1); // no DELETE request was sent + }); + + it('should URL-encode path segments with spaces or non-ASCII characters', async () => { + vi.mocked(requestUrl) + .mockResolvedValueOnce({ status: 200, json: { content: btoa('content'), sha: 'file-sha' } } as unknown as RequestUrlResponse) + .mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse); + + await service.deleteFile('folder/我的 筆記.md', 'main', 'delete note'); + + const calls = vi.mocked(requestUrl).mock.calls; + const getCall = calls[0]?.[0] as RequestUrlParam; + expect(getCall.url).toContain('/contents/folder/'); + expect(getCall.url).not.toContain(' '); + expect(getCall.url).not.toContain('我的'); + }); }); describe('testConnection', () => {