dashboard: surface audit data in run details with progressive disclosure#42269
Conversation
…sure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…gnment, better error message Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0). |
There was a problem hiding this comment.
Pull request overview
This PR enhances the agentic workflows dashboard “Run details” view by adding an on-demand gh aw audit <run_id> --json integration, allowing users to drill into structured audit output with progressive disclosure directly in the canvas UI.
Changes:
- Added backend audit retrieval (
getAudit(runId)) with per-run caching and exposed it via a new/api/audit?run_id=endpoint plus anauditRuncanvas action. - Added frontend audit state + “Run Audit”/“Clear audit” controls and rendered audit results as
<details>/<summary>sections (overview, findings, recommendations, jobs, tool usage, errors/warnings). - Added CSS for tool usage grid layout and standardized disclosure marker styling.
Show a summary per file
| File | Description |
|---|---|
.github/extensions/agentic-workflows-dashboard/dashboard-data.mjs |
Adds getAudit(runId) that runs gh aw audit --json, parses output, and caches by run ID. |
.github/extensions/agentic-workflows-dashboard/extension.mjs |
Exposes /api/audit endpoint and adds auditRun canvas action wiring/documentation. |
.github/extensions/agentic-workflows-dashboard/web/app.js |
Introduces audit UI state and fetch logic (loadAudit/clearAudit) plus helpers for rendering badges/sections. |
.github/extensions/agentic-workflows-dashboard/web/index.html |
Adds “Run Audit” UI and renders structured audit sections using progressive disclosure. |
.github/extensions/agentic-workflows-dashboard/web/styles.css |
Adds tool usage grid styling and suppresses native <details> marker for consistent disclosure UI. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Low
| selectRun(runId) { | ||
| this.selectedRun = this.runs.find(run => run.run_id === runId) ?? null; | ||
| this.auditData = null; | ||
| this.errorAudit = ""; | ||
| }, |
| async loadAudit() { | ||
| if (!this.selectedRun) return; | ||
| this.loadingAudit = true; | ||
| this.errorAudit = ""; | ||
| this.auditData = null; | ||
| try { | ||
| const params = new URLSearchParams({ run_id: String(this.selectedRun.run_id) }); | ||
| const resp = await fetch(`/api/audit?${params.toString()}`); | ||
| const data = await resp.json(); | ||
| if (!resp.ok) throw new Error(data.error ?? `HTTP ${resp.status}`); | ||
| this.auditData = data; | ||
| } catch (error) { | ||
| this.errorAudit = `Audit failed: ${error.message}`; | ||
| } finally { | ||
| this.loadingAudit = false; | ||
| } | ||
| }, |
| properties: { run_id: { type: "string", description: "The workflow run ID to audit (numeric string)." } }, | ||
| additionalProperties: false, | ||
| }, | ||
| handler: async ctx => dataAccess.getAudit(String(ctx.input?.run_id ?? "")), |
| details > summary { | ||
| list-style: none; | ||
| } | ||
|
|
||
| details > summary::-webkit-details-marker { | ||
| display: none; | ||
| } |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #42269 ('dashboard: surface audit data in run details with progressive disclosure') only modifies dashboard extension files: dashboard-data.mjs, extension.mjs, web/app.js, web/index.html, web/styles.css — no Go or JavaScript test files. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: dashboard audit surface
Good progressive-disclosure design and clean separation between backend (getAudit//api/audit) and frontend state. Five issues found — one correctness bug, one security concern, and three lower-priority suggestions.
Blocking
| # | File | Issue |
|---|---|---|
| 1 | app.js:271 |
Race condition — switching runs while audit loads shows wrong run's data |
| 2 | index.html:217 |
Unvalidated URL from audit JSON bound to :href (javascript: XSS risk) |
Non-blocking suggestions
| # | File | Issue |
|---|---|---|
| 3 | app.js:290 |
resp.json() called before resp.ok check — confusing errors on non-JSON server responses |
| 4 | index.html:181 |
btn-danger (red) for a reversible "Clear audit" action |
| 5 | styles.css:62 |
Global details > summary selector; also redundant with inline list-style:none |
Inline comments added for all five.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 55.8 AIC · ⌖ 9.95 AIC · ⊞ 4.9K
| selectRun(runId) { | ||
| this.selectedRun = this.runs.find(run => run.run_id === runId) ?? null; | ||
| this.auditData = null; | ||
| this.errorAudit = ""; |
There was a problem hiding this comment.
Race condition: stale audit state bleeds into newly selected run.
selectRun clears auditData and errorAudit but leaves loadingAudit untouched. If the user clicks Run Audit on Run A, then switches to Run B before the fetch resolves:
- Run B UI shows "Running audit..." (stale
loadingAudit = true) - When Run A's fetch completes,
this.auditData = dataoverwrites Run B's state with Run A's audit results
Suggested fix:
selectRun(runId) {
this.selectedRun = this.runs.find(run => run.run_id === runId) ?? null;
this.auditData = null;
this.errorAudit = "";
this.loadingAudit = false; // reset stale loading state
},And in loadAudit, guard the write with a snapshot check:
const targetRunId = this.selectedRun.run_id;
// ... await fetch ...
if (this.selectedRun?.run_id === targetRunId) {
this.auditData = data;
}@copilot please address this.
| const params = new URLSearchParams({ run_id: String(this.selectedRun.run_id) }); | ||
| const resp = await fetch(`/api/audit?${params.toString()}`); | ||
| const data = await resp.json(); | ||
| if (!resp.ok) throw new Error(data.error ?? `HTTP ${resp.status}`); |
There was a problem hiding this comment.
resp.json() is called before resp.ok is checked — if the server ever returns a non-JSON error body (e.g. an unexpected crash outside the try/catch in extension.mjs), this will throw a SyntaxError and the user sees "Audit failed: Unexpected token..." instead of a meaningful message.
Safer pattern:
const resp = await fetch(`/api/audit?${params.toString()}`);
if (!resp.ok) {
let msg = `HTTP ${resp.status}`;
try { msg = (await resp.json()).error ?? msg; } catch { /* ignore parse error */ }
throw new Error(msg);
}
this.auditData = await resp.json();@copilot please address this.
| <div class="flex-1"> | ||
| <div class="text-small color-fg-muted">URL</div> | ||
| <div class="text-small"> | ||
| <a :href="auditData?.overview?.url ?? '#'" target="_blank" rel="noreferrer" x-show="auditData?.overview?.url" x-text="'View run ↗'"></a> |
There was a problem hiding this comment.
Unvalidated URL from audit JSON used in :href — Alpine's :href binding does not sanitize URLs, so a (redacted) scheme in auditData.overview.urlwould execute when clicked. Whilegh aw audit` output is local, audit data ultimately reflects workflow run metadata which can be influenced by repository content.
Add a URL scheme guard before binding:
auditRunUrl() {
const url = this.auditData?.overview?.url ?? '';
return /^https?:\/\//i.test(url) ? url : '#';
},Then use :href="auditRunUrl()" and :show="auditRunUrl() !== '#'" in the template.
@copilot please address this.
| <div class="d-flex flex-items-center gap-2 mb-2"> | ||
| <button class="btn btn-sm" @click="loadAudit()" :disabled="loadingAudit" x-show="!auditData && !loadingAudit"> | ||
| Run Audit | ||
| </button> |
There was a problem hiding this comment.
btn-danger (red) for a reversible action — "Clear audit" only clears in-memory display state; the user can re-run the audit at any time. Using btn-danger implies a destructive or irreversible operation (matching GitHub's own Primer convention). A plain btn btn-sm or btn-outline btn-sm would match the action's low-risk nature and avoids alarming the user.
@copilot please address this.
| z-index: 1; | ||
| } | ||
|
|
||
| details > summary { |
There was a problem hiding this comment.
Global details > summary selector — This suppresses the native disclosure triangle on all <details> elements in the app, not just audit ones. Currently there are no other <details> usages, but this will silently affect any future additions.
Note also that each <summary> in index.html already has style="list-style:none" inline, making the list-style: none rule here redundant. The ::-webkit-details-marker rule is still useful for old WebKit.
Consider scoping to the audit container:
.awd-audit details > summary { list-style: none; }
.awd-audit details > summary::-webkit-details-marker { display: none; }@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs, /tdd, and /zoom-out — requesting changes on 6 issues. The core feature is solid and well-structured; the asks are targeted polish before merge.
📋 Key Themes & Highlights
Issues to address
- UX correctness — Hardcoded
▾chevron never flips on collapse (all 6<summary>elements) — simple CSS fix - Architecture scope —
details > summaryCSS rule is page-global instead of scoped to the audit panel - Test coverage — 4 new pure helper methods and the
loadAuditerror path are untested - Error context —
runGhAwfailure insidegetAuditbubbles up without the run ID that caused it - Fallback correctness —
auditSeverityClassunknown-severity fallback returnsLabel--accent(blue) instead of neutralLabel--secondary - UX consistency — Empty-state treatment differs between sections: Findings/Recommendations show "No X" while Jobs/Tool Usage/Errors hide entirely
Positive highlights
- ✅ Clean layering:
getAuditin data layer → endpoint in server → state + methods in Alpine → markup in HTML - ✅ JSON parse error message includes a truncated snippet — great diagnostics
- ✅
selectRun()clears stale audit state on navigation — no stale-data bugs - ✅
loadingAuditreset infinally— correct async state management - ✅ Canvas action schema is well-typed with
additionalProperties: false - ✅ The progressive-disclosure structure (Overview open, rest collapsed) is thoughtful UX
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68.6 AIC · ⌖ 9.07 AIC · ⊞ 6.6K
Comment /matt to run again
| <div x-show="auditData"> | ||
| <!-- Overview --> | ||
| <details class="mb-2" open> | ||
| <summary class="btn-link text-bold color-fg-default mb-1" style="cursor:pointer;list-style:none">▾ Overview</summary> |
There was a problem hiding this comment.
[/grill-with-docs] The ▾ chevron is hardcoded text and never flips — users who collapse a section will still see ▾ ("open" indicator), making the toggle state ambiguous.
💡 Suggested CSS-driven fix
Remove the ▾ prefix from every <summary> element and drive the indicator in CSS:
/* styles.css — replaces hardcoded ▾ in markup */
.awd-audit-section details > summary::before {
content: "▾";
display: inline-block;
margin-right: 4px;
transition: transform 0.15s ease;
}
.awd-audit-section details:not([open]) > summary::before {
transform: rotate(-90deg);
}This keeps the visual state in sync with the <details> open/closed attribute automatically, with no JS needed.
@copilot please address this.
| z-index: 1; | ||
| } | ||
|
|
||
| details > summary { |
There was a problem hiding this comment.
[/zoom-out] details > summary is a page-global rule — it will silently suppress native markers on any <details> element added elsewhere in the dashboard in the future, not just the audit panel ones.
💡 Suggested fix — scope the rule
Wrap the audit section in a container with a scoped class and target that instead:
<!-- index.html: add class to the audit wrapper -->
<div class="border-top pt-3 awd-audit-section">/* styles.css */
.awd-audit-section details > summary {
list-style: none;
}
.awd-audit-section details > summary::-webkit-details-marker {
display: none;
}This is the same selector scope used by the existing .awd-tool-usage-header / .awd-tool-usage-row classes — consistent and contained.
@copilot please address this.
| this.setActiveTab("details"); | ||
| }, | ||
|
|
||
| async loadAudit() { |
There was a problem hiding this comment.
[/tdd] loadAudit(), auditHasFindings(), auditSeverityClass(), and auditPriorityClass() are all untested. The severity/priority class mapping is particularly prone to silent failures if the audit API returns an unexpected value.
💡 What to test
The pure helpers are easy to unit test without a browser:
// auditSeverityClass
it('returns danger class for critical severity', () => {
expect(app.auditSeverityClass('critical')).toBe('Label Label--danger');
});
it('returns secondary for unknown severity', () => {
// catches the fallback 'Label--accent' issue (see related comment)
expect(app.auditSeverityClass('info')).toBe('Label Label--secondary');
});
// loadAudit error path
it('sets errorAudit on HTTP error', async () => {
global.fetch = vi.fn().mockResolvedValue({ ok: false, status: 503, json: async () => ({}) });
await app.loadAudit();
expect(app.errorAudit).toMatch(/503/);
expect(app.loadingAudit).toBe(false);
});The existing test file for app.js (if one exists) is the right place to add these. If there is no test file yet, this PR is a good time to start one.
@copilot please address this.
| const hit = getCached(key); | ||
| if (hit) return hit; | ||
|
|
||
| const raw = await runGhAw(["audit", String(runId), "--json"]); |
There was a problem hiding this comment.
[/tdd] If runGhAw throws (e.g., the CLI exits non-zero), the raw error propagates without identifying which run was being audited. Only the JSON parse failure path adds run ID context — the process failure path should too.
💡 Suggested fix
async function getAudit(runId) {
if (!runId) return null;
const key = `audit:${runId}`;
const hit = getCached(key);
if (hit) return hit;
let raw;
try {
raw = await runGhAw(["audit", String(runId), "--json"]);
} catch (err) {
throw new Error(`gh aw audit failed for run ${runId}: ${err.stderr ?? err.message}`);
}
let data;
try {
data = JSON.parse(raw);
} catch (error) {
// ... existing snippet logic
}
setCached(key, data);
return data;
}This makes error messages actionable in the dashboard's errorAudit state and in server logs.
@copilot please address this.
| if (severity === "critical" || severity === "high") return "Label Label--danger"; | ||
| if (severity === "medium") return "Label Label--attention"; | ||
| if (severity === "low") return "Label Label--secondary"; | ||
| return "Label Label--accent"; |
There was a problem hiding this comment.
[/grill-with-docs] The fallback "Label Label--accent" fires for any unrecognized severity value and renders a blue accent badge, which can be misread as "informational" rather than "unknown". A neutral secondary badge is safer as a catch-all.
💡 Suggested fix
auditSeverityClass(severity) {
if (severity === "critical" || severity === "high") return "Label Label--danger";
if (severity === "medium") return "Label Label--attention";
if (severity === "low") return "Label Label--secondary";
return "Label Label--secondary"; // unknown/undefined → neutral, not accent
},If info or other levels are intentional additions from gh aw audit --json, document the full set of valid severity values in a JSDoc comment here so reviewers can verify the mapping is complete.
@copilot please address this.
| </details> | ||
|
|
||
| <!-- Jobs --> | ||
| <details class="mb-2" x-show="(auditData?.jobs ?? []).length > 0"> |
There was a problem hiding this comment.
[/grill-with-docs] Jobs, Tool Usage, and Errors & Warnings use x-show to hide entirely when empty, while Key Findings and Recommendations show explicit "No X" messages. This inconsistency leaves users wondering whether those sections returned empty data or simply were not included in the audit response.
💡 Suggested approach — pick one pattern and apply it consistently
Option A (preferred — matches Findings/Recommendations): always show the section header and add an empty-state message:
<!-- Jobs: remove x-show from <details>, add empty state inside -->
<details class="mb-2">
<summary ...>▾ Jobs <span class="Counter" x-text="..."></span></summary>
<div class="Box mt-1" x-show="(auditData?.jobs ?? []).length > 0">
<!-- existing template loop -->
</div>
<div class="color-fg-muted text-small mt-1" x-show="(auditData?.jobs ?? []).length === 0">No jobs.</div>
</details>Repeat for Tool Usage and Errors & Warnings. This makes it clear when a section has data vs when the audit found nothing.
Option B: hide all empty sections with x-show (simpler, but loses the "no findings" signal). Remove the "No key findings" and "No recommendations" empty-state divs.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Two correctness bugs must be fixed before merge; three medium issues also filed.
### Blocking issues (must fix)
1. Stuck loadingAudit spinner on navigation — selectRun() resets auditData and errorAudit but not loadingAudit. If the user selects a different run while an audit fetch is in-flight, the new run's details panel shows a perpetual "Running audit..." indicator with no way to dismiss it. See inline comment on selectRun.
2. Stale audit data race condition — loadAudit() captures this.selectedRun.run_id at call time but writes results back via this.auditData after await. If the user switches runs while the fetch is pending, the old result overwrites the new run's state. See inline comment on the loadAudit() body.
### Medium issues (filed inline)
resp.json()beforeresp.okcheck — non-JSON error bodies (e.g., HTML 500 page) surface as a confusingSyntaxErrorinstead of the HTTP status code. Defensive JSON parse inside its own try/catch fixes this.- Unsanitized
:hreffrom audit URL — Alpine's dynamichrefbinding accepts any URL scheme; a `(redacted) URI in audit output would execute on click. A one-line scheme regex guard eliminates the class. auditRuncanvas handler returnsnullsilently for empty/invalidrun_id— schema marksrun_idrequired but the handler short-circuits tonullrather than throwing an actionable error.
### Non-blocking observations
- CSS: Firefox still shows the native disclosure marker alongside the custom
▾because::markeris not suppressed — only::-webkit-details-marker. Addingsummary::marker { display: none }orlist-style: noneat the::markerlevel fixes this cross-browser. - The hardcoded
▾arrow in every<summary>does not toggle to▴when the section is open, giving no visual open/closed state feedback since the native arrow is suppressed.
🔎 Code quality review by PR Code Quality Reviewer · 188 AIC · ⌖ 12.1 AIC · ⊞ 5.2K
Comment /review to run again
Comments that could not be inline-anchored
.github/extensions/agentic-workflows-dashboard/web/app.js:291
resp.json() can throw before resp.ok is checked, masking the HTTP error status: when the server returns a non-JSON body (e.g., an HTML 500 from an unexpected crash), resp.json() throws a SyntaxError and the catch block sets errorAudit to "Audit failed: Unexpected token <" — losing the actual HTTP status entirely.
<details>
<summary>💡 Suggested fix</summary>
Parse JSON defensively so the HTTP status is always surfaced:
const resp = await fetch(`/api/audit?${params.toS…
</details>
<details><summary>.github/extensions/agentic-workflows-dashboard/web/index.html:217</summary>
**Alpine's `:href` binding does not sanitize URL schemes**: `auditData?.overview?.url` is bound directly to `href` without validating the scheme. If the audit CLI output ever contains a `(redacted) or `(redacted) URI in the `url` field (e.g., due to malformed audit data), clicking the link executes arbitrary JavaScript in the dashboard context.
<details>
<summary>💡 Suggested fix</summary>
Add a scheme guard — either inline or as a helper method:
```js
// in app.js
safeAuditUrl() {
const u…
</details>
<details><summary>.github/extensions/agentic-workflows-dashboard/web/app.js:180</summary>
**`fetchRuns()` bypasses `selectRun()`, leaving stale audit data visible for the wrong run**: the run-refresh path re-assigns `this.selectedRun` in-place (e.g., by finding the same run in the updated list or falling back to `runs[0]`) without going through `selectRun()`, which is the only place that clears `auditData` and `errorAudit`. Concretely: open run A's details with audit loaded, hit Refresh while run A drops off the list, the fallback picks run B — but the audit panel still shows run A'…
</details>
<details><summary>.github/extensions/agentic-workflows-dashboard/dashboard-data.mjs:212</summary>
**CLI error payload (exit 0, `{"error":"..."}`) passes JSON parsing, gets cached, and is returned as HTTP 200 with no error surfaced in the UI**: `setCached(key, data)` runs whenever `JSON.parse` succeeds, regardless of whether the payload contains an application-level error. If `gh aw audit` exits with code 0 but emits `{"error":"run not found"}`, the response is cached for the full TTL and served as a 200 response. The client's `if (!resp.ok)` guard never fires, so `this.auditData` is set to …
</details>
<details><summary>.github/extensions/agentic-workflows-dashboard/dashboard-data.mjs:202</summary>
**`runId` is passed as a positional argv entry before any `--` end-of-options sentinel, so a value like `"--help"` or `"--timeout=0"` is interpreted as a CLI flag, not a run ID**: `runGhAw(["audit", String(runId), "--json"])` places the run ID at `argv[1]`, before the double-dash that would terminate option parsing. `execFile` prevents shell injection, but flag injection via argv is still possible. Via the canvas `auditRun` action, an LLM agent can supply any string as `run_id`.
<details>
<sum…
</details>
|
@copilot run pr-finisher skill |
…alidation, CSS marker, noopener Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All review comments addressed in commit
Local validation (typecheck + tests) passes. CI will need a human re-trigger to confirm on the updated HEAD. |
|
@copilot run pr-finisher skill |
|
@copilot merge main and recompile |
All code changes are already in place from the previous pass (commit
Hand-off: A maintainer needs to re-trigger CI (close/reopen PR or |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit |
|
🎉 This pull request is included in a new release. Release: |
The Run details tab showed only summary metrics from logs. This adds on-demand
gh aw auditintegration so users can drill into structured audit output for any selected run without leaving the dashboard.Backend
dashboard-data.mjs—getAudit(runId): callsgh aw audit <id> --json, parses and caches result per run IDextension.mjs—/api/audit?run_id=endpoint +auditRuncanvas action (acceptsrun_id: string); system message updated to document the actionFrontend
app.js—auditData/loadingAudit/errorAuditstate;loadAudit(),clearAudit(),auditHasFindings(),auditSeverityClass(),auditPriorityClass()methods;selectRun()clears stale audit on navigationindex.html— Run details tab gains a Run Audit button below the existing metric cards; results render as<details>/<summary>sections:styles.css— tool usage grid layout;<details>native marker suppressed for consistent▾arrow