[] Add ErrorBoundary, Skeleton loading states, and EmptyRepoState to repository pages with unit tests#304
Conversation
… to repository pages with unit tests
![orange-codens[bot]](https://avatars.githubusercontent.com/in/4028936?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍊 Orange Codens レビュー
重大な問題: App Router の Streaming SSR 環境では layout.tsx に配置した ErrorBoundary（Client Component）は Server Component のフェッチエラーをキャッチできないため、PRD が要求するエラーフォールバック UI として機能するには error.tsx の追加が必要です。また RepoFileTree 内で getContents と getCommits が逐次 await されており、並列化によるパフォーマンス改善が必要です（Promise.allSettled 推奨）。テスト面では Retry ボタンのクリック動作と EmptyRepoState の手順テキストの検証が欠落しています。
🔒 セキュリティ: 今回の差分における主要なセキュリティ上の懸念点は2点です。①ErrorBoundary が JavaScript ランタイムの生の error.message をそのままエンドユーザーに表示するため、内部情報漏洩（CWE-209）のリスクがあります。汎用メッセージへの差し替えと外部ロギングへの移行を推奨します。②EmptyRepoState の cloneUrl 展開は React の JSX テキストノード処理により現時点での XSS リスクは低いものの、将来的な <a href> 追加等に備えスキーム検証を追加することを推奨します。認可・SQL injection・SSRF 等の重篤な脆弱性は本 diff には確認されませんでした。
🔴 critical: 1 / 🟠 high: 1 / 🟡 medium: 6 / 🔵 low: 3
その他の指摘 (9 件)
- 🟡 [medium]
__tests__/components/ErrorBoundary.test.tsx:19— Retry ボタンのクリック動作（window.location.reload()）がテストされていない (confidence: 0.90,maintainability.test.errorboundary_no_retry_behavior_test) - 🟡 [medium]
app/(app)/[owner]/[repo]/page.tsx:194—NEXT_PUBLIC_API_URLが空文字の場合に clone URL が壊れた文字列になる (confidence: 0.80,code.null_safety.cloneurl_fallback_construction) - 🟡 [medium]
components/ErrorBoundary.tsx:39— エラーメッセージがそのままエンドユーザーに表示される (confidence: 0.80,sec.sast.info_disclosure.error_message) - 🟡 [medium]
components/ErrorBoundary.tsx:33—fallbackprop が指定された場合にエラーメッセージが表示されないが、テストは fallback なしケースしか検証していない (confidence: 0.75,code.logic.errorboundary_fallback_ignored_on_error) - 🟡 [medium]
app/(app)/[owner]/[repo]/blob/[branch]/[...path]/page.tsx:140—pathPartsがBlobViewerContentからBlobPageに移動されたが、BlobViewerContent内でdownloadUrlの Raw/Download ボタンが重複して表示される設計になっている (confidence: 0.70,code.logic.blobviewercontent_pathparts_unused) - 🟡 [medium]
components/repo/EmptyRepoState.tsx:13— cloneUrl が未サニタイズのまま JSX に直接展開されている (confidence: 0.55,sec.sast.injection.xss) - 🔵 [low]
__tests__/components/repo/EmptyRepoState.test.tsx:7—EmptyRepoStateテストがgit init/git remote add/git pushの手順テキストを検証していない (confidence: 0.90,maintainability.test.emptyrepostate_git_instructions_not_tested) - 🔵 [low]
app/(app)/[owner]/[repo]/commits/page.tsx:54—CommitListのrefParamがundefinedの場合にbuildPageHrefに渡されるが型がoptionalのまま (confidence: 0.55,maintainability.naming.commitlist_refparam_type) - 🔵 [low]
app/(app)/[owner]/[repo]/page.tsx:194— clone_url フォールバック生成に NEXT_PUBLIC_API_URL が使われ未検証のまま子コンポーネントに渡される (confidence: 0.45,sec.sast.open_redirect.clone_url_fallback)
head: 394a3a5 | Orange Codens (P1)
| children: React.ReactNode; | ||
| }) { | ||
| return <ErrorBoundary>{children}</ErrorBoundary>; | ||
| } |
There was a problem hiding this comment.
🔴 [critical] Server Component の layout.tsx が "use client" な ErrorBoundary を直接返しており、Next.js の Server/Client Component 境界に違反する可能性
該当箇所:
import ErrorBoundary from "@/components/ErrorBoundary";
export default function RepoLayout({
children,
}: {
children: React.ReactNode;
}) {
return <ErrorBoundary>{children}</ErrorBoundary>;
}ErrorBoundary は "use client" ディレクティブを持つ Client Component です。Next.js App Router では layout.tsx はデフォルトで Server Component であり、そのまま children（Server Component のツリー）を Client Component で包むことは可能ですが、layout.tsx 自体に "use client" が無いため、この layout が Server Component として扱われます。
React の Error Boundary は Client Component でなければならないため、この構成は一見問題ないように見えますが、App Router の Streaming SSR 環境では Server Component がスローするエラー（fetchエラー等）は Error Boundary ではなく Next.js の error.tsx でしかキャッチできません。ErrorBoundary がキャッチできるのはクライアントサイドのレンダリングエラーのみです。<Suspense> 内の async Server Component が throw したエラーは error.tsx に委譲されるため、このレイアウトで配置した ErrorBoundary は Server Component のフェッチエラーをキャッチしない点を認識した上で使う必要があります。PRD が「エラーフォールバック UI」を要求している場合、error.tsx も併せて必要です。
code.logic.errorboundary_layout_server_mismatch | confidence: 0.85
| if (isApiError(err) && err.status === 404) { | ||
| commitsEmpty = true; | ||
| } else { | ||
| throw err; |
There was a problem hiding this comment.
🟠 [high] RepoFileTree が contents と commits を逐次 await しており、並列化されていない
該当箇所:
async function RepoFileTree({ owner, repo, branch, cloneUrl }) {
let contentsRaw = null;
let contentsNotFound = false;
try {
contentsRaw = await apiClient.getContents<ContentItem[] | ContentItem>(
owner, repo, "", branch,
);
} catch (err) { ... }
let commitsEmpty = false;
try {
const result = await apiClient.getCommits<{ sha: string }[]>(
owner, repo, branch, 1,
);
commitsEmpty = result.commits.length === 0;
} catch (err) { ... }getContents と getCommits は互いに依存しないにもかかわらず逐次実行されています。ネットワーク往復が 2 回直列になりページ表示が遅延します。Promise.all でラップし並列実行すべきです。
const [contentsResult, commitsResult] = await Promise.allSettled([
apiClient.getContents(...),
apiClient.getCommits(...),
]);code.logic.repopageskeleton_double_fetch | confidence: 0.95
…op obsolete skeleton-loading (main has it)
There was a problem hiding this comment.
🍊 Orange Codens レビュー
重大: PRD / Task Goal で必須の Suspense + Skeleton loading が diff 上では一切実装されておらず、blob / commits ページの変更も欠落しています。中程度: repo overview の空リポジトリ判定はコミットリスト空チェックがなく、contents 404 を一律空 repo 扱いするため誤表示リスクがあります。ErrorBoundary / EmptyRepoState コンポーネント自体は Task 仕様どおり実装され、基本テストも追加されています。
🔒 セキュリティ: 本 PR は UI 向けの ErrorBoundary・EmptyRepoState・Suspense スケルトン追加が中心で、認可漏れ・インジェクション・SSRF・暗号誤用などの重大な論理脆弱性は diff 上確認されませんでした。唯一、ErrorBoundary が error.message をそのまま表示する点が CWE-209 に該当する情報漏洩リスク（低 severity）として残ります。
🟠 high: 1 / 🟡 medium: 3 / 🔵 low: 2
その他の指摘 (5 件)
- 🟡 [medium]
app/(app)/[owner]/[repo]/page.tsx:144— 空リポジトリ判定にコミットリスト空チェックが含まれていない (confidence: 0.85,code.logic.incomplete_empty_detection) - 🟡 [medium]
app/(app)/[owner]/[repo]/page.tsx:144— 空リポジトリ判定ロジックのテストが不足 (confidence: 0.82,maintainability.testing.missing_page_tests) - 🟡 [medium]
app/(app)/[owner]/[repo]/page.tsx:129— contents の 404 をすべて空リポジトリとして扱っている (confidence: 0.78,code.error_handling.404_conflated_with_empty_repo) - 🔵 [low]
components/ErrorBoundary.tsx:34— ErrorBoundary の fallback prop がテストされていない (confidence: 0.88,maintainability.testing.fallback_prop_untested) - 🔵 [low]
components/ErrorBoundary.tsx:24— ErrorBoundary が内部エラーメッセージをそのまま UI に表示する (confidence: 0.88,sec.sast.info_disclosure.verbose_error)
head: 998c26a | Orange Codens (P1)
| repo={repo} | ||
| treeRef={branch} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🟠 [high] PRD 要件の Suspense / Skeleton loading が未実装
Task Goal Step 3 では file-tree セクションを <Suspense fallback={<RepoPageSkeleton />}> でラップし、RepoPageSkeleton（3 行の <Skeleton>）を表示する必要があります。PR タイトルも Skeleton loading states を謳っていますが、diff 上の repo overview ページは Suspense / Skeleton の import もラップも追加されておらず、従来どおり同期的に描画されています。
該当箇所:
{isEmptyRepo ? (
<EmptyRepoState cloneUrl={cloneUrl} />
) : (
<FileTree
entries={entries}
owner={owner}
repo={repo}
treeRef={branch}
/>
)}同 PR の [PURPLE-MODIFIED-FILES] に blob/.../page.tsx と commits/page.tsx も含まれていますが、提供 diff にはいずれも含まれておらず、Skeleton loading の実装範囲が PR 説明より大幅に不足しています。
@@ -284,10 +284,18 @@
- {isEmptyRepo ? (
- <EmptyRepoState cloneUrl={cloneUrl} />
- ) : (
- <FileTree
- entries={entries}
- owner={owner}
- repo={repo}
- treeRef={branch}
- />
- )}
+ <Suspense fallback={<RepoPageSkeleton />}>
+ {isEmptyRepo ? (
+ <EmptyRepoState cloneUrl={cloneUrl} />
+ ) : (
+ <FileTree
+ entries={entries}
+ owner={owner}
+ repo={repo}
+ treeRef={branch}
+ />
+ )}
+ </Suspense>maintainability.requirements.incomplete_implementation | confidence: 0.95
1 participant
Summary
Type of Change
Testing
Checklist
Task Goal
Target Repository
Corevice/open-gitAll work for this task belongs in this repository. Do the implementation inside this repository's directory in the workspace; do not search or edit other repositories unless this task explicitly requires a cross-repo change.
Context
Repo, blob, tree, and commits pages have no Skeleton loading placeholders, no error fallback UI, and no empty-state when a repository has zero commits — all required by the PRD acceptance criteria. The
Skeletoncomponent is available from the previous task.Steps
components/ErrorBoundary.tsx— class component implementinggetDerivedStateFromErrorandcomponentDidCatch; state:{ hasError: boolean; message: string }; on error, render a centered card with the message and a<button onClick={() => window.location.reload()}>Retry</button>; in normal state, renderchildren; props:children: ReactNode, optionalfallback?: ReactNodecomponents/repo/EmptyRepoState.tsx— props:cloneUrl: string; render:<h2>No code yet</h2>, a<code>block showing the clone URL, and step-by-step instructions (git init,git remote add origin <url>,git push -u origin main)app/(app)/[owner]/[repo]/page.tsx— wrap the file-tree section in<Suspense fallback={<RepoPageSkeleton />}>; createRepoPageSkeletonas an inline function returning 3<Skeleton className="h-5 w-full" />rows; when the API response indicates an empty repository (contents returns 404 and/or commit list is empty), render<EmptyRepoState cloneUrl={repo.clone_url} />instead of the file treeapp/(app)/[owner]/[repo]/blob/[branch]/[...path]/page.tsx— wrap theBlobViewerin<Suspense fallback={<div className="space-y-2">{[...Array(8)].map((_, i) => <Skeleton key={i} className="h-4 w-full" />)}</div>}>app/(app)/[owner]/[repo]/commits/page.tsx— wrap the commit list in `<Suspense fallback={Changes Made
Implementing Error
Boundary, Empty
RepoState, Susp
ense skeleton
s, and tests
. Exploring the repository
structure and existing
files
.
Creating
the components
and updating
the
pages. Skeleton
is
imported
from
@/components /ui/skeletonper the task spec
.
Updating
the
three page files:
Restoring Raw/D
ownload buttons
in
the blob page by
moving them into the
suspended content component
.
Implemented
error
handling, loading states
, and empty-repo
UI for repository
pages.
**
New
components**
ErrorBoundary—class component
with
getDerived StateFromError/
componentDid Catch;shows error
message and a
Retry button that
reloads the page
Empty RepoState—shows
"No code yet
",
clone
URL in a
<code>block,and git init
/p
ush steps
app/( app)/[owner ]/[repo]/ layout.tsx—wraps all repo sub
-pages in
ErrorBoundary**Page updates
**
Repo
overview —
file
tree wrapped in
<Suspense>withRepoPageSkeleton(3 skeleton
rows); empty
repo (
contents 404 or
no
commits) shows
EmptyRepoState**
Blob page** —
BlobViewerfetch/re
nder moved into
async
BlobViewer Contentinside<Suspense>with8 skeleton rows
Commits
page —
commit list moved
into async
CommitListinside `<Suspense
**Tests
**
Error Boundary.test.tsx— asserts fallback
message and Retry button
when
child throws
EmptyRepoState .test.tsx—asserts heading and clone
URL
[P
URPLE-M
ODIFIED-FILES
]
components/Error
Boundary.tsx
components
/repo/EmptyRepo
State.tsx
app
/(app)/[
owner]/[repo
]/layout.tsx
app/(app)/
[owner]/[
repo]/page.tsx
app/(app
)/[owner]/
[repo]/blob
/[branch]/[
...path]/page
.tsx
app/(
app)/[owner
]/[repo]/
commits/page.tsx
tests/components
/ErrorBoundary.test
.tsx
__tests
__/components/repo/
EmptyRepoState.test
.tsx
[/P
URPLE-M
ODIFIED-FILES
]
Task ID:
019f0d98-fee4-794f-a6e6-52a0c7acca72Generated by Purple Codens