Extract hard-coded routes into centralized constants (resolves #747)#1127
Open
BenjaminMichaelis wants to merge 3 commits into
Open
Extract hard-coded routes into centralized constants (resolves #747)#1127BenjaminMichaelis wants to merge 3 commits into
BenjaminMichaelis wants to merge 3 commits into
Conversation
- Add Constants/RouteConstants.cs with StaticPages string constants, NonContentRoutes HashSet, and SeoMetadata dictionary for sitemap config - Add src/constants/routes.js mirroring the C# constants for the frontend, exporting ROUTES, NON_CONTENT_ROUTES, NAVIGATION_LINKS, and isContentPagePath - Refactor useSiteShell.js isContentPage to use NON_CONTENT_ROUTES.some() with Array path matching as suggested in issue #747, with percentComplete as a secondary check - Refactor SidebarPanel.vue to import NAVIGATION_LINKS from routes.js, removing the inline hard-coded navLinks definition - Refactor SitemapXmlHelpers.cs to use RouteConstants.SeoMetadata.RouteConfig dictionary lookups instead of switch expressions - Update HomeController.cs Route attributes to reference RouteConstants.StaticPages string constants instead of hard-coded string literals
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes previously hard-coded route strings into shared constants on both the frontend and backend to reduce brittleness and simplify future route updates (including the isContentPage logic called out in #747).
Changes:
- Added centralized route constants/config for backend (
RouteConstants.cs) and frontend (routes.js). - Updated frontend consumers (
useSiteShell.js,SidebarPanel.vue) to use shared route/navigation constants. - Updated sitemap and controller routing code (
SitemapXmlHelpers.cs,HomeController.cs) to reference centralized constants.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| EssentialCSharp.Web/src/constants/routes.js | Adds frontend route constants, non-content route list, sidebar navigation definitions, and a reusable isContentPagePath() helper. |
| EssentialCSharp.Web/src/composables/useSiteShell.js | Switches isContentPage detection to use centralized NON_CONTENT_ROUTES logic. |
| EssentialCSharp.Web/src/components/SidebarPanel.vue | Replaces inline nav link definitions with imported NAVIGATION_LINKS. |
| EssentialCSharp.Web/Helpers/SitemapXmlHelpers.cs | Replaces switch expressions with lookup into centralized route SEO metadata config. |
| EssentialCSharp.Web/Controllers/HomeController.cs | Replaces hard-coded [Route] templates with RouteConstants.StaticPages.*. |
| EssentialCSharp.Web/Constants/RouteConstants.cs | Introduces backend centralized static routes, non-content route set, and sitemap SEO route metadata mapping. |
Comments suppressed due to low confidence (3)
EssentialCSharp.Web/Helpers/SitemapXmlHelpers.cs:97
- Same key-normalization issue as
GetChangeFrequencyForRoute:normalizedRoutestrips the leading '/', butRouteConstants.SeoMetadata.RouteConfigkeys include it (e.g./guidelines). This makes the lookup always miss and forces the default priority (0.5) for every controller route.
private static decimal GetPriorityForRoute(string route)
{
var normalizedRoute = route.TrimStart('/').ToLowerInvariant();
if (RouteConstants.SeoMetadata.RouteConfig.TryGetValue(normalizedRoute, out var config))
{
return config.Item2;
}
return 0.5M;
EssentialCSharp.Web/Helpers/SitemapXmlHelpers.cs:85
- There are existing tests for
GenerateSitemapXml, but none appear to validate the per-routeChangeFrequency/Priorityvalues for controller routes (e.g.,/guidelinespriority 0.9,/termsofserviceyearly). Adding assertions for these values would prevent regressions in the newRouteConfiglookup/normalization logic.
private static ChangeFrequency GetChangeFrequencyForRoute(string route)
{
var normalizedRoute = route.TrimStart('/').ToLowerInvariant();
if (RouteConstants.SeoMetadata.RouteConfig.TryGetValue(normalizedRoute, out var config))
{
return (ChangeFrequency)(int)config.Item1;
}
return ChangeFrequency.Monthly;
EssentialCSharp.Web/Constants/RouteConstants.cs:75
IsContentPagecurrently does a directNonContentRoutes.Contains(path)check, so paths like/home/(trailing slash) or mixed-case inputs will be misclassified as content pages even though the frontend logic treats them as static. Consider normalizingpath(trim whitespace, ensure leading '/', remove trailing '/', and handlenull/empty) before checking the set.
/// <summary>
/// Determines if the given path represents a content page (from sitemap)
/// versus a static page (non-content).
/// </summary>
public static bool IsContentPage(string path)
{
return !NonContentRoutes.Contains(path);
}
Comment on lines
76
to
+85
| private static ChangeFrequency GetChangeFrequencyForRoute(string route) | ||
| { | ||
| return route.ToLowerInvariant() switch | ||
| var normalizedRoute = route.TrimStart('/').ToLowerInvariant(); | ||
|
|
||
| if (RouteConstants.SeoMetadata.RouteConfig.TryGetValue(normalizedRoute, out var config)) | ||
| { | ||
| "/termsofservice" => ChangeFrequency.Yearly, | ||
| "/announcements" => ChangeFrequency.Monthly, | ||
| "/guidelines" => ChangeFrequency.Monthly, | ||
| _ => ChangeFrequency.Monthly | ||
| }; | ||
| return (ChangeFrequency)(int)config.Item1; | ||
| } | ||
|
|
||
| return ChangeFrequency.Monthly; |
Comment on lines
+42
to
+64
| public enum ChangeFrequency | ||
| { | ||
| Always, | ||
| Hourly, | ||
| Daily, | ||
| Weekly, | ||
| Monthly, | ||
| Yearly, | ||
| Never | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Maps route paths to (ChangeFrequency, Priority) tuples for sitemap.xml generation. | ||
| /// Priority is a decimal value between 0.0 and 1.0 (0.5 is the default). | ||
| /// </summary> | ||
| public static readonly Dictionary<string, (ChangeFrequency, decimal)> RouteConfig = | ||
| new(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| { StaticPages.Home, (ChangeFrequency.Monthly, 0.5m) }, | ||
| { StaticPages.About, (ChangeFrequency.Monthly, 0.5m) }, | ||
| { StaticPages.Guidelines, (ChangeFrequency.Monthly, 0.9m) }, | ||
| { StaticPages.Announcements, (ChangeFrequency.Monthly, 0.5m) }, | ||
| { StaticPages.TermsOfService, (ChangeFrequency.Yearly, 0.2m) } |
Comment on lines
+36
to
+58
| activePaths: ['/', ROUTES.HOME], | ||
| key: 'home' | ||
| }, | ||
| { | ||
| href: ROUTES.ABOUT, | ||
| label: 'About', | ||
| iconClass: 'fas fa-book me-2', | ||
| activePaths: [ROUTES.ABOUT], | ||
| key: 'about' | ||
| }, | ||
| { | ||
| href: ROUTES.GUIDELINES, | ||
| label: 'Guidelines', | ||
| iconClass: 'fas fa-code me-2', | ||
| activePaths: [ROUTES.GUIDELINES], | ||
| key: 'guidelines' | ||
| }, | ||
| { | ||
| href: ROUTES.ANNOUNCEMENTS, | ||
| label: 'Announcements', | ||
| iconClass: 'fas fa-bullhorn me-2', | ||
| activePaths: [ROUTES.ANNOUNCEMENTS], | ||
| key: 'announcements' |
Comment on lines
+76
to
+89
| /** | ||
| * Determines if the current page is a content page (from sitemap) or a static page. | ||
| * Checks against the NON_CONTENT_ROUTES array and falls back to percentComplete check | ||
| * for additional verification. | ||
| */ | ||
| const isContentPage = computed(() => { | ||
| const currentPath = window.location.pathname.toLowerCase(); | ||
| const isStaticPage = NON_CONTENT_ROUTES.some(route => | ||
| currentPath === route.toLowerCase() || | ||
| currentPath.startsWith(route.toLowerCase() + '/') | ||
| ); | ||
|
|
||
| return !isStaticPage && percentComplete.value !== null; | ||
| }); |
- Use DotnetSitemapGenerator.ChangeFrequency directly in RouteConfig,
eliminating the fragile local enum and the (ChangeFrequency)(int) cast
- Use FrozenSet<string> and FrozenDictionary for NonContentRoutes and
RouteConfig to make collections truly immutable
- Remove TrimStart('/') from GetChangeFrequencyForRoute and
GetPriorityForRoute so dictionary keys ('/home', etc.) actually match
- Use isContentPagePath() from routes.js in useSiteShell.js instead of
duplicating the same inline logic
- Document why TERMS_OF_SERVICE is excluded from NAVIGATION_LINKS
- Add per-route SEO metadata tests to SitemapXmlHelpersTests
Pass StringComparer.OrdinalIgnoreCase explicitly to ToFrozenSet() to guarantee case-insensitive lookup, rather than relying on comparer propagation from the source HashSet.
Comment on lines
+76
to
+83
| /** | ||
| * Determines if the current page is a content page (from sitemap) or a static page. | ||
| * Checks against the NON_CONTENT_ROUTES array via isContentPagePath and falls back | ||
| * to percentComplete check for additional verification that content data is loaded. | ||
| */ | ||
| const isContentPage = computed(() => { | ||
| return isContentPagePath(window.location.pathname) && percentComplete.value !== null; | ||
| }); |
Comment on lines
+60
to
64
| [Route(RouteConstants.StaticPages.Announcements, Name = "Announcements")] | ||
| public IActionResult Announcements() | ||
| { | ||
| ViewBag.PageTitle = "Announcements"; | ||
| return View(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Hard-coded route strings were scattered across multiple frontend and backend files (
useSiteShell.js,SidebarPanel.vue,SitemapXmlHelpers.cs,HomeController.cs), making route changes brittle and error-prone. Issue #747 specifically called out theisContentPagelogic and suggested extracting routes into a constant and usingArray.includes()for clarity.Approach
Two new constant files serve as the single source of truth per layer:
Backend --
EssentialCSharp.Web/Constants/RouteConstants.csStaticPagesinner class withconst stringvalues for each non-content routeNonContentRoutesHashSet for O(1) membership checksSeoMetadata.RouteConfigdictionary mapping routes to(ChangeFrequency, priority)tuples, replacing the two switch expressions inSitemapXmlHelpers.csFrontend --
EssentialCSharp.Web/src/constants/routes.jsROUTESobject with named route constantsNON_CONTENT_ROUTESarray forArray.includes()/.some()checksNAVIGATION_LINKSarray (with icon class, label, activePaths) replacing the inline definition inSidebarPanel.vueisContentPagePath()helper function for reuseConsumers updated:
useSiteShell.js--isContentPagenow usesNON_CONTENT_ROUTES.some()as the primary check, withpercentComplete !== nullas a secondary guardSidebarPanel.vue-- importsNAVIGATION_LINKSinstead of defining the array inlineSitemapXmlHelpers.cs--GetChangeFrequencyForRoute/GetPriorityForRouteuse dictionary lookup instead of switch expressionsHomeController.cs--[Route()]attributes referenceRouteConstants.StaticPagesconstantsNotes
percentComplete !== nullfallback is intentionally preserved inisContentPageso that pages not in the static list still correctly opt in/out of content-page features based on server-side data.Closes #747