fix(apps): reduce memory usage on apps-engine IPreFileUpload event #38068
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 0f7416c The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements temporary file storage for Apps Engine upload handling: writes uploaded file contents to a temp directory at upload time, passes the temp path into app subprocesses (via AppManager/getTempFilePath and Deno runtime permissions), and routes executePreFileUpload through new upload handlers and assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bridge as App Listener Bridge
participant TempFS as Temp File System
participant DenoRT as Deno Runtime
participant App as App Subprocess
Client->>Bridge: POST rooms.media/{roomId} (file)
Bridge->>TempFS: Write file to temp dir (uuid)
TempFS-->>Bridge: temp path
Bridge->>DenoRT: Invoke executePreFileUpload with {file, content} / temp path
DenoRT->>DenoRT: Check App implements IPreFileUpload
DenoRT->>DenoRT: Grant read to temp dir for subprocess
DenoRT->>App: Execute handler with context { file, content }
App-->>DenoRT: Result (allow/reject)
DenoRT-->>Bridge: Handler result
Bridge->>TempFS: Delete temp file
TempFS-->>Bridge: deleted
Bridge-->>Client: 200 or 400 response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (2)
📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (8)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (23)📚 Learning: 2026-01-08T15:07:57.458ZApplied to files:
📚 Learning: 2025-10-06T20:30:45.540ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-10-06T20:32:23.658ZApplied to files:
📚 Learning: 2026-01-08T15:03:59.621ZApplied to files:
📚 Learning: 2025-11-10T19:06:20.146ZApplied to files:
📚 Learning: 2025-10-16T21:09:51.816ZApplied to files:
📚 Learning: 2025-11-19T12:32:29.696ZApplied to files:
📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
📚 Learning: 2025-12-10T21:00:54.909ZApplied to files:
📚 Learning: 2026-01-08T15:07:47.836ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-19T18:20:07.720ZApplied to files:
🧬 Code graph analysis (2)packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (3)
apps/meteor/tests/data/apps/helper.ts (1)
🪛 Biome (2.1.2)apps/meteor/ee/server/apps/orchestrator.js[error] 1-1: Illegal use of an import declaration outside of a module not allowed inside scripts (parse) [error] 1-2: Illegal use of an import declaration outside of a module not allowed inside scripts (parse) [error] 2-3: Illegal use of an import declaration outside of a module not allowed inside scripts (parse) 🪛 markdownlint-cli2 (0.18.1)apps/meteor/tests/data/apps/app-packages/README.md14-14: Hard tabs (MD010, no-hard-tabs) 15-15: Hard tabs (MD010, no-hard-tabs) 16-16: Hard tabs (MD010, no-hard-tabs) ⏰ 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)
🔇 Additional comments (17)
✏️ Tip: You can disable this entire section by setting 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 |
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/14 21:17 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 14 Jan 2026 21:17:45 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38068 +/- ##
===========================================
- Coverage 70.70% 70.66% -0.05%
===========================================
Files 3147 3147
Lines 108851 108851
Branches 19625 19588 -37
===========================================
- Hits 76965 76921 -44
- Misses 29886 29922 +36
- Partials 2000 2008 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
changed the title
d-gubert
force-pushed
the
feat/apps-file-stream
branch
3 times, most recently
from
1b12ada to
d66b200
Compare
d-gubert
changed the title
d-gubert
force-pushed
the
feat/apps-file-stream
branch
from
44e518b to
69549c2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
119-149: Add validation for tempFilePath in constructor.Unlike other dependencies (
metadataStorage,logStorage,bridges,sourceStorage), thetempFilePathparameter lacks validation. Consider adding a check to ensure it's a non-empty string.🛡️ Proposed validation
if (sourceStorage instanceof AppSourceStorage) { this.appSourceStorage = sourceStorage; } else { throw new Error('Invalid instance of the AppSourceStorage'); } + if (typeof tempFilePath !== 'string' || !tempFilePath.trim()) { + throw new Error('Invalid tempFilePath: must be a non-empty string'); + } + this.tempFilePath = tempFilePath;
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.js:
- Around line 89-109: The uploadEvent handler can throw after writing tmpfile
(during convertToApp or executeListener) and never delete tmpfile; wrap the
conversion and listener execution in a try/finally: perform const appFile =
await this.orch.getConverters().get('uploads').convertToApp(file) and await
this.orch.getManager().getListenerManager().executeListener(...) inside the try,
and in the finally always attempt fs.promises.unlink(tmpfile) with the existing
warning log on failure (but do not let unlink errors mask the original error).
In @apps/meteor/tests/data/apps/app-packages/README.md:
- Around line 14-16: Replace the hard tabs used to indent the test setup block
so that the lines containing before(async () => { await
installLocalTestPackage(appImplementsIPreFileUpload); }); use spaces for
indentation; update the indentation for that entire snippet (and any adjacent
lines using tabs) to match the repository's space-based style and re-run the
linter or formatter to ensure consistency.
In @apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts:
- Line 38: Replace the incorrect Chai assertion on the error property (the line
starting with expect(res.body).to.have.property('error')): remove the invalid
`.that.is.string(fileContents)` chain and instead assert the property contains
the file contents, e.g. use `.that.includes(fileContents)` or
`.that.is.a('string').and.includes(fileContents)` so the test verifies the error
message contains the expected fileContents.
In @packages/apps-engine/deno-runtime/deno.jsonc:
- Line 7: The "@std/streams" dependency is pinned to an exact version ("1.0.16")
whereas "@std/cli" uses a caret range; update the "@std/streams" entry to use a
caret range ("^1.0.16") to match versioning style and allow minor/patch updates
unless you intentionally need an exact pin, i.e., change the value for the
"@std/streams" key to "^1.0.16".
In @packages/apps-engine/deno-runtime/handlers/lib/assertions.ts:
- Around line 4-6: The isRecord type guard currently returns true for arrays
because arrays are typeof 'object'; update isRecord(v: unknown): v is
Record<string, unknown> to also ensure v is not null and not an array (e.g., add
a check like !Array.isArray(v) or use Object.prototype.toString) so only plain
objects qualify; keep the function name isRecord and its signature unchanged
while adding the array exclusion to the boolean expression.
- Around line 8-12: The current assertAppAvailable uses a fragile duck-type
check on the protected method extendConfiguration; replace that with a stronger
discriminator by checking a combination of public IApp methods (e.g., verify
typeof (v as App).getID === 'function' && typeof (v as App).getStatus ===
'function' && typeof (v as App).getVersion === 'function') inside
assertAppAvailable so only objects implementing those public methods pass; keep
the same JsonRpcError.internalError throw when the check fails and update the
type assertion to asserts v is App.
In @packages/apps-engine/src/server/compiler/AppImplements.ts:
- Around line 7-13: In the AppImplements constructor the Object.keys iteration
types keys as string which conflicts with the AppInterface type; fix by
iterating using Object.values(AppInterface).forEach((int: AppInterface) => {
this.implemented[int] = false; }) or cast the keys with
(Object.keys(AppInterface) as AppInterface[]).forEach to ensure the loop
variable is AppInterface; update the constructor in AppImplements to use one of
these approaches so implemented is correctly initialized without TypeScript
errors.
🧹 Nitpick comments (4)
packages/apps-engine/src/server/ProxiedApp.ts (1)
83-83: Improved error logging with detailed inspection.The change from logging the raw error object to using
inspectwithdepth: 10provides more detailed visibility into nested error structures, which is valuable for debugging JSON-RPC errors.💡 Optional: Consider adding output limits
For extremely large or complex error objects, you could add limits to prevent excessive log output:
-console.error('JSON-RPC error received: ', inspect(e, { depth: 10 })); +console.error('JSON-RPC error received: ', inspect(e, { depth: 10, maxArrayLength: 100, maxStringLength: 1000 }));This would cap array and string lengths in the output while still providing detailed debugging information.
apps/meteor/ee/server/apps/orchestrator.js (1)
75-85: Validate the temp directory path and consider cleanup strategy.The temp directory creation is sound, but consider these improvements:
- The path from
os.tmpdir()could theoretically be empty or invalid—add a guard- The error message could specify which directory failed to create
- Consider documenting the cleanup strategy for temp files (e.g., does the OS handle it, or should there be a shutdown hook?)
🔒 Proposed validation enhancement
const tempFilePath = path.join(os.tmpdir(), 'apps-engine-temp'); + +if (!tempFilePath || typeof tempFilePath !== 'string') { + throw new Error('Failed to resolve temporary directory path'); +} try { // We call this only once at server startup, so using the synchronous version is fine fs.mkdirSync(tempFilePath); } catch (err) { // If the temp directory already exists, we can continue if (err.code !== 'EEXIST') { - throw new Error('Failed to initialize the Apps-Engine', { cause: err }); + throw new Error(`Failed to initialize the Apps-Engine temp directory at ${tempFilePath}`, { cause: err }); } }packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
16-20: Strengthen IUpload validation.The current validation only checks that
userandroomare objects, which is quite minimal. Consider validating additional required fields to fail fast on malformed uploads.🔍 More thorough validation
function assertIsUpload(v: unknown): asserts v is IUpload { - if (isRecord(v) && isRecord(v.user) && isRecord(v.room)) return; + if ( + isRecord(v) && + isRecord(v.user) && + isRecord(v.room) && + typeof v.name === 'string' && + typeof v.type === 'string' + ) return; throw JsonRpcError.invalidParams({ err: `Invalid 'file' parameter. Expected IUploadDetails, got`, value: v }); }Note: Adjust the fields based on the actual
IUploadinterface requirements.
28-38: Validate params structure before destructuring.The destructuring at line 29 assumes
paramsis an array with a specific structure. Ifparamsis malformed, this will fail silently or throw a cryptic error.🛡️ Add params validation
export default async function handleUploadEvents(method: typeof uploadEvents[number], params: unknown): Promise<Defined | JsonRpcError> { + if (!Array.isArray(params) || params.length === 0) { + return JsonRpcError.invalidParams({ err: 'Expected params to be a non-empty array' }); + } + const [{ file, path }] = params as [{ file?: IUpload, path?: string }]; const app = AppObjectRegistry.get<App>('app'); const handlerFunction = app?.[method as keyof App] as unknown; try { assertAppAvailable(app); assertHandlerFunction(handlerFunction); assertIsUpload(file); assertString(path);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zipis excluded by!**/*.zippackages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.changeset/spotty-steaks-notice.mdapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/ee/server/apps/orchestrator.jsapps/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/apps-hooks-file-upload.tspackages/apps-engine/deno-runtime/deno.jsoncpackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/src/definition/uploads/IFileUploadContext.tspackages/apps-engine/src/server/AppManager.tspackages/apps-engine/src/server/ProxiedApp.tspackages/apps-engine/src/server/compiler/AppImplements.tspackages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/src/server/managers/AppListenerManager.tspackages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tspackages/apps-engine/tests/test-data/utilities.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/src/server/ProxiedApp.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/src/server/managers/AppListenerManager.tsapps/meteor/tests/data/apps/app-packages/index.tspackages/apps-engine/src/server/AppManager.tsapps/meteor/ee/server/apps/orchestrator.jspackages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/tests/test-data/utilities.tsapps/meteor/app/apps/server/bridges/listeners.jspackages/apps-engine/src/definition/uploads/IFileUploadContext.tsapps/meteor/tests/data/apps/helper.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tspackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/src/server/compiler/AppImplements.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/apps-engine/tests/server/compiler/AppImplements.spec.ts
🧠 Learnings (16)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tspackages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tspackages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/index.tspackages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/index.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.mdpackages/apps-engine/tests/test-data/utilities.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tspackages/apps-engine/src/server/compiler/AppImplements.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/data/apps/app-packages/index.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/data/apps/app-packages/index.tsapps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/apps-engine/tests/server/compiler/AppImplements.spec.ts
🧬 Code graph analysis (6)
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
packages/apps-engine/src/definition/uploads/IFileUploadContext.ts (1)
IFileUploadInternalContext(3-6)
apps/meteor/ee/server/apps/orchestrator.js (1)
packages/apps-engine/src/server/AppManager.ts (1)
AppManager(71-1232)
apps/meteor/app/apps/server/bridges/listeners.js (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
packages/apps-engine/tests/server/compiler/AppImplements.spec.ts (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
packages/apps-engine/deno-runtime/handlers/app/handler.ts (1)
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
uploadEvents(14-14)handleUploadEvents(28-73)
packages/apps-engine/src/server/compiler/AppImplements.ts (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
🪛 Biome (2.1.2)
apps/meteor/ee/server/apps/orchestrator.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
apps/meteor/app/apps/server/bridges/listeners.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🪛 markdownlint-cli2 (0.18.1)
apps/meteor/tests/data/apps/app-packages/README.md
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
16-16: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (32)
packages/apps-engine/src/server/ProxiedApp.ts (1)
1-1: LGTM: Standard Node.js import.The import of
inspectfrom the 'util' module is correctly added to support enhanced error logging.packages/apps-engine/deno-runtime/deno.jsonc (1)
7-7: Package version and security check complete.@std/streams@1.0.16 exists in the JSR registry and has no known public security advisories.
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
28-28: LGTM! Consistent type update to IFileUploadInternalContext.The transition from
IFileUploadContexttoIFileUploadInternalContextis applied consistently across the interface definition (line 208), switch case (line 451), and method signature (line 1173). This aligns with the PR's goal of passing temporary file paths instead of file contents for the IPreFileUpload event.Also applies to: 207-210, 451-451, 1173-1179
.changeset/spotty-steaks-notice.md (1)
1-6: LGTM! Clear changeset description.The changeset accurately describes the memory usage fix for the IPreFileUpload event handler.
apps/meteor/tests/data/apps/app-packages/README.md (1)
1-54: Helpful test documentation.The README provides clear instructions and example code for using test app packages, including the app's source code for reference.
packages/apps-engine/tests/test-data/utilities.ts (1)
133-133: LGTM! Appropriate test mock for getTempFilePath.The mock returns a deterministic constant string, which is suitable for test purposes and consistent with other mock implementations in this test utility class.
packages/apps-engine/src/definition/uploads/IFileUploadContext.ts (1)
3-6: LGTM!The new
IFileUploadInternalContextinterface appropriately uses a file path instead of a Buffer, aligning with the PR's goal to reduce memory pressure during file upload events.apps/meteor/tests/data/apps/helper.ts (2)
24-27: LGTM!The parallel cleanup of all apps improves test isolation and prevents leftover state from affecting tests.
42-54: LGTM!The new helper function follows the existing pattern and provides the necessary functionality for installing local test packages.
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts (2)
12-24: LGTM!The test suite structure properly handles setup and teardown, ensuring clean state for each test execution.
42-54: LGTM!The test properly validates the success case when the app doesn't prevent the upload.
apps/meteor/tests/data/apps/app-packages/index.ts (1)
1-3: LGTM!The path resolution is correct and provides the necessary test package location for the integration tests.
apps/meteor/app/apps/server/bridges/listeners.js (2)
1-3: LGTM!The new imports and JSDoc annotation support the file streaming implementation appropriately.
Also applies to: 10-13
20-21: LGTM!The case properly routes IPreFileUpload events to the new uploadEvent handler.
apps/meteor/ee/server/apps/orchestrator.js (2)
1-3: LGTM: Standard Node.js imports for temp directory handling.The imports are appropriate for the temp file infrastructure being added.
92-92: LGTM: Temp file path correctly passed to AppManager.The tempFilePath is properly integrated into the AppManager configuration.
packages/apps-engine/src/server/AppManager.ts (2)
57-62: LGTM: Well-documented interface extension.The
tempFilePathaddition toIAppManagerDepsis properly typed and documented.
173-180: LGTM: Clear and documented getter.The
getTempFilePath()method is appropriately documented and provides proper encapsulation.packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts (3)
14-14: LGTM: Import change supports runtime type checking.Changing from a type-only import to named imports is necessary for the runtime check at line 160 (
appPackage.implemented.doesImplement(AppInterface.IPreFileUpload)).
118-142: LGTM: Temp file path properly initialized.The
tempFilePathis correctly retrieved from the AppManager and stored as a readonly field.
157-167: Good security practice: conditional read permissions for temp directory.The conditional granting of read access to
tempFilePathonly when the app implementsIPreFileUploadfollows the principle of least privilege. This ensures apps cannot access temporary files unless they explicitly handle upload events.packages/apps-engine/deno-runtime/handlers/app/handler.ts (3)
20-24: Type-safe array includes check via global augmentation.This augmentation enables TypeScript to narrow the type of
appMethodwhenuploadEvents.includes(appMethod)returns true. While global augmentations can have broad impact, this is a common pattern for achieving type narrowing with readonly arrays.
40-48: LGTM: Centralized result formatting improves consistency.The
formatResulthelper consolidates logging for success and error cases, ensuring uniform debug output across upload events, UIKit interactions, and listener handlers.
50-60: Proper routing order for upload events.Upload events are evaluated before UIKit interactions and listeners, which ensures proper priority. All three branches now use the centralized
formatResultfor consistent logging.packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
40-48: LGTM: Proper temp file handling with automatic cleanup.The
usingsyntax ensures the temp file is automatically closed when the scope exits, preventing resource leaks. Reading the file contents into a Buffer is appropriate for theIFileUploadContext.
59-72: Good error handling with type distinction.The error handling properly distinguishes between
AppsEngineException,JsonRpcError, and generic errors, mapping each to an appropriate JSON-RPC response.Minor: The catch parameter at line 59 lacks a type annotation. Consider
catch (e: unknown)for clarity.packages/apps-engine/tests/server/compiler/AppImplements.spec.ts (1)
14-18: LGTM! Test correctly validates the updated API.The test properly exercises the new API sequence: calling
setImplementsto register an interface, then usingdoesImplementto query implementation status, and finally verifying internal state consistency withgetValues(). Good coverage of both positive (implemented) and negative (not implemented) cases.packages/apps-engine/src/server/compiler/AppImplements.ts (4)
15-19: Method implementation looks correct.The
setImplementsmethod properly validates enum membership with theincheck. However, note that this creates a type contract that expectsAppInterfaceenum values, not strings. Callers passing strings (like inAppPackageParser.tsline 88) will need to cast appropriately.
21-23: LGTM! Good API improvement.The method now returns the implementation status as a boolean, which is more intuitive than the previous void return. The implementation correctly accesses the internal map.
25-27: LGTM! Type-safe accessor with immutability protection.The return type tightening to
Record<AppInterface, boolean>improves type safety, and usingdeepCloneAndFreezeensures the internal state cannot be mutated by callers.
29-31: LGTM! Consistent serialization behavior.The method correctly delegates to
getValues()and maintains type consistency with the newRecord<AppInterface, boolean>return type.packages/apps-engine/src/server/compiler/AppPackageParser.ts (1)
87-89: The code is properly typed. Theinfovariable is explicitly cast asIAppInfoon line 31 (as IAppInfo), and theIAppInfointerface declaresimplementsasArray<AppInterface>, notArray<string>. Therefore,interfaceNameis typed asAppInterfacewhen it enters the loop, and the parameter passed tosetImplements()matches its expected type. SinceAppInterfaceis a string enum, the string values from the JSON naturally align with the enum values, requiring no additional type conversion.Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 22 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/apps/server/bridges/listeners.js">
<violation number="1" location="apps/meteor/app/apps/server/bridges/listeners.js:99">
P1: Temporary file will not be cleaned up if `convertToApp()` or `executeListener()` throws an error. Wrap the operations in a try-finally block to ensure cleanup always occurs, preventing disk space leaks over time.</violation>
</file>
<file name="packages/apps-engine/deno-runtime/handlers/app/handler.ts">
<violation number="1" location="packages/apps-engine/deno-runtime/handlers/app/handler.ts:20">
P2: Global type augmentation modifies `ReadonlyArray.includes` for the entire runtime. Consider using a scoped type guard function instead to avoid affecting other code that may rely on the standard signature.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.js:
- Around line 84-88: The JSDoc incorrectly marks the async function as returning
void; update the @return/@returns annotation to Promise<void> (or remove the
@returns line entirely) so the JSDoc matches the actual async return type for
the documented function in listeners.js.
- Around line 99-103: The inline comment "Execute both events for backward
compatibility" is misleading because only AppInterface.IPreFileUpload is being
executed; update or remove it: change the comment to "Execute this event for
backward compatibility" (or remove the comment) near the call to
this.orch.getManager().getListenerManager().executeListener(AppInterface.IPreFileUpload,
{ file: appFile, path: tmpfile }), ensuring the comment accurately reflects that
only IPreFileUpload is invoked after converting the file via
this.orch.getConverters().get('uploads').convertToApp(file).
🧹 Nitpick comments (1)
packages/apps-engine/deno-runtime/handlers/app/handler.ts (1)
35-43: Add missing semicolon after function expression.Minor style consistency: the function expression should end with a semicolon.
Suggested fix
const formatResult = (result: Defined | JsonRpcError): Defined | JsonRpcError => { if (result instanceof JsonRpcError) { app?.getLogger().debug(`'${appMethod}' was unsuccessful.`, result.message); } else { app?.getLogger().debug(`'${appMethod}' was successfully called! The result is:`, result); } return result; - } + };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/meteor/app/apps/server/bridges/listeners.jspackages/apps-engine/deno-runtime/deno.jsoncpackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps-engine/deno-runtime/deno.jsonc
- packages/apps-engine/deno-runtime/handlers/lib/assertions.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/deno-runtime/handlers/app/handler.tsapps/meteor/app/apps/server/bridges/listeners.js
🧠 Learnings (1)
📚 Learning: 2026-01-08T15:07:47.836Z
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:47.836Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized; they are instantiated and kept in the runtime context. Therefore, runtime assertions can rely on duck-typing checks of App methods (including protected ones like extendConfiguration) as reliable. When reviewing code in this area, prefer duck-typing checks for App behavior in assertions and avoid assuming serialization/deserialization will preserve App state. This guideline applies to files under this directory.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handler.ts
🧬 Code graph analysis (2)
packages/apps-engine/deno-runtime/handlers/app/handler.ts (2)
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (1)
isOneOf(18-20)packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
uploadEvents(14-14)handleUploadEvents(28-73)
apps/meteor/app/apps/server/bridges/listeners.js (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (9)
packages/apps-engine/deno-runtime/handlers/app/handler.ts (4)
18-19: LGTM!Clean imports for the new upload event handling and type-safe membership check utility.
45-47: LGTM!Correct routing for upload events with proper type narrowing via
isOneOf. The placement before the genericcheck/executelistener check (line 53) correctly ensuresexecutePreFileUploadis routed here rather than tohandleListener.
49-55: LGTM!Good refactoring to use
isOneOffor UIKit interactions, maintaining consistency with the new upload events check. The ordering (upload events → UIKit → generic listeners) correctly routes methods from most specific to least specific.
57-111: LGTM!The switch statement for lifecycle methods remains clean and unchanged. The separate logging path (line 94) for these methods is appropriate since
formatResultis only applied to the early-return event handler branches.apps/meteor/app/apps/server/bridges/listeners.js (5)
1-4: LGTM!The new imports are necessary for the temporary file handling workflow in
uploadEvent.
20-21: LGTM!The routing for
IPreFileUploadfollows the established pattern and correctly delegates to the newuploadEventmethod.
104-110: LGTM!The
finallyblock correctly ensures temporary file cleanup occurs regardless of whether the listener succeeds or fails. Logging cleanup failures as warnings (without re-throwing) is the appropriate behavior since the primary operation has already completed.
10-13: Nice improvement!Adding the JSDoc type annotation for the
orchproperty improves IDE support and code documentation.
89-97: The review comment raises an invalid concern. ThegetTempFilePath()method is guaranteed to return a valid directory path because:
- It's a required
stringparameter in the AppManager constructor (line 119 ofpackages/apps-engine/src/server/AppManager.ts)- At server startup (
apps/meteor/ee/server/apps/orchestrator.jslines 75-81), the temp directory is created viafs.mkdirSync(path.join(os.tmpdir(), 'apps-engine-temp'))- The temp path is verified to exist before AppManager instantiation
- The getter simply returns the initialized
this.tempFilePathfield directlyThe code in listeners.js line 92 is safe.
Likely an incorrect or invalid review comment.
d-gubert
force-pushed
the
feat/apps-file-stream
branch
from
5beb59f to
5e033ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts:
- Around line 16-20: The validation in assertIsUpload currently throws a
JsonRpcError.invalidParams referencing the wrong type name ("IUploadDetails");
update the error message to accurately reference "IUpload" (the type this
function asserts) and keep the rest of the payload (err/value) intact so
debugging shows the correct expected type and the failing value; locate this in
the assertIsUpload function and change the message text only.
- Around line 28-29: The function handleUploadEvents currently destructures
params directly; first validate that params is an array with at least one
element and that params[0] is a non-null object before doing const [{ file, path
}]; specifically check Array.isArray(params) && params.length > 0 && typeof
params[0] === "object" && params[0] !== null, then safely extract file and path
(or default them) and return the appropriate JsonRpcError if the check fails so
existing validation logic (and types like IUpload, Defined, JsonRpcError) can
run only on well-formed input.
In @packages/apps-engine/src/server/compiler/AppImplements.ts:
- Around line 7-13: The forEach callback in the AppImplements constructor is
incorrectly typing the iteratee as AppInterface; change the callback parameter
to string (or remove the explicit type) and then cast/validate each key to
AppInterface before assigning into this.implemented; e.g., iterate
Object.keys(AppInterface) with (key: string) and perform a safe cast like const
intf = key as AppInterface (or check membership) before doing
this.implemented[intf] = false to ensure correct typing and avoid misleading
annotations.
🧹 Nitpick comments (3)
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (1)
5-6: Add missing semicolons for consistency.These import statements are missing trailing semicolons, while all other imports in the file include them.
♻️ Proposed fix
-import type { IFileUploadContext } from '@rocket.chat/apps-engine/definition/uploads/IFileUploadContext.ts' -import type { IUpload } from '@rocket.chat/apps-engine/definition/uploads/IUpload.ts' +import type { IFileUploadContext } from '@rocket.chat/apps-engine/definition/uploads/IFileUploadContext.ts'; +import type { IUpload } from '@rocket.chat/apps-engine/definition/uploads/IUpload.ts';apps/meteor/tests/data/apps/helper.ts (1)
42-54: Consider checking response status for robustness.The function rejects on network/transport errors but resolves with
res.body.appregardless of HTTP status orres.body.success. If the server returns an error response (e.g., 400 withsuccess: false), this would resolve withundefinedrather than rejecting.♻️ Optional: Add response validation
export const installLocalTestPackage = (path: string) => new Promise<App>((resolve, reject) => { void request .post(apps()) .set(credentials) .attach('app', path) .end((err, res) => { if (err) { return reject(err); } + if (!res.body.success) { + return reject(new Error(res.body.error || 'Failed to install app')); + } return resolve(res.body.app); }); });apps/meteor/ee/server/apps/orchestrator.js (1)
75-85: Consider cleaning stale temp files on startup.If the server crashes or fails to clean up temp files, they may persist across restarts. While each file has a unique UUID name (from
crypto.randomUUID()inlisteners.js), orphaned files could accumulate over time.♻️ Optional: Clean existing files in temp directory on startup
try { // We call this only once at server startup, so using the synchronous version is fine fs.mkdirSync(tempFilePath); } catch (err) { // If the temp directory already exists, we can continue if (err.code !== 'EEXIST') { throw new Error('Failed to initialize the Apps-Engine', { cause: err }); } + // Clean up any stale temp files from previous runs + try { + const files = fs.readdirSync(tempFilePath); + for (const file of files) { + fs.unlinkSync(path.join(tempFilePath, file)); + } + } catch (cleanupErr) { + // Non-fatal: log and continue + console.warn('Failed to clean up stale temp files:', cleanupErr.message); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zipis excluded by!**/*.zippackages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.changeset/spotty-steaks-notice.mdapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/ee/server/apps/orchestrator.jsapps/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/apps-hooks-file-upload.tspackages/apps-engine/deno-runtime/deno.jsoncpackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/src/definition/uploads/IFileUploadContext.tspackages/apps-engine/src/server/AppManager.tspackages/apps-engine/src/server/ProxiedApp.tspackages/apps-engine/src/server/compiler/AppImplements.tspackages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/src/server/managers/AppListenerManager.tspackages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tspackages/apps-engine/tests/test-data/utilities.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/spotty-steaks-notice.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts
- packages/apps-engine/src/server/ProxiedApp.ts
- apps/meteor/tests/data/apps/app-packages/index.ts
- packages/apps-engine/deno-runtime/handlers/app/handler.ts
- packages/apps-engine/src/server/managers/AppListenerManager.ts
- packages/apps-engine/deno-runtime/handlers/lib/assertions.ts
- packages/apps-engine/src/definition/uploads/IFileUploadContext.ts
- packages/apps-engine/deno-runtime/deno.jsonc
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/src/server/AppManager.tspackages/apps-engine/tests/test-data/utilities.tspackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tsapps/meteor/tests/data/apps/helper.tspackages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tsapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/ee/server/apps/orchestrator.js
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/apps-engine/tests/server/compiler/AppImplements.spec.ts
🧠 Learnings (25)
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
packages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.tspackages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/apps-engine/tests/test-data/utilities.tsapps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2026-01-08T15:07:47.836Z
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:47.836Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized; they are instantiated and kept in the runtime context. Therefore, runtime assertions can rely on duck-typing checks of App methods (including protected ones like extendConfiguration) as reliable. When reviewing code in this area, prefer duck-typing checks for App behavior in assertions and avoid assuming serialization/deserialization will preserve App state. This guideline applies to files under this directory.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/data/apps/helper.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/data/apps/helper.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2026-01-08T15:03:45.071Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:45.071Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
apps/meteor/tests/data/apps/app-packages/README.md
📚 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:
apps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/tests/data/apps/app-packages/README.md
🧬 Code graph analysis (7)
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (3)
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (3)
isRecord(4-12)assertAppAvailable(22-26)assertHandlerFunction(29-33)packages/apps-engine/src/definition/uploads/IFileUploadContext.ts (1)
IFileUploadContext(8-11)packages/apps-engine/deno-runtime/lib/accessors/mod.ts (1)
AppAccessorsInstance(322-322)
apps/meteor/tests/data/apps/helper.ts (1)
apps/meteor/tests/data/apps/apps-data.ts (1)
apps(10-12)
packages/apps-engine/src/server/compiler/AppImplements.ts (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts (3)
apps/meteor/tests/data/apps/helper.ts (2)
cleanupApps(24-27)installLocalTestPackage(42-54)apps/meteor/tests/data/apps/app-packages/index.ts (1)
appImplementsIPreFileUpload(3-3)packages/core-services/src/index.ts (1)
api(55-55)
packages/apps-engine/tests/server/compiler/AppImplements.spec.ts (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
apps/meteor/app/apps/server/bridges/listeners.js (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
apps/meteor/ee/server/apps/orchestrator.js (1)
packages/apps-engine/src/server/AppManager.ts (1)
AppManager(71-1232)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
apps/meteor/ee/server/apps/orchestrator.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🪛 markdownlint-cli2 (0.18.1)
apps/meteor/tests/data/apps/app-packages/README.md
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
16-16: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ 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 comments (13)
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (1)
40-58: Well-implemented resource management and handler invocation.The use of the
usingkeyword for explicit resource management ensures the temporary file is properly closed, and the handler invocation correctly passes all required accessors. The project targets Deno 1.43.5 and TypeScript 5.9.3, which fully support this feature.apps/meteor/tests/data/apps/app-packages/README.md (1)
1-54: Clear and helpful documentation for test app packages.The README provides useful instructions for using pre-built test app packages, including a practical code example and details about the available IPreFileUpload test app behavior. Based on learnings, formatting concerns are handled by automated linting tools.
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts (1)
1-56: Well-structured end-to-end tests for IPreFileUpload event.The tests properly verify both the rejection and acceptance paths of the IPreFileUpload hook. The setup/teardown correctly cleans up apps and rooms, and the assertions validate the expected API responses. Good use of conditional test execution for EE-only features.
apps/meteor/app/apps/server/bridges/listeners.js (1)
89-111: Solid implementation with proper temp file lifecycle management.The
uploadEventmethod correctly:
- Uses
crypto.randomUUID()for secure, unique temp file names- Wraps the listener execution in try/finally to ensure cleanup
- Logs errors appropriately (error for write failures, warn for cleanup failures)
- Preserves the original error as
causewhen rethrowingThe cleanup-on-failure approach in the finally block is a good pattern for resource management.
packages/apps-engine/src/server/AppManager.ts (1)
57-62: Clean extension of AppManager to support temp file path configuration.The implementation properly:
- Documents the
tempFilePathrequirement inIAppManagerDeps- Stores it as a private readonly field
- Exposes it via a well-documented getter
The validation of the temp path is appropriately delegated to the orchestrator during initialization.
Also applies to: 115-116, 119-119, 149-150, 173-180
packages/apps-engine/tests/test-data/utilities.ts (1)
133-133: Appropriate mock for the newgetTempFilePathmethod.The deterministic return value is suitable for test scenarios where the actual path doesn't matter.
apps/meteor/ee/server/apps/orchestrator.js (1)
75-93: Proper temp directory initialization with appropriate error handling.The implementation correctly:
- Uses
os.tmpdir()for platform-appropriate temp location- Handles the
EEXISTcase gracefully (directory already exists)- Preserves error context with the
causeoption- Uses synchronous call appropriately for startup-time initialization
packages/apps-engine/tests/server/compiler/AppImplements.spec.ts (1)
14-18: LGTM!The test correctly exercises the new API, validating both
setImplementsanddoesImplementmethods with properAppInterfaceenum values.packages/apps-engine/src/server/compiler/AppImplements.ts (4)
21-23: LGTM!The new
doesImplementmethod provides a clear query API, properly separated from thesetImplementssetter method.
25-27: LGTM!The return type correctly reflects the stronger typing of the
implementedfield.
29-31: LGTM!The return type is correctly aligned with
getValues().
15-19: Theinoperator validation works correctly with this string enum. SinceAppInterfaceis a string enum where each member name equals its value (e.g.,IPostMessageSentToBot = 'IPostMessageSentToBot'), checkingint in AppInterfaceeffectively validates that the string is a valid enum member by checking it against the property keys. The implementation is correct.packages/apps-engine/src/server/compiler/AppPackageParser.ts (1)
88-88: No changes needed. The type compatibility is already correct in the codebase.The
interfaceNamevariable is properly typed asAppInterface(notstring) becauseinfo.implementsis declared asArray<AppInterface>in theIAppInfointerface. WhenforEachiterates over this array, each element is typed asAppInterface, which perfectly matches the parameter type expected bysetImplements(int: AppInterface). The runtime validation insetImplementsprovides additional safety. There is no type mismatch or TypeScript compilation issue.Likely an incorrect or invalid review comment.
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
Outdated
Show resolved
Hide resolved
d-gubert
force-pushed
the
feat/apps-file-stream
branch
from
5e033ba to
27e5596
Compare
|
@coderabbitai ignore Your job here is done 🐰 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts:
- Line 5: The import statement for IFileUploadContext in handleUploadEvents.ts
is missing a trailing semicolon; update the import line "import type {
IFileUploadContext } from
'@rocket.chat/apps-engine/definition/uploads/IFileUploadContext.ts'" to include
a semicolon at the end so it matches the file's import formatting and maintains
consistency.
- Line 59: In handleUploadEvents (in the catch block inside the try/catch around
the upload handling), change the malformed catch clause from "catch(e)" to
"catch (e)" to add a space after the catch keyword for consistent
TypeScript/JavaScript formatting and linting; update the catch in the same
function/block where the error is currently handled to match the project's
formatting conventions.
🧹 Nitpick comments (4)
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (1)
40-49: Initialize context variable to prevent potential undefined usage.The
contextvariable is declared but not initialized. If the switch statement were to be extended with additional cases in the future and one case fails to assigncontext, this would lead to a runtime error whencontextis passed to the handler function. While currently there's only one case, defensive initialization improves type safety.♻️ Proposed fix
Option 1: Add an explicit default case that throws an error:
using tempFile = await Deno.open(path, { read: true, create: false }); - let context: IFileUploadContext; switch (method) { case 'executePreFileUpload': { const fileContents = await toArrayBuffer(tempFile.readable); - context = { file, content: Buffer.from(fileContents) }; + const context: IFileUploadContext = { file, content: Buffer.from(fileContents) }; - break; + return await handlerFunction.call( + app, + context, + AppAccessorsInstance.getReader(), + AppAccessorsInstance.getHttp(), + AppAccessorsInstance.getPersistence(), + AppAccessorsInstance.getModifier(), + ); } + default: + throw JsonRpcError.invalidParams({ err: 'Unknown upload event method', value: method }); } - - return await handlerFunction.call( - app, - context, - AppAccessorsInstance.getReader(), - AppAccessorsInstance.getHttp(), - AppAccessorsInstance.getPersistence(), - AppAccessorsInstance.getModifier(), - );Option 2: Move the handler invocation inside each case to eliminate the shared variable.
apps/meteor/app/apps/server/bridges/listeners.js (3)
10-13: Consider omitting implementation comments per coding guidelines.The JSDoc type annotation provides useful type information, but the coding guidelines suggest avoiding code comments in implementation. Since this is type documentation in a
.jsfile, it provides value, so this is optional.
84-88: Consider standard JSDoc convention.Minor: JSDoc typically uses
@returnsinstead of@return. Also, as mentioned earlier, the coding guidelines suggest avoiding implementation comments.
89-111: Add runtime validation for payload structure.The temp directory is properly initialized at server startup with error handling for existing directories. However,
uploadEventdestructurespayload.fileandpayload.contentwithout runtime validation. While the Deno runtime handlers validate these fields, the Meteor listener bridge does not. Add a guard to fail fast with a clear error if the payload is malformed:if (!payload?.file || !payload?.content) { throw new Error('Invalid upload event payload: missing file or content'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
apps/meteor/app/apps/server/bridges/listeners.jspackages/apps-engine/deno-runtime/deno.jsoncpackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps-engine/deno-runtime/handlers/lib/assertions.ts
- packages/apps-engine/deno-runtime/deno.jsonc
🧰 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-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tsapps/meteor/app/apps/server/bridges/listeners.js
🧠 Learnings (2)
📚 Learning: 2026-01-08T15:07:47.836Z
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:47.836Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized; they are instantiated and kept in the runtime context. Therefore, runtime assertions can rely on duck-typing checks of App methods (including protected ones like extendConfiguration) as reliable. When reviewing code in this area, prefer duck-typing checks for App behavior in assertions and avoid assuming serialization/deserialization will preserve App state. This guideline applies to files under this directory.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
🧬 Code graph analysis (1)
packages/apps-engine/deno-runtime/handlers/app/handler.ts (2)
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (1)
isOneOf(18-20)packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
uploadEvents(14-14)handleUploadEvents(28-73)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
⏰ 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 comments (7)
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
16-20: LGTM! Duck-typing validation aligns with codebase patterns.The assertion function correctly uses duck-typing to validate the IUpload structure, which is the preferred approach in this runtime context.
Based on learnings, App objects are never serialized in packages/apps-engine/deno-runtime, so duck-typing checks are reliable.
40-40: LGTM! Proper resource cleanup with explicit disposal.The use of the
usingkeyword ensures automatic cleanup of the file handle, preventing resource leaks.packages/apps-engine/deno-runtime/handlers/app/handler.ts (3)
18-19: LGTM! Clean integration of upload event handling.The new imports properly integrate the upload event handler and the
isOneOfutility for type-safe routing.
35-43: LGTM! Excellent refactor to centralize logging logic.The
formatResulthelper eliminates code duplication and provides consistent logging for all async handler branches. This improves maintainability.
45-55: LGTM! Clear and consistent routing pattern.The routing logic is well-structured with:
- Type-safe method checking using
isOneOf- Consistent use of the
formatResultwrapper for logging- Clear separation of concerns between upload events, UIKit interactions, and listeners
apps/meteor/app/apps/server/bridges/listeners.js (2)
1-4: LGTM! Required imports for temp file handling.The new imports (
crypto,fs,path) are appropriate for the temp file approach. The Biome parse errors about "import outside module" are false positives—this file is part of a module-based project structure.
20-21: LGTM! Correct routing for IPreFileUpload event.The switch case correctly maps the
IPreFileUploadevent to the newuploadEventhandler, consistent with the existing pattern.
|
Tip For best results, initiate chat on the files or code changes. @d-gubert, understood! Thank you for the feedback. Feel free to mention me if you need anything else in the future. 🐰 |
…8068 Squashed commit of the following: commit 27e5596 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Thu Jan 8 12:38:04 2026 -0300 fix: apply review suggestions commit ea7ef78 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Thu Jan 8 10:53:50 2026 -0300 add changeset commit 356b312 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:43:02 2026 -0300 refactor(deno): result formatting in app method handler commit bac0f59 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:41:53 2026 -0300 revert: we'll introduce IPreFileUploadStream later commit a1d1c3a Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:12:42 2026 -0300 test: end-to-end tests for the IPreFileUpload event commit 1152f31 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 19:52:55 2026 -0300 fix: runtime event handling commit 25a7c7b Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:40:17 2026 -0300 chore: add deprecated tag on AppInterface commit 3aef174 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:39:18 2026 -0300 chore: improve error printing on receiving json rpc errors commit c655f73 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:38:04 2026 -0300 feat: adapt listener.js to always execute both upload events commit 8c445e9 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:36:04 2026 -0300 feat: handle temp file in upload events on the Deno runtime commit 2489e64 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:22:48 2026 -0300 feat: Controller spawns runtime process with read permission when app hooks into file upload commit 2a688a9 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:21:18 2026 -0300 feat: AppManager gets temp file path on construction commit 1fff450 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:04:24 2026 -0300 refactor: AppImplements for better hook registration check commit adc2ea8 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 18:55:26 2026 -0300 fix: IPreFileUploadStream docstring and export commit c5731da Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Dec 31 20:01:22 2025 -0300 wip: add new handler for uploads to runtime commit 82d1544 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Dec 29 23:18:56 2025 -0300 wip: create temp file on upload event commit d3da123 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Dec 29 23:18:37 2025 -0300 feat: add new upload event types
…8068 Squashed commit of the following: commit 27e5596 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Thu Jan 8 12:38:04 2026 -0300 fix: apply review suggestions commit ea7ef78 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Thu Jan 8 10:53:50 2026 -0300 add changeset commit 356b312 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:43:02 2026 -0300 refactor(deno): result formatting in app method handler commit bac0f59 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:41:53 2026 -0300 revert: we'll introduce IPreFileUploadStream later commit a1d1c3a Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:12:42 2026 -0300 test: end-to-end tests for the IPreFileUpload event commit 1152f31 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 19:52:55 2026 -0300 fix: runtime event handling commit 25a7c7b Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:40:17 2026 -0300 chore: add deprecated tag on AppInterface commit 3aef174 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:39:18 2026 -0300 chore: improve error printing on receiving json rpc errors commit c655f73 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:38:04 2026 -0300 feat: adapt listener.js to always execute both upload events commit 8c445e9 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:36:04 2026 -0300 feat: handle temp file in upload events on the Deno runtime commit 2489e64 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:22:48 2026 -0300 feat: Controller spawns runtime process with read permission when app hooks into file upload commit 2a688a9 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:21:18 2026 -0300 feat: AppManager gets temp file path on construction commit 1fff450 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:04:24 2026 -0300 refactor: AppImplements for better hook registration check commit adc2ea8 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 18:55:26 2026 -0300 fix: IPreFileUploadStream docstring and export commit c5731da Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Dec 31 20:01:22 2025 -0300 wip: add new handler for uploads to runtime commit 82d1544 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Dec 29 23:18:56 2025 -0300 wip: create temp file on upload event commit d3da123 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Dec 29 23:18:37 2025 -0300 feat: add new upload event types
…8068 Squashed commit of the following: commit 27e5596 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Thu Jan 8 12:38:04 2026 -0300 fix: apply review suggestions commit ea7ef78 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Thu Jan 8 10:53:50 2026 -0300 add changeset commit 356b312 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:43:02 2026 -0300 refactor(deno): result formatting in app method handler commit bac0f59 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:41:53 2026 -0300 revert: we'll introduce IPreFileUploadStream later commit a1d1c3a Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 20:12:42 2026 -0300 test: end-to-end tests for the IPreFileUpload event commit 1152f31 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Jan 7 19:52:55 2026 -0300 fix: runtime event handling commit 25a7c7b Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:40:17 2026 -0300 chore: add deprecated tag on AppInterface commit 3aef174 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:39:18 2026 -0300 chore: improve error printing on receiving json rpc errors commit c655f73 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:38:04 2026 -0300 feat: adapt listener.js to always execute both upload events commit 8c445e9 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:36:04 2026 -0300 feat: handle temp file in upload events on the Deno runtime commit 2489e64 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:22:48 2026 -0300 feat: Controller spawns runtime process with read permission when app hooks into file upload commit 2a688a9 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:21:18 2026 -0300 feat: AppManager gets temp file path on construction commit 1fff450 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 19:04:24 2026 -0300 refactor: AppImplements for better hook registration check commit adc2ea8 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Jan 5 18:55:26 2026 -0300 fix: IPreFileUploadStream docstring and export commit c5731da Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Wed Dec 31 20:01:22 2025 -0300 wip: add new handler for uploads to runtime commit 82d1544 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Dec 29 23:18:56 2025 -0300 wip: create temp file on upload event commit d3da123 Author: Douglas Gubert <douglas.gubert@gmail.com> Date: Mon Dec 29 23:18:37 2025 -0300 feat: add new upload event types
d-gubert
force-pushed
the
feat/apps-file-stream
branch
from
870f3a3 to
4a84a2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts`:
- Around line 59-72: The catch block in handleUploadEvents.ts accesses e.name,
e.message and e.code without narrowing the caught unknown; update the catch to
treat e as unknown and defensively narrow before property access: first check
typeof e === 'object' && e !== null, then use 'in' or property checks to detect
a string 'name' (compare with AppsEngineException.name) and a 'message' string,
and only then construct a JsonRpcError using those values; for the JsonRpcError
instanceof check, first ensure e is an object/non-null before using instanceof
JsonRpcError, and for the final internalError fallback, use a safe message (e
instanceof Error ? e.message : String(e)) and include code only if the narrowed
object has a numeric code property. Ensure references to AppsEngineException,
JsonRpcError, and JsonRpcError.internalError remain the same.
🧹 Nitpick comments (1)
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (1)
18-20: Type narrowing relies on unchecked cast.The
value as Tcast allowsincludes()to compile but doesn't verify membership before the cast. This is functionally correct sinceincludes()performs the actual runtime check, but be aware that ifThas constraints not expressible at runtime (e.g., branded types), this could produce false positives.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zipis excluded by!**/*.zippackages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.changeset/spotty-steaks-notice.mdapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/ee/server/apps/orchestrator.jsapps/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/apps-hooks-file-upload.tspackages/apps-engine/deno-runtime/deno.jsoncpackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/deno-runtime/handlers/app/handler.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/src/definition/uploads/IFileUploadContext.tspackages/apps-engine/src/server/AppManager.tspackages/apps-engine/src/server/ProxiedApp.tspackages/apps-engine/src/server/compiler/AppImplements.tspackages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/src/server/managers/AppListenerManager.tspackages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.tspackages/apps-engine/tests/server/compiler/AppImplements.spec.tspackages/apps-engine/tests/test-data/utilities.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/meteor/tests/data/apps/helper.ts
- packages/apps-engine/deno-runtime/handlers/app/handler.ts
- packages/apps-engine/tests/server/compiler/AppImplements.spec.ts
- .changeset/spotty-steaks-notice.md
- apps/meteor/tests/data/apps/app-packages/index.ts
- packages/apps-engine/src/server/AppManager.ts
- packages/apps-engine/src/server/compiler/AppPackageParser.ts
- packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts
- packages/apps-engine/tests/test-data/utilities.ts
- packages/apps-engine/src/definition/uploads/IFileUploadContext.ts
- packages/apps-engine/deno-runtime/deno.jsonc
- packages/apps-engine/src/server/ProxiedApp.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/src/server/managers/AppListenerManager.tspackages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/src/server/compiler/AppImplements.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/ee/server/apps/orchestrator.js
🧠 Learnings (27)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2026-01-08T15:03:59.621Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2026-01-08T15:07:47.836Z
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:47.836Z
Learning: In packages/apps-engine/deno-runtime, App objects are never serialized; they are instantiated and kept in the runtime context. Therefore, runtime assertions can rely on duck-typing checks of App methods (including protected ones like extendConfiguration) as reliable. When reviewing code in this area, prefer duck-typing checks for App behavior in assertions and avoid assuming serialization/deserialization will preserve App state. This guideline applies to files under this directory.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
packages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
packages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-10-16T21:09:51.816Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/keychain.ts:148-156
Timestamp: 2025-10-16T21:09:51.816Z
Learning: In the RocketChat/Rocket.Chat repository, only platforms with native crypto.randomUUID() support are targeted, so fallback implementations for crypto.randomUUID() are not required in E2EE or cryptographic code.
Applied to files:
packages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/app/apps/server/bridges/listeners.ts
📚 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-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
packages/apps-engine/src/server/compiler/AppImplements.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/apps-engine/src/server/compiler/AppImplements.tsapps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 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-engine/deno-runtime/handlers/lib/assertions.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.tsapps/meteor/tests/data/apps/app-packages/README.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/tests/data/apps/app-packages/README.md
🧬 Code graph analysis (4)
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
packages/apps-engine/src/definition/uploads/IFileUploadContext.ts (1)
IFileUploadInternalContext(3-6)
packages/apps-engine/src/server/compiler/AppImplements.ts (1)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)
apps/meteor/app/apps/server/bridges/listeners.ts (2)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)packages/apps-engine/src/definition/uploads/IPreFileUpload.ts (1)
IPreFileUpload(18-26)
apps/meteor/ee/server/apps/orchestrator.js (1)
packages/apps-engine/src/server/AppManager.ts (1)
AppManager(71-1232)
🪛 Biome (2.1.2)
apps/meteor/ee/server/apps/orchestrator.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🪛 markdownlint-cli2 (0.18.1)
apps/meteor/tests/data/apps/app-packages/README.md
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
16-16: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ 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 comments (20)
packages/apps-engine/src/server/compiler/AppImplements.ts (3)
5-5: Improved type safety withRecord<AppInterface, boolean>.The change from loose string-keyed object to
Record<AppInterface, boolean>provides better compile-time guarantees and editor autocompletion. The initialization pattern correctly populates all enum keys withfalse.Also applies to: 8-12
15-23: Clear API separation between setter and query methods.Splitting the original
doesImplementintosetImplements(mutation) anddoesImplement(query) follows the command-query separation principle. The defensive guard on line 16 appropriately validates the input before mutation.
25-31: LGTM on return type updates and immutability pattern.The updated return types align with the new field typing, and delegating
toJSON()togetValues()maintains DRY principles while preserving the deep-freeze immutability guarantee.apps/meteor/ee/server/apps/orchestrator.js (3)
1-4: LGTM!The imports for
fs,os, andpathare appropriate for the temp directory creation logic. The static analysis warnings about "import declaration outside of a module" are false positives—this file consistently uses ES module syntax throughout.
75-85: LGTM!The temp directory initialization is well-implemented:
- Cross-platform path construction with
path.joinandos.tmpdir()- Synchronous operation is appropriate at startup (as the comment explains)
- EEXIST handling correctly allows idempotent initialization
- Error wrapping with
causepreserves the original error for debugging
87-93: LGTM!The
tempFilePathis correctly passed toAppManagervia its dependency configuration, enabling the runtime to access the temp storage path for IPreFileUpload events.packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
28-28: Type migration toIFileUploadInternalContextlooks correct.The import and type references are consistently updated throughout the file to use
IFileUploadInternalContext(which carriesfilemetadata andpathto the temp file) instead of the previousIFileUploadContextthat embedded the file buffer. This aligns with the PR objective to reduce memory pressure by passing file paths rather than serialized buffer contents.Also applies to: 207-210, 451-451, 1173-1179
apps/meteor/app/apps/server/bridges/listeners.ts (4)
1-3: Imports for temp file handling added appropriately.The
crypto,fs, andpathimports support secure temp file generation and cleanup. Usingcrypto.randomUUID()for temp filenames prevents predictable naming attacks.Also applies to: 14-14
172-183: Type definitions for file upload events are well-structured.The
HandleFileUploadEventtype clearly defines the payload as[IUpload, Buffer], and theHandleEventunion is properly extended to include the new event type.
252-284: Temp file handling implementation is solid.The
uploadEventmethod properly addresses the memory optimization goal:
- Secure temp file naming via
crypto.randomUUID()prevents path traversal and collision issues- Proper error propagation - write failures are logged with context and re-thrown with cause chaining
- Guaranteed cleanup via
finallyblock ensures the temp file is deleted regardless of success or failure in the listener execution- Graceful cleanup failure handling - unlink errors are logged as warnings rather than thrown, which is appropriate since the primary operation has already completed
191-192: Event routing correctly delegates to the specialized handler.apps/meteor/tests/data/apps/app-packages/README.md (1)
1-54: Documentation for test app packages is clear and helpful.The README provides useful guidance for test authors:
- Clear usage example with
installLocalTestPackage- Documents the available test app (IPreFileUpload handler)
- Includes the app source code for reference
apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts (2)
12-24: Test setup and teardown are well-structured.The test properly:
- Gates execution behind
IS_EE- Cleans up apps before installing the test package (avoiding state pollution)
- Creates a unique room with timestamp suffix for isolation
- Cleans up both room and apps in
after()
26-54: Test cases provide good coverage of the file upload hook behavior.The tests validate:
- Rejection path: Files matching the app's rejection pattern are blocked with the expected error response, and the error message contains the file contents (proving the app successfully read the temp file)
- Happy path: Non-matching files are allowed through without interference
This effectively validates that the temp file mechanism works correctly for the IPreFileUpload event.
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (2)
4-12: LGTM! Robust plain-object detection.The prototype-based check correctly identifies plain objects while excluding arrays,
null, and class instances. This is a solid implementation.
22-33: Assertion functions are well-structured.Both
assertAppAvailableandassertHandlerFunctionfollow the correct assertion pattern with appropriate error types. Based on learnings, the duck-typing check onextendConfigurationis reliable since App objects are never serialized in this runtime context.packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (4)
14-14: LGTM! Const assertion for type safety.The
as constassertion correctly narrows the array to a readonly tuple, enabling precise typing for themethodparameter.
16-26: Assertion helpers are appropriate for the validation needs.Both
assertIsUploadandassertStringprovide clear error messages with the actual value for debugging. The IUpload validation checks the nesteduserandroomobjects as expected.
40-48: Good use of explicit resource management withusing.The
usingkeyword ensures the file handle is properly closed even if an exception occurs during reading. Reading the file contents viatoArrayBufferaligns with the PR objective of avoiding stdio transfer for large files.
51-58: Handler invocation and accessor provisioning look correct.The handler is called with the appropriate context and all required accessors (Reader, Http, Persistence, Modifier) matching the expected signature for
executePreFileUpload.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
… hooks into file upload
d-gubert
force-pushed
the
feat/apps-file-stream
branch
from
4a84a2a to
0f7416c
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
To transfer the file contents from the server to the runtime subprocess, the engine needs to package the buffer in a JSON RPC message, serialize it using msgpack and deno then unserializes the contents on its side.
This means the whole file buffer is duplicated THRICE in the host's memory: 1)
jsonrpc-literuns the Buffer throughJSON.stringifyto validate they can be serialized (a problem for the future), which creates a string on the Node process with the Buffer's contents encoded as base64 (larger than the original), and then immediately discards it; 2)msgpackcopies the Buffer's contents to the encoder's own internal buffer while serializing the message; 3)msgpackdecodes the message as an object as it arrives on the Deno process, allocating it all into memory.So essentially, for each app that is subscribed to the
IPreFileUploadevent, the uploaded file's full contents are loaded into memory ~3.3 additional times (base64 encoding is estimated to take ~30% more bytes to represent the original binary content)In this PR, we completely switch up the strategy. When Rocket.Chat triggers the upload event on the Apps-Engine, we stream the file's contents to a temporary file in the file system, and send over to Deno just the file path. Deno is then responsible for reading the file when needed. For the
IPreFileUploadevent, this means we stop base64 encoding the contents, and stop transfering large contents over stdio, reducing pressure on that stream.Issue(s)
SUP-963
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.