fix: persist attachment collapsed state across channel switches#40365
fix: persist attachment collapsed state across channel switches#40365SYED-MHAMIL wants to merge 2 commits intoRocketChat:developfrom
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughThe PR adds attachment-level collapse state persistence to ChangesAttachment Collapse State Persistence
Sequence DiagramsequenceDiagram
participant User as User
participant AudioAtt as AudioAttachment<br/>(or other file type)
participant MsgColl as MessageCollapsible
participant UseCollapse as useCollapse Hook
participant Storage as sessionStorage
User->>AudioAtt: Render attachment (id: "abc123")
AudioAtt->>MsgColl: Render with attachmentId="abc123"
MsgColl->>UseCollapse: Call useCollapse("abc123", initialCollapsed)
UseCollapse->>Storage: Read value from key "persistant-attachment-abc123"
Storage-->>UseCollapse: Return stored collapsed state (or undefined)
UseCollapse-->>MsgColl: Return [collapsed, toggle] + CollapsibleContent
MsgColl-->>AudioAtt: Render collapsible with toggle button
User->>MsgColl: Click collapse/expand button
MsgColl->>UseCollapse: Call toggle() handler
UseCollapse->>UseCollapse: Update local collapsed state
UseCollapse->>Storage: Write new state to "persistant-attachment-abc123"
Storage-->>UseCollapse: Write complete
User->>User: Switch to another channel
User->>User: Switch back to original channel
AudioAtt->>MsgColl: Re-render attachment (same id: "abc123")
MsgColl->>UseCollapse: Call useCollapse("abc123", initialCollapsed)
UseCollapse->>Storage: Read value from "persistant-attachment-abc123"
Storage-->>UseCollapse: Return previously stored state
UseCollapse-->>MsgColl: Return [collapsed, toggle] with persisted state
MsgColl-->>User: Render attachment in saved collapse state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 6/8 reviews remaining, refill in 12 minutes and 30 seconds.Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/components/message/hooks/useCollapse.tsx`:
- Line 52: The debug console.log in the useCollapse hook is printing "reset
state" on every render; remove that console.log('reset state') from the hook
(or, if you want to keep it for local debugging only, guard it behind a
development-only check such as NODE_ENV or __DEV__) so the hook (useCollapse) no
longer emits render-time logs in production.
- Around line 35-42: When attachmentId changes the useEffect currently only
calls setCollapsed if sessionStorage has a value, so the previous attachment's
state can leak; update the effect in useEffect that reads
`persistant-attachment-${attachmentId}` to call setCollapsed(initialCollapsed)
when stored is null (i.e., no saved entry) so the new attachment resets to the
default; ensure you reference `attachmentId`, `setCollapsed`, and
`initialCollapsed` inside that effect (and add `initialCollapsed` to the
dependency array if it isn’t already).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f2b84ea-8cc8-46d1-834b-8c4dd5aa9df2
📒 Files selected for processing (7)
apps/meteor/client/components/message/MessageCollapsible.tsxapps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/content/attachments/file/ImageAttachment.tsxapps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsxapps/meteor/client/components/message/content/urlPreviews/OEmbedCollapsible.tsxapps/meteor/client/components/message/hooks/useCollapse.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/message/content/attachments/file/ImageAttachment.tsxapps/meteor/client/components/message/content/urlPreviews/OEmbedCollapsible.tsxapps/meteor/client/components/message/MessageCollapsible.tsxapps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsxapps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/hooks/useCollapse.tsx
🧠 Learnings (1)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/message/content/attachments/file/ImageAttachment.tsxapps/meteor/client/components/message/content/urlPreviews/OEmbedCollapsible.tsxapps/meteor/client/components/message/MessageCollapsible.tsxapps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsxapps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/hooks/useCollapse.tsx
🔇 Additional comments (6)
apps/meteor/client/components/message/MessageCollapsible.tsx (1)
18-19: Looks good.Forwarding
attachmentIdintouseCollapsematches the new persisted-collapse contract cleanly, and the wrapper still falls back to the existing behavior when no ID is present.apps/meteor/client/components/message/content/attachments/file/ImageAttachment.tsx (1)
28-39: Looks good.Passing the attachment
idthrough here is the right hook-up for per-attachment persistence, and the existing image rendering path is unchanged.apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx (1)
70-87: Looks good.Forwarding
attachmentId={id}aligns this wrapper with the persisted-collapse behavior, and the download/preview logic remains intact.apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx (1)
23-27: Looks good.This forwards the attachment identifier into the persisted-collapse path without changing the audio playback behavior.
apps/meteor/client/components/message/content/urlPreviews/OEmbedCollapsible.tsx (1)
14-20: No concerns.This is formatting-only and does not change the collapse behavior or props passed to
MessageCollapsible.apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
24-32: Looks good.Threading
attachmentIdthrough here is consistent with the persisted-collapse update, and the video rendering path remains unchanged.
| useEffect(() => { | ||
| if (attachmentId) { | ||
| const stored = sessionStorage.getItem(`persistant-attachment-${attachmentId}`); | ||
| if (stored !== null) { | ||
| setCollapsed(stored === 'true'); | ||
| } | ||
| } | ||
| }, [attachmentId]); |
There was a problem hiding this comment.
Reset to the new attachment's default when storage is empty.
When attachmentId changes, this effect only updates state if sessionStorage already has a value. If the new attachment has no saved entry yet, the hook keeps the previous attachment’s collapsed state instead of reinitializing from initialCollapsed.
🔧 Proposed fix
useEffect(() => {
if (attachmentId) {
const stored = sessionStorage.getItem(`persistant-attachment-${attachmentId}`);
- if (stored !== null) {
- setCollapsed(stored === 'true');
- }
+ setCollapsed(stored === null ? initialCollapsed : stored === 'true');
}
-}, [attachmentId]);
+}, [attachmentId, initialCollapsed]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (attachmentId) { | |
| const stored = sessionStorage.getItem(`persistant-attachment-${attachmentId}`); | |
| if (stored !== null) { | |
| setCollapsed(stored === 'true'); | |
| } | |
| } | |
| }, [attachmentId]); | |
| useEffect(() => { | |
| if (attachmentId) { | |
| const stored = sessionStorage.getItem(`persistant-attachment-${attachmentId}`); | |
| setCollapsed(stored === null ? initialCollapsed : stored === 'true'); | |
| } | |
| }, [attachmentId, initialCollapsed]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/components/message/hooks/useCollapse.tsx` around lines 35
- 42, When attachmentId changes the useEffect currently only calls setCollapsed
if sessionStorage has a value, so the previous attachment's state can leak;
update the effect in useEffect that reads
`persistant-attachment-${attachmentId}` to call setCollapsed(initialCollapsed)
when stored is null (i.e., no saved entry) so the new attachment resets to the
default; ensure you reference `attachmentId`, `setCollapsed`, and
`initialCollapsed` inside that effect (and add `initialCollapsed` to the
dependency array if it isn’t already).
|
|
||
| export const useCollapse = (attachmentId : string | undefined,attachmentCollapsed?: boolean): [collapsed: boolean, node: ReactNode] => { | ||
|
|
||
| console.log('reset state') |
There was a problem hiding this comment.
Remove the render-time debug log.
console.log('reset state') runs on every render and will leak noise into production consoles.
🧹 Proposed fix
- console.log('reset state')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('reset state') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/components/message/hooks/useCollapse.tsx` at line 52, The
debug console.log in the useCollapse hook is printing "reset state" on every
render; remove that console.log('reset state') from the hook (or, if you want to
keep it for local debugging only, guard it behind a development-only check such
as NODE_ENV or __DEV__) so the hook (useCollapse) no longer emits render-time
logs in production.
1 participant
Proposed changes
Message attachments (images, GIFs, videos, audio, files) store their collapsed/expanded state only in React component state. When a user switches channels, the message list unmounts and all in-memory state is lost — so every manually collapsed attachment re-expands on return.
This fix replaces the in-memory toggle in
useCollapsewithuseStatebacked bysessionStorage, keyed on the attachment ID. On mount the hook reads the stored value; every toggle writes it back. The user's collapse choice now survives channel switches for the duration of the browser session.Files changed:
useCollapse.tsx— AddedusePersistedCollapsehelper; updated hook to acceptattachmentIdparameter and use sessionStorageMessageCollapsible.tsx— AddedattachmentIdprop, passes it touseCollapseImageAttachment.tsx— Destructuresidand passes asattachmentIdtoMessageCollapsibleVideoAttachment.tsx— Destructuresidand passes asattachmentIdtoMessageCollapsibleAudioAttachment.tsx— Destructuresidand passes asattachmentIdtoMessageCollapsibleGenericFileAttachment.tsx— Destructuresidand passes asattachmentIdtoMessageCollapsibleIssue(s)
Closes #40337
Steps to test or reproduce
After: attachment stays collapsed
Further comments
sessionStoragewas chosen overlocalStorageso the state does not persist across browser sessions or bleed between different users on the same machineattachmentIdis optional; components without IDs fall back to regular toggle behaviorsessionStorageAPIbefore fix: https://github.com/user-attachments/assets/44c08ca8-9f5b-418f-9339-3627bcf1afa4
after fix: https://github.com/user-attachments/assets/e2d14435-d68d-4272-be10-17fb577711a0
Summary by CodeRabbit