Skip to content

Conversation

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Sep 21, 2025

Summary by CodeRabbit

  • New Features

    • Enhanced support for message relations: edits, reactions, and threads with optional fallback and nested reply metadata.
    • Added optional URL field on messages for richer content handling.
    • Expanded media/thumbnail metadata for messages (mimetype, dimensions, size, thumbnail file/info and hashes).
  • Refactor

    • Standardized message type across message payloads for stronger validation and consistency, including in edited/new content.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 21, 2025

Walkthrough

Typed updates to m.room.message: msgtype now uses MessageType; added optional url; replaced m.relates_to with a discriminated union (m.replace, m.annotation, m.thread with optional m.in_reply_to/is_falling_back); m.new_content.msgtype switched to MessageType; added content.info and thumbnail/metadata shapes; staging, core, and federation-sdk types adjusted accordingly.

Changes

Cohort / File(s) Summary
Federation SDK types
packages/federation-sdk/src/index.ts
Tightened m.room.message typings: msgtypeMessageType; added optional url; replaced m.relates_to with discriminated union (m.replace / m.annotation / m.thread with optional m.in_reply_to/is_falling_back); m.new_content.msgtypeMessageType; added content.info with thumbnail and metadata shapes.
Staging/emit service
packages/federation-sdk/src/services/staging-area.service.ts
Added MessageType import; updated emitted m.room.message content shapes to use MessageType for msgtype and explicit m.relates_to union; m.new_content.msgtype typed as MessageType.
Core event types
packages/core/src/events/m.room.message.ts
Extended relation types: added RelationTypeAnnotation and RelationTypeThread; RelationType includes 'm.thread'; MessageRelation union extended to include annotation/thread variants and optional m.in_reply_to/is_falling_back.
Invite service typing
packages/federation-sdk/src/services/invite.service.ts
Narrowed processInvite input to concrete PduForType<'m.room.member'>; added imports (PduForType, PersistentEventBase, PersistentEventFactory); updated call to PersistentEventFactory.createFromRawEvent<'m.room.member'>(...).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant Staging as StagingAreaService
  participant Core as CoreTypes
  participant FedSDK as FederationSDK
  participant Invite as InviteService
  participant Homeserver

  Client->>Staging: submit raw m.room.message
  Staging->>Core: validate/assign relation variant & info shape
  Core-->>Staging: relation variant (replace/annotation/thread) & info metadata
  Staging->>FedSDK: emit typed m.room.message (msgtype: MessageType, url?, new_content, info)
  FedSDK-->>Homeserver: send typed event
  Homeserver-->>FedSDK: ack/result
  Note right of Invite: invite processing now accepts typed PduForType<'m.room.member'> and uses PersistentEventFactory.createFromRawEvent<'m.room.member'>
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • sampaiodiego

Poem

I nibble on types in a cosy heap,
Threads and edits now hop, not creep.
Msgtypes snug, and URLs peek through,
Thumbnails tucked with metadata too.
A rabbit's cheer for tidy types anew 🐇✨

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 is a short, single sentence that accurately and concisely summarizes the primary change in the patch — improving the types for the homeserver.matrix.message event (e.g., msgtype -> MessageType, expanded m.relates_to, added url/info fields) — and is clearly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/improve-message-types

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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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 @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2025

Codecov Report

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #207   +/-   ##
=======================================
  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: 3

🧹 Nitpick comments (2)
packages/federation-sdk/src/services/staging-area.service.ts (1)

1-5: Use type‑only import for MessageType to avoid runtime import.

Importing a type as a value can bloat bundles and break under TS transpilation configs.

Apply this diff:

-import type { EventBase, EventStagingStore, Membership } from '@hs/core';
+import type { EventBase, EventStagingStore, Membership, MessageType } from '@hs/core';
@@
-import { MessageType, createLogger, isRedactedEvent } from '@hs/core';
+import { createLogger, isRedactedEvent } from '@hs/core';
packages/federation-sdk/src/index.ts (1)

118-141: Extract a reusable type alias for the relation shape.

Define and export a MessageRelatesTo type to avoid duplication/divergence across files.

Proposed change within this file:

+export type MessageRelatesTo =
+	| { rel_type: 'm.replace'; event_id: EventID }
+	| { rel_type: 'm.annotation'; event_id: EventID; key: string }
+	| {
+			rel_type: 'm.thread';
+			event_id: EventID;
+			'm.in_reply_to'?: {
+				event_id: EventID;
+				room_id?: string;
+				sender?: string;
+				origin_server_ts?: number;
+			};
+			is_falling_back?: boolean;
+	  }
+	| { 'm.in_reply_to': { event_id: EventID } };
@@
-			'm.relates_to'?:
-				| { rel_type: 'm.replace'; event_id: EventID }
-				| { rel_type: 'm.annotation'; event_id: EventID; key: string }
-				| {
-						rel_type: 'm.thread';
-						event_id: EventID;
-						'm.in_reply_to'?: { event_id: EventID; room_id?: string; sender?: string; origin_server_ts?: number };
-						is_falling_back?: boolean;
-				  };
+			'm.relates_to'?: MessageRelatesTo;
📜 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 17fc361.

📒 Files selected for processing (2)
  • packages/federation-sdk/src/index.ts (2 hunks)
  • packages/federation-sdk/src/services/staging-area.service.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/staging-area.service.ts (2)
packages/core/src/events/m.room.message.ts (1)
  • MessageType (8-11)
packages/room/src/types/_common.ts (1)
  • EventID (8-8)
packages/federation-sdk/src/index.ts (2)
packages/core/src/events/m.room.message.ts (1)
  • MessageType (8-11)
packages/room/src/types/_common.ts (1)
  • EventID (8-8)

Comment on lines +118 to 141
msgtype: MessageType;
url?: string;
'm.relates_to'?:
| {
rel_type: 'm.replace';
event_id: EventID;
}
| {
rel_type: 'm.annotation';
event_id: EventID;
key: string;
}
| {
rel_type: 'm.thread';
event_id: EventID;
'm.in_reply_to'?: {
event_id: EventID;
room_id: string;
sender: string;
origin_server_ts: number;
};
is_falling_back?: boolean;
};
'm.new_content'?: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add reply‑only variant to m.relates_to union.

Replies without a rel_type (only m.in_reply_to) are valid; excluding them can break consumers.

Apply this diff:

 			'm.relates_to'?:
 				| {
 						rel_type: 'm.replace';
 						event_id: EventID;
 				  }
 				| {
 						rel_type: 'm.annotation';
 						event_id: EventID;
 						key: string;
 				  }
 				| {
 						rel_type: 'm.thread';
 						event_id: EventID;
-						'm.in_reply_to'?: {
-							event_id: EventID;
-							room_id: string;
-							sender: string;
-							origin_server_ts: number;
-						};
+						'm.in_reply_to'?: {
+							event_id: EventID;
+							room_id?: string;
+							sender?: string;
+							origin_server_ts?: number;
+						};
 						is_falling_back?: boolean;
-				  };
+				  }
+				| {
+						'm.in_reply_to': {
+							event_id: EventID;
+						};
+				  };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
msgtype: MessageType;
url?: string;
'm.relates_to'?:
| {
rel_type: 'm.replace';
event_id: EventID;
}
| {
rel_type: 'm.annotation';
event_id: EventID;
key: string;
}
| {
rel_type: 'm.thread';
event_id: EventID;
'm.in_reply_to'?: {
event_id: EventID;
room_id: string;
sender: string;
origin_server_ts: number;
};
is_falling_back?: boolean;
};
'm.new_content'?: {
msgtype: MessageType;
url?: string;
'm.relates_to'?:
| {
rel_type: 'm.replace';
event_id: EventID;
}
| {
rel_type: 'm.annotation';
event_id: EventID;
key: string;
}
| {
rel_type: 'm.thread';
event_id: EventID;
'm.in_reply_to'?: {
event_id: EventID;
room_id?: string;
sender?: string;
origin_server_ts?: number;
};
is_falling_back?: boolean;
}
| {
'm.in_reply_to': {
event_id: EventID;
};
};
'm.new_content'?: {
🤖 Prompt for AI Agents
In packages/federation-sdk/src/index.ts around lines 118 to 141, the
m.relates_to union currently requires a rel_type for all variants but Matrix
allows a reply-only form that contains only m.in_reply_to; add a union variant
that models an object with an 'm.in_reply_to' property (matching the existing
shape: event_id: EventID, room_id: string, sender: string, origin_server_ts:
number) and any necessary optional flags (e.g., is_falling_back?: boolean)
without a rel_type so reply-only relations are accepted by the type.

Comment on lines +151 to +165
msgtype: event.event.content?.msgtype as MessageType,
'm.relates_to': event.event.content?.['m.relates_to'] as
| {
rel_type: 'm.replace';
event_id: EventID;
}
| {
rel_type: 'm.annotation';
event_id: EventID;
key: string;
}
| {
rel_type: 'm.thread';
event_id: EventID;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Only include optional m.relates_to when present; align shape with index (incl. replies).

Asserting a non‑optional union while passing possibly undefined is unsound. Also add the reply‑only variant to match your signature in index.ts.

Apply this diff:

-						'm.relates_to': event.event.content?.['m.relates_to'] as
-							| {
-									rel_type: 'm.replace';
-									event_id: EventID;
-							  }
-							| {
-									rel_type: 'm.annotation';
-									event_id: EventID;
-									key: string;
-							  }
-							| {
-									rel_type: 'm.thread';
-									event_id: EventID;
-							  },
+						...(event.event.content?.['m.relates_to']
+							? {
+									'm.relates_to': event.event.content['m.relates_to'] as
+										| {
+												rel_type: 'm.replace';
+												event_id: EventID;
+										  }
+										| {
+												rel_type: 'm.annotation';
+												event_id: EventID;
+												key: string;
+										  }
+										| {
+												rel_type: 'm.thread';
+												event_id: EventID;
+												'm.in_reply_to'?: {
+													event_id: EventID;
+													room_id: string;
+													sender: string;
+													origin_server_ts: number;
+												};
+												is_falling_back?: boolean;
+										  }
+										| {
+												'm.in_reply_to': {
+													event_id: EventID;
+												};
+										  };
+							  }
+							: {}),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
msgtype: event.event.content?.msgtype as MessageType,
'm.relates_to': event.event.content?.['m.relates_to'] as
| {
rel_type: 'm.replace';
event_id: EventID;
}
| {
rel_type: 'm.annotation';
event_id: EventID;
key: string;
}
| {
rel_type: 'm.thread';
event_id: EventID;
},
msgtype: event.event.content?.msgtype as MessageType,
...(event.event.content?.['m.relates_to']
? {
'm.relates_to': event.event.content['m.relates_to'] as
| {
rel_type: 'm.replace';
event_id: EventID;
}
| {
rel_type: 'm.annotation';
event_id: EventID;
key: string;
}
| {
rel_type: 'm.thread';
event_id: EventID;
'm.in_reply_to'?: {
event_id: EventID;
room_id: string;
sender: string;
origin_server_ts: number;
};
is_falling_back?: boolean;
}
| {
'm.in_reply_to': {
event_id: EventID;
};
};
}
: {}),

Comment on lines 166 to 169
'm.new_content': event.event.content?.['m.new_content'] as {
body: string;
msgtype: string;
msgtype: MessageType;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Don’t emit m.new_content when absent.

Current code emits the key with an undefined value; keep it optional.

Apply this diff:

-						'm.new_content': event.event.content?.['m.new_content'] as {
-							body: string;
-							msgtype: MessageType;
-						},
+						...(event.event.content?.['m.new_content']
+							? {
+									'm.new_content': event.event.content['m.new_content'] as {
+										body: string;
+										msgtype: MessageType;
+									};
+							  }
+							: {}),

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/staging-area.service.ts around lines 166
to 169, the code always emits the 'm.new_content' key with a possibly undefined
value; change it to only include that property when
event.event.content?.['m.new_content'] is present (non-null/undefined).
Implement this by conditionally adding the property (e.g., check if value !=
null and then assign or spread the key/value) so the key is omitted entirely
when absent.

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

🧹 Nitpick comments (4)
packages/core/src/events/m.room.message.ts (2)

105-115: Make m.thread m.in_reply_to subfields optional.

Room, sender and ts can be absent; only event_id is required. This avoids over‑constraining consumers.

  export type RelationTypeThread = {
    rel_type: 'm.thread';
    event_id: EventID;
    'm.in_reply_to'?: {
      event_id: EventID;
-     room_id: string;
-     sender: string;
-     origin_server_ts: number;
+     room_id?: string;
+     sender?: string;
+     origin_server_ts?: number;
    };
    is_falling_back?: boolean;
  };

53-57: Add encrypted attachment parity (file + thumbnail_file).

Core FileMessageContent supports thumbnail_url/thumbnail_info but not the encrypted forms used in the SDK (thumbnail_file) and the main encrypted payload (file). Add both to avoid divergent models.

Example addition to FileMessageContent:

// top-level of FileMessageContent (besides url)
file?: {
  hashes: { sha256: string };
  iv: string;
  key: {
    alg: string;
    ext: boolean;
    k: string;
    key_ops: Array<'encrypt' | 'decrypt'>;
    kty: string;
  };
  url: string;
  v: 'v2';
};

// inside info:
thumbnail_file?: {
  hashes: { sha256: string };
  iv: string;
  key: {
    alg: string;
    ext: boolean;
    k: string;
    key_ops: Array<'encrypt' | 'decrypt'>;
    kty: string;
  };
  url: string;
  v: 'v2';
};
packages/federation-sdk/src/index.ts (2)

118-120: Consider adding content.file for encrypted attachments.

You added info.thumbnail_file but the main encrypted payload is typically under content.file. Add it to cover encrypted media.

       content: {
         body: string;
-        msgtype: MessageType;
-        url?: string;
+        msgtype: MessageType;
+        url?: string;
+        file?: {
+          hashes: { sha256: string };
+          iv: string;
+          key: {
+            alg: string;
+            ext: boolean;
+            k: string;
+            key_ops: Array<'encrypt' | 'decrypt'>;
+            kty: string;
+          };
+          url: string;
+          v: 'v2';
+        };

147-173: Reduce duplication by reusing a shared AttachmentInfo type.

This block duplicates attachment metadata now diverging from core. Consider exporting a shared AttachmentInfo (and EncryptedFile) type from @hs/core and reusing here.

📜 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 17fc361 and cda4ca2.

📒 Files selected for processing (2)
  • packages/core/src/events/m.room.message.ts (2 hunks)
  • packages/federation-sdk/src/index.ts (2 hunks)
🔇 Additional comments (2)
packages/federation-sdk/src/index.ts (2)

141-146: LGTM: new_content.msgtype uses MessageType.

Good tightening of the type surface; this will prevent stray msgtypes from leaking in.


121-140: Add reply‑only variant and relax m.thread m.in_reply_to fields.

Replies that contain only m.in_reply_to (no rel_type) are valid; also, room_id, sender, and origin_server_ts should be optional.

       'm.relates_to'?:
         | {
             rel_type: 'm.replace';
             event_id: EventID;
           }
         | {
             rel_type: 'm.annotation';
             event_id: EventID;
             key: string;
           }
         | {
             rel_type: 'm.thread';
             event_id: EventID;
             'm.in_reply_to'?: {
               event_id: EventID;
-              room_id: string;
-              sender: string;
-              origin_server_ts: number;
+              room_id?: string;
+              sender?: string;
+              origin_server_ts?: number;
             };
             is_falling_back?: boolean;
-          };
+          }
+        | {
+            'm.in_reply_to': {
+              event_id: EventID;
+              room_id?: string;
+              sender?: string;
+              origin_server_ts?: number;
+            };
+          };

Run to confirm we have producers/consumers for reply‑only:

#!/bin/bash
rg -nP -C2 "m\\.relates_to[\"']?\\s*:\\s*\\{\\s*['\"]m\\.in_reply_to['\"]" --type=ts
rg -nP -C2 "rel_type:\\s*'m\\.thread'|rel_type:\\s*\"m\\.thread\"" --type=ts

Comment on lines 76 to +86
export type MessageRelation = {
rel_type: RelationType;
event_id: EventID;
} & (RelationTypeReplace | Record<string, never>);
} & (
| RelationTypeReplace
| RelationTypeAnnotation
| RelationTypeThread
| Record<string, never>
);

export type RelationType = 'm.replace' | 'm.annotation';
export type RelationType = 'm.replace' | 'm.annotation' | 'm.thread';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Support reply‑only relations in m.relates_to; don’t force rel_type.

Matrix replies can be expressed as { "m.relates_to": { "m.in_reply_to": { event_id } } } without a rel_type. The current MessageRelation shape requires rel_type, rejecting valid reply‑only events.

Apply this diff to relax the type and stop mandating rel_type:

-export type MessageRelation = {
-	rel_type: RelationType;
-	event_id: EventID;
-} & (
-	| RelationTypeReplace
-	| RelationTypeAnnotation
-	| RelationTypeThread
-	| Record<string, never>
-);
+export type MessageRelation =
+	| RelationTypeReplace
+	| RelationTypeAnnotation
+	| RelationTypeThread;

Outside this range, add a reply‑only variant and widen the reference on BaseMessageContent:

// add near the relation types
export type ReplyRelation = {
  'm.in_reply_to': {
    event_id: EventID;
    room_id?: string;
    sender?: string;
    origin_server_ts?: number;
  };
};

// update BaseMessageContent (line 19)
'm.relates_to'?: MessageRelation | ReplyRelation;
🤖 Prompt for AI Agents
In packages/core/src/events/m.room.message.ts around lines 76 to 86 and update
BaseMessageContent at line 19: the current MessageRelation forces a rel_type
which rejects reply‑only forms; add a new ReplyRelation type (with
'm.in_reply_to' containing event_id and optional room_id, sender,
origin_server_ts) near the relation types, make rel_type optional on the
existing MessageRelation (or add a variant that omits rel_type), and update
BaseMessageContent's 'm.relates_to' at line 19 to accept MessageRelation |
ReplyRelation so reply‑only relations are allowed.

Comment on lines +147 to +173
info?: {
mimetype?: string;
w?: number;
h?: number;
size?: number;
thumbnail_file?: {
hashes: {
sha256: string;
};
iv: string;
key: {
alg: string;
ext: boolean;
k: string;
key_ops: ['encrypt' | 'decrypt'];
kty: string;
};
url: string;
v: 'v2';
};
thumbnail_info?: {
w?: number;
h?: number;
size?: number;
mimetype?: string;
};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix key.key_ops typing; current form is a one‑element tuple.

key_ops: ['encrypt' | 'decrypt'] means “array with exactly one element”. Use an array of allowed ops (or a fixed tuple if you require both).

           thumbnail_file?: {
             hashes: {
               sha256: string;
             };
             iv: string;
             key: {
               alg: string;
               ext: boolean;
               k: string;
-              key_ops: ['encrypt' | 'decrypt'];
+              key_ops: Array<'encrypt' | 'decrypt'>;
               kty: string;
             };
             url: string;
             v: 'v2';
           };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
info?: {
mimetype?: string;
w?: number;
h?: number;
size?: number;
thumbnail_file?: {
hashes: {
sha256: string;
};
iv: string;
key: {
alg: string;
ext: boolean;
k: string;
key_ops: ['encrypt' | 'decrypt'];
kty: string;
};
url: string;
v: 'v2';
};
thumbnail_info?: {
w?: number;
h?: number;
size?: number;
mimetype?: string;
};
};
info?: {
mimetype?: string;
w?: number;
h?: number;
size?: number;
thumbnail_file?: {
hashes: {
sha256: string;
};
iv: string;
key: {
alg: string;
ext: boolean;
k: string;
key_ops: Array<'encrypt' | 'decrypt'>;
kty: string;
};
url: string;
v: 'v2';
};
thumbnail_info?: {
w?: number;
h?: number;
size?: number;
mimetype?: string;
};
};
🤖 Prompt for AI Agents
In packages/federation-sdk/src/index.ts around lines 147 to 173, the key.key_ops
type is currently declared as a single‑element tuple (`key_ops: ['encrypt' |
'decrypt']`) which enforces exactly one entry; change it to an array of allowed
ops by replacing that type with a string array type such as `Array<'encrypt' |
'decrypt'>` or `('encrypt' | 'decrypt')[]` (or use a fixed tuple
`['encrypt','decrypt']` only if you require both values always present).

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

🧹 Nitpick comments (1)
packages/federation-sdk/src/services/invite.service.ts (1)

2-6: Prefer type-only imports for types; add explicit raw PDU types

Import values and types separately to avoid pulling types into runtime and to support a strongly-typed raw PDU in processInvite.

-import {
-	PersistentEventBase,
-	PersistentEventFactory,
-	RoomVersion,
-} from '@hs/room';
+import { PersistentEventFactory } from '@hs/room';
+import type {
+	PersistentEventBase,
+	RoomVersion,
+	PduForType,
+	PduWithHashesAndSignaturesOptional,
+} from '@hs/room';
📜 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 cda4ca2 and 7bf7795.

📒 Files selected for processing (1)
  • packages/federation-sdk/src/services/invite.service.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/room/src/manager/type.ts (1)
  • RoomVersion (14-14)
packages/room/src/manager/factory.ts (1)
  • PersistentEventFactory (37-144)

@ggazzo ggazzo merged commit 901f242 into main Sep 22, 2025
2 of 3 checks passed
@ggazzo ggazzo deleted the chore/improve-message-types branch September 22, 2025 13:27
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 (2)
packages/federation-sdk/src/services/invite.service.ts (2)

2-7: Align imports with raw-PDU typing

To reflect that processInvite should accept a raw PDU (hashes/signatures may be absent before local signing), import PduWithHashesAndSignaturesOptional so we can use it in the parameter type below.

-import {
-	PduForType,
-	PersistentEventBase,
-	PersistentEventFactory,
-	RoomVersion,
-} from '@hs/room';
+import {
+	PduForType,
+	PduWithHashesAndSignaturesOptional,
+	PersistentEventBase,
+	PersistentEventFactory,
+	RoomVersion,
+} from '@hs/room';

129-134: Accept raw PDU (hashes/signatures optional) and declare explicit return type

Change the parameter to the raw PDU shape the factory expects and make the public API return type explicit.

-	async processInvite(
-		event: PduForType<'m.room.member'>,
-		roomId: string,
-		eventId: string,
-		roomVersion: RoomVersion,
-	) {
+	async processInvite(
+		event: PduWithHashesAndSignaturesOptional<PduForType<'m.room.member'>>,
+		roomId: string,
+		eventId: string,
+		roomVersion: RoomVersion,
+	): Promise<PersistentEventBase<RoomVersion, 'm.room.member'>> {

PduWithHashesAndSignaturesOptional is exported at packages/room/src/manager/event-wrapper.ts; call site to verify: packages/homeserver/src/controllers/federation/invite.controller.ts (passes body.event). Add the import and update types as needed.

📜 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 7bf7795 and ba7a37b.

📒 Files selected for processing (1)
  • packages/federation-sdk/src/services/invite.service.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
  • event (102-111)
  • roomId (72-74)
packages/room/src/types/_common.ts (1)
  • PduForType (14-14)
packages/room/src/manager/factory.ts (1)
  • PersistentEventFactory (37-144)
🔇 Additional comments (1)
packages/federation-sdk/src/services/invite.service.ts (1)

142-146: Type-safe factory usage — good change

Explicitly parameterizing createFromRawEvent with 'm.room.member' removes the prior unsafe cast and tightens types. LGTM.

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