feat(dashboard): internal workflow run page with DAG graph#172
Merged
feat(dashboard): internal workflow run page with DAG graph#172
Conversation
Add /$owner/$repo/actions/runs/$runId route showing a workflow run's summary, jobs graph (parsed from YAML via @xyflow/react), and artifacts. Matrix jobs render as a single node with always-visible variant column. Checks on PR detail now link internally to this page when a matching workflow run exists. - New server fns: getWorkflowRun, listWorkflowRunJobs, listWorkflowRunArtifacts, getWorkflowDefinition - Real-time updates via useGitHubSignalStream on workflowRunEntity signal - Re-run actions gated on viewerCanRerun (push/admin perms) - Shared useNow() ticker for synchronized relative-time labels - Extracted check-state-icon helper; added waiting state (no spinner)
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a full Workflow Run feature: new protected routes and pages for runs and jobs, server endpoints and queries for workflow runs/jobs/artifacts/definitions/logs/reruns, a client ReactFlow graph with job/matrix/step-log nodes and controls, shared check-state and copy UI, layout/parse utilities, hooks, types, and icon exports. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Router as Router
participant Loader as Route Loader
participant Server as App Server
participant Cache as Query Cache
participant GitHub as GitHub API
participant UI as Workflow Run UI
User->>Router: navigate to /actions/runs/:runId
Router->>Loader: invoke route loader
Loader->>Server: prefetch workflow run
Server->>Cache: ensure workflow run query
alt cache miss
Cache->>GitHub: getWorkflowRun
GitHub-->>Cache: run metadata
end
Loader->>Server: prefetch jobs, artifacts, definition (when available)
Server->>Cache: listWorkflowRunJobs / listWorkflowRunArtifacts / getWorkflowDefinition
Cache->>GitHub: fetch jobs, artifacts, YAML
GitHub-->>Cache: jobs, artifacts, YAML
Loader-->>Router: return loader data (runTitle)
Router->>UI: render WorkflowRunPage
UI->>Cache: use workflow run/jobs/definition queries
Cache-->>UI: data
UI->>UI: compute layout (buildLayoutFromDefinition / groupJobs)
UI->>UI: render ReactFlow canvas (JobNode / MatrixNode / StepLogNode)
User->>UI: click "Re-run"
UI->>Server: rerunWorkflowRun / rerunFailedWorkflowJobs
Server->>GitHub: trigger rerun API
GitHub-->>Server: ack
Server->>Cache: invalidate run/jobs/artifacts queries
Cache-->>UI: updated data -> UI re-renders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
apps/dashboard/src/lib/github-cache-policy.ts (1)
35-38: Use a longer-lived cache bucket for workflow definitions.If this bucket also backs
getWorkflowDefinition, a 15s TTL is much shorter than the data’s actual change rate. Run/job status is live, but the parsed YAML for a specific run is effectively immutable, so sharing the same short cache policy will just refetch and reparse static data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/github-cache-policy.ts` around lines 35 - 38, The workflowRun cache bucket uses a very short TTL (staleTimeMs: 15 * 1000) which causes immutable workflow definitions to be refetched; update the workflowRun policy (the object named workflowRun in github-cache-policy.ts) to use a much longer staleTimeMs and gcTimeMs appropriate for mostly-static parsed YAML (for example, set staleTimeMs to hours/days and gcTimeMs to several days) so getWorkflowDefinition and other callers sharing this bucket stop re-parsing unchanged data.apps/dashboard/src/components/workflows/graph/edges.ts (1)
9-28: Consider adjacency maps to reduce repeated full-edge scans.Lines 12–17 and 23–28 repeatedly iterate all edges per node expansion. Precomputing incoming/outgoing adjacency can significantly reduce work on larger runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/graph/edges.ts` around lines 9 - 28, The current traversal scans the full edges array for every popped node (the loops using forward/backward and variables edges, current, result), which is O(E·V); precompute adjacency maps before those loops: build outgoing: Map<nodeId, Edge[]> keyed by edge.source and incoming: Map<nodeId, Edge[]> keyed by edge.target (iterating edges once), then replace the inner for (const edge of edges) scans with iterating outgoing.get(current) for forward expansion and incoming.get(current) for backward expansion, keeping the same checks for result.has(edge.id) and pushing edge.target/edge.source as before.apps/dashboard/src/lib/use-now.ts (1)
14-16: Refreshnowwhen first subscriber attaches.Line 14 starts ticking, but initial snapshot can be stale if this module was imported earlier. Refresh once when the first listener subscribes.
Proposed fix
if (intervalId === null) { + now = Date.now(); intervalId = setInterval(tick, 1000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/use-now.ts` around lines 14 - 16, When starting the interval in the subscribe path, call tick() once immediately so the first subscriber gets a fresh snapshot instead of waiting up to 1s; in the block that checks intervalId (the code that currently does if (intervalId === null) { intervalId = setInterval(tick, 1000); }), invoke tick() before creating the interval (use the existing tick function and intervalId variable) so the state is refreshed right when the first listener attaches.apps/dashboard/src/components/workflows/workflow-run-page.tsx (1)
72-75: Consider adding a comment to clarify intent.The viewer query result is unused. If this is intentional (e.g., for prefetching or ensuring cache hydration), a brief comment would clarify the purpose.
+ // Prefetch viewer data to ensure it's cached for child components useQuery({ ...githubViewerQueryOptions(scope), enabled: hasMounted, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/workflow-run-page.tsx` around lines 72 - 75, The useQuery call with githubViewerQueryOptions(scope) is currently invoked solely for side effects (prefetch/cache hydration) since its result is unused; add an inline comment above the useQuery call clarifying that intent (e.g., "prefetch viewer data / ensure cache hydration") so future readers know this is deliberate and not a forgotten bug—referencing the useQuery invocation, githubViewerQueryOptions(scope) options, and the enabled: hasMounted flag.apps/dashboard/src/routes/_protected/$owner/$repo/actions.runs.$runId.tsx (1)
49-49: Avoid swallowing loader prefetch failures silently.Line 49 drops all errors, which makes auth/rate-limit/fetch failures hard to diagnose. Prefer logging or telemetry here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/routes/_protected/`$owner/$repo/actions.runs.$runId.tsx at line 49, The promise chain currently ends with a silent .catch(() => {}) which hides failures from the loader prefetch; replace that silent handler with an error-reporting handler that captures the error and context (owner, repo, runId) and forwards it to your logging/telemetry system or at minimum console.error so auth/rate-limit/fetch issues are visible; locate the prefetch call in actions.runs.$runId (the code that currently uses .catch(() => {})) and change the empty catch to report the error and include identifying context.apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx (1)
192-220: Consider popup node positioning for multi-column layouts.The popup nodes are positioned at a fixed X offset (
matrix.position.x + NODE_WIDTH + COLUMN_GAP + VARIANT_POPUP_GAP). In the fallback column-based layout, this could cause popup nodes to visually overlap with nodes in subsequent columns when there are multiple jobs per column.If this is intentional (e.g., popups only expand on interaction and overlay other content), this is fine. Otherwise, consider either:
- Positioning popups in a layer that doesn't interfere with the main layout
- Adjusting column spacing to accommodate expanded popups
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx` around lines 192 - 220, Popup nodes for matrix variants are placed at a fixed X (popupX = matrix.position.x + NODE_WIDTH + COLUMN_GAP + VARIANT_POPUP_GAP), which can overlap nodes in following columns in multi-column/fallback layouts; update the placement logic in the block that builds popup nodes (matrixNodes loop / popupX variable) to either compute X based on available column width or place popups on a separate overlay layer: e.g., detect when matrix is in a multi-column layout and increase popupX by the width of subsequent columns (or add a flag so the job popup nodes are rendered into an overlay stack instead of the main canvas), and ensure any height calculations using estimateNodeHeight and collapsedPopupIds/togglePopupCollapsed still work with the new placement strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/shared/copy-badge.tsx`:
- Around line 18-26: The handleClick currently sets copied=true and starts the
timeout before ensuring navigator.clipboard.writeText(value) succeeded and the
timeoutRef is not cleaned up on unmount; update handleClick to await the promise
from navigator.clipboard.writeText(value) inside a try/catch and only call
setCopied(true) and start timeoutRef.current = setTimeout(...) after a
successful write (handle rejections by logging or showing failure state), and
add a useEffect cleanup that clearsTimeout(timeoutRef.current) on unmount to
avoid the timeout firing after the component is removed; reference the
handleClick function, timeoutRef, setCopied, and the useEffect cleanup when
making the change.
In `@apps/dashboard/src/components/workflows/graph/height.ts`:
- Around line 35-39: The height calculation in the function that returns the
matrix height overcounts the trailing card gap by using jobs.length *
(H_MATRIX_PILL + H_MATRIX_CARD_GAP); change it to use (jobs.length - 1) gaps
instead and guard against negative counts (e.g., use Math.max(0, jobs.length -
1) or equivalent) so the return becomes H_MATRIX_OUTER_PAD + main + Math.max(0,
jobs.length - 1) * (H_MATRIX_PILL + H_MATRIX_CARD_GAP), referencing the existing
H_MATRIX_OUTER_PAD, main, jobs, H_MATRIX_PILL and H_MATRIX_CARD_GAP symbols.
In `@apps/dashboard/src/components/workflows/graph/job-card.tsx`:
- Around line 63-87: The button that toggles the step list (button using
onToggle, rendering CheckStateIcon, name, duration and NodeChevron) doesn't
expose its expanded state to assistive tech; add aria-expanded={expanded} and
aria-controls pointing to the step-list container's id on that button (only when
onToggle is present), and give the conditional container div (the one rendering
job.steps / StepRow) a stable id (e.g., `job-step-list-{job.id}` or similar
unique id) so screen readers can associate the button with the controlled
region; ensure the aria attributes are omitted when onToggle is not provided.
In `@apps/dashboard/src/components/workflows/workflow-run-artifacts.tsx`:
- Around line 116-142: DigestCell currently ignores clipboard write errors and
never clears its timeout on unmount; update the component so handleCopy properly
handles navigator.clipboard.writeText errors (use async/await with try/catch or
.catch to avoid unhandled rejections and only setCopied when write succeeds) and
ensure timeoutRef is initialized to null (not undefined) and cleared before
setting a new timeout, then add a useEffect cleanup that calls
clearTimeout(timeoutRef.current) on unmount to avoid state updates after
unmount; reference DigestCell, handleCopy and timeoutRef when making these
changes.
In `@apps/dashboard/src/components/workflows/workflow-run-sidebar.tsx`:
- Around line 25-31: The Filter button in workflow-run-sidebar.tsx is rendered
as an interactive control but has no behavior; update the <button> (the element
rendering FilterIcon) to be non-interactive until implemented by adding the
disabled attribute and aria-disabled="true", remove or replace hover/interactive
classes with a disabled style (e.g., opacity-50 cursor-not-allowed) and add a
clear accessible label or title indicating "Filter not implemented" so users and
assistive tech aren't misled; alternatively, if you prefer to implement
filtering now, wire the onClick to the appropriate filter handler (e.g.,
openFilterPanel or applyJobFilter) and ensure the handler is referenced and
tested.
In `@apps/dashboard/src/lib/format-relative-time.ts`:
- Around line 1-4: In formatRelativeTime, guard against invalid dateStr by
parsing it up front (const date = new Date(dateStr)) and checking if
date.getTime() is NaN; if so return an empty string (or a defined fallback)
immediately instead of proceeding to compute seconds, then use that parsed
date.getTime() in the existing seconds calculation so the function never
performs arithmetic on NaN.
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 10315-10335: The code that builds yamlText uses
atob(payload.content.replace(/\n/g, "")) which decodes base64 as Latin-1 and
corrupts UTF-8 multi-byte characters; update the decoding in the block where
payload, encoding and yamlText are handled (the getContent response processing
in github.functions.ts) to decode base64 using
Buffer.from(payload.content.replace(/\n/g, ""), "base64").toString("utf-8") when
encoding === "base64" (leave non-base64 branch unchanged) so that yamlText
preserves UTF-8 correctly.
In `@apps/dashboard/src/routes/_protected/`$owner/$repo/actions.runs.$runId.tsx:
- Around line 20-33: validateSearch currently allows non-integer PR values and
loader converts params.runId without ensuring it's a positive integer; update
validateSearch (function validateSearch) to parse search.pr with parseInt,
ensure Number.isInteger(parsed) and parsed > 0 before returning { pr: parsed }
otherwise return {}; in loader, parse params.runId with parseInt, verify
Number.isInteger(runId) && runId > 0 (and likewise ensure
params.owner/params.repo are present), and if invalid return/throw an early
bad-request response (e.g., throw new Response("Invalid runId", { status: 400
})) before building scope/input and querying.
In `@packages/icons/src/full-screen-icon.tsx`:
- Around line 8-21: Remove the hardcoded English accessible name by making the
SVG decorative by default: delete the aria-label="Fullscreen" and instead set
aria-hidden={!(props['aria-label'] || props['aria-labelledby'])} (or default
aria-hidden=true) and only add role="img" when an aria-label or aria-labelledby
is provided; keep spreading {...rest} so callers can opt in by passing
aria-label/aria-labelledby. Update the SVG in full-screen-icon.tsx (the SVG
element that currently sets aria-label, role, height/width, {...rest}) so it
emits aria-hidden by default and conditionally exposes role/label when the
consumer supplies accessible name props.
---
Nitpick comments:
In `@apps/dashboard/src/components/workflows/graph/edges.ts`:
- Around line 9-28: The current traversal scans the full edges array for every
popped node (the loops using forward/backward and variables edges, current,
result), which is O(E·V); precompute adjacency maps before those loops: build
outgoing: Map<nodeId, Edge[]> keyed by edge.source and incoming: Map<nodeId,
Edge[]> keyed by edge.target (iterating edges once), then replace the inner for
(const edge of edges) scans with iterating outgoing.get(current) for forward
expansion and incoming.get(current) for backward expansion, keeping the same
checks for result.has(edge.id) and pushing edge.target/edge.source as before.
In `@apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx`:
- Around line 192-220: Popup nodes for matrix variants are placed at a fixed X
(popupX = matrix.position.x + NODE_WIDTH + COLUMN_GAP + VARIANT_POPUP_GAP),
which can overlap nodes in following columns in multi-column/fallback layouts;
update the placement logic in the block that builds popup nodes (matrixNodes
loop / popupX variable) to either compute X based on available column width or
place popups on a separate overlay layer: e.g., detect when matrix is in a
multi-column layout and increase popupX by the width of subsequent columns (or
add a flag so the job popup nodes are rendered into an overlay stack instead of
the main canvas), and ensure any height calculations using estimateNodeHeight
and collapsedPopupIds/togglePopupCollapsed still work with the new placement
strategy.
In `@apps/dashboard/src/components/workflows/workflow-run-page.tsx`:
- Around line 72-75: The useQuery call with githubViewerQueryOptions(scope) is
currently invoked solely for side effects (prefetch/cache hydration) since its
result is unused; add an inline comment above the useQuery call clarifying that
intent (e.g., "prefetch viewer data / ensure cache hydration") so future readers
know this is deliberate and not a forgotten bug—referencing the useQuery
invocation, githubViewerQueryOptions(scope) options, and the enabled: hasMounted
flag.
In `@apps/dashboard/src/lib/github-cache-policy.ts`:
- Around line 35-38: The workflowRun cache bucket uses a very short TTL
(staleTimeMs: 15 * 1000) which causes immutable workflow definitions to be
refetched; update the workflowRun policy (the object named workflowRun in
github-cache-policy.ts) to use a much longer staleTimeMs and gcTimeMs
appropriate for mostly-static parsed YAML (for example, set staleTimeMs to
hours/days and gcTimeMs to several days) so getWorkflowDefinition and other
callers sharing this bucket stop re-parsing unchanged data.
In `@apps/dashboard/src/lib/use-now.ts`:
- Around line 14-16: When starting the interval in the subscribe path, call
tick() once immediately so the first subscriber gets a fresh snapshot instead of
waiting up to 1s; in the block that checks intervalId (the code that currently
does if (intervalId === null) { intervalId = setInterval(tick, 1000); }), invoke
tick() before creating the interval (use the existing tick function and
intervalId variable) so the state is refreshed right when the first listener
attaches.
In `@apps/dashboard/src/routes/_protected/`$owner/$repo/actions.runs.$runId.tsx:
- Line 49: The promise chain currently ends with a silent .catch(() => {}) which
hides failures from the loader prefetch; replace that silent handler with an
error-reporting handler that captures the error and context (owner, repo, runId)
and forwards it to your logging/telemetry system or at minimum console.error so
auth/rate-limit/fetch issues are visible; locate the prefetch call in
actions.runs.$runId (the code that currently uses .catch(() => {})) and change
the empty catch to report the error and include identifying context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8b159c6-8535-437a-9134-e8f3e971fd59
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
apps/dashboard/package.jsonapps/dashboard/src/components/checks/check-state-icon.tsxapps/dashboard/src/components/pulls/detail/pull-detail-activity.tsxapps/dashboard/src/components/pulls/detail/pull-detail-header.tsxapps/dashboard/src/components/shared/copy-badge.tsxapps/dashboard/src/components/workflows/graph/build-layout.tsapps/dashboard/src/components/workflows/graph/constants.tsapps/dashboard/src/components/workflows/graph/edges.tsapps/dashboard/src/components/workflows/graph/format.tsapps/dashboard/src/components/workflows/graph/graph-controls.tsxapps/dashboard/src/components/workflows/graph/grouping.tsapps/dashboard/src/components/workflows/graph/height.tsapps/dashboard/src/components/workflows/graph/job-card.tsxapps/dashboard/src/components/workflows/graph/job-node.tsxapps/dashboard/src/components/workflows/graph/matrix-node.tsxapps/dashboard/src/components/workflows/graph/types.tsapps/dashboard/src/components/workflows/workflow-run-artifacts.tsxapps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsxapps/dashboard/src/components/workflows/workflow-run-graph.tsxapps/dashboard/src/components/workflows/workflow-run-header.tsxapps/dashboard/src/components/workflows/workflow-run-page.tsxapps/dashboard/src/components/workflows/workflow-run-sidebar.tsxapps/dashboard/src/components/workflows/workflow-run-summary.tsxapps/dashboard/src/lib/format-relative-time.tsapps/dashboard/src/lib/github-cache-policy.tsapps/dashboard/src/lib/github.functions.tsapps/dashboard/src/lib/github.query.tsapps/dashboard/src/lib/github.types.tsapps/dashboard/src/lib/use-now.tsapps/dashboard/src/routeTree.gen.tsapps/dashboard/src/routes/_protected/$owner/$repo/actions.runs.$runId.tsxpackages/icons/src/full-screen-icon.tsxpackages/icons/src/index.ts
- Isolate useNow() ticker to a JobDuration component; completed jobs no longer re-render every second (only live jobs subscribe) - Move node collapse toggle to React context; unify main/popup collapse state in a single Set<string> so data objects stop carrying per-node closures recreated on each render - Hoist ReactFlow fitViewOptions / proOptions / defaultEdgeOptions to module scope; stabilize onNodeMouseEnter/Leave with useCallback - Clear DigestCell copy timeout on unmount - Animate a wrapper div instead of the SVG element for the pending icon
WorkflowRunSummary subscribed to useNow() at the root, so the whole summary strip re-rendered every second even when the run was completed. Move the subscription into tiny leaf components (RelativeTime, LiveTotalDuration). Completed runs compute duration once and render static; only the relative-time text re-renders on tick.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx (1)
252-260: Consider adding error handling for Fullscreen API calls.
requestFullscreen()andexitFullscreen()can reject if the browser denies the request (e.g., not triggered by user gesture, feature policy restriction). While usingvoidcorrectly discards the Promise, unhandled rejections could occur.🔧 Optional: Add catch handler for robustness
const toggleFullscreen = useCallback(() => { const el = containerRef.current; if (!el) return; if (document.fullscreenElement) { - void document.exitFullscreen(); + document.exitFullscreen().catch(() => {}); } else { - void el.requestFullscreen(); + el.requestFullscreen().catch(() => {}); } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx` around lines 252 - 260, The toggleFullscreen function uses requestFullscreen and exitFullscreen but discards their promises with void which can lead to unhandled rejections; update toggleFullscreen to handle rejections by calling .catch on the promises returned by el.requestFullscreen() and document.exitFullscreen() (or await in a try/catch), and log or handle errors via your logger; reference the containerRef.current element, the toggleFullscreen callback, and the Fullscreen API methods (requestFullscreen, exitFullscreen, document.fullscreenElement) when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx`:
- Around line 252-260: The toggleFullscreen function uses requestFullscreen and
exitFullscreen but discards their promises with void which can lead to unhandled
rejections; update toggleFullscreen to handle rejections by calling .catch on
the promises returned by el.requestFullscreen() and document.exitFullscreen()
(or await in a try/catch), and log or handle errors via your logger; reference
the containerRef.current element, the toggleFullscreen callback, and the
Fullscreen API methods (requestFullscreen, exitFullscreen,
document.fullscreenElement) when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf60484c-d044-4434-8eff-54b16bc64791
📒 Files selected for processing (10)
apps/dashboard/src/components/checks/check-state-icon.tsxapps/dashboard/src/components/workflows/graph/build-layout.tsapps/dashboard/src/components/workflows/graph/job-card.tsxapps/dashboard/src/components/workflows/graph/job-duration.tsxapps/dashboard/src/components/workflows/graph/job-node.tsxapps/dashboard/src/components/workflows/graph/matrix-node.tsxapps/dashboard/src/components/workflows/graph/toggle-context.tsapps/dashboard/src/components/workflows/graph/types.tsapps/dashboard/src/components/workflows/workflow-run-artifacts.tsxapps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/src/components/workflows/graph/types.ts
- apps/dashboard/src/components/checks/check-state-icon.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/dashboard/src/components/workflows/graph/job-node.tsx
- apps/dashboard/src/components/workflows/workflow-run-artifacts.tsx
- apps/dashboard/src/components/workflows/graph/build-layout.ts
- apps/dashboard/src/components/workflows/graph/job-card.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/workflows/workflow-run-summary.tsx`:
- Around line 130-138: The InfoCell component's prop type uses React.ReactNode
but the React namespace isn't imported, causing a TS "Cannot find namespace
'React'" error; fix by importing the type from React and updating the signature:
either add "import React from 'react'" at the top of the file or better add
"import type { ReactNode } from 'react'" and change the prop type to use
ReactNode (e.g., value: ReactNode) so the InfoCell function and its props
compile cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2affca83-2ca5-4f24-b74b-ab8004a92d48
📒 Files selected for processing (1)
apps/dashboard/src/components/workflows/workflow-run-summary.tsx
Adds an inline step log viewer inside the workflow run graph that fetches job logs, filters to the selected step (by ##[group] or timestamp range), and streams updates via React Query while the step is running. Step cards are draggable (real-time via useNodesState), resizable from the bottom right (only visible on hover via a shared NodeHoverProvider), wrap long lines, and render ##[group] blocks as collapsible sections with chevrons. Falls back to the OAuth context when the GitHub App isn't installed on public repos so logs still load.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/dashboard/src/components/workflows/graph/height.ts (1)
36-40:⚠️ Potential issue | 🟡 MinorMatrix height adds one extra trailing gap.
Line 39 counts
H_MATRIX_CARD_GAPfor every item; only inter-item gaps should be counted.📐 Proposed fix
return ( H_MATRIX_OUTER_PAD + main + - jobs.length * (H_MATRIX_PILL + H_MATRIX_CARD_GAP) + jobs.length * H_MATRIX_PILL + + Math.max(0, jobs.length - 1) * H_MATRIX_CARD_GAP );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/graph/height.ts` around lines 36 - 40, The return expression in height.ts currently adds H_MATRIX_CARD_GAP for every job (jobs.length * H_MATRIX_CARD_GAP), producing an extra trailing gap; change it so gaps are counted between items only (use (jobs.length - 1) for the gap count, ensuring you clamp to 0 when jobs.length is 0), i.e. compute total as H_MATRIX_OUTER_PAD + main + jobs.length * H_MATRIX_PILL + Math.max(0, (jobs.length - 1)) * H_MATRIX_CARD_GAP so you reference H_MATRIX_OUTER_PAD, main, H_MATRIX_PILL, H_MATRIX_CARD_GAP and jobs.length when making the fix.apps/dashboard/src/lib/github.functions.ts (1)
10332-10336:⚠️ Potential issue | 🟠 MajorDecode workflow YAML as UTF-8 instead of
atob().This still has the same UTF-8 corruption issue from the earlier review:
atob()returns a Latin-1 binary string, so non-ASCII workflow files can be mojibaked before YAML parsing. UseBuffer.from(..., "base64").toString("utf-8")here instead.Proposed fix
const encoding = payload.encoding ?? "base64"; const yamlText = encoding === "base64" - ? atob(payload.content.replace(/\n/g, "")) + ? Buffer.from( + payload.content.replace(/\n/g, ""), + "base64", + ).toString("utf-8") : payload.content; return parseWorkflowDefinition(yamlText);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/github.functions.ts` around lines 10332 - 10336, The YAML decode uses atob() which yields a Latin-1 binary string and corrupts non-ASCII characters; replace the atob-based branch in the yamlText assignment so that when encoding === "base64" you call Buffer.from(payload.content.replace(/\n/g, ""), "base64").toString("utf-8") instead of atob(...), keeping the existing fallback to payload.content for non-base64 encodings and preserving the encoding and payload.content references.
🧹 Nitpick comments (1)
apps/dashboard/src/components/workflows/graph/step-log-node.tsx (1)
42-44: PreferNodeProps.idover recomputing the node id from data.Using
idfrom node props avoids tight coupling togetStepLogNodeId()internals and prevents close/hover drift if id construction ever changes.♻️ Proposed refactor
-import { getStepLogNodeId, useStepLogActions } from "./step-log-context"; +import { useStepLogActions } from "./step-log-context"; @@ export function StepLogNode({ + id, data, }: NodeProps<Node<StepLogNodeData, "stepLog">>) { @@ - const nodeId = getStepLogNodeId(data.jobId, data.stepNumber); + const nodeId = id;Also applies to: 76-80, 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx` around lines 42 - 44, The component StepLogNode recomputes the node id from data using getStepLogNodeId() which couples rendering to the id construction; instead use the id provided on NodeProps (NodeProps.id) inside StepLogNode and any helper logic that currently calls getStepLogNodeId(data...) (also replace the recomputation occurrences around the other uses in this file). Update references in StepLogNode and the other spots that build or compare node IDs to use the incoming id parameter (and keep StepLogNodeData as-is) so hover/close behavior and comparisons use the canonical id from props rather than reconstructing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/workflows/graph/parse-step-log.ts`:
- Around line 145-157: extractStepLog currently returns the first group match
from extractByGroup and ignores range, which mis-associates duplicate group
names; change logic in extractStepLog so that when extractByGroup(lines,
stepName) yields multiple entries and a range (StepLogRange with
startedAt/completedAt) is provided, prefer the group whose timestamps overlap
the range (use the group's timestamp fields to test overlap with
range.startedAt/ completedAt) and return that single matching group with
strategy "group"; only fall back to the first textual match when no overlapping
group is found or no range is provided; keep extractByTimeRange(lines, range) as
a fallback for when no textual groups exist.
In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx`:
- Around line 178-185: The LogBody is currently only driven by entries/hasLogs
and shows "No log output..." even when the logsQuery failed; update the LogBody
invocations (the one rendering <LogBody entries={entries} ...
isStepLive={isStepLive} /> and the other similar occurrences) to check
logsQuery.isError or logsQuery.error and pass an explicit fetchError prop (or a
boolean like isFetchError) so LogBody can render an error message instead of the
empty-log text; ensure you wire logsQuery.error (or logsQuery.error.message)
into the prop and adjust LogBody to prefer showing the fetch error when present
before falling back to the "no logs" message.
In `@apps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsx`:
- Around line 94-136: The collapsed initial state and the auto-backfill only add
popup node IDs, so aggregate matrix nodes (the plain "matrix-<key>" and
"def-<key>" IDs) are left expanded; update the initializer for collapsedIds and
the useEffect that backfills autoCollapsedRef to also add the aggregate IDs
(e.g., add `matrix-${match[1]}` and `def-${match[1]}` alongside the popup IDs
produced by getPopupNodeId), and when definition.jobs is processed ensure
non-matrix/ matrix aggregate `def-<key>` entries are seeded as well so both
aggregate and popup nodes are collapsed consistently (update collapsedIds,
autoCollapsedRef, and the toAdd set logic in the useEffect accordingly).
- Around line 256-271: The current fallback loop in
workflow-run-graph-canvas.tsx (iterating columns and pushing edges into
builtEdges using getGroupId) creates a full bipartite connection between every
group in column N and N+1, inventing dependencies; change the fallback so it
does not fabricate edges: either skip adding any edges in this fallback path
(leave builtEdges empty for these columns) or only add edges when confirmed by
explicit dependency metadata (check the node/group dependency records you
already have and only push edges for those declared links instead of connecting
every prevGroup to every nextGroup). Ensure you update the loop that produces
builtEdges to consult the explicit dependency structure and reference
getGroupId, prevGroup/nextGroup and builtEdges when making the conditional
decision.
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 10419-10436: The console.log is emitting raw workflow output
(variables logs, preview, groupMarkers) which leaks user data and inflates logs;
change the log call to emit only high-level metadata (e.g., status, dataKind,
bytes) and remove preview and groupMarkers from the public log payload, and if
you need the preview/groupMarkers for troubleshooting, send them only to the
debug logger (guarded by the existing debug flag) or a dedicated debug-only
logger; update the code around the groupMarkers array handling and the
console.log invocation so that groupMarkers and logs.slice(...) are not included
in the standard log path and are only emitted when debug is enabled.
- Around line 10401-10409: The GET /repos/.../actions/jobs/{job_id}/logs call
using ctx.octokit.request is unbounded and should be wrapped in the existing
timeout helper; replace the direct await ctx.octokit.request(...) call with a
call to withGitHubOperationTimeout that invokes ctx.octokit.request (e.g. await
withGitHubOperationTimeout(() => ctx.octokit.request(...), tag)), keeping the
same parameters (owner, repo, job_id) and preserving the tag/label logging;
reference the withGitHubOperationTimeout helper and the ctx.octokit.request
invocation to locate and update the code.
---
Duplicate comments:
In `@apps/dashboard/src/components/workflows/graph/height.ts`:
- Around line 36-40: The return expression in height.ts currently adds
H_MATRIX_CARD_GAP for every job (jobs.length * H_MATRIX_CARD_GAP), producing an
extra trailing gap; change it so gaps are counted between items only (use
(jobs.length - 1) for the gap count, ensuring you clamp to 0 when jobs.length is
0), i.e. compute total as H_MATRIX_OUTER_PAD + main + jobs.length *
H_MATRIX_PILL + Math.max(0, (jobs.length - 1)) * H_MATRIX_CARD_GAP so you
reference H_MATRIX_OUTER_PAD, main, H_MATRIX_PILL, H_MATRIX_CARD_GAP and
jobs.length when making the fix.
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 10332-10336: The YAML decode uses atob() which yields a Latin-1
binary string and corrupts non-ASCII characters; replace the atob-based branch
in the yamlText assignment so that when encoding === "base64" you call
Buffer.from(payload.content.replace(/\n/g, ""), "base64").toString("utf-8")
instead of atob(...), keeping the existing fallback to payload.content for
non-base64 encodings and preserving the encoding and payload.content references.
---
Nitpick comments:
In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx`:
- Around line 42-44: The component StepLogNode recomputes the node id from data
using getStepLogNodeId() which couples rendering to the id construction; instead
use the id provided on NodeProps (NodeProps.id) inside StepLogNode and any
helper logic that currently calls getStepLogNodeId(data...) (also replace the
recomputation occurrences around the other uses in this file). Update references
in StepLogNode and the other spots that build or compare node IDs to use the
incoming id parameter (and keep StepLogNodeData as-is) so hover/close behavior
and comparisons use the canonical id from props rather than reconstructing it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f2a23c8-9d36-4750-ba87-11cf3eabcfa1
📒 Files selected for processing (16)
apps/dashboard/src/components/workflows/graph/constants.tsapps/dashboard/src/components/workflows/graph/graph-config-context.tsapps/dashboard/src/components/workflows/graph/height.tsapps/dashboard/src/components/workflows/graph/hover-context.tsapps/dashboard/src/components/workflows/graph/job-card.tsxapps/dashboard/src/components/workflows/graph/job-node.tsxapps/dashboard/src/components/workflows/graph/parse-step-log.tsapps/dashboard/src/components/workflows/graph/step-log-context.tsapps/dashboard/src/components/workflows/graph/step-log-node.tsxapps/dashboard/src/components/workflows/graph/types.tsapps/dashboard/src/components/workflows/workflow-run-graph-canvas.tsxapps/dashboard/src/components/workflows/workflow-run-graph.tsxapps/dashboard/src/components/workflows/workflow-run-page.tsxapps/dashboard/src/lib/github.functions.tsapps/dashboard/src/lib/github.query.tsapps/dashboard/src/lib/github.types.ts
✅ Files skipped from review due to trivial changes (3)
- apps/dashboard/src/components/workflows/graph/types.ts
- apps/dashboard/src/components/workflows/graph/constants.ts
- apps/dashboard/src/components/workflows/graph/step-log-context.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/src/components/workflows/graph/job-card.tsx
- apps/dashboard/src/components/workflows/workflow-run-page.tsx
- apps/dashboard/src/lib/github.query.ts
Reuses the workflow run header and jobs sidebar so navigating into a job keeps the same context, with the active job highlighted in the sidebar. Extracts the step log renderer so it can be shared between the graph node and the job page, and isolates the duration ticker to leaf components.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/src/components/workflows/graph/step-log-node.tsx (1)
172-179:⚠️ Potential issue | 🟠 MajorQuery errors not surfaced to the user.
The
logsQuery.isErrorstate is not passed toStepLogContent, so when log fetching fails, users see "No log output for this step yet" instead of an error message. This hides real fetch errors and makes troubleshooting harder.🔧 Proposed fix
First, update
StepLogContentprops:export function StepLogContent({ entries, totalLineCount, isLoading, + isError, notAvailable, hasLogs, isStepLive, }: { entries: LogEntry[]; totalLineCount: number; isLoading: boolean; + isError: boolean; notAvailable: boolean; hasLogs: boolean; isStepLive: boolean; }) {Add error state handling in
StepLogContentafter the loading check:if (isLoading && !hasLogs) { return (/* loading UI */); } + + if (isError && !hasLogs) { + return ( + <div className="flex flex-1 items-center justify-center px-4 text-center text-muted-foreground text-xs"> + Failed to load logs. Try refreshing. + </div> + ); + }Then pass it from
StepLogNode:<StepLogContent entries={entries} totalLineCount={totalLineCount} isLoading={logsQuery.isLoading} + isError={logsQuery.isError} notAvailable={notAvailable} hasLogs={hasLogs} isStepLive={isStepLive} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx` around lines 172 - 179, The StepLogContent consumer currently never receives the query error state so users see the "no logs" message on fetch failures; update StepLogContent's props to accept an error flag (e.g., isError: boolean or error?: unknown) and add handling inside StepLogContent after the loading check to render an error message when that flag is true (or when error is present), then pass logsQuery.isError (or logsQuery.error) from StepLogNode into the StepLogContent invocation so the component can surface fetch errors instead of the "no output" message.
🧹 Nitpick comments (3)
apps/dashboard/src/components/workflows/graph/step-log-content.tsx (1)
251-294: Consider memoizingGroupHeaderRowfor consistency withLogRow.
LogRowis wrapped withmemobutGroupHeaderRowis not. For large logs with many collapsible groups, this could cause unnecessary re-renders when parent state changes.♻️ Proposed fix
-function GroupHeaderRow({ +const GroupHeaderRow = memo(function GroupHeaderRow({ name, lineNumber, depth, isOpen, onToggle, lineNoWidth, }: { name: string; lineNumber: number; depth: number; isOpen: boolean; onToggle: () => void; lineNoWidth: string; }) { return ( // ... existing JSX ); -} +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/graph/step-log-content.tsx` around lines 251 - 294, GroupHeaderRow is not memoized while LogRow is, causing unnecessary re-renders for many groups; wrap/export GroupHeaderRow with React.memo (matching how LogRow is memoized) so it only re-renders when its props (name, lineNumber, depth, isOpen, onToggle, lineNoWidth) change, and ensure any inline functions/objects passed from parents remain stable or are handled (e.g., avoid recreating onToggle or lineNoWidth each render) to get the intended performance benefit.apps/dashboard/src/components/workflows/graph/step-log-node.tsx (1)
75-83: Consider wrappinghandleRefreshinuseCallback.The handler is recreated on every render. While not a significant performance issue here, wrapping it in
useCallbackwould be more consistent with React best practices for event handlers passed to child elements.♻️ Proposed fix
+import { useCallback, useMemo, useState } from "react"; -import { useMemo, useState } from "react"; - const handleRefresh = () => { + const handleRefresh = useCallback(() => { void queryClient.invalidateQueries({ queryKey: githubQueryKeys.actions.workflowJobLogs(scope, { owner, repo, jobId: data.jobId, }), }); - }; + }, [queryClient, scope, owner, repo, data.jobId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx` around lines 75 - 83, The handleRefresh function is recreated each render; wrap it in React's useCallback to memoize it and avoid unnecessary re-creation. Update the handleRefresh definition to use useCallback with a dependency array including queryClient and all values used to build the query key (scope, owner, repo, and data.jobId) so the callback updates when any of those change; also ensure useCallback is imported from React. Keep the callback body calling queryClient.invalidateQueries(...) with the same githubQueryKeys.actions.workflowJobLogs(scope, { owner, repo, jobId: data.jobId }) call.apps/dashboard/src/components/workflows/workflow-job-page.tsx (1)
193-202: Redundant query refresh causes potential double network request.
invalidateQueries()automatically triggers a refetch for active queries. CallingonRefresh()(which invokeslogsQuery.refetch()) immediately after is redundant and may cause duplicate network requests.♻️ Remove redundant refetch call
Either rely on invalidation alone (recommended):
const handleInvalidateAll = useCallback(() => { void queryClient.invalidateQueries({ queryKey: githubQueryKeys.actions.workflowJobLogs(scope, { owner, repo, jobId, }), }); - onRefresh(); -}, [queryClient, scope, owner, repo, jobId, onRefresh]); +}, [queryClient, scope, owner, repo, jobId]);Then update the prop passed to
JobHeader:<JobHeader job={job} isLogsFetching={isLogsFetching} - onRefresh={handleInvalidateAll} + onRefresh={() => { + void queryClient.invalidateQueries({ + queryKey: githubQueryKeys.actions.workflowJobLogs(scope, { + owner, + repo, + jobId: jobIdNum, + }), + }); + }} />Or simply use
refetch()alone without invalidation (also removes stale data):const handleInvalidateAll = useCallback(() => { - void queryClient.invalidateQueries({ - queryKey: githubQueryKeys.actions.workflowJobLogs(scope, { - owner, - repo, - jobId, - }), - }); onRefresh(); -}, [queryClient, scope, owner, repo, jobId, onRefresh]); +}, [onRefresh]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflows/workflow-job-page.tsx` around lines 193 - 202, The handler handleInvalidateAll currently calls queryClient.invalidateQueries(...) and then onRefresh(), causing duplicate fetches; remove the redundant onRefresh() call and rely on queryClient.invalidateQueries to trigger refetch for the active logs query (keep the current queryKey usage: githubQueryKeys.actions.workflowJobLogs(scope, { owner, repo, jobId })); alternatively, if you prefer explicit refetching, remove the invalidateQueries call and call logsQuery.refetch() (or keep onRefresh()) but do not do both — also ensure the prop passed into JobHeader aligns with the chosen approach (either pass a no-op or the refetch function instead of calling both).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/workflows/workflow-run-page.tsx`:
- Around line 35-37: Parse and validate the route param runId once after
extracting params in the WorkflowRunPage component: convert runId to a number
(e.g., const parsedRunId = Number(runId)), check Number.isInteger(parsedRunId)
(or !Number.isNaN(parsedRunId)) and handle invalid values (redirect, show an
error, or return null) and then replace all occurrences of Number(runId)
(including inside the useMemo that builds input and any other props/queries)
with parsedRunId so the validated numeric id is reused consistently.
---
Duplicate comments:
In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx`:
- Around line 172-179: The StepLogContent consumer currently never receives the
query error state so users see the "no logs" message on fetch failures; update
StepLogContent's props to accept an error flag (e.g., isError: boolean or
error?: unknown) and add handling inside StepLogContent after the loading check
to render an error message when that flag is true (or when error is present),
then pass logsQuery.isError (or logsQuery.error) from StepLogNode into the
StepLogContent invocation so the component can surface fetch errors instead of
the "no output" message.
---
Nitpick comments:
In `@apps/dashboard/src/components/workflows/graph/step-log-content.tsx`:
- Around line 251-294: GroupHeaderRow is not memoized while LogRow is, causing
unnecessary re-renders for many groups; wrap/export GroupHeaderRow with
React.memo (matching how LogRow is memoized) so it only re-renders when its
props (name, lineNumber, depth, isOpen, onToggle, lineNoWidth) change, and
ensure any inline functions/objects passed from parents remain stable or are
handled (e.g., avoid recreating onToggle or lineNoWidth each render) to get the
intended performance benefit.
In `@apps/dashboard/src/components/workflows/graph/step-log-node.tsx`:
- Around line 75-83: The handleRefresh function is recreated each render; wrap
it in React's useCallback to memoize it and avoid unnecessary re-creation.
Update the handleRefresh definition to use useCallback with a dependency array
including queryClient and all values used to build the query key (scope, owner,
repo, and data.jobId) so the callback updates when any of those change; also
ensure useCallback is imported from React. Keep the callback body calling
queryClient.invalidateQueries(...) with the same
githubQueryKeys.actions.workflowJobLogs(scope, { owner, repo, jobId: data.jobId
}) call.
In `@apps/dashboard/src/components/workflows/workflow-job-page.tsx`:
- Around line 193-202: The handler handleInvalidateAll currently calls
queryClient.invalidateQueries(...) and then onRefresh(), causing duplicate
fetches; remove the redundant onRefresh() call and rely on
queryClient.invalidateQueries to trigger refetch for the active logs query (keep
the current queryKey usage: githubQueryKeys.actions.workflowJobLogs(scope, {
owner, repo, jobId })); alternatively, if you prefer explicit refetching, remove
the invalidateQueries call and call logsQuery.refetch() (or keep onRefresh())
but do not do both — also ensure the prop passed into JobHeader aligns with the
chosen approach (either pass a no-op or the refetch function instead of calling
both).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05ad33e4-0785-4e87-9728-5ba9b94b8b8e
📒 Files selected for processing (8)
apps/dashboard/src/components/workflows/graph/format.tsapps/dashboard/src/components/workflows/graph/step-log-content.tsxapps/dashboard/src/components/workflows/graph/step-log-node.tsxapps/dashboard/src/components/workflows/workflow-job-page.tsxapps/dashboard/src/components/workflows/workflow-run-page.tsxapps/dashboard/src/components/workflows/workflow-run-sidebar.tsxapps/dashboard/src/routeTree.gen.tsapps/dashboard/src/routes/_protected/$owner/$repo/actions.runs.$runId_.jobs.$jobId.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/src/components/workflows/workflow-run-sidebar.tsx
Comment on lines
+35
to
+37
| const input = useMemo( | ||
| () => ({ owner, repo, runId: Number(runId) }), | ||
| [owner, repo, runId], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists
find . -type f -name "workflow-run-page.tsx" | head -5Repository: stylessh/diffkit
Length of output: 123
🏁 Script executed:
# Check file size and read the relevant sections
wc -l apps/dashboard/src/components/workflows/workflow-run-page.tsxRepository: stylessh/diffkit
Length of output: 125
🏁 Script executed:
# Read lines 30-40 to verify the snippet
sed -n '30,40p' apps/dashboard/src/components/workflows/workflow-run-page.tsxRepository: stylessh/diffkit
Length of output: 421
🏁 Script executed:
# Read lines 120-140 to check lines 125 and 136
sed -n '120,140p' apps/dashboard/src/components/workflows/workflow-run-page.tsxRepository: stylessh/diffkit
Length of output: 445
🏁 Script executed:
# Search for all occurrences of Number(runId)
rg "Number\(runId\)" apps/dashboard/src/components/workflows/workflow-run-page.tsx -nRepository: stylessh/diffkit
Length of output: 174
Parse and validate runId once, then reuse it everywhere.
Line 36, 125, and 136 each call Number(runId) without validation. If the param is malformed, NaN propagates into query inputs, dependency arrays, and downstream component props. Add a single validation check after extracting route params and reuse the validated value in all three locations.
Suggested fix
export function WorkflowRunPage() {
const { user } = routeApi.useRouteContext();
const { owner, repo, runId } = routeApi.useParams();
const { pr: prNumberFromSearch } = routeApi.useSearch();
+ const runIdNumber = Number(runId);
+ if (!Number.isInteger(runIdNumber) || runIdNumber <= 0) {
+ throw new Error("Invalid workflow run id");
+ }
+
const scope = useMemo(() => ({ userId: user.id }), [user.id]);
const input = useMemo(
- () => ({ owner, repo, runId: Number(runId) }),
- [owner, repo, runId],
+ () => ({ owner, repo, runId: runIdNumber }),
+ [owner, repo, runIdNumber],
);
@@
<WorkflowRunGraph
@@
- runId={Number(runId)}
+ runId={runIdNumber}
/>
@@
<WorkflowRunSidebar
@@
- runId={Number(runId)}
+ runId={runIdNumber}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/components/workflows/workflow-run-page.tsx` around lines
35 - 37, Parse and validate the route param runId once after extracting params
in the WorkflowRunPage component: convert runId to a number (e.g., const
parsedRunId = Number(runId)), check Number.isInteger(parsedRunId) (or
!Number.isNaN(parsedRunId)) and handle invalid values (redirect, show an error,
or return null) and then replace all occurrences of Number(runId) (including
inside the useMemo that builds input and any other props/queries) with
parsedRunId so the validated numeric id is reused consistently.
…ob page step logs auto-height Replaces raw anchor tags in the workflow graph cards with typed TanStack Links, and adds a scrollable flag to StepLogContent so the job page expands to fit all log output instead of constraining each step to a fixed height.
- Disambiguate duplicate group names in log extraction using step timestamps - Seed matrix aggregate collapse IDs and backfill when definition loads - Remove full-bipartite fallback edges in graph canvas - Surface log fetch errors on job step rows
- Rename workflow job route from /jobs/:jobId to /job/:jobId to match GitHub - Expand step rows based on #step-N URL hash and scroll into view - Toggling a step updates the hash so the deep-link is shareable
Pass hash={step-N} to the step-log card's "open job page" Link so navigating from the graph opens the job page with the matching step already expanded.
Use a slugified step name for the job-page step anchor (e.g. #run-tests) instead of step-N. Falls back to step-N only when the slug is empty. Graph card deep-link uses the same slug helper so navigation from the canvas matches the rendered anchor.
Default every group in the step log viewer to collapsed, and keep any new groups that stream in (live logs) collapsed as well. User-driven expand/collapse state is preserved across re-renders.
GitHub log group headers don't always match the step's display name (e.g. a step named "Run type check" emits a group header "Run pnpm check-types"). We now collect every top-level group, filter by the step's [startedAt, completedAt] window with a small tolerance, and pick the group whose header is closest to startedAt (preferring a name match when available). Falls back to name-only matching when no timestamps are available. This fixes cases where the renderer was pulling in content from previous steps via the time-range fallback.
Reverts the timestamp-based group picker added in 2b36167 — in practice it picked the wrong group on matrix jobs where composite actions emit extra top-level groups with timestamps that overlap the step window. The simpler "first group whose normalized name matches" approach is less clever but gave better results. Also moves the `split(/\r?\n/)` up to JobContainer so large logs are split once per refetch instead of once per JobStepRow.
Sidebar job link was using bg-muted/muted/60 for active/hover. Switch to text-muted-foreground at rest + bg-surface-1 on hover/active to match the dashboard tabs convention.
- /$owner/$repo/actions list with status/event/branch/author filters,
10s refetch when any run is in-progress, RunDuration leaf component
ticks only for live rows
- Actions card on the repo activity sidebar
- New getWorkflowRunLogsBundle server fn fetches the run-level zip
(/actions/runs/{id}/logs) and indexes per-step .txt files using the
same regex + UTF-16 sanitization rules as the gh CLI; JobContainer
prefers the per-step file for completed runs and falls back to the
per-job text endpoint for in-progress runs
Wrap workflow server fns with the existing D1+KV cache + signal-driven revalidation pattern (mirrors getPullStatus, getPullsFromRepo): - getWorkflowRunsForRepo (actionsRepo signal) - getWorkflowRun (workflowRunEntity signal) - listWorkflowRunJobs (workflowRunEntity signal, paginated) - listWorkflowRunArtifacts(workflowRunEntity signal, paginated) - getWorkflowRunLogsBundle(workflowRunEntity signal, 60min fresh) The workflow_run/workflow_job webhook handlers already extract these signal keys, so cache entries are invalidated as soon as a webhook fires. Routes: - actions.index loader prefetches first-page runs + overview + branches - job route prefetches the bundle when the cached run is completed, else falls back to the per-job text endpoint Components: - RepoActionsPage / WorkflowJobPage subscribe via useGitHubSignalStream to the relevant signal keys; replaces the 10s polling refetchInterval on the runs list (signal-stream's 90s poll is the safety net)
- New ActionsIcon (custom play-circle SVG) in @diffkit/icons - "actions" tab type in tab-store, mapped to ActionsIcon in dashboard-tabs - WorkflowRunPage and WorkflowJobPage register tabs keyed by run/job id; iconColor derives from getCheckState (green/red/yellow/muted) via the new getCheckStateColor helper - getGitHubRevalidationSignalKeysForTab returns actionsRepo for actions tabs (per-entity signal subscription stays in the page components) - Repo activity sidebar Actions card uses ActionsIcon
- Required StatePill on PR checks: outline + matching text size so it stays visible against bg-surface-1/50 in light mode and aligns with the right-side status text. - WorkflowRunPage tab title: fall back to run.name / "Run #N" when displayTitle is empty so useRegisterTab never bails on a falsy title.
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.
Summary
Adds an internal
/$owner/$repo/actions/runs/$runIdpage so workflow runs open in QuickHub instead of bouncing to github.com. Graph is parsed from the workflow YAML (needsDAG) and rendered with@xyflow/react; matrix jobs render as a single node with an always-visible variant column. PR check rows now link to this page when a matching workflow run exists.Changes
getWorkflowRun,listWorkflowRunJobs,listWorkflowRunArtifacts,getWorkflowDefinitionwith dedicated cache policy and query keyscomponents/workflows/*(page, header, summary, sidebar, artifacts, graph canvas + graph helpers)viewerCanRerun(push/admin perms), following the action CTA loading patternuseGitHubSignalStreamsubscribed toworkflowRunEntityuseNow()ticker so relative-time labels update in sync instead of on hovercheck-state-iconhelper; added a static "waiting" state (no spinner) coveringqueued/waiting/pendingPullCheckRun.workflowRunIdpopulated incomputePullStatusso PR check rows can link internallyTest Plan
/actions/runs/<id>?pr=<n>?pr=) falls back torun.pullRequests[0]pnpm -w typecheckandpnpm -w lintScreenshots
Summary by CodeRabbit
New Features
UI Improvements