chore: remove unused TUI surface#103
Conversation
|
Important Review skippedToo many files! This PR contains 199 files, which is 49 over the limit of 150. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (199)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request performs a comprehensive removal of the Terminal User Interface (TUI) subsystem. Key changes include the deletion of the entire src/cli/tui directory, encompassing all TUI components, contexts, routes, and plugin infrastructure. Additionally, the @opentui/core and @opentui/solid dependencies have been removed from the project's configuration and lock files, and related build scripts and schema generation logic have been pruned. I have no feedback to provide.
7d8a98c to
e16f2a0
Compare
e16f2a0 to
5ee6c90
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Migrated the remaining actionable top-level review note into an inline comment.
5ee6c90 to
f813209
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
[P2] Minor test naming issues found during strict review.
1. packages/opencode/test/plugin/install.test.ts:151
Test name preserves JSONC comments when adding plugins to server and tui config still implies modification of tui config. The test verifies that existing tui.jsonc is left untouched when installing a package that exposes both server and tui exports.
Consider renaming to: preserves JSONC comments when adding plugins to server config, leaves existing tui config untouched or simply preserves JSONC comments when adding plugins to server config (tui exports ignored).
2. packages/opencode/test/plugin/install.test.ts:60
The plugin() fixture helper creates mock package.json exports including ./tui. This is valid for testing the "package exposes tui exports but we must not process them" scenario. However, the helper lacks a comment documenting this intent.
Adding a brief note like:
// Fixture helper for testing packages that expose server and/or tui exports.
// Used to verify: (1) server exports are processed, (2) tui exports are ignored.Would make the test's purpose clearer for future readers.
All CI passing. Core cleanup is complete and thorough. These are minor documentation polish items.
f813209 to
2a3cc04
Compare
There was a problem hiding this comment.
Code Review Report
Overall Assessment
The PR structure is clear, and the commit split is reviewable. Each commit has one reversible intent. The change correctly removes the TUI runtime and related dependencies.
P0 (Blocking)
None.
P1 (Should Fix)
See the inline comments below.
P2 (Notes)
- The bun.lock diff is large because it removes the OpenTUI dependency tree. This is expected.
- The new test names are clear, including no-tui-routes.test.ts and no-tui-event.test.ts.
- The OpenAPI diff is large because the TUI API surface was removed. This is expected.
Verification Points (All Passed)
| Check | Result |
|---|---|
| OpenTUI dependencies removed | Passed |
| CLI entry points have no TUI residue | Passed |
| SDK API removed correctly | Passed |
| OpenAPI has no /tui endpoints | Passed |
| Plugin exports narrowed correctly | Passed |
| Tests cover the behavior change | Passed |
| Config.Keybinds removed correctly | Passed |
| keybind.ts removed correctly | Passed |
Recommendation
After fixing the P1 comment issue, this should be ready to merge. The scope is controlled, the tests cover the behavior, and I did not find remaining TUI references that should block the PR.
2a3cc04 to
0c4033a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Overall this is a well-executed cleanup PR. The changes systematically remove TUI surface across packages/opencode, packages/plugin, and packages/sdk. The addition of regression tests (no-tui-routes.test.ts, no-tui-event.test.ts) is good practice.
A few minor observations for consideration:
1. patchName retains a now-meaningless parameter
packages/opencode/src/plugin/install.ts:329-330
Since Kind type now only contains "server", the _kind parameter serves no purpose. The underscore prefix correctly indicates it's unused, but conceptually the function could simplify to:
function patchName(): "opencode" {
return "opencode"
}Or even just inline "opencode" at the call site. Not a bug, just a small conceptual redundancy left over from the refactor.
2. readV1Plugin tui check order change
packages/opencode/src/plugin/shared.ts:283-300 (patch)
The original code checked typeof tui !== "function" before rejecting. After the patch, invalid tui exports (non-function) get the error "unsupported tui export" instead of "invalid tui export". This is arguably more appropriate since TUI is no longer a supported target at all, so "unsupported" is semantically correct. Just noting the change in error message semantics.
3. MCP auth no longer notifies via toast
packages/opencode/src/mcp/index.ts:335-346 (patch)
The MCP OAuth failure handling now returns Effect.succeed(undefined) instead of publishing TuiEvent.ToastShow. This is expected given TUI removal. Confirming that GUI has alternative notification mechanisms for these auth warnings (or that they're handled differently in the GUI client).
4. Test fixture comment explains legacy behavior
packages/opencode/test/plugin/install.test.ts:67
Good addition of the comment explaining why fixtures still generate tui exports:
// Fixtures may expose legacy tui exports to verify that plugin install rejects unsupported targets.5. Schema generator ignores extra argument
packages/opencode/script/schema.ts:51-59 (patch)
The script now accepts but ignores the second tuiFile argument. The test in schema.test.ts verifies this behavior by passing an extra file that should not be written. Clean approach.
No blocking issues. The PR accomplishes its stated goal of removing unused TUI surface while preserving server plugin functionality.
Astro-Han
left a comment
There was a problem hiding this comment.
Overall: well-executed cleanup. A few minor observations (P2, non-blocking).
0c4033a to
ddc6301
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Additional nitpicking review comments.
Astro-Han
left a comment
There was a problem hiding this comment.
More nitpicking review comments.
Astro-Han
left a comment
There was a problem hiding this comment.
Code logic nitpicking.
Astro-Han
left a comment
There was a problem hiding this comment.
Final nitpicking comment.
Astro-Han
left a comment
There was a problem hiding this comment.
Test fixture documentation nitpick.
ddc6301 to
35e8619
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Strict nitpicking review round 2.
Astro-Han
left a comment
There was a problem hiding this comment.
More nitpicking comments.
Astro-Han
left a comment
There was a problem hiding this comment.
Build script nitpick.
Astro-Han
left a comment
There was a problem hiding this comment.
Test coverage nitpick.
Astro-Han
left a comment
There was a problem hiding this comment.
Flag removal nitpick.
Astro-Han
left a comment
There was a problem hiding this comment.
Server routes nitpick.
Astro-Han
left a comment
There was a problem hiding this comment.
Package exports nitpick.
Astro-Han
left a comment
There was a problem hiding this comment.
Root package.json nitpick.
35e8619 to
e7b73c6
Compare
Summary
Remove the unused TUI surface now that PawWork is GUI-only:
oc-themespackages treated as unsupported for install/tuiroutes or TUI client helpersWhy
The Tauri compatibility cleanup from #93 is already merged. This completes the second cleanup slice from #91 by removing the unused TUI code path while keeping Electron GUI, embedded server, and PTY paths intact.
Related Issue
Closes #91
How To Verify
Also ran package-level typechecks/builds for
packages/opencode,packages/plugin,packages/sdk/js,packages/app, andpackages/desktop-electron, plus a final residual scan for TUI/OpenTUI symbols.Screenshots or Recordings
Not applicable. This removes unused runtime/API/build surfaces and does not add visible UI behavior.
Checklist
devbranch