-
Notifications
You must be signed in to change notification settings - Fork 13k
chore(federation-matrix): Review build configuration #37858
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
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis PR removes Babel build configuration from the federation-matrix package, restructures TypeScript configuration into separate build and source configs, updates build and lint scripts to reflect new paths, removes Babel-related devDependencies, adjusts Jest config formatting, and updates test assertions to use non-null assertions instead of optional chaining. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
546b178 to
38a0c23
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37858 +/- ##
========================================
Coverage 67.62% 67.63%
========================================
Files 3463 3463
Lines 113693 113693
Branches 20902 20902
========================================
+ Hits 76885 76895 +10
+ Misses 34675 34666 -9
+ Partials 2133 2132 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 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: 1
📜 Review details
Configuration used: Organization 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
ee/packages/federation-matrix/babel.config.js(0 hunks)ee/packages/federation-matrix/jest.config.ts(1 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/tests/end-to-end/room.spec.ts(3 hunks)ee/packages/federation-matrix/tsconfig.build.json(2 hunks)ee/packages/federation-matrix/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- ee/packages/federation-matrix/babel.config.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/jest.config.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/jest.config.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
ee/packages/federation-matrix/jest.config.tsee/packages/federation-matrix/tsconfig.build.jsonee/packages/federation-matrix/tsconfig.jsonee/packages/federation-matrix/package.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/federation-matrix/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/federation-matrix/jest.config.tsee/packages/federation-matrix/tsconfig.build.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/federation-matrix/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/federation-matrix/jest.config.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/tsconfig.build.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
ee/packages/federation-matrix/tsconfig.build.jsonee/packages/federation-matrix/package.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
Applied to files:
ee/packages/federation-matrix/tsconfig.build.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
ee/packages/federation-matrix/tsconfig.build.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
ee/packages/federation-matrix/tsconfig.build.json
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/package.json
🔇 Additional comments (9)
ee/packages/federation-matrix/jest.config.ts (1)
6-6: LGTM! Formatting simplified.The transformIgnorePatterns array has been reformatted to a single line without any functional changes. The ignored patterns remain the same.
ee/packages/federation-matrix/tsconfig.json (1)
4-4: LGTM! Clean separation of source and build configurations.Setting
rootDirto"./src"in the base config and moving declaration generation totsconfig.build.jsonprovides a cleaner separation between source configuration and build output settings.ee/packages/federation-matrix/tsconfig.build.json (1)
4-6: LGTM! Build configuration properly structured.The build-specific TypeScript configuration correctly:
- Sets output directory to
"./dist"matching package.json paths- Enables declaration file generation
- Extends the base tsconfig.json for shared compiler options
This separation ensures clean build artifacts while maintaining consistent base settings.
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (3)
1608-1608: LGTM! Clearer assertion pattern.Replacing
pendingInvitation?.rid!withpendingInvitation!.rid!is appropriate here since Line 1605 already asserts thatpendingInvitationis not undefined. This makes the code's expectations more explicit and consistent with the preceding test assertion.
1647-1647: LGTM! Consistent with preceding assertion.Same pattern as Line 1608 - the non-null assertion on
pendingInvitationaligns with the assertion at Line 1644 that confirms it's not undefined.
1681-1681: LGTM! Consistent assertion pattern.Same pattern as previous occurrences - the non-null assertion on
pendingInvitationis consistent with the assertion at Line 1678.ee/packages/federation-matrix/package.json (3)
12-12: LGTM! Consistent with build configuration changes.Updating the dev script to use
tsconfig.build.jsonaligns with the separation of build and source configurations introduced in this PR.
13-14: LGTM! Broader linting scope.Expanding the lint scripts to cover the entire project directory instead of just
srcensures consistent code quality across all TypeScript files, including tests and configuration.
40-52: LGTM! Babel dependencies removed as intended.The removal of all Babel-related devDependencies (
@babel/cli,@babel/core,@babel/preset-env,@babel/preset-typescript, andbabel-jest) aligns with the PR objective to rely solely on TypeScript for transpilation. This simplifies the build toolchain and reduces maintenance overhead.
| "test:federation": "jest --config jest.config.federation.ts", | ||
| "testunit": "jest", | ||
| "typecheck": "tsc --noEmit --skipLibCheck" | ||
| "typecheck": "tsc --noEmit" |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the package.json file to see current state
cat ee/packages/federation-matrix/package.json | head -30
# Also check if there's a tsconfig.json in that directory
echo "---"
echo "Looking for tsconfig.json:"
ls -la ee/packages/federation-matrix/ | grep -i tsconfigRepository: RocketChat/Rocket.Chat
Length of output: 1212
🏁 Script executed:
#!/bin/bash
# Search for skipLibCheck in the repository
rg "skipLibCheck" --type jsonRepository: RocketChat/Rocket.Chat
Length of output: 1773
🏁 Script executed:
#!/bin/bash
# Navigate to the federation-matrix package and run typecheck
cd ee/packages/federation-matrix
# Time the typecheck
echo "Running typecheck..."
timeout 30 time npm run typecheck 2>&1 || {
EXIT_CODE=$?
if [ $EXIT_CODE -eq 124 ]; then
echo "Typecheck timed out after 30 seconds"
else
echo "Typecheck exited with code: $EXIT_CODE"
fi
}Repository: RocketChat/Rocket.Chat
Length of output: 164
Add --skipLibCheck to match repository pattern.
This package's typecheck script lacks --skipLibCheck, which is used across the repository (presence, network-broker, stream-hub-service, queue-worker, meteor, and others). Skipping type checking of third-party declaration files reduces compile time. Update the script to "typecheck": "tsc --noEmit --skipLibCheck" for consistency and improved performance.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/package.json around line 19, the "typecheck"
script is missing the repository-standard --skipLibCheck flag; update the script
to include --skipLibCheck so it becomes tsc --noEmit --skipLibCheck to skip
third-party declaration checks and match other packages.
Proposed changes (including videos or screenshots)
This pull request cleans up and simplifies the build and configuration setup for the
federation-matrixpackage, focusing on removing unnecessary Babel dependencies, improving TypeScript and ESLint configurations, and making small code style adjustments in tests. The main goals are to streamline the development workflow and reduce maintenance overhead.Build and configuration cleanup:
babel.config.jsand all related Babel dependencies frompackage.json, as the project now relies solely on TypeScript for transpilation. [1] [2]tsconfig.build.jsonandtsconfig.jsonto better separate build output and declarations, and to clarify root directories and output paths. [1] [2]Development workflow improvements:
package.jsonscripts to use the build-specific TypeScript config for thedevscript, and broadened ESLint commands to lint the entire project rather than just thesrcdirectory. Also removedskipLibCheckfrom thetypecheckscript for stricter type checking.Testing and code style:
transformIgnorePatternsinjest.config.tsfor readability.room.spec.tstests by removing unnecessary optional chaining before non-null assertions. [1] [2] [3]Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.