TUI correctness pass — 14 bug fixes (T1–T14)#609
Merged
Conversation
…pp_state (T5) Dialog hardening: - New BaseDialog(ModalScreen[T]) in src/tui/dialogs/base.py owning a single priority escape binding and a dismiss() default. All eight dialogs (build_log, configuration, confirmation, device_details, file_path_input, help_dialog, profile_manager, search_filter) now subclass it. - Migrate dialog-title and dialog-buttons from IDs (#) to classes (.) in main.tcss and every dialog template. Reusing the same DOM id across a modal screen stack was the source of intermittent style/binding bleed. - ConfirmationDialog dismisses False on escape; HelpDialog dismisses True; SearchFilterDialog drops the redundant Cancel button (escape covers it). T5 — background device monitor: - BackgroundMonitor._monitor_device_changes now pushes through app_state.set_devices(...) instead of writing app._devices (which the reactive/filter chain does not observe). Switches to a signature-based change detector so we drop the dependency on the obsolete device_manager.check_for_changes() path. - New tests/tui/test_background_monitor.py covers the push-through and the no-op-on-unchanged-signature paths. Other small fixes piggybacking on the same surface: - ui_coordinator clears stale search_text filter when the input is empty. - main.py subscribes to AppState before installing action handlers so any early state mutation is observed. - VirtualDeviceTable shows an explicit empty-state row when scan returns no devices. All 38 tests in tests/tui pass.
…nt (T1) _get_app() walked .parent from a bound progress_callback method, which has no .app attribute — so it always returned None and every build took the container path. Stop walking the widget tree: store self._config = config at the top of start_build and have _validate_environment read it directly. _get_app is the only caller; remove it entirely. Regression test in tests/tui/test_build_orchestrator_local_build.py covers both the local-build and container-build paths plus verifies _get_app is gone.
handle_build_start awaited _validate_donor_module() but discarded the return value. A False return (user dismissed the install confirmation) let the build proceed anyway. Branch on the value: emit a 'Build cancelled' notice and return early. Regression test in tests/tui/test_handle_build_start_donor_cancel.py covers both branches.
on_data_table_row_selected did `device.bdf == event.row_key`. The underlying `row_key` is Textual's RowKey wrapper; older versions don't implement __eq__ against plain str, so selected_device stayed None and Start Build never enabled. Use `str(row_key.value)` (falling back to the RowKey itself for forward-compat), keeping the comparison stable across Textual versions. Regression test in tests/tui/test_row_selection.py models the old-Textual RowKey behaviour (no str-equality) to lock in the fix.
The five button handlers awaited _load_/_delete_/_export_/_import_/_create_new_profile methods that didn't exist on the dialog — every press raised AttributeError, swallowed silently. Implement them in-dialog using config_manager (already injected) and the simplest existing primitives: - Load: dismiss(name) so the caller in main.py applies the profile - Delete: ConfirmationDialog -> config_manager.delete_profile -> refresh - Export: FilePathInputDialog -> config_manager.export_profile(name, path) - Import: FilePathInputDialog -> config_manager.import_profile(path) -> refresh - Create New: FilePathInputDialog for the name -> save_profile(name, current) All five wrap config_manager calls in try/except and surface failures via self.app.notify(severity='error'). FilePathInputDialog is reused for the name prompt — marked with a TODO(tui-pass-2) to swap in a dedicated name-input primitive in the cleanup pass. Regression tests in tests/tui/test_profile_manager_dialog.py cover each method's happy path, cancel path, and one failure path.
update_state previously passed the live self._state to every subscriber. Any subscriber that mutated the second arg or triggered a re-entrant update_state corrupted the iteration. Pass a shallow copy of both old and new state, document the read-only contract, and snapshot the subscriber list so a callback that unsubscribes itself doesn't skip the next one. Regression test in tests/tui/test_app_state_subscribe.py covers both the mutate-snapshot path and the re-entrant update path.
The persistent-log override in PCILeechTUI.notify shadowed Textual's App.notify, so toasts never appeared and unknown-severity callers were silently dropped. Rename the override to log_notification and stop shadowing — Textual's notify is now reachable for ephemeral toasts. All in-scope persistent-log callsites updated to log_notification (deferred files left alone per the pass scope): src/tui/main.py — 36 callsites src/tui/core/ui_coordinator.py — 27 callsites src/tui/core/error_handler.py — 2 callsites src/tui/utils/graceful_degradation.py — 1 callsite src/tui/dialogs/profile_manager.py — 2 callsites (in helpers + 1 in _refresh) src/tui/dialogs/build_log.py — 5 callsites src/tui/dialogs/device_details.py — 1 callsite Out of scope (dead modules per the pass scope): src/tui/core/device_operations.py, src/tui/core/build_operations.py, src/tui/commands/. Severity values are unchanged at callsites; the log line is just a tag. Textual's notify is the canonical surface for toasts and accepts only 'information' / 'warning' / 'error' — new toast callers should use those. Tests in tests/tui/test_log_notification.py cover the rich-log write, the de-overridden notify, and the swallowed-error path.
…tor (T8) generate_donor_template (coordinator) and _generate_donor_template (main) both emitted the same '✓ Donor info template saved to: ...' line. Drop the duplicate from main.py; keep the unique informational hint about how to use the file. The error branch was dead too (coordinator already handles its own exceptions), so it's gone. Regression test in tests/tui/test_donor_template_notify_once.py asserts no 'saved' message originates from main and that the hint fires exactly once on success.
…ll (T9) Old monitored path: single stdout readline() loop with no stderr drain, no asyncio.wait_for timeout, no cancel poll inside the loop. A grandchild keeping stdout open (Vivado is the typical offender) blocks readline forever — the Stop button is inert during synthesis. New path: - Drain stdout and stderr concurrently as asyncio tasks; stderr is captured into a buffer for error reporting on failure. - Each readline is wrapped in asyncio.wait_for(timeout=READLINE_TIMEOUT_SEC) so the drainer wakes regularly enough to observe cancel/exit. - Main loop polls _should_cancel between process.wait() timeouts; on cancel it terminate()s, waits 2s, then kill()s if still alive. - READLINE_TIMEOUT_SEC = 1.0 module-level constant. Regression tests in tests/tui/test_run_shell_cancel.py: - assert the constant lives at module scope - spawn 'sleep 30', trip _should_cancel, assert it dies in <4s - spawn a process that closes stdout but lingers, assert no hang
The orchestrator held a single ThreadPoolExecutor for its whole lifetime; start_build's finally called shutdown(wait=True). A subsequent _update_resource_usage tick on the dead pool raised RuntimeError, and a second start_build on the same orchestrator was permanently broken. - self._executor: Optional[ThreadPoolExecutor] = None at rest - create in start_build right after the cancel/config setup - finally: nullify the attribute then shutdown the local handle - _update_resource_usage early-return when executor is None (late ticks after shutdown are now graceful no-ops) Note: the plan called out cancelling a 'resource monitor task' before shutdown — there isn't one in this codebase; _update_resource_usage is invoked inline from _update_progress, so the no-op guard is sufficient. Regression tests in tests/tui/test_executor_lifecycle.py cover the rest state, the late-tick no-op, and two sequential builds on one orchestrator.
… _run_shell (T11) Old assembly built a list of multi-word strings, joined them with ' ', then .split() before passing to _run_shell. Paths with spaces got shredded — a donor-info-file like '/tmp/foo bar/donor.json' became three argv entries. - Build args as proper list[str] from the start (one token per element). - Local path: ['python3', 'src/build.py', *build_args] - Container path: podman args + ['python3', '/app/src/build.py', *build_args] - _run_shell now uses shlex.join when given a list, so spaced tokens survive create_subprocess_shell's reparse. Regression test in tests/tui/test_vivado_cmd_quoting.py round-trips a spaced --donor-info-file through shlex.join/split and asserts _run_shell passes the same shell string.
…rors (T12)
Every field had been rendered as Input(str(value)); on save, the dialog
returned raw strings. Dict/list fields came back as repr ('{}', '[]'),
BuildConfiguration.from_dict failed silently in main's outer try/except,
and edits dropped on the floor.
- Bool fields render as Checkbox (returns native bool).
- Dict/list fields (custom_parameters, feature_flags, compatibility_overrides
plus any value that is dict/list at compose time) render as JSON in an
Input and parse with json.loads on save.
- Int/float fields parse with their constructor; ValueError surfaces via
app.log_notification(severity='error') and the dialog stays open so the
user can fix the value (no silent dismiss).
- main._open_configuration_dialog no longer silently swallows from_dict
errors — they're reported via log_notification, live config untouched.
Regression tests in tests/tui/test_configuration_dialog_types.py cover
dict/list round-trip, bool preservation, numeric parse, invalid-number
error path, and cancel.
set_timestamps was a @model_validator(mode='before') that unconditionally rewrote last_used = now() on every BuildConfiguration construction. Every read-path that round-tripped through the model (validate_config probe, load_profile pre-stamp, save_profile schema check) clobbered the field. - set_timestamps now only fills last_used when missing or None. - New BuildConfiguration.mark_used() records an explicit usage timestamp. - load_profile calls mark_used() exactly once on the real load path before save_profile persists. validate_config still doesn't save and no longer mutates last_used as a side-effect. Regression tests in tests/tui/test_last_used_preserved.py: - existing last_used round-trips unchanged - missing last_used is populated once and stable across rebuilds - mark_used updates the field - validate_config does not touch the on-disk file's mtime
DataTable.rows is an OrderedDict[RowKey, Row], not an ordered sequence. The old restore loop iterated it as a list and did row[0] == current_key, which subscripts a RowKey object — after a viewport shift, cursor landed on the wrong row (or row 0 by chance). Track the *absolute index* of the selected device into virtual_rows before the shift. After re-render, compute on-screen = absolute - new_visible_start. If the device scrolled out of the viewport, place the cursor at row 0. Use move_cursor(row=...) (falls back to direct cursor_row assignment for older Textual versions). Regression tests in tests/tui/test_virtual_device_table_cursor.py cover both the in-viewport-follow case and the out-of-viewport clamp case using Textual's run_test harness.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a broad TUI correctness pass covering build orchestration, dialog behavior, notification routing, state updates, profile handling, and regression coverage.
Changes:
- Fixes TUI state/selection/update paths, notification logging, and dialog escape/button behavior.
- Updates build orchestration for local builds, cancellation, executor lifecycle, and shell argument quoting.
- Adds focused regression tests for the reported T1–T14 correctness fixes.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/tui/core/app_state.py |
Sends copied state snapshots to subscribers. |
src/tui/core/background_monitor.py |
Updates device refresh flow through app state. |
src/tui/core/build_orchestrator.py |
Fixes local build validation, shell quoting, cancellation, and executor lifecycle. |
src/tui/core/config_manager.py |
Explicitly stamps profiles on load-to-use. |
src/tui/core/error_handler.py |
Routes errors to persistent notification logging. |
src/tui/core/ui_coordinator.py |
Updates notifications and donor validation cancellation behavior. |
src/tui/dialogs/base.py |
Adds shared modal dialog base with Escape dismiss. |
src/tui/dialogs/build_log.py |
Uses shared dialog styling/base and persistent notifications. |
src/tui/dialogs/configuration.py |
Adds typed config editing and validation feedback. |
src/tui/dialogs/confirmation.py |
Uses shared dialog base and Escape cancel semantics. |
src/tui/dialogs/device_details.py |
Uses shared dialog base/styling and log notifications. |
src/tui/dialogs/file_path_input.py |
Uses shared dialog base/styling. |
src/tui/dialogs/help_dialog.py |
Uses shared dialog base and Escape close behavior. |
src/tui/dialogs/profile_manager.py |
Implements profile load/delete/export/import/create actions. |
src/tui/dialogs/search_filter.py |
Uses shared dialog base and scoped dialog styling. |
src/tui/main.py |
Renames persistent notification method and fixes selection/config/template flows. |
src/tui/models/configuration.py |
Preserves last_used unless explicitly marked used. |
src/tui/styles/main.tcss |
Scopes dialog styling by class instead of IDs. |
src/tui/utils/graceful_degradation.py |
Routes feature notifications to persistent log. |
src/tui/widgets/virtual_device_table.py |
Adds empty state and fixes viewport cursor restoration. |
tests/tui/test_app_state_subscribe.py |
Adds state subscriber regression tests. |
tests/tui/test_background_monitor.py |
Adds background monitor regression tests. |
tests/tui/test_build_orchestrator_local_build.py |
Adds local/container validation tests. |
tests/tui/test_configuration_dialog_types.py |
Adds typed configuration dialog tests. |
tests/tui/test_dialogs_base.py |
Adds shared dialog Escape behavior tests. |
tests/tui/test_donor_template_notify_once.py |
Adds donor template notification dedupe tests. |
tests/tui/test_executor_lifecycle.py |
Adds executor lifecycle tests. |
tests/tui/test_handle_build_start_donor_cancel.py |
Adds donor validation cancellation tests. |
tests/tui/test_last_used_preserved.py |
Adds timestamp preservation tests. |
tests/tui/test_log_notification.py |
Adds notification rename behavior tests. |
tests/tui/test_profile_manager_dialog.py |
Adds profile manager action tests. |
tests/tui/test_row_selection.py |
Adds row key selection tests. |
tests/tui/test_run_shell_cancel.py |
Adds shell cancellation/readline tests. |
tests/tui/test_ui_coordinator.py |
Updates test app notification stub. |
tests/tui/test_virtual_device_table_cursor.py |
Adds viewport cursor regression tests. |
tests/tui/test_vivado_cmd_quoting.py |
Adds shell quoting regression tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+126
to
+130
| signature = tuple( | ||
| (d.bdf, d.driver, d.is_suitable) for d in devices | ||
| ) | ||
|
|
||
| if has_changes: | ||
| # Only update if devices have changed | ||
| devices = await self.app.device_manager.get_devices() | ||
| if signature != self._last_status.get("devices_signature"): |
Comment on lines
+632
to
+633
| fire. For ephemeral toasts (Textual's `Notification`), call | ||
| `self.log_notification(...)` directly — that's now Textual's built-in. |
|
|
||
| Bool fields render as a Checkbox; dict/list fields render as JSON in an | ||
| Input and parse with json.loads on save; int/float fields parse with | ||
| their constructor and surface errors via app.notify instead of silently |
Comment on lines
+166
to
+169
| try: | ||
| ok = self.config_manager.export_profile(name, Path(path_str)) | ||
| if ok: | ||
| self._notify_info(f"Exported profile '{name}' to {path_str}") |
Comment on lines
+57
to
+60
| cmd = ( | ||
| 'python3 -c "import os,sys,time; ' | ||
| 'print(\'hello\'); sys.stdout.close(); os.close(1); time.sleep(0.5)"' | ||
| ) |
review) After T13, export_profile -> load_profile -> mark_used + save mutated the source profile's timestamp on every export. Extract _load_profile_raw (no stamping, no save-back); export_profile uses that. load_profile still does the stamp + save on the real load-to-use path. Regression test: export a profile and assert its source mtime and content are unchanged. Caught by Copilot in PR #609 review.
…ields (PR #609 review) The (bdf, driver, is_suitable) signature missed updates to vendor/device name, IOMMU group, suitability score, and status indicator — all of which VirtualDeviceTable._add_device_row renders. A rescan that changed only those would silently leave stale UI. Include them in the signature. New test asserts a vendor-name-only change still triggers set_devices. Caught by Copilot in PR #609 review.
…review) sys.stdout.close() in the test child already closes fd 1; the trailing os.close(1) raised OSError so the subprocess exited non-zero, meaning the 'closes stdout but keeps running' test was passing for the wrong reason. Remove the second close. Caught by Copilot in PR #609 review.
…string (PR #609 review) CodeQL flagged five non-fatal exception blocks as unexplained. They are intentional (best-effort UI notification, decorative empty-state row, Textual-version-fallback cursor restore) — add a one-line rationale to each. Also update ConfigurationDialog docstring to reference log_notification (post-T7 rename) instead of the old app.notify.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the full 14-task plan in
docs/plans/tui-correctness-pass.md. One commit per task, each with a regression test undertests/tui/. Scope is bug fixes only — no refactors, no dead-code deletion (deferred to a separate pass).BuildOrchestrator._validate_environmenthonorslocal_buildvia stored config instead of a no-op widget walkhandle_build_startaborts the build when_validate_donor_module()returns Falseevent.row_key.valueinstead of the wrapperProfileManagerDialogimplements_load/_delete/_export/_import/_create_new_profile(button presses no longer silently raise)app_state.set_devices(...)and uses a signature-based change detector; newBaseDialogextracted, dialog CSS scoped to classesAppState.update_statepasses shallow copies of old/new state to subscribers; subscriber list snapshotted before iterationApp.notifyoverride renamed tolog_notification; Textual'snotifyleft intact for toasts; 74 callsites migrated_run_shelldrains stdout+stderr concurrently with aREADLINE_TIMEOUT_SEC=1.0poll, honors_should_cancel, terminate→kill on cancelThreadPoolExecutoris build-scoped;_update_resource_usageis a graceful no-op when the pool is gonelist[str]andshlex.join'd in_run_shellso spaced paths surviveConfigurationDialogrenders Checkbox for bools, JSON-encodes dict/list fields, parses numerics with error notify; main surfacesfrom_dicterrorsset_timestampsvalidator only fillslast_usedwhen missing; newmark_used()is the explicit usage stamp;validate_configis read-onlyVirtualDeviceTableviewport-shift cursor restore tracks the absolute index intovirtual_rows; clamps to 0 when scrolled outTest plan
pytest tests/tui -q— 81/81 pass (was 36 before this branch; +45 new regression tests)local_build=Trueconfig now takes the local path on a non-root host (T1).last_usedtimestamp (stat profiles/<name>.jsonbefore/after).