v2.2.0: MachineService, Machine Auto-detect, Profile Catalogue, Shot Annotations, Profile Notes, AI Controls#262
Open
v2.2.0: MachineService, Machine Auto-detect, Profile Catalogue, Shot Annotations, Profile Notes, AI Controls#262
Conversation
This was referenced Mar 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the v2.2.0 milestone feature set: new MachineService abstraction + UI features (AI controls, annotations/notes), plus backend endpoints for discovery, pour-over/recipes, SSE progress, and validation fallbacks.
Changes:
- Added frontend preference plumbing (AI toggles), new markdown editing components, and shot/profile note/annotation UI.
- Added backend services + routes for machine auto-detect, shot annotations, history notes, pour-over integration, recipes, and generation progress SSE.
- Updated tooling/config (eslint hooks rules, dependencies, Docker build defaults syncing, CI tags), plus added Playwright E2E coverage.
Reviewed changes
Copilot reviewed 110 out of 147 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/lib/aiPreferences.ts | Adds localStorage-backed AI preferences and change event dispatching. |
| apps/web/src/index.css | Adds “success glow” animation utility class. |
| apps/web/src/hooks/useWebSocket.ts | Adjusts reconnect logic to use a ref-stored connect function. |
| apps/web/src/hooks/useThemePreference.ts | Reworks “mounted” logic using useSyncExternalStore. |
| apps/web/src/hooks/useMachineService.ts | New hook for accessing MachineService via context. |
| apps/web/src/hooks/useHistory.ts | Extends HistoryEntry DTO with optional notes fields. |
| apps/web/src/hooks/useGenerationProgress.ts | Adds SSE hook for profile generation progress. |
| apps/web/src/hooks/use-mobile.ts | Migrates mobile breakpoint detection to useSyncExternalStore. |
| apps/web/src/hooks/use-desktop.ts | Migrates desktop breakpoint detection to useSyncExternalStore + SSR snapshot. |
| apps/web/src/hooks/index.ts | Re-exports generation progress hook/types/utilities. |
| apps/web/src/components/ui/sidebar.tsx | Stabilizes skeleton width using useState initializer. |
| apps/web/src/components/ui/resizable.tsx | Updates resizable UI wrapper API + lucide icon import path. |
| apps/web/src/components/ui/carousel.tsx | Defers setApi callback to microtask; adds lint suppression. |
| apps/web/src/components/ShotHistoryView.tsx | Adds shot annotations section + AI availability gating + auto-analyze preference. |
| apps/web/src/components/ShotAnnotation.tsx | New shot annotation fetch/save component with MarkdownEditor. |
| apps/web/src/components/ProfileImportDialog.tsx | Adds “generate AI descriptions” toggle gated by AI availability. |
| apps/web/src/components/ProfileBreakdown.tsx | Tightens variable typing in summary/value building. |
| apps/web/src/components/MarkdownText.tsx | Adjusts React key computation for trailing inline text segment. |
| apps/web/src/components/MarkdownEditor.tsx | New reusable edit/preview markdown editor. |
| apps/web/src/components/LiveShotView.tsx | Migrates machine commands to MachineService; adds clickable metric tile (tare). |
| apps/web/src/components/ControlCenterExpanded.tsx | Migrates machine commands to MachineService; filters temp profiles. |
| apps/web/src/components/ControlCenter.tsx | Migrates machine commands to MachineService; lazy-loads confetti; filters temp profiles. |
| apps/web/src/components/BetaBanner.tsx | Adds beta banner driven by /api/version + session dismiss. |
| apps/web/src/components/AdvancedCustomization.tsx | Adds “detailedKnowledge” toggle with warning UI. |
| apps/web/package.json | Bumps web version + updates eslint/react-hooks deps + adds testing-library/dom. |
| apps/web/eslint.config.js | Enables additional react-hooks v7 rules as errors. |
| apps/web/e2e/shot-history.spec.ts | Adds E2E coverage around Run/Schedule navigation & scheduling UI. |
| apps/web/e2e/settings.spec.ts | Adds E2E coverage for Settings navigation/sections. |
| apps/web/e2e/qr-code.spec.ts | Makes QR E2E assertions resilient to non-localhost URLs. |
| apps/web/e2e/profile-generation.spec.ts | Adds E2E coverage for generation form interactions + navigation. |
| apps/web/e2e/pour-over.spec.ts | Adds E2E coverage for pour-over view (Docker-only). |
| apps/web/e2e/live-shot.spec.ts | Adds E2E coverage for Live Shot view navigation (Docker-only). |
| apps/web/e2e/history.spec.ts | Adds E2E coverage for history/catalogue navigation + console/network error detection. |
| apps/web/e2e/control-center.spec.ts | Adds E2E coverage for Control Center (Docker-only). |
| apps/web/e2e/app.spec.ts | Makes E2E tests skip when AI features are disabled in CI. |
| apps/web/e2e/api-integration.spec.ts | Adds browser-driven API integration coverage (Docker-only). |
| apps/server/services/validation_service.py | Adds schema-aware (or basic) profile validation layer with MCP fallback. |
| apps/server/services/shot_annotations_service.py | Adds JSON-backed shot annotations persistence and caching. |
| apps/server/services/settings_service.py | Adds betaChannel field to settings defaults. |
| apps/server/services/recipe_adapter.py | Adds adapter from OPOS recipe JSON → machine profile stages. |
| apps/server/services/pour_over_preferences.py | Adds persistence for pour-over UI mode preferences. |
| apps/server/services/pour_over_adapter.py | Adds template-based pour-over profile adaptation. |
| apps/server/services/machine_discovery_service.py | Adds mDNS + hostname machine auto-detect and verify helper. |
| apps/server/services/history_service.py | Adds history entry note update + lookup by id. |
| apps/server/services/generation_progress.py | Adds in-memory generation progress state + SSE streaming support. |
| apps/server/services/gemini_service.py | Adds distilled knowledge string + AI-availability helper; clarifies error parsing docs. |
| apps/server/services/analysis_service.py | Adds static profile description fallback when AI unavailable/fails. |
| apps/server/requirements.txt | Adds SSE + zeroconf dependencies. |
| apps/server/requirements-test.txt | Adds optional paho-mqtt for integration tests. |
| apps/server/main.py | Syncs bundled defaults at startup; registers new routers; cleans stale temp profiles. |
| apps/server/conftest_integration.py | Adds opt-in integration test fixtures + helper utilities. |
| apps/server/conftest.py | Resets additional caches; adds autouse mocks/reset for validation + generation progress. |
| apps/server/api/routes/shots.py | Adds improved machine-unreachable handling + shot annotation endpoints + AI unavailable error mapping. |
| apps/server/api/routes/recipes.py | Adds recipe list/get endpoints. |
| apps/server/api/routes/pour_over.py | Adds pour-over prepare/cleanup/active endpoints + preferences endpoints. |
| apps/server/api/routes/history.py | Adds history notes GET/PATCH endpoints. |
| apps/server/api/routes/commands.py | Adds POST /api/machine/detect endpoint for auto-detect. |
| apps/server/INTEGRATION_TESTING.md | Documents how to run opt-in integration tests against real hardware. |
| apps/mcp-server | Updates submodule pointer. |
| WINDOWS.md | Adds installer troubleshooting guidance for PowerShell execution behavior. |
| VERSION | Bumps project version string. |
| README.md | Documents addon management post-install. |
| GEMINI.md | Adds Continuity auto-generated project memory file. |
| CLAUDE.md | Adds Continuity auto-generated project memory file. |
| AGENTS.md | Adds Continuity auto-generated project memory file. |
| .vscode/settings.json | Removes workspace-specific Python test runner settings. |
| .github/workflows/build-publish.yml | Adjusts Docker tag rules for latest/beta behavior. |
| .github/skills/workflow.md | Adds agent workflow documentation file. |
| .github/skills/testing.md | Adds testing/build verification documentation file. |
| .github/skills/frontend.md | Adds frontend standards documentation file. |
| .github/skills/backend.md | Adds backend standards documentation file. |
| .github/prompts/write-plan.prompt.md | Adds plan-writing prompt/skill definition file. |
| .github/prompts/worktree.prompt.md | Adds worktree management prompt/skill definition file. |
| .github/prompts/verify.prompt.md | Adds verification-before-completion prompt/skill definition file. |
| .github/prompts/superpowers.prompt.md | Adds skill system usage prompt/skill definition file. |
| .github/prompts/review.prompt.md | Adds requesting-code-review prompt/skill definition file. |
| .github/prompts/receive-review.prompt.md | Adds receiving-code-review prompt/skill definition file. |
| .github/prompts/finish-branch.prompt.md | Adds finishing-a-development-branch prompt/skill definition file. |
| .github/prompts/execute-plan.prompt.md | Adds executing-plans prompt/skill definition file. |
| .github/prompts/dispatch-agents.prompt.md | Adds parallel agent dispatch prompt/skill definition file. |
| .github/prompts/brainstorm.prompt.md | Adds brainstorming prompt/skill definition file. |
| .dockerignore | Ensures pour-over defaults (template/recipes) are included in Docker build context. |
| .cursorrules | Adds Continuity auto-generated project memory file. |
You can also share your feedback on Copilot code review. Take the survey.
Owner
Author
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Contributor
Owner
Author
|
🤖 CI is green (6/6 jobs). Requesting fresh Copilot code review after rebase + feature implementation. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 92 out of 155 changed files in this pull request and generated 10 comments.
You can also share your feedback on Copilot code review. Take the survey.
- Create MachineService interface with brewing, machine, and config commands - Implement MeticAIAdapter that delegates to REST API - Add MachineServiceContext and useMachineService hook - Migrate LiveShotView, ControlCenter, ControlCenterExpanded, PourOverView - Add mock for useMachineService in PourOverView tests - All 277 tests pass
- Convert static import to dynamic import() - Confetti chunk split out to separate ~11KB file - Main bundle reduced by ~11KB - Module only loaded when 100th shot milestone reached
- Add zeroconf>=0.134.0 dependency for mDNS service discovery - Create machine_discovery_service.py with multi-tier discovery: - mDNS browse for _meticulous._tcp.local. - Hostname resolution for meticulous.local - Returns guidance when machine not found - Add POST /api/machine/detect endpoint in commands.py - Add 'Detect' button in SettingsView with spinner and result feedback - Auto-fills IP field when machine is discovered - Add English translation strings for detect/machineFound
- Add DELETE /api/machine/profile/{id} endpoint for profile deletion
- Add PATCH /api/machine/profile/{id} endpoint for profile renaming
- Create ProfileCatalogueView component with:
- List all profiles from machine with metadata
- Rename profiles inline with confirmation
- Delete profiles with confirmation dialog
- Export profile JSON to file
- Show 'in history' badge for imported profiles
- Add Profile Management section in Settings with link to catalogue
- Add translation strings for profile catalogue UI
- Add 'profile-catalogue' view state type
- Create shot_annotations_service.py for persistent annotation storage
- Add GET/PATCH /api/shots/{date}/{filename}/annotation endpoints
- Create MarkdownEditor component with edit/preview toggle
- Create ShotAnnotation component for inline editing with auto-save
- Add annotation section to ShotHistoryView shot detail view
- Store annotations in data/shot_annotations.json keyed by date/filename
- Add translation strings for annotation UI
- Add notes and notes_updated_at fields to HistoryEntry interface
- Add update_entry_notes() and get_entry_by_id() to history_service.py
- Add GET/PATCH /api/history/{entry_id}/notes endpoints
- Add MarkdownEditor to ProfileDetailView for inline note editing
- Reuse existing MarkdownEditor component from shot annotations
- Add English translation strings for notes UI
- Add autoAnalyzeShots preference to control auto-analysis on shot view - Add showAiInHistory preference to control AI summary visibility - Add new toggle switches in Settings view for both preferences - Update ShotHistoryView to respect autoAnalyzeShots setting - Update HistoryView to respect showAiInHistory for coffee_analysis - Add English translation strings for new AI controls
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add GET /api/shots/recent and /api/shots/recent/by-profile endpoints - Create ShotAnalysisView with Recent and By Profile tabs - Add Shot Analysis button to home page - Fix ESLint errors (unused imports, dependency arrays) - Add i18n keys to all 6 locales Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace deprecated asyncio.get_event_loop() with get_running_loop() in machine_discovery_service.py - Add threading.Lock to shot_annotations_service.py to prevent concurrent read/modify/write race conditions on annotation cache Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub Actions: - docker/login-action v3 → v4 - docker/setup-buildx-action v3 → v4 - docker/setup-qemu-action v3 → v4 - docker/metadata-action v5 → v6 - docker/build-push-action v6 → v7 npm (apps/web): - lucide-react ^0.484.0 → ^0.577.0 - Regenerated lockfile (picks up storybook, lodash, rollup patches) Addresses dependabot PRs: #264, #265, #266, #267, #268, #273, #276, #277, #278 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Build and all 277 tests pass clean. Addresses dependabot PR #272. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Build and all 277 tests pass. Addresses dependabot PR #269. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not actively imported yet — bump is safe. Build + 277 tests pass. Addresses dependabot PR #275. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Major version upgrade. Build + 277 tests pass. Bundle size actually decreased slightly (1730 kB vs 1757 kB). Addresses dependabot PR #274. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Major version upgrade. Zod 4 has backwards-compatible z.* API. Build + 277 tests pass. Addresses dependabot PR #271. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r keys Added missing translation keys to all 6 locale files (en, es, de, fr, it, sv) that were causing raw i18n keys to display on the home screen.
53 tasks
- edit_profile: return full profile_dict instead of 5-field subset (v2.3 #258 apply-recommendations needs complete profile after edit) - list_machine_profiles: include stages and variables via deep_convert_to_dict (v2.3 #95 profile recommendation scoring needs actual parameters) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
HistoryView passed handleSaveNotes (API call + toast) as onChange to MarkdownEditor, firing on every character. Separated onChange (local state update) from onSave (explicit save button). Also fixed isSaving → saving prop name to match MarkdownEditor's interface.
…o fallback - Changed validation_service to use ValidationLevel.STRICT (was SAFETY), enforcing emoji naming and other strict rules. - Added recursive unused-variable detection to _basic_validate() fallback so profiles are checked even when the full validator isn't available. - Updated mcp-server submodule with recursive variable scanning fix.
- Fix missing await on getServerUrl() in handleDetectMachine causing 405 error - Fix Swedish 'Skottanalys' → 'Shot-analys' translation - Add 9 missing settings keys to all 5 non-English locales (de, es, fr, it, sv): autoAnalyzeShots, autoAnalyzeShotsDescription, showAiInHistory, showAiInHistoryDescription, detect, machineFound, profileManagement, viewProfileCatalogue, profileCatalogueDescription
- Fix target curves not rendering after recharts v3 migration by replacing Customized component with TargetCurvesSvg using useXAxisScale/useYAxisScale hooks - Fix shot click in ShotAnalysisView to navigate directly to specific shot instead of only opening the profile's shot list - Add 5-minute cache with refresh button to ShotAnalysisView to avoid redundant API calls when switching between views
- Remove autoAnalyzeShots setting and auto-analysis behavior (shots are now analyzed only on manual request) - Remove showAiInHistory setting (coffee analysis always visible) - Remove both settings from aiPreferences.ts, SettingsView, and all 6 locales - Consolidate profile catalogues: move machine profile management (gear icon with sync badge) into HistoryView header, remove from Settings - ProfileCatalogueView now navigates back to profile catalogue, not start - Show actual 1-5 star rating in shot history list (was just boolean indicator) - Clean up unused translations (profileManagement, viewProfileCatalogue, profileCatalogueDescription, autoAnalyzeShots, showAiInHistory) Refs: #234
- Add 'Generate AI Explanation' button on profile detail view for
profiles with static (non-AI) descriptions. Detects static summaries
by checking for 'generated without AI assistance' marker text.
- Add POST /api/profile/{entry_id}/regenerate-description backend endpoint
that regenerates a profile description using AI and updates history.
- Add auto-sync toggle in Profile Catalogue that periodically (every 5 min)
checks for new/updated profiles on the machine and imports them.
- Add POST /api/profiles/auto-sync backend endpoint for automatic import
of new profiles and acceptance of updated ones.
- Add auto-sync preference persistence via localStorage.
- Add translations for all new UI text in all 6 locales.
- Fix unused ArrowsClockwise import lint error in HistoryView.
Closes #234
Owner
Author
v2.2.0 Manual Testing ChecklistTesting against locally built container ( Issue #234 — Generate AI Explanation Button
Import Toggle (regression check)
Auto-sync
Profile Catalogue & Sync (regression)
Profile Detail View (regression)
Settings (regression)
i18n Spot-check
🔄 Testing in progress... |
Owner
Author
Manual QA Testing Results ✅Tested against running Docker container (v2.2.0-beta.1) at PR #262 Feature Tests
Regression Tests
Pre-existing Issues Found (NOT from this PR)
All 11 test items PASSED. Pre-existing i18n gaps logged to tasks.md for follow-up in this PR. |
- Translated title, shotDetails, searching, scanningLogs, checkForNewShots, noShots, shotsCount, replay, compare, analyze, shotAnalysis, analyzeShot, shotSummary, weight, preinfusion, stageAnalysis, profileTarget, exitTriggers, limits, hitLimit, notReached, stageFailed, incomplete, exporting, exportAsImage, viewAiAnalysis, getAiAnalysis, runAnalysisForOverlay in sv/de/es/it/fr - Added missing shotHistory.aiUnavailable to de/es/it/fr - Added shotHistory.lastUpdated key to all 6 locales - Replaced hardcoded 'Last updated:' in ShotHistoryView.tsx with t() call
Owner
Author
QA Checklist — Final Pass (ef9494c)Tested against Docker container rebuilt from latest commit with Feature: Generate AI Explanation (Issue #234)
Feature: Auto-Sync from Machine
Profile Detail & Settings
i18n
Tests & Build
Copilot Review Comments — All 17 Unresolved Threads Responded To
Known Follow-up Items (non-blocking)
|
- i18n: Replace 8 hardcoded English error strings in OrphanResolutionDialog and DeleteProfileDialog with t() translation keys across all 6 locales - deps: Bump eslint ^10.0.2 → ^10.0.3 (PR #270), storybook ^10.2.7 → ^10.2.17 (PR #276) - fix(profiles): Add threading.Lock to history read-modify-write in profile rename cascade to prevent TOCTOU race condition - fix(shots): Bound recent shots cache to 50 entries with TTL-based eviction and clamped cache key parameters to prevent unbounded memory growth - chore: Bump version to 2.2.0-beta.2 All tests pass: 741 backend, 277 frontend. Build clean. Lint: 0 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
v2.2.0-beta.2 — Code Review Hardening (
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 53 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (8)
apps/web/src/components/ShotHistoryView.tsx:1
- This update path introduces
undefined as neverinto aRecord<string, {...}>and then immediately schedules a second state update to delete the key. Besides being type-unsafe, the doublesetAnnotationSummariescan cause unnecessary renders and makes state updates harder to reason about. Prefer a single functional update that either sets the summary or deletes the key (by cloning anddelete), without writingundefinedinto the record.
apps/web/src/components/ShotHistoryView.tsx:1 handleSelectShot,initialShotDate, andinitialShotFilenameare used inside this effect but omitted from the dependency list via an eslint disable. This can lead to stale closures if those props/functions change without a remount. A cleaner approach is to (1) makehandleSelectShotstable viauseCallback, (2) include the props in the dependency array, and (3) call the async handler asvoid handleSelectShot(target)to avoid unhandled promise warnings.
apps/web/src/components/ShotHistoryView.tsx:1handleSelectShot,initialShotDate, andinitialShotFilenameare used inside this effect but omitted from the dependency list via an eslint disable. This can lead to stale closures if those props/functions change without a remount. A cleaner approach is to (1) makehandleSelectShotstable viauseCallback, (2) include the props in the dependency array, and (3) call the async handler asvoid handleSelectShot(target)to avoid unhandled promise warnings.
apps/web/src/components/ShotAnnotation.tsx:1- This is an optimistic UI update (
setRating(newRating)) but failures don’t revert the rating, leaving the UI out of sync with the backend. Capture the previous rating before updating and restore it in thecatchblock if the PATCH fails.
apps/web/src/components/SettingsView.tsx:1 - The fallback
guidancestring is hard-coded English and bypasses i18n, andresponse.okis not checked before attempting to parse JSON. Consider (1) checkingresponse.okand showing a translated error message, and (2) moving the fallback guidance into translations (e.g.,t('settings.detectFailedGuidance')).
apps/web/src/components/SettingsView.tsx:1 - The fallback
guidancestring is hard-coded English and bypasses i18n, andresponse.okis not checked before attempting to parse JSON. Consider (1) checkingresponse.okand showing a translated error message, and (2) moving the fallback guidance into translations (e.g.,t('settings.detectFailedGuidance')).
apps/web/src/components/SettingsView.tsx:1 - The fallback
guidancestring is hard-coded English and bypasses i18n, andresponse.okis not checked before attempting to parse JSON. Consider (1) checkingresponse.okand showing a translated error message, and (2) moving the fallback guidance into translations (e.g.,t('settings.detectFailedGuidance')).
apps/web/src/components/ShotAnalysisView.tsx:1 - These
<button>elements should settype="button"to avoid accidentally acting as submit buttons if this component is ever rendered inside a<form>.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+1014
to
+1018
| # Sort by timestamp descending (handle None timestamps) | ||
| all_shots.sort( | ||
| key=lambda s: float(s["timestamp"]) if s["timestamp"] else 0, | ||
| reverse=True, | ||
| ) |
There was a problem hiding this comment.
float(s["timestamp"]) can raise ValueError if the machine returns a non-numeric timestamp string (or an unexpected type), turning a single bad shot into a 500. Consider a safe parse helper that returns 0 on parse failure (and/or filters invalid timestamps).
Comment on lines
+1037
to
+1039
| @router.get("/shots/recent/by-profile") | ||
| @router.get("/api/shots/recent/by-profile") | ||
| async def get_recent_shots_by_profile(request: Request, limit: int = 50, offset: int = 0): |
There was a problem hiding this comment.
The endpoint accepts offset, but the implementation never applies it (it always groups from offset=0), so pagination semantics are incorrect and the cache key includes an offset that doesn’t actually affect the cached payload. Apply offset consistently (either page first then group, or define grouping-pagination rules explicitly and implement them) so responses match the query params.
Comment on lines
+1053
to
+1059
| # Reuse the flat recent-shots logic | ||
| flat_response = await get_recent_shots(request, limit=limit + offset, offset=0) | ||
| flat_shots = flat_response["shots"] | ||
|
|
||
| # Group by profile_name | ||
| grouped: dict[str, dict] = {} | ||
| for shot in flat_shots: |
There was a problem hiding this comment.
The endpoint accepts offset, but the implementation never applies it (it always groups from offset=0), so pagination semantics are incorrect and the cache key includes an offset that doesn’t actually affect the cached payload. Apply offset consistently (either page first then group, or define grouping-pagination rules explicitly and implement them) so responses match the query params.
Comment on lines
+995
to
+997
| # We need enough shots for offset + limit; collect greedily | ||
| needed = offset + limit | ||
| for date in dates: |
There was a problem hiding this comment.
Even though you stop once all_shots >= needed, within a date you still enqueue tasks for all shot files on that date. On dates with many shots, this can create a very large task list and wasted work. Consider slicing files down to the remaining number needed (e.g., needed - len(all_shots)) before creating tasks.
Comment on lines
+1001
to
+1004
| files_result = await async_get_shot_files(date) | ||
| if hasattr(files_result, "error") and files_result.error: | ||
| continue | ||
| files = sorted([f.name for f in files_result], reverse=True) if files_result else [] |
There was a problem hiding this comment.
Even though you stop once all_shots >= needed, within a date you still enqueue tasks for all shot files on that date. On dates with many shots, this can create a very large task list and wasted work. Consider slicing files down to the remaining number needed (e.g., needed - len(all_shots)) before creating tasks.
Comment on lines
+1151
to
+1154
| except ValueError as e: | ||
| raise HTTPException(status_code=422, detail={"status": "error", "error": str(e)}) | ||
| except Exception as e: | ||
| logger.error( |
There was a problem hiding this comment.
await request.json() can raise a JSON decoding error for malformed bodies, which currently falls into the generic Exception handler and returns a 500. It should return a client error (typically 400/422) for invalid JSON. Add an explicit catch for JSON decode errors and raise HTTPException(status_code=400, ...) (or allow FastAPI’s 422 handling via a Pydantic model).
Comment on lines
+50
to
+53
| const handleSave = useCallback(() => { | ||
| onSave?.() | ||
| setIsEditing(false) | ||
| setShowPreview(false) |
There was a problem hiding this comment.
If onSave performs async work (as in ShotAnnotation / history notes), the editor closes immediately, and users can re-open/edit while a save is still in-flight. Consider making handleSave async and awaiting onSave (or letting the parent control edit mode), only exiting edit mode after success (and keeping it open on failure).
Suggested change
| const handleSave = useCallback(() => { | |
| onSave?.() | |
| setIsEditing(false) | |
| setShowPreview(false) | |
| const handleSave = useCallback(async () => { | |
| if (!onSave) return | |
| try { | |
| await onSave() | |
| setIsEditing(false) | |
| setShowPreview(false) | |
| } catch { | |
| // Keep editor open on save failure; error handling is delegated to the caller | |
| } |
| ) | ||
|
|
||
|
|
||
| @router.get("/api/history/{entry_id}/notes") |
There was a problem hiding this comment.
New history notes endpoints are introduced here, but there are no corresponding tests in the added backend test suite (e.g., covering set/clear notes and 404 on missing entries). Adding tests would help prevent regressions in notes persistence and response shape.
| } | ||
|
|
||
|
|
||
| @router.patch("/api/history/{entry_id}/notes") |
There was a problem hiding this comment.
New history notes endpoints are introduced here, but there are no corresponding tests in the added backend test suite (e.g., covering set/clear notes and 404 on missing entries). Adding tests would help prevent regressions in notes persistence and response shape.
Comment on lines
+237
to
+238
| @router.post("/api/machine/detect") | ||
| async def detect_machine(): |
There was a problem hiding this comment.
The machine auto-detect endpoint is new, but there are no tests validating its response contract (e.g., found false includes guidance, found true includes ip/hostname/method/verified, and error handling). Adding a minimal test with mocked discovery/verify functions would lock down the API behavior expected by SettingsView.
Add TestHistoryNotesEndpoints (5 tests):
- GET /api/history/{id}/notes success and 404
- PATCH /api/history/{id}/notes success, empty clear, and 404
Add TestMachineDetectEndpoint (4 tests):
- Machine found+verified, found+unverified, not found, exception
Addresses review threads 34-36 on PR #262.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
shots.py: - Add _safe_float() helper for timestamp parsing (thread 25) - Apply offset in by-profile endpoint before grouping (threads 26-27) - Limit file enqueuing per-date to remaining needed count (threads 28-30) - Catch JSONDecodeError in PATCH annotation → 400 (threads 31-32) history.py: - Catch JSONDecodeError in PATCH notes → 400 (same pattern as shots.py) MarkdownEditor.tsx: - Make handleSave async, await onSave, keep editor open on failure (thread 33) - Update onSave prop type to support Promise<void> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
All 12 Open Review Threads Addressed (
|
| Thread | File | Fix |
|---|---|---|
| #25 | shots.py |
Safe _safe_float() helper for timestamp parsing — no more ValueError on bad data |
| #26-27 | shots.py |
by-profile endpoint now applies offset before grouping |
| #28-30 | shots.py |
File enqueuing per-date limited to remaining = needed - len(all_shots) |
| #31-32 | shots.py |
JSONDecodeError caught → returns 400, not 500 |
| — | history.py |
Same JSON decode fix applied to PATCH notes endpoint |
| #33 | MarkdownEditor.tsx |
handleSave is now async, awaits onSave, keeps editor open on failure |
New Tests (9 tests added → 750 total)
| Thread | Tests |
|---|---|
| #34-35 | TestHistoryNotesEndpoints — 5 tests (GET/PATCH success, 404, clear notes) |
| #36 | TestMachineDetectEndpoint — 4 tests (found+verified, found+unverified, not found, exception) |
Thread Resolution Summary (all 36 threads)
| Status | Count | Details |
|---|---|---|
| Resolved (outdated/stale) | 14 | Code changed since review |
| Resolved (already addressed) | 10 | getServerUrl await, annotation lock, dual routes, etc. |
| Newly fixed | 12 | All implemented above — zero deferrals |
Deferred items: None. Zero tech debt.
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
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
v2.2.0 implements all 10 milestone features plus comprehensive dependency upgrades, code review hardening, and zero deferred tech debt. This is a major release adding profile management, shot annotations, machine discovery, profile editing, shot analysis, and profile sync capabilities.
Milestone Issues — All Complete ✅
504249020d42f117f0c751feb26e+62edfabf9cbdc1288d55c+dd298bf8337c56118f1663642f4140477e8Code Review — All Threads Resolved ✅
Code Hardening (beta.2)
threading.Lockto history read-modify-write in profile rename cascade to prevent TOCTOU race conditionDependency Upgrades — All 15 Dependabot PRs Addressed ✅
GitHub Actions
npm (apps/web)
Python
Test Results
Related Issues
Closes #252, #188, #216, #225, #234, #192, #179, #182, #257, #259
Tracking: #254
Dependabot PRs addressed: #264, #265, #266, #267, #268, #269, #270, #271, #272, #273, #274, #275, #276, #277, #278
Status: v2.2.0-beta.2 — Ready for User Testing