-
Notifications
You must be signed in to change notification settings - Fork 537
fixed session deletion #3905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fixed session deletion #3905
Conversation
✅ Deploy Preview for hyprnote canceled.
|
✅ Deploy Preview for hyprnote-storybook canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| addDeletion: (data, onConfirm) => { | ||
| const sessionId = data.session.id; | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| get().confirmDeletion(sessionId); | ||
| }, UNDO_TIMEOUT_MS); | ||
|
|
||
| set((state) => ({ | ||
| pendingDeletions: { | ||
| ...state.pendingDeletions, | ||
| [sessionId]: { | ||
| data, | ||
| timeoutId, | ||
| isPaused: false, | ||
| remainingTime: UNDO_TIMEOUT_MS, | ||
| onDeleteConfirm: onConfirm ?? null, | ||
| addedAt: Date.now(), | ||
| }, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 addDeletion does not clear existing timeout when re-adding the same sessionId
When addDeletion is called for a sessionId that already has a pending deletion, the old timeout is not cleared before creating a new one. This leaks the old setTimeout and causes premature confirmation of the replacement deletion.
Root Cause and Impact
In the old code, setTimeoutId explicitly cleared any existing timeout (apps/desktop/src/store/zustand/undo-delete.ts old lines 90-95):
setTimeoutId: (id) => {
const currentId = get().timeoutId;
if (currentId) { clearTimeout(currentId); }
set({ timeoutId: id });
}The new addDeletion at apps/desktop/src/store/zustand/undo-delete.ts:81-100 creates a new setTimeout and overwrites the entry in pendingDeletions without clearing the previous timeout:
addDeletion: (data, onConfirm) => {
const sessionId = data.session.id;
const timeoutId = setTimeout(() => {
get().confirmDeletion(sessionId);
}, UNDO_TIMEOUT_MS);
set((state) => ({ pendingDeletions: { ...state.pendingDeletions, [sessionId]: { ... } } }));
}If a session is deleted, undone via the restore button (which calls clearDeletion), and then deleted again, this is safe because clearDeletion clears the timeout. However, if addDeletion is called twice for the same session without an intervening clearDeletion (e.g., the user triggers delete from the context menu while the dissolving animation is still active), the old timeout fires first and calls confirmDeletion(sessionId). This executes the NEW onDeleteConfirm callback prematurely (before the full 5-second undo window elapses for the second deletion), and then clears the pending deletion entirely. The second timeout then fires on an already-cleared entry and is a no-op.
Impact: The undo window for the replacement deletion is shortened to whatever time remained on the original timeout, potentially confirming a destructive delete before the user has the full 5 seconds to undo.
| addDeletion: (data, onConfirm) => { | |
| const sessionId = data.session.id; | |
| const timeoutId = setTimeout(() => { | |
| get().confirmDeletion(sessionId); | |
| }, UNDO_TIMEOUT_MS); | |
| set((state) => ({ | |
| pendingDeletions: { | |
| ...state.pendingDeletions, | |
| [sessionId]: { | |
| data, | |
| timeoutId, | |
| isPaused: false, | |
| remainingTime: UNDO_TIMEOUT_MS, | |
| onDeleteConfirm: onConfirm ?? null, | |
| addedAt: Date.now(), | |
| }, | |
| }, | |
| })); | |
| addDeletion: (data, onConfirm) => { | |
| const sessionId = data.session.id; | |
| const existing = get().pendingDeletions[sessionId]; | |
| if (existing?.timeoutId) { | |
| clearTimeout(existing.timeoutId); | |
| } | |
| const timeoutId = setTimeout(() => { | |
| get().confirmDeletion(sessionId); | |
| }, UNDO_TIMEOUT_MS); | |
| set((state) => ({ | |
| pendingDeletions: { | |
| ...state.pendingDeletions, | |
| [sessionId]: { | |
| data, | |
| timeoutId, | |
| isPaused: false, | |
| remainingTime: UNDO_TIMEOUT_MS, | |
| onDeleteConfirm: onConfirm ?? null, | |
| addedAt: Date.now(), | |
| }, | |
| }, | |
| })); | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
Changes