fix: classify Matrix direct-message rooms - #7
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe adapter synchronously classifies Matrix rooms as DMs using cached ChangesDirect message classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MatrixClient
participant MatrixAdapter
participant ChatSDK
MatrixClient->>MatrixAdapter: provide m.direct account data
MatrixAdapter->>MatrixClient: request latest m.direct data
MatrixClient-->>MatrixAdapter: return account data
MatrixAdapter->>MatrixAdapter: populate directRoomIDs
ChatSDK->>MatrixAdapter: call isDM(threadId)
MatrixAdapter-->>ChatSDK: return cached DM classification
MatrixClient->>MatrixAdapter: emit m.direct account-data update
MatrixAdapter->>MatrixAdapter: rebuild directRoomIDs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 305-308: The `RoomMemberEvent.Membership` handler in
`src/index.ts` is triggering unbounded `refreshTwoPersonRoom(roomId)` calls for
every membership change, which can spam `getJoinedRoomMembers` and race on the
same room. Update the handler and `refreshTwoPersonRoom` flow to dedupe
in-flight refreshes per `roomId` (and optionally debounce or cap concurrency) so
repeated events for the same room reuse one pending request and only the latest
result updates `twoPersonRoomIDs`.
- Around line 370-378: The DM detection paths are inconsistent because isDM()
already treats two-person rooms as DMs while isDirectRoom() still only checks
m.direct account data. Update isDirectRoom() to also consult the cached
twoPersonRoomIDs set, mirroring the logic in isDM(), so fetchThread and
fetchChannelInfo produce the same ThreadInfo.isDM/ChannelInfo.isDM result for
the same room.
- Around line 300-311: `primeDMRoomIDs()` is only using the local account-data
cache, so DM IDs can be missing on cold start before `ClientEvent.AccountData`
fires. Update the startup flow in `src/index.ts` to prime direct room IDs via
`loadDirectAccountData()` instead of relying solely on the cache, and keep the
existing `replaceDirectRoomIDs(...)` path so `isDM()` sees existing `m.direct`
rooms immediately after initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1c35a86-f4cb-456b-bb93-28d676e5e7be
📒 Files selected for processing (3)
CHANGELOG.mdsrc/index.test.tssrc/index.ts
📜 Review details
🔇 Additional comments (5)
src/index.ts (3)
41-41: LGTM!Also applies to: 212-213
338-339: LGTM!Also applies to: 1225-1225
1084-1091: LGTM! Concurrency-capped priming (Math.min(4, roomIDs.length)) and the exact-two-member fallback logic are correctly implemented and match the test coverage.Also applies to: 1093-1132
src/index.test.ts (1)
5-12: LGTM! Solid coverage ofm.directmapping, authoritative two-person fallback, invalid thread IDs,openDM()immediate classification, and reclassification on bothAccountDataandRoomMember.membershipevents. Traced against the implementation and the assertions correctly reflect the expected cache transitions.Also applies to: 235-246, 479-547
CHANGELOG.md (1)
15-15: LGTM!
There was a problem hiding this comment.
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)
1072-1095: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winWarm restarts can still reuse stale
m.directdata here.
loadDirectAccountData()returns the cached snapshot whenever it is non-empty, soprimeDirectRoomIDs()does not force a homeserver refresh in the persisted-store case.- That leaves
isDM()able to misclassify rooms until the firstClientEvent.AccountDataupdate arrives.- The “member count” wording is stale; this path is now
m.direct-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 1072 - 1095, Update primeDirectRoomIDs() to force a fresh homeserver m.direct fetch on startup instead of relying on the non-empty cached snapshot returned by loadDirectAccountData(), then replace directRoomIDs with the refreshed data while retaining cached data only as an appropriate fallback on fetch failure. Also update the warning text to describe unmarked rooms as non-DMs or channels without mentioning member count.
🧹 Nitpick comments (1)
src/index.test.ts (1)
508-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a companion test for the "non-empty but stale" cache case.
This test covers the empty-cache/server-unavailable path well, but there's no test where
getAccountData()(local cache) already returns a non-empty, stalem.directmapping at startup — the scenario whereloadDirectAccountData()'s cache-shortcut (seesrc/index.tsaroundprimeDirectRoomIDs/loadDirectAccountData) causes priming to skip the server call entirely. Adding a test that seedsgetAccountDatawith stale data and asserts whethergetAccountDataFromServeris (not) called would make the freshness contract explicit and catch regressions either way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.test.ts` around lines 508 - 525, Add a companion test for stale non-empty m.direct cache data, using the existing client setup and initialization flow around primeDirectRoomIDs/loadDirectAccountData. Mock getAccountData to return a non-empty stale mapping, control getAccountDataFromServer, and assert whether the server method is called according to the intended freshness contract, along with verifying the resulting DM behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/index.ts`:
- Around line 1072-1095: Update primeDirectRoomIDs() to force a fresh homeserver
m.direct fetch on startup instead of relying on the non-empty cached snapshot
returned by loadDirectAccountData(), then replace directRoomIDs with the
refreshed data while retaining cached data only as an appropriate fallback on
fetch failure. Also update the warning text to describe unmarked rooms as
non-DMs or channels without mentioning member count.
---
Nitpick comments:
In `@src/index.test.ts`:
- Around line 508-525: Add a companion test for stale non-empty m.direct cache
data, using the existing client setup and initialization flow around
primeDirectRoomIDs/loadDirectAccountData. Mock getAccountData to return a
non-empty stale mapping, control getAccountDataFromServer, and assert whether
the server method is called according to the intended freshness contract, along
with verifying the resulting DM behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e613d00d-affa-4ce1-b357-90bcc78f59a5
📒 Files selected for processing (3)
CHANGELOG.mdsrc/index.test.tssrc/index.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
📜 Review details
🔇 Additional comments (7)
src/index.ts (4)
211-211: LGTM!Also applies to: 319-339
282-306: Listener-before-startClient ordering is correct;isDM()classification depends on it.Registering the
ClientEvent.AccountDatalistener (Lines 298-302) beforeprimeDirectRoomIDs()/startClient()ensures nom.directsync deltas are missed once syncing begins. Good sequencing.
363-370: LGTM!Synchronous, decode-and-lookup implementation matches the Chat SDK contract and the m.direct-only classification policy agreed in prior review discussion.
1188-1188: LGTM!Immediately adding the newly-created DM room to
directRoomIDsafter persisting them.directupdate correctly makesopenDM()-created rooms classifiable right away, matching the PR objective.src/index.test.ts (3)
5-11: LGTM!Also applies to: 234-237
470-506: LGTM!Good coverage: m.direct-mapped room, unmarked two-person room (regression guard for the removed heuristic), invalid thread id,
openDM()-created room, and account-data-driven reclassification.
2746-2761: LGTM!
|
Revision is pushed, as discussed above:
Full suite passes locally (80 tests). @coderabbitai review |
|
✅ Action performedReview finished.
|
1 participant
What this does
Implements Chat SDK's synchronous
isDM(threadId)hook for Matrix rooms.The adapter now:
m.directaccount data as direct messages;m.directdata during startup;m.directaccount data changes;openDM()immediately; andCloses #6
Why
Without
Adapter.isDM(), Chat SDK defaults every Matrix thread toisDM === false. Direct messages then go through channel handlers instead ofonDirectMessage().Matrix room size is not a reliable DM signal: an unmarked room with exactly two joined members is still a channel.
m.directis the explicit per-account classification used here.How I verified it
pnpm typecheckpnpm test(80 tests passed)pnpm buildm.directupdates, unmarked two-person rooms, invalid thread IDs, newly created DMs, and homeserver failure fallback.Assisted by Claude; reviewed and verified by a human before submission.