feat(ui): add generic not-found page for unrecognized urls - #1340
feat(ui): add generic not-found page for unrecognized urls#1340MRashad26 wants to merge 9 commits into
Conversation
LFXV2-2010 — adds a branded 404 experience via a new `NotFoundComponent` wired to the Angular `**` wildcard route. The page renders inside a card with a map-location-dot icon, a generic "Page not found" heading, and two CTAs: "Go to Dashboard" (routerLink to /) and "Go Back" (Location.back()). Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
PR SummaryLow Risk Overview A new Routing adds a terminal Reviewed by Cursor Bugbot for commit 42eead4. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a branded Angular fallback page for unmatched URLs.
Changes:
- Adds a generic 404 component with dashboard and back actions.
- Registers a final wildcard route.
- Supports lazy loading and anonymous client-side navigation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
not-found.component.ts |
Implements back navigation. |
not-found.component.html |
Defines the branded 404 UI and actions. |
app.routes.ts |
Registers the wildcard fallback route. |
Suppressed comments (1)
apps/lfx-one/src/app/app.routes.ts:491
- This renders the 404 UI but still returns HTTP 200 for a direct SSR request:
app.routes.server.tsleaves its**entry at the default status. That creates a soft 404 for crawlers, caches, and monitoring. Use the established/docs/not-foundapproach (a dedicated route withstatus: 404, reached by redirecting the client wildcard) or an SSR response-status mechanism, and verify the response status in E2E.
path: '**',
loadComponent: () => import('./modules/not-found/not-found.component').then((m) => m.NotFoundComponent),
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-1340.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
🧹 Deployment RemovedThe deployment for PR #1340 has been removed. |
LFXV2-2010 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/lfx-one/src/app/app.routes.ts:488
- Moving this Angular route outside
authGuarddoes not make direct requests anonymous.auth.middleware.tsclassifies every unmatched SSR path with the required-auth/fallback and redirects an anonymous GET to login before Angular can match this wildcard. As a result, the advertised anonymous invalid-URL flow never reaches this page. Add server-side handling that permits only genuinely unrecognized URLs to render the 404, without making the required-auth catch-all public.
// Generic 404 — catches all unrecognized URLs and renders a branded not-found page. Must be
// last so it never shadows real routes. Outside the auth guard so anonymous users see it too.
apps/lfx-one/src/app/app.routes.ts:491
- This wildcard selects the component but does not set the SSR response status. The matching
**entry inapp.routes.server.tshas nostatus, so a direct invalid URL is returned as HTTP 200 (a soft 404); the existingdocs/not-foundroute demonstrates the requiredstatus: 404behavior. Set 404 only when this Angular wildcard is matched—do not add 404 to the server-wide**entry, which also renders valid routes.
path: '**',
loadComponent: () => import('./modules/not-found/not-found.component').then((m) => m.NotFoundComponent),
audigregorie
left a comment
audigregorie
left a comment
There was a problem hiding this comment.
Code Review Summary
Clean, well-scoped PR that faithfully mirrors the established AuthErrorComponent pattern — the ** wildcard is correctly placed last and outside the auth guard with a clear "why" comment. One Major worth addressing: Location.back() has no fallback for the page's own described scenario (deep-linked/stale-bookmark 404s have no in-app history, so "Go Back" navigates away from LFX).
Outside the diff
- Optional E2E coverage. The component ships
data-testidhooks (not-found-card,not-found-title,not-found-dashboard-button,not-found-back-button) ready for the dual-architecture E2E suite. A small*.spec.tsasserting the wildcard route renders the card and the dashboard button navigates to/would lock this in — not blocking for a 3-file UI PR, but low-cost and high-value.
What's done well
**wildcard is last and outside the auth guard, with a clear intent comment — won't shadow real routes and anonymous users see it.- Component mirrors the
AuthErrorComponentshell (lfx-header+lfx-card,inject()style,data-testidconvention) — easy to maintain. - Tailwind scale classes (
gray,amber) instead of hard-coded hex, perstyling.md.
Address review comments from @copilot-pull-request-reviewer, @cursor: - app.routes.ts: replace direct ** wildcard component load with a named /not-found route + ** redirectTo '/not-found', so the 404 page has a stable public URL regardless of the originating path - auth.middleware.ts: add /not-found as auth: 'public' (regex-anchored) so unauthenticated users can reach the branded 404 without being redirected to login — same pattern as auth-error and invite/error - app.routes.server.ts: add /not-found server route with status: 404 so SSR returns the correct HTTP status instead of a soft-404 (HTTP 200), fixing crawler, cache, and monitoring behaviour Resolves 2 review threads. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 0ed3f2a Changes Made
Threads Resolved2 of 2 unresolved threads addressed in this iteration. |
Address review comments from @audigregorie: - not-found.component.ts: guard Location.back() with isPlatformBrowser + window.history.length > 1; fall back to router.navigate(['/']) when there is no in-app history (direct link / stale bookmark scenario described in the page copy) (per @audigregorie — Major) - not-found.component.html: add aria-hidden="true" to the decorative warning icon so screen readers skip it (per @audigregorie — Minor) Resolves 2 review threads. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 408bea8 Changes Made
Threads Resolved2 of 2 unresolved threads addressed in this iteration. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
apps/lfx-one/src/server/middleware/auth.middleware.ts:92
- This pattern excludes
/not-found/, so an anonymous request with a trailing slash falls through to the required-auth/rule and is sent to login instead of the public 404. The neighboringauth-errorand public-profile regexes explicitly accept an optional trailing slash; apply the same exact-but-slash-tolerant match here.
{ pattern: /^\/not-found$/, type: 'ssr', auth: 'public' },
Review Feedback AddressedNo Change Needed
Threads Resolved1 of 1 unresolved threads addressed in this iteration. |
…anonymous-user behavior Co-authored-by: MRashad26 <81649923+MRashad26@users.noreply.github.com>
Scoped to the second option from the reviewer: the Updated the route comments in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/lfx-one/src/app/app.routes.ts:499
- Anonymous requests to arbitrary unknown URLs still never execute this redirect.
authMiddlewareruns before Angular SSR (server.ts:304), classifies the original path via the required/catch-all (auth.middleware.ts:94-95), and sends unauthenticated SSR GETs to login (auth.middleware.ts:361-374). The new public rule only helps requests already targeting/not-found, so/foo/barstill shows a login prompt rather than the branded 404 promised by this route. Unknown SSR paths need to be identified/rendered before the required-auth decision while keeping recognized protected routes fail-closed.
},
// Generic catch-all — redirects unrecognized URLs to /not-found so authenticated users get a
apps/lfx-one/src/server/middleware/auth.middleware.ts:92
- This new public-auth boundary is not covered by
auth.middleware.spec.ts, even though that suite regression-tests analogous anchored public rules and their fail-closed variants. Add cases proving anonymous/not-foundis allowed and a prefix such as/not-found-adminremains protected; otherwise a future pattern/order change can silently expose a protected route or make the 404 inaccessible.
{ pattern: /^\/not-found$/, type: 'ssr', auth: 'public' },
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/app/modules/not-found/not-found.component.ts:23
history.lengthcounts the tab's entire session history, including entries from other sites, so it cannot distinguish an in-app navigation from a direct link or stale bookmark opened in a reused tab. In that common direct-entry case this branch callsback()and sends the user away from LFX instead of using the promised dashboard fallback. Base this decision on Angular navigation state or an explicitly tracked same-app previous URL, falling back to/when no LFX navigation exists.
if (isPlatformBrowser(this.platformId) && window.history.length > 1) {
this.location.back();
apps/lfx-one/src/server/middleware/auth.middleware.ts:92
- This new public-route exception changes the authentication boundary but has no corresponding middleware test. Add coverage proving anonymous
/not-foundis allowed and a lookalike such as/not-found-adminstill falls through to required auth, matching the existing anchored-regex tests inauth.middleware.spec.ts. This prevents a future regex/order edit from silently widening or breaking the public surface.
{ pattern: /^\/not-found$/, type: 'ssr', auth: 'public' },
apps/lfx-one/src/app/app.routes.server.ts:21
- The new 404 status mapping and wildcard redirect are not covered by an automated browser test, even though the analogous
docs/not-foundstatus contract is asserted ine2e/docs/public-access.spec.ts:101-107. Add Playwright coverage that requests an unknown authenticated URL, verifies the final/not-foundUI/status behavior, and directly requests/not-foundanonymously. Without it, a route-order or SSR redirect regression can silently restore a soft 200 or login loop.
path: 'not-found',
renderMode: RenderMode.Server,
status: 404,
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
The My Meetings shortcut link has no place on standalone pages (not-found, auth-error, invite, groups) that use lfx-header outside the main app layout. The avatar menu and logo link are sufficient for navigation on these pages. LFXV2-2010 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Add showMyMeetings input (default true) to HeaderComponent so callers can opt out. Set it to false on the not-found page — the CTA has no purpose on a 404 error page. All other standalone pages (groups, auth-error, invite) retain the default and keep the link. LFXV2-2010 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
apps/lfx-one/src/app/shared/components/header/header.component.ts:33
- Consider typing the input signal explicitly (e.g., boolean) so template bindings can’t accidentally pass non-boolean values (like strings) without being caught by type checking.
public readonly showMyMeetings = input(true);
apps/lfx-one/src/app/app.routes.ts:506
- Using an absolute
redirectTo(leading '/') can be problematic with non-root base-hrefs and is less consistent with typical Angular route redirects. PreferredirectTo: 'not-found'for a root-level wildcard redirect.
{
path: '**',
redirectTo: '/not-found',
},
| @Component({ | ||
| selector: 'lfx-not-found', | ||
| imports: [HeaderComponent, CardComponent, ButtonComponent, RouterLink], | ||
| templateUrl: './not-found.component.html', | ||
| }) |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 42eead4. Configure here.
| } else { | ||
| void this.router.navigate(['/']); | ||
| } | ||
| } |
There was a problem hiding this comment.
Go Back leaves site
Medium Severity
goBack() treats window.history.length > 1 as in-app history, but that count includes prior external sites. For the common case where someone opens a broken LFX link from email, Slack, or another site, location.back() still leaves LFX instead of using the dashboard fallback the page copy implies.
Reviewed by Cursor Bugbot for commit 42eead4. Configure here.
| // Generic not-found page — public so anonymous users land on a branded 404 instead of a login | ||
| // prompt when redirected here from an unrecognized URL. Anchored regex prevents startsWith | ||
| // semantics from matching /not-found-admin or similar future paths. | ||
| { pattern: /^\/not-found$/, type: 'ssr', auth: 'public' }, |
There was a problem hiding this comment.
Trailing slash auth gap
Low Severity
The public /not-found auth pattern is anchored as /^\/not-found$/ with no optional trailing slash, unlike the sibling /^\/auth-error\/?$/ pattern. A request to /not-found/ therefore falls through to the required-auth catch-all and sends anonymous users to login instead of the branded public 404.
Reviewed by Cursor Bugbot for commit 42eead4. Configure here.
Summary
NotFoundComponentwith a branded 404 card (amber warning-triangle icon, "Page not found" heading, generic copy about mistyped URLs or stale bookmarks)/not-foundis a named public route classified asauth: 'public'in the Express middleware (same pattern asauth-errorandinvite/error) — accessible without login**wildcard doesredirectTo: '/not-found'so authenticated users who mistype a URL land on the branded page with a clean URLapp.routes.server.tssetsstatus: 404on thenot-foundserver route so SSR emits the correct HTTP status (not a soft 200)isPlatformBrowser + window.history.length > 1; falls back torouter.navigate(['/'])for direct links and stale bookmarksRefs: LFXV2-2010
Scope note: The primary use case is authenticated users who mistype a URL — they see the branded 404 immediately. Anonymous users with a wrong URL go through login first (consistent with every other protected route in this auth-first app) and land on
/not-foundafter authenticating. This is a deliberate architectural constraint: Angular'sauthGuardreturnstrueon the server (trusting Express middleware), so making the catch-alloptionalwould render protected SSR components without auth context.Test plan
/foundation/tlf/groups,/foo/bar) while logged in — should show the not-found card at/not-foundwith HTTP 404/not-founddirectly while not logged in — should show the branded card without a login redirect/(dashboard fallback)//,/foundation/overview,/foundations/tlf/groups) still work as expected