fix: Queryとloaderの非同期状態を統一 (#578) - #591
Closed
hmjn023 wants to merge 4 commits into
Closed
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughQueryClient設定、query key生成、QueryUiState変換を共通化し、Server/Tauriのloaderをprefetch中心へ変更しました。検索・一覧APIへAbortSignalを伝播し、各画面の状態表示とクエリ無効化キーを共通定義へ統一しています。 Changes共通Query基盤
Server側Query統合
Tauri側Query統合
UIフックと画面
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/client/src/api-error.test.ts (1)
1-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CORS_ERROR/UNKNOWNコードのテストが未カバー。
APIErrorのcodeは5種類あるが、テストはNETWORK_ERROR/TIMEOUT（true）とSERVER_ERROR（false）のみを検証している。CORS_ERRORとUNKNOWNの判定結果もアサートしておくと、意図しない仕様変更を早期に検知できる。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/api-error.test.ts` around lines 1 - 18, The isTransientApiError test coverage is incomplete: APIError codes CORS_ERROR and UNKNOWN are not asserted. Update the api-error.test.ts cases around isTransientApiError to include explicit expectations for APIError with CORS_ERROR and UNKNOWN, using the existing APIError and isTransientApiError symbols, so all five code paths are covered alongside the current NETWORK_ERROR, TIMEOUT, and SERVER_ERROR checks.packages/client/src/api-error.ts (1)
26-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winメッセージ文字列マッチによる誤分類のリスク
isTransientApiErrorはAPIError以外のErrorに対してmessageの部分文字列一致（"network","timeout","failed to fetch"）で一時的エラーかどうかを判定している。この関数はcreateAppQueryClientConfig(isTransientApiError)に渡され、QueryClient全体のリトライ可否を左右する（apps/tauri/src/router.tsx,apps/server/src/router.tsx参照）。バリデーションエラーやビジネスロジック上のエラーメッセージにたまたま "network" や "timeout" という単語が含まれる場合、本来リトライすべきでないエラーが誤って一時的エラーとして扱われ、不要なリトライが発生する可能性がある。
error.name === "TypeError"(fetchの典型的な失敗) など、より堅牢な判定基準の併用を検討してほしい。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/api-error.ts` around lines 26 - 40, isTransientApiError currently classifies non-APIError cases by loose message substring matches, which can mislabel business/validation errors as transient. Update isTransientApiError to use stronger checks in addition to APIError.code, such as inspecting error.name (for example TypeError from fetch failures) or other explicit failure signals, and reduce reliance on message.includes in packages/client/src/api-error.ts. Keep the behavior aligned with createAppQueryClientConfig(isTransientApiError) usage in the router setup so only genuinely retryable errors are treated as transient.apps/server/src/infrastructure/api-clients/queries/index.ts (1)
24-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
list系 queryOptions の重複パターンを共通化できます。
tagsQueryOptions〜configQueryOptionsの6関数はいずれも...utils.X.list.queryOptions(), queryKey: XQueryKeys.all(), ...defaultXQueryConfigという同一構造の繰り返しです。小さなヘルパーで抽出するとDRY性が向上します（apps/tauri側にも同様のパターンがあれば併せて統一を検討してください）。♻️ 提案リファクタ例
+function withListQueryConfig<T extends { queryKey: unknown }>( + options: T, + queryKey: readonly unknown[], + config: { staleTime: number }, +) { + return { ...options, queryKey, ...config }; +} + -export const tagsQueryOptions = () => ({ - ...utils.tags.list.queryOptions(), - queryKey: tagsQueryKeys.all(), - ...defaultTagsQueryConfig, -}); +export const tagsQueryOptions = () => + withListQueryConfig( + utils.tags.list.queryOptions(), + tagsQueryKeys.all(), + defaultTagsQueryConfig, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/infrastructure/api-clients/queries/index.ts` around lines 24 - 58, The six query option builders in the query options module all repeat the same `list.queryOptions()` plus `queryKey` plus default config pattern; extract that shared shape into a small helper and have `tagsQueryOptions`, `mediaSourcesQueryOptions`, `allProjectsQueryOptions`, `allCharactersQueryOptions`, `allIpsQueryOptions`, `allAuthorsQueryOptions`, and `configQueryOptions` delegate to it. Keep the existing behavior and type inference intact by wiring the helper around the relevant `utils.*` and `*QueryKeys` / `default*QueryConfig` values, and apply the same consolidation pattern anywhere similar in the `apps/tauri` side if present.apps/server/src/tests/e2e/pages.spec.ts (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win固定のsleepに依存したテストはCIでflakyになりやすい
waitForTimeout(500)の代わりに、page.waitForLoadState("networkidle")や設定フォームの表示を待つ（例:await page.getByRole(...).waitFor()）ようにすると、環境負荷によるタイミングのブレに強くなります。♻️ 提案例
await page.goto("/config"); - await page.waitForTimeout(CLIENT_QUERY_SETTLE_MS); + await page.waitForLoadState("networkidle"); expect(rpcRequestCount).toBeLessThanOrEqual(1);Also applies to: 26-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/tests/e2e/pages.spec.ts` at line 5, The e2e test in pages.spec.ts relies on a fixed CLIENT_QUERY_SETTLE_MS sleep and waitForTimeout(500), which makes it flaky under CI timing variance. Update the affected test flow to wait on a deterministic condition instead, such as page.waitForLoadState("networkidle") or waiting for the specific settings form element via page.getByRole(...).waitFor(), and remove the hardcoded timeout from the test logic.apps/server/src/routes/config.tsx (1)
29-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winpending/error表示に
roleが付与されていない
fetchState === "background-fetching"/"paused"のブロックにはrole="status"が付いていますが、pending（Line 29-31）とerror/offline（Line 33-39）のブロックには付与されていません。エラー・オフライン表示はユーザーへの通知として重要なため、スクリーンリーダーに伝わるようrole="status"（またはエラーはrole="alert"）を統一的に付けることを推奨します。♻️ 提案例
<Show when={state().phase === "pending"}> - <div class="py-10 text-center">Loading settings...</div> + <div class="py-10 text-center" role="status">Loading settings...</div> </Show> <Show when={state().phase === "error" || state().phase === "offline"}> - <div class="py-10 text-red-500"> + <div class="py-10 text-red-500" role="alert"> {state().phase === "offline" ? "Offline. Settings will load after reconnecting." : "Error loading settings."} </div> </Show>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/config.tsx` around lines 29 - 39, The pending and error/offline state messages in the config route are missing accessible live-region roles, unlike the existing background-fetching/paused status blocks. Update the conditional UI in config.tsx so the pending notice and the error/offline notice use an appropriate role consistently, using role="status" for general loading notifications and role="alert" if you want the error/offline message to be announced more urgently. Reference the existing Show blocks around state().phase and keep the change localized to those message containers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/tauri/src/queries/index.ts`:
- Around line 24-58: The query option helpers in queryOptions are reusing the
same queryKey across different queryFn implementations, which can cause cache
collisions and startup-order-dependent behavior. Review tagsQueryOptions,
mediaSourcesQueryOptions, allProjectsQueryOptions, allCharactersQueryOptions,
allIpsQueryOptions, allAuthorsQueryOptions, and configQueryOptions, plus any
related collection queries such as projectsForMediaQueryOptions. Make the key or
the fetcher consistent for each data source, so a given key always maps to one
implementation and shared keys like tagsQueryKeys.all() or
projectsQueryKeys.forMedia(...) are not bound to multiple backends.
---
Nitpick comments:
In `@apps/server/src/infrastructure/api-clients/queries/index.ts`:
- Around line 24-58: The six query option builders in the query options module
all repeat the same `list.queryOptions()` plus `queryKey` plus default config
pattern; extract that shared shape into a small helper and have
`tagsQueryOptions`, `mediaSourcesQueryOptions`, `allProjectsQueryOptions`,
`allCharactersQueryOptions`, `allIpsQueryOptions`, `allAuthorsQueryOptions`, and
`configQueryOptions` delegate to it. Keep the existing behavior and type
inference intact by wiring the helper around the relevant `utils.*` and
`*QueryKeys` / `default*QueryConfig` values, and apply the same consolidation
pattern anywhere similar in the `apps/tauri` side if present.
In `@apps/server/src/routes/config.tsx`:
- Around line 29-39: The pending and error/offline state messages in the config
route are missing accessible live-region roles, unlike the existing
background-fetching/paused status blocks. Update the conditional UI in
config.tsx so the pending notice and the error/offline notice use an appropriate
role consistently, using role="status" for general loading notifications and
role="alert" if you want the error/offline message to be announced more
urgently. Reference the existing Show blocks around state().phase and keep the
change localized to those message containers.
In `@apps/server/src/tests/e2e/pages.spec.ts`:
- Line 5: The e2e test in pages.spec.ts relies on a fixed CLIENT_QUERY_SETTLE_MS
sleep and waitForTimeout(500), which makes it flaky under CI timing variance.
Update the affected test flow to wait on a deterministic condition instead, such
as page.waitForLoadState("networkidle") or waiting for the specific settings
form element via page.getByRole(...).waitFor(), and remove the hardcoded timeout
from the test logic.
In `@packages/client/src/api-error.test.ts`:
- Around line 1-18: The isTransientApiError test coverage is incomplete:
APIError codes CORS_ERROR and UNKNOWN are not asserted. Update the
api-error.test.ts cases around isTransientApiError to include explicit
expectations for APIError with CORS_ERROR and UNKNOWN, using the existing
APIError and isTransientApiError symbols, so all five code paths are covered
alongside the current NETWORK_ERROR, TIMEOUT, and SERVER_ERROR checks.
In `@packages/client/src/api-error.ts`:
- Around line 26-40: isTransientApiError currently classifies non-APIError cases
by loose message substring matches, which can mislabel business/validation
errors as transient. Update isTransientApiError to use stronger checks in
addition to APIError.code, such as inspecting error.name (for example TypeError
from fetch failures) or other explicit failure signals, and reduce reliance on
message.includes in packages/client/src/api-error.ts. Keep the behavior aligned
with createAppQueryClientConfig(isTransientApiError) usage in the router setup
so only genuinely retryable errors are treated as transient.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0767ecf-0f99-4f69-aeed-9ce469774783
📒 Files selected for processing (51)
apps/server/src/infrastructure/api-clients/error-policy.tsapps/server/src/infrastructure/api-clients/queries/index.tsapps/server/src/infrastructure/api-clients/search-api.tsapps/server/src/router.tsxapps/server/src/routes/config.tsxapps/server/src/routes/manager.tsxapps/server/src/routes/search.tsxapps/server/src/routes/sources/$mediaSourceId/components/source-media-page.tsxapps/server/src/routes/sources/$mediaSourceId/index.tsxapps/server/src/routes/sources/index.tsxapps/server/src/tests/e2e/pages.spec.tsapps/tauri/src/collections/authors-collection.tsapps/tauri/src/collections/characters-collection.tsapps/tauri/src/collections/ips-collection.tsapps/tauri/src/collections/projects-collection.tsapps/tauri/src/collections/sources-collection.tsapps/tauri/src/collections/tags-collection.tsapps/tauri/src/infrastructure/api-clients/projects-api.tsapps/tauri/src/infrastructure/api-clients/search-api.tsapps/tauri/src/queries/index.tsapps/tauri/src/router.tsxapps/tauri/src/routes/config.tsxapps/tauri/src/routes/manager.tsxapps/tauri/src/routes/search.tsxapps/tauri/src/routes/sources/$mediaSourceId/$mediaId/index.tsxapps/tauri/src/routes/sources/$mediaSourceId/index.tsxapps/tauri/src/routes/sources/index.tsxpackages/client/src/api-error.test.tspackages/client/src/api-error.tspackages/client/src/create-client.test.tspackages/client/src/create-client.tspackages/client/src/index.tspackages/ui/src/hooks/use-manager-page.tspackages/ui/src/hooks/use-search-page.tspackages/ui/src/hooks/use-source-media-page.tspackages/ui/src/media-sidebar-content.tsxpackages/ui/src/query-options/index.tspackages/ui/src/query-options/prefetch.tspackages/ui/src/query-options/query-client.test.tspackages/ui/src/query-options/query-client.tspackages/ui/src/query-options/search-query.test.tspackages/ui/src/query-options/search-query.tspackages/ui/src/query-options/sources-query.tspackages/ui/src/query-state.test.tspackages/ui/src/query-state.tspackages/ui/src/screens/manager-screen.tsxpackages/ui/src/screens/media-detail-screen.tsxpackages/ui/src/screens/search-screen.tsxpackages/ui/src/screens/source-media-screen.tsxpackages/ui/src/screens/sources-screen.tsxpackages/ui/src/source-media-page.tsx
💤 Files with no reviewable changes (1)
- apps/server/src/infrastructure/api-clients/error-policy.ts
Owner
Author
|
CodeRabbit 指摘に対応しました。
検証:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
Queryとroute loaderの非同期状態をServer/Tauriで統一し、補助データ取得中もApp Shellとページ本体を先に表示できるようにします。
変更内容
検証
fixes #578
Summary by CodeRabbit