fix: code quality enhancements and UI improvements (#23) - #24
Conversation
- Fix plugin scanner warnings: remove !important from styles.css, fix CSS shorthand (0 0 8px), use activeWindow.setTimeout(), remove .zip from release assets - Extract LCS diff algorithm to src/utils/diff.ts with full unit tests - Extract UI render components: ActionBar, FileListItem, DiffPanel reducing SyncStatusView from 853 to 523 lines - Add shared types in src/ui/types.ts - Add unified logger in src/utils/logger.ts replacing 9 console.* calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Panel Adds comprehensive UI component test coverage with 54 new test cases across three components to close issue #23. Includes JSDOM setup polyfills and tooltip mock utilities for DOM-dependent component testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Render checked files incrementally below progress bar during refresh - Progress bar now shows X/Y count and percentage - Remove fileStatus.file guard from push/remove for unsynced files - Fix canPush/canDelete in action bar to include hidden (string-path) files - Fix lint: use pre-declared vi.fn() to avoid unbound-method errors - Fix test import path and mockSettings typing for sync-manager-mapping Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jsdom v29 uses exports field for types, incompatible with moduleResolution "node" in CI — adding @types/jsdom provides standalone type declarations. Cast el to HTMLInputElement directly instead of instanceof window.HTMLInputElement to avoid unresolvable type narrowing across jsdom window context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for binary files in the sync process, updates the gitignore management to scan local files, and refactors UI components. Several issues were identified regarding unsafe type casting when handling potential binary content as strings, which could lead to runtime failures in the conflict modal and gitignore processing. Additionally, there is significant code duplication across the codebase, particularly regarding path normalization and binary file detection, which should be consolidated into shared utility functions.
| if (remoteFile?.content) { | ||
| content = remoteFile.content; | ||
| content = remoteFile.content as string; | ||
| } |
There was a problem hiding this comment.
This is an unsafe type cast on line 143. If remoteFile.content happens to be an ArrayBuffer, this will not convert it to a string at runtime and will likely cause issues in the ignore library. It's safer to perform a type check.
Consider something like this:
if (remoteFile?.content) {
if (typeof remoteFile.content === 'string') {
content = remoteFile.content;
} else {
logger.warn(`Received unexpected binary content for gitignore file: ${fullGitignorePath}`);
}
}|
|
||
| if (remote.sha && lastSynced && remote.sha !== lastSynced.lastSyncedSha) { | ||
| new SyncConflictModal(this.app, name, content, remote.content, (choice) => { | ||
| new SyncConflictModal(this.app, name, content as string, remote.content as string, (choice) => { |
There was a problem hiding this comment.
This is an unsafe type cast. If content or remote.content is an ArrayBuffer (which is now possible for binary files), casting it to string will not work as expected and could cause SyncConflictModal to fail. The modal should be made aware of binary files and perhaps show a message that a text diff is not available, rather than attempting to render one from an ArrayBuffer.
| // Conflict detection for pull (only if local exists) | ||
| if (exists && remote.sha && lastSynced && remote.sha !== lastSynced.lastSyncedSha) { | ||
| new SyncConflictModal(this.app, name, localContent || '', remote.content, (choice) => { | ||
| new SyncConflictModal(this.app, name, (localContent as string) || '', remote.content as string, (choice) => { |
There was a problem hiding this comment.
Similar to a previous comment, this is an unsafe cast for binary files. localContent and remote.content can be ArrayBuffers, which cannot be cast to string this way. The conflict modal needs to handle binary files gracefully, for example by showing a message that a diff is not available, instead of attempting to display one.
| for (const subFolder of listing.folders) { | ||
| await this.scanDir(subFolder, out); | ||
| } | ||
| } catch { /* adapter.list may be unavailable in some environments */ } |
There was a problem hiding this comment.
Silently catching and ignoring errors can hide underlying problems. It's better to at least log a warning here so that if adapter.list fails for an unexpected reason, it can be debugged.
| } catch { /* adapter.list may be unavailable in some environments */ } | |
| } catch (e) { logger.warn(`Failed to scan directory ${vaultDir}:`, e); /* adapter.list may be unavailable in some environments */ } |
| private getNormalizedPath(path: string): string { | ||
| if (!this.settings.vaultFolder) return path; | ||
| const folderPath = this.settings.vaultFolder + '/'; | ||
| if (path.startsWith(folderPath)) { | ||
| return path.substring(folderPath.length); | ||
| } | ||
| if (path === this.settings.vaultFolder) return ''; | ||
| return path; | ||
| } |
There was a problem hiding this comment.
This getNormalizedPath method is duplicated in src/logic/gitignore-manager.ts and src/main.ts. To avoid code duplication and ensure consistent path handling, consider moving this logic to a shared utility file.
| private isBinary(path: string): boolean { | ||
| const ext = path.split('.').pop()?.toLowerCase(); | ||
| if (!ext) return false; | ||
| const BINARY_EXTENSIONS = new Set([ | ||
| 'png', 'jpg', 'jpeg', 'gif', 'bmp', 'ico', 'pdf', 'zip', 'gz', '7z', 'rar', | ||
| 'mp3', 'mp4', 'wav', 'ogg', 'webm', 'mov', 'avi', 'wmv', | ||
| 'doc', 'docx', 'xls', 'xlsx', 'ppt', 'pptx', 'epub', 'exe', 'dll', 'so' | ||
| ]); | ||
| return BINARY_EXTENSIONS.has(ext); | ||
| } |
There was a problem hiding this comment.
This isBinary method is duplicated in several places, including SyncStatusView.ts and git-service-base.ts, and each has a slightly different list of extensions. A new utility file src/utils/path.ts was also added with similar logic. To ensure consistency and avoid maintaining multiple lists of binary extensions, this should be consolidated into a single, shared utility function that is used everywhere.
| private isBinary(path: string): boolean { | ||
| const ext = path.split('.').pop()?.toLowerCase(); | ||
| if (!ext) return false; | ||
| const BINARY_EXTENSIONS = new Set([ | ||
| 'png', 'jpg', 'jpeg', 'gif', 'bmp', 'ico', 'pdf', 'zip', 'gz', '7z', 'rar', | ||
| 'mp3', 'mp4', 'wav', 'ogg', 'webm', 'mov', 'avi', 'wmv', | ||
| 'doc', 'docx', 'xls', 'xlsx', 'ppt', 'pptx', 'epub', 'exe', 'dll', 'so' | ||
| ]); | ||
| return BINARY_EXTENSIONS.has(ext); | ||
| } |
There was a problem hiding this comment.
This isBinary method is duplicated in src/logic/sync-manager.ts and src/services/git-service-base.ts. To avoid code duplication and ensure consistency, this logic should be centralized in a single utility function. The new src/utils/path.ts file would be a great place for it.
- Move contentsEqual to src/utils/path.ts (shared by sync-manager + SyncStatusView) - Replace private isBinary in both files with isBinaryPath from path.ts - Extract fileItemCallbacks() in SyncStatusView to remove duplicate callback objects - Create tests/services/service-test-helpers.ts with shared testConnection, getRepoGitignores, getFile error handling, getLastRequestCall helpers - Refactor github/gitlab service tests to use shared helpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
🎉 This PR is included in version 1.0.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
fileStatus.fileguard so Push/Remove buttons correctly appear for hidden (string-path) filescanPush/canDeletein action bar to include hidden files@types/jsdomto devDependencies for local/CI consistency (jsdom v29 exports-field types incompatible withmoduleResolution: "node")Element.typetype cast insetup-dom.tssync-manager-mapping.test.ts: correct import path, proper typedmockSettings, pre-declaredvi.fn()to avoid unbound-methodCloses #23
Test plan
npm run lintpassesnpm run buildpasses🤖 Generated with Claude Code