Stochastic Ponder 使用時の表示と周辺実装の改善 #1096
Conversation
WalkthroughThis pull request updates several USI-related modules. In the settings module, a new constant and function are added to support and evaluate a stochastic pondering option. The USI player module now uses refined type signatures and state management, replacing an old pondering flag with a new ponder move mechanism and adding a getter for stochastic pondering. In the store modules, method signatures have been modified to use position information and an optional ponder move. Additionally, an asynchronous delay has been introduced in the monitor’s endIteration process. Changes
Suggested labels
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/renderer/store/usi.ts (1)
254-258: Specify an explicit timeout value for clarity.The setTimeout call doesn't specify a delay value. While it defaults to 0ms, it's better to be explicit for maintainability.
- setTimeout(() => { + setTimeout(() => { this.dequeue(); // flush the queue monitor.endIteration(sfen); - }); + }, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/common/settings/usi.ts(2 hunks)src/renderer/players/usi.ts(12 hunks)src/renderer/store/index.ts(3 hunks)src/renderer/store/usi.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
src/renderer/players/usi.ts (4)
16-27: LGTM! Type signatures improved for better type safety.The changes enhance type safety by:
- Using ImmutablePosition instead of string for position data
- Adding explicit Move type for ponder moves
48-48: LGTM! Improved ponder state management.Replacing the boolean
inPonderwithponderMove?: Moveis a good improvement as it:
- Provides more information about what move is being pondered
- Makes the code more explicit and type-safe
- Helps prevent state synchronization issues
Also applies to: 90-96, 111-113
129-131: LGTM! Clean implementation of stochastic ponder support.The changes properly integrate stochastic pondering by:
- Only doing the ponder move if stochastic pondering is disabled
- Adding a clean getter to access the engine setting
Also applies to: 329-331
186-186: LGTM! Consistent handler implementation updates.The changes properly update all handlers to use the new parameter types, maintaining consistency throughout the codebase.
Also applies to: 219-219, 270-270
src/common/settings/usi.ts (1)
14-14: LGTM! Clean implementation of stochastic ponder settings.The changes follow the established patterns in the codebase:
- Consistent naming for the option constant
- Implementation matches other option getter functions
- Proper type safety and null checks
Also applies to: 94-97
src/renderer/store/index.ts (3)
60-60: LGTM! Simplified imports.Removing the separate ponder info handler import aligns with the unified handler approach.
461-463: LGTM! Proper filtering of pondering sessions.The change correctly excludes sessions that are currently pondering from contributing to the candidates list.
534-535: LGTM! Consistent method signature updates.The changes properly update the method signatures to use ImmutablePosition and include ponderMove, maintaining consistency with the changes in the USI player implementation.
Also applies to: 538-544
sunfish-shogi
force-pushed
the
fix-for-stochastic-ponder
branch
from
c43838b to
59d0559
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1096 +/- ##
==========================================
+ Coverage 77.86% 78.45% +0.59%
==========================================
Files 122 122
Lines 15932 15891 -41
Branches 2587 2605 +18
==========================================
+ Hits 12406 12468 +62
+ Misses 3507 3404 -103
Partials 19 19 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tests/renderer/store/index.spec.ts (1)
Line range hint
138-182: Consider extracting the SFEN string to a constant.The test case correctly validates the updateUSIInfo functionality with the new position-based API. However, the readability could be improved by extracting the repeated SFEN string to a constant at the top of the test file.
+const TEST_POSITION_SFEN = "lnsgkgsnl/1r5b1/ppppppppp/9/9/2P6/PP1PPPPPP/1B5R1/LNSGKGSNL w - 2"; + it("updateUSIInfo", () => { vi.useFakeTimers(); const usi = "position startpos moves 7g7f"; - const position = Position.newBySFEN( - "lnsgkgsnl/1r5b1/ppppppppp/9/9/2P6/PP1PPPPPP/1B5R1/LNSGKGSNL w - 2", - ) as ImmutablePosition; + const position = Position.newBySFEN(TEST_POSITION_SFEN) as ImmutablePosition;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/common/settings/usi.ts(2 hunks)src/renderer/players/usi.ts(12 hunks)src/renderer/store/index.ts(3 hunks)src/renderer/store/usi.ts(1 hunks)src/tests/renderer/store/index.spec.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/store/usi.ts
- src/renderer/players/usi.ts
- src/renderer/store/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (5)
src/common/settings/usi.ts (2)
14-14: LGTM!The new constant follows the established naming convention and is well-placed among other option name constants.
94-97: LGTM!The function follows the established pattern of other getter functions and uses the existing helper correctly.
src/tests/renderer/store/index.spec.ts (3)
3-3: LGTM!The import statement correctly includes the required types from the tsshogi library.
Line range hint
188-262: LGTM!The test case correctly validates the multipv functionality with the new position-based API. The assertions properly verify the behavior of multiple PV lines and their updates.
269-310: LGTM!The test case correctly validates the candidates functionality with the new position-based API. The assertions properly verify the behavior of candidate moves with different settings and engines.
sunfish-shogi
force-pushed
the
fix-for-stochastic-ponder
branch
from
59d0559 to
2adc376
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/renderer/players/usi.ts (1)
129-133: Add comments explaining stochastic pondering behavior.The conditional position update based on stochastic pondering would benefit from documentation explaining:
- Why position is not updated during stochastic pondering
- The implications of this behavior on the engine's analysis
Add comments above the condition:
+ // In stochastic pondering mode, we don't update the position as the engine + // needs to analyze multiple possible positions probabilistically if (!this.stochasticPonder) { this.position.doMove(ponderMove); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/common/settings/usi.ts(2 hunks)src/renderer/players/usi.ts(12 hunks)src/renderer/store/index.ts(3 hunks)src/renderer/store/usi.ts(3 hunks)src/tests/renderer/store/index.spec.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/settings/usi.ts
- src/renderer/store/usi.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/renderer/store/index.ts
[warning] 461-462: src/renderer/store/index.ts#L461-L462
Added lines #L461 - L462 were not covered by tests
src/renderer/players/usi.ts
[warning] 219-219: src/renderer/players/usi.ts#L219
Added line #L219 was not covered by tests
[warning] 365-365: src/renderer/players/usi.ts#L365
Added line #L365 was not covered by tests
🔇 Additional comments (6)
src/renderer/players/usi.ts (3)
16-27: LGTM! Type signature changes improve type safety.The changes from string-based USI to strongly-typed ImmutablePosition enhance type safety and make the code more maintainable.
90-97: LGTM! Improved pondering state management.The changes improve state management by:
- Using explicit ponderMove state instead of a boolean flag
- Properly cleaning up the ponderMove state after search
329-331: LGTM! Clean interface for stochastic pondering setting.The getter provides a clean way to access the engine's stochastic pondering setting.
src/tests/renderer/store/index.spec.ts (1)
266-297: LGTM! Comprehensive test coverage for USI info iteration.The new test case thoroughly verifies:
- Proper state management during USI info iterations
- Correct handling of refresh flags
- Multiple engine scenarios
src/renderer/store/index.ts (2)
533-543: LGTM! Clean method signatures for USI info handling.The updated method signatures using ImmutablePosition and optional ponderMove parameter improve type safety and clarity.
460-462: Add test coverage for pondering session filtering.The pondering session filtering logic in the candidates getter lacks test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 461-462: src/renderer/store/index.ts#L461-L462
Added lines #L461 - L462 were not covered by tests
sunfish-shogi
force-pushed
the
fix-for-stochastic-ponder
branch
2 times, most recently
from
b9c4c34 to
8c59249
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/background/usi/engine.ts (1)
627-630: LGTM! Clean refactoring of info handling.The consolidation of info handling into a single path simplifies the code while maintaining functionality. The use of optional chaining (
?.) provides safe callback handling.Consider extracting the info handling logic into a separate method for better maintainability:
- if (this.infoCallback) { - const args = command.substring(5); - this.infoCallback?.(this.currentPosition, parseInfoCommand(args)); - } + this.handleInfoCommand(command.substring(5)); + } + } + + private handleInfoCommand(args: string): void { + this.infoCallback?.(this.currentPosition, parseInfoCommand(args));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/background/usi/engine.ts(1 hunks)src/background/usi/index.ts(1 hunks)src/background/window/ipc.ts(0 hunks)src/command/common/preload.ts(0 hunks)src/common/ipc/channel.ts(0 hunks)src/common/settings/usi.ts(2 hunks)src/renderer/ipc/bridge.ts(0 hunks)src/renderer/ipc/preload.ts(0 hunks)src/renderer/ipc/setup.ts(0 hunks)src/renderer/ipc/web.ts(0 hunks)src/renderer/players/usi.ts(12 hunks)src/renderer/store/index.ts(3 hunks)src/renderer/store/usi.ts(3 hunks)src/tests/background/usi/engine.spec.ts(2 hunks)src/tests/common/settings/usi.spec.ts(4 hunks)src/tests/renderer/store/index.spec.ts(7 hunks)
💤 Files with no reviewable changes (7)
- src/renderer/ipc/web.ts
- src/common/ipc/channel.ts
- src/renderer/ipc/setup.ts
- src/command/common/preload.ts
- src/renderer/ipc/preload.ts
- src/renderer/ipc/bridge.ts
- src/background/window/ipc.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/common/settings/usi.ts
- src/background/usi/index.ts
- src/renderer/store/usi.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/renderer/players/usi.ts
[warning] 219-219: src/renderer/players/usi.ts#L219
Added line #L219 was not covered by tests
src/renderer/store/index.ts
[warning] 461-462: src/renderer/store/index.ts#L461-L462
Added lines #L461 - L462 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
src/tests/background/usi/engine.spec.ts (1)
238-238: LGTM! Test updates match the refactored implementation.The test expectations have been correctly updated to use the consolidated
infohandler instead of the removedponderInfohandler, maintaining test coverage while aligning with the simplified event handling in the engine implementation.Also applies to: 350-350
src/renderer/players/usi.ts (5)
6-6: LGTM!The import of
getUSIEngineStochasticPonderis correctly added to support the new stochastic ponder functionality.
16-28: LGTM! Type changes improve type safety.The handler type changes enhance type safety by:
- Using
ImmutablePositioninstead of string for position data- Adding optional
ponderMoveparameter toonUpdateUSIInfoHandler
48-48: LGTM! Improved ponder state tracking.Replacing
inPonderboolean withponderMove?: Moveenhances the pondering state management by:
- Providing more information about which move is being pondered
- Improving type safety with the Move type
129-131: LGTM! Conditional position update for stochastic ponder.The position update is correctly guarded by the stochastic ponder check, ensuring proper handling of the stochastic pondering feature.
329-331: LGTM! Clean getter implementation.The
stochasticPondergetter is correctly implemented to retrieve the stochastic ponder setting from the engine.src/tests/common/settings/usi.spec.ts (1)
268-328: LGTM! Comprehensive test coverage for stochastic ponder.The test cases for
getUSIEngineStochasticPonderare well-structured and cover all important scenarios:
- Explicit false value
- Explicit true value
- Default value inheritance
- Non-existent option handling
src/tests/renderer/store/index.spec.ts (2)
Line range hint
138-182: LGTM! Test cases properly updated for new API.The
updateUSIInfotest cases are well-updated to:
- Use Position object instead of string
- Test ponderMove functionality
- Maintain comprehensive assertions
266-297: LGTM! Well-structured test for endUSIInfoIteration.The new test case thoroughly verifies the iteration ending functionality:
- Tests iteration ending for specific engines
- Verifies refresh flag behavior
- Checks multiple engine updates
src/renderer/store/index.ts (3)
59-59: LGTM! Simplified imports.Removed
setOnUpdateUSIPonderInfoHandleras ponder information is now handled through the main USI info handler.
460-462: Add test coverage for ponderMove check in candidates method.The ponderMove check is correctly implemented but lacks test coverage.
Please add test cases to verify the behavior when
session.ponderMoveis set. This will help ensure the skip logic works as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 461-462: src/renderer/store/index.ts#L461-L462
Added lines #L461 - L462 were not covered by tests
533-543: LGTM! Improved type safety in USI info handling.The changes enhance type safety and clarity by:
- Using Position instead of string
- Adding ponderMove parameter
- Maintaining consistent parameter ordering
sunfish-shogi
force-pushed
the
fix-for-stochastic-ponder
branch
from
8c59249 to
cf994b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/tests/mock/mate.ts (1)
4-13: Consider adding JSDoc documentation for the mock engine configuration.The mock engine configuration is well-structured with all required fields. Consider adding JSDoc documentation to explain its purpose in tests.
+/** + * Mock USI engine configuration for mate search tests. + * Uses a test engine with minimal configuration. + */ const engine: USIEngine = { uri: "es://usi/test-engine", name: "my usi engine", defaultName: "engine", author: "author", path: "/engines/engines", options: {}, labels: {}, enableEarlyPonder: false, };src/tests/renderer/players/usi.spec.ts (2)
134-134: Consider extracting common test position setup.The same position setup is duplicated across all three checkmate test cases. Consider extracting it into a shared constant or helper function to improve maintainability.
+const TEST_CHECKMATE_POSITION = "position sfen 3sks3/9/4+P4/9/7+B1/9/9/9/9 b S2rb4gs4n4l17p 1"; + it("checkmate", async () => { mockAPI.usiLaunch.mockResolvedValueOnce(100); mockAPI.usiGoMate.mockResolvedValueOnce(); - const usi = "position sfen 3sks3/9/4+P4/9/7+B1/9/9/9/9 b S2rb4gs4n4l17p 1"; + const usi = TEST_CHECKMATE_POSITION;Also applies to: 165-165, 193-193
139-145: Consider extracting common handler setup.The handler setup with mock functions is duplicated across all checkmate test cases. Consider extracting it into a helper function to reduce duplication and improve maintainability.
+function createCheckmateHandler() { + return { + onCheckmate: vi.fn(), + onNotImplemented: vi.fn(), + onTimeout: vi.fn(), + onNoMate: vi.fn(), + onError: vi.fn(), + }; +} + it("checkmate", async () => { // ... - const handler = { - onCheckmate: vi.fn(), - onNotImplemented: vi.fn(), - onTimeout: vi.fn(), - onNoMate: vi.fn(), - onError: vi.fn(), - }; + const handler = createCheckmateHandler();Also applies to: 170-176, 198-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/background/usi/engine.ts(1 hunks)src/background/usi/index.ts(1 hunks)src/background/window/ipc.ts(0 hunks)src/command/common/preload.ts(0 hunks)src/common/ipc/channel.ts(0 hunks)src/common/settings/usi.ts(2 hunks)src/renderer/ipc/bridge.ts(0 hunks)src/renderer/ipc/preload.ts(0 hunks)src/renderer/ipc/setup.ts(0 hunks)src/renderer/ipc/web.ts(0 hunks)src/renderer/players/usi.ts(12 hunks)src/renderer/store/index.ts(3 hunks)src/renderer/store/usi.ts(3 hunks)src/tests/background/usi/engine.spec.ts(2 hunks)src/tests/common/settings/usi.spec.ts(4 hunks)src/tests/mock/mate.ts(1 hunks)src/tests/renderer/players/usi.spec.ts(2 hunks)src/tests/renderer/store/index.spec.ts(10 hunks)
💤 Files with no reviewable changes (7)
- src/renderer/ipc/bridge.ts
- src/common/ipc/channel.ts
- src/background/window/ipc.ts
- src/renderer/ipc/setup.ts
- src/command/common/preload.ts
- src/renderer/ipc/preload.ts
- src/renderer/ipc/web.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/common/settings/usi.ts
- src/background/usi/index.ts
- src/renderer/store/usi.ts
- src/renderer/players/usi.ts
- src/tests/background/usi/engine.spec.ts
- src/tests/renderer/store/index.spec.ts
- src/renderer/store/index.ts
🔇 Additional comments (9)
src/tests/common/settings/usi.spec.ts (3)
15-15: LGTM!The new import is correctly placed and follows the existing import pattern.
139-139: LGTM! Good test code cleanup.The engine object simplification improves test readability by including only the necessary properties for testing. The type assertion is appropriately used to satisfy TypeScript's type checking.
Also applies to: 201-201, 263-263
268-328: LGTM! Comprehensive test coverage.The test cases for
getUSIEngineStochasticPonderare well-structured and cover all important scenarios:
- Explicit false value
- Explicit true value
- Default true value
- Missing option (false)
The implementation follows the established patterns in the codebase.
src/tests/mock/mate.ts (1)
15-17: LGTM!The mate search settings correctly wrap the engine configuration.
src/background/usi/engine.ts (1)
627-630: LGTM! Simplified info command handling.The changes streamline the info command processing by:
- Using a single callback for all states
- Directly passing the current position and parsed info
- Removing the redundant ponder-specific handling
This simplification aligns with the PR objectives of improving reading line processing efficiency.
src/tests/renderer/players/usi.spec.ts (4)
2-9: LGTM! Well-organized imports.The new checkmate-related handlers are properly imported and grouped with other USI-related imports.
131-160: LGTM! Comprehensive test for successful checkmate scenario.The test thoroughly verifies:
- Correct initialization and API calls
- Proper handling of checkmate moves
- Validation that other handlers aren't called
162-188: LGTM! Good coverage of not-implemented scenario.The test properly verifies the behavior when checkmate search is not implemented by the engine.
190-216: LGTM! Well-structured test for no-mate scenario.The test effectively verifies the behavior when no checkmate is found in the position.
説明 / Description
#993
チェックリスト / Checklist
npm testpassednpm run lintwas applied without warnings/docs/webappnot included (except release branch)console.lognot included (except script file)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests