Repository navigation
chore: Inject dispatcher runtime dependencies - #1479
Conversation
📝 WalkthroughWalkthroughThe dispatcher and launch subsystems are refactored to use explicit dependency-injection structs (dispatcherRunDeps, launchDeps) instead of package-level global function variables. Core entrypoints gain WithDeps variants threading these structs through launch handling, real-CLI execution, and self-update freshness enforcement. Tests are updated accordingly. ChangesDispatcher dependency injection refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant tryHandleLaunchRequestWithDeps
participant runLaunchWithDeps
participant launchDeps
Dispatcher->>tryHandleLaunchRequestWithDeps: launch request + deps
tryHandleLaunchRequestWithDeps->>runLaunchWithDeps: run(deps)
runLaunchWithDeps->>launchDeps: findRunningUnityProcess
runLaunchWithDeps->>launchDeps: waitForToolReadiness / killUnityProcess / resolveUnityExecutablePath
launchDeps-->>runLaunchWithDeps: readiness/process result
runLaunchWithDeps-->>tryHandleLaunchRequestWithDeps: exit code
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
4bd6787 to
00c342c
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cli/dispatcher/internal/dispatcher/launch_startup_timeout_error.go">
<violation number="1" location="cli/dispatcher/internal/dispatcher/launch_startup_timeout_error.go:27">
P3: Test error message still references the old function name `waitForLaunchReadiness`. Update it to `waitForLaunchReadinessWithDeps` so failure messages accurately identify the failing function.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
00c342c to
6b0b57e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cli/dispatcher/internal/dispatcher/launch_deps.go (1)
21-32: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
waitForUnityProcessExitdefault silently discards a caller's customdeps.
defaultLaunchDeps().waitForUnityProcessExitis wired to the package-levelwaitForUnityProcessExit(launch.go Line 319-321), which itself callswaitForUnityProcessExitWithDeps(..., defaultLaunchDeps())— rebuilding a brand-new defaultdepsrather than reusing thedepsthe caller constructed. Today this is harmless sincedefaultLaunchDeps()is the only production constructor, but it is a DI trap: if a future caller builds a partially-customizedlaunchDeps(e.g., overriding onlyfindRunningUnityProcess) without also overridingwaitForUnityProcessExit, that override would silently not apply during exit-wait polling, since the defaultwaitForUnityProcessExitfield ignores the containing struct entirely. Tests already work around this by explicitly overridingwaitForUnityProcessExitwhenever they stubfindRunningUnityProcessfor exit-wait scenarios (e.g.launch_test.goTestRunLaunchRestartWritesProcessTransitionResponse), which masks the issue but doesn't fix the underlying inconsistency.Consider having the default field close over the deps identity it belongs to isn't trivially possible in Go without extra indirection (chicken/egg), so a cleaner design might be to only keep
waitForUnityProcessExitWithDepsas the canonical implementation and drop the deps-freewaitForUnityProcessExitpackage function (if unused elsewhere), or clearly document that overridingfindRunningUnityProcessalone does not affect exit-wait behavior unlesswaitForUnityProcessExitis also overridden.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/dispatcher/internal/dispatcher/launch_deps.go` around lines 21 - 32, `defaultLaunchDeps` wires `waitForUnityProcessExit` to a deps-free wrapper that rebuilds `defaultLaunchDeps()`, so any caller-provided overrides in `launchDeps` are ignored during exit waiting. Update the `waitForUnityProcessExit` path in `launch.go` / `launchDeps` so it uses the same dependency set passed into the launch flow, or remove the wrapper and make `waitForUnityProcessExitWithDeps` the single canonical implementation. Make sure `findRunningUnityProcess` and other overridden fields still apply when `waitForUnityProcessExit` is invoked.cli/dispatcher/internal/dispatcher/dispatcher_test.go (1)
400-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDownstream of the
dispatcherReadInstalledVersionglobal noted inrun_dispatcher.go.This helper still saves/restores a package-level
dispatcherReadInstalledVersionvar (lines 407-411) while everything else (runUpdate, etc.) is now injected viadeps. See the companion comment onrun_dispatcher.go(lines 190-236) for the suggested fix of adding this todispatcherRunDepsinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/dispatcher/internal/dispatcher/dispatcher_test.go` around lines 400 - 413, Move the installed-version reader off the package-level dispatcherReadInstalledVersion global and into dispatcherRunDeps so stubDispatcherUpdateHooks can inject it like runUpdate and the other deps. Update the dispatcherRunDeps setup/defaultDispatcherRunDeps and the run_dispatcher.go call sites to read the installed version from deps instead of the global, then simplify stubDispatcherUpdateHooks to override only the dependency and restore state through the deps wiring.cli/dispatcher/internal/dispatcher/run_dispatcher.go (1)
190-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThread
dispatcherReadInstalledVersionthroughdispatcherRunDeps
dispatcherReadInstalledVersionis still a package-level mutable seam, and the tests patch it directly. Consider adding aninstalledVersion func(context.Context) (string, error)field todispatcherRunDepsand plumbing it throughdispatcherInstalledVersionOrEmptyso this dependency follows the same pattern asnow,runRealCLI, andrunUpdate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/dispatcher/internal/dispatcher/run_dispatcher.go` around lines 190 - 236, Thread the installed-version lookup through dispatcherRunDeps instead of relying on the package-level dispatcherReadInstalledVersion seam. Add an installedVersion func(context.Context) (string, error) field to dispatcherRunDeps, wire it into defaultDispatcherRunDeps and dispatcherInstalledVersionOrEmpty, and update enforceDispatcherFreshnessWithDeps to use the injected dependency so tests can override it consistently with now, runRealCLI, and runUpdate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/dispatcher/internal/dispatcher/dispatcher_test.go`:
- Around line 400-413: Move the installed-version reader off the package-level
dispatcherReadInstalledVersion global and into dispatcherRunDeps so
stubDispatcherUpdateHooks can inject it like runUpdate and the other deps.
Update the dispatcherRunDeps setup/defaultDispatcherRunDeps and the
run_dispatcher.go call sites to read the installed version from deps instead of
the global, then simplify stubDispatcherUpdateHooks to override only the
dependency and restore state through the deps wiring.
In `@cli/dispatcher/internal/dispatcher/launch_deps.go`:
- Around line 21-32: `defaultLaunchDeps` wires `waitForUnityProcessExit` to a
deps-free wrapper that rebuilds `defaultLaunchDeps()`, so any caller-provided
overrides in `launchDeps` are ignored during exit waiting. Update the
`waitForUnityProcessExit` path in `launch.go` / `launchDeps` so it uses the same
dependency set passed into the launch flow, or remove the wrapper and make
`waitForUnityProcessExitWithDeps` the single canonical implementation. Make sure
`findRunningUnityProcess` and other overridden fields still apply when
`waitForUnityProcessExit` is invoked.
In `@cli/dispatcher/internal/dispatcher/run_dispatcher.go`:
- Around line 190-236: Thread the installed-version lookup through
dispatcherRunDeps instead of relying on the package-level
dispatcherReadInstalledVersion seam. Add an installedVersion
func(context.Context) (string, error) field to dispatcherRunDeps, wire it into
defaultDispatcherRunDeps and dispatcherInstalledVersionOrEmpty, and update
enforceDispatcherFreshnessWithDeps to use the injected dependency so tests can
override it consistently with now, runRealCLI, and runUpdate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c13d0841-a899-41ca-b4c5-ae3fb97bf331
📒 Files selected for processing (8)
cli/dispatcher/internal/dispatcher/dispatcher_process.gocli/dispatcher/internal/dispatcher/dispatcher_test.gocli/dispatcher/internal/dispatcher/launch.gocli/dispatcher/internal/dispatcher/launch_deps.gocli/dispatcher/internal/dispatcher/launch_focus_log.gocli/dispatcher/internal/dispatcher/launch_startup_timeout_error.gocli/dispatcher/internal/dispatcher/launch_test.gocli/dispatcher/internal/dispatcher/run_dispatcher.go
1 participant
Summary
Verification