Skip to content

feat(viz-popover): grey out a control until the toggle it depends on is on - #78

Merged
cchacs merged 3 commits into
mainfrom
ccr-6378c956-yvg2qf
Oct 4, 2026
Merged

cchacs merged 3 commits into
mainfrom
ccr-6378c956-yvg2qf

Conversation

@cchacs

@cchacs cchacs commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

What

A viz control descriptor can now set dependsOn: '<toggle key>'. The per-panel Viz ⚙ popover then disables and dims that row, with a "Requires …" tooltip, until the named toggle is on. It follows later clicks and the saved state when the popover is rebuilt.

Why: 3D Highway's "Locked zoom" only has an effect while "Lock camera at frets 1–12" is on (and the visible notes are at or below fret 12). In a split panel the slider was listed with no hint of that, so it looked broken.

Changes

  • screen.js: buildVizPopover records each built row and input, and _wireVizControlDeps applies the greying.
    • Only a toggle can gate a control. A missing or non-toggle dependsOn is ignored, so the control stays usable.
    • A control that names itself, or sits on a dependency cycle, is treated as ungated, so a bad descriptor can't lock a control.
    • Gating is transitive: a control behind a greyed-out gate is greyed out too.
  • tests/screen.test.js: six tests for the above, using a makeListeningVizElement helper that records and fires change listeners. The shared DOM stub now also has disabled and title.
  • CLAUDE.md: documents the field and a caveat. dependsOn is only honoured on the legacy factory panelControls path. Core whitelists declared capabilities.visualization.settings descriptors in both static/capabilities.js and plugins/__init__.py, so it is dropped silently there. A plugin that migrates from panelControls to declared settings would lose the greying without any signal until core carries the field.
  • plugin.json: 1.15.0 → 1.16.0.

Pairs with get-flashbacks/feedBack#139, where 3D Highway declares dependsOn: 'cameraLockLow' on its cameraLockZoom control. This PR is harmless without it: no current descriptor sets dependsOn, so nothing changes until the core change lands.

How it was checked

  • node --test tests/*.test.js: 209 pass, 0 fail. The self-reference, cycle and chained-gate tests fail against the first version of this change.
  • Ran the real host with Split Screen in headless Chromium and 3D Highway on a panel:
    • lock off: "Locked zoom" is disabled, 40% opacity, with the tooltip;
    • lock ticked: enabled with no tooltip;
    • lock unticked: disabled again;
    • popover rebuilt with the lock saved on: stays enabled.
  • Not checked: light themes, other viz plugins' popovers, or pop-out follower windows.

🤖 Generated with Claude Code

https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH

…is on

A viz control descriptor can now set dependsOn: '<toggle key>'. The Viz
popover disables and dims that row, with a "Requires ..." tooltip, until
the toggle is on, and follows later clicks and saved state. 3D Highway's
"Locked zoom" does nothing unless "Lock camera at frets 1-12" is on, so it
looked broken in a panel.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH
@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Comment thread screen.js Resolved
Comment thread screen.js Resolved
Comment thread screen.js Resolved
@kilo-code-bot

kilo-code-bot Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • screen.js - 0 issues (all previous gating logic issues fixed in a4fafb5)
  • tests/screen.test.js - 0 issues (comprehensive test coverage added for dependsOn greying)
  • CLAUDE.md - 0 issues (documentation updated to match implementation)
Previous Review Summary (commit babed0c)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit babed0c)

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 2
Issue Details (click to expand)

WARNING

File Line Issue
screen.js 2156 No self-reference/cycle guard: a descriptor whose dependsOn names itself (or a two-toggle cycle) is disabled by its own initial apply() and, with the state persisted in localStorage, can never be re-enabled from the UI

SUGGESTION

File Line Issue
screen.js 2158 Chained gates not resolved: on reads only gate.input.checked, so with A→B→C and C off, B is disabled but A stays enabled and writable behind an unreachable gate
screen.js 2164 No test covers the gating, and makeVizElementStub's addEventListener: noop would let a fully stubbed-out _wireVizControlDeps pass
Files Reviewed (3 files)
  • screen.js - 3 issues (gating logic in buildVizPopover / new _wireVizControlDeps)
  • CLAUDE.md - 0 issues (descriptor-field doc matches the implemented behaviour)
  • plugin.json - 0 issues (1.15.0 → 1.16.0 bump)

Fix these issues in Kilo Cloud

Notes: review is scoped to changed lines at HEAD babed0c. The change itself is sound: Object.create(null) for the built map avoids prototype-key collisions, the wiring runs after the build loop so a gate declared after its dependent still resolves, row.title/labels are set via properties (no innerHTML injection), and addEventListener('change') coexists safely with the toggle's existing onchange. Only the defensive/robustness gaps above are outstanding; none blocks the feature's intent.


Reviewed by free · Input: 527.5K · Output: 16.1K · Cached: 267.8K

…dd tests

A control that named itself, or sat on a dependsOn cycle, was disabled by
its own gate and could never be re-enabled. A control behind a greyed-out
gate also stayed live because only the gate's checked state was read.

Treat self-references and cycle members as ungated, and compute usability
through the whole gate chain. Cover the greying, saved state, ignored
targets, self-reference, cycle and chain cases in the suite; the DOM stub
now has disabled/title and a recording addEventListener variant.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ The gating logic holds up — the live-checkbox read, the Object.create(null) built map and the toggle-only guard all behave as documented. One robustness gap inline, plus two smaller notes.

Reviewed changes — all three changed files read in full at babed0c (the PR's only commit).

  • dependsOn descriptor field — buildVizPopover now records every row/input it builds in a built map keyed by control key, and a new internal _wireVizControlDeps runs after the build loop to grey out and disable any control whose descriptor names a toggle it depends on, with a Requires "<gate label>" tooltip on the row.
  • Live gating — the dependent row's disabled / opacity / cursor / title are recomputed from the gate checkbox's live .checked by an apply() closure, invoked once at build time and again on the gate's change, so it follows both the saved state on rebuild and later clicks.
  • Fail-open guard — an unknown or non-toggle dependsOn target is skipped, leaving the control usable.
  • Docs + version — CLAUDE.md documents the new field and the caveat about the declared capability path; plugin.json goes 1.15.0 → 1.16.0.

ℹ️ dependsOn is dropped silently on the declared path, so it will fail quietly the day highway_3d declares capabilities

The caveat CLAUDE.md now records is correct, but "may strip" understates it: the drop is deterministic and silent in both core layers, and it needs no warning to fire. _normalizeSettings in core's static/capabilities.js builds a whitelist descriptor (key, type, label?, default?, min/max/step, options?), and _validate_capability_settings in core's plugins/__init__.py rebuilds the same whitelist — warning only on unsupported type, missing key, duplicate key and non-numeric bounds, never on an unrecognised key. Verified that plugins/highway_3d/plugin.json still has "capabilities": {}, so the pairing in get-flashbacks/feedBack#139 works today via panelControls.

That makes the sequencing the thing to watch: getPanelControlsFor returns a provider's declared settings first and only falls back to fac.panelControls for highway_3d, and its own comment says the fallback exists "until the plugin declares capability settings". At that point highway_3d's dependsOn stops being read at all and the greying vanishes with no signal in either core layer or in this one — while the plugin's source comment already asserts "hosts grey it out until the lock is on". Worth a note in the plugin guide now that this field exists; no code change needed here.

ℹ️ Nitpicks

  • The new behaviour is untested, and the PR's stated reason for that doesn't hold. "The existing suite stubs the DOM and does not build the popover" — it does: buildVizPopover is exported (screen.js:6167) and seven tests in tests/screen.test.js drive it through makeVizElementStub. Only the live-click half needs help, since the stub's addEventListener: noop swallows the gate's change listener; recording listeners per element is a few lines, then one test declaring a toggle + a dependsOn range pins both the initial disabled state and the un-disable on toggle.
  • _wireVizControlDeps is the only JSDoc block in screen.js without @param tags (67 tags elsewhere, no other exception). @param {*} controls / @param {*} built would match the file.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using big-pickle (free) | 𝕏

Comment thread screen.js Resolved
…-settings path

Add the missing @PARAM tags on _wireVizControlDeps and state the caveat
precisely: core whitelists declared setting descriptors in both layers, so
dependsOn is silently dropped there and would stop working the day a
plugin migrates from panelControls to declared settings.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found. The two commits pushed during this review land cleanly and close the previous review's inline finding and its one cross-cutting concern.

Reviewed changes — the delta against the prior pullfrog review: a4fafb5 (gating rework + tests) and 9c79f4f (docs), both read in full.

  • Cycle-safe, fail-open gating — _wireVizControlDeps now resolves gates via a byKey lookup plus an inCycle chain walk, so a control naming itself or sitting on a cycle is excluded from gated and stays usable. That closes the self-reference lockout from the previous review; the identity clause in gateOf and the cycle walk are mutually redundant, so no descriptor shape can recurse inside usable().
  • Transitive gating — usable() walks the whole gate chain, so a control sitting behind a greyed-out gate is greyed out too rather than staying live off a stale saved checked. One refresh listener per distinct gate replaces the per-control listener fan-out, and refresh() still runs once at build time so the saved state is honoured.
  • Six new tests — makeListeningVizElement records and fires change listeners, buildDepsPopover maps built rows back to their descriptors, and the shared stub gains disabled/title. Confirmed as real coverage rather than theatre: rewriting usable() to read only its immediate gate turns the transitive test red, and inverting the g.key === key branch in inCycle turns the cycle test red. node --test tests/*.test.js → 209 pass, 0 fail.
  • CLAUDE.md — the dependsOn paragraph now records the verified core behaviour (whitelisted in both static/capabilities.js and plugins/__init__.py, so dropped silently on the declared path), the getPanelControlsFor precedence trap that would lose the greying when a plugin starts declaring its settings, and the cycle/transitivity semantics.

Pullfrog  | View workflow run | Using big-pickle (free) | 𝕏

@cchacs
cchacs merged commit 111e69f into main Oct 4, 2026
9 checks passed
@cchacs
cchacs deleted the ccr-6378c956-yvg2qf branch October 4, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants