fix(jsonview): use terminal rows for startup preload - #165
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 5 mixed files remain; 0 existing customizations changed. Compared 5 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34176149161 --repo openai/openai-cli \
--name castiron-custom-code-34176149161-1 --dir /tmp/castiron-custom-code-34176149161-1
git apply --stat /tmp/castiron-custom-code-34176149161-1/custom-code.patch
cat /tmp/castiron-custom-code-34176149161-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 094a4ab96e80c2952c830ae36a9a9f009617e5ec 01fa9c580e39eaef5d9092c5a7cc8f564b55bbb4
python3 scripts/castiron/custom_code_report.py report \
--base 094a4ab96e80c2952c830ae36a9a9f009617e5ec \
--head 01fa9c580e39eaef5d9092c5a7cc8f564b55bbb4 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-01fa9c580e39
cat /tmp/castiron-custom-code-01fa9c580e39/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
There was a problem hiding this comment.
🟡 Changes recommended
The new Linux preload test mutates global os.Stdout in a package with many t.Parallel() tests, which can introduce race/flakiness under concurrent test execution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the JSON interactive explorer’s startup preload logic by using the terminal row count (height) instead of the column count (width), preventing terminal width changes from inadvertently increasing the initial number of prefetched items and triggering iterator errors before the UI starts.
Changes:
- Correct
term.GetSizetuple usage inExploreJSONStreamso the initial preload count is based on terminal rows. - Add a Linux-only PTY regression test covering asymmetric terminal dimensions and non-terminal fallback behavior.
File summaries
| File | Description |
|---|---|
| internal/jsonview/explorer.go | Uses terminal height (rows) rather than width (columns) for initial preload sizing. |
| internal/jsonview/explorer_preload_linux_test.go | Adds Linux PTY tests validating preload sizing by rows and verifying the non-terminal fallback path. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 01fa9c5. term.GetSize returns width then height, so using the second value correctly bounds startup preload by terminal rows. The asymmetric PTY cases cover width invariance, row changes, and non-terminal fallback. The tests do not call t.Parallel, so their temporary os.Stdout swap cannot overlap package parallel tests under Go test scheduling. go test -race ./internal/jsonview, gofmt, go vet, and diff checks pass; hosted CI is green.
Problem and change
The interactive explorer assigns the first
term.GetSizeresult (columns) to its startup preload count. With 24 rows, widening a terminal from 40 to 80 columns therefore increases initial consumption from 40 to 80 items, and can reach an iterator error before the UI appears.Use the second result (rows) for initial prefetch. Both widths now preload 24 items; a 36-row terminal preloads 36. The existing fallback of 20 on terminal-size errors remains. This does not impose a total item limit: existing lazy loading continues beyond the initial batch. RawJSON handling, retained rows, navigation, and iterator error behavior are unchanged.
Add a Linux PTY regression through
ExploreJSONStreamfor asymmetric dimensions and nonterminal fallback. The test stops at the existing post-preload error check before starting the UI.Validation
go test -mod=readonly ./internal/jsonview -count=1,go test -mod=readonly -race ./internal/jsonview -count=1,go test -mod=readonly ./internal/...,go test -mod=readonly ./... -run '^$',go mod verify, andGOFLAGS=-mod=readonly ./scripts/lint(build only)../scripts/testmock suite. Verified pinned Steady/Deno setup could not download the JSR manifest in this environment. No network/security protection was bypassed. No native Mac test run is claimed; full mock validation remains for CI.