spec(frontend/timeline): ノートのJSON fetchに失敗した場合、リトライできるように #1386
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthroughノート取得機能にGuarded中央集約型フェッチメカニズムを導入しました。ノートJSON取得用のfetchNoteJson関数とFibonacciバックオフリトライロジック（最大3回、初期遅延100ms）を追加し、進行中のフェッチを追跡するペンディングマップを実装しました。直接フェッチから非同期リトライ対応フローへ制御フローを変更しました。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 (2)
packages/frontend/src/components/MkTimeline.vue (1)
139-154: エラーの無視について検討してください。実装は適切ですが、
catchブロックでエラーを完全に無視しています。最終的なリトライ失敗時に、デバッグ目的でログを記録することを検討してください。🔎 オプション: エラーログの追加
}).catch(() => { - // ignore + if (_DEV_) console.warn(`Failed to fetch minimized note after retries: ${data.id}`); }).finally(() => {packages/frontend/src/utility/retry.ts (1)
8-33:maxAttemptsが 0 の場合のエッジケースを検討してください。Fibonacci バックオフの実装は正確です。ただし、
maxAttemptsが 0 の場合、ループが実行されず、タスクを一度も試行せずに'Retry attempts exhausted.'エラーがスローされます。これが意図された動作であれば問題ありませんが、
maxAttempts >= 1を強制するか、型で制約を加えることを検討してください。🔎 オプション: 入力バリデーションの追加
export async function retryWithFibonacciBackoff<T>( task: () => Promise<T>, options: RetryOptions, ): Promise<T> { + if (options.maxAttempts < 1) { + throw new Error('maxAttempts must be at least 1'); + } + let nextDelayMultiplier = 1;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/frontend/src/components/MkTimeline.vuepackages/frontend/src/utility/retry.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: lint (backend)
- GitHub Check: lint (misskey-reversi)
- GitHub Check: lint (frontend)
- GitHub Check: lint (misskey-js)
- GitHub Check: lint (frontend-shared)
- GitHub Check: typecheck (backend)
- GitHub Check: Agent
- GitHub Check: docker
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/frontend/src/components/MkTimeline.vue (3)
73-73: LGTM!リトライユーティリティのインポートが適切に追加されています。
156-172: LGTM!
fulfillNoteDataの変更は適切です。即時フェッチの試行とリトライのスケジューリングを分離することで、呼び出し元がブロックされずに処理を継続できます。nullを返すことで、呼び出し元（prepend関数）が適切にハンドリングできています。
122-137: 実装は適切です。Authorization: 'anonymous'ヘッダーは、このコードベース全体で使用されている標準パターンです。
pendingNoteFetchesマップによる重複フェッチ防止は効果的です。また、credentials: 'include'とAuthorization: 'anonymous'の組み合わせは意図された設計であり、クッキーを送信しつつ、サーバーに対して明示的に未認証リクエストであることを示す適切なパターンです。エラーハンドリングも正しく実装されています。packages/frontend/src/utility/retry.ts (1)
1-6: LGTM!
RetryOptions型定義とsleepヘルパー関数は簡潔で適切な実装です。
There was a problem hiding this comment.
Pull request overview
This PR removes the SPDX license header from the retry utility file and deletes its associated test file. Additionally, despite the description stating no functional changes, the PR integrates retry logic with fibonacci backoff into the MkTimeline component for handling minimized note fetches.
- Removed SPDX header from
packages/frontend/src/utility/retry.tsper repository maintainer instruction - Deleted the test file
packages/frontend/test/retry.test.ts - Added retry logic to
MkTimeline.vuefor fetching minimized notes with fibonacci backoff strategy
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/frontend/src/utility/retry.ts | Retry utility without SPDX header, implements fibonacci backoff with configurable attempts and delays |
| packages/frontend/src/components/MkTimeline.vue | Integrated retry utility for minimized note fetching, added fetchNoteJson, scheduleMinimizedNoteRetry functions, and pending fetch tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| throw new Error('Retry attempts exhausted.'); | ||
| } |
There was a problem hiding this comment.
The error message 'Retry attempts exhausted.' on line 32 is unreachable code. The function will either return successfully within the loop (line 17) or throw the original error when the max attempts are reached (line 20). The loop will never complete normally to reach line 32.
| throw new Error('Retry attempts exhausted.'); | |
| } | |
| } |
| const scheduleMinimizedNoteRetry = (data: { id: string }) => { | ||
| if (pendingNoteFetches.has(data.id)) return; | ||
| const retryPromise = retryWithFibonacciBackoff(() => fetchNoteJson(data.id), { | ||
| maxAttempts: 3, | ||
| initialDelayMs: 100, | ||
| }).then((noteData) => { | ||
| void prepend(deepMerge(data, noteData)); | ||
| }).catch(() => { | ||
| // ignore | ||
| }).finally(() => { | ||
| pendingNoteFetches.delete(data.id); | ||
| }); | ||
| pendingNoteFetches.set(data.id, retryPromise); | ||
| }; |
There was a problem hiding this comment.
The new retry logic in scheduleMinimizedNoteRetry and fulfillNoteData lacks test coverage. Since the repository uses comprehensive automated testing with vitest (as evidenced by multiple test files in packages/frontend/test/), the retry behavior should be covered by tests, especially given that a test file for the retry utility was removed.
| }).catch(() => { | ||
| // ignore |
There was a problem hiding this comment.
Silent error handling with empty catch block on lines 147-149 makes debugging difficult. Consider logging the error or providing some indication that the retry failed, especially since this is a user-facing feature where failed note fetches could impact the user experience.
| }).catch(() => { | |
| // ignore | |
| }).catch((error) => { | |
| console.error('Failed to fetch minimized note after retries:', data.id, error); |
| pendingNoteFetches.set(data.id, retryPromise); | ||
| }; | ||
| async function fulfillNoteData(data) { |
There was a problem hiding this comment.
The function parameter 'data' on line 156 lacks type annotation. Adding explicit type information would improve type safety and make the code more maintainable, especially since this parameter is used with properties like 'data.visibility' and 'data.id'.
| export type RetryOptions = { | ||
| maxAttempts: number; | ||
| initialDelayMs: number; | ||
| }; | ||
|
|
||
| const sleep = (ms: number) => new Promise<void>(resolve => setTimeout(resolve, ms)); | ||
|
|
||
| export async function retryWithFibonacciBackoff<T>( | ||
| task: () => Promise<T>, | ||
| options: RetryOptions, | ||
| ): Promise<T> { |
There was a problem hiding this comment.
The retryWithFibonacciBackoff function and RetryOptions type lack documentation comments. Adding JSDoc comments would help users understand the purpose, parameters, and behavior of the retry mechanism, especially the fibonacci backoff strategy.
u1-liquid
changed the title
Motivation
packages/frontend/test/retry.test.tsas requested.MkTimeline.vueunchanged in this change.Description
packages/frontend/src/utility/retry.ts.packages/frontend/test/retry.test.ts.MkTimeline.vuein this commit; this is a housekeeping change.Testing
packages/frontend/test/retry.test.ts) is no longer present, so its assertions are not run.vitest) in CI or locally to validate behavior after these removals.Codex Task
Summary by CodeRabbit
リリースノート
新機能
改善
✏️ Tip: You can customize this high-level summary in your review settings.