Skip to content

Withdraw reader keys before tab eviction and persist every shown error - #1323

Merged
trissim merged 4 commits into
mainfrom
work/toad-keymap--mount-errors
Oct 29, 2026
Merged

trissim merged 4 commits into
mainfrom
work/toad-keymap--mount-errors

Conversation

@trissim

@trissim trissim commented Oct 29, 2026 •

Copy link
Copy Markdown

Summary

4 commits; 14 production files; +177/-105 lines. ReadyContent construction sites: 3 → 1. Strict/optional Window lookup: 2 paths → 1. MainScreen's optional Conversation probes: 4 → 0. Field-validation logging: one error file per refused edit → zero file writes.

Repairs the missing-Window crash Tristan observed while switching tabs/scrolling and preserves actual exception evidence after an error toast disappears. The first head was sent back because late preparation could restore Ready after eviction, and expected field refusals were being sent to the new log path. This head includes those corrections on the same PR.

1. Content state owns mounted reader availability and completion

Before

OpeningContent.close → ReadyContent without a mounted tree
FailedContent.attached → ReadyContent before preparation
native tree eviction → deletes Conversation, leaves content Ready
_prepare_content → awaits mount + preparation, finishes through current screen.content
MainScreen focus/read callers → optional DOM probes

The initial mount was already awaited. The competing lifetime decisions were removal without state withdrawal, two false Ready writers, and completion through whichever content happened to be current after awaits. A tab switch could evict a still-opening tree while its original preparation was paused, then that stale operation would publish Ready over nothing.

After

ScreenContent.finish
  → accepts completion only from the current content object
  → installs/renders the result, then releases that object's completion waiters
OpeningContent.detaching
  → fresh OpeningContent carrying the same completion future
  → old preparation loses permission to finish; waiters stay with the new opening
ReadyContent.detaching → OpeningContent before native removal
ClosedContent → no reader keys, no preparation, no later revival
FailedContent.attached → OpeningContent → real preparation
_prepare_content → captures its starting content before any await
                 → success, refusal and cancellation finish through that object

There is one Ready constructor after successful preparation. Superseded completion cannot publish Ready or Failed onto a replacement, and cannot complete the replacement's waiter. Closing an opening tab releases the existing waiter without claiming mounted readiness. Closing switches state before sidebar teardown yields.

ReadyContent owns keyboard scopes, painted-cursor reads, prompt focus and sidebar return focus. Other states have no mounted-reader effects. All four optional Conversation probes in MainScreen and the duplicate optional Window lookup are deleted. The existing nested read acknowledgment/write operations are retained; no extra read-mark store or copied transcript state.

Pattern IDEN-3: mounted-reader availability was restated at removal, completion and DOM probes. Derive it through ScreenContent at those consumers, with identity-checked completion and pre-removal withdrawal. No is_mounted probe or exception suppression added to keyboard scopes.

2. The existing holder passes its screen through eviction

OperationalSessionPresentation.evict(screen) and remove_native_tree(screen) use the MainScreen already held by their callers. NativeSessionSurface retains the owner/screen pairs captured from _presentations, derives its budget groups from them, and passes the associated screen when evicting. Dispose passes its screen to the release callback.

SessionThreadSidebar has the same evict(screen) contract. MainScreen and sidebar retirement pass their existing screen through; panel capture no longer re-finds it from a weak reference. The local MainScreen import and Conversation ancestor query in removal are deleted.

Readiness withdrawal remains inside actual removal, after capture. P928's pending-Neovim-input refusal must return before capture/removal; that refused eviction therefore retains the actual view, engine and key availability. No competing engine/readiness writer is introduced.

3. Real exceptions are saved; expected refusals are not crash files

The existing diagnostic writer paths.save_traceback becomes paths.save_error(error: BaseException, prefix). It writes only exceptions, retaining complete traceback/chained causes, exclusive creation, mode 0600 and explicit UTF-8. The string branch is deleted from the writer.

ThemedApp.show_error saves a real exception before showing its short literal toast and saved path. ToadApp._handle_exception passes the exception intact. Store/navigation caught errors retain their objects. The CLI terminal-failure caller shares the same writer, and the separate history save is removed.

Log location: $XDG_STATE_HOME/toad/logs/<title>_<timestamp>.txt, default ~/.local/state/toad/logs/. Existing directory/filename ownership is reused; no second logger, daemon or log store. A write failure is not swallowed by show_error.

Core's public ResultShower.show_error contract still accepts Exception | str (agent_comms/cli_result.py:21). ThemedApp preserves that contract and decodes text at that one boundary into a notification without persistence. Paths receives exceptions only. No narrower incompatible override or compatibility alias was introduced. The four owned text-only callers now use native notifications directly: missing agent, unavailable theme, provider-login exit and the already-recorded agent-failure message.

Field refusal paint

ValueEditor no longer calls show_error on invalid edits. It owns transient feedback through Textual's existing var("", init=False) and emits a Feedback message only when that value changes; ValueView paints it in one inline FieldHelp-derived error-colour display. An accepted edit clears an existing refusal, but a clean edit on a clean field emits nothing. Identical repeated refusal text also emits nothing. The declaration requests no initial watcher, editor repaint or layout; no extra dirty flag or duplicate equality guard is added. This applies to the whole editor family, including multiline/select/checkbox controls, rather than fixing only numeric input. No toast, exception formatting, file creation or filesystem write lies on this refusal path.

paths is imported at module top. No new string-to-exception wrapper manufactures a fake traceback.

Source evidence

  • Base: ToAD 6d83f98acc0db434aa48365c518a86c0518418cf; Textual 79a8a6d69eed1d3300b54b12c490bc7d3753ee9c.
  • Original refactor-audit Package map: 308 ToAD + 256 Textual modules, 2,562 class declarations, 1,689 selected syntax sites, zero parse omissions. Rework map at 776f7f9 extends field/focus/lifetime coverage: 2,563 declarations, 1,914 selected syntax sites, zero omissions.
  • Retained maps: /home/ts/.cache/agent-scratch/toad-keymap/tab-error-owner-map.json and tab-error-rework-owner-map.json.
  • Semantic trace includes every content transition, direct preparation caller, native acquisition/removal/eviction, sidebar sibling, focus/read consumer, shown-error producer, public ResultShower contract and diagnostic writer.
  • AST is syntactic source evidence; dynamic bindings/MRO and actual UI scheduling are not proved by the inventory.
  • Fourteen changed modules parse/compile and the actual content/form/lifetime modules import. Declaration signatures read with Python3.14 Format.STRING to respect their TYPE_CHECKING forward references. The first inspection used VALUE evaluation and hit the existing forward-reference NameError; that failed diagnostic is retained, not reported as a pass.
  • git diff --check passes. Rework source log: /home/ts/.cache/agent-scratch/toad-keymap/tab-error-rework-source-strings.log. Final feedback source compile/import/declaration inspection: tab-error-feedback-source.log; BoolEditor, LineEditor and MultilineEditor inherit the same feedback declaration, with init/always_update/layout/repaint all false. No widgets instantiated. The retained Package map plus targeted AST/semantic read covers feedback producers, its sole watcher/constructor and ValueView consumer. Textual Reactive._set owns changed-value dispatch; no custom feedback equality mechanism.
  • Whole src/tests/tools caller search confirms the renamed writer has no remaining save_traceback caller; source has no optional Conversation/Window probes on the changed state surface, and field refusal no longer calls show_error.

Unrun controls and Assumed

No author automated tests, pilot, fake app, app launch or provider input: this checkout's CLAUDE.md prohibits them. Source checks are not a live fix claim. Root owns the combined real-app run and installation.

  1. Reproduce the original keyboard-scope failure on affected main, then check initial mount, evicted-tab return and keys while preparation is held. Switch away during the post-mount awaits and evict the opening view; releasing the old operation must not change the replacement state or release its waiter.
  2. Closing while opening, failed-attachment recovery and concurrent preparation completion retain correct state. Include first-paint/read-mark timing before/after preparation.
  3. An escaped handler exception/chained cause is on disk after the toast disappears, and the toast points at it. CLI terminal failure still saves. Expected text refusals do not create files.
  4. Refused form edits render literal inline feedback; accepted edits clear it. No error toast or new log file per keystroke, across line/multiline/select/checkbox editors.
  5. P928 integration retains its accepted flush fence before capture; a refused eviction does not withdraw keys. Reconcile the two PRs' shared session_presentation.py hunk, without moving readiness withdrawal before the fence.
  6. Known caller updates reserved to root under the author test-edit ban: pass each existing held screen into eviction/retirement in tests/support/core_adapters/mcp_permission_observer.py:104; tests/textual/test_tab_return_keeps_blocks.py:50; test_initial_prompt_handoff.py:63,87; test_plan_display.py:111; test_sidebar_retirement.py:135,143,217,234,257. Root was notified directly (bus37792). No compatibility default conceals these obsolete calls.
  7. Commander-layout owns Collect missing New-agent inputs without inventing empty channel membership #1322 required-input work and reserved the refusal sites for this PR; message-state owns separate queue work. No other worktree was modified.

105 lines deleted overall includes moved read-mark code and the replaced transition/probe/logging paths. This is not 103 distinct obligations discharged. The implementation remains under review, not merged or installed, and auto-merge was disabled for rework.

stale preparation says Ready
current Opening says “you don't work here anymore”
bad digit stays in the field; it no longer rents a file

Copilot AI balanced review requested due to automatic review settings October 29, 2026 07:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@trissim
trissim enabled auto-merge October 29, 2026 07:35
@trissim
trissim disabled auto-merge October 29, 2026 07:42
@trissim
trissim enabled auto-merge October 29, 2026 08:19
@trissim
trissim merged commit 369dbc3 into main Oct 29, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants