Repository navigation
fix: harden GUI-SDL model and Windows ConPTY terminal handling - #18
Conversation
Codex Review Summary: ✅ CompletedThis 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. |
Verification update — 2026-11-07All Frontend-owned CI checks are now green for PR #18 head
The five findings on merged PR #17 have been corrected in this follow-up and all five review threads on #17 are resolved. The only pending integration check is CDIN PR #110's live GUI/TUI path using this exact pinned Frontend commit. PR #18 remains open for review; the remaining GUI product/release scope is tracked in Issue #15. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 258e93d503
ℹ️ 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".
| return -1; | ||
| } | ||
| } | ||
| if (!ReadFile(context->input, buffer, requested, &read_count, NULL)) return -1; |
There was a problem hiding this comment.
When the readiness-driven TUI loop drains terminal input until AGAIN, this unconditional ReadFile can block on the next call after the current console input is consumed, preventing the loop from servicing Backend readiness. The removed availability check specifically prevented that second read from blocking; restore a nonblocking empty-input result.
Useful? React with 👍 / 👎.
| context->input = CreateFileW( | ||
| L"CONIN$", GENERIC_READ | GENERIC_WRITE, | ||
| FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); | ||
| if (context->input == INVALID_HANDLE_VALUE) return -1; | ||
| context->owns_input = 1; |
There was a problem hiding this comment.
In the redirected-stdio ConPTY fallback, terminal_read now reads from CONIN$, but this leaves STD_INPUT_HANDLE unchanged. The standalone preview waits on GetStdHandle(STD_INPUT_HANDLE) in wait_for_terminal_input(); when that redirected pipe has no writer, keyboard input on CONIN$ will not wake the wait. Keep the readiness handle aligned with the handle being read, or expose the console handle to the wait bridge.
Useful? React with 👍 / 👎.
| assert(cdin_frontend_gui_sdl_create(&config, &gui) == | ||
| CDIN_FRONTEND_GUI_SDL_INVALID_ARGUMENT); |
There was a problem hiding this comment.
With cell_session.units = "cells", cdin_frontend_gui_sdl_create() still succeeds because its validation does not check session->units. This new assertion therefore fails when the SDL adapter smoke runs, and the adapter can send pixel dimensions as cell-based resizes; reject non-pixel sessions in the production create path.
Useful? React with 👍 / 👎.
Summary
Follow up the merged GUI-SDL foundation PR #17 with the review corrections and the Windows terminal fix required by CDIN's live TUI smoke.
CONIN$/CONOUT$when ConPTY or redirected standard streams are pipes; preserve/restore console modes and close only handles owned by the adapter.view.linesresponses with the outstanding first-line, line-count and document-version request; cancel the request when a newer document version commits.Verification
Scope
This is a hardening follow-up for Issue #15, not completion of the full desktop editor. PaneTree rendering, production Unicode shaping/bidi/IME, full selection/cursor presentation, panels/widgets and release-grade device-loss recovery remain open acceptance work.
Latest CI result — 2026-11-07
All Frontend-owned checks now pass for head
1fb73848966af166ec9c0b8f1ca317247563649d: Windows, Linux, macOS, Pipeline quality, SDL3 AddressSanitizer model/adapter tests, installed package and clean-prefix downstream CMake consumer. See run 42309550750.The five review findings on merged PR #17 have been corrected here and their original review threads are resolved. CDIN's cross-repository live integration is still a separate outstanding gate; PR #110 pins this exact Frontend commit and must pass its new Windows ConPTY and three-platform E2E run before this integration fix is considered verified end-to-end.