Various improvements #169

Merged
schmelczer merged 78 commits from asch/saturday into main 2025-11-30 15:24:52 +00:00
schmelczer commented 2025-11-23 17:02:13 +00:00 (Migrated from github.com)
  • add AI written draft docs
  • publish docs
  • run spell check on docs
  • run e2e tests on a schedule
  • support arbitrary text file extensions
  • add API version level
  • add awaitAll
  • add proper clean up for obsidian plugin
  • and sync-client
  • add tests for resetting state
  • make logs copy-pastable
  • don't allow changing settings while clean-up is in progress
  • fix file deconflicting
  • restore the coveredvalues from db entries
  • refactor modules
- add AI written draft docs - publish docs - run spell check on docs - run e2e tests on a schedule - support arbitrary text file extensions - add API version level - add `awaitAll` - add proper clean up for obsidian plugin - and sync-client - add tests for resetting state - make logs copy-pastable - don't allow changing settings while clean-up is in progress - fix file deconflicting - restore the coveredvalues from db entries - refactor modules
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-11-27 22:26:21 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull 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:

  • Added configurable mergeable file extensions instead of hardcoded values
  • Created comprehensive documentation site with VitePress
  • Refactored code organization and improved error handling
  • Enhanced test coverage and introduced new utility functions

Reviewed changes

Copilot reviewed 95 out of 97 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
sync-server/src/utils/is_file_type_mergable.rs Made mergeable file extensions configurable via parameter
sync-server/src/consts.rs Added constants for default mergeable extensions and API version
sync-server/src/config/server_config.rs Added configuration field for mergeable file extensions
frontend/sync-client/src/utils/is-file-type-mergable.ts Made mergeable extensions configurable in TypeScript client
frontend/sync-client/src/sync-client.ts Major refactor of SyncClient lifecycle and state management
frontend/sync-client/src/services/server-config.ts Added new ServerConfig class to manage server configuration
frontend/sync-client/src/services/fetch-controller.ts Renamed and improved from ConnectionStatus with better reset handling
frontend/sync-client/src/utils/await-all.ts Added utility for awaiting multiple promises with proper error handling
docs/* Added comprehensive documentation site with guides and architecture docs
frontend/eslint.config.mjs Added linting rules to enforce best practices
Files not reviewed (1)
  • frontend/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull 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:** - Added configurable mergeable file extensions instead of hardcoded values - Created comprehensive documentation site with VitePress - Refactored code organization and improved error handling - Enhanced test coverage and introduced new utility functions ### Reviewed changes Copilot reviewed 95 out of 97 changed files in this pull request and generated 3 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | sync-server/src/utils/is_file_type_mergable.rs | Made mergeable file extensions configurable via parameter | | sync-server/src/consts.rs | Added constants for default mergeable extensions and API version | | sync-server/src/config/server_config.rs | Added configuration field for mergeable file extensions | | frontend/sync-client/src/utils/is-file-type-mergable.ts | Made mergeable extensions configurable in TypeScript client | | frontend/sync-client/src/sync-client.ts | Major refactor of SyncClient lifecycle and state management | | frontend/sync-client/src/services/server-config.ts | Added new ServerConfig class to manage server configuration | | frontend/sync-client/src/services/fetch-controller.ts | Renamed and improved from ConnectionStatus with better reset handling | | frontend/sync-client/src/utils/await-all.ts | Added utility for awaiting multiple promises with proper error handling | | docs/* | Added comprehensive documentation site with guides and architecture docs | | frontend/eslint.config.mjs | Added linting rules to enforce best practices | </details> <details> <summary>Files not reviewed (1)</summary> * **frontend/package-lock.json**: Language not supported </details> --- 💡 <a href="/schmelczer/vault-link/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
@ -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.
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-27 22:26:21 +00:00

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.

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();
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-27 22:26:20 +00:00

The filter should compare against value (the input parameter) rather than this.minValue since this.minValue is 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.

The filter should compare against `value` (the input parameter) rather than `this.minValue` since `this.minValue` is 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);
}
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-27 22:26:20 +00:00

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.

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.
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-11-30 15:03:04 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

Copilot reviewed 110 out of 112 changed files in this pull request and generated 8 comments.

Files not reviewed (1)
  • frontend/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview Copilot reviewed 110 out of 112 changed files in this pull request and generated 8 comments. <details> <summary>Files not reviewed (1)</summary> * **frontend/package-lock.json**: Language not supported </details> --- 💡 <a href="/schmelczer/vault-link/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
@ -138,1 +64,4 @@
await client.destroy();
});
await client.start();
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:04 +00:00

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.


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. ```suggestion ```
@ -23,3 +20,4 @@
"uuid": "^13.0.0",
"@types/node": "^24.8.1",
"ts-loader": "^9.5.2",
"tslib": "2.8.1",
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:04 +00:00

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.

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
) {
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:03 +00:00

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.

		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. ```suggestion public readonly filePath?: string ```
@ -10,3 +11,3 @@
export class FileOperations {
private static readonly PARENTHESES_REGEX = / \((\d+)\)$/;
private static readonly PARENTHESES_REGEX = / \((?<count>\d+)\)$/;
private readonly fs: SafeFileSystemOperations;
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:03 +00:00

[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.

[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.
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:04 +00:00

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.

							void this.outstandingPromises.splice(index, 1); // intentionally ignore the returned array
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. ```suggestion void this.outstandingPromises.splice(index, 1); // intentionally ignore the returned array ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:03 +00:00

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.

		if (!this.locked.has(key)) {
			this.logger?.warn?.(`Attempted to unlock key "${key}" which is not currently locked`);
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. ```suggestion if (!this.locked.has(key)) { this.logger?.warn?.(`Attempted to unlock key "${key}" which is not currently locked`); ```
@ -0,0 +26,4 @@
this.minValue = Math.max(value, this.minValue);
this.seenValues = this.seenValues.filter((v) => v > this.minValue);
this.advanceMinWhilePossible();
}
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:02 +00:00

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.

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) => (
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-11-30 15:03:02 +00:00

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.

    // Handle dotfiles:
    //   - Simple dotfiles (exactly one dot, e.g., ".gitignore") are treated as having no extension.
    //   - Complex dotfiles (multiple dots, e.g., ".config.json") are handled by the regular extension splitting logic.
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. ```suggestion // Handle dotfiles: // - Simple dotfiles (exactly one dot, e.g., ".gitignore") are treated as having no extension. // - Complex dotfiles (multiple dots, e.g., ".config.json") are handled by the regular extension splitting logic. ```
Sign in to join this conversation.
No description provided.