[workflows-shared] Add full step output retrieval to Workflows local explorer - #14822
[workflows-shared] Add full step output retrieval to Workflows local explorer#14822pombosilva wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: bef29ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
943ca72 to
bef29ae
Compare
|
✅ All changesets look good |
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
/bonk |
/bonk review — PR #14822Reviewed the full stack of this change (engine → miniflare resource → OpenAPI/generated client → explorer UI). Summary: the implementation is correct, well-layered, and matches the local-explorer conventions. I verified locally and found no changes required. What I checked
Verification run locally
Minor, non-blocking notes (no action needed):
LGTM. 🔨 |
|
Posted my review on PR #14822. Summary
Findings: the implementation is correct and no code changes were needed. What I verified:
Verification:
I noted two minor non-blocking observations (a cosmetic codegen gap on the |
| return () => { | ||
| active = false; | ||
| }; | ||
| }, [needsFetch, workflowName, instanceId, step.name]); |
There was a problem hiding this comment.
i think restart from step can display stale output? it omits step output/version
| } | ||
| let active = true; | ||
| setState({ status: "loading" }); | ||
| void workflowsGetStepOutput({ |
There was a problem hiding this comment.
do you think 1gb of download all at once would crash the tab?
dario-piotrowicz
left a comment
There was a problem hiding this comment.
LGTM with a couple of minor suggestions 🙂
| return ( | ||
| typeof value === "string" && | ||
| (value.endsWith(STREAM_PREVIEW_TRUNCATED_MARKER) || | ||
| value.startsWith("[ReadableStream")) |
There was a problem hiding this comment.
Nit: I think a regex would be more correct here?
| value.startsWith("[ReadableStream")) | |
| /^\[ReadableStream:\s\d+\s\w+]$/.test(value) |
(Then we can also have some other tests that make sure that incorrect string starting with [ReadableStream are not accepted)
Fixes WOR-1510.
Adds a GET
/workflows/{workflow_name}/instances/{instance_id}/stepthat returns a step's complete, untruncated output, serving ReadableStream outputs as raw application/octet-stream bytes, which the explorer UI fetches on demand when a step's inline preview was truncated.A picture of a cute animal (not mandatory, but encouraged)