Uh oh!
There was an error while loading. Please reload this page.
Delete the dead TUI, and add a check that could have seen it - #653
Conversation
There was a problem hiding this comment.
Pull request overview
Removes unreachable TUI APIs and adds an optional whole-program dead-code report, building on #652’s launcher safeguards.
Changes:
- Deletes two unused TUI implementations totaling 602 lines.
- Adds pinned production and dev-tagged dead-code scans.
- Updates launcher allowlisting and contributor documentation.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Makefile | Adds the non-gating deadcode target. |
internal/tui/spinner.go | Removes the unused spinner API. |
internal/tui/paginated_picker.go | Removes the unused paginated picker API. |
internal/tui/launchers_test.go | Removes deleted files from the launcher allowlist. |
AGENTS.md | Documents dead-code analysis usage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8e02a011db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
internal/tui/spinner.go and internal/tui/paginated_picker.go have no callers — 602 lines of exported API nothing in the repo or the binary reaches, each holding a tea.NewProgram. Verified symbol by symbol before removing; the surviving `Spinner` matches are bubbles/spinner's own field. Neither had a test file. Removing internal Go symbols does not touch .surface, which tracks the CLI command surface, so no .surface-breaking entry is needed. The reason this survived is worth fixing too. `unused` (staticcheck U1000) is the repo's only dead-code check, it runs per-package, and it treats every exported identifier in a non-main package as used by definition. It is structurally incapable of reporting any of this, and no configuration changes that. So add `make deadcode`, which starts from a main and follows the call graph. Rooted at ./cmd/basecamp, and again with -tags=dev, because those answer different questions. Not `./...`: only main packages are roots, so loading every package makes deadcode report everything no main reaches — 1100+ of 1250 lines here, nearly all the dev-tagged workspace tree, which the untagged build genuinely cannot reach. True, and useless. The tool version is pinned and invoked with `go run`, so the output does not depend on what happens to be installed. Deliberately NOT wired into `make check`. It is a report to read, not a gate: some zero-caller exported symbols are a deliberate API surface, and the dev tree is legitimately incomplete. Land the target, look at real output, decide separately. Adding a noisy gate is how a control nobody sized gets built. Scoped to the two TUI files. The other candidates the plan listed — internal/models, internal/sdk, sdk/errors, resolve/comment_thread.go — are a separate backlog; internal/sdk in particular is documented in AGENTS.md as the SDK wrapper and touching it means editing the SDK-sync workflow in the same breath.
Pure removal, plus one new (non-gating) make target. Separate from #652 so that stays reviewable as a bug fix.
Delete
internal/tui/spinner.go(187 lines) andinternal/tui/paginated_picker.go(415 lines) have no callers — 602 lines of exported API nothing in the repo or the binary reaches, each holding atea.NewProgram. Verified symbol by symbol before removing, not by file-level grep; the survivingSpinnermatches elsewhere arebubbles/spinner's own struct field. Neither file had a test.Removing internal Go symbols does not touch
.surface, which tracks the CLI command surface, so no.surface-breakingentry is needed.The allowlist in #652's launcher check shrinks from four files to two, which is the point — both deleted files were on it only as "no callers; slated for deletion."
Why
unusedcould never have found this.golangci.ymlenablesunused(staticcheck U1000), and it is the repo's only dead-code check. It runs per-package and treats every exported identifier in a non-mainpackage as used by definition. That is exactly why 602 lines of exported, zero-callerinternal/tuiAPI survived it. No configuration fixes this — the analysis cannot see across package boundaries.make deadcodegolang.org/x/tools/cmd/deadcodestarts from amainand follows the call graph, so it sees whatunusedstructurally cannot.Rooted at the binary, not
./.... Onlymainpackages are roots, sodeadcode ./...loads every package and then reports everything no main reaches as unreachable — 1100+ of its 1250 lines here are thedev-tagged workspace tree, which the untagged build genuinely cannot reach. True, and useless. Naming the binary gives two answers worth reading:The version is pinned and invoked via
go run, so output does not depend on what happens to be installed on the machine.Deliberately NOT wired into
make check. It is a report to read, not a gate: some zero-caller exported symbols are a deliberate API surface, and thedevtree is legitimately partial. Land the target, look at real output, decide separately. Adding a noisy gate is how a control nobody sized gets built.It earned its keep immediately — it flagged two helpers that #652 had orphaned, which were removed there.
Scope
Held to the two TUI files. The other candidates originally considered —
internal/models,internal/sdk,sdk/errors,resolve/comment_thread.go— are a separate backlog.internal/sdkin particular is documented inAGENTS.mdas the SDK wrapper, and deleting it would mean editing the SDK-sync workflow in the same breath.bin/cigreen on this branch.Summary by cubic
Removes two dead TUI launchers and adds a pinned whole-program dead-code report to catch similar cases. No CLI or runtime changes;
.surfaceunchanged.internal/tui/spinner.goandinternal/tui/paginated_picker.go; drop the dead-launchers block frominternal/tui/forms_test.go; shrink thebubbletealauncher allowlist ininternal/tui/launchers_test.go.make deadcode(pinnedgolang.org/x/tools/cmd/deadcode@v0.40.0) that analyzes./cmd/basecampand again with-tags=dev; invoked viago run; report-only (not inmake check) becauseunusedcannot see cross-package dead code.AGENTS.md(root at the binary; analyze one target at a time; for cross-target runs:go install golang.org/x/tools/cmd/deadcode@v0.40.0thenGOOS=windows deadcode ./cmd/basecamp). No migration required.Written for commit 4cf5fea. Summary will update on new commits.