refactor: move the remaining command wiring into per-feature factories - #45
Merged
Merged
Conversation
Every command except `create` now owns its own commander wiring. `device`, `doctor`, `emulator`, `playground`, `templates` and `webshell` each export `create<Name>Command(deps): Command` from `<name>-feature.ts`, which replaces the `<name>-feature-index.ts` barrel, and `createApp` registers them with `addCommand` in alphabetical order. `AppOptions` is now an intersection of the seven per-feature `<Name>CommandDeps` types plus `checkForNewerVersion` and `runCreate`, so `app.ts` names two dependencies instead of twenty-four and drops from 504 lines to 167. The feature files are wiring only, so the three barrels that held implementation split rather than move: `runPlayground` to `playground-feature-serve.ts`, doctor's orchestration to `doctor-feature-check.ts`, and doctor's pure functions over check results (`buildDoctorReport`, `deriveCapabilities`, `getDoctorExitCode`) to `data-access/doctor-report.ts` — which is where `test/doctor.test.ts` already imported everything else from. `src/templates.ts` keeps a byte-identical export surface, now sourced directly from `data-access/*` and the three `templates-feature-*.ts` files instead of through a barrel. 26 wiring tests move out of `test/core.test.ts` (1446 lines to 965) into the feature suites that own them, along with the `device tune` output-silencing helper. `core.test.ts` keeps only genuinely app-level tests, and gains one: the `addCommand` settings copy is now asserted across all seven feature commands at once rather than for localnet alone, so the localnet-specific guard added with the pilot is removed as duplicated. Verified by deleting the copy line — the new test fails, the suite goes to 1 fail. Behaviour is unchanged: 486 tests pass with and without a TTY, help output and command order are identical, the `emu` alias and the nested `emulator images` level still work, and every feature command still prints usage after an unknown-option error.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (22)
💤 Files with no reviewable changes (6)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe CLI now delegates command construction to feature-owned Commander factories. Each factory supports injectable runners, parses feature options, and handles local validation. The application root aggregates feature dependencies and registers the factories. Tests cover command registration, delegation, validation, and root settings. ChangesCLI command factories
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The command-factory refactor preserves the reviewed CLI behavior and has no identified merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant createApp
participant FeatureCommandFactory
participant FeatureRunner
createApp->>FeatureCommandFactory: create feature command with dependencies
createApp->>FeatureCommandFactory: register command
FeatureCommandFactory->>FeatureRunner: parse and forward command options
FeatureRunner-->>FeatureCommandFactory: return command result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This was referenced Sep 4, 2026
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.
Follow-up to #41, which moved localnet's wiring into a feature-owned factory. This does the same for the other six commands, leaving
createfor a third PR.src/app.tsdrops from 504 lines to 167 and now names two dependencies instead of twenty-four.What changed
Each of
device,doctor,emulator,playground,templatesandwebshellexportscreate<Name>Command(deps): Commandfrom<name>-feature.ts, which replaces its<name>-feature-index.tsbarrel.createAppregisters them withaddCommandin alphabetical order and forwards the whole options object; each factory picks its own keys and applies its own defaults.AppOptionsis now an intersection of the seven per-feature<Name>CommandDepstypes pluscheckForNewerVersionandrunCreate. The keys stay flat, so every existingcreateApp({ runEmulatorList: … })call site is untouched.The feature files are wiring only, so the three barrels that held implementation split rather than move:
playground-feature-index.ts(runPlayground)playground-feature-serve.tsdoctor-feature-index.ts(orchestration)doctor-feature-check.tsdoctor-feature-index.ts(pure report functions)data-access/doctor-report.tsDoctor's split follows what its test file was already saying:
test/doctor.test.tsimported everything fromdata-access/*exceptbuildDoctorReport,deriveCapabilitiesandgetDoctorExitCode, which it reached into the barrel for. Those are pure functions over check results, so they belong indata-accessand that file's imports are now single-layer.src/templates.ts— the published./templatessubpath export — keeps a byte-identical surface, now sourced directly fromdata-access/*and the threetemplates-feature-*.tsfiles. Verified by diffing its exported identifiers againstmain.Tests
26 wiring tests move out of
test/core.test.ts(1446 → 965 lines) into the feature suites that own them — 3 to device, 18 to emulator, 4 to templates, plus thedevice tuneoutput-silencing helper.core.test.tskeeps only genuinely app-level tests.It also gains one. The
addCommandsettings-copy from #41 is now asserted across all seven feature commands at once instead of localnet alone, so the localnet-specific guard is removed as duplicated — with seven commands on that path (eight after PR 3), a single-feature guard was the wrong shape. Verified by deleting the copy line increateApp: the new test fails and the suite goes to 1 fail.Verification
bun run ci— build, lint,tsc -b --noEmit, 486 pass / 0 fail, the same count as before the move. Also 486/0 under a pseudo-TTY.Behaviour is unchanged, smoke-tested on the things
addCommandcould plausibly have broken:emualiasemulator images installlevel--skip-version-checkaccepted on a deep subcommandimageslevelNo changeset: the
solana-mobilebin and the./templatesexport are unchanged.Follow-up
PR 3:
create— its 384-line barrel mixes template resolution, prompts and create-solana-dapp orchestration, which is a judgement-heavy split unrelated to commander and deserves its own review.Summary by CodeRabbit
New Features
Bug Fixes
Tests