From c663721d342eb3c283196b5a4d4f608df57e15be Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 14:49:12 -0400 Subject: [PATCH] NEO-122: Harden quest HUD error/accept rendering. Keep accept merge visible after sync failure, clear stale progress error on successful accept, show sync banner with rows, and block Shift+Q when definitions failed to load. --- client/scripts/quest_hud_controller.gd | 24 +++-- client/test/quest_hud_controller_test.gd | 107 +++++++++++++++++++ docs/reviews/2026-06-07-NEO-122-follow-up.md | 8 +- 3 files changed, 130 insertions(+), 9 deletions(-) diff --git a/client/scripts/quest_hud_controller.gd b/client/scripts/quest_hud_controller.gd index 5b9cd97..88b2d1b 100644 --- a/client/scripts/quest_hud_controller.gd +++ b/client/scripts/quest_hud_controller.gd @@ -84,8 +84,10 @@ func _on_progress_received(snapshot: Dictionary) -> void: func _on_sync_failed(reason: String) -> void: _progress_error = reason - _last_snapshot = {} - _accept_quest_patch = {} + if _accept_quest_patch.is_empty(): + _last_snapshot = {} + elif _last_snapshot.is_empty(): + _last_snapshot = QuestProgressClient.merge_quest_row_into_snapshot({}, _accept_quest_patch) _render_progress_label() @@ -95,6 +97,7 @@ func _on_accept_result_received(quest_id: String, result: Dictionary) -> void: var quest_variant: Variant = result.get("quest", null) if quest_variant is Dictionary: _accept_quest_patch = (quest_variant as Dictionary).duplicate(true) + _progress_error = "" _last_snapshot = QuestProgressClient.merge_quest_row_into_snapshot( _last_snapshot, _accept_quest_patch ) @@ -127,16 +130,18 @@ func _render_progress_label() -> void: if not is_instance_valid(_progress_label): return var header := "Quests:" - if not _progress_error.is_empty(): - _progress_label.text = "%s\nerror — %s" % [header, _progress_error] - return if _last_snapshot.is_empty(): + var lines: PackedStringArray = [header] + if not _progress_error.is_empty(): + lines.append("error — %s" % _progress_error) if not _defs_error.is_empty(): - _progress_label.text = ("%s\ndefinitions error — %s\nLoading…" % [header, _defs_error]) - else: - _progress_label.text = "%s\nLoading…" % header + lines.append("definitions error — %s" % _defs_error) + lines.append("Loading…") + _progress_label.text = "\n".join(lines) return var lines: PackedStringArray = [header] + if not _progress_error.is_empty(): + lines.append("sync error — %s" % _progress_error) if not _defs_error.is_empty(): lines.append("definitions error — %s" % _defs_error) var defs: Array = _defs_snapshot() @@ -300,6 +305,9 @@ func _request_eligible_accept() -> void: if _progress_error.is_empty() and _last_snapshot.is_empty(): _render_accept_feedback_label("Quest accept: progress loading — try again") return + if not _defs_error.is_empty() and _defs_snapshot().is_empty(): + _render_accept_feedback_label("Quest accept: definitions error — try again") + return if _defs_error.is_empty() and _defs_snapshot().is_empty(): _render_accept_feedback_label("Quest accept: definitions loading — try again") return diff --git a/client/test/quest_hud_controller_test.gd b/client/test/quest_hud_controller_test.gd index 906c890..f443f3b 100644 --- a/client/test/quest_hud_controller_test.gd +++ b/client/test/quest_hud_controller_test.gd @@ -274,3 +274,110 @@ func test_stale_sync_reapplies_accept_quest_patch() -> void: controller.call("_on_progress_received", _not_started_snapshot()) # Assert assert_str(progress_label.text).contains("active step") + + +func test_accept_clears_progress_error_and_shows_active_row() -> void: + # Arrange + var progress_client: Node = _make_progress_client(NoopHttpTransport.new()) + var controller: Node = QuestHudController.new() + var progress_label := Label.new() + auto_free(controller) + auto_free(progress_label) + add_child(controller) + await get_tree().process_frame + controller.set("_progress_client", progress_client) + controller.set("_progress_label", progress_label) + controller.set("_last_snapshot", _not_started_snapshot()) + controller.set("_progress_error", "HTTP 404 (player unknown)") + # Act + ( + controller + . call( + "_on_accept_result_received", + QuestHudController.GATHER_QUEST_ID, + { + "accepted": true, + "quest": + { + "questId": QuestHudController.GATHER_QUEST_ID, + "status": "active", + "currentStepIndex": 0, + "objectiveCounters": {}, + }, + } + ) + ) + # Assert + assert_str(progress_label.text).contains("active step") + assert_str(progress_label.text).not_contains("error —") + + +func test_sync_fail_preserves_accept_quest_patch() -> void: + # Arrange + var progress_client: Node = _make_progress_client(NoopHttpTransport.new()) + var controller: Node = QuestHudController.new() + var progress_label := Label.new() + auto_free(controller) + auto_free(progress_label) + add_child(controller) + await get_tree().process_frame + controller.set("_progress_client", progress_client) + controller.set("_progress_label", progress_label) + ( + controller + . set( + "_accept_quest_patch", + { + "questId": QuestHudController.GATHER_QUEST_ID, + "status": "active", + "currentStepIndex": 0, + "objectiveCounters": {}, + } + ) + ) + ( + controller + . set( + "_last_snapshot", + ( + QuestProgressClient + . merge_quest_row_into_snapshot( + _not_started_snapshot(), + { + "questId": QuestHudController.GATHER_QUEST_ID, + "status": "active", + "currentStepIndex": 0, + "objectiveCounters": {}, + } + ) + ) + ) + ) + # Act + controller.call("_on_sync_failed", "HTTP 503") + # Assert + assert_str(progress_label.text).contains("active step") + assert_str(progress_label.text).contains("sync error — HTTP 503") + + +func test_shift_q_shows_definitions_error_when_defs_failed() -> void: + # Arrange + var controller: Node = QuestHudController.new() + var accept_label := Label.new() + var progress_client: Node = QuestProgressClient.new() + var defs_client: Node = QuestDefinitionsClient.new() + auto_free(controller) + auto_free(accept_label) + auto_free(progress_client) + auto_free(defs_client) + add_child(controller) + controller.set("_progress_client", progress_client) + controller.set("_defs_client", defs_client) + controller.set("_accept_label", accept_label) + controller.set("_last_snapshot", _not_started_snapshot()) + controller.set("_progress_error", "") + controller.set("_defs_error", "HTTP 503") + # Act + controller.call("_request_eligible_accept") + # Assert + assert_str(accept_label.text).contains("definitions error") diff --git a/docs/reviews/2026-06-07-NEO-122-follow-up.md b/docs/reviews/2026-06-07-NEO-122-follow-up.md index 6696057..04336e2 100644 --- a/docs/reviews/2026-06-07-NEO-122-follow-up.md +++ b/docs/reviews/2026-06-07-NEO-122-follow-up.md @@ -55,6 +55,12 @@ Seven commits landed after the first review: code-review follow-up (quest-defini 4. ~~**Quest HUD stale after accept** — Merge accept-response `quest` row into snapshot immediately; re-apply patch when stale GET returns `not_started`; queue pending sync when GET busy. **Done.** +5. ~~**Accept hidden by progress error** — Clear `_progress_error` on successful accept merge; render snapshot rows with `sync error — …` banner instead of error-only early return. **Done.** + +6. ~~**Sync fail drops accept merge** — `_on_sync_failed` no longer clears `_accept_quest_patch` or merged snapshot; rebuilds from patch when snapshot empty. **Done.** + +7. ~~**Shift+Q hides definitions failure** — Block eligible accept when `_defs_error` is set and catalog is empty. **Done.** + ## Verification ```bash @@ -64,7 +70,7 @@ godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd \ -a res://test ``` -Full suite: **247** cases, **0** orphans (GdUnit exit **0**). +Full suite: **254** cases, **0** orphans (GdUnit exit **0**). ```bash # Focused NEO-122 / layout subset