fix: dm check on invite #37012
fix: dm check on invite #37012
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 1d56b34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 46 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 166 files out of 295 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughReplaces heuristic DM detection in Matrix invite handling by reading is_direct directly from the membership event content. Removes the isDirectMessage helper, updates joinRoom to use inviteEvent.getContent().is_direct, and adds the PduMembershipEventContent import. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MatrixAPI
participant InviteHandler as invite.ts
Client->>MatrixAPI: Receive m.room.member invite
MatrixAPI->>InviteHandler: joinRoom(inviteEvent)
Note over InviteHandler: Previous: inspect room state to infer DM
InviteHandler->>InviteHandler: content = inviteEvent.getContent<PduMembershipEventContent>()
InviteHandler->>InviteHandler: isDM = content.is_direct
alt isDM
InviteHandler->>MatrixAPI: Proceed with DM-specific join handling
else not DM
InviteHandler->>MatrixAPI: Proceed with normal room join
end
Note over InviteHandler: Control flow simplified by using is_direct flag
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
134-143: Backoff math uses seconds as milliseconds; delays are wrong.You square a “seconds” value, log it as ms, pass it to
setTimeoutas ms, then passdelay*1000as the next “seconds.” This makes the first retry almost immediate and the next backoff explode.Use consistent units:
async function runWithBackoff(fn: () => Promise<void>, delaySec = 5) { try { await fn(); } catch (e) { - const delay = delaySec === 625 ? 625 : delaySec ** 2; - console.log(`error occurred, retrying in ${delay}ms`, e); - setTimeout(() => { - runWithBackoff(fn, delay * 1000); - }, delay); + const nextDelaySec = Math.min(625, delaySec ** 2); + const delayMs = delaySec * 1000; + console.log(`error occurred, retrying in ${delaySec}s`, e); + setTimeout(() => { + void runWithBackoff(fn, nextDelaySec); + }, delayMs); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
2-2: VerifyPduMembershipEventContentincludesis_direct/isDirectin@hs/roomLocal search returned no results — confirm the package's type declarations (e.g. node_modules/@hs/room or package source) expose an optional
is_directorisDirecton membership event content. If absent, guard the property access or update the types/package.
|
|
||
| // we only understand these two types of rooms, plus direct messages | ||
| const isDM = await isDirectMessage(matrixRoom, inviteEvent); | ||
| const isDM = inviteEvent.getContent<PduMembershipEventContent>().is_direct; |
There was a problem hiding this comment.
Don’t rely solely on is_direct; normalize and handle absence.
Some homeservers omit is_direct on invites or use isDirect. Treat it as optional and normalize; otherwise non‑DM rooms may be misclassified, hitting the “neither public, private, nor direct” error path.
Apply:
- const isDM = inviteEvent.getContent<PduMembershipEventContent>().is_direct;
+ const content = inviteEvent.getContent<PduMembershipEventContent & { is_direct?: boolean; isDirect?: boolean }>();
+ const isDM = (content.is_direct ?? content.isDirect) === true;Optionally keep a lightweight heuristic fallback if the flag is absent (e.g., fall back to previous helper or room state) in a follow-up.
📝 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.
| const isDM = inviteEvent.getContent<PduMembershipEventContent>().is_direct; | |
| const content = inviteEvent.getContent<PduMembershipEventContent & { is_direct?: boolean; isDirect?: boolean }>(); | |
| const isDM = (content.is_direct ?? content.isDirect) === true; |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/invite.ts around line 172, the
code currently reads the invite's is_direct directly and may misclassify rooms
when homeservers omit or rename that field; update the logic to treat the field
as optional and normalize both possible names (is_direct and isDirect), coerce
to a boolean (default to false when absent), and ensure downstream branching
uses this normalized value; optionally add a light heuristic fallback (e.g.,
call the existing helper or inspect room state) when no explicit flag is
present, but keep the immediate fix limited to normalization and safe defaulting
so invites aren't incorrectly classified.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/federation #37012 +/- ##
===================================================
- Coverage 69.09% 69.09% -0.01%
===================================================
Files 2959 2959
Lines 102190 102190
Branches 18259 18259
===================================================
- Hits 70605 70604 -1
- Misses 29716 29720 +4
+ Partials 1869 1866 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
fix-dm-check
branch
from
a1869e8 to
1d56b34
Compare
ggazzo
changed the base branch from
feat/federation
to
chore/federation-backup
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit