fix(stickies): restore full state on failed delete#9064
Conversation
- Add trailing slash to getSticky and deleteSticky API paths, fixing unnecessary 301 redirect - Snapshot workspaceStickies, activeStickyId, and recentStickyId before optimistic delete so all three are restored if the API call fails - Re-throw the error so callers show an error toast instead of success Fixes makeplane#9050
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes deleted stickies reappearing after reload by adding trailing slashes to sticky service endpoints and by snapshotting/restoring relevant store state and re-throwing original errors on update/delete failures. ChangesSticky Deletion Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/store/sticky/sticky.store.ts (1)
193-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
updateStickyrollback uses a mutated reference, so failed updates are not actually reverted.Line 194 stores a reference to
this.stickies[id]; Line 200 mutates that same object. On failure, Line 207 reassigns already-mutated data.Suggested fix
updateSticky = async (workspaceSlug: string, id: string, updates: Partial<TSticky>) => { const sticky = this.stickies[id]; if (!sticky) return; + const previousSticky = structuredClone(sticky); try { runInAction(() => { Object.keys(updates).forEach((key) => { const currentStickyKey = key as keyof TSticky; set(this.stickies[id], key, updates[currentStickyKey] || undefined); }); }); this.recentStickyId = id; await this.stickyService.updateSticky(workspaceSlug, id, updates); } catch (error) { console.error("Error in updating sticky:", error); - this.stickies[id] = sticky; + this.stickies[id] = previousSticky; throw error; } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/core/store/sticky/sticky.store.ts` around lines 193 - 208, In updateSticky, the rollback currently reassigns the same mutated object because sticky = this.stickies[id] is a reference; fix by making a deep copy of the original sticky (e.g., use structuredClone or cloneDeep) before runInAction and mutations, then on catch restore this.stickies[id] = originalCopy; ensure you copy before calling set(...) inside updateSticky so the restore returns the pre-update state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/core/store/sticky/sticky.store.ts`:
- Around line 215-220: The deleteSticky logic can throw when
workspaceStickies[workspaceSlug] is undefined; update the code in deleteSticky
to safely default to an empty array before filtering and when taking the backup:
set previousWorkspaceStickies using (this.workspaceStickies[workspaceSlug] ||
[]) and perform the delete with this.workspaceStickies[workspaceSlug] =
(this.workspaceStickies[workspaceSlug] || []).filter(stickyId => stickyId !==
id); keep previousActiveStickyId and previousRecentStickyId unchanged and ensure
the same safe fallback is used anywhere else in deleteSticky that reads
workspaceStickies[workspaceSlug].
---
Outside diff comments:
In `@apps/web/core/store/sticky/sticky.store.ts`:
- Around line 193-208: In updateSticky, the rollback currently reassigns the
same mutated object because sticky = this.stickies[id] is a reference; fix by
making a deep copy of the original sticky (e.g., use structuredClone or
cloneDeep) before runInAction and mutations, then on catch restore
this.stickies[id] = originalCopy; ensure you copy before calling set(...) inside
updateSticky so the restore returns the pre-update state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd37e238-5dbe-46d2-b6f1-dcd579b5427f
📒 Files selected for processing (2)
apps/web/core/services/sticky.service.tsapps/web/core/store/sticky/sticky.store.ts
|
|
2 participants
When deleting a sticky note fails (e.g. network error), the frontend performs a partial rollback. This leaves
the sticky invisible in the list even though its data still exists in memory. The sticky only reappears after a full page reload.
Additionally, the success toast ("Sticky has been successfully removed") was still shown even when the API call failed, because the store swallowed the error instead of re-throwing it.
I've also added a trailing slash to the
deleteStickyandgetStickyAPI paths to avoid an unneeded 301 redirectFixes #9050
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Release Notes