refactor: enhance message content types and schemas #189
Conversation
Updated message content types in the core and federation SDK to better categorize message types into text, file, and location. Introduced new schemas for message content, including detailed structures for file information and new content for edits. This refactor improves type safety and clarity in message handling across the application.
WalkthroughRefactors Matrix m.room.message typing across core, room schema, and federation SDK. Introduces discriminated unions for text/file/location content, explicit message/relation types, and optional m.new_content for edits. Updates MessageService.sendFileMessage to use a shared FileMessageContent type. Adjusts Zod schemas to match the new union-based content model. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
==========================================
+ Coverage 68.48% 68.94% +0.46%
==========================================
Files 58 58
Lines 5210 5288 +78
==========================================
+ Hits 3568 3646 +78
Misses 1642 1642 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/room/src/types/v3-11.ts (2)
341-390: Add m.mentions to BaseMessageContentSchema to match core and avoid rejection if strict mode is enabled.Core types include m.mentions; schema should accept it.
const BaseMessageContentSchema = z.object({ body: z.string().describe('The body of the message.'), @@ formatted_body: z .string() .describe('The formatted body of the message.') .optional(), + 'm.mentions': z + .any() + .describe('Mentions metadata (MSC3952).') + .optional(), });
357-381: Model m.relates_to as a discriminated union to enforce per-rel_type fields (key, m.in_reply_to).Today everything is optional; invalid combos slip through (e.g., annotation without key).
- 'm.relates_to': z - .object({ - rel_type: z - .enum(['m.replace', 'm.annotation', 'm.thread']) - .describe('The type of the relation.') - .optional(), - event_id: z - .string() - .optional() - .describe('The ID of the event that is being related to.'), - is_falling_back: z - .boolean() - .optional() - .describe('Whether this is a fallback for older clients'), - 'm.in_reply_to': z - .object({ - event_id: z - .string() - .describe('The ID of the latest event in the thread for fallback'), - }) - .optional(), - key: z.string().optional().describe('The key for reactions (emoji).'), - }) - .optional() + 'm.relates_to': z + .discriminatedUnion('rel_type', [ + z.object({ + rel_type: z.literal('m.replace'), + event_id: z.string(), + }), + z.object({ + rel_type: z.literal('m.annotation'), + event_id: z.string(), + key: z.string().describe('Reaction key (emoji or aggregating key)'), + }), + z.object({ + rel_type: z.literal('m.thread'), + event_id: z.string(), + is_falling_back: z.boolean().optional(), + 'm.in_reply_to': z.object({ event_id: z.string() }).optional(), + }), + ]) + .optional() .describe('Relation information for edits, replies, reactions, etc.'),packages/core/src/events/m.room.message.ts (1)
75-91: Fix MessageRelation: add m.thread and annotation shape; keep m.new_content only at top-level.Current types omit m.thread and make annotation/thread fields invisible; also duplicate m.new_content inside RelationTypeReplace (confusing vs top-level).
-export type MessageRelation = { - rel_type: RelationType; - event_id: string; -} & (RelationTypeReplace | Record<string, never>); - -export type RelationType = 'm.replace' | 'm.annotation'; - -export type RelationTypeReplace = { - rel_type: 'm.replace'; - event_id: string; - 'm.new_content'?: { - body: string; - msgtype: MessageType; - format?: string; - formatted_body?: string; - }; -}; +export type RelationType = 'm.replace' | 'm.annotation' | 'm.thread'; + +export type ReplaceRelation = { + rel_type: 'm.replace'; + event_id: string; +}; + +export type AnnotationRelation = { + rel_type: 'm.annotation'; + event_id: string; + key: string; +}; + +export type ThreadRelation = { + rel_type: 'm.thread'; + event_id: string; + is_falling_back?: boolean; + 'm.in_reply_to'?: { event_id: string }; +}; + +export type MessageRelation = ReplaceRelation | AnnotationRelation | ThreadRelation;
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/message.service.ts (2)
73-76: Fix typo in error messages (“white” → “while”).User-facing strings at Lines 74 and 113 read “white trying…”. Line 151 is correct. Align all.
-`Room version not found for room ${roomId} white trying to send message`, +`Room version not found for room ${roomId} while trying to send message`,Also applies to: 112-115, 149-153
29-48: Deduplicate FileMessageContent — import canonical type from @hs/core and remove local declarationLocal type in packages/federation-sdk/src/services/message.service.ts (≈lines 30–48) diverges from the canonical FileMessageContent in packages/core/src/events/m.room.message.ts (line 27) — the core type extends BaseMessageContent and includes fields such as m.mentions, format, formatted_body, and m.relates_to. Add a type import (type FileMessageContent) from '@hs/core' to the existing import group and delete the local FileMessageContent block; keep the function signatures using FileMessageContent as-is.
packages/room/src/types/v3-11.ts (1)
393-427: Tighten FileInfo and file URL validation (non-negative integers; mxc:// URIs).Prevents bad data in PDUs and aligns with Matrix content URI expectations — verified repo emits mxc:// (packages/federation-sdk/src/services/media.service.ts) and uses zod int/nonnegative elsewhere.
File: packages/room/src/types/v3-11.ts — apply to FileInfoSchema (lines ~393–427) and FileMessageContentSchema (lines ~435–439).
const FileInfoSchema = z.object({ - size: z.number().describe('The size of the file in bytes.').optional(), - mimetype: z.string().describe('The MIME type of the file.').optional(), - w: z.number().describe('The width of the image/video in pixels.').optional(), - h: z.number().describe('The height of the image/video in pixels.').optional(), - duration: z - .number() + size: z.number().int().nonnegative().describe('The size of the file in bytes.').optional(), + mimetype: z.string().describe('The MIME type of the file.').optional(), + w: z.number().int().positive().describe('The width of the image/video in pixels.').optional(), + h: z.number().int().positive().describe('The height of the image/video in pixels.').optional(), + duration: z + .number().int().nonnegative() .describe('The duration of the audio/video in milliseconds.') .optional(), - thumbnail_url: z - .string() + thumbnail_url: z + .string() + .regex(/^mxc:\/\//, 'The URL of the thumbnail must be an mxc:// URI.') .describe('The URL of the thumbnail image.') .optional(), @@ const FileMessageContentSchema = BaseMessageContentSchema.extend({ msgtype: z.enum(['m.image', 'm.file', 'm.audio', 'm.video']), - url: z.string().describe('The URL of the file.'), + url: z.string().regex(/^mxc:\/\//, 'The URL of the file must be an mxc:// URI.').describe('The URL of the file.'), info: FileInfoSchema.describe('Information about the file.').optional(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/events/m.room.message.ts(3 hunks)packages/federation-sdk/src/services/message.service.ts(2 hunks)packages/room/src/types/v3-11.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/message.service.ts (1)
packages/core/src/events/m.room.message.ts (1)
FileMessageContent(27-44)
packages/core/src/events/m.room.message.ts (1)
packages/federation-sdk/src/services/message.service.ts (1)
FileMessageContent(30-48)
🔇 Additional comments (1)
packages/core/src/events/m.room.message.ts (1)
64-71: m.new_content confirmed top-level only; no nested m.new_content under m.relates_to found.
Search of the repo shows 'm.new_content' used as a top-level sibling to 'm.relates_to' (e.g., packages/room/src/manager/factory.ts ~lines 535–581, packages/core/src/events/m.room.message.ts); no instances of 'm.new_content' nested inside 'm.relates_to' were found.
| // Location message content (m.location) | ||
| const LocationMessageContentSchema = BaseMessageContentSchema.extend({ | ||
| msgtype: z.literal('m.location'), | ||
| geo_uri: z.string().describe('The geo URI of the location.'), | ||
| // Additional location fields can be added here | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate geo_uri scheme.
Guard against arbitrary strings for locations.
const LocationMessageContentSchema = BaseMessageContentSchema.extend({
msgtype: z.literal('m.location'),
- geo_uri: z.string().describe('The geo URI of the location.'),
+ geo_uri: z.string().regex(/^geo:/, 'geo_uri must start with geo:').describe('The geo URI of the location.'),
// Additional location fields can be added here
});📝 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.
| // Location message content (m.location) | |
| const LocationMessageContentSchema = BaseMessageContentSchema.extend({ | |
| msgtype: z.literal('m.location'), | |
| geo_uri: z.string().describe('The geo URI of the location.'), | |
| // Additional location fields can be added here | |
| }); | |
| // Location message content (m.location) | |
| const LocationMessageContentSchema = BaseMessageContentSchema.extend({ | |
| msgtype: z.literal('m.location'), | |
| geo_uri: z.string().regex(/^geo:/, 'geo_uri must start with geo:').describe('The geo URI of the location.'), | |
| // Additional location fields can be added here | |
| }); |
🤖 Prompt for AI Agents
In packages/room/src/types/v3-11.ts around lines 441-447, the geo_uri field
currently accepts any string; change it to validate the URI scheme to prevent
arbitrary values by replacing z.string() with a validation that ensures the
value is a valid URI with an allowed scheme (e.g. starts with "geo:" or matches
a strict URI regex for allowed schemes). Implement using Zod's refine (or regex)
to parse/validate the scheme, provide a clear error message like "geo_uri must
be a valid URI with an allowed scheme (e.g. geo:)", and keep the description;
ensure tests/consumers will reject arbitrary strings and accept valid geo URIs.
| // Main message content schema using discriminated union | ||
| export const PduMessageEventContentSchema = z.union([ | ||
| TextMessageContentSchema.extend({ | ||
| 'm.new_content': NewContentSchema.optional().describe( | ||
| 'The new content for message edits.', | ||
| ), | ||
| }), | ||
| FileMessageContentSchema.extend({ | ||
| 'm.new_content': NewContentSchema.optional().describe( | ||
| 'The new content for message edits.', | ||
| ), | ||
| }), | ||
| LocationMessageContentSchema.extend({ | ||
| 'm.new_content': NewContentSchema.optional().describe( | ||
| 'The new content for message edits.', | ||
| ), | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a real discriminatedUnion on msgtype (current comment says discriminated, code is plain union).
Improves inference and validation performance.
-export const PduMessageEventContentSchema = z.union([
+export const PduMessageEventContentSchema = z.discriminatedUnion('msgtype', [
TextMessageContentSchema.extend({
'm.new_content': NewContentSchema.optional().describe(
'The new content for message edits.',
),
}),
FileMessageContentSchema.extend({
'm.new_content': NewContentSchema.optional().describe(
'The new content for message edits.',
),
}),
LocationMessageContentSchema.extend({
'm.new_content': NewContentSchema.optional().describe(
'The new content for message edits.',
),
}),
]);📝 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.
| // Main message content schema using discriminated union | |
| export const PduMessageEventContentSchema = z.union([ | |
| TextMessageContentSchema.extend({ | |
| 'm.new_content': NewContentSchema.optional().describe( | |
| 'The new content for message edits.', | |
| ), | |
| }), | |
| FileMessageContentSchema.extend({ | |
| 'm.new_content': NewContentSchema.optional().describe( | |
| 'The new content for message edits.', | |
| ), | |
| }), | |
| LocationMessageContentSchema.extend({ | |
| 'm.new_content': NewContentSchema.optional().describe( | |
| 'The new content for message edits.', | |
| ), | |
| }), | |
| ]); | |
| // Main message content schema using discriminated union | |
| export const PduMessageEventContentSchema = z.discriminatedUnion('msgtype', [ | |
| TextMessageContentSchema.extend({ | |
| 'm.new_content': NewContentSchema.optional().describe( | |
| 'The new content for message edits.', | |
| ), | |
| }), | |
| FileMessageContentSchema.extend({ | |
| 'm.new_content': NewContentSchema.optional().describe( | |
| 'The new content for message edits.', | |
| ), | |
| }), | |
| LocationMessageContentSchema.extend({ | |
| 'm.new_content': NewContentSchema.optional().describe( | |
| 'The new content for message edits.', | |
| ), | |
| }), | |
| ]); |
🤖 Prompt for AI Agents
In packages/room/src/types/v3-11.ts around lines 469 to 486, replace the plain
z.union with z.discriminatedUnion on the msgtype field to get proper type
narrowing and faster validation; update each branch to include an explicit
msgtype literal (for example extend TextMessageContentSchema with { msgtype:
z.literal('m.text') }, FileMessageContentSchema with { msgtype:
z.literal('m.file') }, LocationMessageContentSchema with { msgtype:
z.literal('m.location') } or the actual msgtype strings those schemas represent)
and keep the 'm.new_content' optional extension as-is so the discriminated union
matches on msgtype and preserves the edit field.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors Matrix room message events to use more specific, discriminated union types for different message categories (text, file, location) and enhances schema validation for better type safety and maintainability.
- Introduces granular message type categorization with
TextMessageType,FileMessageType, andLocationMessageType - Updates schemas to use discriminated unions for message content validation
- Aligns federation SDK with new type definitions for consistency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/room/src/types/v3-11.ts | Refactors schema to use discriminated unions with dedicated schemas for text, file, and location messages |
| packages/federation-sdk/src/services/message.service.ts | Extracts file message content type and updates method signature for consistency |
| packages/core/src/events/m.room.message.ts | Splits message types into granular categories and updates interfaces to use discriminated unions |
Comments suppressed due to low confidence (1)
packages/federation-sdk/src/services/message.service.ts:1
- The
FileMessageContenttype duplicates the structure already defined inpackages/core/src/events/m.room.message.ts. This creates potential for inconsistencies if one definition is updated but not the other. Consider importing and reusing the type from the core package instead of redefining it.
import {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // New content schema for edits | ||
| const NewContentSchema = z.discriminatedUnion('msgtype', [ | ||
| TextMessageContentSchema.pick({ | ||
| body: true, | ||
| msgtype: true, | ||
| format: true, | ||
| formatted_body: true, | ||
| }), | ||
| FileMessageContentSchema.pick({ | ||
| body: true, | ||
| msgtype: true, | ||
| url: true, | ||
| info: true, | ||
| }), | ||
| LocationMessageContentSchema.pick({ | ||
| body: true, | ||
| msgtype: true, | ||
| geo_uri: true, | ||
| }), |
There was a problem hiding this comment.
The NewContentSchema uses .pick() to select specific fields from each message content schema. This creates a dependency on the structure of the parent schemas and could break if those schemas change. Consider defining explicit schemas for new content to improve maintainability and make the validation logic more explicit.
| // New content schema for edits | |
| const NewContentSchema = z.discriminatedUnion('msgtype', [ | |
| TextMessageContentSchema.pick({ | |
| body: true, | |
| msgtype: true, | |
| format: true, | |
| formatted_body: true, | |
| }), | |
| FileMessageContentSchema.pick({ | |
| body: true, | |
| msgtype: true, | |
| url: true, | |
| info: true, | |
| }), | |
| LocationMessageContentSchema.pick({ | |
| body: true, | |
| msgtype: true, | |
| geo_uri: true, | |
| }), | |
| // Explicit schemas for new content edits | |
| const NewTextMessageContentEditSchema = z.object({ | |
| body: z.string().describe('The textual content of the message.'), | |
| msgtype: z.literal('m.text').or(z.literal('m.notice')).or(z.literal('m.emote')), | |
| format: z.string().optional().describe('The format of the message.'), | |
| formatted_body: z.string().optional().describe('The formatted body of the message.'), | |
| }); | |
| const NewFileMessageContentEditSchema = z.object({ | |
| body: z.string().describe('The textual description of the file.'), | |
| msgtype: z.enum(['m.image', 'm.file', 'm.audio', 'm.video']), | |
| url: z.string().describe('The URL of the file.'), | |
| info: FileInfoSchema.describe('Information about the file.').optional(), | |
| }); | |
| const NewLocationMessageContentEditSchema = z.object({ | |
| body: z.string().describe('The textual description of the location.'), | |
| msgtype: z.literal('m.location'), | |
| geo_uri: z.string().describe('The geo URI of the location.'), | |
| }); | |
| // New content schema for edits | |
| const NewContentSchema = z.discriminatedUnion('msgtype', [ | |
| NewTextMessageContentEditSchema, | |
| NewFileMessageContentEditSchema, | |
| NewLocationMessageContentEditSchema, |
This pull request refactors how Matrix room message events are typed and validated, introducing more granular and explicit types for different message kinds (text, file, location) across the codebase. It also updates the message event schema to use discriminated unions for better type safety and extensibility. The changes improve clarity, maintainability, and validation for message content and edits.
Type system improvements and refactoring:
MessageTypeunion intoTextMessageType,FileMessageType, andLocationMessageType, and introduced corresponding content types (TextMessageContent,FileMessageContent,LocationMessageContent). TheRoomMessageEventand related functions now use these more specific types, with support for a discriminated union for edits (NewContent). [1] [2] [3]Schema and validation enhancements:
PduMessageEventContentSchemato use a discriminated union for message content, with dedicated schemas for text, file, and location messages. Added a detailed schema for file info and for new content in edits, improving validation and extensibility. [1] [2] [3]Federation SDK alignment:
message.service.ts) to use the newFileMessageContenttype for file messages, ensuring consistency with the core event types and schemas. [1] [2]Summary by CodeRabbit
New Features
Refactor