fix(apps): logs are lost in nested requests#38374
Conversation
🦋 Changeset detectedLatest commit: b5015cd 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 |
|
Looks like this PR is ready to merge! 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThread RequestContext/request-scoped logger through the Deno apps runtime, add wrapAppForRequest / wrapComposedApp proxies to bind per-request this/context, update handlers and tests to use the request-scoped logger, and add an E2E test plus test app exercising nested-request log collection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler as API Handler
participant ReqCtx as RequestContext
participant WrappedApp as wrapAppForRequest
participant App as App Code
participant Logger as Req-Scoped Logger
Client->>API_Handler: JSON‑RPC request
API_Handler->>ReqCtx: create RequestContext (includes logger)
API_Handler->>WrappedApp: invoke method via wrapAppForRequest(app, ReqCtx)
WrappedApp->>App: call method (this -> proxied app)
App->>Logger: log via this.logger (proxied to ReqCtx.logger)
App->>API_Handler: trigger nested RPC/action
API_Handler->>ReqCtx: create/reuse nested RequestContext
API_Handler->>WrappedApp: invoke nested handler with wrapped ReqCtx
App->>Logger: nested logs (routed to request-scoped logger)
API_Handler-->>Client: respond (logs remain scoped)
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. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38374 +/- ##
===========================================
- Coverage 70.53% 70.48% -0.05%
===========================================
Files 3176 3176
Lines 111142 111142
Branches 20047 20043 -4
===========================================
- Hits 78394 78343 -51
- Misses 30701 30753 +52
+ Partials 2047 2046 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/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", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/18 18:05", "02/18 18:46 (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.11]
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.11]
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.12]
line "omnichannel-transcript-service" [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.13, 0.13, 0.13]
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.11]
line "queue-worker-service" [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.13, 0.13, 0.13]
line "rocketchat" [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.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 18 Feb 2026 18:46:31 GMT |
d-gubert
force-pushed
the
feat/apps-logs-redirect
branch
from
585160e to
1090894
Compare
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
1cffed5 to
3ae098b
Compare
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
3ae098b to
6a65244
Compare
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
2 times, most recently
from
139f9e9 to
88bb77d
Compare
![coderabbitai[bot]](http://www.gproxx.com/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts`:
- Around line 17-19: The proxy handler incorrectly returns the raw target when
property === 'getOriginalRef', so accessing proxy.getOriginalRef yields the app
object instead of a callable function; change the handler in wrapAppForRequest
(the proxy creation for Reversible<T>) to return a function that when called
returns the original target (e.g., return () => target) so
proxy.getOriginalRef() is callable and returns the original app object.
🧹 Nitpick comments (3)
packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts (1)
2-2: Unused import:Definedis imported but not used.The
Definedtype is imported fromjsonrpc-litebut is not referenced anywhere in the function body. The function returnsnullon success andJsonRpcErroron failure.Suggested fix
-import { Defined, JsonRpcError } from 'jsonrpc-lite'; +import { JsonRpcError } from 'jsonrpc-lite';Also update the return type annotation if desired:
-export default async function handleScheduler(request: RequestContext): Promise<Defined | JsonRpcError> { +export default async function handleScheduler(request: RequestContext): Promise<null | JsonRpcError> {packages/apps-engine/deno-runtime/handlers/videoconference-handler.ts (1)
48-50: Consider usinglogger.errorfor failure logging instead oflogger.debug.Other handlers like
scheduler-handler.ts(line 45-46) uselogger.errorfor failure cases. Usingdebuglevel here may cause failures to be missed in production logs when debug logging is disabled.Suggested fix
} catch (e) { - logger.debug(`Video Conference Provider's ${methodName} was unsuccessful.`); + logger.error(`Video Conference Provider's ${methodName} was unsuccessful.`); return new JsonRpcError(e.message, -32000); }packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts (1)
140-142: Minor: Method name in mock request may not reflect the actual operation.The test for
executePreviewItemuses method'slashcommand:executor-and-preview:previewer'in the mock request, but the actual method being tested isexecutePreviewItem. While this likely doesn't affect test correctness (since the method string is primarily for logging context), it could be confusing for future maintainers.Suggested fix
- const mockRequest = createMockRequest({ method: 'slashcommand:executor-and-preview:previewer', params: [mockContext] }); + const mockRequest = createMockRequest({ method: 'slashcommand:executor-and-preview:executePreviewItem', params: [mockPreviewItem, mockContext] });
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
3 times, most recently
from
4a8a5b8 to
a87128f
Compare
![cubic-dev-ai[bot]](http://www.gproxx.com/https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 36 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="packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts">
<violation number="1" location="packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts:103">
P3: The mock request for executePreviewItem uses the previewer method name and omits the preview item parameter. This test won't mirror the real RPC call and may miss regressions tied to executePreviewItem request context/logging. Use the executePreviewItem method and include both params.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts
Show resolved
Hide resolved
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
ee5711e to
ef8bb22
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts`:
- Around line 140-142: The test constructs mockRequest with an incorrect method
string; update the createMockRequest call that builds mockRequest so its method
matches the handler being tested (use the executePreviewItem method name instead
of 'slashcommand:executor-and-preview:previewer') so handlePreviewItem actually
receives a request representing executePreviewItem; locate the createMockRequest
invocation that creates mockRequest and change the method to the
executePreviewItem identifier used by handlePreviewItem and executePreviewItem
while leaving the params ([mockContext]) and the rest of the test
(mockCommandExecutorAndPreview, mockPreviewItem, mockContext) unchanged.
- Around line 103-105: The test calls handleExecutor referencing mockPreviewItem
before it's declared and uses the wrong method string; move the mockPreviewItem
declaration (the object defined in the later test) to a shared scope above both
tests so it's available when createMockRequest is called, and change the method
string in the failing test's createMockRequest call from
'slashcommand:executor-and-preview:executePreviewItem' to
'slashcommand:executor-and-preview:previewer' so the previewer test uses the
correct method when invoking handleExecutor({ AppAccessorsInstance:
mockAppAccessors, request: mockRequest }, mockCommandExecutorAndPreview,
'previewer', [mockContext]).
🧹 Nitpick comments (3)
apps/meteor/tests/data/apps/app-packages/README.md (1)
67-98: Consider adding import statements for consistency.The code example omits import statements, while the earlier IPreFileUpload example includes them (lines 32-37). Adding imports would improve consistency and make the example more complete for reference.
📦 Optional: Add imports for completeness
Add these imports at the beginning of the code example:
```typescript +import { + IHttp, + IModify, + IPersistence, + IRead, +} from '@rocket.chat/apps-engine/definition/accessors'; +import { App } from '@rocket.chat/apps-engine/definition/App'; +import { IConfigurationExtend, IEnvironmentRead } from '@rocket.chat/apps-engine/definition/accessors'; +import { IPostMessageSent } from '@rocket.chat/apps-engine/definition/messages'; +import { IMessage } from '@rocket.chat/apps-engine/definition/messages'; +import { ISlashCommand, SlashCommandContext } from '@rocket.chat/apps-engine/definition/slashcommands'; +import { IApp } from '@rocket.chat/apps-engine/definition/IApp'; + export class NestedRequestsApp extends App implements IPostMessageSent {Also, minor spacing fix on line 75:
- public command= 'nest'; + public command = 'nest';packages/apps-engine/deno-runtime/lib/messenger.ts (1)
152-163: Consider defensive access to logger insuccessResponse.While
RequestContexttype requiresloggerincontext,successResponseaccesses it without optional chaining (lines 154, 156), unlike the defensive approach inerrorResponse. If a malformed context is ever passed, this would throw at runtime.Given that
errorResponsealready uses optional chaining for resilience, consider applying the same pattern here for consistency and defensive coding:♻️ Suggested defensive change
export async function successResponse({ id, result }: SuccessResponseDescriptor, req: RequestContext): Promise<void> { const payload = { value: result } as Record<string, unknown>; - const { logger } = req.context; + const { logger } = req?.context || {}; - if (logger.hasEntries()) { + if (logger?.hasEntries()) { payload.logs = logger.getLogs(); }packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
44-46: Consider usinglogger.errorfor the failure case.The error path uses
logger.debugwhich may make failures harder to spot in logs. Other handlers likescheduler-handler.ts(line 45-46) uselogger.errorfor error cases.♻️ Suggested change
} catch (e) { - logger.debug(`${path}'s ${call} was unsuccessful.`); + logger.error(`${path}'s ${call} was unsuccessful.`); return new JsonRpcError(e.message || 'Internal server error', -32000); }
packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts
Outdated
Show resolved
Hide resolved
packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.ts
Outdated
Show resolved
Hide resolved
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
ef8bb22 to
5c7190f
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/handler.ts`:
- Around line 52-60: The code calls appMethod.startsWith(...) without ensuring
appMethod is defined, causing a crash when method lacks ':'; update the listener
branch in the conditional that currently references appMethod.startsWith to
first guard that appMethod is a string (e.g., check appMethod !== undefined &&
typeof appMethod === 'string' or simply appMethod) before calling startsWith;
locate the conditional around uploadEvents, uikitInteractions,
handleUploadEvents, handleUIKitInteraction and handleListener and change the
final else-if to only evaluate startsWith when appMethod is present.
🧹 Nitpick comments (2)
apps/meteor/tests/data/apps/app-packages/README.md (1)
75-75: Minor formatting inconsistency in code snippet.There's a missing space before
=in the property declaration.Suggested fix
- public command= 'nest'; + public command = 'nest';packages/apps-engine/deno-runtime/lib/tests/messenger.test.ts (1)
7-9: Prefer type-only imports for RequestContext and JsonRpc.
Both are used exclusively in type positions (annotations and type casts), soimport typeclarifies intent and avoids unnecessary value imports.♻️ Suggested change
-import { RequestContext } from '../requestContext.ts'; -import { JsonRpc } from 'jsonrpc-lite'; +import type { RequestContext } from '../requestContext.ts'; +import type { JsonRpc } from 'jsonrpc-lite';
packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts
Outdated
Show resolved
Hide resolved
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
5c7190f to
64200b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/apps-engine/deno-runtime/handlers/listener/handler.ts (1)
23-30:⚠️ Potential issue | 🟠 MajorAdd fail-fast validation for missing ':' in
method.
Right now,method.split(':')can yield an undefined event interface and fall through to a method-not-found response. The handlers in this area are expected to throw immediately when the separator is missing.Proposed fix
export default async function handleListener(request: RequestContext): Promise<Defined | JsonRpcError> { const { method, params } = request; + if (!method.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + } const [, evtInterface] = method.split(':'); const app = AppObjectRegistry.get<App>('app');Based on learnings, enforce a fail-fast check: the request context method must contain a ':' separator.
packages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.ts (1)
8-20:⚠️ Potential issue | 🟠 MajorAdd fail-fast method validation before processing params.
Please add a check near Line 8 to ensure the request method contains a ':'; this handler currently skips the required validation.
Suggested fix
-export default async function handleOnUninstall(request: RequestContext): Promise<boolean> { - const { params } = request; +export default async function handleOnUninstall(request: RequestContext): Promise<boolean> { + const { params, method } = request; + + if (!method?.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + }Based on learnings: In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator. If the method string lacks a colon, throw a runtime error immediately (e.g., throw new Error('Invalid request state: method missing colon')) instead of defensive handling. This applies to all TypeScript handler files within the deno-runtime/handlers directory to ensure invalid requests are caught early and consistently.
packages/apps-engine/deno-runtime/handlers/app/handleOnUpdate.ts (1)
8-22:⚠️ Potential issue | 🟠 MajorAdd the mandatory request method separator check.
This handler should fail fast when the request method is missing the
:separator.Proposed fix
- const { params } = request; + const { params, method } = request; + if (typeof method !== 'string' || !method.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + }Based on learnings: In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator. If the method string lacks a colon, throw a runtime error immediately (e.g., throw new Error('Invalid request state: method missing colon')) instead of defensive handling. This applies to all TypeScript handler files within the deno-runtime/handlers directory to ensure invalid requests are caught early and consistently.
🤖 Fix all issues with AI agents
In `@apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts`:
- Around line 24-33: The test currently calls executeAppSlashCommand('nest',
roomId, execId) then immediately calls loadHistory and asserts targetMessage
exists, which is flaky; modify the test to poll loadHistory for the presence of
the message `nested_test_message ${execId}` (searching via messages.find like
existing code) with a bounded timeout and short interval (e.g., retry every
200-500ms up to a few seconds), and only assert existence after the poll
succeeds or fail with a clear timeout error; implement the poll logic in this
test around the loadHistory/targetMessage check so executeAppSlashCommand,
loadHistory, targetMessage, roomId and execId are used as before.
In `@packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts`:
- Around line 44-48: The catch block in scheduler-handler.ts uses an
unknown-typed err and accesses err.message unsafely; update the catch to
normalize the error by checking if err is an instance of Error (or has a string
message) and extract a safe message (e.g., err.message) or fall back to
String(err)/JSON.stringify for non-Error values, then pass that safe message
into logger.error (alongside the original err) and into
JsonRpcError.internalError({ message: safeMessage }) so both logging and the RPC
error use a type-safe message.
In `@packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts`:
- Around line 100-103: The error message in handlePreviewItem should include the
missing method name and remove the extra space; update the throw to reference
executePreviewItem (e.g. "Method executePreviewItem not found on slashcommand
${command.command}") so it matches the clarity of the check in handleExecutor
and points to the exact missing method on the command object.
🧹 Nitpick comments (5)
packages/apps-engine/deno-runtime/main.ts (1)
59-61: Consider using spread syntax instead ofObject.assignto avoid mutatingpayload.
Object.assign(payload, ...)mutates the originalpayloadobject in place. While this works, it could cause subtle issues ifpayloadis referenced elsewhere. A non-mutating approach is generally safer:♻️ Suggested refactor
- const context: RequestContext = Object.assign(payload, { - context: { logger } - }) + const context: RequestContext = { + ...payload, + context: { logger }, + };apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts (1)
32-35: Remove the eslint-disable comment by guarding the null case.You can avoid the non-null assertion and the lint suppression by throwing early when the message is missing.
♻️ Proposed change
- expect(targetMessage, 'Target message from nested request not found').to.exist; - - // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` -- we're checking existence above - const { _id: messageId } = targetMessage!; + if (!targetMessage) { + throw new Error('Target message from nested request not found'); + } + const { _id: messageId } = targetMessage;As per coding guidelines: “Avoid code comments in the implementation.”
packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts (1)
20-23: Add fail-fast validation for colon separator in method string.The destructuring at line 23 assumes the
callstring contains colons. If the method string lacks a colon separator,commandNameandmethodwill beundefined, leading to confusing downstream errors. Based on learnings, handlers in this directory should throw immediately when the method format is invalid.♻️ Proposed fix
const { method: call, params } = request; const { logger } = request.context; +if (!call.includes(':')) { + throw new Error('Invalid request state: method missing colon'); +} + const [, commandName, method] = call.split(':');packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts (1)
13-13: Consider adding fail-fast validation for the method separator.If
methodlacks a:separator,processorIdwill beundefined, leading to a less informative "method not found" error. Based on learnings from this codebase, handlers should fail fast with a clear error when the method format is invalid.♻️ Proposed fix to add fail-fast validation
const [, processorId] = method.split(':'); + if (!processorId) { + throw new Error(`Invalid request state: method "${method}" missing colon separator`); + } if (!Array.isArray(params)) {packages/apps-engine/deno-runtime/handlers/app/handler.ts (1)
54-60: Routing logic is correct but the structure could be cleaner.The if-else chain and subsequent switch operate on mutually exclusive method sets, so there's no risk of
resultbeing overwritten. However, the two-phase dispatch (if-else then switch) is slightly harder to follow.Consider consolidating into a single routing mechanism for clarity in future maintenance, though this is not blocking.
d-gubert
changed the title
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
2 times, most recently
from
4b4778d to
b70b65b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/apps-engine/deno-runtime/handlers/listener/handler.ts (1)
23-31:⚠️ Potential issue | 🟠 MajorAdd fail-fast colon validation before splitting
method.
method.split(':')can yield an undefined event interface when the method is malformed; this handler must throw immediately when the colon is missing.✅ Suggested fix
export default async function handleListener(request: RequestContext): Promise<Defined | JsonRpcError> { const { method, params } = request; + if (!method.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + } const [, evtInterface] = method.split(':'); const app = AppObjectRegistry.get<App>('app');Based on learnings: “In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator. If the method string lacks a colon, throw a runtime error immediately (e.g., throw new Error('Invalid request state: method missing colon')) instead of defensive handling. This applies to all TypeScript handler files within the deno-runtime/handlers directory to ensure invalid requests are caught early and consistently.”
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (1)
30-33:⚠️ Potential issue | 🟡 MinorFail fast when
methodlacks ':'If the separator is missing,
methodbecomesundefinedand downstream logic throws less clear errors. Throw a runtime error immediately after splitting.Based on learnings: In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator. If the method string lacks a colon, throw a runtime error immediately (e.g., throw new Error('Invalid request state: method missing colon')) instead of defensive handling. This applies to all TypeScript handler files within the deno-runtime/handlers directory to ensure invalid requests are caught early and consistently.🔧 Suggested fix
- const [, method] = rawMethod.split(':') as ['app', typeof uploadEvents[number]]; + const [, method] = rawMethod.split(':') as ['app', typeof uploadEvents[number]]; + if (!method) { + throw new Error('Invalid request state: method missing colon'); + }packages/apps-engine/deno-runtime/handlers/uikit/handler.ts (1)
28-33:⚠️ Potential issue | 🟡 MinorAdd explicit ':' validation before dispatch.
When
reqMethodlacks a colon,methodisundefinedand the handler returns methodNotFound instead of failing fast. Throw a clear runtime error right after splitting.Based on learnings: In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator. If the method string lacks a colon, throw a runtime error immediately (e.g., throw new Error('Invalid request state: method missing colon')) instead of defensive handling. This applies to all TypeScript handler files within the deno-runtime/handlers directory to ensure invalid requests are caught early and consistently.🔧 Suggested fix
- const [, method] = reqMethod.split(':'); + const [, method] = reqMethod.split(':'); + if (!method) { + throw new Error('Invalid request state: method missing colon'); + }
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts`:
- Around line 19-24: The handler assumes request.method contains ':' before
splitting which can produce undefined commandName/method; in slashCommandHandler
check the request.method (local variable call) contains at least one ':' (e.g.,
call.includes(':')) before performing call.split(':'), and if not throw or
return an immediate error (fail-fast) from the function using the same
JsonRpcError/Defined error shape used elsewhere in this file so invalid requests
are rejected early.
In `@packages/apps-engine/deno-runtime/handlers/videoconference-handler.ts`:
- Around line 9-14: The handler videoConferenceHandler must fail fast when the
RPC method lacks a ':' separator: before doing const [, providerName,
methodName] = call.split(':'), check that call includes ':' and if not throw new
Error('Invalid request state: method missing colon'); this ensures the function
validates the request context early and prevents undefined
providerName/methodName downstream.
🧹 Nitpick comments (2)
packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
10-11: Consider adding fail-fast validation for the method format.Per the established pattern in this runtime, if the
callstring lacks a:separator, the code should throw immediately rather than silently produceundefinedvalues forpathandhttpMethod.♻️ Proposed fix
export default async function apiHandler(request: RequestContext): Promise<JsonRpcError | Defined> { const { method: call, params } = request; + if (!call.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + } const [, path, httpMethod] = call.split(':');Based on learnings: "In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator."
packages/apps-engine/deno-runtime/handlers/outboundcomms-handler.ts (1)
9-11: Consider adding fail-fast validation for method format.The method string is split expecting at least three colon-separated segments. If the format is invalid (missing colons),
providerNameandmethodNamewould beundefined, leading to a less informative error downstream.This is pre-existing behavior, but based on learnings from similar handlers, adding an early validation would improve debuggability:
💡 Optional: Add early validation
export default async function outboundMessageHandler(request: RequestContext): Promise<JsonRpcError | Defined> { const { method: call, params } = request; + if (!call.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + } const [, providerName, methodName] = call.split(':');Based on learnings: "In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator."
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
b70b65b to
0eb698d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/handlers/api-handler.ts`:
- Around line 44-46: The catch block in api-handler.ts uses an untyped catch
variable `e` and accesses `e.message`, which is unsafe; update the catch
handling in the block that logs via `logger.debug` and returns `new
JsonRpcError(...)` to first normalize `e` (e.g., `const errMsg = (e instanceof
Error && e.message) ? e.message : String(e ?? 'Internal server error')`) and use
that `errMsg` in both the debug log and the `JsonRpcError` call; ensure you keep
the existing `logger.debug(`${path}'s ${call} was unsuccessful.`)` context but
append the normalized error details and avoid directly accessing `e.message`.
In `@packages/apps-engine/deno-runtime/handlers/outboundcomms-handler.ts`:
- Around line 10-11: After splitting the incoming request method into call,
providerName, and methodName (const { method: call, params } = request; const [,
providerName, methodName] = call.split(':')), add a fail-fast guard that
verifies providerName and methodName are present (non-empty). If the split does
not produce both parts, throw a clear runtime error (e.g., new Error("Invalid
method format: expected '<prefix>:<provider>:<method>'")) immediately so the
handler fails fast instead of later crashing when invoking methodName.apply().
🧹 Nitpick comments (3)
packages/apps-engine/deno-runtime/handlers/listener/handler.ts (1)
24-25: Consider adding a fail-fast check for the':'separator inmethod.If
methodlacks a':',evtInterfacewill beundefinedand the handler will fall through to themethodNotFoundreturn — safe, but not explicit. A fail-fast throw would be consistent with the pattern enforced in other handler files (e.g.,handler.ts).This is pre-existing behavior, so fine to defer.
packages/apps-engine/deno-runtime/main.ts (1)
59-61:Object.assignmutatespayloadin-place.This adds a
contextproperty directly onto the existingpayloadobject. It works here becausepayloadisn't reused after this point, but a spread-based construction ({ ...payload, context: { logger } }) would be clearer about intent and avoid surprises if future code reads frompayloadbefore the assignment.Also, the closing
)on line 61 is missing a semicolon — minor inconsistency with the rest of the file.Suggested alternative
- const context: RequestContext = Object.assign(payload, { - context: { logger } - }) + const context: RequestContext = Object.assign(payload, { + context: { logger }, + });Or, if you want to avoid mutation:
- const context: RequestContext = Object.assign(payload, { - context: { logger } - }) + const context: RequestContext = { + ...payload, + context: { logger }, + } as RequestContext;packages/apps-engine/deno-runtime/handlers/app/handler.ts (1)
52-60: Theif/else-ifchain (lines 54-60) andswitch(lines 62-93) are not guarded against each other — verify mutual exclusivity is maintained.Currently the two dispatch blocks are independent: the
if/else-ifruns first, then theswitchalways executes. This works becauseuploadEvents,uikitInteractions, andcheck*/execute*methods never overlap with the switch cases (construct,initialize,setStatus, lifecycle hooks, etc.). If a future method name overlaps,resultwould be silently overwritten by the switch.Consider adding an
elseor a guard before the switch to make the mutual exclusivity explicit:One way to make intent explicit
if (isOneOf(appMethod, uploadEvents)) { result = handleUploadEvents(request); } else if (isOneOf(appMethod, uikitInteractions)) { result = handleUIKitInteraction(request); } else if (appMethod.startsWith('check') || appMethod.startsWith('execute')) { result = handleListener(request); - } - - switch (appMethod) { + } else switch (appMethod) {
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
0eb698d to
1116954
Compare
coderabbitai
bot
added
stat: QA assured
dionisio-bot
bot
removed
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/handlers/api-handler.ts`:
- Around line 10-11: The handler currently destructures const { method: call,
params } = request and blindly does const [, path, httpMethod] =
call.split(':'), which yields undefined when the method string lacks a ':'; add
a fail-fast check immediately after extracting call (in the same handler that
uses request) to verify call.includes(':') (or that call.split(':').length >= 3)
and throw a runtime Error with a clear message if the separator is missing so
path and httpMethod are never undefined.
In `@packages/apps-engine/deno-runtime/handlers/app/handler.ts`:
- Around line 95-99: The code throws a JsonRpcError for an unknown method but
because JsonRpcError doesn't extend Error the catch logic rewraps it with a
generic -32000 code; instead of throwing, return the existing JsonRpcError so
the error bypasses the instanceof check and preserves the -32601 code—i.e., in
the handler replace `throw new JsonRpcError(\`Unknown method "${appMethod}"\`,
-32601)` with returning that JsonRpcError (consistent with the later `return
await result.then(formatResult)` pattern) so the original error is preserved by
the surrounding catch logic.
In `@packages/apps-engine/deno-runtime/handlers/outboundcomms-handler.ts`:
- Around line 19-27: The handler currently calls (method as Function).apply(...)
without verifying method is a function; add the same guard used in
videoconference-handler.ts and api-handler.ts: check typeof method !==
'function' (using the local variable method retrieved from provider[methodName
as keyof IOutboundMessageProviders]) and if so throw the
JsonRpcError.methodNotFound for methodName so the runtime returns a clear RPC
error instead of a TypeError; keep using wrapComposedApp(provider, request) and
the existing args when calling apply.
🧹 Nitpick comments (3)
packages/apps-engine/deno-runtime/handlers/uikit/handler.ts (1)
29-34: Missing fail-fast check for:separator in the method string.Per the established pattern in other handlers in this directory, the request method must contain a
:separator. If it's missing, the handler should throw immediately rather than silently falling through tomethodNotFound.Proposed fix
const { method: reqMethod, params } = request; - const [, method] = reqMethod.split(':'); + + if (!reqMethod.includes(':')) { + throw new Error('Invalid request state: method missing colon'); + } + + const [, method] = reqMethod.split(':');Based on learnings: "In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator."
packages/apps-engine/deno-runtime/handlers/app/handler.ts (1)
52-60: Dispatch relies on the if-else chain and the switch being mutually exclusive by convention.The if-else chain (Lines 54–59) and the switch (Lines 62–93) run sequentially; a method matching both would have
resultoverwritten by the switch. Currently there is no overlap (uploadEvents,uikitInteractions, andcheck*/execute*patterns do not collide with the switch cases), so this is functionally correct. However, the structure is fragile — adding a future case to the switch that starts withcheckorexecutewould silently shadow the if-else result.Consider either combining into a single dispatch structure or adding an early return /
elseguard after the if-else chain whenresultis already assigned.packages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.ts (1)
19-25:mockApp.getLoggermay now be dead code.The
mockAppstill definesgetLogger, but the handler now reads the logger fromrequest.context.loggerinstead. If no other code path callsgetLoggeron the app during this test, this mock method is unused.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/meteor/tests/data/apps/app-packages/nested-requests_0.0.1.zipis excluded by!**/*.zip
📒 Files selected for processing (35)
.changeset/chilled-lemons-admire.mdapps/meteor/tests/data/apps/app-packages/README.mdapps/meteor/tests/data/apps/app-packages/index.tsapps/meteor/tests/data/slashcommands.helpers.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/api-handler.tspackages/apps-engine/deno-runtime/handlers/app/construct.tspackages/apps-engine/deno-runtime/handlers/app/handleInitialize.tspackages/apps-engine/deno-runtime/handlers/app/handleOnDisable.tspackages/apps-engine/deno-runtime/handlers/app/handleOnEnable.tspackages/apps-engine/deno-runtime/handlers/app/handleOnInstall.tspackages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.tspackages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.tspackages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.tspackages/apps-engine/deno-runtime/handlers/app/handleOnUpdate.tspackages/apps-engine/deno-runtime/handlers/app/handleSetStatus.tspackages/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/deno-runtime/handlers/listener/handler.tspackages/apps-engine/deno-runtime/handlers/outboundcomms-handler.tspackages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.tspackages/apps-engine/deno-runtime/handlers/tests/api-handler.test.tspackages/apps-engine/deno-runtime/handlers/tests/helpers/mod.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tspackages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.tspackages/apps-engine/deno-runtime/handlers/tests/upload-event-handler.test.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.tspackages/apps-engine/deno-runtime/handlers/uikit/handler.tspackages/apps-engine/deno-runtime/handlers/videoconference-handler.tspackages/apps-engine/deno-runtime/lib/messenger.tspackages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/lib/wrapAppForRequest.tspackages/apps-engine/deno-runtime/main.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/meteor/tests/data/apps/app-packages/README.md
- apps/meteor/tests/data/apps/app-packages/index.ts
- packages/apps-engine/deno-runtime/handlers/tests/upload-event-handler.test.ts
- packages/apps-engine/deno-runtime/handlers/tests/api-handler.test.ts
- packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
- packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts
- apps/meteor/tests/data/slashcommands.helpers.ts
- packages/apps-engine/deno-runtime/lib/messenger.ts
- packages/apps-engine/deno-runtime/handlers/app/handleInitialize.ts
- packages/apps-engine/deno-runtime/handlers/tests/slashcommand-handler.test.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/handleOnUpdate.tspackages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.tspackages/apps-engine/deno-runtime/main.tspackages/apps-engine/deno-runtime/handlers/tests/helpers/mod.tspackages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tspackages/apps-engine/deno-runtime/handlers/app/handleOnDisable.tspackages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/app/handleSetStatus.tspackages/apps-engine/deno-runtime/handlers/videoconference-handler.tspackages/apps-engine/deno-runtime/handlers/api-handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.tspackages/apps-engine/deno-runtime/handlers/listener/handler.tspackages/apps-engine/deno-runtime/handlers/uikit/handler.tspackages/apps-engine/deno-runtime/handlers/app/handleOnInstall.tspackages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.tspackages/apps-engine/deno-runtime/handlers/outboundcomms-handler.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/deno-runtime/handlers/app/handleOnEnable.tspackages/apps-engine/deno-runtime/handlers/app/construct.tspackages/apps-engine/deno-runtime/handlers/app/handler.ts
🧠 Learnings (26)
📚 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/handleOnUpdate.tspackages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.tspackages/apps-engine/deno-runtime/main.tspackages/apps-engine/deno-runtime/handlers/tests/helpers/mod.tspackages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tspackages/apps-engine/deno-runtime/handlers/app/handleOnDisable.tspackages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.tspackages/apps-engine/deno-runtime/handlers/app/handleSetStatus.tspackages/apps-engine/deno-runtime/handlers/videoconference-handler.tspackages/apps-engine/deno-runtime/handlers/api-handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.tspackages/apps-engine/deno-runtime/handlers/listener/handler.tspackages/apps-engine/deno-runtime/handlers/uikit/handler.tspackages/apps-engine/deno-runtime/handlers/app/handleOnInstall.tspackages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.tspackages/apps-engine/deno-runtime/handlers/outboundcomms-handler.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/deno-runtime/handlers/app/handleOnEnable.tspackages/apps-engine/deno-runtime/handlers/app/construct.tspackages/apps-engine/deno-runtime/handlers/app/handler.ts
📚 Learning: 2026-01-30T20:24:43.485Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: packages/apps-engine/deno-runtime/handlers/app/handler.ts:52-60
Timestamp: 2026-01-30T20:24:43.485Z
Learning: In packages/apps-engine/deno-runtime/handlers, enforce a fail-fast check: the request context method must contain a ':' separator. If the method string lacks a colon, throw a runtime error immediately (e.g., throw new Error('Invalid request state: method missing colon')) instead of defensive handling. This applies to all TypeScript handler files within the deno-runtime/handlers directory to ensure invalid requests are caught early and consistently.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleOnUpdate.tspackages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.tspackages/apps-engine/deno-runtime/handlers/tests/helpers/mod.tspackages/apps-engine/deno-runtime/handlers/scheduler-handler.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tspackages/apps-engine/deno-runtime/handlers/app/handleOnDisable.tspackages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.tspackages/apps-engine/deno-runtime/handlers/app/handleSetStatus.tspackages/apps-engine/deno-runtime/handlers/videoconference-handler.tspackages/apps-engine/deno-runtime/handlers/api-handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.tspackages/apps-engine/deno-runtime/handlers/listener/handler.tspackages/apps-engine/deno-runtime/handlers/uikit/handler.tspackages/apps-engine/deno-runtime/handlers/app/handleOnInstall.tspackages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.tspackages/apps-engine/deno-runtime/handlers/outboundcomms-handler.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.tspackages/apps-engine/deno-runtime/handlers/lib/assertions.tspackages/apps-engine/deno-runtime/handlers/app/handleOnEnable.tspackages/apps-engine/deno-runtime/handlers/app/construct.tspackages/apps-engine/deno-runtime/handlers/app/handler.ts
📚 Learning: 2026-01-30T20:24:52.390Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: packages/apps-engine/deno-runtime/handlers/app/handler.ts:52-60
Timestamp: 2026-01-30T20:24:52.390Z
Learning: In packages/apps-engine/deno-runtime handlers, the `method` parameter in request contexts is expected to always contain a `:` separator. If it lacks the separator, a runtime error is desirable (fail-fast approach) rather than defensive handling, as it indicates an invalid request state that should be caught immediately.
Applied to files:
packages/apps-engine/deno-runtime/main.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
packages/apps-engine/deno-runtime/main.tspackages/apps-engine/deno-runtime/handlers/videoconference-handler.tspackages/apps-engine/deno-runtime/handlers/slashcommand-handler.tspackages/apps-engine/deno-runtime/handlers/outboundcomms-handler.tspackages/apps-engine/deno-runtime/handlers/app/handler.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/deno-runtime/handlers/tests/helpers/mod.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.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:
packages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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:
packages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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:
packages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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:
packages/apps-engine/deno-runtime/lib/tests/messenger.test.tspackages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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:
packages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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:
packages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.tsapps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.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/app-logs-nested-requests.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/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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/app-logs-nested-requests.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/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.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/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.ts
📚 Learning: 2026-02-04T12:08:56.950Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:08:56.950Z
Learning: In the E2E tests under apps/meteor/tests/end-to-end/apps, prefer using a fixed sleep (e.g., 1s) instead of implementing polling or retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.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/app-logs-nested-requests.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 `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.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:
apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.tspackages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.ts
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
packages/apps-engine/deno-runtime/handlers/listener/handler.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:
packages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
packages/apps-engine/deno-runtime/handlers/app/handler.ts
🧬 Code graph analysis (18)
packages/apps-engine/deno-runtime/handlers/app/handleOnUpdate.ts (1)
packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.ts (1)
packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/tests/helpers/mod.ts (3)
packages/apps-engine/deno-runtime/lib/messenger.ts (1)
RequestDescriptor(8-8)packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/logger/src/index.ts (1)
Logger(27-91)
packages/apps-engine/deno-runtime/lib/tests/messenger.test.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/handlers/tests/helpers/mod.ts (1)
createMockRequest(5-16)
packages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.ts (2)
packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts (1)
handleScheduler(9-49)packages/apps-engine/deno-runtime/handlers/tests/helpers/mod.ts (1)
createMockRequest(5-16)
packages/apps-engine/deno-runtime/handlers/app/handleOnDisable.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.ts (1)
packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts (4)
apps/meteor/tests/data/apps/helper.ts (2)
cleanupApps(24-27)installLocalTestPackage(42-54)apps/meteor/tests/data/apps/app-packages/index.ts (1)
appCausingNestedRequests(4-4)apps/meteor/tests/data/slashcommands.helpers.ts (1)
executeAppSlashCommand(13-35)apps/meteor/tests/data/apps/apps-data.ts (1)
apps(10-12)
packages/apps-engine/deno-runtime/handlers/videoconference-handler.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapComposedApp(42-60)
packages/apps-engine/deno-runtime/handlers/api-handler.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapComposedApp(42-60)
packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapComposedApp(42-60)
packages/apps-engine/deno-runtime/handlers/listener/handler.ts (1)
packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/uikit/handler.ts (2)
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (1)
isOneOf(32-34)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/app/handleOnInstall.ts (1)
packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/outboundcomms-handler.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapComposedApp(42-60)
packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (1)
packages/core-typings/src/Apps.ts (1)
App(69-133)
packages/apps-engine/deno-runtime/handlers/app/handleOnEnable.ts (2)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/lib/wrapAppForRequest.ts (1)
wrapAppForRequest(6-16)
packages/apps-engine/deno-runtime/handlers/app/handler.ts (4)
packages/apps-engine/deno-runtime/lib/requestContext.ts (1)
RequestContext(5-10)packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (1)
isOneOf(32-34)packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts (2)
uploadEvents(16-16)handleUploadEvents(30-78)packages/apps-engine/deno-runtime/handlers/uikit/handler.ts (1)
uikitInteractions(11-17)
⏰ 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 (33)
packages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.ts (1)
6-6: LGTM — request-scoped logger correctly bound viawrapAppForRequest.The
.call(wrapAppForRequest(app, request), ...)pattern properly ensuresthis.loggerinsideonSettingUpdatedresolves to the per-request logger, which is consistent with the same refactor applied across other app lifecycle handlers.Also applies to: 24-30
packages/apps-engine/deno-runtime/handlers/app/handleOnUninstall.ts (1)
24-31: LGTM — request-scopedthisbinding viawrapAppForRequestfollows the established handler pattern.Using
.call(wrapAppForRequest(app, request), ...)correctly rebindsthisso thatthis.loggerinsideonUninstallreturns the per-request logger from theRequestContext, preventing log cross-contamination in nested requests. This is consistent with the approach inhandleOnEnable.ts,handleOnInstall.ts, and other sibling handlers.packages/apps-engine/deno-runtime/handlers/app/handleOnEnable.ts (1)
6-21: Clean, consistent request-scoped logger binding — LGTM.The change correctly binds the request-scoped logger via
wrapAppForRequestproxy asthiscontext foronEnable. This is consistent with the sibling handler updates (e.g.,handleInitialize.ts) and directly addresses the interleaved-logs bug described in the PR objectives.packages/apps-engine/deno-runtime/handlers/app/handleSetStatus.ts (1)
7-7: LGTM — request-scopedthisbinding is clean and consistent.The use of
.call(wrapAppForRequest(app, request), status)correctly rebindsthisto the per-request proxy, which is the pattern applied across all the other app handlers in this PR. This should properly scope the logger to each request and fix the log interleaving issue.Also applies to: 30-30
packages/apps-engine/deno-runtime/handlers/app/handleOnDisable.ts (1)
6-17: Clean, focused fix for request-scoped logging — LGTM.The approach is sound: wrapping the app in a
ProxyviawrapAppForRequestto interceptthis.loggerand binding it asthisthrough.call()ensuresonDisableuses the per-request logger. The duck-typing guard on line 11 is consistent with the runtime's assertion conventions.packages/apps-engine/deno-runtime/handlers/lib/assertions.ts (2)
14-26: LGTM — clean extraction ofisRecordandisPlainObject.The single-expression
isRecordand the prototype-basedisPlainObjectare standard and correct.
36-44: LGTM —isAppduck-typing guard is appropriate here.Delegating the
extendConfigurationcheck to a reusable type guard and wiring it intoassertAppAvailableis a good refactor. Based on learnings, App objects in this runtime are never serialized, so duck-typing on a method likeextendConfigurationis reliable.packages/apps-engine/deno-runtime/handlers/uikit/handler.ts (2)
11-17: LGTM —as constassertion andisOneOfusage.Making
uikitInteractionsa readonly tuple and using theisOneOftype guard provides proper type narrowing formethodthrough the switch statement.
76-84: LGTM —wrapAppForRequestcorrectly binds the per-request logger.Using the proxy as the
thiscontext ensures thatthis.loggerinside the interaction handler resolves to the request-scoped logger, which is the core fix for the nested-request log contamination issue.packages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.ts (2)
6-6: LGTM — import is used and correctly referenced.
24-30: Clean binding of per-request context via.call()— looks good.Using
Function.prototype.callwithwrapAppForRequest(app, request)asthiscorrectly scopes the logger (and any other request-bound state) to the current request, which addresses the log interleaving issue..changeset/chilled-lemons-admire.md (1)
1-6: LGTM!Changeset is correctly formatted with appropriate patch-level bumps and a clear description matching the PR objective.
apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts (3)
1-11: LGTM!Imports are clean and all appear to be used.
13-16: LGTM!EE-gating and cleanup hooks are properly set up.
46-66: LGTM — log assertions correctly validate the nested-request logging fix.The scan for both
slashcommand:nest:executorandapp:executePostMessageSentlogs keyed bymessageId, plus thelengthOf(1)check onexecIdentries, effectively verifies that logs are neither lost nor cross-contaminated across nested requests.packages/apps-engine/deno-runtime/handlers/app/handleOnUpdate.ts (1)
24-31: LGTM!The
wrapAppForRequestbinding and expanded argument list (context, reader, http, persistence, modifier) are consistent with the otherhandleOn*handlers in this PR.packages/apps-engine/deno-runtime/handlers/listener/handler.ts (2)
30-34: Good addition of the!appguard.Previously,
appcould beundefinedand still reach theapplycall. The combined!app || typeof eventExecutor !== 'function'check prevents that.
42-42: LGTM — request-scopedthisbinding viawrapAppForRequest.Correctly uses
.apply(wrapAppForRequest(app, request), args)to bind the request-scoped logger asthis.loggerduring event execution.packages/apps-engine/deno-runtime/handlers/tests/helpers/mod.ts (1)
1-16: LGTM!Clean test helper that constructs a properly-typed
RequestContextwith a request-scoped logger. The hardcodedidand no-opserializeare appropriate for unit tests.packages/apps-engine/deno-runtime/handlers/videoconference-handler.ts (1)
23-27: Good additions: method type guard andwrapComposedAppbinding.The
typeof method !== 'function'check (line 23) prevents opaque errors from.apply()on non-functions, andwrapComposedAppcorrectly propagates the request-scoped logger through the provider's app reference.Also applies to: 37-43
packages/apps-engine/deno-runtime/handlers/tests/videoconference-handler.test.ts (2)
108-121: Good test coverage for the newtypeof method !== 'function'guard.The assertion against
-32601(methodNotFound) correctly validates the new check added in the handler at lines 23-27.
37-46: LGTM — consistent migration tocreateMockRequest.All test cases properly use the shared helper, preserving the original method strings and param shapes.
Also applies to: 48-58, 60-71, 73-85, 87-95, 97-106
packages/apps-engine/deno-runtime/handlers/app/handleOnInstall.ts (1)
24-31: LGTM!Consistent with
handleOnUpdateand otherhandleOn*handlers — correctly binds the request-scoped app proxy and passes the full accessor set.packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
32-39: LGTM —wrapComposedAppbinding for API endpoint invocation.Correctly wraps the endpoint with the request context so the app's logger is request-scoped during execution.
packages/apps-engine/deno-runtime/lib/tests/messenger.test.ts (3)
7-19: Clean migration to request-scoped context in tests.The
createMockRequesthelper and per-testcontextinitialization inbeforeEachproperly ensure test isolation and align with the new request-context-driven approach. Each test gets a fresh logger instance, preventing state leakage between tests.
27-60: Success response test correctly validates embedded logs.The test verifies that the per-request logger's entries are correctly attached to the success response payload under
result.logs, which is the core of the fix for interleaved logs. TheJsonRpccast on Line 39 is appropriate since the spy captures the argument generically.
62-98: Error response test correctly validates embedded logs.Similarly validates that logs appear in
error.data.logsfor error responses, completing coverage for both response paths.packages/apps-engine/deno-runtime/handlers/slashcommand-handler.ts (1)
54-57: Request-scoped context threading viaDepsandwrapComposedApplooks correct.The
Depstype cleanly encapsulates bothAppAccessorsInstanceand therequest, andwrapComposedAppis applied consistently in bothhandleExecutor(viaexecutor.apply) andhandlePreviewItem(viacommand.executePreviewItem.call). This ensures the per-request logger is properly bound to the command'sthiscontext during execution.Also applies to: 65-65, 86-86, 100-100, 119-120
packages/apps-engine/deno-runtime/handlers/app/handler.ts (1)
20-23: Fail-fast guard for missing:is still absent — intentional per prior discussion.Per the learning from a prior review, the fail-fast crash when
methodlacks:is the desired behavior. Noting for context thatappMethodwill beundefinedandstartsWithat Line 58 will throw aTypeError, which is caught by the generic catch block.packages/apps-engine/deno-runtime/handlers/app/construct.ts (1)
77-103: Correct: per-request logger injected at construction time.The app is constructed with the request-scoped logger from
request.context.logger. For subsequent requests, thewrapAppForRequestproxy mechanism overrides the logger, so the construction-time logger is only used during theconstructcall itself. This is consistent with the overall approach.packages/apps-engine/deno-runtime/main.ts (1)
57-61: Core fix: per-request Logger creation correctly scopes logs to each request.A new
Loggeris instantiated for each incoming request and attached to theRequestContext. This is what prevents log entries from being interleaved across concurrent requests — each request's handler tree now writes to its own logger instance, and the logs are collected and returned with the response.packages/apps-engine/deno-runtime/handlers/tests/scheduler-handler.test.ts (1)
7-7: Clean migration tocreateMockRequestfor consistent test context.The test now properly provides a
RequestContextwith a per-request logger, matching the handler's expectation. The assertion remains valid.Also applies to: 42-43
packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts (1)
9-13: Request-scoped logger andwrapComposedAppintegration looks correct.The handler consistently uses
request.context.loggerfor all log calls and wraps the processor withwrapComposedAppto ensure the per-request logger propagates throughthiscontext during execution. This correctly addresses the interleaved-logs issue for scheduled jobs.Also applies to: 29-29, 32-33, 41-41, 44-47
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
d-gubert
removed
the
stat: QA assured
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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="packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts">
<violation number="1" location="packages/apps-engine/deno-runtime/handlers/scheduler-handler.ts:36">
P2: `assertAppAvailable` throws a `JsonRpcError`, but the catch block always wraps errors and drops the original code/data. This loses the new `DRT_APP_NOT_AVAILABLE` error details (and may return an undefined message). Handle `JsonRpcError` explicitly before wrapping.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
8292e83 to
341cca1
Compare
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
47d0f5a to
211ddd4
Compare
d-gubert
force-pushed
the
feat/apps-logs-redirect2
branch
from
211ddd4 to
b5015cd
Compare
d-gubert
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
4 participants
Proposed changes (including videos or screenshots)
While handling a request, the engine would lose the reference to the correct Logger object that contained logs for that request due to concurrency. This was especially noticeable in "nested requests": when handling a request on the runtime caused a side-effect that triggered another request to the runtime that should execute fully before handling control over the previous one.
For example, if an App implements the
IPostMessageSentinterface AND provides a slashcommand that sends a message, the execution flow would look like:slashcommandrequest to runtime;slashcommandrequest, the runtime would send its own request forsendMessageto the Apps-Engine. As the appawaits that call,slashcommandwould pause executionIPostMessageSentevent. This would cause the Apps-Engine to send apost-messagerequest to the runtime.post-messagerequest fully, while theslashcommandrequest is paused. Here, the global logger reference would be overwritten for thepost-messagerequest, and the logger used forslashcommandwould be lost.post-messagerequest.sendMessagerequest.slashcommandrequest. At this point, any further log messages will essentially be "lost" - technically, they would end up getting mixed up with the messages from thepost-messagerequest, but the log entry stored in the DB would look nothing like aslashcommandrequest entry.This PR ensures that the logger is persisted throughout the request handling with no mix-ups. This creates some requirements for app developers that should be kept in mind - a new doc page will be published for these instructions.
Issue(s)
SUP-824
CONN-436
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation
Chores