feat(reports): scheduled analytics reports with trend snapshots (#7)#174
feat(reports): scheduled analytics reports with trend snapshots (#7)#174LeoLin990405 wants to merge 1 commit into
Conversation
…gsonww#7) Recurring HTML/JSON reports over a configured time window, generated on a schedule, with a run history of downloadable artifacts. Addresses hoangsonww#7. Zero new dependencies. Backend (/api/reports): - Report definitions (template, frequency, hour, timezone, formats, window) and run logs persisted in two additive tables; both survive Clear Data like alert_rules. - Generator reuses the same analytics computations as /api/analytics, windowed to the report period (session volume trend, daily events, agent status distribution, top tools + failure-prone operations, token usage + cost via calculateCost), so the numbers are consistent with the in-app charts by construction. Window bounds are half-open [start, end) with the same tz_offset modifier convention as analytics. - HTML artifact is a self-contained, print-friendly document (inline @media print) — "Save as PDF" works from the browser with no headless-Chrome dependency. JSON artifact is the structured data. Every dynamic value in the HTML is escaped. - Scheduler: an unref'd, env-gated (DASHBOARD_REPORTS_DISABLED) periodic tick that runs due definitions (next_run_at-driven), persists each run, advances the schedule on success AND error (so a failing def can't wedge the tick), and broadcasts report_run. A failed generation becomes an error-status run with an actionable error string — never an unhandled throw. window_days is bounded (validator + the window computation is inside the run try) so an out-of-range value degrades to an error run instead of a RangeError. - Run-now endpoint + artifact download endpoint (correct Content-Type, 404 for an ungenerated format). All queries parameterized. Frontend: - Reports page: definition list with human schedule + next-run + enabled toggle + last-status, create/edit modal (template, frequency, day-of-week for weekly, hour, formats, window), Run now, and per-definition run history with status/error + View HTML (new tab → print-to-PDF) / Download JSON. Full en/zh/vi i18n. Tests: server/__tests__/reports.test.js (20) — definition CRUD + validation (incl. the window_days bound), computeNextRun determinism, windowed-number consistency with the analytics-style queries, on-demand run + artifact download + Content-Type/404, error-run capture (no crash), the out-of-range window_days degrade-to-error path, runDueReports due/not-due selection, and HTML escaping of a hostile session name.
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for scheduled analytics reports, including database schema updates, an Express router for managing report definitions and runs, a background scheduler, and a frontend management UI. The implementation includes robust error handling for report generation and artifact management. My review identified a high-severity input validation issue regarding timezone offsets, a missing foreign key constraint for referential integrity, and a need for database transactions during deletion to ensure atomicity.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!Number.isInteger(tz_offset)) { | ||
| return { ok: false, error: "tz_offset must be an integer (minutes from UTC)" }; | ||
| } |
There was a problem hiding this comment.
The tz_offset parameter lacks range validation. If an extremely large integer is passed (e.g., via direct API request), computeNextRun will throw a RangeError when creating a new Date object. Because computeNextRun is called outside the try/catch block in runReportForDefinition, this will throw an unhandled error, preventing touchAfterRun from advancing the schedule. Consequently, the scheduler will repeatedly attempt to run the same failing definition on every tick, causing an infinite loop of failures. Adding a reasonable range check (e.g., between -1440 and 1440 minutes) prevents this.
| if (!Number.isInteger(tz_offset)) { | |
| return { ok: false, error: "tz_offset must be an integer (minutes from UTC)" }; | |
| } | |
| if (!Number.isInteger(tz_offset) || tz_offset < -1440 || tz_offset > 1440) { | |
| return { ok: false, error: "tz_offset must be an integer between -1440 and 1440" }; | |
| } |
| created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | ||
| ); |
There was a problem hiding this comment.
The report_runs table references report_definitions(id) via definition_id but does not define a foreign key constraint. Adding a foreign key constraint with ON DELETE CASCADE ensures referential integrity at the database level and automatically cleans up associated runs when a definition is deleted.
| created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | |
| ); | |
| created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), | |
| FOREIGN KEY (definition_id) REFERENCES report_definitions(id) ON DELETE CASCADE | |
| ); |
| stmts.deleteRunsForDef.run(req.params.id); | ||
| stmts.deleteDef.run(req.params.id); | ||
| res.json({ ok: true }); |
There was a problem hiding this comment.
Deleting a report definition involves executing two separate delete queries (deleting associated runs and then deleting the definition itself). These queries should be wrapped in a database transaction to ensure atomicity and prevent orphaned records if one of the operations fails.
db.transaction(() => {
stmts.deleteRunsForDef.run(req.params.id);
stmts.deleteDef.run(req.params.id);
})();
res.json({ ok: true });
Summary
Implements Scheduled Analytics Reports (issue #7): recurring HTML/JSON reports over a configured time window, generated on a schedule, with a run history of downloadable artifacts. Trend numbers reuse the same computations as the in-app Analytics page (windowed), so they're consistent by construction. Zero new dependencies. Closes #7.
Changes
Backend (
/api/reports)alert_rules./api/analyticscomputations, windowed to the report period — session volume trend, daily events, agent status distribution, top tools + failure-prone operations, token usage + cost (viacalculateCost). Window bounds are half-open[start, end)with the sametz_offsetmodifier convention as analytics.@media print), so Save-as-PDF works from the browser with no headless-Chrome dependency (I flagged this in the issue — happy to wire a real server-side PDF lib if you'd prefer the dep). Every dynamic value in the HTML is escaped.unref'd, env-gated (DASHBOARD_REPORTS_DISABLED)next_run_at-driven tick that runs due definitions, persists each run, advances the schedule on success and error (a failing def can't wedge the tick), and broadcastsreport_run. A failed generation becomes anerror-status run with an actionable error string — never an unhandled throw.Content-Type, 404 for an ungenerated format). All queries parameterized;window_daysbounded so an out-of-range value degrades to an error run instead of aRangeError.Frontend
Type of Change
How to Test
npm run test:server— all pass (364), incl.server/__tests__/reports.test.js(20 tests: CRUD/validation,computeNextRundeterminism, windowed-number consistency with analytics, run + artifact download, error-run capture, HTML escaping).cd client && npm run build— cleantsc -b && vite build.npm run dev→ Reports in the sidebar → New report (pick a template + frequency + formats) → Run now → expand history → View HTML (opens the print-friendly artifact in a new tab; ⌘P → Save as PDF) / Download JSON.next_run_at-driven andunref'd; setDASHBOARD_REPORTS_DISABLED=1to turn it off.Acceptance criteria
[start,end), explicit tz)/api/reports/runs/:id/artifact?format=html|json)error-status run + message; never crashes the tick)Checklist
npm test) — see notenpm run format:check)/templatesendpoint + i18n)