[APPS] Copy apps-function-query runtime into apps plugin#322
[APPS] Copy apps-function-query runtime into apps plugin#322yoannmoinet merged 6 commits intomasterfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929592a5a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| return executeActionResponse.result.data; |
There was a problem hiding this comment.
Return dev-server result directly instead of
.result.data
The dev-server transport currently assumes a success body shaped like { success: true, result: { data: ... } }, but the in-repo /__dd/executeAction handler returns { success: true, result } directly (see packages/plugins/apps/src/vite/dev-server.ts and its test assertion in dev-server.test.ts). With this code, non-iframe calls will resolve undefined for normal backend-function outputs (or crash if result is null), so generated proxies will lose return values once this runtime is wired in.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified E2E on the stacked PR #320 (where this transport is actually wired in via globalThis.DD_APPS_RUNTIME). The hypothesized mismatch doesn't reproduce: the Datadog preview-async queries API itself returns outputs: { data: <value> }, so the wire shape is already what devServerTransport.result.data expects. The recommended result: { data: result } wrap would have double-nested it.
Repro on PR #320, dev server running against staging:
curl -sS -X POST http://127.0.0.1:5173/__dd/executeAction \
-H 'Content-Type: application/json' \
-d '{"functionName":"<hash>.getGreeting","args":["World"]}'
# {"success":true,"result":{"data":{"message":"Hello, World! ..."}}}Why static review missed it: dev-server.ts declares its own loose ExecuteActionResponse with result?: unknown, so the satisfies at the response site has no teeth. And dev-server.test.ts mocks Datadog's API as outputs: { result: 'hello' } — without the data layer the real API adds — so the mock has drifted from reality and wouldn't catch a real shape change either.
Smaller real fix, split across the stack:
This PR (#322):
- Extract canonical
ExecuteActionRequest/ExecuteActionResponse<TData>frombackend/client/types.tsinto a newbackend/protocol.ts(single source of truth, neutral location). - Update
transports/dev-server-transport.tsandtransports/post-message-transport/types.tsimports to pull from there.
PR #320:
- Replace
dev-server.ts's local interfaces with imports from../backend/protocol; typeexecuteScriptViaDatadogas returning{ data: unknown }sosatisfies ExecuteActionResponseactually checks the wire contract. - Update
dev-server.test.tsmocks to use the realoutputs: { data: ... }shape so future drift IS caught.
No wire-behavior change. Closing this as not-reproduced; the type + mock work above prevents recurrence. Resolving #3105276088 and #3105612205 as duplicates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41540bfc4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| return executeActionResponse.result.data; |
There was a problem hiding this comment.
Align result parsing with /__dd/executeAction payload
This transport assumes successful responses are shaped as {"success": true, "result": { "data": ... }}, but the current dev-server handler returns {"success": true, "result": ...} (packages/plugins/apps/src/vite/dev-server.ts:302). As written, successful backend calls will resolve to undefined (or throw when result is null) instead of the actual function output once this runtime is wired in, which breaks normal backend-function execution in dev.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Responded in the previous thread.
The backend/client files use browser APIs (window, MessageEvent, fetch) that need DOM types, but adding them via /// <reference lib="dom" /> polluted the entire workspace compilation — causing Node.js types (Blob, ReadableStream, Headers) to clash with DOM equivalents across @dd/core and other packages. Fix: split into two tsconfigs — the main one excludes backend/client, and a new tsconfig.client.json includes only those files with lib: ["es2022", "dom"]. The typecheck script chains both. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Testing
|
…e DOM lib routing
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d972daf59d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| return executeActionResponse.result.data; |
There was a problem hiding this comment.
Return the executeAction result without forcing
.data
devServerTransport currently assumes the success payload is { success: true, result: { data: ... } }, but the in-repo /__dd/executeAction handler returns { success: true, result } directly (packages/plugins/apps/src/vite/dev-server.ts, handleExecuteAction) and its tests assert body.result is the final value (packages/plugins/apps/src/vite/dev-server.test.ts). In that common path, executeActionResponse.result.data is undefined, so backend function calls will silently lose their return value in dev mode once this runtime is wired up.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Responded in the previous thread.
yoannmoinet
left a comment
There was a problem hiding this comment.
Codex review makes sense to me, will you follow it?

Motivation
Own the
executeBackendFunctionruntime inside the apps plugin so it can evolve alongside codegen and drop the@datadog/apps-function-querydep from user bundles. This PR only copies the source; the upstack PR wires it up.Changes
Copied
@datadog/apps-function-queryfrom web-ui intopackages/plugins/apps/src/backend/client/. Nothing imports it yet.Adaptations needed to fit this repo:
*.unit.ts→*.test.tsto match Jest'stestMatch./// <reference lib="dom" />+/* eslint-env browser */on files using browser globals (repo tsconfig islib: ["es2022"]).try/catchtest as.rejects.toMatchObject(...)to satisfyjest/no-jasmine-globals+jest/no-conditional-expect.QA Instructions
yarn workspace @dd/apps-plugin typecheck— 0 errors insrc/backend/client/.yarn workspace @dd/tests test:unit packages/plugins/apps/src/backend/client— 6 tests pass.yarn eslint 'packages/plugins/apps/src/backend/client/**/*.ts' --quiet— clean.Blast Radius
New files only, unreachable until the upstack PR wires them up. No dependency or public API changes.
Documentation