201 lines
13 KiB
Markdown
201 lines
13 KiB
Markdown
# Live-GUI State Sync — Design
|
|
|
|
**Date:** 2026-06-05
|
|
**Status:** Draft
|
|
**Track:** live_gui_state_sync_20260605 (sub-project of v2)
|
|
|
|
## Problem Statement
|
|
|
|
`App` (`src/gui_2.py`) and `AppController` (`src/app_controller.py`) maintain **parallel state** for the same logical fields. `set_value` writes to the **Controller**, but several code paths read from the **App**, returning stale or wrong values.
|
|
|
|
### Concrete failures (from 2026-06-05 batched test run, batches 7, 46, 65, 68)
|
|
|
|
1. **`test_auto_switch_sim::test_auto_switch_sim`** — sets `ui_separate_tier1=True` and `show_windows['Diagnostics']=True`, saves `Tier3Profile`, sets to False, triggers tier-3 auto-switch. Expects `show_windows['Diagnostics']=True` restored. **Fails: profile captures from App but is set on Controller.**
|
|
|
|
2. **`test_workspace_profiles_restoration::test_workspace_profiles_restoration`** — sets `ui_separate_tier1=True`, saves `test_restore`, sets to False, loads. Expects True. **Fails: same root cause.**
|
|
|
|
3. **`test_undo_redo_lifecycle::test_undo_redo_lifecycle`** (NEW regression) — sets `ai_input="Initial Input"`, modifies to `"Modified Input"`, clicks `btn_undo`. Expects `ai_input="Initial Input"`. **Fails: snapshot reads `app.ui_ai_input` but `set_value` writes to `controller.ui_ai_input`.**
|
|
|
|
### Discovery (2026-06-05 execution): State sync is NOT the root cause
|
|
|
|
Initial hypothesis: App and Controller maintain parallel state for settable fields. Verified during execution: **the App class already has `__getattr__` (line 478) and `__setattr__` (line 483) that auto-delegate to the controller.** Writes go through `__setattr__` → controller. Reads go through `__getattr__` → controller. The state is correctly synced at the descriptor level. The original spec assumption was wrong.
|
|
|
|
## REAL root cause: `_capture_workspace_profile` is not a class method
|
|
|
|
During execution, AST analysis of `src/gui_2.py` reveals the actual bug:
|
|
|
|
```
|
|
$ uv run python -c "import ast; ..."
|
|
App methods (count): 59
|
|
WORKSPACE METHOD: _apply_workspace_profile # ← exists
|
|
# ← _capture_workspace_profile MISSING
|
|
```
|
|
|
|
`_capture_workspace_profile` is defined at line 607 of `src/gui_2.py` with 2-space indent (intended as a class method), but the AST walks it as **nested inside `_apply_snapshot`** (line 572). The body of `_apply_snapshot` (lines 573-635) absorbs the next `def` as a nested function.
|
|
|
|
This means when the live_gui calls `self._app._capture_workspace_profile(name)`, Python's normal class lookup fails to find `_capture_workspace_profile` on the App class. `__getattr__('_capture_workspace_profile')` is triggered, which delegates to `self.controller._capture_workspace_profile`. The controller does NOT have this method. `AttributeError` is raised. The save callback fails silently. The test's `load_workspace_profile` finds no profile to load (because save failed). The test fails.
|
|
|
|
### Why AST sees it as nested
|
|
|
|
The likely cause is the user's recent cleanup commit `873edf42` ("began to go through the files and organize imports and gui_2.py's new context defs") which touched `src/gui_2.py:261` lines. The cleanup reorganized method placement. Either:
|
|
- Indentation was accidentally off by 1 space on some lines.
|
|
- A blank line or comment that closed a function body was removed.
|
|
- Method definitions were moved but their indentation wasn't updated.
|
|
|
|
Specific to the bug: `_apply_snapshot` has a `try:` (line 574) without an `except` (only a `finally:` at line 604). This is valid Python syntax, but the indentation of subsequent lines may have been off, causing the AST to consume the next `def` into the `try` block.
|
|
|
|
## Audit of duplicated fields (retained from original spec, for context)
|
|
|
|
Static analysis of the 71 settable fields in `AppController._settable_fields` vs the 12 `panel_states` keys captured in `App._capture_workspace_profile`, plus the `show_windows` dict and snapshot fields:
|
|
|
|
| Field | In `_settable_fields` (Controller)? | Read by App code? | Sync bug? |
|
|
|---|---|---|---|
|
|
| `show_windows` | yes | `_capture_workspace_profile` (line 627), `_apply_workspace_profile` (line 633) | **YES** |
|
|
| `ui_separate_task_dag` | yes | `_capture_workspace_profile` (line 615) | **YES** |
|
|
| `ui_separate_usage_analytics` | yes | `_capture_workspace_profile` (line 616) | **YES** |
|
|
| `ui_separate_tier1` | yes | `_capture_workspace_profile` (line 617) | **YES** |
|
|
| `ui_separate_tier2` | yes | `_capture_workspace_profile` (line 618) | **YES** |
|
|
| `ui_separate_tier3` | yes | `_capture_workspace_profile` (line 619) | **YES** |
|
|
| `ui_separate_tier4` | yes | `_capture_workspace_profile` (line 620) | **YES** |
|
|
| `ui_ai_input` | yes (`ai_input -> ui_ai_input`) | `_take_snapshot` (line 551), `_apply_snapshot` (line 569) | **YES** |
|
|
| `ui_separate_context_preview` | no (NOT in settable_fields) | `_capture_workspace_profile` (line 611) | no — App-only |
|
|
| `ui_separate_message_panel` | no | `_capture_workspace_profile` (line 612) | no — App-only |
|
|
| `ui_separate_response_panel` | no | `_capture_workspace_profile` (line 613) | no — App-only |
|
|
| `ui_separate_tool_calls_panel` | no | `_capture_workspace_profile` (line 614) | no — App-only |
|
|
| `ui_separate_external_tools` | no | `_capture_workspace_profile` (line 621) | no — App-only |
|
|
| `ui_discussion_split_h` | no | `_capture_workspace_profile` (line 622) | no — App-only |
|
|
|
|
**8 confirmed sync bugs.** Plus `ui_ai_input` (snapshot) is a 9th.
|
|
|
|
## Root Cause
|
|
|
|
`App.__init__` creates a separate `AppController` instance and later sets `self.controller._app = self` (bidirectional link). The two objects each declare their own `self.ui_separate_tier1 = False` (App) and `self.ui_separate_tier1 = False` (Controller) in their respective `__init__`s. They are independent Python attributes.
|
|
|
|
`set_value` (`src/api_hooks.py`, line 614) calls `setattr(controller, attr_name, value)` — writes to Controller. But `_capture_workspace_profile` reads `self.ui_separate_tier1` where `self` is the App — never updated.
|
|
|
|
## Design
|
|
|
|
### Goal
|
|
|
|
Eliminate the dual state. **Single source of truth: the Controller.** The App becomes a thin "view" layer that exposes Controller fields as Python properties. `set_value` continues to write to the Controller. All reads (from save, snapshot, render) transparently read from the Controller.
|
|
|
|
### Approach: Properties on App that delegate to Controller
|
|
|
|
Add `@property` definitions on the `App` class for each field that has a Controller counterpart. The getter returns `self.controller.X`. The setter (where App code writes, e.g. snapshot restore) also delegates to `self.controller.X`.
|
|
|
|
**Hypothetical example for `ui_separate_tier1`:**
|
|
|
|
```python
|
|
# In App class (src/gui_2.py)
|
|
|
|
@property
|
|
def ui_separate_tier1(self) -> bool:
|
|
return self.controller.ui_separate_tier1
|
|
|
|
@ui_separate_tier1.setter
|
|
def ui_separate_tier1(self, value: bool) -> None:
|
|
self.controller.ui_separate_tier1 = value
|
|
```
|
|
|
|
This makes `app.ui_separate_tier1` and `controller.ui_separate_tier1` the same value, regardless of which path writes. The only writes are via the property setter (or `set_value` via the Controller directly), and all reads go through the getter.
|
|
|
|
### Why this approach
|
|
|
|
- **Minimal blast radius**: The App class only adds properties; no method bodies change. Methods that read `self.X` continue to work — they just get the Controller's value via the property.
|
|
- **Bidirectional**: Setter support is critical for `_apply_snapshot` and `_apply_workspace_profile` which set App fields directly (`self.ui_ai_input = snapshot.ai_input`). They go through the property setter, which writes to the Controller.
|
|
- **No double-write footgun**: A "sync on set_value" alternative requires remembering to write to BOTH objects. A property approach is a single point of truth.
|
|
- **Easy to migrate incrementally**: Each field is one property pair. Can be added one at a time with a regression test for each.
|
|
|
|
### Alternatives considered
|
|
|
|
- **A2: Merge App and Controller into one class.** Rejected: would be a 5532-line → 4000-line merge with high risk. The Controller already lives in a separate file; the App delegates to it via `self.controller.X`. Merging would lose the existing boundary.
|
|
- **A3: Sync on every set_value (write to both).** Rejected: requires touching every writer; easy to miss a site. Property approach is one place per field.
|
|
- **A4: Pass Controller as a method argument everywhere.** Rejected: invasive; requires changing method signatures throughout `gui_2.py` and `app_controller.py`.
|
|
|
|
## File Changes
|
|
|
|
### Modify: `src/gui_2.py` (App class)
|
|
|
|
Add `@property` + `@X.setter` for each of the 8 sync-bug fields, plus `ui_ai_input`:
|
|
|
|
```python
|
|
@property
|
|
def ui_separate_tier1(self) -> bool:
|
|
return self.controller.ui_separate_tier1
|
|
|
|
@ui_separate_tier1.setter
|
|
def ui_separate_tier1(self, value: bool) -> None:
|
|
self.controller.ui_separate_tier1 = value
|
|
```
|
|
|
|
Fields to add properties for:
|
|
- `ui_ai_input` (snapshot bug)
|
|
- `ui_separate_task_dag`
|
|
- `ui_separate_usage_analytics`
|
|
- `ui_separate_tier1` through `ui_separate_tier4`
|
|
- `show_windows` (special: dict, not bool)
|
|
|
|
For `show_windows`, the property needs care — `set_value` may pass a new dict; the property should do `self.controller.show_windows = value` to allow full replacement, but for in-place updates (`self.show_windows["X"] = True`), the property getter returns the Controller's dict reference (so in-place mutations work) and the property setter can either replace or do nothing (since the dict is shared).
|
|
|
|
```python
|
|
@property
|
|
def show_windows(self) -> Dict[str, bool]:
|
|
return self.controller.show_windows
|
|
|
|
@show_windows.setter
|
|
def show_windows(self, value: Dict[str, bool]) -> None:
|
|
self.controller.show_windows = value
|
|
```
|
|
|
|
**Do NOT** add properties for fields that are App-only (no Controller counterpart): `ui_separate_context_preview`, `ui_separate_message_panel`, `ui_separate_response_panel`, `ui_separate_tool_calls_panel`, `ui_separate_external_tools`, `ui_discussion_split_h`, etc. — they remain as plain App attributes.
|
|
|
|
### Add: `tests/test_app_controller_state_sync.py` (new)
|
|
|
|
A new unit test that encodes the contract: **for every field in `_settable_fields` that is also referenced as `self.X` in the App class's `_capture_workspace_profile` and `_take_snapshot`/`_apply_snapshot`, writes to `app.X` and `controller.X` must be observed by both.**
|
|
|
|
```python
|
|
def test_ui_separate_tier1_setter_delegates_to_controller():
|
|
"""The App's ui_separate_tier1 property is a delegate to the Controller.
|
|
Writes through app.ui_separate_tier1 = X are visible at controller.ui_separate_tier1,
|
|
and writes through set_value (which goes to controller) are visible at app.ui_separate_tier1."""
|
|
from src import app_controller, gui_2
|
|
from src.app_controller import AppController
|
|
# Don't fully init App (too heavy); use lightweight setup
|
|
app = gui_2.App.__new__(gui_2.App)
|
|
app.controller = AppController()
|
|
app._app = app # back-ref
|
|
# set_value goes to controller
|
|
app.controller.ui_separate_tier1 = True
|
|
assert app.ui_separate_tier1 is True # reads through property
|
|
# direct set through app's property
|
|
app.ui_separate_tier1 = False
|
|
assert app.controller.ui_separate_tier1 is False # write visible at controller
|
|
```
|
|
|
|
This is a regression test for the contract.
|
|
|
|
### Test impact
|
|
|
|
After the fix, these tests should pass:
|
|
- `test_auto_switch_sim::test_auto_switch_sim` (writes to `app.show_windows` and `app.ui_separate_tier1` are observed by save)
|
|
- `test_workspace_profiles_sim::test_workspace_profiles_restoration` (same)
|
|
- `test_undo_redo_lifecycle::test_undo_redo_lifecycle` (snapshot reads from `app.ui_ai_input` get the Controller's value)
|
|
|
|
If `test_undo_redo_lifecycle` is **also** a flake or a regression from the user's recent cleanup commit `873edf42`, the property fix may not be sufficient. In that case, the test will continue to fail and need its own investigation track.
|
|
|
|
## Risk Assessment
|
|
|
|
| Risk | Likelihood | Impact | Mitigation |
|
|
|---|---|---|---|
|
|
| Existing App code does `del app.ui_X` to reset state | Low | Low | property setter can be a no-op for `del` (raises AttributeError); review call sites |
|
|
| App class is 5532 lines — risk of regression | High | Medium | Per-field property addition; one regression test per field; ship in a single atomic commit |
|
|
| User's recent cleanup commit `873edf42` may have added or removed attribute references | Medium | Low | Run targeted regression test after each property addition |
|
|
| New properties shadow existing class attributes | Low | High | Use `dir(app)` to verify no shadow before commit |
|
|
|
|
## Out of Scope
|
|
|
|
- **prior_session test mock setup** — separate track (`prior_session_test_harden_20260605`).
|
|
- **wait-for-ready test pattern** — separate track (`wait_for_ready_test_pattern_20260605`).
|
|
- **Other App/Controller sync bugs not in the 8 listed** — audit will continue; if more found, queue as v3 sub-track.
|
|
- **Refactoring App and Controller into one class** — deferred; property approach is sufficient for now.
|