feat: implement cron trigger functionality and improve error handling in console#19
feat: implement cron trigger functionality and improve error handling in console#19andersonleal merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR enhances connection error handling by introducing CORS detection utilities and moves connection health monitoring to the root layout. It implements cron trigger manual invocation with backend Rust handlers, frontend UI controls, and improves API error messages across multiple components. Documentation and empty-state messaging are also updated. Changes
Sequence DiagramsequenceDiagram
participant User as User (UI)
participant Frontend as Frontend<br/>(triggers.tsx)
participant API as Bridge API<br/>(functions.rs)
participant Engine as Engine
User->>Frontend: Click "Run Now" for Cron Trigger
Frontend->>Frontend: Validate trigger has id and function_id
alt Validation Fails
Frontend->>User: Show error/warning message
else Validation Passes
Frontend->>API: POST /api/functions/invoke<br/>with cron trigger metadata
API->>API: handle_cron_trigger receives request
alt function_id provided
API->>Engine: Invoke function with cron payload
else function_id missing
API->>Engine: Query triggers by id to resolve function_id
API->>Engine: Invoke resolved function with cron payload
end
API->>Frontend: Return execution result
Frontend->>User: Display success/error result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/console-frontend/src/routes/logs.tsx (1)
438-450: Heading "No logs found" may be misleading for a missing adapter condition.The condition
!hasLoggingAdapterspecifically checks whether a logging adapter is configured, but the heading "No logs found" suggests logs were searched but none exist. This could confuse users who might not realize they need to configure an adapter.Consider a more explicit heading like "Logging Not Configured" or "No Logging Adapter" to match the actual condition, while keeping the helpful instruction text.
💡 Suggested heading clarification
<AlertCircle className="w-5 h-5 text-muted" /> </div> - <h3 className="text-xs font-medium mb-1 text-foreground">No logs found</h3> + <h3 className="text-xs font-medium mb-1 text-foreground">Logging Not Configured</h3> <p className="text-[11px] text-muted leading-relaxed"> Configure a logging adapter in config.yaml. Logs will appear here once available. </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/console-frontend/src/routes/logs.tsx` around lines 438 - 450, The UI shows a misleading heading when the adapter is missing: inside the conditional branch gated by isLoading and hasLoggingAdapter (the block where !isLoading && !hasLoggingAdapter is true) replace the h3 text "No logs found" with a clearer heading such as "Logging Not Configured" (or "No Logging Adapter") so the message matches the actual condition; update the h3 element in logs.tsx accordingly while leaving the explanatory paragraph unchanged.packages/console-rust/src/bridge/functions.rs (2)
612-720: Consider adding tracing for observability.Other handlers in this file include
tracing::debug!statements (e.g., line 437). Adding similar logging tohandle_cron_triggerwould improve debuggability for manual cron invocations.📋 Example tracing addition
async fn handle_cron_trigger(bridge: &III, input: Value) -> Value { let body = input.get("body").unwrap_or(&input); + + tracing::debug!(?input, "Received cron trigger request"); let trigger_id = body .get("trigger_id")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/console-rust/src/bridge/functions.rs` around lines 612 - 720, Add tracing::debug! calls inside handle_cron_trigger to improve observability: log entry with the incoming input (or trigger_id and provided_function_id) near the start of the function, log the resolved trigger and function_id after you look up the trigger (use symbols trigger_id, provided_function_id, trigger, function_id), and log just before invoking bridge.call_with_timeout as well as on error branches that return error_response; keep messages concise and include relevant IDs to correlate logs.
629-636: Consider validating emptyfunction_idconsistently withtrigger_id.The
trigger_idextraction (line 621) includes an empty string check withSome(id) if !id.is_empty(), butprovided_function_iddoes not. If a caller passesfunction_id: "", the code will skip the trigger lookup and attempt to call an empty function name rather than falling back to resolving it from the trigger.♻️ Proposed fix for consistent validation
let provided_function_id = body .get("function_id") .and_then(|v| v.as_str()) .or_else(|| input.get("function_id").and_then(|v| v.as_str())) - .map(|v| v.to_string()); + .filter(|v| !v.is_empty()) + .map(|v| v.to_string());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/console-rust/src/bridge/functions.rs` around lines 629 - 636, The code extracts provided_function_id without rejecting empty strings, so if function_id: "" is passed it will be treated as a valid ID and bypass trigger resolution; update the provided_function_id extraction (the chain that calls body.get("function_id") and input.get("function_id")) to reject empty strings the same way trigger_id does (e.g., only map to Some(id) when id is not empty) so that the subsequent function_id logic (the if let Some(function_id) = provided_function_id { ... }) falls back to resolving from trigger_id when an empty string is supplied.packages/console-frontend/src/api/utils.ts (1)
63-71: Consider preserving the original error for debugging.Re-throwing with
new Error(getConnectionErrorMessage(error))loses the original error's stack trace, which could complicate debugging in development.♻️ Optional: Preserve original error as cause
} catch (error) { - throw new Error(getConnectionErrorMessage(error)) + const message = getConnectionErrorMessage(error) + throw new Error(message, { cause: error }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/console-frontend/src/api/utils.ts` around lines 63 - 71, The catch block in the fetch wrapper discards the original error's stack by throwing a new Error from getConnectionErrorMessage(error); update the catch to preserve the original error (e.g., pass the caught error as the cause when creating the new Error or attach the original error to the new Error) so debugging stack traces remain available; locate the try/catch around fetch(`${getDevtoolsApi()}${devtoolsPath}`, options) in this file and modify the throw to include the original error (reference symbols: getDevtoolsApi, devtoolsPath, unwrapResponse, getConnectionErrorMessage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/console-frontend/src/api/events/invocation.ts`:
- Around line 3-40: In extractApiError, avoid calling res.text() after
res.json() has consumed the body: when content-type includes 'application/json',
always await res.json() once and return a string based on that parsed value
(e.g., wrappedBody.error, data.error, or JSON.stringify(data) or
JSON.stringify(wrappedBody ?? data)) instead of falling through to res.text();
only call res.text() if JSON.parse fails (caught exception) or content-type is
not JSON. Ensure you update the branch that currently falls through (when parsed
value isn't an object) to return a stringified representation of the parsed JSON
so the response body is not consumed twice.
---
Nitpick comments:
In `@packages/console-frontend/src/api/utils.ts`:
- Around line 63-71: The catch block in the fetch wrapper discards the original
error's stack by throwing a new Error from getConnectionErrorMessage(error);
update the catch to preserve the original error (e.g., pass the caught error as
the cause when creating the new Error or attach the original error to the new
Error) so debugging stack traces remain available; locate the try/catch around
fetch(`${getDevtoolsApi()}${devtoolsPath}`, options) in this file and modify the
throw to include the original error (reference symbols: getDevtoolsApi,
devtoolsPath, unwrapResponse, getConnectionErrorMessage).
In `@packages/console-frontend/src/routes/logs.tsx`:
- Around line 438-450: The UI shows a misleading heading when the adapter is
missing: inside the conditional branch gated by isLoading and hasLoggingAdapter
(the block where !isLoading && !hasLoggingAdapter is true) replace the h3 text
"No logs found" with a clearer heading such as "Logging Not Configured" (or "No
Logging Adapter") so the message matches the actual condition; update the h3
element in logs.tsx accordingly while leaving the explanatory paragraph
unchanged.
In `@packages/console-rust/src/bridge/functions.rs`:
- Around line 612-720: Add tracing::debug! calls inside handle_cron_trigger to
improve observability: log entry with the incoming input (or trigger_id and
provided_function_id) near the start of the function, log the resolved trigger
and function_id after you look up the trigger (use symbols trigger_id,
provided_function_id, trigger, function_id), and log just before invoking
bridge.call_with_timeout as well as on error branches that return
error_response; keep messages concise and include relevant IDs to correlate
logs.
- Around line 629-636: The code extracts provided_function_id without rejecting
empty strings, so if function_id: "" is passed it will be treated as a valid ID
and bypass trigger resolution; update the provided_function_id extraction (the
chain that calls body.get("function_id") and input.get("function_id")) to reject
empty strings the same way trigger_id does (e.g., only map to Some(id) when id
is not empty) so that the subsequent function_id logic (the if let
Some(function_id) = provided_function_id { ... }) falls back to resolving from
trigger_id when an empty string is supplied.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/console-rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
README.mdpackages/console-frontend/src/api/config-provider.tsxpackages/console-frontend/src/api/events/invocation.tspackages/console-frontend/src/api/utils.tspackages/console-frontend/src/components/flow/nodes/node-header.tsxpackages/console-frontend/src/routes/__root.tsxpackages/console-frontend/src/routes/index.tsxpackages/console-frontend/src/routes/logs.tsxpackages/console-frontend/src/routes/triggers.tsxpackages/console-rust/src/bridge/functions.rspackages/console-rust/src/bridge/triggers.rs
| async function extractApiError(res: Response, fallback: string): Promise<string> { | ||
| const contentType = res.headers.get('content-type') || '' | ||
|
|
||
| if (contentType.includes('application/json')) { | ||
| try { | ||
| const data = await res.json() | ||
| if (data && typeof data === 'object') { | ||
| const wrappedBody = | ||
| 'status_code' in data && | ||
| 'body' in data && | ||
| data.body && | ||
| typeof data.body === 'object' && | ||
| !Array.isArray(data.body) | ||
| ? (data.body as Record<string, unknown>) | ||
| : null | ||
|
|
||
| if (wrappedBody?.error && typeof wrappedBody.error === 'string') { | ||
| return wrappedBody.error | ||
| } | ||
|
|
||
| if ('error' in data && typeof data.error === 'string') { | ||
| return data.error | ||
| } | ||
|
|
||
| return JSON.stringify(wrappedBody ?? data) | ||
| } | ||
| } catch { | ||
| // Fall back to text below when body is not valid JSON. | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| const text = await res.text() | ||
| return text || fallback | ||
| } catch { | ||
| return fallback | ||
| } | ||
| } |
There was a problem hiding this comment.
Response body may be consumed twice, causing silent failures.
If res.json() succeeds but the JSON doesn't contain an error field, the code falls through to res.text() on Line 35. However, the response body stream has already been consumed by res.json(), so res.text() will return an empty string or throw.
🐛 Proposed fix to prevent double consumption
async function extractApiError(res: Response, fallback: string): Promise<string> {
const contentType = res.headers.get('content-type') || ''
if (contentType.includes('application/json')) {
try {
const data = await res.json()
if (data && typeof data === 'object') {
const wrappedBody =
'status_code' in data &&
'body' in data &&
data.body &&
typeof data.body === 'object' &&
!Array.isArray(data.body)
? (data.body as Record<string, unknown>)
: null
if (wrappedBody?.error && typeof wrappedBody.error === 'string') {
return wrappedBody.error
}
if ('error' in data && typeof data.error === 'string') {
return data.error
}
return JSON.stringify(wrappedBody ?? data)
}
+ // JSON parsed but wasn't an object - stringify it
+ return JSON.stringify(data)
} catch {
// Fall back to text below when body is not valid JSON.
}
}
try {
const text = await res.text()
return text || fallback
} catch {
return fallback
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/console-frontend/src/api/events/invocation.ts` around lines 3 - 40,
In extractApiError, avoid calling res.text() after res.json() has consumed the
body: when content-type includes 'application/json', always await res.json()
once and return a string based on that parsed value (e.g., wrappedBody.error,
data.error, or JSON.stringify(data) or JSON.stringify(wrappedBody ?? data))
instead of falling through to res.text(); only call res.text() if JSON.parse
fails (caught exception) or content-type is not JSON. Ensure you update the
branch that currently falls through (when parsed value isn't an object) to
return a stringified representation of the parsed JSON so the response body is
not consumed twice.
Summary
engine::console::cron_triggerand expose it atPOST /_console/cron/trigger.Type of Change
Checklist
Additional Context
developis not present in this repository, so this PR targetsmain.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI/UX Updates
Documentation