refactor(cli): decompose runCli into explicit phase functions - #1373
Merged
Conversation
runCli inlined parse/help short-circuits, binding resolution, remote auth + materialization, four special-cased command kinds, dispatch, and a catch block coupled to ~10 closure-mutated let bindings across ~390 lines. Each phase is now a named function over an explicit CliRunContext: parseCliInputOrExit, resolveRunContextOrExit, runReactDevtoolsCli, resolveRemoteContext, buildClientConfig, maybeStartDaemonLogTail, createReplayReporterForTest, dispatchCliCommand, handleRunCliFailure. The context is mutated in place by resolveRemoteContext so the failure handler observes exactly the state the throwing phase saw — matching the previous closure semantics, including the close-with-no-daemon success path and daemon-log-tail-on-error. Behavior is unchanged; runCli itself is now ~75 lines of orchestration.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
added
the
ready-for-human
Member
Author
|
Reviewed exact head Residual risk: this broad refactor has no single dedicated phase-order/partial-mutation test, so future ordering changes still rely on distributed CLI coverage. |
|
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.
What
Item 1 of the code-quality follow-ups:
runCli()was one ~390-line function inlining every phase of a CLI invocation, sharing ~10 mutableletbindings by closure, with a catch block coupled to nearly all of them. It is now ~75 lines of orchestration over named phase functions, each with explicit inputs:parseCliInputOrExit— parse + parse-error formatting, warnings,--version/help short-circuits, no-command usage exitresolveRunContextOrExit— binding/platform defaulting, daemon paths, session, connection-defaults merge; produces theCliRunContextrunReactDevtoolsCli— the react-devtools special case (config + port-reverse callback)resolveRemoteContext— batch-step parsing, remote-auth resolution, remote-connection materialization, the open-without-runtime-hints warningbuildClientConfig— the client config projection (was a closure)maybeStartDaemonLogTail/createReplayReporterForTest— the two conditional setupsdispatchCliCommand— batch mapping + removed-runtimeerror + generic dispatch + unhandled-command throwhandleRunCliFailure+printDaemonLogTailOnError— the catch block, including theclose-with-no-daemon success pathBehavior preservation
The one structural subtlety: the old catch block read
effectiveFlags/resolvedRuntimemid-mutation through the closure — auth and materialization reassign them, and an error thrown between phases must be formatted with the partially-updated state.CliRunContextis therefore mutated in place byresolveRemoteContext(called out in the type's doc), sohandleRunCliFailuresees exactly what the throwing phase saw. Exit codes, error formatting, phase order (warnings → version → help → binding → react-devtools/web → upgrade notifier → batch parse → auth → materialization → warning → cdp → log tail → dispatch), and thefinallylog-tail stop are unchanged; special-case bodies were moved verbatim.Net +81 lines — this is a navigability refactor, not a deletion; the win is that each phase now has a name, an explicit signature, and a visible mutation contract.
Verification
typecheck·lint·check:fallow·format· full unit suite 487/487 (one earlier run hit the documented contention-timeout flake while three sibling worktrees ran concurrent suites; clean rerun on identical code).