Skip to content

Conversation

@deepakbhagatiitr
Copy link

@deepakbhagatiitr deepakbhagatiitr commented Nov 16, 2025

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

  • Updated isReadOnly to accept the permission as a parameter instead of reading it statically.
  • Made RoomView subscribe to permission changes from Redux.
  • Added detection so the component re-renders when the permission changes.
  • The composer now enables/disables immediately when permissions change.

Changes

  • Modified isReadOnly to accept postReadOnlyPermission as an optional parameter.
  • Updated RoomView to get the permission from Redux props.
  • Added permission change detection in shouldComponentUpdate.
  • The component now updates immediately when the permission changes

Summary by CodeRabbit

  • Refactor
    • Read-only logic updated to honor an optional post-readonly permission source, keeping backward compatibility.
    • Room view now reacts to changes in that permission so read-only status and UI updates happen more reliably when permissions change.

- 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
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 16, 2025

Walkthrough

Adds an optional postReadOnlyPermission parameter that flows from Redux into RoomView props and is forwarded to the isReadOnly helper; isReadOnly and its internal canPostReadOnly use the provided permission when present, otherwise fall back to the existing Redux snapshot.

Changes

Cohort / File(s) Summary
Helper Function Enhancement
app/lib/methods/helpers/isReadOnly.ts
Extended isReadOnly(room, username)isReadOnly(room, username, postReadOnlyPermission?: string[]). canPostReadOnly now accepts/uses the optional permission argument and falls back to the stored Redux snapshot when undefined.
Interface Definition
app/views/RoomView/definitions.ts
Added optional prop postReadOnlyPermission?: string[] to IRoomViewProps.
Component Integration
app/views/RoomView/index.tsx
Mapped state.permissions['post-readonly']postReadOnlyPermission in mapStateToProps; included the prop in shouldComponentUpdate comparisons and passed it into isReadOnly in setReadOnly.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify state.permissions['post-readonly'] access and typing in mapStateToProps.
  • Inspect shouldComponentUpdate dequal comparison to ensure it includes the new prop correctly.
  • Confirm isReadOnly and canPostReadOnly handle undefined permission consistently and preserve previous behavior.

Suggested reviewers

  • diegolmello

Poem

🐰 I hopped through Redux fields today,

found a permission along the way.
I carried it gentle, soft and light,
through props and helpers — out of sight.
Optional, tidy — then all was right. 🎩✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: making the readonly permission reactive in RoomView by fixing a bug where permission changes weren't immediately reflected in the UI.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 67cb546 and 401bb5e.

📒 Files selected for processing (1)
  • app/lib/methods/helpers/isReadOnly.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/lib/methods/helpers/isReadOnly.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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: Passing postReadOnlyPermission into isReadOnly is correct; consider avoiding redundant setState

Wiring postReadOnlyPermission through to isReadOnly here 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 readOnly actually 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 setState calls.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 3532447 and 67cb546.

📒 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: postReadOnlyPermission prop wiring looks consistent

The new optional postReadOnlyPermission?: string[] aligns with the existing permission props on IRoomViewProps and with how it’s mapped from Redux in RoomView. No issues here.

app/views/RoomView/index.tsx (3)

248-287: shouldComponentUpdate correctly reacts to postReadOnlyPermission changes

Including postReadOnlyPermission in the props comparison (with dequal) ensures that Redux permission updates will no longer be blocked by shouldComponentUpdate, allowing the component to rerender and recompute readOnly when the permission changes. This matches the PR’s intent.


316-319: componentDidUpdate comment matches the intended read‑only refresh behavior

The updated comments clarify that setReadOnly is called after every update so the read‑only state is always derived from the latest room and permission data. Given shouldComponentUpdate already filters out no‑op state updates, this is acceptable from a behavior standpoint.


1570-1588: Redux mapping for postReadOnlyPermission is consistent with other permissions

Mapping postReadOnlyPermission: state.permissions['post-readonly'] mirrors how the other permission props are sourced and matches the IRoomViewProps type. 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant