init remote sessions #212
init remote sessions #212
Conversation
WalkthroughThis pull request includes the removal of several files related to session management and WebSocket functionality in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebSocketServer
participant AgentSocket
participant UserSocket
User->>WebSocketServer: Connect
WebSocketServer->>AgentSocket: Create AgentSocket
AgentSocket-->>WebSocketServer: Success
WebSocketServer->>UserSocket: Create UserSocket
UserSocket-->>WebSocketServer: Success
User->>UserSocket: Send Message
UserSocket->>AgentSocket: Forward Message
AgentSocket->>UserSocket: Send Response
UserSocket-->>User: Response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
🧹 Outside diff range and nitpick comments (46)
packages/validators/src/session/session-delete.ts (1)
8-8: Consider enhancing the schema validationTwo suggestions for improvement:
- The description could be more specific about what a "Target ID" represents and when it's expected to be present
- Consider adding format validation for the ID fields if they follow a specific pattern
- targetId: z.string().describe("Target ID for the session").optional(), + targetId: z + .string() + .describe("Identifier of the target instance where the session is being deleted (required for remote sessions)") + .regex(/^[a-zA-Z0-9-_]+$/, "Target ID must contain only alphanumeric characters, hyphens, and underscores") + .optional(),packages/validators/src/session/session-create.ts (1)
13-13: Add format validation for targetIdConsider adding a specific format validation for targetId to ensure it matches your expected format (e.g., UUID, specific pattern).
Example if using UUIDs:
- targetId: z.string().describe("Target ID for the session"), + targetId: z + .string() + .uuid() + .describe("Target ID for the session (must be a valid UUID)"),apps/target-proxy/src/server.ts (2)
30-33: Health check endpoint looks good, consider adding more detailed status information.The implementation is clean and follows REST conventions. However, for better monitoring capabilities, consider enriching the health check response with additional metrics like uptime, version, or connection status to dependent services.
app.get("/api/proxy/health", (_req, res) => { - res.status(200).json({ status: "ok" }); + res.status(200).json({ + status: "ok", + version: process.env.npm_package_version, + uptime: process.uptime(), + timestamp: new Date().toISOString() + }); });
Line range hint
14-14: Security: Restrict CORS origins when credentials are allowed.The current CORS configuration allows credentials with any origin, which could expose the application to security risks. Consider explicitly listing allowed origins.
-app.use(cors({ credentials: true })); +app.use(cors({ + credentials: true, + origin: process.env.ALLOWED_ORIGINS?.split(',') || 'http://localhost:3000' +}));apps/target-proxy/Dockerfile (2)
Line range hint
1-39: Critical: Fix incompatible package manager commands and build filterThere are several issues in the Dockerfile that need attention:
- The installer stage uses
apkcommands but the base imagenode:20-slimis Debian-based. This will fail asapkis Alpine Linux's package manager.- The turbo build command is still filtering for "webshell" despite this being the target-proxy application.
Apply these fixes:
FROM base AS installer - RUN apk add --no-cache libc6-compat - RUN apk update + RUN apt-get update && apt-get install -y libc6 \ + && rm -rf /var/lib/apt/lists/* WORKDIR /app # ... (other commands remain the same) - RUN pnpm turbo build --filter=webshell... + RUN pnpm turbo build --filter=target-proxy...Also, consider adding explicit COPY instructions for only the necessary files instead of copying everything with
., to reduce the image size and potential security risks.Consider adding a .dockerignore file to exclude unnecessary files and directories:
+ node_modules + npm-debug.log + .git + .env + *.md + .gitignore + .dockerignore
Security: File permissions need to be set before switching to non-root user
The Dockerfile has a security issue in the
runnerstage where files are copied after switching to the non-root userexpressjs. This means the copied files will be owned by root, potentially causing permission issues when the application tries to access them.To fix this:
- Move the
COPY --from=installer /app .instruction before theUSER expressjsline- Add
chowncommand to set proper ownership of the application filesExample fix:
FROM base AS runner WORKDIR /app # Don't run production as root RUN addgroup --system --gid 1001 expressjs RUN adduser --system --uid 1001 expressjs COPY --from=installer /app . RUN chown -R expressjs:expressjs . USER expressjs CMD ["node", "apps/target-proxy/dist/index.js"]🔗 Analysis chain
Line range hint
31-35: Security: Verify file permissions for non-root userWhile the Dockerfile correctly sets up a non-root user, we should ensure proper file permissions for the copied files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any file permission related configurations # Expected: Should find proper permission settings for the application files # Look for any permission related configurations rg -i "chmod|chown|permission" apps/target-proxy # Check package.json scripts for any permission related commands fd -t f "package.json" "apps/target-proxy" -x jq -r '.scripts'Length of output: 446
Script:
#!/bin/bash # Let's check the Dockerfile for COPY instructions and their order relative to user setup rg -A 2 "^COPY" apps/target-proxy/Dockerfile # Check if there are any other file operations in Dockerfile cat apps/target-proxy/DockerfileLength of output: 1270
apps/target-proxy/src/routing.ts (2)
16-24: Improve route handling security and maintainabilityThe current route handling implementation has several areas for improvement:
- Path matching is too permissive
- API versioning is hardcoded
- Missing rate limiting and authentication checks
Consider:
- Using exact path matching or regex patterns
- Extracting API version and paths to constants
- Adding middleware for rate limiting and authentication
+const API_VERSION = 'v1'; +const ROUTES = { + SESSION: `/api/${API_VERSION}/target/proxy/session`, + CONTROLLER: `/api/${API_VERSION}/target/proxy/controller` +} as const; + export const addSocket = (expressApp: Express) => { const server = createServer(expressApp); + + // Rate limiting map + const connectionAttempts = new Map<string, number>(); + server.on("upgrade", (request, socket, head) => { + // Rate limiting + const clientIp = request.socket.remoteAddress || ''; + const attempts = connectionAttempts.get(clientIp) || 0; + if (attempts > 100) { // 100 attempts per IP + socket.destroy(); + return; + } + connectionAttempts.set(clientIp, attempts + 1); + // ... URL validation ... - if (pathname.startsWith("/api/v1/target/proxy/session")) { + if (pathname === ROUTES.SESSION) { sessionOnUpgrade(request, socket, head); return; } - if (pathname.startsWith("/api/v1/target/proxy/controller")) { + if (pathname === ROUTES.CONTROLLER) { controllerOnUpgrade(request, socket, head); return; }
25-28: Add logging for rejected connectionsThe fallback handling should include logging for security monitoring and debugging purposes.
+ console.warn(`Rejected WebSocket connection attempt to invalid path: ${pathname}`); socket.destroy(); }); + + // Periodic cleanup of rate limiting map + setInterval(() => { + connectionAttempts.clear(); + }, 60 * 60 * 1000); // Clear every hour + return server;packages/validators/src/session/index.ts (1)
Line range hint
19-27: Please provide context about the session resize functionalitySince this PR is initializing remote sessions, it would be helpful to understand:
- The purpose and use cases for session resizing
- How it integrates with the overall session management architecture
- Any specific constraints or limitations we should be aware of
apps/webservice/src/app/[workspaceSlug]/term/Terminal.tsx (3)
9-9: Remove debug console.log statementRemove the console.log statement before merging to production.
- console.log(sessionId);
19-24: Add cleanup and optimize terminal fittingThe effect should include cleanup and the fitting operation should be debounced for better performance.
+ const debouncedFit = useCallback( + debounce(() => { + if (fitAddon) fitAddon.fit(); + }, 100), + [fitAddon] + ); + useEffect(() => { if (readyState !== ReadyState.OPEN) return; if (terminalRef.current == null) return; terminalRef.current.focus(); - fitAddon.fit(); + debouncedFit(); + + // Add window resize listener + window.addEventListener('resize', debouncedFit); + + return () => { + window.removeEventListener('resize', debouncedFit); + debouncedFit.cancel(); + }; }, [getWebSocket, terminalRef, fitAddon, readyState]);
26-32: Enhance accessibility and semantic structureAdd proper ARIA attributes and semantic structure to improve accessibility.
return ( - <div> + <div + role="region" + aria-label="Terminal Session" + className="terminal-container" + > <div className="h-full w-full"> - <div ref={divRef} className="h-full w-full" /> + <div + ref={divRef} + className="h-full w-full" + role="application" + aria-label="Terminal Window" + tabIndex={0} + /> </div> </div> );apps/target-proxy/src/controller/utils.ts (1)
4-6: Consider adding a cleanup mechanism for message handlers.The closure maintains an array of check functions that grows with each
iscall. Consider adding aclear()method to prevent potential memory leaks, especially if handlers are added dynamically during the application lifecycle.return { + clear() { + checks.length = 0; + return this; + }, is<T>(schema: z.ZodSchema<T>, callback: (data: T) => Promise<void> | void) {apps/webservice/src/app/[workspaceSlug]/term/page.tsx (1)
11-14: Consider adding error handling for dynamic import.While the dynamic import is correctly implemented, consider wrapping the Terminal component in an error boundary to gracefully handle loading failures.
+import { ErrorBoundary } from '@ctrlplane/ui/error-boundary'; const Terminal = dynamic( () => import("./Terminal").then((mod) => mod.SessionTerminal), { ssr: false }, ); + +const TerminalWithErrorBoundary = ({ sessionId }: { sessionId: string }) => ( + <ErrorBoundary fallback={<div>Failed to load terminal. Please refresh.</div>}> + <Terminal sessionId={sessionId} /> + </ErrorBoundary> +);apps/webservice/src/app/[workspaceSlug]/_components/terminal/useResizableHeight.tsx (4)
1-10: Add TypeScript interface and JSDoc documentation.Consider adding type definitions and documentation to improve maintainability and developer experience.
+interface UseResizableHeightReturn { + height: number; + handleMouseDown: React.MouseEventHandler<HTMLDivElement>; +} + +/** + * Custom hook for managing resizable height with mouse drag interactions + * @param initialHeight - The starting height in pixels + * @param minHeight - The minimum allowed height in pixels + * @returns Object containing current height and mouse down handler + */ export const useResizableHeight = ( initialHeight: number, minHeight: number, -) => { +): UseResizableHeightReturn => {
12-19: Prevent text selection during resize operation.Add prevention of text selection during resize to improve user experience.
const handleMouseDown = useCallback<React.MouseEventHandler<HTMLDivElement>>( (e) => { + e.preventDefault(); // Prevent text selection during resize setIsDragging(true); dragStartY.current = e.clientY; dragStartHeight.current = height; }, [height], );
21-30: Consider performance optimization and maximum height constraint.The mouse move handler could benefit from throttling for better performance, and you might want to add a maximum height constraint to prevent excessive resizing.
+import { throttle } from 'lodash'; + const handleMouseMove = useCallback( (e: MouseEvent) => { if (!isDragging) return; const deltaY = dragStartY.current - e.clientY; - const newHeight = Math.max(minHeight, dragStartHeight.current + deltaY); + const maxHeight = window.innerHeight * 0.8; // 80% of viewport height + const newHeight = Math.min( + maxHeight, + Math.max(minHeight, dragStartHeight.current + deltaY) + ); setHeight(newHeight); }, [isDragging, minHeight], ); + +const throttledHandleMouseMove = useCallback( + throttle(handleMouseMove, 16), // ~60fps + [handleMouseMove] +);
36-45: Optimize event listeners for better performance.Consider using passive event listeners and the throttled mouse move handler.
useEffect(() => { if (isDragging) { - window.addEventListener("mousemove", handleMouseMove); + window.addEventListener("mousemove", throttledHandleMouseMove, { passive: true }); window.addEventListener("mouseup", handleMouseUp); } return () => { - window.removeEventListener("mousemove", handleMouseMove); + window.removeEventListener("mousemove", throttledHandleMouseMove); window.removeEventListener("mouseup", handleMouseUp); }; -}, [isDragging, handleMouseMove, handleMouseUp]); +}, [isDragging, throttledHandleMouseMove, handleMouseUp]);apps/webservice/src/app/[workspaceSlug]/layout.tsx (1)
16-22: Consider aligning component and file names for better maintainability.The file name
TerminalSessionsDrawerdoesn't match the exported component nameTerminalDrawer. Consider renaming either the file or the export to maintain consistency.packages/validators/package.json (1)
43-46: Consider documenting the session module's purposeSince this is a new module for session management, it would be helpful to add JSDoc comments or a README in the session directory explaining its purpose and usage.
Would you like me to help create documentation for this new module?
apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx (4)
11-21: Consider adding validation constraints to the type definitionThe
SessionContextTypeinterface could benefit from additional type constraints for the terminal dimensions inresizeSession.type SessionContextType = { sessionIds: { sessionId: string; targetId: string }[]; createSession: (targetId: string) => void; removeSession: (id: string) => void; resizeSession: ( sessionId: string, targetId: string, - cols: number, - rows: number, + cols: number & { __type: 'PositiveInteger' }, + rows: number & { __type: 'PositiveInteger' }, ) => void; };
25-31: Improve error message specificityThe error message should reference the correct component name for better debugging.
- throw new Error("useSession must be used within a SessionProvider"); + throw new Error("useTerminalSessions must be used within a TerminalSessionsProvider");
58-59: Remove console.log from production codeDebug statements should not be present in production code.
- console.log(resizePayload); sendJsonMessage(resizePayload);
1-87: Consider implementing session persistence and recoveryAs this component manages terminal sessions, consider these architectural improvements:
- Implement session persistence to handle page refreshes
- Add session recovery mechanism for dropped WebSocket connections
- Consider implementing a session heartbeat to detect stale sessions
Would you like me to provide detailed implementation suggestions for any of these improvements?
apps/webservice/src/app/[workspaceSlug]/_components/terminal/CreateDialogSession.tsx (3)
34-35: Fix typo in state setter name.The setter name has a typo:
setModelOpenshould besetModalOpenfor consistency.-const [isModalOpen, setModelOpen] = useState(false); +const [isModalOpen, setModalOpen] = useState(false);
42-53: Consider improving the target fetching implementation.Several improvements could make the target fetching more robust:
- The hardcoded filter values should be constants or enums
- The limit of 500 might not be sufficient - consider pagination
- Loading and error states should be handled
Example implementation:
const FILTER_TYPE = { KIND: 'kind' } as const; const TARGET_TYPE = { SESSION: 'TargetSession' } as const; // In component: const { data: targets, isLoading, error } = api.target.byWorkspaceId.list.useQuery( { workspaceId: workspace.data?.id ?? "", limit: 500, filter: { type: FILTER_TYPE.KIND, operator: "equals", value: TARGET_TYPE.SESSION, }, }, { enabled: workspace.data != null }, ); if (isLoading) return <LoadingSpinner />; if (error) return <ErrorMessage error={error} />;
93-128: Consider performance and accessibility improvements for the target selector.The command list implementation could benefit from:
- Virtualization for large datasets
- ARIA labels for better screen reader support
- Keyboard navigation improvements
Consider using a virtualized list component like
react-windoworreact-virtualizedfor the command list to handle large datasets efficiently.apps/target-proxy/src/controller/index.ts (3)
39-42: Handle unexpected errors during connection handlingWhile errors from
onConnectare caught, it might be beneficial to provide more specific error handling or retry logic depending on the type of error encountered. This ensures that transient issues do not adversely affect users or agents trying to connect.
50-52: Include client IP address in upgrade request logsIncluding the client's IP address in the logs can be helpful for monitoring and debugging purposes.
Apply this diff to include the IP address:
logger.debug("WebSocket upgrade request received", { url: request.url, + ip: request.socket.remoteAddress, });
36-43: Set maximum number of listeners for 'connection' eventTo avoid potential memory leaks due to a large number of connections, it's good practice to set a maximum number of listeners or handle situations where the server might be overwhelmed by too many connections.
apps/target-proxy/src/sessions/index.ts (1)
9-9: Consider making 'MAX_HISTORY_BYTES' configurableThe
MAX_HISTORY_BYTESconstant is currently set to1024bytes. Depending on the application's requirements, it might be beneficial to make this value configurable to accommodate different use cases.apps/target-proxy/src/controller/user-socket.ts (1)
41-41: Remove unused constructor parameter_The constructor parameter
_: IncomingMessageis unused. Consider removing it to simplify the code.Apply this diff:
private constructor( private readonly socket: WebSocket, - private readonly _: IncomingMessage, public readonly user: User, ) {Also, update the constructor call at line 36 to remove the
requestparameter:return new UserSocket(socket, { id: user.id });apps/webservice/src/components/xterm/XTermAddon.tsx (2)
72-86: Simplify the_sendBinarymethod to avoid unnecessary conversionsThe current implementation converts the
datastring to aUint8Arrayand then back to a string, which is inefficient.Consider simplifying the method:
private _sendBinary(data: string): void { if (!this._checkOpenSocket()) return; - const buffer = new Uint8Array(data.length); - for (let i = 0; i < data.length; ++i) buffer[i] = data.charCodeAt(i) & 255; - const input: SessionInput = { type: "session.input", targetId: this.options.targetId, sessionId: this.options.sessionId, - data: String.fromCharCode.apply(null, Array.from(buffer)), + data, }; this._socket.send(JSON.stringify(input)); }Since
datais already a string, you can use it directly without extra processing, improving performance.
114-117: Remove unnecessary condition in thedisposefunctionThe check
if (!handler)is unnecessary becausehandleris a constant parameter and cannot be reassigned tonullorundefined.Apply this diff to remove the redundant condition:
dispose: () => { - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (!handler) { - // Already disposed - return; - } socket.removeEventListener(type, handler); },Alternatively, if you want to prevent multiple disposals, track the disposal state:
+ let isDisposed = false; dispose: () => { + if (isDisposed) { + // Already disposed + return; + } socket.removeEventListener(type, handler); + isDisposed = true; },This ensures that the event listener is removed only once.
apps/webservice/src/components/xterm/SessionTerminal.tsx (5)
27-29: IncludedivRefin the dependency array ofreloadTerminal.The
divRefis used withinreloadTerminalbut not included in its dependency array. While refs are generally stable, it's good practice to include all external variables used inside a hook to avoid potential issues.
54-60: Review dependencies in theuseEffecthook.In the
useEffectstarting at line 54,divRefis referenced but not included in the dependency array. Consider addingdivRef.currentto the dependencies or ensure that its exclusion is intentional and won't lead to unexpected behavior.
74-74: Remove unusedsessionIdprop fromSocketTerminalcomponent.The
sessionIdprop is declared but not utilized within the component. Removing it can clean up the code and prevent confusion about its purpose.Apply this diff to remove the unused prop:
- sessionId: string;
81-93: Ensure all dependencies are included in theuseEffectdependency array.In the
useEffectstarting at line 81, variables such aswidth,height, andonResizeare used but not included in the dependency array. This could lead to unexpected behavior if those variables change.
104-116: Add missing dependencies to theuseDebouncehook.Within the
useDebounce, you are usingterminalRef,fitAddon, andonResize, which are not included in the dependency array. Including all dependencies ensures that the debounced effect responds correctly to changes.apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsDrawer.tsx (5)
115-121: Unused state variable 'isOpen'The
isOpenstate is initialized but never updated, which means the drawer will always be visible. If the drawer's visibility is not meant to be toggled, consider removing theisOpenstate and the conditional logic associated with it. Otherwise, implement the necessary logic to control the drawer's visibility.Apply this diff to remove the unnecessary state variable and simplify the class name:
- const [isOpen] = React.useState(true); ... - className={`fixed bottom-0 left-0 right-0 z-30 bg-black drop-shadow-2xl ${isOpen ? "" : "hidden"}`} + className="fixed bottom-0 left-0 right-0 z-30 bg-black drop-shadow-2xl"
84-85: RenamesessionIdstosessionsfor clarityThe variable
sessionIdsappears to be an array of session objects containingsessionIdandtargetId, not just IDs. For better clarity, consider renamingsessionIdstosessions.Apply this diff to rename the variable:
- const { sessionIds } = useTerminalSessions(); + const { sessions } = useTerminalSessions(); ... - {sessionIds.map((s, idx) => ( + {sessions.map((s, idx) => (Also applies to: 96-103
96-99: Remove redundantkeyprop from nested componentYou are assigning the
keyprop to both the<Fragment>and the<ResizablePanel>using the sames.sessionId. Since thekeyis only required on the outermost element in a list, you can remove the redundantkeyfrom the<ResizablePanel>to prevent unnecessary duplication.Apply this diff to remove the redundant key:
<Fragment key={s.sessionId}> {idx !== 0 && <ResizableHandle className="bg-neutral-700" />} - <ResizablePanel key={s.sessionId} className="px-4"> + <ResizablePanel className="px-4"> <SessionTerminal {...s} /> </ResizablePanel> </Fragment>
68-68: Avoid hard-coded height valuesUsing hard-coded values like
30pxinh-[calc(100%-30px)]can lead to layout issues if the component's surrounding elements change size. Consider using relative units, CSS variables, or calculating the height dynamically to ensure the layout remains consistent.
35-35: Extract WebSocket URL to a configuration or constantHardcoding the WebSocket URL can make future maintenance and updates more error-prone if the endpoint changes. Consider extracting
/api/v1/target/proxy/session/${sessionId}into a constant or configuration file for better maintainability.Apply this diff to extract the URL:
+ const WEBSOCKET_URL = `/api/v1/target/proxy/session/${sessionId}`; const { getWebSocket, readyState } = useWebSocket( - `/api/v1/target/proxy/session/${sessionId}`, + WEBSOCKET_URL, { shouldReconnect: () => true }, );apps/target-proxy/src/controller/agent-socket.ts (2)
74-74: Remove redundant assignment ofthis.targetThe assignment
this.target = target;is unnecessary becausethis.targetis already initialized via the constructor parameterpublic readonly target: Target.You can safely remove the redundant assignment:
private constructor( private readonly socket: WebSocket, private readonly _: IncomingMessage, public readonly target: Target, ) { - this.target = target; this.socket.on( "message", ifMessage() .is(agentConnect, async (data) => {
Line range hint
119-121: Improve type safety inwaitForResponsemethodThe
predicatefunction inwaitForResponseusesresponse: any, which can lead to type safety issues:waitForResponse<T>(predicate: (response: any) => boolean, timeoutMs = 5000) {Consider replacing
anywith a generic type or a specific type to enhance type safety:- waitForResponse<T>(predicate: (response: any) => boolean, timeoutMs = 5000): Promise<T> { + waitForResponse<T>(predicate: (response: T) => boolean, timeoutMs = 5000): Promise<T> {Ensure that when calling
waitForResponse, the typeTcorrectly represents the expected response type.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
apps/ctrlshell/README.md(0 hunks)apps/ctrlshell/src/payloads/session-create.ts(0 hunks)apps/ctrlshell/src/payloads/session-input.ts(0 hunks)apps/ctrlshell/src/payloads/session-output.ts(0 hunks)apps/ctrlshell/src/routing.ts(0 hunks)apps/ctrlshell/src/session-auditor.ts(0 hunks)apps/ctrlshell/src/user-socket.ts(0 hunks)apps/ctrlshell/src/utils.ts(0 hunks)apps/target-proxy/Dockerfile(1 hunks)apps/target-proxy/README.md(1 hunks)apps/target-proxy/package.json(1 hunks)apps/target-proxy/src/controller/agent-socket.ts(3 hunks)apps/target-proxy/src/controller/index.ts(1 hunks)apps/target-proxy/src/controller/user-socket.ts(1 hunks)apps/target-proxy/src/controller/utils.ts(1 hunks)apps/target-proxy/src/routing.ts(1 hunks)apps/target-proxy/src/server.ts(1 hunks)apps/target-proxy/src/session-auditor.ts(1 hunks)apps/target-proxy/src/sessions/index.ts(1 hunks)apps/target-proxy/src/sessions/servers.ts(1 hunks)apps/webservice/next.config.js(1 hunks)apps/webservice/package.json(3 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/CreateDialogSession.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsDrawer.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/useResizableHeight.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/layout.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/term/Terminal.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/term/page.tsx(1 hunks)apps/webservice/src/components/xterm/SessionTerminal.tsx(1 hunks)apps/webservice/src/components/xterm/XTermAddon.tsx(1 hunks)packages/db/src/schema/access-agent.ts(1 hunks)packages/db/src/schema/target-agent.ts(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/menubar.tsx(1 hunks)packages/ui/src/popover.tsx(1 hunks)packages/validators/package.json(1 hunks)packages/validators/src/session/index.ts(2 hunks)packages/validators/src/session/session-create.ts(1 hunks)packages/validators/src/session/session-delete.ts(1 hunks)packages/validators/src/session/session-resize.ts(1 hunks)
💤 Files with no reviewable changes (8)
- apps/ctrlshell/README.md
- apps/ctrlshell/src/payloads/session-create.ts
- apps/ctrlshell/src/payloads/session-input.ts
- apps/ctrlshell/src/payloads/session-output.ts
- apps/ctrlshell/src/routing.ts
- apps/ctrlshell/src/session-auditor.ts
- apps/ctrlshell/src/user-socket.ts
- apps/ctrlshell/src/utils.ts
✅ Files skipped from review due to trivial changes (2)
- apps/target-proxy/src/sessions/servers.ts
- packages/ui/src/popover.tsx
🧰 Additional context used
🪛 Markdownlint
apps/target-proxy/README.md
5-5: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 GitHub Check: Lint
apps/target-proxy/src/controller/agent-socket.ts
[failure] 131-131:
'any' overrides all other types in this union type
packages/db/src/schema/access-agent.ts
[failure] 4-4:
Unsafe call of a(n) error type typed value
[failure] 4-4:
Unsafe call of a(n) error type typed value
[failure] 5-5:
Unsafe call of a(n) error type typed value
[failure] 5-5:
Unsafe call of a(n) error type typed value
🔇 Additional comments (30)
packages/db/src/schema/access-agent.ts (1)
3-6: Consider adding audit fields and indexes
Since this table is part of a session management system, consider adding:
- Timestamp fields for auditing
- An index on the name field if you plan to query by name
export const accessAgent = pgTable("access_agent", {
id: uuid("id").primaryKey(),
name: varchar("name", { length: 256 }).notNull(),
+ createdAt: timestamp("created_at").notNull().defaultNow(),
+ updatedAt: timestamp("updated_at").notNull().defaultNow(),
+ }, (table) => {
+ return {
+ nameIdx: index("access_agent_name_idx").on(table.name),
+ }
});Let's verify if similar patterns are used in other schema files:
✅ Verification successful
Based on the search results, I can see that timestamp fields and indexes are consistently used across the schema files in the codebase. Let me verify the specific patterns:
- Most tables include
createdAttimestamp fields withdefaultNow()andnotNull() - Many tables also include
updatedAttimestamp fields with$onUpdate - Indexes are commonly used for fields that are queried frequently or need uniqueness constraints
Here's the final response:
Add timestamp fields and name index to match schema patterns
The schema should follow the established patterns in the codebase:
- Add
createdAtwithwithTimezone: trueoption as used in other schemas - Add
updatedAtwith$onUpdatetrigger as seen in job.ts and dashboard.ts - Add index on
namefield as other schemas consistently use indexes for queryable fields
export const accessAgent = pgTable("access_agent", {
id: uuid("id").primaryKey(),
name: varchar("name", { length: 256 }).notNull(),
+ createdAt: timestamp("created_at", { withTimezone: true }).notNull().defaultNow(),
+ updatedAt: timestamp("updated_at", { withTimezone: true })
+ .notNull()
+ .defaultNow()
+ .$onUpdate(() => new Date()),
+ }, (table) => {
+ return {
+ nameIdx: index("access_agent_name_idx").on(table.name),
+ }
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other schema files use timestamps and indexes
fd -e ts -p 'schema.*\.ts$' | xargs rg -A 5 'timestamp|index\('
Length of output: 10584
🧰 Tools
🪛 GitHub Check: Lint
[failure] 4-4:
Unsafe call of a(n) error type typed value
[failure] 4-4:
Unsafe call of a(n) error type typed value
[failure] 5-5:
Unsafe call of a(n) error type typed value
[failure] 5-5:
Unsafe call of a(n) error type typed value
packages/validators/src/session/session-delete.ts (1)
8-8: Verify schema consistency with session-create.ts
The targetId is optional here but required in session-create.ts. This inconsistency might lead to validation issues. Please verify if this difference is intentional.
✅ Verification successful
Let me analyze the schema definitions across session-related files:
session-create.ts:targetIdis requiredsession-resize.ts:targetIdis requiredsession-delete.ts:targetIdis optional
Let me verify the complete schema structure and usage to understand if this inconsistency is justified.
Let me check the index.ts file to understand how these schemas are used together, and search for any WebSocket or proxy router implementations that might explain the optional vs required pattern.
Based on the usage patterns shown in the code, I can now generate a final response:
Optional targetId in session.delete is correct by design
The implementation shows that the targetId field being optional in session-delete.ts is intentional and correct:
- In
apps/target-proxy/src/controller/agent-socket.ts, thedeleteSessionmethod only usessessionIdin the delete payload, not requiringtargetId - In contrast, session creation and resize operations consistently require
targetIdas shown in the TerminalSessionsProvider usage - The schema differences align with the actual implementation requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check targetId requirement across session schemas
# Expected: Find all session-related schemas and their targetId definitions
# Search for all session schema files
echo "Searching for session schema files..."
fd "session-.*\.ts$" packages/validators/src
# Check targetId definitions in these files
echo "Checking targetId definitions..."
rg -A 3 "targetId:" packages/validators/src/session/
Length of output: 1473
Script:
#!/bin/bash
# Get full schema definitions and any relevant imports/types
echo "=== session-create.ts ==="
cat packages/validators/src/session/session-create.ts
echo -e "\n=== session-delete.ts ==="
cat packages/validators/src/session/session-delete.ts
echo -e "\n=== session-resize.ts ==="
cat packages/validators/src/session/session-resize.ts
# Search for usage of these schemas
echo -e "\nSearching for schema usage..."
rg -l "session-(create|delete|resize)" --type ts
Length of output: 1855
Script:
#!/bin/bash
# Check the session index file
echo "=== Session Index File ==="
cat packages/validators/src/session/index.ts
# Search for WebSocket/proxy router implementations using these schemas
echo -e "\n=== WebSocket/Proxy Usage ==="
rg -A 5 "session\.(create|delete|resize)" --type ts
# Look for any documentation or comments about targetId
echo -e "\n=== TargetId Documentation ==="
rg -i "targetid" -A 3 --type md
Length of output: 6215
packages/db/src/schema/target-agent.ts (2)
1-3: LGTM! Clean and minimal imports.
The imports are well-structured, using specific imports rather than wildcards, and correctly reference the target schema.
5-10: Verify session management architecture.
Let's ensure this schema aligns with the broader session management requirements:
- Is there a separate table for session authentication/authorization?
- How does this relate to the removed WebSocket session handling mentioned in the PR summary?
✅ Verification successful
Let me gather more specific information about the session schemas and authentication.
The previous results show multiple session-related files, but we need to verify the specific schema relationships and authentication setup. This will help us understand if the minimal schema is appropriate in the context of the broader architecture.
Schema aligns with the session management architecture
The minimal targetSession schema is appropriate because:
- User authentication is handled separately through the
sessiontable inauth.ts - The
targetSessiontable specifically manages target-agent connections, while WebSocket session handling intarget-proxymanages the real-time communication - The schema follows a clear separation of concerns:
- User sessions (auth.ts) → User authentication/authorization
- Target sessions (target-agent.ts) → Target connection tracking
- WebSocket sessions (target-proxy) → Real-time communication
The removed WebSocket session handling mentioned in the PR summary is correctly replaced by this architecture, where persistent session state is stored in the database and WebSocket communication is handled separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other session-related schemas and their relationships
# Look for other session-related tables
echo "Searching for session-related schemas..."
fd -e ts -e js --full-path '*/schema/*' | xargs rg -l 'session|Session'
# Look for WebSocket-related code that might interact with sessions
echo "Searching for WebSocket session handling..."
rg -l 'WebSocket.*session|session.*WebSocket'
Length of output: 4987
Script:
#!/bin/bash
# Check auth schema and target session relationships
echo "Checking auth schema..."
cat packages/db/src/schema/auth.ts
echo -e "\nChecking target-agent schema context..."
cat packages/db/src/schema/target-agent.ts
echo -e "\nChecking session management in target-proxy..."
cat apps/target-proxy/src/sessions/index.ts
Length of output: 6611
packages/validators/src/session/session-resize.ts (1)
1-11: LGTM! The schema structure is well-defined.
The schema correctly validates the essential fields needed for session resizing with appropriate types and descriptive comments.
packages/validators/src/session/session-create.ts (1)
1-16: Well-structured schema with clear field descriptions!
The schema provides a clear contract for session creation with good documentation for each field.
apps/target-proxy/src/server.ts (1)
Line range hint 14-14: Verify CORS impact on WebSocket connections.
Since this PR introduces WebSocket functionality, ensure that the CORS configuration doesn't interfere with WebSocket connections.
✅ Verification successful
CORS configuration is correctly implemented for WebSocket connections
The verification shows that:
- WebSocket servers are initialized with
noServer: trueoption, which means they rely on the HTTP server's upgrade handling - The CORS middleware is properly configured at the Express app level with
credentials: true - The WebSocket upgrade handling is managed through separate handlers (
controllerOnUpgrade), which inherits the CORS settings from the main HTTP server
This setup follows the recommended pattern where CORS is handled at the HTTP server level before the WebSocket upgrade, ensuring WebSocket connections will work properly with the current CORS configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for WebSocket-related code and CORS configurations
# Look for WebSocket server setup code
echo "Checking for WebSocket server configuration..."
rg -l "WebSocket|ws:|wss:" --type ts
# Look for CORS configurations in WebSocket setup
echo "Checking for CORS in WebSocket setup..."
rg "cors|origin" -C 5 --type ts | grep -i "websocket"
Length of output: 1648
Script:
#!/bin/bash
# Let's check the WebSocket server setup in the target-proxy app
echo "Checking WebSocket server setup in target-proxy..."
rg -A 10 "new WebSocket" apps/target-proxy/src/
# Check how WebSocket server is initialized
echo "Checking WebSocket initialization..."
cat apps/target-proxy/src/sessions/servers.ts
# Check CORS configuration in server setup
echo "Checking CORS configuration..."
rg -A 5 "cors" apps/target-proxy/src/
Length of output: 2796
apps/target-proxy/src/routing.ts (1)
1-5: Verify the implementation of WebSocket upgrade handlers
The code imports two critical WebSocket upgrade handlers. Let's verify their implementations to ensure proper error handling and security measures.
✅ Verification successful
WebSocket handlers implement proper security measures
The verification shows both handlers have appropriate security measures:
-
Session handler:
- Validates session ID presence and format
- Rejects connections with missing/invalid session IDs
- Properly destroys socket on validation failure
- Uses secure session management with
sessionServersmap
-
Controller handler:
- Uses
noServer: truefor manual upgrade handling - Implements error handling for connections
- Closes connections on errors
- Logs security-relevant events
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of WebSocket upgrade handlers
# Expected: Both handlers should exist and implement proper security checks
# Check for the handler implementations
echo "Checking session handler implementation:"
rg -A 10 "export.*sessionOnUpgrade.*=" "apps/target-proxy/src"
echo "Checking controller handler implementation:"
rg -A 10 "export.*controllerOnUpgrade.*=" "apps/target-proxy/src"
# Look for security-related patterns in these files
echo "Checking for security measures:"
rg -l "authenticate|authorize|validate" "apps/target-proxy/src"
Length of output: 1928
Script:
#!/bin/bash
# Let's check more of the session handler implementation to see the security measures
echo "Full session handler implementation:"
rg -B 5 -A 15 "export const sessionOnUpgrade" apps/target-proxy/src/sessions/index.ts
echo "\nChecking getSessionId implementation:"
rg -B 2 -A 10 "getSessionId" apps/target-proxy/src/sessions/index.ts
echo "\nChecking WebSocket server setup and security in controller:"
rg -B 5 -A 15 "const wss = new" apps/target-proxy/src/controller/index.ts
Length of output: 2214
packages/validators/src/session/index.ts (1)
Line range hint 11-17: LGTM! Type declarations follow consistent patterns
The new SessionResize type follows the established pattern of inferring types from Zod schemas, maintaining type safety with runtime validation.
apps/webservice/src/app/[workspaceSlug]/term/Terminal.tsx (1)
4-4: Verify the import alias configuration
The import uses the ~ alias. Ensure that this alias is properly configured in your build setup (e.g., tsconfig.json or webpack config).
✅ Verification successful
Import alias configuration is correctly set up
The ~ alias is properly configured in apps/webservice/tsconfig.json to map to ./src/*. The imported component useSessionTerminal exists at the correct location apps/webservice/src/components/xterm/SessionTerminal.tsx, and this import pattern is consistently used across multiple files in the project. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for tsconfig.json or other config files that define path aliases
# Expected: Find configuration for the "~" alias
# Search for tsconfig files
echo "Searching for tsconfig files..."
fd tsconfig.json
# Search for webpack config files
echo "Searching for webpack config files..."
fd webpack.config.js
# Search for path alias definitions
echo "Searching for path alias configurations..."
rg -l "paths.*~"
Length of output: 1120
Script:
#!/bin/bash
# Let's check the content of the webservice's tsconfig.json since that's the relevant project
cat apps/webservice/tsconfig.json
# Also check for any next.config.js since this appears to be a Next.js app (based on the [workspaceSlug] directory structure)
fd next.config.js
# Let's also verify if the imported component actually exists
fd SessionTerminal.tsx
fd SessionTerminal.ts
# And check if there are any other files using this same import pattern
rg "from ['\"]~/components" -l
Length of output: 1398
apps/target-proxy/src/controller/utils.ts (1)
1-3: LGTM! Appropriate type imports for WebSocket and schema validation.
apps/webservice/src/app/[workspaceSlug]/term/page.tsx (2)
1-10: LGTM! Imports are well-organized and necessary.
The imports cover all required dependencies for WebSocket communication, React functionality, and UI components.
19-19:
Hardcoded targetId should be configurable.
The targetId should not be hardcoded and should be passed as a prop or retrieved from an API.
apps/target-proxy/package.json (2)
14-16: Verify new workspace dependencies
Three new workspace dependencies have been added. Let's ensure they exist and consider version pinning.
#!/bin/bash
# Description: Verify workspace dependencies exist and check versioning patterns
# Check if the new dependencies exist in the workspace
echo "Checking for dependency packages..."
for pkg in "job-dispatch" "logger" "validators"; do
fd "package.json" -x jq -r 'select(.name=="@ctrlplane/'$pkg'") | .name' {} \;
done
# Check versioning patterns across the workspace
echo "Analyzing workspace dependency version patterns..."
fd "package.json" -x jq -r '.dependencies | select(.) | to_entries[] | select(.value | startswith("workspace:")) | .value' {} \; | sort | uniq -c2-2: Verify package rename impact across the monorepo
The package has been renamed from @ctrlplane/ctrlshell to @ctrlplane/target-proxy. This is a breaking change that requires verification of all dependent packages.
#!/bin/bash
# Description: Check for any remaining references to the old package name
# and verify all workspace dependencies are updated
# Search for old package name references
echo "Checking for old package name references..."
rg "@ctrlplane/ctrlshell"
# List all package.json files that might need updates
echo "Checking package.json files for dependencies..."
fd "package.json" -x grep -l "@ctrlplane/ctrlshell" {} \;apps/webservice/src/app/[workspaceSlug]/_components/terminal/useResizableHeight.tsx (2)
32-34: LGTM!
The mouse up handler is simple and correctly handles the cleanup of the dragging state.
47-48: LGTM!
The hook returns a clean and minimal API with just the necessary values.
apps/webservice/next.config.js (2)
42-44: Verify impact of WebSocket route changes
The removal of /webshell/ws route and addition of new session proxy routes indicates an architectural change. Ensure all dependent components have been updated to use the new endpoints.
#!/bin/bash
# Description: Check for any remaining references to the old WebSocket endpoint
# Search for old WebSocket endpoint references
rg -l "webshell/ws"
# Look for WebSocket connection initialization
ast-grep --pattern 'new WebSocket($url)'39-41: Verify security measures for the proxy controller endpoint
The new proxy route exposes a controller endpoint. Ensure proper authentication and authorization are implemented at the API level to prevent unauthorized access.
✅ Verification successful
Based on the search results, I can see that the API endpoints are properly secured with multiple layers of authentication and authorization:
-
The proxy controller endpoint is protected by:
- Authentication middleware (
authn) that verifies API keys - Authorization middleware (
authz) that checks permissions - OpenAPI security schemes requiring API key authentication
- Next.js API route protection
- Authentication middleware (
-
The codebase shows a robust auth implementation:
- API key validation
- Permission-based access control
- Role-based authorization
- Proper error handling returning 401/403 status codes
The proxy controller endpoint is properly secured with authentication and authorization mechanisms
The review comment's concern about security has been addressed through the existing authentication and authorization layers in the codebase. The proxy route is protected by the same security mechanisms as other sensitive API endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for authentication middleware in the controller endpoint
# Look for authentication/authorization checks in the controller
ast-grep --pattern 'app.use($_, auth$_)'
# Check for security middleware or guards
rg -A 5 "controller.*router"
Length of output: 171
Script:
#!/bin/bash
# Search for authentication/security related code in API routes and middleware
rg -A 5 "middleware|auth|protect|guard" apps/webservice
# Look for the controller implementation
fd "controller" --type f
# Check for security headers or configurations
rg -A 5 "security|cors|helmet" apps/webservice
Length of output: 68193
packages/validators/package.json (1)
43-46: Verify the existence of session module files
The new export path for the session module follows the package's established pattern. However, we should verify that the referenced files exist and are properly structured.
✅ Verification successful
Export configuration is correctly structured and source files exist
The session module is properly configured in the package.json and all required source files are present:
- The
src/sessiondirectory exists with all necessary TypeScript files index.tsproperly exports all session-related types and functions- The missing
distdirectory is expected as it's generated during build time
The export configuration follows the same pattern as other modules in the package and correctly maps to the existing source files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of session module files
# Check if the source and distribution files/directories exist
echo "Checking source files..."
fd -t f "index.ts" "src/session"
echo "Checking for session-related schema files..."
fd -t f "session-.*\.ts" "src/session"
# Verify the module's exports
echo "Checking session module exports..."
rg -t typescript "export" "src/session/index.ts"
Length of output: 586
Script:
#!/bin/bash
# Let's first check if the session directory exists in the validators package
echo "Checking for session directory..."
fd -t d "session" "packages/validators"
echo "Checking for session-related TypeScript files..."
fd -t f ".*\.ts" "packages/validators"
echo "Checking package.json exports..."
cat packages/validators/package.json
Length of output: 4319
Script:
#!/bin/bash
# Let's verify the content of session/index.ts and check if it exports the expected functionality
echo "Checking session/index.ts exports..."
cat "packages/validators/src/session/index.ts"
# Also verify if the dist directory exists since it's referenced in package.json
echo -e "\nChecking dist directory structure..."
fd -t d "session" "packages/validators/dist"
Length of output: 1288
apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx (1)
80-87: LGTM: Provider implementation follows React best practices
The context provider implementation is clean and properly structured.
packages/ui/package.json (2)
35-35: LGTM! Dependency version is consistent
The added Radix UI menubar dependency follows the same versioning pattern as other Radix UI components in the package.
35-35: Verify the relationship between menubar and remote sessions
The PR objective mentions initializing remote sessions, but this change introduces menubar UI components. Let's verify how these changes are related.
apps/webservice/package.json (2)
74-74: LGTM! Appropriate WebSocket client choice for React.
The react-use-websocket package is a solid choice for managing WebSocket connections in React applications.
Let's check for any security-related issues in the package:
#!/bin/bash
# Description: Check for any security advisories for react-use-websocket
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-use-websocket") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check if there are any WebSocket security configurations in the codebase
rg -l 'new WebSocket|useWebSocket' --type ts --type tsx45-51: LGTM! Comprehensive terminal setup with all essential addons.
The combination of xterm addons provides a full-featured terminal experience with clipboard support, responsive sizing, search capability, and clickable links. The WebGL addon in devDependencies is a good choice for optional GPU acceleration.
Let's verify the compatibility between the xterm core and its addons:
Also applies to: 101-101
✅ Verification successful
This script will help us identify if there are any version conflicts between different package.json files in the repository, which could indicate compatibility issues between xterm and its addons. Let's analyze the results to ensure consistent versioning across the project.
The xterm dependencies are consistently defined only in the webservice app
The verification shows that xterm and its addons are only used in the webservice application, with no conflicting versions across the codebase. The dependencies follow a consistent pattern with specific version constraints:
- Core xterm package is at ^5.5.0
- Addons have varying but specific version ranges
- No duplicate or conflicting declarations found in other package.json files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any known compatibility issues between xterm and its addons
# by examining their peer dependencies
curl -s https://registry.npmjs.org/@xterm/xterm/5.5.0 | jq '.peerDependencies'
for addon in addon-attach addon-clipboard addon-fit addon-search addon-unicode11 addon-web-links addon-webgl; do
echo "Checking $addon..."
curl -s "https://registry.npmjs.org/@xterm/$addon/latest" | jq '.peerDependencies'
done
Length of output: 437
Script:
#!/bin/bash
# Let's try a different approach to check package compatibility
# by examining package.json files in the repository for any potential version conflicts
# First, let's find all package.json files
fd package.json --type f --exec sh -c '
echo "=== Checking $1 ==="
# Extract all @xterm dependencies and their versions
grep -A 10 "\"@xterm/" "$1" | grep ":" | sed "s/[,\"]//g"
' sh {}
Length of output: 2381
apps/webservice/src/app/[workspaceSlug]/_components/terminal/CreateDialogSession.tsx (1)
1-33: LGTM! Well-structured imports and component declaration.
The component follows React best practices with proper client-side directive and TypeScript typing.
apps/target-proxy/src/controller/index.ts (1)
12-34:
Verify authentication and authorization for connecting clients
Ensure that only authenticated and authorized agents and users are allowed to establish WebSocket connections. If not already implemented in AgentSocket.from and UserSocket.from, consider adding appropriate checks to prevent unauthorized access.
Run the following script to check for authentication logic in AgentSocket and UserSocket classes:
apps/target-proxy/src/sessions/index.ts (1)
70-70: Verify compatibility of 'Array.prototype.at()' method
The use of Array.prototype.at() may not be supported in all runtime environments, as it was introduced in ECMAScript 2022. This could lead to compatibility issues in environments using older JavaScript engines.
Run the following script to check Node.js version and verify support for Array.prototype.at():
If the script outputs 3, then the method is supported. If not, consider using an alternative:
- const sessionId = pathname.split("/").at(-1);
+ const parts = pathname.split("/");
+ const sessionId = parts[parts.length - 1];✅ Verification successful
Let me check the package.json to verify the Node.js version requirements for the project.
Let me try a different approach to check the package.json files:
Let me try one more time to check the root package.json directly:
Array.prototype.at() is fully supported in the project's environment
The project requires Node.js version >=20.12.0 as specified in the root package.json, and the current Node.js version (v22.9.0) confirms that Array.prototype.at() is supported. This method has been available since Node.js 16.6.0, so there are no compatibility concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Node.js version and 'Array.prototype.at()' support
# Output Node.js version
echo "Node.js version: $(node -v)"
# Test 'Array.prototype.at()' support
node -e "console.log([1, 2, 3].at(-1))"
Length of output: 113
Script:
#!/bin/bash
# Check package.json for Node.js version requirements
fd package.json --exec cat {} | jq -r 'select(.engines != null) | .engines.node'
Length of output: 207
Script:
#!/bin/bash
# Find and display Node.js version requirements from package.json files
for f in $(fd package.json); do
echo "=== $f ==="
grep -A1 '"engines"' "$f" || true
done
Length of output: 5335
Script:
#!/bin/bash
# Display the entire content of root package.json
cat package.json
Length of output: 1538
apps/target-proxy/src/controller/agent-socket.ts (1)
131-131:
Address TypeScript union type issue due to 'any' type
The union type in the send method includes types that may resolve to any, causing TypeScript to treat the entire union as any, which undermines type safety:
send(data: SessionCreate | SessionDelete | SessionInput | SessionResize) {Run the following script to identify if any of the types in the union are any:
This script looks for exported types or interfaces that are assigned any. Ensuring all types are properly defined will maintain type safety in your method signatures.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 131-131:
'any' overrides all other types in this union type
packages/ui/src/menubar.tsx (1)
1-220: Overall implementation is correct and follows best practices
The components are well-defined using React.forwardRef, and the code is clean and maintainable.
| import { pgTable } from "drizzle-orm/pg-core"; | ||
|
|
There was a problem hiding this comment.
Add missing imports and documentation
Please add the missing imports and documentation:
- import { pgTable } from "drizzle-orm/pg-core";
+ import { pgTable, uuid, varchar } from "drizzle-orm/pg-core";
+ /**
+ * Schema for access agents that manage remote sessions.
+ * This table stores the unique identifiers and names of access agents
+ * that can establish connections to target systems.
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { pgTable } from "drizzle-orm/pg-core"; | |
| import { pgTable, uuid, varchar } from "drizzle-orm/pg-core"; | |
| /** | |
| * Schema for access agents that manage remote sessions. | |
| * This table stores the unique identifiers and names of access agents | |
| * that can establish connections to target systems. | |
| */ |
| export const targetSession = pgTable("target_session", { | ||
| id: uuid("id").primaryKey(), | ||
| targetId: uuid("target_id") | ||
| .references(() => target.id, { onDelete: "cascade" }) | ||
| .notNull(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential fields for session management and monitoring.
While the basic structure is sound, consider adding the following fields for better session management and debugging capabilities:
- Timestamps (created_at, updated_at) for auditing
- Session status (active, disconnected, etc.)
- Last activity timestamp
- Connection metadata (client IP, user agent)
Here's a suggested enhancement:
export const targetSession = pgTable("target_session", {
id: uuid("id").primaryKey(),
targetId: uuid("target_id")
.references(() => target.id, { onDelete: "cascade" })
.notNull(),
+ status: text("status").notNull().default("active"),
+ createdAt: timestamp("created_at").notNull().defaultNow(),
+ updatedAt: timestamp("updated_at").notNull().defaultNow(),
+ lastActiveAt: timestamp("last_active_at").notNull().defaultNow(),
+ metadata: jsonb("metadata").notNull().default({}),
});Consider adding the following indexes for better query performance:
- Index on
targetIdfor faster lookups of sessions by target - Index on
statusandlastActiveAtfor efficient session cleanup - Composite index on
(targetId, status)if you frequently query active sessions for a target
Would you like me to provide the SQL for these indexes?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const targetSession = pgTable("target_session", { | |
| id: uuid("id").primaryKey(), | |
| targetId: uuid("target_id") | |
| .references(() => target.id, { onDelete: "cascade" }) | |
| .notNull(), | |
| }); | |
| export const targetSession = pgTable("target_session", { | |
| id: uuid("id").primaryKey(), | |
| targetId: uuid("target_id") | |
| .references(() => target.id, { onDelete: "cascade" }) | |
| .notNull(), | |
| status: text("status").notNull().default("active"), | |
| createdAt: timestamp("created_at").notNull().defaultNow(), | |
| updatedAt: timestamp("updated_at").notNull().defaultNow(), | |
| lastActiveAt: timestamp("last_active_at").notNull().defaultNow(), | |
| metadata: jsonb("metadata").notNull().default({}), | |
| }); |
| import type WebSocket from "ws"; | ||
| import { z } from "zod"; | ||
|
|
||
| import { ifMessage } from "./controller/utils"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining a strict message schema instead of using z.any()
While the imports are well-structured, using z.any() later in the code defeats the purpose of using zod for type safety. Consider defining specific message schemas to better validate and type your WebSocket messages.
Example schema definition:
const MessageSchema = z.object({
type: z.string(),
payload: z.unknown(),
timestamp: z.number().optional()
});
type Message = z.infer<typeof MessageSchema>;| .is(z.any(), (data) => { | ||
| console.log(data); | ||
| }) |
There was a problem hiding this comment.
Replace console.log with structured logging and sensitive data handling
Direct console logging of WebSocket messages is not suitable for production and may expose sensitive information.
Consider:
- Implementing structured logging with proper log levels
- Sanitizing sensitive data before logging
- Using environment-based logging configuration
Example implementation:
import { Logger } from './logger'; // Create a proper logger utility
const logger = new Logger('SessionAuditor');
// In your message handler
.is(MessageSchema, (data) => {
logger.info('Session message received', {
type: data.type,
timestamp: new Date().toISOString(),
// Avoid logging sensitive payload data
hasPayload: !!data.payload
});
})| export const auditSessions = (socket: WebSocket) => { | ||
| socket.on( | ||
| "message", | ||
| ifMessage() | ||
| .is(z.any(), (data) => { | ||
| console.log(data); | ||
| }) | ||
| .handle(), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Implement proper error handling and connection lifecycle management
The current implementation lacks error handling and proper WebSocket lifecycle management.
Add error handling and connection cleanup:
export const auditSessions = (socket: WebSocket) => {
+ socket.on('error', (error) => {
+ console.error('WebSocket error:', error);
+ });
+
+ socket.on('close', () => {
+ // Cleanup resources if needed
+ });
+
socket.on(
"message",
ifMessage()
.is(z.any(), (data) => {
console.log(data);
})
.handle(),
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const auditSessions = (socket: WebSocket) => { | |
| socket.on( | |
| "message", | |
| ifMessage() | |
| .is(z.any(), (data) => { | |
| console.log(data); | |
| }) | |
| .handle(), | |
| ); | |
| }; | |
| export const auditSessions = (socket: WebSocket) => { | |
| socket.on('error', (error) => { | |
| console.error('WebSocket error:', error); | |
| }); | |
| socket.on('close', () => { | |
| // Cleanup resources if needed | |
| }); | |
| socket.on( | |
| "message", | |
| ifMessage() | |
| .is(z.any(), (data) => { | |
| console.log(data); | |
| }) | |
| .handle(), | |
| ); | |
| }; |
💡 Codebase verification
Based on the examination of the session management implementation, I can now provide a final response:
The session auditor is not integrated with the session management system
The session management system in sessions/index.ts implements a complete WebSocket-based session handling with message history, but the auditSessions function is not imported or used anywhere. The current auditor implementation exists in isolation and doesn't contribute to the session management functionality.
- The session management system uses
WebSocketServerfor handling multiple client connections - The auditor uses a single
WebSocketinstance without integration into the server infrastructure - Consider integrating the auditor by:
- Adding it to the
wss.on("connection", ...)handler increateSessionSocket - Or implementing it as a middleware in the
sessionOnUpgradefunction
- Adding it to the
🔗 Analysis chain
Verify the integration with the broader session management system
The auditing implementation seems isolated. We should verify how it integrates with the session management system mentioned in the PR description.
Let me gather more information about the session management system by examining the content of the discovered files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other session-related files and their interaction with the auditor
echo "Searching for session-related files..."
fd -e ts -e js --full-path "session|websocket" apps/target-proxy/src
echo "Checking WebSocket server implementation..."
rg -l "new WebSocket.Server" apps/target-proxy/src
echo "Checking audit function usage..."
rg "auditSessions" apps/target-proxy/src
Length of output: 641
Script:
#!/bin/bash
# Check the content of session-related files
echo "Content of sessions/index.ts:"
cat apps/target-proxy/src/sessions/index.ts
echo -e "\nContent of sessions/servers.ts:"
cat apps/target-proxy/src/sessions/servers.ts
# Look for any imports of session-auditor
echo -e "\nChecking imports of session-auditor:"
rg "session-auditor" apps/target-proxy/src
Length of output: 3099
| terminal.loadAddon(new SearchAddon()); | ||
| terminal.loadAddon(new ClipboardAddon()); | ||
| terminal.loadAddon(new WebLinksAddon()); | ||
| terminal.loadAddon(new AttachAddon(getWebsocket() as WebSocket)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the existing ws variable instead of calling getWebsocket() again.
Since ws is already obtained and checked for nullity, you can pass it directly to the AttachAddon to avoid redundant function calls and potential discrepancies.
Apply this diff to use the ws variable:
- terminal.loadAddon(new AttachAddon(getWebsocket() as WebSocket));
+ terminal.loadAddon(new AttachAddon(ws as WebSocket));Committable suggestion skipped: line range outside the PR's diff.
| terminal.loadAddon(new Unicode11Addon()); | ||
| terminal.loadAddon(new WebglAddon()); |
There was a problem hiding this comment.
💡 Codebase verification
Add error handling for WebGL addon initialization
The code currently loads the WebGL and Unicode11 addons without any error handling, which could cause issues in environments where WebGL is not supported. Add a try-catch block specifically for the WebGL addon initialization:
// Load Unicode11 addon (generally well-supported)
terminal.loadAddon(new Unicode11Addon());
// Add fallback for WebGL
try {
terminal.loadAddon(new WebglAddon());
} catch (e) {
console.warn('WebGL rendering is not available, falling back to canvas renderer');
}🔗 Analysis chain
Verify compatibility of Unicode11Addon and WebglAddon.
Ensure that the Unicode11Addon and WebglAddon are compatible with the user's environment. If WebGL is not supported on some client browsers, consider implementing a fallback to prevent potential rendering issues.
Run the following script to check for compatibility concerns:
Let me search for any error handling or fallback mechanisms in the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for known issues with WebglAddon and Unicode11Addon in the codebase or documentation.
# Check for documented compatibility notes in the project.
rg -i "webgladdon|unicode11addon|compatibility|fallback" docs/ README.md
# List browsers or environments where WebGL might not be supported.
echo "Please verify WebGL support on target browsers: https://caniuse.com/webgl"
Length of output: 277
Script:
#!/bin/bash
# Search for WebGL and Unicode addon usage with surrounding context
rg -A 5 -B 5 "WebglAddon|Unicode11Addon"
# Search for any error handling related to these addons
ast-grep --pattern 'try {
$$$
loadAddon($$$)
$$$
} catch'
Length of output: 2592
| const apiKey = request.headers["x-api-key"]?.toString(); | ||
| if (apiKey == null) { | ||
| logger.error("Agent connection rejected - missing API key"); | ||
| throw new Error("API key is required."); | ||
| } | ||
|
|
There was a problem hiding this comment.
Implement validation of the API key
The apiKey is extracted but not validated against any authentication mechanism. This could allow unauthorized access if the API key is incorrect or invalid.
Consider adding logic to validate the apiKey to ensure that only authorized agents can connect. Do you need assistance in implementing the API key validation?
| const agentName = request.headers["x-agent-name"]?.toString(); | ||
| if (agentName == null) { | ||
| logger.warn("Agent connection rejected - missing agent name"); | ||
| return null; | ||
| } | ||
|
|
||
| const apiKey = request.headers["x-api-key"]?.toString(); | ||
| if (apiKey == null) { | ||
| logger.error("Agent connection rejected - missing API key"); | ||
| throw new Error("API key is required."); | ||
| } | ||
|
|
||
| const workspaceSlug = request.headers["x-workspace"]?.toString(); | ||
| if (workspaceSlug == null) { | ||
| logger.error("Agent connection rejected - missing workspace slug"); | ||
| throw new Error("Workspace slug is required."); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error handling for missing headers
The error handling for missing headers is inconsistent:
- For
x-agent-name, the code logs a warning and returnsnull. - For
x-api-keyandx-workspace, the code logs an error and throws an exception.
Consider making the error handling consistent. Here's a suggested refactor to throw exceptions for all missing headers:
const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
- logger.warn("Agent connection rejected - missing agent name");
- return null;
+ logger.error("Agent connection rejected - missing agent name");
+ throw new Error("Agent name is required.");
}
const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
logger.error("Agent connection rejected - missing API key");
throw new Error("API key is required.");
}
const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
logger.error("Agent connection rejected - missing workspace slug");
throw new Error("Workspace slug is required.");
}Alternatively, if returning null is preferred, apply it consistently:
const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
logger.warn("Agent connection rejected - missing agent name");
return null;
}
const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
logger.error("Agent connection rejected - missing API key");
- throw new Error("API key is required.");
+ return null;
}
const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
logger.error("Agent connection rejected - missing workspace slug");
- throw new Error("Workspace slug is required.");
+ return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const agentName = request.headers["x-agent-name"]?.toString(); | |
| if (agentName == null) { | |
| logger.warn("Agent connection rejected - missing agent name"); | |
| return null; | |
| } | |
| const apiKey = request.headers["x-api-key"]?.toString(); | |
| if (apiKey == null) { | |
| logger.error("Agent connection rejected - missing API key"); | |
| throw new Error("API key is required."); | |
| } | |
| const workspaceSlug = request.headers["x-workspace"]?.toString(); | |
| if (workspaceSlug == null) { | |
| logger.error("Agent connection rejected - missing workspace slug"); | |
| throw new Error("Workspace slug is required."); | |
| const agentName = request.headers["x-agent-name"]?.toString(); | |
| if (agentName == null) { | |
| logger.error("Agent connection rejected - missing agent name"); | |
| throw new Error("Agent name is required."); | |
| } | |
| const apiKey = request.headers["x-api-key"]?.toString(); | |
| if (apiKey == null) { | |
| logger.error("Agent connection rejected - missing API key"); | |
| throw new Error("API key is required."); | |
| } | |
| const workspaceSlug = request.headers["x-workspace"]?.toString(); | |
| if (workspaceSlug == null) { | |
| logger.error("Agent connection rejected - missing workspace slug"); | |
| throw new Error("Workspace slug is required."); | |
| ``` | |
| Option 2 (All return null): | |
| ```suggestion | |
| const agentName = request.headers["x-agent-name"]?.toString(); | |
| if (agentName == null) { | |
| logger.warn("Agent connection rejected - missing agent name"); | |
| return null; | |
| } | |
| const apiKey = request.headers["x-api-key"]?.toString(); | |
| if (apiKey == null) { | |
| logger.error("Agent connection rejected - missing API key"); | |
| return null; | |
| } | |
| const workspaceSlug = request.headers["x-workspace"]?.toString(); | |
| if (workspaceSlug == null) { | |
| logger.error("Agent connection rejected - missing workspace slug"); | |
| return null; | |
| } |
| /> | ||
| ); | ||
| }; | ||
| MenubarShortcut.displayname = "MenubarShortcut"; |
There was a problem hiding this comment.
Fix typo in 'displayName' property of 'MenubarShortcut' component
The property should be displayName with an uppercase 'N' to correctly set the component's display name.
Apply this diff to fix the typo:
-MenubarShortcut.displayname = "MenubarShortcut";
+MenubarShortcut.displayName = "MenubarShortcut";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MenubarShortcut.displayname = "MenubarShortcut"; | |
| MenubarShortcut.displayName = "MenubarShortcut"; |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
packages/db/src/schema/target-session.ts (2)
6-14: Consider adding timestamp fields for better auditingWhile the basic schema structure is solid, consider adding
createdAtandupdatedAttimestamp fields. These are valuable for auditing and debugging purposes.export const targetSession = pgTable("target_session", { id: uuid("id").primaryKey(), targetId: uuid("target_id") .references(() => target.id) .notNull(), createdBy: uuid("created_by_id") .references(() => user.id) .notNull(), + createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at").defaultNow().notNull(), });
8-13: Consider performance and data integrity requirementsFor optimal database performance and data integrity, consider:
- Adding indexes on
targetIdandcreatedByif you'll be querying by these fields frequently- Specifying cascade behavior for foreign key references (e.g., what happens to sessions when a target or user is deleted?)
targetId: uuid("target_id") .references(() => target.id) + .onDelete('cascade') // or 'set null' depending on your requirements .notNull(), createdBy: uuid("created_by_id") .references(() => user.id) + .onDelete('set null') // or 'cascade' depending on your requirements .notNull(),apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx (3)
27-33: Fix error message in custom hook.The error message refers to "useSession" but the hook is named "useTerminalSessions".
- throw new Error("useSession must be used within a SessionProvider"); + throw new Error("useTerminalSessions must be used within a TerminalSessionsProvider");
43-45: Add WebSocket connection state handling.The WebSocket connection lacks status tracking for the UI.
- const { sendJsonMessage } = useWebSocket(url, { + const { sendJsonMessage, readyState } = useWebSocket(url, { shouldReconnect: () => true, }); + const connectionStatus = { + [ReadyState.CONNECTING]: 'Connecting', + [ReadyState.OPEN]: 'Connected', + [ReadyState.CLOSING]: 'Closing', + [ReadyState.CLOSED]: 'Disconnected', + [ReadyState.UNINSTANTIATED]: 'Uninstantiated', + }[readyState];
60-61: Remove console.log statement.Remove debugging console.log from production code.
- console.log(resizePayload); sendJsonMessage(resizePayload);apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsDrawer.tsx (2)
29-30: Add loading and error states for target data.The component directly accesses
target.data?.namewithout handling loading or error states from the query.Consider adding proper loading and error states:
const target = api.target.byId.useQuery(targetId); const { resizeSession } = useTerminalSessions(); + +if (target.isLoading) { + return <div className="text-sm text-muted-foreground">Loading target info...</div>; +} + +if (target.error) { + return <div className="text-sm text-red-500">Error loading target: {target.error.message}</div>; +} // In the render: -{target.data?.name} ({targetId} / {sessionId}) +{target.data.name} ({targetId} / {sessionId})Also applies to: 46-46
123-125: Add smooth transition for drawer visibility.The drawer visibility toggle lacks a smooth transition effect.
Add CSS transition:
- className={`fixed bottom-0 left-0 right-0 z-30 bg-black drop-shadow-2xl ${ - isDrawerOpen ? "" : "hidden" - }`} + className={`fixed bottom-0 left-0 right-0 z-30 bg-black drop-shadow-2xl transition-transform duration-300 ease-in-out ${ + isDrawerOpen ? "translate-y-0" : "translate-y-full" + }`}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/SidebarCreateMenu.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/CreateDialogSession.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsDrawer.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/layout.tsx(4 hunks)packages/db/src/schema/target-session.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/_components/terminal/CreateDialogSession.tsx
- apps/webservice/src/app/[workspaceSlug]/layout.tsx
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx (2)
1-23: LGTM! Well-structured type definitions and imports.
The type definitions and imports are comprehensive and properly organized.
82-96: LGTM! Clean context provider implementation.
The provider implementation is well-structured and provides all necessary values to its children.
apps/webservice/src/app/[workspaceSlug]/SidebarCreateMenu.tsx (1)
19-19: LGTM!
The import statement follows the established pattern and correctly references the new session dialog component.
apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsDrawer.tsx (1)
32-32: Verify session ID validation.
The WebSocket URL is constructed using the sessionId parameter without visible validation.
Let's verify the session ID validation:
✅ Verification successful
Based on the search results, I can now generate a final response. The code reveals that authentication is implemented through a middleware and session validation is performed at the API route level.
WebSocket session endpoint is protected by authentication middleware
The findings show:
- Authentication middleware (
auth()) is applied to API routes - The WebSocket connection is established through an authenticated endpoint
- Session IDs are generated using UUID v4, ensuring uniqueness and unpredictability
- The terminal session is scoped within the authenticated workspace context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for session ID validation in the codebase
echo "Searching for session ID validation..."
rg -A 5 "session.*validation|validateSession|isValidSession"
# Search for WebSocket endpoint security measures
echo "Checking WebSocket endpoint security..."
rg -A 10 "proxy/session/.*middleware|validateWebSocketConnection"
Length of output: 306
Script:
#!/bin/bash
# Let's try a broader search for session-related security checks
echo "Searching for session security patterns..."
rg -A 10 "sessionId.*validate|session.*auth|session.*check"
# Look for the API route handler implementation
echo "Looking for proxy session endpoint implementation..."
rg -A 15 "route.*proxy/session/\[sessionId\]"
# Search for middleware that might handle session validation
echo "Checking for auth middleware..."
rg -A 10 "middleware.*session|auth.*middleware"
# Let's also check the terminal session related files
echo "Checking terminal session related files..."
fd -e ts -e tsx terminal.*session
Length of output: 13723
Script:
#!/bin/bash
# Let's check the API routes for session validation
echo "Checking API routes for session validation..."
fd -e ts "route.ts" apps/webservice/src/app/api/v1/target/proxy/
# Let's examine the terminal session provider for security measures
echo "Examining terminal session provider..."
cat "apps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsx"
# Let's look for authentication middleware in the API routes
echo "Looking for API route authentication..."
rg -A 10 "auth\(.*\)|middleware.*auth" "apps/webservice/src/app/api"
# Check for any session validation in the target proxy implementation
echo "Checking target proxy implementation..."
rg -A 10 "proxy.*session|validateTargetSession" "apps/webservice/src/app/api/v1"
Length of output: 4933
Script:
#!/bin/bash
# Let's check for the proxy controller and session route handlers
echo "Checking for proxy controller and session routes..."
fd -t f -e ts proxy apps/webservice/src/app/api/v1
# Let's examine the auth middleware implementation
echo "Examining auth middleware implementation..."
rg -A 10 "export.*auth.*=|function.*auth.*\(" apps/webservice/src/
# Let's look for any session validation in the target-related code
echo "Checking target-related code for session validation..."
fd target apps/webservice/src -t f -e ts -e tsx | xargs rg -l "session"
# Let's check for WebSocket security measures
echo "Checking WebSocket security..."
rg -A 10 "WebSocket|ws:|wss:" apps/webservice/src
Length of output: 32630
| <CreateSessionDialog> | ||
| <DropdownMenuItem onSelect={(e) => e.preventDefault()}> | ||
| Remote Session | ||
| </DropdownMenuItem> | ||
| </CreateSessionDialog> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding workspace and callback props for consistency
The CreateSessionDialog implementation differs from other dialog components in the file as it's missing:
- The
workspaceprop that's passed to other dialogs - An
onSuccessoronClosecallback to close the dropdown menu when the dialog completes
Consider updating the implementation to match the pattern:
- <CreateSessionDialog>
+ <CreateSessionDialog
+ workspace={props.workspace}
+ onSuccess={() => setOpen(false)}
+ >
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
Remote Session
</DropdownMenuItem>
</CreateSessionDialog>Committable suggestion skipped: line range outside the PR's diff.
| <CreateSessionDialog> | ||
| <Button variant="ghost" size="icon" className="h-6 w-6"> | ||
| <IconPlus className="h-5 w-5 text-neutral-400" /> | ||
| </Button> | ||
| </CreateSessionDialog> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for terminal control buttons.
The create and close buttons lack proper accessibility attributes.
Add ARIA labels and keyboard support:
<CreateSessionDialog>
<Button
variant="ghost"
size="icon"
className="h-6 w-6"
+ aria-label="Create new terminal session"
>
<IconPlus className="h-5 w-5 text-neutral-400" />
</Button>
</CreateSessionDialog>
<Button
variant="ghost"
size="icon"
className="h-6 w-6"
onClick={() => setIsDrawerOpen(false)}
+ aria-label="Close terminal sessions drawer"
>
<IconX className="h-5 w-5 text-neutral-400" />
</Button>Also applies to: 90-98
| <div | ||
| className="absolute left-0 right-0 top-0 h-1 cursor-ns-resize bg-neutral-800 hover:bg-neutral-700" | ||
| onMouseDown={handleMouseDown} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance resize handle accessibility and user experience.
The resize handle lacks keyboard support and proper ARIA attributes for accessibility.
Add keyboard support and ARIA attributes:
<div
className="absolute left-0 right-0 top-0 h-1 cursor-ns-resize bg-neutral-800 hover:bg-neutral-700"
onMouseDown={handleMouseDown}
+ role="separator"
+ aria-orientation="horizontal"
+ aria-label="Resize terminal drawer"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'ArrowUp') {
+ // TODO: Implement keyboard resize logic
+ } else if (e.key === 'ArrowDown') {
+ // TODO: Implement keyboard resize logic
+ }
+ }}
/>Committable suggestion skipped: line range outside the PR's diff.
| const { getWebSocket, readyState } = useWebSocket( | ||
| `/api/v1/target/proxy/session/${sessionId}`, | ||
| { shouldReconnect: () => true }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for WebSocket connection failures.
The WebSocket connection lacks explicit error handling. While shouldReconnect is set to always retry, users should be informed about connection issues.
Consider adding error handling:
const { getWebSocket, readyState } = useWebSocket(
`/api/v1/target/proxy/session/${sessionId}`,
- { shouldReconnect: () => true },
+ {
+ shouldReconnect: (closeEvent) => {
+ console.error('WebSocket connection closed:', closeEvent);
+ return closeEvent.code !== 1000; // Don't reconnect on normal closure
+ },
+ reconnectAttempts: 5,
+ reconnectInterval: 3000,
+ onError: (event) => {
+ console.error('WebSocket error:', event);
+ // TODO: Add user notification about connection issues
+ },
+ },
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/validators/src/session/index.ts (1)
3-7: Confirm the necessity of '.js' extensions in import statementsIn TypeScript projects, import statements typically omit the
.jsfile extension. Unless your build configuration specifically requires the.jsextension for module resolution, consider removing it for consistency and adherence to TypeScript best practices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/validators/src/session/index.ts(1 hunks)
🔇 Additional comments (1)
packages/validators/src/session/index.ts (1)
1-21: LGTM
The session validators and their corresponding types are correctly imported and exported. The code is clean and follows good practices.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
apps/target-proxy/src/controller/agent-socket.ts (2)
49-56: Consider making target metadata configurable.The target information contains hardcoded values that might need to be configurable:
version: "ctrlplane/v1"kind: "TargetSession"identifierprefix:"ctrlplane/target-agent/"Consider moving these to configuration constants or environment variables for better maintainability.
89-106: Consider making session timeouts configurable.The session creation and deletion methods use a hardcoded timeout of 5000ms from waitForResponse. This might need adjustment based on network conditions or operational requirements.
Consider accepting a timeout parameter or using a configurable default:
+ private static readonly DEFAULT_TIMEOUT = 5000; + createSession(session: SessionCreate) { this.send(session); return this.waitForResponse( (response) => response.type === "session.created", + this.DEFAULT_TIMEOUT ); } async deleteSession(sessionId: string) { const deletePayload: SessionDelete = { type: "session.delete", sessionId, }; this.send(deletePayload); return this.waitForResponse( (response) => response.type === "session.delete.success", + this.DEFAULT_TIMEOUT ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/target-proxy/src/controller/agent-socket.ts(1 hunks)turbo.json(1 hunks)
🔇 Additional comments (3)
turbo.json (1)
18-19: LGTM! Build output paths properly configured.
The addition of .vercel/output/** and dist/** to the build outputs is correct and necessary for:
- Proper caching of Vercel deployment artifacts
- Tracking compiled distribution files
Let's verify the build output structure:
✅ Verification successful
Build output paths correctly configured and actively used
The verification confirms that the .vercel/output/** and dist/** output paths in turbo.json are properly aligned with the project's build system:
- Multiple packages use TypeScript compilation with
outDirconfiguration in their tsconfig.json files - Build scripts across packages use
tsc,tsup, andnext buildwhich generate artifacts in these directories - The
ncc buildcommand in GitHub integration explicitly uses the output directory pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and usage of new output directories
# Check for dist directories in the codebase
fd -t d "^dist$"
# Check for references to these paths in package.json build scripts
fd -t f "package.json" --exec rg -l "dist|.vercel/output"
Length of output: 579
Script:
#!/bin/bash
# Let's check the actual build script contents to verify the output paths usage
# Check build scripts in package.json files
fd -t f "package.json" -x jq -r '.scripts | select(.build != null) | .build' {}
# Check for any TypeScript/build configurations that might reference these paths
fd -t f "tsconfig.json" -x grep -l "outDir\|dist" {}
# Look for build-related configuration files
fd -t f "(build|rollup|webpack).config.(js|ts)"
Length of output: 1063
apps/target-proxy/src/controller/agent-socket.ts (2)
1-20: LGTM! Well-organized imports.
The imports are properly organized with clear separation between types and implementations.
29-34: API key validation is still missing.
Referencing the previous review comment about implementing API key validation.
| const onMessage = (event: MessageEvent) => { | ||
| try { | ||
| const response = JSON.parse( | ||
| typeof event.data === "string" ? event.data : "", | ||
| ); | ||
| if (predicate(response)) { | ||
| clearTimeout(timeout); | ||
| socket.removeEventListener("message", onMessage); | ||
| resolve(response); | ||
| } | ||
| } catch { | ||
| clearTimeout(timeout); | ||
| socket.removeEventListener("message", onMessage); | ||
| reject(new Error("Failed to parse response")); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure cleanup in all error paths.
The message handler's catch block should only clear the timeout and remove the listener if they haven't already been cleared/removed.
const onMessage = (event: MessageEvent) => {
+ let cleaned = false;
+ const cleanup = () => {
+ if (!cleaned) {
+ cleaned = true;
+ clearTimeout(timeout);
+ socket.removeEventListener("message", onMessage);
+ }
+ };
try {
const response = JSON.parse(
typeof event.data === "string" ? event.data : "",
);
if (predicate(response)) {
- clearTimeout(timeout);
- socket.removeEventListener("message", onMessage);
+ cleanup();
resolve(response);
}
} catch {
- clearTimeout(timeout);
- socket.removeEventListener("message", onMessage);
+ cleanup();
reject(new Error("Failed to parse response"));
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onMessage = (event: MessageEvent) => { | |
| try { | |
| const response = JSON.parse( | |
| typeof event.data === "string" ? event.data : "", | |
| ); | |
| if (predicate(response)) { | |
| clearTimeout(timeout); | |
| socket.removeEventListener("message", onMessage); | |
| resolve(response); | |
| } | |
| } catch { | |
| clearTimeout(timeout); | |
| socket.removeEventListener("message", onMessage); | |
| reject(new Error("Failed to parse response")); | |
| } | |
| }; | |
| const onMessage = (event: MessageEvent) => { | |
| let cleaned = false; | |
| const cleanup = () => { | |
| if (!cleaned) { | |
| cleaned = true; | |
| clearTimeout(timeout); | |
| socket.removeEventListener("message", onMessage); | |
| } | |
| }; | |
| try { | |
| const response = JSON.parse( | |
| typeof event.data === "string" ? event.data : "", | |
| ); | |
| if (predicate(response)) { | |
| cleanup(); | |
| resolve(response); | |
| } | |
| } catch { | |
| cleanup(); | |
| reject(new Error("Failed to parse response")); | |
| } | |
| }; |
| .is(agentConnect, async (data) => { | ||
| await upsertTargets(db, [ | ||
| { | ||
| ...target, | ||
| config: data.config, | ||
| metadata: data.metadata, | ||
| version: "ctrlplane/v1", | ||
| }, | ||
| ]); | ||
| }) |
There was a problem hiding this comment.
Add error handling for target update.
The agentConnect handler lacks error handling for the database operation.
.is(agentConnect, async (data) => {
+ try {
await upsertTargets(db, [
{
...target,
config: data.config,
metadata: data.metadata,
version: "ctrlplane/v1",
},
]);
+ } catch (error) {
+ logger.error("Failed to update target with agent data", {
+ error,
+ targetId: target.id,
+ });
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .is(agentConnect, async (data) => { | |
| await upsertTargets(db, [ | |
| { | |
| ...target, | |
| config: data.config, | |
| metadata: data.metadata, | |
| version: "ctrlplane/v1", | |
| }, | |
| ]); | |
| }) | |
| .is(agentConnect, async (data) => { | |
| try { | |
| await upsertTargets(db, [ | |
| { | |
| ...target, | |
| config: data.config, | |
| metadata: data.metadata, | |
| version: "ctrlplane/v1", | |
| }, | |
| ]); | |
| } catch (error) { | |
| logger.error("Failed to update target with agent data", { | |
| error, | |
| targetId: target.id, | |
| }); | |
| } | |
| }) |
| private updateStatus(data: AgentHeartbeat) { | ||
| console.log("status", data.timestamp); | ||
| } |
There was a problem hiding this comment.
Replace console.log with proper logging.
Using console.log is not appropriate for production code.
- console.log("status", data.timestamp);
+ logger.debug("Agent heartbeat received", { timestamp: data.timestamp });Committable suggestion skipped: line range outside the PR's diff.
| const workspace = await db.query.workspace.findFirst({ | ||
| where: eq(schema.workspace.slug, workspaceSlug), | ||
| }); | ||
| if (workspace == null) { | ||
| logger.error("Agent connection rejected - workspace not found"); | ||
| return null; | ||
| } | ||
|
|
||
| const targetInfo: InsertTarget = { | ||
| name: agentName, | ||
| version: "ctrlplane/v1", | ||
| kind: "TargetSession", | ||
| identifier: `ctrlplane/target-agent/${agentName}`, | ||
| workspaceId: workspace.id, | ||
| }; | ||
| const [target] = await upsertTargets(db, [targetInfo]); | ||
| if (target == null) return null; | ||
| return new AgentSocket(socket, request, target); |
There was a problem hiding this comment.
Add error handling for database operations.
The database operations lack proper error handling. If the database query or upsert fails, it could throw unhandled exceptions.
Add try-catch blocks:
- const workspace = await db.query.workspace.findFirst({
- where: eq(schema.workspace.slug, workspaceSlug),
- });
+ let workspace;
+ try {
+ workspace = await db.query.workspace.findFirst({
+ where: eq(schema.workspace.slug, workspaceSlug),
+ });
+ } catch (error) {
+ logger.error("Failed to query workspace", { error, workspaceSlug });
+ return null;
+ }
if (workspace == null) {
logger.error("Agent connection rejected - workspace not found");
return null;
}
const targetInfo: InsertTarget = {
name: agentName,
version: "ctrlplane/v1",
kind: "TargetSession",
identifier: `ctrlplane/target-agent/${agentName}`,
workspaceId: workspace.id,
};
- const [target] = await upsertTargets(db, [targetInfo]);
+ let target;
+ try {
+ [target] = await upsertTargets(db, [targetInfo]);
+ } catch (error) {
+ logger.error("Failed to upsert target", { error, targetInfo });
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const workspace = await db.query.workspace.findFirst({ | |
| where: eq(schema.workspace.slug, workspaceSlug), | |
| }); | |
| if (workspace == null) { | |
| logger.error("Agent connection rejected - workspace not found"); | |
| return null; | |
| } | |
| const targetInfo: InsertTarget = { | |
| name: agentName, | |
| version: "ctrlplane/v1", | |
| kind: "TargetSession", | |
| identifier: `ctrlplane/target-agent/${agentName}`, | |
| workspaceId: workspace.id, | |
| }; | |
| const [target] = await upsertTargets(db, [targetInfo]); | |
| if (target == null) return null; | |
| return new AgentSocket(socket, request, target); | |
| let workspace; | |
| try { | |
| workspace = await db.query.workspace.findFirst({ | |
| where: eq(schema.workspace.slug, workspaceSlug), | |
| }); | |
| } catch (error) { | |
| logger.error("Failed to query workspace", { error, workspaceSlug }); | |
| return null; | |
| } | |
| if (workspace == null) { | |
| logger.error("Agent connection rejected - workspace not found"); | |
| return null; | |
| } | |
| const targetInfo: InsertTarget = { | |
| name: agentName, | |
| version: "ctrlplane/v1", | |
| kind: "TargetSession", | |
| identifier: `ctrlplane/target-agent/${agentName}`, | |
| workspaceId: workspace.id, | |
| }; | |
| let target; | |
| try { | |
| [target] = await upsertTargets(db, [targetInfo]); | |
| } catch (error) { | |
| logger.error("Failed to upsert target", { error, targetInfo }); | |
| return null; | |
| } | |
| if (target == null) return null; | |
| return new AgentSocket(socket, request, target); |
Summary by CodeRabbit
Release Notes
New Features
/api/proxy/healthfor monitoring service status.Documentation
Bug Fixes
Chores