Repository navigation
fix(tui): warn when CLI config is invalid - #56335
1052326311 wants to merge 4 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes the reported case. With an invalid top-level leader in cli.json, the base starts with defaults and shows nothing. With this PR, a warning toast names the file and the failing field (before/after screenshots below). The PR's config, TUI and Mini splash tests pass, and both packages type-check.
Public API: unchanged. The only addition is an optional diagnostic() on the internal CLI/TUI config interfaces. The cli.json format and its schema are not affected.
Concerns:
- The warning only appears at startup. The TUI watches
cli.jsonand reloads it when it changes. If you save an invalid edit while the TUI is running, every setting silently resets to the defaults and no warning appears. That is the same silent failure the issue reports, just on a path users hit often (see inline). The PR says a valid reload clears the diagnostic. That only updates the stored value; a toast that is already on screen stays for its full 15s. - JSON parse errors give a character offset, which is hard to find in an editor (inline).
- Settings passed through
OPENCODE_CLI_CONFIG_CONTENTare still dropped without any message. A minor gap, but the same problem.
Related: #55670
| ) | ||
| } | ||
|
|
||
| function ConfigDiagnosticToast(props: { diagnostic?: string }) { |
There was a problem hiding this comment.
The diagnostic is read once at startup, and this shows it once on mount. ConfigProvider (packages/tui/src/config/index.tsx) re-runs host.get() whenever cli.json changes. So if you introduce an invalid field while the TUI is running, apply({}) resets every setting to the defaults with no warning, which is the silent failure from #56318 again. Could the watcher's reload path read host.diagnostic?.() after each get() and show the toast when it changes (and possibly dismiss it once the file is valid again)? Then startup and live edits would both go through one place.
| const error = errors[0] | ||
| if (error) { | ||
| const field = getLocation(text, error.offset).path.join(".") || "root" | ||
| return { reason: `${field}: ${printParseErrorCode(error.error)} at offset ${error.offset}` } |
There was a problem hiding this comment.
at offset 123 is hard to act on in an editor. Converting the offset to a line:column (counting newlines in text.slice(0, error.offset)) would make parse errors much easier to fix.
|
Addressed both points from review 5556764944 in
Local verification: |
|
Merged Fresh local checks on this head: CLI config 20/20, TUI/Mini 57/57, TUI client MCP 3/3, core MCP 72/72; CLI/TUI/core |
There was a problem hiding this comment.
Both earlier points are fixed. I edited cli.json while the TUI was running. Adding an invalid field shows the warning naming leader, broken JSON reports a line and column (at 3:1), and fixing the file clears the toast. On the base, none of these edits shows anything. The CLI config tests and the TUI diagnostic, config and Mini splash tests pass, and both packages type-check.
Public API: unchanged. The only addition is an optional diagnostic on the internal CLI/TUI config interfaces.
The only new concern is the size and timing of the added live-reload tests (inline).
packages/tui/test/config-v2.test.tsx:331: This test waits on realfs.watchevents inBun.sleep(10)loops with 2s deadlines. On a slow or loaded CI machine the watcher can take longer, so it may fail at random. It also expects "mouse on" after the invalid reload only because{}falls back to the default, which is hard to follow. The live-reload behaviour is already covered inconfig-diagnostic.test.tsx. Could you drop this one, or reduce it to a single invalid-then-valid reload driven by the fixture rather than wall-clock polling?packages/tui/test/config-diagnostic.test.tsx:22: This test goes through a long chain of states (invalid, the same invalid file again, valid, invalid, then valid while another toast is showing) using read and check counters. That makes it hard to follow and adds a lot of timing-dependent steps. Please cut it down to the core case: an invalid edit shows the warning and a valid edit clears it. The other-toast case could be a small separate test if you want to keep it.
|
Addressed review 5624771262 in |
There was a problem hiding this comment.
The new commits only change tests and address the earlier test feedback. The polling test in config-v2.test.tsx is gone. The live-reload test in config-diagnostic.test.tsx is now two small tests that wait for rendered frames. They passed on each of three runs and packages/tui type-checks. Production code is unchanged since my last review, where I confirmed that live invalid edits show the warning and valid edits clear it.
Public API: unchanged. The only addition is an optional diagnostic on the internal CLI/TUI config interfaces.
No further concerns.
Issue for this PR
Closes #56318
Type of change
What does this PR do?
An invalid field in
cli.jsonmakes the whole file fall back to defaults without telling the user. This keeps strict whole-file rejection and the existing fallback, but reports the file and failing field in the standard TUI and Mini.The standard TUI warning now also follows live file edits. Repeated events with the same diagnostic do not repeat the warning; a valid reload clears it without dismissing an unrelated current notification. JSON parse errors report a line and column instead of an offset.
How did you verify your code works?
Verified at
0562f78cce57d4d9e64ea2c357182b07138b73f7againstdev@4d30794bcf4dccdcadbcb1f62bb35b4943a96426:cd packages/cli && bun test --timeout 30000 test/config.test.ts: 20 passed.cd packages/tui && bun test --timeout 30000 test/config-diagnostic.test.tsx test/config-v2.test.tsx test/mini/splash.test.ts: 57 passed;bun test --timeout 30000 test/client-mcp.test.tsx: 3 passed.cd packages/core && bun run script/test.ts test/mcp.test.ts: 72 passed.bun typecheckandbun run typecheck:tscpassed in CLI, TUI, and core. Rootbun run checkpassed (34/34 tasks), including the pre-push hook. The native macOS arm64 CLI build passed with--single --skip-install --skip-web-ui; the PR diff passesgit diff --check.dev.Test-only follow-up at 49e2920: Removed the duplicate fs.watch polling test and split the rendered invalid-edit/valid-recovery case from the unrelated-toast preservation case. On this head, the two focused TUI files passed 25/25 and 250/250 across ten reruns; CLI config passed 20/20. TUI bun typecheck and typecheck:tsc, root bun run check (34/34), Prettier on both changed tests, and git diff --check passed. Production files are unchanged from 0562f78.
Screenshots / recordings
The before image is a baseline-equivalent control frame with the diagnostic omitted, not a native screenshot from the old commit. Both images capture the real TUI renderer at startup; live reload is covered by the rendered TUI test above.
Checklist
Added by the opencode bot (AI-generated).
Related: #55670