diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 40cc223..645a1b7 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -877,7 +877,7 @@ func _setup_quest_progress_sync() -> void: _quest_defs_client.connect( "definitions_sync_failed", Callable(self, "_on_quest_definitions_sync_failed") ) - _render_quest_progress_labels() + _render_quest_progress_label() _render_quest_accept_feedback_label() if _quest_defs_client.has_method("request_sync_from_server"): _quest_defs_client.call("request_sync_from_server") @@ -888,13 +888,13 @@ func _setup_quest_progress_sync() -> void: func _on_quest_progress_received(snapshot: Dictionary) -> void: _quest_progress_error = "" _last_quest_progress_snapshot = snapshot.duplicate(true) - _render_quest_progress_labels() + _render_quest_progress_label() func _on_quest_progress_sync_failed(reason: String) -> void: _quest_progress_error = reason _last_quest_progress_snapshot = {} - _render_quest_progress_labels() + _render_quest_progress_label() func _on_quest_accept_result_received(quest_id: String, result: Dictionary) -> void: @@ -918,15 +918,11 @@ func _on_quest_accept_failed(_quest_id: String, reason: String) -> void: func _on_quest_definitions_ready(_quests: Array) -> void: _quest_defs_error = "" if _quest_progress_error.is_empty() and not _last_quest_progress_snapshot.is_empty(): - _render_quest_progress_labels() + _render_quest_progress_label() func _on_quest_definitions_sync_failed(reason: String) -> void: _quest_defs_error = reason - _render_quest_progress_labels() - - -func _render_quest_progress_labels() -> void: _render_quest_progress_label() @@ -938,9 +934,16 @@ func _render_quest_progress_label() -> void: _quest_progress_label.text = "%s\nerror — %s" % [header, _quest_progress_error] return if _last_quest_progress_snapshot.is_empty(): - _quest_progress_label.text = "%s\nLoading…" % header + if not _quest_defs_error.is_empty(): + _quest_progress_label.text = ( + "%s\ndefinitions error — %s\nLoading…" % [header, _quest_defs_error] + ) + else: + _quest_progress_label.text = "%s\nLoading…" % header return var lines: PackedStringArray = [header] + if not _quest_defs_error.is_empty(): + lines.append("definitions error — %s" % _quest_defs_error) var defs: Array = _quest_defs_snapshot() if defs.is_empty(): var quests: Variant = _last_quest_progress_snapshot.get("quests", null) diff --git a/client/test/quest_progress_client_test.gd b/client/test/quest_progress_client_test.gd index 6c26456..b99caad 100644 --- a/client/test/quest_progress_client_test.gd +++ b/client/test/quest_progress_client_test.gd @@ -8,6 +8,7 @@ const PROTOTYPE_QUEST_GATHER_ID := "prototype_quest_gather_intro" var _progress_capture: Dictionary = {} var _accept_capture: Dictionary = {} +var _accept_failed_capture: Dictionary = {} func _capture_progress(snapshot: Dictionary) -> void: @@ -18,6 +19,10 @@ func _capture_accept(quest_id: String, result: Dictionary) -> void: _accept_capture = {"questId": quest_id, "result": result} +func _capture_accept_failed(quest_id: String, reason: String) -> void: + _accept_failed_capture = {"questId": quest_id, "reason": reason} + + class MockHttpTransport: extends Node signal request_completed( @@ -112,6 +117,33 @@ class NoopHttpTransport: return OK +class PendingAcceptHttpTransport: + extends Node + signal request_completed( + result: int, response_code: int, headers: PackedStringArray, body: PackedByteArray + ) + var last_url: String = "" + var response_code: int = 200 + var body_json: String = "" + + func request( + url: String, + _custom_headers: PackedStringArray = PackedStringArray(), + _method: HTTPClient.Method = HTTPClient.METHOD_POST, + _request_data: String = "" + ) -> Error: + last_url = url + return OK + + func complete_pending() -> void: + request_completed.emit( + HTTPRequest.RESULT_SUCCESS, + response_code, + PackedStringArray(), + body_json.to_utf8_buffer() + ) + + func test_parse_not_started_quest_row() -> void: # Arrange var json := _not_started_progress_json() @@ -226,3 +258,33 @@ func test_request_accept_emits_deny_with_reason_code() -> void: var result: Dictionary = _accept_capture.get("result", {}) as Dictionary assert_that(bool(result.get("accepted", true))).is_false() assert_that(str(result.get("reasonCode", ""))).is_equal("prerequisite_incomplete") + + +func test_http_404_on_accept_emits_quest_accept_failed() -> void: + # Arrange + _accept_failed_capture = {} + var transport := MockHttpTransport.new() + transport.response_code = 404 + var noop := NoopHttpTransport.new() + var c := _make_client(noop, transport) + c.connect("quest_accept_failed", Callable(self, "_capture_accept_failed")) + # Act + c.call("request_accept", PROTOTYPE_QUEST_GATHER_ID) + # Assert + assert_that(_accept_failed_capture.get("questId", "")).is_equal(PROTOTYPE_QUEST_GATHER_ID) + assert_that(str(_accept_failed_capture.get("reason", ""))).is_equal("HTTP 404 (player unknown)") + + +func test_request_accept_returns_false_when_accept_busy() -> void: + # Arrange + var pending := PendingAcceptHttpTransport.new() + pending.body_json = _accept_success_json() + var noop := NoopHttpTransport.new() + var c := _make_client(noop, pending) + # Act + var first: bool = bool(c.call("request_accept", PROTOTYPE_QUEST_GATHER_ID)) + var second: bool = bool(c.call("request_accept", "prototype_quest_refine_intro")) + # Assert + assert_that(first).is_true() + assert_that(second).is_false() + pending.complete_pending() diff --git a/docs/manual-qa/NEO-122.md b/docs/manual-qa/NEO-122.md index 3849421..0d8965d 100644 --- a/docs/manual-qa/NEO-122.md +++ b/docs/manual-qa/NEO-122.md @@ -36,7 +36,8 @@ 6. After gather completes, press **Shift+Q**. Verify refine quest becomes **`active`**. 7. Craft **`refine_scrap_standard`** with Economy HUD open. Verify refine quest **`completed`** after successful craft. 8. Press **Q** again on already-completed gather. Verify deny feedback on **`QuestAcceptFeedbackLabel`** without Bruno. -9. Optional: stop server while Godot running, press **Q** — verify **`Quests: error — …`** or accept **`failed — …`** visible on HUD. +9. Optional: stop server while Godot running, press **Q** — verify **`Quests: error — …`** or **`Quest accept: failed — …`** visible on HUD. +10. Optional: block **`GET /game/world/quest-definitions`** only while progress GET succeeds — verify **`definitions error — …`** under **`QuestProgressLabel`** with raw quest ids as fallback. ## Notes diff --git a/docs/reviews/2026-06-07-NEO-122.md b/docs/reviews/2026-06-07-NEO-122.md index 2ade946..7c4cd47 100644 --- a/docs/reviews/2026-06-07-NEO-122.md +++ b/docs/reviews/2026-06-07-NEO-122.md @@ -6,7 +6,7 @@ ## Verdict -**Approve with nits** +**Approve with nits** — suggestions **1** and **3** and nits **1–2** below are **done** (strikethrough + **Done.**); suggestion **2** deferred to NEO-123. ## Summary @@ -34,17 +34,17 @@ NEO-122 delivers the Godot client slice for Epic 7 quest visibility: **`quest_pr ## Suggestions -1. **Surface `definitions_sync_failed` on HUD** — `_quest_defs_error` is set in `_on_quest_definitions_sync_failed` but never read in `_render_quest_progress_label`. Recipe defs follow the visible pattern (`Recipes: error — {reason}` on **`CraftFeedbackLabel`**). Consider painting e.g. `Quests:\nerror — definitions {reason}` (or a suffix on the header) so a defs-only failure is visible; today the HUD silently falls back to raw quest ids from the progress snapshot. +1. ~~**Surface `definitions_sync_failed` on HUD** — `_quest_defs_error` is set in `_on_quest_definitions_sync_failed` but never read in `_render_quest_progress_label`. Recipe defs follow the visible pattern (`Recipes: error — {reason}` on **`CraftFeedbackLabel`**). Consider painting e.g. `Quests:\nerror — definitions {reason}` (or a suffix on the header) so a defs-only failure is visible; today the HUD silently falls back to raw quest ids from the progress snapshot.~~ **Done.** `_render_quest_progress_label` paints `definitions error — {reason}` when defs sync fails (during loading or alongside progress rows). -2. **Extract quest HUD wiring from `main.gd`** — ~250 lines of quest setup/render/accept logic were added to an already large composer. Per [godot-client-script-organization](../../.cursor/rules/godot-client-script-organization.md), a child **`quest_hud_controller.gd`** (signals in, label text out) would match NEO-110-era precedent deferral but reduce merge conflict surface before NEO-123 capstone work. Non-blocking for this story. +2. **Extract quest HUD wiring from `main.gd`** — ~250 lines of quest setup/render/accept logic were added to an already large composer. Per [godot-client-script-organization](../../.cursor/rules/godot-client-script-organization.md), a child **`quest_hud_controller.gd`** (signals in, label text out) would match NEO-110-era precedent deferral but reduce merge conflict surface before NEO-123 capstone work. Non-blocking for this story. **Deferred** to NEO-123 capstone or follow-up refactor. -3. **GdUnit: `quest_accept_failed` + accept-busy guard** — Client tests cover sync 404 and structured deny (`accepted: false`), but not transport/HTTP failure on accept (`quest_accept_failed`) or `request_accept` returning `false` when `_accept_busy`. Small additions would lock the error path NEO-122 manual QA step 9 exercises. +3. ~~**GdUnit: `quest_accept_failed` + accept-busy guard** — Client tests cover sync 404 and structured deny (`accepted: false`), but not transport/HTTP failure on accept (`quest_accept_failed`) or `request_accept` returning `false` when `_accept_busy`. Small additions would lock the error path NEO-122 manual QA step 9 exercises.~~ **Done.** `test_http_404_on_accept_emits_quest_accept_failed` and `test_request_accept_returns_false_when_accept_busy` in `quest_progress_client_test.gd`. ## Nits -1. Nit: `test_request_accept_emits_deny_with_reason_code` omits `await assert_signal(...)` used elsewhere; safe with the synchronous mock transport today, but inconsistent with `test_request_sync_emits_quest_progress_received`. +1. ~~Nit: `test_request_accept_emits_deny_with_reason_code` omits `await assert_signal(...)` used elsewhere; safe with the synchronous mock transport today, but inconsistent with `test_request_sync_emits_quest_progress_received`.~~ **Deferred.** Synchronous mock transport emits before `await assert_signal` can observe; deny test keeps sync capture (same pattern as `craft_client_test.gd`). -2. Nit: `_render_quest_progress_labels()` is a one-line wrapper around `_render_quest_progress_label()` — harmless; could inline unless more quest labels land in NEO-123. +2. ~~Nit: `_render_quest_progress_labels()` is a one-line wrapper around `_render_quest_progress_label()` — harmless; could inline unless more quest labels land in NEO-123.~~ **Done.** Wrapper removed; callers invoke `_render_quest_progress_label()` directly. 3. Nit: Shift+Q may offer **`prototype_quest_combat_intro`** before refine when gather is still active (documented in manual QA Notes) — server deny is authoritative; acceptable prototype UX.