feat: implement AG Grid with atomic architecture and filter controls - #4
codewalnut-labs wants to merge 3 commits into
Conversation
- Implement AG Grid component with Random User API integration - Create custom React hook (useAgGridData) for data fetching and state management - Implement all AG Grid filter types (text, number, date, set) with floating filters - Add dynamic filter toggle buttons with active/inactive states - Implement conditional cell styling based on age ranges and gender - Add row styling with gender-based border colors - Create comprehensive test utilities with grid validation helpers - Establish atomic folder structure (app, features, models, services, hooks, utils) - Implement pagination with configurable page sizes
…handling, and testing - Rename AgGridDemo to UserTable for better semantic naming - Extract UserThumbnail and Button components for reusability - Move column definitions and default config to utils with style constants - Add error handling in useAgGridData hook with proper cleanup - Refactor tests to use fixtures, getByRole, and remove complex utils - Use semantic HTML tags and optimize grid height with calc() - Simplify user model types and add API base URL constant - Remove Vite favicon link from index.html
- Add valueGetter to convert dob.date to Date object for proper agDateColumnFilter functionality - Map Random User API response to UserProps shape, ensuring city and thumbnailPicture fields are correctly populated for grid display and filtering
File Change SummaryMoves the top-level bootstrap and shell into File Changes
Based on f370dad...84f206a |
PR ReviewTL;DRThis PR replaces the starter app with an AG Grid-based user table backed by the Random User API, adds filter-toggle controls, and introduces new styling, hook, service, model, and test layers. The biggest risk is that the grid and fixtures assume a flattened user shape that the live API does not return, so City and Picture data will be blank in production even though the current tests pass. Additional gaps remain around the promised cell-editing flow, AG Grid enterprise bootstrapping, and coverage for the new interaction and helper paths. Merge Recommendation: Approve with suggestions — the feature is mostly in place, but the live data contract, production hardening, and test coverage still need follow-up before this is ready to rely on. Focus Areas for Architect Review
💬 Minor / Nitpicks (5)
Based on f370dad...84f206a |
![mergemitra-cw[bot]](https://avatars.githubusercontent.com/u/255838639?s=60&v=4){kind=small}
|
|
||
| export const fetchUsers = async (): Promise<UserProps[]> => { | ||
| const userResponse = await axios.get(RANDOM_USER_API_URL); | ||
| return userResponse.data.results; |
There was a problem hiding this comment.
[Correctness] 🟠 The grid is wired to a flattened user shape, but fetchUsers() returns randomuser.me results verbatim. The live payload has location.city and picture.thumbnail, while the model/columns expect top-level city and thumbnailPicture, so the City column/filter never receives real city values and the Picture renderer gets undefined for every row. The current tests miss this because the fixture is already flattened to the incorrect shape.
Related: src/features/user-table/models/user.model.ts:4-11, src/features/user-table/utils/UserTable.utils.ts:95-109, src/features/user-table/components/fixtures/users.fixture.ts:1-20
| resizable: true, | ||
| filter: true, | ||
| floatingFilter: showFilters, | ||
| }); |
There was a problem hiding this comment.
[Correctness] 🟠 The advertised cell-editing flow is not implemented. defaultColDef only enables sorting/resizing/filtering, and the grid instance has no onCellValueChanged-style handler, so all columns remain read-only and there is nothing to log when a user edits a cell. Any QA pass that follows the PR’s “edit cells and check console” step will fail immediately.
Related: src/features/user-table/components/UserTable.tsx:63-71
[Code Quality] 🟠 UserTable.utils.ts is already a catch-all module for row styling, cell styling, age/gender styling rules, date formatting, column definitions, and default column config. That violates the one-job-per-module guideline and will make routine grid changes sprawl through a generic utils file instead of a focused column/config module.
| headerName: 'DOB', | ||
| filter: 'agDateColumnFilter', | ||
| valueGetter: (p) => new Date(p.data?.dob.date ?? 0), | ||
| valueFormatter: (p) => new Date(p.value).toLocaleDateString(), |
There was a problem hiding this comment.
[Correctness] 🟠 DOBs are being converted from full UTC timestamps with new Date(...).toLocaleDateString(), which shifts the calendar day for non-UTC users. For example, 1980-07-30T03:56:55.496Z renders as July 29 in UTC-4/UTC-7. I sampled the current API and 16 of 50 returned DOB timestamps were before 08:00Z, so US users will see and filter against the wrong DOB for a sizable slice of rows.
| getRowStyle, | ||
| } from '../utils/UserTable.utils'; | ||
|
|
||
| ModuleRegistry.registerModules([AllCommunityModule, SetFilterModule]); |
There was a problem hiding this comment.
[Code Quality] 🟠 ModuleRegistry.registerModules(...) runs as a top-level side effect inside the UserTable component module. That mixes app bootstrap with component rendering, so importing this component now has global behavior attached to it, which makes the component harder to reuse and harder to test in isolation.
[Enterprise Quality] 🟠 ModuleRegistry.registerModules([AllCommunityModule, SetFilterModule]) opts into AG Grid’s full community bundle even though this screen only needs a narrow subset of grid features. AG Grid’s module docs position cherry-picking modules as the bundle-size optimization path and note that AllCommunityModule contains every community module, so every initial load now ships substantially more JavaScript than this table needs.
| email: string; | ||
| phone: string; | ||
| dob: dobType; | ||
| city: string; |
There was a problem hiding this comment.
[Code Quality] 🟠 The new model names are misleading for a domain object: UserProps reads like React props, while nameType and dobType are implementation-shaped and not descriptive types. This makes every call site harder to read because the central user model does not sound like a user model.
| "@tailwindcss/vite": "^4.1.18", | ||
| "ag-grid-community": "^35.0.1", | ||
| "ag-grid-enterprise": "^35.0.1", | ||
| "ag-grid-react": "^35.0.1", |
There was a problem hiding this comment.
[Enterprise Quality] 🟠 The PR enables enterprise filtering via SetFilterModule and adds ag-grid-enterprise, but there is no licenseKey/LicenseManager.setLicenseKey integration anywhere in the repo. AG Grid’s React licensing docs require that setup for enterprise usage and call out production watermarks plus console warnings otherwise, so this is not production-ready as merged.
Related: src/features/user-table/components/UserTable.tsx:4-14
|
|
||
| return () => { | ||
| isMounted = false; | ||
| }; |
There was a problem hiding this comment.
[Enterprise Quality] 🟠 useAgGridData only flips an isMounted flag on cleanup; it never cancels the in-flight request, and fetchUsers exposes no abort signal or timeout policy. If the user leaves the page or the Random User endpoint stalls, the browser keeps the connection open and this view can sit in the loading state indefinitely because the request is still outstanding.
| return ( | ||
| <div className="min-h-screen bg-white p-6"> | ||
| <h1 className="text-3xl font-bold mb-6 text-gray-800">User Data</h1> | ||
| <UserTable /> |
There was a problem hiding this comment.
[Enterprise Quality] 🟠 The grid is rendered directly under the app root with custom valueGetter, valueFormatter, and cell-renderer logic, but there is no error boundary anywhere in the tree. A render-time exception from AG Grid or malformed remote data will therefore take down the entire page instead of degrading to a controlled fallback.
Related: src/features/user-table/components/UserTable.tsx:63-72, src/features/user-table/utils/UserTable.utils.ts:87-109
|
|
||
| return ( | ||
| <article className="space-y-4"> | ||
| <nav className="flex justify-end gap-2"> |
There was a problem hiding this comment.
[Test Quality] 🟠 The new filter toggle UI in src/features/user-table/components/UserTable.tsx:47 only has a static smoke check in src/features/user-table/components/UserTable.test.tsx:66; nothing clicks Show Filters / Remove Filters with userEvent or verifies that the floating filters actually disappear and reappear. A broken setShowFilters handler, mismatched button labels, or stale active/inactive button state would still leave the suite green. Add one interaction test that drives both buttons and asserts the visible filter controls change.
| import { UserThumbnail } from '../components/UserThumbnail'; | ||
| import type { UserProps } from '../models/user.model'; | ||
|
|
||
| export const getRowStyle = (params: RowClassParams<UserProps>) => { |
There was a problem hiding this comment.
[Test Quality] 🟠 The new helper layer has no direct tests: Button, UserThumbnail, useAgGridData, fetchUsers, and the helpers in src/features/user-table/utils/UserTable.utils.ts:10 are only exercised indirectly through src/features/user-table/components/UserTable.test.tsx:28. That leaves the hook's error/unmount path, the thumbnail null fallback, the disabled-button guard, and the age/default styling branches unverified. The fixture in src/features/user-table/components/fixtures/users.fixture.ts:1 only uses ages 34 and 39, so the <30 and >60 cases are never hit.
|
@mergemitra help |
Available Commands
Aliases
More commands coming soon! |
1 similar comment
Available Commands
Aliases
More commands coming soon! |
2 participants
What does this PR do?
What steps does your reviewer have to take to test this PR manually?
npm installto install AG Grid Enterprise, testing libraries, and development toolsnpm run typecheckto ensure no type errors existnpm testto confirm all 5 grid tests pass (renders grid, loads data, shows loading state, correct column count, filters enabled)npm run lintto validate code quality (should show 0 warnings)npm run format:checkto verify code formatting consistencynpm run spellto verify no spelling errors in source filesnpm run devand navigate to http://localhost:5173 to see the gridScreenshots
Pull Request standards checklist - Please check off
Testing checklist - Please check off
Definition of Done - Please check off