Added xterm support for scrolling follow-along, fixed PVC leftovers after deleting STS. - #80
Conversation
HUAHUAI23
commented
Nov 12, 2025
HUAHUAI23
commented
- Added xterm support for scrolling follow-along,
- fixed PVC leftovers after deleting STS.
There was a problem hiding this comment.
Pull Request Overview
This pull request adds xterm.js-based terminal support with scrolling follow-along functionality and fixes PVC cleanup issues in StatefulSet deletion.
Key changes:
- Replaces iframe-based terminal with native xterm.js implementation featuring auto-scroll indicators
- Implements automatic PVC deletion for StatefulSets with retention policy configuration
- Enhances project filtering API with keyword search and date range capabilities
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json, pnpm-lock.yaml | Adds xterm.js and related addon dependencies |
| lib/k8s/sandbox-manager.ts | Adds PVC cleanup logic and retention policy configuration for StatefulSets |
| hooks/use-terminal.ts | New hook for terminal WebSocket connection management (appears unused) |
| hooks/use-projects.ts | Updates project filtering with keyword/date filters and changes default refetch behavior |
| components/terminal/xterm-terminal.tsx | New xterm.js-based terminal component with scroll indicators |
| components/terminal/terminal-display.tsx | Replaces iframe with XtermTerminal component |
| components/terminal/terminal-container.tsx | Enhanced comments for tab management |
| app/projects/page.tsx | Removes explicit namespace parameter from useProjects |
| app/api/projects/route.ts | Implements keyword/date filtering and automatic namespace detection |
| app/globals.css | Adds fade-in animation for scroll indicator |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { refetchInterval = 3000, refetchOnWindowFocus = false, namespace } = options; | ||
| const { | ||
| refetchInterval = 3000, | ||
| refetchOnWindowFocus = true, |
There was a problem hiding this comment.
The default value for refetchOnWindowFocus has changed from false to true. This behavioral change means projects will now automatically refetch when the window regains focus, which could cause unexpected API calls and affect user experience. Consider documenting this change in the PR description or reverting to the original default if this wasn't intentional.
| refetchOnWindowFocus = true, | |
| refetchOnWindowFocus = false, |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [wsUrl]); |
There was a problem hiding this comment.
The useEffect dependency array only includes wsUrl, but the effect uses several callback props (stableOnReady, stableOnConnected, stableOnDisconnected) and rendererType. While these are wrapped with useCallback, they should still be included in the dependency array or the eslint rule should not be disabled. The current implementation may cause stale closures if these callbacks change.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [wsUrl]); | |
| }, [wsUrl, stableOnReady, stableOnConnected, stableOnDisconnected, rendererType]); |
| persistentVolumeClaimRetentionPolicy: { | ||
| whenDeleted: 'Delete', // Delete PVCs when StatefulSet is deleted | ||
| whenScaled: 'Retain', // Keep PVCs when scaling down (for potential scale-up) | ||
| }, |
There was a problem hiding this comment.
The persistentVolumeClaimRetentionPolicy requires Kubernetes 1.23+ as mentioned in the deletePVCs comment. This should be documented here as well to warn about potential compatibility issues with older clusters that may not support this field.
| // If user doesn't have kubeconfig configured, log warning but don't fail | ||
| // Return empty array instead of filtering by namespace | ||
| logger.warn( | ||
| `User ${session.user.id} does not have KUBECONFIG configured, returning all projects` | ||
| ) |
There was a problem hiding this comment.
The comment says 'Return empty array' but the code actually returns all projects without namespace filtering. This is misleading and potentially a security issue - users without kubeconfig will see projects from all namespaces instead of no projects. The behavior should either match the comment or the comment should be corrected to reflect the actual behavior.
| // If user doesn't have kubeconfig configured, log warning but don't fail | |
| // Return empty array instead of filtering by namespace | |
| logger.warn( | |
| `User ${session.user.id} does not have KUBECONFIG configured, returning all projects` | |
| ) | |
| // If user doesn't have kubeconfig configured, log warning and return empty array | |
| logger.warn( | |
| `User ${session.user.id} does not have KUBECONFIG configured, returning empty array` | |
| ) | |
| return NextResponse.json([]) |
| let lineFeedTimeout: NodeJS.Timeout | null = null; | ||
| terminal.onLineFeed(() => { | ||
| if (lineFeedTimeout) clearTimeout(lineFeedTimeout); | ||
| lineFeedTimeout = setTimeout(() => { |
There was a problem hiding this comment.
The lineFeedTimeout variable is declared inside the init function but never cleaned up in the cleanup function. This could cause the timeout to fire after component unmount. Add if (lineFeedTimeout) clearTimeout(lineFeedTimeout) to the cleanup function at line 497.
| const handleConnected = useCallback(() => { | ||
| setStatus('connected') | ||
| shouldReconnectRef.current = autoReconnect | ||
| onConnected?.() | ||
| }, [autoReconnect, onConnected]) |
There was a problem hiding this comment.
Unused variable handleConnected.
| const handleConnected = useCallback(() => { | |
| setStatus('connected') | |
| shouldReconnectRef.current = autoReconnect | |
| onConnected?.() | |
| }, [autoReconnect, onConnected]) |
| const handleDisconnected = useCallback(() => { | ||
| setStatus('disconnected') | ||
| onDisconnected?.() | ||
|
|
||
| // Schedule reconnection if enabled | ||
| if (shouldReconnectRef.current && wsUrl) { | ||
| console.log(`[useTerminal] Reconnecting in ${reconnectDelay}ms...`) | ||
| reconnectTimeoutRef.current = setTimeout(() => { | ||
| setStatus('connecting') | ||
| // Trigger re-render to reconnect | ||
| setWsUrl((prev) => (prev ? `${prev}` : null)) | ||
| }, reconnectDelay) | ||
| } | ||
| }, [wsUrl, reconnectDelay, onDisconnected]) |
There was a problem hiding this comment.
Unused variable handleDisconnected.
| const handleDisconnected = useCallback(() => { | |
| setStatus('disconnected') | |
| onDisconnected?.() | |
| // Schedule reconnection if enabled | |
| if (shouldReconnectRef.current && wsUrl) { | |
| console.log(`[useTerminal] Reconnecting in ${reconnectDelay}ms...`) | |
| reconnectTimeoutRef.current = setTimeout(() => { | |
| setStatus('connecting') | |
| // Trigger re-render to reconnect | |
| setWsUrl((prev) => (prev ? `${prev}` : null)) | |
| }, reconnectDelay) | |
| } | |
| }, [wsUrl, reconnectDelay, onDisconnected]) |
✅ PR Check Results: PassedBuild Checks
✨ Great work!All checks passed successfully. Your PR is ready for review. Details:
🔗 View Details: |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
