Implement routing refactoring with review fixes for PR #17#18
Implement routing refactoring with review fixes for PR #17#18
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Created src/types/routes.ts with proper type definitions (using semicolons and import type)
- Created src/Components/handler/Metahandler.tsx for dynamic document title handling
- Created src/config/routes.tsx to separate route configuration from App component
- Refactored src/App.tsx to use dynamic route generation
- Updated index.html title to "Web / 湘南藤沢高専"
All review comments addressed:
✓ flatRoutes calculation moved outside component to avoid re-rendering
✓ Join route correctly uses /join path
✓ Consistent object key formatting (no quotes)
✓ Proper import type { ReactNode } usage
✓ Semicolons added after type definitions for consistency
Co-authored-by: tanahiro2010 <129029050+tanahiro2010@users.noreply.github.com>
- Add comment explaining protected field in RouteMeta (reserved for future auth) - Extract hardcoded DEFAULT_ROUTE constant for consistency - Optimize MetaHandler to use Map for O(1) route lookup instead of O(n) array find Co-authored-by: tanahiro2010 <129029050+tanahiro2010@users.noreply.github.com>
- Use for-of loop instead of .map() to avoid intermediate array when creating routesByPath Map - Add DEFAULT_TITLE fallback in MetaHandler for routes without explicit titles Co-authored-by: tanahiro2010 <129029050+tanahiro2010@users.noreply.github.com>
- Handle empty string child path by returning parent or "/" for better robustness Co-authored-by: tanahiro2010 <129029050+tanahiro2010@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the app’s routing to be configuration-driven with typed route metadata, enabling centralized route definitions and dynamic document title updates based on the current route.
Changes:
- Added typed route definitions with optional metadata (
title,navLabel,visibleInNav,protected). - Introduced centralized nested route configuration with helpers to flatten routes and build a
routesByPathlookup map. - Updated the app to render routes from the flattened configuration and added a
MetaHandlerto managedocument.title; updated the base HTML title.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/routes.ts | Defines route and metadata types used by the new routing config. |
| src/config/routes.tsx | Central route configuration, route flattening, and O(1) path lookup map. |
| src/Components/handler/Metahandler.tsx | Updates document.title on navigation using the path lookup map. |
| src/App.tsx | Switches from hardcoded routes to iterating flatRoutes and mounts MetaHandler. |
| index.html | Updates the base document title string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| const match = routesByPath.get(pathname); | ||
| document.title = match?.meta?.title ?? DEFAULT_TITLE; | ||
| }, [pathname]); |
There was a problem hiding this comment.
useEffect uses routesByPath and DEFAULT_TITLE from outer scope but they are not included in the dependency array, which will trigger react-hooks/exhaustive-deps warnings and can lead to stale values if either ever changes. Include them in the deps array (or intentionally suppress the rule with a comment if you want to treat them as stable constants).
| }, [pathname]); | |
| }, [pathname, routesByPath, DEFAULT_TITLE]); |
| import MetaHandler from "./Components/handler/Metahandler"; | ||
| import { flatRoutes } from "./config/routes"; |
There was a problem hiding this comment.
The file is named Metahandler.tsx while the exported component is MetaHandler and other components use PascalCase filenames (e.g. src/Components/ui/Button.tsx, src/Components/layout/Section.tsx). Consider renaming the file to MetaHandler.tsx and updating this import to avoid casing inconsistencies (and potential issues on case-sensitive filesystems).
|
|
||
| const isVotePageEnabled = import.meta.env.VITE_LT1_VOTE_ENABLED === "true"; | ||
|
|
||
| const DEFAULT_ROUTE = "/events/lt-1"; |
There was a problem hiding this comment.
DEFAULT_ROUTE is defined here but /events/lt-1 is still hardcoded in other components (e.g. links in LT1Register/VotePresenter). Export DEFAULT_ROUTE (or another shared constant) so those callers can import it, reducing duplication and the risk of routes drifting out of sync if the default event route changes.
| const DEFAULT_ROUTE = "/events/lt-1"; | |
| export const DEFAULT_ROUTE = "/events/lt-1"; |
Implements the routing system refactoring proposed in PR #17, addressing all code review feedback. Converts hardcoded routes to configuration-based approach with metadata support and dynamic title management.
Changes
Type system (
src/types/routes.ts): Route types with metadata (title,navLabel,visibleInNav,protected)Route configuration (
src/config/routes.tsx): Centralized nested route definitions with:flattenRoutes()recursively flattens nested structurejoinPaths()handles path concatenation with edge casesroutesByPathMap for O(1) lookups (built without intermediate arrays)DEFAULT_ROUTEconstant eliminates hardcoded/events/lt-1duplicatesMetaHandler (
src/Components/handler/Metahandler.tsx): Updatesdocument.titleon route change using Map lookup with fallbackApp component (
src/App.tsx): Simplified to iterate overflatRoutesarrayHTML title: Updated to "Web / 湘南藤沢高専"
Review fixes applied
/to/join(eliminated conflict)flatRoutescomputed at module level (prevents re-computation on render)import typeusage for ReactNodeExample
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.