Various improvements #169
No reviewers
Labels
No labels
bug
dependencies
docker
documentation
duplicate
enhancement
good first issue
help wanted
invalid
javascript
question
rust
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: andras/vault-link#169
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "asch/saturday"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
awaitAllPull request overview
This PR implements various improvements to the VaultLink sync system, including making file type merging configurable, adding comprehensive documentation, improving error handling, refactoring for better maintainability, and enhancing the testing infrastructure.
Key Changes:
Reviewed changes
Copilot reviewed 95 out of 97 changed files in this pull request and generated 3 comments.
Show a summary per file
Files not reviewed (1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -227,1 +240,4 @@/*** Deconflicts the given path by appending (1), (2), etc. before the file extension until a non-existent path is found.* The returned path has a lock acquired on it; it must be released by the caller when no longer needed.Changed from accessing
.groups?.[0]to[1]. This assumes the regex has a capturing group at index 1, which matches the regex pattern\\((\\d+)\\)$. However, this is fragile - if the regex changes, this code breaks silently. Consider using named capture groups for better maintainability.@ -0,0 +25,4 @@public set min(value: number) {this.minValue = Math.max(value, this.minValue);this.seenValues = this.seenValues.filter((v) => v > this.minValue);this.advanceMinWhilePossible();The filter should compare against
value(the input parameter) rather thanthis.minValuesincethis.minValueis updated on line 26 and may not reflect the original intent. If the goal is to filter values greater than the new minimum, this is correct but unclear.@ -0,0 +18,4 @@if (jitterScaleInSeconds > 0) {await sleep(Math.random() * jitterScaleInSeconds * 1000);}The callback parameters should be properly typed as non-null in the parent class. Using optional chaining (
callback?.()) to handle null callbacks is correct, but the type signature should match WebSocket's actual type which requires non-null callbacks.Pull request overview
Copilot reviewed 110 out of 112 changed files in this pull request and generated 8 comments.
Files not reviewed (1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -138,1 +64,4 @@await client.destroy();});await client.start();Using a global variable to prevent multiple instances could cause issues if the plugin is reloaded without a full page refresh. Consider using Obsidian's plugin management system or a more robust singleton pattern rather than a global flag.
@ -23,3 +20,4 @@"uuid": "^13.0.0","@types/node": "^24.8.1","ts-loader": "^9.5.2","tslib": "2.8.1",These dependencies (byte-base64, minimatch, p-queue, reconcile-text, uuid) have been moved from 'dependencies' to 'devDependencies'. However, these packages are used in the runtime code (not just development/build), so they should remain in 'dependencies'. Moving runtime dependencies to devDependencies will cause issues for consumers of this package.
@ -3,0 +2,4 @@public constructor(message: string,public readonly filePath: string) {Adding a required filePath parameter to FileNotFoundError changes its constructor signature, which is a breaking change for any existing code that constructs this error. Consider making filePath optional with a default value to maintain backwards compatibility.
@ -10,3 +11,3 @@export class FileOperations {private static readonly PARENTHESES_REGEX = / \((\d+)\)$/;private static readonly PARENTHESES_REGEX = / \((?<count>\d+)\)$/;private readonly fs: SafeFileSystemOperations;[nitpick] The named capture group is called 'count' but is accessed as 'groups?.count' in the code at line 265. For consistency with the previous code that used groups?.[0], consider either keeping the array access pattern or updating the comment to explain the named group convention.
The comment 'ignore the returned promise' is misleading - splice returns an array of removed elements, not a promise. Consider either removing the comment or clarifying that you're intentionally not using the return value.
The unlock method now silently returns when unlocking a key that isn't locked, removing the error that was previously thrown. This change makes debugging harder - consider logging a warning when attempting to unlock an unlocked key, especially since the comment on line 122 still mentions throwing an error.
@ -0,0 +26,4 @@this.minValue = Math.max(value, this.minValue);this.seenValues = this.seenValues.filter((v) => v > this.minValue);this.advanceMinWhilePossible();}Filtering the entire seenValues array on every min update could be inefficient for large arrays. Since seenValues is sorted, consider using binary search to find the first value > this.minValue and slicing from that index instead of filtering.
@ -22,0 +19,4 @@let name_parts = file_name.rsplitn(2, '.').collect::<Vec<_>>();let mut reverse_parts = name_parts.into_iter().rev();match (reverse_parts.next(), reverse_parts.next()) {(Some(stem), maybe_extension) => (The comment states '.config.json should split as .config + .json' but the code treats it as a complex dotfile (not simple). Consider clarifying the comment to explain that simple dotfiles have exactly one dot (e.g., '.gitignore') while complex dotfiles like '.config.json' have multiple dots and are handled by the regular path splitting logic.