feat(resource-list): integrate API with TanStack Query and add detail page - #9
LikithaYadavG wants to merge 3 commits into
Conversation
Change SummaryThis PR integrates the resource list UI with the backend API using TanStack Query (via useFilteredResources/useResourceById hooks) and adds a resource detail page with routing. It introduces mapping constants to translate UI filter labels to API enums, refactors some component handler variable names for clarity, and updates/extends tests to cover the new data-fetching behavior and UI states. File Changes
|
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| })); | ||
| }; | ||
|
|
||
| const apiFilters: ResourceFilters = useMemo(() => { |
There was a problem hiding this comment.
Severity: 🟠 Major
Guard label→enum mappings so unknown labels never become undefined API filters, and tighten constants typing with as const.
Why this matters: With noUncheckedIndexedAccess-style safety, RESOURCE_TYPE_MAP[label] can be undefined, which can produce invalid query params and flaky filtering.
Suggested Change:
// src/constants/resources.ts
export const RESOURCE_TYPE_MAP = {
Article: "article",
Video: "video",
Documentation: "docs",
GitHub: "github",
} as const satisfies Record<string, ResourceType>;
export const SKILL_LEVEL_MAP = {
Beginner: "beginner",
Intermediate: "intermediate",
Expert: "expert",
} as const satisfies Record<string, SkillLevel>;
type ResourceTypeLabel = keyof typeof RESOURCE_TYPE_MAP;
type SkillLevelLabel = keyof typeof SKILL_LEVEL_MAP;
export const isResourceTypeLabel = (label: string): label is ResourceTypeLabel =>
label in RESOURCE_TYPE_MAP;
export const isSkillLevelLabel = (label: string): label is SkillLevelLabel =>
label in SKILL_LEVEL_MAP;
// src/features/resource-list-page/ResourceListPage.tsx
const apiFilters: ResourceFilters = useMemo(() => {
const selectedTypes = selectedFilters.resourceTypes ?? [];
const selectedLevels = selectedFilters.skillLevels ?? [];
return {
searchQuery: searchQuery.trim() || undefined,
types:
selectedTypes.length > 0
? selectedTypes.filter(isResourceTypeLabel).map((l) => RESOURCE_TYPE_MAP[l])
: undefined,
levels:
selectedLevels.length > 0
? selectedLevels.filter(isSkillLevelLabel).map((l) => SKILL_LEVEL_MAP[l])
: undefined,
};
}, [searchQuery, selectedFilters]);
| user, | ||
| ...render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <MemoryRouter initialEntries={[`/resources/${resourceId}`]}> |
There was a problem hiding this comment.
Severity: 🟠 Major
Align the test route (/resources/:id) with the actual app route (resource/:id) so routing behavior is validated correctly.
Suggested Change:
// Use the same route shape as src/routes/routes.tsx and ResourceCard navigation
<MemoryRouter initialEntries={[`/resource/${resourceId}`]}>
<Routes>
<Route path="/resource/:id" element={<ResourceDetailPage />} />
</Routes>
</MemoryRouter>
| export const ResourceDetailPage = () => { | ||
| const { id } = useParams<{ id: string }>(); | ||
| const navigate = useNavigate(); | ||
| const { data: resource, isLoading, isError } = useResourceById(id ?? ""); |
There was a problem hiding this comment.
Severity: 🟠 Major
Avoid triggering useResourceById with an empty id by disabling the query (or otherwise guarding) when the route param is missing.
Why this matters: Fetching with "" can hit an invalid endpoint and show the wrong UI state (error vs not-found).
Suggested Change:
export const ResourceDetailPage = () => {
const { id } = useParams<{ id: string }>();
const resourceId = id?.trim();
// If your hook supports options, prefer enabling only when id is present.
const { data: resource, isLoading, isError } = useResourceById(resourceId ?? "", {
enabled: Boolean(resourceId),
});
// If no id, render not-found directly.
if (!resourceId) {
return (
<ErrorState
variant="info"
title="Resource not found"
message="The resource you're looking for doesn't exist or has been removed."
/>
);
}
// ...rest unchanged
};
| import { render, screen } from "@testing-library/react"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { MemoryRouter } from "react-router-dom"; | ||
|
|
||
| import { ResourceCard } from "./ResourceCard"; | ||
|
|
||
| const navigate = vi.fn(); | ||
|
|
||
| vi.mock("react-router-dom", async () => { | ||
| const actual = | ||
| await vi.importActual<typeof import("react-router-dom")>( | ||
| "react-router-dom", | ||
| ); | ||
|
|
||
| return { | ||
| ...actual, | ||
| useNavigate: () => navigate, | ||
| }; | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| navigate.mockClear(); | ||
| }); | ||
|
|
||
| type RenderOptions = { |
There was a problem hiding this comment.
Severity: 🔴 Critical
Import React types (or avoid the React. namespace) so tests compile, and assert the expected navigation target.
Suggested Change:
import type { ComponentProps } from "react";
type RenderOptions = {
additionalProps?: Partial<ComponentProps<typeof ResourceCard>>;
};
const defaultProps: ComponentProps<typeof ResourceCard> = {
resource: {
id: "123",
// ...
},
};
// ...
expect(navigate).toHaveBeenCalledWith("/resource/123");
| import { render, screen } from "@testing-library/react"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { describe, it } from "vitest"; |
There was a problem hiding this comment.
Severity: 🔴 Critical
Add the missing expect import from vitest to prevent test runtime/compile failures.
Suggested Change:
import { describe, expect, it } from "vitest";
| }; | ||
|
|
||
| return ( | ||
| <Card |
There was a problem hiding this comment.
Severity: 🟠 Major
Make the clickable card keyboard-accessible (or render it as a semantic link/button) so non-mouse users can open resource details.
Suggested Change:
const handleCardKeyDown: React.KeyboardEventHandler = (event) => {
if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
handleCardClick();
}
};
<Card
hover
onClick={handleCardClick}
onKeyDown={handleCardKeyDown}
role="button"
tabIndex={0}
aria-label={`View details for ${resource.title}`}
>
{/* ... */}
</Card>If Card supports rendering asChild, consider wrapping the content in a Link instead for native semantics.
|
|
||
| it("should render the search bar input", () => { | ||
| renderResourceListPage(); | ||
| it("should render filter dropdowns", async () => { |
There was a problem hiding this comment.
Severity: 🟠 Major
Add a regression test that selecting filter options calls the API with mapped enum values (e.g., Article → article).
Why this matters: The UI uses label→enum mapping; without a test, a label typo or map change can silently break filtering.
Suggested Change:
it("should call API with mapped filters when user selects options", async () => {
mockFetchResourcesByFilters.mockResolvedValue([]);
const { user } = renderResourceListPage();
await user.click(screen.getByRole("button", { name: /resource type/i }));
await user.click(screen.getByRole("checkbox", { name: /article/i }));
await user.click(screen.getByRole("button", { name: /skill levels/i }));
await user.click(screen.getByRole("checkbox", { name: /beginner/i }));
await waitFor(() => {
expect(mockFetchResourcesByFilters).toHaveBeenCalledWith(
expect.objectContaining({
types: ["article"],
levels: ["beginner"],
}),
);
});
});
| it("should render resource title", async () => { | ||
| mockFetchResourceById.mockResolvedValue({ |
There was a problem hiding this comment.
Severity: 💬 Minor [nitpick]
Extract a shared resource fixture to avoid repeating the same object in multiple tests.
Suggested Change:
const buildResource = (overrides: Partial<Resource> = {}): Resource => ({
id: "1",
title: "React Basics",
description: "Learn the basics of React",
type: "article",
level: "beginner",
tags: ["react", "frontend"],
url: "https://example.com",
created_at: "2024-01-01T00:00:00Z",
updated_at: "2024-01-01T00:00:00Z",
...overrides,
});
mockFetchResourceById.mockResolvedValue(buildResource());
PR OverviewPR Type: Feature Focus Areas for Architect ReviewConfirm the intended URL convention for resource detail ( Validate the boundary strategy for React Query hooks when required params are missing (e.g., disable queries via Decide whether clickable cards should be implemented as semantic PR InsightsPotential PR Improvements
Strengths
|
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @cw-pr-agent rereview |
|
@cw-pr-agent rereview |
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
| ]; | ||
| ] as const; | ||
|
|
||
| export const RESOURCE_TYPE_MAP: Record<string, ResourceType> = { |
There was a problem hiding this comment.
Severity: 🟠 Major
Tighten the maps with satisfies + label type guards so unknown UI labels can’t become undefined API filter values.
Why this matters: Record<string, ...> makes MAP[label] look safe in TS even though unknown labels return undefined at runtime.
Suggested Change:
export const RESOURCE_TYPE_MAP = {
Article: "article",
Video: "video",
Documentation: "docs",
GitHub: "github",
} as const satisfies Record<string, ResourceType>;
export const SKILL_LEVEL_MAP = {
Beginner: "beginner",
Intermediate: "intermediate",
Expert: "expert",
} as const satisfies Record<string, SkillLevel>;
export type ResourceTypeLabel = keyof typeof RESOURCE_TYPE_MAP;
export type SkillLevelLabel = keyof typeof SKILL_LEVEL_MAP;
export const isResourceTypeLabel = (label: string): label is ResourceTypeLabel =>
label in RESOURCE_TYPE_MAP;
export const isSkillLevelLabel = (label: string): label is SkillLevelLabel =>
label in SKILL_LEVEL_MAP;
// ResourceListPage.tsx (mapping)
// selectedTypes.filter(isResourceTypeLabel).map((l) => RESOURCE_TYPE_MAP[l])
|
|
||
| it("should render the search bar input", () => { | ||
| renderResourceListPage(); | ||
| it("should render filter dropdowns", async () => { |
There was a problem hiding this comment.
Severity: 🟠 Major
Add a regression test asserting the API is called with mapped enum values when users select UI filter labels.
Why this matters: A label typo or mapping change can silently break filtering without a focused test.
Suggested Change:
it("should call API with mapped filters when user selects options", async () => {
mockFetchResourcesByFilters.mockResolvedValue([]);
const { user } = renderResourceListPage();
await user.click(screen.getByRole("button", { name: /resource type/i }));
await user.click(screen.getByRole("checkbox", { name: /article/i }));
await user.click(screen.getByRole("button", { name: /skill levels/i }));
await user.click(screen.getByRole("checkbox", { name: /beginner/i }));
await waitFor(() => {
expect(mockFetchResourcesByFilters).toHaveBeenCalledWith(
expect.objectContaining({
types: ["article"],
levels: ["beginner"],
}),
);
});
});
| export const ResourceDetailPage = () => { | ||
| const { id } = useParams<{ id: string }>(); | ||
| const navigate = useNavigate(); | ||
| const { data: resource, isLoading, isError } = useResourceById(id ?? ""); |
There was a problem hiding this comment.
Severity: 🟠 Major
Guard the detail query so useResourceById is not triggered with an empty/missing route param.
Why this matters: Fetching with "" can hit an invalid endpoint and show the wrong UI state (error vs not-found).
Suggested Change:
const { id } = useParams<{ id: string }>();
const resourceId = id?.trim();
if (!resourceId) {
return (
<ErrorState
variant="info"
title="Resource not found"
message="The resource you're looking for doesn't exist or has been removed."
/>
);
}
const { data: resource, isLoading, isError } = useResourceById(resourceId);
| }; | ||
|
|
||
| return ( | ||
| <Card |
There was a problem hiding this comment.
Severity: 🟠 Major
Make the clickable card keyboard-accessible (or render it as a semantic Link) so non-mouse users can open resource details.
Suggested Change:
const handleCardKeyDown: React.KeyboardEventHandler = (event) => {
if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
handleCardClick();
}
};
<Card
hover
onClick={handleCardClick}
onKeyDown={handleCardKeyDown}
role="button"
tabIndex={0}
aria-label={`View details for ${resource.title}`}
>
{/* ... */}
</Card>If possible, prefer a Link for native semantics: to={\/resource/${resource.id}`}`.
| }; | ||
| }); | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
Severity: 💬 Minor [nitpick]
Move the beforeEach into the describe block to keep test setup scoped and easier to reason about.
Suggested Change:
describe("ResourceList", () => {
beforeEach(() => {
navigate.mockClear();
});
// tests...
});
Re-review checklist
PR OverviewPR Type: Feature Focus Areas for Architect ReviewConfirm label→enum mapping is runtime-safe (type guards + Decide the canonical boundary behavior for Align clickable card semantics with accessibility expectations (prefer ValidationRoute path consistency and navigation target assertions appear resolved in this diff; remaining concerns are map safety + mapping test coverage, missing-id query guarding, and card accessibility semantics. PR InsightsPotential PR Improvements
Strengths
Rereview ImpressionsProgress Since Last Review
New issues introduced (if any)
|
ca4c347 to
b062778
Compare
…resource navigation
b062778 to
c6258a0
Compare
1 participant
PR Template & Definition of Done
What does this PR do?
What steps does your reviewer have to take to test this PR manually?
Pull Request standards checklist - Please check off
Testing checklist - Please check off
If you have not followed and completed any of the above, please explain why below.
N/A
Definition of Done - Please check off