feat: add compilation status polling with progressive log display#345
Conversation
- Add IPC handler for compilation-status API endpoint - Implement polling after successful program upload - Track log count to display only new messages - Poll every 1 second until SUCCESS or FAILED status - Display final compilation result with exit code - Handle timeout after 5 minutes of polling This implements real-time compilation feedback from the OpenPLC Runtime by continuously polling the compilation-status endpoint and displaying new log messages as they become available, preventing duplicate messages by tracking the number of previously displayed logs. Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Free review on us!CodeRabbit is offering free reviews until Wed Oct 08 2025 to showcase some of the refinements we've made. Comment |
…equest The previous implementation tried to use rendererProcessBridge (ipcRenderer) from the main process context, causing 'Cannot read properties of undefined' error. Now makes direct HTTPS GET requests to /api/compilation-status following the same pattern as the upload request. This fixes the bug reported by the user where compilation status polling failed with the error message about 'invoke' being undefined. Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements compilation status polling for the OpenPLC Runtime integration, providing real-time feedback during the compilation process through progressive log display.
- Adds new IPC handler for compilation-status API with proper request/response structure
- Implements polling mechanism that tracks compilation progress and displays new logs incrementally
- Adds comprehensive error handling with timeout protection and proper termination conditions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/modules/ipc/renderer.ts | Adds renderer bridge method for compilation status API |
| src/main/modules/ipc/main.ts | Implements main process IPC handler for compilation status requests |
| src/main/modules/compiler/compiler-module.ts | Implements polling logic with progressive log display and deduplication |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const pollCompilationStatus = async () => { | ||
| let lastLogCount = 0 | ||
| const startTime = Date.now() | ||
| const timeout = 5 * 60 * 1000 | ||
| const pollInterval = 1000 |
There was a problem hiding this comment.
The magic numbers for timeout (5 minutes) and poll interval (1 second) should be extracted as named constants to improve maintainability and make them configurable.
| const pollCompilationStatus = async () => { | |
| let lastLogCount = 0 | |
| const startTime = Date.now() | |
| const timeout = 5 * 60 * 1000 | |
| const pollInterval = 1000 | |
| // Extracted magic numbers as named constants for maintainability | |
| const COMPILATION_STATUS_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes | |
| const COMPILATION_STATUS_POLL_INTERVAL_MS = 1000; // 1 second | |
| const pollCompilationStatus = async () => { | |
| let lastLogCount = 0 | |
| const startTime = Date.now() | |
| const timeout = COMPILATION_STATUS_TIMEOUT_MS | |
| const pollInterval = COMPILATION_STATUS_POLL_INTERVAL_MS |
There was a problem hiding this comment.
Devin, could you take a look at this comment please?
| const statusData = await new Promise<{ | ||
| status: string | ||
| logs: string[] | ||
| exit_code: number | null | ||
| }>((resolve, reject) => { | ||
| const req = https.request( | ||
| { | ||
| hostname: runtimeIpAddress, | ||
| port: 8443, | ||
| path: '/api/compilation-status', | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: `Bearer ${runtimeJwtToken}`, | ||
| }, | ||
| rejectUnauthorized: false, | ||
| }, | ||
| (res: IncomingMessage) => { | ||
| let data = '' | ||
| res.on('data', (chunk: Buffer) => { | ||
| data += chunk.toString() | ||
| }) | ||
| res.on('end', () => { | ||
| if (res.statusCode === 200) { | ||
| try { | ||
| const parsed = JSON.parse(data) as { | ||
| status: string | ||
| logs: string[] | ||
| exit_code: number | null | ||
| } | ||
| resolve(parsed) | ||
| } catch (parseError) { | ||
| reject( | ||
| new Error( | ||
| `Failed to parse compilation status response: ${parseError instanceof Error ? parseError.message : String(parseError)}`, | ||
| ), | ||
| ) | ||
| } | ||
| } else { | ||
| reject(new Error(`HTTP ${res.statusCode}: ${data}`)) | ||
| } | ||
| }) | ||
| }, | ||
| ) | ||
| req.on('error', (error: Error) => { | ||
| reject(error) | ||
| }) | ||
| req.end() | ||
| }) |
There was a problem hiding this comment.
The HTTPS request logic is duplicated from the existing _makeRuntimeApiRequest method. Consider reusing that method instead of reimplementing the same logic inline.
| const statusData = await new Promise<{ | |
| status: string | |
| logs: string[] | |
| exit_code: number | null | |
| }>((resolve, reject) => { | |
| const req = https.request( | |
| { | |
| hostname: runtimeIpAddress, | |
| port: 8443, | |
| path: '/api/compilation-status', | |
| method: 'GET', | |
| headers: { | |
| Authorization: `Bearer ${runtimeJwtToken}`, | |
| }, | |
| rejectUnauthorized: false, | |
| }, | |
| (res: IncomingMessage) => { | |
| let data = '' | |
| res.on('data', (chunk: Buffer) => { | |
| data += chunk.toString() | |
| }) | |
| res.on('end', () => { | |
| if (res.statusCode === 200) { | |
| try { | |
| const parsed = JSON.parse(data) as { | |
| status: string | |
| logs: string[] | |
| exit_code: number | null | |
| } | |
| resolve(parsed) | |
| } catch (parseError) { | |
| reject( | |
| new Error( | |
| `Failed to parse compilation status response: ${parseError instanceof Error ? parseError.message : String(parseError)}`, | |
| ), | |
| ) | |
| } | |
| } else { | |
| reject(new Error(`HTTP ${res.statusCode}: ${data}`)) | |
| } | |
| }) | |
| }, | |
| ) | |
| req.on('error', (error: Error) => { | |
| reject(error) | |
| }) | |
| req.end() | |
| }) | |
| const statusData = await _makeRuntimeApiRequest({ | |
| hostname: runtimeIpAddress, | |
| port: 8443, | |
| path: '/api/compilation-status', | |
| method: 'GET', | |
| headers: { | |
| Authorization: `Bearer ${runtimeJwtToken}`, | |
| }, | |
| rejectUnauthorized: false, | |
| }); |
There was a problem hiding this comment.
Devin, can you take a look at this comment please?
- Add COMPILATION_STATUS_TIMEOUT_MS and COMPILATION_STATUS_POLL_INTERVAL_MS constants - Add RUNTIME_API_PORT constant for API port configuration - Extract HTTPS GET request logic to _makeRuntimeApiGetRequest helper method - Update polling logic to use constants and helper method - Reduces code duplication and improves maintainability Addresses Copilot suggestions in PR review. Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
|
Thanks for the feedback! I've addressed both suggestions:
Note on architecture: I created the helper method within CompilerModule rather than using MainProcessBridge's
The refactored code now follows the same pattern as MainProcessBridge (helper method + constants) while maintaining proper module boundaries. All changes have been pushed and both lint and build checks pass successfully. |
…Bridge - Make _makeRuntimeApiRequest public as makeRuntimeApiRequest - Pass MainProcessBridge instance to compileProgram - Remove duplicate _makeRuntimeApiGetRequest method from CompilerModule - Remove duplicate RUNTIME_API_PORT constant from CompilerModule - Update polling logic to use mainProcessBridge.makeRuntimeApiRequest - Consolidates all HTTPS request logic in MainProcessBridge Addresses code review feedback to eliminate duplication. Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const timeout = CompilerModule.COMPILATION_STATUS_TIMEOUT_MS | ||
| const pollInterval = CompilerModule.COMPILATION_STATUS_POLL_INTERVAL_MS | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Using an infinite while loop without a clear exit condition makes the code harder to reason about. Consider refactoring to use a more explicit loop condition or state machine pattern.
There was a problem hiding this comment.
Devin, could you put a specific condition here instead of an infinite loop? Maybe tie this loop with the compilation status, for example if the API poll fails, or if the status changes to success or failed, the loop must end.
| mainProcessBridge: { | ||
| makeRuntimeApiRequest: <T = void>( | ||
| ipAddress: string, | ||
| jwtToken: string, | ||
| endpoint: string, | ||
| responseParser?: (data: string) => T, | ||
| ) => Promise<{ success: true; data?: T } | { success: false; error: string }> | ||
| }, |
There was a problem hiding this comment.
The inline type definition for mainProcessBridge creates tight coupling and makes the interface difficult to maintain. Consider extracting this to a proper interface or using the actual MainProcessBridge type.
There was a problem hiding this comment.
Devin, can you take a look at this please?
|
Thanks for pointing this out! I've investigated the inline type definition for Current SituationThe inline structural type definition is verbose and creates tight coupling: mainProcessBridge: {
makeRuntimeApiRequest: <T = void>(
ipAddress: string,
jwtToken: string,
endpoint: string,
responseParser?: (data: string) => T,
) => Promise<{ success: true; data?: T } | { success: false; error: string }>
}Why Not Import MainProcessBridge Directly?I investigated importing the
Recommended SolutionFollowing the existing pattern in
This approach: Would you like me to implement this refactoring? |
|
Thanks for catching this! You're absolutely right - while the loop does have exit conditions (timeout check, API failures, SUCCESS/FAILED states), they're hidden inside the loop body as I'll refactor it to use an explicit loop condition instead of let shouldContinuePolling = true
const startTime = Date.now()
const timeout = CompilerModule.COMPILATION_STATUS_TIMEOUT_MS
const pollInterval = CompilerModule.COMPILATION_STATUS_POLL_INTERVAL_MS
while (shouldContinuePolling) {
// Check timeout
if (Date.now() - startTime > timeout) {
_mainProcessPort.postMessage({
logLevel: 'error',
message: 'Compilation status polling timed out after 5 minutes.',
})
shouldContinuePolling = false
continue
}
await new Promise((resolve) => setTimeout(resolve, pollInterval))
try {
const result = await mainProcessBridge.makeRuntimeApiRequest<{
status: string
logs: string[]
exit_code: number | null
}>(runtimeIpAddress, runtimeJwtToken, '/api/compilation-status', (data: string) => {
return JSON.parse(data) as { status: string; logs: string[]; exit_code: number | null }
})
if (!result.success) {
_mainProcessPort.postMessage({
logLevel: 'error',
message: `Error polling compilation status: ${result.error}`,
})
shouldContinuePolling = false
continue
}
const { status, logs, exit_code } = result.data!
// Handle new logs (same as before)
if (logs.length > lastLogCount) {
const newLogs = logs.slice(lastLogCount)
newLogs.forEach((log) => {
_mainProcessPort.postMessage({
logLevel: 'info',
message: log,
})
})
lastLogCount = logs.length
}
// Check terminal states
if (status === 'SUCCESS') {
_mainProcessPort.postMessage({
logLevel: 'info',
message: `Compilation completed successfully (exit code: ${exit_code ?? 0}).`,
})
shouldContinuePolling = false
} else if (status === 'FAILED') {
_mainProcessPort.postMessage({
logLevel: 'error',
message: `Compilation failed (exit code: ${exit_code ?? 1}).`,
})
shouldContinuePolling = false
}
} catch (pollError) {
_mainProcessPort.postMessage({
logLevel: 'error',
message: `Error polling compilation status: ${pollError instanceof Error ? pollError.message : String(pollError)}`,
})
shouldContinuePolling = false
}
}This makes it immediately clear that the loop continues only while Would you like me to implement this refactoring? |
…n polling - Changed while(true) to while(shouldContinuePolling) for clarity - All break statements replaced with shouldContinuePolling = false - Makes exit conditions (timeout, SUCCESS, FAILED, errors) explicit - Improves code readability and maintainability Addresses GitHub PR review feedback from @thiagoralves Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
|
✅ Refactoring complete! I've updated the code to use an explicit loop condition instead of Changes made:
The refactoring improves code readability while preserving all existing behavior. Lint and build checks pass successfully. Commit: 6efeb27 |
Pull request info
References
This PR addresses the task to implement real-time compilation status tracking from the OpenPLC Runtime API with progressive log display and deduplication.
Link to Devin run: https://app.devin.ai/sessions/5d8dddaa658b4d37ab518ff605e9755e
Requested by: @thiagoralves
Description of the changes proposed
MainProcessBridge._makeRuntimeApiRequestpublic to eliminate code duplication and centralize all HTTPS request logic in one place/api/compilation-statusendpoint every second until completion (SUCCESS/FAILED) or timeout (5 minutes)lastLogCountand using array slicingwhile(true)towhile(shouldContinuePolling)for better code clarity and maintainabilityKey implementation details
DOD checklist
Critical areas for review
logs.slice(lastLogCount)- verify this handles all edge casesmainProcessBridgeparameter creates tight coupling - consider extracting to interface