Skip to content

refactor: eliminate duplication and fix regressions - #16

Closed
ClaudiaFang wants to merge 7 commits into
masterfrom
fix-lint
Closed

ClaudiaFang wants to merge 7 commits into
masterfrom
fix-lint

Conversation

@ClaudiaFang

Copy link
Copy Markdown
Member

This PR incorporates additional fixes and refactoring after the merge of #15.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the codebase to improve modularity and maintainability, notably by introducing a BaseGitService to unify GitHub and GitLab logic and decomposing large methods in SyncStatusView and SyncManager. However, the refactoring of listFiles in both Git services introduced regressions where the rootPath prefix is no longer stripped, which will break path matching in the UI. Furthermore, the status refresh logic and batch push operations are currently inefficient, performing excessive sequential network requests that could lead to API rate limiting and performance issues in larger vaults.

Comment on lines +61 to +64
return data.tree
.filter(item => item.type === 'blob')
.map(item => item.path);

// Filter by rootPath if set
if (this.rootPath) {
const prefix = this.rootPath + '/';
return allFiles
.filter(file => file.startsWith(prefix))
.map(file => file.substring(prefix.length));
}

return allFiles;
.map(item => item.path)
.filter(p => !this.rootPath || p.startsWith(this.rootPath));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The listFiles implementation contains a regression and a logic error:

  1. Regression: It no longer strips the rootPath prefix from the returned paths. Since the rest of the application (e.g., SyncStatusView and GitignoreManager) expects paths relative to the configured root, this will break path matching and status detection for any user with a non-empty rootPath.
  2. Imprecise Filter: Using startsWith(this.rootPath) can match unintended directories (e.g., if rootPath is docs, it will incorrectly match docs-internal/). It should check for a proper path boundary.
        const prefix = this.rootPath ? this.rootPath.replace(/\/+$/, '') + '/' : '';
        return data.tree
            .filter(item => item.type === 'blob')
            .map(item => item.path)
            .filter(p => !prefix || p.startsWith(prefix))
            .map(p => prefix ? p.substring(prefix.length) : p);

Comment on lines +62 to +69
const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/tree?ref=${branch}&recursive=true&per_page=100`;
const response = await this.safeRequest(url, 'GET');
const data = response.json as GitLabTreeItem[];

return data
.filter(item => item.type === 'blob')
.map(item => item.path);

// Filter by rootPath if set
if (this.rootPath) {
const prefix = this.rootPath + '/';
return allFiles
.filter(file => file.startsWith(prefix))
.map(file => file.substring(prefix.length));
}

return allFiles;
.map(item => item.path)
.filter(p => !this.rootPath || p.startsWith(this.rootPath));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the GitHub service, listFiles here has significant regressions:

  1. It no longer uses the path parameter in the GitLab API request, which is inefficient as it fetches the entire repository tree instead of just the relevant subtree.
  2. It does not strip the rootPath prefix from the results, which breaks path matching in the UI for subfolder-mapped vaults.
  3. The startsWith filter is imprecise and can match sibling directories with similar names.
Suggested change
const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/tree?ref=${branch}&recursive=true&per_page=100`;
const response = await this.safeRequest(url, 'GET');
const data = response.json as GitLabTreeItem[];
return data
.filter(item => item.type === 'blob')
.map(item => item.path);
// Filter by rootPath if set
if (this.rootPath) {
const prefix = this.rootPath + '/';
return allFiles
.filter(file => file.startsWith(prefix))
.map(file => file.substring(prefix.length));
}
return allFiles;
.map(item => item.path)
.filter(p => !this.rootPath || p.startsWith(this.rootPath));
const searchPath = this.rootPath ? this.rootPath.replace(/\/+$/, '') : '';
const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/tree?ref=${branch}&recursive=true&per_page=100${searchPath ? `&path=${encodeURIComponent(searchPath)}` : ''}`;
const response = await this.safeRequest(url, 'GET');
const data = response.json as GitLabTreeItem[];
const prefix = searchPath ? searchPath + '/' : '';
return data
.filter(item => item.type === 'blob')
.map(item => item.path)
.filter(p => !prefix || p.startsWith(prefix))
.map(p => prefix ? p.substring(prefix.length) : p);

Comment thread src/ui/SyncStatusView.ts
Comment on lines +611 to +620
private async performStatusCheck(filesToCheck: Array<TFile | string>): Promise<void> {
const total = filesToCheck.length;
for (let i = 0; i < total; i++) {
const file = filesToCheck[i];
if (file) {
await this.refreshFileStatus(file);
checked++;
const c = this.containerEl.children[1];
if (c) {
const fill = c.querySelector('.ssv-progress-fill');
const text = c.querySelector('.ssv-progress-text');
if (fill && text) {
const pct = Math.round((checked / total) * 100);
fill.setAttr('style', `width: ${pct}%`);
text.textContent = `Checking files… ${checked}/${total} (${pct}%)`;
}
}
}
this.updateRefreshProgress(i + 1, total);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This loop performs a sequential network request (getFile) for every file in the vault during a refresh. For vaults with many files, this will be extremely slow and will likely trigger API rate limits on GitHub/GitLab, leading to a poor user experience.

Recommendation: Optimize the status check by leveraging the blob SHAs already provided by the Git provider's tree API (e.g., in listFiles). Compare these remote SHAs with local metadata or calculated hashes to identify changed files. Full content should only be fetched when a diff is explicitly requested or during the actual sync operation.

Comment thread src/logic/sync-manager.ts
Comment on lines +315 to +318
const remote = await this.gitService.getFile(path, this.settings.branch);
await this.gitService.pushFile(path, content, this.settings.branch, `Update ${name} from Obsidian`, remote.sha || undefined);
const newRemote = await this.gitService.getFile(path, this.settings.branch);
this.settings.syncMetadata[path] = { lastSyncedSha: newRemote.sha, lastSyncedAt: Date.now(), lastKnownPath: path };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In a batch push operation, performing two extra getFile calls per file (one before to get the SHA and one after to update metadata) is inefficient and doubles the network overhead.

  1. The existingSha is often already known from the preceding conflict check or local metadata.
  2. The pushFile API response from GitHub (and commit info from GitLab) can be used to update metadata without an additional fetch. Consider updating the pushFile interface to return the new SHA.

@ClaudiaFang
ClaudiaFang deleted the fix-lint branch June 28, 2026 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant