-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: 1Password初期化の並列化と非同期サービス設定の実装 #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 複数の1Passwordシークレットを並列で取得するように改善 - service.jsがPromiseを返すように変更し、非同期初期化に対応 - バッチ処理で一度の認証で全てのシークレットを取得 - キャッシュ機能で重複した取得を防止 - ローディング画面を追加し、初期化中のUXを改善 - Playwrightテストを非同期初期化に対応 - 初期化時間を大幅に短縮(約13秒→数秒)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements parallel initialization of 1Password secrets and asynchronous service configuration to improve application startup performance. The initialization time is reduced from approximately 13 seconds to just a few seconds.
- Parallelize 1Password CLI calls with batch processing and session management
- Implement asynchronous service initialization with Promise-based loading
- Add loading screen and update tests to handle async initialization
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/integration/app.play.js | Add helper function and test cases for async initialization, including loading screen verification |
| src/browser/service-instance.js | Refactor service loading to support async initialization with Promise handling |
| src/browser/App.js | Add loading screen component and async initialization logic in componentDidMount |
| ); | ||
|
|
||
| // サービスの初期化状態は別管理(ServiceStoreの変更で上書きされないように) | ||
| this._serviceInitialized = false; |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using instance variable _serviceInitialized instead of component state can lead to inconsistent rendering. Consider using this.state.serviceInitialized instead to ensure proper React lifecycle management and re-rendering.
| try { | ||
| await waitForInitialization(); | ||
| this._serviceInitialized = true; | ||
| this.forceUpdate(); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using forceUpdate() is generally discouraged in React as it bypasses the normal component lifecycle. Consider using setState() to trigger re-renders in a more predictable way.
| this._serviceInitialized = false; | ||
| } | ||
|
|
||
| componentDidMount() { | ||
| async componentDidMount() { | ||
| // サービスの初期化を待つ | ||
| try { | ||
| await waitForInitialization(); | ||
| this._serviceInitialized = true; | ||
| this.forceUpdate(); | ||
| } catch (error) { | ||
| console.error("Failed to initialize services:", error); | ||
| // エラーが発生しても表示(シークレットがフォールバック値になる) | ||
| this._serviceInitialized = true; | ||
| this.forceUpdate(); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using forceUpdate() is generally discouraged in React as it bypasses the normal component lifecycle. Consider using setState() to trigger re-renders in a more predictable way.
| this._serviceInitialized = false; | |
| } | |
| componentDidMount() { | |
| async componentDidMount() { | |
| // サービスの初期化を待つ | |
| try { | |
| await waitForInitialization(); | |
| this._serviceInitialized = true; | |
| this.forceUpdate(); | |
| } catch (error) { | |
| console.error("Failed to initialize services:", error); | |
| // エラーが発生しても表示(シークレットがフォールバック値になる) | |
| this._serviceInitialized = true; | |
| this.forceUpdate(); | |
| // this._serviceInitialized is now managed in state as 'initialized' | |
| } | |
| async componentDidMount() { | |
| // サービスの初期化を待つ | |
| try { | |
| await waitForInitialization(); | |
| this.setState({ initialized: true }); | |
| } catch (error) { | |
| console.error("Failed to initialize services:", error); | |
| // エラーが発生しても表示(シークレットがフォールバック値になる) | |
| this.setState({ initialized: true }); |
|
|
||
| // ローディング画面の存在を確認(すぐに確認) | ||
| const loadingText = window.locator("text=Loading..."); | ||
| const isLoadingVisible = await loadingText.isVisible().catch(() => false); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using .catch(() => false) to handle potential exceptions silently can mask real errors. Consider using a more explicit try-catch block or checking if the element exists first.
| const isLoadingVisible = await loadingText.isVisible().catch(() => false); | |
| const isLoadingVisible = await loadingText.isVisible(); |
概要
変更内容
1. 1Password CLI呼び出しの並列化
2. 非同期初期化への対応
service.jsがPromiseを返すように変更waitForInitialization関数でサービス初期化を待機3. テストの更新
パフォーマンス改善
🤖 Generated with Claude Code