refactor(workers): consolidate the command family onto shared helpers - #6431
Draft
johnstonmatt wants to merge 7 commits into
Draft
refactor(workers): consolidate the command family onto shared helpers#6431johnstonmatt wants to merge 7 commits into
johnstonmatt wants to merge 7 commits into
Conversation
Five handlers opened with the same three service acquisitions, the same ref resolution and suffix, and closed with the same two finalizers. The ordering is the whole point of that block and is easy to get subtly wrong: four of them resolved the ref above both finalizers, so an unlinked non-interactive checkout failed before `telemetryState.flush` was installed and wrote no post-run event. `legacyWorkersRun` owns the ordering once. Telemetry wraps the resolution; the linked-project cache stays under the ref, having nothing to write without one. `logs` already had the fix and now shares the same path.
The machine-output check, the structured emission and the fall-through to text were written out in all five handlers. Four of them branched on `output.format` alone, which ignores `-o`'s priority over `--output-format` — so `-o pretty --output-format json` emitted JSON from a run that had asked for text. `legacyEmitWorkersPayload` makes the decision once, keeping the existing "returns whether it emitted" contract so callers still skip their text rendering the same way. The precedence is pinned in one place rather than once per command.
`supabase experimental workers` was a literal in roughly thirty call sites. The move under `experimental` had to rewrite every one, and two follow-up commits exist because some were missed or left pointing at a command that no longer existed. `legacyWorkersCommand` owns the path, with `legacyWorkersPushCommand` and `legacyWorkersStatusCommand` for the two suggestions that recur. A unit test pins the exact spelling, since these strings are copy-pasted out of a terminal.
Three commands raised `WorkerNotDeployedError` with a byte-identical detail and their own suggestion. `legacyWorkerNotDeployed` owns the sentence that is the same everywhere and takes the way out that is not: `status` and `logs` point at `push`, while `delete` deliberately points at `list`.
Five more call sites — push's progress lines and per-worker output, delete's confirmation guard, new's prompt gate — each spelled out `output.format` plus `machineOutput` by hand, and each got the precedence wrong the same way: `-o pretty --output-format json` asks for text but was treated as a machine run. `legacyWorkersRendersText` answers it once. `push` threads that instead of `machineOutput`, and `new` now emits through the shared payload helper.
`pollOnce` was a 60-line closure doing three jobs, and carried two `Ref`s that were only ever read and written together — one cursor split in half, where advancing the timestamp without recording the ids replays the overlap. `FollowCursor` is that one value, and the paging walk moves to `drainSince`, so the poll body reads as its three steps: read the cursor, drain the window, emit what is new. Trims the comments here and in the helpers this pass added. Rationale that belongs to a change rather than to the code — what used to be wrong, and how many callers had it wrong — lives in the commit that made it, not in a docblock the next reader pays for.
`deployOneWorker` opened with a sixty-line bare block — the author's own marker that it was a separate job. `assertDeployableSource` is that job: does this path hold something worth deploying. What is left reads as its steps — describe, check the source, resolve the spec, package, upload, deploy, report. Costs a few lines in parameter plumbing and buys a function that can be read, and tested, without the deploy around it.
johnstonmatt
changed the base branch from
develop
to
FUNC-848/workers-deploy-wait-flag
September 2, 2026 01:37
johnstonmatt
changed the title
johnstonmatt
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pure refactor of the
workerscommand family, one consolidation per commit, stacked on #6371. The duplication it removes was hiding a handful of bugs, fixed here as a side effect:legacyWorkersRunowns the shared command scaffold — service acquisition, ref resolution and suffix, and the two finalizers — with the ordering written once. Four handlers resolved the ref above both finalizers, so an unlinked non-interactive run failed before the telemetry flush was installed and wrote no post-run event.legacyEmitWorkersPayloadmakes the "emit a structured payload" decision once, andlegacyWorkersRendersTextnames the "renders human text" condition. Both fix the same precedence bug across ten hand-written call sites:-o pretty --output-format jsonasks for text but was treated as a machine run.legacyWorkersCommand, withlegacyWorkersPushCommandandlegacyWorkersStatusCommandfor the two recurring suggestions, replaces thesupabase experimental workersliteral in roughly thirty call sites. A unit test pins the spelling, since these strings get copy-pasted out of a terminal.legacyWorkerNotDeployedbuilds the sentence that is identical in three commands and takes the way out that is not:statusandlogspoint atpush,deletedeliberately points atlist.workers logs: the follow loop's two always-pairedRefs become oneFollowCursor, and the paging walk moves todrainSince, so the poll body reads as its three steps — read the cursor, drain the window, emit what is new.workers push: the sixty-line source check lifts out ofdeployOneWorkerintoassertDeployableSource, leaving the deploy readable as its steps.Also trims comments that explained a change rather than the code. Rationale about what used to be wrong, and how many callers had it wrong, lives in these commit messages rather than in a docblock every later reader pays for.