-
Notifications
You must be signed in to change notification settings - Fork 3
Add nested dynamic route for crypto endpoints with separate symbol/currency parameters #7
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
Conversation
Co-authored-by: RobinMillford <99179344+RobinMillford@users.noreply.github.com>
Co-authored-by: RobinMillford <99179344+RobinMillford@users.noreply.github.com>
… validation Co-authored-by: RobinMillford <99179344+RobinMillford@users.noreply.github.com>
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.
Pull request overview
This pull request adds support for a nested dynamic route pattern /crypto/[symbol]/[currency] that redirects to the existing /crypto/[symbol%2Fcurrency] format. This addresses a 404 error that occurred when users accessed crypto detail pages with separated path segments instead of URL-encoded slashes.
Key Changes:
- Added a client-side redirect component that combines separate symbol/currency parameters into a slash-separated, URL-encoded format
- Implemented basic input validation to ensure parameters are non-empty strings
- Added E2E tests to verify both URL patterns work correctly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
app/crypto/[symbol]/[currency]/page.tsx |
New redirect component that transforms /crypto/888/USD → /crypto/888%2FUSD using router.replace() |
e2e/crypto-routes.spec.ts |
New E2E test suite validating both nested route and standard URL-encoded route patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if (symbol && currency) { | ||
| // Validate that symbol and currency are strings and not empty | ||
| if (typeof symbol === 'string' && typeof currency === 'string' && | ||
| symbol.trim().length > 0 && currency.trim().length > 0) { | ||
| // Combine symbol and currency into the format expected by the main crypto page | ||
| // e.g., /crypto/888/USD -> /crypto/888%2FUSD | ||
| const combinedSymbol = `${symbol}/${currency}`; | ||
| const encodedSymbol = encodeURIComponent(combinedSymbol); | ||
| router.replace(`/crypto/${encodedSymbol}`); | ||
| } | ||
| } |
Copilot
AI
Dec 17, 2025
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.
The redirect logic doesn't handle the case where symbol or currency parameters are missing or invalid. If the validation on lines 15-16 fails, the component will just display "Redirecting..." indefinitely without actually redirecting or showing an error message to the user.
Consider adding an else clause that redirects to the crypto listings page or shows an error message:
if (symbol && currency && symbol.trim().length > 0 && currency.trim().length > 0) {
const combinedSymbol = `${symbol}/${currency}`;
const encodedSymbol = encodeURIComponent(combinedSymbol);
router.replace(`/crypto/${encodedSymbol}`);
} else {
// Redirect to crypto listings if parameters are invalid
router.replace('/cryptos');
}| test.describe('Crypto Routes', () => { | ||
| test('should handle /crypto/[symbol]/[currency] route', async ({ page }) => { | ||
| // Test the nested route pattern /crypto/888/USD | ||
| await page.goto('/crypto/888/USD'); | ||
|
|
||
| // Wait for the redirect to complete by waiting for the URL change | ||
| await page.waitForURL('**/crypto/888%2FUSD', { timeout: 5000 }); | ||
|
|
||
| // Check that we were redirected to the correct URL format | ||
| const url = page.url(); | ||
| expect(url).toContain('/crypto/888%2FUSD'); | ||
| }); | ||
|
|
||
| test('should handle standard crypto route format', async ({ page }) => { | ||
| // Test the standard route format with URL-encoded slash | ||
| await page.goto('/crypto/BTC%2FUSD'); | ||
|
|
||
| // Wait for page to load | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Verify we're on the crypto page | ||
| const url = page.url(); | ||
| expect(url).toContain('/crypto/BTC%2FUSD'); | ||
| }); | ||
| }); |
Copilot
AI
Dec 17, 2025
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.
The test suite doesn't cover edge cases for the redirect functionality. Consider adding tests for:
- Invalid parameters (empty symbol or currency)
- Special characters in symbol/currency that need proper URL encoding
- Parameters with whitespace
- Parameters that contain forward slashes already
These edge cases are important to test since the redirect component handles URL encoding and parameter validation.
| // Wait for the redirect to complete by waiting for the URL change | ||
| await page.waitForURL('**/crypto/888%2FUSD', { timeout: 5000 }); |
Copilot
AI
Dec 17, 2025
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.
The comment on line 8 says "Wait for the redirect to complete by waiting for the URL change" but the next line waits for a specific URL pattern. Consider updating the comment to be more specific about what's being waited for:
// Wait for client-side redirect from /crypto/888/USD to /crypto/888%2FUSD| const combinedSymbol = `${symbol}/${currency}`; | ||
| const encodedSymbol = encodeURIComponent(combinedSymbol); | ||
| router.replace(`/crypto/${encodedSymbol}`); |
Copilot
AI
Dec 17, 2025
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.
While encodeURIComponent is used for URL encoding, there's no sanitization of the symbol and currency parameters before they're combined. Malicious input could potentially include characters that, while URL-encoded, might cause issues downstream or in logging.
Consider adding input validation to ensure symbol and currency only contain expected characters (alphanumeric and common separators):
const isValidInput = (str: string) => /^[A-Za-z0-9._-]+$/.test(str);
if (isValidInput(symbol) && isValidInput(currency)) {
const combinedSymbol = `${symbol}/${currency}`;
router.replace(`/crypto/${encodeURIComponent(combinedSymbol)}`);
}| <div className="min-h-screen bg-background flex items-center justify-center"> | ||
| <p className="text-muted-foreground">Redirecting...</p> | ||
| </div> |
Copilot
AI
Dec 17, 2025
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.
The "Redirecting..." message on line 28 lacks accessibility attributes. Screen readers may not announce this loading state properly, leaving visually impaired users unaware of what's happening.
Consider adding ARIA attributes:
<div className="min-h-screen bg-background flex items-center justify-center" role="status" aria-live="polite">
<p className="text-muted-foreground" aria-label="Redirecting to cryptocurrency page">Redirecting...</p>
</div>| test('should handle standard crypto route format', async ({ page }) => { | ||
| // Test the standard route format with URL-encoded slash | ||
| await page.goto('/crypto/BTC%2FUSD'); | ||
|
|
||
| // Wait for page to load | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Verify we're on the crypto page | ||
| const url = page.url(); | ||
| expect(url).toContain('/crypto/BTC%2FUSD'); | ||
| }); |
Copilot
AI
Dec 17, 2025
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.
The test on line 16 is named "should handle standard crypto route format" but it doesn't assert much beyond verifying the URL. It doesn't check if the page content actually loads or if there are any errors.
Consider adding assertions to verify the page loaded successfully:
test('should handle standard crypto route format', async ({ page }) => {
await page.goto('/crypto/BTC%2FUSD');
await page.waitForLoadState('networkidle');
// Verify URL is correct
expect(page.url()).toContain('/crypto/BTC%2FUSD');
// Verify page content loads without errors
const heading = page.locator('h1');
await expect(heading).toBeVisible({ timeout: 10000 });
});| useEffect(() => { | ||
| if (symbol && currency) { | ||
| // Validate that symbol and currency are strings and not empty | ||
| if (typeof symbol === 'string' && typeof currency === 'string' && | ||
| symbol.trim().length > 0 && currency.trim().length > 0) { | ||
| // Combine symbol and currency into the format expected by the main crypto page | ||
| // e.g., /crypto/888/USD -> /crypto/888%2FUSD | ||
| const combinedSymbol = `${symbol}/${currency}`; | ||
| const encodedSymbol = encodeURIComponent(combinedSymbol); | ||
| router.replace(`/crypto/${encodedSymbol}`); | ||
| } | ||
| } | ||
| }, [symbol, currency, router]); |
Copilot
AI
Dec 17, 2025
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.
The useEffect runs on every render when symbol, currency, or router change. Since this is a redirect component, the effect should ideally run only once on mount. The router object from Next.js is stable and doesn't change between renders, but including it in dependencies follows React's rules.
However, consider adding an early return to prevent unnecessary processing if a redirect has already been initiated. You could use a ref to track if redirect has been called:
const hasRedirected = useRef(false);
useEffect(() => {
if (hasRedirected.current) return;
if (symbol && currency && symbol.trim().length > 0 && currency.trim().length > 0) {
hasRedirected.current = true;
const combinedSymbol = `${symbol}/${currency}`;
router.replace(`/crypto/${encodeURIComponent(combinedSymbol)}`);
}
}, [symbol, currency, router]);| if (typeof symbol === 'string' && typeof currency === 'string' && | ||
| symbol.trim().length > 0 && currency.trim().length > 0) { |
Copilot
AI
Dec 17, 2025
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.
The validation check on lines 15-16 is redundant. The parameters from useParams() are already strings when they come from Next.js route parameters. The typeof checks for string will always be true since you're already casting them to strings on lines 9-10. Additionally, if the params don't exist, the condition on line 13 will already catch that case.
Consider simplifying to just check for empty strings:
if (symbol && currency && symbol.trim().length > 0 && currency.trim().length > 0) {
The crypto detail page expected URLs like
/crypto/BTC%2FUSD(slash-encoded), but/crypto/888/USD(separate path segments) returned 404.Changes
/app/crypto/[symbol]/[currency]/page.tsx: Client-side redirect component that combines path segments into slash-encoded formatBehavior
/crypto/888/USD→ 404/crypto/888/USD→ redirects →/crypto/888%2FUSD→ 200/crypto/BTC%2FUSD→ 200/crypto/BTC%2FUSD→ 200 (unchanged)Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
fonts.googleapis.com/usr/local/bin/node node /home/REDACTED/work/Ai-Finance/Ai-Finance/node_modules/.bin/next build(dns block)/usr/local/bin/node /usr/local/bin/node /home/REDACTED/work/Ai-Finance/Ai-Finance/node_modules/next/dist/server/lib/start-server.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.