Skip to content

Conversation

@sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented Nov 24, 2025

caused by #301

Summary by CodeRabbit

  • Chores
    • Improved API ergonomics by adding default parameter values to internal service methods, reducing boilerplate in common use cases.

✏️ Tip: You can customize this high-level summary in your review settings.

@sampaiodiego sampaiodiego changed the title fix: getMissingEvents being called without default values regression: fix getMissingEvents being called without default values Nov 24, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 24, 2025

Walkthrough

The getMissingEvents method in EventService now includes default parameter values: limit defaults to 10 and minDepth defaults to 0. This allows callers to omit these parameters while maintaining backward compatibility with existing code that provides explicit values.

Changes

Cohort / File(s) Summary
EventService method signature update
packages/federation-sdk/src/services/event.service.ts
Added default parameter values to getMissingEvents: limit = 10 and minDepth = 0

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Verify default values align with intended behavior and existing call patterns
  • Confirm no unexpected side effects from parameter defaults in callers

Possibly related PRs

Suggested reviewers

  • ggazzo

Poem

🐰 A rabbit hops through parameter land,
Where defaults now make calls quite grand,
Ten events deep, from zero we start,
Simpler code, a cleaner part! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding default values to the getMissingEvents method parameters to fix a regression where the method was being called without providing all required parameters.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-get-missing-default

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between c3605da and 1b62d14.

📒 Files selected for processing (1)
  • packages/federation-sdk/src/services/event.service.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.

Applied to files:

  • packages/federation-sdk/src/services/event.service.ts
🔇 Additional comments (1)
packages/federation-sdk/src/services/event.service.ts (1)

489-495: LGTM! Defaults are appropriate and verified backward compatible.

The verification confirms this change is safe:

  • profiles.controller.ts (line 128-134): Already explicitly passes limit and minDepth with fallback defaults (body.limit || 10, body.min_depth || 0), so it remains compatible
  • staging-area.service.ts (line 205-212): Calls federation.service.getMissingEvents with all 6 arguments explicitly, unaffected
  • federation.service.ts (line 160-167): Already has matching defaults in place

All callers continue to work unchanged, and the new defaults in event.service.ts align with what the calling code was already providing.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.79%. Comparing base (c3605da) to head (1b62d14).

Files with missing lines Patch % Lines
...kages/federation-sdk/src/services/event.service.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #307   +/-   ##
=======================================
  Coverage   52.79%   52.79%           
=======================================
  Files          96       96           
  Lines       12589    12589           
=======================================
  Hits         6646     6646           
  Misses       5943     5943           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sampaiodiego sampaiodiego merged commit 06f4fd1 into main Nov 25, 2025
3 checks passed
@sampaiodiego sampaiodiego deleted the fix-get-missing-default branch November 25, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants