Add React Router support with 404 page#59
Conversation
WalkthroughThis PR introduces client-side routing using React Router. It adds ChangesClient-Side Routing Implementation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
github-actions
Bot
added
no-issue-linked
configuration
dependencies
frontend
javascript
size/M
first-time-contributor
and removed
size/M
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/App.css`:
- Around line 5-16: The .home-container, .notfound-container rule contains
unnecessary blank lines before declarations which triggers stylelint's
declaration-empty-line-before; remove the empty lines so declarations
(min-height, display, flex-direction, justify-content, align-items, text-align)
are directly one-per-line without preceding blank lines and preserve existing
ordering and indentation in the .home-container, .notfound-container block to
satisfy the CSS style guide.
- Line 7: Replace the static viewport height declaration in App.css (the
min-height: 100vh property) with a dynamic viewport fallback pattern to avoid
mobile clipping: keep the legacy fallback first (min-height: 100vh) and then
override it with the dynamic unit (min-height: 100dvh) so modern browsers use
100dvh while older ones fall back to 100vh.
- Around line 18-31: The global selectors h1, a, and a:hover are leaking styles;
scope them to route/container classes instead (e.g., use a route wrapper class
like .route-container or specific page classes) so headings and links in other
views aren't affected; update the rules that target h1, a, and a:hover to target
.route-container h1, .route-container a, and .route-container a:hover (or the
specific route/page class names you use) and ensure class names follow the
project's naming conventions for CSS files.
In `@src/App.tsx`:
- Line 5: Replace hardcoded user-facing strings in the App component with i18n
lookups: import and use the translation hook (e.g., useTranslation or t) in the
App function and replace occurrences like "<h1>Hello, OrgExplorer!</h1>" and the
strings on lines 12–13 with t('key') calls (define keys such as "app.title" and
the other two keys in your locale resource files). Ensure you add the new keys
to the appropriate locale JSON and keep the original text as the default value
in the resource file.
- Line 1: Replace native anchors with React Router's Link: add import { Link }
from 'react-router-dom' and change <a href="/"> usages to <Link to="/"> to avoid
full page reloads (search for the anchor in App.tsx). Externalize the hardcoded
UI strings "Hello, OrgExplorer!", "404 - Page Not Found", and "Go Back Home" to
the i18n resource files and replace usages with the project i18n lookup (e.g.,
useTranslation hook or existing t function) so the component reads labels via
t('...') instead of string literals; update imports to include the i18n hook
(useTranslation or project equivalent) and use the same translation keys in the
resource files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1b729e5-cfef-4080-a8ba-a24e8a8363cb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/App.csssrc/App.tsxsrc/index.csssrc/main.tsx
| .home-container, | ||
| .notfound-container { | ||
| min-height: 100vh; | ||
|
|
||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
| text-align: center; | ||
| } |
There was a problem hiding this comment.
Fix stylelint violations in the route container block.
Line 9, Line 12, and Line 15 have empty lines before declarations (declaration-empty-line-before), which will fail stylelint.
Proposed fix
.home-container,
.notfound-container {
min-height: 100vh;
-
display: flex;
flex-direction: column;
-
justify-content: center;
align-items: center;
-
text-align: center;
}As per coding guidelines, "**/*.css: Review the CSS code against the google css style guide... and best practices associated with CSS."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .home-container, | |
| .notfound-container { | |
| min-height: 100vh; | |
| display: flex; | |
| flex-direction: column; | |
| justify-content: center; | |
| align-items: center; | |
| text-align: center; | |
| } | |
| .home-container, | |
| .notfound-container { | |
| min-height: 100vh; | |
| display: flex; | |
| flex-direction: column; | |
| justify-content: center; | |
| align-items: center; | |
| text-align: center; | |
| } |
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 9-9: Expected no empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 12-12: Expected no empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 15-15: Expected no empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/App.css` around lines 5 - 16, The .home-container, .notfound-container
rule contains unnecessary blank lines before declarations which triggers
stylelint's declaration-empty-line-before; remove the empty lines so
declarations (min-height, display, flex-direction, justify-content, align-items,
text-align) are directly one-per-line without preceding blank lines and preserve
existing ordering and indentation in the .home-container, .notfound-container
block to satisfy the CSS style guide.
|
|
||
| .home-container, | ||
| .notfound-container { | ||
| min-height: 100vh; |
There was a problem hiding this comment.
Use dynamic viewport height to avoid mobile clipping/jump.
min-height: 100vh can produce layout jumps on mobile browser UI changes. Add 100dvh with fallback.
Proposed fix
.home-container,
.notfound-container {
- min-height: 100vh;
+ min-height: 100vh;
+ min-height: 100dvh;As per coding guidelines, "**/*.css: ...adhere to best practices recommended by lighthouse or similar tools for performance."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| min-height: 100vh; | |
| .home-container, | |
| .notfound-container { | |
| min-height: 100vh; | |
| min-height: 100dvh; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/App.css` at line 7, Replace the static viewport height declaration in
App.css (the min-height: 100vh property) with a dynamic viewport fallback
pattern to avoid mobile clipping: keep the legacy fallback first (min-height:
100vh) and then override it with the dynamic unit (min-height: 100dvh) so modern
browsers use 100dvh while older ones fall back to 100vh.
| h1 { | ||
| font-size: 4rem; | ||
| margin-bottom: 20px; | ||
| } | ||
|
|
||
| a { | ||
| color: #4ea1ff; | ||
| text-decoration: none; | ||
| font-size: 18px; | ||
| } | ||
|
|
||
| a:hover { | ||
| text-decoration: underline; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Scope typography/link rules to route containers to prevent global style bleed.
These global selectors can unintentionally restyle headings/links in unrelated views.
Proposed refactor
-h1 {
+.home-container h1,
+.notfound-container h1 {
font-size: 4rem;
margin-bottom: 20px;
}
-a {
+.home-container a,
+.notfound-container a {
color: `#4ea1ff`;
text-decoration: none;
font-size: 18px;
}
-a:hover {
+.home-container a:hover,
+.notfound-container a:hover {
text-decoration: underline;
}As per coding guidelines, "**/*.css: ...adhere to similar naming conventions for classes, ids."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/App.css` around lines 18 - 31, The global selectors h1, a, and a:hover
are leaking styles; scope them to route/container classes instead (e.g., use a
route wrapper class like .route-container or specific page classes) so headings
and links in other views aren't affected; update the rules that target h1, a,
and a:hover to target .route-container h1, .route-container a, and
.route-container a:hover (or the specific route/page class names you use) and
ensure class names follow the project's naming conventions for CSS files.
| @@ -1,12 +1,28 @@ | |||
| import './App.css' | |||
| import { Routes, Route } from 'react-router-dom' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/App.tsxRepository: AOSSIE-Org/OrgExplorer
Length of output: 694
Use React Router Link instead of native anchor, and externalize hardcoded UI strings.
Using <a href="/"> triggers a full page reload in this React Router SPA. Additionally, user-visible strings ("Hello, OrgExplorer!", "404 - Page Not Found", "Go Back Home") are hardcoded and should be externalized to i18n resource files per coding guidelines.
Suggested fix
-import { Routes, Route } from 'react-router-dom'
+import { Routes, Route, Link } from 'react-router-dom'
@@
- <a href="/">Go Back Home</a>
+ <Link to="/">Go Back Home</Link>Also externalize hardcoded strings to i18n resources.
Also applies to: 13-13 (and strings on lines 5, 12)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/App.tsx` at line 1, Replace native anchors with React Router's Link: add
import { Link } from 'react-router-dom' and change <a href="/"> usages to <Link
to="/"> to avoid full page reloads (search for the anchor in App.tsx).
Externalize the hardcoded UI strings "Hello, OrgExplorer!", "404 - Page Not
Found", and "Go Back Home" to the i18n resource files and replace usages with
the project i18n lookup (e.g., useTranslation hook or existing t function) so
the component reads labels via t('...') instead of string literals; update
imports to include the i18n hook (useTranslation or project equivalent) and use
the same translation keys in the resource files.
| function App() { | ||
| function Home() { | ||
| return ( | ||
| <h1>Hello, OrgExplorer!</h1> |
There was a problem hiding this comment.
Externalize user-facing strings to i18n resources.
These hardcoded strings should come from localization resources to meet the repo’s i18n requirement.
As per coding guidelines, "User-visible strings should be externalized to resource files (i18n)".
Also applies to: 12-13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/App.tsx` at line 5, Replace hardcoded user-facing strings in the App
component with i18n lookups: import and use the translation hook (e.g.,
useTranslation or t) in the App function and replace occurrences like
"<h1>Hello, OrgExplorer!</h1>" and the strings on lines 12–13 with t('key')
calls (define keys such as "app.title" and the other two keys in your locale
resource files). Ensure you add the new keys to the appropriate locale JSON and
keep the original text as the default value in the resource file.
|
Hi @rahul Vyas,
I’m a first-time contributor and I’ve been exploring the OrgExplorer
codebase. I recently worked on PR #59 and would like to continue
contributing.
Could you suggest a beginner-friendly issue or improvement area I can work
on next?
Thanks!
…On Thu, 14 May 2026 at 16:03, Rahul Vyas ***@***.***> wrote:
Closed #59 <#59>.
—
Reply to this email directly, view it on GitHub
<#59 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFPPYFZWOPR5XBDWUG277MT42WOHZAVCNFSM6AAAAACY4QNY3GVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMRVGUZDINRVHA2DCOI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hey @coderpurohit, wlc to our community, we will open some beginner friendly issues soon once we start working. And don't forget to checkout our other projects that meet your niche and interest. Check for good first issue. |
2 participants
Addressed Issues
Fixes #40
Summary
Implemented basic client-side routing using
react-router-dom.Changes made
BrowserRouterApp.tsx#Validation