Skip to content

fix(sidebar): collapse when resizing below minimum width - #1741

Merged
kshivang merged 2 commits into
mainfrom
fix/sidebar-resize-collapse
Sep 27, 2026
Merged

kshivang merged 2 commits into
mainfrom
fix/sidebar-resize-collapse

Conversation

@kshivang

Copy link
Copy Markdown
Contributor

Description

Dragging the vertical sidebar inward previously stopped at 120 dp, so the drag could never collapse it. Match BossTerm's resize behavior: allow a live preview down to 44 dp, then collapse on release below BossConsole's existing 120 dp expanded minimum.

Collapsing preserves the previous expanded width for reopening. Releasing at or above the minimum saves the resized width, and cancelling a drag discards its preview without changing settings. The existing maximum and settings-slider range remain unchanged.

This follows the released native title-bar work in #1736. It changes the shared vertical-sidebar resize interaction; platform title-bar gates remain unchanged.

Type and version impact

  • Bug fix; patch impact, version managed by release automation

Validation

  • Regression coverage for collapse threshold, restored width, reversing the drag, and maximum width.
  • Focused regression run: 19 tests passed, including existing resize and sidebar-reveal coverage.
  • Desktop compilation, ktlint and detekt checked locally.
  • Interactive drag behavior has not been manually verified in the running app.

Review focus

The collapse decision uses the requested width before expanded-width clamping. Settings are written only on release; cancellation clears local preview state.

@supabase

supabase Bot commented Sep 26, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project pcnwqamqdnsadranufjv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@github-actions

Copy link
Copy Markdown
Contributor

Claude diff review of b56b525 (no tests executed).

Review: PR #1741 (b56b525) — sidebar drag-to-collapse

Diff-only review; I have not inspected surrounding code, other call sites, or run tests. Claims about code outside the diff are explicitly marked uncertain.


Confirmed defects / high-value findings

1. sidebarResizeResult can collapse but never un-collapses — asymmetric state transition
SidebarResize.kt:11-19:

if (requestedWidth < TabBarVerticalWidthRange.start) settings.copy(tabBarCollapsed = true)
else settings.copy(tabBarVerticalWidth = clampBarWidth(requestedWidth))

The else branch never clears tabBarCollapsed. If this function is ever called with settings.tabBarCollapsed == true (e.g. a drag/commit that originates from the hidden-sidebar hover edge — HiddenSidebarHoverEdge is imported in SplitView.kt:15, or a rail/peek state where the handle is still composed), the width is persisted but the sidebar stays collapsed, silently discarding the user's drag. The fix is one token: settings.copy(tabBarVerticalWidth = clampBarWidth(requestedWidth), tabBarCollapsed = false).
Reachability depends on whether the handle can be dragged while collapsed (enabled = !bar.railShown at SplitView.kt:2838 is the only gate visible in the diff), so reachability is uncertain, but the asymmetry itself is a defect in the function's contract and is untested (see #6).

2. onCommit now receives an unclamped value — contract change for every call site
VerticalTabBarResizeHandle.kt:72: onDragEnd = { latestCommit(startWidth + accumulated.toDp().value) } (previously clampBarWidth(...)). Clamping now lives only in sidebarResizeResult. The single call site shown (SplitView.kt:2843-2848) routes through it correctly, but any other caller of VerticalTabBarResizeHandle will now persist arbitrary (including negative) widths without a compiler error, since the signature is unchanged. Adding the required onCancel param (VerticalTabBarResizeHandle.kt:43) will force other call sites to be touched, so they'd be noticed — but the silent loss of clamping in onCommit is the risk. Please confirm this is the only call site, and consider documenting on the onCommit param that the value is raw/unclamped.

3. Drag-cancel now discards the resize instead of committing it — behavioural regression
VerticalTabBarResizeHandle.kt:73 changes onDragCancel from committing the accumulated width to latestCancel(), and SplitView.kt:2841 maps that to draggedWidth = null. detectDragGestures fires onDragCancel not only for user cancellation but also when the pointer stream is interrupted (e.g. loss of pointer capture / window focus while dragging outside the window, on some desktop backends). Previously such an interruption preserved the resize; now it reverts to the persisted width. This may be intentional ("reverse before release"), but it is a user-visible behaviour change that is not covered by any test in the diff, and the cancel path is the one most likely to be hit accidentally.


Uncertain observations / nits

4. Magic 44f floor, decoupled from the range and potentially crash-prone
SidebarResize.kt:7: width.coerceIn(44f, TabBarVerticalWidthRange.endInclusive). Two issues:

  • Float.coerceIn(min, max) throws IllegalArgumentException when min > max. If TabBarVerticalWidthRange.endInclusive were ever configured below 44f, every drag frame throws inside a pointer-input handler. Cheap defensive fix: derive the floor as minOf(44f, TabBarVerticalWidthRange.endInclusive) or assert the invariant.
  • The preview floor (44f) and the collapse threshold (TabBarVerticalWidthRange.start, SidebarResize.kt:15) are independent constants. If start < 44f, the collapse threshold becomes unreachable via preview feedback (the user sees the bar stop at 44 but release still commits the raw pointer value, so collapse can still trigger from an off-screen drag with no visual cue). Extract 44f as a named constant next to TabBarVerticalWidthRange so the invariant 44f < start is explicit.

5. Possible one-frame snap-back on collapse
SplitView.kt:2842-2848: draggedWidth = null is set synchronously while the settings write happens in barWidthScope.launch { ... }. Between those, the bar renders at the persisted (still expanded) width before collapsing. Whether this is visible depends on how draggedWidth is consumed downstream (not in the diff).

6. Package-layering inversion
SidebarResize.kt:3 imports clampBarWidth from ...window_panel.components.main_window_panels, while VerticalTabBarResizeHandle.kt:3 imports sidebarResizePreview from ...components.sidebar. The two packages now depend on each other. Consider moving both helpers (and the range/floor constants) into one place — ai.rever.boss.window already owns TabBarVerticalWidthRange.

7. NaN passthrough (theoretical)
SidebarResize.kt:15 — NaN < start is false, and coerceIn returns NaN unchanged, so a NaN drag delta would persist tabBarVerticalWidth = NaN. Only reachable via a degenerate pointer event; mentioning for completeness.


Test gaps (SidebarResizeTest.kt)

The added tests cover sidebarResizeResult/sidebarResizePreview pure-function behaviour reasonably (boundary at start, clamp at endInclusive, field preservation via assertEquals(settings, result.copy(tabBarCollapsed = false)) at line 21). Missing:

  • Collapsed → expand (finding ⬆️ actions:(deps): Bump supabase/setup-cli from 2 to 3 #1): no test asserts sidebarResizeResult(settings.copy(tabBarCollapsed = true), 300f).tabBarCollapsed == false. Adding it would have caught the asymmetry.
  • Cancel wiring (finding ⬆️ actions:(deps): Bump actions/setup-go from 6 to 7 #3): nothing exercises onDragCancel → onCancel, nor that onCommit is not invoked on cancel. The SplitView wiring (onCancel = { draggedWidth = null }) and the onDragEnd unclamped handoff are entirely untested.
  • Unclamped commit (finding ⬆️ actions:(deps): Bump actions/setup-java from 4 to 5 #2): no test for sidebarResizeResult(settings, -500f) / very large values arriving from the now-unclamped onDragEnd.
  • SidebarResizeTest.kt:31-32 hardcodes 44f/80f, duplicating the magic number; if the floor changes the assertion assertEquals(80f, sidebarResizePreview(80f)) silently stops testing what it intends (it passes for any floor ≤ 80).

Security

Nothing security-relevant: pure numeric/state transformation, no I/O, no untrusted input, no serialization boundary introduced in this diff.

@kshivang
kshivang merged commit a6b2d80 into main Sep 27, 2026
10 checks 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.

1 participant