Skip to content

fix(notebook): wrap colab sliders before track collapse - #409

Open
FlorinSenoner wants to merge 6 commits into
mainfrom
fix/346-colab-slider-responsive
Open

fix(notebook): wrap colab sliders before track collapse#409
FlorinSenoner wants to merge 6 commits into
mainfrom
fix/346-colab-slider-responsive

Conversation

@FlorinSenoner

Copy link
Copy Markdown
Collaborator

Summary

  • make generated Colab parameter groups wrap before fixed slider chrome collapses their tracks
  • add a regression test that executes the notebook's real widget-generation cell
  • document the responsive-layout requirement and implementation in OpenSpec

Root cause

Each generated parameter group used flex: 1 1 220px, while the real ipywidgets slider chrome requires about 228 px before its track has any usable width. With three groups enabled, the flex row kept all cards on one line at narrow notebook widths, and the tracks collapsed inside overflow-hidden cards.

Fix

Raise the group flex basis to 300px. This makes groups wrap to the next row before their slider tracks collapse, without changing slider values or behavior.

Reproduction and verification

On current main, executing the real Generate ProtSpace notebook cell with UMAP and t-SNE enabled produced:

  • 1280×720: cards about 365 px; tracks about 137 px
  • 900×720: cards 262 px; tracks 34 px
  • 800×720: cards about 228.7 px; tracks about 0.67 px

After the fix at 800×720:

  • UMAP/PaCMAP/LocalMAP cards: 345 px; tracks: 117 px
  • t-SNE card: 694 px on the next row; tracks: 466 px
  • clicking the repaired n_neighbors track updated the real widget value from 25 to 249

Tests

  • pytest apps/protspace/tests/test_notebook_layout.py
  • ruff check apps/protspace/tests/test_notebook_layout.py
  • ruff format --check apps/protspace/tests/test_notebook_layout.py
  • jq empty apps/protspace/notebooks/ProtSpace_Preparation.ipynb
  • openspec validate fix-colab-slider-responsive --strict
  • pnpm precommit

Closes #346

@FlorinSenoner
FlorinSenoner marked this pull request as ready for review August 1, 2026 20:40
@tsenoner

tsenoner commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Automated review

Does it solve #346? Yes — raising the DR parameter card flex basis from 220px to 300px makes the groups wrap onto a new row before the ipywidgets slider chrome (~228px) squeezes the track to nothing, which is the "breakpoint is too late" the issue describes. I also checked the notebook's other wrap container: the annotation columns at flex: 1 1 200px hold Checkbox widgets laid out at width: auto, which have no track to collapse, so leaving those at 200px is correct.

Found 2 issues:

  1. The new test pins the constant rather than the property it stands for. MIN_PARAMETER_GROUP_BASIS_PX = 300 restates the notebook literal, so the assertion only proves the two numbers still agree. The real requirement is that the basis exceeds the slider chrome, which is a function of description_width (110px) plus the card's padding and border — raise either of those and the tracks collapse again at 300px while this test stays green.

for group, _methods in parameter_groups:
flex_basis = group.layout.flex.split()[-1]
assert flex_basis.endswith("px")
basis_px = int(flex_basis.removesuffix("px"))
assert basis_px >= MIN_PARAMETER_GROUP_BASIS_PX

  1. flex: 1 1 300px keeps flex-shrink: 1, so the guarantee only holds while the notebook area is at least ~300px wide. Wrapping protects the multi-card case, but once a single card is alone on its row it shrinks with the container, and with overflow: hidden the track disappears exactly as before. Pairing the basis with a min_width would make the floor hold at any width.

"_l = {\"width\": \"100%\"}\n",
"_g = {\"border\": \"1px solid #666\", \"padding\": \"6px 10px\", \"margin\": \"2px\", \"flex\": \"1 1 300px\", \"overflow\": \"hidden\"}\n",
"\n",

🤖 Generated with Claude Code

Reviewed at a6ed921 against issue #346.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Triaged both points against current head a6ed92117c0b049018efbbd4c7d26c41d81124aa:

  1. Non-actionable for this PR. The test executes the real Generate cell and enforces every emitted parameter group’s explicit OpenSpec contract (flex-basis >= 300px); it is not intended to infer browser chrome from style literals. The design assigns actual geometry to rendered-browser verification, which recorded usable 117px/466px tracks in the reported 800px terminal-compressed case. Deriving the threshold only from description_width, padding, and border would still omit ipywidgets/browser-internal fixed widths.

  2. Non-actionable for [BUG] Colab sliders in preparation notebook disappearing on certain scaling #346’s reported scope. flex-shrink: 1 can shrink a lone card when the entire content area is narrower than its 300px basis, but [BUG] Colab sliders in preparation notebook disappearing on certain scaling #346 is the too-late multi-card wrap under terminal compression. At the reproduced 800px width, current head wraps the cards to 345px/694px with usable tracks. Adding min_width: "300px" would instead force overflow below 300px and would not guarantee that the control remains visible.

No follow-up implementation is needed for these two points.

tsenoner and others added 2 commits August 6, 2026 11:42
- Drop the redundant isinstance list branch when joining notebook cell source
- Assert param_grid wraps and each parameter card keeps overflow hidden, so a
  nowrap row can no longer reintroduce the slider-track collapse undetected
- Drop 'plus configured gaps' from the spec scenarios; ipywidgets Layout has no
  gap trait, so the notebook's gap value never reaches the DOM

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016qoU16kDQxz6U3H2UWbbm2
The previous assertion used `"wrap" in flex_flow`, which also passes for
"row nowrap" -- the exact regression it was written to catch. Verified by
mutation: swapping the notebook's flex_flow to "row nowrap" left the test
green.

Split on whitespace and match the flex-wrap token, and echo the observed
value in the failure message. The same mutation now fails the test, and
the unmutated notebook still passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016qoU16kDQxz6U3H2UWbbm2
@tsenoner

tsenoner commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Adversarial review

Reviewed in an isolated worktree by three independent lenses (code quality, adversarial correctness, issue-resolution audit), with every finding then put through a refuter whose default position was that it is a false positive. 7 raised, 5 survived refutation.

Applied and pushed (e0ddb248)

Behavior-preserving cleanups, verified green before pushing:

  • Fix 1 (simplification) - apps/protspace/tests/test_notebook_layout.py: replaced the two-line source = cell.get(...) + isinstance(source, list) ternary with the single line source_text = "".join(cell.get("source", [])).
  • Fix 2 (test-gap) - apps/protspace/tests/test_notebook_layout.py: after assert parameter_groups, ... added param_grid = namespace["param_grid"] plus assert "wrap" in (param_grid.layout.flex_flow or ""), "Parameter grid must wrap; a nowrap row lets the cards shrink past their tracks".
  • Fix 2 (optional part) - same file: added assert group.layout.overflow == "hidden" as the first statement inside the for group, _methods in parameter_groups: loop. Verified against the change's design.md decision 2, which explicitly commits to keeping overflow: hidden, and against the notebook cell where _g sets "overflow": "hidden".
  • Fix 3 (docs) - openspec/changes/fix-colab-slider-responsive/specs/colab-preparation-controls/spec.md: dropped "plus configured gaps" from both scenarios (lines 9 and 15). Notebook left untouched, per the instruction.

Follow-up commit (386e15e9)

  • Corrected the wrap assertion I had just added. The first version used "wrap" in flex_flow,
    which also passes for "row nowrap" — the exact regression it was written to catch. Confirmed by
    mutation: flipping the notebook to flex_flow: "row nowrap" left the test green. It now matches the
    flex-wrap token exactly ("wrap" in flex_flow.split()); the same mutation fails, the unmutated
    notebook passes.

Issue resolution — resolves the issue

The scoping is correct and, unusually, the stated root cause survives independent verification — I
re-derived the 228px chrome figure from the shipped ipywidgets CSS and it matches to the pixel. The
author correctly identified that only cell 5 has sliders, correctly left the annotation columns at
flex: 1 1 200px alone (ipynb:499 — those have no overflow key, so their automatic minimum size
is min-content and they cannot collapse the way the slider cards did), and correctly fixed the wrap
threshold rather than the description widths.
Two pieces of the stated reasoning do not hold up, though neither invalidates the fix:
(a) design.md asserts "the fix must work in Colab without adding CSS, JavaScript, or dependencies"
and dismisses CSS as "injecting breakpoint CSS into Colab's per-cell widget frame." That constraint
is false: the notebook already ships a <style> block inside cell 2, with a comment explicitly
documenting the per-cell-iframe rule ("This CSS must be in the SAME cell as the Tab widget"). CSS
was an available, already-proven technique here. The flex-basis fix is still the simpler choice, but
the doc justifies it against a premise the repo contradicts.
(b) design.md Decision 2 says "Keep ... the existing overflow: hidden" and then gives a rationale
that is entirely about not injecting CSS — it never analyses what overflow: hidden does. Per CSS
Flexbox 1 section 4.5, a flex item whose overflow is not visible has its automatic minimum size
resolved to 0, so flex-shrink: 1 lets the card shrink arbitrarily below its 300px basis. That is
the one thing standing between this fix and a hard guarantee, and it was kept without being
examined.
"Closes #346" is warranted. The reported configuration (terminal open on a 1717px window) is inside
the repaired band, verified both by the PR's browser measurements and by my own arithmetic. The
title's "on certain scaling" could in principle mean browser zoom, and deep zoom is the one case
still broken (see gap 1) — but at any zoom level that leaves the notebook pane above ~300 CSS px the
fix holds, and the body plus screenshot make terminal compression the actual complaint.

Gaps found by the issue audit (4)
  • No minimum-width floor: overflow: hidden in _g (ProtSpace_Preparation.ipynb:449) zeroes the flex item's automatic minimum size, so flex-shrink: 1 still lets a lone card shrink below the new 300px basis without limit. Once the container drops under ~250 CSS px the track collapses again — and because overflow is hidden, it is clipped rather than scrollable. The PR raised the wrap threshold but did not establish a floor.
    • Why it matters: The change's own spec.md over-promises relative to the code. Scenario 1 asserts unconditionally that "each visible slider retains a usable horizontal track instead of collapsing to its thumb" when the content area becomes too narrow; at a 250px content width that is false, and the failure mode is the exact symptom in issue [BUG] Colab sliders in preparation notebook disappearing on certain scaling #346. The issue title says "on certain scaling", and browser zoom at 250-300% on the reporter's terminal-compressed pane lands in that residual band. Notably, the annotation cards at ipynb:499 do NOT have this problem precisely because they omit overflow, so the codebase already contains the safer pattern.
    • Suggested follow-up: Add "min_width": "300px" to _g (I verified min_width is a real ipywidgets Layout trait; gap is not). The enclosing param_grid HBox inherits .jupyter-widget-box { overflow: auto }, so the result is a horizontal scrollbar instead of a dead slider. Alternatively drop overflow: hidden so the automatic minimum size falls back to min-content (~228px). Either way, extend test_notebook_layout.py to assert the floor so the spec scenario becomes true as written.
  • gap is not an ipywidgets Layout trait, so param_grid's layout={"flex_flow": "row wrap", "gap": "6px", "width": "100%"} (ipynb:465) silently discards the gap — as do the methods HBox at ipynb:747 (gap: 6px) and the annotations HBox (gap: 10px). I confirmed by exec'ing the cell: Layout().trait_names() has no gap, and traitlets emits DeprecationWarning: Passing unrecognized arguments to super(Layout).__init__(gap='6px') ... This error will be raised in a future release of traitlets. The new test at test_notebook_layout.py:26-31 explicitly filters that exact warning away.
    • Why it matters: Two problems. First, accuracy: design.md's Context and spec.md's scenarios both reason about "at least 300 px plus configured gaps to each group" — the gaps do not exist in the rendered output, so the documented layout contract describes something the notebook never produces (the arithmetic error is only ~12px, so the fix still works). Second, and more serious: traitlets states this will become a hard error. When that lands, the entire Generate cell raises TypeError on Colab and the notebook stops working outright. The author's own test surfaced this signal in the exact code path under review and then suppressed it rather than filing or fixing it.
    • Suggested follow-up: Replace the three dead gap entries with a supported mechanism (card margins already provide spacing, or use grid_gap on a GridBox layout), and drop the warning filter from the test so a future unrecognized-Layout-arg regression fails loudly instead of silently. If it is out of scope for this PR, open a follow-up issue — the future-traitlets breakage is a latent total failure of the Generate cell, not a cosmetic nit.
  • The regression guard only pins the flex basis. test_notebook_layout.py asserts basis_px >= 300 per group and nothing else — it does not assert param_grid.layout.flex_flow == "row wrap", does not assert the sliders' description_width (110px, ipynb:447), and does not relate the required basis to the chrome it is supposed to clear.
    • Why it matters: The 300px number is only correct relative to a 228px chrome budget, and that budget is not pinned. Bumping description_width from 110px to 180px, or adding a second readout, pushes chrome past 300px and reintroduces zero-width tracks with a green test. Dropping row wrap from the container reintroduces the bug in a worse form (no wrapping at all) and the test still passes, even though "the groups wrap onto another row" is the literal THEN clause of the spec's first scenario.
    • Suggested follow-up: Assert the container's wrap mode (param_grid.layout.flex_flow) and derive the required basis from the chrome components rather than hardcoding 300 — e.g. compute int(description_width) + 8 + 12 + 72 + 4 + padding + border from the cell's own _s/_g dicts and assert the basis exceeds it by a usable-track margin. That turns the test into a guard on the actual invariant instead of a guard on one magic number.
  • Merging cuts a PyPI patch release for a notebook-only change. protspace-release.yml path-filters on apps/protspace/**, which this PR touches, and semantic-release parses the squash-commit body — which will contain fix(notebook): wrap colab sliders before track collapse.
    • Why it matters: The wheel packages only src/protspace (hatch config in apps/protspace/pyproject.toml), so the notebook is not in the distribution at all. The release publishes a version whose sdist/wheel content is byte-identical to the previous one, and the CHANGELOG gains a fix: entry describing a change users of the package cannot observe. Per the repo's own convention in AGENTS.md, changes with no package-user-visible effect should not drive a version bump.
    • Suggested follow-up: Either retitle the squash commit to chore(notebook): / docs(notebook): at merge time, or accept the no-op release deliberately. This is a merge-time decision only — do not change the branch commit, since the fix: prefix is accurate for the notebook itself.

Findings needing a decision (2)

These were left for you rather than auto-applied: each changes behavior, needs a product call, or reaches outside this diff.

1. param_grid sets "gap", which is not an ipywidgets Layout trait, while the same notebook already uses the recognized grid_gap form elsewhere.

apps/protspace/notebooks/ProtSpace_Preparation.ipynb:465 · medium · reuse

Verified against ipywidgets 8.1.8: "gap" in Layout().trait_names() is False, "grid_gap" is True.
Constructing the Generate cell emits DeprecationWarning: Passing unrecognized arguments to super(Layout).__init__(gap='6px') and the value never reaches the DOM. So the 6px gutter that this
PR's own spec builds its wrap threshold on (specs/colab-preparation-controls/spec.md:9,15 — "at
least 300 px plus configured gaps") does not exist: wrapped cards are separated only by their
margin: 2px. The identical mistake drops gap: 6px on the methods HBox (line 747) and gap: 10px
on the annotations HBox (line 751), while lines 139 and 498 already use the working grid_gap
spelling for GridBox — so the correct trait name is present in this very file. Cost: the responsive
contract this PR is specifying is stated in terms of spacing the notebook never applies, so the next
person tuning the basis reasons from numbers that are 2px off per card and re-derives the same dead
config.

Suggested fix

Pick one, in apps/protspace/notebooks/ProtSpace_Preparation.ipynb. Option A (apply the intended
spacing): rename the key \"gap\" -> \"grid_gap\" at lines 465, 747 and 751, matching the working
grid_gap spelling already at lines 139/498. Option B (drop the dead config): delete \"gap\": \"6px\", from lines 465 and 747 and \"gap\": \"10px\", from line 751, and additionally apply
finding 6's spec edit so the spec no longer references gaps. Either way the change should be made by
editing the JSON source lines surgically (per design.md's own risk note about notebook
reserialization).

2. The 300 px threshold is derived as 300 - 228 = 72 px of track, but flex-basis is a border-box size here, so the real reserve at the wrap point is ~46 px.

openspec/changes/fix-colab-slider-responsive/design.md:23 · medium · correctness

ipywidgets ships .jupyter-widgets { box-sizing: border-box; } (verified in the installed bundle:
.venv/share/jupyter/labextensions/@jupyter-widgets/jupyterlab-manager/static/lib_index_js.*.js), and
every widget root element carries that class. So flex: 1 1 300px on the card sets its BORDER box
to 300 px; the card's own padding: 6px 10px (20 px) and 1px solid #666 border (2 px) come out of
it, leaving 278 px of content. The slider inside has inline width: 100% plus the default
.jupyter-widgets { margin: 2px }, so it renders 278 px wide starting 2 px in and overflows the
card by 4 px (clipped by overflow: hidden). Concrete scenario: Colab content area at the new wrap
threshold (3 visible cards = 3 x (300 + 4 margin) = 912 px), UMAP + PaCMAP selected so groups 1-3
are visible -> each n_neighbors/mn_ratio slider gets 278 - 2 - 228 = ~48 px of track for a 2..500
range (~10 values per rendered pixel), not the ~72 px the design claims, and the right edge of the
readout is clipped. The spec's normative "at least 300 px" inherits the same arithmetic error, so
the requirement is ~30-45% tighter than the rationale it is justified by.

Suggested fix

Minimal doc-only correction, in openspec/changes/fix-colab-slider-responsive/design.md decision 1:
replace 'so a 300 px basis preserves roughly 72 px of track before the flex container wraps' with
'the basis is a border-box size (ipywidgets sets box-sizing: border-box on every widget root), so
a 300 px basis leaves 278 px of card content and about 48 px of track at the wrap threshold after
the card's 20 px padding, 2 px border, and the slider's 4 px margins.' If ~72 px of real track is
actually wanted, instead change _g in apps/protspace/notebooks/ProtSpace_Preparation.ipynb:449 to
"flex": "1 1 326px" and raise MIN_PARAMETER_GROUP_BASIS_PX in
apps/protspace/tests/test_notebook_layout.py:8 and the '300 px' figures in specs/colab-preparation-
controls/spec.md to 326 — that is a product decision, not a mechanical edit.

2 further finding(s) were raised and refuted during verification.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Follow-up to #409 (comment), assessed against final head ad37ede1b8f148a2aa8a853ecd976f0c5b9ef059:

  1. Residual shrink below 300 px — actionable, fixed. Each parameter group now sets min_width: "300px" alongside flex: "1 1 300px". The regression executes the real Generate cell and requires every group's minimum width to be at least its parsed flex basis. This preserves the usable reserve for a lone card; the parent ipywidgets HBox already supplies overflow: auto for sub-300px panes.
  2. CSS/design rationale — partially actionable, clarified. The design now says no new CSS is added and explains the existing parent scrolling behavior. The flex-basis/min-width change remains smaller than modifying the notebook's separate per-cell style block.
  3. Unrecognized gap traits — valid but pre-existing and out of scope for [BUG] Colab sliders in preparation notebook disappearing on certain scaling #346. Git blame places all three gap entries in the March notebook code, and this PR does not modify those layouts. The spec's earlier gap wording was already removed at e0ddb248. The focused test suppresses only the current deprecation warnings; a future TypeError would still fail cell execution, so the filter cannot mask the hard break. No unrelated three-layout rewrite was added here.
  4. Regression guard/chrome relationship — partly already addressed, partly duplicate. Exact wrap-token coverage was added at 386e15e9, so the stated missing-wrap assertion is outdated. This follow-up adds the missing minimum-width invariant. The 300 px basis remains the explicit OpenSpec contract; inferring browser chrome from a few Python style literals would omit widget/browser CSS and duplicate the rendered-geometry check.
  5. PyPI release effect — valid merge-time consideration, no branch change. The release workflow includes apps/protspace/**, but the review itself identifies this as a squash-title/merge decision. I did not change PR metadata or rewrite the branch.
  6. 48 px arithmetic — not supported by the recorded measurements. The 228 px figure is the observed outer-card-to-track delta (for example, 345 − 117 and 262 − 34), so subtracting card padding and border from it again double-counts those dimensions. The design now states that empirical relationship explicitly.

Verification on this exact head:

  • RED: the new minimum-width regression failed with group.layout.min_width == None.
  • GREEN: pytest apps/protspace/tests/test_notebook_layout.py -q → 1 passed.
  • ruff check / ruff format --check for the focused test → clean.
  • jq empty apps/protspace/notebooks/ProtSpace_Preparation.ipynb → clean.
  • openspec validate fix-colab-slider-responsive --strict → valid.
  • Staged pnpm precommit and the commit hook's independent rerun → passed.

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.

[BUG] Colab sliders in preparation notebook disappearing on certain scaling

2 participants