chore: state management DTOs and update profiles controller types #195
Conversation
WalkthroughAdds federation state DTOs for GetState and GetStateIds, re-exports them, updates a controller to import these DTOs and removes a type assertion, adjusts the MakeJoin response DTO (membership union, removes origin), and extends the StateStore type in the federation SDK with an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Remote HS (Client)
participant API as Homeserver API (Profiles Controller)
participant SVC as Profiles/State Service
participant REPO as State Repository
participant DB as Database
Note over C,API: GET /_matrix/federation/v1/state_ids/:roomId?event_id=...
C->>API: Request (validated by GetStateIds* DTOs)
API->>SVC: getStateIds(roomId, event_id)
SVC->>REPO: fetchStateIds(roomId, event_id)
REPO->>DB: query (uses StateStore with _id)
DB-->>REPO: pdu_ids, auth_chain_ids
REPO-->>SVC: state ids
SVC-->>API: GetStateIdsResponse
API-->>C: 200 { pdu_ids, auth_chain_ids }
rect rgba(200,235,255,0.25)
Note right of API: New DTO-driven validation paths<br/>and repository type includes _id
end
sequenceDiagram
autonumber
participant C as Remote HS (Client)
participant API as Homeserver API (Profiles Controller)
participant SVC as Profiles/State Service
participant REPO as State Repository
participant DB as Database
Note over C,API: GET /_matrix/federation/v1/state/:roomId?event_id=...
C->>API: Request (validated by GetState* DTOs)
API->>SVC: getState(roomId, event_id)
SVC->>REPO: fetchState(roomId, event_id)
REPO->>DB: read state/auth chain events
DB-->>REPO: events
REPO-->>SVC: pdus, auth_chain
SVC-->>API: GetStateResponse
API-->>C: 200 { pdus, auth_chain }
Note over API: make_join handler now returns typed value (no as any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
=======================================
Coverage 80.41% 80.41%
=======================================
Files 58 58
Lines 4514 4514
=======================================
Hits 3630 3630
Misses 884 884 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/membership-types
branch
from
48215d2 to
396d77f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/federation-sdk/src/repositories/state.repository.ts (1)
37-41: "Latest" sort is reversed — should be descending by createdAtThe method name says "Latest" but the query sorts ascending, returning the earliest document.
Apply this diff:
- return this.collection.findOne({ roomId }, { sort: { createdAt: 1 } }); + return this.collection.findOne({ roomId }, { sort: { createdAt: -1 } });packages/homeserver/src/dtos/federation/profiles.dto.ts (1)
57-69: Include Matrix room version '12' or derive versions from a central source (don’t hardcode 1..11)packages/homeserver/src/dtos/federation/profiles.dto.ts:57–69 currently enumerates '1'..'11'; Matrix v12 became stable on 2025-08-11 — add '12' or read room versions from a central enum/config to avoid future drift.
🧹 Nitpick comments (10)
packages/federation-sdk/src/repositories/state.repository.ts (5)
33-35: Validate stateId before constructing ObjectIdPassing an invalid string to new ObjectId(...) throws. Return null or a typed error early.
Apply this diff:
- return this.collection.findOne({ _id: new ObjectId(stateId) }); + if (!ObjectId.isValid(stateId)) { + return null; + } + return this.collection.findOne({ _id: new ObjectId(stateId) });
55-61: Harden $in query against invalid ObjectId stringsMap guards will prevent runtime exceptions and accidental full scans.
Apply this diff:
- return this.collection - .find({ _id: { $in: stateIds.map((id) => new ObjectId(id)) } }) - .sort({ createdAt: 1 /* order as is saved */ }); + const ids = stateIds + .filter((id) => ObjectId.isValid(id)) + .map((id) => new ObjectId(id)); + return this.collection + .find({ _id: { $in: ids } }) + .sort({ createdAt: 1 /* chronological order */ });
37-47: Add supporting indexes for the queried patternsFrequent queries sort/filter by roomId and createdAt. Add compound indexes to avoid COLLSCANs.
- Create: { roomId: 1, createdAt: -1 } (covers latest and ascending scans)
- Create: { roomId: 1, 'delta.identifier': 1 }
- Optional: { 'delta.identifier': 1 } if cross-room lookups are common
101-103: Clarify the TODO and fix the typo"whit" -> "with". Consider explaining the StateMapKey format (e.g., "type:state_key") to justify the colon requirement.
- // TODO: why it must to end whit `:` ? + // TODO: Document why StateMapKey must end with ':' (format 'type:state_key'?).
14-16: Use StateStore (not WithId) for the collection and all return typesStateStore already declares _id; WithId is redundant. Update types consistently:
- packages/federation-sdk/src/container.ts (lines 84,86): change register<Collection<WithId>> → register<Collection> and useValue: db.collection('states').
- packages/federation-sdk/src/repositories/state.repository.ts: change private readonly collection: Collection<WithId> → Collection and replace all WithId occurrences (method returns, FindCursor, InsertOneResult) with StateStore. Affected lines: 31, 33, 39, 45, 51, 57, 66, 73, 91, 100.
- private readonly collection: Collection<WithId<StateStore>>, + private readonly collection: Collection<StateStore>,I can push a follow‑up diff updating all return types.
packages/homeserver/src/dtos/federation/state-ids.dto.ts (1)
8-10: Validate event_id with a dedicated EventIdDtoStronger validation reduces bad queries early and aligns with other DTOs (you already have RoomIdDto).
Apply this diff:
-import { RoomIdDto } from '../common/validation.dto'; +import { RoomIdDto, EventIdDto } from '../common/validation.dto'; @@ export const GetStateIdsQueryDto = t.Object({ - event_id: t.String({ description: 'Event ID to get state at' }), + event_id: EventIdDto, });Add this helper in common/validation.dto.ts (outside this file):
export const EventIdDto = t.String({ pattern: '^\\$[A-Za-z0-9_=/\\.\\-\\+]+:(.+)$', description: 'Matrix event ID in format $event:server.com', examples: ['$abc123:example.com'], });I can open a follow-up PR to add EventIdDto and replace usages in state.dto.ts as well.
packages/homeserver/src/controllers/federation/profiles.controller.ts (2)
85-85: Let the service negotiate room versions (avoid forcing ['1'] fallback)Defaulting to ['1'] risks negotiating an old version. Prefer passing ver as-is and letting ProfilesService choose sensible defaults.
- return profilesService.makeJoin(roomId, userId, ver ?? ['1']); + return profilesService.makeJoin(roomId, userId, ver);Confirm the third parameter is optional in ProfilesService.makeJoin; if not, prefer injecting a server-default list (e.g., from config).
2-2: Remove unused import: RoomVersion-import { type RoomVersion } from '@hs/room';packages/homeserver/src/dtos/federation/state.dto.ts (2)
8-10: Use EventIdDto for event_id (same rationale as state-ids DTO)Keeps validation consistent and stricter.
-import { RoomIdDto } from '../common/validation.dto'; +import { RoomIdDto, EventIdDto } from '../common/validation.dto'; @@ export const GetStateQueryDto = t.Object({ - event_id: t.String({ description: 'Event ID to get state at' }), + event_id: EventIdDto, });
12-19: Consider reusing a canonical Event DTO instead of Record<string, any>If you already define an event shape in common/event.dto, referencing it here improves type-safety and docs. If events are heterogeneous by design, ignore this.
Is there a suitable event DTO in common/event.dto that matches the PDU structure returned here?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/federation-sdk/src/repositories/state.repository.ts(1 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/dtos/federation/profiles.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state-ids.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/homeserver/src/dtos/federation/state-ids.dto.ts (1)
packages/homeserver/src/dtos/common/validation.dto.ts (1)
RoomIdDto(9-13)
packages/homeserver/src/dtos/federation/state.dto.ts (1)
packages/homeserver/src/dtos/common/validation.dto.ts (1)
RoomIdDto(9-13)
🔇 Additional comments (3)
packages/homeserver/src/dtos/federation/state-ids.dto.ts (1)
12-17: LGTM — schema matches expected shapes (IDs only)packages/homeserver/src/dtos/index.ts (1)
10-11: LGTM — central re-exports wiredpackages/homeserver/src/controllers/federation/profiles.controller.ts (1)
79-86: Typed return withoutas any— good cleanup
| const MembershipDto = t.Union([ | ||
| t.Literal('join'), | ||
| t.Literal('leave'), | ||
| t.Literal('invite'), | ||
| t.Literal('ban'), | ||
| t.Literal('knock'), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t widen membership for make_join; it should be 'join' only
The event template returned by make_join must carry membership 'join'. Allowing other values here weakens validation.
Apply this diff (keep MembershipDto for other contexts, but restrict this field):
- membership: MembershipDto,
+ membership: t.Literal('join'),Also applies to: 91-91
🌐 Web query:
Matrix Federation make_join response: confirm that the event content.membership in the template must be 'join' per the current spec.
💡 Result:
Yes. The make_join response's event.template content.membership is required to be "join". See the Server‑Server API /make_join response which specifies the event.content.membership must be the value "join". [1][2]
Sources:
[1] Matrix spec — Server‑Server API, GET /_matrix/federation/v1/make_join (response “event” / Membership Event Content). [2] Matrix legacy federation doc — /make_join (shows example and membership = "join").
Don’t widen membership for make_join; restrict to 'join'
Matrix /make_join requires event.template.content.membership === "join" — restrict the DTO accordingly.
File: packages/homeserver/src/dtos/federation/profiles.dto.ts (lines 79-86; also applies to line 91)
Apply this diff (keep MembershipDto for other contexts, but restrict this field):
- membership: MembershipDto,
+ membership: t.Literal('join'),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/homeserver/src/dtos/federation/profiles.dto.ts around lines 79-86
(and also adjust usage at line 91), the membership type used for make_join is
too broad; create or use a narrowed type that only allows the literal 'join' for
the make_join path instead of the existing MembershipDto union. Keep the
existing MembershipDto for other contexts, then replace the membership field
used for make_join with a t.Literal('join') (or a new MakeJoinMembershipDto
alias) and update the type at line 91 accordingly so
event.template.content.membership is restricted to "join".
Summary by CodeRabbit