fix(ui): preserve scroll position in messages.Model on resize; thread.Model fix partial

[laraibg786]

Root cause

Both messages.Model and thread.Model store scroll position as a raw absolute line count into a fully flattened, word-wrapped render cache (yOffset in the messages pane, vp.YOffset() in the thread pane). On any pane-width change, that cache is fully rebuilt at the new width — every message/reply re-wraps, changing every entry's line height and therefore what any given line number points at. The old raw line count was reused verbatim afterward (only clamped into the new valid range), so after a resize the same number pointed at unrelated content. That's the jump.

Approach

Added entryAndOffsetForLine(line) to both models: given a flattened line number, it returns which cache entry contains it and the offset within that entry. Using this:

• Before a width-driven cache rebuild, the entry + within-entry offset the viewport is currently anchored on is captured.
• After the rebuild, the scroll offset is re-derived from that same entry's new position, instead of reusing the stale raw line count.
• If the viewport was pinned to the bottom before the resize, it is re-pinned to the new bottom instead of using the entry anchor — matching the bottom-anchor invariant already maintained elsewhere in these files (e.g. on new messages arriving, or reaction/reply-count updates).
• Thread-pane specific: entryOffsets there covers replies only — the parent message block is a content prefix, not a cache entry — so entryAndOffsetForLine returns an explicit ok bool for "this line is inside the parent prefix" vs. "entry 0". This PR handles the bottom-anchored and entry-anchored cases only; anchoring while scrolled inside the parent prefix itself is a known gap, scoped out of this PR — see "Known limitation" below.
• The restored within-entry offset is clamped to the anchored entry's new height, since widening a pane shrinks entry heights; without the clamp, an offset that was valid at the old (taller) width could overshoot into a later entry after rewrap.

Known limitation (scoped out, tracked separately)

When the thread-pane viewport is scrolled to a line inside the parent-message prefix (not yet showing any reply), anchorOK is false and the resize-anchor switch has no case for it — the pre-resize YOffset carries through unchanged, which can land on an unrelated reply after a large enough width change. This has been postponed to fix in another PR.

Testing

Added regression tests in internal/ui/messages/model_test.go and internal/ui/thread/model_test.go covering:

• Narrowing the pane while scrolled mid-list.
• Widening the pane while scrolled mid-list (the entry-height-shrink edge case).
• A bottom-anchored viewport staying pinned to the bottom across a resize.
• (Thread only) A viewport at YOffset == 0 staying at 0 across a resize.
• (Thread only) The known limitation above, as a characterization test.

Each test asserts against the actual rendered view output (via a unique marker embedded in each message/reply) rather than against the internal helper the fix introduces, so a bug inside that helper can't cancel itself out between the "before" and "after" measurements.

Every test was confirmed non-vacuous by temporarily reverting its corresponding piece of the fix and observing the test fail with the expected mismatch, then restoring the fix and confirming it passes again — with one exception, caught in review: the YOffset == 0 test above cannot distinguish fixed from broken, since 0 clamps to 0 either way. Its doc comment now says so plainly, and the new characterization test covers the case it was mistakenly thought to cover.

closes #238

[laraibg786]

@gammons Please review this and let me know if there are any blockers in merging this. I would be happy to update.

[gammons]

Thanks for the patience on the review, and sorry it sat. The diagnosis here is exactly right and unusually well written up — "a raw line count into a cache that gets rebuilt at a new width" is the bug, the entry+offset anchor is the correct fix, and the four-way test split (narrow / widen / bottom-pinned / top) is the right shape.

Verified on your branch merged with current main (clean merge):

check	result
go build ./... / go vet ./... / gofmt -l .	pass / pass / empty
golangci-lint run	0 issues
go test ./... -race -count=1	pass
go test ./internal/ui/thread/ -run Lockstep	pass, including the TestLockstep_ReactionHitTestFrames tripwire

The helper itself is byte-identical between the two models, and the bottom-pin and height-clamp branches are genuinely parallel. That's the part AGENTS.md cares most about and you got it right.

But there's a case of the same bug still in the thread pane, and it's the one your ok bool is described as handling.

thread/model.go:1785-1799 is a two-case switch: bottom-anchored, or anchor-restore. When anchorOK is false — the viewport is inside the parent message, which is a content prefix rather than a cache entry — neither arm fires, so the raw pre-rewrap YOffset is carried through untouched. Reproduced on your branch with a long parent (300 words, wrapping to 81 lines at width 30):

BEFORE (width 30, yOffset=40): inside the parent, no reply visible
AFTER  (width 200):            yOffset STILL 40  ->  now showing REPLYMARK007

The parent shrinks to 9 lines on widening, line 40 is now deep in the reply list, and the viewport lands on reply 7. That is precisely the "same number, unrelated content" symptom this PR closes, one region of the pane earlier. Long opening messages are ordinary, so this is reachable in normal use.

The PR description frames the ok bool as deliberately distinguishing "inside the parent prefix" from "entry 0", which reads as though the parent case is handled. It's only correct by coincidence at YOffset == 0, because 0 clamps to 0 under both the old and new code.

Which brings me to the test. TestScrollAnchorAtTopOfThreadPreservedAcrossWidthResize passes with the entire fix reverted — I stubbed entryAndOffsetForLine to always return ok=false and it still went green. It anchors at offset 0, the one offset that cannot distinguish fixed from broken. So the only test covering the parent-prefix path has no teeth, and it's guarding the path that's actually broken.

That matters more than the usual "please add a test", because of this in the description:

Every test was confirmed non-vacuous by temporarily reverting its corresponding piece of the fix and observing the test fail with the expected mismatch.

That's the right methodology — it's what I'd ask for — but it isn't true for this one, and the gap it left is where the remaining bug is. Worth checking how that slipped: if the revert was of the whole fix rather than the parent-prefix branch specifically, a 0-offset test would still look like it failed for the right reason in a batch run.

Two smaller things from the same audit, both uncovered:

• oldEntryCount == len(m.replies) (thread, line 1789) — delete it, zero tests fail.
• m.cacheMsgLen == len(m.messages) in messages' widthChanged (messages/model.go:3002) — delete it, zero tests fail.

Both are guards against a count change racing a width change. Not bugs today; just deletable without signal.

One divergence worth recording even though I can't reach it. The gate that decides whether any of this runs is not parallel between the models:

model	gate
messages/model.go:3002	m.cache != nil && m.cacheWidth != width && ... — the render cache's own fields
thread/model.go:1482	m.viewCacheValid && m.viewWidth != width — a second-level flag

viewCacheValid is invalidated by ClickAt, PatchUserName, SetChannelNames, SetUnreadBoundary and SetFocused — and SetFocused runs every frame from view_thread.go:41. If a focus flip and a width change land in the same cycle with no render between, widthChanged reads false and the anchor logic is skipped entirely. I could not find a path through the current App loop that does that — it renders once per message — so this is not exploitable today. But messages.Model's SetFocused deliberately avoids touching its cache and documents why; the thread side doesn't. It's exactly the class of scroll-math divergence AGENTS.md warns the lockstep test cannot see, and Phase 3's pane hooks will walk straight into it. A comment on thread/model.go:1482 explaining the asymmetry would be enough.

Marking changes requested, on the strength of the parent-prefix case alone — it's the same bug class the PR closes, it's reproducible, and the test that should cover it is vacuous. Either extend the anchorOK == false branch to anchor on the parent's own re-wrapped height, or narrow the PR's scope and document the parent region as a known limitation. Either way, please re-point that test at a non-zero mid-parent offset; it would have caught this.

The rest of the work is good and I'd like to take it.

[laraibg786]

Thanks for the thorough review, and sorry for overlooking the parent-prefix case. You're right it's the same bug class, one region earlier. Filed #254 to track it with a repro, rather than fixing it here to keep this PR manageable.

Also fixed the vacuous top-of-thread test — it only proved the YOffset == 0 boundary — and added a characterization test that actually pins the gap until fix for that issue lands. PR description and title updated to match.

If you have any further comments or suggestion. I would be happy to address.

[gammons]

Thanks for filing #254 and for re-scoping the description; that's a clean way to handle the parent-prefix case, and I'm happy for it to land separately.

The test changes you describe haven't reached the PR, though. The branch is still at 13ba8e7 from 09-21, and TestScrollAnchorAtTopOfThreadPreservedAcrossWidthResize is unchanged at internal/ui/thread/model_test.go:1022. There's also no characterization test for the gap. None of your fork's branches have moved since 09-23, so I think the commit is sitting unpushed locally. Once it's pushed I'll re-review. With #254 split out, those two tests are the main thing left.

[laraibg786]

My bad, I forgot to push the changes to remote. Thanks for noticing. I have updated the PR and I think it should be good to review this time.

[gammons]

Got it, thanks. bcb00a7 is on the branch and it resolves the review.

• The parent-prefix case is now scoped out cleanly on three fronts: Bug: thread pane scroll position not preserved on resize when anchored inside the parent message #254 tracks it, the widthChanged block in thread/model.go has a comment naming the gap, and the description says so. That's the right way to leave a known limitation in the code.
• TestScrollAnchorInsideParentPrefixNotPreservedAcrossWidthResize is a good characterization test. It pins today's behaviour at a non-zero offset inside the parent, and its failure message tells whoever fixes Bug: thread pane scroll position not preserved on resize when anchored inside the parent message #254 to turn it into a real preservation test. I checked it on current code: it reproduces the jump, so it genuinely pins the gap.
• The vacuous test now honestly documents that it only covers the YOffset == 0 boundary. That's a fair resolution: it no longer claims coverage it doesn't provide.

Merged with current main, which includes #255's changes to thread/model.go, it merges cleanly. Build, vet, gofmt and go test ./internal/ui/... -race all pass, as do both lockstep tests and all five scroll-anchor tests.

Two optional items from last round, not blocking:

• Two guards are still untested: oldEntryCount == len(m.replies) in thread/model.go and m.cacheMsgLen == len(m.messages) in the messages/model.go widthChanged check. Either can be deleted without a test failing.
• The widthChanged gates differ between the two models. The thread pane keys off viewCacheValid, which SetFocused resets, while the messages pane keys off the render cache itself. It isn't reachable today, but a one-line comment at the thread gate would save Phase 3 from rediscovering it.

Happy to take those in a follow-up. Marking ready to merge.