feat(apps): apps-engine split, dynamic import apps#40185
Open
d-gubert wants to merge 10 commits intofeat/apps-engine-split--pr2a-copy-to-appsfrom
Open
feat(apps): apps-engine split, dynamic import apps#40185d-gubert wants to merge 10 commits intofeat/apps-engine-split--pr2a-copy-to-appsfrom
d-gubert wants to merge 10 commits intofeat/apps-engine-split--pr2a-copy-to-appsfrom
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
|
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:
WalkthroughGenerates an ephemeral Deno config at runtime that rewrites ChangesDeno static config & lint
Ephemeral runtime config & spawn
Deno runtime type schema
Room manager narrowing
Bulk import-specifier normalization (type-only)
UIHelper import fixes & explicit casts
Minor lint/comment removal
Misc
Sequence Diagram(s)sequenceDiagram
participant Runtime as AppsEngineDenoRuntime
participant FS as FileSystem
participant StaticCfg as StaticDenoConfig
participant Process as DenoProcess
Runtime->>FS: resolve('@rocket.chat/apps-engine/package.json')
FS-->>Runtime: absolute apps-engine path
Runtime->>StaticCfg: read static deno.jsonc
StaticCfg-->>Runtime: config content
Runtime->>Runtime: rewrite imports['@rocket.chat/apps-engine/'] -> file://<absolute path>
Runtime->>FS: write ephemeral deno.runtime.jsonc in temp dir
FS-->>Runtime: temp config path
Runtime->>Process: spawn Deno --config=temp --allow-read=[temp, static cfg dir, apps-engine dir] (env DENO_DIR)
Process-->>Runtime: subprocess started
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This was referenced Apr 16, 2026
d-gubert
force-pushed
the
feat/apps-engine-split--pr2a-copy-to-apps
branch
from
64502db to
f0a4b79
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2a-copy-to-apps
branch
from
654fe6a to
8ce8b49
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2b-dynamic-import-map
branch
from
969ce6b to
af58d57
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2a-copy-to-apps
branch
2 times, most recently
from
83b87e5 to
6821c9b
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2b-dynamic-import-map
branch
from
af58d57 to
68964e4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/apps-engine-split--pr2a-copy-to-apps #40185 +/- ##
=============================================================================
- Coverage 70.03% 69.98% -0.05%
=============================================================================
Files 3301 3301
Lines 120462 120462
Branches 21551 21583 +32
=============================================================================
- Hits 84361 84311 -50
- Misses 32813 32843 +30
- Partials 3288 3308 +20
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
✅ Layne — scan passed No security issues found on latest push. |
Member
|
/layne exception-approve LAYNE-b36330a09811ddaf reason: false positive |
|
✅ Exception recorded for LAYNE-b36330a09811ddaf by @julio-rocketchat: "false positive". Re-running scan... |
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: 1
🧹 Nitpick comments (1)
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts (1)
107-123: Consider adding error handling for file operations.The function reads and parses the static config without error handling. If the static config file is missing or contains invalid JSON, this will throw an unhandled exception during subprocess spawn.
While the static analysis tool flags a path traversal concern, this is a false positive in context—
tempDiroriginates from trusted internalAppManager.getTempFilePath(), and per prior discussion, an attacker with write access to the temp directory already has far greater control than the deno-runtime sandbox provides.🛡️ Optional: wrap file operations for clearer error messages
export function generateRuntimeDenoConfig(staticConfigPath: string, tempDir: string): string { - const staticConfig = JSON.parse(fs.readFileSync(staticConfigPath, 'utf8')); + let staticConfig: Record<string, unknown>; + try { + staticConfig = JSON.parse(fs.readFileSync(staticConfigPath, 'utf8')); + } catch (e) { + throw new Error(`Failed to read deno config from ${staticConfigPath}: ${e.message}`); + } const appsEngineSrcUrl = `file://${getAppsEngineSourceDir()}/`;Based on learnings: granting
--allow-readto the fullthis.tempFilePathis intentional, and spoofing the symlink requires attacker-level system access that grants far greater control than the sandbox provides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts` around lines 107 - 123, The function generateRuntimeDenoConfig lacks error handling around fs.readFileSync, JSON.parse, and fs.writeFileSync which can throw on missing/invalid files or write errors; update generateRuntimeDenoConfig to wrap the read/parse/write operations in a try/catch, use the symbols fs.readFileSync, JSON.parse, and fs.writeFileSync and the variables staticConfigPath and runtimeConfigPath to locate the calls, and on error throw or rethrow a clear, contextual Error (including staticConfigPath and the original error message) so callers can surface meaningful failures instead of unhandled exceptions; keep getAppsEngineSourceDir usage and return runtimeConfigPath on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/deno-runtime/lib/room.ts`:
- Around line 5-11: IRoomManager's signature for doGetUsernamesOfRoomById must
be tightened to match IInternalBridge (accept a string, not string | undefined)
and call sites must guard against undefined: update the interface method
declaration doGetUsernamesOfRoomById(id: string): Promise<Array<string>> and
then where this.id is passed into the bridge call (calls to
getBridges().getInternalBridge().doGetUsernamesOfRoomById(...)) replace the raw
this.id with a safe default (e.g., this.id ?? '') so the bridge always receives
a string.
---
Nitpick comments:
In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts`:
- Around line 107-123: The function generateRuntimeDenoConfig lacks error
handling around fs.readFileSync, JSON.parse, and fs.writeFileSync which can
throw on missing/invalid files or write errors; update generateRuntimeDenoConfig
to wrap the read/parse/write operations in a try/catch, use the symbols
fs.readFileSync, JSON.parse, and fs.writeFileSync and the variables
staticConfigPath and runtimeConfigPath to locate the calls, and on error throw
or rethrow a clear, contextual Error (including staticConfigPath and the
original error message) so callers can surface meaningful failures instead of
unhandled exceptions; keep getAppsEngineSourceDir usage and return
runtimeConfigPath on success.
🪄 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: 145625bd-7a45-44fe-9826-af1b31aeb49f
📒 Files selected for processing (3)
packages/apps/deno-runtime/deno.jsoncpackages/apps/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📜 Review details
🧰 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/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
📚 Learning: 2026-03-18T16:45:52.113Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Applied to files:
packages/apps/deno-runtime/deno.jsoncpackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
packages/apps/deno-runtime/deno.jsonc
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
packages/apps/deno-runtime/deno.jsonc
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
packages/apps/deno-runtime/deno.jsonc
📚 Learning: 2026-01-08T15:07:57.458Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
Applied to files:
packages/apps/deno-runtime/deno.jsoncpackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-03-17T16:08:37.572Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39690
File: packages/ui-voip/package.json:11-11
Timestamp: 2026-03-17T16:08:37.572Z
Learning: In `packages/ui-voip/package.json` (RocketChat/Rocket.Chat), the team deliberately chose to use `rm -rf dist` directly in the `"build"` script instead of `rimraf`, as they decided against introducing the `rimraf` dependency for this package. Do not flag `rm -rf dist` in the ui-voip build script as a cross-platform issue requiring rimraf.
Applied to files:
packages/apps/deno-runtime/deno.jsonc
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/apps/deno-runtime/deno.jsonc
📚 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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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 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/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.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/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-03-18T16:47:56.781Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🪛 GitHub Check: Layne Security Scan
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
[failure] 119-120: [pi_agent] pi_agent/command-injection
[R119-R120] The generateRuntimeDenoConfig function writes a file to path.join(tempDir, 'deno_runtime.jsonc') without validating the tempDir parameter. While the current call site uses a readonly tempFilePath from the AppManager, the function is exported and could be called directly with attacker-controlled paths.
If an attacker can control the tempDir parameter (e.g., by influencing manager.getTempFilePath() or calling the exported function directly), they could inject path traversal sequences like ../ to write files to arbitrary locations on the filesystem (e.g., /etc/cron.d/malicious or overwriting critical system files).
To fix this issue, validate and sanitize the tempDir parameter before using it, or use path.resolve() and ensure the resulting path is within an allowed directory. Consider validating that the path doesn't contain .. segments or use a library like path-is-inside to ensure the target path is within the intended temp directory.
🔇 Additional comments (5)
packages/apps/deno-runtime/lib/room.ts (2)
47-47: LGTM: Dependency narrowing achieved.Successfully narrows the private field type from
AppManagertoIRoomManager, achieving the PR objective of decoupling deno-runtime from the concrete AppManager implementation.
62-62: LGTM: Constructor correctly narrowed to minimal interface.The constructor parameter type change from
AppManagertoIRoomManagercorrectly implements dependency inversion, allowing any manager satisfying the minimal interface contract. This achieves the decoupling objective stated in the PR.Note: Ensure the
IRoomManagerinterface signature issue (flagged separately) is resolved to maintain type safety.packages/apps/deno-runtime/deno.jsonc (1)
5-5: LGTM!The updated import alias path correctly points to the apps-engine source directory relative to the monorepo structure. This static mapping serves as a build-time default, while the runtime path resolution via
generateRuntimeDenoConfig()ensures location-independence across different deployment environments.packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts (2)
89-97: LGTM!The
require.resolvepattern provides robust cross-environment path resolution, consistent with the existinggetDenoConfigPath()function. This correctly handles monorepo, Meteor bundle, and standalone installations.
201-231: LGTM!The
spawnProcess()integration is well-structured:
- Correctly resolves the apps-engine source directory and adds it to
--allow-readpermissions- Generates the runtime config once per spawn with the absolute path
- Uses the generated config path for the Deno subprocess
This achieves the PR objective of making deno-runtime location-independent.
d-gubert
force-pushed
the
feat/apps-engine-split--pr2a-copy-to-apps
branch
from
6821c9b to
f9d22fd
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2b-dynamic-import-map
branch
from
db0e30b to
a5aacd5
Compare
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/deno-runtime/lib/room.ts (1)
5-5: ⚡ Quick winRemove implementation comment to match repository style rule.
Drop this inline comment and let the type/interface name carry intent.
Suggested diff
-/** Minimal interface covering the only AppManager capability used by Room */ interface IRoomManager {As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/deno-runtime/lib/room.ts` at line 5, Remove the inline implementation comment above the AppManager type used by Room: delete the "/** Minimal interface covering the only AppManager capability used by Room */" JSDoc so the interface/type name alone conveys intent (locate the AppManager interface/type referenced by Room and remove that comment line).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/deno-runtime/lib/room.ts`:
- Line 45: The _USERNAMES field is typed as Promise<Array<string>> | undefined
but getUsernames() currently awaits the result and assigns an Array<string> into
_USERNAMES; change getUsernames() to assign the Promise itself (remove the
await) so _USERNAMES stores a Promise like the usernames getter does, e.g., set
_USERNAMES = (the async call returning usernames) without awaiting, and
return/await that Promise where callers need the resolved value; update any
local variables in getUsernames() to treat the assignment as a Promise rather
than an Array<string>.
---
Nitpick comments:
In `@packages/apps/deno-runtime/lib/room.ts`:
- Line 5: Remove the inline implementation comment above the AppManager type
used by Room: delete the "/** Minimal interface covering the only AppManager
capability used by Room */" JSDoc so the interface/type name alone conveys
intent (locate the AppManager interface/type referenced by Room and remove that
comment line).
🪄 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: e72d3c6b-a85c-4249-b31e-6c59cdf01481
📒 Files selected for processing (46)
packages/apps/deno-runtime/deno.jsoncpackages/apps/deno-runtime/handlers/api-handler.tspackages/apps/deno-runtime/handlers/app/construct.tspackages/apps/deno-runtime/handlers/app/handleGetStatus.tspackages/apps/deno-runtime/handlers/app/handleInitialize.tspackages/apps/deno-runtime/handlers/app/handleOnDisable.tspackages/apps/deno-runtime/handlers/app/handleOnEnable.tspackages/apps/deno-runtime/handlers/app/handleOnInstall.tspackages/apps/deno-runtime/handlers/app/handleOnPreSettingUpdate.tspackages/apps/deno-runtime/handlers/app/handleOnSettingUpdated.tspackages/apps/deno-runtime/handlers/app/handleOnUninstall.tspackages/apps/deno-runtime/handlers/app/handleOnUpdate.tspackages/apps/deno-runtime/handlers/app/handleSetStatus.tspackages/apps/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps/deno-runtime/handlers/lib/assertions.tspackages/apps/deno-runtime/handlers/listener/handler.tspackages/apps/deno-runtime/handlers/outboundcomms-handler.tspackages/apps/deno-runtime/handlers/scheduler-handler.tspackages/apps/deno-runtime/handlers/slashcommand-handler.tspackages/apps/deno-runtime/handlers/tests/api-handler.test.tspackages/apps/deno-runtime/handlers/tests/helpers/mod.tspackages/apps/deno-runtime/handlers/tests/upload-event-handler.test.tspackages/apps/deno-runtime/handlers/uikit/handler.tspackages/apps/deno-runtime/handlers/videoconference-handler.tspackages/apps/deno-runtime/lib/accessors/builders/BlockBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/DiscussionBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/LivechatMessageBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/MessageBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/RoomBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/UserBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/VideoConferenceBuilder.tspackages/apps/deno-runtime/lib/accessors/extenders/HttpExtender.tspackages/apps/deno-runtime/lib/accessors/extenders/MessageExtender.tspackages/apps/deno-runtime/lib/accessors/extenders/RoomExtender.tspackages/apps/deno-runtime/lib/accessors/extenders/VideoConferenceExtend.tspackages/apps/deno-runtime/lib/accessors/http.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/deno-runtime/lib/accessors/modify/ModifyCreator.tspackages/apps/deno-runtime/lib/accessors/modify/ModifyExtender.tspackages/apps/deno-runtime/lib/accessors/modify/ModifyUpdater.tspackages/apps/deno-runtime/lib/accessors/notifier.tspackages/apps/deno-runtime/lib/codec.tspackages/apps/deno-runtime/lib/room.tspackages/apps/deno-runtime/lib/roomFactory.tspackages/apps/deno-runtime/lib/wrapAppForRequest.tspackages/apps/src/server/accessors/ExternalComponentsExtend.ts
💤 Files with no reviewable changes (1)
- packages/apps/src/server/accessors/ExternalComponentsExtend.ts
✅ Files skipped from review due to trivial changes (44)
- packages/apps/deno-runtime/handlers/app/handleOnEnable.ts
- packages/apps/deno-runtime/handlers/videoconference-handler.ts
- packages/apps/deno-runtime/lib/wrapAppForRequest.ts
- packages/apps/deno-runtime/handlers/outboundcomms-handler.ts
- packages/apps/deno-runtime/handlers/listener/handler.ts
- packages/apps/deno-runtime/lib/codec.ts
- packages/apps/deno-runtime/handlers/app/handleOnUpdate.ts
- packages/apps/deno-runtime/handlers/app/handleUploadEvents.ts
- packages/apps/deno-runtime/handlers/lib/assertions.ts
- packages/apps/deno-runtime/handlers/scheduler-handler.ts
- packages/apps/deno-runtime/lib/accessors/builders/MessageBuilder.ts
- packages/apps/deno-runtime/handlers/app/handleOnInstall.ts
- packages/apps/deno-runtime/handlers/app/handleOnDisable.ts
- packages/apps/deno-runtime/lib/accessors/modify/ModifyCreator.ts
- packages/apps/deno-runtime/handlers/app/handleOnPreSettingUpdate.ts
- packages/apps/deno-runtime/handlers/tests/helpers/mod.ts
- packages/apps/deno-runtime/handlers/uikit/handler.ts
- packages/apps/deno-runtime/lib/accessors/builders/LivechatMessageBuilder.ts
- packages/apps/deno-runtime/handlers/slashcommand-handler.ts
- packages/apps/deno-runtime/handlers/app/handleSetStatus.ts
- packages/apps/deno-runtime/lib/accessors/http.ts
- packages/apps/deno-runtime/lib/accessors/modify/ModifyExtender.ts
- packages/apps/deno-runtime/lib/accessors/notifier.ts
- packages/apps/deno-runtime/lib/accessors/builders/UserBuilder.ts
- packages/apps/deno-runtime/handlers/app/handleInitialize.ts
- packages/apps/deno-runtime/lib/accessors/mod.ts
- packages/apps/deno-runtime/handlers/app/handleOnUninstall.ts
- packages/apps/deno-runtime/lib/accessors/extenders/MessageExtender.ts
- packages/apps/deno-runtime/lib/accessors/extenders/VideoConferenceExtend.ts
- packages/apps/deno-runtime/handlers/app/construct.ts
- packages/apps/deno-runtime/lib/accessors/extenders/HttpExtender.ts
- packages/apps/deno-runtime/handlers/app/handleGetStatus.ts
- packages/apps/deno-runtime/lib/roomFactory.ts
- packages/apps/deno-runtime/lib/accessors/builders/BlockBuilder.ts
- packages/apps/deno-runtime/deno.jsonc
- packages/apps/deno-runtime/handlers/tests/upload-event-handler.test.ts
- packages/apps/deno-runtime/handlers/app/handleOnSettingUpdated.ts
- packages/apps/deno-runtime/lib/accessors/modify/ModifyUpdater.ts
- packages/apps/deno-runtime/handlers/tests/api-handler.test.ts
- packages/apps/deno-runtime/lib/accessors/builders/VideoConferenceBuilder.ts
- packages/apps/deno-runtime/lib/accessors/builders/DiscussionBuilder.ts
- packages/apps/deno-runtime/lib/accessors/builders/RoomBuilder.ts
- packages/apps/deno-runtime/lib/accessors/extenders/RoomExtender.ts
- packages/apps/deno-runtime/handlers/api-handler.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/deno-runtime/lib/room.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-04-23T13:34:53.287Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:53.287Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 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:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-04-17T17:38:15.994Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: packages/ui-kit/src/interactions/UserInteraction.ts:33-33
Timestamp: 2026-04-17T17:38:15.994Z
Learning: In RocketChat/Rocket.Chat (`packages/ui-kit/src/interactions/UserInteraction.ts`), `ViewSubmitUserInteraction` and `ViewClosedUserInteraction` intentionally do NOT include `rid` when the interaction originates from a **modal** surface. Modals are not scoped to a room, so no room id is available in that context. The `rid?: string` field is optional to support the contextual bar surface (where room context exists) while remaining absent for modals. Do not flag the absence of `rid` in modal viewSubmit/viewClosed interactions as a missing-context bug.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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 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/deno-runtime/lib/room.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/deno-runtime/lib/room.ts
🔇 Additional comments (1)
packages/apps/deno-runtime/lib/room.ts (1)
53-53: Good null-guard in deprecatedusernamesgetter.Early-returning
[]whenidis missing prevents invalid bridge calls on this path.
d-gubert
force-pushed
the
feat/apps-engine-split--pr2b-dynamic-import-map
branch
from
a5aacd5 to
1a62b06
Compare
coderabbitai
Bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts (1)
183-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrite the generated runtime config to the temp directory, not alongside the static package config.
this.denoEphemeralConfigPathcurrently points next todeno.jsonc(package install path). In immutable/read-only deployments thiswriteFileSyncpath will fail and prevent runtime startup. Usethis.tempFilePath(already intended for writable runtime artifacts).Suggested fix
- this.denoEphemeralConfigPath = this.denoConfigPath.replace('.jsonc', '.runtime.jsonc'); + this.denoEphemeralConfigPath = path.join(this.tempFilePath, 'deno.runtime.jsonc');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts` around lines 183 - 199, The generated runtime config is being written next to the static package config via this.denoEphemeralConfigPath (derived from this.denoConfigPath), which fails in read-only deployments; change the ephemeral config target to a writable location by using this.tempFilePath (e.g. build this.denoEphemeralConfigPath from this.tempFilePath and the basename of this.denoConfigPath) and then call generateEphemeralDenoConfig with that updated this.denoEphemeralConfigPath so the runtime config is created in the temp directory instead of alongside the static package files.
🧹 Nitpick comments (2)
packages/apps/deno-runtime/lib/room.ts (1)
5-5: ⚡ Quick winRemove the implementation comment above
IRoomManager.Line 5 adds an implementation comment in a
.tsfile; this repo guideline asks to avoid code comments in implementation.As per coding guidelines: "
**/*.{ts,tsx,js}: ... Avoid code comments in the implementation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/deno-runtime/lib/room.ts` at line 5, Remove the implementation comment placed above the IRoomManager declaration: delete the line "/** Minimal interface covering the only AppManager capability used by Room */" so the TypeScript file contains no implementation comments per guidelines; locate the IRoomManager interface in room.ts and ensure only the interface declaration remains without the preceding explanatory comment.packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts (1)
84-100: ⚡ Quick winRemove new implementation comments to align with repository guideline.
Please remove these newly added inline/block comments and keep intent expressed through naming and types.
As per coding guidelines "Avoid code comments in the implementation".
Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts` around lines 84 - 100, Remove the newly added block/inline comments that precede getAppsEngineDir and the following deno-runtime config generation function (the comment describing generation of deno_runtime.jsonc); keep the function names and behavior unchanged and rely on clear naming and types for intent instead of comments, and also remove the similar comment occurrence elsewhere in this file that was added at the same region so the implementation follows the repository guideline of avoiding in-line/block implementation comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts`:
- Around line 183-199: The generated runtime config is being written next to the
static package config via this.denoEphemeralConfigPath (derived from
this.denoConfigPath), which fails in read-only deployments; change the ephemeral
config target to a writable location by using this.tempFilePath (e.g. build
this.denoEphemeralConfigPath from this.tempFilePath and the basename of
this.denoConfigPath) and then call generateEphemeralDenoConfig with that updated
this.denoEphemeralConfigPath so the runtime config is created in the temp
directory instead of alongside the static package files.
---
Nitpick comments:
In `@packages/apps/deno-runtime/lib/room.ts`:
- Line 5: Remove the implementation comment placed above the IRoomManager
declaration: delete the line "/** Minimal interface covering the only AppManager
capability used by Room */" so the TypeScript file contains no implementation
comments per guidelines; locate the IRoomManager interface in room.ts and ensure
only the interface declaration remains without the preceding explanatory
comment.
In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts`:
- Around line 84-100: Remove the newly added block/inline comments that precede
getAppsEngineDir and the following deno-runtime config generation function (the
comment describing generation of deno_runtime.jsonc); keep the function names
and behavior unchanged and rely on clear naming and types for intent instead of
comments, and also remove the similar comment occurrence elsewhere in this file
that was added at the same region so the implementation follows the repository
guideline of avoiding in-line/block implementation comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a3e28c5-354f-4758-b4c2-2bd6693ec13f
📒 Files selected for processing (49)
packages/apps/.gitignorepackages/apps/deno-runtime/deno.jsoncpackages/apps/deno-runtime/handlers/api-handler.tspackages/apps/deno-runtime/handlers/app/construct.tspackages/apps/deno-runtime/handlers/app/handleGetStatus.tspackages/apps/deno-runtime/handlers/app/handleInitialize.tspackages/apps/deno-runtime/handlers/app/handleOnDisable.tspackages/apps/deno-runtime/handlers/app/handleOnEnable.tspackages/apps/deno-runtime/handlers/app/handleOnInstall.tspackages/apps/deno-runtime/handlers/app/handleOnPreSettingUpdate.tspackages/apps/deno-runtime/handlers/app/handleOnSettingUpdated.tspackages/apps/deno-runtime/handlers/app/handleOnUninstall.tspackages/apps/deno-runtime/handlers/app/handleOnUpdate.tspackages/apps/deno-runtime/handlers/app/handleSetStatus.tspackages/apps/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps/deno-runtime/handlers/lib/assertions.tspackages/apps/deno-runtime/handlers/listener/handler.tspackages/apps/deno-runtime/handlers/outboundcomms-handler.tspackages/apps/deno-runtime/handlers/scheduler-handler.tspackages/apps/deno-runtime/handlers/slashcommand-handler.tspackages/apps/deno-runtime/handlers/tests/api-handler.test.tspackages/apps/deno-runtime/handlers/tests/helpers/mod.tspackages/apps/deno-runtime/handlers/tests/upload-event-handler.test.tspackages/apps/deno-runtime/handlers/uikit/handler.tspackages/apps/deno-runtime/handlers/videoconference-handler.tspackages/apps/deno-runtime/lib/accessors/builders/BlockBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/DiscussionBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/LivechatMessageBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/MessageBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/RoomBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/UserBuilder.tspackages/apps/deno-runtime/lib/accessors/builders/VideoConferenceBuilder.tspackages/apps/deno-runtime/lib/accessors/extenders/HttpExtender.tspackages/apps/deno-runtime/lib/accessors/extenders/MessageExtender.tspackages/apps/deno-runtime/lib/accessors/extenders/RoomExtender.tspackages/apps/deno-runtime/lib/accessors/extenders/VideoConferenceExtend.tspackages/apps/deno-runtime/lib/accessors/http.tspackages/apps/deno-runtime/lib/accessors/mod.tspackages/apps/deno-runtime/lib/accessors/modify/ModifyCreator.tspackages/apps/deno-runtime/lib/accessors/modify/ModifyExtender.tspackages/apps/deno-runtime/lib/accessors/modify/ModifyUpdater.tspackages/apps/deno-runtime/lib/accessors/notifier.tspackages/apps/deno-runtime/lib/codec.tspackages/apps/deno-runtime/lib/room.tspackages/apps/deno-runtime/lib/roomFactory.tspackages/apps/deno-runtime/lib/wrapAppForRequest.tspackages/apps/src/server/accessors/ExternalComponentsExtend.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.tspackages/apps/src/server/runtime/deno/typings.ts
💤 Files with no reviewable changes (1)
- packages/apps/src/server/accessors/ExternalComponentsExtend.ts
✅ Files skipped from review due to trivial changes (43)
- packages/apps/.gitignore
- packages/apps/deno-runtime/handlers/app/handleOnEnable.ts
- packages/apps/deno-runtime/handlers/api-handler.ts
- packages/apps/deno-runtime/handlers/outboundcomms-handler.ts
- packages/apps/deno-runtime/handlers/uikit/handler.ts
- packages/apps/deno-runtime/handlers/app/handleOnUninstall.ts
- packages/apps/deno-runtime/handlers/app/handleUploadEvents.ts
- packages/apps/deno-runtime/lib/accessors/builders/MessageBuilder.ts
- packages/apps/deno-runtime/handlers/tests/api-handler.test.ts
- packages/apps/deno-runtime/handlers/app/handleOnUpdate.ts
- packages/apps/deno-runtime/handlers/tests/helpers/mod.ts
- packages/apps/deno-runtime/lib/codec.ts
- packages/apps/deno-runtime/handlers/app/handleOnInstall.ts
- packages/apps/deno-runtime/handlers/scheduler-handler.ts
- packages/apps/deno-runtime/handlers/app/handleOnDisable.ts
- packages/apps/deno-runtime/lib/accessors/http.ts
- packages/apps/deno-runtime/lib/wrapAppForRequest.ts
- packages/apps/deno-runtime/lib/accessors/builders/LivechatMessageBuilder.ts
- packages/apps/deno-runtime/handlers/listener/handler.ts
- packages/apps/deno-runtime/lib/accessors/extenders/HttpExtender.ts
- packages/apps/deno-runtime/lib/accessors/extenders/MessageExtender.ts
- packages/apps/deno-runtime/lib/accessors/builders/UserBuilder.ts
- packages/apps/deno-runtime/lib/accessors/modify/ModifyExtender.ts
- packages/apps/deno-runtime/lib/roomFactory.ts
- packages/apps/deno-runtime/handlers/app/handleGetStatus.ts
- packages/apps/deno-runtime/handlers/app/handleOnPreSettingUpdate.ts
- packages/apps/deno-runtime/lib/accessors/extenders/VideoConferenceExtend.ts
- packages/apps/deno-runtime/lib/accessors/extenders/RoomExtender.ts
- packages/apps/deno-runtime/lib/accessors/builders/VideoConferenceBuilder.ts
- packages/apps/deno-runtime/handlers/tests/upload-event-handler.test.ts
- packages/apps/deno-runtime/lib/accessors/notifier.ts
- packages/apps/deno-runtime/handlers/app/handleOnSettingUpdated.ts
- packages/apps/deno-runtime/lib/accessors/modify/ModifyUpdater.ts
- packages/apps/deno-runtime/lib/accessors/builders/BlockBuilder.ts
- packages/apps/deno-runtime/handlers/app/construct.ts
- packages/apps/deno-runtime/lib/accessors/mod.ts
- packages/apps/deno-runtime/lib/accessors/builders/RoomBuilder.ts
- packages/apps/deno-runtime/handlers/videoconference-handler.ts
- packages/apps/deno-runtime/lib/accessors/builders/DiscussionBuilder.ts
- packages/apps/deno-runtime/handlers/lib/assertions.ts
- packages/apps/deno-runtime/handlers/slashcommand-handler.ts
- packages/apps/deno-runtime/handlers/app/handleSetStatus.ts
- packages/apps/deno-runtime/handlers/app/handleInitialize.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps/src/server/runtime/deno/typings.ts
- packages/apps/deno-runtime/lib/accessors/modify/ModifyCreator.ts
- packages/apps/deno-runtime/deno.jsonc
📜 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🧰 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/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
📚 Learning: 2026-04-30T14:57:42.623Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40185
File: packages/apps/deno-runtime/lib/room.ts:45-45
Timestamp: 2026-04-30T14:57:42.623Z
Learning: In `packages/apps/deno-runtime/lib/room.ts`, the `getUsernames()` method intentionally uses `await` before assigning the result to `this._USERNAMES` (typed as `Promise<Array<string>> | undefined`), storing the resolved `Array<string>` rather than the Promise. This is a deliberate backward-compatibility workaround with the existing API and should not be flagged as a type mismatch or cache-contract violation in future reviews.
Applied to files:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-04-23T13:34:53.287Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:53.287Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-04-17T17:38:15.994Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: packages/ui-kit/src/interactions/UserInteraction.ts:33-33
Timestamp: 2026-04-17T17:38:15.994Z
Learning: In RocketChat/Rocket.Chat (`packages/ui-kit/src/interactions/UserInteraction.ts`), `ViewSubmitUserInteraction` and `ViewClosedUserInteraction` intentionally do NOT include `rid` when the interaction originates from a **modal** surface. Modals are not scoped to a room, so no room id is available in that context. The `rid?: string` field is optional to support the contextual bar surface (where room context exists) while remaining absent for modals. Do not flag the absence of `rid` in modal viewSubmit/viewClosed interactions as a missing-context bug.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-04-17T18:33:27.211Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: apps/meteor/tests/e2e/apps/uikit-interactions.spec.ts:123-151
Timestamp: 2026-04-17T18:33:27.211Z
Learning: In RocketChat/Rocket.Chat (`apps/meteor/tests/e2e/apps/uikit-interactions.spec.ts`), `executeBlockActionHandler` invocations originating from a **modal** surface intentionally do NOT include a `block_action_room` (room property) in the interaction payload. Modals are not scoped to a room, so no room id is available in that context. Do not flag the absence of a room assertion in the modal block-action test as a missing coverage bug; instead, document it explicitly with a `test.step` asserting the room entry is `undefined`.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 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:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
packages/apps/deno-runtime/lib/room.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:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
packages/apps/deno-runtime/lib/room.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
packages/apps/deno-runtime/lib/room.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 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/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.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/deno-runtime/lib/room.tspackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-03-18T16:45:52.113Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:181-188
Timestamp: 2026-03-18T16:45:52.113Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, granting `--allow-read` to the full `this.tempFilePath` (entire temp directory) in the Deno subprocess spawn options is intentional. The `deno.jsonc` config file can affect path resolution in ways that require read access to the broader temp directory, not just the `deno-runtime` subdirectory symlink. Do not flag this as an overly broad permission grant.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-03-18T16:47:56.781Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-01-08T15:07:57.458Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:8-12
Timestamp: 2026-01-08T15:07:57.458Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized and are instantiated and kept in the runtime context, so duck-typing checks on App methods (including protected methods like extendConfiguration) are reliable for runtime assertions.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🔇 Additional comments (1)
packages/apps/deno-runtime/lib/room.ts (1)
53-56: Good hardening on manager decoupling andidguard.The
IRoomManagernarrowing plus the Line 53 early return avoids calling the bridge with an invalid room id in the deprecated getter path.Also applies to: 64-64
d-gubert
force-pushed
the
feat/apps-engine-split--pr2a-copy-to-apps
branch
from
d95a3db to
7950594
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2b-dynamic-import-map
branch
from
1a62b06 to
e4796a8
Compare
d-gubert
force-pushed
the
feat/apps-engine-split--pr2a-copy-to-apps
branch
from
7950594 to
292fae1
Compare
…lass room.ts previously imported `AppManager` from the server layer (`@rocket.chat/apps-engine/server/AppManager.ts`) solely to type the private [PrivateManager] symbol field that backs the deprecated `usernames` getter. This created a cross-boundary import from the Deno subprocess into the Node.js host. Replace with a minimal inline `IRoomManager` interface that exposes only the one method actually called: getBridges().getInternalBridge().doGetUsernamesOfRoomById() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…th at spawn time
The static deno.jsonc used `"@rocket.chat/apps-engine/": "./../src/"` — a
relative path that only works when deno-runtime/ and apps-engine/src/ are
siblings in the same package. Now that deno-runtime lives in @rocket.chat/apps
and the definition source lives in @rocket.chat/apps-engine, the relative path
would resolve to the wrong location.
Solution: generate a `deno_runtime.jsonc` into the temp directory before each
Deno subprocess spawn. The generated config copies all settings from the static
deno.jsonc and injects `@rocket.chat/apps-engine/` as an absolute `file:` URL
resolved via `require.resolve('@rocket.chat/apps-engine/package.json')`. This
works correctly in any environment (monorepo dev, Meteor bundle, CI) without
assumptions about directory layout.
Changes:
- `getAppsEngineSourceDir()`: resolves apps-engine src/ via require.resolve
- `generateRuntimeDenoConfig()`: reads static config, injects resolved path,
writes to tempDir/deno_runtime.jsonc, returns the path
- `spawnProcess()`: calls generateRuntimeDenoConfig, uses the generated config,
adds appsEngineSrcDir to --allow-read
- deno.jsonc: remove the now-redundant static @rocket.chat/apps-engine/ entry
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
d-gubert
force-pushed
the
feat/apps-engine-split--pr2b-dynamic-import-map
branch
from
e4796a8 to
bc9fbb1
Compare
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)
This PR adapts the path resolution of the apps-engine package for the deno runtime. Some assumptions changed:
Issue(s)
Steps to test or reproduce
Further comments
Related to the "Apps-Engine split" stack:
@rocket.chat/apps-engineto@rocket.chat/appsinternal package #40395Deprecated
Summary by CodeRabbit
Refactor
Chores