Develop #41
Develop #41
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the application from Vite + React Router to Next.js 16 with the App Router, representing a fundamental architectural shift in build tooling, routing, and rendering patterns.
Key Changes:
- Migration from Vite to Next.js 16 with App Router and Turbopack
- Replacement of React Router with Next.js file-based routing using route groups
- Environment variables migrated from
VITE_*toNEXT_PUBLIC_*prefix - Updated Docker configurations for Next.js standalone builds
- New protected route layout with client-side authentication gating
- Asset handling moved from Vite glob imports to Next.js public folder
- Session/navigation state migrated from React Router location state to sessionStorage
Reviewed changes
Copilot reviewed 93 out of 106 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Removed Vite/React Router deps, added Next.js 16, updated scripts to use next commands |
| tsconfig.json | Consolidated from composite config to single Next.js-compatible config with App Router paths |
| tsconfig.node.json | Removed vite.config.ts from includes as Vite is removed |
| tsconfig.app.json | Removed @pages alias as pages directory no longer exists in App Router |
| vite.config.ts | Deleted - replaced by next.config.mjs |
| next.config.mjs | New Next.js config with standalone output for Docker deployment |
| postcss.config.mjs | Added with @tailwindcss/postcss plugin for Next.js |
| src/main.tsx | Deleted - Next.js uses app/layout.tsx as entry point |
| src/App.tsx | Deleted - routing logic moved to App Router structure |
| app/layout.tsx | New root layout with font loading and Providers wrapper |
| app/(protected)/ | New route group with authentication guard via protected-layout.tsx |
| app/(public)/login/page.tsx | Login page moved to public route group with redirect logic |
| src/lib/config.ts | Environment variables changed from import.meta.env to process.env.NEXT_PUBLIC_* |
| src/lib/api/*.ts | Added SSR-safe localStorage checks (typeof window !== 'undefined') and improved error handling |
| src/lib/assets/serviceLogos.ts | Changed from Vite glob imports to public URL loading with image preloading |
| src/lib/hooks/useRunningJob.ts | New custom hook extracting job fetching logic for reuse across pages |
| src/components//.tsx | Added 'use client' directives, replaced React Router Link/hooks with Next.js equivalents |
| src/shared//.tsx | Updated Link components to Next.js, added rel="noreferrer" to external links |
| src/index.css | Removed @import for fonts (now loaded via next/font), updated CSS variable references |
| public/services/*.svg | Service logos moved from src/assets to public for Next.js static serving |
| Dockerfile_* | Updated to multi-stage builds for Next.js standalone output with Node 20 Alpine |
| README.md | Completely rewritten to document Next.js architecture and setup |
| AGENTS.md | Updated development guidelines for Next.js App Router patterns |
| eslint.config.js | Added .next to ignores, expanded react-refresh allowExportNames for App Router exports |
| scripts/*.ts | Updated asset paths from src/assets to public directory |
Comments suppressed due to low confidence (2)
app/(protected)/deeploys/job/[jobId]/page.tsx:101
- The navigation pattern using state is being replaced with sessionStorage, but the session cleanup logic in the job detail page has been removed. When the alert is dismissed, the data is not properly removed from sessionStorage, which could cause the alert to reappear if the page is refreshed before navigating away.
The onClick handler should call sessionStorage.removeItem for the specific key to ensure the data is cleaned up.
app/(public)/login/page.tsx:23
- Unused variable setEscrowContractAddress.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 107 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
app/(protected)/deeploys/job/[jobId]/edit/page.tsx:213
- The session storage key uses
job.id.toString()butjob.idis abigint. WhiletoString()works on bigint, it's safer and more explicit to useString(job.id)orjob.id.toString()with type checking to avoid potential issues with the bigint serialization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --font-mona: var(--font-mona-sans), 'Mona Sans', sans-serif; | ||
| --font-roboto-mono: var(--font-roboto-mono), 'Roboto Mono', monospace; |
There was a problem hiding this comment.
The font family fallbacks reference CSS variables that may not be defined if Next.js font loading fails or during SSR. The fallback chain should include generic system fonts: font-family: var(--font-mona-sans, var(--font-mona, 'Mona Sans', -apple-system, BlinkMacSystemFont, system-ui, sans-serif)) to ensure text is always readable.
| export async function loadServiceLogo(filename: string): Promise<string | null> { | ||
| const key = `../../assets/services/${filename}`; | ||
| const loader = serviceLogoLoaders[key]; | ||
|
|
||
| if (!loader) { | ||
| console.warn(`[loadServiceLogo] Logo not found for filename: ${filename}`); | ||
| if (!filename || typeof window === 'undefined') { | ||
| return null; | ||
| } | ||
|
|
||
| if (cachedLogos.has(key)) { | ||
| return cachedLogos.get(key) as string; | ||
| const cachedLogo = logoCache.get(filename); | ||
| if (cachedLogo) { | ||
| return cachedLogo; | ||
| } | ||
|
|
||
| const module = (await loader()) as { default: string }; | ||
| const logoUrl = module.default; | ||
| const logoUrl = `/services/${filename}`; | ||
| const logoPromise = preloadLogo(logoUrl).then((resolvedUrl) => { | ||
| if (!resolvedUrl) { | ||
| console.warn(`[loadServiceLogo] Logo not found for filename: ${filename}`); | ||
| } | ||
|
|
||
| cachedLogos.set(key, logoUrl); | ||
| return resolvedUrl; | ||
| }); | ||
|
|
||
| return logoUrl; | ||
| logoCache.set(filename, logoPromise); | ||
| return logoPromise; | ||
| } |
There was a problem hiding this comment.
The service logo loading has been changed from Vite's import.meta.glob to a public URL approach using /services/${filename}. However, there's no validation that these files exist in the public/services/ directory at build time. If a service references a logo that doesn't exist in public/services/, it will silently fail at runtime. Consider adding a build-time validation step or using Next.js static imports for known logos.
| async (error) => { | ||
| const originalRequest = error.config; | ||
| if (error.response.status === 401 && !originalRequest._retry) { | ||
| const status: number | undefined = error?.response?.status; | ||
| const originalRequest = error?.config; | ||
|
|
||
| if (status === 401 && originalRequest && !originalRequest._retry) { | ||
| originalRequest._retry = true; | ||
| const refreshToken = localStorage.getItem('refreshToken'); | ||
| const refreshToken = typeof window !== 'undefined' ? localStorage.getItem('refreshToken') : null; | ||
| if (refreshToken) { | ||
| return axiosDeeploy | ||
| .post('/auth/refresh', { | ||
| refreshToken: refreshToken, | ||
| }) | ||
| .then((res) => { | ||
| if (res.status === 200) { | ||
| localStorage.setItem('accessToken', res.data.accessToken); | ||
| return axiosDeeploy(originalRequest); | ||
| } | ||
| try { | ||
| const refreshClient = axios.create({ baseURL: config.deeployUrl }); | ||
| const res = await refreshClient.post('/auth/refresh', { refreshToken }); | ||
|
|
||
| if (res.status === 200 && typeof window !== 'undefined' && res.data?.accessToken) { | ||
| localStorage.setItem('accessToken', res.data.accessToken); | ||
| return axiosDeeploy(originalRequest); | ||
| }); | ||
| } | ||
| } catch (refreshError) { | ||
| return Promise.reject(toApiError(refreshError, 'Session refresh failed.')); | ||
| } | ||
| } | ||
| } | ||
| return error.response; | ||
|
|
||
| return Promise.reject(toApiError(error, 'Deeploy request failed.')); |
There was a problem hiding this comment.
The new API error handling returns Promise.reject(toApiError(...)) but the old code returned error.response. This is a breaking change that could affect consumers expecting the old response format. Ensure all call sites are updated to handle the new ApiError instances properly, especially in catch blocks that may be checking for response.status or response.data.
No description provided.