Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes race conditions that occur when users quickly switch between notes, which could result in data from one note being saved to or displayed for another note. The fix involves capturing note IDs before async operations and validating that the current note ID hasn't changed after async operations complete.
Changes:
- Added race condition checks in the load() function to prevent setting note data if the current ID changed during the async fetch
- Captured note ID before save operations to ensure content is saved to the correct note even if the user switches notes during the save
- Updated the noteTitle watcher to use note ID instead of route path for better consistency
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| watch(noteTitle, (currentNoteTitle) => { | ||
| if (route.name == 'note') { | ||
| patchOpenedPageByUrl( | ||
| route.path, | ||
| { | ||
| title: currentNoteTitle, | ||
| url: route.path, | ||
| }); | ||
| if (route.name == 'note' && currentId.value !== null) { | ||
| /** | ||
| * URL may have changed, use note id | ||
| */ | ||
| const noteUrl = `/note/${currentId.value}`; | ||
|
|
||
| patchOpenedPageByUrl(noteUrl, { | ||
| title: currentNoteTitle, | ||
| url: noteUrl, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The noteTitle watcher has a potential race condition. While currentId.value is checked, it's not captured at the time the watcher is triggered. If currentId changes between when noteTitle is recalculated and when this watcher callback runs, the title from one note could be used to update the opened page for a different note. Consider capturing currentId.value at the start of the watcher callback, similar to how savedNoteId is captured in the save function.
| title: currentNoteTitle, | ||
| url: route.path, | ||
| }); | ||
| if (route.name == 'note' && currentId.value !== null) { |
There was a problem hiding this comment.
Use strict equality (===) instead of loose equality (==) for consistency with the rest of the codebase and to avoid potential type coercion issues.
| if (route.name == 'note' && currentId.value !== null) { | |
| if (route.name === 'note' && currentId.value !== null) { |
|
Please explain the exact root cause of the problem in the PR description and how your fix resolves it. |
| const specifiedNoteTools = resolveToolsByContent(content); | ||
|
|
||
| /** | ||
| * Id may be changed |
There was a problem hiding this comment.
JSdoc is not clear for me, please explain the case.
| const response = await noteService.getNoteById(id); | ||
|
|
||
| /** | ||
| * Id changed, loaded another note |
There was a problem hiding this comment.
please, explain the case more straightforward
| if (currentId.value !== id) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
can't understand why we need to check this condition one more time here (you've checked it right above, what is the difference here?)
3 participants
Fixed autosave and quick tab switching. The note id might not match the id in the url