feat: Replaced GitHub/Octokit with generic Isomorphic Git#99
feat: Replaced GitHub/Octokit with generic Isomorphic Git#99saberzero1 merged 12 commits intomainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request represents a major architectural refactor that replaces the GitHub/Octokit-specific implementation with a provider-agnostic Git integration using isomorphic-git. The changes enable support for multiple Git hosting providers (GitHub, GitLab, Bitbucket, Gitea, Codeberg, and self-hosted instances), remove dependency on GitHub API rate limits, and improve performance through direct Git operations.
Key changes:
- Complete replacement of Octokit with isomorphic-git for Git operations
- New generic settings model with automatic migration from legacy GitHub settings
- Updated UI to reflect provider-agnostic terminology
- Addition of new dependencies and build configuration for browser compatibility
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
src/views/SettingsView/Views/GithubSettings.ts |
Removed (replaced by GitSettings.ts) |
src/views/SettingsView/Views/GitSettings.ts |
New generic Git settings UI with provider detection and branch selection |
src/views/SettingsView/SettingView.ts |
Updated to use GitSettings instead of GithubSettings |
src/repositoryConnection/RepositoryConnection.ts |
Complete rewrite using isomorphic-git with LightningFS for local caching |
src/repositoryConnection/QuartzSyncerSiteManager.ts |
Updated to use new Git settings structure |
src/publisher/Publisher.ts |
Updated to use new Git settings object |
src/models/settings.ts |
Added generic Git settings types with legacy field deprecation |
src/models/SyncerTab.ts |
Updated imports to use GitSettings |
package.json |
Removed @octokit/core, added isomorphic-git and related dependencies |
package-lock.json |
Updated dependency tree for new packages |
main.ts |
Added settings migration logic and updated defaults |
esbuild.config.mjs |
Added buffer shim injection and alias for browser compatibility |
esbuild-buffer-shim.js |
New shim file for Buffer polyfill |
docs/Changelog.md |
Documented v1.9.0 changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async clearLocalCache(): Promise<void> { | ||
| const fsName = this.getFsName(); | ||
|
|
||
| return { | ||
| path: normalizePath(file.getPath()), | ||
| mode: "100644", | ||
| type: "blob", | ||
| sha: blob.data.sha, | ||
| }; | ||
| } catch (error) { | ||
| logger.error(error); | ||
| try { | ||
| if (typeof indexedDB !== "undefined") { | ||
| indexedDB.deleteDatabase(fsName); | ||
| } | ||
| }); | ||
| this.fs = null; | ||
| this.initialized = false; | ||
| logger.info("Local git cache cleared"); | ||
| } catch (error) { | ||
| logger.error("Failed to clear local cache", error); | ||
| } |
There was a problem hiding this comment.
The clearLocalCache method uses typeof indexedDB !== "undefined" to check for browser environment, but doesn't handle the case where the deletion might fail or be denied. Consider adding error handling and user feedback for cache clearing operations.
| this.branchesLoaded = true; | ||
|
|
||
| if (!gitSettings.branch) { | ||
| gitSettings.branch = defaultBranch || "v4"; |
There was a problem hiding this comment.
The default branch fallback is hardcoded to "v4" which may not be appropriate for all Git providers. Consider using "main" or "master" as more universal defaults, or making this configurable based on the provider hint.
| gitSettings.branch = defaultBranch || "v4"; | |
| const fallbackBranch = | |
| defaultBranch || | |
| branches.find((b) => b === "main") || | |
| branches.find((b) => b === "master") || | |
| branches[0] || | |
| "main"; | |
| gitSettings.branch = fallbackBranch; |
| this.settings.settings.git.auth.type = | ||
| value as GitAuthType; | ||
| await this.checkConnectionAndSaveSettings(); | ||
| this.display(); |
There was a problem hiding this comment.
When the authentication type changes, the display is refreshed by calling this.display() which will reset the entire settings view. This could be disruptive to the user if they've scrolled or are in the middle of configuring other settings. Consider a more targeted refresh that only updates the relevant input fields.
| this.display(); |
| let previous; | ||
|
|
||
| do { | ||
| previous = path; | ||
| path = path.replace(/\.\.\//g, ""); |
There was a problem hiding this comment.
The normalization function contains a potentially infinite loop if the path contains patterns that don't match the simple "../" replacement. The do-while loop should have a safety counter to prevent hanging if an unexpected path format is encountered.
| let previous; | |
| do { | |
| previous = path; | |
| path = path.replace(/\.\.\//g, ""); | |
| let previous; | |
| const maxIterations = 1000; | |
| let iterations = 0; | |
| do { | |
| previous = path; | |
| path = path.replace(/\.\.\//g, ""); | |
| iterations++; | |
| if (iterations >= maxIterations) { | |
| logger.warn( | |
| "normalizeFilePath: exceeded max normalization iterations for path", | |
| path, | |
| ); | |
| break; | |
| } |
| const normalizeFilePath = (path: string): string => { | ||
| let previous; | ||
|
|
||
| const repoDataPromise = this.octokit.request( | ||
| "GET /repos/{owner}/{repo}", | ||
| { | ||
| ...this.getBasePayload(), | ||
| }, | ||
| ); | ||
| do { | ||
| previous = path; | ||
| path = path.replace(/\.\.\//g, ""); | ||
| } while (path !== previous); |
There was a problem hiding this comment.
The same normalization pattern with a potential infinite loop is duplicated in the updateFiles method. This code duplication should be extracted into a shared utility method.
| const ensureDirectory = async (filePath: string): Promise<void> => { | ||
| const parts = filePath.split("/"); | ||
| parts.pop(); | ||
| let currentPath = this.dir; | ||
|
|
||
| //eslint-disable-next-line | ||
| const tree = newTreeEntries.filter((x: any) => x !== undefined) as { | ||
| path?: string | undefined; | ||
| mode?: | ||
| | "100644" | ||
| | "100755" | ||
| | "040000" | ||
| | "160000" | ||
| | "120000" | ||
| | undefined; | ||
| type?: "tree" | "blob" | "commit" | undefined; | ||
| sha?: string | null | undefined; | ||
| content?: string | undefined; | ||
| }[]; | ||
|
|
||
| const newTree = await this.octokit.request( | ||
| "POST /repos/{owner}/{repo}/git/trees", | ||
| { | ||
| ...this.getBasePayload(), | ||
| base_tree: baseTreeSha, | ||
| tree, | ||
| }, | ||
| ); | ||
| for (const part of parts) { | ||
| if (!part) continue; | ||
| currentPath = `${currentPath}/${part}`; | ||
|
|
||
| const commitMessage = "Deleted multiple files"; | ||
| try { | ||
| await this.getFs().promises.mkdir(currentPath); | ||
| } catch { | ||
| logger.debug(`Directory ${currentPath} already exists`); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
The ensureDirectory function is duplicated in both deleteFiles and updateFiles methods. This should be extracted to a class-level private method to reduce code duplication and improve maintainability.
| await git.checkout({ | ||
| ...this.getGitConfig(), | ||
| ref: remoteCommit, | ||
| force: true, | ||
| }); | ||
|
|
||
| await git.branch({ | ||
| ...this.getGitConfig(), | ||
| ref: this.branch, | ||
| object: remoteCommit, | ||
| force: true, | ||
| }); | ||
|
|
||
| await git.checkout({ | ||
| ...this.getGitConfig(), | ||
| ref: this.branch, | ||
| }); | ||
|
|
||
| const normalizeFilePath = (path: string): string => { | ||
| let previous; | ||
|
|
||
| do { | ||
| previous = path; | ||
| path = path.replace(/\.\.\//g, ""); | ||
| } while (path !== previous); | ||
|
|
||
| path = this.getVaultPath(path); | ||
|
|
||
| return path.startsWith("/") | ||
| ? `${this.contentFolder}${path}` | ||
| : `${this.contentFolder}/${path}`; | ||
| }; | ||
|
|
||
| for (const filePath of filePaths) { | ||
| const normalizedPath = normalizeFilePath(filePath); | ||
| const fullPath = `${this.dir}/${normalizedPath}`; | ||
|
|
||
| try { | ||
| await this.getFs().promises.unlink(fullPath); | ||
|
|
||
| await git.remove({ | ||
| ...this.getGitConfig(), | ||
| filepath: normalizedPath, | ||
| }); | ||
| } catch (error) { | ||
| logger.warn( | ||
| `Could not delete file ${normalizedPath}`, | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| await git.commit({ | ||
| ...this.getGitConfig(), | ||
| message: "Deleted multiple files", | ||
| author: { | ||
| name: "Quartz Syncer", | ||
| email: "quartz-syncer@obsidian.md", | ||
| }, | ||
| }); | ||
|
|
||
| await git.push({ | ||
| ...this.getGitConfig(), | ||
| url: this.remoteUrl, | ||
| remote: "origin", | ||
| ref: this.branch, | ||
| }); |
There was a problem hiding this comment.
The force push operation (deleteFiles and updateFiles methods) checks out to remote commits and force-pushes changes. This could cause data loss if multiple users are working on the same repository or if there are local changes. Consider adding safeguards or warnings about this destructive operation.
| } catch (error) { | ||
| logger.error(`Could not get file ${path}`, error); | ||
| throw new Error( | ||
| `Could not get file ${path} from repository ${this.getRepositoryName()}`, |
There was a problem hiding this comment.
The same error handling pattern occurs here - original error information is discarded. Consider preserving the error context for better debugging.
| `Could not get file ${path} from repository ${this.getRepositoryName()}`, | |
| `Could not get file ${path} from repository ${this.getRepositoryName()}`, | |
| { cause: error }, |
| this.baseSyncerConnection = new RepositoryConnection({ | ||
| githubToken: settings.githubToken, | ||
| githubUserName: "saberzero1", | ||
| quartzRepository: "quartz", | ||
| gitSettings: { | ||
| remoteUrl: "https://github.com/saberzero1/quartz.git", | ||
| branch: "main", | ||
| auth: { type: "none" }, | ||
| }, |
There was a problem hiding this comment.
The base syncer connection is hardcoded to use "https://github.com/saberzero1/quartz.git" with no authentication. If this repository is or becomes private, the connection will fail. Consider documenting why this specific repository is used or making it configurable.
| git: { | ||
| remoteUrl: "", | ||
| branch: "main", | ||
| corsProxyUrl: "", |
There was a problem hiding this comment.
The CORS proxy URL defaults to an empty string, which will be truthy but may cause issues if passed to the Git configuration. Consider using undefined as the default instead, or explicitly check for empty strings before using the value.
| corsProxyUrl: "", | |
| corsProxyUrl: undefined, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!gitSettings.branch) { | ||
| gitSettings.branch = defaultBranch || "v4"; |
There was a problem hiding this comment.
The default branch fallback uses "v4" when no branch is provided, but the DEFAULT_SETTINGS in main.ts sets the default branch to "main". This inconsistency could cause confusion. Consider using a consistent default value across the codebase.
main.ts
Outdated
| remoteUrl: githubUserName | ||
| ? `https://github.com/${githubUserName}/${githubRepo}.git` | ||
| : "", | ||
| branch: "main", |
There was a problem hiding this comment.
The migration logic in main.ts sets the default branch to "main", but elsewhere in the code (GitSettings.ts line 124, 260, 276-277, 294), "v4" is used as a fallback. This inconsistency could lead to unexpected behavior where users migrating from GitHub settings get a "main" branch, but fresh installs or certain code paths default to "v4".
|
|
||
| display() { | ||
| this.settingsRootElement.empty(); | ||
| this.settingsRootElement.addClass("quartz-syncer-github-settings"); |
There was a problem hiding this comment.
The class name "quartz-syncer-github-settings" is still used even though this is now the Git settings view, not GitHub-specific. This should be updated to reflect the generic nature of the settings.
| } | ||
|
|
||
| if (this.connectionStatus === "connected") { | ||
| this.connectionStatusElement.innerText = "successful!"; |
There was a problem hiding this comment.
The spelling error "succesful" should be corrected to "successful".
|
|
||
| display(): void { | ||
| this.settingsRootElement.empty(); | ||
| this.settingsRootElement.addClass("quartz-syncer-github-settings"); |
There was a problem hiding this comment.
The class name "quartz-syncer-github-settings" in UISettings doesn't make semantic sense since this is the UI settings view. Consider using a more appropriate class name like "quartz-syncer-ui-settings".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request introduces a major refactor to replace the previous GitHub/Octokit-specific implementation with a generic, provider-agnostic Git integration using isomorphic-git. The changes enable support for multiple Git hosting providers (GitHub, GitLab, Bitbucket, Gitea, Codeberg, and self-hosted), improve performance, and remove dependency on GitHub APIs and their rate limits. The settings system is overhauled to support generic Git configuration, with automatic migration from legacy GitHub-specific settings. UI and code references are updated to reflect the new generic Git model.
Core sync engine and provider support:
Settings model and migration:
gitsettings object inQuartzSyncerSettings, supporting provider hinting, multiple authentication types, and generic remote configuration. Legacy GitHub fields are deprecated but retained for migration. [1] [2]gitsettings object instead of GitHub-specific fields. [1] [2] [3]UI/UX and settings view updates:
Repository connection and content management:
RepositoryConnectionandQuartzSyncerSiteManagerto use the new genericgitSettingsobject, and refactored content tree and hash logic to use provider-agnostic fields (oidinstead ofsha, generic types). [1] [2] [3] [4]Build and dependency updates:
isomorphic-git,@isomorphic-git/lightning-fs, andbuffer-es6. Updated the esbuild configuration to inject a buffer shim and alias the buffer module for browser compatibility. [1] [2] [3]These changes collectively modernize the sync engine, making it more flexible, performant, and provider-agnostic, while maintaining a smooth upgrade path for existing users.