Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the TypeScript build configuration to prevent test files from being compiled and included in production builds, particularly when building Docker images. The changes ensure that only source code from the src/ directory is compiled by TypeScript, while test files continue to be handled separately by Vitest.
- Added
rootDirconfiguration to properly scope TypeScript compilation to the source directory - Removed test files from TypeScript compilation include patterns
- Explicitly excluded test directory to prevent compilation of test files
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
backend/src/routes/puppet.ts
Outdated
| onCommand: (cmd: string) => { streamingManager.emitCommand(executionId, cmd); }, | ||
| onStdout: (chunk: string) => { streamingManager.emitStdout(executionId, chunk); }, | ||
| onStderr: (chunk: string) => { streamingManager.emitStderr(executionId, chunk); }, |
There was a problem hiding this comment.
[nitpick] Inconsistent arrow function syntax for streaming callbacks. In tasks.ts, the callbacks use explicit return type annotations and are formatted across multiple lines (e.g., onCommand: (cmd: string): void => { ... }), while in this file they're inline with braces but no return type. Consider standardizing the pattern across all route files for consistency.
| onCommand: (cmd: string) => { streamingManager.emitCommand(executionId, cmd); }, | |
| onStdout: (chunk: string) => { streamingManager.emitStdout(executionId, chunk); }, | |
| onStderr: (chunk: string) => { streamingManager.emitStderr(executionId, chunk); }, | |
| onCommand: (cmd: string): void => { | |
| streamingManager.emitCommand(executionId, cmd); | |
| }, | |
| onStdout: (chunk: string): void => { | |
| streamingManager.emitStdout(executionId, chunk); | |
| }, | |
| onStderr: (chunk: string): void => { | |
| streamingManager.emitStderr(executionId, chunk); | |
| }, |
backend/src/routes/packages.ts
Outdated
| onCommand: (cmd: string) => { streamingManager.emitCommand(executionId, cmd); }, | ||
| onStdout: (chunk: string) => { streamingManager.emitStdout(executionId, chunk); }, | ||
| onStderr: (chunk: string) => { streamingManager.emitStderr(executionId, chunk); }, |
There was a problem hiding this comment.
[nitpick] Inconsistent arrow function syntax for streaming callbacks. In tasks.ts, the callbacks use explicit return type annotations and are formatted across multiple lines (e.g., onCommand: (cmd: string): void => { ... }), while in this file they're inline with braces but no return type. Consider standardizing the pattern across all route files for consistency.
| onCommand: (cmd: string) => { streamingManager.emitCommand(executionId, cmd); }, | |
| onStdout: (chunk: string) => { streamingManager.emitStdout(executionId, chunk); }, | |
| onStderr: (chunk: string) => { streamingManager.emitStderr(executionId, chunk); }, | |
| onCommand: (cmd: string): void => { | |
| streamingManager.emitCommand(executionId, cmd); | |
| }, | |
| onStdout: (chunk: string): void => { | |
| streamingManager.emitStdout(executionId, chunk); | |
| }, | |
| onStderr: (chunk: string): void => { | |
| streamingManager.emitStderr(executionId, chunk); | |
| }, |
backend/src/routes/commands.ts
Outdated
| onCommand: (cmd: string) => { streamingManager.emitCommand(executionId, cmd); }, | ||
| onStdout: (chunk: string) => { streamingManager.emitStdout(executionId, chunk); }, | ||
| onStderr: (chunk: string) => { streamingManager.emitStderr(executionId, chunk); }, |
There was a problem hiding this comment.
[nitpick] Inconsistent arrow function syntax for streaming callbacks. In tasks.ts, the callbacks use explicit return type annotations and are formatted across multiple lines (e.g., onCommand: (cmd: string): void => { ... }), while in this file they're inline with braces but no return type. Consider standardizing the pattern across all route files for consistency.
| onCommand: (cmd: string) => { streamingManager.emitCommand(executionId, cmd); }, | |
| onStdout: (chunk: string) => { streamingManager.emitStdout(executionId, chunk); }, | |
| onStderr: (chunk: string) => { streamingManager.emitStderr(executionId, chunk); }, | |
| onCommand: (cmd: string): void => { | |
| streamingManager.emitCommand(executionId, cmd); | |
| }, | |
| onStdout: (chunk: string): void => { | |
| streamingManager.emitStdout(executionId, chunk); | |
| }, | |
| onStderr: (chunk: string): void => { | |
| streamingManager.emitStderr(executionId, chunk); | |
| }, |
- Replace String() constructor with .toString() method for better type safety - Use nullish coalescing operator (??) instead of logical OR (||) - Add explicit Promise.resolve() return for async handlers - Remove debug console.log statements from main.ts - Improve error message handling with proper null checks - Enhance code consistency across backend services and routes Files modified: - backend/src/bolt/BoltService.ts - backend/src/config/ConfigService.ts - backend/src/routes/commands.ts - backend/src/routes/executions.ts - backend/src/routes/packages.ts - backend/src/routes/puppet.ts - backend/src/routes/streaming.ts - backend/src/services/ExecutionQueue.ts - backend/src/services/StreamingExecutionManager.ts - frontend/src/lib/api.ts - frontend/src/main.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| asyncHandler((_req: Request, res: Response): Promise<void> => { | ||
| res.json({ | ||
| activeExecutions: streamingManager.getActiveExecutionCount(), | ||
| }); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
The handler function is not declared as async, so it must explicitly return a Promise in all code paths. Currently, line 93 returns Promise.resolve() for the error path, but lines 90-92 don't return a promise for the success path.
Either:
- Add
returnbeforePromise.resolve()on line 93, and wrap the entire function inasyncand remove the explicit Promise.resolve(), OR - Change lines 90-93 to:
return Promise.resolve(res.json({...}));
For consistency with other handlers in the codebase that ARE async (e.g., executions.ts lines 40, 102), consider making this handler async as well.
| asyncHandler((_req: Request, res: Response): Promise<void> => { | |
| res.json({ | |
| activeExecutions: streamingManager.getActiveExecutionCount(), | |
| }); | |
| return Promise.resolve(); | |
| asyncHandler(async (_req: Request, res: Response): Promise<void> => { | |
| res.json({ | |
| activeExecutions: streamingManager.getActiveExecutionCount(), | |
| }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.