fix switch bug(oss host), and optimise branch create flow.#112
Conversation
branch switch wrote oss_host as a bare hostname, so every later ossFetch threw "Failed to parse URL". Extract the buildOssHost helper that create/link already use into src/lib/config.ts and use it from switch too, so all three sites share one canonical URL builder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The POST /branches request can hang for ~2 minutes before returning, during which the CLI printed nothing — easy to mistake for a hang. Wrap createBranchApi and pollUntilReady in a single clack.spinner() that animates through the slow POST and updates its message in place on each provisioning state change. JSON mode is unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR centralizes OSS host URL construction into a shared utility, refactors commands to use it, normalizes legacy config, integrates a spinner into branch creation/polling/switch flows, updates tests, and bumps the package version to 0.1.70. ChangesOSS Host Utility Extraction
Branch Create Spinner Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)src/commands/branch/create.test.tsMicrosoft Presidio Analyzer failed to scan this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/branch/create.ts (1)
56-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDelay the success spinner until after auto-switch completes.
In the default
--switchpath, Lines 57-60 mark the flow as successful beforerunBranchSwitch(...)runs. If the switch step fails, the terminal still shows a success spinner entry even though the overall command errors. Keep the spinner active through the switch, or change it to a neutral “Switching context...” message first.🤖 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 `@src/commands/branch/create.ts` around lines 56 - 71, The success spinner is stopped before the optional auto-switch runs, which can show a success even if runBranchSwitch fails; modify the flow around spinner, runBranchSwitch, and outputJson so the spinner remains active through the switch: if opts.switch and ready.branch_state === 'ready' either change spinner text to a neutral message like "Switching context..." (using the existing spinner instance) while awaiting runBranchSwitch({ name, apiUrl, json, silent: json }), or defer calling spinner?.stop(...) until after runBranchSwitch completes successfully; ensure spinner is stopped with the appropriate success or error message and only then call outputJson({ branch: ready }).
🤖 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.
Outside diff comments:
In `@src/commands/branch/create.ts`:
- Around line 56-71: The success spinner is stopped before the optional
auto-switch runs, which can show a success even if runBranchSwitch fails; modify
the flow around spinner, runBranchSwitch, and outputJson so the spinner remains
active through the switch: if opts.switch and ready.branch_state === 'ready'
either change spinner text to a neutral message like "Switching context..."
(using the existing spinner instance) while awaiting runBranchSwitch({ name,
apiUrl, json, silent: json }), or defer calling spinner?.stop(...) until after
runBranchSwitch completes successfully; ensure spinner is stopped with the
appropriate success or error message and only then call outputJson({ branch:
ready }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3987b47e-087c-4c64-ab7f-3a85e95931c4
📒 Files selected for processing (7)
package.jsonsrc/commands/branch/create.tssrc/commands/branch/switch.test.tssrc/commands/branch/switch.tssrc/commands/create.tssrc/commands/projects/link.tssrc/lib/config.ts
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Code Review — #112
Summary: Solid bug fix and UX improvement. The https:// scheme fix in switch.ts is correct and the extraction of buildOssHost into config.ts properly eliminates three independent copies of the same string-building logic. The clack spinner in branch create is a clear UX win over the previous silent hang.
Requirements context
No /docs/superpowers/ directory exists in this repo. Review assessed against the PR description and code intent directly.
Findings
Critical
(none)
Suggestion
[Software Engineering] buildOssHost has no direct unit test — src/lib/config.ts:110
The bug being fixed (missing https:// scheme) was caused by an unchecked string expression. buildOssHost is now the canonical implementation used by four call sites, but it has no unit test of its own. The switch test (switch.test.ts:11) exercises the behavior end-to-end, but it does so via a mock that independently hard-codes the expected format — it doesn't call the real function. A trivial test in a config.test.ts would give early warning if the function's output ever drifts:
it('buildOssHost always returns an https URL', () => {
expect(buildOssHost('p1ky-x9p', 'us-east')).toBe('https://p1ky-x9p.us-east.insforge.app');
});[Software Engineering] No test covers the interactive (non-JSON) spinner path — src/commands/branch/create.ts:38-64
All three tests in create.test.ts invoke the command with --json, which sets spinner = null and skips every clack call. The spinner refactoring — spinner.start, spinner.message inside pollUntilReady, spinner.stop on success and error — is exercised zero times by the test suite. If @clack/prompts is not mocked and a future test removes --json, the spinner will hit a real TTY and may hang or throw. Consider adding one non-JSON happy-path test with @clack/prompts mocked (the mock used in link.test.ts can serve as a template):
vi.mock('@clack/prompts', () => ({
spinner: () => ({ start: vi.fn(), message: vi.fn(), stop: vi.fn() }),
}));Information
-
No security-relevant changes. All external inputs (branch name, mode) were already validated upstream; no new user input reaches SQL, shell, or HTTP without validation.
@clack/promptsis an existing dependency (not newly introduced). -
No performance concerns.
POLL_INTERVAL_MSandPOLL_TIMEOUT_MSare unchanged; the spinner merely replacesoutputInfocalls inside the polling loop. -
src/lib/config.ts:103-111JSDoc is well-written. The comment explains why the scheme is required (fetch()rejects bare hostnames), which is exactly the right level of documentation for a bug-prevention helper. -
Version bump (0.1.69 → 0.1.70) in
package.json. Consistent with what appears to be the project's release-in-PR pattern; nothing wrong here, just noting it is included.
Verdict
approved (informational — explicit GitHub approval is a separate human action)
No Critical findings. The two Suggestions (direct unit test for buildOssHost, non-JSON test for the spinner path) are worth addressing in a follow-up but are not blocking.
Address PR #112 review: - `getProjectConfig()` now repairs bare-hostname `oss_host` values written by the buggy `branch switch` (pre-0.1.70). Without this, users who already switched on the broken build stay broken until they re-switch. - `branch create` keeps the spinner active through `runBranchSwitch` (silent: true always now), so a switch failure renders as a red error frame instead of the misleading "✓ Branch is ready" line followed by an error. - Add direct unit test for `buildOssHost` and a non-JSON spinner-path test for `branch create` (mocks `@clack/prompts`), closing two gaps flagged in review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/commands/branch/create.ts`:
- Around line 71-73: The catch currently always reports "Branch '<name>'
creation failed" even if the error came from runBranchSwitch(); change the
control flow so errors from branch creation and errors from switching are
handled separately: move the runBranchSwitch() call into its own try/catch (or
detect and rethrow with a different error type/message) and call spinner?.stop
with "Branch '<name>' creation failed" only for creation errors, and with a
distinct message like "Switching to branch '<name>' failed" for
runBranchSwitch() failures; update the throw to rethrow the original error after
the appropriate spinner?.stop call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3d11b11-aeb9-4435-a46c-a089ac73a7e6
📒 Files selected for processing (4)
src/commands/branch/create.test.tssrc/commands/branch/create.tssrc/lib/config.test.tssrc/lib/config.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/config.test.ts
![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 across 4 files (changes from recent commits).
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="src/commands/branch/create.ts">
<violation number="1" location="src/commands/branch/create.ts:64">
P2: If `runBranchSwitch` throws, the catch block reports "creation failed" even though the branch was successfully created and provisioned. This misleads users into thinking the branch doesn't exist. Consider distinguishing switch failures from creation failures in the spinner stop message.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
If runBranchSwitch threw after the branch was successfully created and provisioned, the catch reported "Branch X creation failed" — but the branch was alive in the cloud, so users would either re-create it (name collision) or assume nothing happened. Track a `provisioned` flag and pick the right copy in catch: switch failures now say "Branch X is ready, but switching context failed — run \`insforge branch switch X\` to retry". Only true creation/poll failures keep the original message. Add a regression test mocking runBranchSwitch to throw and asserting the spinner does not say "creation failed". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary by cubic
Fixes branch switch writing a bare
oss_hostand auto-heals existing configs. Improves branch create with a single@clack/promptsspinner covering create, provisioning, and the auto-switch, with clear switch‑failure messaging; JSON mode unchanged.Bug Fixes
oss_hostviabuildOssHost()(https);getProjectConfig()normalizes legacy bare-host values on read.New Features
runBranchSwitchnow runs withsilent: true.{ branch }payload.Written for commit 34f5383. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests