-
Notifications
You must be signed in to change notification settings - Fork 0
Migration to next.js #33
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
05e0266 to
7cb249f
Compare
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 PR migrates the application from Vite + React Router to Next.js 16 with the App Router, introducing server-side rendering capabilities and file-based routing.
Key changes:
- Migration from Vite to Next.js build system with updated environment variables (
VITE_*→NEXT_PUBLIC_*) - Replacement of React Router with Next.js App Router navigation (
useNavigate→useRouter,Linkprops updated) - Implementation of route groups for public/protected layouts with authentication gating
Reviewed changes
Copilot reviewed 85 out of 96 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates dependencies: removes Vite, React Router; adds Next.js and Tailwind PostCSS plugin |
| next.config.mjs | Adds Next.js configuration with React strict mode and external packages for SSR compatibility |
| app/layout.tsx | Root layout with Next.js font optimization and metadata configuration |
| app/(protected)/protected-layout.tsx | Authentication wrapper that redirects unauthenticated users to login |
| src/lib/config.ts | Updates environment variable references from import.meta.env.VITE_* to process.env.NEXT_PUBLIC_* |
| src/lib/api/*.ts | Adds SSR-safe localStorage checks and improved error handling with new ApiError class |
| src/components/**/*.tsx | Adds 'use client' directives and migrates navigation hooks/components |
| Dockerfile_* | Updates from nginx-based static serving to Node.js runtime for Next.js server |
| eslint.config.js | Adds .next to ignore list and allows Next.js-specific export names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 87 out of 98 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
app/(public)/login/page.tsx:7
- The
RestrictedAccessimport is present but appears unused in the visible diff. If this component is conditionally rendered based onhasOracles, ensure it's actually used; otherwise, remove the import.
import RestrictedAccess from '@components/auth/RestrictedAccess';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import * as types from '@typedefs/blockchain'; | ||
| import { EthAddress } from '@typedefs/blockchain'; |
Copilot
AI
Dec 15, 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 import on line 2 imports all types as a namespace, while line 3 imports a specific type from the same module. This is redundant since EthAddress could be accessed via types.EthAddress. Consider consolidating to either import all types individually or use the namespace consistently.
|
|
||
| if (!loader) { | ||
| console.warn(`[loadServiceLogo] Logo not found for filename: ${filename}`); | ||
| if (!filename || typeof window === 'undefined') { |
Copilot
AI
Dec 15, 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.
Checking for typeof window === 'undefined' is a valid SSR guard, but since this function is async and returns a Promise, consider documenting that it should only be called from client-side code or returning an error/null more explicitly when called server-side to avoid silent failures.
app/layout.tsx
Outdated
| variable: '--font-roboto-mono', | ||
| }); | ||
|
|
||
| //TODO check if this disable is correct |
Copilot
AI
Dec 15, 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 TODO comment indicates uncertainty about the eslint-disable. The disable is actually correct here—metadata exports are Next.js-specific exports that don't violate the spirit of the rule. Consider removing the TODO and adding a comment explaining why this is an expected exception.
| //TODO check if this disable is correct | |
| // The following eslint-disable is intentional: Next.js metadata exports are special-case and do not violate the spirit of the rule. |
| `/burn-report/download-burn-report-json?startTime=${start}&endTime=${end}`, | ||
| 'burn_report.json', | ||
| ); | ||
| downloadJsonFile(`/burn-report/download-burn-report-json?startTime=${start}&endTime=${end}`, 'burn_report.json'); |
Copilot
AI
Dec 15, 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.
This line was reformatted from a multi-line call to a single line. While the formatting itself is valid, lines over ~120 characters can reduce readability. Consider keeping the original multi-line format for better readability.
| downloadJsonFile(`/burn-report/download-burn-report-json?startTime=${start}&endTime=${end}`, 'burn_report.json'); | |
| downloadJsonFile( | |
| `/burn-report/download-burn-report-json?startTime=${start}&endTime=${end}`, | |
| 'burn_report.json' | |
| ); |
| inputs, | ||
| logo: answers.logo, | ||
| color: answers.color, | ||
| color: answers.color as Service['color'], |
Copilot
AI
Dec 15, 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 type assertion as Service['color'] suggests that answers.color might not be strictly typed. Consider adding type guards or validation to ensure answers.color is actually a valid Service['color'] value before the assertion to prevent runtime errors.
371a33d to
30932b9
Compare
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
Copilot reviewed 87 out of 98 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
app/(protected)/tunnels/[id]/page.tsx:360
- Missing rel="noreferrer" attribute on external link. For security and privacy, external links opened with target="_blank" should include rel="noreferrer" to prevent the new page from accessing window.opener and to avoid sending referrer information.
app/(public)/login/page.tsx:23 - Unused variable setEscrowContractAddress.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Usage Notes | ||
|
|
||
| - Authentication uses SIWE; tokens (`accessToken`, `refreshToken`) are kept in `localStorage` and reused by the Axios interceptors in `src/lib/api`. |
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 documentation explicitly states that authentication tokens (accessToken, refreshToken) are stored in localStorage and reused by Axios interceptors. Storing long-lived auth tokens in localStorage makes them directly accessible to any XSS payload, allowing an attacker who finds a scripting bug anywhere in the app to exfiltrate tokens and impersonate users. Prefer short-lived, HTTP-only, SameSite-protected cookies for session tokens, or another storage mechanism that is not readable from JavaScript, and update the auth flow and interceptors accordingly.
| - Authentication uses SIWE; tokens (`accessToken`, `refreshToken`) are kept in `localStorage` and reused by the Axios interceptors in `src/lib/api`. | |
| - Authentication uses SIWE; session tokens (`accessToken`, `refreshToken`) are managed via short-lived, HTTP-only, `SameSite`-protected cookies by the backend and are automatically sent with requests that Axios interceptors handle in `src/lib/api` (they are **not** stored in `localStorage`). |
| Deeploy workflows talk to the [edge_node](https://github.com/Ratio1/edge_node) API through wrappers in `src/lib/api/deeploy.ts`. Configure the base URL by setting `NEXT_PUBLIC_API_URL` and `NEXT_PUBLIC_ENVIRONMENT` (and optionally `NEXT_PUBLIC_DEV_ADDRESS`) in your env file (prefer `.env.local`); `src/lib/config.ts` routes requests across devnet/testnet/mainnet. Local storage must expose `accessToken`/`refreshToken` to satisfy the Axios interceptors. When working against a local edge node, run its server first, then point `NEXT_PUBLIC_API_URL` to the exposed port (e.g., `http://localhost:5000`). | ||
|
|
||
| Because this is Next.js, be mindful of client/server boundaries: modules that access `localStorage` (like `src/lib/api/deeploy.ts`) must only run in client components/hooks (files with `'use client'`) and should not be imported/executed from server components. |
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.
This section notes that accessToken/refreshToken must be present in localStorage to satisfy Axios interceptors. Persisting authentication tokens in localStorage significantly increases the blast radius of any XSS vulnerability, since injected scripts can read and exfiltrate these values to fully hijack user sessions. Refactor the authentication system to keep tokens in HTTP-only, SameSite cookies (or another non-JS-readable store) and adjust the API client so it no longer depends on localStorage.
| Deeploy workflows talk to the [edge_node](https://github.com/Ratio1/edge_node) API through wrappers in `src/lib/api/deeploy.ts`. Configure the base URL by setting `NEXT_PUBLIC_API_URL` and `NEXT_PUBLIC_ENVIRONMENT` (and optionally `NEXT_PUBLIC_DEV_ADDRESS`) in your env file (prefer `.env.local`); `src/lib/config.ts` routes requests across devnet/testnet/mainnet. Local storage must expose `accessToken`/`refreshToken` to satisfy the Axios interceptors. When working against a local edge node, run its server first, then point `NEXT_PUBLIC_API_URL` to the exposed port (e.g., `http://localhost:5000`). | |
| Because this is Next.js, be mindful of client/server boundaries: modules that access `localStorage` (like `src/lib/api/deeploy.ts`) must only run in client components/hooks (files with `'use client'`) and should not be imported/executed from server components. | |
| Deeploy workflows talk to the [edge_node](https://github.com/Ratio1/edge_node) API through wrappers in `src/lib/api/deeploy.ts`. Configure the base URL by setting `NEXT_PUBLIC_API_URL` and `NEXT_PUBLIC_ENVIRONMENT` (and optionally `NEXT_PUBLIC_DEV_ADDRESS`) in your env file (prefer `.env.local`); `src/lib/config.ts` routes requests across devnet/testnet/mainnet. Authentication is handled via secure, HTTP-only, `SameSite` cookies set by the backend; Axios is configured to rely on these cookies being sent automatically by the browser rather than on tokens stored in `localStorage`. When working against a local edge node, run its server first, then point `NEXT_PUBLIC_API_URL` to the exposed port (e.g., `http://localhost:5000`). | |
| Because this is Next.js, be mindful of client/server boundaries: any code that touches browser-only APIs (such as `window`, `document`, or `localStorage`) must only run in client components/hooks (files with `'use client'`) and should not be imported/executed from server components. Do not persist authentication tokens in `localStorage` or `sessionStorage`; rely on HTTP-only cookies or other non–JavaScript-readable mechanisms instead. |
30932b9 to
ac1748c
Compare
ac1748c to
ff09a92
Compare
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
Copilot reviewed 87 out of 100 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
app/(protected)/deeploys/project-draft/[projectHash]/page.tsx:64
- Using sessionStorage to pass state between pages is fragile and can lead to stale data. Consider using URL search params or Next.js-specific state management solutions like storing in route context or using a global state manager for this cross-page communication pattern.
app/(protected)/deeploys/dashboard/page.tsx:38 - Missing error handling for sessionStorage access. In browsers with strict privacy settings or in incognito mode, sessionStorage operations can throw exceptions. Wrap the sessionStorage calls in a try-catch block to prevent the component from crashing.
app/(public)/login/page.tsx:23 - Unused variable setEscrowContractAddress.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const ScrollToTop = () => { | ||
| const { pathname } = useLocation(); | ||
| const pathname = usePathname(); |
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.
Missing null check for pathname. The usePathname hook can return null during server-side rendering or when called outside a navigation context. While the nullish coalescing operator provides a fallback, the component should handle the null case more explicitly to prevent potential routing issues.
| export default function ErrorPage({ error, reset }: { error: Error & { digest?: string }; reset: () => void }) { | ||
| useEffect(() => { | ||
| console.error('[app/error.tsx]', error); | ||
| }, [error]); | ||
|
|
||
| return ( | ||
| <div className="flex w-full flex-1 justify-center pt-24"> | ||
| <DetailedAlert | ||
| variant="red" | ||
| icon={<RiErrorWarningLine />} | ||
| title="Something went wrong" | ||
| description={ | ||
| <div> | ||
| An unexpected error occurred. Please try again, or return to the home page. | ||
| {process.env.NODE_ENV === 'development' && ( | ||
| <div className="col gap-1 px-3 py-2 pt-2 font-mono text-sm"> | ||
| <div>{error.message}</div> | ||
| {!!error.digest && <div className="text-slate-500">{error.digest}</div>} | ||
| </div> | ||
| )} | ||
| </div> | ||
| } | ||
| fullWidth | ||
| /> | ||
| </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 error boundary logs the error but doesn't call the reset function. The reset parameter passed to this component should be exposed to users (e.g., via a "Try Again" button) to allow them to recover from the error state without requiring a full page refresh.
No description provided.