Skip to content

Conversation

@Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Feb 27, 2025

内容

タイトルのとおりです。
store/project.tsstore/project/index.tsに移動し、ついでに関数をファイル切り出しします。

このPRには提案が3つ含まれてます:

  • SAVE_PROJECT_*系3つのユースケースを別々に関数に分け、if分岐をなくすリファクタリング
  • 例外を引数に受け取って良い感じのメッセージに変えてダイアログを表示してくれるshowErrorDialog関数
  • storeごとにディレクトリを切って、ディレクトリ内で定義した関数をそのstoreから使うことで関数切り出しをやりやすくする案

その他

エラーメッセージはこんな感じ

image

@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Feb 27, 2025

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:b597935

@Hiroshiba Hiroshiba marked this pull request as ready for review February 28, 2025 10:21
@Hiroshiba Hiroshiba requested a review from a team as a code owner February 28, 2025 10:21
@Hiroshiba
Copy link
Member Author

いけてそうだったのでdraft開けました!!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR refactors the project saving functionality by extracting related functions into a dedicated helper file and reorganizing store actions and types to remove conditionals and improve clarity.

  • Separates the SAVE_PROJECT_* use cases into distinct functions in saveProjectHelper.ts
  • Introduces showErrorDialog for common error display and simplifies error message handling in ResultError
  • Updates store directories and action names for improved maintainability and consistency

Reviewed Changes

File Description
src/store/project/saveProjectHelper.ts New helper functions for project saving logic
src/store/project/index.ts Store actions now reference the new helper functions and updated action names
src/components/Dialog/Dialog.ts Adds an exported showErrorDialog for handling error display
src/type/result.ts Modifies ResultError to use only the result code in the error message for brevity
src/store/type.ts Updates and adds TypeScript types for actions and action context
src/components/Talk/ToolBar.vue Updates store actions calls to match the new action names
src/components/Menu/MenuBar/MenuBar.vue Updates store actions calls to match the refactored action names

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.


constructor(public readonly result: FailureResult<E>) {
super(`${result.code}: ${result.error.message}`, { cause: result.error });
super(`${result.code}`, { cause: result.error });
Copy link

Copilot AI Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResultError now only includes the result code in its error message, which may reduce debugging clarity. Consider including additional error detail or the original message to aid in troubleshooting.

Suggested change
super(`${result.code}`, { cause: result.error });
super(`${result.code}: ${result.error.message}`, { cause: result.error });

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ごもっともなんだけど、errorToMessage関数に投げることを考えると、result.error.messagecause: error側でも現れるから、ここはエラーコードだけをメッセージにするのが良いかなぁと。

@Hiroshiba Hiroshiba removed the request for review from sevenc-nanashi March 18, 2025 02:42
Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

特に問題なさそう。

…ルを保存する関数をユースケースに応じて3つに分割
@Hiroshiba
Copy link
Member Author

Hiroshiba commented Mar 18, 2025

レビューありがとうございます、マージします!!

これでstoreの関数を外に書き出しやすくなる!!!!

@Hiroshiba Hiroshiba enabled auto-merge March 18, 2025 03:41
@Hiroshiba Hiroshiba added this pull request to the merge queue Mar 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 18, 2025
@Hiroshiba Hiroshiba added this pull request to the merge queue Mar 18, 2025
Merged via the queue into VOICEVOX:main with commit e9d7605 Mar 18, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants