Repository navigation
fix(wear): ask the client's own bind-on-wear question - #2575
Conversation
A bind-on-wear item now prompts with qnai CannotTradeAskWearing, which the client renders in its own language, instead of a qna carrying a server-side string. The ASK_BIND resource is no longer used and is removed. Closes #1645 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gSmkUMqSCt7JBTq4HTzuc
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Update bind-on-wear confirmation src/NosCore.GameObject/Messaging/Handlers/UseItem/WearHandler.cs, test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/WearHandlerTests.cs, src/NosCore.Data/Enumerations/I18N/LanguageKey.cs, src/NosCore.Data/Resource/LocalizedResources*.resx, documentation/manual-test-plan.md |
WearHandler now sends a QnaiPacket with CannotTradeAskWearing instead of a localized QnaPacket. The test checks the question and its yes response, which uses UseItemPacket mode 1. The old ASK_BIND key and translations are removed. The manual test plan covers accepting or declining the prompt. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: denislauri1999
Merge Risk: 
74159
-
The bind-on-wear confirmation flow is correctly localized and preserves the existing acceptance behavior.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: using the client's own bind-on-wear question when wearing an item. Linked Issues check Linked issue #1645reports that WearHandler uses the wrong packet for the bind-on-wear prompt. The official packet isqnai #wear^44^0^1 1195 0 0 0. The PR changes WearHandler to send QnaiPacket with…Out of Scope Changes check All changes support the linked objective or its direct cleanup. WearHandler.cs changes the prompt packet to qnai with 1195. WearHandlerTests.cs covers the new packet and the absence of qna. Removing A… Full details: Linked Issues check
-
Explanation
Linked issue
#1645reports that WearHandler uses the wrong packet for the bind-on-wear prompt. The official packet isqnai #wear^44^0^1 1195 0 0 0. The PR changes WearHandler to send QnaiPacket with Game18NConstString.CannotTradeAskWearing (1195), which matches the question packet type and question id of the official packet. WearHandlerTests checks for a qnai with 1195, a yes response using u_i mode 1, and no qna. The author reports a browser check in which the client showed its own question, and the Confirm and Cancel paths behaved as expected. The yes-response callback in the official packet (#wear^44^0^1) is not implemented. The PR keepsu_i ... 1. The PR description states that this needs a third field on WearPacket in NosCore.Packets and is deferred. The issue does not list this callback as a separate requirement, so it does not change the verdict for the core packet objective. Evidence used: PR summary, PR description, and the changed-file summaries. No repository inspection was made.
Full details: Out of Scope Changes check
-
Explanation
All changes support the linked objective or its direct cleanup. WearHandler.cs changes the prompt packet to qnai with 1195. WearHandlerTests.cs covers the new packet and the absence of qna. Removing ASK_BIND from LanguageKey, the .resx files, and the designer is required cleanup, because the string is no longer used after the WearHandler change. manual-test-plan.md adds two manual checks for the changed behavior. These are documentation for the change and are not unrelated work. The file src/NosCore.Data/Resource/LocalizedResources.Designer.cs is in ignored_files, so its diff was not reviewed. This does not affect the verdict, because the designer removal is cleanup and not a stated requirement. The PR description says the designer removal is part of the change, and the author reports a successful build with 0 warnings and 0 errors. No unrelated behavior change was found in the summarized diff.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Requested by Erwan · project thread
Closes #1645
What changed
Wearing an unbound item that binds on wear used to prompt with a
qnathat carried a server-side English string (ASK_BIND). Now it sendsqnaiwithGame18NConstString.CannotTradeAskWearing(1195), so the client renders its own question in its own language. This matches the trace in the issue:qnai … 1195 0 0 0.WearHandlernow sendsQnaiPacketwithCannotTradeAskWearing.ASK_BINDis no longer used, so it is removed fromLanguageKey, every.resxand the designer.WearHandlerTestsnow checks for aqnaiwith 1195, au_iyes packet in mode 1, and noqna.documentation/manual-test-plan.mdhas two new checks under Equipment gates.Left as is: the yes packet is still
u_i … 1(mode 1), which still goes through the same equip path. The trace in the issue shows#wear^slot^type^1instead. That needs a third field onWearPacketin NosCore.Packets, so it is not part of this PR.How it was verified
dotnet build NosCore.sln: 0 warnings, 0 errors.dotnet test: Core, Database, WebApi, GameObject (647) and PacketHandlers (428) all pass.Before (master): the server's English string.
After (this branch): the client's own question.
After, Confirm: the fairy is worn.
After, Cancel: the fairy stays in the inventory.
Still needs a human (manual-test-plan.md → Equipment gates)
Both new checks passed in the browser client, in English only:
$CreateIteman item that binds when worn and double-click it: the client's own "cannot be traded after wearing" question opens, in the client's language. Checked with an English client. Other languages were not checked.A Windows client in another language would confirm the "in the client's language" part.
🤖 Generated with Claude Code
https://claude.ai/code/session_014gSmkUMqSCt7JBTq4HTzuc
Summary by CodeRabbit