Improve workflows logs and add events#261
Draft
LakshanSS wants to merge 6 commits intoopenchoreo:mainfrom
Draft
Conversation
LakshanSS
requested review from
jhivandb,
kaviththiranga and
sameerajayasoma
as code owners
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enhances the openchoreo-workflows plugin to provide per-step visibility into workflow runs by adding backend routes to fetch run status, step-level logs, and Kubernetes events, alongside new frontend components and hooks to render this data in accordion-based tabs with real-time polling for active steps. Changes
Sequence DiagramsequenceDiagram
actor User
participant LogsComponent as WorkflowRunLogs<br/>Component
participant API as GenericWorkflowsClient<br/>API
participant Backend as GenericWorkflowService<br/>Backend
participant External as External APIs<br/>(Status, Observability)
User->>LogsComponent: Mount component with runName
LogsComponent->>LogsComponent: Call useWorkflowRunStatus hook
LogsComponent->>API: getWorkflowRunStatus(namespaceName, runName)
API->>Backend: getWorkflowRunStatus()
Backend->>External: Fetch run status (or derive from run data)
External-->>Backend: Return status with steps array
Backend-->>API: Return WorkflowRunStatusResponse
API-->>LogsComponent: Return status, steps, hasLiveObservability
LogsComponent->>LogsComponent: Render steps as accordions
User->>LogsComponent: Expand a step accordion
LogsComponent->>API: getWorkflowRunStepLogs(namespaceName, runName, step)
API->>Backend: getWorkflowRunLogs(step, sinceSeconds)
Backend->>External: Query observability backend for logs
External-->>Backend: Return per-step logs
Backend-->>API: Return LogEntry[]
API-->>LogsComponent: Update step's logs state
LogsComponent->>LogsComponent: Detect running step
Note over LogsComponent: Poll every 10 seconds
loop While step is running
LogsComponent->>API: getWorkflowRunStepLogs(step, sinceSeconds)
API->>Backend: Fetch latest logs (since last fetch)
Backend->>External: Query for new log entries
External-->>Backend: Return new entries
Backend-->>API: Return LogEntry[] (incremental)
API-->>LogsComponent: Update logs with new entries
end
LogsComponent->>LogsComponent: Terminal status reached
LogsComponent->>LogsComponent: Stop polling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/openchoreo-workflows-backend/src/router.ts (1)
128-147:⚠️ Potential issue | 🟡 MinorValidate
sinceSecondsto avoid NaN/negative inputs reaching the service.
Right now invalid values quietly becomeNaNand flow downstream. Consider rejecting non‑finite or negative values up front.🔧 Suggested validation
- if (step) { + if (step) { + const parsedSinceSeconds = + sinceSeconds !== undefined ? Number(sinceSeconds) : undefined; + if ( + parsedSinceSeconds !== undefined && + (!Number.isFinite(parsedSinceSeconds) || parsedSinceSeconds < 0) + ) { + throw new InputError('sinceSeconds must be a non-negative number'); + } const logs = await workflowService.getWorkflowRunStepLogs( namespaceName as string, runName, step as string, - sinceSeconds ? parseInt(sinceSeconds as string, 10) : undefined, + parsedSinceSeconds, (environmentName as string) || 'development', userToken, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` around lines 128 - 147, Validate the sinceSeconds query before calling workflowService: parse it into a number (e.g., Number(...) or parseInt) and reject requests where it is NaN, non-finite, non-integer, or negative by throwing an InputError/returning 400; apply this check in the router.get handler before the calls to getWorkflowRunStepLogs and getWorkflowRunLogs so invalid values never reach those service methods (refer to the router.get('/workflow-runs/:runName/logs') handler and the getWorkflowRunStepLogs / getWorkflowRunLogs calls to locate the insert point).
🧹 Nitpick comments (2)
plugins/openchoreo-workflows/src/components/WorkflowRunEvents/styles.ts (1)
3-73: Optional: extract shared step/accordion styles with WorkflowRunLogs to avoid drift.
Both files define nearly identical step container/accordion/header styles; a shared hook (or base style object) would keep them consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/styles.ts` around lines 3 - 73, Extract the duplicated step/accordion/header style definitions from useStyles (keys: stepsContainer, stepAccordion, stepHeader, stepTitle, stepStatusChip) into a shared style helper (e.g., create a new function or object like getStepStyles or useStepStylesBase) and import it into both WorkflowRunEvents and WorkflowRunLogs; update the component-local useStyles to merge or spread the shared styles into its makeStyles result and keep only file-specific overrides (e.g., eventsContainer, messageCell) so both components reuse the same canonical step styles and avoid drift.plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts (1)
617-727: Consider extracting observer URL + client setup into a helper.
That block is now duplicated in logs and events; a private helper would reduce drift and simplify future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts` around lines 617 - 727, Extract the repeated observer-URL fetch and observability-client creation into a private helper (e.g., private async getObservabilityClientForEnvironment(namespaceName: string, environmentName: string, token?: string, runName?: string): Promise<{ obsClient: ObservabilityClient, observerUrl: string }>) that encapsulates the logic using createOpenChoreoApiClient, the GET to '/namespaces/{namespaceName}/environments/{envName}/observer-url', the same error handling that throws ObservabilityNotConfiguredError or bubbles other errors, and then returns createObservabilityClientWithUrl(observerUrl, token, this.logger) (plus observerUrl if needed); replace the inlined observer URL + client setup in getWorkflowRunEvents and the duplicate logs method to call this new helper and use its returned client; keep identical error types (ObservabilityNotConfiguredError, HttpNotImplementedError) and logging behavior so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx`:
- Around line 78-90: The auto-selection logic for activeStepName in
WorkflowRunEvents keeps overriding user selection on each poll because
activeStepName is captured in the effect closure; change it to auto-select only
once by adding a persistent flag (e.g., hasAutoSelectedRef) that is initialized
false and checked inside the polling/update effect: only when
hasAutoSelectedRef.current is false and there's no activeStepName and data.steps
exist should you compute defaultStep (using the same runningStep/defaultStep
logic) call setActiveStepName(defaultStep.name) and then set
hasAutoSelectedRef.current = true; reference the existing symbols
activeStepName, data.steps, setActiveStepName and mirror the one-time selection
pattern used in WorkflowRunLogs.
---
Outside diff comments:
In `@plugins/openchoreo-workflows-backend/src/router.ts`:
- Around line 128-147: Validate the sinceSeconds query before calling
workflowService: parse it into a number (e.g., Number(...) or parseInt) and
reject requests where it is NaN, non-finite, non-integer, or negative by
throwing an InputError/returning 400; apply this check in the router.get handler
before the calls to getWorkflowRunStepLogs and getWorkflowRunLogs so invalid
values never reach those service methods (refer to the
router.get('/workflow-runs/:runName/logs') handler and the
getWorkflowRunStepLogs / getWorkflowRunLogs calls to locate the insert point).
---
Nitpick comments:
In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts`:
- Around line 617-727: Extract the repeated observer-URL fetch and
observability-client creation into a private helper (e.g., private async
getObservabilityClientForEnvironment(namespaceName: string, environmentName:
string, token?: string, runName?: string): Promise<{ obsClient:
ObservabilityClient, observerUrl: string }>) that encapsulates the logic using
createOpenChoreoApiClient, the GET to
'/namespaces/{namespaceName}/environments/{envName}/observer-url', the same
error handling that throws ObservabilityNotConfiguredError or bubbles other
errors, and then returns createObservabilityClientWithUrl(observerUrl, token,
this.logger) (plus observerUrl if needed); replace the inlined observer URL +
client setup in getWorkflowRunEvents and the duplicate logs method to call this
new helper and use its returned client; keep identical error types
(ObservabilityNotConfiguredError, HttpNotImplementedError) and logging behavior
so behavior remains unchanged.
In `@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/styles.ts`:
- Around line 3-73: Extract the duplicated step/accordion/header style
definitions from useStyles (keys: stepsContainer, stepAccordion, stepHeader,
stepTitle, stepStatusChip) into a shared style helper (e.g., create a new
function or object like getStepStyles or useStepStylesBase) and import it into
both WorkflowRunEvents and WorkflowRunLogs; update the component-local useStyles
to merge or spread the shared styles into its makeStyles result and keep only
file-specific overrides (e.g., eventsContainer, messageCell) so both components
reuse the same canonical step styles and avoid drift.
plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/openchoreo-workflows-backend/src/router.ts (1)
128-181:⚠️ Potential issue | 🟠 MajorValidate
sinceSecondsto avoid passing NaN to the service.
parseIntcan yieldNaNfor invalid input (e.g.,sinceSeconds=abc), which can cause downstream errors or confusing results. Reject invalid/non‑numeric values early withInputErrorand optionally enforce non‑negative values.✅ Suggested fix (validation)
- const logs = await workflowService.getWorkflowRunStepLogs( + const parsedSinceSeconds = + sinceSeconds !== undefined + ? parseInt(sinceSeconds as string, 10) + : undefined; + if ( + parsedSinceSeconds !== undefined && + (!Number.isFinite(parsedSinceSeconds) || parsedSinceSeconds < 0) + ) { + throw new InputError('sinceSeconds must be a non-negative number'); + } + + const logs = await workflowService.getWorkflowRunStepLogs( namespaceName as string, runName, step as string, - sinceSeconds ? parseInt(sinceSeconds as string, 10) : undefined, + parsedSinceSeconds, (environmentName as string) || 'development', userToken, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` around lines 128 - 181, The route handler registered with router.get('/workflow-runs/:runName/logs') must validate the query param sinceSeconds before calling workflowService.getWorkflowRunStepLogs: parse the raw sinceSeconds string, check it's numeric and a non-negative integer (e.g., use const parsed = Number(sinceSeconds); if (!Number.isFinite(parsed) || Number.isNaN(parsed) || parsed < 0) throw new InputError('sinceSeconds must be a non-negative integer');), then pass parsed (or undefined when absent) into getWorkflowRunStepLogs; ensure this validation happens before any try/catch logic that calls getWorkflowRunStepLogs so you never pass NaN downstream.
🧹 Nitpick comments (2)
plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx (2)
53-56: Consider movingterminalStatusesoutside the component.This array is static and never changes, so wrapping it in
useMemowith an empty dependency array is unnecessary overhead. Moving it to module scope eliminates the hook call on every render.🔧 Suggested refactor
+const TERMINAL_STATUSES = ['completed', 'failed', 'succeeded', 'error']; + export const WorkflowRunLogs = ({ runName }: WorkflowRunLogsProps) => { const classes = useStyles(); // ... - const terminalStatuses = useMemo( - () => ['completed', 'failed', 'succeeded', 'error'], - [], - ); - - const isTerminalStatus = (status?: string) => - status ? terminalStatuses.includes(status.toLowerCase()) : false; + const isTerminalStatus = (status?: string) => + status ? TERMINAL_STATUSES.includes(status.toLowerCase()) : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx` around lines 53 - 56, The terminalStatuses array is static and currently wrapped in useMemo inside the WorkflowRunLogs component; move the declaration of terminalStatuses (the array ['completed','failed','succeeded','error']) to module scope (above the WorkflowRunLogs component) and remove the useMemo usage so the component references the top-level terminalStatuses constant directly to avoid unnecessary hook overhead.
309-315: Consider using a stable key for log entries.Using array index as
keyworks for append-only lists but can cause subtle rendering issues if the list is reordered or items are removed (e.g., through deduplication across fetches). SinceLogEntryhastimestampandlogfields, you could derive a stable key.🔧 Suggested improvement
- {activeLogs.map((logEntry, index) => ( - <Box key={index} style={{ marginBottom: '4px' }}> + {activeLogs.map((logEntry, index) => ( + <Box key={`${logEntry.timestamp ?? index}-${index}`} style={{ marginBottom: '4px' }}>Or use the same composite key logic from
deduplicateLogs:<Box key={`${logEntry.timestamp ?? ''}-${logEntry.log.slice(0, 50)}`} ...>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx` around lines 309 - 315, Replace the unstable array index key in WorkflowRunLogs' rendering of activeLogs (inside the map over activeLogs in the WorkflowRunLogs component) with a stable composite key derived from the LogEntry fields (e.g., use logEntry.timestamp and a prefix of logEntry.log similar to the deduplicateLogs logic); ensure you handle missing timestamp safely (fallback to empty string) and limit the log slice length to avoid huge keys so React can reliably track items across reorders/deduplications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx`:
- Around line 141-221: The effect that polls events (useEffect) currently
computes isRunningStep from statusState.steps but only lists statusState?.status
in its dependency array, so it won't re-run when a step's phase changes; update
the dependency array to include statusState?.steps (or statusState) so that
changes to step phases trigger recomputation of isRunningStep, causing
fetchEvents/interval setup in that effect (functions: fetchEvents,
isRunningStep, setEventsByStep) to start/stop polling correctly and clear the
interval when unmounted or when isRunningStep flips.
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx`:
- Around line 220-222: The effect in WorkflowRunLogs watches [client,
namespaceName, runName, activeStepName, statusState?.status] but also reads
statusState?.steps and activeStep?.phase (derived from steps), so add
statusState?.steps to the dependency array (or memoize the steps and depend on
that stable value) so the effect re-evaluates when individual step phases change
and stops polling correctly; update the dependency list near the useEffect
closing bracket that currently references client, namespaceName, runName,
activeStepName, statusState?.status.
---
Outside diff comments:
In `@plugins/openchoreo-workflows-backend/src/router.ts`:
- Around line 128-181: The route handler registered with
router.get('/workflow-runs/:runName/logs') must validate the query param
sinceSeconds before calling workflowService.getWorkflowRunStepLogs: parse the
raw sinceSeconds string, check it's numeric and a non-negative integer (e.g.,
use const parsed = Number(sinceSeconds); if (!Number.isFinite(parsed) ||
Number.isNaN(parsed) || parsed < 0) throw new InputError('sinceSeconds must be a
non-negative integer');), then pass parsed (or undefined when absent) into
getWorkflowRunStepLogs; ensure this validation happens before any try/catch
logic that calls getWorkflowRunStepLogs so you never pass NaN downstream.
---
Nitpick comments:
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx`:
- Around line 53-56: The terminalStatuses array is static and currently wrapped
in useMemo inside the WorkflowRunLogs component; move the declaration of
terminalStatuses (the array ['completed','failed','succeeded','error']) to
module scope (above the WorkflowRunLogs component) and remove the useMemo usage
so the component references the top-level terminalStatuses constant directly to
avoid unnecessary hook overhead.
- Around line 309-315: Replace the unstable array index key in WorkflowRunLogs'
rendering of activeLogs (inside the map over activeLogs in the WorkflowRunLogs
component) with a stable composite key derived from the LogEntry fields (e.g.,
use logEntry.timestamp and a prefix of logEntry.log similar to the
deduplicateLogs logic); ensure you handle missing timestamp safely (fallback to
empty string) and limit the log slice length to avoid huge keys so React can
reliably track items across reorders/deduplications.
plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx
Outdated
Show resolved
Hide resolved
plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx
Outdated
Show resolved
Hide resolved
LakshanSS
force-pushed
the
edit-catlog-laki
branch
from
53974bf to
620b294
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
plugins/openchoreo-workflows-backend/src/router.ts (2)
140-140: RedundantNumber.isNaNcheck.
Number.isFinite()already returnsfalseforNaNvalues, making theNumber.isNaN(parsed)check redundant.♻️ Suggested simplification
- if (!Number.isFinite(parsed) || Number.isNaN(parsed) || parsed < 0) { + if (!Number.isFinite(parsed) || parsed < 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` at line 140, The condition in the router validation uses a redundant Number.isNaN(parsed) because Number.isFinite(parsed) already excludes NaN; update the if condition (the check that references parsed in router.ts) to remove Number.isNaN(parsed) and keep the remaining checks (Number.isFinite(parsed) and parsed < 0) so the validation still rejects non-finite and negative values.
173-173: Use the extractedstepvariable for consistency.Line 150 uses the extracted
stepvariable, but line 173 accessesreq.query.stepdirectly. For consistency and readability, use the already-extracted variable.♻️ Suggested fix
- if (req.query.step) { + if (step) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` at line 173, The code inconsistently references req.query.step directly instead of using the previously extracted step variable; locate the request handler where step is extracted (variable name "step") and replace the direct usage of req.query.step with the extracted step variable to keep access consistent and readable (e.g., in the conditional that currently reads if (req.query.step) use if (step) and similarly use step elsewhere instead of req.query.step).plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx (2)
50-56: Consider using a module-level constant instead ofuseMemofor static array.
terminalStatusesis a static array that never changes. UsinguseMemowith empty dependencies is unnecessary overhead.WorkflowRunLogs.tsxalready uses a module-level constantTERMINAL_STATUSESfor this purpose—aligning both components would improve consistency and clarity.♻️ Suggested refactor
+const TERMINAL_STATUSES = ['completed', 'failed', 'succeeded', 'error']; + export const WorkflowRunEvents = ({ runName }: WorkflowRunEventsProps) => { const classes = useStyles(); // ... other state ... - const terminalStatuses = useMemo( - () => ['completed', 'failed', 'succeeded', 'error'], - [], - ); - - const isTerminalStatus = (status?: string) => - status ? terminalStatuses.includes(status.toLowerCase()) : false; + const isTerminalStatus = (status?: string) => + status ? TERMINAL_STATUSES.includes(status.toLowerCase()) : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx` around lines 50 - 56, Replace the unnecessary useMemo-created terminalStatuses with a module-level constant to match WorkflowRunLogs.tsx: remove useMemo and declare a top-level TERMINAL_STATUSES array, then update isTerminalStatus to use TERMINAL_STATUSES (and keep its lowercase check). This removes the react hook overhead and aligns naming with the existing WorkflowRunLogs.tsx constant.
305-369: Minor: Conditional render insideAccordionDetailsis redundant.The check
activeStepName === step.nameon line 306 is redundant since theAccordionis already controlled by theexpandedprop (line 287). When collapsed,AccordionDetailscontent is typically not rendered or is hidden. However, this is harmless and may serve as a defensive guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx` around lines 305 - 369, Summary: Remove the redundant conditional rendering inside AccordionDetails that checks activeStepName === step.name. Fix: In the WorkflowRunEvents component, replace the ternary that wraps AccordionDetails (the check using activeStepName === step.name) by rendering the detailed events UI unconditionally inside AccordionDetails and remove the else branch that shows "Expand this step to view events." Keep the existing inner logic for loading, no-events, and events table (activeEvents, eventsLoading, formatTimestamp, classes) intact so behavior is unchanged when Accordion is collapsed/expanded via the Accordion's expanded prop.plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx (1)
60-138: Consider extracting shared status-fetching logic into a custom hook.Both
WorkflowRunLogsandWorkflowRunEventscontain nearly identical status-fetching logic with polling, auto-step selection, and error handling. Extracting this into a shared hook (e.g.,useWorkflowRunStatus) would reduce duplication and ensure consistent behavior across components.This is a recommended refactor that could be deferred to a follow-up PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx` around lines 60 - 138, The status-fetching useEffect in WorkflowRunLogs duplicates logic found in WorkflowRunEvents; extract this into a reusable hook (e.g., useWorkflowRunStatus) that encapsulates polling, error handling, auto-step selection and cleanup. Move the fetchStatus logic (calls to client.getWorkflowRunStatus, setStatusState, isTerminalStatus checks, auto-selection using hasAutoSelectedStepRef/setActiveStepName, observability error detection, and interval management) into the hook API that returns {statusState, statusLoading, statusError, isObservabilityNotConfigured, setActiveStepName} (or callbacks as needed), then replace the current useEffect with a call to useWorkflowRunStatus in both WorkflowRunLogs and WorkflowRunEvents to remove duplication and keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/openchoreo-workflows-backend/src/router.ts`:
- Line 140: The condition in the router validation uses a redundant
Number.isNaN(parsed) because Number.isFinite(parsed) already excludes NaN;
update the if condition (the check that references parsed in router.ts) to
remove Number.isNaN(parsed) and keep the remaining checks
(Number.isFinite(parsed) and parsed < 0) so the validation still rejects
non-finite and negative values.
- Line 173: The code inconsistently references req.query.step directly instead
of using the previously extracted step variable; locate the request handler
where step is extracted (variable name "step") and replace the direct usage of
req.query.step with the extracted step variable to keep access consistent and
readable (e.g., in the conditional that currently reads if (req.query.step) use
if (step) and similarly use step elsewhere instead of req.query.step).
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx`:
- Around line 50-56: Replace the unnecessary useMemo-created terminalStatuses
with a module-level constant to match WorkflowRunLogs.tsx: remove useMemo and
declare a top-level TERMINAL_STATUSES array, then update isTerminalStatus to use
TERMINAL_STATUSES (and keep its lowercase check). This removes the react hook
overhead and aligns naming with the existing WorkflowRunLogs.tsx constant.
- Around line 305-369: Summary: Remove the redundant conditional rendering
inside AccordionDetails that checks activeStepName === step.name. Fix: In the
WorkflowRunEvents component, replace the ternary that wraps AccordionDetails
(the check using activeStepName === step.name) by rendering the detailed events
UI unconditionally inside AccordionDetails and remove the else branch that shows
"Expand this step to view events." Keep the existing inner logic for loading,
no-events, and events table (activeEvents, eventsLoading, formatTimestamp,
classes) intact so behavior is unchanged when Accordion is collapsed/expanded
via the Accordion's expanded prop.
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx`:
- Around line 60-138: The status-fetching useEffect in WorkflowRunLogs
duplicates logic found in WorkflowRunEvents; extract this into a reusable hook
(e.g., useWorkflowRunStatus) that encapsulates polling, error handling,
auto-step selection and cleanup. Move the fetchStatus logic (calls to
client.getWorkflowRunStatus, setStatusState, isTerminalStatus checks,
auto-selection using hasAutoSelectedStepRef/setActiveStepName, observability
error detection, and interval management) into the hook API that returns
{statusState, statusLoading, statusError, isObservabilityNotConfigured,
setActiveStepName} (or callbacks as needed), then replace the current useEffect
with a call to useWorkflowRunStatus in both WorkflowRunLogs and
WorkflowRunEvents to remove duplication and keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/openchoreo-workflows-backend/src/router.ts (1)
136-168:⚠️ Potential issue | 🟠 MajorsinceSeconds is ignored for non‑step logs.
parsedSinceSecondsis computed but never forwarded togetWorkflowRunLogsin the non‑step path, so the filter silently does nothing.🔧 Suggested fix
const logs = await workflowService.getWorkflowRunLogs( namespaceName as string, runName, (environmentName as string) || 'development', - userToken, + userToken, + undefined, + parsedSinceSeconds, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` around lines 136 - 168, The parsedSinceSeconds value is computed but not passed to the non-step log path; update the call to getWorkflowRunLogs to accept and forward parsedSinceSeconds (similar to the getWorkflowRunStepLogs call) so the sinceSeconds filter is applied, ensuring you pass parsedSinceSeconds (which may be undefined) along with namespaceName, runName, environmentName/default and userToken in the non-step branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts`:
- Around line 509-510: The current object spread uses truthy checks that drop
values like 0 (e.g., ...(step ? { step } : {}), ...(sinceSeconds ? {
sinceSeconds } : {})) so callers cannot pass 0; update these conditions to
explicit undefined checks (e.g., use step !== undefined and sinceSeconds !==
undefined) inside the GenericWorkflowService method where step and sinceSeconds
are spread so 0 is preserved while still omitting undefined.
- Around line 355-392: The current logic silently falls back to deriving status
for any non-OK response from the upstream status endpoint; change the client.GET
handling so the fallback to this.getWorkflowRun(namespaceName, runName, token)
only happens when the upstream endpoint is truly missing (HTTP 404 or 501 Not
Implemented). Specifically, inside the block around the client.GET call (the GET
to '/namespaces/{namespaceName}/workflow-runs/{runName}/status'), detect
response.status (or err.response?.status if the GET throws) and: if status is
404 or 501 proceed to the existing fallback, otherwise propagate the error
(rethrow or return a rejected promise) and log via this.logger.error with the
response/error details; keep the existing successful-path that returns
statusData unchanged.
- Around line 633-651: The endpoint currently returns raw data without
validating the API envelope; update the method in GenericWorkflowService (the
block calling client.GET for
'/namespaces/{namespaceName}/workflow-runs/{runName}/events') to check the API
envelope after response.ok—e.g. assert data?.success is true—and then return the
inner payload (cast to WorkflowRunEventEntry[]) such as data.data or data.events
as used elsewhere; if success is false or the payload is missing, throw an Error
including response.status/response.statusText and any data.error to avoid
surfacing error objects as event lists.
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx`:
- Around line 54-137: Old logs and error flags persist across run/namespace
changes causing stale UI; add a small effect that watches runName and
namespaceName (and optionally activeStepName) and resets state by calling
setLogsByStep({}), setLogsError(null), setIsLogsObsNotConfigured(false) (and
setLogsLoading(false) if desired) so logsByStep, logsError and observability
flag are cleared whenever the run or namespace changes; reference the existing
state setters setLogsByStep, setLogsError, setIsLogsObsNotConfigured,
setLogsLoading to implement this reset.
In `@plugins/openchoreo-workflows/src/hooks/useWorkflowRunStatus.ts`:
- Around line 30-77: Add a small reset effect that runs when namespaceName or
runName change to clear per-run state: set hasAutoSelectedStepRef.current =
false, call setActiveStepName(null), and setIsObservabilityNotConfigured(false)
so previous runs don't interfere with auto-selection or show stale observability
flags; place this in a useEffect watching [namespaceName, runName] alongside the
existing fetch effect (referencing hasAutoSelectedStepRef, setActiveStepName,
setIsObservabilityNotConfigured).
---
Outside diff comments:
In `@plugins/openchoreo-workflows-backend/src/router.ts`:
- Around line 136-168: The parsedSinceSeconds value is computed but not passed
to the non-step log path; update the call to getWorkflowRunLogs to accept and
forward parsedSinceSeconds (similar to the getWorkflowRunStepLogs call) so the
sinceSeconds filter is applied, ensuring you pass parsedSinceSeconds (which may
be undefined) along with namespaceName, runName, environmentName/default and
userToken in the non-step branch.
plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts
Outdated
Show resolved
Hide resolved
plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts
Outdated
Show resolved
Hide resolved
plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts
Outdated
Show resolved
Hide resolved
plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx
Show resolved
Hide resolved
LakshanSS
force-pushed
the
edit-catlog-laki
branch
from
4791d47 to
286421b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
plugins/openchoreo-workflows-backend/src/router.ts (2)
174-190: Inconsistent HTTP status codes for ObservabilityNotConfiguredError.When
stepis provided, the handler returns HTTP 503 with a JSON error object. Whenstepis absent, it returns HTTP 200 with an error embedded in the response body. This inconsistency could confuse frontend consumers and complicate error handling.Consider unifying the behavior—either always return 503 for this error type, or always embed the error in a 200 response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` around lines 174 - 190, The handler in router.ts returns inconsistent status codes for ObservabilityNotConfiguredError: when the local variable step is truthy it sends res.status(503).json(...), but when step is falsy it sends res.json({...}) with HTTP 200; unify behavior by always returning the same HTTP status (e.g., always use res.status(503).json(...)) and the same error payload shape so frontend consumers can rely on a single error contract—update both the branch that calls res.status(503).json(...) and the branch that currently calls res.json(...) to use the chosen status code and identical error fields (refer to the existing res.status(503).json and res.json blocks and the step variable to locate where to change).
136-143: Minor: Validation message is slightly misleading.The error message states "sinceSeconds must be a non-negative integer" but the code accepts any non-negative number and floors it. This is functionally fine but could confuse API consumers. Consider either:
- Accepting floats explicitly in the message, or
- Using
Number.isInteger(parsed)for stricter validation.♻️ Option A: Update message to reflect actual behavior
if (!Number.isFinite(parsed) || parsed < 0) { - throw new InputError('sinceSeconds must be a non-negative integer'); + throw new InputError('sinceSeconds must be a non-negative number'); }♻️ Option B: Enforce integer strictly
- if (!Number.isFinite(parsed) || parsed < 0) { + if (!Number.isInteger(parsed) || parsed < 0) { throw new InputError('sinceSeconds must be a non-negative integer'); } - parsedSinceSeconds = Math.floor(parsed); + parsedSinceSeconds = parsed;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/router.ts` around lines 136 - 143, The validation message for sinceSeconds is misleading because the code accepts any non-negative number and floors it; update the InputError message in the block that handles sinceSeconds (symbols: sinceSeconds, parsedSinceSeconds, parsed, Number.isFinite, Math.floor, InputError) to reflect actual behavior (e.g., "sinceSeconds must be a non-negative number" or "must be a non-negative number; fractional values will be floored to an integer") OR, if you want strict integer validation, replace the Number.isFinite check with Number.isInteger(parsed) and keep the original message; pick one approach and apply it consistently in that conditional.plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts (1)
708-716:HttpNotImplementedErroris exported but never used in the codebase.The error class is included in the public API (
services/index.ts) but is not thrown or imported anywhere. The 501 status handling in the fallback logic (lines 375-387) would be a natural fit for this error, but it throws a genericErrorinstead. Either remove this unused export or integrate it into the appropriate error handling path if intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts` around lines 708 - 716, HttpNotImplementedError is exported but unused; either remove the class export from GenericWorkflowService.ts or replace the generic Error thrown in the 501 fallback with this typed error. Locate the fallback logic in GenericWorkflowService (where a 501 response currently leads to throwing new Error(...)) and change it to throw new HttpNotImplementedError('<meaningful message>'), and ensure services/index.ts exports still references HttpNotImplementedError (or remove that export if you delete the class). Update any imports/usages accordingly so the codebase no longer exports an unused symbol.plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx (1)
162-239: Scope events errors to the active step instead of short‑circuiting the entire view.
Right now any non‑observability events error hides all steps, which conflicts with the per‑step visibility goal. Keep the global info/error alert only for observability/status errors, and render events errors inline for the active accordion.♻️ Suggested refactor
- const isObservabilityNotConfigured = - isStatusObsNotConfigured || isEventsObsNotConfigured; - const combinedError = statusError || eventsError; + const isObservabilityNotConfigured = + isStatusObsNotConfigured || isEventsObsNotConfigured; + const combinedError = statusError; if (combinedError) { if (isObservabilityNotConfigured) { return ( <Alert severity="info"> <Typography variant="body1">{combinedError}</Typography> </Alert> ); } return ( <Alert severity="error"> <Typography variant="body1">{combinedError}</Typography> </Alert> ); }<AccordionDetails> <Box className={classes.eventsContainer}> + {eventsError && activeStepName === step.name && ( + <Alert severity={isEventsObsNotConfigured ? 'info' : 'error'}> + <Typography variant="body2">{eventsError}</Typography> + </Alert> + )} {eventsLoading && activeEvents.length === 0 && ( <Box className={classes.inlineLoadingContainer}> <CircularProgress size={18} /> <Typography variant="body2" color="textSecondary"> Loading events... </Typography> </Box> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx` around lines 162 - 239, The top-level combinedError currently short-circuits the entire view (combinedError = statusError || eventsError) causing any eventsError to hide all steps; change the top-level check to only consider status/observability errors (use statusError and isObservabilityNotConfigured) so the global Alert shows only for status/observability issues, and remove eventsError from that combinedError logic. Then, inside the AccordionDetails where events are rendered (around activeEvents, eventsLoading), display eventsError inline for the active accordion (render an Alert severity="error" when eventsError is present and the accordion is the active step) so events failures are scoped per-step; keep existing variables like isObservabilityNotConfigured, statusError, eventsError, activeStepName, activeEvents, and eventsLoading to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts`:
- Around line 650-663: The endpoint path used in the client.GET call in
GenericWorkflowService (currently
'/namespaces/{namespaceName}/workflow-runs/{runName}/events') doesn't match the
OpenAPI spec; change the request path to
'/namespaces/{namespaceName}/projects/{projectName}/components/{componentName}/workflow-runs/{runName}/events'
and add projectName and componentName to the params.path object alongside
namespaceName and runName, keeping the existing conditional inclusion of step in
params.query; also search for callers of this method to ensure
projectName/componentName are available or threaded through the method signature
if needed and verify against the upstream API if the shorter path actually
exists before making the change.
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx`:
- Around line 192-198: formatTimestamp currently relies on try/catch but new
Date(...) doesn’t throw for invalid input; instead, construct the Date (const d
= new Date(timestamp)) and check its validity (e.g. Number.isNaN(d.getTime()) or
isNaN(d.getTime())) before calling toLocaleString; if invalid return the
original timestamp, otherwise return d.toLocaleString(); update the function
named formatTimestamp in WorkflowRunEvents.tsx to use this validity check rather
than relying on exceptions.
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx`:
- Around line 27-35: The deduplicateLogs function currently builds a key using
entry.timestamp which is optional, causing different log lines with no timestamp
to be collapsed; update deduplicateLogs so it only de-duplicates entries that
have a timestamp (e.g., if entry.timestamp === undefined or null, always include
the entry), or alternatively use a stable unique field (like entry.id or another
identifier) when available; modify the filter logic in deduplicateLogs to check
entry.timestamp before constructing the key and falling back to bypassing
deduplication for timestamp-less entries.
- Around line 81-126: On any successful fetch inside fetchLogs (both full
fetches and incremental polls where you setLogsByStep), clear transient error
state by calling setLogsError(null) and reset observability flag via
setIsLogsObsNotConfigured(false); place these calls after updating setLogsByStep
(and after the cancelled check) so subsequent successful polls remove prior
errors and the UI unblocks.
---
Nitpick comments:
In `@plugins/openchoreo-workflows-backend/src/router.ts`:
- Around line 174-190: The handler in router.ts returns inconsistent status
codes for ObservabilityNotConfiguredError: when the local variable step is
truthy it sends res.status(503).json(...), but when step is falsy it sends
res.json({...}) with HTTP 200; unify behavior by always returning the same HTTP
status (e.g., always use res.status(503).json(...)) and the same error payload
shape so frontend consumers can rely on a single error contract—update both the
branch that calls res.status(503).json(...) and the branch that currently calls
res.json(...) to use the chosen status code and identical error fields (refer to
the existing res.status(503).json and res.json blocks and the step variable to
locate where to change).
- Around line 136-143: The validation message for sinceSeconds is misleading
because the code accepts any non-negative number and floors it; update the
InputError message in the block that handles sinceSeconds (symbols:
sinceSeconds, parsedSinceSeconds, parsed, Number.isFinite, Math.floor,
InputError) to reflect actual behavior (e.g., "sinceSeconds must be a
non-negative number" or "must be a non-negative number; fractional values will
be floored to an integer") OR, if you want strict integer validation, replace
the Number.isFinite check with Number.isInteger(parsed) and keep the original
message; pick one approach and apply it consistently in that conditional.
In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts`:
- Around line 708-716: HttpNotImplementedError is exported but unused; either
remove the class export from GenericWorkflowService.ts or replace the generic
Error thrown in the 501 fallback with this typed error. Locate the fallback
logic in GenericWorkflowService (where a 501 response currently leads to
throwing new Error(...)) and change it to throw new
HttpNotImplementedError('<meaningful message>'), and ensure services/index.ts
exports still references HttpNotImplementedError (or remove that export if you
delete the class). Update any imports/usages accordingly so the codebase no
longer exports an unused symbol.
In
`@plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx`:
- Around line 162-239: The top-level combinedError currently short-circuits the
entire view (combinedError = statusError || eventsError) causing any eventsError
to hide all steps; change the top-level check to only consider
status/observability errors (use statusError and isObservabilityNotConfigured)
so the global Alert shows only for status/observability issues, and remove
eventsError from that combinedError logic. Then, inside the AccordionDetails
where events are rendered (around activeEvents, eventsLoading), display
eventsError inline for the active accordion (render an Alert severity="error"
when eventsError is present and the accordion is the active step) so events
failures are scoped per-step; keep existing variables like
isObservabilityNotConfigured, statusError, eventsError, activeStepName,
activeEvents, and eventsLoading to implement this change.
plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts
Show resolved
Hide resolved
plugins/openchoreo-workflows/src/components/WorkflowRunEvents/WorkflowRunEvents.tsx
Show resolved
Hide resolved
plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx
Show resolved
Hide resolved
plugins/openchoreo-workflows/src/components/WorkflowRunLogs/WorkflowRunLogs.tsx
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/openchoreo-workflows-backend/src/services/GenericWorkflowService.ts`:
- Around line 650-663: The code calls client.GET with the untyped path string
'/namespaces/{namespaceName}/workflow-runs/{runName}/events' (in
GenericWorkflowService) using an "as any" cast which suggests the OpenAPI client
may not define this endpoint; confirm the upstream API spec actually exposes
this namespace-level events path and if it does, update the OpenAPI-generated
client so client.GET accepts that route (remove the "as any" cast and use the
generated route/method); if the upstream API only exposes the
project/component-scoped path, change the call to the correct generated endpoint
(the one that includes projects/components) and adapt the path params
(namespaceName/runName -> project/component/...) or add a server-side proxy
wrapper that maps namespace calls to the supported endpoint, and update any
typings in GenericWorkflowService accordingly to avoid the unsafe "as any" cast.
2858e0f to
19cd6ef
Compare
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
LakshanSS
force-pushed
the
edit-catlog-laki
branch
from
19cd6ef to
d1d007a
Compare
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Summary by CodeRabbit
Release Notes