Skip to content

Conversation

@ggazzo
Copy link
Member

@ggazzo ggazzo commented Nov 28, 2025

Summary by CodeRabbit

  • Refactor
    • Reorganized state type definitions across federation and room packages for improved consistency.
    • Consolidated state type handling to strengthen internal type safety.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

Walkthrough

This PR consolidates state type definitions by introducing a new shared State interface with a typed get method, removing local type declarations across federation-sdk and room packages, and renaming the existing State type to StateEventIdMap to prevent naming conflicts.

Changes

Cohort / File(s) Summary
State type consolidation (federation-sdk)
packages/federation-sdk/src/services/state.service.spec.ts, packages/federation-sdk/src/services/state.service.ts
Removed local State type declaration and imports; updated to use imported State from @rocket.chat/federation-room. Methods buildStateFromEvents and _resolveState now return State instead of Map<StateMapKey, PersistentEventBase>.
New State interface definition
packages/room/src/manager/event-wrapper.ts
Added exported State interface extending Map<StateMapKey, PersistentEventBase> with a generic typed get method that performs type-narrowing based on key pattern matching (returns PersistentEventBase<RoomVersion, I> when key matches ${infer I}:${string} where I extends PduType).
State resolution type updates
packages/room/src/state_resolution/definitions/algorithm/v2.ts
Imported new State type; updated resolveStateV2Plus return type to Promise<State> and cast unconflictedStateMap initialization to State.
State definitions refactoring
packages/room/src/state_resolution/definitions/definitions.ts
Updated function signatures to use StateEventIdMap instead of State for event-ID-centric state representations; iterativeAuthChecks now returns Promise<State> and internal variables newState and authEventStateMap use correct types.
Type alias rename
packages/room/src/types/_common.ts
Renamed exported type State to StateEventIdMap to clarify that it represents Map<StateMapKey, EventID> and avoid naming collision with the new State interface.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Type consistency validation: Verify all method return types and variable assignments align with the new State interface across federation-sdk and room packages.
  • Generic get method typing: Review the conditional type logic in the new State.get method to ensure type-narrowing works correctly for all expected key patterns.
  • Rename propagation: Confirm StateEventIdMap is used consistently in all state-resolution definitions and that no references to the old State type name persist in those contexts.

Possibly related PRs

Suggested reviewers

  • rodrigok
  • sampaiodiego

Poem

🐰 Hoppy typing tales so fine,
State maps now share a design,
Generic gets and narrowing grace,
Type-safe hopping through state space!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 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 refactoring effort: introducing a State type interface to replace raw Map type annotations throughout the codebase.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/state-type

📜 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 2833339 and ca61e55.

📒 Files selected for processing (6)
  • packages/federation-sdk/src/services/state.service.spec.ts (1 hunks)
  • packages/federation-sdk/src/services/state.service.ts (4 hunks)
  • packages/room/src/manager/event-wrapper.ts (1 hunks)
  • packages/room/src/state_resolution/definitions/algorithm/v2.ts (3 hunks)
  • packages/room/src/state_resolution/definitions/definitions.ts (4 hunks)
  • packages/room/src/types/_common.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.

Applied to files:

  • packages/room/src/state_resolution/definitions/algorithm/v2.ts
  • packages/room/src/types/_common.ts
  • packages/room/src/manager/event-wrapper.ts
  • packages/federation-sdk/src/services/state.service.ts
  • packages/federation-sdk/src/services/state.service.spec.ts
  • packages/room/src/state_resolution/definitions/definitions.ts
🧬 Code graph analysis (3)
packages/room/src/state_resolution/definitions/algorithm/v2.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
  • State (47-55)
packages/federation-sdk/src/index.ts (1)
  • State (49-49)
packages/federation-sdk/src/specs/federation-api.ts (1)
  • State (171-171)
packages/federation-sdk/src/services/state.service.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
  • State (47-55)
packages/federation-sdk/src/index.ts (1)
  • State (49-49)
packages/federation-sdk/src/specs/federation-api.ts (1)
  • State (171-171)
packages/room/src/state_resolution/definitions/definitions.ts (4)
packages/room/src/types/_common.ts (3)
  • StateEventIdMap (24-24)
  • StateMapKey (22-22)
  • EventID (8-8)
packages/federation-sdk/src/index.ts (2)
  • EventID (31-31)
  • State (49-49)
packages/room/src/manager/event-wrapper.ts (2)
  • State (47-55)
  • event (139-148)
packages/federation-sdk/src/specs/federation-api.ts (1)
  • State (171-171)
🔇 Additional comments (12)
packages/federation-sdk/src/services/state.service.spec.ts (1)

3-3: LGTM! Import change aligns with centralized State type.

The import of State from @rocket.chat/federation-room replaces the local type declaration, consolidating the type definition to a single source. This is consistent with the new State interface defined in event-wrapper.ts.

packages/room/src/state_resolution/definitions/algorithm/v2.ts (2)

61-61: LGTM! Import updated to use centralized State type.

The import correctly brings in both PersistentEventBase and the new State interface from the event wrapper module.


86-86: Type cast is acceptable but consider a factory helper.

The as State cast at line 141 is necessary because new Map() doesn't satisfy the typed get method signature of the State interface. While this works correctly at runtime (since the interface only adds type-level refinements to get), you might consider adding a utility function like createState() to avoid scattered casts if this pattern recurs elsewhere.

Also applies to: 141-141

packages/room/src/manager/event-wrapper.ts (1)

47-55: Well-designed State interface with type-narrowed access.

The interface elegantly provides type-safe access to state events:

  • Extending Map maintains backward compatibility with existing code
  • The conditional return type on get allows callers to receive properly typed events based on the key pattern
  • For example, state.get('m.room.member:@user:server') will return PersistentEventBase<RoomVersion, 'm.room.member'> | undefined

This is a clean approach to adding type safety without runtime overhead.

packages/federation-sdk/src/services/state.service.ts (3)

17-17: LGTM! State type now imported from centralized location.

The import consolidates the State type definition, ensuring consistency across the codebase.


355-361: Type annotations are correct for State initialization.

The explicit State type annotation on new Map() at lines 356 and 374 ensures the maps conform to the State interface. This is the appropriate pattern since plain Map creation doesn't satisfy the interface's typed get signature automatically.

Also applies to: 374-374


835-838: Return type update aligns with new State type.

The _resolveState method now correctly returns Promise<State> instead of Promise<Map<StateMapKey, PersistentEventBase>>, which is semantically equivalent but provides the typed get method benefit.

packages/room/src/state_resolution/definitions/definitions.ts (4)

2-6: LGTM! Imports correctly differentiate between ID-based and event-based state types.

The updated imports properly separate:

  • StateEventIdMap for ID-based state mappings (used in partitionState, getAuthChainDifference)
  • State for full event-based state mappings (used in iterativeAuthChecks)

This distinction adds semantic clarity to the codebase.

Also applies to: 12-12


54-55: Return type correctly uses StateEventIdMap.

The partitionState function returns state maps containing event IDs (not full events), so StateEventIdMap is the appropriate type. This aligns with how the function extracts event.eventId rather than storing the full event.


154-156: Parameter type correctly uses StateEventIdMap.

The getAuthChainDifference function iterates over states and accesses eventid values (line 163), making StateEventIdMap the correct parameter type.


581-585: Return type and local variables correctly use State.

The iterativeAuthChecks function works with full PersistentEventBase events, so using the State type (which maps to events, not IDs) is correct. The authEventStateMap at line 585 is also correctly typed as State since it stores full events retrieved from the store.

packages/room/src/types/_common.ts (1)

24-24: Perfect! The verification is complete. The results confirm that:

  1. No imports of the old State type from _common.ts exist - all imports are correctly using StateMapKey, EventID, etc.
  2. The bare State usages in other files (definitions.ts and state.service.ts) are correctly importing the new State interface from event-wrapper.ts, not from _common.ts
  3. The types are correctly differentiated:
    • StateEventIdMap (from _common.ts): Map<StateMapKey, EventID> — maps state keys to event IDs only
    • State (from event-wrapper.ts): Map<StateMapKey, PersistentEventBase> — maps state keys to full event objects

The rename is properly isolated and all usages have been correctly updated.


Type rename is correctly implemented with all usages properly updated.

The rename from State to StateEventIdMap in _common.ts successfully avoids the naming collision with the new State interface in event-wrapper.ts. Verification confirms no code is importing the old State from _common.ts, and all files correctly reference the new State from event-wrapper.ts where full event objects are needed.


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.

@ggazzo ggazzo marked this pull request as ready for review November 28, 2025 13:26
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.84%. Comparing base (2833339) to head (ca61e55).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   52.84%   52.84%   -0.01%     
==========================================
  Files          96       96              
  Lines       12583    12581       -2     
==========================================
- Hits         6650     6648       -2     
  Misses       5933     5933              

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

@sampaiodiego sampaiodiego merged commit 7b454d2 into main Nov 28, 2025
3 checks passed
@sampaiodiego sampaiodiego deleted the chore/state-type branch November 28, 2025 17:33
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