Fix: Deleting the attachment from the card lead to error page#184
Fix: Deleting the attachment from the card lead to error page#184Rajat-Dabade wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGuarded attachment updates in the attachments store: the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/src/store/attachments.ts (1)
48-50: Consider adding similar defensive check to the reducer.The reducer directly accesses
state.attachments[action.payload.blockId]without verifying the attachment exists. While this action is typically dispatched during upload (when the attachment should exist), applying the same defensive pattern would provide consistency and guard against edge cases.🛡️ Optional hardening
updateUploadPrecent: (state, action: PayloadAction<{blockId: string, uploadPercent: number}>) => { - state.attachments[action.payload.blockId].uploadingPercent = action.payload.uploadPercent + if (state.attachments[action.payload.blockId]) { + state.attachments[action.payload.blockId].uploadingPercent = action.payload.uploadPercent + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/store/attachments.ts` around lines 48 - 50, The reducer updateUploadPrecent directly indexes state.attachments[action.payload.blockId] without checking existence; add a defensive guard in updateUploadPrecent that first reads const attachment = state.attachments[action.payload.blockId] and if (!attachment) return (or no-op), otherwise set attachment.uploadingPercent = action.payload.uploadPercent so the reducer won't throw when the attachment is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webapp/src/store/attachments.ts`:
- Around line 48-50: The reducer updateUploadPrecent directly indexes
state.attachments[action.payload.blockId] without checking existence; add a
defensive guard in updateUploadPrecent that first reads const attachment =
state.attachments[action.payload.blockId] and if (!attachment) return (or
no-op), otherwise set attachment.uploadingPercent = action.payload.uploadPercent
so the reducer won't throw when the attachment is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 241ef987-5e56-4382-816f-9208c4187cc9
📒 Files selected for processing (1)
webapp/src/store/attachments.ts
harshilsharma63
left a comment
harshilsharma63
left a comment
There was a problem hiding this comment.
We need to add tests for this.
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
3 participants
Summary
Deleting an attachment after a hard refresh would successfully call the API and show the success flash message, but then immediately redirect the user to the error page.
When an attachment is deleted, the server sends an update through WebSocket saying the attachment is deleted. The Redux store updates correctly and removes that attachment. At that exact moment, the AttachmentElement component is still on the screen. So when the selector runs again, it tries to find the attachment using the old blockId, but it’s already gone, and now it gets undefined. Then the code tries to access .uploadingPercent on that undefined value, which causes a crash. This is caught by
ErrorBoundary, and redirects the user to/error?id=unknown.Fix: Added optional chaining fallback so the selector safely returns 0 when the block no longer exists in the store
Steps to Reproduce
Before fix: redirected to the error page despite the API succeeding
After fix: attachment is deleted cleanly with the success flash message
Ticket Link
https://mattermost.atlassian.net/browse/MM-68059
Change Impact: 🟢 Low
Regression Risk: Minimal. Changes are confined to a single Redux selector and a reducer guard in webapp/src/store/attachments.ts to defensively handle missing attachment entries (returns 0 instead of accessing undefined). No behavioral changes for valid states, no shared utilities or auth/data persistence touched, and blast radius is limited to attachment rendering flows.
QA Recommendation: Minimal manual QA. Validate the reported scenario (hard refresh, reopen card with attachment, delete attachment) to ensure deletion succeeds and no error-page redirect occurs. Automated rollback is straightforward.
Generated by CodeRabbitAI