Skip to content

Conversation

@debdutdeb
Copy link
Member

@debdutdeb debdutdeb commented Sep 19, 2025

https://rocketchat.atlassian.net/browse/FDR-140

Summary by CodeRabbit

  • Bug Fixes
    • Improved stability when processing room state by safely handling missing power settings, preventing rare crashes and sync failures in rooms with incomplete or corrupted history.
    • Reduces unexpected errors during membership changes, event authorization, and timeline loading in affected rooms.
    • Users should experience fewer sync interruptions, smoother room access, and more consistent behavior across clients when joining or viewing older rooms.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Introduces a null-safe fallback for missing power level events in resolveStateV2Plus by defaulting to a new PowerLevelEvent().toEventBase() when not found in partiallyResolvedState. Adds the corresponding import and removes an unused import. No other logic or public APIs are altered.

Changes

Cohort / File(s) Summary of Changes
State resolution v2 power level fallback
packages/room/src/state_resolution/definitions/algorithm/v2.ts
Added import for PowerLevelEvent; removed unused import. In resolveStateV2Plus, replaced non-null assertion of power level event with a nullish-coalescing fallback to new PowerLevelEvent().toEventBase(), changing control flow to always provide a default event base when missing.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant R as resolveStateV2Plus
  participant S as partiallyResolvedState
  participant P as PowerLevelEvent

  R->>S: get("m.room.power_levels")
  alt Event exists
    S-->>R: powerLevelEventBase
    note right of R: Use retrieved event base
  else Missing
    R->>P: new PowerLevelEvent()
    P-->>R: toEventBase()
    note right of R: Use default event base
  end

  R->>R: Continue state resolution with event base
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nudge the state with gentle paws,
When power’s lost, I mind the laws—
A default seed I softly place,
So flows can hop at steady pace.
Thump-thump! No gaps, no fright—
The room now keeps its power right. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: use default power level when one isn't in state" directly and concisely reflects the primary change in the diff: replacing a required power_level assertion with a fallback default PowerLevelEvent when the event is missing. It is specific to the behavioral fix, uses clear language, and is understandable by teammates scanning the commit history.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fdr140

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

@debdutdeb debdutdeb marked this pull request as ready for review September 19, 2025 20:39
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.01%. Comparing base (19bcb9b) to head (be2b806).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #206   +/-   ##
=======================================
  Coverage   81.01%   81.01%           
=======================================
  Files          63       63           
  Lines        4692     4692           
=======================================
  Hits         3801     3801           
  Misses        891      891           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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: 0

🧹 Nitpick comments (1)
packages/room/src/state_resolution/definitions/algorithm/v2.ts (1)

269-273: Good fallback; verify the synthetic PL event shape won’t break mainline ordering.

Defaulting to a PL event avoids crashes when it’s missing, but please confirm:

  • PowerLevelEvent().toEventBase() returns a PersistentEventBase with type 'm.room.power_levels' and state_key ''.
  • It includes required fields used by mainlineOrdering (e.g., eventId, origin_server_ts) or that mainlineOrdering does not depend on them for the PL pivot.
  • Passing a non-persisted/synthetic event won’t trigger wrappedStore lookups by eventId.

If any of the above aren’t guaranteed, consider guarding mainlineOrdering to treat a “synthetic PL” as a pure config carrier (no lookups), or inject a sentinel eventId to avoid accidental fetches.

Also suggest adding a brief comment here to document why a default PL is safe per the Matrix spec defaults.

Add a focused test to lock this in:

// pseudo-test
it('resolves when no m.room.power_levels in partiallyResolvedState', async () => {
  // construct states with no PL event
  const result = await resolveStateV2Plus(statesWithoutPL, store);
  expect(result).toBeInstanceOf(Map);
  // optionally: assert deterministic ordering of a small conflicting set
});
📜 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 19bcb9b and be2b806.

📒 Files selected for processing (1)
  • packages/room/src/state_resolution/definitions/algorithm/v2.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/room/src/state_resolution/definitions/algorithm/v2.ts (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
  • getStateMapKey (10-15)
🔇 Additional comments (1)
packages/room/src/state_resolution/definitions/algorithm/v2.ts (1)

62-62: Import looks correct and scoped to usage.

Pathing aligns with existing manager imports. No issues.

@ggazzo ggazzo merged commit 47941d6 into main Sep 23, 2025
3 checks passed
@ggazzo ggazzo deleted the fdr140 branch September 23, 2025 21:27
@coderabbitai coderabbitai bot mentioned this pull request Nov 3, 2025
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.

4 participants