Skip to content

fix: address DataTable, Modal, and ErrorBoundary review feedback#9

Merged
intel352 merged 2 commits intofeat/issue-3-4-components-a11yfrom
copilot/sub-pr-6
Feb 24, 2026
Merged

fix: address DataTable, Modal, and ErrorBoundary review feedback#9
intel352 merged 2 commits intofeat/issue-3-4-components-a11yfrom
copilot/sub-pr-6

Conversation

Copy link
Contributor

Copilot AI commented Feb 24, 2026

Addresses 9 review comments from the component library PR: type safety gaps, duplicate ESC handling, duplicate aria-labelledby IDs, missing imports, and pagination state drift.

DataTable

  • key: keyof T & string — was string, allowing keys that don't exist on T; render value param, sortKey state, and handleSort updated accordingly
  • Remove role="grid" — native <table> semantics are correct; grid implies unimplemented arrow-key navigation
  • Page clampinguseEffect resets page to 0..totalPages-1 when data.length/pageSize changes, preventing stuck empty views
  • ReactNode import — was referencing React.ReactNode without importing React
// Before
key: string;
render?: (value: unknown, row: T) => React.ReactNode;

// After
key: keyof T & string;
render?: (value: T[keyof T & string], row: T) => ReactNode;

Modal

  • Remove onKeyDown ESC handler — ESC was handled by both onKeyDown and the native cancel event, potentially calling onClose twice; now relies solely on the cancel listener
  • useId() for title id — hard-coded "modal-title" caused duplicate IDs with multiple Modal instances; replaced with useId() so aria-labelledby and <h2 id> are always paired and unique
  • type MouseEvent import — was using React.MouseEvent without importing React

ErrorBoundary test

  • Added beforeEach/afterEach to vitest imports — were used but not imported, causing test suite failure at runtime

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot AI changed the title [WIP] Add shared React components with accessibility support fix: address DataTable, Modal, and ErrorBoundary review feedback Feb 24, 2026
Copilot AI requested a review from intel352 February 24, 2026 01:47
@intel352 intel352 marked this pull request as ready for review February 24, 2026 01:51
@intel352 intel352 merged commit 86f8bbb into feat/issue-3-4-components-a11y Feb 24, 2026
@intel352 intel352 deleted the copilot/sub-pr-6 branch February 24, 2026 01:51
intel352 added a commit that referenced this pull request Feb 24, 2026
* feat: expand component library with StatusBadge, DataTable, Modal, LoadingSpinner, ErrorBoundary + accessibility (#3, #4)

Add 5 new shared React components with full TypeScript types,
unit tests, and accessibility support:

- StatusBadge: color-coded workflow status indicator
- DataTable: sortable, paginated data table
- Modal: confirmation/error/info dialogs
- LoadingSpinner: loading state indicator
- ErrorBoundary: React error boundary wrapper

All components use semantic HTML, ARIA labels, and support
keyboard navigation (WCAG compliance).

Closes #3, Closes #4

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: address DataTable, Modal, and ErrorBoundary review feedback (#9)

* Initial plan

* fix: address PR review comments for DataTable, Modal, and ErrorBoundary

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* fix: address Copilot review feedback on component library

- DataTable: type key as keyof T, import ReactNode, remove role="grid",
  clamp pagination, memoize sorting
- Modal: fix double-escape, import event types, use useId for title
- ErrorBoundary: remove mismatched aria-label on retry button
- ErrorBoundary tests: import beforeEach/afterEach from vitest
- LoadingSpinner: move keyframe injection to useEffect
- StatusBadge: use strict types for size style records

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove useEffect setState for pagination clamping in DataTable

Replace useEffect + setState pagination clamping with derived safePage
value to satisfy react-hooks/set-state-in-effect lint rule. The flat
DataTable already had safePage derived inline; remove the redundant
effect. The directory-based DataTable now derives safePage from the
raw page state and totalPages.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove duplicate test assertion, use useEffect for SSR compat

Remove duplicate 'has accessible form with aria-label' test that was
identical to 'renders a <form> element for the login form'. Switch
LoadingSpinner keyframe injection from useLayoutEffect to useEffect
to avoid SSR warnings in Next.js/Remix consumers.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
intel352 added a commit that referenced this pull request Feb 26, 2026
…aceholder (#13)

* feat: expand component library with StatusBadge, DataTable, Modal, LoadingSpinner, ErrorBoundary + accessibility (#3, #4)

Add 5 new shared React components with full TypeScript types,
unit tests, and accessibility support:

- StatusBadge: color-coded workflow status indicator
- DataTable: sortable, paginated data table
- Modal: confirmation/error/info dialogs
- LoadingSpinner: loading state indicator
- ErrorBoundary: React error boundary wrapper

All components use semantic HTML, ARIA labels, and support
keyboard navigation (WCAG compliance).

Closes #3, Closes #4

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: address DataTable, Modal, and ErrorBoundary review feedback (#9)

* Initial plan

* fix: address PR review comments for DataTable, Modal, and ErrorBoundary

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* fix: address Copilot review feedback on component library

- DataTable: type key as keyof T, import ReactNode, remove role="grid",
  clamp pagination, memoize sorting
- Modal: fix double-escape, import event types, use useId for title
- ErrorBoundary: remove mismatched aria-label on retry button
- ErrorBoundary tests: import beforeEach/afterEach from vitest
- LoadingSpinner: move keyframe injection to useEffect
- StatusBadge: use strict types for size style records

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove useEffect setState for pagination clamping in DataTable

Replace useEffect + setState pagination clamping with derived safePage
value to satisfy react-hooks/set-state-in-effect lint rule. The flat
DataTable already had safePage derived inline; remove the redundant
effect. The directory-based DataTable now derives safePage from the
raw page state and totalPages.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove duplicate test assertion, use useEffect for SSR compat

Remove duplicate 'has accessible form with aria-label' test that was
identical to 'renders a <form> element for the login form'. Switch
LoadingSpinner keyframe injection from useLayoutEffect to useEffect
to avoid SSR warnings in Next.js/Remix consumers.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix 1: Deduplicate DataTable component

Delete the flat src/components/DataTable.tsx file (the stale implementation) and update the test file to use the subdirectory version at src/components/DataTable/DataTable.tsx. Changed column definitions from 'label' to 'header', added required 'rowKey' prop, and updated test assertions to match the subdirectory component's API.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix 2: Remove hardcoded 'admin' placeholder from LoginPage

Changed the default username placeholder from 'admin' to 'Enter username' for better UX and to avoid hardcoding test credentials in the UI.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix 3: Abstract localStorage in authStore

Added StorageAdapter interface and storage configuration option to AuthStoreConfig. The store now accepts an optional storage adapter (defaults to localStorage) allowing for testable storage behavior and SSR compatibility. All localStorage calls now use the configurable storage instance.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix DataTable export and remove stale SortState/SortDirection type re-exports

- Use named re-export from DataTable/index.ts (not default) in components/index.ts
- Remove non-existent SortState and SortDirection from components/index.ts and src/index.ts
- Fix DataTable.test.tsx button queries to use 'Next page'/'Previous page' matching aria-labels

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix lint: remove unused 'vi' import from DataTable.test.tsx

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants