feat: add compact event log drawer + HUD row (issues #139, #146, #147, #148)#174
Conversation
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
Recommendation: Request Changes
The PR delivers most of what issue #139 asks for, but it is materially misrepresented by its title and body, and the scene changes appear to be missing nodes that the script references. Please reconcile the description with the diff and verify the event-drawer scene nodes exist before merge.
Change-by-change findings
scripts/main.gd
Goal completion → log (matches PR body and issue acceptance criterion):
if not active_goal.is_empty() and RotatingGoal.is_goal_complete(active_goal):
var goal_id = String(active_goal.get("id", "unknown"))
var new_goal = RotatingGoal.rotate_after_completion(active_goal, completed_goal_ids)
completed_goal_ids.append(active_goal["id"])
active_goal = new_goal
push_event("Goal completed: %s. The colony moves on." % goal_id)Good — goal_id is captured before active_goal is reassigned, so the message will carry the correct id. This satisfies the "Goal completion from #131/#134 can write to the log" acceptance criterion.
render_event_drawer() and toggle_event_drawer(): These are the core of the event-drawer feature asked for in the issue. The collapsed label shows "Last: " + latest_text and the expanded log shows the last 6 entries, which lines up with the suggested collapsed/expanded contents. The event_drawer_label.gui_input toggle wiring in wire_controls() is consistent with the existing click-to-toggle pattern used elsewhere in this file (e.g. the tile panel handler around line 608).
render_hud_row() and the new HUD @onready vars: ~50 lines of new HUD rendering code (worker cap n/m, food warning states, active goal progress) are added but not mentioned anywhere in the PR body or title. The title says "goal completion events to log + bounded log tests", but the diff also ships an entire HUD row renderer. This is the bulk of the +100 additions to main.gd and should either be its own PR or explicitly called out in the description.
var event_drawer_visible := false: Fine as a module-level flag, though note event_drawer_panel.visible is touched in toggle_event_drawer() without an is_instance_valid guard. If the EventDrawerPanel node does not exist in scenes/main.tscn, this line will error on the first toggle. The other two new event-drawer references are guarded.
scenes/main.tscn
The patch adds HudWorkerCap, HudFoodWarning, and HudGoalLabel nodes — again, not mentioned in the PR body, and unrelated to the event log feature.
More importantly, the script now references %EventDrawerLabel, %EventDrawerPanel, and %EventDrawerLog via @onready, but the diff does not show those nodes being added to scenes/main.tscn. The unique-name % resolution in @onready happens at scene load; missing nodes will produce a runtime error like Node not found: EventDrawerLabel and the @onready vars will be null. Please confirm those three nodes exist in the tscn (likely the diff is just truncated and they were added elsewhere) and that event_drawer_panel is reachable from the scene root, otherwise toggle_event_drawer() will crash.
tests/test_runner.gd
test_bounded_event_log is a reasonable direct test of the push_event contract: it pushes 12, asserts cap of 8, asserts LIFO order (newest at index 0, oldest kept at index 7 is tick 4), and asserts eviction count. This satisfies the issue's "Add tests for bounded log behavior if implemented as pure logic" criterion.
One small note: the test uses a local events array rather than calling the real push_event() on the main script, so it does not catch a regression where someone changes the cap or the eviction direction in push_event itself. A second tiny test that drives the real main.push_event() a few times would close that loop.
Sources
- Issue #139 acceptance criteria (bounded list, latest-event-when-collapsed, expanded recent history, goal completion writes to log, tests for bounded log).
scripts/main.gdlines 1267–1274 (goal completion push_event), 2233–2266 (toggle/render event drawer), 1656–1710 (render_hud_row, not described in PR).tests/test_runner.gdlines 27 (registration) and 380–410 (test body).scenes/main.tscnpatch adds only HudWorkerCap / HudFoodWarning / HudGoalLabel; no EventDrawer* nodes appear in the diff.
Standards Compliance
- The bounded list is "pure logic" and the PR adds a test for it, consistent with AGENTS.md's validation guidance and CONTRIBUTING.md's smoke-test-before-pushing rule.
- The implementation is consistent with the "glanceable, low-attention" desktop-companion feel the issue explicitly asks for: collapsed by default, compact when opened, no full journal.
- The non-described HUD row additions are tangential to the stated PR goal and should either be split out or added to the description so reviewers can sign off on them deliberately.
Linked Issue Fit
| Acceptance criterion | Met? |
|---|---|
| Recent events stored in small bounded list | Yes (push_event, max 8) |
| UI shows latest event when collapsed | Yes (EventDrawerLabel "Last: …") |
| Expanded drawer shows short recent history | Yes (last 6, tick-prefixed) |
| Goal completion writes to log | Yes |
| Worker cap/upkeep events from #133 | Not in this PR — issue allows "later", acceptable |
| Tests for bounded log behavior | Yes |
Unknowns / Needs Verification
- Do
EventDrawerLabel,EventDrawerPanel, andEventDrawerLognodes exist inscenes/main.tscn? The provided diff does not show them being added; if they are missing, the game will error on scene load or on the first drawer toggle. Please add the missing nodes or confirm they are present. - Are the HudWorkerCap / HudFoodWarning / HudGoalLabel additions intentional for this PR? They duplicate work from commit
b23a82d feat: show worker cap, upkeep warning, and active goal in compact HUD rowalready on main. If they are needed because that commit never landed inscenes/main.tscn, that should be stated in the PR body. - Why are the
render_hud_row()andrender_event_drawer()changes bundled into a "goal completion events" PR? Consider splitting into two PRs for cleaner review and history.
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
Recommendation: Approve
This PR implements the event log drawer feature for issue #139 cleanly. It adds the UI scaffolding, hooks goal completion into push_event, and provides a focused unit test for the bounded log eviction behavior. The work is internally consistent, follows repository conventions, and satisfies the issue's stated acceptance criteria.
Change-by-change findings
scenes/main.tscn (+72/-1)
- Bumps
load_stepsfrom 2 to 5 to reflect the new nodes. Good. - Adds three HUD row labels (
HudWorkerCap,HudFoodWarning,HudGoalLabel) underBackdrop/Margin/Root/Left/HudRowwithunique_name_in_owner = true, matching the project'sunique_name_in_owneraccess pattern used elsewhere in the file. - Adds
EventDrawerLabel(collapsed state) andEventDrawerPanel(expanded state) at scene root withanchors_preset = 8(center bottom) andvisible = falsefor the panel, satisfying the "collapsed by default" requirement in issue #139. EventDrawerLoglives inside aMarginContainerso the text has consistent padding.- Minor: there is an extra blank line before
text = "Activity"in theActivityLabelblock — cosmetic only.
scripts/main.gd (+100)
- New
@onreadyvars for the HUD row and event drawer nodes; uses%accessors, consistent with existing code. - New
event_drawer_visible := falsestate var. wire_controls()connects agui_inputhandler onevent_drawer_labelthat callstoggle_event_drawer()on left click. UsesControl.MOUSE_FILTER_STOPso the label is clickable. Consistent with the existing tile-panelgui_inputpattern._on_tick()capturesgoal_idbeforerotate_after_completion()mutatesactive_goal, so the event text uses the completed goal's id. Then callspush_event("Goal completed: %s. The colony moves on." % goal_id). This satisfies "Goal completion from #131/#134 can write to the log."render_all()now callsrender_hud_row()andrender_event_drawer(). The HUD row renderscurrent/capworkers, a⚠ STARVING/⚠ LOW FOODwarning (using the existingget_low_food_level()helper), and a compact goal progress line. UsesmatchonRotatingGoal.GOAL_TYPE_*constants.toggle_event_drawer()flips visibility and callsrender_all(). Therender_all()call is unnecessary (only panel visibility changes) but is harmless and consistent with other toggles in the file (e.g.close_settings).render_event_drawer()readsstate.get("events", [])(safe on missing key) and shows up to 6 entries in the log witht%02d <text>formatting; falls back to"No events yet."when empty.- The new
cap()shadowing insiderender_hud_rowis local to the function and does not collide with the globalcap()helper, but it does overload the identifier. Worth a rename in a follow-up, but not blocking. - The new
render_hud_row()overlaps in spirit withrender_sidebar()/render_goal()for some fields (worker cap, goal text), which is a minor duplication. Out of scope for this PR since the issue specifically asks for a compact HUD row.
tests/test_runner.gd (+32)
- Registers
test_bounded_event_log(game_state)in_initialize(). Placement is logical (right aftertest_event_log). test_bounded_event_log()verifies:- 12 pushes produce a cap of 8.
- LIFO order is preserved:
events[0].tick == 11(newest),events[7].tick == 4(oldest kept). - Eviction count math (12 - 8 = 4).
- Empty log stays empty.
- Single event fits.
- Caveat: the test simulates the bounded logic inline rather than calling the real
push_event(). This is acceptable per the issue ("Add tests for bounded log behavior if implemented as pure logic"), and the test mirrors the exactpush_eventbody, so it is faithful. A stronger test would call the production function, but that is not required.
Sources
- PR diff and metadata (this PR is the primary source).
- Linked issue #139 "ux: add compact event log drawer".
- Repository impact scan for
event,events,push_event,bounded,goal,completionconfirms existingpush_eventinfrastructure andstate.eventspersistence are reused, not duplicated. - Repository history shows prior work on goal rotation (
#145,#142) and the rotating goal model provides theidandtypefields this PR reads.
Standards Compliance
- Branch name
saffron/issue-139-event-log-drawerdoes not matchfeat/short-descriptionfrom CONTRIBUTING.md, but the project has an establishedsaffron/issue-NN-*convention in recent history (e.g.,saffron/issue-146-worker-cap-calculation,saffron/issue-137-resource-trend-indicators); AGENTS.md also says "Push directly tomainunless told otherwise." Acceptable. - PR title uses
feat:prefix, conventional. - The PR is scoped: it touches only
main.tscn,main.gd, andtest_runner.gd, all of which are the correct files for this feature (the project is intentionally monolithic per CONTRIBUTING.md). - Headless test command (
./.tools/Godot_v4.2.2-stable_linux.x86_64 --headless --path . --script res://tests/test_runner.gd) will pick up the new test on the next run. - macOS GDScript-warning-as-error caveat from AGENTS.md: I cannot run the project to verify zero warnings, but the new GDScript is straightforward and uses typed locals (
var goal_id = ...,var current_workers := ..., etc.) consistent with the rest of the file.
Linked Issue Fit
Mapping to issue #139 acceptance criteria:
- Recent events are stored in a small bounded list. — Satisfied.
push_eventalready caps at 8 and is reused. - UI can show latest event while collapsed. — Satisfied.
EventDrawerLabelstarts visible withtext = "Last: —"and is updated byrender_event_drawer()to the latest event. - Expanded drawer shows a short recent history. — Satisfied.
EventDrawerLogshows up to 6 entries. - Goal completion from #131/#134 can write to the log. — Satisfied.
_on_tick()now callspush_eventwith the completed goal id. - Worker cap/upkeep events from #133 can write to the log later. — Not added in this PR, but the issue explicitly says "later". A scan shows
push_eventis already called from the worker-cap branch (main.gd:1088), so the wiring exists; expanding to more worker events is a separate follow-up. - Add tests for bounded log behavior if implemented as pure logic. — Satisfied by
test_bounded_event_log.
The collapsed-state label "Last: —" is also a reasonable default and matches the suggested collapsed state in the issue body.
Evidence Provider Findings
- No evidence providers configured for this run.
Tool Harness Findings
- Tool harness produced a planning parse warning (
Could not parse planning response as JSON) with zero planned requests. No actionable findings to evaluate; flagged here for transparency only.
Unknowns / Needs Verification
- I could not run the headless test suite from this environment, so I have not personally confirmed the new
test_bounded_event_logpasses alongside the rest of the suite. The test logic is straightforward and should pass; recommend running./.tools/Godot_v4.2.2-stable_linux.x86_64 --headless --path . --script res://tests/test_runner.gdbefore merge. - The
caplocal-scope name insiderender_hud_rowshadows the file-levelcap(text: String)helper within that function. Safe but a rename to e.g.cap_valuewould be clearer in a future cleanup. - The branch is not rebased onto the latest
mainhead as far as I can tell from the provided history; the PR diff already includes the worker-cap HUD additions (b23a82d) which appear to be in the working branch. Worth a re-check at merge time.
Overall, the PR meets the issue's acceptance bar with no blocking issues. Approve.
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
Recommendation: Approve
This PR addresses the core acceptance criteria of issue #139 (compact event log drawer) and follows the repository's testing and scripting conventions. The work is internally consistent, the new test verifies the bounded log behavior, and the integration with push_event() is correctly placed in _on_tick() for goal completion.
Change-by-Change Findings
scripts/main.gd
- Goal completion event (lines 1267-1275): Captures
goal_idbeforeactive_goalis reassigned tonew_goal, then callspush_eventafter the rotation. This ordering is correct — thegoal_idlocal is unaffected by the reassignment, and the appendedactive_goal["id"]incompleted_goal_idsis also the old id. ✅ render_hud_row()andrender_event_drawer()added torender_all(). The HUD row work appears to be a bundled UI change from issues #146/#147/#148 (worker cap, food warning, active goal). While not strictly required for #139, the branch is titled around #139 and the git log shows three separatefeat:/fix:commits on this branch. Bundling is reasonable but is worth flagging.render_event_drawer()correctly reads fromstate.get("events", []), formats ticks witht%02d, and caps the visible log at 6 events while the underlying store keeps 8. This is a small but clean decoupling between the store size and the UI display size.toggle_event_drawer()usesgui_inputon the label withMOUSE_FILTER_STOP— this is a sensible pattern (matches the existingtile_panel.gui_inputstyle at line 608-611) and respects the "collapsed by default" requirement in #139 (panel startsvisible = false).event_drawer_visiblestate is in-memory only (not persisted) — appropriate for a transient UI affordance, and not in conflict with any stated requirement.
scenes/main.tscn
- Adds the new
EventDrawerLabel/EventDrawerPanel/EventDrawerMargin/EventDrawerLognodes, plus the HUD row nodes (HudWorkerCap,HudFoodWarning,HudGoalLabel). EventDrawerPanelstarts hidden (visible = false) — matches the "collapsed by default" requirement.- The
unique_name_in_owner = trueflags let the%NodeNamelookups inmain.gdresolve. ✅ - A minor formatting nit: the diff shows a stray blank line inside the existing
[node name="ActivityLabel"]block, which is a whitespace change rather than a behavioral one. Not blocking.
tests/test_runner.gd
test_bounded_event_logis registered in_initialize()and validates: cap at 8, LIFO order (newest at index 0), eviction count of 4, empty-log behavior, and single-event behavior. ✅- The test simulates the same
push_front+while size > MAXloop thatpush_event()uses. Becausepush_event()is a method onmain.gdthat mutatesstate.events, a pure-logic test like this is the right shape for a unit test (per the issue's own guidance: "Add tests for bounded log behavior if implemented as pure logic").
Standards Compliance
- Branch naming:
saffron/issue-139-event-log-drawerfollows the established contributor-prefix pattern used by other branches in the repo (e.g.,saffron/issue-146-worker-cap-calculation). - Test conventions: New test function signature
func test_bounded_event_log(gs: Node) -> void:matches the existing_assert_eq/_assert_emptyhelpers and follows the print-banner style used throughouttest_runner.gd. - GDScript style: GDScript 4 idioms used (
@onready,match,mini(),String()casts,is_instance_valid). The variable shadowing ofcap(local) with the globalcap()function insiderender_hud_rowwas caught and fixed in commitc3489ee— good iteration. - AGENTS.md alignment: Implementation respects the "bottom dock is the primary mode" and "popup overlays, not persistent sidebars" guidance — the event drawer is a bottom-anchored, transient overlay that toggles in/out. ✅
Linked Issue Fit (#139)
| Acceptance criterion | Status |
|---|---|
| Recent events stored in a small bounded list | ✅ (push_event already capped at 8; reinforced by test) |
| UI shows latest event while collapsed | ✅ (EventDrawerLabel shows "Last: …") |
| Expanded drawer shows short recent history | ✅ (EventDrawerPanel shows up to 6 lines) |
| Goal completion from #131/#134 writes to log | ✅ (new push_event in _on_tick) |
| Worker cap/upkeep events from #133 write to log later | ⏳ Not in this PR — issue text explicitly says "later"; out of scope for this PR |
| Tests for bounded log behavior | ✅ (test_bounded_event_log) |
| Collapsed by default, desktop-companion feel | ✅ (panel visible = false, subtle color modulations) |
All in-scope criteria are met.
Evidence Provider Findings
No evidence providers configured — no findings to report.
Tool Harness Findings
Tool harness reports 0 planned requests and a planning parse warning. No tool-harness-driven blockers. The provided diff and the git log for the branch (4 commits ending in c3489ee) are sufficient to verify the change.
Unknowns or Needs Verification
get_low_food_level()return values:render_hud_row()matches on"starving"and"low". This function is referenced elsewhere in the codebase (per thecapgrep) but its body was not in the provided diff. The PR will only render correctly if those strings are the actual return values. Recommend a quick visual check on first run.rotating_goal.gdconstants: Thematch goal_typearms useRotatingGoal.GOAL_TYPE_RESOURCE,GOAL_TYPE_BUILD, andGOAL_TYPE_BUILD_COMPLETE. These are presumably defined inscripts/rotating_goal.gd; the diff does not modify that file, so they should already exist. Out-of-scope to verify here, but worth noting.- Scope note: The branch bundles HUD row work from #146/#147/#148 alongside the #139 work. The git log shows this is a multi-commit branch (
b23a82d→d7c806c→c9ae972→c3489ee). The PR title only mentions #139. If a more atomic PR is preferred for review hygiene, consider splitting — but the actual code quality is sound either way.
Overall: Solid, well-scoped implementation of the issue's intent. Approve.
… row Add three new HUD row labels (HudWorkerCap, HudFoodWarning, HudGoalLabel) to the main scene, displaying: - Worker count / cap (e.g. '1 / 3') - Food/upkeep warning when food is low or starving (⚠ LOW FOOD, ⚠ STARVING) - Active rotating goal name and progress (e.g. 'Goal: Wood (5/10)') render_hud_row() is called from render_all() alongside existing render_* functions. Food warning only shown when relevant (low/starving). Goal progress hidden when no active goal or when complete.
Add EventDrawerLabel, EventDrawerPanel, EventDrawerMargin, and EventDrawerLog nodes that are referenced by @onready vars in scripts/main.gd but were missing from the scene file. Without these nodes, Godot crashes at scene load with: 'Nonexistent function 'visible' in base 'null instance'' Fixes #139
c3489ee to
a6f53cb
Compare
The bounded_event_log test passes an Array to _assert_empty, but the function signature only accepted Dictionary. Change parameter type to Variant so both Dictionaries and Arrays work.
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
Recommendation: Approve with minor notes
This PR cleanly addresses linked issue #139 (compact event log drawer) and layers HUD row improvements on top of the already-merged work for #146, #147, #148. Acceptance criteria for #139 are met, the bounded log is testable as pure logic, and the scene/code/tests stay internally consistent.
Change-by-change findings
scripts/main.gd (+100)
- Adds
@onreadyreferences for the six new HUD/drawer nodes — names match the%UniqueNamenodes in the scene file. ✓ event_drawer_visiblestate +toggle_event_drawer()are minimal and correct; thegui_inputlambda onevent_drawer_labelwithMOUSE_FILTER_STOPcleanly turns the collapsed label into a click target. Minor: the lambda could be a named_on_event_drawer_label_gui_input(event)for symmetry with other handlers, but it's a style nit, not a correctness issue.push_event("Goal completed: %s. The colony moves on." % goal_id)is inserted afterrotate_after_completion, satisfying the "Goal completion from #131/#134 can write to the log" criterion.render_hud_row()reusesactive_worker_count(),get_worker_cap(),get_low_food_level(), andRotatingGoal.*— all from the already-merged #146/#147/#148 PRs. No duplicated logic. The earliercap-variable shadow was already fixed (commita6f53cb).render_event_drawer()shows the latest event while collapsed and up to 6 most-recent events in the expanded panel;state.eventsis already bounded to 8 by the existingpush_eventloop, so the 6-row render is well within the cap.
scenes/main.tscn (+72 / -1)
- Adds
HudWorkerCap,HudFoodWarning,HudGoalLabelunderBackdrop/Margin/Root/Left/HudRowand the fourEventDrawer*nodes at the scene root withanchor_top=anchor_bottom=1.0, keeping the bottom-strip-first convention from AGENTS.md. load_steps=2 → 5is correct for the four new top-level sub-resources.- Minor: the merge introduced an extra blank line inside the
ActivityLabelblock; cosmetic only.
tests/test_runner.gd (+34 / -1)
_assert_empty(d: Variant, ...)is the right call: the bounded-log test needs to assert against an emptyArray, and the helper previously typedDictionary. The corresponding README note is misplaced (see below).test_bounded_event_logis registered in_initialize()and covers cap, LIFO ordering, eviction count, and edge cases (empty / single event) — matches the issue's "Add tests for bounded log behavior if implemented as pure logic" guidance.- Caveat: the test simulates the bounded logic locally rather than calling
main.push_event(...). Acceptable per the issue's "if implemented as pure logic" qualifier, but a follow-up that drives the realpush_event12 times against amaininstance would lock in the contract more tightly.
README.md (+1)
- The added line
# test fix: _assert_empty accepts Variantis dropped into the Contributors / desktop-companion-dock paragraph. It reads as a commit-message note leaking into user-facing docs. Recommend moving it to a CHANGELOG entry or removing it; a one-line bullet in the Contributors section about the change is fine, but a raw hash-comment is not the right format here.
Standards Compliance
- AGENTS.md UX rules: Bottom-strip-first ✓, glanceable collapsed state ✓, no persistent app-sidebar (drawer is overlay) ✓, no new menu chrome ✓.
- AGENTS.md / CONTRIBUTING.md: Tests added for new behavior ✓, changes concentrated in
main.gdper the "monolithic" architecture note ✓. - Release process: No tag churn; PR lands on a feature branch as expected. ✓
- Workspace conventions: Scene nodes use
unique_name_in_owner = trueconsistently with existing%…lookups. ✓
Linked Issue Fit (#139)
- Bounded list with small cap → ✓ (8, LIFO via
push_front+pop_back). - Collapsed UI shows latest event → ✓ (
EventDrawerLabel, "Last: …"). - Expanded drawer shows short recent history → ✓ (last 6, with
t%02dtick prefix). - Goal completion (#131/#134) writes to log → ✓ (
push_eventafterrotate_after_completion). - Worker cap/upkeep (#133) can write later → ✓ (existing
push_eventcall sites from the merged #133/#146/#147 work remain compatible; the newrender_hud_rowandrender_event_drawerdon't block future wiring). - Tests for bounded log → ✓ (
test_bounded_event_log).
Refs to #146/#147/#148 are satisfied via integration with the already-merged PRs (#160, #167, #162) — the HUD row renders the helpers those issues introduced rather than re-implementing them.
Evidence Provider Findings
No evidence providers configured — nothing to evaluate.
Tool Harness Findings
Tool harness returned 0 planned requests with a planning-warning. No actionable signal; treated as no-op.
Unknowns / Needs Verification
event_drawer_label.gui_input.connect(...)is wired inwire_controls()after@onreadyresolution; should be safe, but a headless smoke run after merge is worth confirming the lambda closure overselfworks under Godot 4.2.2.- The
t%02dformat inrender_event_drawerwill mis-align after tick 100. Cosmetic; considert%03dor a wider format if the game is expected to run that long. - The README contributors-section addition is the only documentation regression; should be relocated or reworded before merge to keep the section user-facing.
Sources
- PR metadata, diff, and linked issue #139 body (provided corpus).
- Repository impact scan confirming
get_low_food_level,get_worker_cap,RotatingGoal.*, andpush_eventare all defined and used consistently. - Repository history showing prior shadow-
capfix (a6f53cb) and prior EventDrawer-node fix (66d6469) already applied — the diff under review reflects the post-fix state. - AGENTS.md for UX and validation conventions.
Fixes #139
Refs #146, #147, #148
This PR bundles three related UI improvements:
Event log drawer (#139)
HUD row (#146, #147, #148)
Scene changes