Conversation
Signed-off-by: Daniel Ntege <danientege785@gmail.com>
Signed-off-by: Daniel Ntege <danientege785@gmail.com>
…ript loading Signed-off-by: Daniel Ntege <danientege785@gmail.com>
danielmarv
force-pushed
the
next-js-tests
branch
from
32bc507 to
32dc155
Compare
There was a problem hiding this comment.
Pull request overview
This PR performs code cleanup and configuration improvements for the Next.js application, primarily focusing on test file formatting, tooling configuration updates, and documentation enhancements.
Changes:
- Removed explanatory comments from test files (home, contact, about, accessibility) while maintaining functionality
- Updated Next.js layout to use the Script component with proper strategy for external script loading
- Fixed React useEffect dependency array bug in maven-prs component
- Corrected ESLint configuration paths and updated pre-commit hook for better reliability
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/home.spec.ts | Removed redundant comments while keeping test logic intact |
| tests/e2e/contact.spec.ts | Removed redundant comments while keeping test logic intact |
| tests/e2e/accessibility.spec.ts | Removed comments including one useful alt text explanation |
| tests/e2e/about.spec.ts | Removed redundant comments while keeping test logic intact |
| src/app/[locale]/layout.tsx | Improved external script loading using Next.js Script component with afterInteractive strategy |
| react-src/maven-prs.tsx | Fixed useEffect missing dependency (status prop) |
| package.json | Changed lint command from "next lint" to "eslint ." for direct ESLint usage |
| eslint.config.js | Corrected path from "src/react-src/" to "react-src/" and added "public/**" to ignored paths |
| README.md | Updated architecture documentation for 2026, added development setup instructions |
| .husky/pre-commit | Modernized for Husky v9+, added pnpm availability check with fallback |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cd "$REPO_ROOT" | ||
| pnpm run lint | ||
| else | ||
| echo "pnpm not found. Skipping lint." >&2 |
There was a problem hiding this comment.
The pre-commit hook now allows commits to proceed when pnpm is not found, only printing a warning. This means linting can be bypassed in environments without pnpm installed. Consider whether this is the desired behavior, or if the hook should fail with exit 1 when pnpm is unavailable to ensure consistent code quality checks across all environments.
| echo "pnpm not found. Skipping lint." >&2 | |
| echo "Error: pnpm not found. Cannot run lint. Please install pnpm to proceed." >&2 | |
| exit 1 |
| const img = images.nth(i); | ||
| const alt = await img.getAttribute('alt'); | ||
| // Alt can be empty string for decorative images, but must exist | ||
|
|
There was a problem hiding this comment.
A useful explanatory comment was removed here. The previous comment "Alt can be empty string for decorative images, but must exist" provided important context about why the test checks for not.toBeNull() rather than checking for a non-empty string. This distinction between decorative images (empty alt) and content images (descriptive alt) is an important accessibility concept. Consider restoring this comment to maintain the test's documentation.
| // Alt can be an empty string for decorative images, but the alt attribute must exist. |
1 participant
No description provided.