-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: make readonly permission reactive in RoomView #6804
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: develop
Are you sure you want to change the base?
fix: make readonly permission reactive in RoomView #6804
Conversation
- Modified isReadOnly to accept postReadOnlyPermission as optional parameter - Updated RoomView to get permission from Redux via mapStateToProps - Added permission change detection in shouldComponentUpdate - Component now updates immediately when post-readonly permission changes
WalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
participant Redux as Redux State
participant Component as RoomView (mapStateToProps)
participant Helper as isReadOnly()
Redux->>Component: postReadOnlyPermission (state.permissions['post-readonly'])
Component->>Component: shouldComponentUpdate checks prop change
rect rgb(240,248,255)
Note over Component,Helper: Read-only evaluation (new flow)
Component->>Helper: setReadOnly(..., postReadOnlyPermission)
Helper->>Helper: canPostReadOnly(postReadOnlyPermission?)
alt permission provided
Helper-->>Component: use provided permission → determine RO
else fallback
Helper->>Redux: read permission snapshot
Helper-->>Component: determine RO from snapshot
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/views/RoomView/index.tsx (1)
574-579: PassingpostReadOnlyPermissionintoisReadOnlyis correct; consider avoiding redundantsetStateWiring
postReadOnlyPermissionthrough toisReadOnlyhere is the key to making the composer reactive to live permission changes and aligns with the helper’s new signature.If you want to avoid scheduling redundant updates, you could add a small guard so state is only updated when
readOnlyactually changes:setReadOnly = async () => { const { room } = this.state; - const { user, postReadOnlyPermission } = this.props; - const readOnly = await isReadOnly(room as ISubscription, user.username as string, postReadOnlyPermission); - this.setState({ readOnly }); + const { user, postReadOnlyPermission } = this.props; + const readOnly = await isReadOnly(room as ISubscription, user.username as string, postReadOnlyPermission); + if (readOnly !== this.state.readOnly) { + this.setState({ readOnly }); + } };This isn’t required for correctness but trims unnecessary
setStatecalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/lib/methods/helpers/isReadOnly.ts(1 hunks)app/views/RoomView/definitions.ts(1 hunks)app/views/RoomView/index.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/methods/helpers/isReadOnly.ts (2)
app/definitions/ISubscription.ts (1)
ISubscription(40-119)app/lib/methods/helpers/helpers.ts (1)
hasPermission(94-118)
app/views/RoomView/index.tsx (2)
app/lib/methods/helpers/isReadOnly.ts (1)
isReadOnly(17-36)app/definitions/ISubscription.ts (1)
ISubscription(40-119)
🔇 Additional comments (4)
app/views/RoomView/definitions.ts (1)
14-36:postReadOnlyPermissionprop wiring looks consistentThe new optional
postReadOnlyPermission?: string[]aligns with the existing permission props onIRoomViewPropsand with how it’s mapped from Redux inRoomView. No issues here.app/views/RoomView/index.tsx (3)
248-287:shouldComponentUpdatecorrectly reacts topostReadOnlyPermissionchangesIncluding
postReadOnlyPermissionin the props comparison (withdequal) ensures that Redux permission updates will no longer be blocked byshouldComponentUpdate, allowing the component to rerender and recomputereadOnlywhen the permission changes. This matches the PR’s intent.
316-319:componentDidUpdatecomment matches the intended read‑only refresh behaviorThe updated comments clarify that
setReadOnlyis called after every update so the read‑only state is always derived from the latest room and permission data. GivenshouldComponentUpdatealready filters out no‑op state updates, this is acceptable from a behavior standpoint.
1570-1588: Redux mapping forpostReadOnlyPermissionis consistent with other permissionsMapping
postReadOnlyPermission: state.permissions['post-readonly']mirrors how the other permission props are sourced and matches theIRoomViewPropstype. This cleanly closes the loop from Redux → props →isReadOnly.
Add nullish coalescing to handle case where hasPermission returns undefined on error, preventing runtime crash when accessing permission[0]
When an admin grants the "post-readonly" permission while a user is in a read-only room, the composer stays disabled until they navigate away or restart the app. The UI doesn't update immediately.
Issue(s)
Fixes bug: #6802
Solution
isReadOnlyto accept the permission as a parameter instead of reading it statically.Changes
isReadOnlyto acceptpostReadOnlyPermissionas an optional parameter.shouldComponentUpdate.Summary by CodeRabbit