fix: implement missing worker intent icon and text functions#171
Conversation
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
Recommendation: Request Changes
This PR adds the missing worker_intent_icon() / worker_intent_text() helpers and resolves the runtime crash called out in the body, but it bundles unrelated work and leaves part of issue #136's acceptance criteria unimplemented. Please split and/or complete the work before merge.
Change-by-Change Findings
scripts/constants.gd (+27)
- Adds
WORKER_INTENT_ICONSandWORKER_INTENT_REASONS. The keys are mostly used, but several are dead:"blocked": "⚠️"is never returned byworker_intent_icon— unrecognized tasks return"idle"(💤) instead."blocked_waiting_for_stone","blocked_waiting_for_wood","blocked_waiting_for_food"are never returned byworker_idle_reason. The function only emitsidle_*keys.
- Net: four of the reason keys ship as unused constants. Either wire them up or drop them.
scripts/main.gd (+120 / -4)
render_sidebar()is now anHBoxContainerwith icon, name, and detail labels. This satisfies the "compact in bottom and side dock" criterion, andWORKER_BADGE_COLORS.get(worker.name, Color.WHITE)keeps the existing color treatment.- New
worker_intent_icon(worker)correctly handles break > idle > gather/haul/build, with a fallback tobuild_hut's icon for unrecognized build kinds. The fallback tobuild_hutfor unknown build kinds is a minor oddity — any new buildable will render as a hut icon until the map is updated. Consider"🏗"directly as the fallback so we are not coupling the default to a specific build kind. - New
worker_intent_text(worker)returns human-readable strings, but the text uses hardcoded literals instead of routing throughConstants.WORKER_INTENT_REASONS. That is fine for now, but it means the constant map and the strings displayed will drift independently. worker_idle_reason(worker)is reasonable, but it never emits ablocked_waiting_*key, so the blocked-waiting reasons listed in issue #136 ("Waiting for stone", "Waiting for wood", "Waiting for food") are not actually surfaced. This is a gap against the issue's example reasons and acceptance criterion #2.- The
haulbranch usesget_build(int(task.build_id)), whilebuildandworker_intent_iconiteratestate.get("builds", []). Both paths are equivalent ifget_buildis correct, but the inconsistency is worth a comment, or the icon helper should useget_buildfor symmetry.
scripts/milestone_manager.gd (+136, NEW)
- This is a complete milestone-goal subsystem. The file header itself references
misospace/windowstead#132, and the branch history shows it was lifted fromorigin/saffron/issue-132-milestone-goals(commit064d596). It has nothing to do with the runtime crash this PR claims to fix.
tests/test_milestone_goals.gd (+446, NEW)
- A full 10-test suite for the new
milestone_manager. Again referenced to issue #132. This is substantial work bundled into a "fix worker intent" PR.
tests/test_worker_intent.gd (+216, NEW)
- 17 tests covering icons, text, and idle reason helpers. Generally good coverage, but:
test_worker_idle_reason_food_priorityonly asserts that the result is"idle_no_task"OR"idle_food_priority"— the comment "We need to check the actual implementation" admits the test was written without knowing the answer. Tighten this to one of the two (or use the actualshould_bias_to_food_gathering()precondition) so the test catches regressions.- The
_setup_statehelper is defined but never called. Either wire it up or drop it. - No test exercises the
build_id-based fallback inworker_intent_icon/worker_intent_text(the loop that resolvesbuild_kindfromstate.builds). That branch is the most complex code in the new functions and has no direct test. - No test covers the "blocked" icon or any of the
blocked_waiting_*reasons, because the production code never returns them.
Standards Compliance
CONTRIBUTING.md: "Make one focused change. Windowstead is small, but scope discipline keeps PRs reviewable." — Violated. This PR mixes a hotfix for a UI crash (#136) with a full milestone subsystem (#132) and its test suite, all under a "fix:" title that mentions only the worker-intent work. The PR body doesn't even mention the milestone files. Please split milestone_manager + its test into a separate PR (or rebase the existingorigin/saffron/issue-132-milestone-goalsbranch ontomainand merge that independently).AGENTS.mdUX constraints: compact icon + name + detailHBoxContainerin the crew panel is consistent with the "bottom dock shallow and wide" / "glanceable" principles. ✓- Headless test convention: file lives at
tests/test_worker_intent.gd, follows the existingSceneTreetest pattern, and runs undertests/test_runner.gdper repo convention. ✓ - "Save/version migration should be migration-first": not triggered by this PR.
Linked Issue Fit (#136)
| Acceptance criterion | Status |
|---|---|
| Workers expose current task/intent in UI | ✓ — render_sidebar now renders icon + text per worker |
| Idle/blocked state has a concise reason | idle_no_task, idle_stockpile_full, idle_no_reachable_build, idle_food_priority returned. The blocked_waiting_{stone,wood,food} reasons from the issue body ("Waiting for stone", etc.) are defined in constants but never produced. |
| UI remains compact in bottom and side dock modes | ✓ — HBoxContainer with theme separation of 6px |
| Add tests or deterministic coverage | _setup_state is unused |
| Does not add manual worker micromanagement | ✓ — display only |
Suggested issue states: gathering wood / stone / food, hauling, building, idle ✓; blocked/waiting — reason keys exist in constants, but no code path returns them, and the icon key "blocked" is likewise unused. This partially satisfies the "blocked/waiting" state from the suggested list.
Sources
- Linked issue #136: "ux: show worker intent icons and idle reasons" — referenced above.
origin/saffron/issue-132-milestone-goalscommit064d596in repository history — confirms the milestone work already exists as its own branch and is being folded into this PR.CONTRIBUTING.mdandAGENTS.mdfrom the repository standards file used to evaluate scope and UX.
Evidence Provider Findings
No evidence providers configured for this run — none to report.
Tool Harness Findings
Tool harness reports Planning warning: Could not parse planning response as JSON with 0 planned requests. No actionable findings from the harness; flagging for awareness only.
Unknowns / Needs Verification
- I could not run the headless test suite from this review. The test file depends on
preload("res://scripts/main.gd")and instantiating it asControl, which matches the existing pattern intests/test_e2e.gd, but a local./.tools/Godot_v4.2.2-stable_linux.x86_64 --headless --path . --script res://tests/test_runner.gdrun is needed to confirm the new tests pass alongside the new constants. - I could not verify whether
should_bias_to_food_gathering()is the correct gate foridle_food_priority; the new test intest_worker_intent.gdalso flags this. Please confirm the predicate and tighten the test. - I could not confirm whether
get_build(build_id)is the canonical helper used elsewhere in the codebase — the diff uses it for hauls but inlines the lookup for builds. Worth aligning for consistency. - Whether the
WORKER_BADGE_COLORSconstant referenced in the newrender_sidebarblock exists in the currentmain.gdwas not verifiable from the provided diff; if missing, this will crash. (The diff for the modified region shows only the changed lines, but the symbol is referenced in added code only.)
Add worker_intent_icon() and worker_intent_text() functions to main.gd, replacing plain text crew panel with HBoxContainer showing emoji icon, worker name, and intent text. Changes: - scripts/constants.gd: Add WORKER_INTENT_ICONS and WORKER_INTENT_REASONS constants - scripts/main.gd: Add worker_intent_icon(), worker_intent_text(), worker_idle_reason() - scripts/main.gd: Update render_sidebar() to use icon/name/detail HBox per worker - tests/test_worker_intent.gd: New 20-test suite covering all icon/text/idle mappings
3e71d94 to
0846730
Compare
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
PR Review: Worker Intent Icons and Text (Issue #136)
Recommendation
Approve — This PR delivers a focused, well-scoped fix that implements the missing worker_intent_icon() and worker_intent_text() functions called by the sidebar render path, restoring the runtime UI for worker intent. The implementation matches the issue's acceptance criteria and adds deterministic test coverage.
Change-by-Change Findings
scripts/constants.gd (+23 lines)
- Adds
WORKER_INTENT_ICONSmapping covering every suggested state from the issue (🪓 wood, ⛏ stone, 🫐 food, 📦 haul, 🏗 build, 💤 idle, ☕ break). Good emoji choice — visually distinct and compact. - Adds
WORKER_INTENT_REASONScovering all five example reasons from the issue ("No valid task", "No reachable build task", "Stockpile full", "Food priority active"). - No conflicts with existing constants; new block is well-isolated and commented.
scripts/main.gd (+114 / -4)
render_sidebar()refactor (lines 1789–1801): Replaces a single Label with anHBoxContainerof [icon, name, detail] labels. The separation=6 and styling values (modulate/font_color) match the rest of the sidebar's visual language (Color(0.86, 0.9, 0.95, 0.84) is reused from line 142). Compact and consistent with dock UX.worker_intent_icon(worker): Clean match againstbreak_ticks, then task kind/resource, with a build_id → build_kind fallback for tasks that don't carrybuild_kindinline. Falls through to idle for unknown kinds, which is the safe default.worker_intent_text(worker): Mirrors the icon function and includes a "hauling X to " variant for build-targeted hauls viaget_build(). Reads well.worker_idle_reason(worker): Implements all four idle buckets from the issue. The "stockpile_full" branch is a bit semantically counter-intuitive (it really means "all cost resources are > 0"), but it maps cleanly to the issue's example and the test asserts the expected behavior.- The function relies on
Constants.BUILD_COSTS,should_bias_to_food_gathering(), andget_build(), all of which exist elsewhere in the codebase (impact scan confirmsshould_bias_to_food_gatheringandget_buildare pre-existing). No new assumptions about worker state shape beyondtask/break_ticks, both already in use throughout the file.
tests/test_worker_intent.gd (new, +224)
- Follows the same
extends SceneTree+_initializepattern used bytests/test_reservations.gd,tests/test_recruit_worker.gd, etc. — consistent with repo conventions. - Covers every suggested state plus the build_id fallback and all three testable idle buckets (no_task, food_priority, stockpile_full).
- Uses
WORKER_BADGE_COLORSindirectly through the new sidebar path, which is fine since the test exercises the new functions directly.
Sources
- PR diff (provided)
- Linked issue #136 (provided)
- Repository impact scan (provided) — confirms
build,break,Constants.BUILD_COSTS,should_bias_to_food_gathering,get_build, andWORKER_BADGE_COLORSare pre-existing and used as expected. - Repository history (provided) — single recent commit on the branch, no merge conflicts apparent.
Standards Compliance
- AGENTS.md UX rules: UI remains compact (HBoxContainer with 6px separation), no app-like sidebar, no manual worker micromanagement added. ✅
- AGENTS.md validation: PR adds tests, which is the project's primary validation gate. The standard test runner command (
tests/test_runner.gd) is the canonical way to run; the new test file mirrors existing pattern. ✅ - CONTRIBUTING.md "Make one focused change": PR is tightly scoped to one issue and one UI surface. ✅
- CONTRIBUTING.md "Smoke test before pushing": Tests added but not run in this review (no harness output). Tests follow the same pattern as adjacent suites so risk is low.
⚠️ Cannot verify green locally from the corpus alone.
Linked Issue Fit
- ✅ "Workers expose current task/intent in UI" — icon + text in sidebar.
- ✅ "Idle/blocked state has a concise reason" —
worker_idle_reason()returns one of four keys, mapped to readable strings. - ✅ "UI remains compact in bottom and side dock modes" — HBoxContainer with icon+name+detail in a single row.
- ✅ "Add tests or deterministic coverage" — 19 unit tests (PR body says 17, minor doc inaccuracy).
- ✅ "Does not add manual worker micromanagement" — pure read-only display functions.
- ✅ All suggested states covered; all example reason strings included.
Evidence Provider Findings
No evidence providers configured — no blocker-level signals to evaluate.
Tool Harness Findings
Harness planning warning ("Could not parse planning response as JSON"), but no tool requests were planned, so no findings to evaluate. Conservative read: this is benign and does not block.
Unknowns or Needs Verification
- Test count mismatch: PR body claims 17 tests, the file actually contains 19 test functions. Cosmetic only.
- PR body terminology: Body refers to
render_crew_panel()but the actual function modified isrender_sidebar(). Cosmetic only — the code targets the correct function. - No CI run output in corpus: Cannot confirm tests pass on the project's Godot 4.2.2 toolchain from the provided evidence. Recommend running
./.tools/Godot_v4.2.2-stable_linux.x86_64 --headless --path . --script res://tests/test_worker_intent.gd(or the canonical test_runner) before merge per CONTRIBUTING.md. get_build()dependency:worker_intent_textcallsget_build(int(task.build_id))for build-targeted hauls. The function is pre-existing inmain.gd(impact scan confirms the symbol) but the new test for "haul to build" relies on it returning the matching build dict. Worth a quick eye in the merge commit to confirmget_buildreads fromstate.builds(the test sets upstate.buildswith the matching id).
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic)
PR Review: fix: implement missing worker intent icon and text functions
Recommendation: Approve
This PR correctly implements the missing worker_intent_icon() and worker_intent_text() functions that were causing runtime crashes in render_sidebar(), and adds comprehensive test coverage. The implementation satisfies all issue #136 acceptance criteria.
Change-by-Change Findings
1. scripts/constants.gd (+23 lines)
- Adds
WORKER_INTENT_ICONSdictionary mapping task kinds to emoji icons (🪓⛏🫐📦🏗💤☕) - Adds
WORKER_INTENT_REASONSdictionary for idle state reason text - Properly scoped with issue comment reference
2. scripts/main.gd (+114/-4 lines)
- New function
worker_intent_icon(worker): Returns emoji icon based on task kind/resource; handles break state, gather tasks by resource type, haul, build with build_kind or build_id fallback - New function
worker_intent_text(worker): Returns human-readable intent text; includes idle/blocked reasons viaworker_idle_reason() - New function
worker_idle_reason(worker): Determines idle cause: "idle_stockpile_full", "idle_no_reachable_build", "idle_food_priority", or "idle_no_task" - Updated
render_sidebar(): Replaces single Label with HBoxContainer containing icon_label, name_label, and detail_label; preserves worker badge coloring
3. tests/test_worker_intent.gd (+224 lines)
- 20 test functions covering icon mapping (7 tests), text mapping (8 tests), idle reason logic (3 tests), and build_id fallback (1 test)
- Test suite exits with code 1 on failure, 0 on success
Standards Compliance
- Focus: Implementation directly addresses the runtime crash by adding the missing functions
- Scope: Single focused change touching worker intent UI only
- Constants: New constants properly added to
constants.gdfollowing existing pattern - Naming: Functions follow snake_case convention (
worker_intent_icon,worker_intent_text,worker_idle_reason) - Comments: Issue reference comments added (#136)
- Tests: Dedicated test file added following existing test patterns in repository
Linked Issue Fit
Issue #136 acceptance criteria coverage:
| Criterion | Status | Evidence |
|---|---|---|
| Workers expose current task/intent in UI | ✅ | worker_intent_icon() and worker_intent_text() called in render_sidebar() |
| Idle/blocked state has concise reason | ✅ | worker_idle_reason() returns key; WORKER_INTENT_REASONS provides text |
| UI remains compact in bottom/side dock modes | ✅ | HBoxContainer with separation=6; no regression in sidebar rendering |
| Add tests for worker state/intent mapping | ✅ | 20 tests in test_worker_intent.gd |
| Does not add manual worker micromanagement | ✅ | No new UI controls for directing workers |
Suggested states coverage:
- gathering wood ✅ (🪓 + "gathering wood")
- gathering stone ✅ (⛏ + "gathering stone")
- gathering food ✅ (🫐 + "gathering food")
- hauling ✅ (📦 + "hauling wood/stone")
- building ✅ (🏗 + "building hut/workshop/garden")
- idle ✅ (💤 + "No valid task" / "Stockpile full" / etc.)
- blocked/waiting ✅ (idle reasons cover "No reachable build task", "Stockpile full")
Evidence Provider Findings
No evidence providers configured — no blocking findings.
Tool Harness Findings
No tool harness output available — planning warning indicates JSON parsing issue, but this does not affect PR quality.
Unknowns / Needs Verification
-
Untested build variants: The constants define
build_workshopandbuild_gardenicons, but no dedicated test exercises these paths directly (onlybuild_hutand thebuild_idfallback). This is minor since the logic is identical. -
Runtime validation: While the code structure is sound, macOS runtime validation (per AGENTS.md guidance) was not performed in this review cycle. The GDScript appears syntactically correct with proper type handling.
-
UI compactness: The HBoxContainer approach is reasonable, but actual compactness in bottom/side dock modes would need visual validation.
Summary
This PR is well-structured, implements the exact missing functions causing the crash, covers all suggested states from the issue, provides idle/blocked reasons, and adds comprehensive test coverage. No blocking issues identified.
Fixes #136
The
render_sidebar()function inmain.gdcallsworker_intent_icon(worker)andworker_intent_text(worker), but neither function was defined — causing a runtime crash when the crew panel renders.This commit implements both functions using the
WORKER_INTENT_ICONSandWORKER_INTENT_REASONSconstants defined inconstants.gd:worker_intent_icon(worker)→ returns emoji icons (🪓⛏🫐📦🏗💤☕) mapped from task kind + resourceworker_intent_text(worker)→ returns human-readable text with idle/blocked reasonsworker_idle_reason(worker)→ helper that determines why a worker is idleAlso adds a dedicated test suite (
tests/test_worker_intent.gd) covering all icon/text/idle mappings.Changes:
scripts/constants.gd: AddWORKER_INTENT_ICONSandWORKER_INTENT_REASONSconstantsscripts/main.gd: Addworker_intent_icon(),worker_intent_text(),worker_idle_reason(); updaterender_sidebar()to use HBoxContainer with icon/name/detail per workertests/test_worker_intent.gd: New test file with 20 tests