Skip to content

fix(ui): keep threads visible at narrow widths - #246

Closed
mkozjak wants to merge 2 commits into
gammons:mainfrom
mkozjak:fix/issue-244-thread-open
Closed

mkozjak wants to merge 2 commits into
gammons:mainfrom
mkozjak:fix/issue-244-thread-open

Conversation

@mkozjak

@mkozjak mkozjak commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #223 and fixes #244.

Pressing Enter on a selected message previously failed to open its thread at common terminal widths. The layout auto-hidden the thread pane when the proportional 35% width calculated below the 30-column minimum, even when there was enough total space for both panes.

This change clamps the thread pane to its minimum width when possible, keeps the thread status consistent when auto-hide is unavoidable, and adds regression coverage for the layout boundary and Enter flow.

@gammons

gammons commented Sep 24, 2026

Copy link
Copy Markdown
Owner

The production fix is correct — I verified it independently and it does exactly what #223 and #244 need. The problem is one of the two new tests, which fails and is failing for a reason worth understanding.

The layout change is right. I swept Compute across widths 20–200 on main and on this branch. Behaviour is identical everywhere except the band the fix targets:

width main this PR
111 auto-hidden, msg=71 unchanged
112 auto-hidden, msg=72 thread visible, msg=40 / thread=30
113–123 auto-hidden thread visible, thread clamped to 30
124 thread visible (35% naturally ≥ 30) identical — reconverges
200 msg=102 / thread=56 identical

And the guard makes the trade impossible in the bad direction: it requires paneContentWidth >= minMsgWidth + minThreadW, so maxThreadWidth >= 30 always and the later cap can never push the message pane under its own 40-column minimum. No pane is broken to un-break the other.

I also drove a real tea.WindowSizeMsg plus tea.KeyPressMsg{Code: tea.KeyEnter} through a.Update with the sidebar in its normal visible state:

width=120  threadVisible=true   focusedPanel=PanelThread   statusbar shows "> Thread"
width=108  threadVisible=false  focusedPanel=PanelMessages  no stale "Thread"
width=90   threadVisible=false  focusedPanel=PanelMessages  no stale "Thread"

So Enter genuinely opens the thread at the default 120 columns now, and the one-line a.statusbar.SetInThread(false) in app.go correctly stops the stale indicator when auto-hide is unavoidable. Credit for using a.Update in the new tests rather than runKeyCases — that's the real reducer chain, which is what this bug needed.

The golden re-bless is clean. I checked it specifically, because a re-blessed golden is where a real regression hides. The old thread_open scenario used w: 140 with a comment admitting it had to dodge the very bug being fixed ("at 120 the thread pane AUTO-HIDES"). This PR moves it to the real default of 120 and drops goldenThreadMinWidth 124 → 112. I re-rendered both, confirmed the widths are genuinely 140 and 120, and the only differences are the expected reflow. go test ./internal/ui -run TestGolden -update then produces no further diff, so the committed file is current, not stale.

What's blocking: TestNormalModeEnterKeepsThreadOpenAtDefaultWidth fails.

--- FAIL: TestNormalModeEnterKeepsThreadOpenAtDefaultWidth/108
    mode_normal_keys_test.go:2182: threadVisible = false after rendering at 108 columns
--- FAIL: TestNormalModeEnterKeepsThreadOpenAtDefaultWidth/80
    mode_normal_keys_test.go:2182: threadVisible = false after rendering at 80 columns

Deterministic, at the PR head and after merging current main, and CI agrees — test is red on this PR.

The root cause is the assertion at line 2209, a.sidebarVisible != (width >= 112). There is no sidebar auto-collapse in slk. a.sidebarVisible initialises true at app.go:759 and is only ever flipped by the explicit ToggleSidebar handler; case tea.WindowSizeMsg sets a.width, a.height and forceSixelRepaint, nothing else. With the sidebar genuinely still visible at 80 and 108 — as it is in production — there isn't 70 columns of content left after a 6-column rail and a 32-column sidebar, so Compute correctly auto-hides, and the test's own premise fails.

Your sibling test proves the point: TestNormalModeEnterAutoHideClearsThreadStatus at width 79 expects sidebarVisible = true and the thread auto-hidden. The two tests disagree with each other about how the app works.

So this is a defect in the test's mental model, not in panellayout.go. The fix is small — keep 120 and 112 as "thread stays open", and either drop 80/108 or assert the auto-hide case for them, mirroring the sibling test.

Two minor things while you're in there:

  • withSize → withWindowSize at mode_normal_keys_test.go:2155 and :2231. AGENTS.md calls these out as not interchangeable: only withWindowSize goes through the real tea.WindowSizeMsg path. It doesn't change the result today — I checked, because sub-model dimensions come from frame.Compute at render time — but these two tests specifically claim to exercise the production Enter path, and WindowSizeMsg already carries sixel-repaint behaviour that will grow.
  • One mutation survives: loosening the clamp to threadWidth < minThreadW-1 is caught by nothing. The new layout test's widths never land on 121, the one width where the natural 35% split is 29 and the off-by-one would show. Dropping the clamp entirely is caught, broadly. A width-121 case closes it.

Marking changes requested purely for the red CI — the repo's own pre-PR bar in AGENTS.md is go test ./... -race green. The algorithm, the golden and the design doc are all in good shape, and I expect this to be ready to merge once that one test is corrected.

(Also worth cross-referencing #244 in the PR description — the design doc names it, the description names #223, and both are legitimately fixed here.)

@gammons gammons added the changes requested Blocking issues found in review label Sep 24, 2026
@gammons

gammons commented Sep 25, 2026

Copy link
Copy Markdown
Owner

going to close this in favor of #255.

@mkozjak mkozjak closed this Sep 25, 2026
@mkozjak
mkozjak deleted the fix/issue-244-thread-open branch September 25, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Blocking issues found in review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enter doesn't create a thread anymore Bug: Cannot reply to a thread in narrow terminals

2 participants