chore: migrate presence endpoints to new API pattern#38882
chore: migrate presence endpoints to new API pattern#38882ggazzo merged 10 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 6efacf2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors two presence REST endpoints from legacy Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/presence.ts (1)
47-47:ajv.compile<void>type parameter doesn't match the schema shape.The schema describes
{ success: true }(an object with a required success property), but the generic isvoid. Whileajv.compile<void>is used throughout the codebase for simple success responses, the same file uses specific type parameters elsewhere (e.g.,ajv.compile<{ current: number; max: number }>). For consistency and type accuracy, consider using{ success: true }so the compiled validator's type aligns with what it validates.♻️ Suggested type alignment
- 200: ajv.compile<void>({ + 200: ajv.compile<{ success: true }>({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/presence.ts` at line 47, The ajv.compile<void> call at the 200 response schema is typed incorrectly for the schema (it validates an object { success: true }); update the generic from void to the correct shape (e.g., { success: true }) so the compiled validator's TypeScript type matches the schema, and scan other ajv.compile usages in this file to align similar simple-success responses to the same type for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-presence-api-chained-pattern.mdapps/meteor/app/api/server/v1/presence.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
apps/meteor/app/api/server/v1/presence.ts
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.changeset/refactor-presence-api-chained-pattern.md (1)
1-5: LGTM!Changeset accurately describes the migration and the
patchbump is appropriate for a behavior-preserving refactor.apps/meteor/app/api/server/v1/presence.ts (3)
2-6: LGTM — imports are clean and appropriate for the new pattern.
40-67:presence.enableBroadcastendpoint looks correct — behavior preserved.
authRequired,permissionsRequired, andtwoFactorRequiredare all carried over properly. The action handler delegates toPresence.toggleBroadcast(true)and returns a bare success response, consistent with the legacy behavior.
10-38: No action needed. ThePresence.getConnectionCount()method returns exactly{ current: number; max: number }, andAPI.v1.success()correctly adds thesuccess: truefield to produce the response{ current, max, success }. This matches the schema precisely and will validate without issues againstadditionalProperties: false.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/api/server/v1/presence.ts`:
- Line 47: The ajv.compile<void> call at the 200 response schema is typed
incorrectly for the schema (it validates an object { success: true }); update
the generic from void to the correct shape (e.g., { success: true }) so the
compiled validator's TypeScript type matches the schema, and scan other
ajv.compile usages in this file to align similar simple-success responses to the
same type for consistency.
smirk-dev
force-pushed
the
refactor/migrate-presence-api-chained-pattern
branch
from
9a3791a to
3cabb39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/presence.ts (1)
12-24:ajv.compile<T>generic is missingsuccessfrom the type parameter.The schema marks
successasrequired, so the fully correct TypeScript type should include it alongsidecurrentandmax.♻️ Proposed fix
- 200: ajv.compile<{ current: number; max: number }>({ + 200: ajv.compile<{ current: number; max: number; success: boolean }>({As per coding guidelines, TypeScript should use "accurate typing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/presence.ts` around lines 12 - 24, The AJV schema marks "success" as required but the generic passed to ajv.compile<{ current: number; max: number }> omits it; update the type parameter on ajv.compile to include success (e.g., { current: number; max: number; success: boolean }) so the TypeScript type matches the schema and required fields for the compiled validator.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-presence-api-chained-pattern.mdapps/meteor/app/api/server/v1/presence.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
apps/meteor/app/api/server/v1/presence.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/presence.ts (1)
apps/meteor/app/api/server/index.ts (1)
API(51-51)
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
.changeset/refactor-presence-api-chained-pattern.md (1)
1-5: LGTM!The changeset correctly uses a
patchbump and the description accurately reflects the migration scope.apps/meteor/app/api/server/v1/presence.ts (2)
2-2: All three imports are correctly exported from@rocket.chat/rest-typings. Theajvinstance is intentionally re-exported as a shared, pre-configured instance created inpackages/rest-typings/src/v1/Ajv.ts. This is a deliberate architectural pattern—a single AJV instance is created once with custom configuration (formats, keywords) and reused throughout the package, rather than creating new instances in each validator. No action required.
29-33: No action required—the schema validation will pass correctly.
Presence.getConnectionCount()returns{ current: number; max: number }, andAPI.v1.success(result)automatically addssuccess: trueto the result object before returning. The final response body will be{ current: number; max: number; success: true }, which matches the schema exactly with no additional properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/presence.ts`:
- Around line 43-53: The ajv.compile<void> type parameter is incorrect for the
schema that validates an object with a required success: true property; update
the call site (ajv.compile<void>) to use an accurate type that matches the
schema (e.g., ajv.compile<{ success: true }>) so the compiled ValidateFunction
is a correct type guard for the validated shape and replace any related
references if you introduce a named type alias (e.g., SuccessResponse) to keep
types clear.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/presence.ts`:
- Around line 12-24: The AJV schema marks "success" as required but the generic
passed to ajv.compile<{ current: number; max: number }> omits it; update the
type parameter on ajv.compile to include success (e.g., { current: number; max:
number; success: boolean }) so the TypeScript type matches the schema and
required fields for the compiled validator.
Migrates presence.getConnections (GET) and presence.enableBroadcast (POST) from the legacy addRoute() pattern to the new chained .get()/.post() API pattern with typed AJV response schemas for 200, 401, and 403 responses. Part of RocketChat#38876
smirk-dev
force-pushed
the
refactor/migrate-presence-api-chained-pattern
branch
from
3cabb39 to
12db41e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-presence-api-chained-pattern.mdapps/meteor/app/api/server/v1/presence.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
apps/meteor/app/api/server/v1/presence.ts
🧠 Learnings (1)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/presence.ts
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.changeset/refactor-presence-api-chained-pattern.md (1)
1-5: LGTM!Changeset is accurate —
patchis the right bump for a behavior-preserving refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/presence.ts`:
- Around line 12-24: The ajv.compile generic is missing the required success
property and misrepresents the validator's type guard; update the ajv.compile
type parameter used in this file (the ValidateFunction created by ajv.compile)
to include the success field (matching the schema's enum [true], e.g., success:
true) so the TypeScript type accurately reflects the runtime schema for the
validator.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/presence.ts`:
- Around line 43-53: The AJV compile call is incorrectly typed as
ajv.compile<void>, misrepresenting the validated result; change the generic on
ajv.compile to match the schema shape (e.g., ajv.compile<{ success: true }>) so
the ValidateFunction reflects that the validator returns data is { success: true
} rather than void; update the ajv.compile<void> invocation to ajv.compile<{
success: true }>(...) in the presence response schema.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38882 +/- ##
===========================================
- Coverage 70.65% 70.62% -0.03%
===========================================
Files 3190 3190
Lines 112732 112732
Branches 20431 20426 -5
===========================================
- Hits 79649 79621 -28
- Misses 31035 31064 +29
+ Partials 2048 2047 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The CodeBlock and KatexBlock components are lazy-loaded via React.lazy(). In CI environments under resource contention, the default 1000ms waitFor timeout can be insufficient for Suspense to resolve the dynamic imports, causing intermittent test failures. Increase the timeout to 5000ms to make these tests more resilient in slow CI environments.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gazzodown/src/Markup.spec.tsx (1)
233-233: Consider extracting the shared timeout to a named constant.All three
waitForsites hardcode5000. Defining a single constant (e.g.,SUSPENSE_TIMEOUT) would make the intent explicit and keep future adjustments to one place.♻️ Proposed refactor
+const SUSPENSE_TIMEOUT = 5000; ... - await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: 5000 }); + await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: SUSPENSE_TIMEOUT }); ... - await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: 5000 }); + await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: SUSPENSE_TIMEOUT }); ... - await waitFor(() => expect(container.querySelector('math')).toBeTruthy(), { timeout: 5000 }); + await waitFor(() => expect(container.querySelector('math')).toBeTruthy(), { timeout: SUSPENSE_TIMEOUT });Also applies to: 253-253, 274-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gazzodown/src/Markup.spec.tsx` at line 233, Extract the repeated hardcoded timeout value (5000) used in the three waitFor calls into a named constant (e.g., SUSPENSE_TIMEOUT) at the top of the Markup.spec.tsx test file and replace each waitFor(..., { timeout: 5000 }) invocation with waitFor(..., { timeout: SUSPENSE_TIMEOUT }); update the three occurrences that call waitFor (the ones currently using 5000) so the single constant controls the timeout in all tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/gazzodown/src/Markup.spec.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
packages/gazzodown/src/Markup.spec.tsx
🧠 Learnings (11)
📚 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 `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 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:
packages/gazzodown/src/Markup.spec.tsx
📚 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:
packages/gazzodown/src/Markup.spec.tsx
📚 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:
packages/gazzodown/src/Markup.spec.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 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 descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 Learning: 2026-02-04T12:09:05.769Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/gazzodown/src/Markup.spec.tsx`:
- Line 233: Extract the repeated hardcoded timeout value (5000) used in the
three waitFor calls into a named constant (e.g., SUSPENSE_TIMEOUT) at the top of
the Markup.spec.tsx test file and replace each waitFor(..., { timeout: 5000 })
invocation with waitFor(..., { timeout: SUSPENSE_TIMEOUT }); update the three
occurrences that call waitFor (the ones currently using 5000) so the single
constant controls the timeout in all tests.
| ); | ||
|
|
||
| await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument()); | ||
| await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: 5000 }); |
There was a problem hiding this comment.
why did you add a timeout here?
There was a problem hiding this comment.
These tests render components inside (code blocks and KaTeX), which means the actual component is lazy-loaded asynchronously. The default waitFor timeout is 1000ms, which can be too tight on slower CI runners causing flaky failures where the lazy-loaded chunk hasn't resolved yet. Bumping to 5000ms gives enough headroom for the dynamic import to complete without making the tests meaningfully slower since waitFor polls and resolves as soon as the assertion passes, it only waits the full 5s on actual failure. This was specifically triggered by intermittent CI failures on these three tests.
- Fix ajv.compile<void> to use { success: true } for enableBroadcast
- Add success: true to getConnections TypeScript generic type
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
ggazzo
changed the title
ggazzo
added
the
stat: QA assured
ggazzo
changed the title
github-project-automation
bot
moved this from Backlog
to Done
in rest api conversion to new endpoints
4 participants
Summary
Migrates the
presence.getConnections(GET) andpresence.enableBroadcast(POST) endpoints from the legacyAPI.v1.addRoute()pattern to the new chained.get()/.post()API pattern.Changes
addRoutecalls withAPI.v1.get()andAPI.v1.post()chained patternpresence.getConnections: response schema validatescurrentandmaxnumber fieldspresence.enableBroadcast: response schema validates success-only responsemanage-user-status),twoFactorRequiredon broadcastMotivation
Part of the ongoing effort to migrate REST API endpoints to the new chained API pattern that enables typed response validation and future OpenAPI spec generation.
Closes part of #38876
COMM-144
Summary by CodeRabbit
Refactor
Tests