Repository navigation
GLOOK-59: unify run and reporting UX for GitHub reports and Dependabot alerts - #77
Conversation
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Presentational run UI components for the reports and vulnerability syncs tabs: RunStatusChip, RunHealthBadge, RunCard (with progress/logs block), and RunsToolbar. Built on Task 1's status/format/health helpers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…list Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add org to listSyncs available response and ListSyncsResponse type, and convert SyncCard into a thin adapter over RunCard using fromSyncStatus, triggerLabel, formatRunTime, and syncHealth. Drops the now-unused local STATUS_COLOR/STATUS_BG/duration/triggerLabel/startTime helpers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… inline errors Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nts, fix completed-progress pct order - Add a `finishedFired: Set<string>` owned by ReportsTabs (alongside `observedRunning`) and threaded into ReportsTab, so the finish side effect (list mutate + global SWR cache bust) runs at most once per report id. Without it, switching to the syncs tab and back remounted every observed-finished card with a fresh in-hook ref, re-firing the global cache bust on every return to the reports tab. - Fix progress `pct`: check `status === 'completed'` before the completed/total ratio, so a completed run with skipped developers reads 100% instead of the raw ratio, per the brief. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…bility dashboards Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…es; docs Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…om badges row, PageHeader child spacing, NavBar sync-date timezone - use-report-progress.ts / reports-tab.tsx: re-arm the progress hook's once-guard and force a real revalidation of the progress SWR key when a watched card's status transitions back into running/pending (Resume succeeding, or a new run reusing the id) — previously the stale cached stopped/failed data kept refreshInterval at 0 and both once-guards (firedOnce ref, finishedFired set) silenced the finish side effect forever. - DataFreshness.tsx / vulnerabilities-content.tsx: add a bannerOnly render mode so the failed-sync banner renders full-width in PageHeader's children instead of being squeezed into the no-wrap freshness/badges flex row. - PageHeader.tsx: give the children slot top spacing (mt-3) so banners/notices don't sit flush against the title row. - NavBar.tsx: pass timeZone: 'America/New_York' on the vulnerability last-sync date to match formatRunTime. - Test-only: app-config-freshness.test.ts covers a stopped latest run (latestRunFailed: false) — already correct in getReportFreshness, no production change needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…es → Security Brings in GLOOK-58 (Recharts migration) and GLOOK-60 (stable vulnerabilities layout). Conflicts resolved by keeping main's layout work and reapplying GLOOK-59's "Owning team" labels and accent tokens on top; the Runs/syncs tab kept GLOOK-59's shared RunCard, which now carries main's bg-chart-track bar colour. Naming: nav "Runs" → "Reports", page tabs "Commits & PRs" (GitHub · Jira) and "Dependabot alerts" (GitHub), "Sync now" → "Sync alerts", dashboard "Vulnerabilities" → "Security", Settings row "Dependabot alerts sync". URLs unchanged. docker-compose: pass the VULNERABILITIES_ORG / VULN_* variables through to the app container (empty values fall back to module defaults). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… report schedules
The sync was scheduled by its own env-driven cron (VULN_SYNC_CRON) and only
shown read-only in Settings. It is now one `kind = 'vuln_sync'` row in the
shared `schedules` table, run by the same scheduler manager as report
schedules, and edited the same way: cadence, timezone, pause/resume, last
and next run.
- schedules.kind ('report' default | 'vuln_sync') on MySQL and SQLite.
- The manager dispatches vuln_sync rows to the module's startSync (sync
semantics untouched) and skips them while the feature is off.
- VULN_SYNC_CRON/TZ only seed the row on first boot; afterwards the row is
the source of truth, so a Settings edit survives restarts and deploys.
- The row can't be deleted (pause is the off switch) and has no org, period
or test-mode fields; its Last Run comes from the latest sync.
- Settings → Schedules gains a Type column; the read-only row is removed.
The schedules tab moved to settings/schedules-tab.tsx so it can be tested.
- The Dependabot alerts tab shows "Schedule paused" when paused and a
Manage schedules link; the Commits & PRs "Next scheduled" line ignores
the alerts row.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
msogin
left a comment
There was a problem hiding this comment.
review-loop (1 loop · Sr. Architect + Correctness, Sr. Frontend/Test Eng + Gaps)
Verdict: safe to merge once the two Important items are fixed. No Critical findings.
| # | Issue | Triage |
|---|---|---|
| I1 | Report freshness counts the vuln_sync schedule cadence |
Fix |
| I2 | New run/stop/delete flow untested | Fix |
| S1 | ensureVulnScheduleRow race / no unique constraint |
Defer |
| S2 | vuln_sync PUT registers even when the feature is off | Ask |
| S3 | last_report_status carries sync statuses |
Ask |
| S4 | Progress hooks retry forever on 404 | Ask |
| S5 | Pending reports can't be stopped | Ask |
| S6 | RunCard header not keyboard-accessible | Defer |
Deferred and not posted inline (they pre-date this PR): the reports list shows the empty state during loading and on error; a failed stats fetch shows "Loading stats..." forever; the New Report modal lacks dialog a11y. Note for the release notes: after first boot, changes to VULN_SYNC_CRON/VULN_SYNC_TZ are ignored.
Findings outside the diff hunks
src/lib/vulnerabilities/scheduler.ts:746: [review-loop · Suggestion · Defer]ensureVulnScheduleRowis read-then-insert with no unique constraint, and the init guard is set after the awaits. Concurrent inits (multi-instance / HMR) could create twovuln_syncrows, both registered, whilegetVulnSchedule(LIMIT 1, no ORDER BY) shows one. Consider a fixed id +INSERT IGNORE/INSERT OR IGNORE, or a unique index.src/lib/schedule/service.ts:587: [review-loop · Suggestion · Question] Thevuln_syncbranch registers the cron job even when vulnerabilities are disabled. Ticks just return'disabled', so it's harmless, but it's inconsistent withinitScheduler, which skips the row. Skip registration (or reject the PUT) when the feature is off?src/lib/schedule/service.ts:566: [review-loop · Suggestion · Question] For thevuln_syncrow,last_report_statusgets sync statuses (succeeded/partial), not report statuses. Can you confirmschedules-tab.tsxmaps this viafromSyncStatusfor that kind? Otherwise consider a separate field.
| `SELECT created_at FROM reports WHERE status = 'completed' ORDER BY created_at DESC LIMIT 1`) as [any[], any]; | ||
| const [latest] = await db.execute( | ||
| `SELECT status FROM reports WHERE status IN ('completed','failed','stopped') ORDER BY created_at DESC LIMIT 1`) as [any[], any]; | ||
| const [schedules] = await db.execute(`SELECT cron_expr, timezone, enabled FROM schedules`) as [any[], any]; |
There was a problem hiding this comment.
[review-loop · Important · Fix] getReportFreshness reads all schedules, which now includes the kind='vuln_sync' row. staleAfterMs takes the tightest cadence, so a daily vuln sync makes a weekly GitHub report look stale after ~1.5 days (and makes reports go stale even with no report schedule). Every other reader in this PR filters by kind.
Suggested: SELECT cron_expr, timezone, enabled FROM schedules WHERE kind = 'report'.
There was a problem hiding this comment.
Fixed in 583e075 — getReportFreshness now reads WHERE kind = 'report'. Test: app-config-freshness.test.ts (weekly report + daily vuln_sync → not stale); confirmed it fails without the filter.
| globalMutate(() => true, undefined, { revalidate: true }); | ||
| } | ||
|
|
||
| async function handleRun(e: React.FormEvent) { |
There was a problem hiding this comment.
[review-loop · Important · Fix] The rewritten run flow (handleRun / stopReport / deleteReport) is untested. Progress now depends on mutate() → list returns pending/running row → observedRunning effect → progress hook. A regression would leave new runs without live progress and no test would fail. Only the resume error path is covered.
Suggested tests in reports-tab.test.tsx: (a) submit → pending row → progress block renders and /api/report/<id>/progress fetched; (b) non-ok POST/stop/delete shows inline error; (c) delete confirm mutates the list.
There was a problem hiding this comment.
Added in 583e075, reports-tab.test.tsx: (a) submit → list returns the pending row → /api/report/r9/progress polled and the progress block renders; (b) failed start / stop / delete each show the server error inline (no alert); (c) confirming delete removes the row.
| fetcher, | ||
| { | ||
| dedupingInterval: 1000, | ||
| onErrorRetry: (_err, _key, _config, revalidate, opts) => { setTimeout(() => revalidate(opts), 1500); }, |
There was a problem hiding this comment.
[review-loop · Suggestion · Question] onErrorRetry retries every 1.5s forever, including on 404 (for example, a report deleted while it's watched). Same pattern in useSyncProgress. Is this intended? If not, add if (err.status === 404) return; in both hooks.
There was a problem hiding this comment.
Not intended — fixed in 583e075. Both useReportProgress and the syncs tab's steadyRetry now stop on a 404. Test: progress-hooks-404.test.tsx (fails without the guard).
| Resume | ||
| </button> | ||
| )} | ||
| {r.status === 'running' && ( |
There was a problem hiding this comment.
[review-loop · Suggestion · Question] Stop only shows for status === 'running', but pending counts as running elsewhere (New/Resume are blocked), so a pending report can't be stopped from the UI. Use isRunningNow here?
There was a problem hiding this comment.
Yes — fixed in 583e075. The card uses isRunningNow (running or pending) for Stop, and stopReport now accepts a pending report too (it previously threw ReportNotRunningError). Tests in reports-tab.test.tsx and report-service.test.ts.
| export default function RunCard(p: RunCardProps) { | ||
| return ( | ||
| <div className={`group bg-gray-900 border rounded-xl transition-colors ${p.expanded ? 'border-gray-700' : 'border-gray-800 hover:border-gray-700'}`}> | ||
| <div className={`flex items-center justify-between p-4 ${p.expandable ? 'cursor-pointer' : ''}`} |
There was a problem hiding this comment.
[review-loop · Suggestion · Defer] The expandable header is a clickable div with no role="button"/tabIndex/key handler/aria-expanded, so keyboard users can't expand cards. This pre-dates the PR, but now it's one shared component, so one fix covers reports and syncs. Actions are also hover-only (opacity-0 group-hover), which hurts touch devices.
There was a problem hiding this comment.
Fixed here rather than deferred, in 583e075: the expandable header is role=button, tabIndex=0, aria-expanded, toggles on Enter/Space (keys on an action button stay the action's own), with a focus ring. Actions are always visible below md and hover/focus-revealed above it. Tests in run-card.test.tsx.
msogin
left a comment
There was a problem hiding this comment.
Smartling review
Ready to merge? With fixes.
The freshness query not filtering by kind will mislabel reports as stale whenever vuln sync runs more often than reports (a one-line fix). The duplicate-seed race for the vuln_sync row should get a uniqueness guarantee. Everything else is minor.
Recommendations
schedulesnow holds two kinds of row, so every consumer that reads it should filter bykindexplicitly.- Add tests showing report staleness ignores
vuln_sync, and that seeding thevuln_syncrow is idempotent.
See the inline comments for details (3 warnings, 2 suggestions, 1 question).
| `SELECT created_at FROM reports WHERE status = 'completed' ORDER BY created_at DESC LIMIT 1`) as [any[], any]; | ||
| const [latest] = await db.execute( | ||
| `SELECT status FROM reports WHERE status IN ('completed','failed','stopped') ORDER BY created_at DESC LIMIT 1`) as [any[], any]; | ||
| const [schedules] = await db.execute(`SELECT cron_expr, timezone, enabled FROM schedules`) as [any[], any]; |
There was a problem hiding this comment.
🟡 warning
getReportFreshness queries schedules without a kind filter, so the vuln_sync row (often daily) drives staleAfterMs. Because the tightest schedule wins, weekly report schedules will show as stale after ~36h.
Fix: add WHERE kind = 'report'.
There was a problem hiding this comment.
Fixed in 583e075 — same change (WHERE kind = 'report'), see the reply above.
| export function getNextSyncRun(): string | null { | ||
| const next = g.__glooker_vuln_job?.nextRun(); | ||
| return next ? toIsoSecond(next) : null; | ||
| async function ensureVulnScheduleRow(org: string): Promise<Schedule> { |
There was a problem hiding this comment.
🟡 warning
ensureVulnScheduleRow checks for the row and then inserts it, with no uniqueness guarantee. Concurrent instances or HMR double-init can create duplicate vuln_sync rows. Every later read uses LIMIT 1 with no ORDER BY, so Settings may edit a different row from the one that fires.
Fix: give the row a fixed id or a unique constraint, and use INSERT IGNORE / ON CONFLICT DO NOTHING.
There was a problem hiding this comment.
Fixed in 583e075: the row has a fixed id (VULN_SCHEDULE_ID = 'vuln-sync') and is seeded with INSERT IGNORE (SQLite translator → INSERT OR IGNORE), so concurrent seeds collapse into one row. All readers now ORDER BY created_at, id, so they agree even if an older row exists. Test: concurrent inits → one row with the fixed id.
| const { cron, tz } = getSyncSchedule(); | ||
| const id = uuidv4(); | ||
| await db.execute( | ||
| `INSERT INTO schedules (id, org, period_days, cron_expr, timezone, enabled, test_mode, kind) |
There was a problem hiding this comment.
🟡 warning
org is seeded from VULNERABILITIES_ORG on first boot and never updated, while listSchedules/listSyncs read the current env org. If the env org changes, the stored row goes stale.
Fix: sync org on init, or don't store it for vuln_sync.
There was a problem hiding this comment.
Fixed in 583e075 — init updates the row's org to the current VULNERABILITIES_ORG on every boot. Test in vuln-schedule-unified.test.ts.
| [cronExpr, timezone, enabled ? 1 : 0, id], | ||
| ); | ||
| const row = { ...existing[0], cron_expr: cronExpr, timezone, enabled: enabled ? 1 : 0 }; | ||
| if (enabled) registerSchedule(row); else unregisterSchedule(id); |
There was a problem hiding this comment.
🔵 suggestion
updateSchedule registers the vuln_sync cron even when the feature is disabled. initScheduler skips the row in that case, so the two are inconsistent. Fix: guard with isVulnerabilitiesEnabled().
There was a problem hiding this comment.
Fixed in 583e075 — updateSchedule only registers a vuln_sync job when isVulnerabilitiesEnabled(); the edit itself is still saved. Test in vuln-schedule-unified.test.ts.
| for (const schedule of rows) { | ||
| // A vuln_sync row only runs while the feature is enabled; its own init seeds and | ||
| // registers it (initVulnerabilityScheduler), so skip it here when the feature is off. | ||
| if (schedule.kind === 'vuln_sync' && !(await vulnerabilitiesEnabled())) continue; |
There was a problem hiding this comment.
🔵 suggestion
vulnerabilitiesEnabled() (a dynamic import) is awaited once per row inside the loop. Compute it once before the loop.
There was a problem hiding this comment.
Fixed in 583e075 — computed once before the loop.
|
|
||
| async function del(id: string) { | ||
| try { | ||
| await fetch(`/api/schedule/${id}`, { method: 'DELETE' }); |
There was a problem hiding this comment.
🟣 question
del/toggle ignore res.ok. A 400 from ScheduleNotDeletableError would be swallowed silently. Should these handlers surface server errors the way save() does?
There was a problem hiding this comment.
Yes — fixed in 583e075. del and toggle now check res.ok and surface the server's error the way save() does (so ScheduleNotDeletableError is shown). Test in settings-schedules-tab.test.tsx.
- Report freshness reads only kind = 'report' schedules; a daily Dependabot alerts schedule no longer makes weekly reports look stale. - The vuln_sync schedule row has a fixed id and is seeded with INSERT IGNORE, so concurrent boots can't create duplicates; readers order deterministically. Its org follows VULNERABILITIES_ORG on every boot. - Editing the vuln_sync row while the feature is off saves it but registers no job (same rule as initScheduler). The feature check is hoisted out of the boot loop. - Pending reports can be stopped (UI and service). - Progress polling stops retrying on 404 (report/sync no longer exists). - RunCard header is keyboard-operable (role=button, tabIndex, Enter/Space, aria-expanded); actions are always visible on small screens. - Settings: delete/toggle surface server errors instead of swallowing them. - Tests: the start → pending row → live progress flow, inline errors for failed start/stop/delete, delete confirm, plus a test per fix above. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review addressed in 583e075. Every inline comment has a reply. Summary:
Still open (they predate this PR): the reports list shows its empty state while loading and on error; a failed stats fetch shows "Loading stats…" forever; the New Report modal lacks dialog a11y. Release note: after first boot, changes to
|
Closes GLOOK-59.
GitHub reports and the vulnerability (Dependabot alerts) sync from GLOOK-43 share one lifecycle — a scheduled or manual pull, live progress, run history, a dashboard over the latest good data — but had drifted into two separate UX patterns and two copies of the run-card code. This makes them structurally consistent. Pipeline and sync semantics are unchanged; the vulnerability module's guardrails (two-phase sync, completeness guard, no team join) are untouched.
What changes for users
/reports, URLs unchanged): titled "Reports", with tabs Commits & PRs (GitHub · Jira) and Dependabot alerts (GitHub). Both tabs render through one shared run card: status, subject, trigger (Scheduled / Manual · email), duration, health badge, live progress and logs. Each tab has its own schedule line and primary action (+ New report / Sync alerts). Report action failures show inline instead ofalert().teamcustom property is labelled Owning team so it isn't confused with Glooker teams. Vulnerability pages use the theme accent instead of hard-coded indigo.Implementation notes
src/lib/runs/— shared run model (status mapping, formatting, health, staleness).src/components/runs/RunCard.tsx— the one card, with thin adapters for reports and syncs.reports.trigger_kind/triggered_by(nullable; NULL on pre-existing rows, which show no trigger). The report list returns a compacthealthsummary instead of rawrun_metadata.schedules.kind('report'default |'vuln_sync'). The scheduler manager dispatchesvuln_syncrows to the module's existingstartSync.VULN_SYNC_CRON/VULN_SYNC_TZnow only seed the row on first boot; afterwards Settings is the source of truth./api/llm-configaddsreportFreshness/vulnerabilityFreshness. Report staleness = the largest gap between upcoming scheduled runs + 12h (no enabled schedule → never stale); sync staleness keeps the 36h threshold.docker-compose.ymlnow passes theVULNERABILITIES_ORG/VULN_*variables through to the app container.Decisions for review
src/lib/vulnerabilities/scheduler.ts— @maescomua please review.staleAfterMs).Testing
npx jest: 207 suites / 2042 tests passing;tsc --noEmitclean; production build succeeds.kindcolumn migrated, the alerts schedule seeded once (no duplicate on restart), existing report schedules and reports intact, all pages load.Note: locally,
npm ci --legacy-peer-depsdrops the@testing-library/dompeer and breaks jsdom suites; CI runs plainnpm ciand is unaffected.🤖 Generated with Claude Code