-
Notifications
You must be signed in to change notification settings - Fork 19
fix: edu events being emitted even when not configured to #310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe pull request adds config-driven guards to the event service's typing and presence processing methods. Both Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
- Coverage 53.15% 52.84% -0.31%
==========================================
Files 96 96
Lines 12573 12583 +10
==========================================
- Hits 6683 6650 -33
- Misses 5890 5933 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/event.service.ts (1)
321-324: LGTM! Consider defensive coding and observability improvements.The config-driven guard correctly prevents typing event emission when disabled. The early return is safe since no processing has occurred yet.
Consider these optional improvements:
- Defensive config access to prevent runtime errors if the config structure is unexpected:
const config = this.configService.getConfig('edu'); -if (!config.processTyping) { +if (!config?.processTyping) { return; }
- Add debug logging for observability when skipping processing:
const config = this.configService.getConfig('edu'); if (!config?.processTyping) { + this.logger.debug('Skipping typing EDU processing (disabled in config)'); return; }
📜 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)
packages/federation-sdk/src/services/event.service.ts(2 hunks)
🔇 Additional comments (1)
packages/federation-sdk/src/services/event.service.ts (1)
351-354: Rewritten review comment: The config-driven guard correctly prevents presence event emission when disabled. The early return safely prevents all processing.The EDU config structure in
packages/federation-sdk/src/services/config.service.tsdefinesprocessTypingandprocessPresenceas required boolean properties (lines 34-37, validated by Zod schema at 75-78). SincegetConfig('edu')returnsAppConfig['edu']with these properties guaranteed to exist, optional chaining (?.) is unnecessary and the current direct property access is correct.The pattern matches
processTypingEDU(line 321), which also uses direct access without optional chaining.Optional improvement: Consider adding debug logging for observability:
const config = this.configService.getConfig('edu'); if (!config.processPresence) { + this.logger.debug('Skipping presence EDU processing (disabled in config)'); return; }
It doesn't make sense that
homeserveris emitting the eventshomeserver.matrix.presenceandhomeserver.matrix.typingif the configuration says to not process them.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.