feat(admin): consolidate dashboard workbench API and align calendar counts - #45
Conversation
…ounts Unify workbench eligibility (active + non-rejected) across calendar metrics and patient pool, replace 3–4 dashboard REST calls with one workbench endpoint, bound week/month aggregation in SQL, and batch thumbnail image-access to cut request volume. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Solid follow-up to #44: shared workbench eligibility filters (active + non-rejected), a consolidated GET /v1/staff/dashboard/workbench, week-bounded SQL aggregation, and batch image-access for thumbnails. The direction is right — fewer round trips, calendar counts aligned with the patient pool, and good backend regression coverage.
What looks good
workbench_upload_where_clauses()centralizes the filter used by attention, history overview (scope=workbench), and available dates — fixes the rejected/inactive calendar mismatch.- Week metrics now come from a 7-day bounded query instead of loading full calendar months.
POST /v1/staff/uploads/image-access/batch+useUploadImageUrlsbatching addresses the thumbnail N+1 called out on #44 (for components using the hook).test_staff_workbench_api.pycovers the important alignment cases (rejected-only day, inactive patient, week metrics vs attention, batch partial errors).get_history_overview_calendar_monthmonth-range filter is a nice perf win for the history tab.
Blocking
Frontend unit tests fail in CI (history-overview/__tests__/page.test.tsx). The jest mock still stubs fetchUploadImageAccess, but useUploadImageUrls now calls fetchUploadImageAccessBatch — 7 tests throw fetchUploadImageAccessBatch is not a function. Update the mock to include fetchUploadImageAccessBatch (returning { items: [{ upload_id, image_url }] }).
Nits (non-blocking)
list_workbench_datesloads everycreated_atinto Python and dedupes in memory. Fine for now, but consider a SQLDISTINCTon Taipei-local date as upload volume grows.- Unified workbench fetch refetches
available_datesandweek_dayswhen onlyselectedDatechanges within the same week (previously available dates were fetched once on mount). Minor extra load; could cache client-side or split stable fields later. upload-thumb.tsxstill uses per-thumbfetchUploadImageAccess— out of scope here, but the N+1 remains for that component._serialize_today_attention:rowsparam is untyped; could useSequence[TodayAttentionPatientRow].
CI (at review time)
| Check | Status |
|---|---|
| Lint | pass |
| Frontend build | pass |
| K8s render | pass |
| Frontend unit tests | fail (mock above) |
| Backend tests | pending |
Static review only in this environment; backend tests not run locally.
Verdict: Request changes for the failing test mock; otherwise this is in good shape to merge once CI is green.
Sent by Cursor Automation: PD Care PR Reviewer
| ) | ||
| if accessible_patient_ids is not None: | ||
| base_query = base_query.where(Patient.id.in_(accessible_patient_ids)) | ||
| created_ats = session.execute(base_query).scalars().all() |
There was a problem hiding this comment.
This pulls every workbench-eligible created_at into Python and dedupes dates in a set. Correct, but it scales with total upload count and runs on every workbench load (including date changes within the same week). Consider pushing distinct Taipei-local dates into SQL (e.g. func.date_trunc / timezone conversion) so the DB returns one row per day.
| import { useEffect, useMemo, useState } from "react"; | ||
|
|
||
| import { fetchUploadImageAccess } from "@/lib/api/staff"; | ||
| import { fetchUploadImageAccessBatch } from "@/lib/api/staff"; |
There was a problem hiding this comment.
Switching to batch fetch is the right call. Heads-up: history-overview/__tests__/page.test.tsx still mocks only fetchUploadImageAccess, so CI currently fails with fetchUploadImageAccessBatch is not a function. Add a mock for the batch helper (return { items: [{ upload_id, image_url }] }).
| const cancelled = isCancelled ?? (() => false); | ||
| setLoading(true); | ||
| try { | ||
| const data = await fetchWorkbenchDashboard({ |
There was a problem hiding this comment.
Consolidating to one endpoint simplifies the page nicely. Small trade-off: loadWorkbench also depends on weekStartDateKey, so changing the selected day within the same week still refetches available_dates and week_days even though only attention changed. Previously available dates were fetched once on mount. Not a blocker — just something to watch if dashboard feels sluggish later.
useUploadImageUrls now calls fetchUploadImageAccessBatch; update the staff API jest mock and add a shared helper so history-overview page tests render instead of crashing on mount. Co-authored-by: Cursor <cursoragent@cursor.com>
Type today-attention row serialization and avoid refetching stable calendar fields when only the selected date changes within the same week. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@cursoragent review again |
PR Review: feat(admin): consolidate dashboard workbench APIVerdict: Approve — ready to merge. CI is green on #45 (lint, backend tests, frontend unit tests, build, K8s render). Follow-up commits since the first review addressed the actionable items. Previously flagged items — resolved
What works well1. Eligibility alignment is the core win
2. Sensible API consolidation
const loadWorkbench = useCallback(
async (options?: { forceFull?: boolean; isCancelled?: () => boolean }) => {
// ...
const weekChanged = forceFull || cachedWeekStartRef.current !== weekStartDateKey;
// ...
if (weekChanged) {
const data = await fetchWorkbenchDashboard({ ... });
// sets available_dates, week_days, attention
} else {
const data = await fetchTodayAttention({ localDate: selectedDate });
// attention only
}
3. Batch image access is well-matched
4. Serializer extraction
Remaining nits (non-blocking)1. created_ats = session.execute(base_query).scalars().all()
dates = {to_taipei_date(created_at) for created_at in created_ats}
return sorted(dates, reverse=True)This scales with total upload count and runs on every full workbench load (week change, review save). Fine for current volumes; a follow-up could push distinct Taipei-local dates into SQL. Not a merge blocker. 2. No unit tests for Dashboard load/caching logic ( 3. Manual test plan still open PR checklist items for dev dashboard date counts and network tab (one workbench + one batch image call) remain unchecked — worth a quick manual pass before prod. 4. History overview vs dashboard scope History overview still calls SummaryThe PR delivers what it promises: calendar counts and patient pool share eligibility rules, the dashboard loads in fewer requests, and batch thumbnails reduce N+1 image-access calls. Prior review feedback has been addressed; CI is clean. Recommendation: Merge after optional manual verification of the open test-plan items. Track  |
Push workbench available_dates deduplication into the database with dialect-aware local-date expressions instead of loading every upload timestamp into Python. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Test plan