feat: matrix adapter parity with API-first history and cursor v1#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds cursor-based server paging and new fetch/open DM APIs, DM/account-data persistence and invite-auto-join config, attachment and E2EE handling, a persistent Matrix-backed chat state store, extensive unit and E2E test suites/helpers, ESLint/tooling and CI/pnpm updates, and a package/version bump to 0.2.0. Changes
Sequence DiagramssequenceDiagram
participant Client
participant MatrixAdapter
participant Storage as AccountData
participant MatrixSDK as MatrixSDK
Client->>MatrixAdapter: openDM(userId)
activate MatrixAdapter
MatrixAdapter->>MatrixAdapter: check cached DM mapping
alt cached
MatrixAdapter-->>Client: return roomId
else
MatrixAdapter->>Storage: load direct account data
activate Storage
Storage-->>MatrixAdapter: mappings
deactivate Storage
MatrixAdapter->>MatrixSDK: find/create DM room
activate MatrixSDK
MatrixSDK-->>MatrixAdapter: roomId
deactivate MatrixSDK
MatrixAdapter->>Storage: persist mapping
activate Storage
Storage-->>MatrixAdapter: persisted
deactivate Storage
MatrixAdapter-->>Client: return roomId
end
deactivate MatrixAdapter
sequenceDiagram
participant Client
participant MatrixAdapter
participant CursorMgr as CursorManager
participant MatrixSDK as MatrixSDK
participant EventMapper as EventMapper
Client->>MatrixAdapter: fetchMessages(targetId, options)
activate MatrixAdapter
alt cursor provided
MatrixAdapter->>CursorMgr: decode cursor
activate CursorMgr
CursorMgr-->>MatrixAdapter: token, dir, kind, context
deactivate CursorMgr
end
alt thread (relations)
MatrixAdapter->>MatrixSDK: /relations (thread)
activate MatrixSDK
MatrixSDK-->>MatrixAdapter: raw relation events
deactivate MatrixSDK
opt include root
MatrixAdapter->>MatrixSDK: fetch root event
activate MatrixSDK
MatrixSDK-->>MatrixAdapter: root event
deactivate MatrixSDK
end
else room timeline
MatrixAdapter->>MatrixSDK: /messages (room)
activate MatrixSDK
MatrixSDK-->>MatrixAdapter: raw room events
deactivate MatrixSDK
end
MatrixAdapter->>EventMapper: map & decrypt events
activate EventMapper
EventMapper-->>MatrixAdapter: mapped events
deactivate EventMapper
MatrixAdapter->>CursorMgr: encode next cursor
activate CursorMgr
CursorMgr-->>MatrixAdapter: mxv1 cursor
deactivate CursorMgr
MatrixAdapter-->>Client: { messages, nextCursor }
deactivate MatrixAdapter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/index.ts (4)
604-655: Consider making thread list direction configurable.The direction is hardcoded to
"backward"(Line 609). While this is likely the most common use case (newest threads first), theListThreadsOptionsinterface may support adirectionoption that could be respected here.♻️ Optional: Support configurable direction
async listThreads( channelId: string, options: ListThreadsOptions = {} ): Promise<ListThreadsResult<MatrixEvent>> { const roomID = this.decodeChannelID(channelId); - const direction: CursorDirection = "backward"; + const direction: CursorDirection = options.direction ?? "backward"; const limit = options.limit ?? 50;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 604 - 655, The listThreads function hardcodes direction to "backward"; change it to read a direction from options (e.g., const direction: CursorDirection = options.direction ?? "backward"), map that to the Matrix Direction enum when calling createThreadListMessagesRequest (use Direction.Backward or Direction.Forward based on the direction variable), and ensure you pass the same direction value into encodeCursorV1 so the returned cursor reflects the requested direction; update any uses of the local direction variable in listThreads (including the cursor encoding) and keep the existing decodeCursorV1 call for incoming cursors.
778-835: Edge case consideration for limit=1 with root inclusion.When
args.includeRootis true andargs.limitis 1,relationLimitbecomesMath.max(1 - 1, 1) = 1. This fetches 1 relation, then combines with root, resulting in 2 events before slicing to 1. The final slice (Line 832) correctly limits the result, but the user would only see the root and miss any replies.This behavior seems intentional (root takes priority on first page), but worth documenting or validating that callers expect this.
Consider adding a brief comment explaining this behavior:
const relationLimit = args.includeRoot ? Math.max(args.limit - 1, 1) : args.limit; + // When includeRoot is true and limit is small, root takes priority over replies🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 778 - 835, In fetchThreadMessagesPage, add a short inline comment near the relationLimit calculation and the final return explaining the edge-case: when args.includeRoot is true and args.limit === 1 we intentionally request one relation and then prepend the root (rootEvent) which results in the root being returned and any replies being omitted due to the final slice; mention that this prioritizes the root on the first page and callers should expect/validate this behavior (refer to symbols relationLimit, args.includeRoot, args.limit, rootEvent, dedupedReplies and the final slice).
961-972: Consider validating DM room membership.
findExistingDirectRoomIDreturns the first room ID fromm.directwithout verifying the user has actually joined that room. If a DM room was previously left or rejected, this could return a stale room ID.For robustness, consider checking room membership before returning:
private findExistingDirectRoomID( direct: DirectAccountData, userID: string ): string | null { const candidates = direct[userID] ?? []; for (const roomID of candidates) { if (roomID) { + const room = this.client?.getRoom(roomID); + if (room?.getMyMembership() === "join") { return roomID; + } } } return null; }This prevents using rooms the bot has left. However, this also adds sync-state dependency, so evaluate if the trade-off is worthwhile for your use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 961 - 972, findExistingDirectRoomID currently returns the first room ID from direct[userID] without verifying membership; update it to iterate candidates and validate that the room is still valid by checking membership before returning (e.g., use the matrix client/room API to confirm the target user's membership is 'join' and that the bot itself is joined to the room). In practice replace the simple for-loop in findExistingDirectRoomID with a check per roomID that queries the room (via your Matrix client helpers or room.getMember(...).membership) and only return roomID when membership checks pass; if none pass, return null. Keep in mind this adds sync-state dependency so gate or document accordingly.
854-870: Consider logging errors in fetchRoomEventMapped.The catch block silently returns
nullwithout logging. While graceful degradation is appropriate, logging the error (at debug level) would aid troubleshooting when messages unexpectedly returnnull.♻️ Optional: Add debug logging
} catch (error) { + this.logger.debug("Failed to fetch room event", { + roomID, + eventID, + error: String(error), + }); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 854 - 870, fetchRoomEventMapped currently swallows exceptions in its catch and returns null; update the catch in fetchRoomEventMapped to log the caught error at debug (or appropriate) level before returning null so failures to fetch/decrypt are recorded. Specifically, wrap the existing try/catch's catch clause to capture the error (e) and call the logger (same logger used elsewhere in the file) with a descriptive message including roomID and eventID and the error, e.g. "Failed to fetch/decrypt event", then keep returning null; this touches fetchRoomEventMapped and references requireClient().fetchRoomEvent, mapRawEvent, and tryDecryptEvent for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.ts`:
- Around line 604-655: The listThreads function hardcodes direction to
"backward"; change it to read a direction from options (e.g., const direction:
CursorDirection = options.direction ?? "backward"), map that to the Matrix
Direction enum when calling createThreadListMessagesRequest (use
Direction.Backward or Direction.Forward based on the direction variable), and
ensure you pass the same direction value into encodeCursorV1 so the returned
cursor reflects the requested direction; update any uses of the local direction
variable in listThreads (including the cursor encoding) and keep the existing
decodeCursorV1 call for incoming cursors.
- Around line 778-835: In fetchThreadMessagesPage, add a short inline comment
near the relationLimit calculation and the final return explaining the
edge-case: when args.includeRoot is true and args.limit === 1 we intentionally
request one relation and then prepend the root (rootEvent) which results in the
root being returned and any replies being omitted due to the final slice;
mention that this prioritizes the root on the first page and callers should
expect/validate this behavior (refer to symbols relationLimit, args.includeRoot,
args.limit, rootEvent, dedupedReplies and the final slice).
- Around line 961-972: findExistingDirectRoomID currently returns the first room
ID from direct[userID] without verifying membership; update it to iterate
candidates and validate that the room is still valid by checking membership
before returning (e.g., use the matrix client/room API to confirm the target
user's membership is 'join' and that the bot itself is joined to the room). In
practice replace the simple for-loop in findExistingDirectRoomID with a check
per roomID that queries the room (via your Matrix client helpers or
room.getMember(...).membership) and only return roomID when membership checks
pass; if none pass, return null. Keep in mind this adds sync-state dependency so
gate or document accordingly.
- Around line 854-870: fetchRoomEventMapped currently swallows exceptions in its
catch and returns null; update the catch in fetchRoomEventMapped to log the
caught error at debug (or appropriate) level before returning null so failures
to fetch/decrypt are recorded. Specifically, wrap the existing try/catch's catch
clause to capture the error (e) and call the logger (same logger used elsewhere
in the file) with a descriptive message including roomID and eventID and the
error, e.g. "Failed to fetch/decrypt event", then keep returning null; this
touches fetchRoomEventMapped and references requireClient().fetchRoomEvent,
mapRawEvent, and tryDecryptEvent for context.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
CHANGELOG.mdREADME.mdpackage.jsonsrc/index.test.tssrc/index.ts
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~123-~123: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tchChannelMessages(channelId, options)` is supported for top-level room timeline m...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~124-~124: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s. - fetchMessages(threadId, options) is API-first and server-paginated (not `ro...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t). - listThreads(channelId, options) is server-backed via Matrix thread list AP...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
CHANGELOG.md
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r context-aware single-message fetch. - Added `fetchChannelMessages(channelId, option...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (22)
package.json (1)
3-3: LGTM!The version bump from
0.1.0to1.0.0appropriately signals the breaking changes documented in the changelog (opaque cursor format, legacy cursor rejection, new API surface).CHANGELOG.md (1)
3-16: LGTM!The changelog clearly documents all breaking changes (cursor format, legacy rejection), new additions (openDM, fetchMessage, fetchChannelMessages), reworked APIs (fetchMessages, listThreads), and intentionally unsupported features. This provides clear upgrade guidance for consumers.
README.md (1)
119-145: LGTM!The documentation additions provide clear guidance on feature parity, breaking changes, cursor migration strategy, and pagination behavior. The cursor migration section appropriately warns users to invalidate stored cursors when upgrading.
src/index.test.ts (9)
28-77: LGTM!The
makeRawEventandmapRawToEventtest helpers appropriately construct mock Matrix event structures and handle the thread relation parsing logic that mirrors the actual implementation.
178-189: LGTM!The
decodeCursorTokentest utility correctly validates themxv1:prefix and decodes the base64url JSON payload, enabling tests to verify cursor structure.
762-774: LGTM!The test correctly verifies that legacy cursors are rejected with the documented error message, ensuring the breaking change is enforced.
776-844: LGTM!The test comprehensively validates:
- Thread replies are filtered out from room-level message fetches
- Cursor encoding includes correct metadata (kind, token, roomID, direction)
- Subsequent pages use the decoded token correctly
846-913: LGTM!The test correctly validates that thread messages include the root event on the first page, all messages have the correct
threadId, and the cursor usesthread_relationskind with the root event ID.
915-955: LGTM!The test validates that
fetchChannelMessagescorrectly filters out thread replies and returns only top-level messages with proper cursor encoding.
957-992: LGTM!The test validates that
fetchMessagecorrectly returns the message when it matches the thread context and returnsnullfor context mismatches (message belongs to a different thread).
994-1049: LGTM!Excellent coverage of the
openDMresolution hierarchy:
- Cached state adapter mapping (fastest path)
- Matrix
m.directaccount data lookup with persistence- Room creation with full persistence to both state and account data
1051-1106: LGTM!The test validates server-backed thread listing with correct extraction of reply counts and last reply timestamps from aggregated relations, plus proper cursor encoding.
src/index.ts (10)
55-129: LGTM!The cursor types are well-structured with:
- Versioned prefix (
mxv1:) enabling future cursor format evolution- Clear kind discriminators for context validation
- Proper typing for Matrix
m.directaccount data format
430-456: LGTM!The
openDMimplementation correctly:
- Checks cached state first (fast path)
- Falls back to Matrix
m.directaccount data with persistence- Creates new DM room with full persistence when needed
- Validates the
room_idresponse from createRoom
458-520: LGTM!The
fetchMessagesimplementation correctly:
- Validates cursor context (kind, roomID, rootEventID)
- Routes to appropriate fetch method based on thread context
- Includes root event only on first page (
includeRoot = !cursor)- Applies thread override for consistent
threadIdvalues
554-572: LGTM!The
fetchMessageimplementation correctly validates that the fetched message belongs to the specified thread context, returningnullfor mismatches. This prevents cross-context message access.
687-743: LGTM!The cursor encoding/decoding implementation provides robust validation:
- Prefix check prevents legacy cursor usage
- Kind validation ensures cursors can't be reused across different API calls
- Context validation (roomID, rootEventID) prevents cross-room/thread cursor reuse
- Clear error messages aid debugging
745-776: LGTM!The room messages pagination correctly:
- Maps direction to Matrix SDK constants
- Filters events by type and room context
- Conditionally excludes thread replies based on
includeThreadReplies- Returns chronologically sorted results with pagination token
837-852: LGTM!The raw event mapping correctly uses the SDK's event mapper with room ID fallback, and applies decryption to all mapped events.
872-892: LGTM!The chronological sorting implementation correctly:
- Deduplicates events by ID
- Sorts by timestamp with event ID as tiebreaker for determinism
- Handles edge case of events without IDs
894-912: LGTM!The context validation helpers correctly distinguish:
- Top-level messages (no thread association)
- Thread-scoped messages (matches
threadRootIdOR is the root event itself)
657-685: LGTM!The internal message parser correctly handles the thread ID override pattern needed for pagination results, ensuring all messages in a thread page have consistent
threadIdvalues.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CHANGELOG.md (2)
10-14: Optional: Vary sentence structure for readability.Three consecutive bullets begin with "Added", which is flagged by the style checker. While this is standard changelog format, varying the phrasing can improve readability.
✍️ Example alternatives
### New - Added `openDM(userId)` with persisted mapping and `m.direct` reuse/create behavior. -- Added `fetchMessage(threadId, messageId)` for context-aware single-message fetch. -- Added `fetchChannelMessages(channelId, options)` for top-level channel timeline history. +- `fetchMessage(threadId, messageId)` for context-aware single-message fetch. +- `fetchChannelMessages(channelId, options)` for top-level channel timeline history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 10 - 14, The three changelog bullets all start with "Added"—update them to vary sentence structure while preserving meaning: keep the first as "Added `openDM(userId)` with persisted mapping and `m.direct` reuse/create behavior," change the second to something like "Introduced `fetchMessage(threadId, messageId)` for context-aware single-message retrieval," and rephrase the third to "Introduced `fetchChannelMessages(channelId, options)` to provide top-level channel timeline history." Ensure the unique symbols `openDM`, `fetchMessage`, and `fetchChannelMessages` remain exact and the behavioral notes (persisted mapping, `m.direct` behavior, context-aware single-message fetch, top-level timeline history) are preserved.
5-8: Consider adding specific migration instructions.The breaking change is clearly documented, but the guidance "must be cleared on upgrade" could be more actionable. Consider adding specific instructions or code examples showing how users should handle the cursor migration (e.g., clearing state, handling the error, etc.).
📝 Example enhancement
### Breaking Changes - Pagination cursors now use opaque `mxv1:<base64url-json>` values across `fetchMessages`, `fetchChannelMessages`, and `listThreads`. -- Legacy cursors are now rejected with `Invalid cursor format. Expected mxv1 cursor.`. Stored cursors from older versions must be cleared on upgrade. +- Legacy cursors are now rejected with `Invalid cursor format. Expected mxv1 cursor.`. + - **Migration**: Clear any persisted cursor state before upgrading, or handle the error by discarding old cursors and re-fetching from the beginning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 5 - 8, Update the breaking-changes entry to include concrete migration steps for users of fetchMessages, fetchChannelMessages, and listThreads: describe how to detect legacy cursors (those not matching the mxv1:<base64url-json> pattern), instruct to clear or invalidate stored cursors in common storage locations (localStorage/sessionStorage, IndexedDB, server DB) and provide an example flow for graceful recovery (catch the Invalid cursor format error, drop the cursor, retry the request without a cursor to fetch the first page, then persist the new mxv1 cursor), and suggest server-side migration guidance (run a one-time job to nullify legacy cursor fields or migrate them to empty). Ensure the text names the affected APIs (fetchMessages, fetchChannelMessages, listThreads) and the expected cursor format mxv1:<base64url-json> so users can implement checks and handle upgrade errors consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 10-14: The three changelog bullets all start with "Added"—update
them to vary sentence structure while preserving meaning: keep the first as
"Added `openDM(userId)` with persisted mapping and `m.direct` reuse/create
behavior," change the second to something like "Introduced
`fetchMessage(threadId, messageId)` for context-aware single-message retrieval,"
and rephrase the third to "Introduced `fetchChannelMessages(channelId, options)`
to provide top-level channel timeline history." Ensure the unique symbols
`openDM`, `fetchMessage`, and `fetchChannelMessages` remain exact and the
behavioral notes (persisted mapping, `m.direct` behavior, context-aware
single-message fetch, top-level timeline history) are preserved.
- Around line 5-8: Update the breaking-changes entry to include concrete
migration steps for users of fetchMessages, fetchChannelMessages, and
listThreads: describe how to detect legacy cursors (those not matching the
mxv1:<base64url-json> pattern), instruct to clear or invalidate stored cursors
in common storage locations (localStorage/sessionStorage, IndexedDB, server DB)
and provide an example flow for graceful recovery (catch the Invalid cursor
format error, drop the cursor, retry the request without a cursor to fetch the
first page, then persist the new mxv1 cursor), and suggest server-side migration
guidance (run a one-time job to nullify legacy cursor fields or migrate them to
empty). Ensure the text names the affected APIs (fetchMessages,
fetchChannelMessages, listThreads) and the expected cursor format
mxv1:<base64url-json> so users can implement checks and handle upgrade errors
consistently.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.mdREADME.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r context-aware single-message fetch. - Added `fetchChannelMessages(channelId, option...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (2)
README.md (1)
119-128: LGTM! Clear and comprehensive capabilities documentation.The new Capabilities section effectively communicates the adapter's feature set, clearly distinguishes supported from unsupported features, and appropriately cross-references the CHANGELOG for migration details.
CHANGELOG.md (1)
3-27: Well-structured 1.0.0 changelog.The changelog effectively organizes breaking changes, new features, changes, and fixes with clear technical details. The structure follows best practices and provides good visibility into the release scope.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
77-77: Consider clarifying where advanced options are documented.The line mentions "Advanced options are available" but doesn't specify where users can find documentation for these options. Consider adding a reference to the relevant documentation or configuration interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 77, Clarify where the "Advanced options" are documented by updating the sentence that mentions device/session persistence, E2EE storage, and MATRIX_SDK_LOG_LEVEL to include a pointer to the specific documentation or configuration page (for example, add "see the Advanced configuration section" or a link to the relevant docs page), and ensure you reference the exact options (device/session persistence, E2EE storage, and the MATRIX_SDK_LOG_LEVEL env var) so readers can find full configuration details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Line 77: Clarify where the "Advanced options" are documented by updating the
sentence that mentions device/session persistence, E2EE storage, and
MATRIX_SDK_LOG_LEVEL to include a pointer to the specific documentation or
configuration page (for example, add "see the Advanced configuration section" or
a link to the relevant docs page), and ensure you reference the exact options
(device/session persistence, E2EE storage, and the MATRIX_SDK_LOG_LEVEL env var)
so readers can find full configuration details.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md
📜 Review details
🔇 Additional comments (5)
README.md (5)
15-41: LGTM!The Quick Start section provides a clear, complete example with proper imports and configuration structure.
43-61: LGTM!The Authentication section clearly documents both supported authentication methods with practical code examples.
106-112: LGTM!The Capabilities section clearly documents supported and unsupported features, which helps set proper expectations for users of this adapter.
81-81: The referenced fileexamples/.env.exampleexists in the repository with appropriate environment variable examples.
3-3: The Chat SDK documentation URL is valid and points to the correct documentation. No action required.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/index.ts (2)
779-836: Consider potential edge case with limit calculation.When
includeRootis true, the code fetcheslimit - 1relations to leave room for the root. However, if the root event fails to fetch or doesn't pass context validation (lines 823-827), the returned array could have fewer thanlimititems even when more relations exist.This is likely acceptable behavior, but worth noting that clients may see slightly fewer items than requested on first pages where root fetching fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 779 - 836, When includeRoot is true, change the relation fetch logic in fetchThreadMessagesPage so we overfetch relations instead of pre-reserving a slot; specifically, stop computing relationLimit as Math.max(args.limit - 1, 1) and fetch args.limit (or args.limit + 1 if you want to be extra safe), then after decrypt/sort/filter, dedupe the root (using event.getId() !== args.rootEventID) and build the final events array as [...rootArray, ...dedupedReplies].slice(0, args.limit) so we still return up to args.limit items even if the root fails validation; adjust any nextToken handling accordingly (use relationResponse.nextBatch ?? null) and keep the rest of the logic in fetchThreadMessagesPage the same.
962-973: Consider room membership validation for existing direct rooms.
findExistingDirectRoomIDreturns the first room ID from them.directaccount data, but this room may be in a state the bot can't use (e.g.,left,banned, orinvitemembership). The caller would only discover this when attempting to send a message.Consider validating room membership before returning, or document this as expected behavior where the caller handles failures.
💡 Optional enhancement to validate room membership
private findExistingDirectRoomID( direct: DirectAccountData, userID: string ): string | null { const candidates = direct[userID] ?? []; for (const roomID of candidates) { - if (roomID) { + if (roomID && this.isJoinedRoom(roomID)) { return roomID; } } return null; } + +private isJoinedRoom(roomID: string): boolean { + try { + const room = this.requireClient().getRoom(roomID); + return room?.getMyMembership() === "join"; + } catch { + return false; + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 962 - 973, findExistingDirectRoomID currently returns the first roomID from the m.direct account data without confirming the bot's membership; change it to validate that the room exists and the bot's membership is usable (e.g., "join") before returning. In practice, inside findExistingDirectRoomID use the roomID candidates from direct[userID], query the current client/store for that room (or room member state) and skip any room where the bot's membership is "leave", "ban"/"banned", "invite" (or another non-joined state); return the first room with a valid "join" membership or null if none remain. Ensure you reference the same DirectAccountData and userID parameters and update callers/tests if you prefer to surface failures differently (or alternatively add a short doc comment to the function if you choose not to validate here).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.ts`:
- Around line 779-836: When includeRoot is true, change the relation fetch logic
in fetchThreadMessagesPage so we overfetch relations instead of pre-reserving a
slot; specifically, stop computing relationLimit as Math.max(args.limit - 1, 1)
and fetch args.limit (or args.limit + 1 if you want to be extra safe), then
after decrypt/sort/filter, dedupe the root (using event.getId() !==
args.rootEventID) and build the final events array as [...rootArray,
...dedupedReplies].slice(0, args.limit) so we still return up to args.limit
items even if the root fails validation; adjust any nextToken handling
accordingly (use relationResponse.nextBatch ?? null) and keep the rest of the
logic in fetchThreadMessagesPage the same.
- Around line 962-973: findExistingDirectRoomID currently returns the first
roomID from the m.direct account data without confirming the bot's membership;
change it to validate that the room exists and the bot's membership is usable
(e.g., "join") before returning. In practice, inside findExistingDirectRoomID
use the roomID candidates from direct[userID], query the current client/store
for that room (or room member state) and skip any room where the bot's
membership is "leave", "ban"/"banned", "invite" (or another non-joined state);
return the first room with a valid "join" membership or null if none remain.
Ensure you reference the same DirectAccountData and userID parameters and update
callers/tests if you prefer to surface failures differently (or alternatively
add a short doc comment to the function if you choose not to validate here).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/index.test.tssrc/index.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.test.ts (1)
src/index.ts (1)
MatrixAdapter(132-1891)
🔇 Additional comments (28)
src/index.test.ts (13)
16-17: LGTM!Good additions to
makeEvent- addingisRelationandgetServerAggregatedRelationstubs aligns the mock with the Matrix SDK'sMatrixEventinterface, enabling tests for thread-related functionality.
28-39: LGTM!The
makeRawEventhelper correctly creates raw event objects matching the Matrix/messagesAPI response structure, with sensible defaults for all required fields.
41-77: LGTM!The
mapRawToEventhelper accurately simulates the Matrix SDK's event mapper behavior, including:
- Proper extraction of
m.relates_tofor thread detection- Correct derivation of
threadRootIdfrom thread relations- Delegation to
makeEventfor consistent mock structure
79-100: LGTM!Type definitions for test mocks are well-structured and match the expected response shapes from Matrix SDK methods.
115-144: LGTM!The extended
makeClientmock comprehensively covers the new Matrix API interactions:
createRoomfor DM creationcreateMessagesRequestfor room-level paginationcreateThreadListMessagesRequestfor thread listingfetchRoomEventfor single event retrievalgetAccountDataFromServerfor direct account datarelationsfor thread relationssetAccountDatafor persisting direct mappings
178-189: LGTM!The
decodeCursorTokentest helper correctly mirrors the implementation's cursor format, using base64url decoding after stripping themxv1:prefix. This enables tests to verify cursor payload structure.
801-813: LGTM!Good test coverage for legacy cursor rejection. This validates the breaking change that explicitly rejects non-mxv1 cursors with a clear error message.
815-883: LGTM!Comprehensive test for room-level message fetching:
- Verifies thread replies are filtered out (only
$top1,$top2returned)- Validates chronological ordering
- Confirms mxv1 cursor structure with correct kind, token, and direction
- Tests continuation with cursor-based pagination
885-952: LGTM!Thorough test for thread message fetching:
- Validates root event inclusion on first page
- Confirms chronological ordering of replies
- Verifies all messages have the correct thread-scoped threadId
- Validates cursor payload includes
rootEventIDfor thread context
954-994: LGTM!Good test for
fetchChannelMessagesthat confirms:
- Thread replies are filtered out at channel level
- Cursor is properly encoded with
room_messageskind- Only top-level messages are returned
996-1031: LGTM!Good test coverage for
fetchMessage:
- Successfully retrieves a message by ID within thread context
- Correctly returns
nullwhen message belongs to a different thread (mismatch case)
1033-1088: LGTM!Comprehensive test for
openDMcovering all three code paths:
- Cache hit - returns existing DM from state adapter
- Direct account data hit - finds existing room and persists to cache
- Room creation - creates new DM, persists to both account data and cache
Good verification of side effects (createRoom calls, setAccountData, state.set).
1090-1145: LGTM!Good test for
listThreads:
- Validates server-side thread API integration
- Verifies thread ID encoding
- Confirms reply count extraction from bundled relations
- Validates cursor structure with
thread_listkindsrc/index.ts (15)
55-57: LGTM!Well-defined constants and types for the cursor system:
- Clear prefix for cursor versioning (
mxv1:)- Discriminated cursor kinds for different pagination contexts
- Explicit direction type prevents invalid values
Also applies to: 117-129
321-323: LGTM!Good refactor to delegate
parseMessagetoparseMessageInternal, enabling consistent message parsing with optional thread ID override for API-fetched messages.
357-369: LGTM!The
com.beeper.dont_render_editedmarker is correctly added to edit payloads, matching the commit message intent. This allows clients to skip re-rendering edited messages when appropriate.
431-457: LGTM!The
openDMmethod follows a sensible lookup hierarchy:
- Local cache (fast path)
- Server-side
m.directaccount data- Room creation as fallback
Good persistence to both local cache and account data for durability.
459-521: LGTM!The
fetchMessagesmethod properly handles both room-level and thread-level pagination:
- Validates cursor context (kind, roomID, rootEventID) via
decodeCursorV1- Uses appropriate page fetcher based on presence of
rootEventID- Returns correctly encoded continuation cursors
523-553: LGTM!
fetchChannelMessagescorrectly delegates tofetchRoomMessagesPagewithincludeThreadReplies: false, ensuring only top-level channel messages are returned.
555-573: LGTM!
fetchMessagecorrectly:
- Fetches and maps a single raw event
- Validates the event belongs to the expected thread context
- Returns
nullfor mismatches, enabling safe lookups
605-656: LGTM!The
listThreadsimplementation correctly:
- Uses server-side thread API (
createThreadListMessagesRequest)- Extracts thread summaries from bundled relations
- Derives
replyCountandlastReplyAtfrom aggregated data- Returns properly encoded continuation cursor
688-744: LGTM!The cursor encoding/decoding is well-implemented:
encodeCursorV1: Simple JSON + base64url encoding with version prefixdecodeCursorV1: Comprehensive validation of structure, kind, room, direction, token, and optional thread scope- Clear error messages for each validation failure
The strict validation prevents cursor misuse across different pagination contexts.
746-777: LGTM!
fetchRoomMessagesPagecorrectly:
- Translates cursor direction to Matrix SDK's
Directionenum- Maps raw events and decrypts as needed
- Filters to message events in the correct room
- Filters out thread replies when
includeThreadRepliesis false- Sorts chronologically
838-853: LGTM!
mapRawEventsandmapRawEventcorrectly:
- Use the SDK's event mapper for proper event hydration
- Ensure
room_idis present for mapping- Decrypt events after mapping
855-871: LGTM!
fetchRoomEventMappedcorrectly handles errors by catching and returningnull, enabling graceful fallback when events are unavailable or inaccessible.
873-893: LGTM!
sortEventsChronologicallyis well-implemented:
- Deduplicates by event ID to prevent duplicate messages
- Preserves events without IDs (edge case safety)
- Sorts primarily by timestamp, with event ID as tiebreaker for stable ordering
895-913: LGTM!The context-checking helpers are correctly implemented:
isTopLevelMessageEvent: Checks boththreadRootIdabsence andisRelationto identify non-threaded messagesisMessageEventInContext: Validates event type, room, and thread membership
915-986: LGTM!The DM storage and account data helpers are well-structured:
getDMStorageKey: Consistent key format with URL encodingloadPersistedDMRoomID/persistDMRoomID: Simple cache operationsloadDirectAccountData/normalizeDirectAccountData: Defensive parsing of potentially malformed account datafindExistingDirectRoomID: Returns first valid room ID from candidatespersistDirectAccountDataRoom: Appends room to existing mapping, avoiding duplicates
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
877-893: Consider logging errors infetchRoomEventMappedbefore returning null.The silent catch may hide legitimate issues (e.g., network errors vs. 404). A debug-level log would aid troubleshooting without affecting the "fetch or null" contract.
📝 Optional: Add debug logging
- } catch { + } catch (error) { + this.logger.debug("Failed to fetch room event", { roomID, eventID, error }); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 877 - 893, The function fetchRoomEventMapped swallows all errors; change the catch to capture the error (catch (err)) and emit a debug-level log with context (roomID and eventID) and the error before returning null; update the catch block for fetchRoomEventMapped so failures from requireClient().fetchRoomEvent, mapRawEvent or tryDecryptEvent are logged (e.g. this.logger?.debug or an equivalent logger) then return null to preserve the existing contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 2103-2106: The isRecord type guard currently returns true for
arrays (since typeof [] === "object"), causing functions like decodeCursorV1,
extractAttachments, and isValidStoredSession to treat arrays as plain objects;
update the isRecord implementation to explicitly exclude arrays (e.g., add a
check such as !Array.isArray(value)) so it only returns true for plain object
records, which will ensure those callers only accept non-array objects.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 877-893: The function fetchRoomEventMapped swallows all errors;
change the catch to capture the error (catch (err)) and emit a debug-level log
with context (roomID and eventID) and the error before returning null; update
the catch block for fetchRoomEventMapped so failures from
requireClient().fetchRoomEvent, mapRawEvent or tryDecryptEvent are logged (e.g.
this.logger?.debug or an equivalent logger) then return null to preserve the
existing contract.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.mdsrc/index.test.tssrc/index.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.test.ts (1)
src/index.ts (2)
MatrixAdapter(147-1924)createMatrixAdapter(1928-1983)
🔇 Additional comments (18)
src/index.ts (10)
28-48: LGTM: New SDK imports align with server-side pagination features.The added imports (
Direction,IEvent,IThreadBundledRelationship,ThreadFilterType,THREAD_RELATION_TYPE) properly support the new cursor-based pagination and thread listing capabilities.
132-144: LGTM: Well-structured cursor and DM types.The cursor types (
CursorKind,CursorDirection,CursorV1Payload) provide clear contracts for the three pagination contexts.DirectAccountDatacorrectly models the Matrixm.directaccount data structure.
449-475: LGTM: Robust DM open flow with proper caching hierarchy.The implementation correctly prioritizes cached mappings, falls back to server
m.directaccount data, and only creates a new room when necessary. The persistence to both local state and server account data ensures consistency.
477-539: LGTM: Clean cursor-based pagination with proper context validation.The routing logic correctly distinguishes room-level from thread-level fetches, and the
includeRoot = !cursorpattern ensures the thread root appears only on the first page.
713-766: LGTM: Thorough cursor validation with clear error messages.The validation covers all required fields and context mismatches. The strict rejection of legacy cursors (non-mxv1) ensures clean migration.
801-858: LGTM: Thread message fetching with proper root handling.The implementation correctly handles:
- Limit adjustment when including root
- Root event retrieval from response or separate fetch
- Deduplication of root from replies
- Final slice to respect requested limit
895-915: LGTM: Robust event sorting with deduplication.The implementation handles edge cases like duplicate events and missing IDs, with stable sorting via ID comparison for timestamp ties.
623-674: LGTM: Server-backed thread listing with proper aggregation handling.The implementation correctly uses
createThreadListMessagesRequestand extracts thread metadata from server-aggregated relations (IThreadBundledRelationship).
1444-1452: LGTM: Proper thread context resolution for reactions.The fallback to room-level thread ID when the target event isn't found locally is reasonable, as the reaction will still be processed in a valid context.
1918-1921: Defensive SDK logging configuration is reasonable.Using
Reflect.getto accesssetLevelhandles potential SDK version differences gracefully.README.md (2)
107-114: LGTM: Capabilities section accurately documents the new API surface.The documentation correctly reflects the implemented features (
openDM,fetchMessage,fetchChannelMessages,fetchMessages,listThreads) and explicitly notes unsupported features.
1-8: LGTM: Clear introduction with sync-based architecture noted.The README accurately describes that the adapter uses sync polling rather than webhooks, which is important for deployment understanding.
src/index.test.ts (6)
29-78: LGTM: Well-structured raw event mapping for tests.The
makeRawEventandmapRawToEventutilities correctly simulate the matrix-js-sdk event mapper behavior, including thread relation detection and aggregation access.
221-239: LGTM: Useful test helpers for cursor and value assertions.
decodeCursorTokenenables verifying cursor structure in tests, andrequireValueprovides clear failure messages for missing values.
389-445: LGTM: Thorough test for reaction thread context resolution.The test correctly verifies that reactions to thread messages are routed with the appropriate thread ID rather than the room-level ID.
856-940: LGTM: Comprehensive cursor validation and pagination tests.The tests verify:
- Legacy cursor rejection with clear error message
- Thread reply filtering from room-level fetches
- Cursor token structure and context
- Pagination continuation with correct parameters
1093-1148: LGTM: Comprehensive DM open flow test covering all scenarios.The test validates the full caching hierarchy:
- Cached mapping reuse (no API calls)
- Server
m.directfallback with local persistence- Room creation with both local and server persistence
1150-1207: LGTM: Thread listing test with aggregation verification.The test validates that
listThreadscorrectly extractsreplyCountandlastReplyAtfrom server-aggregated relations and produces valid mxv1 cursors.
- Delete unused isMessageEvent (isMessageEventInContext is the active one) - Replace unbounded processedTimelineEventIDs.clear() with sliding window - Extract rawEmoji helper to deduplicate addReaction/removeReaction - Pre-filter non-message events before decryption in fetchThreadMessagesPage - Cache getJoinedMembers() and getSessionStorageKey() in local variables - Consolidate three duplicate timeline event log statements into one - Simplify redundant post-decrypt type guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index.ts (1)
1008-1018:m.directwrites should re-read latest server state before overwrite.Line 1017 writes a full account-data object from the earlier snapshot. Under concurrent updates, this can drop mappings written by another client/process.
♻️ Suggested hardening
private async persistDirectAccountDataRoom( userID: string, roomID: string, existing: DirectAccountData ): Promise<void> { - const updated: DirectAccountData = { ...existing }; - const existingRooms = updated[userID] ?? []; + const latest = this.normalizeDirectAccountData( + await this.requireClient().getAccountDataFromServer(EventType.Direct) + ); + const updated: DirectAccountData = { ...latest, ...existing }; + const existingRooms = updated[userID] ?? []; if (!existingRooms.includes(roomID)) { updated[userID] = [...existingRooms, roomID]; await this.requireClient().setAccountData(EventType.Direct, updated); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1008 - 1018, persistDirectAccountDataRoom currently overwrites the earlier snapshot and can drop concurrent updates; fix it by re-reading the latest server state for EventType.Direct before writing: call the client to fetch current direct account data (e.g. await this.requireClient().getAccountData(EventType.Direct) or the equivalent read method on your client), merge the roomID into the userID entry from that fresh object (preserving any other users/rooms), then call this.requireClient().setAccountData(EventType.Direct, merged) as before; update function persistDirectAccountDataRoom to perform this read-merge-write and handle missing/null responses safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 484-491: The cursor decoding (decodeCursorV1) currently validates
cursor shape but doesn't enforce that the decoded direction matches the
requested pagination direction, allowing reuse of a backward cursor for a
forward fetch (and vice versa) which causes skips/duplicates; update the call
sites that build `cursor` from `options.cursor` (the block using options.cursor,
and similar sites at the other locations) to check the decoded cursor's dir
against the requested direction (e.g., the fetch `direction`/`dir` parameter or
whether this call is for "room_messages" vs "thread_relations") and throw or
null out the cursor if they mismatch; alternatively, add a direction parameter
to decodeCursorV1 and have decodeCursorV1 itself validate and return an
error/null when dir !== requestedDir so callers like the code that constructs
`cursor` must handle invalid-direction cursors consistently.
- Around line 928-930: The isTopLevelMessageEvent function currently only
excludes thread relations (THREAD_RELATION_TYPE.name) but still returns edit
events because it only checks a single relation type; update the filter to
exclude any relation events by using event.isRelation() with no args (or
explicitly also check for 'm.replace') so the function (isTopLevelMessageEvent)
returns false for events with any relation (including edits) as well as thread
roots; adjust the condition that uses event.threadRootId and
event.isRelation(THREAD_RELATION_TYPE.name) to instead use !event.threadRootId
&& !event.isRelation() (or add && !event.isRelation('m.replace')) to prevent
edit events from leaking into top-level history.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 1008-1018: persistDirectAccountDataRoom currently overwrites the
earlier snapshot and can drop concurrent updates; fix it by re-reading the
latest server state for EventType.Direct before writing: call the client to
fetch current direct account data (e.g. await
this.requireClient().getAccountData(EventType.Direct) or the equivalent read
method on your client), merge the roomID into the userID entry from that fresh
object (preserving any other users/rooms), then call
this.requireClient().setAccountData(EventType.Direct, merged) as before; update
function persistDirectAccountDataRoom to perform this read-merge-write and
handle missing/null responses safely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/index.ts
📜 Review details
🔇 Additional comments (1)
src/index.ts (1)
2082-2084: Good guard update inisRecord.Excluding arrays here is the right fix and makes downstream cursor/session/content validation safer.
Add .claude/settings.local.json to .gitignore to avoid committing local Claude configuration files (sensitive or environment-specific settings).
Introduce invite auto-join behavior and outbound file/media handling for Matrix. - Add MatrixInviteAutoJoinConfig and env vars (MATRIX_INVITE_AUTOJOIN_ENABLED, MATRIX_INVITE_AUTOJOIN_ALLOWLIST) and document usage in README. Auto-join only accepts m.room.member invites targeted at the bot and enforces inviter and room allowlists. - Implement invite auto-join logic in the adapter with tests covering allowlisted and non-allowlisted inviters. - Add support for uploading local files/attachments via the SDK (uploadContent) and posting media events; URL-only attachments are appended as links to the message body. Includes unit tests for uploading and URL-only attachments. - Refactor postMessage to build multiple room message contents (text + media) and send them sequentially. - Add various helpers and hygiene: parsing env lists, normalizing strings/lists, converting directions for the SDK, trimming reaction cache, avoiding storing accessToken in persisted session metadata, and fixes to event mapping and buffers/Blob normalization. - Update types, tests, and README accordingly. Also includes small bug fixes and internal refactors to message parsing, event mapping, and client interactions.
Multiple refactors and fixes: add SyncState import and use enum values for sync checks; introduce SDKLogLevel type and safer parse guard for SDK log levels. Rework message/attachment flow: require non-empty toRoomMessageContents, change collectUploads to accept attachments, extract attachments once, upload files in parallel (Promise.all), and simplify link-only line collection. Improve binary handling by creating Blobs from Node buffers via Uint8Array. Replace several ad-hoc cache-trimming blocks with a reusable eviction utility (evictOldestEntries) and add a private evictOldest helper; apply eviction to timeline and reaction caches. Fix session persistence to preserve original createdAt when reusing restored sessions. Also replace decodeChannelID usages with decodeThreadId().roomID and some small API/typing cleanups.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/index.ts (1)
948-950:⚠️ Potential issue | 🟠 MajorEdit events (
m.replace) may leak into top-level history results.The filter only excludes
m.threadrelations. Edit events withrel_type: m.replacewill still pass through and appear as standalone messages in room history.🔧 Proposed fix to exclude all relation events
private isTopLevelMessageEvent(event: MatrixEvent): boolean { - return !event.threadRootId && !event.isRelation(THREAD_RELATION_TYPE.name); + return !event.threadRootId && !event.getRelation(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 948 - 950, The current isTopLevelMessageEvent only excludes thread relations and thread roots, so edit events (rel_type: m.replace) still qualify; update isTopLevelMessageEvent to also exclude any relation events by returning false when the MatrixEvent has any relation (e.g., use event.isRelation() with no argument or check event.getRelation()/event.getAssociatedRelation() depending on SDK API) in addition to the existing !event.threadRootId check so that all relation types (including m.replace) are filtered out.
🧹 Nitpick comments (1)
src/index.ts (1)
1223-1229: Consider whether empty-body fallback is intentional.When both
textBodyis empty and there are no uploads, the code creates a message with an empty body. This could result in sending blank messages iftoRoomMessageContentsis called with a message that has no text and no uploadable attachments.💡 Alternative: throw or return empty array
if (contents.length === 0) { - contents.push({ - ...textContent, - body: "", - msgtype: textMsgType, - }); + throw new Error("Cannot post message with no content"); }Or if truly empty messages are sometimes valid, this can remain as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1223 - 1229, The current fallback that pushes an object with an empty body into contents (the contents.push({...textContent, body: "", msgtype: textMsgType}) line in toRoomMessageContents) will produce blank messages when textBody is empty and there are no uploads; change the logic to explicitly handle that case by checking whether textContent.body (or textBody) is empty and there are no uploadable attachments and then either return an empty array (preferred) or throw an error, rather than pushing an empty message; update any callers/tests that expect the old behavior accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/index.ts`:
- Around line 948-950: The current isTopLevelMessageEvent only excludes thread
relations and thread roots, so edit events (rel_type: m.replace) still qualify;
update isTopLevelMessageEvent to also exclude any relation events by returning
false when the MatrixEvent has any relation (e.g., use event.isRelation() with
no argument or check event.getRelation()/event.getAssociatedRelation() depending
on SDK API) in addition to the existing !event.threadRootId check so that all
relation types (including m.replace) are filtered out.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 1223-1229: The current fallback that pushes an object with an
empty body into contents (the contents.push({...textContent, body: "", msgtype:
textMsgType}) line in toRoomMessageContents) will produce blank messages when
textBody is empty and there are no uploads; change the logic to explicitly
handle that case by checking whether textContent.body (or textBody) is empty and
there are no uploadable attachments and then either return an empty array
(preferred) or throw an error, rather than pushing an empty message; update any
callers/tests that expect the old behavior accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.gitignoreREADME.mdsrc/index.test.tssrc/index.tssrc/types.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/types.ts (1)
MatrixAdapterConfig(30-49)
src/index.test.ts (1)
src/index.ts (2)
MatrixAdapter(171-2175)createMatrixAdapter(2179-2245)
🔇 Additional comments (18)
.gitignore (1)
11-11: LGTM!Ignoring local Claude settings is appropriate to keep developer-specific configurations out of version control.
src/types.ts (1)
41-54: LGTM!The
MatrixInviteAutoJoinConfiginterface is well-defined with appropriate optional properties. The integration intoMatrixAdapterConfigis clean and follows the existing configuration pattern.src/index.ts (8)
726-779: Cursor direction validation is incomplete.The decoded cursor's
diris validated for shape but not checked against the requested fetch direction. This allows a backward cursor to be reused with a forward fetch (or vice versa), which can cause skipped or duplicated messages.
485-511: LGTM!The
openDMimplementation follows a sensible cascading lookup: cached state → server account data → create new room. The persistence to both local state and Matrix account data ensures consistency.
2380-2393: LGTM!Good memory management pattern using insertion-order eviction. The default thresholds (max 10K, target 5K) provide reasonable bounds while avoiding frequent eviction churn.
2395-2397: LGTM!The
isRecordhelper now correctly excludes arrays, addressing the previous review concern.
818-880: LGTM!The thread message fetching correctly handles root inclusion, deduplication, and limit enforcement. The
Math.max(args.limit - 1, 1)ensures at least one relation is fetched even whenlimit=1withincludeRoot=true.
1357-1359: Auto-join runs beforeliveSyncReadycheck, potentially joining rooms during initial sync.
maybeAutoJoinInviteis called at line 1358 before theliveSyncReadyguard at line 1361. This means the bot could auto-join rooms from historical/backfilled invite events during initial sync, not just live invites.If this is intentional (to catch up on pending invites), the behavior is fine. If only live invites should trigger auto-join, move the call after the
liveSyncReadycheck.
2169-2172: LGTM!Using
Reflect.getto safely accesssetLevelis a defensive pattern that avoids runtime errors if the SDK's logger interface changes.
926-946: LGTM!The sorting logic correctly orders events chronologically with a deterministic event ID tiebreaker. Deduplication by event ID prevents duplicates in results.
src/index.test.ts (6)
30-79: LGTM!The
makeRawEventandmapRawToEventhelpers provide clean abstractions for creating test fixtures. The mapping logic correctly handles thread relations and aggregated relations, enabling thorough testing of the cursor-based pagination features.
113-144: LGTM!The
AdapterInternalstype and helper functions (asMatrixClient,getInternals) provide type-safe access to internal adapter state for testing, which is a common and acceptable testing pattern.
146-196: LGTM!The
makeClientmock is well-structured with appropriate default implementations for all new SDK methods. The mock structure enables flexible test scenarios through method-level overrides.
995-1069: LGTM!Excellent test coverage for cursor-based pagination. The tests verify legacy cursor rejection, proper mxv1 cursor structure, thread reply filtering, and correct token passing between pages.
1207-1262: LGTM!Comprehensive test coverage for the
openDMflow, validating all three lookup paths (cached state,m.directaccount data, create new room) and proper persistence to both state adapter and Matrix account data.
1264-1316: LGTM!The
listThreadstest validates server-side thread API usage, proper extraction of thread aggregations (reply count, latest event timestamp), and correct mxv1 cursor generation for pagination.README.md (2)
82-106: LGTM!The Invite Auto-Join documentation clearly explains the feature behavior, configuration options, and interaction with other allowlists. The code example and behavior notes align with the implementation.
134-152: LGTM!The Capabilities and Notes sections accurately document the new API surface, media handling, and intentionally unsupported features. The reference to CHANGELOG.md for migration guidance is helpful for users upgrading.
Add tests for fetchMessage to return null on M_NOT_FOUND and to propagate transient server errors. Import MatrixError and update fetchRoomEvent handling to return null for M_NOT_FOUND/404 while rethrowing other errors. Filter server /sync chunk to message events before mapping to avoid processing non-message events. Run session/device persistence calls in parallel via Promise.all to avoid unnecessary sequential awaits. Replace usage of messageTypeForFile with messageTypeForMimeType and remove the now-unused helper; minor cleanup of direction usage in thread list cursor encoding.
Add an end-to-end test suite and supporting files (e2e/.env.example, e2e/e2e.test.ts, e2e/helpers.ts, vitest.config.e2e.ts), update test and source code (src/index.ts, src/index.test.ts), and adjust test runner config (vitest.config.ts, package.json). Update CHANGELOG to 0.2.0 documenting new APIs (openDM, fetchMessage, fetchChannelMessages), server-backed pagination, SDK bump and fixes (clean shutdown listeners, guard decodeRecoveryKey). Also add a lockfile and dependency updates.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
e2e/helpers.ts (2)
96-98: Casting reaction event toanybypasses type safety.The
as anycast on line 97 loses type information. Consider defining a proper type for the reaction event or using a more specific cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 96 - 98, Replace the unsafe any cast by using a concrete reaction event type: define or import an appropriate interface/type (e.g., ReactionEvent) and update the callback usage so chat.onReaction's handler is strongly typed (for example: chat.onReaction(async (event: ReactionEvent) => { reactionCallback?.(event); })). Also update the type signature of reactionCallback (where it's declared) to accept ReactionEvent so no casting is necessary.
131-146: Potential cleanup issue ifsubscribethrows synchronously.If
subscribethrows before returning the cleanup function, the timer will still be pending butcleanupwill be undefined. This could leave orphaned timers in error scenarios.🛡️ Proposed fix for safer cleanup handling
export function waitForEvent<T>( subscribe: (callback: (value: T) => void) => (() => void) | void, timeoutMs = 10_000 ): Promise<T> { return new Promise<T>((resolve, reject) => { + let cleanup: (() => void) | void; const timer = setTimeout(() => { cleanup?.(); reject(new Error(`waitForEvent timed out after ${timeoutMs}ms`)); }, timeoutMs); - const cleanup = subscribe((value) => { + cleanup = subscribe((value) => { clearTimeout(timer); + cleanup?.(); resolve(value); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 131 - 146, In waitForEvent, if subscribe throws synchronously the timeout timer can be left running because cleanup is never assigned; to fix, declare cleanup (let cleanup: (() => void) | void) before creating the timer, then call subscribe inside a try/catch, assigning its return to cleanup; in the catch block clearTimeout(timer) and reject(err); keep the existing callback logic that clears the timer and resolves, and ensure the timeout handler still calls cleanup?. This targets the waitForEvent function and the subscribe/cleanup/timer variables.src/index.ts (2)
831-833: Off-by-one potential whenlimit - 1equals 0.When
args.limitis 1 andargs.includeRootis true,relationLimitbecomesMath.max(1 - 1, 1) = 1, which is correct. However, the intent seems to be reserving space for the root event, but if only 1 relation is fetched plus 1 root, the final slice at line 882 limits toargs.limit(1), potentially dropping the relation.The logic appears correct due to the
Math.max(..., 1)safeguard, but consider adding a comment clarifying the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 831 - 833, The relationLimit calculation using const relationLimit = args.includeRoot ? Math.max(args.limit - 1, 1) : args.limit is correct but ambiguous; add a short clarifying comment next to relationLimit (and reference the later final slicing behavior that trims results to args.limit) explaining that when includeRoot is true we reserve one slot for the root by fetching up to limit-1 relations (with Math.max to avoid zero), and that the final result is sliced to args.limit so the root plus relations will not exceed the requested limit.
1022-1033:findExistingDirectRoomIDreturns first candidate without validation.The function returns the first non-empty room ID from the
m.directaccount data without verifying that the room still exists or that the bot is a member. This could return a stale room ID if the room was deleted or the bot was kicked.This is likely acceptable for the happy path, as
createRoomwithis_direct: truewould be the fallback if communication fails, but consider documenting this assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1022 - 1033, findExistingDirectRoomID currently returns the first non-empty room ID from the DirectAccountData without validating the room; change it to verify the room still exists and the bot is a member before returning (e.g. query the room state/membership via the client or room store for each candidate and skip stale/non-member rooms), falling back to null only if no valid room is found; alternatively add a clear comment in findExistingDirectRoomID documenting the assumption that callers will handle stale IDs (e.g., by attempting createRoom when send fails) if you choose not to perform the validation here.e2e/e2e.test.ts (1)
251-255: Consider strengthening the edit verification assertion.The test verifies the edited message exists but doesn't assert the actual edited content. The comment acknowledges this depends on homeserver aggregation, but this weakens the test's value.
const editedMsg = fetched.messages.find((m) => m.id === messageId); expect(editedMsg).toBeDefined(); - // The original event text or the edited text should reflect the edit - // (depends on homeserver aggregation; at minimum the edit event was sent) + // Verify edit was applied (homeserver-dependent: may show edited or original content) + expect( + editedMsg?.text.includes(editedTag) || editedMsg?.metadata?.edited + ).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/e2e.test.ts` around lines 251 - 255, The test currently only checks that an edited message exists (using fetched.messages.find with messageId) but not that its text changed; update the assertion to verify the edit content when available: after locating editedMsg (from fetched.messages.find(...)), assert that either the message's current body equals the expected edited text (e.g., editedText) OR, if the homeserver returns edit metadata, that the edit metadata/related content contains the edited text; use the existing editedMsg, fetched.messages and messageId identifiers to locate the message and add a conditional assertion that accepts both the aggregated edited body or the explicit edit relation containing the edited text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/e2e.test.ts`:
- Around line 251-255: The test currently only checks that an edited message
exists (using fetched.messages.find with messageId) but not that its text
changed; update the assertion to verify the edit content when available: after
locating editedMsg (from fetched.messages.find(...)), assert that either the
message's current body equals the expected edited text (e.g., editedText) OR, if
the homeserver returns edit metadata, that the edit metadata/related content
contains the edited text; use the existing editedMsg, fetched.messages and
messageId identifiers to locate the message and add a conditional assertion that
accepts both the aggregated edited body or the explicit edit relation containing
the edited text.
In `@e2e/helpers.ts`:
- Around line 96-98: Replace the unsafe any cast by using a concrete reaction
event type: define or import an appropriate interface/type (e.g., ReactionEvent)
and update the callback usage so chat.onReaction's handler is strongly typed
(for example: chat.onReaction(async (event: ReactionEvent) => {
reactionCallback?.(event); })). Also update the type signature of
reactionCallback (where it's declared) to accept ReactionEvent so no casting is
necessary.
- Around line 131-146: In waitForEvent, if subscribe throws synchronously the
timeout timer can be left running because cleanup is never assigned; to fix,
declare cleanup (let cleanup: (() => void) | void) before creating the timer,
then call subscribe inside a try/catch, assigning its return to cleanup; in the
catch block clearTimeout(timer) and reject(err); keep the existing callback
logic that clears the timer and resolves, and ensure the timeout handler still
calls cleanup?. This targets the waitForEvent function and the
subscribe/cleanup/timer variables.
In `@src/index.ts`:
- Around line 831-833: The relationLimit calculation using const relationLimit =
args.includeRoot ? Math.max(args.limit - 1, 1) : args.limit is correct but
ambiguous; add a short clarifying comment next to relationLimit (and reference
the later final slicing behavior that trims results to args.limit) explaining
that when includeRoot is true we reserve one slot for the root by fetching up to
limit-1 relations (with Math.max to avoid zero), and that the final result is
sliced to args.limit so the root plus relations will not exceed the requested
limit.
- Around line 1022-1033: findExistingDirectRoomID currently returns the first
non-empty room ID from the DirectAccountData without validating the room; change
it to verify the room still exists and the bot is a member before returning
(e.g. query the room state/membership via the client or room store for each
candidate and skip stale/non-member rooms), falling back to null only if no
valid room is found; alternatively add a clear comment in
findExistingDirectRoomID documenting the assumption that callers will handle
stale IDs (e.g., by attempting createRoom when send fails) if you choose not to
perform the validation here.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CHANGELOG.mde2e/.env.examplee2e/e2e.test.tse2e/helpers.tspackage.jsonsrc/index.test.tssrc/index.tsvitest.config.e2e.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- package.json
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/helpers.ts (1)
src/index.ts (1)
MatrixAdapter(171-2189)
src/index.ts (1)
src/types.ts (1)
MatrixAdapterConfig(30-49)
🔇 Additional comments (10)
vitest.config.ts (1)
1-15: LGTM!The explicit
includepattern properly scopes unit tests tosrc/**/*.test.ts, cleanly separating them from the new E2E test suite.e2e/.env.example (1)
1-15: LGTM!The environment template clearly documents required and optional variables with appropriate placeholder values. No secrets are committed.
vitest.config.e2e.ts (1)
1-8: LGTM!The E2E-specific configuration correctly scopes tests to
e2e/**/*.test.tswith a 30-second timeout appropriate for network-dependent integration tests.e2e/e2e.test.ts (1)
280-281: Pagination assertion may be fragile with shared room state.If the room has pre-existing messages from previous test runs,
page1.messages.lengthcould exceed 3, potentially causing flaky assertions. The test correctly usestoBeGreaterThanOrEqual(1)andtoBeLessThanOrEqual(3), which handles this reasonably.src/index.ts (3)
726-779: Cursor direction mismatch still not enforced.As noted in a previous review,
decodeCursorV1validates thatdiris "forward" or "backward" but doesn't verify it matches the requested fetch direction. A backward cursor could be reused with a forward fetch, potentially causing pagination inconsistencies.🔧 Proposed fix to add direction validation
private decodeCursorV1( cursor: string, expectedKind: CursorKind, expectedRoomID: string, - expectedRootEventID?: string + expectedRootEventID?: string, + expectedDirection?: CursorDirection ): CursorV1Payload { // ... existing validation ... if (parsed.dir !== "forward" && parsed.dir !== "backward") { throw new Error("Invalid cursor format. Invalid direction."); } + if (expectedDirection && parsed.dir !== expectedDirection) { + throw new Error(`Invalid cursor direction. Expected ${expectedDirection}.`); + } // ... }Then update call sites to pass the expected direction.
955-957: Edit events may still leak into top-level history results.As noted in a previous review,
isTopLevelMessageEventonly excludesm.threadrelations. Edit events (m.replace) are relation events and could still appear as standalone messages in room/channel history.🔧 Proposed fix to exclude all relation events
private isTopLevelMessageEvent(event: MatrixEvent): boolean { - return !event.threadRootId && !event.isRelation(THREAD_RELATION_TYPE.name); + return !event.threadRootId && !event.getRelation(); }
2394-2407:evictOldestEntriesrelies on insertion-order iteration for eviction.The function assumes that iterating over
collection.keys()yields keys in insertion order (oldest first), which is true for JavaScriptMapandSet. This is a reasonable assumption but worth noting in a comment for future maintainers.src/index.test.ts (3)
139-145: Internal accessor pattern is pragmatic but fragile.The
getInternalsandasMatrixClientfunctions use type assertions to access private members. This is a common pattern for testing private methods but creates maintenance burden if internal APIs change.Consider documenting this pattern or adding a comment explaining that these accessors are intentionally used for testing internal behavior that isn't exposed via public API.
1073-1137: Thread message fetch test validates root inclusion and cursor structure well.The test properly verifies:
- Root event is included on the first page
- All messages have the correct thread ID
- Cursor payload contains expected fields
1233-1288: Comprehensive openDM test covers all three code paths.The test exercises:
- Cached DM room ID lookup
m.directaccount data lookup- Room creation with persistence
This provides good coverage of the layered caching strategy.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
383-392:⚠️ Potential issue | 🟠 MajorDon’t hard-fail on timeline lag after a successful send.
The send call can succeed while local sync has not yet materialized the event; Line 391 can then throw even though the message was posted. This also impacts the analogous return path in
editMessage(Line 431).🔧 Proposed change
- return { - id: response.event_id, - threadId, - raw: this.mustGetEventByID(roomID, response.event_id), - }; + const raw = + this.requireClient().getRoom(roomID)?.findEventById(response.event_id) ?? + (await this.fetchRoomEventMapped(roomID, response.event_id)); + if (!raw) { + throw new Error(`Sent Matrix event could not be resolved: ${response.event_id}`); + } + + return { + id: response.event_id, + threadId, + raw, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 383 - 392, The code currently assumes mustGetEventByID will always find the just-sent event, causing a hard failure if sync/timeline lag occurs; wrap the call that builds the return (the mustGetEventByID call used when constructing the raw field after sendRoomMessage) in a try/catch and on failure return a safe fallback (e.g., raw: null or a minimal event object derived from the send response) instead of throwing; apply the same pattern to the analogous path in editMessage so timeline lag doesn't convert a successful send/edit into an exception.
🧹 Nitpick comments (1)
e2e/e2e.test.ts (1)
21-27: Split core-vs-recovery credential gating to avoid over-skipping E2E coverage.Lines 21–27 make the entire suite depend on recovery keys, but only one scenario truly needs them. This can hide regressions in core flows when recovery env vars are absent.
🔧 Suggested direction
-const hasCredentials = Boolean( +const hasCoreCredentials = Boolean( process.env.E2E_BASE_URL && process.env.E2E_BOT_LOGIN_TOKEN && - process.env.E2E_BOT_RECOVERY_KEY && - process.env.E2E_SENDER_LOGIN_TOKEN && - process.env.E2E_SENDER_RECOVERY_KEY + process.env.E2E_SENDER_LOGIN_TOKEN ); +const hasRecoveryCredentials = Boolean( + process.env.E2E_BOT_RECOVERY_KEY && process.env.E2E_SENDER_RECOVERY_KEY +); -describe.skipIf(!hasCredentials)("E2E Matrix Adapter", () => { +describe.skipIf(!hasCoreCredentials)("E2E Matrix Adapter", () => { // ... - it("restores historical encrypted messages on a fresh device using recovery key", async () => { + it.skipIf(!hasRecoveryCredentials)( + "restores historical encrypted messages on a fresh device using recovery key", + async () => { // ... - }); + } + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/e2e.test.ts` around lines 21 - 27, The current boolean hasCredentials gates the whole e2e suite on recovery keys; split it into two flags (e.g., hasCoreCredentials and hasRecoveryCredentials) derived from process.env, use hasCoreCredentials for the general suite gating and only use hasRecoveryCredentials to guard the single recovery scenario/test (update any skip or describe conditional that references hasCredentials). Locate and update the symbol hasCredentials in e2e.test.ts and replace its usages: create const hasCoreCredentials = Boolean(process.env.E2E_BASE_URL && process.env.E2E_BOT_LOGIN_TOKEN && process.env.E2E_SENDER_LOGIN_TOKEN) and const hasRecoveryCredentials = Boolean(process.env.E2E_BOT_RECOVERY_KEY && process.env.E2E_SENDER_RECOVERY_KEY), then apply hasCoreCredentials to the suite-level checks and hasRecoveryCredentials only to the recovery-specific test or describe block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 3: The package.json "version" field is still set to "0.2.0" but this PR
is a breaking release and should use the intended major version; update the JSON
"version" property value from "0.2.0" to "1.0.0" (or the agreed major release
number) so the package reflects a breaking semver change and downstream
consumers receive the correct version signal.
In `@src/index.ts`:
- Around line 1156-1159: The call to persistDeviceIDForResolvedUser in the
Promise.all is missing the resolved device ID so a device discovered via whoami
can be dropped; update the Promise.all call to pass the resolved.deviceID into
persistDeviceIDForResolvedUser (only when present) so the function receives the
device ID discovered by whoami, i.e. change the invocation around
persistDeviceIDForResolvedUser(userID) to include resolved.deviceID (or guard
and pass undefined/skip if not set) alongside the existing
persistSession(resolved) call.
- Around line 2101-2109: The current extractFilesFromMessage implementation uses
isRecord(file) which doesn't ensure required FileUpload fields; update
extractFilesFromMessage to only return entries where file is an object and has a
non-empty filename string and a valid binary payload (e.g. Buffer | Uint8Array |
ArrayBuffer or an acceptable base64 string) for the data field, rather than just
isRecord(file). In practice modify the filter in extractFilesFromMessage to
check typeof file.filename === "string" && file.filename.trim() !== "" and
validate file.data shape (use Buffer.isBuffer, instanceof Uint8Array/ArrayBuffer
or typeof === "string" for base64), and skip (or log) any invalid items so only
fully-formed FileUpload objects are returned to downstream upload handlers.
- Around line 518-521: The cached DM roomID returned by loadPersistedDMRoomID
must be validated before reuse: in the flow around loadPersistedDMRoomID and
encodeThreadId, call the matrix client/room lookup (e.g., getRoom, getRoomState,
or check membership via room.getJoinedMembers) to confirm the room still exists
and the user is a member; if the room is missing or the user is not a member,
delete/clear the persisted mapping and proceed to create/open a fresh DM (i.e.,
do not return encodeThreadId for a stale ID). Ensure this validation and cleanup
happens before the early return so openDM can self-heal stale mappings.
- Around line 1139-1144: The code builds a local snapshot `updated` from
`existing` and calls `this.requireClient().setAccountData(EventType.Direct,
updated)`, which can overwrite concurrent updates; instead, read the latest
account data immediately before writing and merge safely: call the client to
fetch current EventType.Direct content, combine its mapping for `userID` with
`existingRooms` plus `roomID` (ensuring no duplicates), then call
`setAccountData(EventType.Direct, merged)` so you don't drop mappings added by
other sessions; reference symbols: `existing`, `updated`, `existingRooms`,
`userID`, `roomID`, `setAccountData`, and `EventType.Direct`.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 383-392: The code currently assumes mustGetEventByID will always
find the just-sent event, causing a hard failure if sync/timeline lag occurs;
wrap the call that builds the return (the mustGetEventByID call used when
constructing the raw field after sendRoomMessage) in a try/catch and on failure
return a safe fallback (e.g., raw: null or a minimal event object derived from
the send response) instead of throwing; apply the same pattern to the analogous
path in editMessage so timeline lag doesn't convert a successful send/edit into
an exception.
---
Nitpick comments:
In `@e2e/e2e.test.ts`:
- Around line 21-27: The current boolean hasCredentials gates the whole e2e
suite on recovery keys; split it into two flags (e.g., hasCoreCredentials and
hasRecoveryCredentials) derived from process.env, use hasCoreCredentials for the
general suite gating and only use hasRecoveryCredentials to guard the single
recovery scenario/test (update any skip or describe conditional that references
hasCredentials). Locate and update the symbol hasCredentials in e2e.test.ts and
replace its usages: create const hasCoreCredentials =
Boolean(process.env.E2E_BASE_URL && process.env.E2E_BOT_LOGIN_TOKEN &&
process.env.E2E_SENDER_LOGIN_TOKEN) and const hasRecoveryCredentials =
Boolean(process.env.E2E_BOT_RECOVERY_KEY &&
process.env.E2E_SENDER_RECOVERY_KEY), then apply hasCoreCredentials to the
suite-level checks and hasRecoveryCredentials only to the recovery-specific test
or describe block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 438ced23-aeeb-4192-b798-5bf6da46c696
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
e2e/.env.examplee2e/e2e.test.tse2e/helpers.tseslint.config.mjspackage.jsonsrc/index.test.tssrc/index.tsvitest.config.e2e.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/.env.example
- vitest.config.e2e.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (4)
src/index.test.ts (1)
src/index.ts (2)
MatrixAdapter(174-2500)createMatrixAdapter(2504-2570)
src/index.ts (2)
src/types.ts (1)
MatrixAdapterConfig(30-49)e2e/helpers.ts (1)
roomID(24-26)
e2e/e2e.test.ts (1)
e2e/helpers.ts (15)
E2EParticipant(29-38)roomID(24-26)createParticipant(40-51)env(8-27)getOrCreateRoom(175-199)waitForEncryptedRoom(275-283)waitForJoinedMemberCount(285-297)nonce(368-370)waitForFetchedMessage(299-318)sleep(364-366)shutdownParticipant(113-117)waitForMatchingMessage(320-345)waitForEvent(201-240)createParticipantFromSession(53-111)waitForRoom(262-273)
e2e/helpers.ts (1)
src/index.ts (1)
MatrixAdapter(174-2500)
🔇 Additional comments (3)
eslint.config.mjs (1)
12-25: Strong type-safety guardrails in lint config.The combination of
@typescript-eslint/no-explicit-anyplus restricted syntax forTSAnyKeywordassertions is a good enforcement layer.src/index.test.ts (1)
1255-1285: Great regression coverage for cursor validation paths.The tests for legacy cursor rejection and direction mismatch directly protect the new
mxv1paging contract.e2e/helpers.ts (1)
201-239:waitForEventcleanup and timeout handling is solid.The settle/cleanup flow here is robust and avoids common subscription-leak races.
Implement inbound Matrix rich-text normalization (strip reply fallbacks, parse Matrix pills) and outbound rendering of markdown/Chat SDK mention placeholders to formatted_body with m.mentions. Expose room metadata (roomID, DM status, topic, canonical alias, avatar MXC, encryption info) via fetchThread/fetchChannelInfo. Update tests/e2e and build config, and upgrade dependencies across lockfiles (bump chat/state packages, add marked/node-html-parser, eslint/types tooling, Redis client v5 and other transitive updates). README and types updated to reflect new behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
package.json (1)
36-38: Consider updatingmarkedto a newer version.Both
marked@15.0.12andnode-html-parser@7.1.0are valid versions. However,markedis 2 major versions behind the latest release (17.0.4). Consider updating to^17.0.0if the upgrade path is compatible with your codebase. Thenode-html-parser@7.1.0version is already at the latest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 36 - 38, Update the "marked" dependency in package.json from "^15.0.12" to a newer major (e.g., "^17.0.0") to align with the current upstream; run your test suite and search for usages of marked (import statements and any code calling marked.parse/marked.Renderer or options) to verify API compatibility, adjust any breaking API changes if tests fail, and then run npm install or yarn install to update lockfile.src/index.test.ts (2)
1293-1297: Consider strengthening the assertion to verify session structure.
expect.objectContaining({})matches any object and doesn't validate the persisted session's structure. Consider verifying key fields to catch regressions in session serialization.♻️ Suggested improvement
expect(state.set).toHaveBeenCalledWith( "matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot", - expect.objectContaining({}), + expect.objectContaining({ + accessToken: "persisted-token", + userID: "@bot:beeper.com", + deviceID: "DEVICE1", + authType: "password", + }), undefined );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.test.ts` around lines 1293 - 1297, The assertion currently uses expect.objectContaining({}) which accepts any object; update the test that calls state.set (the expect on state.set with the key "matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot") to assert specific session fields instead of an empty matcher — e.g., replace expect.objectContaining({}) with expect.objectContaining({ userId: expect.any(String), accessToken: expect.any(String), expiresAt: expect.any(Number) }) or similar fields your session object uses so the test validates the persisted session structure and types.
577-577: Consider usingvi.waitForor fake timers for more deterministic test behavior.The
setTimeout(resolve, 10)pattern is timing-dependent and can cause flaky tests on slow CI runners. Since the retry logic likely uses timers internally, consider using Vitest's fake timers for deterministic control.♻️ Suggested approach using fake timers
- await new Promise((resolve) => setTimeout(resolve, 10)); + vi.useFakeTimers(); + // Trigger pending timers to process retry + await vi.runAllTimersAsync(); + vi.useRealTimers();Alternatively, if the adapter exposes promise-based completion:
await vi.waitFor(() => { expect(fakeClient.joinRoom).toHaveBeenCalledTimes(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.test.ts` at line 577, Replace the ad-hoc sleep with deterministic timer control: remove the await new Promise(setTimeout...) and instead either use Vitest's fake timers (vi.useFakeTimers(), advance timers to trigger retry timers, then vi.useRealTimers()) or replace with vi.waitFor to assert completion; target the test that asserts fakeClient.joinRoom calls (reference fakeClient.joinRoom and the retry logic under test) so the test advances timers or waits until expect(fakeClient.joinRoom).toHaveBeenCalledTimes(2) becomes true rather than relying on a fixed 10ms sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/e2e.test.ts`:
- Around line 48-54: Tests are reusing a shared roomID which causes flakiness
for history-sensitive assertions; update the setup to create a fresh room per
test instead of reusing roomID by calling a room-creation helper (or
getOrCreateRoom) with a unique suffix (e.g., test name or timestamp) for each
test, and wait for readiness via waitForEncryptedRoom and
waitForJoinedMemberCount for that new room; alternatively, if you must reuse
rooms, make the pagination/thread-list assertions filter messages by a unique
per-test tag added to each message so functions like roomID, getOrCreateRoom,
waitForEncryptedRoom, and waitForJoinedMemberCount are used against the
fresh/uniquely tagged data to ensure assertions only consider the current test's
messages.
- Around line 21-27: The current hasCredentials boolean gates the entire E2E
suite on recovery keys; instead change its definition to only require login
tokens (E2E_BASE_URL, E2E_BOT_LOGIN_TOKEN, E2E_SENDER_LOGIN_TOKEN) and remove
recovery key checks from hasCredentials, then update the specific test(s) that
exercise fresh-device recovery to check for both E2E_BOT_RECOVERY_KEY and
E2E_SENDER_RECOVERY_KEY before calling createParticipant(recoveryKey) so only
those recovery-specific cases are skipped when recovery keys are missing.
In `@README.md`:
- Around line 11-13: The install command in README is missing the
`@chat-adapter/state-memory` package referenced later; update the npm install line
(the command that currently installs "chat `@beeper/chat-adapter-matrix`
matrix-js-sdk") to also include "@chat-adapter/state-memory" so the sample
imports that reference state-memory will resolve when following Quick Start.
---
Nitpick comments:
In `@package.json`:
- Around line 36-38: Update the "marked" dependency in package.json from
"^15.0.12" to a newer major (e.g., "^17.0.0") to align with the current
upstream; run your test suite and search for usages of marked (import statements
and any code calling marked.parse/marked.Renderer or options) to verify API
compatibility, adjust any breaking API changes if tests fail, and then run npm
install or yarn install to update lockfile.
In `@src/index.test.ts`:
- Around line 1293-1297: The assertion currently uses
expect.objectContaining({}) which accepts any object; update the test that calls
state.set (the expect on state.set with the key
"matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot") to assert specific
session fields instead of an empty matcher — e.g., replace
expect.objectContaining({}) with expect.objectContaining({ userId:
expect.any(String), accessToken: expect.any(String), expiresAt:
expect.any(Number) }) or similar fields your session object uses so the test
validates the persisted session structure and types.
- Line 577: Replace the ad-hoc sleep with deterministic timer control: remove
the await new Promise(setTimeout...) and instead either use Vitest's fake timers
(vi.useFakeTimers(), advance timers to trigger retry timers, then
vi.useRealTimers()) or replace with vi.waitFor to assert completion; target the
test that asserts fakeClient.joinRoom calls (reference fakeClient.joinRoom and
the retry logic under test) so the test advances timers or waits until
expect(fakeClient.joinRoom).toHaveBeenCalledTimes(2) becomes true rather than
relying on a fixed 10ms sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8cd5861-998d-4304-b5b0-9187b6a99244
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
README.mde2e/e2e.test.tspackage.jsonsrc/index.test.tssrc/index.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/e2e.test.ts (1)
e2e/helpers.ts (13)
E2EParticipant(29-38)roomID(24-26)createParticipant(40-51)env(8-27)waitForEncryptedRoom(275-283)nonce(368-370)waitForFetchedMessage(299-318)sleep(364-366)shutdownParticipant(113-117)waitForMatchingMessage(320-345)waitForEvent(201-240)createParticipantFromSession(53-111)waitForRoom(262-273)
🔇 Additional comments (10)
package.json (3)
3-3: Version mismatch: should be1.0.0per PR objectives.This issue was already flagged in a previous review. The PR objectives state this is a breaking release targeting
1.0.0, but the version remains0.2.0.
25-29: LGTM!The new scripts are well-structured:
lint:typesfor ESLint,test:e2ewith a dedicated Vitest config, andtypecheckproperly chaining linting before type checking.
41-47: ESLint 10.x dependency versions are valid. All specified versions exist and are properly compatible:eslint^10.0.2,@eslint/js^10.0.1, andtypescript-eslint^8.56.1 are available on npm and match in major version.src/index.test.ts (7)
72-150: LGTM!The event helpers (
makeEvent,makeRawEvent,mapRawToEvent) are well-structured with sensible defaults and proper handling of relation metadata including thread roots and aggregated relations.
188-236: LGTM!The
makeClientmock factory provides comprehensive coverage of Matrix client methods with sensible defaults for pagination endpoints, room operations, and account data management.
285-318: LGTM!The
makeStateAdapterutility properly wraps the memory state withvi.fn()spies while maintaining theafterReadypattern to ensure initialization completes before operations. Clean approach for verifying state interactions.
849-900: LGTM!Comprehensive test coverage for Matrix rich text normalization including reply fallback stripping, pill parsing, and author metadata extraction. The assertions properly verify both plain text and formatted markdown output.
1801-1870: LGTM!Excellent test for attachment handling with proper global stub cleanup using
try/finally. The test verifies both the attachment metadata extraction and the authenticated fetch flow for downloading media.
1872-1927: LGTM!The
openDMtest comprehensively covers the three-tier lookup strategy: cached mapping → m.direct account data → room creation with persistence. Good coverage of the DM mapping lifecycle.
1965-2017: LGTM!The thread listing test properly validates the server-side thread API integration including the mxv1 cursor format, reply counts from aggregated relations, and pagination token handling.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
e2e/e2e.test.ts (1)
558-558: Consider usingtoBeGreaterThanOrEqualfor clearer failure output.Vitest's
toBeGreaterThanOrEqualmatcher produces more informative failure messages thanexpect(x >= n).toBe(true).✨ Suggested improvement
- expect((channelInfo.memberCount ?? 0) >= 2).toBe(true); + expect(channelInfo.memberCount ?? 0).toBeGreaterThanOrEqual(2);Same applies to Lines 580 and 626.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/e2e.test.ts` at line 558, Replace boolean assertions that use expect((channelInfo.memberCount ?? 0) >= 2).toBe(true) (and the similar checks at the other occurrences) with Vitest's numeric matcher expect(channelInfo.memberCount ?? 0).toBeGreaterThanOrEqual(2) so failures print clearer diffs; update the three occurrences (the one using channelInfo.memberCount and the two analogous assertions referenced at lines 580 and 626) to use toBeGreaterThanOrEqual with the same numeric threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/e2e.test.ts`:
- Around line 488-502: The current pagination loop only pulls 3 pages (15
messages) which can miss some offlinePosts in shared rooms; update the test
around bot.adapter.fetchMessages, fetchedIds, cursor and offlinePosts to either
(preferred) continue fetching pages in a loop until either cursor is falsy or
all offlinePosts IDs are present in fetchedIds, or (alternative) collect all
fetched messages and filter them by restartTag before asserting that every
offlinePosts ID is present; ensure the loop exits on cursor falsy to avoid
infinite loops and keep the final assertions using the filtered/fully-fetched
set.
In `@src/index.test.ts`:
- Around line 287-307: The helper currently calls base.connect() inside ready
and again inside the exported connect spy, causing double connections; remove
the await base.connect() call from the ready IIFE and instead move the initial
population loop (for ... of Object.entries(initial) await base.set(...)) into
the connect spy (the vi.fn() for connect) so that base.connect() is only called
once (inside connect) and initial keys are populated after a single successful
base.connect(); keep afterReady, get, set, and other spies unchanged but ensure
connect awaits both base.connect() and the initial population before resolving.
- Around line 1920-1975: The test reuses the same fakeClient across three openDM
flows (cache, m.direct, create) which can mask regressions; either split into
separate tests for each branch or reset/replace the mocks between phases—for
example, create a fresh fakeClient (or call jest.clearAllMocks()/mockReset on
fakeClient methods) before constructing adapterFromDirect and adapterCreate so
calls to createRoom and setAccountData in the middle phase are asserted
independently; update references in the test to use the new fakeClient instances
or cleared mocks for adapterFromDirect and adapterCreate (symbols: fakeClient,
adapterFromCache, adapterFromDirect, adapterCreate, openDM).
- Around line 567-577: Replace the real sleep in the test with Vitest fake
timers: wrap the test steps that trigger joinRoomWithRetry (the call to
timelineHandler with makeEvent) in vi.useFakeTimers(), call timelineHandler,
then drive the retry timers with vi.runAllTimersAsync() (and restore timers
after). Ensure you target the retry mechanism inside joinRoomWithRetry so the
test no longer awaits setTimeout(10) but advances fake timers instead.
---
Nitpick comments:
In `@e2e/e2e.test.ts`:
- Line 558: Replace boolean assertions that use expect((channelInfo.memberCount
?? 0) >= 2).toBe(true) (and the similar checks at the other occurrences) with
Vitest's numeric matcher expect(channelInfo.memberCount ??
0).toBeGreaterThanOrEqual(2) so failures print clearer diffs; update the three
occurrences (the one using channelInfo.memberCount and the two analogous
assertions referenced at lines 580 and 626) to use toBeGreaterThanOrEqual with
the same numeric threshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bd99772-ec44-4947-863c-19a5567727d6
📒 Files selected for processing (4)
e2e/e2e.test.tspackage.jsonsrc/index.test.tssrc/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.test.ts (2)
src/index.ts (2)
MatrixAdapter(214-3130)createMatrixAdapter(3134-3200)e2e/helpers.ts (1)
roomID(24-26)
🔇 Additional comments (7)
e2e/e2e.test.ts (7)
21-31: Credential gating correctly separates core and recovery requirements.The refactored approach properly gates the suite on
hasCoreCredentials(login tokens only) while isolating the recovery-specific test withhasRecoveryCredentialsat Line 706. This addresses the prior concern about requiring recovery keys for all tests.
50-56: Shared room state concern already flagged.This was previously identified; per-test room isolation or unique tag filtering would improve determinism.
706-739: Recovery test properly isolated with conditional skip.Using
it.skipIf(!hasRecoveryCredentials)ensures this test only runs when recovery keys are available, while the rest of the suite can execute with just login tokens.
741-765: LGTM!The typing notification test correctly sets up a member event listener, triggers typing via the adapter, and properly cleans up the subscription.
92-143: Well-structured messaging and formatting tests.The tests use unique nonce tags, verify encryption state via
isEncrypted()andgetWireType(), and properly assert message author attribution. The mention detection withisMentionand markdown preservation viastringifyMarkdownare correctly validated.
201-284: Reaction tests correctly use event-based verification with cleanup.Both add and remove flows wait for the corresponding event callback before asserting, and the cleanup functions (
return () => sender.onReaction(null)) prevent listener leaks.
662-704: Comprehensive attachment test covers live and fetched paths.The test validates encrypted file upload, verifies
mxc://URL presence, and confirmsfetchData()returns correct content for both live message and API-fetched message. The encryption state is also checked.
- add Matrix chat-state store and tests to support parity-focused history handling - update adapter/types/tests for the new state flow - migrate project and CI/publish workflows to pnpm on Node 22
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
11-13:⚠️ Potential issue | 🟡 MinorQuick Start install command is missing a required package.
The install command does not include
@chat-adapter/state-memory, which is imported on line 19 and used on line 24. Following the documented steps results in a module-not-found error.📝 Proposed fix
-pnpm add chat `@beeper/chat-adapter-matrix` matrix-js-sdk +pnpm add chat `@beeper/chat-adapter-matrix` matrix-js-sdk `@chat-adapter/state-memory`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 11 - 13, Update the Quick Start install command to include the missing package `@chat-adapter/state-memory` so consumers don't hit a module-not-found error; modify the README's install snippet that currently lists chat, `@beeper/chat-adapter-matrix`, and matrix-js-sdk to also include `@chat-adapter/state-memory` (this package is imported and used later in the example), ensuring the documented command installs all runtime dependencies referenced by the sample imports/usages.
🧹 Nitpick comments (5)
e2e/helpers.ts (3)
388-403: Fragile access to private adapter internals.
getInitializedClientusesReflect.getto access the privateclientfield. This will break silently if the internal field is renamed or restructured.Consider exposing a public accessor on
MatrixAdapterfor test scenarios, or documenting this as a known test-only coupling point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 388 - 403, getInitializedClient currently reaches into adapter internals via Reflect.get(adapter, "client"), which is fragile; add a public accessor on MatrixAdapter (e.g., getClient or getClientForTests) that returns MatrixClient | undefined and update getInitializedClient to call that accessor and validate via isMatrixClient, or alternatively add a documented test-only method on MatrixAdapter and use it here; update references to the client access in getInitializedClient and keep isMatrixClient unchanged so the runtime type guard still verifies the returned value.
213-214: Hardcoded sleep for sync propagation.The 2-second hardcoded delay is a timing-based workaround that may cause flaky tests under load or slow environments.
Consider polling for the expected room state instead, similar to
waitForRoomused elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 213 - 214, Replace the hardcoded await sleep(2_000) used to wait for inviteAutoJoin propagation with a polling-based check that waits until the room state reflects the sender's membership; call the existing waitForRoom (or extract its polling logic) to repeatedly fetch the room and assert the sender is joined rather than sleeping, and remove the sleep call from e2e/helpers.ts so tests wait deterministically for the room membership condition.
9-31: Consider adding validation for required environment variables.The env getters use non-null assertions (
!) which will throw crypticundefinederrors at runtime if required variables are missing. For e2e tests, clearer errors would improve debugging.💡 Optional: Add validation helper
export const env = { get baseURL(): string { - return process.env.E2E_BASE_URL!; + const value = process.env.E2E_BASE_URL; + if (!value) throw new Error("E2E_BASE_URL is required"); + return value; }, // ... similar for other required vars };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 9 - 31, The env getters currently use non-null assertions (env.baseURL, env.botLoginToken, env.senderLoginToken) which yield cryptic runtime errors if variables are missing; add explicit validation by replacing the direct non-null assertions with a small helper (e.g., getRequiredEnv(key): string) or inline checks that read process.env and throw a descriptive Error like "Missing required E2E env var: E2E_BASE_URL" so failures are clear; keep optional vars (botRecoveryKey, senderRecoveryKey, roomID, redisURL) unchanged but ensure required getters (baseURL, botLoginToken, senderLoginToken) call the validator and return a non-optional string.src/store/chat-state-matrix-store.test.ts (1)
148-152: Type assertion bypasses type safety in tests.Using
as neverto bypass type checking can mask issues if theISyncResponsestructure changes.💡 Alternative: Use partial mock that satisfies the interface
Consider creating a typed helper that produces a minimal valid
ISyncResponseinstead of casting tonever.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/chat-state-matrix-store.test.ts` around lines 148 - 152, The test is bypassing TypeScript safety by using `as never` when calling `store.setSyncData`; instead create a small helper that returns a correctly typed minimal ISyncResponse (e.g. add or update `makeSavedSync` to produce an object shaped as ISyncResponse or add a new `makeValidSyncResponse(next_batch, roomsData, accountData): ISyncResponse`) and pass that typed object to `store.setSyncData` (remove the `as never`), so the call to `store.setSyncData(...)` uses a real ISyncResponse and fails type-checks if the interface changes.src/store/chat-state-matrix-store.ts (1)
176-203: Consider concurrency if save() may be called rapidly.If
save()can be invoked from multiple sources (e.g., auto-save timers, manual triggers, destroy), concurrent calls could interleave thePromise.alloperations. In typical single-consumer usage this is fine, but if needed, a simple in-flight guard could prevent overlapping writes:private saving: Promise<void> | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/chat-state-matrix-store.ts` around lines 176 - 203, Add an in-flight guard property (e.g., private saving: Promise<void> | null = null) and use it inside save() so overlapping invocations reuse/wait on the same promise instead of starting concurrent writes; implement save() to early-return the existing this.saving when present (or await it if you need to ensure ordering with force), then assign this.saving to the internal async work that builds the meta (using latestSavedSync, savedSyncKey, metaKey, snapshotTtlMs, state.set, wantsSave(), dirty, lastSavedAt) and finally clears this.saving in a .finally() handler so subsequent calls can proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/get-access-token.ts`:
- Line 1: Add a top-level "engines" field to package.json to require Node 22+ by
adding "engines": { "node": ">=22" } so scripts that rely on the experimental
--experimental-strip-types shebang (e.g., scripts/get-access-token.ts) won't run
on older Node versions; update package.json accordingly and commit the change.
---
Duplicate comments:
In `@README.md`:
- Around line 11-13: Update the Quick Start install command to include the
missing package `@chat-adapter/state-memory` so consumers don't hit a
module-not-found error; modify the README's install snippet that currently lists
chat, `@beeper/chat-adapter-matrix`, and matrix-js-sdk to also include
`@chat-adapter/state-memory` (this package is imported and used later in the
example), ensuring the documented command installs all runtime dependencies
referenced by the sample imports/usages.
---
Nitpick comments:
In `@e2e/helpers.ts`:
- Around line 388-403: getInitializedClient currently reaches into adapter
internals via Reflect.get(adapter, "client"), which is fragile; add a public
accessor on MatrixAdapter (e.g., getClient or getClientForTests) that returns
MatrixClient | undefined and update getInitializedClient to call that accessor
and validate via isMatrixClient, or alternatively add a documented test-only
method on MatrixAdapter and use it here; update references to the client access
in getInitializedClient and keep isMatrixClient unchanged so the runtime type
guard still verifies the returned value.
- Around line 213-214: Replace the hardcoded await sleep(2_000) used to wait for
inviteAutoJoin propagation with a polling-based check that waits until the room
state reflects the sender's membership; call the existing waitForRoom (or
extract its polling logic) to repeatedly fetch the room and assert the sender is
joined rather than sleeping, and remove the sleep call from e2e/helpers.ts so
tests wait deterministically for the room membership condition.
- Around line 9-31: The env getters currently use non-null assertions
(env.baseURL, env.botLoginToken, env.senderLoginToken) which yield cryptic
runtime errors if variables are missing; add explicit validation by replacing
the direct non-null assertions with a small helper (e.g., getRequiredEnv(key):
string) or inline checks that read process.env and throw a descriptive Error
like "Missing required E2E env var: E2E_BASE_URL" so failures are clear; keep
optional vars (botRecoveryKey, senderRecoveryKey, roomID, redisURL) unchanged
but ensure required getters (baseURL, botLoginToken, senderLoginToken) call the
validator and return a non-optional string.
In `@src/store/chat-state-matrix-store.test.ts`:
- Around line 148-152: The test is bypassing TypeScript safety by using `as
never` when calling `store.setSyncData`; instead create a small helper that
returns a correctly typed minimal ISyncResponse (e.g. add or update
`makeSavedSync` to produce an object shaped as ISyncResponse or add a new
`makeValidSyncResponse(next_batch, roomsData, accountData): ISyncResponse`) and
pass that typed object to `store.setSyncData` (remove the `as never`), so the
call to `store.setSyncData(...)` uses a real ISyncResponse and fails type-checks
if the interface changes.
In `@src/store/chat-state-matrix-store.ts`:
- Around line 176-203: Add an in-flight guard property (e.g., private saving:
Promise<void> | null = null) and use it inside save() so overlapping invocations
reuse/wait on the same promise instead of starting concurrent writes; implement
save() to early-return the existing this.saving when present (or await it if you
need to ensure ordering with force), then assign this.saving to the internal
async work that builds the meta (using latestSavedSync, savedSyncKey, metaKey,
snapshotTtlMs, state.set, wantsSave(), dirty, lastSavedAt) and finally clears
this.saving in a .finally() handler so subsequent calls can proceed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eaa89baf-75c7-4ecb-9f5e-094361f8aa4e
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/publish.ymlREADME.mde2e/e2e.test.tse2e/helpers.tspackage.jsonscripts/get-access-token.tssrc/index.test.tssrc/index.tssrc/store/chat-state-matrix-store.test.tssrc/store/chat-state-matrix-store.tssrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/e2e.test.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
src/types.ts (1)
src/index.ts (2)
MatrixSyncStoreConfig(3451-3451)MatrixCreateStoreOptions(3450-3450)
e2e/helpers.ts (1)
src/index.ts (1)
MatrixAdapter(228-3376)
🔇 Additional comments (21)
src/index.test.ts (3)
295-331: The state adapter helper callsbase.connect()twice.The
readyIIFE callsbase.connect(), and theconnectspy also callsbase.connect()viaafterReady. This causes the underlying memory state to be connected twice when tests callconnect().
580-605: RealsetTimeoutmakes test timing-sensitive.The test uses
await new Promise((resolve) => setTimeout(resolve, 10))to wait for retry logic, which can be flaky on slower CI systems. Consider using Vitest fake timers (vi.useFakeTimers()andvi.runAllTimersAsync()) to drive the retry deterministically.
2060-2115: Consider resetting mocks between test phases or splitting into separate tests.This test reuses the same
fakeClientinstance across three distinct openDM flows (cache-hit, m.direct lookup, create-room) without resetting mocks between phases. While each phase creates a fresh adapter, the accumulated mock call history could mask regressions—for example, if the middle phase incorrectly calledcreateRoom, it would still pass because phase 3's assertion only checks thatcreateRoomwas called at some point..github/workflows/ci.yml (1)
20-41: LGTM!The pnpm migration is well-executed:
- pnpm version 10.30.3 is pinned consistently with
package.json- Node.js 22 is current LTS
--frozen-lockfileensures reproducible CI buildsscripts/get-access-token.ts (1)
302-302: LGTM!Header text updated to be runtime-agnostic, consistent with the Bun → Node migration.
.github/workflows/publish.yml (1)
23-58: LGTM!Publish workflow aligns with CI changes. The
--provenanceflag for npm provenance attestations is a good security practice.package.json (2)
67-68: LGTM!The
packageManagerfield ensures consistent tooling across environments via corepack.
41-47: No action needed. ESLint v10.0.3 is the current stable version as of March 2026, and the specified versions (^10.0.2for eslint and^10.0.1for@eslint/js) are valid and will install without issues.> Likely an incorrect or invalid review comment.README.md (1)
134-156: LGTM!The Capabilities and Notes sections provide clear documentation of the adapter's feature set, including explicit callouts for unsupported features (
postEphemeral,openModal,stream).src/types.ts (2)
14-29: LGTM!The new store configuration types (
MatrixSyncStoreConfig,MatrixCreateStoreOptions) are well-structured with appropriate optional fields and required dependencies clearly defined.
72-75: LGTM!
MatrixInviteAutoJoinConfigis clean and matches the documented behavior in README.md.e2e/helpers.ts (2)
260-278: LGTM!The
waitForConditionpolling utility has appropriate timeout handling and configurable intervals.
317-386: LGTM!Good transient error handling in
waitForFetchedMessageandwaitForMatchingMessage. TheisTransientMatrixErrorcheck ensures rate-limiting and temporary server errors don't cause immediate test failures.src/store/chat-state-matrix-store.test.ts (2)
8-72: LGTM!Well-structured test helpers with clear factory patterns. The scope key encoding in
makeStorematches the expected production format.
84-247: LGTM!Comprehensive test coverage for the
ChatStateMatrixStore:
- Startup loading and sync token retrieval
- Dirty flag and persistence interval behavior
- Cross-instance data persistence
- Full data deletion cleanup
src/store/chat-state-matrix-store.ts (6)
1-31: LGTM!Well-structured imports and type definitions. The
STORE_VERSIONconstant enables future migration handling, and the persistence types cleanly separate concerns (meta, to-device state, room index).
33-75: LGTM!Solid defensive helper functions with proper type guards. The normalization functions gracefully handle malformed or corrupted storage data, which is important for a persistence layer.
77-109: LGTM!Clean class structure with well-organized private fields. Configuration options are clearly separated from runtime state, and all collections are properly initialized.
111-162: LGTM!Well-implemented startup with parallel loading and proper handling of legacy data formats (lines 152-158). The
startedguard is effective given JavaScript's single-threaded event loop model.
267-394: LGTM!Well-implemented cache-through patterns with proper index management. The fire-and-forget persistence in
setFilterIdByName(lines 391-393) is appropriate since filter IDs can be regenerated if lost.
396-514: LGTM!Comprehensive
deleteAllDataimplementation that properly captures room IDs before clearing maps. Good use ofencodeURIComponentin key generation (lines 508, 512) to prevent key injection from room IDs containing special characters.
- Make test state adapter initialization run on explicit connect - Use fake timers for invite auto-join to remove timing flakiness - Split openDM coverage into cached, m.direct, and create-and-persist cases
- Fetch E2E history pages until all offline message IDs are seen or cursor ends - Make test state adapter lazily connect once before get/set operations
- Expand usage docs with env-based `createMatrixAdapter()` setup and lifecycle - Document auth modes, thread model, feature matrix, and limitations - Add comprehensive options/env var tables plus persistence/E2EE guidance
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
package.json (1)
3-3:⚠️ Potential issue | 🟠 MajorPublish this breaking change as
1.0.0, not0.2.0.Line 3 still advertises a minor release, but this PR is framed as a breaking release with
1.0.0parity semantics. Shipping it as0.2.0gives downstream consumers the wrong semver signal.🔧 Proposed change
- "version": "0.2.0", + "version": "1.0.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 3, The package.json "version" field currently reads "0.2.0" which contradicts the breaking-change intent; update the version value in package.json to "1.0.0" so the manifest and semver correctly signal a major, breaking release (change the "version" property string from "0.2.0" to "1.0.0").
🧹 Nitpick comments (1)
e2e/e2e.test.ts (1)
566-566: Consider using Vitest's built-in matchers for clearer assertions.The pattern
expect((x ?? 0) >= 2).toBe(true)is functional but less expressive. Consider:- expect((channelInfo.memberCount ?? 0) >= 2).toBe(true); + expect(channelInfo.memberCount).toBeGreaterThanOrEqual(2);If
memberCountcan be undefined, add a guard or useexpect(channelInfo.memberCount ?? 0).toBeGreaterThanOrEqual(2).The same pattern appears at lines 588 and 634.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/e2e.test.ts` at line 566, Replace the boolean-compare style assertion with Vitest's numeric matcher: change occurrences of expect((channelInfo.memberCount ?? 0) >= 2).toBe(true) to use expect(channelInfo.memberCount ?? 0).toBeGreaterThanOrEqual(2); do the same for the other instances referenced (the similar checks at lines around the other occurrences) so that tests use the expressive toBeGreaterThanOrEqual matcher on channelInfo.memberCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 32: The "typecheck" script currently shells out to pnpm ("pnpm lint:types
&& tsc --noEmit"), which breaks on machines/CI without pnpm/Corepack; update the
package.json "typecheck" script to invoke the local lint script without relying
on pnpm (e.g., use "npm run lint:types && tsc --noEmit" or simply "npm run
lint:types --silent && tsc --noEmit") so it runs with npm (and works on yarn as
well when using the package manager's run command), and ensure the referenced
script name "lint:types" remains unchanged.
In `@src/index.test.ts`:
- Around line 1489-1492: The current assertion uses expect.objectContaining({})
which is empty and won't catch regressions; update the test that calls state.set
(the assertion around state.set for the key "matrix:session:...") to assert the
required persisted session fields produced by persistSession() — e.g., replace
expect.objectContaining({}) with expect.objectContaining({ accessToken:
expect.any(String), userId: expect.any(String), deviceId: expect.any(String),
homeserver: expect.any(String), expiresAt: expect.any(Number) }) or the actual
fields your persistSession function writes (include refreshToken or other keys
if present) so the test verifies the expected schema rather than an empty
object. Ensure this references the same state.set call and the session key used
in the diff.
---
Duplicate comments:
In `@package.json`:
- Line 3: The package.json "version" field currently reads "0.2.0" which
contradicts the breaking-change intent; update the version value in package.json
to "1.0.0" so the manifest and semver correctly signal a major, breaking release
(change the "version" property string from "0.2.0" to "1.0.0").
---
Nitpick comments:
In `@e2e/e2e.test.ts`:
- Line 566: Replace the boolean-compare style assertion with Vitest's numeric
matcher: change occurrences of expect((channelInfo.memberCount ?? 0) >=
2).toBe(true) to use expect(channelInfo.memberCount ??
0).toBeGreaterThanOrEqual(2); do the same for the other instances referenced
(the similar checks at lines around the other occurrences) so that tests use the
expressive toBeGreaterThanOrEqual matcher on channelInfo.memberCount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b323d98-c3e7-48df-85d2-3d1576635709
📒 Files selected for processing (3)
e2e/e2e.test.tspackage.jsonsrc/index.test.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.test.ts (2)
src/index.ts (2)
MatrixAdapter(228-3376)createMatrixAdapter(3380-3446)e2e/helpers.ts (2)
roomID(25-27)baseURL(10-12)
e2e/e2e.test.ts (1)
e2e/helpers.ts (14)
E2EParticipant(33-42)roomID(25-27)createParticipant(44-55)env(9-31)waitForEncryptedRoom(293-301)waitForJoinedMemberCount(303-315)nonce(409-411)waitForFetchedMessage(317-344)sleep(405-407)shutdownParticipant(131-135)waitForMatchingMessage(346-386)waitForEvent(219-258)createParticipantFromSession(57-118)waitForRoom(280-291)
🔇 Additional comments (13)
e2e/e2e.test.ts (10)
21-29: Credential gating correctly separates core and recovery tests.The split between
hasCoreCredentials(login tokens only) andhasRecoveryCredentials(recovery keys) ensures that non-recovery tests run when only login tokens are available, while the recovery-specific test at Line 714 is independently gated.
50-56: Shared room state across tests may cause flakiness.All tests reuse the same
roomID, which means history-sensitive tests (pagination, thread listing) can be affected by messages from prior tests. For more deterministic results, consider creating fresh rooms for history-sensitive cases or filtering fetched messages by the unique per-test tag.
92-143: LGTM!The basic message tests properly use unique tags for identification, verify encryption state, and use
waitForFetchedMessagewith predicates for reliable assertions.
145-199: LGTM!The thread round-trip test thoroughly covers thread creation, child message visibility across participants, and bot replies. The
encodeThreadId/decodeThreadIdverification is a good sanity check.
201-247: LGTM!The reaction tests use proper event-based verification with cleanup, testing both directions (bot→sender, sender→bot).
334-371: LGTM!Using the raw Matrix client API to send a reply with realistic fallback markup is a good approach for testing the adapter's fallback stripping logic.
489-507: LGTM!The pagination loop correctly fetches pages until either all offline posts are found or the cursor is exhausted. This addresses the earlier concern about fixed page limits missing messages in shared rooms.
670-712: LGTM!The file attachment test thoroughly verifies encrypted upload, live event delivery, and fetched message attachments with content verification via
fetchData.
714-747: LGTM!The recovery test is correctly gated with
skipIf(!hasRecoveryCredentials)and properly tests fresh device key recovery by shutting down the existing sender, posting a message, then restoring with a new login and recovery key.
749-773: LGTM!The typing notification test properly verifies that the
startTypingcall propagates to other room members via theRoomMemberEvent.Typingevent.src/index.test.ts (3)
295-334: State-adapter readiness handling looks correct now.
connect()is single-shot and the rest of the helper gates on the same readiness promise, so this no longer double-connects or races initialization.
550-599: Good switch to fake timers for invite retry.Driving the retry path with
vi.runAllTimersAsync()removes the sleep-based flake from this test.
2069-2133: Nice split of theopenDMbranches.These separate tests now assert cache-hit,
m.direct, and create-room flows independently, so one branch can’t mask unexpectedcreateRoomorsetAccountDatacalls from another.
| "test:e2e": "vitest run --config vitest.config.e2e.ts", | ||
| "test:watch": "vitest", | ||
| "typecheck": "tsc --noEmit", | ||
| "typecheck": "pnpm lint:types && tsc --noEmit", |
There was a problem hiding this comment.
Make typecheck package-manager agnostic.
Line 32 shells out to pnpm, so npm run typecheck—the command listed in this PR—fails on machines/CI jobs without pnpm or Corepack enabled. Inline the command chain instead of nesting a pnpm-specific call.
🔧 Proposed change
- "typecheck": "pnpm lint:types && tsc --noEmit",
+ "typecheck": "eslint . && tsc --noEmit",📝 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.
| "typecheck": "pnpm lint:types && tsc --noEmit", | |
| "typecheck": "eslint . && tsc --noEmit", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 32, The "typecheck" script currently shells out to pnpm
("pnpm lint:types && tsc --noEmit"), which breaks on machines/CI without
pnpm/Corepack; update the package.json "typecheck" script to invoke the local
lint script without relying on pnpm (e.g., use "npm run lint:types && tsc
--noEmit" or simply "npm run lint:types --silent && tsc --noEmit") so it runs
with npm (and works on yarn as well when using the package manager's run
command), and ensure the referenced script name "lint:types" remains unchanged.
| expect(state.set).toHaveBeenCalledWith( | ||
| "matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot", | ||
| expect.any(Object), | ||
| expect.objectContaining({}), | ||
| undefined |
There was a problem hiding this comment.
Strengthen the persisted-session assertion.
expect.objectContaining({}) is vacuous, so this test won't catch schema regressions in what persistSession() writes. Assert the required stored fields instead.
🔧 Proposed change
expect(state.set).toHaveBeenCalledWith(
"matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot",
- expect.objectContaining({}),
+ expect.objectContaining({
+ accessToken: "persisted-token",
+ userID: "@bot:beeper.com",
+ deviceID: "DEVICE1",
+ authType: "password",
+ baseURL,
+ username: "bot",
+ }),
undefined
);📝 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.
| expect(state.set).toHaveBeenCalledWith( | |
| "matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot", | |
| expect.any(Object), | |
| expect.objectContaining({}), | |
| undefined | |
| expect(state.set).toHaveBeenCalledWith( | |
| "matrix:session:https%3A%2F%2Fhs.beeper.com:username:bot", | |
| expect.objectContaining({ | |
| accessToken: "persisted-token", | |
| userID: "@bot:beeper.com", | |
| deviceID: "DEVICE1", | |
| authType: "password", | |
| baseURL, | |
| username: "bot", | |
| }), | |
| undefined | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.test.ts` around lines 1489 - 1492, The current assertion uses
expect.objectContaining({}) which is empty and won't catch regressions; update
the test that calls state.set (the assertion around state.set for the key
"matrix:session:...") to assert the required persisted session fields produced
by persistSession() — e.g., replace expect.objectContaining({}) with
expect.objectContaining({ accessToken: expect.any(String), userId:
expect.any(String), deviceId: expect.any(String), homeserver:
expect.any(String), expiresAt: expect.any(Number) }) or the actual fields your
persistSession function writes (include refreshToken or other keys if present)
so the test verifies the expected schema rather than an empty object. Ensure
this references the same state.set call and the session key used in the diff.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
9-11:⚠️ Potential issue | 🟡 MinorInstallation command is missing a required dependency.
The install command on line 10 does not include
@chat-adapter/state-memory, but line 19 importscreateMemoryStatefrom this package and line 26 uses it. Users following the Quick Start will encounter a module-not-found error.📦 Proposed fix
-pnpm add chat `@beeper/chat-adapter-matrix` +pnpm add chat `@beeper/chat-adapter-matrix` `@chat-adapter/state-memory`Also applies to: 17-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 9 - 11, The installation command is missing the required package `@chat-adapter/state-memory` referenced by createMemoryState in the README; update the example install command that currently lists "chat" and "@beeper/chat-adapter-matrix" to also include "@chat-adapter/state-memory" so users can import createMemoryState and use it without a module-not-found error.
🧹 Nitpick comments (1)
README.md (1)
180-210: Add installation instruction for the Redis state adapter.The example imports and uses
@chat-adapter/state-redisbut doesn't mention that users need to install it first. Consider adding a note before the example or in the code comment to avoid module-not-found errors.📝 Suggested addition
Add before line 184 or as a code comment:
First, install the Redis state adapter: \```bash pnpm add `@chat-adapter/state-redis` \```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 180 - 210, Add a short installation note before the example showing persistence/E2EE that instructs users to install the Redis state adapter (package `@chat-adapter/state-redis`) to avoid module-not-found errors; mention the install command (e.g., pnpm add `@chat-adapter/state-redis`) and/or add an inline code comment above the import of createRedisState to remind readers to install the dependency before using createRedisState in the Chat example.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Around line 9-11: The installation command is missing the required package
`@chat-adapter/state-memory` referenced by createMemoryState in the README; update
the example install command that currently lists "chat" and
"@beeper/chat-adapter-matrix" to also include "@chat-adapter/state-memory" so
users can import createMemoryState and use it without a module-not-found error.
---
Nitpick comments:
In `@README.md`:
- Around line 180-210: Add a short installation note before the example showing
persistence/E2EE that instructs users to install the Redis state adapter
(package `@chat-adapter/state-redis`) to avoid module-not-found errors; mention
the install command (e.g., pnpm add `@chat-adapter/state-redis`) and/or add an
inline code comment above the import of createRedisState to remind readers to
install the dependency before using createRedisState in the Chat example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11259c18-4136-431a-be1b-44a12bb244dd
📒 Files selected for processing (1)
README.md
📜 Review details
🔇 Additional comments (1)
README.md (1)
264-271: All referenced files exist in the repository as documented. No action needed.
- Add `createIsolatedRoom` helper that creates an encrypted room and waits for encryption + both joins - Update pagination, restart catch-up, and thread-list E2E tests to use per-test rooms - Validate `createRoom` returns a `room_id` before proceeding
- Make persistence/state-backed Matrix store automatic when Chat state is available - Enable E2EE based on recovery key or e2ee config, and persist secrets via state - Refine device ID resolution order (explicit, auth-derived, then persisted) - Update docs, examples, and tests for new env vars and invite auto-join semantics
1 participant
Summary
openDM,fetchMessage,fetchChannelMessages, API-firstfetchMessages, and server-backedlistThreadsmxv1cursor codec with strict validation and hard rejection of legacy cursorsmatrix-js-sdk(/messages,/relations,/threads) instead of relying on local room timelinepostEphemeral,openModal, and nativestreamintentionally unsupported in this pass1.0.0with breaking-change notesTesting