-
Notifications
You must be signed in to change notification settings - Fork 0
[] Add ErrorBoundary, Skeleton loading states, and EmptyRepoState to repository pages with unit tests #304
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
|
|
||
| import ErrorBoundary from "@/components/ErrorBoundary"; | ||
|
|
||
| function ThrowingChild(): never { | ||
| throw new Error("Something went wrong"); | ||
| } | ||
|
|
||
| describe("ErrorBoundary", () => { | ||
| it("renders fallback UI with message and Retry button when child throws", () => { | ||
| const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
|
|
||
| render( | ||
| <ErrorBoundary> | ||
| <ThrowingChild /> | ||
| </ErrorBoundary>, | ||
| ); | ||
|
|
||
| expect(screen.getByText("Something went wrong")).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "Retry" })).toBeInTheDocument(); | ||
|
|
||
| consoleError.mockRestore(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import EmptyRepoState from "@/components/repo/EmptyRepoState"; | ||
|
|
||
| describe("EmptyRepoState", () => { | ||
| it('renders "No code yet" heading and clone URL', () => { | ||
| const cloneUrl = "https://git.example.com/org/repo.git"; | ||
|
|
||
| render(<EmptyRepoState cloneUrl={cloneUrl} />); | ||
|
|
||
| expect( | ||
| screen.getByRole("heading", { name: "No code yet" }), | ||
| ).toBeInTheDocument(); | ||
| expect(screen.getByText(cloneUrl)).toBeInTheDocument(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import ErrorBoundary from "@/components/ErrorBoundary"; | ||
|
|
||
| export default function RepoLayout({ | ||
| children, | ||
| }: { | ||
| children: React.ReactNode; | ||
| }) { | ||
| return <ErrorBoundary>{children}</ErrorBoundary>; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import Link from "next/link"; | |
| import { notFound } from "next/navigation"; | ||
| import { RepoRefSelector } from "@/components/repo/BranchSelector"; | ||
| import CloneUrlCopy from "@/components/repo/CloneUrlCopy"; | ||
| import EmptyRepoState from "@/components/repo/EmptyRepoState"; | ||
| import FileTree, { type TreeEntry } from "@/components/repo/FileTree"; | ||
| import { | ||
| apiClient, | ||
|
|
@@ -18,6 +19,7 @@ interface RepoMetadata { | |
| private: boolean; | ||
| visibility?: string; | ||
| default_branch: string; | ||
| clone_url?: string; | ||
| stargazers_count: number; | ||
| watchers_count: number; | ||
| forks_count: number; | ||
|
|
@@ -110,7 +112,12 @@ export default async function RepoPage({ | |
| const branches = branchesRaw.length > 0 ? branchesRaw : [{ name: metadata.default_branch ?? "main" }]; | ||
| const branch = resolveBranchRef(refParam, branches, metadata.default_branch ?? "main"); | ||
|
|
||
| const cloneUrl = | ||
| metadata.clone_url ?? | ||
| `${process.env.NEXT_PUBLIC_API_URL ?? ""}/${owner}/${repo}.git`; | ||
|
|
||
| let contentsRaw: ContentItem[] | ContentItem | null = null; | ||
| let contentsNotFound = false; | ||
| try { | ||
| contentsRaw = await apiClient.getContents<ContentItem[] | ContentItem>( | ||
| owner, | ||
|
|
@@ -119,7 +126,11 @@ export default async function RepoPage({ | |
| branch, | ||
| ); | ||
| } catch (err) { | ||
| if (!isApiError(err) || err.status !== 404) throw err; | ||
| if (isApiError(err) && err.status === 404) { | ||
| contentsNotFound = true; | ||
| } else { | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| const [readmeRaw] = await Promise.all([fetchReadme(owner, repo, branch)]); | ||
|
|
@@ -130,6 +141,7 @@ export default async function RepoPage({ | |
| ? [contentsRaw] | ||
| : []; | ||
| const entries = mapContents(contents); | ||
| const isEmptyRepo = contentsNotFound || entries.length === 0; | ||
| const readmeHtml = readmeRaw ? renderMarkdown(readmeRaw) : null; | ||
|
|
||
| return ( | ||
|
|
@@ -269,12 +281,16 @@ export default async function RepoPage({ | |
| </div> | ||
| </div> | ||
|
|
||
| <FileTree | ||
| entries={entries} | ||
| owner={owner} | ||
| repo={repo} | ||
| treeRef={branch} | ||
| /> | ||
| {isEmptyRepo ? ( | ||
| <EmptyRepoState cloneUrl={cloneUrl} /> | ||
| ) : ( | ||
| <FileTree | ||
| entries={entries} | ||
| owner={owner} | ||
| repo={repo} | ||
| treeRef={branch} | ||
| /> | ||
| )} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [high] PRD 要件の Suspense / Skeleton loading が未実装 Task Goal Step 3 では file-tree セクションを 該当箇所: {isEmptyRepo ? (
<EmptyRepoState cloneUrl={cloneUrl} />
) : (
<FileTree
entries={entries}
owner={owner}
repo={repo}
treeRef={branch}
/>
)}同 PR の [PURPLE-MODIFIED-FILES] に @@ -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>
|
||
| </div> | ||
|
|
||
| {readmeHtml && ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| "use client"; | ||
|
|
||
| import { Component, type ErrorInfo, type ReactNode } from "react"; | ||
|
|
||
| interface ErrorBoundaryProps { | ||
| children: ReactNode; | ||
| fallback?: ReactNode; | ||
| } | ||
|
|
||
| interface ErrorBoundaryState { | ||
| hasError: boolean; | ||
| message: string; | ||
| } | ||
|
|
||
| export default class ErrorBoundary extends Component< | ||
| ErrorBoundaryProps, | ||
| ErrorBoundaryState | ||
| > { | ||
| constructor(props: ErrorBoundaryProps) { | ||
| super(props); | ||
| this.state = { hasError: false, message: "" }; | ||
| } | ||
|
|
||
| static getDerivedStateFromError(error: Error): ErrorBoundaryState { | ||
| return { hasError: true, message: error.message }; | ||
| } | ||
|
|
||
| componentDidCatch(error: Error, errorInfo: ErrorInfo): void { | ||
| console.error(error, errorInfo); | ||
| } | ||
|
|
||
| render() { | ||
| if (this.state.hasError) { | ||
| if (this.props.fallback) { | ||
| return this.props.fallback; | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex min-h-[200px] items-center justify-center p-6"> | ||
| <div className="rounded-lg border border-[#d0d7de] bg-white p-6 text-center shadow-sm"> | ||
| <p className="mb-4 text-sm text-[#57606a]">{this.state.message}</p> | ||
| <button | ||
| type="button" | ||
| onClick={() => window.location.reload()} | ||
| className="rounded-md bg-[color:var(--primary)] px-4 py-2 text-sm text-white hover:bg-[color:var(--primary-hover)]" | ||
| > | ||
| Retry | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return this.props.children; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| interface EmptyRepoStateProps { | ||
| cloneUrl: string; | ||
| } | ||
|
|
||
| export default function EmptyRepoState({ cloneUrl }: EmptyRepoStateProps) { | ||
| return ( | ||
| <div className="p-8 text-center"> | ||
| <h2 className="text-lg font-semibold text-[#24292f] m-0 mb-4">No code yet</h2> | ||
| <p className="text-sm text-[#57606a] mb-4"> | ||
| Get started by cloning this repository and pushing your first commit. | ||
| </p> | ||
| <code className="block rounded-md border border-[#d0d7de] bg-[#f6f8fa] px-4 py-2 font-mono text-sm text-[#24292f] mb-6"> | ||
| {cloneUrl} | ||
| </code> | ||
| <ol className="list-decimal text-left text-sm text-[#57606a] space-y-2 max-w-md mx-auto pl-5"> | ||
| <li> | ||
| <code className="font-mono text-[#24292f]">git init</code> | ||
| </li> | ||
| <li> | ||
| <code className="font-mono text-[#24292f]"> | ||
| git remote add origin {cloneUrl} | ||
| </code> | ||
| </li> | ||
| <li> | ||
| <code className="font-mono text-[#24292f]"> | ||
| git push -u origin main | ||
| </code> | ||
| </li> | ||
| </ol> | ||
| </div> | ||
| ); | ||
| } |
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.
🔴 [critical] Server Component の layout.tsx が
"use client"な ErrorBoundary を直接返しており、Next.js の Server/Client Component 境界に違反する可能性該当箇所:
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