-
Notifications
You must be signed in to change notification settings - Fork 216
feat(router): add notFound fallback — renders DefaultNotFound on unmatched routes (#2120) #2146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| ### `notFound` — Custom 404 fallback | ||
|
|
||
| **Type:** `React.ComponentType<{ path: string }>` | optional | ||
|
|
||
| When navigation targets a path with no registered route, the router renders | ||
| the `notFound` component instead of a blank screen. If `notFound` is not | ||
| provided, the built-in `DefaultNotFound` screen is used. | ||
|
|
||
| **Default behavior** (no `notFound` prop): | ||
|
|
||
| ```tsx | ||
| // Navigating to an unregistered path renders: | ||
| // ╭─────────────────────────────╮ | ||
| // │ Route Not Found │ | ||
| // │ │ | ||
| // │ No screen is registered for: /settings/unknown | ||
| // │ │ | ||
| // │ Press Backspace to go back · Press Q to quit | ||
| // ╰─────────────────────────────╯ | ||
| ``` | ||
|
|
||
| **Custom fallback:** | ||
|
|
||
| ```tsx | ||
| <Router | ||
| config={{ | ||
| routes: [ | ||
| { path: '/', component: Home }, | ||
| { path: '/about', component: About }, | ||
| ], | ||
| notFound: ({ path }) => ( | ||
| <Box border="round"> | ||
| <Text color="red">No screen at {path}</Text> | ||
| <Text dim>Press Q to quit</Text> | ||
| </Box> | ||
| ), | ||
| }} | ||
| /> | ||
| ``` | ||
|
|
||
| **Importing `DefaultNotFound` directly** (if you want to compose it): | ||
|
|
||
| ```tsx | ||
| import { DefaultNotFound } from '@termuijs/router'; | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // packages/router/src/DefaultNotFound.tsx | ||
| // New file for Issue #2120 — default fallback rendered when no route matches | ||
|
|
||
| import { Box, Text } from '@termuijs/widgets'; | ||
| import { useKeymap } from '@termuijs/jsx'; | ||
| import { useRouter } from './useRouter'; | ||
|
|
||
| export interface NotFoundProps { | ||
| /** The path that was requested but had no registered route */ | ||
| path: string; | ||
| } | ||
|
|
||
| /** | ||
| * DefaultNotFound | ||
| * | ||
| * Rendered automatically by <Router> when navigation targets an unregistered path. | ||
| * Users can override this by passing a custom `notFound` component to RouterConfig. | ||
| * | ||
| * Key bindings: | ||
| * Backspace / b → navigate(-1) — go back to the previous screen | ||
| * q → process.exit(0) — quit the terminal app | ||
| */ | ||
| export function DefaultNotFound({ path }: NotFoundProps) { | ||
| const { navigate } = useRouter(); | ||
|
|
||
| useKeymap({ | ||
| // Go back to the previous route | ||
| 'backspace': () => navigate(-1), | ||
| 'b': () => navigate(-1), | ||
| // Quit the app (standard terminal convention) | ||
| 'q': () => process.exit(0), | ||
| 'ctrl+c': () => process.exit(0), | ||
| }); | ||
|
|
||
| return ( | ||
| <Box | ||
| border="round" | ||
| padding={1} | ||
| flexDirection="column" | ||
| gap={1} | ||
| > | ||
| {/* Title */} | ||
| <Text bold color="red"> | ||
| Route Not Found | ||
| </Text> | ||
|
|
||
| {/* The unmatched path */} | ||
| <Text> | ||
| {'No screen is registered for: '} | ||
| <Text bold color="yellow">{path}</Text> | ||
| </Text> | ||
|
|
||
| {/* Help text */} | ||
| <Text dim> | ||
| Press Backspace to go back · Press Q to quit | ||
| </Text> | ||
| </Box> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||||
| // packages/router/src/DefaultNotFound.tsx | ||||||
| import { type VNode } from '@termuijs/jsx'; | ||||||
|
|
||||||
| export function DefaultNotFound({ path }: { path: string }): VNode { | ||||||
| return { | ||||||
| type: 'box', | ||||||
| props: { | ||||||
| border: 'single', | ||||||
| borderColor: 'yellow', | ||||||
| padding: 2, | ||||||
| flexDirection: 'column', | ||||||
| alignItems: 'center', | ||||||
| justifyContent: 'center', | ||||||
| }, | ||||||
| children: [ | ||||||
| { | ||||||
| type: 'text', | ||||||
| props: { color: 'yellow', bold: true, size: 2 }, | ||||||
| children: ['404 – Page Not Found'] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Hardcoded non-ASCII character without The en dash 🌐 Suggested fix- children: ['404 – Page Not Found']
+ children: [caps.unicode ? '404 – Page Not Found' : '404 - Page Not Found']📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||
| }, | ||||||
| { | ||||||
| type: 'text', | ||||||
| props: { color: 'gray', dim: true }, | ||||||
| children: [`Path: ${path}`] | ||||||
| }, | ||||||
| { | ||||||
| type: 'text', | ||||||
| props: { color: 'gray' }, | ||||||
| children: ['The route you are looking for does not exist.'] | ||||||
| } | ||||||
| ] | ||||||
| } as any; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
As per coding guidelines, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||
| } | ||||||
|
Comment on lines
+4
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'packages/router/src/*NotFound*' 'packages/data/src/*NotFound*' 'packages/router/src/router.ts'
printf '\n== outline: router DefaultNotFound ==\n'
ast-grep outline packages/router/src/DefaultNotFound.tsx --view expanded || true
printf '\n== outline: data DefaultNotFound ==\n'
ast-grep outline packages/data/src/DefaultNotFound.tsx --view expanded || true
printf '\n== outline: router.ts ==\n'
ast-grep outline packages/router/src/router.ts --view expanded || true
printf '\n== router DefaultNotFound snippet ==\n'
sed -n '1,220p' packages/router/src/DefaultNotFound.tsx
printf '\n== data DefaultNotFound snippet ==\n'
sed -n '1,260p' packages/data/src/DefaultNotFound.tsx
printf '\n== router.ts relevant snippet ==\n'
sed -n '1,260p' packages/router/src/router.ts
printf '\n== keymap / notfound references ==\n'
rg -n "useKeymap|Backspace|ctrl\\+c|\\bq\\b|DefaultNotFound|NotFound" packages/router/src packages/data/srcRepository: Karanjot786/TermUI Length of output: 19757 Restore the interactive not-found fallback in 🤖 Prompt for AI Agents |
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| // packages/router/src/__tests__/not-found.test.ts | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win File must be Static analysis confirms this: parse errors on every JSX-containing line (9, 10, 24, 32, 43, 47, 61, 76). This file will fail to compile/run as-is. 🐛 Proposed fixRename Also applies to: 9-10, 22-81 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| // Tests for Issue #2120 — 404 fallback behavior | ||
| import { describe, it, expect } from 'vitest'; | ||
| import { render } from '@termuijs/testing'; | ||
| import { Router } from '../Router'; | ||
| import { Text } from '@termuijs/widgets'; | ||
|
Comment on lines
+3
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift Tests should use the real As per coding guidelines, Also applies to: 23-81 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| // ── Minimal test screens ────────────────────────────────────────────────────── | ||
| function HomeScreen() { return <Text>Home Screen</Text>; } | ||
| function AboutScreen() { return <Text>About Screen</Text>; } | ||
| // ───────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| const baseConfig = { | ||
| routes: [ | ||
| { path: '/', component: HomeScreen }, | ||
| { path: '/about', component: AboutScreen }, | ||
| ], | ||
| }; | ||
|
|
||
| describe('Router: notFound fallback (Issue #2120)', () => { | ||
|
|
||
| it('renders the matched component for a registered route', () => { | ||
| const t = render( | ||
| <Router config={{ ...baseConfig, initialRoute: '/' }} /> | ||
| ); | ||
| expect(t.getByText('Home Screen')).toBeTruthy(); | ||
| t.unmount(); | ||
| }); | ||
|
|
||
| it('renders DefaultNotFound for an unregistered route', () => { | ||
| const t = render( | ||
| <Router config={{ ...baseConfig, initialRoute: '/does-not-exist' }} /> | ||
| ); | ||
| // DefaultNotFound must show the "Route Not Found" title | ||
| expect(t.getByText('Route Not Found')).toBeTruthy(); | ||
| // And must display the unmatched path | ||
| expect(t.getByText('/does-not-exist')).toBeTruthy(); | ||
| t.unmount(); | ||
| }); | ||
|
|
||
| it('renders a custom notFound component when config.notFound is provided', () => { | ||
| function CustomNotFound({ path }: { path: string }) { | ||
| return <Text>Custom 404 for {path}</Text>; | ||
| } | ||
|
|
||
| const t = render( | ||
| <Router config={{ | ||
| ...baseConfig, | ||
| initialRoute: '/unknown', | ||
| notFound: CustomNotFound, | ||
| }} /> | ||
| ); | ||
| expect(t.getByText('Custom 404 for /unknown')).toBeTruthy(); | ||
| // Confirm the default NotFound is NOT shown | ||
| expect(() => t.getByText('Route Not Found')).toThrow(); | ||
| t.unmount(); | ||
| }); | ||
|
|
||
| it('shows correct unmatched path in DefaultNotFound after navigate()', async () => { | ||
| const t = render( | ||
| <Router config={{ ...baseConfig, initialRoute: '/' }} /> | ||
| ); | ||
|
|
||
| // Navigate to a route that doesn't exist | ||
| t.navigate('/settings/profile/missing'); | ||
|
|
||
| await t.waitFor(() => { | ||
| expect(t.getByText('Route Not Found')).toBeTruthy(); | ||
| expect(t.getByText('/settings/profile/missing')).toBeTruthy(); | ||
| }); | ||
| t.unmount(); | ||
| }); | ||
|
|
||
| it('DefaultNotFound renders the help text with key hints', () => { | ||
| const t = render( | ||
| <Router config={{ ...baseConfig, initialRoute: '/not-real' }} /> | ||
| ); | ||
| // The help text line must be visible | ||
| expect(t.getByText('Press Backspace to go back · Press Q to quit')).toBeTruthy(); | ||
| t.unmount(); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: Karanjot786/TermUI
Length of output: 291
🏁 Script executed:
Repository: Karanjot786/TermUI
Length of output: 9068
🏁 Script executed:
Repository: Karanjot786/TermUI
Length of output: 4401
Move
DefaultNotFoundintopackages/routerpackages/data/src/DefaultNotFound.tsximports./useRouter, but that module doesn’t exist inpackages/data;useRouterlives inpackages/router/src/hooks.ts. This component belongs inpackages/router/src/DefaultNotFound.tsx, replacing the stale placeholder there.🤖 Prompt for AI Agents