fix(event-log): preserve RichTextLabel scroll across event_rev bumps - #382
Conversation
_render_event_log wiped the sidebar event log with clear() and rebuilt it via append_text() on every event_rev bump, resetting the player's scroll position so a new event (build finishing, worker break, ambient event, recruit click) snapped an already-scrolling log back to the top. Capture the v-scroll bar value before clear() and write it back after the append. The restore is deferred to the next idle frame: with scroll-following on, the layout pass that follows the rebuild snaps the bar back to the newest line, so the value must be re-applied after that pass. Add tests/test_event_log_scroll.gd, a regression test that drives the real _render_event_log() against a scene-free main.gd with a real RichTextLabel and asserts the scroll position survives an event_rev bump. It fails without the fix (scroll snaps to the newest line) and passes with it. Fixes #380 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — primary route
Recommendation: Approve
This PR correctly fixes issue PR 380 with a minimal, targeted change.
Change-by-Change Findings
scripts/main.gd — Scroll preservation in _render_event_log
The fix captures VScrollBar.value before clear() and restores it via call_deferred("set_value", saved_scroll) after rebuilding the log. The deferred restore is necessary because with scroll_following on, the layout pass that follows append_text would snap the bar back to the newest line — the value must be applied after that pass.
- The null-check guard on
scroll_bar != nullis defensive and correct. - The inline comment explains the deferred restore rationale clearly.
- No side effects; the event_rev cache gate remains intact.
tests/test_event_log_scroll.gd — New regression suite
Three test cases cover the acceptance criteria from issue PR 380:
test_scroll_survives_event_rev_bump— single bump, scroll held at arbitrary target, also asserts the event list is bounded atMAX_EVENT_LOG(20) and newest entry is first.test_scroll_preserved_across_multiple_bumps— five rapid bumps in a loop, scroll preserved throughout.test_idle_render_does_not_touch_scroll— confirms early-return path (noevent_revchange) never touches the scroll bar.
All assert_eq calls use the required three-argument form (actual, expected, name), matching the AGENTS.md testing trap warning.
Standards Compliance
No issues. The fix:
- Touches only
scripts/main.gdas specified in the issue's expected files. - Uses the new test file approach suggested by the issue.
- Follows GDScript conventions consistent with the surrounding codebase.
- Passes all CI checks (Linux, macOS, Windows export validation, headless smoke test, script test suite).
Linked Issue Fit
Issue PR 380 asked for preserving RichTextLabel scroll position across event_rev bumps. The PR implements the second acceptance criterion exactly: it reads get_v_scroll_bar().value before clear() and writes it back after append_text. The deferred restore is necessary to avoid the scroll_following layout-pass snap-back described in both the test comments and the issue evidence. Regression test coverage is added as required.
Tool Harness Findings
No tool harness output present in the corpus; reviewing the diff directly.
Unknowns / Needs Verification
None. The fix is self-contained, CI passes, and the test suite exercises the exact scenario from the issue.
2 participants
Preserve RichTextLabel scroll position in the event log by capturing the VScrollBar value before rebuilding the text.
Fixes #380
Opened by foreman on review GO (workload wl-misospace-windowstead-380).