feat(apps): introduce read accessor for media calls#40863
Draft
d-gubert wants to merge 3 commits into
Draft
Conversation
Contributor
|
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 |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
d-gubert
force-pushed
the
feat/apps-media-calls
branch
from
2bc6cd3 to
4126852
Compare
🦋 Changeset detectedLatest commit: 0fc81ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
d-gubert
force-pushed
the
feat/sip-metadata
branch
from
d2a38c3 to
bbd7101
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/sip-metadata #40863 +/- ##
=====================================================
- Coverage 70.13% 69.57% -0.56%
=====================================================
Files 3341 3326 -15
Lines 123558 122681 -877
Branches 22588 21969 -619
=====================================================
- Hits 86654 85355 -1299
- Misses 33565 33978 +413
- Partials 3339 3348 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
fix(apps): align type definitions
d-gubert
force-pushed
the
feat/apps-media-calls
branch
from
f85555a to
0fc81ce
Compare
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/apps/tests/test-data/bridges/mediaCallBridge.ts (1)
6-8: ⚡ Quick winConsider returning a mock instead of throwing.
The stub currently throws, which will break any test that uses this bridge through
TestsAppBridges. For better test resilience and to match patterns used in other test bridges in this directory, consider returning a mock call fromTestData.getMediaCall().♻️ Proposed implementation returning a mock
+import { TestData } from '../utilities'; + import type { IMediaCall } from '`@rocket.chat/apps-engine/definition/mediaCalls`'; import { MediaCallBridge } from '../../../src/server/bridges'; export class TestsMediaCallBridge extends MediaCallBridge { public getById(callId: string, appId: string): Promise<IMediaCall> { - throw new Error('Method not implemented.'); + return Promise.resolve(TestData.getMediaCall()); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/tests/test-data/bridges/mediaCallBridge.ts` around lines 6 - 8, The getById method currently throws and breaks tests; change it to return a mock media call by calling TestData.getMediaCall(callId, appId) (or TestData.getMediaCall() if signature differs) and return it as a resolved Promise instead of throwing; update the public getById(callId: string, appId: string): Promise<IMediaCall> implementation to return that mock so TestsAppBridges consumers get a valid IMediaCall object.apps/meteor/tests/end-to-end/apps/app-media-call-reader.ts (1)
12-14: 💤 Low valueVerify test fixture data availability.
The test assumes these call IDs are seeded by
callHistoryTestData.ts. If the seed data is missing or the IDs change, the tests will fail without clear diagnostics. Consider adding an initial query to verify the fixtures exist, or document the dependency clearly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/end-to-end/apps/app-media-call-reader.ts` around lines 12 - 14, Tests rely on the fixture IDs INTERNAL_CALL_ID and EXTERNAL_CALL_ID which are expected to be seeded by callHistoryTestData.ts; add an initial verification step at the start of the test (e.g., a query or assertion using the same lookup used in the test harness) that checks those call records exist and fail fast with a clear error message if they are missing, or alternatively document the dependency on callHistoryTestData.ts in the test setup comments so maintainers know to ensure the seed data is present and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/tired-teeth-cheat.md:
- Line 7: Update the changelog entry text to use the plural "media calls" for
accuracy: locate the sentence mentioning "Added the new MediaCallRead accessor
that allows apps to read information from the system's media call" and change it
to "Added the new MediaCallRead accessor that allows apps to read information
from the system's media calls" so it reflects that the accessor reads from the
collection of media call records.
In `@packages/apps-engine/src/definition/accessors/IMediaCallRead.ts`:
- Line 14: The return nullability is inconsistent: update the
IMediaCallRead.getById signature to return Promise<IMediaCall | null> (instead
of Promise<IMediaCall | undefined>) so it matches the MediaCallBridge.ts
implementation and the value forwarded by MediaCallRead.ts; then update any
implementations/usages of IMediaCallRead.getById to handle null (or adjust
MediaCallRead.ts to explicitly return null) to ensure consistent null semantics
across IMediaCallRead.getById, MediaCallBridge, and MediaCallRead.
In `@packages/apps/src/server/bridges/MediaCallBridge.ts`:
- Around line 9-17: The bridge currently returns null from doGetById which
conflicts with the accessor contract that expects undefined; update
MediaCallBridge by changing doGetById's and its abstract getById signature to
return Promise<IMediaCall | undefined> instead of Promise<IMediaCall | null>,
and replace the literal return null with return undefined so the sentinel value
matches packages/apps/src/server/accessors/MediaCallRead.ts and callers using
=== undefined remain correct.
---
Nitpick comments:
In `@apps/meteor/tests/end-to-end/apps/app-media-call-reader.ts`:
- Around line 12-14: Tests rely on the fixture IDs INTERNAL_CALL_ID and
EXTERNAL_CALL_ID which are expected to be seeded by callHistoryTestData.ts; add
an initial verification step at the start of the test (e.g., a query or
assertion using the same lookup used in the test harness) that checks those call
records exist and fail fast with a clear error message if they are missing, or
alternatively document the dependency on callHistoryTestData.ts in the test
setup comments so maintainers know to ensure the seed data is present and
consistent.
In `@packages/apps/tests/test-data/bridges/mediaCallBridge.ts`:
- Around line 6-8: The getById method currently throws and breaks tests; change
it to return a mock media call by calling TestData.getMediaCall(callId, appId)
(or TestData.getMediaCall() if signature differs) and return it as a resolved
Promise instead of throwing; update the public getById(callId: string, appId:
string): Promise<IMediaCall> implementation to return that mock so
TestsAppBridges consumers get a valid IMediaCall object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f838a7f-ca6a-412b-886f-0a2109886b30
⛔ Files ignored due to path filters (1)
apps/meteor/tests/data/apps/app-packages/media-call-reader-test_0.0.1.zipis excluded by!**/*.zip
📒 Files selected for processing (27)
.changeset/tired-teeth-cheat.mdapps/meteor/app/apps/server/bridges/bridges.jsapps/meteor/app/apps/server/bridges/mediaCalls.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tsapps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/app-media-call-reader.tspackages/apps-engine/src/definition/accessors/IMediaCallRead.tspackages/apps-engine/src/definition/accessors/IRead.tspackages/apps-engine/src/definition/accessors/index.tspackages/apps-engine/src/definition/mediaCalls/IMediaCall.tspackages/apps-engine/src/definition/mediaCalls/index.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/src/AppsEngine.tspackages/apps/src/server/accessors/MediaCallRead.tspackages/apps/src/server/accessors/Reader.tspackages/apps/src/server/accessors/index.tspackages/apps/src/server/bridges/AppBridges.tspackages/apps/src/server/bridges/MediaCallBridge.tspackages/apps/src/server/bridges/index.tspackages/apps/src/server/managers/AppAccessorManager.tspackages/apps/src/server/permissions/AppPermissions.tspackages/apps/tests/server/accessors/MediaCallRead.test.tspackages/apps/tests/server/accessors/Reader.test.tspackages/apps/tests/test-data/bridges/appBridges.tspackages/apps/tests/test-data/bridges/mediaCallBridge.tspackages/apps/tests/test-data/utilities.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
🧰 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/apps/src/server/accessors/index.tspackages/apps-engine/src/definition/accessors/index.tspackages/apps/tests/server/accessors/MediaCallRead.test.tspackages/apps-engine/src/definition/accessors/IMediaCallRead.tsapps/meteor/app/apps/server/bridges/mediaCalls.tsapps/meteor/tests/data/apps/app-packages/index.tspackages/apps/src/server/accessors/MediaCallRead.tspackages/apps/src/server/bridges/index.tspackages/apps/src/server/permissions/AppPermissions.tspackages/apps-engine/src/definition/accessors/IRead.tspackages/apps-engine/src/definition/mediaCalls/index.tspackages/apps/tests/server/accessors/Reader.test.tspackages/apps/tests/test-data/utilities.tspackages/apps/src/server/bridges/AppBridges.tspackages/apps/tests/test-data/bridges/mediaCallBridge.tspackages/apps/tests/test-data/bridges/appBridges.tspackages/apps-engine/src/definition/mediaCalls/IMediaCall.tspackages/apps/src/server/bridges/MediaCallBridge.tsapps/meteor/app/apps/server/bridges/bridges.jspackages/apps/src/server/managers/AppAccessorManager.tsapps/meteor/tests/data/apps/helper.tspackages/apps/src/server/accessors/Reader.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/src/AppsEngine.tsapps/meteor/tests/end-to-end/apps/app-media-call-reader.ts
🧠 Learnings (8)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/src/server/accessors/index.tspackages/apps-engine/src/definition/accessors/index.tspackages/apps/tests/server/accessors/MediaCallRead.test.tspackages/apps-engine/src/definition/accessors/IMediaCallRead.tsapps/meteor/app/apps/server/bridges/mediaCalls.tsapps/meteor/tests/data/apps/app-packages/index.tspackages/apps/src/server/accessors/MediaCallRead.tspackages/apps/src/server/bridges/index.tspackages/apps/src/server/permissions/AppPermissions.tspackages/apps-engine/src/definition/accessors/IRead.tspackages/apps-engine/src/definition/mediaCalls/index.tspackages/apps/tests/server/accessors/Reader.test.tspackages/apps/tests/test-data/utilities.tspackages/apps/src/server/bridges/AppBridges.tspackages/apps/tests/test-data/bridges/mediaCallBridge.tspackages/apps/tests/test-data/bridges/appBridges.tspackages/apps-engine/src/definition/mediaCalls/IMediaCall.tspackages/apps/src/server/bridges/MediaCallBridge.tspackages/apps/src/server/managers/AppAccessorManager.tsapps/meteor/tests/data/apps/helper.tspackages/apps/src/server/accessors/Reader.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/src/AppsEngine.tsapps/meteor/tests/end-to-end/apps/app-media-call-reader.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/src/server/accessors/index.tspackages/apps-engine/src/definition/accessors/index.tspackages/apps/tests/server/accessors/MediaCallRead.test.tspackages/apps-engine/src/definition/accessors/IMediaCallRead.tsapps/meteor/app/apps/server/bridges/mediaCalls.tsapps/meteor/tests/data/apps/app-packages/index.tspackages/apps/src/server/accessors/MediaCallRead.tspackages/apps/src/server/bridges/index.tspackages/apps/src/server/permissions/AppPermissions.tspackages/apps-engine/src/definition/accessors/IRead.tspackages/apps-engine/src/definition/mediaCalls/index.tspackages/apps/tests/server/accessors/Reader.test.tspackages/apps/tests/test-data/utilities.tspackages/apps/src/server/bridges/AppBridges.tspackages/apps/tests/test-data/bridges/mediaCallBridge.tspackages/apps/tests/test-data/bridges/appBridges.tspackages/apps-engine/src/definition/mediaCalls/IMediaCall.tspackages/apps/src/server/bridges/MediaCallBridge.tspackages/apps/src/server/managers/AppAccessorManager.tsapps/meteor/tests/data/apps/helper.tspackages/apps/src/server/accessors/Reader.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/src/AppsEngine.tsapps/meteor/tests/end-to-end/apps/app-media-call-reader.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/src/server/accessors/index.tspackages/apps-engine/src/definition/accessors/index.tspackages/apps/tests/server/accessors/MediaCallRead.test.tspackages/apps-engine/src/definition/accessors/IMediaCallRead.tsapps/meteor/app/apps/server/bridges/mediaCalls.tsapps/meteor/tests/data/apps/app-packages/index.tspackages/apps/src/server/accessors/MediaCallRead.tspackages/apps/src/server/bridges/index.tspackages/apps/src/server/permissions/AppPermissions.tspackages/apps-engine/src/definition/accessors/IRead.tspackages/apps-engine/src/definition/mediaCalls/index.tspackages/apps/tests/server/accessors/Reader.test.tspackages/apps/tests/test-data/utilities.tspackages/apps/src/server/bridges/AppBridges.tspackages/apps/tests/test-data/bridges/mediaCallBridge.tspackages/apps/tests/test-data/bridges/appBridges.tspackages/apps-engine/src/definition/mediaCalls/IMediaCall.tspackages/apps/src/server/bridges/MediaCallBridge.tspackages/apps/src/server/managers/AppAccessorManager.tsapps/meteor/tests/data/apps/helper.tspackages/apps/src/server/accessors/Reader.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/src/AppsEngine.tsapps/meteor/tests/end-to-end/apps/app-media-call-reader.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.
Applied to files:
packages/apps/src/server/accessors/index.tspackages/apps/src/server/accessors/MediaCallRead.tspackages/apps/src/server/bridges/index.tspackages/apps/src/server/permissions/AppPermissions.tspackages/apps/src/server/bridges/AppBridges.tspackages/apps/src/server/bridges/MediaCallBridge.tspackages/apps/src/server/managers/AppAccessorManager.tspackages/apps/src/server/accessors/Reader.tspackages/apps/src/AppsEngine.ts
📚 Learning: 2026-05-06T20:47:53.078Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40186
File: apps/meteor/app/apps/server/bridges/uiInteraction.ts:2-2
Timestamp: 2026-05-06T20:47:53.078Z
Learning: Deep imports must be used in this repository because Meteor’s bundler does not respect package.json exports subpath mappings. Import using deep paths (e.g., rocket.chat/apps/dist/server/bridges/UiInteractionBridge) instead of relying on exports. Do not suggest or apply changes to exports maps in Meteor-consuming packages (e.g., packages/apps/package.json) as a fix for deep imports. This guideline applies to all TypeScript files under apps/meteor/app/apps/server/bridges.
Applied to files:
apps/meteor/app/apps/server/bridges/mediaCalls.ts
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/tired-teeth-cheat.md
📚 Learning: 2026-02-04T12:08:56.950Z
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:08:56.950Z
Learning: In the E2E tests under apps/meteor/tests/end-to-end/apps, prefer using a fixed sleep (e.g., 1s) instead of implementing polling or 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:
apps/meteor/tests/end-to-end/apps/app-media-call-reader.ts
📚 Learning: 2026-03-16T11:50:11.087Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:11.087Z
Learning: In end-to-end tests under apps/meteor/tests/end-to-end/apps, there is an established pattern: call createAgent() and makeAgentAvailable() immediately after updateSetting('Livechat_enabled', true) with no intermediate delay. Do not flag this sequence as a race condition or require a sleep/delay. This pattern is used across 10+ tests in the codebase.
Applied to files:
apps/meteor/tests/end-to-end/apps/app-media-call-reader.ts
🪛 Biome (2.4.16)
apps/meteor/app/apps/server/bridges/bridges.js
[error] 17-17: Illegal use of an import declaration outside of a module
(parse)
🔇 Additional comments (27)
apps/meteor/app/apps/server/bridges/bridges.js (1)
17-17: LGTM!Also applies to: 65-65, 180-182
apps/meteor/app/apps/server/bridges/mediaCalls.ts (1)
1-16: LGTM!packages/apps/src/server/accessors/MediaCallRead.ts (1)
1-15: LGTM!packages/apps/src/server/accessors/Reader.ts (1)
6-6: LGTM!Also applies to: 38-38, 101-103
packages/apps/src/server/accessors/index.ts (1)
12-12: LGTM!Also applies to: 62-62
packages/apps/src/server/managers/AppAccessorManager.ts (1)
25-25: LGTM!Also applies to: 196-197, 216-216
packages/apps/deno-runtime/lib/accessors/mod.ts (1)
257-257: LGTM!packages/apps/src/AppsEngine.ts (1)
11-11: LGTM!packages/apps-engine/src/definition/mediaCalls/IMediaCall.ts (1)
1-41: LGTM!packages/apps-engine/src/definition/mediaCalls/index.ts (1)
1-1: LGTM!packages/apps-engine/src/definition/accessors/IRead.ts (1)
6-6: LGTM!Also applies to: 59-59
packages/apps-engine/src/definition/accessors/index.ts (1)
22-22: LGTM!packages/apps/src/server/bridges/AppBridges.ts (1)
15-15: LGTM!Also applies to: 56-57, 116-116
packages/apps/src/server/bridges/index.ts (1)
17-17: LGTM!Also applies to: 39-39
packages/apps/src/server/permissions/AppPermissions.ts (1)
132-134: LGTM!packages/apps/tests/test-data/utilities.ts (1)
508-542: LGTM!packages/apps/tests/test-data/bridges/appBridges.ts (1)
15-15: LGTM!Also applies to: 40-40, 115-115, 146-146, 261-263
packages/apps/tests/server/accessors/MediaCallRead.test.ts (1)
11-27: LGTM!packages/apps/tests/server/accessors/Reader.test.ts (1)
9-9: LGTM!Also applies to: 41-41, 45-66, 78-78
apps/meteor/tests/data/apps/app-packages/index.ts (1)
17-17: LGTM!apps/meteor/tests/data/apps/helper.ts (3)
7-35: Good error handling and async ZIP reading.The implementation correctly wraps the callback-based
readAsTextAsyncin a promise usingPromise.withResolvers(), and provides clear error context by wrapping the caught exception.
73-87: LGTM!
17-17:Promise.withResolvers()is supported by the project’s declared Node.js version.The repository pins Node.js to 22.22.2 via
engines.nodeinpackage.json(root andpackages/message-parser) and usesnode:22.22.2-*in Dockerfiles;Promise.withResolvers()is available in Node.js 22.0.0+.apps/meteor/tests/end-to-end/apps/app-media-call-reader.ts (3)
29-54: Comprehensive assertions for internal call shape.The test thoroughly validates the returned call object structure, including nested properties (createdBy.type, caller.type, callee.type) and array lengths (uids, features).
56-77: Comprehensive assertions for external call shape.Correctly validates external call differences: sipCallId presence, callee type 'sip', and single UID.
92-102: Correctly tests permission enforcement.Verifies that without
media-call.readpermission, the API returns{ call: null }instead of the call data, confirming the permission gate works as expected.apps/meteor/tests/data/apps/app-packages/README.md (1)
670-740: LGTM!
| '@rocket.chat/meteor': minor | ||
| --- | ||
|
|
||
| Added the new MediaCallRead accessor that allows apps to read information from the system's media call |
Contributor
There was a problem hiding this comment.
Use plural "media calls" for clarity.
The description reads "system's media call" (singular), but the accessor reads from the system's collection of media calls. Change to "media calls" (plural) to accurately reflect that the system manages multiple call records.
📝 Proposed fix
-Added the new MediaCallRead accessor that allows apps to read information from the system's media call
+Added the new MediaCallRead accessor that allows apps to read information from the system's media calls📝 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.
Suggested change
| Added the new MediaCallRead accessor that allows apps to read information from the system's media call | |
| Added the new MediaCallRead accessor that allows apps to read information from the system's media calls |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/tired-teeth-cheat.md at line 7, Update the changelog entry text
to use the plural "media calls" for accuracy: locate the sentence mentioning
"Added the new MediaCallRead accessor that allows apps to read information from
the system's media call" and change it to "Added the new MediaCallRead accessor
that allows apps to read information from the system's media calls" so it
reflects that the accessor reads from the collection of media call records.
| * @param id the id of the media call | ||
| * @returns the media call | ||
| */ | ||
| getById(id: string): Promise<IMediaCall | undefined>; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "IMediaCallRead contract:"
rg -n -C2 'interface IMediaCallRead|getById\(id: string\): Promise<.*>' packages/apps-engine/src/definition/accessors/IMediaCallRead.ts
echo
echo "Bridge permission branch and return type:"
rg -n -C3 'doGetById\(|return null|getById\(' packages/apps/src/server/bridges/MediaCallBridge.ts
echo
echo "Accessor delegation return type:"
rg -n -C3 'class MediaCallRead|getById\(id: string\)|doGetById' packages/apps/src/server/accessors/MediaCallRead.tsRepository: RocketChat/Rocket.Chat
Length of output: 1328
Align IMediaCallRead.getById nullability with permission-denied behavior.
In packages/apps-engine/src/definition/accessors/IMediaCallRead.ts (line 14), getById returns Promise<IMediaCall | undefined>, but packages/apps/src/server/bridges/MediaCallBridge.ts returns Promise<IMediaCall | null> and yields null when the read permission is missing; packages/apps/src/server/accessors/MediaCallRead.ts forwards that value directly. Update the interface/bridge to use null consistently, or normalize null to undefined in MediaCallRead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps-engine/src/definition/accessors/IMediaCallRead.ts` at line 14,
The return nullability is inconsistent: update the IMediaCallRead.getById
signature to return Promise<IMediaCall | null> (instead of Promise<IMediaCall |
undefined>) so it matches the MediaCallBridge.ts implementation and the value
forwarded by MediaCallRead.ts; then update any implementations/usages of
IMediaCallRead.getById to handle null (or adjust MediaCallRead.ts to explicitly
return null) to ensure consistent null semantics across IMediaCallRead.getById,
MediaCallBridge, and MediaCallRead.
Comment on lines
+9
to
+17
| public async doGetById(callId: string, appId: string): Promise<IMediaCall | null> { | ||
| if (this.hasReadPermission(appId)) { | ||
| return this.getById(callId, appId); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| protected abstract getById(callId: string, appId: string): Promise<IMediaCall | null>; |
Contributor
There was a problem hiding this comment.
Align bridge nullability with the accessor contract.
Line 14 returns null, but packages/apps/src/server/accessors/MediaCallRead.ts exposes getById as Promise<IMediaCall | undefined> and delegates directly to doGetById. This leaks a different sentinel across the boundary and can break strict === undefined checks in app code.
Proposed fix
export abstract class MediaCallBridge extends BaseBridge {
- public async doGetById(callId: string, appId: string): Promise<IMediaCall | null> {
+ public async doGetById(callId: string, appId: string): Promise<IMediaCall | undefined> {
if (this.hasReadPermission(appId)) {
- return this.getById(callId, appId);
+ return (await this.getById(callId, appId)) ?? undefined;
}
- return null;
+ return undefined;
}
protected abstract getById(callId: string, appId: string): Promise<IMediaCall | null>;📝 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.
Suggested change
| public async doGetById(callId: string, appId: string): Promise<IMediaCall | null> { | |
| if (this.hasReadPermission(appId)) { | |
| return this.getById(callId, appId); | |
| } | |
| return null; | |
| } | |
| protected abstract getById(callId: string, appId: string): Promise<IMediaCall | null>; | |
| public async doGetById(callId: string, appId: string): Promise<IMediaCall | undefined> { | |
| if (this.hasReadPermission(appId)) { | |
| return (await this.getById(callId, appId)) ?? undefined; | |
| } | |
| return undefined; | |
| } | |
| protected abstract getById(callId: string, appId: string): Promise<IMediaCall | null>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/src/server/bridges/MediaCallBridge.ts` around lines 9 - 17, The
bridge currently returns null from doGetById which conflicts with the accessor
contract that expects undefined; update MediaCallBridge by changing doGetById's
and its abstract getById signature to return Promise<IMediaCall | undefined>
instead of Promise<IMediaCall | null>, and replace the literal return null with
return undefined so the sentinel value matches
packages/apps/src/server/accessors/MediaCallRead.ts and callers using ===
undefined remain correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
DMV-13
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
media-call.readpermission to control app access to media callsTests