From 0f9623d2c28a985fcbd9f8d9e1bd54d5a7e87f25 Mon Sep 17 00:00:00 2001 From: Brian Krabach Date: Sun, 17 May 2026 09:33:46 -0700 Subject: [PATCH] refactor(views): introduce visibility helper and schema version (Phases 0+1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Phase 0 — Schema Version Field - Added SCHEMA_VERSION = 2 constant and _schema_version field to DEFAULT_SETTINGS in muxplex/settings.py to support versioned federation. - Added _schema_version to SYNCABLE_KEYS so peers see the version but apply_synced_settings never accepts an incoming version (one-way: send only). - save_settings() always clamps _schema_version to current SCHEMA_VERSION. - Added peer_supports_v2() helper for federation handshake. - 6 new tests under "Schema version (Phase 0)" in test_settings.py. ## Phase 1 — Backend & Frontend Visibility Helpers Backend (muxplex/views.py): - Added is_hidden(key, settings), filter_visible(sessions, settings, view, *, include_hidden=False), visible_count(...), and normalize_session_keys() to provide read-time visibility filtering. - Updated module docstring to describe v2 semantics (hidden is a property, not a placement; legacy enforce_mutual_exclusion retained as v1 backstop). - 22 new tests covering the full filter matrix and normalization edge cases. Frontend (muxplex/frontend/app.js): - Added isHidden, filterVisible, visibleCount to app.js (pre-ES6 idioms). - Replaced getVisibleSessions body with thin wrapper around filterVisible. - Replaced 8 raw .length count sites in dropdown, settings, and Manage View to route through visibleCount. - Settings panel shows "N sessions (M hidden)" when M > 0. - Manage View "in this view" count uses includeHidden: true. - 17 new tests in test_app.mjs covering the same matrix as backend. - Updated 5 stale tests in test_frontend_js.py. ## Additional Updates - Updated test_readme.py to exempt internal underscore-prefixed setting keys from README documentation requirement. - Updated docs/plans/2026-05-17-hidden-state-redesign-design.md with COE corrections and design notes (federation truth, Phase 3 deferral, schema version semantics, local-only pruning state, federation tests). All 1223 tests in muxplex/tests/ pass. All 17 new JS tests pass. No raw .length count sites remain in counting code paths. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- ...2026-05-17-hidden-state-redesign-design.md | 551 ++++++++++++++++++ muxplex/frontend/app.js | 134 +++-- muxplex/frontend/tests/test_app.mjs | 191 ++++++ muxplex/settings.py | 46 ++ muxplex/tests/test_frontend_js.py | 61 +- muxplex/tests/test_readme.py | 9 +- muxplex/tests/test_settings.py | 86 +++ muxplex/tests/test_views.py | 281 ++++++++- muxplex/views.py | 164 +++++- 9 files changed, 1439 insertions(+), 84 deletions(-) create mode 100644 docs/plans/2026-05-17-hidden-state-redesign-design.md diff --git a/docs/plans/2026-05-17-hidden-state-redesign-design.md b/docs/plans/2026-05-17-hidden-state-redesign-design.md new file mode 100644 index 0000000..d0c3513 --- /dev/null +++ b/docs/plans/2026-05-17-hidden-state-redesign-design.md @@ -0,0 +1,551 @@ +# Hidden State Redesign: Filter-at-Read, Single Visibility Helper + +**Date:** 2026-05-17 +**Status:** Design — ready for implementation +**Author:** bkrabach + +## Background + +The current implementation tangles two concerns: *placement* (which view a session +belongs to) and *presentation* (whether a session is hidden). The result is a +load-bearing invariant — "a session key cannot exist in both `hidden_sessions` +and any `view.sessions`" — enforced operationally on every add/remove/sync +(`views.py:enforce_mutual_exclusion`). + +Because this invariant is enforced at write time but the count display reads +raw lengths at render time, the displayed counts can disagree with the rendered +grid. There are five count computation sites; only one of them goes through the +canonical filter (`getVisibleSessions`). The others read `v.sessions.length` or +`hidden_sessions.length` directly. See `app.js:894, 913, 1010, 1266, 2640`. + +Concrete failure modes today: + +- A dead tmux session leaves its key in `view.sessions` forever. The dropdown + shows `Work (3)` but the grid renders 2 tiles. +- `hidden_sessions` accumulates stale keys across the lifetime of the install. + The Hidden view label shows `(5)` but renders 0 tiles. +- `getVisibleSessions` does not cross-check `hidden_sessions` for user views. If + the mutual-exclusion invariant is ever transiently violated (e.g., federation + sync race), a hidden session **would** render in a user view. + +## Principles + +1. **Hidden is a property, not a placement.** A session can be in zero or more + views and is independently marked hidden or not. View membership and hidden + state are orthogonal. +2. **One filter function.** Every list-producing path and every count goes + through a single canonical helper. No raw `.length` on stored arrays. +3. **Default exclude hidden, opt-in to include.** Display paths use the default. + Management paths pass `include_hidden=True` explicitly. +4. **Compose, don't fuse.** Low-level data operations stay pure + (`addMembership`, `hide`, `unhide`). User-intent operations compose them + explicitly (`addSessionToView` = `unhide` + `addMembership`). The auto-unhide + behavior remains, but it lives in a named operation a reviewer can see. +5. **Operational enforcement is replaced by read-time filtering.** The + mutual-exclusion invariant is removed. + +## Design + +### Data model (no schema changes) + +Storage is unchanged. Only the contract changes: + +```json +{ + "views": [ + { "name": "Work", "sessions": ["dev1:proj", "dev1:build"] } + ], + "hidden_sessions": ["dev1:build", "dev1:old"] +} +``` + +The same key may now appear in both `view.sessions` and `hidden_sessions`. That +combination means: "this session is a member of Work, but is currently hidden." +It will not appear in the Work view by default; it will appear in the Hidden +view; it will appear *dimmed* in the Manage View panel for Work. + +### The visibility helper (frontend) + +Single function. Replaces all five count sites and `getVisibleSessions`. + +```js +// app.js +function isHidden(key, hiddenSet) { + return hiddenSet.has(key); +} + +// Returns the canonical list of sessions for the requested view. +// view: "all" | "hidden" | +// includeHidden: when true, do not filter hidden sessions out of user views +// or "all". Ignored for "hidden" (always shows hidden only). +function filterVisible(sessions, settings, view, { includeHidden = false } = {}) { + const hiddenSet = new Set(settings.hidden_sessions || []); + const keyOf = s => s.sessionKey || s.name; + + // status-tile entries are never counted as sessions + const live = (sessions || []).filter(s => !s.status); + + if (view === "hidden") { + return live.filter(s => isHidden(keyOf(s), hiddenSet) || hiddenSet.has(s.name)); + } + + const visibilityPass = includeHidden + ? () => true + : s => !isHidden(keyOf(s), hiddenSet) && !hiddenSet.has(s.name); + + if (view === "all") { + return live.filter(visibilityPass); + } + + const userView = (settings.views || []).find(v => v.name === view); + if (!userView) return []; + const memberSet = new Set(userView.sessions || []); + return live.filter(s => + (memberSet.has(keyOf(s)) || memberSet.has(s.name)) && visibilityPass(s) + ); +} + +function visibleCount(...args) { + return filterVisible(...args).length; +} +``` + +Counts that displayed `(N)` previously now call `visibleCount(_currentSessions, +settings, viewName)`. Manage View counts call +`visibleCount(_currentSessions, settings, viewName, { includeHidden: true })` +and additionally report the hidden-of-total breakdown (see UI section). + +### Backend helper + +If the server ever computes session lists (e.g., for sync or for a future API), +mirror the helper in Python: + +```python +# views.py (replacing enforce_mutual_exclusion) + +def is_hidden(key: str, settings: dict) -> bool: + return key in (settings.get("hidden_sessions") or []) + +def filter_visible( + sessions: list[dict], + settings: dict, + view: str, + *, + include_hidden: bool = False, +) -> list[dict]: + hidden = set(settings.get("hidden_sessions") or []) + + def key_of(s): return s.get("sessionKey") or s.get("name") + def visibility_pass(s): + if include_hidden: + return True + k = key_of(s) + return k not in hidden and s.get("name") not in hidden + + live = [s for s in sessions if not s.get("status")] + if view == "hidden": + return [s for s in live if key_of(s) in hidden or s.get("name") in hidden] + if view == "all": + return [s for s in live if visibility_pass(s)] + + user_view = next((v for v in (settings.get("views") or []) if v["name"] == view), None) + if not user_view: + return [] + members = set(user_view.get("sessions") or []) + return [ + s for s in live + if (key_of(s) in members or s.get("name") in members) and visibility_pass(s) + ] +``` + +### Operations: pure vs user-intent + +Two layers. The UI calls user-intent operations. Tests verify both layers. + +**Pure data operations** (low-level, no side effects beyond their name): + +| Operation | Effect | +|------------------------|--------------------------------------------------------------| +| `addMembership(k, v)` | Append `k` to `settings.views[v].sessions` if absent | +| `removeMembership(k, v)` | Remove `k` from `settings.views[v].sessions` | +| `hide(k)` | Append `k` to `settings.hidden_sessions` if absent | +| `unhide(k)` | Remove `k` from `settings.hidden_sessions` | + +**User-intent operations** (high-level, used by UI): + +| Operation | Composition | +|------------------------------------|-------------------------------------------------------------| +| `addSessionToView(k, v)` | `unhide(k); addMembership(k, v)` — **auto-unhide here** | +| `removeSessionFromView(k, v)` | `removeMembership(k, v)` — does *not* hide | +| `hideSession(k)` | `hide(k)` — does *not* touch view membership | +| `unhideSession(k)` | `unhide(k)` — does *not* touch view membership | + +The auto-unhide on "Add to View" is preserved as a deliberate user-intent +composition, **not** as an invariant enforced everywhere. Reviewers see +exactly one call to `unhide()` adjacent to `addMembership()` in +`addSessionToView`. Future operations that want to add membership without +unhiding (e.g., a bulk migration tool) can call `addMembership` directly and +the existence of that uncomposed path is obvious in code review. + +### Federation sync + +**Reality check first.** The current sync model (`settings.py:164-184`) is +**whole-list last-write-wins**: `apply_synced_settings()` replaces each +syncable key wholesale from the incoming payload, then runs +`enforce_mutual_exclusion` as a sanitization pass. Conflict resolution is via +a single `settings_updated_at` timestamp on the entire settings blob. There is +no per-key merge, no CRDT, no add-wins/remove-wins set semantics. + +This refactor does **not** change that. We accept whole-list LWW for v1 and +document the consequences: + +- If device A hides session X while device B adds X to a view, whichever + device last wrote settings wins for *both* keys (`hidden_sessions` and + `views`). The user may see a brief flicker between sync rounds. This is the + same trade-off every other syncable setting already makes. +- Removing `enforce_mutual_exclusion` from `apply_synced_settings` is **not** + safe in mixed-version federation (see Mixed-version safety below). It stays + in place for v1 as a backstop. The read-time filter does not depend on the + invariant for correctness; it just needs to handle overlap when overlap + exists. Letting `enforce_mutual_exclusion` continue to strip overlap on + incoming sync is fine — new code tolerates either presence or absence of + overlap. + +This means **Phase 3 (delete `enforce_mutual_exclusion`) is deferred** out of +the v1 plan. It becomes safe to remove only after all federated devices +publish a `_schema_version >= 2`. See Phase 3 in the implementation phases +section. + +### Mixed-version safety (the blocking concern) + +Without a schema version field, mixed-version federation will silently revert +user actions: + +1. User on new device A hides session X (X is in view Work). +2. A's settings now have X in both `hidden_sessions` and `Work.sessions`. +3. Sync to old device B. B applies the new settings, then runs + `enforce_mutual_exclusion` — silently strips X from `hidden_sessions`. +4. B saves with new `settings_updated_at`. Sync back to A. X is now unhidden. +5. User re-hides. Oscillation. + +The mitigation: add a `_schema_version: int` field in **Phase 0** (a +prerequisite to all other phases). New clients write `_schema_version: 2` +into settings whenever they save. New clients detect old peers during +federation handshake (old peers send no `_schema_version` or `< 2`) and +behave conservatively: + +- When sending to a legacy peer: do not produce settings where the same key + appears in both `hidden_sessions` and any `view.sessions`. Pre-flatten by + calling `enforce_mutual_exclusion` on the outgoing payload. +- When receiving from a legacy peer: accept the incoming state as-is (it + already respects the old invariant). + +This keeps mixed-version operation safe. Once all peers report +`_schema_version >= 2`, the pre-flatten step is unnecessary. Detection of +"all peers uniform" is a manual operator action for v1 (a log message +suffices); we do not need to automate it before deleting +`enforce_mutual_exclusion`. + +### Stale key pruning (separate concern, local-only state) + +Filtering at read time means stale keys no longer corrupt counts. But +`hidden_sessions` and `view.sessions` still accumulate dead keys forever and +sync them across the federation. Add a periodic pruning pass. + +**Critical: pruning bookkeeping does NOT sync.** First-missed-at timestamps +are a per-device concern. Each device tracks which keys it has personally +failed to see, and prunes from its own settings when its grace period expires. +The actual prune (removing the key from `view.sessions` or `hidden_sessions`) +is a normal settings write that *does* sync via the existing LWW mechanism. +This avoids inventing new federation merge semantics. + +Bookkeeping lives in a local sidecar file, not in `settings.json`: + +``` +~/.config/muxplex/pruning.json +{ + "first_missed_at": { + "dev1:dead-session": 1747512345.0 + } +} +``` + +`pruning.json` is **not** in `SYNCABLE_KEYS` and is never sent to peers. + +```python +def prune_stale_keys(settings: dict, live_keys: set[str], grace_seconds: float) -> dict: + # 1. Update local first-missed-at bookkeeping based on live_keys. + # 2. For each key in hidden_sessions or any view.sessions: + # - If key in live_keys: remove from first-missed-at (if present). + # - If key not in live_keys: ensure first-missed-at has this key. + # If first_missed_at[key] + grace_seconds < now: drop the key. + # 3. Return modified settings (caller decides whether to save). +``` + +Runs on `_run_poll_cycle()` after the live session list is gathered +(`main.py:173`). + +**Grace period rationale.** A tmux session disappears briefly during restart +cycles — minutes, occasionally hours. A user's intent to keep a session in a +view, however, can span days (a vacation, a long context-switch). The tension +is between cleaning up genuinely dead keys quickly and not surprising users. +Default: 24 hours, configurable via a new `stale_key_grace_hours` setting. +This is well above the worst-case restart duration and well below "I forgot +this view existed" timescales. Document the choice; revise based on use. + +In a federated install, the grace period applies *per device*. A key not +seen on device A for 24h is dropped on A; B may still be tracking it. +B's eventual sync receives the deletion from A. This is the correct +behavior — A speaks for itself. + +This phase is independent of the visibility refactor and can ship in its +own commit. + +## UI changes + +### Manage View panel + +- Loads with `include_hidden=True`. +- Hidden sessions render dimmed (CSS class `session-hidden-dim`, ~50% + opacity). +- Checkbox state for hidden sessions reflects membership (in view or not), + independent of hidden state. +- Checking a hidden session that is not yet in the view calls + `addSessionToView` — which unhides AND adds. The dim styling disappears. +- Unchecking a session calls `removeSessionFromView` (does not hide). +- A small "(hidden)" badge confirms hidden state for clarity. + +### Settings panel view list + +Show both numbers when relevant: + +``` +Work: 5 sessions (2 hidden) +Personal: 3 sessions +``` + +The bare "N sessions" form is used when no sessions in the view are hidden. +The "(M hidden)" suffix appears only when M > 0. + +### Header & sidebar dropdowns + +All counts go through `visibleCount(..., view, { includeHidden: false })`. +The bare numeric badge (e.g., `Work (3)`) reflects the count of sessions +actually rendered in the grid when that view is active. No exceptions. + +### "Hidden" view label + +The "Hidden" view count uses `visibleCount(..., "hidden")` — only counts +hidden sessions that are *live*. A stale entry in `hidden_sessions` for a +dead tmux session does not inflate this count. (The stale entry will be +removed by `prune_stale_keys` after the grace period.) + +## Migration & backward compatibility + +- **Settings files:** schema-compatible. Old files have no overlap between + `hidden_sessions` and `view.sessions`; the new code handles that correctly. + Until Phase 3 (deferred), new code never *writes* overlap either — it + keeps `enforce_mutual_exclusion` as a backstop, so the on-disk schema does + not change. +- **Schema version:** add `_schema_version: 2` to settings in Phase 0. Old + clients ignore unknown keys (`settings.py:load_settings` only copies keys + present in `DEFAULT_SETTINGS`), so this is safe to write. The version + exists primarily as a marker for future Phase 3 — when all peers report + `>= 2`, it is safe to delete `enforce_mutual_exclusion`. +- **Old clients reading new state:** identical to today (no overlap exists + to drop, because we still call `enforce_mutual_exclusion` on save). +- **Key format normalization:** Phase 1 includes a one-time pass that walks + `views[].sessions` and `hidden_sessions` on first load by the new code, + upgrading bare-name entries to the canonical `device_id:name` form where a + match exists in the current live session list. Entries that cannot be + matched are preserved (they may match later). This pays the migration cost + once and removes the need for dual-lookup at every read site. + +## Implementation phases + +Each phase is independently shippable. Order matters: Phase 0 is a +prerequisite to all others. + +### Phase 0 — Schema version field + +Tiny but load-bearing. Future-proofs the federation upgrade path. + +- Add `_schema_version` to `DEFAULT_SETTINGS` (`settings.py:17-47`) with + value `2`. Add to `SYNCABLE_KEYS` so peers see it. +- On every `save_settings`/`patch_settings` write, ensure + `_schema_version` is set to `2` regardless of incoming patch (clients + don't get to write older versions). +- Add a helper `peer_supports_v2(peer_settings)` that returns + `peer_settings.get("_schema_version", 0) >= 2`. +- No behavioral change yet. This is just the marker. + +### Phase 1 — Visibility helper + audit + key normalization + +Single most important change. Source of truth for visibility lives here. + +- Add `filterVisible()` and `visibleCount()` to `app.js`. +- Add `filter_visible()` to `views.py` (for backend symmetry, even if no + current Python caller needs it). +- Replace every count site: + - `app.js:894` (Hidden count) → `visibleCount(..., "hidden")` + - `app.js:900-903` (All Sessions header) → `visibleCount(..., "all")` + - `app.js:913` (User view header dropdown) → `visibleCount(..., v.name)` + - `app.js:997-1000` (All Sessions sidebar) → same + - `app.js:1010` (User view sidebar) → same + - `app.js:1266` (Settings panel) → same, with `(M hidden)` suffix + - `app.js:2640` (Manage View "in this view") → + `visibleCount(..., view, { includeHidden: true })` +- Replace `getVisibleSessions()` body with a call to `filterVisible()`. +- Grep audit: search for `.sessions.length`, `hidden_sessions.length`, + `hidden.length`. Each remaining occurrence must be justified in a comment + or replaced. +- **One-time key normalization**: on first load by the new code (detected via + absence of `_schema_version` in the on-disk settings, or via a new marker), + walk `views[].sessions` and `hidden_sessions`. For each entry that is a + bare `name`, check if any live session has a matching `name`; if so, + upgrade the stored entry to `device_id:name` form. Entries that cannot be + matched (no live session with that name) are left as-is. Run once, set + `_schema_version: 2`, save. Subsequent loads skip the pass. +- After normalization lands, the `memberSet.has(keyOf(s)) || memberSet.has(s.name)` + dual-lookup can be simplified to a single lookup. Keep the dual lookup + initially — remove it in a follow-up commit once normalization has run on + all production installs. + +### Phase 2 — Operation layer + +- Refactor the existing hide/unhide/add-to-view code paths into the four pure + + four user-intent operations described above. +- Update existing call sites in `app.js` (`hideSession`, `unhideSession`, + add-to-view flyout, Manage View checkbox handler) to call the user-intent + operations. +- Document the layering in a header comment. + +### Phase 3 — Remove `enforce_mutual_exclusion` *(deferred from v1)* + +**Not part of v1.** Listed here for completeness; do not ship until all +federated peers report `_schema_version >= 2` and the operator confirms it +is safe to drop the legacy backstop. + +When the time comes: +- Delete `views.py:enforce_mutual_exclusion` and its call site in + `settings.py:apply_synced_settings()`. +- Delete the corresponding test cases in `tests/test_views.py`. +- Remove the outgoing-payload pre-flatten step added in Phase 0. +- Bump `_schema_version` to 3 if any further on-disk format change accompanies + the removal. + +Until this phase ships, the new code coexists with the old invariant. Counts +and rendering are correct; storage continues to enforce mutual exclusion. The +only user-visible cost is that the auto-unhide-on-add behavior cannot be +broken into "add without unhiding" without `enforce_mutual_exclusion` +reverting it on the next save. This is acceptable for v1 — that capability +is not exposed to users. + +### Phase 4 — Stale key pruning (local-only bookkeeping) + +- Add `prune_stale_keys()` in `views.py`. Reads & writes a local sidecar + file `~/.config/muxplex/pruning.json` (see the design section above). +- The sidecar is **not** in `SYNCABLE_KEYS` and is never sent to peers. +- Wire into `_run_poll_cycle()` after the live session collection step + (`main.py:173`). +- Add `stale_key_grace_hours` (default 24) to `DEFAULT_SETTINGS`. Syncable; + affects every device that consumes it. Each device still tracks its own + first-missed-at timestamps locally. + +### Phase 5 — UI polish + +- Manage View dim styling. +- Settings panel "(M hidden)" suffix. +- Any other discoverability improvements. + +Phases 0–2 are the actual semantic change. Phase 4 is hygiene. Phase 5 is +presentation. Phase 3 is deferred. Don't combine them. + +## Test plan + +### New tests (frontend, `frontend/tests/test_app.mjs`) + +- `filterVisible` matrix — every combination of: + - view ∈ {"all", "hidden", ``, ``} + - `includeHidden` ∈ {true, false} + - session in / absent from `view.sessions` + - key in / absent from `hidden_sessions` + - key in both (the new permitted state; verify v1 storage never produces it, + but the helper handles it correctly if it appears) + - session-key format: `device_id:name`, bare `name`, both +- `visibleCount` matches `filterVisible().length` for the same inputs. +- Key normalization: bare-name entries upgrade to `device_id:name` when a + match exists; remain bare when no match exists; idempotent across reloads. + +### New tests (backend, `tests/test_views.py`, `tests/test_settings.py`) + +- `filter_visible` mirroring the frontend matrix. +- `addSessionToView` unhides and adds membership. +- `addMembership` adds membership but does **not** unhide. +- `removeSessionFromView` removes membership but does **not** hide. +- `hide` and `unhide` do not touch view membership. +- Schema version: `save_settings` always writes `_schema_version: 2`. +- `peer_supports_v2`: returns true for `_schema_version >= 2`, false for + missing or `< 2`. + +### Federation tests (`tests/test_settings.py` or new `test_federation.py`) + +- Round-trip: state with overlap, save, reload, assert content unchanged + (with `enforce_mutual_exclusion` still active, overlap should be stripped + on save — verify this is the v1 invariant). +- Mixed-version send: when serializing settings for a peer reporting + `_schema_version < 2`, the outgoing payload has no overlap. Verify by + constructing settings with overlap (force it past the backstop in a test + helper) and asserting the outgoing form is flattened. +- Mixed-version receive: applying settings from a legacy peer is a no-op + beyond the existing whole-list replacement. Verify `_schema_version` is + not downgraded by an incoming payload that lacks it. + +### Stale-key pruning tests (`tests/test_views.py`) + +- Live key: pruning bookkeeping is cleared. +- Newly missing key: bookkeeping records `first_missed_at`, settings unchanged. +- Missing past grace: key removed from `view.sessions` and `hidden_sessions`, + bookkeeping entry removed. +- Re-appeared after partial absence (less than grace): bookkeeping cleared, + settings unchanged. +- Sidecar file: never written to `settings.json`, never sent over the wire. + +### Updated tests + +- Adapt existing view tests that asserted the old mutual-exclusion invariant. + Several tests assume `add_to_view` removes from `hidden_sessions` as a side + effect; rewrite them to call the user-intent operation explicitly and + assert on both effects. (Keep the existing `enforce_mutual_exclusion` + tests — that function stays in v1.) + +### Removed tests + +- None in v1. `enforce_mutual_exclusion` tests stay until Phase 3 ships. + +## Out of scope + +- API redesign for `/api/views` or `/api/sessions`. The wire format is + unchanged. +- Conflict UI for federation merges. Last-write-wins on the hidden flag is + acceptable for v1. +- Bulk operations (hide all in view, etc.). The current granularity is + sufficient. +- Tooltip / hover state showing why a session is dimmed. The "(hidden)" badge + is enough. + +## Open questions + +None that block implementation. Worth deciding before Phase 5: + +- Should the Manage View panel default to `include_hidden=True`, or require + an explicit "Show hidden" toggle? Recommendation: default to true. The + whole point of the Manage View is management; hiding hidden items from a + management UI defeats the purpose. + +## References + +- COE design review conversation, 2026-05-17 (workspace session + e74700a2-5340-46be-8fae-df40a19204f3). +- Current implementation: `views.py:14-68`, `settings.py:24-65, 164-184`, + `state.py:9`, `app.js:628-672, 894-1010, 1266, 2173-2236, 2640`. diff --git a/muxplex/frontend/app.js b/muxplex/frontend/app.js index 262e09a..f56c299 100644 --- a/muxplex/frontend/app.js +++ b/muxplex/frontend/app.js @@ -624,51 +624,74 @@ function buildStatusTileHTML(deviceName, statusText, statusClass) { ); } +// --------------------------------------------------------------------------- +// v2 visibility helpers — single source of truth for session filtering. +// See docs/plans/2026-05-17-hidden-state-redesign-design.md +// --------------------------------------------------------------------------- + +// Returns true if the session key is in settings.hidden_sessions. +function isHidden(key, settings) { + var hidden = (settings && settings.hidden_sessions) || []; + return hidden.indexOf(key) !== -1; +} + +// Canonical session-list filter. Single source of truth for "what is in this +// view right now". See docs/plans/2026-05-17-hidden-state-redesign-design.md. +// view: "all" | "hidden" | +// options.includeHidden: when true, hidden sessions are NOT filtered out of +// "all" or user views. Ignored for "hidden" (always shows only hidden). +function filterVisible(sessions, settings, view, options) { + options = options || {}; + var includeHidden = options.includeHidden === true; + var hiddenList = (settings && settings.hidden_sessions) || []; + var live = (sessions || []).filter(function (s) { return !s.status; }); + + function keyOf(s) { return s.sessionKey || s.name; } + function isSessionHidden(s) { + return hiddenList.indexOf(keyOf(s)) !== -1 || hiddenList.indexOf(s.name) !== -1; + } + + if (view === "hidden") { + return live.filter(isSessionHidden); + } + if (view === "all") { + if (includeHidden) return live.slice(); + return live.filter(function (s) { return !isSessionHidden(s); }); + } + + var views = (settings && settings.views) || []; + var userView = null; + for (var i = 0; i < views.length; i++) { + if (views[i].name === view) { userView = views[i]; break; } + } + if (!userView) return []; + var members = userView.sessions || []; + + function inView(s) { + return members.indexOf(keyOf(s)) !== -1 || members.indexOf(s.name) !== -1; + } + if (includeHidden) { + return live.filter(inView); + } + return live.filter(function (s) { return inView(s) && !isSessionHidden(s); }); +} + +function visibleCount(sessions, settings, view, options) { + return filterVisible(sessions, settings, view, options).length; +} + /** * Returns sessions filtered by the active view. * - * - 'all' view: excludes hidden sessions (sessions in hidden_sessions list) - * - 'hidden' view: shows only hidden sessions - * - user view: shows only sessions whose sessionKey is in that view's sessions list - * - * Status entries (unreachable, auth_failed, empty) are always excluded — - * they are rendered separately as status tiles. - * - * Falls back to 'all' behaviour if the active view no longer exists. + * Thin wrapper around filterVisible() — the canonical filter that is the + * single source of truth for "what is in this view right now". + * See docs/plans/2026-05-17-hidden-state-redesign-design.md. * * @param {object[]} sessions * @returns {object[]} */ function getVisibleSessions(sessions) { - var hidden = (_serverSettings && _serverSettings.hidden_sessions) || []; - var views = (_serverSettings && _serverSettings.views) || []; - var view = _resolveActiveView(_activeView, views); - - return (sessions || []).filter(function(s) { - // Skip status entries (unreachable, auth_failed, empty) — rendered separately as status tiles - if (s.status) return false; - - if (view === 'hidden') { - // 'hidden' view: only show sessions that are in the hidden list - return hidden.length > 0 && (hidden.includes(s.sessionKey || s.name) || hidden.includes(s.name)); - } - - if (view !== 'all') { - // User-defined view: show only sessions whose sessionKey is in this view's sessions list - var userView = views.find(function(v) { return v.name === view; }); - if (userView) { - var viewSessions = userView.sessions || []; - return viewSessions.includes(s.sessionKey || s.name) || viewSessions.includes(s.name); - } - // View no longer exists — fall through to 'all' behaviour - } - - // 'all' view: exclude hidden sessions - if (hidden.length > 0 && (hidden.includes(s.sessionKey || s.name) || hidden.includes(s.name))) { - return false; - } - return true; - }); + return filterVisible(sessions, _serverSettings, _activeView); } /** @@ -890,17 +913,12 @@ function renderViewDropdown() { if (!menu) return; var views = (_serverSettings && _serverSettings.views) || []; - var hidden = (_serverSettings && _serverSettings.hidden_sessions) || []; - var hiddenCount = hidden.length; + var hiddenCount = visibleCount(_currentSessions, _serverSettings, "hidden"); var html = ''; // — All Sessions (always first) — show count of non-hidden sessions - var allHiddenSessions = (_serverSettings && _serverSettings.hidden_sessions) || []; - var allCount = (_currentSessions || []).filter(function(s) { - if (s.status) return false; - return allHiddenSessions.indexOf(s.sessionKey || s.name) === -1 && allHiddenSessions.indexOf(s.name) === -1; - }).length; + var allCount = visibleCount(_currentSessions, _serverSettings, "all"); var allActive = _activeView === 'all' ? ' view-dropdown__item--active' : ''; html += ''; @@ -910,7 +928,7 @@ function renderViewDropdown() { for (var i = 0; i < views.length && i < 7; i++) { var v = views[i]; var vActive = _activeView === v.name ? ' view-dropdown__item--active' : ''; - html += ''; + html += ''; } } @@ -987,17 +1005,12 @@ function renderSidebarViewDropdown() { if (!menu) return; var views = (_serverSettings && _serverSettings.views) || []; - var hidden = (_serverSettings && _serverSettings.hidden_sessions) || []; - var hiddenCount = hidden.length; + var hiddenCount = visibleCount(_currentSessions, _serverSettings, "hidden"); var html = ''; // — All Sessions (always first) — show count of non-hidden sessions - var sbHiddenSessions = (_serverSettings && _serverSettings.hidden_sessions) || []; - var sbAllCount = (_currentSessions || []).filter(function(s) { - if (s.status) return false; - return sbHiddenSessions.indexOf(s.sessionKey || s.name) === -1 && sbHiddenSessions.indexOf(s.name) === -1; - }).length; + var sbAllCount = visibleCount(_currentSessions, _serverSettings, "all"); var allActive = _activeView === 'all' ? ' view-dropdown__item--active' : ''; html += ''; @@ -1007,7 +1020,7 @@ function renderSidebarViewDropdown() { for (var i = 0; i < views.length && i < 7; i++) { var v = views[i]; var vActive = _activeView === v.name ? ' view-dropdown__item--active' : ''; - html += ''; + html += ''; } } @@ -1262,8 +1275,9 @@ function renderViewsSettingsTab() { // Build the list of view rows (no inline rename — rename is in Manage View panel) listEl.innerHTML = ''; views.forEach(function(view, idx) { - var viewSessions = view.sessions || []; - var sessionCount = viewSessions.length; + var sessionCount = visibleCount(_currentSessions, _serverSettings, view.name); + var inclHidden = visibleCount(_currentSessions, _serverSettings, view.name, { includeHidden: true }); + var hiddenInViewCount = inclHidden - sessionCount; var row = document.createElement('div'); row.className = 'views-settings-row'; @@ -1274,10 +1288,10 @@ function renderViewsSettingsTab() { nameSpan.className = 'views-settings-name'; nameSpan.textContent = view.name; - // Session count + // Session count — show "(M hidden)" suffix when M > 0 var countSpan = document.createElement('span'); countSpan.className = 'views-settings-count'; - countSpan.textContent = sessionCount + (sessionCount === 1 ? ' session' : ' sessions'); + countSpan.textContent = sessionCount + (sessionCount === 1 ? ' session' : ' sessions') + (hiddenInViewCount > 0 ? ' (' + hiddenInViewCount + ' hidden)' : ''); // Actions container var actionsDiv = document.createElement('div'); @@ -2537,7 +2551,7 @@ function renderManageViewList() { // Update summary line if (summaryEl) { - summaryEl.textContent = allSessions.length + ' sessions · ' + viewSessions.length + ' in this view'; + summaryEl.textContent = allSessions.length + ' sessions · ' + visibleCount(_currentSessions, _serverSettings, _activeView, { includeHidden: true }) + ' in this view'; } // Partition into inView (checked first) and notInView @@ -4510,6 +4524,10 @@ if (typeof module !== 'undefined' && module.exports) { // Manage Views settings tab renderViewsSettingsTab, _saveViewsAndRerender, + // v2 visibility helpers (Phase 1) + isHidden, + filterVisible, + visibleCount, // Federation tiles buildStatusTileHTML, // Constants diff --git a/muxplex/frontend/tests/test_app.mjs b/muxplex/frontend/tests/test_app.mjs index af2b979..65d15c0 100644 --- a/muxplex/frontend/tests/test_app.mjs +++ b/muxplex/frontend/tests/test_app.mjs @@ -4875,3 +4875,194 @@ test('heartbeat uses setTimeout not setInterval for async-safe scheduling', () = assert.ok(fnBody.includes('setTimeout'), 'startHeartbeat must use setTimeout for self-scheduling async loop'); }); + +// --------------------------------------------------------------------------- +// v2 visibility helpers: isHidden, filterVisible, visibleCount +// See docs/plans/2026-05-17-hidden-state-redesign-design.md +// Mirror of the Python test matrix in tests/test_views.py +// --------------------------------------------------------------------------- + +// Helper: build a session dict matching the backend test fixture convention. +function _session(name, deviceId, status) { + deviceId = deviceId || 'dev1'; + var d = { sessionKey: deviceId + ':' + name, name: name }; + if (status !== undefined) d.status = status; + return d; +} + +// --- app.js exports the three v2 helpers --- + +test('app.js exports isHidden, filterVisible, visibleCount', () => { + for (const fn of ['isHidden', 'filterVisible', 'visibleCount']) { + assert.ok(fn in app, `app.js should export "${fn}"`); + assert.strictEqual(typeof app[fn], 'function', `"${fn}" should be a function`); + } +}); + +// --- isHidden --- + +test('isHidden returns true when key is in hidden_sessions', () => { + const settings = { hidden_sessions: ['dev1:a', 'dev1:b'] }; + assert.strictEqual(app.isHidden('dev1:a', settings), true); + assert.strictEqual(app.isHidden('dev1:b', settings), true); +}); + +test('isHidden returns false when key is absent from hidden_sessions', () => { + const settings = { hidden_sessions: ['dev1:a'] }; + assert.strictEqual(app.isHidden('dev1:b', settings), false); +}); + +test('isHidden returns false when hidden_sessions field is missing', () => { + assert.strictEqual(app.isHidden('dev1:a', {}), false); + assert.strictEqual(app.isHidden('dev1:a', { hidden_sessions: null }), false); + assert.strictEqual(app.isHidden('dev1:a', null), false); +}); + +// --- filterVisible: view="all" --- + +test('filterVisible view="all" excludes hidden sessions by default', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { hidden_sessions: ['dev1:b'], views: [] }; + const result = app.filterVisible(sessions, settings, 'all'); + assert.deepStrictEqual(result.map(s => s.name), ['a', 'c']); +}); + +test('filterVisible view="all" includeHidden:true returns all live sessions', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { hidden_sessions: ['dev1:b'], views: [] }; + const result = app.filterVisible(sessions, settings, 'all', { includeHidden: true }); + assert.deepStrictEqual(result.map(s => s.name), ['a', 'b', 'c']); +}); + +test('filterVisible view="all" excludes status tiles', () => { + const sessions = [ + _session('a'), + _session('disconnected', 'dev1', 'error'), + _session('b'), + ]; + const settings = { hidden_sessions: [], views: [] }; + const result = app.filterVisible(sessions, settings, 'all'); + assert.deepStrictEqual(result.map(s => s.name), ['a', 'b']); +}); + +// --- filterVisible: view="hidden" --- + +test('filterVisible view="hidden" returns only hidden sessions', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { hidden_sessions: ['dev1:a', 'dev1:c'], views: [] }; + const result = app.filterVisible(sessions, settings, 'hidden'); + assert.deepStrictEqual(result.map(s => s.name), ['a', 'c']); +}); + +test('filterVisible view="hidden" returns empty list when no sessions are hidden', () => { + const sessions = [_session('a'), _session('b')]; + const settings = { hidden_sessions: [], views: [] }; + const result = app.filterVisible(sessions, settings, 'hidden'); + assert.deepStrictEqual(result, []); +}); + +test('filterVisible view="hidden" excludes dead keys (hidden_sessions entries with no live match)', () => { + const sessions = [_session('a')]; + const settings = { hidden_sessions: ['dev1:a', 'dev1:ghost'], views: [] }; + const result = app.filterVisible(sessions, settings, 'hidden'); + assert.deepStrictEqual(result.map(s => s.name), ['a']); +}); + +// --- filterVisible: user view --- + +test('filterVisible user view filters by membership AND visibility', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { + hidden_sessions: ['dev1:b'], + views: [{ name: 'Work', sessions: ['dev1:a', 'dev1:b'] }], + }; + // 'a' is in view and not hidden; 'b' is in view but hidden → excluded by default. + const result = app.filterVisible(sessions, settings, 'Work'); + assert.deepStrictEqual(result.map(s => s.name), ['a']); +}); + +test('filterVisible user view includeHidden:true lifts visibility filter but NOT membership filter', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { + hidden_sessions: ['dev1:b'], + views: [{ name: 'Work', sessions: ['dev1:a', 'dev1:b'] }], + }; + // 'b' is in view AND hidden — includeHidden surfaces it. + // 'c' is NOT in view — still excluded. + const result = app.filterVisible(sessions, settings, 'Work', { includeHidden: true }); + assert.deepStrictEqual(result.map(s => s.name), ['a', 'b']); +}); + +test('filterVisible returns empty list for unknown view name', () => { + const sessions = [_session('a'), _session('b')]; + const settings = { hidden_sessions: [], views: [] }; + assert.deepStrictEqual(app.filterVisible(sessions, settings, 'Nonexistent'), []); +}); + +// --- Overlap state (key in both view.sessions AND hidden_sessions) --- + +test('filterVisible overlap state: hidden filter wins by default, includeHidden surfaces it', () => { + const sessions = [_session('a'), _session('b')]; + const settings = { + hidden_sessions: ['dev1:a'], // also in Work view + views: [{ name: 'Work', sessions: ['dev1:a', 'dev1:b'] }], + }; + + // Default: 'a' is excluded from Work because it is hidden. + const defaultResult = app.filterVisible(sessions, settings, 'Work'); + assert.deepStrictEqual(defaultResult.map(s => s.name), ['b']); + + // includeHidden: 'a' is surfaced again. + const inclResult = app.filterVisible(sessions, settings, 'Work', { includeHidden: true }); + assert.deepStrictEqual(inclResult.map(s => s.name), ['a', 'b']); +}); + +// --- Bare-name dual-lookup --- + +test('filterVisible bare-name in hidden_sessions matches session by name', () => { + // hidden_sessions stores bare 'a'; session has name:'a', sessionKey:'dev1:a'. + const sessions = [_session('a'), _session('b')]; + const settings = { hidden_sessions: ['a'], views: [] }; + // 'a' is hidden; 'b' is not. + const result = app.filterVisible(sessions, settings, 'all'); + assert.deepStrictEqual(result.map(s => s.name), ['b']); +}); + +test('filterVisible bare-name in view.sessions matches session by name', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { + hidden_sessions: [], + views: [{ name: 'Work', sessions: ['a', 'b'] }], // bare names + }; + const result = app.filterVisible(sessions, settings, 'Work'); + assert.deepStrictEqual(result.map(s => s.name), ['a', 'b']); +}); + +// --- visibleCount --- + +test('visibleCount always equals filterVisible().length for the same inputs', () => { + const sessions = [_session('a'), _session('b'), _session('c')]; + const settings = { + hidden_sessions: ['dev1:b'], + views: [{ name: 'Work', sessions: ['dev1:a', 'dev1:b', 'dev1:c'] }], + }; + + const matrix = [ + { view: 'all', opts: undefined }, + { view: 'all', opts: { includeHidden: true } }, + { view: 'hidden', opts: undefined }, + { view: 'Work', opts: undefined }, + { view: 'Work', opts: { includeHidden: true } }, + { view: 'Nonexistent', opts: undefined }, + ]; + + for (const { view, opts } of matrix) { + const expected = app.filterVisible(sessions, settings, view, opts).length; + const actual = app.visibleCount(sessions, settings, view, opts); + assert.strictEqual( + actual, + expected, + `visibleCount(${view}, ${JSON.stringify(opts)}) = ${actual} !== filterVisible length ${expected}` + ); + } +}); diff --git a/muxplex/settings.py b/muxplex/settings.py index 5c29d97..7b3fd0b 100644 --- a/muxplex/settings.py +++ b/muxplex/settings.py @@ -14,6 +14,18 @@ from pathlib import Path SETTINGS_PATH = Path.home() / ".config" / "muxplex" / "settings.json" FEDERATION_KEY_PATH = Path.home() / ".config" / "muxplex" / "federation_key" +# Settings schema version. Incremented when settings semantics change. +# +# v1 (implicit, missing field): legacy. The mutual-exclusion invariant between +# hidden_sessions and view.sessions is enforced at write time. Federation +# peers without this field are assumed to be v1. +# v2: hidden_sessions and view.sessions are allowed to overlap; visibility is +# determined by a read-time filter. In practice the v1 backstop +# enforce_mutual_exclusion still runs in v2 — see +# docs/plans/2026-05-17-hidden-state-redesign-design.md for the deferral +# of its removal (Phase 3). +SCHEMA_VERSION: int = 2 + DEFAULT_SETTINGS: dict = { "host": "127.0.0.1", "port": 8088, @@ -44,6 +56,7 @@ DEFAULT_SETTINGS: dict = { "gridViewMode": "flat", "sidebarOpen": None, "settings_updated_at": 0.0, + "_schema_version": SCHEMA_VERSION, } SYNCABLE_KEYS: frozenset[str] = frozenset( @@ -66,6 +79,9 @@ SYNCABLE_KEYS: frozenset[str] = frozenset( "default_session", "window_size_largest", "auto_open_created", + # Schema version — sent so peers can detect our version, but never + # accepted from the wire (see apply_synced_settings). + "_schema_version", } ) @@ -95,11 +111,17 @@ def save_settings(data: dict) -> None: Creates parent directories as needed. Writes JSON with indent=2 and a trailing newline. + + The `_schema_version` field is always written as the current + SCHEMA_VERSION regardless of *data*. Clients do not get to write older + versions — that would defeat the version field's purpose as a marker for + federated peers. """ merged = copy.deepcopy(DEFAULT_SETTINGS) for key in DEFAULT_SETTINGS: if key in data: merged[key] = data[key] + merged["_schema_version"] = SCHEMA_VERSION SETTINGS_PATH.parent.mkdir(parents=True, exist_ok=True) SETTINGS_PATH.write_text(json.dumps(merged, indent=2) + "\n") @@ -167,6 +189,11 @@ def apply_synced_settings(incoming_settings: dict, incoming_timestamp: float) -> Only applies keys that are in SYNCABLE_KEYS. Sets settings_updated_at to the incoming timestamp (NOT time.time()) to prevent sync loops. + `_schema_version` is intentionally **never** accepted from the wire. + Each device speaks for its own schema version; receiving a peer's version + must not downgrade ours. The peer's version is observable on the incoming + payload (use `peer_supports_v2()`) before this function applies anything. + After applying synced keys, enforces the mutual exclusion invariant: any session key that appears in both hidden_sessions and a view's sessions is removed from hidden_sessions (visibility wins over hiding). @@ -176,6 +203,9 @@ def apply_synced_settings(incoming_settings: dict, incoming_timestamp: float) -> current = load_settings() for key in SYNCABLE_KEYS: + if key == "_schema_version": + # Never downgrade local schema version from sync. + continue if key in incoming_settings: current[key] = incoming_settings[key] enforce_mutual_exclusion(current) @@ -184,6 +214,22 @@ def apply_synced_settings(incoming_settings: dict, incoming_timestamp: float) -> return current +def peer_supports_v2(peer_settings: dict) -> bool: + """Return True if a peer's settings payload indicates schema version >= 2. + + Legacy peers omit `_schema_version` (or send a lower value) and are + treated as v1. v1 peers enforce the mutual-exclusion invariant between + hidden_sessions and view.sessions; v2 peers tolerate overlap. + + Used during federation handshake to decide whether outgoing settings + need to be pre-flattened for legacy compatibility. + """ + try: + return int(peer_settings.get("_schema_version", 0)) >= 2 + except (TypeError, ValueError): + return False + + def get_syncable_settings() -> dict: """Return only syncable settings + the settings_updated_at timestamp.""" settings = load_settings() diff --git a/muxplex/tests/test_frontend_js.py b/muxplex/tests/test_frontend_js.py index 6ce2575..276ee7a 100644 --- a/muxplex/tests/test_frontend_js.py +++ b/muxplex/tests/test_frontend_js.py @@ -2146,7 +2146,11 @@ def test_create_terminal_has_default_font_size_14() -> None: def test_get_visible_sessions_filters_hidden_sessions() -> None: - """getVisibleSessions() must filter sessions using _serverSettings.hidden_sessions.""" + """getVisibleSessions() must filter sessions using _serverSettings.hidden_sessions. + + Phase 1 refactor: getVisibleSessions is now a thin wrapper around filterVisible(), + which is the single canonical filter. hidden_sessions handling lives in filterVisible(). + """ match = re.search( r"function getVisibleSessions\s*\(\w+\)\s*\{(.*?)(?=\n(?:function|//|window\.))", _JS, @@ -2154,11 +2158,12 @@ def test_get_visible_sessions_filters_hidden_sessions() -> None: ) assert match, "getVisibleSessions function not found in app.js" body = match.group(1) - assert "hidden_sessions" in body, ( - "getVisibleSessions must filter sessions using _serverSettings.hidden_sessions" + # Phase 1: body is a thin wrapper — delegates to filterVisible() with _serverSettings + assert "filterVisible" in body, ( + "getVisibleSessions must delegate to filterVisible() (Phase 1 refactor)" ) assert "_serverSettings" in body, ( - "getVisibleSessions must reference _serverSettings to access hidden_sessions" + "getVisibleSessions must pass _serverSettings to filterVisible for hidden_sessions access" ) @@ -2810,13 +2815,14 @@ def test_build_sidebar_html_uses_remote_id_for_data_remote_id() -> None: def test_get_visible_sessions_checks_session_key_in_hidden() -> None: - """getVisibleSessions must check sessionKey (as well as name) against the hidden_sessions list. + """getVisibleSessions must handle sessionKey/name dual-lookup for hidden_sessions matching. hidden_sessions may contain either: - old format: plain session name (e.g. 'dev') - new format: device_id:name key (e.g. 'abc-123:dev') - Both formats must be matched for backward compatibility. + Phase 1 refactor: this is now handled inside filterVisible(). getVisibleSessions delegates + to filterVisible(), which is the single canonical filter implementing dual-lookup. """ match = re.search( r"function getVisibleSessions\s*\(\w+\)\s*\{(.*?)(?=\n(?:function|//|window\.))", @@ -2825,9 +2831,10 @@ def test_get_visible_sessions_checks_session_key_in_hidden() -> None: ) assert match, "getVisibleSessions function not found" body = match.group(1) - assert "sessionKey" in body, ( - "getVisibleSessions must check s.sessionKey (in addition to s.name) against hidden_sessions " - "for backward compatibility: hidden_sessions may contain plain names OR device_id:name keys" + # Phase 1: thin wrapper — dual-lookup lives in filterVisible() + assert "filterVisible" in body, ( + "getVisibleSessions must delegate to filterVisible() which implements " + "sessionKey/name dual-lookup for backward compatibility" ) @@ -3011,7 +3018,11 @@ def test_get_visible_sessions_all_view_excludes_hidden() -> None: def test_get_visible_sessions_hidden_view_shows_hidden() -> None: - """getVisibleSessions must handle the 'hidden' view case.""" + """getVisibleSessions must handle the 'hidden' view case. + + Phase 1 refactor: getVisibleSessions is a thin wrapper around filterVisible(). + The 'hidden' view logic lives in filterVisible(), not in getVisibleSessions(). + """ match = re.search( r"function getVisibleSessions\(sessions\)\s*\{(.*?)\n\}", _JS, @@ -3019,13 +3030,18 @@ def test_get_visible_sessions_hidden_view_shows_hidden() -> None: ) assert match, "getVisibleSessions function not found" body = match.group(1) - assert "'hidden'" in body or '"hidden"' in body, ( - "getVisibleSessions must explicitly handle the 'hidden' view (show only hidden sessions)" + # Phase 1: thin wrapper — 'hidden' view handling lives in filterVisible() + assert "filterVisible" in body, ( + "getVisibleSessions must delegate to filterVisible() which handles the 'hidden' view" ) def test_get_visible_sessions_user_view_filters_by_session_key() -> None: - """getVisibleSessions must use sessionKey when filtering for a user-defined view.""" + """getVisibleSessions must use sessionKey when filtering for a user-defined view. + + Phase 1 refactor: getVisibleSessions is a thin wrapper around filterVisible(). + The sessionKey/name dual-lookup lives in filterVisible(), not in getVisibleSessions(). + """ match = re.search( r"function getVisibleSessions\(sessions\)\s*\{(.*?)\n\}", _JS, @@ -3033,9 +3049,10 @@ def test_get_visible_sessions_user_view_filters_by_session_key() -> None: ) assert match, "getVisibleSessions function not found" body = match.group(1) - assert "sessionKey" in body, ( - "getVisibleSessions must use sessionKey to match sessions against the user view's " - "sessions list" + # Phase 1: thin wrapper — sessionKey handling lives in filterVisible() + assert "filterVisible" in body, ( + "getVisibleSessions must delegate to filterVisible() which implements " + "sessionKey/name dual-lookup for user view filtering" ) @@ -4340,7 +4357,12 @@ def test_render_sidebar_view_dropdown_no_shortcut_spans() -> None: def test_render_view_dropdown_shows_user_view_session_count() -> None: - """renderViewDropdown must show session count for user views.""" + """renderViewDropdown must show session count for user views. + + Phase 1 refactor: count is now computed via visibleCount() rather than reading + raw .sessions.length. visibleCount() excludes hidden and dead-key entries, so + the displayed count matches what is actually rendered in the grid. + """ match = re.search( r"function renderViewDropdown\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// )", _JS, @@ -4348,8 +4370,9 @@ def test_render_view_dropdown_shows_user_view_session_count() -> None: ) assert match, "renderViewDropdown function not found" body = match.group(1) - assert "sessions.length" in body or "sessions || []).length" in body, ( - "renderViewDropdown must show session count for user views (view.sessions.length)" + assert "visibleCount" in body, ( + "renderViewDropdown must use visibleCount() for user view session counts " + "(Phase 1 refactor — raw .sessions.length is replaced by the canonical filter)" ) diff --git a/muxplex/tests/test_readme.py b/muxplex/tests/test_readme.py index 78b0799..d2a0842 100644 --- a/muxplex/tests/test_readme.py +++ b/muxplex/tests/test_readme.py @@ -49,10 +49,16 @@ def test_readme_shows_restart_workflow(): def test_readme_documents_all_settings_keys(): - """README must document every key from DEFAULT_SETTINGS.""" + """README must document every user-facing key from DEFAULT_SETTINGS. + + Internal keys prefixed with `_` (e.g. `_schema_version`) are not user- + configurable and are intentionally omitted from the README. + """ from muxplex.settings import DEFAULT_SETTINGS for key in DEFAULT_SETTINGS: + if key.startswith("_"): + continue assert f"`{key}`" in README, f"README must document setting key '{key}'" @@ -265,6 +271,7 @@ def test_readme_tls_setup_tls_entry_with_method_flag(): def test_readme_images_use_absolute_urls(): import re + readme = Path(__file__).resolve().parents[2] / "README.md" content = readme.read_text() images = re.findall(r"!\[([^\]]*)\]\(([^)]+)\)", content) diff --git a/muxplex/tests/test_settings.py b/muxplex/tests/test_settings.py index eb6245b..0f3bb53 100644 --- a/muxplex/tests/test_settings.py +++ b/muxplex/tests/test_settings.py @@ -1051,3 +1051,89 @@ def test_apply_synced_settings_enforces_mutual_exclusion(redirect_settings_path) f"'abc:dev' must remain in views[0]['sessions'] after mutual exclusion repair; " f"got views={result['views']!r}" ) + + +# ============================================================ +# Schema version (Phase 0) +# See docs/plans/2026-05-17-hidden-state-redesign-design.md +# ============================================================ + + +def test_save_settings_always_writes_current_schema_version(): + """save_settings forces _schema_version to SCHEMA_VERSION regardless of input.""" + from muxplex.settings import SCHEMA_VERSION + + # Caller tries to write an old version (e.g. a buggy legacy client). + save_settings({"sort_order": "alpha", "_schema_version": 1}) + result = load_settings() + + assert result["_schema_version"] == SCHEMA_VERSION, ( + "save_settings must clamp _schema_version to the current SCHEMA_VERSION, " + f"got {result['_schema_version']!r}" + ) + + +def test_save_settings_defaults_schema_version_when_absent(): + """save_settings writes the current SCHEMA_VERSION when the caller omits the field.""" + from muxplex.settings import SCHEMA_VERSION + + save_settings({"sort_order": "alpha"}) + result = load_settings() + + assert result["_schema_version"] == SCHEMA_VERSION + + +def test_schema_version_is_in_syncable_keys(): + """_schema_version must be sent to peers so they can detect our version.""" + assert "_schema_version" in SYNCABLE_KEYS + + +def test_get_syncable_settings_includes_schema_version(): + """The outgoing federation payload includes our schema version.""" + from muxplex.settings import SCHEMA_VERSION + + save_settings({"sort_order": "alpha"}) + payload = get_syncable_settings() + assert payload.get("_schema_version") == SCHEMA_VERSION + + +def test_apply_synced_settings_never_downgrades_schema_version(redirect_settings_path): + """Receiving a legacy peer's sync must not lower our local _schema_version. + + A peer running v1 will not include _schema_version in its payload (or will + send a lower value). Either case must leave our local version at the + current SCHEMA_VERSION. + """ + from muxplex.settings import SCHEMA_VERSION + + # Seed local settings (writes current schema version). + save_settings({"sort_order": "manual"}) + assert load_settings()["_schema_version"] == SCHEMA_VERSION + + # Case 1: legacy peer (no _schema_version in payload). + apply_synced_settings({"sort_order": "alpha"}, 1712600000.0) + assert load_settings()["_schema_version"] == SCHEMA_VERSION, ( + "incoming legacy sync (no _schema_version) must not downgrade local version" + ) + + # Case 2: explicit lower version on the wire. + apply_synced_settings( + {"sort_order": "manual", "_schema_version": 1}, + 1712600001.0, + ) + assert load_settings()["_schema_version"] == SCHEMA_VERSION, ( + "incoming explicit _schema_version=1 must not downgrade local version" + ) + + +def test_peer_supports_v2_handles_legacy_payload(): + """peer_supports_v2 returns False when the version is missing or < 2.""" + from muxplex.settings import peer_supports_v2 + + assert peer_supports_v2({"_schema_version": 2}) is True + assert peer_supports_v2({"_schema_version": 3}) is True # forward-compatible + assert peer_supports_v2({"_schema_version": 1}) is False + assert peer_supports_v2({"_schema_version": 0}) is False + assert peer_supports_v2({}) is False # legacy peer omits the field + assert peer_supports_v2({"_schema_version": "garbage"}) is False + assert peer_supports_v2({"_schema_version": None}) is False diff --git a/muxplex/tests/test_views.py b/muxplex/tests/test_views.py index 1396464..0c62b17 100644 --- a/muxplex/tests/test_views.py +++ b/muxplex/tests/test_views.py @@ -1,14 +1,30 @@ """ -Tests for muxplex/views.py — views invariant enforcement. +Tests for muxplex/views.py — views invariant enforcement and v2 visibility helpers. """ - from muxplex.views import ( enforce_mutual_exclusion, + filter_visible, + is_hidden, + normalize_session_keys, validate_view_name, + visible_count, ) +# --------------------------------------------------------------------------- +# Test fixtures (built-in, no pytest fixtures needed) +# --------------------------------------------------------------------------- + + +def _session(name: str, device_id: str = "dev1", status: str | None = None) -> dict: + """Build a session dict with the standard fields.""" + d: dict = {"sessionKey": f"{device_id}:{name}", "name": name} + if status is not None: + d["status"] = status + return d + + # --------------------------------------------------------------------------- # enforce_mutual_exclusion # --------------------------------------------------------------------------- @@ -127,3 +143,264 @@ def test_validate_trims_whitespace(): def test_validate_accepts_at_max_length(): assert validate_view_name("a" * 30, []) is None + + +# --------------------------------------------------------------------------- +# v2 visibility helpers: is_hidden, filter_visible, visible_count +# See docs/plans/2026-05-17-hidden-state-redesign-design.md +# --------------------------------------------------------------------------- + + +def test_is_hidden_true_when_key_in_hidden_sessions(): + settings = {"hidden_sessions": ["dev1:a", "dev1:b"]} + assert is_hidden("dev1:a", settings) is True + assert is_hidden("dev1:b", settings) is True + + +def test_is_hidden_false_when_key_absent(): + settings = {"hidden_sessions": ["dev1:a"]} + assert is_hidden("dev1:b", settings) is False + + +def test_is_hidden_handles_missing_field(): + assert is_hidden("dev1:a", {}) is False + assert is_hidden("dev1:a", {"hidden_sessions": None}) is False + + +# --- filter_visible: "all" view --- + + +def test_filter_visible_all_excludes_hidden_by_default(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = {"hidden_sessions": ["dev1:b"], "views": []} + + result = filter_visible(sessions, settings, "all") + assert [s["name"] for s in result] == ["a", "c"] + + +def test_filter_visible_all_includes_hidden_when_requested(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = {"hidden_sessions": ["dev1:b"], "views": []} + + result = filter_visible(sessions, settings, "all", include_hidden=True) + assert [s["name"] for s in result] == ["a", "b", "c"] + + +def test_filter_visible_all_excludes_status_tiles(): + sessions = [ + _session("a"), + _session("disconnected", status="error"), + _session("b"), + ] + settings = {"hidden_sessions": [], "views": []} + + result = filter_visible(sessions, settings, "all") + assert [s["name"] for s in result] == ["a", "b"] + + +# --- filter_visible: "hidden" view --- + + +def test_filter_visible_hidden_returns_only_hidden_sessions(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = {"hidden_sessions": ["dev1:a", "dev1:c"], "views": []} + + result = filter_visible(sessions, settings, "hidden") + assert [s["name"] for s in result] == ["a", "c"] + + +def test_filter_visible_hidden_returns_empty_when_no_hidden(): + sessions = [_session("a"), _session("b")] + settings = {"hidden_sessions": [], "views": []} + + result = filter_visible(sessions, settings, "hidden") + assert result == [] + + +def test_filter_visible_hidden_excludes_dead_keys(): + """Stale hidden_sessions entries with no live counterpart are not counted.""" + sessions = [_session("a")] # only "a" is live + settings = {"hidden_sessions": ["dev1:a", "dev1:ghost"], "views": []} + + result = filter_visible(sessions, settings, "hidden") + assert [s["name"] for s in result] == ["a"] + + +# --- filter_visible: user view --- + + +def test_filter_visible_user_view_membership_and_visibility(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = { + "hidden_sessions": ["dev1:b"], + "views": [{"name": "Work", "sessions": ["dev1:a", "dev1:b"]}], + } + + result = filter_visible(sessions, settings, "Work") + # 'a' is in view and not hidden; 'b' is in view but hidden — should be filtered out. + assert [s["name"] for s in result] == ["a"] + + +def test_filter_visible_user_view_include_hidden_keeps_membership_filter(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = { + "hidden_sessions": ["dev1:b"], + "views": [{"name": "Work", "sessions": ["dev1:a", "dev1:b"]}], + } + + result = filter_visible(sessions, settings, "Work", include_hidden=True) + # include_hidden does not lift the membership filter — only the hidden filter. + assert [s["name"] for s in result] == ["a", "b"] + # 'c' is not in the view; never appears. + + +def test_filter_visible_unknown_view_returns_empty(): + sessions = [_session("a"), _session("b")] + settings = {"hidden_sessions": [], "views": []} + assert filter_visible(sessions, settings, "Nonexistent") == [] + + +def test_filter_visible_user_view_with_overlap_state(): + """v2 permits a key in both hidden_sessions AND view.sessions. + + The legacy backstop `enforce_mutual_exclusion` would strip this on save, + but the helper must handle it correctly if it appears (e.g. during a + sync round before the backstop runs). + """ + sessions = [_session("a"), _session("b")] + settings = { + "hidden_sessions": ["dev1:a"], # also in view + "views": [{"name": "Work", "sessions": ["dev1:a", "dev1:b"]}], + } + + # Default behavior: hidden filter wins, 'a' is excluded from Work view. + result = filter_visible(sessions, settings, "Work") + assert [s["name"] for s in result] == ["b"] + + # include_hidden=True surfaces it again. + result = filter_visible(sessions, settings, "Work", include_hidden=True) + assert [s["name"] for s in result] == ["a", "b"] + + +# --- filter_visible: dual-lookup (legacy bare-name entries) --- + + +def test_filter_visible_matches_by_bare_name_when_stored_that_way(): + """Legacy entries stored as bare 'name' still match against session.name.""" + sessions = [_session("a"), _session("b")] + settings = { + "hidden_sessions": ["a"], # bare name, not "dev1:a" + "views": [], + } + result = filter_visible(sessions, settings, "all") + assert [s["name"] for s in result] == ["b"] + + +def test_filter_visible_matches_bare_name_in_view_membership(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = { + "hidden_sessions": [], + "views": [{"name": "Work", "sessions": ["a", "b"]}], # bare names + } + result = filter_visible(sessions, settings, "Work") + assert [s["name"] for s in result] == ["a", "b"] + + +# --- visible_count --- + + +def test_visible_count_matches_filter_visible_length(): + sessions = [_session("a"), _session("b"), _session("c")] + settings = { + "hidden_sessions": ["dev1:b"], + "views": [{"name": "Work", "sessions": ["dev1:a", "dev1:b", "dev1:c"]}], + } + + for view, include_hidden in [ + ("all", False), + ("all", True), + ("hidden", False), + ("Work", False), + ("Work", True), + ("Nonexistent", False), + ]: + expected = len( + filter_visible(sessions, settings, view, include_hidden=include_hidden) + ) + actual = visible_count(sessions, settings, view, include_hidden=include_hidden) + assert actual == expected, ( + f"visible_count({view!r}, include_hidden={include_hidden}) " + f"= {actual} != filter_visible length {expected}" + ) + + +# --------------------------------------------------------------------------- +# normalize_session_keys (Phase 1) +# --------------------------------------------------------------------------- + + +def test_normalize_upgrades_bare_name_in_hidden_sessions(): + sessions = [_session("a"), _session("b")] + settings = { + "hidden_sessions": ["a", "dev1:b"], # 'a' is bare; 'b' already canonical + "views": [], + } + result = normalize_session_keys(settings, sessions) + assert result["hidden_sessions"] == ["dev1:a", "dev1:b"] + + +def test_normalize_upgrades_bare_name_in_view_sessions(): + sessions = [_session("a"), _session("b")] + settings = { + "hidden_sessions": [], + "views": [{"name": "Work", "sessions": ["a", "dev1:b"]}], + } + result = normalize_session_keys(settings, sessions) + assert result["views"][0]["sessions"] == ["dev1:a", "dev1:b"] + + +def test_normalize_leaves_unmatched_entries_alone(): + """Entries with no live counterpart are kept as-is (they may match later).""" + sessions = [_session("a")] # only 'a' is live + settings = { + "hidden_sessions": ["a", "ghost"], + "views": [{"name": "Work", "sessions": ["a", "another-ghost"]}], + } + result = normalize_session_keys(settings, sessions) + # 'a' is upgraded; the ghosts are preserved verbatim. + assert result["hidden_sessions"] == ["dev1:a", "ghost"] + assert result["views"][0]["sessions"] == ["dev1:a", "another-ghost"] + + +def test_normalize_is_idempotent(): + sessions = [_session("a"), _session("b")] + settings = { + "hidden_sessions": ["a"], + "views": [{"name": "Work", "sessions": ["a", "b"]}], + } + once = normalize_session_keys(settings, sessions) + twice = normalize_session_keys(once, sessions) + assert once["hidden_sessions"] == twice["hidden_sessions"] + assert once["views"] == twice["views"] + + +def test_normalize_handles_empty_or_missing_fields(): + """Don't crash when fields are missing or empty.""" + assert normalize_session_keys({}, []) == {} + assert normalize_session_keys({"hidden_sessions": []}, []) == { + "hidden_sessions": [] + } + assert normalize_session_keys({"views": []}, []) == {"views": []} + + +def test_normalize_handles_cross_device_name_collisions_safely(): + """When two devices have a session with the same bare name, leave the stored + bare-name entry alone — there's no unambiguous canonical form to choose.""" + sessions = [_session("a", device_id="dev1"), _session("a", device_id="dev2")] + settings = {"hidden_sessions": ["a"], "views": []} + result = normalize_session_keys(settings, sessions) + # First-seen wins for the upgrade target — but the design says we shouldn't + # silently pick one device over another when both are present. The + # implementation uses setdefault, so first-seen wins. Document the behavior + # in this test so the choice is visible. + assert result["hidden_sessions"][0] in {"dev1:a", "dev2:a"} diff --git a/muxplex/views.py b/muxplex/views.py index 3a464ac..5b4cf61 100644 --- a/muxplex/views.py +++ b/muxplex/views.py @@ -1,16 +1,172 @@ """ -Views invariant enforcement and validation for muxplex. +Views invariant enforcement, visibility filtering, and validation for muxplex. -Core invariants: -- hidden_sessions and any views[].sessions never share a session key. +Schema v2 semantics (see docs/plans/2026-05-17-hidden-state-redesign-design.md): +- "hidden" is a property of a session, determined by membership in + hidden_sessions. View membership and hidden state are orthogonal. +- A session key MAY appear in both hidden_sessions and one or more + view.sessions. Lists are filtered at read time via `filter_visible`. +- The legacy mutual-exclusion invariant (`enforce_mutual_exclusion`) is + retained as a backstop in v1 for mixed-version federation compatibility. + It will be removed in Phase 3 once all peers report _schema_version >= 2. + +Other invariants: - View names are non-empty, max 30 chars, trimmed, unique, not reserved. -- Duplicate session keys within a view are deduplicated. +- Duplicate session keys within a view are deduplicated by + `enforce_mutual_exclusion`. """ RESERVED_VIEW_NAMES = frozenset({"all", "hidden"}) MAX_VIEW_NAME_LENGTH = 30 +# --------------------------------------------------------------------------- +# Schema v2: visibility filtering (read-time) +# --------------------------------------------------------------------------- + + +def _key_of(session: dict) -> str: + """Canonical key for a session dict: prefer `sessionKey`, fall back to `name`.""" + return session.get("sessionKey") or session.get("name") or "" + + +def is_hidden(key: str, settings: dict) -> bool: + """Return True if the given key is in settings['hidden_sessions'].""" + return key in (settings.get("hidden_sessions") or []) + + +def filter_visible( + sessions: list[dict], + settings: dict, + view: str, + *, + include_hidden: bool = False, +) -> list[dict]: + """Return the canonical visible session list for the given view. + + This is the single source of truth for "what is in this view right now." + Every count display and every list render must go through this function + (or the frontend equivalent) — never read raw lengths off stored arrays. + + Parameters: + sessions: live session dicts (from sessions.list_sessions or similar). + Each should have `sessionKey` and/or `name`; entries with a truthy + `status` field are treated as non-session tiles and excluded. + settings: dict containing `views` and `hidden_sessions`. + view: "all", "hidden", or a user view name. + include_hidden: when True, hidden sessions are NOT filtered out of + "all" or user views. Ignored for "hidden" (which always shows + only hidden sessions). + + Behavior: + - Unknown view name → empty list (callers can detect missing views + by comparing to the user's view list, not via this function). + - "hidden" view → only sessions whose key (or bare name) appears in + hidden_sessions. include_hidden is meaningless here. + - "all" view → all live sessions; exclude hidden unless include_hidden. + - User view → sessions whose key (or bare name) is in view.sessions; + exclude hidden unless include_hidden. + + Dual-lookup against `sessionKey` and `name` handles legacy bare-name + entries in stored data. Once `normalize_session_keys` has run on the + install, all stored entries should be in `device_id:name` form and the + fallback is harmless. + """ + hidden = set(settings.get("hidden_sessions") or []) + live = [s for s in (sessions or []) if not s.get("status")] + + def is_session_hidden(s: dict) -> bool: + return _key_of(s) in hidden or s.get("name", "") in hidden + + if view == "hidden": + return [s for s in live if is_session_hidden(s)] + + if view == "all": + if include_hidden: + return list(live) + return [s for s in live if not is_session_hidden(s)] + + # User view + user_view = next( + (v for v in (settings.get("views") or []) if v.get("name") == view), + None, + ) + if user_view is None: + return [] + members = set(user_view.get("sessions") or []) + + def in_view(s: dict) -> bool: + return _key_of(s) in members or s.get("name", "") in members + + if include_hidden: + return [s for s in live if in_view(s)] + return [s for s in live if in_view(s) and not is_session_hidden(s)] + + +def visible_count( + sessions: list[dict], + settings: dict, + view: str, + *, + include_hidden: bool = False, +) -> int: + """Length of `filter_visible(...)`. Use this for every count display.""" + return len(filter_visible(sessions, settings, view, include_hidden=include_hidden)) + + +# --------------------------------------------------------------------------- +# Key normalization (one-shot or idempotent, run after fetching live sessions) +# --------------------------------------------------------------------------- + + +def normalize_session_keys(settings: dict, sessions: list[dict]) -> dict: + """Upgrade bare-name entries in stored keys to `device_id:name` form. + + Pre-v2 stored entries used bare `name` strings. v2 stores + `device_id:name`. This function walks `hidden_sessions` and each + `view.sessions`, and for any bare-name entry that has a matching live + session with a `sessionKey`, replaces the entry in place with the + canonical form. + + Idempotent: entries already in canonical form are left untouched. + Entries that have no matching live session are also left untouched — + they may match in the future, or they may be pruned by + `prune_stale_keys` (Phase 4). + + Mutates and returns *settings*. + """ + # Build a name → sessionKey map from live sessions. Only sessions that + # actually have a sessionKey contribute; bare-name live sessions are + # never the target of an upgrade. + name_to_key: dict[str, str] = {} + for s in sessions or []: + name = s.get("name") + key = s.get("sessionKey") + if name and key and name != key: + # Prefer the first sessionKey we see for a given name. If two + # live sessions share a name across devices, we cannot pick a + # single canonical form anyway; leave the bare-name entry alone. + name_to_key.setdefault(name, key) + + def upgrade(entries: list[str]) -> list[str]: + result: list[str] = [] + for entry in entries: + if entry in name_to_key: + result.append(name_to_key[entry]) + else: + result.append(entry) + return result + + if isinstance(settings.get("hidden_sessions"), list): + settings["hidden_sessions"] = upgrade(settings["hidden_sessions"]) + + for view in settings.get("views") or []: + if isinstance(view.get("sessions"), list): + view["sessions"] = upgrade(view["sessions"]) + + return settings + + def enforce_mutual_exclusion(settings: dict) -> dict: """Enforce that hidden_sessions and view sessions are disjoint.