andras/vault-link
1
0
Fork 0
Code Issues Pull requests 18 Projects Releases 55 Packages Wiki Activity Actions 1

WIP: Smart create call #184

Draft
schmelczer wants to merge 46 commits from asch/smart-create into main
pull from: asch/smart-create
merge into: andras:main
Conversation 1 Commits 46 Files changed 195 +18732 -9439
schmelczer commented 2025-12-14 23:30:23 +00:00 (Migrated from github.com)
Copy link
  • bump to Node 25 and Rust 1.92
  • remove eclint
  • create error package
  • remove unnecessary npm deps, including WS
  • use prettier and rustfmt instead of editorconfig
  • disallow creating document with UUID
  • add force_merge param for all creation requests to allow merging with existing file at path
  • don't create fake remote docs on first startup
  • fix apply & connect being broken if syncing is disabled
  • fix websocket never reconnecting
- bump to Node 25 and Rust 1.92 - remove eclint - create error package - remove unnecessary npm deps, including WS - use prettier and rustfmt instead of editorconfig - disallow creating document with UUID - add force_merge param for all creation requests to allow merging with existing file at path - don't create fake remote docs on first startup - fix apply & connect being broken if syncing is disabled - fix websocket never reconnecting
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2026-01-04 15:36:33 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy link

Pull request overview

This PR updates the project's toolchain (Node.js 25, Rust 1.92), refactors the document creation workflow to support merging with existing files, and improves error handling and WebSocket reliability. The changes also include code organization improvements by consolidating error classes into a dedicated package and replacing eclint with Prettier for formatting.

Key Changes:

  • Introduces a force_merge parameter for document creation to enable merging with existing files at the same path
  • Removes client-side document ID assignment, shifting all ID generation to the server
  • Fixes WebSocket reconnection issues by adding connection timeout handling

Reviewed changes

Copilot reviewed 61 out of 78 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
sync-server/src/server/create_document.rs Implements force_merge functionality to merge with existing documents at the same path
sync-server/src/server/update_document.rs Extracts merge logic into reusable merge_with_stored_version function
sync-server/src/server/requests.rs Removes document_id field and adds force_merge parameter to CreateDocumentVersion
sync-server/src/utils/find_first_available_path.rs Updates to use get_latest_non_deleted_document_by_path and improves logging
frontend/sync-client/src/persistence/database.ts Moves documentId from DocumentRecord to DocumentMetadata and removes hasInitialSyncCompleted tracking
frontend/sync-client/src/sync-operations/unrestricted-syncer.ts Refactors document sync logic to use new handleMaybeMergingResponse helper and removes fake document creation
frontend/sync-client/src/sync-operations/syncer.ts Removes document ID generation and fake remote document creation on startup
frontend/sync-client/src/services/websocket-manager.ts Adds connection timeout handling and error callback to fix reconnection issues
frontend/sync-client/src/services/sync-service.ts Updates create method to use force_merge parameter instead of document_id
*.rs, *.ts, *.json Updates Node.js to version 25 and Rust to version 1.92 across configuration files
Comments suppressed due to low confidence (1)

sync-server/src/server/update_document.rs:1

  • Variable assignment to contentHash appears to be a reassignment that shadows the parameter, but it's not used after this point in the function. The reassignment should either update the metadata being saved (lines 520-528) or be removed if unnecessary.
use anyhow::{Context as _, anyhow};

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

## Pull request overview This PR updates the project's toolchain (Node.js 25, Rust 1.92), refactors the document creation workflow to support merging with existing files, and improves error handling and WebSocket reliability. The changes also include code organization improvements by consolidating error classes into a dedicated package and replacing eclint with Prettier for formatting. **Key Changes:** - Introduces a `force_merge` parameter for document creation to enable merging with existing files at the same path - Removes client-side document ID assignment, shifting all ID generation to the server - Fixes WebSocket reconnection issues by adding connection timeout handling ### Reviewed changes Copilot reviewed 61 out of 78 changed files in this pull request and generated 5 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | sync-server/src/server/create_document.rs | Implements force_merge functionality to merge with existing documents at the same path | | sync-server/src/server/update_document.rs | Extracts merge logic into reusable `merge_with_stored_version` function | | sync-server/src/server/requests.rs | Removes `document_id` field and adds `force_merge` parameter to `CreateDocumentVersion` | | sync-server/src/utils/find_first_available_path.rs | Updates to use `get_latest_non_deleted_document_by_path` and improves logging | | frontend/sync-client/src/persistence/database.ts | Moves `documentId` from `DocumentRecord` to `DocumentMetadata` and removes `hasInitialSyncCompleted` tracking | | frontend/sync-client/src/sync-operations/unrestricted-syncer.ts | Refactors document sync logic to use new `handleMaybeMergingResponse` helper and removes fake document creation | | frontend/sync-client/src/sync-operations/syncer.ts | Removes document ID generation and fake remote document creation on startup | | frontend/sync-client/src/services/websocket-manager.ts | Adds connection timeout handling and error callback to fix reconnection issues | | frontend/sync-client/src/services/sync-service.ts | Updates create method to use `force_merge` parameter instead of `document_id` | | *.rs, *.ts, *.json | Updates Node.js to version 25 and Rust to version 1.92 across configuration files | </details> <details> <summary>Comments suppressed due to low confidence (1)</summary> **sync-server/src/server/update_document.rs:1** * Variable assignment to `contentHash` appears to be a reassignment that shadows the parameter, but it's not used after this point in the function. The reassignment should either update the metadata being saved (lines 520-528) or be removed if unnecessary. ``` use anyhow::{Context as _, anyhow}; ``` </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>.
frontend/local-client-cli/package.json
@ -23,3 +21,1 @@
"tsx": "^4.20.6",
"typescript": "5.8.3",
"webpack": "^5.99.9",
"tsx": "^4.21.0",
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-01-04 15:36:32 +00:00
Copy link

The packages commander and watcher are listed under devDependencies but should be in dependencies since they're required at runtime for the CLI tool to function. devDependencies are only for build-time and testing tools.

The packages `commander` and `watcher` are listed under `devDependencies` but should be in `dependencies` since they're required at runtime for the CLI tool to function. devDependencies are only for build-time and testing tools.
frontend/package.json
@ -21,2 +34,3 @@
"update": "ncu -u -ws"
"lint": "eslint --fix sync-client obsidian-plugin test-client deterministic-tests local-client-cli && prettier --write \"**/*.ts\"",
"update": "ncu -u"
},
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-01-04 15:36:33 +00:00
Copy link

The prettier configuration override for *.md files uses 2-space indentation, but the command in update-api-types.sh and other scripts formats all files including markdown. This could cause inconsistent formatting in code blocks within markdown files that contain other languages. Consider excluding code blocks from this rule or being more specific about which markdown content gets 2-space indentation.

The prettier configuration override for `*.md` files uses 2-space indentation, but the command in update-api-types.sh and other scripts formats all files including markdown. This could cause inconsistent formatting in code blocks within markdown files that contain other languages. Consider excluding code blocks from this rule or being more specific about which markdown content gets 2-space indentation.
frontend/sync-client/src/services/websocket-manager.ts
@ -231,7 +305,18 @@ export class WebSocketManager {
}
};
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-01-04 15:36:32 +00:00
Copy link

There's a potential race condition: if the connection opens just as the timeout fires, both the onopen handler (line 193) and the timeout callback could attempt to clear connectionTimeoutId. While this particular case is harmless, it's better to clear the timeout inside the callback before calling close() to avoid the race. Move this.connectionTimeoutId = undefined; after the close() call or remove it from the callback entirely since it will be cleared in the onclose handler.

There's a potential race condition: if the connection opens just as the timeout fires, both the `onopen` handler (line 193) and the timeout callback could attempt to clear `connectionTimeoutId`. While this particular case is harmless, it's better to clear the timeout inside the callback before calling `close()` to avoid the race. Move `this.connectionTimeoutId = undefined;` after the close() call or remove it from the callback entirely since it will be cleared in the onclose handler.
frontend/sync-client/src/sync-operations/unrestricted-syncer.ts
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-01-04 15:36:32 +00:00
Copy link

When oldPath is undefined but response.relativePath != originalRelativePath is true (meaning the server changed the path), using originalRelativePath as the movedFrom value is incorrect. The file wasn't actually moved from originalRelativePath locally - this was a server-side path change. Consider handling this case separately or using the document's actual previous path from metadata.

When `oldPath` is undefined but `response.relativePath != originalRelativePath` is true (meaning the server changed the path), using `originalRelativePath` as the `movedFrom` value is incorrect. The file wasn't actually moved from `originalRelativePath` locally - this was a server-side path change. Consider handling this case separately or using the document's actual previous path from metadata.
sync-server/src/utils/find_first_available_path.rs
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-01-04 15:36:31 +00:00
Copy link

The log message is misleading because it's placed inside the loop iteration where a candidate path is found to be taken, but it says 'Finding first available path' which suggests it's starting the search. Consider moving this log to line 12 before the loop starts, or rewording it to 'Candidate path {candidate} is already taken, trying next path'.

            "Candidate path `{candidate}` is already taken while finding first available path for `{sanitized_relative_path}` in vault `{vault_id}`, trying next candidate"
The log message is misleading because it's placed inside the loop iteration where a candidate path is found to be taken, but it says 'Finding first available path' which suggests it's starting the search. Consider moving this log to line 12 before the loop starts, or rewording it to 'Candidate path `{candidate}` is already taken, trying next path'. ```suggestion "Candidate path `{candidate}` is already taken while finding first available path for `{sanitized_relative_path}` in vault `{vault_id}`, trying next candidate" ```
This pull request has changes conflicting with the target branch.
  • .github/workflows/check.yml
  • .github/workflows/deploy-docs.yml
  • .github/workflows/e2e.yml
  • .github/workflows/publish-plugin.yml
  • .gitignore
  • CLAUDE.md
  • frontend/deterministic-tests/README.md
  • frontend/deterministic-tests/package.json
  • frontend/deterministic-tests/src/cli.ts
  • frontend/deterministic-tests/src/consts.ts
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin asch/smart-create:asch/smart-create
git checkout asch/smart-create

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main
git merge --no-ff asch/smart-create
git checkout asch/smart-create
git rebase main
git checkout main
git merge --ff-only asch/smart-create
git checkout asch/smart-create
git rebase main
git checkout main
git merge --no-ff asch/smart-create
git checkout main
git merge --squash asch/smart-create
git checkout main
git merge --ff-only asch/smart-create
git checkout main
git merge asch/smart-create
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something isn't working

  
dependencies

Pull requests that update a dependency file

  
docker

Pull requests that update docker code

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update javascript code

  
question

Further information is requested

  
rust

Pull requests that update rust code

  
wontfix

This will not be worked on

No labels
bug
dependencies
docker
documentation
duplicate
enhancement
good first issue
help wanted
invalid
javascript
question
rust
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: andras/vault-link#184
No description provided.
Delete branch "asch/smart-create"

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?