Repository navigation
feat(viz-popover): grey out a control until the toggle it depends on is on - #78
Conversation
…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
Up to standards ✅🟢 Issues
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
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
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Notes: review is scoped to changed lines at HEAD 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
There was a problem hiding this comment.
ℹ️ The gating logic holds up — the live-checkbox read, the
Object.create(null)builtmap 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).
dependsOndescriptor field —buildVizPopovernow records every row/input it builds in abuiltmap keyed by control key, and a new internal_wireVizControlDepsruns after the build loop to grey out and disable any control whose descriptor names a toggle it depends on, with aRequires "<gate label>"tooltip on the row.- Live gating — the dependent row's
disabled/opacity/cursor/titleare recomputed from the gate checkbox's live.checkedby anapply()closure, invoked once at build time and again on the gate'schange, so it follows both the saved state on rebuild and later clicks. - Fail-open guard — an unknown or non-toggle
dependsOntarget is skipped, leaving the control usable. - Docs + version —
CLAUDE.mddocuments the new field and the caveat about the declared capability path;plugin.jsongoes 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:
buildVizPopoveris exported (screen.js:6167) and seven tests intests/screen.test.jsdrive it throughmakeVizElementStub. Only the live-click half needs help, since the stub'saddEventListener: noopswallows the gate'schangelistener; recording listeners per element is a few lines, then one test declaring a toggle + adependsOnrange pins both the initial disabled state and the un-disable on toggle. _wireVizControlDepsis the only JSDoc block inscreen.jswithout@paramtags (67 tags elsewhere, no other exception).@param {*} controls/@param {*} builtwould match the file.
big-pickle (free) | 𝕏
…-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
There was a problem hiding this comment.
✅ 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 —
_wireVizControlDepsnow resolves gates via abyKeylookup plus aninCyclechain walk, so a control naming itself or sitting on a cycle is excluded fromgatedand stays usable. That closes the self-reference lockout from the previous review; the identity clause ingateOfand the cycle walk are mutually redundant, so no descriptor shape can recurse insideusable(). - 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 savedchecked. Onerefreshlistener per distinct gate replaces the per-control listener fan-out, andrefresh()still runs once at build time so the saved state is honoured. - Six new tests —
makeListeningVizElementrecords and fireschangelisteners,buildDepsPopovermaps built rows back to their descriptors, and the shared stub gainsdisabled/title. Confirmed as real coverage rather than theatre: rewritingusable()to read only its immediate gate turns the transitive test red, and inverting theg.key === keybranch ininCycleturns the cycle test red.node --test tests/*.test.js→ 209 pass, 0 fail. CLAUDE.md— thedependsOnparagraph now records the verified core behaviour (whitelisted in bothstatic/capabilities.jsandplugins/__init__.py, so dropped silently on the declared path), thegetPanelControlsForprecedence trap that would lose the greying when a plugin starts declaring its settings, and the cycle/transitivity semantics.
big-pickle (free) | 𝕏

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:buildVizPopoverrecords each built row and input, and_wireVizControlDepsapplies the greying.dependsOnis ignored, so the control stays usable.tests/screen.test.js: six tests for the above, using amakeListeningVizElementhelper that records and fireschangelisteners. The shared DOM stub now also hasdisabledandtitle.CLAUDE.md: documents the field and a caveat.dependsOnis only honoured on the legacy factorypanelControlspath. Core whitelists declaredcapabilities.visualization.settingsdescriptors in bothstatic/capabilities.jsandplugins/__init__.py, so it is dropped silently there. A plugin that migrates frompanelControlsto 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 itscameraLockZoomcontrol. This PR is harmless without it: no current descriptor setsdependsOn, 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.🤖 Generated with Claude Code
https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH