chore(ci): add GitHub Actions CI + PR/Issue templates #2573
Conversation
|
@HarushiSingla is attempting to deploy a commit to the Vivek Prajapati's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a CI workflow for Node 18.x/20.x, updates README with a CI badge, enhances index.html with accessibility landmarks and styles, introduces a global catch-all NotFound route in App.jsx, and expands the pull request template with structured sections. No public APIs or exports are changed. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser
participant Router as React Router
participant App as App.jsx
participant NF as NotFound
User->>Router: Navigate to URL
Router->>App: Match routes
alt Known route
App-->>User: Render matched component
else Unmatched route
App->>NF: Route path="*"
NF-->>User: Render NotFound
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/App.jsx (1)
246-247: Remove dead NotFound route and consolidate 404 handlingThe
NotFoundcomponent isn’t defined anywhere insrc/—the only 404 page component isError. Keeping both routes causes a broken catch‐all and confusing UX. Please remove theNotFoundroute (and its import, if present) and rely solely on the existing<Error />route.Locations to fix:
- src/App.jsx
• Line 261: remove<Route path="*" element={<NotFound />} />
• Top imports: dropNotFoundimport if it existsSuggested diff:
{/* 404 Error page route */} <Route path="*" element={<Error />} /> </Route> @@ - <Route path="*" element={<NotFound />} />
🧹 Nitpick comments (7)
.github/pull_request_template.md (2)
22-42: Nice template enhancements; consider adding “Breaking changes” and “Checklist: CI passes”The new sections will help standardize PRs. Two small additions often pay off:
- Add a “Breaking changes” section to surface user-facing or API-breaking deltas.
- In the checklist, add “CI passes locally (lint/tests)” to push contributors to run checks before opening PRs.
## Checklist - [ ] My code follows the project style (naming, formatting). - [ ] I added/updated relevant tests (if applicable). - [ ] I added/updated documentation (if applicable). - [ ] Screenshots attached (if UI changes). + - [ ] CI passes locally (lint/tests). + +## Breaking changes +<!-- Describe any breaking changes and required migrations, if any. -->
26-31: Tighten “Type of change” taxonomy for consistencyConsider aligning with conventional commit types. This helps with tooling and changelog generation.
-## Type of change -- [ ] Bug fix -- [ ] New feature -- [ ] Documentation -- [ ] CI / Build -- [ ] Other (please describe): +## Type of change +- [ ] feat (new feature) +- [ ] fix (bug fix) +- [ ] docs (documentation) +- [ ] ci (CI changes) +- [ ] build (build system or external dependencies) +- [ ] chore (maintenance/no production code change) +- [ ] refactor (non-functional refactor) +- [ ] perf (performance) +- [ ] test (adding/updating tests) +- [ ] Other (please describe):README.md (1)
5-6: Make the CI badge clickableWrap the badge in a link to the workflow page so readers can jump to runs.
- +[](https://github.com/codervivek5/VigyBag/actions/workflows/ci.yml)src/App.jsx (1)
4-4: Avoid two competing catch-all routes; remove NotFound importYou already have a nested catch-all (“Error”) under the root layout. The new top-level NotFound route is effectively redundant in React Router v6 and likely never reached when the “/” parent matches with its child “*”. Keep a single 404 route for consistency.
-import NotFound from "./pages/NotFound";.github/workflows/ci.yml (3)
19-19: Add Node 22.x (current LTS) to the matrixCovers current LTS while keeping 18.x/20.x for broader compatibility.
- node-version: [18.x, 20.x] + node-version: [18.x, 20.x, 22.x]
12-14: Add minimal permissions and cancel-in-progress for faster/safer CIConservative token permissions and concurrency save resources on rapid pushes.
pull_request: branches: - main - develop +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: build:
25-30: Cache lockfiles in nested packages (monorepo-friendly)If package-lock.json isn’t at the repo root (README hints a nested frontend path), this ensures caching still works.
- name: Use Node.js ${{ matrix.node-version }} uses: actions/setup-node@v4 with: node-version: ${{ matrix.node-version }} - cache: 'npm' + cache: 'npm' + cache-dependency-path: | + **/package-lock.json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/pull_request_template.md(1 hunks).github/workflows/ci.yml(1 hunks)README.md(1 hunks)index.html(1 hunks)src/App.jsx(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/pull_request_template.md
[grammar] ~26-~26: There might be a mistake here.
Context: ... PR does and why. --> ## Type of change - [ ] Bug fix - [ ] New feature - [ ] Docu...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...hy. --> ## Type of change - [ ] Bug fix - [ ] New feature - [ ] Documentation - [ ...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...f change - [ ] Bug fix - [ ] New feature - [ ] Documentation - [ ] CI / Build - [ ]...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...ix - [ ] New feature - [ ] Documentation - [ ] CI / Build - [ ] Other (please descr...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...ure - [ ] Documentation - [ ] CI / Build - [ ] Other (please describe): ## How to ...
(QB_NEW_EN)
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
31-42: CI steps run in repo root; no front-with-code/VigyBag folder exists
The workflow’snpmcommands target the rootpackage.json, and our scans show:
- A
package.jsonat the repo root- No
front-with-codeorVigyBagdirectory in the project- No workspace configuration in the root
package.jsonTherefore the current working directory is correct and no changes are needed.
Likely an incorrect or invalid review comment.
index.html (1)
11-44: Accessible, low-risk helpers look goodClear focus styles, a robust skip link, and reduced-motion guard are solid improvements.
| <main id="main-content" tabindex="-1"> | ||
| <div id="root"></div> | ||
| </main> | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure there’s only one
landmark at a timeWrapping #root in
is great. Just confirm React pages don’t render another , which would create nested landmarks.If additional
elements exist, switch those to a div or section with role="region" and appropriate aria-labels.🏁 Script executed:
#!/bin/bash
# Search for other <main> elements in the React app that could cause nested landmarks
rg -n -C2 -S --type=jsx --type=tsx --type=js '<main\b' srcLength of output: 88
🏁 Script executed:
#!/bin/bash
# Search for all <main> tags in JS/JSX/TSX files without relying on rg’s built-in types
rg -n -C2 '<main\b' -g '*.js' -g '*.jsx' -g '*.tsx' srcLength of output: 17250
Single main landmark: prevent nested <main>
We’ve identified that nearly every page component under src/User/pages/** and src/Admin/Pages/** declares its own <main>, which will be nested inside the <main id="main-content"> wrapper in index.html. HTML specs allow only one <main> landmark per page. To resolve, pick one of two approaches:
- Option A: Replace the outer
<main>inindex.htmlwith a<div role="main">
index.html (line 52):- <main id="main-content" tabindex="-1"> + <div id="main-content" role="main" tabindex="-1">
- Option B: Keep the outer
<main>and convert all inner page<main>tags to a semantically appropriate element (e.g.<section>or<div role="region" aria-label="…">), for example:
• src/User/pages/Home/Home.jsx:179
• src/User/pages/Shipping/shipping.jsx:11
• src/Admin/Pages/AdminPanel.jsx:54
• …and so on for each page-level<main>
Ensure only one <main> landmark exists in the final rendered DOM.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
index.html lines 52-55: the outer wrapper currently uses a <main
id="main-content"> which causes nested <main> landmarks because individual page
components also render their own <main>; choose one approach and implement it:
either replace the outer <main> with a <div role="main"> in index.html to keep
inner page semantics, or keep the outer <main> and update every page-level
<main> under src/User/pages/** and src/Admin/Pages/** to a semantically
appropriate element (e.g., <section> or <div role="region" aria-label="…">) so
only a single <main> exists in the final DOM; apply the chosen change
consistently across all listed page files (e.g., Home.jsx, shipping.jsx,
AdminPanel.jsx, etc.).
1 participant
This PR adds:
Summary by CodeRabbit
New Features
Documentation
Chores
Style