局面集を使った連続対局機能 #1071
局面集を使った連続対局機能 #1071
Conversation
|
Warning Rate limit exceeded@sunfish-shogi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (21)
WalkthroughThis pull request introduces enhanced functionality for handling SFEN (Shogi Forsyth-Edwards Notation) files across multiple components of the application. The changes include new IPC handlers for selecting and loading SFEN files, updates to game settings to support position lists, localization additions for new UI elements, and modifications to the game management system to support flexible starting positions. Changes
Sequence DiagramsequenceDiagram
participant User
participant GameDialog
participant Bridge
participant Background
participant FileSystem
User->>GameDialog: Select "Position List"
GameDialog->>Bridge: showSelectSFENDialog()
Bridge->>Background: Request SFEN file selection
Background->>FileSystem: Open file dialog
FileSystem-->>Background: Return selected file path
Background-->>Bridge: Return file path
Bridge-->>GameDialog: Display selected file path
GameDialog->>Bridge: loadSFENFile(path)
Bridge->>Background: Request file contents
Background->>FileSystem: Read SFEN file
FileSystem-->>Background: Return file contents
Background-->>Bridge: Return SFEN positions
Bridge-->>GameDialog: Update game settings
Possibly related PRs
Suggested labels
Poem
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 #1071 +/- ##
==========================================
+ Coverage 76.45% 76.57% +0.11%
==========================================
Files 120 120
Lines 15361 15443 +82
Branches 2455 2476 +21
==========================================
+ Hits 11745 11826 +81
- Misses 3594 3595 +1
Partials 22 22 ☔ View full report in Codecov by Sentry. |
sunfish-shogi
force-pushed
the
add-start-position-list
branch
from
4532579 to
ac8526f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
src/renderer/store/game.ts (1)
139-152: SimplifymaxPositionsCalculationThe calculation of
maxPositionsin theresetmethod could be simplified for better readability. Currently, it usesMath.ceil(params.maxGames / (params.swapPlayers ? 2 : 1)), which might be confusing.Consider using a more straightforward calculation or adding comments to clarify the logic.
src/tests/common/settings/game.spec.ts (1)
33-35: Update Test Cases for New Game Settings PropertiesThe test settings now include
startPositionListFileandstartPositionListOrder. However, the test does not check scenarios where these properties have non-default values.Consider enhancing the test to include cases with actual file paths and different order settings to ensure that normalization handles all valid inputs appropriately.
src/renderer/view/menu/MobileGameMenu.vue (1)
85-86: Consider making start position list settings configurable.The hardcoded values for
startPositionListFileandstartPositionListOrdershould ideally be configurable through user preferences or game settings dialog, especially since this is a mobile menu component.src/common/settings/game.ts (3)
20-23: Consider translating comments to English for international collaboration.The Japanese comments should be translated to English to maintain consistency and improve collaboration with international contributors.
- | "current" // 現局面 - | "list"; // 局面集 + | "current" // current position + | "list"; // position list
20-23: Add JSDoc comments for the new type and properties.Consider adding comprehensive JSDoc comments to document:
- The purpose and usage of
GameStartPositionType- The meaning of each type variant
- The purpose of the new properties in
GameSettings/** * Represents the type of starting position for a game. * @since v1.21.0 */ export type GameStartPositionType = | InitialPositionType | "current" // current position | "list"; // position list export type GameSettings = { // ... other properties ... /** * The type of starting position to use for the game. * @since v1.21.0 */ startPosition: GameStartPositionType; /** * Path to the SFEN file containing the list of starting positions. * Only used when startPosition is "list". */ startPositionListFile: string; /** * Order in which positions from the list should be used. * Only used when startPosition is "list". */ startPositionListOrder: "sequential" | "shuffle";Also applies to: 39-41
74-77: Add a comment explaining the version compatibility logic.The comment about v1.21.0 compatibility should be in English and provide more context about why this normalization is necessary.
- // v1.21.0 までは startPosition を省略可能で、それが現在の current に相当していた。 + // Prior to v1.21.0, startPosition was optional and its absence was equivalent to the current "current" value. + // This normalization ensures backward compatibility with older settings.src/renderer/ipc/bridge.ts (1)
53-54: Add JSDoc comments for the new Bridge methods.Consider adding comprehensive documentation for the new methods, including:
- Purpose of each method
- Parameter descriptions
- Return value descriptions
- Error handling behavior
/** * Shows a file dialog for selecting an SFEN file. * @param lastPath The last used path to set as the initial directory * @returns Promise resolving to the selected file path or rejecting if selection was cancelled * @throws {Error} If the dialog cannot be shown */ showSelectSFENDialog(lastPath: string): Promise<string>; /** * Loads and parses an SFEN file. * @param path Path to the SFEN file to load * @returns Promise resolving to an array of SFEN strings from the file * @throws {Error} If the file cannot be read or parsed */ loadSFENFile(path: string): Promise<string[]>;src/renderer/ipc/preload.ts (1)
123-125: Consider adding error handling.While the implementation is correct, consider adding try-catch blocks to handle potential IPC errors, similar to how file operations might fail.
async loadSFENFile(path: string): Promise<string[]> { - return await ipcRenderer.invoke(Background.LOAD_SFEN_FILE, path); + try { + return await ipcRenderer.invoke(Background.LOAD_SFEN_FILE, path); + } catch (error) { + console.error('Failed to load SFEN file:', error); + throw error; + } }src/tests/renderer/store/game.spec.ts (1)
144-216: Add edge case tests for StartPositionList.The test coverage for StartPositionList could be improved by adding the following scenarios:
- Test with maxGames = 0
- Test with maxGames > number of available positions
- Test file path with special characters
it("StartPositionList/edge-cases", async () => { mockAPI.loadSFENFile.mockResolvedValueOnce([ "position startpos moves 2g2f", "position startpos moves 7g7f", ]); const list = new StartPositionList(); // Test maxGames = 0 await expect( list.reset({ filePath: "test.sfen", swapPlayers: false, order: "sequential", maxGames: 0, }), ).rejects.toThrow("Invalid maxGames: must be greater than 0"); // Test maxGames > available positions await list.reset({ filePath: "test.sfen", swapPlayers: false, order: "sequential", maxGames: 5, }); expect(list.next()).toBe("position startpos moves 2g2f"); expect(list.next()).toBe("position startpos moves 7g7f"); expect(list.next()).toBe("position startpos moves 2g2f"); // Test file path with special characters mockAPI.loadSFENFile.mockResolvedValueOnce([ "position startpos moves 2g2f", ]); await expect( list.reset({ filePath: "path with spaces/file#1.sfen", swapPlayers: false, order: "sequential", maxGames: 1, }), ).resolves.toBeUndefined(); });src/tests/mock/player.ts (1)
24-26: Improve error message for unexpected USI.The error message could be more descriptive by including the list of expected USI moves.
if (!m) { - throw new Error("unexpected USI: " + usi); + throw new Error( + `Unexpected USI: ${usi}. Expected one of: ${Object.keys(moves).join(", ")}` + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/background/window/ipc.ts(1 hunks)src/command/common/preload.ts(1 hunks)src/common/i18n/locales/en.ts(2 hunks)src/common/i18n/locales/ja.ts(2 hunks)src/common/i18n/locales/vi.ts(2 hunks)src/common/i18n/locales/zh_tw.ts(2 hunks)src/common/i18n/text_template.ts(2 hunks)src/common/ipc/channel.ts(1 hunks)src/common/settings/game.ts(4 hunks)src/renderer/ipc/api.ts(1 hunks)src/renderer/ipc/bridge.ts(1 hunks)src/renderer/ipc/preload.ts(1 hunks)src/renderer/ipc/web.ts(1 hunks)src/renderer/store/game.ts(6 hunks)src/renderer/view/dialog/GameDialog.vue(7 hunks)src/renderer/view/menu/MobileGameMenu.vue(1 hunks)src/tests/common/settings/game.spec.ts(1 hunks)src/tests/mock/game.ts(1 hunks)src/tests/mock/player.ts(1 hunks)src/tests/renderer/store/game.spec.ts(10 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/renderer/store/game.ts
[warning] 326-326: src/renderer/store/game.ts#L326
Added line #L326 was not covered by tests
🪛 Biome (1.9.4)
src/tests/renderer/store/game.spec.ts
[error] 18-19: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🔇 Additional comments (13)
src/common/i18n/text_template.ts (1)
280-280: LGTM! New text entries are well-structured.The new properties
positionListandbeginFromThisPositionare correctly added to theTextstype interface, maintaining proper alphabetical ordering and type consistency.Also applies to: 293-293
src/common/i18n/locales/ja.ts (1)
285-285: LGTM! Japanese translations are accurate and consistent.The translations
positionList: "局面集"andbeginFromThisPosition: "この局面から開始"are appropriate and maintain consistency with the application's Japanese language style.Also applies to: 298-298
src/common/i18n/locales/en.ts (1)
285-285: LGTM! English translations are clear and accurate.The translations
positionList: "Position List"andbeginFromThisPosition: "Begin from this position"are clear, grammatically correct, and effectively convey the intended meaning.Also applies to: 298-298
src/common/i18n/locales/vi.ts (2)
285-285: Translate the newpositionListentry to Vietnamese.The entry is currently using Japanese text "局面集" with a TODO comment. Please provide a Vietnamese translation to maintain language consistency.
Would you like me to help translate this entry? Based on the English translation "Position List" and the context of the application, I can suggest some Vietnamese translations.
298-298: Translate the newbeginFromThisPositionentry to Vietnamese.The entry is currently using Japanese text "この局面から開始" with a TODO comment. Please provide a Vietnamese translation to maintain language consistency.
Would you like me to help translate this entry? Based on the English translation "Begin from this position" and the context of the application, I can suggest some Vietnamese translations.
src/renderer/store/game.ts (2)
326-326: Increase Test Coverage for Error Handling PathThe line
this.startPositionList.clear();at line 326 is not covered by tests, as indicated by the static analysis tool.To ensure robustness, add a test case that triggers this error handling path in the
startmethod and verifies thatstartPositionList.clear()functions as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 326-326: src/renderer/store/game.ts#L326
Added line #L326 was not covered by tests
656-656: EnsurestartPositionListIs Cleared at Game EndAt line 656,
this.startPositionList.clear();is called when the game ends.It's good practice to reset the start position list when the game concludes to prevent residual data from affecting subsequent games.
src/tests/mock/game.ts (1)
48-49: Verify Default Values for New PropertiesThe properties
startPositionListFileandstartPositionListOrderare added with default values of an empty string and"sequential", respectively.Ensure that these default values are appropriate and that they won't cause issues when initializing games without explicit values. It might also be beneficial to set
startPositionListOrderto a constant or enum to prevent misuse.src/common/ipc/channel.ts (1)
19-20: LGTM! New IPC channels follow existing patterns.The new channels for SFEN dialog and file loading are well-named and properly integrated into the existing enum structure.
src/renderer/ipc/api.ts (1)
69-70: LGTM! The new SFEN file handling methods are well-defined.The method signatures are consistent with the existing file handling patterns in the API interface.
src/renderer/ipc/web.ts (1)
236-241: LGTM! Consistent error handling for web-unsupported features.The implementation follows the established pattern for features not available in the web application.
src/command/common/preload.ts (1)
170-175: LGTM! Consistent error handling for command-line-unsupported features.The implementation follows the established pattern for features not available in the command line tool.
src/renderer/ipc/preload.ts (1)
120-122: LGTM! The showSelectSFENDialog implementation follows IPC patterns.The method correctly uses ipcRenderer.invoke and maintains consistency with other dialog methods.
| @@ -279,6 +279,7 @@ export const zh_tw: Texts = { | |||
| swapSenteGote: "先後交換", | |||
| pieceToss: "振り駒", // TODO: translate | |||
| currentPosition: "現在局面", | |||
| positionList: "局面集", // TODO: translate | |||
There was a problem hiding this comment.
Translation needed for Traditional Chinese.
While positionList: "局面集" is appropriate, beginFromThisPosition: "この局面から開始" is in Japanese instead of Traditional Chinese. This needs to be translated to maintain consistency.
Consider translating it to something like: "從此局面開始" to match Traditional Chinese language style.
Also applies to: 295-295
There was a problem hiding this comment.
Please never mention the lines which have "TODO: translate" comment.
There was a problem hiding this comment.
@sunfish-shogi Understood. I'll exclude lines with "TODO: translate" comments from reviews.
✏️ Learnings added
Learnt from: sunfish-shogi
PR: sunfish-shogi/shogihome#1071
File: src/common/i18n/locales/zh_tw.ts:282-282
Timestamp: 2025-01-12T12:31:24.988Z
Learning: Lines marked with "TODO: translate" comment should be excluded from review comments as they are intentionally left untranslated.
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.
| export class StartPositionList { | ||
| private usiList: string[] = []; | ||
| private maxRepeat: number = 1; | ||
| private index = 0; | ||
| private repeat = 0; | ||
|
|
||
| clear(): void { | ||
| this.usiList = []; | ||
| this.maxRepeat = 1; | ||
| this.index = 0; | ||
| this.repeat = 0; | ||
| } | ||
|
|
||
| async reset(params: { | ||
| filePath: string; | ||
| swapPlayers: boolean; | ||
| order: "sequential" | "shuffle"; | ||
| maxGames: number; | ||
| }): Promise<void> { | ||
| // load SFEN file | ||
| const usiList = await api.loadSFENFile(params.filePath); | ||
| if (params.order === "shuffle") { | ||
| for (let i = usiList.length - 1; i > 0; i--) { | ||
| const j = Math.floor(Math.random() * (i + 1)); | ||
| [usiList[i], usiList[j]] = [usiList[j], usiList[i]]; | ||
| } | ||
| } | ||
| const maxPositions = Math.ceil(params.maxGames / (params.swapPlayers ? 2 : 1)); | ||
| if (usiList.length > maxPositions) { | ||
| usiList.length = maxPositions; | ||
| } | ||
|
|
||
| // validate USI | ||
| if (usiList.length === 0) { | ||
| throw new Error("No available positions in the list."); | ||
| } | ||
| for (let i = 0; i < usiList.length; i++) { | ||
| const record = Record.newByUSI(usiList[i]); | ||
| if (!(record instanceof Record)) { | ||
| throw new Error(`Invalid USI: ${record}: ${usiList[i]}`); | ||
| } | ||
| } | ||
|
|
||
| this.usiList = usiList; | ||
| this.maxRepeat = params.swapPlayers ? 2 : 1; | ||
| this.index = 0; | ||
| this.repeat = 0; | ||
| } | ||
|
|
||
| next(): string { | ||
| if (this.usiList.length === 0) { | ||
| return "position startpos"; | ||
| } | ||
| if (this.repeat < this.maxRepeat) { | ||
| this.repeat++; | ||
| } else { | ||
| this.index++; | ||
| this.repeat = 1; | ||
| if (this.index >= this.usiList.length) { | ||
| this.index = 0; | ||
| } | ||
| } | ||
| return this.usiList[this.index]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Unit Tests for StartPositionList Class
The newly introduced StartPositionList class is crucial for managing the list of starting positions, including file loading, shuffling, validation, and retrieval of positions. To ensure its reliability and correctness, especially given the error handling and randomization logic, it's important to add comprehensive unit tests covering all its methods (clear, reset, next).
Would you like assistance in creating these unit tests to improve code coverage and maintainability?
| const record = Record.newByUSI(usiList[i]); | ||
| if (!(record instanceof Record)) { | ||
| throw new Error(`Invalid USI: ${record}: ${usiList[i]}`); | ||
| } |
There was a problem hiding this comment.
Enhance Error Handling in USI Validation
In the reset method, the validation of USI strings might not handle exceptions thrown by Record.newByUSI(). If an invalid USI string causes an exception, it could result in an unhandled error.
Consider wrapping the USI parsing in a try-catch block to gracefully handle any exceptions and provide clearer error messages.
Apply this diff to improve error handling:
for (let i = 0; i < usiList.length; i++) {
- const record = Record.newByUSI(usiList[i]);
- if (!(record instanceof Record)) {
- throw new Error(`Invalid USI: ${record}: ${usiList[i]}`);
- }
+ try {
+ Record.newByUSI(usiList[i]);
+ } catch (e) {
+ throw new Error(`Invalid USI at index ${i}: ${e.message}`);
+ }
}📝 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.
| const record = Record.newByUSI(usiList[i]); | |
| if (!(record instanceof Record)) { | |
| throw new Error(`Invalid USI: ${record}: ${usiList[i]}`); | |
| } | |
| try { | |
| Record.newByUSI(usiList[i]); | |
| } catch (e) { | |
| throw new Error(`Invalid USI at index ${i}: ${e.message}`); | |
| } |
| const onSelectStartPositionListFile = async () => { | ||
| useBusyState().retain(); | ||
| try { | ||
| const sfenPath = await api.showSelectSFENDialog(startPositionListFile.value.value); | ||
| if (sfenPath) { | ||
| startPositionListFile.value.value = sfenPath; | ||
| } | ||
| } finally { | ||
| useBusyState().release(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file selection.
The onSelectStartPositionListFile method should handle potential errors that may occur during file selection.
const onSelectStartPositionListFile = async () => {
useBusyState().retain();
try {
const sfenPath = await api.showSelectSFENDialog(startPositionListFile.value.value);
if (sfenPath) {
startPositionListFile.value.value = sfenPath;
}
+ } catch (e) {
+ useErrorStore().add(e);
} finally {
useBusyState().release();
}
};📝 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.
| const onSelectStartPositionListFile = async () => { | |
| useBusyState().retain(); | |
| try { | |
| const sfenPath = await api.showSelectSFENDialog(startPositionListFile.value.value); | |
| if (sfenPath) { | |
| startPositionListFile.value.value = sfenPath; | |
| } | |
| } finally { | |
| useBusyState().release(); | |
| } | |
| }; | |
| const onSelectStartPositionListFile = async () => { | |
| useBusyState().retain(); | |
| try { | |
| const sfenPath = await api.showSelectSFENDialog(startPositionListFile.value.value); | |
| if (sfenPath) { | |
| startPositionListFile.value.value = sfenPath; | |
| } | |
| } catch (e) { | |
| useErrorStore().add(e); | |
| } finally { | |
| useBusyState().release(); | |
| } | |
| }; |
| </div> | ||
| <div v-show="startPosition === 'list'" class="form-item"> | ||
| <ToggleButton | ||
| label="シャッフル" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded Japanese text needs i18n.
The label "シャッフル" is hardcoded in Japanese. This should be internationalized using the translation system.
- label="シャッフル"
+ label="{{ t.shuffle }}"Committable suggestion skipped: line range outside the PR's diff.
| <!-- i18n --> | ||
| <button class="thin" @click="onSelectStartPositionListFile">選択</button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded Japanese text needs i18n.
The button label "選択" is hardcoded in Japanese. This should be internationalized using the translation system.
- <button class="thin" @click="onSelectStartPositionListFile">選択</button>
+ <button class="thin" @click="onSelectStartPositionListFile">{{ t.select }}</button>Committable suggestion skipped: line range outside the PR's diff.
| ipcMain.handle(Background.LOAD_SFEN_FILE, async (event, path: string): Promise<string[]> => { | ||
| validateIPCSender(event.senderFrame); | ||
| if (!path.endsWith(".sfen")) { | ||
| throw new Error(`${t.fileExtensionNotSupported}: ${path}`); | ||
| } | ||
| const data = await fs.readFile(path, "utf-8"); | ||
| return data | ||
| .replace(/^\uFEFF/, "") // remove BOM | ||
| .split(/[\r\n]+/) // split by line | ||
| .filter((line) => line !== ""); // remove empty lines | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance SFEN file validation and error handling.
The SFEN file loading logic could be improved in several ways:
- Add file existence check before reading
- Add file size limit check to prevent memory issues
- Validate SFEN format for each line
ipcMain.handle(Background.LOAD_SFEN_FILE, async (event, path: string): Promise<string[]> => {
validateIPCSender(event.senderFrame);
if (!path.endsWith(".sfen")) {
throw new Error(`${t.fileExtensionNotSupported}: ${path}`);
}
+ try {
+ const stats = await fs.stat(path);
+ if (!stats.isFile()) {
+ throw new Error(`${t.fileNotFound}: ${path}`);
+ }
+ if (stats.size > 10 * 1024 * 1024) { // 10MB limit
+ throw new Error(t.fileTooLarge);
+ }
const data = await fs.readFile(path, "utf-8");
- return data
+ const lines = data
.replace(/^\uFEFF/, "") // remove BOM
.split(/[\r\n]+/) // split by line
.filter((line) => line !== ""); // remove empty lines
+ // Validate SFEN format for each line
+ for (const line of lines) {
+ if (!line.startsWith("position ")) {
+ throw new Error(`${t.invalidSFENFormat}: ${line}`);
+ }
+ }
+ return lines;
+ } catch (e) {
+ if (e instanceof Error) {
+ throw e;
+ }
+ throw new Error(`${t.failedToLoadFile}: ${path}`);
+ }
});📝 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.
| ipcMain.handle(Background.LOAD_SFEN_FILE, async (event, path: string): Promise<string[]> => { | |
| validateIPCSender(event.senderFrame); | |
| if (!path.endsWith(".sfen")) { | |
| throw new Error(`${t.fileExtensionNotSupported}: ${path}`); | |
| } | |
| const data = await fs.readFile(path, "utf-8"); | |
| return data | |
| .replace(/^\uFEFF/, "") // remove BOM | |
| .split(/[\r\n]+/) // split by line | |
| .filter((line) => line !== ""); // remove empty lines | |
| }); | |
| ipcMain.handle(Background.LOAD_SFEN_FILE, async (event, path: string): Promise<string[]> => { | |
| validateIPCSender(event.senderFrame); | |
| if (!path.endsWith(".sfen")) { | |
| throw new Error(`${t.fileExtensionNotSupported}: ${path}`); | |
| } | |
| try { | |
| const stats = await fs.stat(path); | |
| if (!stats.isFile()) { | |
| throw new Error(`${t.fileNotFound}: ${path}`); | |
| } | |
| if (stats.size > 10 * 1024 * 1024) { // 10MB limit | |
| throw new Error(t.fileTooLarge); | |
| } | |
| const data = await fs.readFile(path, "utf-8"); | |
| const lines = data | |
| .replace(/^\uFEFF/, "") // remove BOM | |
| .split(/[\r\n]+/) // split by line | |
| .filter((line) => line !== ""); // remove empty lines | |
| // Validate SFEN format for each line | |
| for (const line of lines) { | |
| if (!line.startsWith("position ")) { | |
| throw new Error(`${t.invalidSFENFormat}: ${line}`); | |
| } | |
| } | |
| return lines; | |
| } catch (e) { | |
| if (e instanceof Error) { | |
| throw e; | |
| } | |
| throw new Error(`${t.failedToLoadFile}: ${path}`); | |
| } | |
| }); |
| vi.mock("@/renderer/ipc/api"); | ||
|
|
||
| export const mockAPI = api as Mocked<API>; |
There was a problem hiding this comment.
Remove test file exports.
The mockAPI export from a test file could lead to test interdependencies. Consider making it local to this file.
-export const mockAPI = api as Mocked<API>;
+const mockAPI = api as Mocked<API>;📝 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.
| vi.mock("@/renderer/ipc/api"); | |
| export const mockAPI = api as Mocked<API>; | |
| vi.mock("@/renderer/ipc/api"); | |
| const mockAPI = api as Mocked<API>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-19: Do not export from a test file.
(lint/suspicious/noExportsInTest)
sunfish-shogi
force-pushed
the
add-start-position-list
branch
from
ac8526f to
8aba567
Compare
sunfish-shogi
force-pushed
the
add-start-position-list
branch
from
8aba567 to
a8bf73d
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/renderer/view/dialog/GameDialog.vue (1)
456-466:⚠️ Potential issueAdd error handling for file selection.
The file selection handler should catch and handle potential errors.
Apply this diff to add error handling:
const onSelectStartPositionListFile = async () => { useBusyState().retain(); try { const sfenPath = await api.showSelectSFENDialog(startPositionListFile.value.value); if (sfenPath) { startPositionListFile.value.value = sfenPath; } + } catch (e) { + useErrorStore().add(e); } finally { useBusyState().release(); } };
🧹 Nitpick comments (3)
src/renderer/store/game.ts (3)
170-184: Improve error handling in the next method.The next() method silently returns "position startpos" when the list is empty, which could mask potential issues. Consider throwing an error instead.
Apply this diff:
next(): string { if (this.usiList.length === 0) { - return "position startpos"; + throw new Error("Position list is empty"); } if (this.repeat < this.maxRepeat) { this.repeat++; } else { this.index++; this.repeat = 1; if (this.index >= this.usiList.length) { this.index = 0; } } return this.usiList[this.index]; }
296-311: Add error handling for startPosition list initialization.The position list initialization could benefit from additional error handling and logging.
Apply this diff:
if (settings.startPosition === "list") { + try { await this.startPositionList.reset({ filePath: settings.startPositionListFile, swapPlayers: settings.swapPlayers, order: settings.startPositionListOrder, maxGames: settings.repeat, }); + } catch (e) { + throw new Error(`Failed to initialize position list: ${e.message}`); + } }
339-354: Add comments explaining position handling logic.The position handling switch case would benefit from additional comments explaining the logic for each case.
Apply this diff:
switch (this.settings.startPosition) { case "current": + // Restore to the starting move number for continuous games if (this.recordManager.record.current.ply !== this.startPly) { this.recordManager.changePly(this.startPly); this.recordManager.removeNextMove(); } break; case "list": + // Load next position from the position list and add a comment this.recordManager.importRecord(this.startPositionList.next(), { type: RecordFormatType.USI, markAsSaved: true, }); this.recordManager.updateComment(t.beginFromThisPosition); break; default: + // Reset to initial position for standard games this.recordManager.resetByInitialPositionType(this.settings.startPosition); }
🛑 Comments failed to post (1)
src/common/i18n/locales/vi.ts (1)
228-228:
⚠️ Potential issueMissing Vietnamese translations for new strings.
The following strings still contain Japanese text and need to be translated to Vietnamese:
shuffle: "シャッフル"→ Should be translated to VietnamesepositionList: "局面集"→ Should be translated to VietnamesebeginFromThisPosition: "この局面から開始"→ Should be translated to VietnameseAlso applies to: 286-286, 299-299
説明 / Description
#1002
チェックリスト / Checklist
npm testpassednpm run lintwas applied without warnings/docs/webappnot included (except release branch)console.lognot included (except script file)Summary by CodeRabbit
New Features
Localization
Improvements