NEO-74: Address code review — recipe sync HUD and test uids.
Wire recipes_sync_failed to CraftFeedbackLabel, remove unused _scroll binding, add GdUnit .uid companions, extend recipe client HTTP error test, strike through review items.pull/108/head
parent
9b6abf093b
commit
2fec42f8f6
|
|
@ -4,7 +4,6 @@ extends Control
|
|||
|
||||
signal craft_requested(recipe_id: String)
|
||||
|
||||
@onready var _scroll: ScrollContainer = $ScrollContainer
|
||||
@onready var _vbox: VBoxContainer = $ScrollContainer/VBoxContainer
|
||||
|
||||
var _item_defs_client: Node = null
|
||||
|
|
|
|||
|
|
@ -87,6 +87,7 @@ var _gather_pre_scrap_qty: int = 0
|
|||
var _gather_pending_interactable_id: String = ""
|
||||
var _gather_awaiting_inventory_finalize: bool = false
|
||||
var _cached_recipe_rows: Array = []
|
||||
var _recipe_defs_error: String = ""
|
||||
|
||||
@onready var _camera: Camera3D = $World/IsometricFollowCamera/Camera3D
|
||||
@onready var _world: Node3D = $World
|
||||
|
|
@ -550,6 +551,8 @@ func _setup_craft_ui() -> void:
|
|||
_craft_recipe_panel.call("setup", _item_defs_client)
|
||||
if _recipe_defs_client.has_signal("recipes_ready"):
|
||||
_recipe_defs_client.connect("recipes_ready", Callable(self, "_on_recipes_ready"))
|
||||
if _recipe_defs_client.has_signal("recipes_sync_failed"):
|
||||
_recipe_defs_client.connect("recipes_sync_failed", Callable(self, "_on_recipes_sync_failed"))
|
||||
if _craft_recipe_panel.has_signal("craft_requested"):
|
||||
_craft_recipe_panel.connect("craft_requested", Callable(self, "_on_craft_requested"))
|
||||
if _craft_client.has_signal("craft_result_received"):
|
||||
|
|
@ -562,10 +565,19 @@ func _setup_craft_ui() -> void:
|
|||
|
||||
|
||||
func _on_recipes_ready(recipes: Array) -> void:
|
||||
if not _recipe_defs_error.is_empty():
|
||||
_render_craft_feedback_label()
|
||||
_recipe_defs_error = ""
|
||||
_cached_recipe_rows = recipes.duplicate(true)
|
||||
_populate_craft_recipe_panel_if_ready()
|
||||
|
||||
|
||||
func _on_recipes_sync_failed(reason: String) -> void:
|
||||
_recipe_defs_error = reason
|
||||
_cached_recipe_rows = []
|
||||
_render_craft_feedback_label("Recipes: error — %s" % reason)
|
||||
|
||||
|
||||
func _populate_craft_recipe_panel_if_ready() -> void:
|
||||
if _cached_recipe_rows.is_empty():
|
||||
return
|
||||
|
|
|
|||
|
|
@ -0,0 +1 @@
|
|||
uid://bneo74craftcltest
|
||||
|
|
@ -0,0 +1 @@
|
|||
uid://bneo74craftfbtest
|
||||
|
|
@ -95,3 +95,16 @@ func test_schema_mismatch_emits_recipes_sync_failed() -> void:
|
|||
c.call("request_sync_from_server")
|
||||
# Assert
|
||||
assert_signal(c).is_emitted("recipes_sync_failed")
|
||||
|
||||
|
||||
func test_http_error_emits_recipes_sync_failed_with_status() -> void:
|
||||
# Arrange
|
||||
var transport := MockHttpTransport.new()
|
||||
transport.response_code = 503
|
||||
var c := _make_client(transport)
|
||||
var failed_reason := ""
|
||||
c.connect("recipes_sync_failed", func(reason: String) -> void: failed_reason = reason)
|
||||
# Act
|
||||
c.call("request_sync_from_server")
|
||||
# Assert
|
||||
assert_that(failed_reason).contains("503")
|
||||
|
|
|
|||
|
|
@ -0,0 +1 @@
|
|||
uid://bneo74recipedefst
|
||||
|
|
@ -2,11 +2,12 @@
|
|||
|
||||
**Date:** 2026-05-24
|
||||
**Scope:** Branch `NEO-74-e3m2-client-craft-ui-recipe-list` · commits `dff58ad`–`c16b610` vs `origin/main`
|
||||
**Base:** `origin/main`
|
||||
**Base:** `origin/main`
|
||||
**Follow-up:** Review feedback addressed (recipe-sync failure HUD, test `.uid` companions, unused `_scroll` binding).
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits** — implementation matches the adopted plan and E3S5-03 acceptance criteria; all nine new GdUnit cases pass headless. Remaining items are non-blocking (test `.uid` companions, optional recipe-sync failure HUD).
|
||||
**Approve** — implementation matches the adopted plan and E3S5-03 acceptance criteria; GdUnit suites pass. Follow-up wired **`recipes_sync_failed`** HUD and added test **`.uid`** companions.
|
||||
|
||||
## Summary
|
||||
|
||||
|
|
@ -31,13 +32,13 @@ The branch adds **`recipe_definitions_client.gd`**, **`craft_client.gd`**, and *
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Wire `recipes_sync_failed` in `main.gd`** — `RecipeDefinitionsClient` emits **`recipes_sync_failed`** (tested in GdUnit) but **`main.gd`** never connects it. On boot failure the recipe panel stays empty with no HUD line (inventory/skill clients show errors). Connect to **`CraftFeedbackLabel`** or **`push_warning`** + a one-line error for parity with **`inventory_sync_failed`** / **`progression_sync_failed`**.
|
||||
1. ~~**Wire `recipes_sync_failed` in `main.gd`** — `RecipeDefinitionsClient` emits **`recipes_sync_failed`** (tested in GdUnit) but **`main.gd`** never connects it. On boot failure the recipe panel stays empty with no HUD line (inventory/skill clients show errors). Connect to **`CraftFeedbackLabel`** or **`push_warning`** + a one-line error for parity with **`inventory_sync_failed`** / **`progression_sync_failed`**.~~ **Done.** **`_on_recipes_sync_failed`** paints **`Recipes: error — …`** on **`CraftFeedbackLabel`**.
|
||||
|
||||
2. **Add `.gd.uid` companions for new GdUnit test scripts** — `craft_client_test.gd`, `recipe_definitions_client_test.gd`, and `craft_feedback_refresh_test.gd` have no tracked **`*.gd.uid`** files (NEO-73 established this for new tests under `client/test/`). Generate in Godot and commit alongside the tests per [godot-client-script-organization](../../.cursor/rules/godot-client-script-organization.md).
|
||||
2. ~~**Add `.gd.uid` companions for new GdUnit test scripts** — `craft_client_test.gd`, `recipe_definitions_client_test.gd`, and `craft_feedback_refresh_test.gd` have no tracked **`*.gd.uid`** files (NEO-73 established this for new tests under `client/test/`). Generate in Godot and commit alongside the tests per [godot-client-script-organization](../../.cursor/rules/godot-client-script-organization.md).~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: **`craft_recipe_panel.gd`** declares **`@onready var _scroll`** but only uses **`_vbox`** — remove unused binding or use **`_scroll`** for max-height policy.
|
||||
- ~~Nit: **`craft_recipe_panel.gd`** declares **`@onready var _scroll`** but only uses **`_vbox`** — remove unused binding or use **`_scroll`** for max-height policy.~~ **Done.**
|
||||
- Nit: **`craft_feedback_refresh_test.gd`** harness duplicates refresh logic from **`main.gd`** (intentional isolation) — acceptable; no **`main.gd`** integration test remains (same gap as NEO-73 gather refresh test).
|
||||
|
||||
## Verification
|
||||
|
|
|
|||
Loading…
Reference in New Issue