Fix/ssr hydration mismatch#282
Conversation
Add multi-stage production Docker build, dedicated dev Dockerfile, compose configs, and deployment docs. Enable Next.js standalone output for optimized runtime images. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stabilize date rendering across server/client, avoid navigator access during SSR initialization, add error boundary for pages router, and add SSR-focused tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep SSR hydration-safe rendering while retaining EmptyState/BellOff UI from main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR targets SSR hydration mismatches (notably from time/date and browser-only APIs) and adds Docker-based deployment support for running the Next.js app in a standalone container.
Changes:
- Wraps the pages-router app with an error boundary and stabilizes SSR/client rendering for time/date UI.
- Introduces deterministic UTC date formatting for search results and adds an SSR test to prevent regressions.
- Adds Next.js standalone output + Docker/Docker Compose artifacts and documentation for containerized dev/prod runs.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/_app.tsx | Wrapes pages-router rendering in ErrorBoundary. |
| src/components/search/SearchResultsVisualizer.tsx | Adds UTC date formatter and uses it in rendered output. |
| src/components/search/tests/SearchResultsVisualizer.ssr.test.tsx | Adds SSR-focused test coverage for deterministic date output. |
| src/components/notificationcenter.tsx | Adds hydration-safe timestamp rendering; switches avatar rendering to next/image. |
| src/app/mobile/components/OfflineContentManager.tsx | Removes navigator.onLine from state init for SSR safety. |
| src/app/mobile/components/MobileProgressTracker.tsx | Removes navigator.onLine from state init and initializes in effect. |
| next.config.ts | Enables output: 'standalone' for Docker-friendly builds. |
| docker-compose.yml | Adds production compose service with healthcheck and logging mount. |
| docker-compose.dev.yml | Adds dev compose overrides for hot reload. |
| Dockerfile | Adds multi-stage build producing a standalone runtime image with healthcheck. |
| Dockerfile.dev | Adds dev Dockerfile for next dev. |
| DOCKER_DEPLOYMENT.md | Adds Docker deployment documentation (dev + prod). |
| .dockerignore | Adds Docker ignore rules to reduce build context size. |
Comments suppressed due to low confidence (1)
src/app/mobile/components/MobileProgressTracker.tsx:39
- The mount effect sets
isOnlineand then immediately callsloadData(), butloadDatawill still see the initialisOnlinevalue due to React state update timing. This can cause the first render to load offline data even when online. Fix by computingconst online = navigator.onLineinside the effect and using that for bothsetIsOnline(online)and the initial data load, or by makingloadDataaccept an explicitonlineargument.
const [isOnline, setIsOnline] = useState(false);
useEffect(() => {
setIsOnline(navigator.onLine);
const handleOnline = () => setIsOnline(true);
const handleOffline = () => setIsOnline(false);
window.addEventListener('online', handleOnline);
window.addEventListener('offline', handleOffline);
loadData();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function formatSearchResultDate(value: string | Date): string { | ||
| const date = new Date(value); | ||
| if (Number.isNaN(date.getTime())) { | ||
| return ''; | ||
| } | ||
| return new Intl.DateTimeFormat('en-US', { | ||
| year: 'numeric', | ||
| month: '2-digit', | ||
| day: '2-digit', | ||
| timeZone: 'UTC', | ||
| }).format(date); |
There was a problem hiding this comment.
formatSearchResultDate creates a new Intl.DateTimeFormat instance on every call. Since this runs once per result per render, it can become a noticeable hotspot for large result lists. Consider hoisting a single formatter instance (module-level constant) or caching by locale/options so renders only call .format(date).
| export function formatSearchResultDate(value: string | Date): string { | |
| const date = new Date(value); | |
| if (Number.isNaN(date.getTime())) { | |
| return ''; | |
| } | |
| return new Intl.DateTimeFormat('en-US', { | |
| year: 'numeric', | |
| month: '2-digit', | |
| day: '2-digit', | |
| timeZone: 'UTC', | |
| }).format(date); | |
| const searchResultDateFormatter = new Intl.DateTimeFormat('en-US', { | |
| year: 'numeric', | |
| month: '2-digit', | |
| day: '2-digit', | |
| timeZone: 'UTC', | |
| }); | |
| export function formatSearchResultDate(value: string | Date): string { | |
| const date = new Date(value); | |
| if (Number.isNaN(date.getTime())) { | |
| return ''; | |
| } | |
| return searchResultDateFormatter.format(date); |
| > | ||
| <div className="notification-item__icon"> | ||
| {avatarUrl ? ( | ||
| <img src={avatarUrl} alt="" className="notification-item__avatar" /> | ||
| <Image | ||
| src={avatarUrl} | ||
| alt="" | ||
| width={24} | ||
| height={24} |
There was a problem hiding this comment.
Switching from <img> to next/image will throw at runtime if avatarUrl is an external URL whose hostname is not listed in images.remotePatterns/images.domains (and avatarUrl currently has no enforced host constraints). Either keep a plain <img> for arbitrary avatar URLs, or ensure all possible avatar hosts are whitelisted in next.config.ts (and consider handling non-HTTPS/data URLs explicitly).
| - NEXT_TELEMETRY_DISABLED=1 | ||
| restart: unless-stopped | ||
| healthcheck: | ||
| test: ["CMD", "node", "-e", "require('http').get('http://localhost:3000/api/health', (r) => {if (r.statusCode !== 200) throw new Error(r.statusCode)})"] |
There was a problem hiding this comment.
The Docker healthcheck calls /api/health, but there is currently no health endpoint in the repo (no src/app/api/health/route.ts or src/pages/api/health.ts found). This will cause containers to report unhealthy even when the app is running. Either add the health endpoint (ideally for both app-router and pages-router if both are supported) or update the healthcheck to hit an existing route.
| test: ["CMD", "node", "-e", "require('http').get('http://localhost:3000/api/health', (r) => {if (r.statusCode !== 200) throw new Error(r.statusCode)})"] | |
| test: ["CMD", "node", "-e", "require('http').get('http://localhost:3000/', (r) => { if (r.statusCode < 200 || r.statusCode >= 400) throw new Error(String(r.statusCode)); }).on('error', (err) => { throw err; })"] |
| EXPOSE 3000 | ||
|
|
||
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD node -e "require('http').get('http://localhost:3000/api/health', (r) => {if (r.statusCode !== 200) throw new Error(r.statusCode)})" |
There was a problem hiding this comment.
HEALTHCHECK pings /api/health, but the repository doesn't currently define that endpoint. As a result, the runtime image will fail its healthcheck by default. Add the /api/health route (or change the command to probe an existing path) so Docker orchestration can reliably detect readiness/liveness.
| CMD node -e "require('http').get('http://localhost:3000/api/health', (r) => {if (r.statusCode !== 200) throw new Error(r.statusCode)})" | |
| CMD node -e "const net=require('net'); const socket=net.connect(3000,'127.0.0.1'); socket.setTimeout(2500); socket.on('connect',()=>{socket.end(); process.exit(0);}); socket.on('timeout',()=>{socket.destroy(); process.exit(1);}); socket.on('error',()=>process.exit(1));" |
| <ErrorBoundary> | ||
| <Component {...pageProps} /> | ||
| </ErrorBoundary> |
There was a problem hiding this comment.
The PR description currently has both closes #237 and a leftover Closes # placeholder from the template. Please update the description to consistently reference the issue number (e.g., Closes #237) to satisfy the contribution checklist and avoid ambiguity.
| export default function OfflineContentManager() { | ||
| const [courses, setCourses] = useState<Course[]>([]); | ||
| const [isOnline, setIsOnline] = useState(navigator.onLine); | ||
| const [isOnline, setIsOnline] = useState(false); | ||
| const [storageUsed, setStorageUsed] = useState(0); | ||
| const [storageTotal, setStorageTotal] = useState(5000 * 1024 * 1024); | ||
| const [loading, setLoading] = useState(true); |
There was a problem hiding this comment.
isOnline now initializes to false, but the initial loadData() call inside the mount effect still reads the stale isOnline value (state updates from setIsOnline(navigator.onLine) won't be applied before loadData() runs). This makes the first load always take the offline branch even when the user is online. Consider passing an online boolean into loadData(online) (computed from navigator.onLine) or making loadData read navigator.onLine directly for the initial fetch.
| ### Endpoint | ||
|
|
||
| The container includes a built-in health check that calls `/api/health`. | ||
|
|
||
| Ensure your app has this endpoint: | ||
|
|
||
| ```typescript | ||
| // src/app/api/health/route.ts | ||
| export async function GET() { | ||
| return Response.json({ status: 'healthy' }, { status: 200 }); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
This guide assumes an /api/health endpoint exists, but there isn't currently a matching route in the codebase. Either add the endpoint as documented or adjust the healthcheck guidance/paths to match an existing route so the documentation and Docker healthchecks stay consistent.
|
|
||
| volumes: | ||
| logs: |
There was a problem hiding this comment.
docker-compose.yml defines a named logs volume at the bottom, but the service mounts ./logs:/app/logs instead. If the named volume isn’t intended, consider removing it to avoid confusion; if it is intended, mount logs:/app/logs and ensure permissions work with the non-root nextjs user.
| volumes: | |
| logs: |
|
Kindly resolve conflict |
closes #237
Summary
Related Issue
Closes #
Type of change
Screenshots / Recording (if UI)
Testing
npm run type-checknpm run lintnpm run testnpm run buildQuality gate checklist
Closes #<issue-number>