Fix USI monitor #1099
Fix USI monitor #1099
Conversation
WalkthroughThe changes refactor the USI (Universal Shogi Interface) event handling by replacing the old best-move handler with a new search initiation handler. In the players module, type definitions, variables, and functions have been renamed to reflect this focus. Similar updates are applied in the store and monitoring modules, where iteration data is renamed and streamlined. Additionally, view components have been updated to pass and display the new monitor object appropriately, and corresponding test assertions have been adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as USI Player
participant Handler as onStartSearch Handler
participant Store as USI Store
participant Monitor as USI Monitor
participant View as EngineAnalytics View
Player->>Handler: startSearch(sessionID, position)
Handler->>Store: update session data
Store->>Monitor: add/update USIInfo in infoList
Monitor->>View: refresh monitor display
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1099 +/- ##
==========================================
- Coverage 78.45% 78.44% -0.01%
==========================================
Files 122 122
Lines 15891 15893 +2
Branches 2605 2606 +1
==========================================
Hits 12468 12468
- Misses 3404 3406 +2
Partials 19 19 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/store/index.ts (1)
476-482: Simplify nested ternary operators for better readabilityThe computation of
scoreuses nested ternary operators, which can be difficult to read and maintain. Consider refactoring using if-else statements or simplifying the ternary expressions for improved readability.Apply this diff to refactor the code:
- const score = - info.score !== undefined - ? info.score - : info.scoreMate - ? info.scoreMate > 0 - ? 1e8 - info.scoreMate - : -1e8 - info.scoreMate - : undefined; + let score; + if (info.score !== undefined) { + score = info.score; + } else if (info.scoreMate !== undefined) { + if (info.scoreMate > 0) { + score = 1e8 - info.scoreMate; + } else { + score = -1e8 - info.scoreMate; + } + }src/renderer/store/usi.ts (1)
113-113: Add unit tests for new code pathsThe newly added lines are not covered by unit tests, as indicated by the code coverage report. To ensure the reliability and correctness of these new code paths, consider adding unit tests that cover these scenarios.
Also applies to: 116-116, 135-135, 138-138, 154-154, 160-160
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 113-113: src/renderer/store/usi.ts#L113
Added line #L113 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/renderer/players/usi.ts(6 hunks)src/renderer/store/index.ts(5 hunks)src/renderer/store/usi.ts(7 hunks)src/renderer/view/tab/EngineAnalytics.vue(1 hunks)src/renderer/view/tab/EngineAnalyticsElement.vue(9 hunks)src/tests/renderer/store/index.spec.ts(7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/renderer/store/usi.ts
[warning] 113-113: src/renderer/store/usi.ts#L113
Added line #L113 was not covered by tests
[warning] 116-116: src/renderer/store/usi.ts#L116
Added line #L116 was not covered by tests
[warning] 135-135: src/renderer/store/usi.ts#L135
Added line #L135 was not covered by tests
[warning] 138-138: src/renderer/store/usi.ts#L138
Added line #L138 was not covered by tests
[warning] 154-154: src/renderer/store/usi.ts#L154
Added line #L154 was not covered by tests
[warning] 160-160: src/renderer/store/usi.ts#L160
Added line #L160 was not covered by tests
🔇 Additional comments (8)
src/renderer/view/tab/EngineAnalytics.vue (1)
8-8: Property binding updated successfullyThe change from
:info="monitor"to:monitor="monitor"correctly reflects the updated prop name in theEngineAnalyticsElementcomponent.src/renderer/players/usi.ts (3)
16-35: LGTM! The new handler type and implementation look good.The change from best-move handling to search initiation is a good architectural decision that provides better control over the search process.
76-156: LGTM! Consistent integration of search initiation handler.The
onStartSearchhandler is correctly integrated across all search methods, maintaining consistency in how searches are initiated and monitored.
177-212: LGTM! Best move handling looks good.The method properly handles all move types (normal moves, resign, win) and includes appropriate error handling for invalid moves.
src/renderer/view/tab/EngineAnalyticsElement.vue (2)
1-125: LGTM! Consistent renaming of info prop to monitor.The template has been updated to consistently use the new
monitorprop name throughout, improving code clarity.
127-255: LGTM! Script section properly updated for monitor prop.All computed properties and methods have been correctly updated to use the new
monitorprop and associated types.src/tests/renderer/store/index.spec.ts (2)
140-269: LGTM! Tests properly updated for new property names.The test assertions have been correctly updated to use
infoListandlatestInfo, maintaining test coverage while reflecting the new data structure.
271-302: LGTM! Tests updated for renamed method and properties.The test has been properly updated to use the renamed
endUSIIterationmethod andinfoListproperty.
src/renderer/store/usi.ts
Outdated
| // USI プロトコルにおいて nodes は読み筋と関係なく定期的に送る事ができるとされている。 | ||
| // ただ、多くのエンジンが読み筋と一緒に送ってくるため読み筋等がある場合にはそちらにも記録する。 | ||
| if (update.nodes !== undefined) { | ||
| iteration.nodes = update.nodes; | ||
| info.nodes = update.nodes; | ||
| } | ||
| this.iterations.unshift(iteration); | ||
| this.infoList.unshift(info); | ||
| } |
There was a problem hiding this comment.
Conditional check may always be true
The condition if (Object.keys(info).length !== 0) will always be true because info is initialized with required properties (id, position, color). If the intention is to check for any additional data before pushing info into infoList, consider refining the condition.
Apply this diff to correct the condition:
- if (Object.keys(info).length !== 0) {
+ // Check if any meaningful data has been added beyond the required fields
+ if (Object.keys(info).length > 3) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // USI プロトコルにおいて nodes は読み筋と関係なく定期的に送る事ができるとされている。 | |
| // ただ、多くのエンジンが読み筋と一緒に送ってくるため読み筋等がある場合にはそちらにも記録する。 | |
| if (update.nodes !== undefined) { | |
| iteration.nodes = update.nodes; | |
| info.nodes = update.nodes; | |
| } | |
| this.iterations.unshift(iteration); | |
| this.infoList.unshift(info); | |
| } | |
| // USI プロトコルにおいて nodes は読み筋と関係なく定期的に送る事ができるとされている。 | |
| // ただ、多くのエンジンが読み筋と一緒に送ってくるため読み筋等がある場合にはそちらにも記録する。 | |
| if (update.nodes !== undefined) { | |
| info.nodes = update.nodes; | |
| } | |
| // Check if any meaningful data has been added beyond the required fields | |
| if (Object.keys(info).length > 3) { | |
| this.infoList.unshift(info); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: src/renderer/store/usi.ts#L160
Added line #L160 was not covered by tests
sunfish-shogi
force-pushed
the
fix-usi-monitor
branch
from
fa31800 to
fb48813
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/renderer/players/usi.ts(6 hunks)src/renderer/store/index.ts(5 hunks)src/renderer/store/usi.ts(7 hunks)src/renderer/view/tab/EngineAnalytics.vue(1 hunks)src/renderer/view/tab/EngineAnalyticsElement.vue(9 hunks)src/tests/renderer/store/index.spec.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/view/tab/EngineAnalytics.vue
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/renderer/store/usi.ts
[warning] 114-114: src/renderer/store/usi.ts#L114
Added line #L114 was not covered by tests
[warning] 117-117: src/renderer/store/usi.ts#L117
Added line #L117 was not covered by tests
[warning] 136-136: src/renderer/store/usi.ts#L136
Added line #L136 was not covered by tests
[warning] 139-139: src/renderer/store/usi.ts#L139
Added line #L139 was not covered by tests
[warning] 155-155: src/renderer/store/usi.ts#L155
Added line #L155 was not covered by tests
[warning] 161-161: src/renderer/store/usi.ts#L161
Added line #L161 was not covered by tests
[warning] 166-167: src/renderer/store/usi.ts#L166-L167
Added lines #L166 - L167 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
src/renderer/store/usi.ts (2)
5-20: LGTM! The type definition is well-structured.The
USIInfotype is well-defined with all necessary properties for USI (Universal Shogi Interface) information handling. The properties are properly typed and optional where appropriate.
172-174: LGTM! The method is simplified.The
endIterationmethod is simplified by removing the unusedsfenparameter and focusing on its core responsibility.src/renderer/players/usi.ts (3)
16-16: LGTM! The handler type is well-defined.The
onStartSearchHandlertype is properly defined with the necessary parameters for handling search initiation.
26-31: LGTM! The handlers are properly initialized.The handlers are initialized with empty functions as default implementations.
91-91: LGTM! The search methods consistently notify about search initiation.The search methods (
startSearch,startPonder,startMateSearch,startResearch) consistently callonStartSearchwith the current session ID and position.Also applies to: 132-132, 146-146, 155-155
src/renderer/view/tab/EngineAnalyticsElement.vue (3)
57-58: LGTM! The v-for directive is properly updated.The template correctly iterates over either
monitor.infoListormonitor.latestInfobased on the history mode.
147-147: LGTM! The prop type is properly defined.The
monitorprop is properly typed asUSIPlayerMonitorand marked as required.
188-213: LGTM! The preview method is properly updated.The
showPreviewmethod correctly handles theUSIInfotype and properly extracts all necessary information for the preview dialog.src/tests/renderer/store/index.spec.ts (2)
158-162: LGTM! The test assertions are properly updated.The test assertions correctly validate the new
infoListstructure and its properties.
254-254: Add test coverage for missing code paths.Several lines in
src/renderer/store/usi.tsare not covered by tests according to the static analysis hints.Consider adding test cases for:
- Setting
selectiveDepth- Setting
timeMs- Setting
lowerBoundandupperBound- Setting
textfromstringproperty- Early return when no meaningful data is added
- Setting
nodesin infosrc/renderer/store/index.ts (4)
59-59: LGTM! Import statement updated to reflect new USI handler.The import statement has been correctly updated to use the new search initiation handler, aligning with the architectural changes.
209-210: LGTM! Constructor updated to use new USI handlers.The constructor correctly sets up the new USI event handlers, maintaining proper separation of concerns.
533-535: LGTM! USI iteration handling simplified.The method has been simplified by removing the position parameter and delegating to the monitor's
endIterationmethod.
465-498: LGTM! Score calculation and iteration handling improved.The changes include:
- Proper null checks and type guards for info properties
- Improved position validation
- Clear score calculation logic with mate score handling
| // 要素が何もない場合はリストに登録しない。 | ||
| if (Object.keys(info).length === baseInfoKeyCount) { | ||
| return; | ||
| } | ||
| // USI プロトコルにおいて nodes は読み筋と関係なく定期的に送る事ができるとされている。 | ||
| // ただ、多くのエンジンが読み筋と一緒に送ってくるため読み筋等がある場合にはそちらにも記録する。 | ||
| if (update.nodes !== undefined) { | ||
| info.nodes = update.nodes; | ||
| } | ||
|
|
||
| this.infoList.unshift(info); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the condition for adding info to the list.
The condition Object.keys(info).length === baseInfoKeyCount checks if only the required fields are present. However, this might miss cases where some fields are undefined but still counted in Object.keys().
Consider using a more explicit check:
- if (Object.keys(info).length === baseInfoKeyCount) {
+ // Check if any meaningful data has been added beyond the required fields
+ if (!info.depth && !info.selectiveDepth && !info.timeMs && !info.nodes &&
+ !info.score && !info.scoreMate && !info.pv && !info.text) {I'll start by gathering more context—specifically, I want to inspect how baseInfoKeyCount is defined and how info is populated in the file. This will help us determine if the current check might indeed allow an info object with only "undefined" extra fields to pass the condition.
Below is a shell script to search for occurrences of baseInfoKeyCount in the repository and to show the surrounding snippet (lines 150-180) from src/renderer/store/usi.ts.
I'll review the output to verify whether the condition using Object.keys(info).length === baseInfoKeyCount might indeed be problematic as described in the review comment.
🏁 Script executed:
#!/bin/bash
# Show where baseInfoKeyCount is referenced/defined in the repository.
rg "baseInfoKeyCount" -n .
# Display snippet from src/renderer/store/usi.ts to see the context and usage of `info`
sed -n '150,180p' src/renderer/store/usi.tsLength of output: 900
I'll gather more context regarding the creation of the info object to see which keys are always defined and whether any could be accidentally added with an undefined value. This will help us verify if the current check using Object.keys(info).length === baseInfoKeyCount is sufficiently precise or if we need to switch to a more explicit validation.
Below is a shell script to locate the definition of info and to inspect the relevant code snippet.
🏁 Script executed:
#!/bin/bash
# Search for the definition or assignment of the `info` object in the file.
rg "info\s*=\s*{" -n src/renderer/store/usi.ts
# Display the code snippet where `info` is defined, along with nearby context.
sed -n '90,120p' src/renderer/store/usi.tsLength of output: 1027
I'll gather further context by locating the definition of the USIInfo type to see what optional fields exist and whether their values may be undefined. This will help us assess if the current check using Object.keys(info).length === baseInfoKeyCount might let through entries with only undefined “meaningful” fields.
Below is a shell script to search for USIInfo in the repository:
🏁 Script executed:
#!/bin/bash
# Search for the definition of the USIInfo type in the codebase.
rg "type USIInfo" -n .Length of output: 136
I'll inspect the definition of USIInfo to see what keys it contains and if any fields might be undefined or treated as optional. This will help us verify whether the current condition is appropriately filtering out entries that only have required fields with potential undefined extra fields.
Below is a shell script to display the definition of USIInfo from src/renderer/store/usi.ts.
🏁 Script executed:
#!/bin/bash
# Display the definition of USIInfo from src/renderer/store/usi.ts.
sed -n '1,30p' src/renderer/store/usi.tsLength of output: 849
Action Required: Fix Missing Check for Nodes Property
Our investigation shows that the current condition relies on the key count of the info object—using Object.keys(info).length === baseInfoKeyCount—which is computed before the optional nodes property is added. As a result, if an update only contains a nodes value (with all other optional fields undefined), the check incorrectly returns early and discards the info update. An explicit check that includes nodes (and the other optional fields) is needed to ensure that any meaningful extra data (including nodes) isn’t mistakenly filtered out.
- The required fields are
id,position, andcolor(with abaseInfoKeyCountof 3). - Other properties (like
depth,selectiveDepth,timeMs,score,scoreMate,pv,text) are conditionally added before the check. - The
nodesfield is added after the early return. Thus, ifnodesis the only extra field provided, the object still has only 3 keys at the time of the check and is erroneously skipped. - We recommend either moving the assignment of
nodesbefore the check or performing an explicit property existence check (while carefully handling falsy valid values) to correctly capture any update that includesnodes.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 161-161: src/renderer/store/usi.ts#L161
Added line #L161 was not covered by tests
[warning] 166-167: src/renderer/store/usi.ts#L166-L167
Added lines #L166 - L167 were not covered by tests
There was a problem hiding this comment.
Nodes property should be assigned with least one other propety.
There was a problem hiding this comment.
@sunfish-shogi Understood, thank you for the clarification. The current implementation correctly ensures that nodes are only recorded when accompanied by other meaningful properties.
✏️ Learnings added
Learnt from: sunfish-shogi
PR: sunfish-shogi/shogihome#1099
File: src/renderer/store/usi.ts:159-170
Timestamp: 2025-02-06T16:37:32.561Z
Learning: In the USI monitor implementation, the nodes property should only be recorded when at least one other meaningful property (depth, selectiveDepth, timeMs, score, etc.) is present in the info object. This prevents recording nodes-only updates.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
説明 / Description
チェックリスト / Checklist
npm testpassednpm run lintwas applied without warnings/docs/webappnot included (except release branch)console.lognot included (except script file)Summary by CodeRabbit
Refactor
Tests