chore(husky): remove deprecated bootstrap lines; style: run prettier …#5
chore(husky): remove deprecated bootstrap lines; style: run prettier …#5a-elkhiraooui-ciscode wants to merge 5 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the repository's build tooling, testing infrastructure, and code quality setup. It migrates from Vite's library mode to tsup for bundling, introduces comprehensive unit and E2E testing with Vitest and Playwright, and applies consistent code formatting with Prettier.
Changes:
- Replaced Vite library build with tsup, producing dual ESM/CJS outputs
- Added comprehensive test coverage using Vitest (unit) and Playwright (E2E)
- Standardized code formatting across the codebase using Prettier
Reviewed changes
Copilot reviewed 85 out of 96 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Configured Vitest with jsdom environment, coverage thresholds, and test directories |
| tsup.config.ts | Added tsup configuration for dual-format builds with proper file extensions |
| package.json | Updated build scripts, added test/lint/format commands, and reorganized dependencies |
| src/index.ts | Expanded public API exports to include all components, hooks, and types |
| src/components/Form/ZodDynamicForm.tsx | Fixed typo 'textt' → 'text' and applied formatting |
| src/hooks/*.ts | Added new authentication and accessibility hooks with proper TypeScript types |
| tests/unit/**/*.test.tsx | Added comprehensive unit tests for components and hooks |
| tests/e2e/**/*.spec.ts | Added Playwright E2E tests for examples app |
| .husky/pre-commit | Updated to run format, lint, typecheck, and test |
| examples/ | Added examples app with vite config for component demonstration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [success, setSuccess] = useState<boolean>(false); | ||
|
|
||
| function update<K extends keyof PasswordResetInput>(key: K, value: PasswordResetInput[K]) { | ||
| setValues((v: PasswordResetInput) => ({ ...v, [key]: value })); |
There was a problem hiding this comment.
The type annotation on the updater function parameter v: PasswordResetInput is redundant since TypeScript infers it from the state type. Remove it for consistency with the .ts version in the same directory.
| setValues((v: PasswordResetInput) => ({ ...v, [key]: value })); | |
| setValues((v) => ({ ...v, [key]: value })); |
| const [result, setResult] = useState<LoginResult<TUser> | null>(null); | ||
|
|
||
| function update<K extends keyof LoginCredentials>(key: K, value: LoginCredentials[K]) { | ||
| setValues((v: LoginCredentials) => ({ ...v, [key]: value })); |
There was a problem hiding this comment.
The type annotation on the updater function parameter v: LoginCredentials is redundant since TypeScript infers it from the state type. Remove it for consistency with the .ts version in the same directory.
| setValues((v: LoginCredentials) => ({ ...v, [key]: value })); | |
| setValues((v) => ({ ...v, [key]: value })); |
| @@ -1,19 +1,25 @@ | |||
| import React from 'react'; | |||
| import { Link } from 'react-router'; | |||
| import { Link } from 'react-router-dom'; | |||
There was a problem hiding this comment.
The import changed from 'react-router' to 'react-router-dom', but the test file src/components/Breadcrumbs/Breadcrumb.test.tsx still mocks 'react-router' instead of 'react-router-dom'. This inconsistency should be resolved.
| }); | ||
| const fields: FieldConfigDynamicForm[] = [ | ||
| { key: 'name', label: 'Name', type: 'text' }, | ||
| { key: 'email', label: 'Email', type: 'email' }, |
There was a problem hiding this comment.
Field type 'email' is not a valid FieldType according to the model definition in src/models/FieldConfigDynamicForm.ts. The valid types are: 'text', 'number', 'textarea', 'select', 'checkbox', 'multiSelect', 'custom'. Use 'text' instead of 'email'.
| { key: 'email', label: 'Email', type: 'email' }, | |
| { key: 'email', label: 'Email', type: 'text' }, |
| }, [container, selector, initialIndex]); | ||
| } | ||
|
|
||
| export default undefined; |
There was a problem hiding this comment.
Exporting undefined as the default export serves no purpose. Remove this line or export the function as default if it's intended to be the primary export.
| export default undefined; | |
| export default useKeyboardNavigation; |
|
|
||
| export default undefined; |
There was a problem hiding this comment.
Exporting undefined as the default export serves no purpose. Remove this line.
| export default undefined; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 97 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -250,7 +242,7 @@ export default function ControlledZodDynamicForm({ | |||
| //Show erros validation | |||
There was a problem hiding this comment.
Corrected spelling of 'erros' to 'errors' in comment.
| //Show erros validation | |
| // Show errors validation |
| @@ -1,19 +1,25 @@ | |||
| import React from 'react'; | |||
| import { Link } from 'react-router'; | |||
| import { Link } from 'react-router-dom'; | |||
There was a problem hiding this comment.
The import changed from 'react-router' to 'react-router-dom', which is correct for client-side routing. However, verify that 'react-router-dom' is listed in peerDependencies (already confirmed in package.json line 47), and ensure all other files using 'react-router' imports are also updated consistently.
|
|
||
| export default undefined; |
There was a problem hiding this comment.
Exporting undefined as default is unusual and may confuse consumers. Since the hooks are already exported as named exports, consider removing the default export entirely.
| export default undefined; |
| npm run format:write | ||
| npm run lint | ||
| npm run typecheck | ||
| npm test |
There was a problem hiding this comment.
Running all these commands sequentially in pre-commit can be slow and frustrate developers. Consider using a tool like lint-staged to only check staged files, or move some checks (like full test suite) to pre-push or CI.
| npm test |
| {columns.map(col => ( | ||
| <th key={String(col.key)} className="px-4 py-3">{col.title}</th> | ||
| {columns.map((col, i) => ( | ||
| <th key={i} className="px-4 py-3"> |
There was a problem hiding this comment.
Using array index as key is not recommended when the column order might change. Since columns have a key property, consider using String(col.key) or a stable identifier as the key instead of the index.
| <th key={i} className="px-4 py-3"> | |
| <th key={String(col.key)} className="px-4 py-3"> |
Zaiidmo
left a comment
There was a problem hiding this comment.
It would be appreciated if the workflows directory stays the same as it is, as well as husky's pre-commit and pre-push configurations
|
Unaccepted changes in the operations files ! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 98 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useT: () => (key: string, vars?: any) => (typeof key === 'string' ? key : 't'), | ||
| })); | ||
|
|
||
| vi.mock('react-router', () => ({ |
There was a problem hiding this comment.
Mock uses 'react-router' but the component now imports from 'react-router-dom'. Update the mock to match the actual import source.
| vi.mock('react-router', () => ({ | |
| vi.mock('react-router-dom', () => ({ |
|
|
||
| export default undefined; |
There was a problem hiding this comment.
Exporting undefined as default is unnecessary and confusing. Since the file exports a named function, remove the default export entirely.
| export default undefined; |
…write
Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes