From a41d39294ac3ec2484f35b0ad751f168a0d1ab84 Mon Sep 17 00:00:00 2001 From: Robert Hamilton Date: Sat, 4 Apr 2026 22:34:39 -0500 Subject: [PATCH 01/17] Release v1.0.0-beta.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add Authorization header to Supabase reachability check The PostgREST root endpoint requires both `apikey` and `Authorization` headers. Missing the latter caused a 401 during setup, showing "Cannot reach server" even with valid credentials. * fix: auto-validate and surface env var connection failures on setup page When VITE_SUPABASE_URL and VITE_SUPABASE_PUB_KEY are set but invalid, the setup page now auto-validates on mount and shows the specific error (e.g. "Server responded with 401") instead of silently presenting a blank form. Also adds a banner explaining that environment variables were detected but failed. * fix: use Supabase client for connection validation instead of raw fetch Raw fetch with apikey/Authorization headers only works with legacy anon keys. New publishable keys (sb_publishable_xxx) require the Supabase client library which handles auth headers internally. This switch supports both key formats transparently. * fix: resolve duplicate migration timestamp collision Two migrations shared timestamp 20260329000001, causing a primary key violation in supabase_migrations. Renumbered create_sharing_tables to 20260329000002 and expand_rls_for_sharing to 20260329000003. * chore: update Claude Code configuration and skills * feat: complete sync pull path with dynamic upsert and force-pull (Step 9) (#28) Implement the stubbed pull/upsert path so remote changes from Supabase are actually written to local SQLite, completing bidirectional sync. - Fix broken timestamp parsing in upsert_row (ISO 8601 strings were silently parsed as 0, causing all remote updates to be discarded) - Implement dynamic column upsert via pragma_table_info with type coercion (timestamps, booleans, JSONB) between Supabase and SQLite - Add pull_table/pull_all for paginated force-pull via Supabase REST API - Wire sync_force_pull command with proper state transitions - Add auto-pull on first sign-in (detect last_pull_at == 0) - Add 28 new tests (49 total), all passing Co-authored-by: Robert Hamilton * feat: events & packing lists (Step 13.5) (#30) * feat(events): add events & packing lists feature (Step 13.5) Introduces EVENT as a new session category with full support for event metadata (date, location, URL, requirements) and a categorized packing checklist with drag-and-drop reorder, optimistic toggle, progress tracking, and countdown notifications. Database: - Migration adds EVENT to category CHECK constraints on session_templates and scheduled_sessions; adds event_metadata JSONB to session_templates and workout_logs; creates event_items table with polymorphic FK (template XOR log), RLS policies, partial indices, and updated_at trigger Domain: - eventRequirementSchema, eventMetadataSchema, eventItemSchema (Zod) - EVENT added to SessionType enum; eventMetadata optional on templates and logs; EventItemRow added to database.types.ts Data layer: - toEventItem/fromEventItem mappers; eventMetadata parsing in toSessionTemplate (JSON.parse) and toWorkoutLog (direct JSONB) - DataAdapter interface: getEventItems, saveEventItem, updateEventItem, deleteEventItem, toggleEventItemPacked (accepts target bool, no race), reorderEventItems (Promise.all), cloneSessionTemplate (isPacked reset) - SupabaseAdapter implements all methods; getSessionTemplateFull fetches eventItems for EVENT templates; updateSessionTemplateFull uses 3-way reconciliation to preserve item UUIDs and isPacked state - TauriAdapter stubs (event support deferred, W-8) Hooks: - useEventItems, useCreateEventItem, useUpdateEventItem, useDeleteEventItem, useToggleEventItemPacked (optimistic), useReorderEventItems (optimistic), useNextUpcomingEvent UI: - event-builder/: EventTemplateForm (Sheet overlay, create/edit), EventDetail (metadata + map link + packing), PackingList (DnD, collapsible categories, error feedback), PackingItem (checkbox toggle), EventProgressBar (ember on surface-steel, ARIA progressbar), EventCard (library), EventCountdownBadge (Today screen, TODAY edge case), RequirementEditor, EventItemEditor, shared styles.ts - Library: EventCard for EVENT templates, NEW EVENT button - log/$workoutId: interactive EventDetail for event logs - history/$workoutId: read-only EventDetail for event history - Today screen: countdown badge when next event within 30 days - Program builder: flag icon + ember accent for EVENT sessions, event date displayed in program-preview grid and detail sections Native (Rust): - event_reminders channel registered in notification.rs - EventReminderState: background Tokio task (60s poll), checks configured intervals (7d/3d/1d), quiet hours, packing progress in notification body, deterministic notification ID - schedule_event_reminders / cancel_event_reminders Tauri commands - Clippy fixes: RemindedState type alias, needless_borrow in lib.rs, manual_range_contains in session_reminder.rs - Lock contention fix: single mutex guard across event loop in tick() Tasks: S001, S002, S003, S004, S005, S006, S007, S008, S009, S010, S011, S012, S013, S014 Feature: 002-events-packing-lists * docs(planning): mark 002-events-packing-lists complete Updates Steps.md status to Complete, session.md with full wave execution log and fixes applied, and implementation-plan.md. Feature: 002-events-packing-lists * chore(rust): apply cargo fmt across codebase * chore: add mise config and cortex hooks * fix(events): address PR #30 review findings - Fix timezone bug in combineDateTime: store local date strings instead of converting to UTC, preventing event dates from shifting by a day - Add eventUrl validation in form validate() before save - Switch sequential item persistence to Promise.allSettled with error collection for partial-failure reporting - Fix DST-unsafe daysUntil calculation using date-only comparison - Make TauriAdapter getEventItems throw consistently with write stubs - Add "TODAY" special case in Rust event reminder notification title - Eliminate double hydration of requirements/items in form init - Add EventItem mapper roundtrip tests and splitDateTime/combineDateTime edge case tests (19 new tests) --------- Co-authored-by: Robert Hamilton * feat(groups): coach program assignment Adds the ability for a group coach to assign one of their programs to a member. The program is atomically transferred to the member's ownership with source tagged as COACH_ASSIGNED and the coach recorded as created_by, enabling the member to filter out programs they received vs created. Includes the two-step AssignProgramDialog UI and integrates it into MemberCard. Both the Tauri SQLite command and the Supabase SECURITY DEFINER RPC perform the same three authorization checks (caller is COACH, target is MEMBER, program owned by caller) before executing the transfer. * fix(sync): resolve incremental push breakage and missing error state - push.rs: timestamp_millis() -> timestamp() so last_push_at is stored in seconds, matching the updated_at column it filters against; the millis value was always larger than any updated_at, causing zero rows to be found on every push after the first - sync.rs: sync_force_push now transitions to SyncState::Error on failure, matching the sync_force_pull pattern; previously a failed push left the engine in Idle, hiding the error from the frontend * docs(planning): add spec, tech, and steps for coach program assignment * feat(chat): add chat data layer + QA fixes (#33) * feat(chat): add chat data layer (Step 21) Builds the full persistence layer for the chat domain across Supabase, SQLite, Rust, and TypeScript. No UI -- pure data plumbing for Steps 22-25. - Supabase migration: conversations, conversation_participants, messages, media_attachments with RLS (CH-1), append-only messages (CH-7), and direct conversation uniqueness via trigger (CH-2) - SQLite migration: mirrors Postgres with sync_status on messages for offline queueing (pending -> synced/failed state machine) - Rust: ConversationRow/MessageRow/etc structs, 10 Tauri commands (CRUD for conversations, messages, participants, media), chat tables in SYNCABLE_TABLES for push/pull sync - TypeScript: Zod schemas (Conversation, Message, MediaAttachment, WorkoutSnapshot), DB row types, bidirectional mappers, 13-method ChatAdapter facet on DataAdapter interface, Supabase and Tauri adapter implementations, mock adapter updated Tasks: S001, S002, S003, S004, S005, S006, S007, S008, S009, S010, S011 Feature: 004-Chat-Data-Layer * docs(planning): add spec and steps for chat data layer (Step 21) Feature 004 planning artifacts: Spec.md covering requirements, testable assertions, and invariants (CH-1, CH-2, CH-4, CH-7); Steps.md with 4-wave execution plan for db-engineer, rust-engineer, domain-engineer, and quality-engineer. Feature: 004-Chat-Data-Layer * fix(qa): address all QA feedback -- sync resilience, lint, and stale tests Rust sync hardening: - sync/mod.rs: replace `return` with `continue` so queue flush failure no longer kills the sync loop permanently; transition to Error state when initial pull fails rather than silently proceeding to push - sync/pull.rs: add SYNCABLE_TABLES allowlist guard in upsert_row and delete_row - sync/push.rs: add SYNCABLE_TABLES allowlist guard in push_table - sync/queue.rs: normalize enqueue timestamp to seconds (was millis, inconsistent with push/pull) Frontend lint: - packing-list.tsx: replace setState-in-useEffect anti-pattern with useMemo derivation for mutation error feedback - event-template-form.tsx: extract splitDateTime/combineDateTime to date-time-utils.ts to fix Fast Refresh violation (non-component exports alongside component) Tests (31 stale failures fixed): - Update 5 test files with correct mixed-case text after CSS uppercase refactor - Rewrite mobile-nav.test.tsx: add useAuth mock, update to 5+1 nav structure, correct icons and routes * fix(review): resolve PR #33 review findings for chat data layer Critical fixes (C-1 through C-5): - Add missing INSERT policy on conversations table (C-1) - Filter user's own messages from getUnreadCounts in Supabase adapter (C-2) - Change z.url() to z.string().url() for media attachment URLs (C-3) - Return Err instead of Ok(()) from SYNCABLE_TABLES allowlist guards (C-4) - Add missing UPDATE policy on media_attachments table (C-5) Important fixes (I-2, I-3): - Add participant check before message insert in send_message (I-2) - Validate direct conversations require exactly 2 participants (I-3) Tasks created (S013-S017) for: data mapper tests, dedicated get_messages_since command, updatedAt mapping fix, unread counts resilience, and sync guard test coverage. Suggestions S-1 through S-6 tracked in Ideas backlog. Review: Context/Reviews/0001-pr33-chat-data-layer-review.md * feat(chat): implement review remediation tasks S013-S017 S013: Add 36 data mapper unit tests for chat entities - Conversation, ConversationParticipant, Message, MediaAttachment - Round-trip, null handling, and Zod validation tests S014: Add dedicated get_messages_since Rust command - SQL-level WHERE created_at > ? filter replaces client-side filtering - Eliminates silent truncation at 1000 message limit S015: Fix toConversationParticipant updatedAt mapping - Use last_read_at ?? joined_at instead of always joined_at S016: Improve getUnreadCounts error resilience - Single conversation failure no longer aborts all counts - Returns partial results with console.warn for failed queries S017: Remove #[cfg(not(test))] from sync allowlist guards - Tests now exercise the same allowlist path as production - Updated test fixtures to use real SYNCABLE_TABLES entries Review: Context/Reviews/0001-pr33-chat-data-layer-review.md --------- Co-authored-by: Robert Hamilton * feat: Supabase Realtime, chat data layer, events & coach assignment (#34) * docs(planning): add spec, tech, and steps for supabase realtime integration 4-phase blueprint for Step 22 (Supabase Realtime Integration). Establishes architecture for Broadcast channel management, cursor-based message pagination, RealtimeManager module, and all chat TanStack Query hooks. ADR-005: cursor-based getMessages replaces offset-based (no existing callers). * feat(chat): add Supabase Realtime broadcast channels and chat hooks Wire up Supabase Realtime Broadcast for live message delivery and typing indicators. Introduces the RealtimeManager singleton, cursor-based message pagination, platform-agnostic foreground detection, and all TanStack Query hooks needed by the Chat UI (Step 23). New files: - src/lib/realtime-schemas.ts -- Zod schemas for broadcast payloads - src/lib/realtime-manager.ts -- channel lifecycle, typing state, catch-up - src/lib/foreground-detector.ts -- visibilitychange / Tauri focus detection - src/hooks/use-chat.ts -- useConversations, useMessages (infinite), useSendMessage (optimistic), useRealtimeMessages, +6 more - src/components/chat-realtime-listener.tsx -- app-level cache update listener Modified: - data-adapter.ts / supabase-adapter.ts / tauri-adapter.ts / chat.rs Replace offset-based getMessages with cursor-based (before?: ISO timestamp) - main.tsx: mount ChatRealtimeListener alongside SyncListener - auth.tsx: call resetRealtimeManager() on SIGNED_OUT Tasks: S001, S002, S003, S004, S005, S006, S007 * fix: resolve QA lint and code quality issues across frontend and Rust Fix clippy errors in test code (manual_range_contains, approx_constant), ESLint no-constant-binary-expression in utils test, and three code quality issues in realtime-manager: validate typing payloads with Zod schema, replace fragile onTyping callback chain with Set-based listener pattern, and explicitly discard handleForeground Promise. * fix(chat): address PR #34 review findings for Supabase Realtime Multi-listener pattern (addMessageListener/addTypingListener) replacing single-callback ownership, broadcast try/catch in useSendMessage, channel lifecycle ownership fix, auth integration via useAuth hook, and Wave 5 polish: named constants, toMessageFromBroadcast mapper, simplified clearAllTyping, foreground-detector error logging and async/await rewrite, broadcastTyping .catch(), MessagePaginationOptions interface, user_name max(200) constraint. --------- Co-authored-by: Robert Hamilton * feat(chat): complete chat UI with messaging and conversations (#35) * docs(planning): add spec, tech, and steps for chat UI (Step 23) * feat(chat): build complete chat UI (Step 23) Routes, conversation list, virtualized message detail, compose bar, typing indicators, new conversation flow, group management, and client-side blocking. Navigation updated -- COMMS replaces Social on mobile, added after Vault on desktop sidebar with unread badge. Conversation type extended with participantUserIds so ConversationDetail can resolve the other user's display name and blocking state for direct conversations. Supabase adapter fetches participants alongside conversations; Tauri adapter was already receiving them and now uses them. Tasks: S001, S002, S003, S004, S005, S006, S007, S008, S009, S010, S011, S012, S013, S014 Feature: 006-Chat-UI * docs(planning): add quick plan for conversation participant IDs Feature: 006-Chat-UI * fix: update mobile-nav tests for Comms tab and fix Rust formatting Update test assertions for Social→Comms rename, add useUnreadCounts mock, and fix rustfmt formatting in event_reminder tests. * fix(chat): resolve PR #35 review findings Critical fixes: - C1: Contact search filter was a no-op; now passes search prop to ContactRow which renders null when display name does not match - C2: Sender name in group bubbles showed raw UUID; resolved via useUserProfile inside MessageBubble - C3: Send failure gave zero feedback; added sendError state and inline "Failed to send" indicator on ComposeBar - C4: Conversation creation failure was silent; added error state and inline message in NewConversationSheet - C5: MessageList had no error/loading state; added spinner and error fallback from useMessages isLoading/isError Important fixes: - I1: Typing broadcast used user.email; now uses displayName via useUserProfile - I2/I3: useBlockedUsers storage key scoped to userId; added console.error/warn on localStorage corruption - I4: Optimistic message had senderId undefined; now uses current user ID from useAuth - I5: LeaveDialog failure was silent; added inline error message - I6: ParticipantSheet add failure was silent; shows error including Tauri offline message - I7: Removed rounded-full from avatar containers and unread dots - I8: Added comment clarifying group threshold (2 selected + server adds self = 3 total participants) Suggestions: - S3: Removed build-artifact comment from conversation-list.tsx - S9: Fixed error copy from "pull to retry" to "try again" Tasks: C1, C2, C3, C4, C5, I1, I2, I3, I4, I5, I6, I7, I8, S3, S9 Feature: 006-Chat-UI * docs(planning): resolve PR #35 review and add test/polish tasks Marks 0003-pr35-chat-ui-review.md as fully resolved (5 critical, 8 important, 2 suggestions fixed inline; 6 tasks created; 5 suggestions dismissed with rationale). Adds Wave 7 tasks to Steps.md: - S015-T: deriveItems unit tests - S016-T: useBlockedUsers unit tests - S017-T: formatTypingText unit tests - S018-T: relativeTime + getInitials unit tests - S019: Migrate useBlockedUsers to Zustand store - S020: Wire archive action in conversation header Feature: 006-Chat-UI * refactor(chat): extract utils, add blocked-users store, verify review Extract pure functions (relativeTime, getInitials, formatTypingText, deriveItems) into chat-utils.ts and add tests. Migrate useBlockedUsers to Zustand store with user-scoped persistence. Mark PR #35 review as verified with follow-up tracked in GitHub issues #43-#51. --------- Co-authored-by: Robert Hamilton * feat: add link preview metadata with OG tags, Twitter Cards, and useMetadata hook (#53) Add comprehensive link preview support so shared URLs display rich previews on social platforms and messaging apps. Includes a useMetadata hook for dynamic per-route title and description updates, used on share pages. * fix(auth): prevent PKCE code race condition on OAuth callback (#54) Supabase JS v2.100.1 defaults to detectSessionInUrl: true with PKCE flow, causing _initialize() to auto-exchange the auth code before the /auth/callback route handler can. The second exchange attempt fails (code already consumed), redirecting users to /sign-in?reason=oauth_error. Disable detectSessionInUrl since we have a dedicated callback route that handles PKCE code exchange manually. * fix(auth): enable PKCE flow for OAuth sign-in (#55) Without explicit flowType: 'pkce', the Supabase client uses implicit flow, returning tokens as hash fragments (#access_token=...) that the /auth/callback route cannot read (it expects ?code= query params). Setting flowType: 'pkce' makes Supabase return a PKCE authorization code as a query parameter that the callback can exchange for a session. * fix(db): resolve RLS infinite recursion on group_members and conversation_participants (#56) The group_members and conversation_participants SELECT policies self-referenced their own tables, triggering PostgreSQL error 42P17 (infinite recursion). This cascaded 500s to every table whose sharing policies join group_members (workout_logs, session_templates, etc.). Creates SECURITY DEFINER helpers (is_group_member, is_group_coach, is_conversation_participant) that bypass RLS for inner membership lookups and replaces all six affected policies. Also removes the PWA manifest (manifest.webmanifest) and its link tag since Vercel deployment protection returns 401 on the unauthenticated manifest fetch. * feat(builder): program builder UI with responsive week grid (#58) * feat(builder): program builder UI with responsive week grid Implements the complete program builder and session builder UIs. Program builder: - Sidebar layout (desktop) / stacked layout (mobile) at /builder route - Drag-and-drop block reordering with @dnd-kit - Week grid defaults to 5-day Mon-Fri view with a toggle to show all 7 days, improving readability at constrained content widths (~94px/cell at 1024px narrowed to nothing legible; 5-day gives ~132px/cell min) - Session slots reduced from 80px to 56px min-height with type-colored left borders (STRENGTH/CONDITIONING/SE/MIXED/EVENT) - Mobile block editor with vertical day-row tap targets - Session picker bottom sheet and copy-week dialog - Program preview overlay with per-session exercise/load tables Session builder: - Set scheme editor with per-type form variants - Activity group editor and session template form Infrastructure: - Collapsible shadcn/ui component added - Removed boilerplate App.tsx/App.css - Expanded index.css with builder-specific design tokens * fix(builder): declutter block header with overflow menu Replace inline action buttons (move up/down, delete) with a dropdown overflow menu in both desktop and mobile block editors. This prevents touch target overlap on narrow viewports and isolates the destructive delete action behind a two-tap interaction. * feat(builder): guard destructive actions with confirmation and undo Block and week deletions now require confirmation via AlertDialog, showing impact ("This will remove N weeks and M sessions.") before proceeding. Session removal fires immediately but shows a 5-second undo toast via sonner, letting users recover accidental taps without blocking the fast path. - Add alert-dialog.tsx shadcn primitive (Radix AlertDialog) - Add confirm-delete-dialog.tsx reusable builder component - Wire block delete in BlockEditor and MobileBlockCard to dialog - Wire week delete in WeekGrid and MobileWeekSection to dialog - Add undo toast to SessionSlot and MobileDayRow session removal - Mount Toaster in main.tsx (bottom-center) * feat(builder): replace full-screen preview with inline week detail Remove the disconnected ProgramPreview overlay from the builder flow. Exercise details now toggle inline per-week via a visibility icon, keeping the edit-preview feedback loop tight. Save button moves to the header. Shared formatting helpers extracted to session-detail.tsx. Also fixes Collapsible asChild bug in block-editor and mobile-block-editor. * fix(builder): remove interstitial from session template create flow Tapping "Create new template" in the session picker now opens the inline form immediately, skipping the Library recommendation callout. The Library option is preserved as a subtle link ("Want more options? Create in the Library") below the form for users who need it. Removes showCreateCallout state and the two-step reveal pattern. * refactor(builder): consolidate session constants and polish UI details - Extract SESSION_TINT and SESSION_TYPE_BADGE from session-slot, mobile-block-editor, and program-preview into constants.ts - Remove duplicate @keyframes block-enter and slot-fill in index.css - Bump section-level labels (WEEK N, day columns, EXERCISE/SETS/LOAD) from text-[11px] font-medium to text-xs font-semibold for visual hierarchy over metadata badges which stay at text-[11px] font-medium - Upgrade session picker SheetTitle to text-sm font-display - Add empty state copy when zero blocks exist in BlockList and MobileBlockEditor - Clarify copy-week dialog: description now states sessions will be replaced, and per-week "(N existing)" label gains warning colour * feat(builder): replace collapsed save error with structured inline validation Validation errors now appear at the relevant field rather than as a single joined string below the save button. Program name errors render under the name input, block name and week errors render on each block card (visible even when collapsed), and server/auth errors remain global in the header. * feat(builder): add recently-used templates and reduce week grid noise Surface the 5 most recently assigned session templates at the top of the picker sheet so users with large template libraries reach common picks faster. Adds lastAssignedAt tracking across both Tauri/SQLite and Supabase backends. Reduce visual noise in the week grid: restyle empty slots as muted "Rest" labels instead of prominent calls-to-action, bring the 5/7-day toggle to mobile, and collapse identical weeks within a block to a compact summary row (with manual expand). * refactor(builder): extract session-detail helpers to fix Fast Refresh lint errors * feat: display broadcast infrastructure (Step 27) (#59) * feat(chat): video, image, and file sharing (#57) feat(chat): video, image, and file sharing (Feature 007) Full-stack media sharing: Cloudflare Stream video uploads with TUS, image/file uploads via Supabase Storage, real-time transcoding status via webhook, signed URL caching, and inline media rendering in chat. Review: 4 critical + 10 important findings resolved and verified. 12 enhancements implemented. 1,305 tests passing. * docs: add plan for Step 27 -- display broadcast infrastructure Spec.md, Tech.md, and Steps.md for Feature 008. Covers DisplaySnapshot Zod schema, publisher module, store integration, display_visible toggle, and Supabase Realtime Broadcast channel plumbing. 14 tasks across 4 waves with 3 agents. * feat: add display_visible column to user profiles SQLite and Supabase migrations for display broadcast toggle, Tauri model and command updates to support the new field. * feat: add DisplaySnapshot domain type and display publisher DisplaySnapshot type with Zod schema, display-publisher for broadcasting workout state, data-mapper and store updates. * feat: add PushToDisplayButton and display broadcast hook useDisplayBroadcast hook, PushToDisplayButton component, workout log and profile route integrations. * test: add display snapshot and publisher tests Unit tests for DisplaySnapshot schema, display-publisher, display-snapshot builder, and updated data-mapper/adapter tests. * docs: update Steps.md and implementation plan for display broadcast * fix(review): resolve PR #59 review findings Fixes: C1 (RLS auth), C2 (image signed URLs), C3 (snapshot try-catch), C4 (Cloudflare subdomain), I1-I9 (error handling, schema tightening, z.url() revert, publisher readiness), M1/M3/M5/M6 (env guards, request parsing, query error logging, JSDoc) Tasks: S024-T, S025-T added for media-provider and timeout tests Comments: 9 comment issues resolved Review: Context/Reviews/0005-pr59-display-broadcast-review.md * test(media): add unit tests for CloudflareStreamProvider and transcoding timeout S024-T: media-provider tests covering signed URL TTL cache hit/miss, null client error, Zod validation of malformed responses, and cache deduplication (13 tests). S025-T: use-media-upload tests covering 5-minute transcoding timeout marks processing attachment as failed, unaffected ready attachments, query cache invalidation, and timer cleanup on unmount (4 tests). Tasks: S024-T, S025-T Feature: 007-video-image-sharing * docs(review): mark PR #59 review as verified * feat(edge-functions): deploy CF video edge functions with Vault secrets and local JWT signing Deploys three Supabase Edge Functions for Cloudflare Stream video support (Step 25). All Cloudflare credentials read from Supabase Vault using CF_* secret names (CF_ACCOUNT_ID, CF_STREAM_TOKEN, CF_WEBHOOK_SECRET, CF_SIGNING_KEY_PEM, CF_SIGNING_KEY_ID, CF_CUSTOMER_SUBDOMAIN). chat-media-signed-url: replaces Cloudflare token API round-trip with local RS256 JWT signing via CF_SIGNING_KEY_PEM + CF_SIGNING_KEY_ID. No outbound HTTP call needed per playback request. chat-media-upload-url: reads CF_ACCOUNT_ID + CF_STREAM_TOKEN from Vault. chat-media-webhook: reads CF_WEBHOOK_SECRET + CF_ACCOUNT_ID from Vault. Adds get_secret() SQL migration granting service_role-only read access to vault.decrypted_secrets. Fixes duplicate migration timestamp 20260403000002 -> 20260403000003 for session_template_last_assigned. Updates mock-supabase test util to support keyed RPC results (get_secret:SECRET_NAME) for parameter-specific vault mocking. Removes CLOUDFLARE_ACCOUNT_ID from STANDARD_ENV test fixture. * chore: update app icons and branding assets Replace default Tauri icons with Ardent Forge branding across all platforms (Android, iOS, Windows, macOS). Update app display name in Android strings.xml and tauri.conf.json. * fix(db): rename migration 008 and auto-recreate DB on checksum mismatch Rename session_template_last_assigned to migration 009 to make room for 008_add_display_visible. Add debug-only auto-recreate logic so migration checksum mismatches during development delete and rebuild the local database instead of crashing. * fix(auth): use Chrome Custom Tabs for Google OAuth on Android Replace window.open with tauri-plugin-opener's inAppBrowser mode to launch Google OAuth in Chrome Custom Tabs instead of an embedded WebView. Google blocks OAuth in WebViews under their "Use secure browsers" policy. Add opener plugin registration and URL scope permissions for mobile capability. * chore: update Claude agent configs and mise tooling * feat(display): display route with board and focused views (#60) * feat(display): display route with board and focused views (Step 28) Implements the /display route -- a full-viewport, unauthenticated gym TV page that subscribes to the Supabase Broadcast display channel and renders active workout sessions in real time. Board view: CSS grid of session cards (1-4 per page), auto-cycling every 10 seconds when 5+ sessions are active. Each card shows display name, session type, elapsed time, current exercise, set progress, and a compact rest timer. Focused view: full-screen single-user layout with set table (prescribed vs actual with ember accent on current set), large rest timer with pulse animation under 10 seconds, and distance-readable typography (2-3.5rem). Idle placeholder: large clock with NO ACTIVE SESSIONS label. Infrastructure added: - display-store.ts: Zustand store with sessions Map, focus state, pagination, staleness pruning - display-subscriber.ts: Broadcast channel subscriber with Zod validation and connection status callbacks - use-timer-interpolation.ts: rAF-based countdown hook (1 React re-render per second, not per frame) - display_hello responder: phones re-publish their snapshot on reconnect so the display rebuilds state after network drops Tasks: S001, S002, S003, S004, S001-T, S005, S006, S007 Feature: 009-display-route-board * docs(context): feature spec, tech plan, and steps for display route (009) Adds planning artifacts for Step 28: display route with board view, focused view, timer interpolation, and display_hello reconnection. Feature: 009-display-route-board * refactor(display): address QA findings from board/focused route review - Fix React Compiler lint error: remove synchronous setSeconds(0) from useEffect in use-timer-interpolation; derive idle value via return instead - Extract useElapsedTime hook; remove duplicated useState+setInterval logic from session-card and focused-view - Add exponential backoff reconnection to display-subscriber (2s base, 30s max); destroyDisplaySubscriber cancels pending retry - Fix focused-view snapshot type: replace Map infer gymnastics with direct DisplaySnapshot import - Fix pruneStale: early-return when no sessions are stale to avoid unnecessary Map clones and Zustand notifications - Fix board-view interval: use getState().setCurrentPage() consistently instead of mixing captured selector with getState() reads - Fix duplicate JSDoc on setHelloResponder in display-publisher - Use cn() helper in page-indicator for conditional classes - Add comment in display.tsx explaining dedicated Supabase client - Add pruneStale edge case tests: ghost key in lastSeenAt, early-return Map identity check Feature: 009-display-route-board * fix(display): address QA findings from board/focused route review Resolves all 14 findings from PR #60 review (P6-001 through P6-014): Critical fixes: - Call publishHello() on reconnect via _hasConnectedBefore flag (P6-001) - Remove dead channel with removeChannel() before nulling _channel (P6-002) - Wrap boot() in try-catch; set disconnected status on failure (P6-003) Store fixes: - Merge dual sessions/lastSeenAt maps into single Map (P6-013) - Reset currentPage to 0 in clearAllSessions (P6-004) - Clamp setCurrentPage to [0, totalPages-1] (P6-005) - Extract DisplayConnectionStatus to display-snapshot.ts; import everywhere (P6-014) Subscriber fixes: - Call onStatusChange('disconnected') before early return when client null (P6-006) Hook fixes: - Guard useElapsedTime against NaN timestamps; clamp elapsed to Math.max(0) (P6-007) Comment fix: - Update __root.tsx bypass comment to include /display (P6-008) Tasks: P6-001, P6-002, P6-003, P6-004, P6-005, P6-006, P6-007, P6-008, P6-013, P6-014 Feature: 009-display-route-board * docs(context): add Wave 4 follow-up tasks and mark review verified - Add S008-S011 to Steps.md Wave 4 (reconnect retry tests, use-elapsed-time tests, schema .min(1) hardening, invalid payload tests for session_ended and focus events) - Add review capture file 0006-pr60-display-route-board-review.md with all findings and resolutions documented - Mark review verified after confirming all fixes landed Tasks: P6-009, P6-010, P6-011, P6-012 Feature: 009-display-route-board * fix(display): address QA findings from PR #60 review (#61) * feat(display): display route with board and focused views (Step 28) Implements the /display route -- a full-viewport, unauthenticated gym TV page that subscribes to the Supabase Broadcast display channel and renders active workout sessions in real time. Board view: CSS grid of session cards (1-4 per page), auto-cycling every 10 seconds when 5+ sessions are active. Each card shows display name, session type, elapsed time, current exercise, set progress, and a compact rest timer. Focused view: full-screen single-user layout with set table (prescribed vs actual with ember accent on current set), large rest timer with pulse animation under 10 seconds, and distance-readable typography (2-3.5rem). Idle placeholder: large clock with NO ACTIVE SESSIONS label. Infrastructure added: - display-store.ts: Zustand store with sessions Map, focus state, pagination, staleness pruning - display-subscriber.ts: Broadcast channel subscriber with Zod validation and connection status callbacks - use-timer-interpolation.ts: rAF-based countdown hook (1 React re-render per second, not per frame) - display_hello responder: phones re-publish their snapshot on reconnect so the display rebuilds state after network drops Tasks: S001, S002, S003, S004, S001-T, S005, S006, S007 Feature: 009-display-route-board * docs(context): feature spec, tech plan, and steps for display route (009) Adds planning artifacts for Step 28: display route with board view, focused view, timer interpolation, and display_hello reconnection. Feature: 009-display-route-board * refactor(display): address QA findings from board/focused route review - Fix React Compiler lint error: remove synchronous setSeconds(0) from useEffect in use-timer-interpolation; derive idle value via return instead - Extract useElapsedTime hook; remove duplicated useState+setInterval logic from session-card and focused-view - Add exponential backoff reconnection to display-subscriber (2s base, 30s max); destroyDisplaySubscriber cancels pending retry - Fix focused-view snapshot type: replace Map infer gymnastics with direct DisplaySnapshot import - Fix pruneStale: early-return when no sessions are stale to avoid unnecessary Map clones and Zustand notifications - Fix board-view interval: use getState().setCurrentPage() consistently instead of mixing captured selector with getState() reads - Fix duplicate JSDoc on setHelloResponder in display-publisher - Use cn() helper in page-indicator for conditional classes - Add comment in display.tsx explaining dedicated Supabase client - Add pruneStale edge case tests: ghost key in lastSeenAt, early-return Map identity check Feature: 009-display-route-board * fix(display): address QA findings from board/focused route review Resolves all 14 findings from PR #60 review (P6-001 through P6-014): Critical fixes: - Call publishHello() on reconnect via _hasConnectedBefore flag (P6-001) - Remove dead channel with removeChannel() before nulling _channel (P6-002) - Wrap boot() in try-catch; set disconnected status on failure (P6-003) Store fixes: - Merge dual sessions/lastSeenAt maps into single Map (P6-013) - Reset currentPage to 0 in clearAllSessions (P6-004) - Clamp setCurrentPage to [0, totalPages-1] (P6-005) - Extract DisplayConnectionStatus to display-snapshot.ts; import everywhere (P6-014) Subscriber fixes: - Call onStatusChange('disconnected') before early return when client null (P6-006) Hook fixes: - Guard useElapsedTime against NaN timestamps; clamp elapsed to Math.max(0) (P6-007) Comment fix: - Update __root.tsx bypass comment to include /display (P6-008) Tasks: P6-001, P6-002, P6-003, P6-004, P6-005, P6-006, P6-007, P6-008, P6-013, P6-014 Feature: 009-display-route-board * docs(context): add Wave 4 follow-up tasks and mark review verified - Add S008-S011 to Steps.md Wave 4 (reconnect retry tests, use-elapsed-time tests, schema .min(1) hardening, invalid payload tests for session_ended and focus events) - Add review capture file 0006-pr60-display-route-board-review.md with all findings and resolutions documented - Mark review verified after confirming all fixes landed Tasks: P6-009, P6-010, P6-011, P6-012 Feature: 009-display-route-board * feat(discovery): add discovery client and server URL flow (#63) * docs(planning): add Feature 009 plan for discovery client + server URL Creates the full 4-phase planning artifacts for Refactor A of the onboarding redesign: server URL input with auto-discovery replaces raw Supabase credential entry as the primary setup flow. Tasks: S001, S001-T, S002, S003, S003-T, S004, S004-D Feature: 009-discovery-client-server-url ADR-006 * feat(setup): add discovery client and server URL flow Replace raw Supabase credential inputs on the setup screen with a primary server address field that auto-discovers backend credentials via GET /.well-known/ardent-forge.json. Manual Supabase URL and key entry moves into a collapsible "Manual configuration" section as a fallback. - Add discoverInstance() in src/lib/discovery.ts with URL normalization, 8-second AbortController timeout, and typed DiscoveryResult discriminated union (NETWORK_ERROR, NOT_FOUND, INVALID_RESPONSE) - Add 11 Vitest unit tests covering happy path, URL normalization variants, and all error branches - Refactor setup.tsx: server URL primary input, discovery + validation state machine, "or" divider, collapsible manual section, disabled QR placeholder for future Refactor B - Add /.well-known/ardent-forge.json handle block to Caddyfile with {$SITE_URL} and {$ANON_KEY} env var interpolation - Add ANON_KEY to caddy service environment in docker-compose.yml - Document discovery endpoint for static host and Docker deployments in docs/self-hosting.md; update mobile app configuration section Tasks: S001, S001-T, S002, S003, S003-T, S004 Feature: 009-discovery-client-server-url ADR: ADR-006 * docs(planning): mark Feature 009 Steps.md as complete * fix(discovery): harden URL validation, use Zod, deduplicate connect logic - Validate URL protocol via new URL() parse; reject non-http/https schemes (file://, ftp://, etc.) before any fetch is made - Replace manual typeof field checks with a Zod schema (DiscoverySchema) to align with project convention and improve type safety - Extract shared validateAndSave() helper in setup.tsx, eliminating duplicated validate-save-init-navigate block between handleConnect and handleDiscoverAndConnect - Add 4 new tests: file:// rejection, ftp:// rejection, empty string, whitespace-only input Feature: 009-discovery-client-server-url * fix(review): resolve PR review findings for discovery feature Fixes (P6-001..008): add logging to all error paths, capture error params in catch blocks, correct error type semantics (INVALID_INPUT for validation, INVALID_RESPONSE for JSON parse), fix JSDoc, add aria-expanded to disclosure toggle, add double-submission guard, fix comment scope. Refactor: unify 4 parallel state variables into a single SetupState discriminated union with 7 phases, eliminating impossible states. Also wraps handlers in try-catch (P6-009..011) and fixes success state ordering as part of the refactor. Rules: create .claude/rules/error-handling.md codifying error type semantics and catch block conventions (P6-017, P6-018). Tasks: S005..S007-T added to Steps.md for schema tightening and test coverage gaps (P6-012..015). Review: Context/Reviews/0006-discovery-client-server-url-2026-04-04.md * fix(discovery): tighten schema validators and add missing test coverage S005: Use z.string().url() for supabase_url, z.string().min(1) for supabase_publishable_key and version in DiscoverySchema. Validation is now strictest at the system boundary. S005-T: Add test for missing supabase_publishable_key field. S006-T: Add test for 500 HTTP response (non-404 error code). S007-T: Add test for URL with path component being stripped. Review: P6-012, P6-013, P6-014, P6-015 * docs(review): mark discovery feature review as verified * feat(display): idle mode with Edge Function broadcast (#62) * docs(planning): add feature 009 idle mode spec, tech plan, and steps Full 4-phase blueprint for Step 29 (Idle Mode + Edge Function). - Spec.md: 8 must-haves, 17 testable assertions, all open questions resolved (24h clock default, /display?clock=12h override, max 3 sessions, silently hide schedule when Edge Function unreachable) - Tech.md: 8 architecture decisions -- idle_snapshot event type extension, Realtime HTTP broadcast API, Supabase cron schedule, program hierarchy JOIN query, server_time drift correction, CSS-only transitions, TanStack Router search param validation - Steps.md: 10 tasks across 4 phases, frontend-specialist + backend-engineer + quality-engineer, parallel Phase 1 (types + Edge Function) and Phase 2 (ClockDisplay + IdleView + transition wrapper) * feat(display): implement idle mode with Edge Function broadcast Adds idle mode to the /display route -- the ambient state shown when no workouts are active. A server-side Edge Function queries today's remaining scheduled sessions every 60 seconds and broadcasts them to the display channel so the TV always shows useful information. - IdleSnapshot Zod schema + idle_snapshot event type in DisplayEventType - ClockDisplay component (8rem Space Grotesk, server-time drift correction) - IdleView component (clock, date, sessions list, next-up badge, footer) - useDisplayMode hook deriving idle/board/focused from session map - useIdleSnapshot hook subscribing to display Broadcast channel - DisplayModeTransition wrapper (300ms fade for idle↔board, 400ms zoom for board↔focused) - /display route composed end-to-end with all hooks and components - display-idle-snapshot Edge Function with cron every 60 seconds - get_display_idle_sessions RPC (service_role only, filters completed sessions) - /display bypasses config guard in __root.tsx (public, no-auth route) - docs/self-hosting.md: Remote Display section Tasks: S001, S001-T, S002, S003, S003-T, S004, S005, S005-T, S006, S006-T, S007, S008, S009, S009-D Feature: 009-idle-mode * docs(planning): mark feature 009 idle mode as complete Feature: 009-idle-mode * fix(display): address QA findings from idle mode review Frontend: - Refactor useDisplayMode to use setState-during-render pattern (React "storing info from previous renders") instead of refs-in-useMemo, fixing react-hooks/refs ESLint violations and the ref-as-dep stability bug - Add NaN guard and 60s periodic re-sync to ClockDisplay offset to handle NTP corrections and DST transitions on long-running displays - Wire real Realtime channel status to connectionStatus in display route, replacing the hardcoded 'connected' placeholder - Fix useIdleSnapshot cleanup to safely void removeChannel promise; add dev-mode console.warn on safeParse failures for easier debugging - Fix IdleView session key to use display_name+session_name instead of array index - Remove redundant cleanup() calls in display test afterEach blocks Backend: - Add day_of_week IS NOT NULL guard to get_display_idle_sessions RPC to prevent silent exclusion of sessions with unset day_of_week - Add composite index idx_workout_logs_session_user_date to prevent full table scan on the cron-driven workout_logs LEFT JOIN - Replace CREATE OR REPLACE with DROP IF EXISTS + CREATE for atomic GRANT application - Remove non-null assertions on sessions[0] in Edge Function - Add LIMIT 3 comment and verify_jwt rationale comment in config - Improve Edge Function error messages to include error codes and response status for easier production debugging Feature: 009-idle-mode * fix(display): resolve PR review findings from 0006-idle-mode-review Fixes: clock drift bug (P6-001), null client silence (P6-002), lying connection status (P6-003), broad path bypass (P6-004), duplicate channel (P6-005), missing CLOSED handling (P6-006), production-silent parse errors (P6-007), duplicate DisplayMode type (P6-008), unsafe RPC cast (P6-009), redundant comments (P6-010) Tasks: S011-S014 added to Steps.md for missing test coverage Rules: .min(1) on display-facing strings, Tech.md D4 corrected Review: Context/Reviews/0006-idle-mode-review.md * test(display): add review remediation tests S011-S014 S011: useIdleSnapshot hook tests (9 tests) -- valid/invalid payload, null client, cleanup, connection status callback for all channel states S012: DisplayModeTransition tests (8 tests) -- transition class mapping for all mode pairs including duration assertions S013: IdleSnapshot schema edge cases (7 tests) -- invalid session_type, empty arrays, invalid ISO dates, .min(1) enforcement S014: ClockDisplay correction test strengthened to assert exact time value Review: Context/Reviews/0006-idle-mode-review.md * docs(reviews): mark 0006-idle-mode-review as verified All 16 findings confirmed resolved by review-verify agent. Feature: 009-idle-mode * fix(display): add onIdleSnapshot to subscriber test mock handlers Fixes TS build error from merge integration -- the test's mock handlers were missing the new onIdleSnapshot callback added to DisplayEventHandlers. * chore(harness): update skills, hooks, and Claude Code settings * docs: update hosting PRD and add onboarding refactoring plans * feat(onboarding): QR code gen + scan for server setup (#64) * docs(planning): add Feature 010 QR code gen + scan blueprint Spec, Tech, and Steps for Refactor B of the onboarding redesign (QR code generation in Settings + scan/paste on setup screen). Covers invite link format, qrcode.react config, barcode scanner plugin setup, and isTauri()-based platform branching. Updates onboarding-refactoring-plans.md with researched plugin specifics. Feature: 010-qr-code-gen-scan * feat(onboarding): add QR code server sharing and scan on setup Adds QR-based server sharing to reduce friction for onboarding new users to a self-hosted Ardent Forge instance. - Settings backend section now shows a "Share this server" area with a QRCodeSVG encoding `ardentforge://connect?url=...&key=...` and a "Copy invite link" button, guarded by hasConfig() - Setup screen QR button activates native camera scan via @tauri-apps/plugin-barcode-scanner (Tauri/Android) or toggles an inline paste field (browser); both paths parse the invite link and auto-trigger connection validation - New src/lib/invite-link.ts provides buildInviteLink() and parseInviteLink() as a pure shared utility - Barcode scanner plugin registered under #[cfg(mobile)] and barcode-scanner:default added to mobile capabilities Tasks: S001, S002, S003, S004, S005, S006 Feature: 010-qr-code-gen-scan * fix(qr-scan): address QA findings from code review - Add .catch() to unawaited processInviteLink() calls in paste/keydown handlers to prevent silent unhandled promise rejections - Replace duplicate dynamic import of cancel with cancelRef to avoid redundant module resolution on scan overlay dismiss - Change perms === 'denied' guard to perms !== 'granted' for defensive permission handling on non-standard permission states - Move tauri-plugin-barcode-scanner to target-specific mobile dep in Cargo.toml to avoid enlarging desktop binary - Remove unreachable pathname fallback branch from parseInviteLink - Add comment to buildInviteLink documenting publishable key intent - Rename misleading test and add true malformed-URL test coverage - Apply rustfmt formatting to db.rs Feature: 010-qr-code-gen-scan * fix(qr-scan): resolve P7 review findings and add component tests Fix bare catch blocks in handleCopy and connection-validator, add try/catch to processInviteLink to prevent UI getting stuck, handle unhandled promise from handleScan onClick, add user-facing error toasts for clipboard failures, fix buildInviteLink to use SCHEME/HOST constants, downgrade parse error log to silent null return, add aria-label to paste field, improve QR scan failure toast message. Add component tests for processInviteLink valid/invalid flows and backend-settings share section conditional rendering and copy button. Tasks: S006-T1, S006-T2 Feature: 010-qr-code-gen-scan Review: Context/Reviews/0007-pr64-qr-code-gen-scan-review.md * docs(planning): resolve P7 review and add S006-T1/T2 tasks to Steps.md Mark all 14 findings in the QR code gen/scan review as resolved. Add S006-T1 (processInviteLink component tests) and S006-T2 (backend-settings share section tests) to Feature 010 Steps.md. Feature: 010-qr-code-gen-scan * docs(review): mark P7 review verified by review-verify agent * refactor(review-resolve): simplify bulk triage to bulk accept Renames bulk triage to bulk accept to better reflect the workflow -- the capture category already IS the suggested action. Removes the requirement for a reason on discard (individual and bulk). * chore(harness): add gitignore entries and reformat hooks Adds .claude/backups/, .claude/worktrees/, .serena/, and hooks/skill-activation/.state/ to .gitignore. Reformats all hook TypeScript files to double-quote style for consistency. * feat(builders): progressive disclosure and contextual help (Step 14.5) * docs(planning): add Step 14.5 feature plan and user flow docs Documents the progressive disclosure and contextual help plan for the template builder and program builder (Step 14.5). Adds Flow 7 builder progressive disclosure and contextual help system diagrams to 10-user-flows.md. Feature: 010-Progressive-Disclosure Tasks: S001–S014-T * feat(builders): progressive disclosure and contextual help (Step 14.5) Adds context-aware field visibility, staged group creation, set scheme filtering, and inline help to the template and program builders. New primitives and utilities: - HelpTrigger component (Popover desktop / Drawer mobile via useMediaQuery) - shadcn Popover and Drawer with Iron & Ember styling - useMediaQuery hook (SSR-safe, listener cleanup) - CollapsedFieldsRow for expandable hidden field groups - visibility-maps.ts: data-driven lookup tables for all field visibility rules - help-content.ts: GROUP_TYPE_HELP, BLOCK_TYPE_HELP, SOURCE_HELP constants - text-label-large and text-body-small typography utilities in index.css Template builder changes: - Category-aware Scoring/Time Cap visibility with CollapsedFieldsRow - Activity group staged reveal: Stage 1 (type selector only), Stage 2 (type- dependent rest fields + exercises) - Set scheme type list filtered by session category; "Show all types" escape - Load type options filtered by set scheme type; null = hide entire section - Session picker gains "Create new event" inline creation flow Program builder changes: - Block type HelpTrigger with one-liner summary (desktop + mobile) - Source badge HelpTrigger with all 7 sources Tasks: S001, S002, S003, S004, S005, S006, S007, S008, S009, S010, S011, S012, S013 Feature: 010-Progressive-Disclosure * fix(builders): resolve QA findings from progressive disclosure review Fixes two blocking hook violations and seven warnings across the builder components introduced in Step 14.5. Errors fixed: - Move useCallback above early return in LoadSpecEditor to satisfy Rules of Hooks (conditional hook call) - Rewrite useMediaQuery with useSyncExternalStore to eliminate synchronous setState in effect and double render Warnings fixed: - Remove eslint-disable suppression in LoadSpecEditor; expand useEffect deps to [allowedLoads, availableTypes, value.type, onChange] - Replace div[role=button] nested inside + ) + + if (isDesktop) { + return ( + + {/* ... */} + + ) + } + return ( + + {/* ... */} + + ) +} +``` + +The `useMediaQuery` hook does not exist in the codebase. Options: + +- Add a lightweight custom hook (~10 lines) using `window.matchMedia` +- The shadcn Drawer docs recommend this pattern and provide an example + +### D2. Category visibility: data-driven mapping + +**Decision:** Define a `CATEGORY_FIELD_VISIBILITY` constant mapping `SessionType` to field visibility flags, rather than inline conditionals. + +**Rationale:** The visibility rules are a pure function of category. A lookup table is more readable, testable, and extensible than scattered `if/else` chains. + +```typescript +const CATEGORY_FIELD_VISIBILITY: Record = { + STRENGTH: { scoring: false, timeCap: false }, + CONDITIONING: { scoring: true, timeCap: true }, + SE: { scoring: false, timeCap: true }, + MIXED: { scoring: true, timeCap: true }, + EVENT: { scoring: false, timeCap: false }, // unused but safe +} +``` + +### D3. Set scheme filtering: data-driven mapping + +**Decision:** Define a `CATEGORY_SCHEME_TYPES` constant mapping `SessionType` to the default-visible set scheme types. + +```typescript +import type { SetScheme } from '@/domain/types/set-scheme' + +const CATEGORY_SCHEME_TYPES: Record = { + STRENGTH: ['fixedSets', 'percentageSets', 'workToMax'], + CONDITIONING: [ + 'cardioSteadyState', + 'cardioInterval', + 'ruckMarch', + 'emom', + 'amrapTimed', + 'descendingReps', + ], + SE: ['forReps', 'timedHold', 'percentageOfMaxReps'], + MIXED: [], // empty = show all + EVENT: [], +} +``` + +When `CATEGORY_SCHEME_TYPES[category]` is empty, all types are shown and the "Show all types" button is hidden. + +### D4. Load filtering: data-driven mapping + +**Decision:** Define a `SCHEME_LOAD_VISIBILITY` constant mapping `SetScheme['type']` to either `null` (load section hidden) or an array of visible `LoadSpec['type']` values. + +```typescript +const SCHEME_LOAD_VISIBILITY: Record = { + fixedSets: ['absolute', 'rpe', 'bodyweight', 'bodyweightPlus', 'unspecified'], + percentageSets: null, // hidden + workToMax: ['absolute', 'rpe', 'unspecified'], + forReps: ['absolute', 'bodyweight', 'bodyweightPlus', 'unspecified'], + timedHold: ['absolute', 'bodyweight', 'bodyweightPlus', 'unspecified'], + percentageOfMaxReps: null, // hidden + cardioSteadyState: null, // hidden + cardioInterval: null, // hidden + ruckMarch: null, // hidden + emom: ['absolute', 'rpe', 'bodyweight', 'bodyweightPlus', 'unspecified'], + amrapTimed: ['absolute', 'rpe', 'bodyweight', 'bodyweightPlus', 'unspecified'], + descendingReps: ['absolute', 'rpe', 'bodyweight', 'bodyweightPlus', 'unspecified'], +} +``` + +### D5. Group field visibility: data-driven mapping + +**Decision:** Extend the existing `GROUP_TYPES_WITH_ROUNDS` pattern into a full visibility map. + +```typescript +const GROUP_FIELD_VISIBILITY: Record< + GroupType, + { + restBetweenRounds: boolean + restBetweenActivities: boolean + rounds: boolean + } +> = { + STRAIGHT_SETS: { restBetweenRounds: false, restBetweenActivities: true, rounds: false }, + SUPERSET: { restBetweenRounds: false, restBetweenActivities: true, rounds: false }, + CIRCUIT: { restBetweenRounds: true, restBetweenActivities: true, rounds: true }, + COMPLEX: { restBetweenRounds: false, restBetweenActivities: false, rounds: false }, + EMOM: { restBetweenRounds: false, restBetweenActivities: false, rounds: false }, + AMRAP: { restBetweenRounds: false, restBetweenActivities: false, rounds: false }, + COUPLET: { restBetweenRounds: false, restBetweenActivities: true, rounds: true }, +} +``` + +### D6. Staged group reveal: `groupType` starts as `null` + +**Decision:** Change the initial group creation to set `groupType: null` instead of defaulting to `STRAIGHT_SETS`. The `ActivityGroupEditor` checks for `null` and renders Stage 1 (type selector only). After selection, it transitions to Stage 2. + +**Impact on types:** The `ActivityGroupData` type (local to the form, not the domain schema) needs `groupType: GroupType | null`. The domain `ActivityGroup` Zod schema is unaffected since groups are only saved after type selection. + +**Impact on `handleAddGroup`:** No longer creates a default activity. The first activity is added after type selection (Stage 2) or when the user taps "+ Select exercise". + +### D7. Help content: co-located static constants + +**Decision:** Store help text as static constants in a dedicated file `src/components/builders/help-content.ts` rather than in each component. + +**Rationale:** Centralizes all help copy for easy review and editing. These are not i18n strings (per Won't Have), just organized constants. + +```typescript +export const GROUP_TYPE_HELP: Record = { ... } +export const BLOCK_TYPE_HELP: Record = { ... } +export const SOURCE_HELP: Record = { ... } +``` + +--- + +## Prop Threading + +New props that need to flow through the component tree: + +| Prop | From | To | Type | +| ---------------------- | --------------------------- | ---------------------------------------------------------------- | ---------------------- | +| `sessionCategory` | `SessionTemplateForm` | `SetSchemeEditor` (via `ActivityGroupEditor` > `ActivityEditor`) | `SessionType` | +| `showAllTypes` | `SessionTemplateForm` state | `SetSchemeEditor` (via same path) | `boolean` | +| `onShowAllTypesChange` | `SessionTemplateForm` | `SetSchemeEditor` (via same path) | `(v: boolean) => void` | +| `schemeType` | `SetSchemeEditor` | `LoadSpecEditor` | `SetScheme['type']` | + +Threading `sessionCategory` through 3 levels (Form > GroupEditor > ActivityEditor > SetSchemeEditor) is acceptable for this bounded hierarchy. React Context would be overengineered for a single prop. + +--- + +## Risk Assessment + +| Risk | Likelihood | Impact | Mitigation | +| ---------------------------------------------------------------------------- | ---------- | ------ | ----------------------------------------------------------------------------------------------------- | +| Set scheme editor (1205 lines) is large; changes could introduce regressions | Medium | High | Unit test the visibility mapping constants independently; manual testing of all 12 scheme types | +| Staged group reveal changes `handleAddGroup` default behavior | Low | Medium | The change is additive (null check gates Stage 1); existing save validation catches incomplete groups | +| "Show all types" state threading through 3 levels adds prop drilling | Low | Low | Bounded hierarchy; if it grows, extract to context later | +| Drawer (vaul) adds a new dependency | Low | Low | It's a standard shadcn component; consistent with existing pattern | +| Category change + value preservation could cause stale form state | Medium | Medium | Values are preserved in React state; only UI visibility changes. Validation runs on save. | + +--- + +## New Dependencies + +| Package | Version | Purpose | Size | +| ------------------------- | ------- | ----------------------------------------- | ----- | +| `@radix-ui/react-popover` | latest | Popover primitive for HelpTrigger desktop | ~10KB | +| `vaul` | latest | Drawer primitive for HelpTrigger mobile | ~8KB | + +Both are installed via shadcn CLI (`bunx --bun shadcn@latest add popover drawer`). + +--- + +## Files Changed Summary + +| Category | Files | New/Modified | +| ---------------- | --------------------------------------------------------------------------------- | ------------------------------- | +| UI primitives | `popover.tsx`, `drawer.tsx`, `help-trigger.tsx` | New | +| Shared constants | `help-content.ts` | New | +| Custom hook | `use-media-query.ts` | New | +| Template builder | `session-template-form.tsx`, `activity-group-editor.tsx`, `set-scheme-editor.tsx` | Modified | +| Template builder | `collapsed-fields-row.tsx` | New | +| Program builder | `block-editor.tsx`, `mobile-block-editor.tsx`, `program-form.tsx` | Modified | +| Styles | `src/index.css` | Modified (typography utilities) | +| **Total** | **12 files** | 6 new, 6 modified | diff --git a/Context/Features/010-qr-code-gen-scan/Spec.md b/Context/Features/010-qr-code-gen-scan/Spec.md new file mode 100644 index 00000000..91bbdc7f --- /dev/null +++ b/Context/Features/010-qr-code-gen-scan/Spec.md @@ -0,0 +1,71 @@ +# Feature 010: QR Code Generation + Scan on Setup + +## Phase 1 -- Specification + +### Overview + +Add QR code-based server sharing to Ardent Forge. A configured user can generate a QR code (and copyable invite link) from Settings that encodes their backend connection details. A new user can scan that QR code (mobile) or paste the invite link (browser/desktop) on the setup screen to auto-configure their app without manually typing Supabase credentials. + +### Problem Statement + +Onboarding new users to a self-hosted Ardent Forge instance currently requires them to manually enter a server URL or raw Supabase credentials. This is error-prone and unfriendly, especially on mobile devices. There is no way for an existing user to share their server configuration with others. + +### User Stories + +1. **As a configured user**, I want to display a QR code in Settings so I can show it to someone who needs to connect to my server. +2. **As a configured user**, I want to copy an invite link so I can send it via chat/email to someone who needs to connect. +3. **As a new mobile user**, I want to scan a QR code on the setup screen so I can connect without typing URLs. +4. **As a new browser/desktop user**, I want to paste an invite link on the setup screen so I can connect without typing URLs. +5. **As a new user**, I want scanned/pasted credentials to auto-validate so I can proceed to account creation immediately. + +### Requirements + +#### Must Have + +- **M1**: Settings Backend section shows "Share this server" area with QR code encoding `ardentforge://connect?url={supabaseUrl}&key={publishableKey}` +- **M2**: "Copy invite link" button copies the `ardentforge://connect?...` deep link string to clipboard with success toast +- **M3**: QR code and invite link section only renders when `configStore.hasConfig()` is true +- **M4**: Setup screen QR scan button opens native camera via `@tauri-apps/plugin-barcode-scanner` on Android +- **M5**: Barcode scanner uses `windowed: true` mode with dark overlay and viewfinder cutout +- **M6**: Browser mode replaces camera scan with a "Paste invite link" text input +- **M7**: Successful scan/paste of a valid `ardentforge://connect` URL extracts `url` and `key` params, pre-populates the manual config fields, expands the Advanced section, and auto-triggers validation +- **M8**: Invalid QR/link content shows error toast ("Invalid invite link") +- **M9**: Barcode scanner permissions added to `src-tauri/capabilities/mobile.json` +- **M10**: Existing setup flow (server URL discovery and manual entry) continues to work unchanged + +#### Should Have + +- **S1**: Explainer text below QR code: "Share this with anyone who wants to connect to this server. They will still need to create an account." +- **S2**: QR code renders on `surface-iron` card background, centered, with Iron and Ember styling + +#### Will Not Have + +- **W1**: Deep link handler for `ardentforge://connect` (Refactor C scope) +- **W2**: QR code expiration or revocation +- **W3**: Embedded credentials in QR (only URL + publishable key, which are non-secret) + +### Testable Assertions + +| ID | Assertion | 验证 Method | +| ---- | ---------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------- | +| TA-1 | QR code in Settings encodes `ardentforge://connect?url={encoded}&key={encoded}` with current config values | Unit test: render component, decode QR SVG content | +| TA-2 | Copy button writes correct deep link string to clipboard | Integration test: click button, read clipboard | +| TA-3 | QR section does not render when config store is empty | Unit test: render with empty config, assert section absent | +| TA-4 | Tauri renders native scan button; browser renders paste field | Platform-conditional render test (`isTauri()` branch) | +| TA-5 | Browser paste field parses valid `ardentforge://connect` URL correctly | Unit test: parse function with valid/invalid inputs | +| TA-6 | Invalid link shows toast with "Invalid invite link" | Unit test: trigger invalid input, assert toast call | +| TA-7 | Valid scan/paste pre-populates Advanced fields and triggers validation | Integration test: simulate input, assert field values and validation call | +| TA-8 | Existing setup flows (discovery + manual) still work | Regression: existing test suite passes | + +### Open Questions + +None -- all resolved: + +1. ~~**QR code size**~~: Resolved -- determine optimal dimensions during Phase 2 research. +2. ~~**iOS plist**~~: Resolved -- iOS is not a supported target. Dropped M11 (NSCameraUsageDescription). + +### Dependencies + +- `qrcode.react` (new dependency for QR generation) +- `@tauri-apps/plugin-barcode-scanner` (new dependency for mobile QR scanning) +- Existing: `sonner` (toasts), `lucide-react` (QrCode icon already imported in setup.tsx), config store, connection validator diff --git a/Context/Features/010-qr-code-gen-scan/Steps.md b/Context/Features/010-qr-code-gen-scan/Steps.md new file mode 100644 index 00000000..99dcc54f --- /dev/null +++ b/Context/Features/010-qr-code-gen-scan/Steps.md @@ -0,0 +1,325 @@ +# Feature 010: QR Code Generation + Scan on Setup + +## Phase 3 -- Implementation Steps + +### Team Composition + +| Role | Agent | Stacks | +| ------------------- | ---------- | -------------------------------------------- | +| Frontend Specialist | `frontend` | React, TypeScript, Tailwind, TanStack Router | +| Tauri Specialist | `tauri` | Rust, Tauri plugins, capabilities, Cargo | +| Quality Engineer | `qa` | Testing, validation, regression | + +### Dependency Graph + +``` +S001 (deps) ──┐ +S002 (invite) ─┤──► M1: Milestone -- Foundation + │ +S003 (tauri) ──┘ + │ +S004 (settings QR) ─────► M2: Milestone -- QR Generation + │ +S005 (setup scan/paste) ─► M3: Milestone -- QR Consumption + │ +S006 (tests) ─────────────► M4: Milestone -- Validation +``` + +--- + +### Wave 1: Foundation (parallel) + +#### S001: Install frontend dependencies + +**Agent:** `frontend` +**Files:** `package.json` +**Depends on:** nothing +**Parallel:** yes (with S002, S003) + +1. Run `bun add qrcode.react` +2. Verify `qrcode.react` appears in `package.json` dependencies +3. Run `bun install` to confirm clean lockfile + +**Done:** `qrcode.react` in dependencies, no lockfile errors. + +--- + +#### S002: Create invite link utility + +**Agent:** `frontend` +**Files:** `src/lib/invite-link.ts` (NEW) +**Depends on:** nothing +**Parallel:** yes (with S001, S003) + +1. Create `src/lib/invite-link.ts` with two exported functions: + +```typescript +export function buildInviteLink(supabaseUrl: string, publishableKey: string): string +``` + +- Returns `ardentforge://connect?url=${encodeURIComponent(supabaseUrl)}&key=${encodeURIComponent(publishableKey)}` + +```typescript +export function parseInviteLink(raw: string): { url: string; key: string } | null +``` + +- Parse input as URL. If scheme is not `ardentforge:` or path/host is not `connect`, return `null`. +- Extract `url` and `key` query params. If either is missing or empty after decoding, return `null`. +- Return `{ url, key }`. +- Handle edge cases: extra whitespace (trim), missing protocol, garbage input (try/catch URL constructor). + +2. No side effects, no imports beyond standard `URL` API. + +**Done:** Both functions exported, pure logic, no dependencies on app state. + +**Assertions:** TA-5 + +--- + +#### S003: Register barcode scanner Tauri plugin + +**Agent:** `tauri` +**Files:** `src-tauri/Cargo.toml`, `src-tauri/src/lib.rs`, `src-tauri/capabilities/mobile.json`, `package.json` +**Depends on:** nothing +**Parallel:** yes (with S001, S002) + +1. Run `bun tauri add barcode-scanner` -- this handles: + - Adding `tauri-plugin-barcode-scanner` to `src-tauri/Cargo.toml` + - Adding `@tauri-apps/plugin-barcode-scanner` to `package.json` + - Registering the plugin in `src-tauri/src/lib.rs` (`.plugin(tauri_plugin_barcode_scanner::init())`) + +2. Verify `src-tauri/src/lib.rs` has the plugin registered in the builder chain alongside existing plugins (log, notification, deep_link, opener). + +3. Add `"barcode-scanner:default"` to the permissions array in `src-tauri/capabilities/mobile.json`. This grants: `allow-scan`, `allow-cancel`, `allow-check-permissions`, `allow-request-permissions`, `allow-open-app-settings`, `allow-vibrate`. + +4. Verify the Rust project compiles: `cd src-tauri && cargo check` + +**Done:** Plugin registered, permissions granted, Rust compiles. + +**Assertions:** TA-4 (partial -- plugin available) + +--- + +### >> M1: Foundation Milestone + +**Gate:** S001 + S002 + S003 all complete. +**Verify:** `bun install` clean, `invite-link.ts` exists with both exports, `cargo check` passes. +**Assertions covered:** TA-5 (invite link parsing) + +--- + +### Wave 2: QR Generation in Settings + +#### S004: Add "Share this server" section to backend settings + +**Agent:** `frontend` +**Files:** `src/components/profile/backend-settings.tsx` +**Depends on:** S001 (qrcode.react installed), S002 (buildInviteLink available) +**Parallel:** no + +1. Import `QRCodeSVG` from `qrcode.react` and `buildInviteLink` from `src/lib/invite-link.ts`. + +2. Add a new section below the existing backend URL display and "Change backend" button area. Structure: + - Section heading: "Share this server" -- use existing section heading pattern: `font-sans text-xs font-medium uppercase tracking-widest text-warm-ash` + - QR code card: `bg-surface-iron p-6` container, centered. Render `QRCodeSVG` with: + - `value={buildInviteLink(config.supabaseUrl, config.supabaseKey)}` + - `size={256}` + - `level="M"` + - `marginSize={4}` + - `fgColor="#e5e2e1"` (bone-white) + - `bgColor="#201f1f"` (surface-iron) + - `title="Scan to connect to this Ardent Forge server"` + - "Copy invite link" button: secondary style button below the QR code. On click: + - `navigator.clipboard.writeText(buildInviteLink(config.supabaseUrl, config.supabaseKey))` + - Show `toast('Invite link copied')` via sonner + - Explainer text: `text-sm text-warm-ash/70` -- "Share this with anyone who wants to connect to this server. They will still need to create an account." + +3. Guard the entire section with `configStore.hasConfig()` (should always be true on Settings page, but makes the contract explicit). + +4. Read the existing `backend-settings.tsx` patterns for state access, styling classes, and toast usage. Match exactly. + +**Done:** QR code and copy button visible in Settings when configured. + +**Assertions:** TA-1, TA-2, TA-3, S1, S2 + +--- + +### >> M2: QR Generation Milestone + +**Gate:** S004 complete. +**Verify:** Settings page shows QR code with correct encoded value. Copy button works. Section hidden when config empty. +**Assertions covered:** TA-1, TA-2, TA-3 + +--- + +### Wave 3: QR Consumption on Setup Screen + +#### S005: Activate QR scan/paste on setup screen + +**Agent:** `frontend` +**Files:** `src/routes/setup.tsx` +**Depends on:** S002 (parseInviteLink), S003 (barcode scanner plugin registered) +**Parallel:** no + +1. Read `src/routes/setup.tsx` thoroughly. Note the existing disabled QR button at ~line 188-195 and the manual configuration (Advanced) section state. + +2. **Replace the disabled QR button** with platform-branched behavior: + + **Tauri branch (`isTauri()` is true -- Android):** + - QR icon button becomes active (remove `disabled`, `opacity-30`, `cursor-not-allowed`). + - On tap, execute the camera permission + scan flow: + + ```typescript + import { + scan, + cancel, + checkPermissions, + requestPermissions, + openAppSettings, + Format, + } from '@tauri-apps/plugin-barcode-scanner' + + async function handleScan() { + let perms = await checkPermissions() + if (perms === 'prompt') perms = await requestPermissions() + if (perms === 'denied') { + toast('Camera permission required') + await openAppSettings() + return + } + const result = await scan({ windowed: true, formats: [Format.QRCode] }) + await cancel() + processInviteLink(result.content) + } + ``` + + - Before calling `scan()`, set a `scanning` state that applies a transparent/overlay CSS class to the setup page so the native camera feed shows through. + - After `cancel()`, remove the overlay. + + **Browser branch (`!isTauri()`):** + - QR icon button toggles a paste input field inline. + - `ForgeInput` with placeholder "Paste invite link", matching existing underline input styling. + - On Enter keypress or paste event, extract the value and call `processInviteLink(value)`. + +3. **Shared `processInviteLink(raw: string)` function** (local to setup.tsx): + - Call `parseInviteLink(raw)` from `src/lib/invite-link.ts`. + - If `null`, show `toast('Invalid invite link')` and return. + - If valid: + - Set `showAdvanced` to `true` (expand the Advanced section). + - Pre-populate the `advancedUrl` and `advancedKey` state fields with parsed values. + - Auto-trigger validation by calling the existing `handleAdvancedConnect()` function (or equivalent manual validation path). + +4. **Scanning overlay component** (Tauri only): + - When `scanning` state is true, render a full-screen overlay with: + - `bg-black/80` background (semi-transparent dark). + - Centered viewfinder cutout (transparent rectangle, ~280x280px, with `border-2 border-ember` outline). + - "Cancel" button at bottom that calls `cancel()` and clears the `scanning` state. + - The actual camera feed renders natively underneath the transparent WebView. + +5. Keep all existing setup screen behavior (server URL discovery, manual config, env var auto-detect) completely unchanged. + +**Done:** QR scan works on Android, paste works in browser, both feed into validation. + +**Assertions:** TA-4, TA-6, TA-7 + +--- + +### >> M3: QR Consumption Milestone + +**Gate:** S005 complete. +**Verify:** Scan button active on Tauri, paste field on browser, both parse and auto-validate. Invalid input shows toast. Existing flows unaffected. +**Assertions covered:** TA-4, TA-5, TA-6, TA-7 + +--- + +### Wave 4: Validation + +#### S006: Tests and regression + +**Agent:** `qa` +**Files:** `src/lib/__tests__/invite-link.test.ts` (NEW), existing test suite +**Depends on:** S004, S005 +**Parallel:** no + +1. **Unit tests for `invite-link.ts`:** + - `buildInviteLink` produces correct URL format with encoded params + - `parseInviteLink` round-trips with `buildInviteLink` output + - `parseInviteLink` returns `null` for: empty string, random text, wrong scheme (`https://connect?...`), missing `url` param, missing `key` param, malformed URL + - `parseInviteLink` handles: extra whitespace, trailing newlines + +2. **Run existing test suite:** `bun run test` -- all existing tests pass (regression check). + +3. **Run linter:** `bun run lint` -- no new lint errors. + +4. **Run type check:** `bun run build` (includes `tsc` check) -- no type errors. + +**Done:** All tests pass, no regressions, no lint/type errors. + +**Assertions:** TA-5, TA-8 + +--- + +#### S006-T1: Component tests for processInviteLink valid/invalid flows + +**Agent:** `qa` +**Files:** `src/routes/__tests__/setup.test.tsx` (NEW) +**Depends on:** S005, S006 +**Parallel:** no + +1. Create component test file for the setup page QR/paste flow: + - Test that a valid invite link populates url/key fields, expands Advanced section, triggers validation + - Test that an invalid invite link shows "Invalid invite link" toast and leaves fields unchanged + - Mock `validateConnection`, `getConfigStore`, and `parseInviteLink` as needed + +**Done:** Component tests verify processInviteLink valid/invalid paths. + +**Assertions:** TA-6, TA-7 + +--- + +#### S006-T2: Component tests for "Share this server" conditional rendering and copy button + +**Agent:** `qa` +**Files:** `src/components/profile/__tests__/backend-settings.test.tsx` (NEW) +**Depends on:** S004, S006 +**Parallel:** yes (with S006-T1) + +1. Create component test file for backend settings QR section: + - Test that QR section is absent when config is null and present when configured (TA-3) + - Test that copy button writes correct invite link to clipboard and shows toast (TA-2) + - Mock `getConfigStore` and `navigator.clipboard` + +**Done:** Component tests verify conditional rendering and copy behavior. + +**Assertions:** TA-2, TA-3 + +--- + +### >> M4: Validation Milestone (Final) + +**Gate:** S006 complete. +**Verify:** All unit tests pass, existing suite green, lint clean, types clean. +**Assertions covered:** All (TA-1 through TA-8) + +--- + +### Execution Summary + +| Wave | Steps | Agent(s) | Parallel | +| ---- | ---------------- | ------------------------- | ----------- | +| 1 | S001, S002, S003 | frontend, frontend, tauri | yes (all 3) | +| 2 | S004 | frontend | no | +| 3 | S005 | frontend | no | +| 4 | S006 | qa | no | + +**Total steps:** 6 +**Critical path:** S002 -> S004 -> S005 -> S006 +**Estimated execution:** All waves sequential after Wave 1 parallel foundation. + +### Recommended Execution + +``` +/impl 010 +``` + +Tasks are isolated by domain (frontend owns most work, tauri handles plugin registration, qa validates). No cross-domain coordination needed -- `/impl` over `/team-impl`. diff --git a/Context/Features/010-qr-code-gen-scan/Tech.md b/Context/Features/010-qr-code-gen-scan/Tech.md new file mode 100644 index 00000000..d106c914 --- /dev/null +++ b/Context/Features/010-qr-code-gen-scan/Tech.md @@ -0,0 +1,126 @@ +# Feature 010: QR Code Generation + Scan on Setup + +## Phase 2 -- Technical Design + +### Architecture Overview + +This feature adds two independent UI surfaces connected by a shared data format: + +1. **QR Generation (Settings)** -- New section in `backend-settings.tsx` that renders a QR code and copy button encoding the current backend config as an `ardentforge://connect` URL. +2. **QR Consumption (Setup)** -- Activates the existing disabled QR button in `setup.tsx` with platform-branched input (native camera scan on Android via Tauri, paste field on web browser). + +A shared **invite link utility** (`src/lib/invite-link.ts`) handles URL construction and parsing, used by both surfaces and reusable by Refactor C (deep link handler). + +### Key Decisions + +#### D1: QR Code Library -- `qrcode.react` + +**Choice:** `QRCodeSVG` from `qrcode.react` + +**Why:** Lightweight, client-side-only, zero external service calls, ships TypeScript types, and renders as inline SVG (no canvas or image export needed). The SVG output integrates cleanly with the Iron & Ember design system since we can control bg/fg colors via props. + +**Config:** + +- `size={256}` -- At the expected payload (~175 chars), the QR matrix is ~37x37 modules. At 256px, each module is ~6.9px, well above the ~2-3px minimum for reliable mobile camera scanning. +- `level="M"` -- 15% error correction. Standard for mobile scanning; balances density vs. recoverability. +- `marginSize={4}` -- QR spec-required quiet zone for scanner reliability. +- `fgColor="#e5e2e1"` (bone-white) / `bgColor="#201f1f"` (surface-iron) -- Inverted from traditional black-on-white to match Iron & Ember. Sufficient contrast ratio (~10:1) for scanner readability. + +#### D2: Platform Detection -- `isTauri()` (existing) + +**Choice:** Use the existing `isTauri()` check from `@tauri-apps/api/core`. No new dependency needed. + +**Why:** There is no desktop Tauri build -- the app runs as either mobile Tauri (Android) or web browser. `isTauri() === true` means Android, `isTauri() === false` means browser. This is the same pattern used throughout the codebase (config store, auth, sync bridge, etc.). + +**Implementation:** + +```typescript +import { isTauri } from '@tauri-apps/api/core' + +// Tauri = Android (native camera scan) +// Browser = paste field fallback +{isTauri() ? : } +``` + +#### D3: Barcode Scanner -- `@tauri-apps/plugin-barcode-scanner` + +**Choice:** Native scanner via `@tauri-apps/plugin-barcode-scanner` with `windowed: true`. + +**Why:** The plugin accesses the camera natively, bypassing WebView camera APIs entirely. The `windowed` mode makes the WebView transparent and renders the camera feed underneath -- the app renders a dark overlay with a viewfinder cutout. + +**Permission flow:** + +1. On scan button tap, call `checkPermissions()`. +2. If `'prompt'`, call `requestPermissions()`. +3. If `'denied'`, show toast with "Camera permission required" and call `openAppSettings()`. +4. If `'granted'`, proceed to `scan({ windowed: true, formats: [Format.QRCode] })`. +5. On result, call `cancel()` to dismiss camera, then process `result.content`. + +**Setup:** `bun tauri add barcode-scanner` handles Rust crate + JS bindings. Add `barcode-scanner:default` to `src-tauri/capabilities/mobile.json`. + +#### D4: Invite Link Format + +**Format:** `ardentforge://connect?url={encodedSupabaseUrl}&key={encodedPublishableKey}` + +**Why:** Uses the existing `ardentforge://` custom URL scheme already registered for deep links. The `/connect` path distinguishes it from the existing `/auth` path used for OAuth callbacks. Query params are URI-encoded. + +**Shared utility (`src/lib/invite-link.ts`):** + +```typescript +export function buildInviteLink(supabaseUrl: string, publishableKey: string): string +export function parseInviteLink(raw: string): { url: string; key: string } | null +``` + +`parseInviteLink` returns `null` for any string that isn't a valid `ardentforge://connect` URL with both params present. This function is reused by: + +- Settings copy button (build) +- Setup scan/paste handler (parse) +- Refactor C deep link handler (parse) -- future + +#### D5: Browser Fallback -- Paste Field + +**Choice:** Inline text input toggled by the QR icon button, not a modal or dialog. + +**Why:** Consistent with the setup screen's existing underline input pattern. No camera APIs needed in the browser. The user pastes an `ardentforge://connect?...` string, and on Enter/blur the app parses and auto-validates. + +**UX flow:** + +1. Tap QR icon button (same position as mobile scan button). +2. A `ForgeInput` appears inline with placeholder "Paste invite link". +3. On paste or Enter, run `parseInviteLink()`. +4. Valid: expand Advanced section, pre-populate fields, auto-trigger validation. +5. Invalid: toast "Invalid invite link", clear the input. + +### Files Modified + +| File | Change | +| --------------------------------------------- | --------------------------------------------------------------- | +| `src/lib/invite-link.ts` | **NEW** -- `buildInviteLink()`, `parseInviteLink()` | +| `src/components/profile/backend-settings.tsx` | Add "Share this server" section with QR code + copy button | +| `src/routes/setup.tsx` | Activate QR scan button; add platform-branched scan/paste logic | +| `src-tauri/src/lib.rs` | Register `tauri_plugin_barcode_scanner` | +| `src-tauri/Cargo.toml` | Add `tauri-plugin-barcode-scanner` dependency | +| `src-tauri/capabilities/mobile.json` | Add `barcode-scanner:default` permission | +| `package.json` | Add `qrcode.react`, `@tauri-apps/plugin-barcode-scanner` | + +### Integration Points + +- **Config store** (`src/lib/config-store.ts`): QR generation reads `getConfig()`. Setup scan/paste calls `setConfig()` + `initSupabaseFromConfig()` via existing `validateAndSave` flow. +- **Connection validator** (`src/lib/connection-validator.ts`): Unchanged. Scan/paste results feed into existing `validateConnection()`. +- **Toast system** (`sonner`): Used for copy success, invalid link errors, and camera permission denial. +- **Discovery flow** (`src/lib/discovery.ts`): Unchanged. QR/paste is an alternative input path that bypasses discovery entirely (goes straight to manual validation). + +### Risks and Mitigations + +| Risk | Severity | Mitigation | +| ------------------------------------------------------------------------------------ | -------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Inverted QR colors (bone-white on surface-iron) may not scan on some low-end cameras | Medium | Test on physical Android device. Fallback: offer a high-contrast toggle or use standard black-on-white with a card overlay. | +| Barcode scanner `windowed` mode requires WebView transparency CSS | Low | Well-documented pattern. Overlay component handles the transparent background and viewfinder cutout. | +| Publishable key in QR code | Low | Publishable keys are non-secret by design (equivalent to a public API key). The QR does not contain the service role key. Explainer text clarifies users still need to create an account. | + +### Testing Strategy + +- **Unit tests:** `invite-link.ts` -- `buildInviteLink` round-trips with `parseInviteLink`, edge cases (missing params, wrong scheme, extra params). +- **Component tests:** QR section renders/hides based on config state. Copy button calls clipboard API with correct string. +- **Manual testing:** Physical Android device scan with `windowed` mode. Browser paste flow. +- **Regression:** Existing setup flow tests pass unchanged. diff --git a/Context/Features/011-ci-cd-pipeline/Spec.md b/Context/Features/011-ci-cd-pipeline/Spec.md new file mode 100644 index 00000000..4d23bc48 --- /dev/null +++ b/Context/Features/011-ci-cd-pipeline/Spec.md @@ -0,0 +1,99 @@ +# Feature 011: CI/CD Pipeline for Android App + +**Status:** Draft +**Date:** 2026-04-04 +**Author:** Claude (Cortex) + +--- + +## Overview + +Establish a complete CI/CD pipeline using GitHub Actions for the Ardent Forge Android app. The pipeline covers PR validation (lint, typecheck, unit tests, E2E tests), Android debug/release builds, Google Play Store deployment, and Supabase migration management -- all from zero (no existing CI infrastructure). + +## Problem Statement + +Ardent Forge has no CI/CD infrastructure. All builds, tests, and deployments are manual. This creates risk of: + +- Regressions landing on `develop` undetected +- Inconsistent release builds (local machine variance) +- Manual, error-prone Play Store uploads +- Database migrations applied ad-hoc without validation +- No gate between code review and verified build success + +## User Stories + +1. **As a developer**, I want PRs to automatically run lint, typecheck, unit tests, and E2E tests so regressions are caught before merge. +2. **As a developer**, I want every PR to verify the Android app compiles so I know my changes don't break the build. +3. **As a release manager**, I want tagged releases to automatically build a signed AAB, upload it to Google Play, and apply Supabase migrations so the release process is hands-free. +4. **As a developer**, I want CI builds to be fast via aggressive caching so the feedback loop stays tight. + +## Requirements + +### Must Have + +| ID | Requirement | +| --- | --------------------------------------------------------------------------------------------------------------------------------------- | +| M1 | **PR Validation Workflow** -- On every PR to `develop`: run ESLint, TypeScript type-check (`tsc -b`), and Vitest unit tests | +| M2 | **E2E Testing on PRs** -- Run Playwright end-to-end tests as part of PR validation | +| M3 | **Android Debug Build on PR** -- Build a debug APK on every PR to verify Tauri+Gradle compilation succeeds | +| M4 | **Android Release Build on Tag** -- Build a signed release AAB/APK when a version tag (e.g., `v1.0.0`) is pushed | +| M5 | **Artifact Publishing** -- Upload signed AAB/APK as GitHub Release assets on tagged releases | +| M6 | **Google Play Store Deployment** -- Automatically upload the signed AAB to Google Play Console (internal/beta track) on tagged releases | +| M7 | **Supabase Migration on Release** -- Run Supabase migrations against the production database on tagged releases | +| M8 | **Secret Management** -- Android keystore, Play Console service account key, and Supabase credentials stored as GitHub Secrets | +| M9 | **Dependency Caching** -- Cache Bun dependencies, Cargo/Rust toolchain, Gradle builds, and Android SDK components | + +### Should Have + +| ID | Requirement | +| --- | ------------------------------------------------------------------------------------------------------------------------------------------- | +| S1 | **Version auto-injection** -- Derive `versionCode` from GitHub run number or tag; inject `versionName` from git tag into `tauri.properties` | +| S2 | **Build status badges** -- Add workflow status badges to README | +| S3 | **Concurrency controls** -- Cancel superseded workflow runs on the same PR | +| S4 | **Branch protection rules** -- Document required status checks for `develop` branch protection | + +### Won't Have (this phase) + +| ID | Exclusion | Rationale | +| --- | --------------------------------------- | ---------------------------------------------------------------------- | +| W1 | iOS build pipeline | No iOS target configured yet | +| W2 | Desktop (Linux/macOS/Windows) builds | Desktop distribution not prioritized | +| W3 | Play Store production track deployment | Start with internal/beta; promote manually until confidence is built | +| W4 | Automated rollback on migration failure | Requires more sophisticated migration tooling; manual rollback for now | + +## Testable Assertions + +| ID | Assertion | +| --- | --------------------------------------------------------------------------------------------------------------- | +| TA1 | A PR to `develop` triggers lint + typecheck + unit tests; failure blocks merge | +| TA2 | A PR to `develop` triggers Playwright E2E tests; failure blocks merge | +| TA3 | A PR to `develop` triggers an Android debug APK build; failure blocks merge | +| TA4 | Pushing a `v*` tag triggers a release build producing a signed AAB | +| TA5 | The signed AAB is uploaded as a GitHub Release asset | +| TA6 | The signed AAB is uploaded to Google Play Console internal/beta track | +| TA7 | Pushing a `v*` tag triggers Supabase migrations against the production instance | +| TA8 | CI caching reduces second-run build time by at least 40% compared to cold build | +| TA9 | All secrets (keystore, Play Console key, Supabase credentials) are injected via GitHub Secrets, never committed | + +## Open Questions (Resolved) + +| ID | Question | Resolution | +| --- | --------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------- | +| OQ1 | Which Google Play track should be the initial deployment target? | **Internal testing track.** Safest starting point; promote to beta/production manually from Play Console. | +| OQ2 | Should Supabase migrations run before or after the Play Store upload? | **Before.** If migrations break, abort the release before the app reaches users. Ensures API is ready when users update. | +| OQ3 | Tag-based release strategy? | **Pre-release tags (`-alpha`, `-beta`, `-rc`) from `develop`; stable tags from `main`.** Two workflow triggers with branch filtering. | +| OQ4 | Is there an existing Google Play Console project and service account? | **Yes.** Both exist and are ready for CI integration. | +| OQ5 | Should E2E tests run against local Supabase or a staging environment? | **Local Supabase** via `supabase start`. Hermetic, no external dependencies, Docker-based. Already configured in `docker-compose.yml`. | + +## Dependencies + +| Dependency | Type | Notes | +| -------------------------------------------- | -------- | ------------------------------------------------ | +| GitHub repository (rghsoftware/ardent-forge) | External | Workflows live in `.github/workflows/` | +| Google Play Console | External | Service account with upload permissions required | +| Supabase CLI | Tooling | For `supabase db push` in CI | +| Android SDK 36 + NDK | Tooling | Must be available on CI runner | +| Rust + Android cross-compilation targets | Tooling | `aarch64-linux-android`, etc. | +| Bun runtime | Tooling | For frontend build and test execution | +| Java 21 | Tooling | For Gradle/Android builds | +| Playwright | Tooling | For E2E test execution in CI | diff --git a/Context/Features/011-ci-cd-pipeline/Steps.md b/Context/Features/011-ci-cd-pipeline/Steps.md new file mode 100644 index 00000000..128f668c --- /dev/null +++ b/Context/Features/011-ci-cd-pipeline/Steps.md @@ -0,0 +1,398 @@ +# Feature 011: CI/CD Pipeline -- Implementation Steps + +**Status:** Draft +**Date:** 2026-04-04 +**Spec:** [Spec.md](./Spec.md) +**Tech:** [Tech.md](./Tech.md) + +--- + +## Team Composition + +| Role | Agent | Domain | +| ----------------- | ------------------- | ------------------------------------------------- | +| CI/CD Engineer | `cicd-engineer` | GitHub Actions workflows, caching, secrets | +| Backend Engineer | `backend-engineer` | Supabase migration job, E2E Supabase setup | +| Frontend Engineer | `frontend-engineer` | Playwright setup, E2E test scaffolding | +| Android Engineer | `android-engineer` | Gradle signing config, build commands, Play Store | +| Quality Engineer | `quality-engineer` | Validation of all workflows | + +--- + +## Wave 1: Foundation (parallel) + +### S001 -- Create `.github/workflows/` directory and CI validation workflow + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/ci.yml` +**Depends on:** none + +Create the PR validation workflow (`ci.yml`) with: + +- Trigger: `pull_request` targeting `develop` or `main` +- Concurrency: cancel in-progress runs per PR (`ci-${{ github.ref }}`) +- Job `validate`: + - `actions/checkout@v4` + - `oven-sh/setup-bun@v2` (pinned version) + - `actions/cache@v4` for `~/.bun/install/cache` keyed on `bun.lock` + - `bun install` + - `bun run lint` + - `bun run build` (includes `tsc -b` + Vite build) + - `bun run test` + +**Acceptance criteria:** + +- Workflow triggers on PRs to `develop` and `main` +- Lint, typecheck, and unit test failures block the PR +- Superseded runs are cancelled + +**Ref:** TA1 + +--- + +### S002 -- Add Playwright and E2E test scaffolding + +**Agent:** `frontend-engineer` +**Files:** `package.json`, `playwright.config.ts`, `e2e/smoke.spec.ts` +**Depends on:** none + +1. Add `@playwright/test` as a dev dependency (`bun add -d @playwright/test`) +2. Create `playwright.config.ts`: + - `testDir: './e2e'` + - Projects: Chromium only + - `baseURL: 'http://localhost:4173'` (Vite preview) + - `webServer` config to run `bun run preview` before tests + - Reporter: HTML (for CI artifact upload) +3. Create `e2e/smoke.spec.ts`: + - Test: app loads without crashing + - Test: login/auth page renders +4. Add script to `package.json`: `"test:e2e": "playwright test"` +5. Update `.gitignore` with `playwright-report/`, `test-results/` + +**Acceptance criteria:** + +- `bun run test:e2e` runs Playwright tests locally +- Smoke test verifies app renders + +--- + +### S003 -- Configure Android release signing in Gradle + +**Agent:** `android-engineer` +**Files:** `src-tauri/gen/android/app/build.gradle.kts` +**Depends on:** none + +Modify `src-tauri/gen/android/app/build.gradle.kts` to add: + +1. `signingConfigs` block that reads from environment variables: + - `ANDROID_KEYSTORE_PATH` (file path) + - `ANDROID_KEYSTORE_PASSWORD` + - `ANDROID_KEY_ALIAS` + - `ANDROID_KEY_PASSWORD` +2. Only apply release signing config when env vars are present (guard with `if` so local debug builds are unaffected) +3. Wire `signingConfig` into the `release` build type + +**Acceptance criteria:** + +- Local debug builds (`bunx tauri android build`) still work without env vars set +- When env vars are set, release builds use the configured signing config +- No secrets are hardcoded + +**Ref:** M8 + +--- + +## Milestone 1: PR Validation Foundation + +**Gate:** S001, S002, S003 all complete +**Assertions verified:** TA1 (partial -- E2E not yet in CI), TA9 + +--- + +## Wave 2: CI Workflow Completion (parallel) + +### S004 -- Add E2E test job to CI workflow + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/ci.yml` +**Depends on:** S001, S002 + +Add `e2e` job to `ci.yml`: + +1. `actions/checkout@v4` +2. `oven-sh/setup-bun@v2` + Bun cache +3. `supabase/setup-cli@v1` +4. `bun install` +5. `bunx supabase start` (requires Docker, available on `ubuntu-latest`) +6. `bun run build` +7. `bunx playwright install --with-deps chromium` +8. `bun run test:e2e` +9. On failure: upload `playwright-report/` as artifact via `actions/upload-artifact@v4` + +Set `VITE_SUPABASE_URL` and `VITE_SUPABASE_ANON_KEY` environment variables from `bunx supabase status` output for the E2E tests. + +**Acceptance criteria:** + +- E2E job runs in parallel with `validate` job +- E2E failure blocks PR merge +- Playwright report is downloadable on failure + +**Ref:** TA2 + +--- + +### S005 -- Add Android debug build job to CI workflow + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/ci.yml` +**Depends on:** S001 + +Add `android-debug` job to `ci.yml`: + +1. `actions/checkout@v4` +2. `actions/setup-java@v4` (Java 21, temurin, `cache: gradle`) +3. `android-actions/setup-android@v3` + `sdkmanager "ndk;"` +4. `dtolnay/rust-toolchain@stable` + `rustup target add aarch64-linux-android` +5. `Swatinem/rust-cache@v2` with `workspaces: "src-tauri -> target"`, `shared-key: android` +6. `oven-sh/setup-bun@v2` + Bun cache +7. `bun install` +8. Set `ANDROID_NDK_HOME` environment variable +9. `bunx tauri android build --target aarch64 --apk` (debug, single arch for speed) +10. Optionally upload debug APK as artifact + +**Acceptance criteria:** + +- Android debug build runs in parallel with `validate` and `e2e` +- Build failure blocks PR merge +- Caching is effective (Cargo, Gradle, Bun all cached) + +**Ref:** TA3 + +--- + +## Milestone 2: Complete PR Validation + +**Gate:** S004, S005 complete +**Assertions verified:** TA1, TA2, TA3, TA8 (caching) + +--- + +## Wave 3: Release Workflow (sequential within, parallel setup) + +### S006 -- Create release workflow with signed AAB build + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/release.yml` +**Depends on:** S003, S005 + +Create `release.yml` with: + +- Trigger: `push` tags matching `v*` +- Job `build-release`: + 1. Same toolchain setup as S005 (Java, Android SDK/NDK, Rust, Bun) + 2. Decode keystore: `echo "${{ secrets.KEYSTORE_BASE64 }}" | base64 -d > $RUNNER_TEMP/upload-key.jks` + 3. Inject version into `src-tauri/gen/android/app/tauri.properties`: + - `versionName` from tag: `${GITHUB_REF_NAME#v}` + - `versionCode` from `${{ github.run_number }}` + 4. Set signing env vars: `ANDROID_KEYSTORE_PATH`, `ANDROID_KEYSTORE_PASSWORD`, `ANDROID_KEY_ALIAS`, `ANDROID_KEY_PASSWORD` + 5. `bunx tauri android build --target aarch64 --aab` + 6. Upload AAB as workflow artifact via `actions/upload-artifact@v4` + +**Acceptance criteria:** + +- Pushing a `v*` tag triggers the workflow +- Signed AAB is produced and uploaded as artifact +- Version info in AAB matches the git tag + +**Ref:** TA4 + +--- + +### S007 -- Add Supabase migration job to release workflow + +**Agent:** `backend-engineer` +**Files:** `.github/workflows/release.yml` +**Depends on:** S006 + +Add `migrate` job to `release.yml`: + +1. `actions/checkout@v4` +2. `oven-sh/setup-bun@v2` +3. `supabase/setup-cli@v1` +4. `bunx supabase link --project-ref ${{ secrets.SUPABASE_PROJECT_REF }}` +5. `bunx supabase db push` + - Uses `SUPABASE_ACCESS_TOKEN` secret for authentication + +Job runs in **parallel** with `build-release`. Both must succeed before `publish` job starts. + +**Acceptance criteria:** + +- Migration job runs on tagged releases +- Migration failure blocks the publish job +- Supabase credentials are injected via secrets, never committed + +**Ref:** TA7 + +--- + +### S008 -- Add publish job (GitHub Release + Play Store) + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/release.yml` +**Depends on:** S006, S007 + +Add `publish` job to `release.yml`, gated on both `build-release` and `migrate`: + +1. Download AAB artifact via `actions/download-artifact@v4` +2. Create GitHub Release via `softprops/action-gh-release@v2`: + - `tag_name: ${{ github.ref_name }}` + - `prerelease: ${{ contains(github.ref_name, '-') }}` + - Attach AAB file + - Generate release notes from commits +3. Upload to Play Store via `r0adkll/upload-google-play@v3`: + - `serviceAccountJsonPlainText: ${{ secrets.GOOGLE_PLAY_SERVICE_ACCOUNT_JSON }}` + - `packageName: com.rghsoftware.ardentforge` + - `releaseFiles: ` + - `track: internal` + - `status: completed` + +**Acceptance criteria:** + +- Publish only runs after both build and migration succeed +- GitHub Release created with AAB attached +- Pre-release tags get `prerelease: true` flag +- AAB uploaded to Play Console internal testing track + +**Ref:** TA5, TA6 + +--- + +## Milestone 3: Release Pipeline Complete + +**Gate:** S006, S007, S008 complete +**Assertions verified:** TA4, TA5, TA6, TA7 + +--- + +## Wave 4: Polish and Documentation (parallel) + +### S009 -- Add concurrency controls and branch protection docs + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/ci.yml`, `.github/workflows/release.yml` +**Depends on:** S008 + +1. Verify concurrency groups are set on both workflows: + - `ci.yml`: `ci-${{ github.ref }}` with `cancel-in-progress: true` + - `release.yml`: `release-${{ github.ref }}` (no cancel -- don't cancel a release mid-flight) +2. Create `.github/BRANCH_PROTECTION.md` documenting recommended branch protection settings: + - `develop`: require `validate`, `e2e`, `android-debug` status checks + - `main`: require `validate`, `e2e`, `android-debug` status checks +3. Add workflow status badges to the top of `README.md` (if README exists) + +**Acceptance criteria:** + +- Superseded CI runs are cancelled +- Release runs are never cancelled +- Branch protection requirements are documented + +**Ref:** S3, S4 + +--- + +### S009-T -- Validate all workflows + +**Agent:** `quality-engineer` +**Files:** all `.github/workflows/*.yml` +**Depends on:** S008 + +Validate: + +1. YAML syntax is valid for all workflow files +2. All secret references match the documented secrets in Tech.md +3. All action versions are pinned (no `@master` or `@main`) +4. Caching keys are correct and consistent +5. Job dependency graph is correct (`needs:` declarations) +6. Concurrency groups are properly configured +7. No secrets are exposed in logs (no `echo ${{ secrets.* }}`) +8. Tag filtering logic correctly differentiates pre-release from stable +9. `tauri.properties` version injection uses correct sed/echo syntax +10. E2E job properly starts and tears down local Supabase + +**Acceptance criteria:** + +- All workflows pass validation +- No security issues in secret handling +- Dependency graph matches Tech.md architecture + +**Ref:** TA9 + +--- + +### S009-D -- Document secrets setup and first-time prerequisites + +**Agent:** `cicd-engineer` +**Files:** `.github/README.md` +**Depends on:** S008 + +Create `.github/README.md` covering: + +1. List of all required GitHub Secrets with descriptions +2. How to generate and encode the upload keystore (`base64 -w 0 upload-key.jks`) +3. How to create the Google Play Console service account +4. How to get the Supabase access token and project ref +5. First-time Play Store upload requirement (manual initial upload) +6. Branch protection setup instructions +7. How to trigger a release (tag and push workflow) + +**Acceptance criteria:** + +- A new developer can set up the pipeline from this document alone +- All prerequisites are listed +- No secrets or credentials are included in the document + +--- + +### S010 -- Add secret validation steps to release workflow jobs + +**Agent:** `cicd-engineer` +**Files:** `.github/workflows/release.yml` +**Depends on:** S008 + +Add an upfront "Validate secrets" step at the beginning of each release workflow job that uses secrets (`build-release`, `migrate`, `publish`). Each step should validate that all required secrets for that job are non-empty, failing fast with a clear error message instead of letting builds run 10-15 minutes before hitting opaque failures. + +Required validations per job: + +- `build-release`: `KEYSTORE_BASE64`, `KEYSTORE_PASSWORD`, `KEY_ALIAS`, `KEY_PASSWORD` +- `migrate`: `SUPABASE_ACCESS_TOKEN`, `SUPABASE_PROJECT_REF` +- `publish`: `GOOGLE_PLAY_SERVICE_ACCOUNT_JSON` + +**Acceptance criteria:** + +- Each job fails within 10 seconds if a required secret is missing +- Error messages clearly identify which secret is missing +- All 7 secrets across the release workflow are validated + +**Ref:** TA9, PR Review P8-015 + +--- + +## Milestone 4: Feature Complete + +**Gate:** S009, S009-T, S009-D all complete +**Assertions verified:** All (TA1-TA9) + +--- + +## Summary + +| Wave | Steps | Parallel | Key Deliverable | +| ---- | -------------------- | ----------------------------------- | --------------------------------------------------- | +| 1 | S001, S002, S003 | Yes (3 agents) | CI skeleton, Playwright scaffold, Gradle signing | +| 2 | S004, S005 | Yes (2 agents) | Complete PR validation (lint + E2E + Android debug) | +| 3 | S006, S007, S008 | S006+S007 parallel, S008 sequential | Full release pipeline with Play Store + migrations | +| 4 | S009, S009-T, S009-D | Yes (3 agents) | Polish, validation, documentation | + +**Total steps:** 10 (7 implementation + 1 test + 1 documentation + 1 polish) +**Estimated agents in parallel (max):** 3 +**Critical path:** S001 -> S005 -> S006 -> S008 -> S009-T diff --git a/Context/Features/011-ci-cd-pipeline/Tech.md b/Context/Features/011-ci-cd-pipeline/Tech.md new file mode 100644 index 00000000..ef9483d3 --- /dev/null +++ b/Context/Features/011-ci-cd-pipeline/Tech.md @@ -0,0 +1,276 @@ +# Feature 011: CI/CD Pipeline -- Technical Plan + +**Status:** Draft +**Date:** 2026-04-04 +**Spec:** [Spec.md](./Spec.md) + +--- + +## Architecture Overview + +Two GitHub Actions workflows, triggered by different events, sharing a common toolchain setup: + +``` +PR to develop v*-alpha/beta/rc tag (develop) v* stable tag (main) + | | | + v v v +[ci.yml] [release.yml] [release.yml] + - lint - signed AAB build - signed AAB build + - typecheck - GitHub Release (prerelease) - GitHub Release + - unit tests - Play Store (internal track) - Play Store (internal track) + - E2E tests - Supabase migrations (prod) - Supabase migrations (prod) + - debug APK build +``` + +### Workflow Files + +| File | Trigger | Purpose | +| ------------------------------- | ----------------------------- | ------------------------------------------------------------------------ | +| `.github/workflows/ci.yml` | PR to `develop`, PR to `main` | Lint, typecheck, unit tests, E2E tests, debug APK build | +| `.github/workflows/release.yml` | Push tag `v*` | Signed AAB build, GitHub Release, Play Store upload, Supabase migrations | + +Two workflows total. `release.yml` handles both pre-release and stable tags, differentiating via tag pattern matching (`contains(github.ref, '-')` for pre-release flag). + +--- + +## Toolchain and Actions + +### Runner + +**`ubuntu-latest`** for all workflows. Cheaper than macOS runners, Android SDK/NDK install is straightforward, and Tauri's Rust cross-compilation does not require macOS. + +### Tool Installation Order + +Each workflow installs tools in this dependency order: + +| Step | Tool | Action / Method | Version | +| ---- | -------------------- | -------------------------------------------------- | ------------------------- | +| 1 | Java 21 | `actions/setup-java@v4` (temurin, `cache: gradle`) | `21` | +| 2 | Android SDK + NDK | `android-actions/setup-android@v3` + `sdkmanager` | SDK 36, NDK pinned | +| 3 | Rust stable | `dtolnay/rust-toolchain@stable` | stable (min 1.77.2) | +| 4 | Rust Android targets | `dtolnay/rust-toolchain` `targets` input | `aarch64-linux-android` | +| 5 | Bun | `oven-sh/setup-bun@v2` | pinned (currently latest) | +| 6 | Supabase CLI | `supabase/setup-cli@v1` | latest (E2E + release) | + +### Caching Strategy + +| Cache | Method | Key | +| ------ | ------------------------------------ | ---------------------------------------------------------- | +| Gradle | `actions/setup-java` `cache: gradle` | Auto-keyed on `*.gradle*` + wrapper properties | +| Cargo | `Swatinem/rust-cache@v2` | `workspaces: "src-tauri -> target"`, `shared-key: android` | +| Bun | `actions/cache@v4` | `~/.bun/install/cache` keyed on `bun.lock` | + +### Key Actions + +| Purpose | Action | Version | +| ----------------- | ---------------------------------- | ------------------------- | +| Checkout | `actions/checkout@v4` | v4 | +| Java | `actions/setup-java@v4` | v4 | +| Android SDK | `android-actions/setup-android@v3` | v3 | +| Rust | `dtolnay/rust-toolchain@stable` | stable | +| Rust cache | `Swatinem/rust-cache@v2` | v2 | +| Bun | `oven-sh/setup-bun@v2` | v2 | +| Generic cache | `actions/cache@v4` | v4 | +| Supabase CLI | `supabase/setup-cli@v1` | v1 (CLI pinned to 2.84.4) | +| Play Store upload | `r0adkll/upload-google-play@v3` | v3 | +| GitHub Release | `softprops/action-gh-release@v2` | v2 | + +--- + +## Workflow Details + +### 1. `ci.yml` -- PR Validation + +**Trigger:** `pull_request` targeting `develop` or `main` + +**Concurrency:** Cancel in-progress runs for the same PR (`concurrency: { group: ci-${{ github.ref }}, cancel-in-progress: true }`) + +**Jobs:** + +#### Job: `validate` + +Runs on `ubuntu-latest`. Steps: + +1. Checkout +2. Setup Bun + cache +3. `bun install` +4. `bun run lint` (ESLint) +5. `bun run build` (includes `tsc -b` typecheck + Vite build) +6. `bun run test` (Vitest) + +#### Job: `e2e` + +Runs on `ubuntu-latest`. Needs Docker for local Supabase. Steps: + +1. Checkout +2. Setup Bun + cache +3. Setup Supabase CLI +4. `bun install` +5. `bunx supabase start` (spins up local Supabase via Docker) +6. `bun run build` (build frontend for E2E) +7. `bunx playwright install --with-deps chromium` +8. `bunx playwright test` +9. Upload Playwright report as artifact on failure + +#### Job: `android-debug` + +Runs on `ubuntu-latest`. Steps: + +1. Checkout +2. Setup Java 21 (with Gradle cache) +3. Setup Android SDK + NDK +4. Setup Rust + Android targets + Rust cache +5. Setup Bun + cache +6. `bun install` +7. `bunx tauri android build --target aarch64 --apk` (debug build, single arch for speed) +8. Upload debug APK as artifact (optional, for manual testing) + +All three jobs run **in parallel**. All must pass for PR merge. + +### 2. `release.yml` -- Tagged Release + +**Trigger:** `push` with tag pattern `v*` + +**Branch filtering:** The tag determines pre-release vs stable: + +- Tags containing `-alpha`, `-beta`, `-rc` are marked as pre-release on the GitHub Release +- Tags without a suffix are stable releases + +**Jobs:** + +#### Job: `build-release` + +Steps: + +1. Checkout +2. Setup Java 21 (with Gradle cache) +3. Setup Android SDK + NDK +4. Setup Rust + Android targets + Rust cache +5. Setup Bun + cache +6. `bun install` +7. Decode keystore from `KEYSTORE_BASE64` secret to `$RUNNER_TEMP/upload-key.jks` +8. Inject version into `tauri.properties`: + - `versionName` from git tag (strip `v` prefix) + - `versionCode` from `github.run_number` (monotonically increasing) +9. `bunx tauri android build --target aarch64 --aab` (signed release AAB) +10. Upload AAB as workflow artifact + +#### Job: `migrate` + +Runs **before** `build-release` completes (parallel start, but release publishing waits for both). Steps: + +1. Checkout +2. Setup Supabase CLI +3. `bunx supabase link --project-ref ${{ secrets.SUPABASE_PROJECT_REF }}` +4. `bunx supabase db push` (apply pending migrations to production) + +**Failure behavior:** If migrations fail, the entire release workflow fails. The AAB is not published. This protects against deploying an app that expects schema changes that did not land. + +#### Job: `publish` + +Needs: `build-release`, `migrate` (both must succeed). Steps: + +1. Download AAB artifact +2. Create GitHub Release via `softprops/action-gh-release@v2`: + - Tag name from `github.ref` + - Pre-release flag: `${{ contains(github.ref_name, '-') }}` + - Attach AAB file +3. Upload to Play Store via `r0adkll/upload-google-play@v3`: + - `track: internal` + - `status: completed` + - `packageName: com.rghsoftware.ardentforge` + +--- + +## Signing Architecture + +**Approach:** Google Play App Signing (Google holds the app signing key; we use an upload key in CI). + +**Keystore handling in CI:** + +1. Upload keystore is base64-encoded and stored as `KEYSTORE_BASE64` GitHub Secret +2. Decoded at build time to `$RUNNER_TEMP/upload-key.jks` (ephemeral, cleaned up with the runner) +3. Credentials passed via environment variables to Gradle + +**Required modification to `src-tauri/gen/android/app/build.gradle.kts`:** +Add a `signingConfigs` block that reads from environment variables: + +```kotlin +signingConfigs { + create("release") { + val keystorePath = System.getenv("ANDROID_KEYSTORE_PATH") + if (keystorePath != null) { + storeFile = file(keystorePath) + storePassword = System.getenv("ANDROID_KEYSTORE_PASSWORD") + keyAlias = System.getenv("ANDROID_KEY_ALIAS") + keyPassword = System.getenv("ANDROID_KEY_PASSWORD") + } + } +} +``` + +Then reference it in the `release` build type's `signingConfig`. + +--- + +## Secrets Required + +| Secret | Content | Used In | +| ---------------------------------- | ---------------------------------------------------------- | ------------- | +| `KEYSTORE_BASE64` | Base64-encoded upload keystore (.jks) | `release.yml` | +| `KEYSTORE_PASSWORD` | Keystore password | `release.yml` | +| `KEY_ALIAS` | Key alias within keystore | `release.yml` | +| `KEY_PASSWORD` | Key password | `release.yml` | +| `GOOGLE_PLAY_SERVICE_ACCOUNT_JSON` | Play Console service account JSON key | `release.yml` | +| `SUPABASE_PROJECT_REF` | Supabase remote project reference (`azaioeglqubjqbdssnmi`) | `release.yml` | +| `SUPABASE_ACCESS_TOKEN` | Supabase personal access token for CLI auth | `release.yml` | + +--- + +## E2E Testing Setup + +Playwright is **not yet configured** in the project. This feature will add: + +- `@playwright/test` as a dev dependency +- `playwright.config.ts` configured for Chromium, targeting `http://localhost:4173` (Vite preview server) +- `e2e/` directory for test files +- Initial smoke test: app loads, auth page renders + +E2E tests run against a local Supabase instance (`bunx supabase start`) seeded with `supabase/seed.sql`. The Vite preview server is started via `bun run preview` (port 4173) pointing at the local Supabase URL. + +--- + +## Version Injection Strategy + +The `src-tauri/gen/android/app/tauri.properties` file controls Android versioning: + +```properties +tauri.android.versionName=0.8.0 +tauri.android.versionCode=8000 +``` + +**In CI:** + +- `versionName` is derived from the git tag: `echo "${GITHUB_REF_NAME#v}"` (e.g., `v1.2.0` becomes `1.2.0`) +- `versionCode` uses `${{ github.run_number }}` -- a monotonically increasing integer provided by GitHub Actions, guaranteeing each upload to Play Store has a higher version code + +This is written to `tauri.properties` before the build step. + +--- + +## Risks and Mitigations + +| Risk | Impact | Mitigation | +| ----------------------------------------------------- | ----------------------------------- | ------------------------------------------------------------------------------ | +| Cold Android build takes 15-20 min | Slow PR feedback | Aggressive caching (Cargo, Gradle, Bun); debug build targets single arch only | +| NDK version drift on runner images | Build breakage | Pin NDK version explicitly via `sdkmanager` | +| Supabase migration fails on release | Blocked release | Migration runs as a gate; release only publishes if migrations succeed | +| Keystore secret rotation | Requires coordinated update | Document rotation procedure; use Play App Signing so upload key is replaceable | +| First Play Store upload must be manual | One-time setup step | Document as a prerequisite in Steps.md | +| `gen/android/` Gradle files modified by Tauri updates | CI build breaks after Tauri upgrade | Pin `@tauri-apps/cli` version; test upgrades in a dedicated PR | + +--- + +## ADRs Created + +- [ADR-007: GitHub Actions for CI/CD over alternatives](../../Decisions/ADR-007-github-actions-cicd.md) diff --git a/Context/Features/011-deep-link-handler/Spec.md b/Context/Features/011-deep-link-handler/Spec.md new file mode 100644 index 00000000..9e0aad54 --- /dev/null +++ b/Context/Features/011-deep-link-handler/Spec.md @@ -0,0 +1,66 @@ +# Feature 011: Deep Link Handler for `ardentforge://connect` + +## Overview + +Enable the `ardentforge://connect?url=...&key=...` deep link to function as a first-class onboarding path. When a user taps an invite link or scans a QR code that produces one, the app opens directly to the appropriate screen with credentials pre-populated and auto-validated. This completes the onboarding link chain: QR generation (010) produces links, this feature consumes them at the OS level. + +## Problem Statement + +The `ardentforge://connect` URL scheme is already generated by the QR code and invite link features (Refactor B / Feature 010), but tapping one of these links on a device with the app installed does nothing useful. The deep link plugin only registers the `auth` host, and the `onOpenUrl` listener only processes OAuth callbacks. Users must manually copy-paste invite links into the setup screen rather than having the app handle them natively. + +## User Stories + +1. **As an unconfigured user**, I tap an `ardentforge://connect` link shared by a friend, and the app opens to the setup screen with the server credentials already filled in and validating -- I just wait for the green check and sign in. + +2. **As a configured user**, I tap an invite link for the same server I'm already on, and see a brief "Already connected to this server" confirmation -- no disruption to my session. + +3. **As a configured user**, I tap an invite link for a different server, and the app navigates me to the backend settings where I can review and confirm the switch (with the standard data-wipe warning on Tauri). + +4. **As a browser user**, I visit `/connect?url=...&key=...` and get the same behavior as the native deep link -- redirect to setup or settings with pre-populated values. + +5. **As a user who receives a malformed link**, I see an "Invalid invite link" toast rather than a crash or silent failure. + +## Requirements + +### Must Have + +- **M1:** Register `ardentforge://connect` as a handled deep link host in Tauri config and Android manifest +- **M2:** Extend the `onOpenUrl` listener to dispatch based on URL host/path (`auth` vs `connect`) +- **M3:** Implement onboarding handler that parses `url` and `key` query params from `ardentforge://connect` URLs +- **M4:** Unconfigured app: navigate to `/setup` with pre-populated Supabase URL and key, auto-trigger validation +- **M5:** Configured app, same instance: show "Already connected to this server" toast, no navigation +- **M6:** Configured app, different instance: navigate to backend settings with pre-populated change form +- **M7:** Malformed deep link (missing params): show "Invalid invite link" toast +- **M8:** Add TanStack Router `/connect` route for browser-mode equivalent +- **M9:** `/connect` route applies same logic: unconfigured -> setup, configured same -> toast, configured different -> settings +- **M10:** Setup screen accepts `url` and `key` via route search params for pre-population and auto-validation +- **M11:** Existing OAuth deep link flow (`ardentforge://auth/callback`) continues to work without regression + +### Won't Have (this iteration) + +- **W1:** Universal links / App Links (HTTPS-based deep links requiring domain verification) -- custom scheme only +- **W2:** Deep link for other actions beyond `connect` (e.g., `ardentforge://workout/123`) +- **W3:** Prompt to install the app if it's not installed (deferred to app store listing) +- **W4:** Desktop app support -- this is a mobile + browser app only + +## Testable Assertions + +- **TA1:** `onOpenUrl` callback receiving `ardentforge://connect?url=X&key=Y` on an unconfigured app results in navigation to `/setup` with `url=X` and `key=Y` in search params +- **TA2:** `onOpenUrl` callback receiving `ardentforge://connect?url=X&key=Y` where X matches current config shows toast and does not navigate +- **TA3:** `onOpenUrl` callback receiving `ardentforge://connect?url=X&key=Y` where X differs from current config navigates to backend settings +- **TA4:** `onOpenUrl` callback receiving `ardentforge://connect` (missing params) shows "Invalid invite link" toast +- **TA5:** `onOpenUrl` callback receiving `ardentforge://auth/callback?code=Z` still triggers OAuth exchange (regression check) +- **TA6:** Browser navigation to `/connect?url=X&key=Y` on unconfigured app redirects to `/setup?url=X&key=Y` +- **TA7:** Browser navigation to `/connect?url=X&key=Y` on configured app (same) shows toast +- **TA8:** Browser navigation to `/connect?url=X&key=Y` on configured app (different) redirects to profile/settings +- **TA9:** Setup screen mounted with `url` and `key` search params expands advanced section, pre-populates fields, and auto-triggers validation +- **TA10:** `tauri.conf.json` deep-link plugin config includes `connect` host entry + +## Open Questions + +1. **Backend settings pre-population**: The backend settings component (`BackendSettings`) currently uses local state for the change form. Pre-populating from a deep link requires either route search params on `/profile` or a transient Zustand store. _Recommendation: use a lightweight Zustand transient store to avoid coupling settings layout to route params._ + +## Dependencies + +- **Feature 010 (QR Code Gen + Scan):** Produces the `ardentforge://connect` URLs that this feature handles. The `buildInviteLink` and `parseInviteLink` utilities from `src/lib/invite-link.ts` are reused directly. +- **Existing deep-link plugin:** Already initialized in `lib.rs` and configured in `tauri.conf.json`. This feature extends the existing config rather than adding a new plugin. diff --git a/Context/Features/011-deep-link-handler/Steps.md b/Context/Features/011-deep-link-handler/Steps.md new file mode 100644 index 00000000..af024e8a --- /dev/null +++ b/Context/Features/011-deep-link-handler/Steps.md @@ -0,0 +1,298 @@ +# Feature 011: Deep Link Handler -- Implementation Steps + +## Team Composition + +| Role | Agent | Stacks | +| ------------------- | ---------- | ------------------------------------------- | +| Frontend Specialist | `frontend` | React, TypeScript, TanStack Router, Zustand | + +Single-agent feature -- all changes are frontend/config, tightly coupled, and must land together. + +## Implementation Waves + +### Wave 1: Foundation (new modules + config) + +These tasks have no dependencies on each other and can execute in parallel. + +--- + +#### S001: Create pending connect transient store + +**Agent:** `frontend` +**Files:** `src/lib/pending-connect.ts` (new) +**Depends on:** nothing +**Parallel:** yes (with S002, S003, S004) + +Create `src/lib/pending-connect.ts` with a minimal Zustand store: + +```typescript +interface PendingConnectState { + pending: { url: string; key: string } | null + setPending: (url: string, key: string) => void + clear: () => void +} +``` + +Export `usePendingConnect` (the Zustand hook). The store must be accessible outside React via `usePendingConnect.getState()`. + +**Done when:** Store exports `usePendingConnect` with `pending`, `setPending`, `clear`. + +--- + +#### S002: Create deep link handler module + +**Agent:** `frontend` +**Files:** `src/lib/deep-link-handler.ts` (new) +**Depends on:** S001 (import pending connect store) +**Parallel:** no -- needs S001's export + +Create `src/lib/deep-link-handler.ts` with a single exported async function: + +```typescript +export async function handleConnectLink(urlStr: string): Promise +``` + +Logic: + +1. Call `parseInviteLink(urlStr)` from `src/lib/invite-link.ts`. If null, `toast('Invalid invite link')` and return. +2. Call `await getConfigStore().hasConfig()`. +3. If no config: `window.location.href = '/setup?url=${encodeURIComponent(parsed.url)}&key=${encodeURIComponent(parsed.key)}'` and return. +4. If config exists: `await getConfigStore().getConfig()` and compare `config.supabaseUrl` with `parsed.url`. +5. Same instance: `toast('Already connected to this server')` and return. +6. Different instance: call `usePendingConnect.getState().setPending(parsed.url, parsed.key)`, then `window.location.href = '/profile'`. + +Imports: `parseInviteLink` from `@/lib/invite-link`, `getConfigStore` from `@/lib/config-store`, `usePendingConnect` from `@/lib/pending-connect`, `toast` from `sonner`. + +**Done when:** `handleConnectLink` correctly branches for all four cases (invalid, unconfigured, same, different). + +--- + +#### S003: Register `connect` host in Tauri config + +**Agent:** `frontend` +**Files:** `src-tauri/tauri.conf.json` +**Depends on:** nothing +**Parallel:** yes (with S001, S004) + +Add a second entry to `plugins.deep-link.mobile`: + +```json +"deep-link": { + "mobile": [ + { "schemes": ["ardentforge"], "host": "auth" }, + { "schemes": ["ardentforge"], "host": "connect" } + ] +} +``` + +**Done when:** `tauri.conf.json` has both `auth` and `connect` hosts registered. + +--- + +#### S004: Exempt `/connect` from root route guard + +**Agent:** `frontend` +**Files:** `src/routes/__root.tsx` +**Depends on:** nothing +**Parallel:** yes (with S001, S002, S003) + +Add `location.pathname === '/connect'` to the existing bypass condition in `beforeLoad`: + +```typescript +if ( + location.pathname === '/setup' || + location.pathname === '/connect' || + location.pathname.startsWith('/s/') || + location.pathname === '/display' +) + return +``` + +**Done when:** Navigating to `/connect` does not redirect to `/setup` regardless of config state. + +--- + +### Wave 2: Integration (wire up existing components) + +These tasks depend on Wave 1 outputs and modify existing files. + +--- + +#### S005: Add dispatch branch to `onOpenUrl` listener + +**Agent:** `frontend` +**Files:** `src/lib/auth.tsx` +**Depends on:** S002 +**Parallel:** yes (with S006, S007, S008) + +In the `onOpenUrl` callback (around line 173), add a hostname check before the existing OAuth logic: + +```typescript +const url = new URL(urlStr) +if (url.hostname === 'connect') { + await handleConnectLink(urlStr) + return +} +``` + +Add import: `import { handleConnectLink } from '@/lib/deep-link-handler'` + +The existing OAuth handling (code exchange, error redirect) remains completely untouched -- the new branch returns early before reaching it. + +**Done when:** Deep links with `connect` hostname dispatch to `handleConnectLink`; `auth` hostname links still trigger OAuth exchange. + +--- + +#### S006: Create `/connect` browser route + +**Agent:** `frontend` +**Files:** `src/routes/connect.tsx` (new) +**Depends on:** S002, S004 +**Parallel:** yes (with S005, S007, S008) + +Create a TanStack Router file route at `src/routes/connect.tsx`: + +1. `validateSearch`: extract `url` and `key` as optional strings. +2. Component: a minimal component that calls `handleConnectLink` in a `useEffect` on mount. Constructs the full `ardentforge://connect?url=...&key=...` string from search params and passes it to `handleConnectLink`. If either param is missing, show toast and redirect to `/setup`. +3. Renders nothing visible (or a brief loading indicator) since the handler navigates away immediately. + +**Done when:** Browser navigation to `/connect?url=X&key=Y` triggers the same three-way branching as the Tauri deep link. + +--- + +#### S007: Setup screen accepts pre-populated search params + +**Agent:** `frontend` +**Files:** `src/routes/setup.tsx` +**Depends on:** S002 +**Parallel:** yes (with S005, S006, S008) + +Two changes to the setup route: + +1. **Add `validateSearch`** to the route definition: + + ```typescript + validateSearch: (search): { url?: string; key?: string } => ({ + url: (search.url as string) || undefined, + key: (search.key as string) || undefined, + }), + ``` + +2. **Add a `useEffect`** in `SetupPage` that checks for pre-populated values from search params (via `Route.useSearch()`). If both `url` and `key` are present: + - Set `url` and `key` state + - Set `advancedOpen` to `true` + - Call `validateAndSave(search.url, search.key)` to auto-trigger validation + +Guard with a ref to prevent double-execution (same pattern as the existing `autoValidated` ref). + +**Done when:** Navigating to `/setup?url=X&key=Y` expands the advanced section, pre-populates fields, and auto-validates. + +--- + +#### S008: BackendSettings consumes pending connect store + +**Agent:** `frontend` +**Files:** `src/components/profile/backend-settings.tsx` +**Depends on:** S001 +**Parallel:** yes (with S005, S006, S007) + +Add a `useEffect` in `BackendSettings` that checks the pending connect store on mount: + +```typescript +useEffect(() => { + const { pending, clear } = usePendingConnect.getState() + if (pending) { + setEditing(true) + setUrl(pending.url) + setKey(pending.key) + clear() + } +}, []) +``` + +When pending values exist, the component opens the editing form pre-populated with the new backend's URL and key. The user reviews and clicks "Connect" to trigger the existing `handleSubmit` flow (validation, Tauri confirmation dialog, backend change). + +**Done when:** After a deep link for a different instance, BackendSettings opens with pre-populated fields ready for user confirmation. + +--- + +### Wave 3: Verification + +--- + +#### S009: Route tree regeneration + build verification + +**Agent:** `frontend` +**Files:** `src/routeTree.gen.ts` (auto-generated) +**Depends on:** S006 +**Parallel:** no + +Run `bun run build` to: + +1. Trigger TanStack Router codegen (regenerates `routeTree.gen.ts` with the new `/connect` route) +2. Verify TypeScript compilation passes +3. Verify no import errors or type mismatches + +**Done when:** `bun run build` succeeds with zero errors. + +--- + +#### S010: Unit tests for deep link handler + +**Agent:** `frontend` +**Files:** `src/lib/__tests__/deep-link-handler.test.ts` (new) +**Depends on:** S002 +**Parallel:** yes (with S009) + +Write tests for `handleConnectLink()`: + +| Test case | Setup | Assertion | +| ---------------------------------------- | ------------------------------------ | ------------------------------------------------------------- | +| Invalid link (not ardentforge://connect) | n/a | `toast('Invalid invite link')` called | +| Missing params | `ardentforge://connect` (no url/key) | `toast('Invalid invite link')` called | +| Unconfigured app | Mock `hasConfig()` -> false | `window.location.href` set to `/setup?url=...&key=...` | +| Configured, same instance | Mock `getConfig()` -> matching URL | `toast('Already connected to this server')` called | +| Configured, different instance | Mock `getConfig()` -> different URL | `setPending` called, `window.location.href` set to `/profile` | + +Mock `getConfigStore` (return mock object), `toast` (from sonner), and `window.location.href` (use `Object.defineProperty` or jsdom assignment). + +**Done when:** All 5 test cases pass via `bun run test`. + +--- + +## Milestone Summary + +| Wave | Tasks | Parallel? | Validates | +| ---- | --------- | ---------------------------------------- | ----------------------------------------------- | +| 1 | S001-S004 | S001+S003+S004 parallel, S002 after S001 | New modules exist, config updated | +| 2 | S005-S008 | All parallel | Deep links dispatch correctly, UI pre-populates | +| 3 | S009-S010 | S009+S010 parallel | Build passes, tests pass | + +#### S011: Tests for /connect route and auth.tsx connect dispatch + +**Agent:** `frontend` +**Files:** `src/routes/__tests__/connect.test.tsx` (new), `src/lib/__tests__/auth-oauth.test.tsx` +**Depends on:** S006, S005 +**Parallel:** no + +Add tests for: + +1. `/connect` route component: missing-params guard (toasts + redirects to /setup), error catch when `handleConnectLink` rejects (toasts + redirects to /setup), URL reconstruction from search params. +2. `auth.tsx` connect dispatch: verify `url.hostname === 'connect'` routes to `handleConnectLink` instead of OAuth code exchange. Verify non-connect hostnames still go through OAuth flow. + +**Done when:** Both the `/connect` route edge cases and the `onOpenUrl` connect dispatch branch have test coverage. + +--- + +## Milestone Summary + +| Wave | Tasks | Parallel? | Validates | +| ---- | --------- | ---------------------------------------- | ----------------------------------------------- | +| 1 | S001-S004 | S001+S003+S004 parallel, S002 after S001 | New modules exist, config updated | +| 2 | S005-S008 | All parallel | Deep links dispatch correctly, UI pre-populates | +| 3 | S009-S010 | S009+S010 parallel | Build passes, tests pass | +| 4 | S011 | Independent | Route + dispatch edge cases covered | + +**Total tasks:** 11 +**Critical path:** S001 -> S002 -> S005 (Tauri dispatch wired up) +**Estimated effort:** 0.5 days diff --git a/Context/Features/011-deep-link-handler/Tech.md b/Context/Features/011-deep-link-handler/Tech.md new file mode 100644 index 00000000..7f54d62e --- /dev/null +++ b/Context/Features/011-deep-link-handler/Tech.md @@ -0,0 +1,184 @@ +# Feature 011: Deep Link Handler -- Technical Plan + +## Architecture Overview + +The deep link handler adds a dispatch layer to the existing `onOpenUrl` listener and a parallel browser route, both funneling into a shared handler function. No new plugins, no Rust changes, no schema changes. + +``` + ┌──────────────────────┐ + │ OS deep link event │ + │ ardentforge://... │ + └──────────┬───────────┘ + │ + ┌──────────▼───────────┐ + │ onOpenUrl listener │ + │ (auth.tsx) │ + └──────────┬───────────┘ + │ dispatch on hostname + ┌────────────────┼────────────────┐ + │ │ + ┌─────────▼──────────┐ ┌──────────▼──────────┐ + │ hostname: "auth" │ │ hostname: "connect" │ + │ (existing OAuth) │ │ handleConnectLink() │ + └────────────────────┘ └──────────┬──────────┘ + │ + ┌─────────────────────┼─────────────────────┐ + │ │ │ + ┌─────────▼────────┐ ┌─────────▼────────┐ ┌─────────▼────────┐ + │ No config │ │ Same instance │ │ Diff instance │ + │ → /setup?url&key│ │ → toast │ │ → /profile │ + └──────────────────┘ └──────────────────┘ │ + pending store │ + └──────────────────┘ + + ┌──────────────────────┐ + │ Browser: /connect │──── same handleConnectLink() ────► same 3 branches + └──────────────────────┘ +``` + +## Key Decisions + +### D1: Shared handler function vs inline logic + +**Decision:** Extract `handleConnectLink(urlStr: string)` into `src/lib/deep-link-handler.ts`. + +Both the Tauri `onOpenUrl` callback and the browser `/connect` route need the same three-way branching logic (unconfigured / same instance / different instance). A shared function avoids duplication and is independently testable. + +The function uses `parseInviteLink()` from `src/lib/invite-link.ts` (already exists) and `getConfigStore()` for config checks. It returns nothing -- it performs navigation and toasts as side effects, matching the existing pattern in the auth deep link handler. + +**Navigation:** Uses `window.location.href` for redirects (same pattern as existing `auth.tsx` deep link handler at line 179). This works because the handler runs outside React's render cycle in Tauri mode, and in the browser `/connect` route it triggers a full page transition which is acceptable for a one-time redirect. + +### D2: Transient Zustand store for backend settings pre-population + +**Decision:** Create `src/lib/pending-connect.ts` with a minimal Zustand store. + +**Why not route search params on `/profile`:** The profile page is a general-purpose authenticated route. Adding `connectUrl` and `connectKey` search params couples it to deep link concerns. The search params would also be visible in the URL bar and browser history, leaking backend credentials into navigation state. + +**Why Zustand transient store:** Zustand stores are accessible both inside React (`usePendingConnect()` hook) and outside React (`usePendingConnect.getState().setPending(url, key)`). This lets the Tauri deep link handler (non-React) set the pending values, then the BackendSettings component (React) consumes them on mount. The store is ephemeral -- values are cleared after consumption and lost on page reload, which is the correct behavior. + +```typescript +// src/lib/pending-connect.ts +import { create } from 'zustand' + +interface PendingConnectState { + pending: { url: string; key: string } | null + setPending: (url: string, key: string) => void + clear: () => void +} + +export const usePendingConnect = create((set) => ({ + pending: null, + setPending: (url, key) => set({ pending: { url, key } }), + clear: () => set({ pending: null }), +})) +``` + +### D3: Setup screen pre-population via route search params + +**Decision:** Add `url` and `key` search params to the `/setup` route definition. + +Unlike `/profile`, the setup route is purpose-built for onboarding. Search params here are semantically correct and don't leak sensitive info (the setup screen already displays these values in form fields). The route's `validateSearch` extracts and sanitizes the params. A `useEffect` in `SetupPage` detects their presence, expands the advanced section, pre-populates the fields, and auto-triggers validation. + +### D4: Tauri deep-link config -- add `connect` host + +**Decision:** Add a second entry to the `plugins.deep-link.mobile` array in `tauri.conf.json`. + +```json +"deep-link": { + "mobile": [ + { "schemes": ["ardentforge"], "host": "auth" }, + { "schemes": ["ardentforge"], "host": "connect" } + ] +} +``` + +This registers `ardentforge://connect` as a handled URL scheme on Android. The Android manifest (`gen/android/`) is auto-regenerated by Tauri on the next build. No manual manifest editing needed. + +### D5: Root route guard exemption for `/connect` + +**Decision:** Add `/connect` to the allowlist in `__root.tsx` `beforeLoad`. + +The root route guard redirects to `/setup` when no config exists. The `/connect` route must be reachable regardless of config state (it handles the "unconfigured" case itself). Adding `location.pathname === '/connect'` to the existing bypass condition is a one-line change. + +### D6: Dispatch strategy in `onOpenUrl` + +**Decision:** Add a hostname check at the top of the existing `onOpenUrl` callback in `auth.tsx`. + +```typescript +const url = new URL(urlStr) +if (url.hostname === 'connect') { + await handleConnectLink(urlStr) + return +} +// ... existing OAuth handling continues unchanged +``` + +This is the minimal change -- a single early return branch. The existing OAuth code is untouched, eliminating regression risk. + +## Stack-Specific Details + +### Frontend (React/TypeScript) + +**New files:** +| File | Purpose | +|------|---------| +| `src/lib/deep-link-handler.ts` | `handleConnectLink()` -- shared handler for connect deep links | +| `src/lib/pending-connect.ts` | Zustand transient store for pending backend change values | +| `src/routes/connect.tsx` | TanStack Router `/connect` route (browser fallback) | + +**Modified files:** +| File | Change | +|------|--------| +| `src/lib/auth.tsx` | Add dispatch branch in `onOpenUrl` callback (~3 lines) | +| `src/routes/__root.tsx` | Add `/connect` to guard allowlist (~1 line) | +| `src/routes/setup.tsx` | Add search param validation + auto-populate effect (~15 lines) | +| `src/components/profile/backend-settings.tsx` | Consume pending connect store on mount (~10 lines) | + +### Tauri (Config only) + +**Modified files:** +| File | Change | +|------|--------| +| `src-tauri/tauri.conf.json` | Add `connect` host to deep-link plugin config | + +No Rust code changes. No new plugins. Android manifest regenerated automatically. + +## Integration Points + +1. **`parseInviteLink()` (invite-link.ts):** Already handles `ardentforge://connect?url=...&key=...` parsing. Reused as-is by `handleConnectLink()`. + +2. **`getConfigStore()` (config-store.ts):** Used by `handleConnectLink()` to check current config state. All methods are async (Promise-based), which the handler already accounts for. + +3. **`validateAndSave()` (setup.tsx):** The setup screen's existing save pipeline. Pre-populated search params trigger this via the auto-validate `useEffect`, not by calling it directly from the handler. + +4. **`BackendSettings` (backend-settings.tsx):** The existing `editing` state and `url`/`key` form fields are pre-populated from the pending connect store. The existing `handleSubmit` flow (validate, confirm dialog on Tauri, apply change) runs unchanged. + +5. **`toast()` (sonner):** Standalone function, callable outside React. Used by `handleConnectLink()` for "Already connected" and "Invalid invite link" messages. + +## Risks and Mitigations + +| Risk | Impact | Mitigation | +| ------------------------------------------------------------ | ------ | ----------------------------------------------------------------------------------------------------------------------------------------------- | +| `onOpenUrl` dispatch breaks OAuth flow | High | Early return only on `hostname === 'connect'`; all other URLs fall through to existing handler unchanged | +| Android manifest not regenerated | Medium | Verify `gen/android/` after `tauri.conf.json` change; if stale, `bun tauri android init` regenerates | +| Setup `beforeLoad` guard conflicts with pre-populated params | Medium | Search params pass through redirect -- if config exists, root guard redirects to `/`, which is correct (configured users shouldn't reach setup) | +| Pending connect store consumed before BackendSettings mounts | Low | The store is set synchronously before navigation; React render picks it up on mount. `clear()` called only after consuming values | +| `window.location.href` loses React state | Low | Acceptable for one-time deep link redirect. Matches existing auth handler pattern. New page load bootstraps fresh state | + +## Testing Strategy + +**Unit tests (Vitest):** + +- `handleConnectLink()`: mock `getConfigStore()` and assert navigation + toast behavior for all three branches + invalid input +- `parseInviteLink()`: already tested; no changes needed + +**Manual testing:** + +- Android: `adb shell am start -a android.intent.action.VIEW -d "ardentforge://connect?url=...&key=..."` to simulate deep link +- Browser: navigate to `/connect?url=...&key=...` directly +- Verify OAuth deep links still work after dispatch change + +## ADR + +### ADR-007: Transient Zustand store for cross-route deep link state + +See `Context/Decisions/ADR-007-transient-store-deep-link-state.md`. diff --git a/Context/Features/012-TauriAdapter-Tests/Spec.md b/Context/Features/012-TauriAdapter-Tests/Spec.md new file mode 100644 index 00000000..d98a35d5 --- /dev/null +++ b/Context/Features/012-TauriAdapter-Tests/Spec.md @@ -0,0 +1,78 @@ +# Feature 012: TauriAdapter Unit Tests + +**Status:** Done +**Issue:** [#16](https://github.com/rghsoftware/ardent-forge/issues/16) +**Created:** 2026-04-04 + +## Overview + +Add comprehensive unit test coverage for `TauriAdapter` (`src/lib/tauri-adapter.ts`, 2308 lines, 88 class methods + 30 standalone functions). This is the entire offline-first data path -- bugs here cause silent data loss for desktop users. + +## Problem Statement + +The TauriAdapter has zero test coverage. The SupabaseAdapter has comprehensive tests (1822 lines in `src/lib/__tests__/supabase-adapter.test.ts`). Both adapters implement the same `DataAdapter` interface, so the TauriAdapter should have equivalent coverage. + +The TauriAdapter contains non-trivial logic in: + +- Type conversion helpers (SQLite integers to booleans, JSON parsing, null guards) +- Error translation (`TauriAppError` to `AdapterError`) +- Row-to-domain mappers (20+ functions) +- Complex aggregation methods that compose multiple `invoke()` calls +- Argument serialization for Rust IPC + +## User Stories + +1. As a developer, I want TauriAdapter tests so regressions in the offline data path are caught before release. +2. As a developer, I want the test structure to mirror the SupabaseAdapter tests so both adapters are easy to compare and maintain. + +## Requirements + +### Must Have + +- **M1:** Test all 4 conversion helpers: `intToBool()`, `parseJson()`, `requireString()`, `isoToUnixSeconds()` +- **M2:** Test `isTauriAppError` type guard and `AdapterError` error translation +- **M3:** Test `invokeCommand` wrapper (success path + error wrapping) +- **M4:** Test all 20+ row mapper functions (`toExerciseRow`, `toWorkoutLogRow`, etc.) +- **M5:** Test CRUD operations -- exercise, workout log, logged activity group/activity/set, user profile, 1RM history +- **M6:** Test complex aggregation methods: `getWorkoutLogFull`, `createWorkoutLogFull`, `getExerciseWorkoutHistory`, `getWeeklyVolume`, `getVaultSummary` +- **M7:** Test session template CRUD: `getSessionTemplates`, `getSessionTemplateFull`, `createSessionTemplateFull`, `updateSessionTemplateFull`, `deleteSessionTemplate` +- **M8:** Test program CRUD: `getPrograms`, `getProgramFull`, `createProgramFull`, `updateProgramFull`, `deleteProgram` +- **M9:** Test program activation methods: `getActiveProgram`, `setActiveProgram`, `updateActiveProgram`, `clearActiveProgram` +- **M10:** Test accountability group + member + invite methods +- **M11:** Test direct connection methods +- **M12:** Test chat/conversation methods: `createConversation`, `getConversations`, `sendMessage`, `getMessages`, `getUnreadCounts`, etc. +- **M13:** Mock `@tauri-apps/api/core` `invoke` function (same pattern as existing auth test mocks) +- **M14:** Test standalone helpers: `getMonday()`, `formatWeekLabel()` + +### Should Have + +- **S1:** Test filter argument serialization in `getExercises()` (category, movement pattern, search, custom-only filters) +- **S2:** Test error paths -- `invoke` rejecting with TauriAppError vs unknown error +- **S3:** Test null/undefined edge cases in row mappers (nullable columns) +- **S4:** Test methods that return `null` when entity not found (`getExercise`, `getWorkoutLog`, `getSessionTemplate`, etc.) + +### Won't Have (this iteration) + +- Integration tests with actual SQLite/Tauri runtime +- Performance benchmarks +- E2E tests through the UI layer + +## Testable Assertions + +- **TA1:** All conversion helpers produce correct output for representative inputs (including edge cases like null, 0, empty string) +- **TA2:** `AdapterError` correctly maps `TauriAppError` fields to `message`, `code`, `details` +- **TA3:** Every row mapper transforms a Tauri response interface into the correct domain Row type +- **TA4:** CRUD methods invoke the correct Tauri command name with the correct argument shape +- **TA5:** Aggregation methods compose multiple invoke results correctly (e.g. `getWorkoutLogFull` assembles log + groups + activities + sets) +- **TA6:** Methods that throw `not implemented` (event items, share links, clone) do so consistently +- **TA7:** `getExercises` passes filter arguments through to invoke correctly +- **TA8:** All tests pass (`bun run test`) + +## Open Questions + +None -- scope is well-defined by the issue and the existing SupabaseAdapter test precedent. + +## Dependencies + +- Existing `@tauri-apps/api/core` mock pattern (used in auth tests) +- `DataAdapter` interface and domain types from `src/lib/database.types.ts` / `src/domain/types/` diff --git a/Context/Features/012-TauriAdapter-Tests/Steps.md b/Context/Features/012-TauriAdapter-Tests/Steps.md new file mode 100644 index 00000000..f720df0c --- /dev/null +++ b/Context/Features/012-TauriAdapter-Tests/Steps.md @@ -0,0 +1,357 @@ +# Feature 012: TauriAdapter Unit Tests -- Implementation Steps + +**Status:** Done +**Created:** 2026-04-04 + +## Team Composition + +| Role | Agent Type | Rationale | +| ----------- | ---------------- | -------------------------------------------------------------------------- | +| test-writer | backend-engineer | Single specialist -- all work is in one test file with one mock dependency | + +This is a focused, single-file task. No cross-domain coordination needed, so a single specialist is sufficient. + +## Implementation Steps + +### Wave 1: Scaffolding + Helpers + Error Handling + +#### S001: Scaffold test file with mock setup, fixtures, and conversion helper tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** nothing +**Parallel:** no (foundational) + +**Tasks:** + +1. Create `src/lib/__tests__/tauri-adapter.test.ts` +2. Add `vi.mock('@tauri-apps/api/core', () => ({ invoke: vi.fn() }))` before imports +3. Import `invoke` from `@tauri-apps/api/core`, `TauriAdapter`, `AdapterError` from `../tauri-adapter` +4. Define Tauri response fixtures at module level (matching `TauriXxxResponse` shapes -- integers for bools, JSON strings for objects): + - `tauriExerciseResponse` (with `is_bilateral: 1`, `supports_1rm: 1`, `is_custom: 0`, `muscle_groups` as JSON string, etc.) + - `tauriWorkoutLogResponse` + - `tauriLoggedActivityGroupResponse` + - `tauriLoggedActivityResponse` + - `tauriLoggedSetResponse` + - `tauriUserProfileResponse` + - `tauriOneRepMaxHistoryResponse` + - `tauriSessionTemplateResponse`, `tauriActivityGroupResponse`, `tauriActivityResponse` + - `tauriProgramResponse`, `tauriBlockResponse`, `tauriBlockWeekResponse`, `tauriScheduledSessionResponse` + - `tauriProgramActivationResponse` + - `tauriAccountabilityGroupResponse`, `tauriGroupMemberResponse`, `tauriGroupInviteResponse` + - `tauriDirectConnectionResponse` + - `tauriConversationResponse`, `tauriMessageResponse`, `tauriMediaAttachmentResponse` +5. Add `beforeEach` with `vi.clearAllMocks()` and `adapter = new TauriAdapter('user-001')` +6. Write `describe('Error handling')`: + - `describe('invokeCommand')`: + - `it('wraps TauriAppError in AdapterError')` -- mock invoke to reject with `{ kind: 'NOT_FOUND', message: 'not found' }`, call any adapter method, assert `AdapterError` thrown with correct `kind` and `message` + - `it('preserves field property on AdapterError')` -- reject with `{ kind: 'VALIDATION', message: '...', field: 'name' }` + - `it('re-throws non-TauriAppError unchanged')` -- reject with `new Error('network')`, assert plain Error thrown + - `describe('AdapterError')`: + - `it('has name "AdapterError"')` + - `it('maps all error kinds')` -- test each of NOT_FOUND, CONFLICT, VALIDATION, DATABASE, INTERNAL + +**Acceptance criteria:** + +- File compiles with no TypeScript errors +- Error handling tests pass +- Fixtures cover all Tauri response interfaces used by the adapter + +--- + +### Wave 2: Core CRUD Operations + +#### S002: Exercise operation tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S001 +**Parallel:** no (same file, sequential waves) + +**Tasks:** + +1. `describe('Exercise operations')`: + - `describe('getExercises')`: + - `it('returns mapped exercises with no filters')` -- mock invoke to return `[tauriExerciseResponse]`, assert result has booleans (not ints), parsed objects (not JSON strings), correct field mapping + - `it('passes category filter to invoke')` -- assert invoke called with `{ userId: 'user-001', category: 'BARBELL' }` + - `it('passes movement pattern filter')` + - `it('passes search filter')` + - `it('passes customOnly filter')` + - `it('returns empty array when no exercises')` -- mock invoke to return `[]` + - `describe('getExercise')`: + - `it('returns mapped exercise by id')` + - `it('returns null when not found')` -- mock invoke to return `null` + - `describe('createExercise')`: + - `it('invokes create command with correct args and returns mapped result')` + +**Validates:** TA1, TA3, TA4, TA7 + +#### S003: Workout log operation tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S002 + +**Tasks:** + +1. `describe('Workout log operations')`: + - `describe('getWorkoutLogs')` -- returns mapped list, passes userId and limit + - `describe('getWorkoutLogsSummary')` -- returns summary list with limit/offset + - `describe('getWorkoutLog')` -- returns single or null + - `describe('getWorkoutLogFull')` -- mock invoke to return `{ log, groups, activities, sets }` composite, assert all sub-arrays mapped correctly (TA5) + - `describe('createWorkoutLog')` -- correct args, returns mapped result + - `describe('updateWorkoutLog')` -- correct args + - `describe('deleteWorkoutLog')` -- invokes delete command + - `describe('createLoggedActivityGroup')` -- correct args including userId + - `describe('createLoggedActivity')` -- correct args + - `describe('createLoggedSet')` -- correct args with set field mapping + - `describe('updateLoggedSet')` -- correct args + - `describe('createWorkoutLogFull')` -- complex creation with groups/activities/sets (TA5) + +**Validates:** TA3, TA4, TA5 + +#### S004: User profile and 1RM tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S003 + +**Tasks:** + +1. `describe('User profile operations')`: + - `getUserProfile` -- returns mapped profile or null + - `updateUserProfile` -- passes partial updates +2. `describe('One rep max operations')`: + - `saveOneRepMax` -- correct args + - `getOneRepMaxHistory` -- returns list +3. `describe('Exercise history')`: + - `getRecentlyUsedExerciseIds` -- returns string array + - `getExerciseWorkoutHistory` -- returns composite `WorkoutWithSets[]` + +**Validates:** TA3, TA4 + +--- + +### Wave 3: Templates, Programs, Social + +#### S005: Session template operation tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S004 + +**Tasks:** + +1. `describe('Session template operations')`: + - `getSessionTemplates` -- returns mapped list + - `getSessionTemplate` -- single or null + - `getSessionTemplateFull` -- composite with groups and activities + - `createSessionTemplateFull` -- complex creation + - `updateSessionTemplateFull` -- complex update + - `deleteSessionTemplate` -- invokes delete + - `cloneSessionTemplate` -- throws not implemented (TA6) + +**Validates:** TA3, TA4, TA6 + +#### S006: Program and activation tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S005 + +**Tasks:** + +1. `describe('Program operations')`: + - `getPrograms` -- returns mapped list + - `getProgramFull` -- composite with blocks/weeks/sessions + - `createProgramFull` -- complex creation + - `updateProgramFull` -- complex update + - `deleteProgram` -- invokes delete + - `assignProgramToMember` -- correct args +2. `describe('Program activation')`: + - `getActiveProgram` -- returns activation or null + - `setActiveProgram` -- correct args including optional startDate + - `updateActiveProgram` -- passes partial updates + - `clearActiveProgram` -- invokes clear + +**Validates:** TA3, TA4 + +#### S007: Accountability group, member, invite, and connection tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S006 + +**Tasks:** + +1. `describe('Accountability groups')`: + - `createGroup`, `getGroups`, `getGroup`, `updateGroup`, `deleteGroup` +2. `describe('Group members')`: + - `getGroupMembers`, `removeGroupMember`, `updateMemberRole` +3. `describe('Group invites')`: + - `createInvite`, `getGroupInvites`, `revokeInvite`, `joinGroupByCode` +4. `describe('Direct connections')`: + - `requestConnection`, `getConnections`, `getPendingConnections`, `acceptConnection`, `declineConnection`, `removeConnection`, `updateConnectionWriteAccess` +5. `describe('Activity feed')`: + - `getGroupActivityFeed`, `getConnectionActivityFeed` + +**Validates:** TA3, TA4 + +--- + +### Wave 4: Chat, Analytics, Stubs, Verification + +#### S008: Chat operation tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S007 + +**Tasks:** + +1. `describe('Chat operations')`: + - `createConversation` -- correct args including type, participantIds, optional title/groupId + - `getConversations` -- returns mapped list + - `getConversation` -- single or null + - `findDirectConversation` -- returns conversation or null + - `sendMessage` -- correct args + - `getMessages` -- passes before/limit options + - `getMessagesSince` -- passes since param + - `updateLastRead` -- invokes command + - `getUnreadCounts` -- returns Map + - `addParticipant` -- correct args + - `leaveConversation` -- invokes command + - `toggleArchive` -- invokes command + - `saveMediaAttachment` -- correct args + - `getMediaAttachments` -- passes messageIds array + - `updateMediaAttachment` -- throws not implemented (TA6) + +**Validates:** TA3, TA4, TA6 + +#### S009: Analytics and standalone helper tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S008 + +**Tasks:** + +1. `describe('Analytics')`: + - `describe('getWeeklyVolume')` -- mock invoke to return raw workout data, assert weekly aggregation with correct week labels (exercises `getMonday`/`formatWeekLabel` indirectly) + - `describe('getVaultSummary')` -- mock invoke, assert summary shape +2. `describe('Not-implemented stubs')`: + - Test all methods that throw "not implemented": event item methods (`getEventItems`, `saveEventItem`, `updateEventItem`, `deleteEventItem`, `toggleEventItemPacked`, `reorderEventItems`), share link methods (`getShareLinks`, `getShareLinksForEntity`, `createShareLink`, `revokeShareLink`, `deleteShareLink`) + - Each should throw with a descriptive message (TA6) + +**Validates:** TA5, TA6 + +#### S009-T: Run full test suite and fix failures + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S009 + +**Tasks:** + +1. Run `bun run test` -- ensure all new tests pass +2. Run `bun run lint` -- ensure no linting errors +3. Fix any TypeScript errors, assertion failures, or mock issues +4. Verify no existing tests are broken + +**Validates:** TA8 + +--- + +### Wave 5: Review Follow-ups + +#### S010: Add malformed JSON handling tests for parseJson + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S009-T + +**Tasks:** + +1. Add tests verifying behavior when Rust returns malformed JSON in fields like `aliases`, `muscle_groups`, `equipment_required` +2. Test with values like `"not-valid-json{"` to catch regressions in the conversion layer +3. Verify silent `undefined` propagation does not occur + +**Review ref:** P9-004 + +--- + +#### S011: Add DATABASE and INTERNAL error kind tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S009-T + +**Tasks:** + +1. Add error handling tests for DATABASE error kind (most common production IPC failure) +2. Add error handling tests for INTERNAL error kind +3. Verify `AdapterError` correctly wraps these error shapes + +**Review ref:** P9-005 + +--- + +#### S012: Add Supabase getUnreadCounts happy path test + +**Agent:** test-writer +**Files:** `src/lib/__tests__/supabase-adapter.test.ts` +**Depends on:** S009-T + +**Tasks:** + +1. Add happy path test for `getUnreadCounts` where participations exist with actual unread messages +2. Verify the returned Map contains correct conversation IDs and counts + +**Review ref:** P9-006 + +--- + +#### S013: Add isoToUnixSeconds edge case tests + +**Agent:** test-writer +**Files:** `src/lib/__tests__/tauri-adapter.test.ts` +**Depends on:** S009-T + +**Tasks:** + +1. Add tests for `isoToUnixSeconds` with invalid ISO strings, empty strings, and null timestamps +2. Verify `NaN` is not silently propagated (could cause data corruption) +3. Test indirectly via adapter methods that use timestamp conversion + +**Review ref:** P9-007 + +--- + +#### S014: Restore .bind() position comments in chat.rs INSERT OR REPLACE + +**Agent:** test-writer +**Files:** `src-tauri/src/commands/chat.rs` +**Depends on:** S009-T + +**Tasks:** + +1. Locate the INSERT OR REPLACE query in chat.rs (~line 640 area) +2. Add position-numbered comments to each `.bind()` call matching the SQL parameter positions +3. Verify comments match the actual SQL column order + +**Review ref:** P9-008 + +--- + +## Milestone Summary + +| Milestone | After Step | Testable Assertions | +| ----------------- | ---------- | ----------------------------------------------------- | +| M1: Foundation | S001 | TA1 (partial), TA2 | +| M2: Core CRUD | S004 | TA1, TA3 (partial), TA4 (partial), TA5 (partial), TA7 | +| M3: Full Coverage | S009 | TA3, TA4, TA5, TA6 | +| M4: Green Suite | S009-T | TA8 | + +## Execution Recommendation + +**Use `/impl 012`** -- this is a single-specialist, single-file task with no cross-domain coordination needed. diff --git a/Context/Features/012-TauriAdapter-Tests/Tech.md b/Context/Features/012-TauriAdapter-Tests/Tech.md new file mode 100644 index 00000000..53a6655c --- /dev/null +++ b/Context/Features/012-TauriAdapter-Tests/Tech.md @@ -0,0 +1,152 @@ +# Feature 012: TauriAdapter Unit Tests -- Technical Plan + +**Status:** Done +**Created:** 2026-04-04 + +## Architecture Overview + +The test file `src/lib/__tests__/tauri-adapter.test.ts` will mock the Tauri IPC layer (`invoke` from `@tauri-apps/api/core`) and exercise every exported function and class method in `src/lib/tauri-adapter.ts`. + +### Test Layer Diagram + +``` +[Test File] + | + +--> vi.mock('@tauri-apps/api/core') -- replaces invoke with vi.fn() + | + +--> import { TauriAdapter, AdapterError } from '../tauri-adapter' + | + +--> Each test: + 1. vi.mocked(invoke).mockResolvedValue(fixtureData) + 2. Call adapter method + 3. Assert return value shape + invoke called with correct cmd/args +``` + +## Key Technical Decisions + +### KD1: Mock Strategy -- vi.mock at module level + +**Decision:** Mock `@tauri-apps/api/core` with `vi.mock()` at the top of the file (before imports), exactly matching the pattern in `adapter.test.ts` and `config-store.test.ts`. + +```ts +vi.mock('@tauri-apps/api/core', () => ({ + invoke: vi.fn(), +})) +``` + +**Why:** The `invokeCommand()` helper in tauri-adapter.ts imports `invoke` at module level. Vitest's hoisted `vi.mock()` intercepts this import before the module initializes, so all calls to `invoke` inside `invokeCommand` go through the mock. + +**Alternative considered:** Manual dependency injection (pass `invoke` as a constructor param). Rejected because it would require changing production code solely for testability and diverge from the existing pattern. + +### KD2: Fixture Design -- Tauri Response shapes, not Row shapes + +**Decision:** Fixtures must use the `TauriXxxResponse` interface shapes (integers for booleans, JSON strings for object columns), NOT the `XxxRow` types from `database.types.ts`. + +**Why:** The row mappers (`toExerciseRow`, etc.) are the code under test. If fixtures already used Row shapes, the mappers would be trivially passing data through. Using Tauri response shapes exercises the actual conversion logic (e.g., `intToBool(1)` -> `true`, `parseJson('{"a":1}', 'col')` -> `{a:1}`). + +### KD3: Testing Unexported Functions + +**Decision:** Test standalone helpers (`intToBool`, `parseJson`, `requireString`, `isoToUnixSeconds`, row mappers, `getMonday`, `formatWeekLabel`) indirectly through the class methods that use them. + +**Why:** These functions are not exported from `tauri-adapter.ts`. Rather than exporting them solely for testing: + +- Conversion helpers are exercised through row mapper assertions (verify the returned object has booleans, parsed objects, etc.) +- `invokeCommand` is exercised through every adapter method that delegates to Rust (not-implemented stubs throw errors without going through `invokeCommand`) +- `isTauriAppError` + `AdapterError` are exercised through error-path tests where `invoke` rejects with a `TauriAppError`-shaped object + +If any helper has edge cases that can't be reached through public API (e.g., `parseJson` with malformed JSON), we can test that by having `invoke` return a response with an invalid JSON string field. + +**Alternative considered:** Export helpers behind a `_testing` namespace. Rejected -- the indirect testing approach is sufficient and avoids polluting the module's public API. + +### KD4: Test File Structure -- Mirror SupabaseAdapter + +**Decision:** Organize tests in nested `describe` blocks matching the SupabaseAdapter test structure: + +``` +describe('TauriAdapter') + describe('Conversion helpers (via mapped results)') + describe('Error handling') + describe('invokeCommand') + describe('AdapterError') + describe('Exercise operations') + describe('getExercises') + describe('getExercise') + describe('createExercise') + describe('Workout log operations') + ... + describe('Session template operations') + ... + describe('Program operations') + ... + describe('Program activation') + ... + describe('Accountability groups') + ... + describe('Direct connections') + ... + describe('Chat operations') + ... + describe('Analytics') + describe('getWeeklyVolume') + describe('getVaultSummary') + describe('Standalone helpers') + describe('getMonday') + describe('formatWeekLabel') + describe('Not-implemented stubs') +``` + +### KD5: Standalone Helpers Are Not Exported + +**Decision:** `getMonday` and `formatWeekLabel` are standalone functions at the end of the file. They are NOT exported and are tested indirectly through `getWeeklyVolume`, which uses both. + +### KD6: beforeEach Pattern + +**Decision:** Each test gets a fresh adapter and cleared mocks: + +```ts +let adapter: TauriAdapter + +beforeEach(() => { + vi.clearAllMocks() + adapter = new TauriAdapter('user-001') +}) +``` + +**Why:** Matches the SupabaseAdapter pattern. `vi.clearAllMocks()` resets call counts and return values, preventing test leakage. + +## Stack-Specific Details + +### Vitest Configuration + +- Environment: `node` (no browser globals needed -- TauriAdapter is pure TypeScript) +- Setup file: `src/test/setup.ts` (Testing Library cleanup only, no Tauri setup) +- Alias: `@/` maps to `./src` +- Worktree exclusion: test file goes in `src/lib/__tests__/` which is included in the default test run + +### Mock Patterns + +| What | How | Where | +| ------------------------------------ | ---------------------------------------------------------------------------- | -------------------- | +| `invoke` from `@tauri-apps/api/core` | `vi.mock()` + `vi.mocked(invoke).mockResolvedValue()` | Top of test file | +| Error paths | `vi.mocked(invoke).mockRejectedValue({ kind: 'NOT_FOUND', message: '...' })` | Per-test | +| Unknown errors | `vi.mocked(invoke).mockRejectedValue(new Error('network'))` | Error handling tests | + +### Type Considerations + +- Tauri response interfaces (`TauriExerciseResponse`, etc.) are not exported. Fixtures will use inline object literals typed to match the interface shapes. +- The `TauriAppError` interface is not exported either. Error fixtures use plain objects matching `{ kind, message, field? }`. + +## Risks and Mitigations + +| Risk | Likelihood | Impact | Mitigation | +| ------------------------------------------ | ---------- | ------ | ---------------------------------------------------------------------------------------------------- | +| Unexported types make fixtures fragile | Medium | Low | Type assertions via `satisfies` where possible; tests will break on shape changes (which is desired) | +| Large test file (est. 2000+ lines) | High | Low | Well-structured describe blocks; row fixtures at top; each domain section is independent | +| `getMonday`/`formatWeekLabel` not exported | Medium | Low | Test indirectly through `getWeeklyVolume`; if insufficient coverage, extract to a utils file | + +## Integration Points + +- **No external dependencies** beyond `@tauri-apps/api/core` mock +- **No database** needed (all SQLite access is behind `invoke`) +- **No network** needed (pure unit tests) +- Tests validate the contract between the TypeScript frontend and the Rust backend by asserting the exact command names and argument shapes passed to `invoke` diff --git a/Context/QuickPlans/2026-04-03-recently-used-templates.md b/Context/QuickPlans/2026-04-03-recently-used-templates.md new file mode 100644 index 00000000..caf09815 --- /dev/null +++ b/Context/QuickPlans/2026-04-03-recently-used-templates.md @@ -0,0 +1,76 @@ +# Quick Plan: Recently Used Section in Session Picker + +**Task:** Add a "Recently Used" section to `SessionPickerSheet` that shows the last 3-5 assigned templates pinned above the full list. + +**Goal:** Reduce picker friction when users have 20+ templates by surfacing the most recently assigned ones at the top without requiring any user action. + +--- + +## Approach + +### 1. Domain type + schema (`src/domain/types/session.ts`) +Add optional `lastAssignedAt: z.string().datetime().optional()` to `sessionTemplateSchema`. Inherits from `syncableEntitySchema` — this is purely additive. + +### 2. SQLite migration (`src-tauri/migrations/`) +New file `004_session_template_last_assigned.sql`: +```sql +ALTER TABLE session_templates ADD COLUMN last_assigned_at INTEGER; +``` + +### 3. SQLite adapter update (`src-tauri/src/`) +In the template repository's assign/update path, set `last_assigned_at = unixepoch()` when a template is assigned to a session slot. The Tauri command that fires when `onSelect` is called in the builder needs to touch this column. + +> Need to confirm the exact command name — likely in `src-tauri/src/commands/` or similar. + +### 4. Supabase migration (`supabase/migrations/`) +New migration adding column to `session_templates`: +```sql +ALTER TABLE session_templates ADD COLUMN last_assigned_at TIMESTAMPTZ; +``` + +### 5. Hook update (`src/hooks/use-session-templates.ts`) +Expose a `recentTemplates` derived value: filter templates where `lastAssignedAt` is set, sort by `lastAssignedAt DESC`, slice to top 5. + +Alternatively, add a `useRecentSessionTemplates(userId, limit)` hook that queries SQLite directly with `ORDER BY last_assigned_at DESC LIMIT 5` to keep it efficient at scale. + +### 6. Picker update (`src/components/program-builder/session-picker-sheet.tsx`) +- If `recentTemplates.length > 0` and search is empty and filter is `ALL`: render a "Recent" section header + up to 5 recent template buttons above the full list. +- Hide the recent section when the user is actively searching or filtering (it would be redundant and confusing). +- Reuse the same `handleSelect` callback -- no behavior change on selection. +- No new state needed. + +### 7. Trigger `last_assigned_at` update +In `builder.tsx` (or wherever `onSelect` lands), after assigning the template to the draft, call a new Tauri command `touch_template_last_assigned(templateId)` or piggyback on an existing upsert. + +--- + +## File Checklist + +| File | Change | +|---|---| +| `src/domain/types/session.ts` | Add `lastAssignedAt` field | +| `src-tauri/migrations/004_*.sql` | New migration | +| `supabase/migrations/YYYYMMDD_*.sql` | New migration | +| `src-tauri/src/...` | Update assign path to set `last_assigned_at` | +| `src/hooks/use-session-templates.ts` | Expose recent templates | +| `src/components/program-builder/session-picker-sheet.tsx` | Render "Recent" section | +| `src/routes/_authenticated/builder.tsx` | Trigger timestamp update on select | + +--- + +## Verification + +- Assign a template to a slot; reopen the picker -- it appears in "Recent" at the top. +- After assigning 6+ different templates, only the 5 most recent appear. +- Typing in the search box hides the Recent section. +- Changing the category filter (non-ALL) hides the Recent section. +- Fresh install with no assignments: Recent section is absent (no empty header). +- Existing templates with no `last_assigned_at` degrade gracefully (no crash, no entry in Recent). + +--- + +## Risks + +- **Tauri command location:** Need to confirm where template assignment is persisted in the Rust layer before adding the timestamp update. Incorrect placement could miss updates or double-count. +- **Sync conflict:** If `last_assigned_at` is synced to Supabase, a multi-device user's recent list would reflect the last device used -- acceptable behavior, but worth being aware of. +- **Migration ordering:** SQLite migrations run at app launch via Tauri; Supabase migrations run via `npx supabase db push`. Both must be present before testing end-to-end. diff --git a/Context/QuickPlans/2026-04-03-week-grid-density.md b/Context/QuickPlans/2026-04-03-week-grid-density.md new file mode 100644 index 00000000..33f9c7da --- /dev/null +++ b/Context/QuickPlans/2026-04-03-week-grid-density.md @@ -0,0 +1,77 @@ +# Quick Plan: Week Grid Density Improvements + +**Task:** Three UX improvements to reduce visual noise in the program builder week grid without losing the spatial calendar metaphor. + +**Goal:** Make the grid feel intentional and complete rather than sparse and unfinished, especially on mobile and in multi-week blocks. + +--- + +## Change 1: Mobile 5/7-day toggle + +**Where:** `src/routes/_authenticated/builder.tsx` + `src/components/program-builder/mobile-block-editor.tsx` + +**What:** The `showWeekends` state already exists in the builder route and already flows to `BlockList` (desktop). It is never passed to `MobileBlockEditor`. `MobileDayRow` hardcodes `DAY_ORDER` ([1,2,3,4,5,6,0]) with no filtering. + +**Approach:** +1. Pass `showWeekends` and `onToggleWeekends` down to `MobileBlockEditor` as props. +2. Add the toggle button to the mobile block editor header (currently has block controls — add the same "5 days / 7 days" toggle button that desktop uses, but visible on mobile). +3. In `MobileWeekSection`, filter `DAY_ORDER` to `WEEKDAY_COLUMNS` when `showWeekends=false`, same logic as `WeekGrid`. +4. If a weekend session exists on a week where weekends are hidden, show the same "+N weekend session(s)" note that `WeekGrid` already shows. + +**Files:** +- `src/routes/_authenticated/builder.tsx` -- pass props to MobileBlockEditor +- `src/components/program-builder/mobile-block-editor.tsx` -- accept props, add toggle button, filter day list + +--- + +## Change 2: Rest day visual treatment + +**Where:** `src/components/program-builder/session-slot.tsx` (desktop) + `MobileDayRow` in `mobile-block-editor.tsx` + +**What:** Empty slots currently look like unfilled calls-to-action (dashed border, `+` icon). This reads as "something is missing" rather than "this is a rest day". The fix is a subtle visual that reads as intentional without removing the ability to assign a session. + +**Approach:** +- Replace the dashed-border `+` button for empty slots with a quieter treatment: no border, muted background (`bg-surface-gunmetal/40` or similar), and a small "Rest" label in `text-warm-ash/30` instead of a `+` icon. +- Keep the slot fully interactive -- clicking still opens the picker. Add a hover state that reveals the `+` icon to indicate it's assignable. +- On mobile, the empty `MobileDayRow` currently says "Tap to assign" -- replace with just the day label and a very muted right-side `+` that appears on hover/focus. + +**Files:** +- `src/components/program-builder/session-slot.tsx` -- restyle empty state +- `src/components/program-builder/mobile-block-editor.tsx` -- restyle empty `MobileDayRow` + +--- + +## Change 3: Collapse identical weeks + +**Where:** `src/components/program-builder/block-editor.tsx` (desktop) + `mobile-block-editor.tsx` (mobile) + +**What:** In a 4-week block where weeks 2-4 repeat week 1's pattern (common in linear progression programs), each week currently renders full height. This triples the vertical space with duplicate structure. + +**Approach:** +- Add a `weekMatchesPrevious(week: WeekDraft, previous: WeekDraft): boolean` utility in `builder-state.ts` that compares session arrays by `dayOfWeek` and `sessionTemplateId`. +- In `BlockEditor` and `MobileBlockCard`, for any week where `weekMatchesPrevious` is true, render a collapsed summary row instead of the full grid: "Week N -- same as Week 1" with an expand chevron. +- The collapsed row should still show the session count and type badges (e.g., "3 sessions: STR / COND / SE") so the user knows what's in the week without expanding. +- Clicking the row or chevron expands it to the full grid inline. State is local (`expandedWeeks: Set`) in `BlockEditor`. +- Default: all weeks expanded. The collapse only applies when `weekMatchesPrevious` returns true, so a new empty block always shows all weeks expanded. + +**Files:** +- `src/components/program-builder/builder-state.ts` -- add `weekMatchesPrevious` utility +- `src/components/program-builder/block-editor.tsx` -- render collapsed summary for matching weeks +- `src/components/program-builder/mobile-block-editor.tsx` -- same for mobile + +--- + +## Verification + +- Mobile builder shows 5-day grid by default; toggle switches to 7-day +- Weekend sessions on a 5-day mobile view show the "+N weekend" note +- Empty slots read as rest days visually; hover/focus reveals the assignable state +- A 4-week block where weeks 2-4 are identical renders collapsed "same as Week 1" rows +- Expanding a collapsed week shows the full grid +- A block with all different weeks renders fully expanded with no collapsed rows + +## Risks + +- **Change 3 interaction with remove/assign**: When a week is collapsed and the user assigns or removes a session elsewhere that makes the week no longer identical, it should auto-expand. Need to invalidate the collapsed state in `handleSessionSelected` and `handleRemoveSession`. +- **Change 2 hover on touch devices**: "hover to reveal +" won't work on mobile tap. The empty mobile row should always show a subtle `+` since there's no hover state. +- **Change 1 weekend sessions note position**: On mobile the note goes below `MobileWeekSection` header, not below a horizontal grid row — needs a different placement than desktop. diff --git a/Context/QuickPlans/2026-04-04-address-all-backlog.md b/Context/QuickPlans/2026-04-04-address-all-backlog.md new file mode 100644 index 00000000..68f34bdf --- /dev/null +++ b/Context/QuickPlans/2026-04-04-address-all-backlog.md @@ -0,0 +1,75 @@ +# Quick Plan: Address All Backlog + +**Date:** 2026-04-04 +**Task:** Resolve all open backlog items: B001 (bug), P9-010 (domain type tests), P9-011 (unread counts test), and S-6 (N+1 query). +**Goal:** Clear the backlog to zero open items before starting the next feature. + +--- + +## Items + +### B001 -- invokeCommand missing log (High Bug) + +**File:** `src/lib/tauri-adapter.ts:504-512` +**Approach:** In the `invokeCommand` catch block, add `console.error('[tauri-adapter] invokeCommand failed:', err)` before the re-throw. Must not swallow the error -- log then rethrow. +**Verification:** Catch block has `[tauri-adapter]` prefixed log and still re-throws `AdapterError`. + +--- + +### P9-010 -- Domain type tests: conversation, message, media (High) + +**File:** New test file alongside existing domain type tests (e.g. `src/domain/types/__tests__/chat.test.ts` or equivalent) +**Approach:** Add Vitest unit tests covering the Zod refinements added in PR #71: + +- Direct conversation + `groupId` set → rejected +- Group conversation + `groupId` set → accepted +- `leftAt > joinedAt` → accepted; `leftAt < joinedAt` → rejected; `leftAt === joinedAt` → check boundary +- System message + `senderId` set → rejected +- Negative `durationSeconds` → rejected; zero → accepted +- Negative `fileSizeBytes` → rejected; zero → accepted + +**Approach:** Check existing domain test files (e.g. `src/domain/types/__tests__/`) for pattern, then add `chat.test.ts` following the same structure. +**Verification:** `bun run test` passes with all new cases green. + +--- + +### P9-011 -- getUnreadCounts batching test (Medium) + +**File:** Test file for `getUnreadCounts` (likely in `src/lib/__tests__/` or co-located with the adapter tests) +**Approach:** Add Vitest tests for the `Promise.allSettled` behavior: + +- All participations succeed → returns full map +- One participation fails → partial results returned (others still present); failed entry absent or defaulted +- Empty participations array → returns empty map +- `null` `last_read_at` → treated as "all unread" (0 read) + **Verification:** All four cases covered and passing. + +--- + +### S-6 -- findDirectConversation N+1 (Low) + +**File:** `src/lib/tauri-adapter.ts` -- `findDirectConversation` +**Approach:** Replace the per-conversation loop with a single batched query that fetches all participant sets for the candidate conversation IDs in one IPC call, then filters in JS. If a batch IPC command doesn't exist, evaluate whether to add one or leave this deferred. +**Decision gate:** If no batch command exists and adding one is non-trivial (new Rust command needed), defer S-6 and leave it in the backlog as Low. Do not over-engineer. +**Verification:** `findDirectConversation` issues at most 2 IPC calls regardless of candidate count. + +--- + +## Execution Order + +1. B001 (5 min, isolated) -- fix first, trivial +2. P9-010 (domain type tests) -- add test file, ~10-15 cases +3. P9-011 (unread counts test) -- add test file, 4 cases +4. S-6 (N+1 query) -- investigate batch feasibility; defer if non-trivial + +## Risks + +- Domain type test file location: check existing pattern before creating +- S-6 may require a new Tauri command; if so, scope expands beyond quick plan -- defer +- `getUnreadCounts` test may need mock setup matching existing adapter test patterns + +## Verification + +- `bun run test` passes +- `bun run lint` clean +- Backlog items B001, P9-010, P9-011 removed; S-6 either resolved or re-evaluated diff --git a/Context/QuickPlans/2026-04-04-enhancement-issues-batch.md b/Context/QuickPlans/2026-04-04-enhancement-issues-batch.md new file mode 100644 index 00000000..c00060a1 --- /dev/null +++ b/Context/QuickPlans/2026-04-04-enhancement-issues-batch.md @@ -0,0 +1,138 @@ +# Quick Plan: Batch Enhancement Issues + +**Task:** Address all open GitHub issues labeled "enhancement" (#31, #39-#42, #46-#49, #51) +**Goal:** Close out review-sourced polish, perf, and small feature items in one session +**Branch:** `chore/enhancements` (from `develop`) + +--- + +## Pre-Flight: Already Resolved Items + +Research shows the following items from Group 4 (#42/#49) and Group 5 (#51) are **already implemented**: + +| Issue Item | Status | Evidence | +| ------------------------------------------------- | ------ | ---------------------------------------------------- | +| S009: Extract TYPING_EXPIRY_MS/TYPING_DEBOUNCE_MS | Done | `realtime-manager.ts` L17/L20 | +| S010: Extract toMessageFromBroadcast mapper | Done | `realtime-schemas.ts` L53-62 | +| S011: Remove empty SUBSCRIBED branch | Done | Branch does not exist in current code | +| S013: Log error before visibilitychange fallback | Done | `foreground-detector.ts` L115-123 | +| S015: Extract MessagePaginationOptions interface | Done | `data-adapter.ts` L97-102 | +| S016: Flatten .then() to async/await | Done | `foreground-detector.ts` uses async/await throughout | +| S017: Add z.string().max(200) to user_name | Done | `realtime-schemas.ts` L29 | +| S018: Fix getSession() comment | Done | Comment does not exist; auth uses useAuth() | +| S019: Migrate useBlockedUsers to Zustand | Done | `use-blocked-users.ts` wraps `useBlockedUsersStore` | + +**Action:** Close issues #42 and #49 (all items resolved). Update #51 to remove S019 (done). + +--- + +## Remaining Work + +### Wave 1 -- Parallel (no file conflicts) + +#### Agent A: Zod Refinements (Issues #40/#47) + +**Files:** `src/domain/types/conversation.ts`, `src/domain/types/message.ts`, `src/domain/types/media.ts` + +1. `conversation.ts` -- Add `.refine()` preventing `groupId` on direct conversations: + - When `type === 'direct'`, `groupId` must be undefined + - Apply to `conversationSchema` (L15-21) + +2. `conversation.ts` -- Add temporal ordering refinement: + - `leftAt > joinedAt` when both are present + - Apply to `conversationParticipantSchema` (L27-35) + +3. `message.ts` -- Add `.refine()` preventing `senderId` on system messages: + - When `messageType === 'system'`, `senderId` must be undefined + - Apply to `messageSchema` (L93-100) + +4. `media.ts` -- Add `.nonnegative()` to numeric fields: + - `durationSeconds: z.number().int().nonnegative().optional()` + - `fileSizeBytes: z.number().int().nonnegative().optional()` + - Apply to `mediaAttachmentSchema` (L30-43) + +**Verification:** `bun run build` passes (type check); existing tests pass. + +#### Agent B: Realtime Remaining Polish (Issues #42/#49 leftovers) + +**Files:** `src/lib/realtime-manager.ts` + +1. S012: Simplify `clearAllTyping` iteration (L113-120) -- review and simplify if possible +2. S014: `broadcastTyping` `.catch(() => {})` at L289-296 swallows errors silently. Per project error-handling conventions, add `[realtime-manager]` prefixed logging. + +**Verification:** `bun run build` passes. + +#### Agent C: Chat UI -- Archive Button (Issue #51, remaining item S020) + +**Files:** `src/components/chat/conversation-header.tsx`, parent component(s) + +1. Investigate whether `onArchive` prop is wired to real functionality in the parent +2. If archive is not implemented upstream, remove the "Archive" menu option from the dropdown (both group and direct branches) to avoid a no-op button +3. If archive IS implemented, verify it works and close S020 + +**Verification:** Archive button either works or is removed. No silent no-ops. + +#### Agent D: Event Route + Badge Navigation (Issue #31) + +**Files:** `src/components/event-builder/event-countdown-badge.tsx`, new `src/routes/_authenticated/events.$templateId.tsx` + +1. Create `src/routes/_authenticated/events.$templateId.tsx` with a minimal event detail page (placeholder content is fine -- the route just needs to exist) +2. Update `event-countdown-badge.tsx`: + - Remove `_templateId` destructuring rename, use `templateId` directly + - Update `handleNavigate` to navigate to `/events/$templateId` + - Remove the TODO comment +3. Follow existing route patterns (see `_authenticated/groups.$groupId.tsx` as reference) + +**Verification:** `bun run build` passes; navigating from badge reaches the new route. + +### Wave 2 -- Sequential (shared adapter files) + +#### Agent E: getUnreadCounts N+1 Fix (Issues #39/#46) + +**Files:** `src/lib/supabase-adapter.ts` + +Current state (L2082-2122): loops over participations, fires one COUNT query per conversation. + +**Approach -- batch with single query:** + +- Build a single query that counts unread messages per conversation using `.in('conversation_id', conversationIds)` with group-by, or +- Use a Supabase RPC function for server-side aggregation +- Preserve the existing behavior: exclude own messages, handle null `last_read_at` (count all) +- Keep `console.warn` + partial results on per-conversation errors + +**Verification:** `bun run build` passes; manual test confirms unread counts still display correctly. + +#### Agent F: Chat Query Optimizations (Issues #41/#48) + +**Files:** `src/lib/tauri-adapter.ts`, `supabase/migrations/` (new migration) + +1. **findDirectConversation N+1** (`tauri-adapter.ts` L2129-2148): + - Currently fetches all conversations, then calls `get_conversation` per direct conv + - Add error context to the loop (wrap in try/catch with `[tauri-adapter]` prefix) + - Consider a batched approach if a Rust command supports it, otherwise document the limitation + +2. **CH-2 uniqueness trigger O(N)** (migration SQL): + - Current trigger calls `direct_conversation_pair()` on every existing direct conversation per INSERT + - Create a new migration that adds a materialized lookup (e.g., `direct_conversation_pairs` table or a functional index) + - Ensure the trigger uses the index for O(1) lookup instead of full scan + +**Verification:** `bun run build` passes; `npx supabase db push` applies cleanly. + +--- + +## Post-Implementation + +1. Close duplicate issues: #42 (dup of #49), #41 (dup of #48), #40 (dup of #47), #39 (dup of #46) +2. Close resolved issues with commit references +3. Run `bun run build && bun run test` for final validation + +--- + +## Risks + +| Risk | Mitigation | +| ----------------------------------------------------- | ------------------------------------------------------------------------- | +| Zod `.refine()` breaks existing data parsing | Test with real data; refinements only reject clearly invalid combinations | +| CH-2 migration changes break uniqueness enforcement | Test with local Supabase; verify trigger still prevents duplicate directs | +| getUnreadCounts batch query returns different results | Compare output against current N+1 approach with test data | +| Event route placeholder is too bare | Minimal is fine -- issue only asks for the route to exist for navigation | diff --git a/Context/QuickPlans/2026-04-04-gh-issues-32-51.md b/Context/QuickPlans/2026-04-04-gh-issues-32-51.md new file mode 100644 index 00000000..65bc4504 --- /dev/null +++ b/Context/QuickPlans/2026-04-04-gh-issues-32-51.md @@ -0,0 +1,63 @@ +# Quick Plan: GitHub Issues #32 & #51 + +**Date:** 2026-04-04 +**Issues:** #32 (event_metadata comment), #51 (archive button) + +--- + +## Issue #32 -- event_metadata column comment formatting + +**Goal:** Rewrite the `session_templates.event_metadata` column comment in a follow-up migration to eliminate the mixed quoted/unquoted pseudo-schema syntax. + +**Current (ambiguous):** + +```sql +'JSONB event metadata for EVENT templates. Expected shape: ' +'{"location": string, "eventDate": ISO-8601, "distance": {"value": number, "unit": "mi"|"km"}, ' +'"cutoffTime": {"seconds": number}, "elevation": {"value": number, "unit": "ft"|"m"}}. ' +'NULL for non-EVENT categories.'; +``` + +**Approach:** + +1. Create `supabase/migrations/_fix_event_metadata_comments.sql` +2. Use `$$` dollar quoting and a plaintext field list: + +```sql +comment on column session_templates.event_metadata is +$$JSONB event metadata for EVENT session templates. NULL for non-EVENT categories. + +Fields: + location text -- event venue or location name + eventDate text -- ISO-8601 date string (e.g. "2026-06-15") + distance jsonb -- { value: number, unit: "mi" | "km" } + cutoffTime jsonb -- { seconds: number } + elevation jsonb -- { value: number, unit: "ft" | "m" } +$$; +``` + +3. Close issue #32. + +**Verification:** `bunx supabase db push` applies cleanly locally. + +**Risks:** Comment-only migration -- no schema changes, zero risk. + +--- + +## Issue #51 -- chat UI archive button + +**Finding:** Archive IS already wired up: + +- `conversation-header.tsx` calls `onArchive()` in both direct and group menu variants +- `conversation-detail.tsx:98` -- `handleArchive` calls `toggleArchive.mutateAsync(conversationId)` then navigates to `/comms` + +The issue is stale. The archive button was wired when `toggleArchive` mutation was added post-PR #35. + +**Approach:** Close #51 with a comment confirming it was resolved. + +--- + +## Execution + +- **#32:** Use `/impl` -- single migration file, trivial +- **#51:** Close manually right now diff --git a/Context/QuickPlans/2026-04-04-s6-find-direct-conversation-n1.md b/Context/QuickPlans/2026-04-04-s6-find-direct-conversation-n1.md new file mode 100644 index 00000000..3350e061 --- /dev/null +++ b/Context/QuickPlans/2026-04-04-s6-find-direct-conversation-n1.md @@ -0,0 +1,93 @@ +# Quick Plan: S-6 -- Fix findDirectConversation N+1 + +**Task:** Eliminate N+1 IPC calls in `TauriAdapter.findDirectConversation` and add a dedicated Rust command for optimal single-query lookup. + +**Goal:** Replace the current loop of `get_conversation` calls with (A) an immediate TS fix using already-fetched participant data, then (B) a dedicated `find_direct_conversation` Rust command. + +## Current Problem + +`tauri-adapter.ts:2138` calls `getConversations()` (which already returns `participantUserIds` on each `Conversation`), then loops over direct conversations calling `get_conversation` individually to re-fetch participant data it already has. This is N+1 IPC roundtrips. + +## Phase A: TypeScript-Only Fix (1 file) + +**File:** `src/lib/tauri-adapter.ts` + +Rewrite `findDirectConversation` (line 2138) to filter using the `participantUserIds` already present on the `Conversation` objects returned by `getConversations()`: + +```typescript +async findDirectConversation(otherUserId: string): Promise { + const conversations = await this.getConversations() + return ( + conversations.find( + (c) => + c.type === 'direct' && + c.participantUserIds.includes(this.userId) && + c.participantUserIds.includes(otherUserId), + ) ?? null + ) +} +``` + +**Test update:** `src/lib/__tests__/tauri-adapter.test.ts:2279` -- remove the second `mockResolvedValueOnce` (the `get_conversation` mock) since only one IPC call (`get_conversations`) is made now. + +## Phase B: Dedicated Rust Command (3 files) + +### B1. `src-tauri/src/commands/chat.rs` -- add `find_direct_conversation` + +New command following the `get_conversation` pattern (line 242). Single SQL query: + +```sql +SELECT c.id, c."type", c.title, c.group_id, c.created_at, c.updated_at +FROM conversations c +JOIN conversation_participants cp1 ON cp1.conversation_id = c.id +JOIN conversation_participants cp2 ON cp2.conversation_id = c.id +WHERE c."type" = 'direct' + AND cp1.user_id = ? AND cp1.left_at IS NULL + AND cp2.user_id = ? AND cp2.left_at IS NULL +LIMIT 1 +``` + +Then fetch participants for the matched conversation (same pattern as `get_conversation`). Return `Option`. + +### B2. `src-tauri/src/lib.rs` -- register command + +Add `commands::chat::find_direct_conversation` to `generate_handler!` (after line 138). + +### B3. `src/lib/tauri-adapter.ts` -- use new command + +Update `findDirectConversation` to call `find_direct_conversation` instead of `getConversations`: + +```typescript +async findDirectConversation(otherUserId: string): Promise { + const result = await invokeCommand( + 'find_direct_conversation', + { user_id: this.userId, other_user_id: otherUserId }, + ) + if (!result) return null + const userIds = result.participants.filter((p) => p.left_at == null).map((p) => p.user_id) + return toConversation(toConversationRowFromTauri(result.conversation), userIds) +} +``` + +### B4. Tests + +- Update `tauri-adapter.test.ts` `findDirectConversation` tests to mock `find_direct_conversation` command +- Add test: returns conversation when both users are active participants +- Add test: returns null when no matching direct conversation +- Add test: returns null when other user has left + +## Verification + +- `bun run test` -- all existing + new tests pass +- `bun run build` -- TypeScript compiles clean +- `cd src-tauri && cargo check` -- Rust compiles clean +- Manual: verify `findDirectConversation` makes exactly 1 IPC call (check devtools network/console) + +## Risks + +- **Schema mismatch:** The SQL join must produce columns matching `ConversationRow`. Mitigated by using the exact same column list as `get_conversation`. +- **Edge case:** Two direct conversations between the same users (shouldn't happen per domain rules, but `LIMIT 1` handles it safely). + +## Execution + +Recommend `/impl` -- this is single-domain (Rust backend + TS adapter), no cross-domain coordination needed. diff --git a/Context/Reviews/0005-pr59-display-broadcast-review.md b/Context/Reviews/0005-pr59-display-broadcast-review.md new file mode 100644 index 00000000..c287eee8 --- /dev/null +++ b/Context/Reviews/0005-pr59-display-broadcast-review.md @@ -0,0 +1,222 @@ +# PR #59 Review: Display Broadcast Infrastructure + +**Date:** 2026-04-04 +**PR:** feat: display broadcast infrastructure (Step 27) +**Branch:** `feat/remote-display` -> `develop` +**Reviewers:** Claude Code (automated -- code, tests, types, errors, comments) +**Decision:** Changes requested +**Status:** 🟢 Resolved + +--- + +## Scope + +8,695 additions / 63 deletions across 75 files covering two feature areas: + +1. **Display broadcast infrastructure** -- `DisplaySnapshot` domain type, `display-publisher` service, `useDisplayBroadcast` hook, `PushToDisplayButton`, `display_visible` column +2. **Chat media** -- Cloudflare Stream video upload/playback, Supabase Storage image/file upload, attachment picker, video player, media upload service, edge functions + +--- + +## Critical (must fix before merge) + +**C1 -- Storage upload RLS lacks authorization** +- `supabase/migrations/20260403000002_media_storage.sql:33-40` +- `chat_images_insert_authenticated` and `chat_files_insert_authenticated` only check `auth.uid() is not null`. Any authenticated user can upload to any conversation's path. +- Fix: add `is_conversation_participant((string_to_array(name, '/'))[2]::uuid)` to both insert policies. +- **Status:** ✅ Fixed +- **Resolution:** Added `is_conversation_participant` check to both insert RLS policies + +**C2 -- Uploaded images never display in chat** +- `src/lib/media-upload-service.ts` + `src/components/chat/media-message-content.tsx` +- Image/file uploads set `status: 'ready'` but never populate `playbackUrl` or `thumbnailUrl`. `MediaMessageContent` returns `null` when no URL exists -- uploaded images silently vanish. +- Fix: generate and store a signed URL after upload, or add URL resolution in the component. +- **Status:** ✅ Fixed +- **Resolution:** Added `createSignedUrl` calls after upload in service; added `useStorageSignedUrl` fallback hook in component; updated test mocks + +**C3 -- Snapshot builder can crash core workout flow** +- `src/stores/active-workout-store.ts` -- `_publishCurrentState` +- No try-catch around `buildDisplaySnapshot()`. Any runtime error propagates into `completeSet`, `startRest`, etc., crashing workout UI for a secondary feature. +- Fix: wrap in try-catch with `console.error`. +- **Status:** ✅ Fixed +- **Resolution:** Wrapped `buildDisplaySnapshot` + `publishDisplaySnapshot` in try-catch in `_publishCurrentState()` + +**C4 -- Cloudflare signed URL uses wrong identifier** +- `supabase/functions/chat-media-signed-url/index.ts:~73` +- Signed URL constructed as `https://customer-${accountId}.cloudflarestream.com/...`. Cloudflare customer subdomain is separate from account ID. Likely produces invalid playback URLs. +- Fix: use a dedicated `CLOUDFLARE_CUSTOMER_SUBDOMAIN` env var and update the URL template. +- **Status:** ✅ Fixed +- **Resolution:** Added `CLOUDFLARE_CUSTOMER_SUBDOMAIN` env var with validation; updated URL template + +--- + +## Important + +**I1 -- Empty `.catch(() => {})` on TUS abort** (`src/lib/media-upload-service.ts`) +- Cancellation failures silently ignored. Upload may complete in background producing ghost attachments. +- Fix: log the error. +- **Status:** ✅ Fixed +- **Resolution:** Replaced empty catch with `console.error('[media-upload] TUS abort failed:', err)` + +**I2 -- Video player bare catch block** (`src/components/chat/video-player.tsx`) +- `hls.js` import failure caught with no logging, no `setError(true)`. Native fallback also has no `error` event listener. Result: black rectangle with no diagnostic info. +- Fix: add `console.warn` + `setError(true)` in catch; add video `error` event listener. +- **Status:** ✅ Fixed +- **Resolution:** Added `console.warn` in catch, `setError(true)` when native fallback unavailable, video `error` event listeners for both paths + +**I3 -- Transcoding timeout never cleared on unmount** (`src/hooks/use-media-upload.ts`) +- 5-minute `setTimeout` runs after navigation, causing stale query invalidations. `retry` callback also doesn't `void` the upload promise. +- Fix: store timer ID in ref, clear in `useEffect` cleanup. `void upload(...)` in `retry`. +- **Status:** ✅ Fixed +- **Resolution:** Added `transcodingTimerRef` with `useEffect` cleanup on unmount + +**I4 -- `isBroadcasting` doesn't reflect publisher readiness** (`src/lib/display-publisher.ts` + `src/hooks/use-display-broadcast.ts`) +- Hook returns `isBroadcasting: true` based on store state, not whether publisher has a working client. User sees active broadcast UI while nothing publishes. +- Fix: export `isPublisherReady()` and use in `isBroadcasting` calculation. +- **Status:** ✅ Fixed +- **Resolution:** Exported `isPublisherReady()` from publisher; included in `isBroadcasting` calculation in hook + +**I5 -- `MediaMessageContent` returns `null` for missing URL / unknown type** (`src/components/chat/media-message-content.tsx`) +- Produces empty message bubbles with no user-facing explanation. +- Fix: return a fallback UI element instead of `null`. +- **Status:** ✅ Fixed +- **Resolution:** Added "Media unavailable" fallback UI element for missing URL and unknown media types + +**I6 -- `retry` callback ignores upload Promise** (`src/hooks/use-media-upload.ts`) +- Fix: `void upload(file, type)`. +- **Status:** ✅ Fixed +- **Resolution:** Added `void` before `upload(...)` call in retry callback + +**I7 -- `displayWeightSchema` weaker than source `weightSchema`** (`src/domain/types/display-snapshot.ts`) +- `unit` is `z.string()` (should be `z.enum(['lb', 'kg'])`), `value` missing `.nonnegative()`, `reps` missing `.int().nonnegative()`. +- **Status:** ✅ Fixed +- **Resolution:** Tightened schema -- `unit` to `z.enum(['lb', 'kg'])`, added `.nonnegative()` to value, `.int().nonnegative()` to reps + +**I8 -- `z.url()` migration may break consumers** (`src/domain/types/media.ts`, `src/lib/realtime-schemas.ts`) +- Zod 4's `z.url()` may infer as `URL` object rather than `string`. If consumers pass to JSX `src={}`, this silently breaks. +- Fix: verify inferred type; revert to `z.string().url()` if needed. +- **Status:** ✅ Fixed +- **Resolution:** Reverted `z.url()` to `z.string().url()` in both files (4 occurrences) + +**I9 -- Empty `userId` passed to `useDisplayBroadcast` during loading** (`src/routes/_authenticated/log.$workoutId.tsx:~85`) +- `workoutLog?.userId ?? ''` triggers unnecessary profile query with empty ID. +- Fix: add `enabled: !!userId` guard in the hook. +- **Status:** ✅ Fixed (already guarded) +- **Resolution:** `useUserProfile` already has `enabled: !!userId && userId.length > 0` guard; no additional changes needed + +--- + +## Medium + +**M1** -- Edge functions use `Deno.env.get(...)!` non-null assertions for `SUPABASE_URL`/`SUPABASE_ANON_KEY`. Add explicit guards returning 500. +- **Status:** ✅ Fixed +- **Resolution:** Replaced all non-null assertions with explicit guards returning 500 "Server configuration error" in all three edge functions + +**M2** -- Webhook returns 200 when realtime broadcast fails post-DB-update. Video stays "Processing..." until manual refresh. Add error logging; consider Sentry. +- **Status:** ✅ Fixed (already handled) +- **Resolution:** Webhook already catches broadcast failures in try-catch with `console.error` and returns 200; no changes needed + +**M3** -- `req.json()` parse failure returns 500 instead of 400 in signed-url and upload-url edge functions. +- **Status:** ✅ Fixed +- **Resolution:** Wrapped `req.json()` in try-catch returning 400 "Invalid request body" in both functions + +**M4** -- All new modules use raw `console.error`/`console.warn` instead of project logging functions (`logError`, `logForDebugging`). Files: `display-publisher.ts`, `use-media-upload.ts`, `media-upload-service.ts`. +- **Status:** ✅ Dismissed +- **Resolution:** No project-wide `logError`/`logForDebugging` functions exist; raw console methods are the current project convention + +**M5** -- `useDisplayBroadcast` ignores error states from `useUserProfile` and `useExercises`. On query failure, `exerciseNameMap` silently defaults to `{}` producing "Unknown Exercise" for all exercises on the display. +- **Status:** ✅ Fixed +- **Resolution:** Added error destructuring from both queries with `console.warn` logging on error + +**M6** -- JSDoc on `configureDisplayPublisher` says "user's ID and visibility preference" but function only accepts `displayVisible`. (`src/lib/display-publisher.ts:53-54`) +- **Status:** ✅ Fixed +- **Resolution:** Updated JSDoc to "user's display visibility preference" + +--- + +## Test Coverage Gaps + +**T1 -- `media-provider.ts` has zero test coverage** (criticality 8/10) +- `CloudflareStreamProvider` contains signed URL caching with TTL expiry and Zod response validation -- exactly the code where a field name mismatch broke video in the previous PR. +- Tests needed: `isExpired` TTL logic, cache hit, cache miss on expiry, null client throws, Zod rejects malformed response. +- **Status:** ✅ Task created +- **Resolution:** Added as S024-T in Steps.md (007-video-image-sharing) + +**T2 -- Transcoding timeout behavior untested** (criticality 8/10) +- 5-minute timeout logic has no test. With fake timers: verify processing attachment marked `failed`, ready attachment unaffected, cache invalidated. +- **Status:** ✅ Task created +- **Resolution:** Added as S025-T in Steps.md (007-video-image-sharing) + +**Other gaps (lower priority):** +- `publishSessionEnded`/`publishFocusEvent` not tested with `displayVisible = false` +- `cancelUpload` test is a false positive -- tests no-op path, not active abort +- `chat-media-webhook` broadcast emission not verified in tests + +--- + +## Comment Issues + +- `src/lib/display-snapshot.ts:128` -- ✅ Fixed: Changed "last activity in the last group" to "last entry in the flattened activity list" +- `src/lib/display-snapshot.ts:108` -- ✅ Fixed: Removed "Pure function" claim +- `src/lib/media-upload-service.ts:9` -- ✅ Fixed: Removed `(S010)` step reference +- `src/domain/types/media.ts:27` -- ✅ Fixed: Replaced `(CH-6, TA-12)` with "the external provider" +- `src/lib/media-constraints.ts:4` -- ✅ Fixed: Removed `(CH-6)` reference +- `supabase/functions/chat-media-upload-url/index.ts:151-155` -- ✅ Fixed: Removed `stream-media-id` fallback that conflated UID with TUS URL +- `src/components/chat/media-message-content.tsx:26,59,79` -- ✅ Fixed: Removed `// --- Video ---` noise comments +- `src/components/chat/video-player.tsx:128,152` -- ✅ Fixed: Removed redundant JSX comments +- `src/components/chat/image-lightbox.tsx:26` -- ✅ Fixed: Removed redundant comment + +--- + +## Type Design Notes + +- `displayWeightSchema` is intentionally separate from `weightSchema` but has looser constraints -- ✅ tightened to match source invariants (see I7) +- `z.boolean().default(true)` on `user.displayVisible` would centralize the `?? true` default that's currently scattered across consumers +- `buildDisplaySnapshot()` constructs `DisplaySnapshot` directly without running output through `displaySnapshotSchema.parse()` -- consider dev-mode validation + +--- + +## Strengths + +- Display snapshot Zod schema has thorough test coverage (28 cases) including boundary values and discriminated union validation +- `RestTimerState` discriminated union is well-designed -- makes "running timer with no start time" structurally impossible +- `is_visible: z.literal(true)` cleverly encodes the business invariant +- Webhook signature verification uses constant-time comparison +- Edge function tests cover CORS, auth, validation, happy paths, and DB error paths (25 cases) +- JSDoc on `useDisplayBroadcast`, `verifyWebhookSignature`, and `SnapshotContext` is clear and accurate +- RLS infinite recursion migration is well-commented and uses correct `SECURITY DEFINER` approach + +--- + +## Actions + +- [x] Fix C1-C4 (merge blockers) +- [x] Fix I1-I9 (important issues) +- [x] Fix M1-M5 (medium hardening) +- [x] Add T1, T2 tests +- [x] Address comment issues +- [x] Run `bun run test` and `bun run build` before re-review + +--- + +## Resolution Summary +**Resolved at:** 2026-04-04 +**Session:** PR #59 review resolution + +| Category | Total | Fixed | Tasks Created | Dismissed | Deferred | +|---|---|---|---|---|---| +| Critical | 4 | 4 | -- | -- | -- | +| Important | 9 | 9 | -- | -- | -- | +| Medium | 6 | 4 | -- | 2 | -- | +| Test Gaps | 2 | -- | 2 | -- | -- | +| Comments | 9 | 9 | -- | -- | -- | +| **Total** | **30** | **26** | **2** | **2** | **0** | + +## Resolution Checklist + +- [x] All [FIX] findings resolved inline +- [x] All [TASK] findings added to Steps.md +- [x] All comment issues addressed +- [x] `bun run build` passes +- [x] `bun run test` passes (1361/1361) +- [x] Review verified diff --git a/Context/Reviews/0006-discovery-client-server-url-2026-04-04.md b/Context/Reviews/0006-discovery-client-server-url-2026-04-04.md new file mode 100644 index 00000000..69bd9049 --- /dev/null +++ b/Context/Reviews/0006-discovery-client-server-url-2026-04-04.md @@ -0,0 +1,181 @@ +# PR Review: discovery-client-server-url + +**Date:** 2026-04-04 +**Feature:** Context/Features/009-discovery-client-server-url/ +**Branch:** worktree-feat+discovery-client-server-url +**Reviewers:** code-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer, type-design-analyzer +**Status:** 🟢 Resolved + +## Summary + +18 findings across 5 review agents. 4 critical (logging, error handling, schema weakness), 6 important (a11y, race condition, error type mismatches, misleading docs), 8 suggestions (type extraction, test gaps, comment refinements). The discovery client logic is well-structured and thoroughly tested; the main concerns are around error observability, defensive error handling in the UI layer, and Zod schema leniency at the system boundary. + +## Findings + +### Fix-Now + +#### [FIX] P6-001: Zero logging in discovery.ts -- all 5 error paths are silent +- **File:** src/lib/discovery.ts (entire file) +- **Severity:** Critical +- **Detail:** The file has 5 distinct error return paths and zero console.error/console.warn calls. Every other error-handling file in the codebase follows the `console.error('[prefix]', err)` convention. Discovery failures will be invisible in the dev console, making debugging impossible when users report "discovery didn't work." +- **Status:** ✅ Fixed +- **Resolution:** Added `console.error('[discovery]', ...)` logging to all 6 error paths (URL parse, protocol check, fetch failure, HTTP error, JSON parse, schema validation). + +#### [FIX] P6-002: Bare `catch {}` blocks discard error objects entirely +- **File:** src/lib/discovery.ts:32, src/lib/discovery.ts:82 +- **Severity:** Critical +- **Detail:** Both the URL-parsing catch (line 32) and the JSON-parsing catch (line 82) use parameterless `catch {}`. The actual error object is thrown away. Non-obvious failures (body already consumed, memory pressure, browser extension interference) become untraceable. Capture the error parameter and log it. +- **Status:** ✅ Fixed +- **Resolution:** Changed both bare `catch {}` to `catch (err)` with `console.error` logging. + +#### [FIX] P6-003: Wrong error type `NETWORK_ERROR` for URL parse failure +- **File:** src/lib/discovery.ts:32-38 +- **Severity:** High +- **Detail:** When `new URL(normalized)` throws (invalid URL syntax), the code returns `error: 'NETWORK_ERROR'`. This is a validation error, not a network error. If a caller ever branches on error type to decide whether to retry, malformed URLs would be retried as if they were transient network problems. +- **Status:** ✅ Fixed +- **Resolution:** Added `INVALID_INPUT` to `DiscoveryError` union. URL parse and protocol check errors now return `INVALID_INPUT`. Updated tests accordingly. + +#### [FIX] P6-004: Wrong error type `NOT_FOUND` for JSON parse failure +- **File:** src/lib/discovery.ts:83-89 +- **Severity:** High +- **Detail:** When `response.json()` fails on a 200 response (e.g., HTML page from a generic web server), the error type is `NOT_FOUND`. But the endpoint WAS found (HTTP 200); the body is just not JSON. Should be `INVALID_RESPONSE` to match semantic intent and align with the schema validation failure on line 94. +- **Status:** ✅ Fixed +- **Resolution:** Changed JSON parse failure error type from `NOT_FOUND` to `INVALID_RESPONSE`. Updated test expectation. + +#### [FIX] P6-005: Misleading JSDoc -- "passed directly to createClient" +- **File:** src/lib/discovery.ts:19-20 +- **Severity:** Medium +- **Detail:** The JSDoc states credentials "can be passed directly to `createClient`" but the actual consumer pattern is `validateConnection` then `initSupabaseFromConfig`. A future maintainer reading this might bypass the config store and initialization flow, producing a disconnected client instance. Replace with "can be passed to `validateConnection` and then persisted via the config store." +- **Status:** ✅ Fixed +- **Resolution:** Updated JSDoc to reference `validateConnection` and config store instead of `createClient`. + +#### [FIX] P6-006: Missing `aria-expanded` on manual configuration toggle +- **File:** src/routes/setup.tsx:196-203 +- **Severity:** High +- **Detail:** The disclosure button that toggles the advanced/manual section lacks `aria-expanded={advancedOpen}` and `aria-controls`. Screen readers cannot communicate the expanded/collapsed state to users. The `.claude/rules/react-typescript.md` accessibility guideline requires interactive elements to be fully keyboard/screen-reader accessible. +- **Status:** ✅ Fixed +- **Resolution:** Added `aria-expanded={advancedOpen}` and `aria-controls="manual-config-section"` to toggle button, plus `id="manual-config-section"` on the controlled panel. + +#### [FIX] P6-007: No double-submission guard on handleDiscoverAndConnect +- **File:** src/routes/setup.tsx:84 +- **Severity:** Medium +- **Detail:** The Connect button disables during `discovering`/`validating` states (line 161), but `handleDiscoverAndConnect` itself has no early-return guard. Rapid clicks or keyboard events before React re-renders could trigger concurrent discovery+validation flows, causing a race condition on shared state. +- **Status:** ✅ Fixed +- **Resolution:** Added early-return guard `if (discoveryStatus === 'discovering' || status === 'validating') return` at the top of `handleDiscoverAndConnect`. + +#### [FIX] P6-008: Comment scope mismatch on validation feedback +- **File:** src/routes/setup.tsx:178 +- **Severity:** Low +- **Detail:** Comment says "(shown after discovery succeeds)" but the feedback block is shared by both the discovery and manual flows. Change to "(shared by both discovery and manual flows)." +- **Status:** ✅ Fixed +- **Resolution:** Updated comment to "(shared by both discovery and manual flows)". + +### Missing Tasks + +#### [TASK] P6-009: No top-level try-catch in handleDiscoverAndConnect +- **File:** src/routes/setup.tsx:84-109 +- **Severity:** Critical +- **Detail:** If `discoverInstance` or `validateAndSave` throws an unexpected runtime error (TypeError, programming bug), the promise rejection propagates unhandled. React does not catch async errors from event handlers. The UI freezes permanently in "discovering" state with no error feedback and no recovery path. Wrap the entire body in a try-catch that resets to an error state. +- **Relates to:** TA-7, TA-8 +- **Status:** ✅ Fixed inline +- **Resolution:** Implemented as part of SetupState refactor. `handleDiscoverAndConnect` now wrapped in try-catch that resets to `discovery-failed` phase. Task S005 no longer needed. + +#### [TASK] P6-010: No top-level try-catch in handleConnect +- **File:** src/routes/setup.tsx:74-82 +- **Severity:** High +- **Detail:** Same pattern as P6-009. If `validateAndSave` throws unexpectedly, the UI stays stuck in "validating" state. Wrap in try-catch with fallback error state. +- **Relates to:** TA-10 +- **Status:** ✅ Fixed inline +- **Resolution:** Implemented as part of SetupState refactor. `handleConnect` now wrapped in try-catch that resets to `validation-failed` phase. Task S006 no longer needed. + +#### [TASK] P6-011: validateAndSave sets success state before async operations complete +- **File:** src/routes/setup.tsx:51-66 +- **Severity:** High +- **Detail:** The try block wraps 4 operations (state update, config persistence, Supabase init, navigation) in one catch. `setStatus('ok')` fires at the top of the block. If `initSupabaseFromConfig` or `router.navigate` throws, the user sees a success flash followed by a misleading "Connected but failed to save" error. Move success state to after all async operations complete. +- **Relates to:** TA-11 +- **Status:** ✅ Fixed inline +- **Resolution:** Implemented as part of SetupState refactor. `setState({ phase: 'success' })` now fires after `setConfig` and `initSupabaseFromConfig` complete. Task S007 no longer needed. + +#### [TASK] P6-012: Tighten DiscoverySchema Zod validators +- **File:** src/lib/discovery.ts:9-13 +- **Severity:** Critical +- **Detail:** `DiscoverySchema` uses bare `z.string()` for all three fields. The downstream `backendConfigSchema` uses `z.string().url()` and `z.string().min(1)`. A discovery response with `supabase_url: ""` or `supabase_url: "not-a-url"` passes discovery validation, then fails later with confusing errors. Validation should be strictest at the system boundary. Use `z.string().url()` for `supabase_url`, `z.string().min(1)` for `supabase_publishable_key` and `version`. +- **Relates to:** TA-5 +- **Status:** ✅ Task created +- **Resolution:** Added as S005 in Steps.md + +#### [TASK] P6-013: Add test for missing supabase_publishable_key +- **File:** src/lib/__tests__/discovery.test.ts +- **Severity:** Medium +- **Detail:** Tests cover missing `version` and `supabase_url` but not missing `supabase_publishable_key` -- the most security-sensitive field. The spec (Steps.md S001-T, test case 10) explicitly requires it. A regression making this field optional would go undetected. +- **Relates to:** TA-5 +- **Status:** ✅ Task created +- **Resolution:** Added as S005-T in Steps.md + +#### [TASK] P6-014: Add test for non-404 HTTP errors (e.g., 500) +- **File:** src/lib/__tests__/discovery.test.ts +- **Severity:** Low +- **Detail:** Only 404 is tested. The code handles all non-2xx via `!response.ok`. A 500 test would verify the dynamic status-code message template (`Server returned ${response.status}...`) and prevent regressions if someone narrows the branch to only handle 404. +- **Relates to:** TA-4 +- **Status:** ✅ Task created +- **Resolution:** Added as S006-T in Steps.md + +#### [TASK] P6-015: Add test for URL with path component +- **File:** src/lib/__tests__/discovery.test.ts +- **Severity:** Low +- **Detail:** `forge.example.com/app` silently strips `/app` via `parsed.origin`. This is correct behavior per the spec, but it is a non-obvious transformation. A test documenting this behavior would prevent accidental regressions and serve as documentation. +- **Relates to:** TA-1, TA-2 +- **Status:** ✅ Task created +- **Resolution:** Added as S007-T in Steps.md + +### Architectural Concerns + +#### [ADR] P6-016: Two parallel state machines with implicit coordination in setup.tsx +- **File:** src/routes/setup.tsx:38-40 +- **Severity:** Medium +- **Detail:** `discoveryStatus` + `discoveryMessage` and `status` + `message` are four separate `useState` calls forming a de facto compound state machine. Impossible states are representable (e.g., `discoveryStatus === 'discovery-failed'` with `status === 'ok'`). As the page gains more features (QR scanning, deep links), this will become harder to reason about. Consider unifying into a single discriminated union `SetupState` type. Lower priority -- acceptable to defer to a future refactor if needed. +- **Relates to:** Tech.md, future Refactor B (QR code) +- **Status:** ✅ Fixed inline +- **Resolution:** Unified 4 state variables into a single `SetupState` discriminated union with 7 phases (idle, discovering, discovery-failed, validating, validation-failed, schema-missing, success). Impossible states eliminated. QR scanning (Refactor B) extends by adding `| { phase: 'scanning' } | { phase: 'scan-failed'; error: string }` to the union. + +### Convention Gaps + +#### [RULE] P6-017: Error type semantics -- NETWORK_ERROR used for non-network failures +- **Files:** src/lib/discovery.ts:36, src/lib/discovery.ts:44 +- **Severity:** Medium +- **Detail:** `NETWORK_ERROR` is used for URL validation failures (bad syntax, wrong protocol) which are input errors, not network errors. The `DiscoveryError` union should include `INVALID_INPUT` for input validation failures. This pattern could recur in future client modules that contact external services. Consider adding a convention note to `.claude/rules/` about error type semantics at system boundaries. +- **Suggested rule:** `.claude/rules/error-handling.md` -- "Error types at system boundaries must distinguish input validation failures from network/transport failures." +- **Status:** ✅ Rule updated +- **Resolution:** Added to `.claude/rules/error-handling.md` (new file). Code fix applied in P6-003. + +#### [RULE] P6-018: Bare catch blocks without error parameter +- **Files:** src/lib/discovery.ts:32, src/lib/discovery.ts:82 +- **Severity:** Medium +- **Detail:** Two catch blocks use `catch {}` with no parameter, discarding the error object entirely. The codebase convention elsewhere is `catch (err)` with `console.error`. This should be codified as a rule so linting or review catches it automatically. +- **Suggested rule:** `.claude/rules/error-handling.md` -- "Never use bare `catch {}`. Always capture the error parameter and log it with a bracketed module prefix." +- **Status:** ✅ Rule updated +- **Resolution:** Added to `.claude/rules/error-handling.md` (new file). Code fix applied in P6-002. + +## Resolution Summary +**Resolved at:** 2026-04-04 +**Session:** Review resolution for discovery-client-server-url + +| Category | Total | Fixed | Tasks Created | ADRs | Rules | Dismissed | Deferred | +|---|---|---|---|---|---|---|---| +| [FIX] | 8 | 8 | -- | -- | -- | -- | -- | +| [TASK] | 7 | 3 | 4 | -- | -- | -- | -- | +| [ADR] | 1 | 1 | -- | -- | -- | -- | -- | +| [RULE] | 2 | -- | -- | -- | 2 | -- | -- | +| **Total** | **18** | **12** | **4** | **0** | **2** | **0** | **0** | + +Notes: +- P6-009, P6-010, P6-011 (TASK) were resolved inline as part of the SetupState discriminated union refactor rather than deferred to new tasks +- P6-016 (ADR) was resolved inline per user decision -- no separate ADR created +- Remaining tasks (S005, S005-T, S006-T, S007-T) are for schema tightening and test coverage additions + +## Resolution Checklist +- [x] All [FIX] findings resolved (P6-001 through P6-008) +- [x] All [TASK] findings resolved or tracked (P6-009 through P6-015) +- [x] All [ADR] findings resolved (P6-016 -- fixed inline) +- [x] All [RULE] findings applied (P6-017, P6-018) +- [x] Review verified by review-verify agent diff --git a/Context/Reviews/0006-idle-mode-review.md b/Context/Reviews/0006-idle-mode-review.md new file mode 100644 index 00000000..37ff0f4e --- /dev/null +++ b/Context/Reviews/0006-idle-mode-review.md @@ -0,0 +1,165 @@ +# PR Review: feat/idle-mode -> develop + +**Date:** 2026-04-04 +**Feature:** Context/Features/009-idle-mode/ +**Branch:** worktree-feat+idle-mode +**Reviewers:** code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer +**Status:** :green_circle: Resolved + +## Summary + +16 findings across 20 changed files (~1700 lines). 3 critical (clock drift bug, silent null client on /display, lying connection status), 6 important (path matching, duplicate channel, missing CLOSED handling, production logging, duplicate type, unsafe cast), 4 task gaps (missing tests, schema edge cases), 3 convention/comment cleanup items. The Edge Function and SQL migration are solid; frontend error handling and observability are the main concerns. + +## Findings + +### Fix-Now + +#### [FIX] P6-001: Clock re-sync block computes increasingly wrong offset from stale prop +- **File:** src/components/display/clock-display.tsx:29-37 +- **Severity:** Critical +- **Detail:** The 60-tick re-sync inside `setInterval` re-parses the same stale `serverTimeCorrection` string against an advancing `Date.now()`, making `offsetRef.current` drift progressively negative. The first `useEffect` (lines 14-20) already correctly resets the offset when the prop changes. The re-sync block is redundant and introduces drift. The accompanying comment ("NTP corrections and DST transitions") is misleading. +- **Relates to:** A-001 (clock accuracy), A-013 (long-running stability), S-3 (drift correction) +- **Status:** :white_check_mark: Fixed +- **Resolution:** Removed the 60-tick re-sync block and misleading NTP/DST comment from clock-display.tsx + +#### [FIX] P6-002: `/display` route silently non-functional when Supabase client not initialized +- **File:** src/components/display/use-idle-snapshot.ts:19-20 +- **Severity:** Critical +- **Detail:** The `/display` route bypasses the auth/config guard in `__root.tsx`, but `getSupabaseClient()` depends on that initialization. On a fresh visit to `/display` (no prior app visit), the client is always `null`. The hook silently returns without logging. Combined with P6-003, the display shows a clock, says "Connected", and never receives data -- with zero signal anything is wrong. +- **Relates to:** A-004 (sessions render on idle_state), A-009 (Edge Function publishes every 60s) +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added console.error when Supabase client is null in useIdleSnapshot + +#### [FIX] P6-003: Connection status defaults to "Connected" when no connection exists +- **File:** src/routes/display.tsx:30-39 +- **Severity:** Critical +- **Detail:** Initial state is `'connected'`, but if the Supabase client is null the subscription effect returns early without updating state. The footer permanently displays "Connected" with no actual Realtime connection. This actively misleads the operator. +- **Relates to:** A-013 (hours of stable operation) +- **Status:** :white_check_mark: Fixed +- **Resolution:** Default connectionStatus changed to 'reconnecting'; status now driven by useIdleSnapshot callback + +#### [FIX] P6-004: Path bypass in `__root.tsx` too broad -- matches `/display*` +- **File:** src/routes/__root.tsx:11 +- **Severity:** High +- **Detail:** `startsWith('/display')` matches `/display-anything`, `/displayfoo`, etc. The existing `/s/` bypass uses a trailing slash for precision. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Changed to exact match `location.pathname === '/display'` in __root.tsx + +#### [FIX] P6-005: Duplicate `display-status` channel wastes a subscription +- **File:** src/routes/display.tsx:42-54 +- **Severity:** High +- **Detail:** A separate `display-status` channel is created solely for connection monitoring. `useIdleSnapshot` already subscribes to the real `display` channel. Two separate channels; the status one never receives broadcasts, so its status only reflects WebSocket health of a purposeless channel. +- **Relates to:** A-013 (no resource leaks) +- **Status:** :white_check_mark: Fixed +- **Resolution:** Removed duplicate display-status channel; useIdleSnapshot now accepts onConnectionStatus callback and reports channel status from the real display channel + +#### [FIX] P6-006: `CLOSED` channel status not handled +- **File:** src/routes/display.tsx:42-48 +- **Severity:** Medium +- **Detail:** Subscription callback only handles `SUBSCRIBED`, `CHANNEL_ERROR`, `TIMED_OUT`. Supabase also emits `CLOSED` (network drops, server restarts). Over hours of operation, missed `CLOSED` events leave the status indicator stuck. +- **Relates to:** A-013 (hours of stable operation) +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added CLOSED to the reconnecting branch in useIdleSnapshot subscribe callback + +#### [FIX] P6-007: Invalid payloads silently discarded in production +- **File:** src/components/display/use-idle-snapshot.ts:25-29 +- **Severity:** High +- **Detail:** `safeParse` failures only log in dev mode (`import.meta.env.DEV`). In production, schema drift between Edge Function and frontend produces zero signal -- the display just shows a bare clock forever. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Changed from DEV-only console.warn to unconditional console.error for validation failures + +#### [FIX] P6-008: Duplicate `DisplayMode` type definition +- **File:** src/components/display/use-display-mode.ts:4, src/components/display/display-mode-transition.tsx:3 +- **Severity:** Medium +- **Detail:** `type DisplayMode = 'idle' | 'board' | 'focused'` defined independently in two files. Adding a mode to one but not the other produces a silent mismatch. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Created src/components/display/types.ts with shared DisplayMode type; both files import from there + +#### [FIX] P6-009: Unsafe type cast of RPC result in Edge Function +- **File:** supabase/functions/display-idle-snapshot/index.ts:65 +- **Severity:** Medium +- **Detail:** `as ScheduledSession[]` cast with no shape validation. Column renames in the SQL function would produce `undefined` fields that the frontend Zod schema rejects silently (see P6-007). +- **Status:** :white_check_mark: Fixed +- **Resolution:** Replaced unsafe `as` cast with runtime .filter() that validates all four string fields before including a row + +#### [FIX] P6-010: Remove redundant "what" comments +- **File:** src/routes/display.tsx:25,28,30-31,37; src/components/display/idle-view.tsx:23,29,53,64 +- **Severity:** Low +- **Detail:** ~8 comments that restate what self-documenting code already says (e.g., `// Idle snapshot from Edge Function broadcast` above `const idleSnapshot = useIdleSnapshot()`). Remove or replace with "why" comments where the architectural choice is non-obvious (e.g., why a separate display-status channel exists). +- **Status:** :white_check_mark: Fixed +- **Resolution:** Removed 8 redundant "what" comments from display.tsx and idle-view.tsx + +### Missing Tasks + +#### [TASK] P6-011: Add tests for `useIdleSnapshot` hook +- **File:** src/components/display/use-idle-snapshot.ts +- **Severity:** High +- **Detail:** Zero test coverage for the hook with the most real async/channel logic. Needs tests for: valid payload updates state, invalid payload discarded (state stays null), null client returns null without throwing, `removeChannel` called on unmount, subscription status handling. +- **Relates to:** A-004, A-013 (memory leaks / cleanup) +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S011 in Steps.md Phase 5 + +#### [TASK] P6-012: Add tests for `DisplayModeTransition` +- **File:** src/components/display/display-mode-transition.tsx +- **Severity:** Medium +- **Detail:** `getTransitionClasses` contains branching logic mapping mode transitions to CSS classes (300ms vs 400ms, zoom-in-95). Pure function that directly implements A-007 and A-017. No tests. +- **Relates to:** A-007 (300ms idle-to-board), A-017 (400ms board-to-focused) +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S012 in Steps.md Phase 5 + +#### [TASK] P6-013: Add schema edge case tests +- **File:** src/domain/types/__tests__/display-snapshot.test.ts +- **Severity:** Low +- **Detail:** Missing tests for: invalid session_type value (e.g., "YOGA"), empty scheduled_sessions array, invalid ISO date string for server_time. These catch Zod schema regressions at the parse boundary. +- **Relates to:** A-010 (payload structure) +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S013 in Steps.md Phase 5 + +#### [TASK] P6-014: Strengthen ClockDisplay server correction test assertion +- **File:** src/components/display/__tests__/clock-display.test.tsx:62-76 +- **Severity:** Low +- **Detail:** Test only asserts time is "different" after correction (`not.toBe(initialTime)`), not that it matches the expected corrected value. Would pass even with wrong-sign correction. +- **Relates to:** A-001 (clock accuracy), S-3 (drift correction) +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S014 in Steps.md Phase 5 + +### Architectural Concerns + +_None identified. The architecture follows established patterns and the feature is well-scoped._ + +### Convention Gaps + +#### [RULE] P6-015: String fields in display schemas should have `.min(1)` +- **Files:** src/domain/types/display-snapshot.ts +- **Severity:** Medium +- **Detail:** `display_name`, `session_name`, `day_label` accept empty strings via `z.string()`. For display-facing types rendered as visible text on a TV, empty values produce blank cards. Adding `.min(1)` catches this at parse time. +- **Suggested rule:** Add to `.claude/rules/` domain types conventions: "Display-facing string fields must use `.min(1)` to prevent blank UI elements" +- **Status:** :white_check_mark: Rule applied +- **Resolution:** Added .min(1) to all display-facing string fields in idleSnapshotSchema (display_name, session_name, day_label in both scheduled_sessions and next_session) + +#### [RULE] P6-016: Tech.md D4 documents wrong option as chosen +- **Files:** Context/Features/009-idle-mode/Tech.md (D4 section) +- **Severity:** Low +- **Detail:** Tech.md D4 documents "Option (a) Raw SQL JOIN traversal in Edge Function" as the chosen approach, but implementation uses `supabase.rpc('get_display_idle_sessions')` -- an RPC function (Option c). Documentation is stale. +- **Suggested rule:** N/A -- one-off documentation fix +- **Status:** :white_check_mark: Fixed +- **Resolution:** Updated Tech.md D4 to document Option (c) RPC as the chosen approach + +## Resolution Checklist +- [x] All [FIX] findings resolved (10 items) +- [x] All [TASK] findings added to Steps.md (4 items) +- [x] All [ADR] findings have ADRs created or dismissed (0 items) +- [x] All [RULE] findings applied or dismissed (2 items) +- [x] Review verified by review-verify agent + +## Resolution Summary +**Resolved at:** 2026-04-04 +**Session:** Resolve idle mode PR review findings + +| Category | Total | Fixed | Tasks Created | ADRs | Rules | Dismissed | Deferred | +|---|---|---|---|---|---|---|---| +| [FIX] | 10 | 10 | -- | -- | -- | -- | -- | +| [TASK] | 4 | -- | 4 | -- | -- | -- | -- | +| [ADR] | 0 | -- | -- | 0 | -- | -- | -- | +| [RULE] | 2 | 1 | -- | -- | 1 | -- | -- | +| **Total** | **16** | **10** | **4** | **0** | **1** | **0** | **0** | diff --git a/Context/Reviews/0006-pr60-display-route-board-review.md b/Context/Reviews/0006-pr60-display-route-board-review.md new file mode 100644 index 00000000..2a3a0c01 --- /dev/null +++ b/Context/Reviews/0006-pr60-display-route-board-review.md @@ -0,0 +1,205 @@ +# PR Review: worktree-feat+display-route-board -> develop + +**Date:** 2026-04-04 +**Feature:** Context/Features/009-display-route-board/ +**Branch:** worktree-feat+display-route-board +**PR:** #60 +**Reviewers:** code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer +**Status:** :green_circle: Resolved + +## Summary + +18 findings across 26 files (2860 additions). 3 critical issues center on the reconnection +flow: `publishHello()` is dead code, channel objects leak on reconnect, and `boot()` has an +unhandled promise rejection. 7 important issues cover store robustness, untested reconnection +logic, and hidden-view timer waste. 8 suggestions for schema hardening, type deduplication, +and documentation accuracy. + +**Breakdown:** 8 FIX, 4 TASK, 1 ADR, 1 RULE (plus 4 suggestions deferred to backlog) + +## Findings + +### Fix-Now + +#### [FIX] P6-001: `publishHello()` never called after reconnection +- **File:** src/lib/display-subscriber.ts:100-118 +- **Severity:** Critical +- **Detail:** Tech.md TD-5 requires publishing `display_hello` after reconnecting so phones + re-broadcast their snapshots. `publishHello()` is implemented and exported but never wired + into the reconnection flow. After reconnect, sessions only reappear when phones independently + trigger new state changes. Violates requirement M8 and testable assertion TA-10. +- **Relates to:** Spec.md M8, Tech.md TD-5, TA-10 +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `_hasConnectedBefore` flag; `publishHello()` called on SUBSCRIBED when reconnecting (not first connect) + +#### [FIX] P6-002: Channel leak on reconnection -- dead channel not removed from Supabase client +- **File:** src/lib/display-subscriber.ts:109 +- **Severity:** Critical +- **Detail:** On terminal status (`TIMED_OUT`, `CHANNEL_ERROR`, `CLOSED`), `_channel` is set + to `null` without calling `_client.removeChannel(_channel)` first. The guard in + `subscribeToDisplay` checks `_channel` which is already null, so the old channel is never + cleaned up. Orphaned channel objects accumulate across reconnect cycles -- problematic for a + long-running kiosk that may run for days. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Call `_client.removeChannel(_channel)` before nulling `_channel` in terminal status handler + +#### [FIX] P6-003: Unhandled promise rejection in `boot()` causes blank screen +- **File:** src/routes/display.tsx:59-94 +- **Severity:** Critical +- **Detail:** The `boot()` async function has no try-catch. If `resolveConfig()` throws + (localStorage blocked in private browsing, network error in `validateConnection()`), or + `createClient()` throws (malformed URL), the promise rejects silently. The display shows a + blank page with zero feedback. A gym owner sees a black screen with no diagnostic info. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Wrapped `boot()` body in try-catch; sets connectionStatus to 'disconnected' on failure + +#### [FIX] P6-004: `clearAllSessions` does not reset `currentPage` +- **File:** src/stores/display-store.ts (clearAllSessions action) +- **Severity:** High +- **Detail:** `clearAllSessions` resets `sessions`, `lastSeenAt`, and `focusedUserId` but not + `currentPage`. If the display is on page 2 when all sessions clear, and new sessions arrive, + `getPageSessions` returns an empty array because `currentPage` is still 2 but there is only + 1 page of content. Results in a blank board with active sessions. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `currentPage: 0` to `clearAllSessions` reset + +#### [FIX] P6-005: `setCurrentPage` has no bounds clamping +- **File:** src/stores/display-store.ts (setCurrentPage action) +- **Severity:** Medium +- **Detail:** Accepts any number without validation. Setting `currentPage` to -1 or 999 + produces an empty `getPageSessions` result. The auto-paging interval in board-view.tsx + could push the page beyond bounds if sessions are removed between ticks. +- **Status:** :white_check_mark: Fixed +- **Resolution:** `setCurrentPage` now clamps to `[0, totalPages-1]`; updated tests to verify bounds + +#### [FIX] P6-006: `subscribeToDisplay` silently returns when client not initialized +- **File:** src/lib/display-subscriber.ts:53-57 +- **Severity:** Medium +- **Detail:** When `_client` is null, the function logs `console.warn` and returns without + notifying the caller via `onStatusChange`. The connection footer stays stuck on + "Disconnected" with no reconnection attempts and no actionable feedback. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `handlers.onStatusChange('disconnected')` call before early return + +#### [FIX] P6-007: `useElapsedTime` has no NaN guard for invalid timestamps +- **File:** src/hooks/use-elapsed-time.ts:11-12 +- **Severity:** Medium +- **Detail:** `new Date(startedAt).getTime()` produces `NaN` if `startedAt` is an invalid + string. While Zod validates upstream, this hook is exported publicly. `Math.floor(NaN / 1000)` + produces `NaN`, rendering "NaN:NaN:NaN" on the gym TV. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `Number.isNaN(startMs)` guard returning 0; also clamped elapsed to `Math.max(0, ...)` + +#### [FIX] P6-008: Stale comment in `__root.tsx` bypass list +- **File:** src/routes/__root.tsx:7 +- **Severity:** Low +- **Detail:** Comment says "Allow /setup and public share routes to load without a backend + configuration" but `/display` was just added to the bypass list. Comment no longer reflects + the full set of bypassed routes. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Updated comment to include `/display` in the description + +### Missing Tasks + +#### [TASK] P6-009: Reconnection retry logic is entirely untested +- **File:** src/lib/display-subscriber.ts:105-118 +- **Severity:** Critical +- **Detail:** The exponential backoff retry mechanism (delay computation, cap at 30s, retry + attempt reset on success, cancel on destroy) has zero test coverage. This is the most complex + untested code path in the PR and is architecturally central to the display's "set it and + forget it" reliability requirement. +- **Relates to:** Spec.md M8, Tech.md TD-5 +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S008 in Steps.md (Wave 4) + +#### [TASK] P6-010: `use-elapsed-time.ts` has zero tests +- **File:** src/hooks/use-elapsed-time.ts +- **Severity:** Medium +- **Detail:** 19-line hook used by both `SessionCard` and `FocusedViewContent` with date + arithmetic that could regress. Trivial to test with `renderHook` following the same pattern + as `use-timer-interpolation.test.ts`. Test cases: returns 0 at start, increases over time, + resets when `startedAt` changes, handles future timestamps. +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S009 in Steps.md (Wave 4) + +#### [TASK] P6-011: Add `.min(1)` to string fields in display snapshot schema +- **File:** src/domain/types/display-snapshot.ts +- **Severity:** Medium +- **Detail:** `user_id`, `display_name`, `session_name`, and `current_exercise` accept empty + strings. An empty `user_id` silently creates a session keyed to `""` in the store. Empty + `display_name` / `session_name` produce blank cards on the TV. +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S010 in Steps.md (Wave 4) + +#### [TASK] P6-012: Invalid payloads for `session_ended` and `focus` events not tested +- **File:** src/lib/__tests__/display-subscriber.test.ts +- **Severity:** Low +- **Detail:** Invalid `workout_snapshot` payload test exists, but `session_ended` and `focus` + events have no invalid payload tests. These go through `userIdPayloadSchema.safeParse()` and + should be dropped with a warning. A parameterized test for each would provide parity. +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S011 in Steps.md (Wave 4) + +### Architectural Concerns + +#### [ADR] P6-013: Dual-Map synchronization hazard in display store +- **File:** src/stores/display-store.ts +- **Severity:** High +- **Detail:** `sessions` and `lastSeenAt` are parallel Maps that must always have matching key + sets. Every mutation must update both. A single missed spot creates a memory leak (orphaned + `lastSeenAt` entries) or a crash (missing `lastSeenAt` for a session). Additionally, Zustand's + `setState()` can bypass actions entirely. Merging into a single + `Map` eliminates the invariant + structurally. +- **Relates to:** Tech.md display store design +- **Status:** :white_check_mark: Fixed +- **Resolution:** Merged dual maps into single `Map`. No ADR needed -- straightforward refactor that eliminates the invariant structurally. Added `getSnapshot` and `getSessionCount` selectors for consumers. + +### Convention Gaps + +#### [RULE] P6-014: `ConnectionStatus` type defined in 3 separate places +- **Files:** src/stores/display-store.ts, src/lib/display-subscriber.ts (inline literals in + DisplayEventHandlers.onStatusChange), subscriber status getter +- **Severity:** Medium +- **Detail:** The same `'connected' | 'disconnected' | 'reconnecting'` union is defined + independently in three locations. These can drift without compile-time errors. Should be + extracted to a single shared definition imported by both the store and subscriber. +- **Suggested rule:** Add to `.claude/rules/` or domain conventions: shared infrastructure + types (connection status, event types) should be defined once and imported. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Extracted `DisplayConnectionStatus` type to `src/domain/types/display-snapshot.ts`; store and subscriber both import from the single source + +### Deferred to Backlog (Suggestions) + +The following are valid observations but lower priority. They can be captured as backlog +items rather than blocking merge: + +- **S1:** Idle view components remain mounted with CSS opacity -- timers tick when hidden + (src/routes/display.tsx:128-159). Consider conditional rendering or `visible` prop. +- **S2:** Reconnection retry has no max attempt limit -- infinite 30s retries with no "gave up" + state (src/lib/display-subscriber.ts:111-118). +- **S3:** `publishHello` JSDoc says "presence" which is misleading given Supabase Presence API + (src/lib/display-subscriber.ts:122). +- **S4:** `DisplayEventType` enum declared but never consumed programmatically + (src/domain/types/display-snapshot.ts). Either consume it or remove it. + +## Resolution Summary +**Resolved at:** 2026-04-04 +**Session:** Review resolution for PR #60 + +| Category | Total | Fixed | Tasks Created | ADRs | Rules | Dismissed | Deferred | +|---|---|---|---|---|---|---|---| +| [FIX] | 8 | 8 | -- | -- | -- | -- | -- | +| [TASK] | 4 | -- | 4 | -- | -- | -- | -- | +| [ADR] | 1 | 1 | -- | -- | -- | -- | -- | +| [RULE] | 1 | 1 | -- | -- | 1 | -- | -- | +| **Total** | **14** | **10** | **4** | **0** | **1** | **0** | **0** | + +Note: P6-013 (ADR) was resolved as a direct refactor (merged dual maps) rather than creating a formal ADR -- the fix was small enough to just do it. + +## Resolution Checklist +- [x] All [FIX] findings resolved (P6-001 through P6-008) +- [x] All [TASK] findings added to Steps.md (P6-009 through P6-012) +- [x] All [ADR] findings have ADRs created or dismissed (P6-013) +- [x] All [RULE] findings applied or dismissed (P6-014) +- [x] Review verified by review-verify agent diff --git a/Context/Reviews/0007-pr64-qr-code-gen-scan-review.md b/Context/Reviews/0007-pr64-qr-code-gen-scan-review.md new file mode 100644 index 00000000..c8ae89d0 --- /dev/null +++ b/Context/Reviews/0007-pr64-qr-code-gen-scan-review.md @@ -0,0 +1,162 @@ +# PR Review: worktree-feat+qr-code-gen-scan -> develop + +**Date:** 2026-04-04 +**Feature:** Context/Features/010-qr-code-gen-scan/ +**Branch:** worktree-feat+qr-code-gen-scan +**PR:** #64 +**Reviewers:** code-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer +**Status:** :green_circle: Resolved + +## Summary + +14 findings across 4 review agents. 2 critical (bare catch convention violation, missing try/catch leaving UI stuck), 4 important (unhandled promise, missing user feedback, constant drift risk, accessibility gap), 4 suggestions, and 4 test coverage gaps. New invite-link utility is well-tested (13 unit tests) but UI components have zero component-level tests for the new functionality. + +## Findings + +### Fix-Now + +#### [FIX] P7-001: Bare `catch {}` in handleCopy violates error-handling convention + +- **File:** src/components/profile/backend-settings.tsx:57-59 +- **Severity:** Critical +- **Detail:** The existing `handleCopy` function uses a bare `catch {}` with comment "Copy is non-critical, swallow silently." This directly violates `.claude/rules/error-handling.md`. Worse, `setCopied(true)` runs unconditionally after the catch, so the user sees "Copied" feedback even when clipboard write fails. Fix: capture error, log with `[backend-settings]` prefix, only set copied on success, show error toast on failure. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Captured error, added `[backend-settings]` log prefix, moved `setCopied(true)` inside try block, added error toast + +#### [FIX] P7-002: `processInviteLink` missing try/catch around `validateAndSave` + +- **File:** src/routes/setup.tsx:118-129 +- **Severity:** Critical +- **Detail:** Unlike sibling handlers `handleConnect` and `handleDiscoverAndConnect` which wrap `validateAndSave` in try/catch with state updates, `processInviteLink` does not. If validation throws unexpectedly, the UI gets stuck showing "Connecting..." with no recovery path. Also causes the scan path's outer catch to conflate scan failures with validation failures, showing "QR scan failed" when the scan actually succeeded. Fix: add try/catch matching the pattern in `handleConnect`, setting `validation-failed` state on error. +- **Relates to:** TA-7 +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added try/catch around `validateAndSave` matching `handleConnect` pattern, sets `validation-failed` state on error + +#### [FIX] P7-003: Unhandled promise from `handleScan()` in button onClick + +- **File:** src/routes/setup.tsx:241 +- **Severity:** High +- **Detail:** The QR button's `onClick` calls `handleScan()` (async) without `.catch()`. If the dynamic import fails before the internal try block, the rejection is unhandled. The paste field handlers correctly use `.catch()` but the Tauri branch does not. Fix: add `.catch()` matching the pattern used for `processInviteLink` calls. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `.catch()` on `handleScan()` in onClick handler with error log and user-facing toast + +#### [FIX] P7-004: "Copy invite link" gives no user feedback on clipboard failure + +- **File:** src/components/profile/backend-settings.tsx:286-294 +- **Severity:** High +- **Detail:** The catch block logs the error with correct module prefix but shows no toast. User clicks button, nothing visible happens on failure. Fix: add `toast('Failed to copy invite link')` in the catch block. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `toast('Failed to copy invite link')` in catch block + +#### [FIX] P7-005: `buildInviteLink` hardcodes scheme/host instead of using constants + +- **File:** src/lib/invite-link.ts:7 +- **Severity:** High +- **Detail:** `SCHEME` and `HOST` constants are declared at lines 1-2 and used in `parseInviteLink`, but `buildInviteLink` hardcodes `ardentforge://connect` in the template literal. If the scheme or host changes, the two functions silently diverge. Fix: use `${SCHEME}//${HOST}` in the template. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Replaced hardcoded string with `${SCHEME}//${HOST}` template + +#### [FIX] P7-006: Paste field missing `aria-label` + +- **File:** src/routes/setup.tsx:253 +- **Severity:** High +- **Detail:** The `ForgeInput` for the paste field has a `placeholder` but no `aria-label`, violating accessibility requirements. Screen readers cannot identify the input's purpose. Fix: add `aria-label="Paste invite link"`. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `aria-label="Paste invite link"` to ForgeInput + +#### [FIX] P7-007: `parseInviteLink` logs `console.error` for expected invalid input + +- **File:** src/lib/invite-link.ts:24-26 +- **Severity:** Low +- **Detail:** Returning `null` for invalid input is by design, but `console.error` treats every parse failure as an error worth logging. Users pasting garbage text will flood the console. Fix: downgrade to `console.debug` or remove the log entirely since `null` return is the expected signal. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Removed console.error from catch block; `null` return is sufficient signal + +#### [FIX] P7-008: `.catch()` on processInviteLink in paste/keydown logs but gives no user feedback + +- **File:** src/routes/setup.tsx:259-261, 268-270 +- **Severity:** Medium +- **Detail:** The `.catch()` handlers on `processInviteLink` promise in the onKeyDown and onPaste handlers log the error but show no toast. If an unexpected error occurs, the user sees nothing. Fix: add `toast('Something went wrong. Please try again.')` in the catch blocks. Note: fixing P7-002 (adding try/catch inside `processInviteLink`) reduces the likelihood of these catches firing, but they should still inform the user as a safety net. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added `toast('Something went wrong. Please try again.')` in both .catch() handlers + +#### [FIX] P7-009: Generic "QR scan failed" toast for all failure types + +- **File:** src/routes/setup.tsx:154-158 +- **Severity:** Low +- **Detail:** The catch block shows "QR scan failed" for dynamic import failures, permission issues, hardware failures, and validation errors alike. At minimum, append "Try pasting the invite link instead." to give the user an actionable alternative. Mostly resolved by P7-002 (which stops validation errors from reaching this catch). +- **Status:** :white_check_mark: Fixed +- **Resolution:** Updated toast to "QR scan failed. Try pasting the invite link instead." in both catch locations + +#### [FIX] P7-010: Comment on `buildInviteLink` says "encodes the key" (slightly misleading) + +- **File:** src/lib/invite-link.ts:4-5 +- **Severity:** Low +- **Detail:** Comment says the function "Encodes the Supabase publishable (anon) key" but it actually builds an entire invite link URL containing both URL and key. A reader might interpret "encodes" as a transformation of the key itself. Minor rewording suggested: "The publishable (anon) key is not a secret -- safe to embed in QR codes and invite links since it is already bundled into every client build." +- **Status:** :white_check_mark: Fixed +- **Resolution:** Reworded comment to clarify the key is not a secret and safe to embed + +### Missing Tasks + +#### [TASK] P7-011: Component tests for processInviteLink valid/invalid flows + +- **File:** src/routes/setup.tsx +- **Severity:** High +- **Detail:** `processInviteLink` is the central integration point for the paste/scan flow. No component-level tests verify that: (1) a valid invite link populates url/key fields, expands Advanced, triggers validation; (2) an invalid link shows toast and leaves fields unchanged. A regression here makes the entire QR feature non-functional. +- **Relates to:** TA-6, TA-7, S006 +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S006-T1 in Steps.md + +#### [TASK] P7-012: Component tests for "Share this server" conditional rendering and copy button + +- **File:** src/components/profile/backend-settings.tsx +- **Severity:** Medium +- **Detail:** No tests verify: (1) QR section absent when config is null and present when configured (TA-3); (2) copy button writes correct invite link to clipboard and shows toast (TA-2). These are spec requirements M2 and M3. +- **Relates to:** TA-2, TA-3, S006 +- **Status:** :white_check_mark: Task created +- **Resolution:** Added as S006-T2 in Steps.md + +#### [TASK] P7-013: Unit test for empty-string params in parseInviteLink + +- **File:** src/lib/**tests**/invite-link.test.ts +- **Severity:** Low +- **Detail:** Tests cover missing params but not explicitly empty-valued params like `ardentforge://connect?url=&key=abc`. The `!url || !key` guard should catch it, but an explicit test documents the behavior and prevents regression if the guard changes. +- **Relates to:** TA-5 +- **Status:** :white_check_mark: Fixed +- **Resolution:** Added two tests for empty-valued url and key params in invite-link.test.ts + +### Architectural Concerns + +_None identified._ + +### Convention Gaps + +#### [RULE] P7-014: Pre-existing bare `catch {}` in connection-validator.ts + +- **Files:** src/lib/connection-validator.ts:25-26 +- **Severity:** Medium +- **Detail:** The `createClient` call is wrapped in a bare `catch {}` without capturing or logging the error. This predates PR #64 but is now called by new code paths (processInviteLink -> validateAndSave -> validateConnection). Same violation pattern as P7-001. Suggests the error-handling convention should be enforced more broadly, possibly via a lint rule or pre-commit hook. +- **Suggested rule:** Consider adding an ESLint rule (`no-empty` or custom) to flag bare catch blocks, or add a note in `.claude/rules/error-handling.md` to audit existing code for violations. +- **Status:** :white_check_mark: Fixed +- **Resolution:** Fixed bare catch in connection-validator.ts to capture error and log with `[connection-validator]` prefix + +## Resolution Summary + +**Resolved at:** 2026-04-04 +**Session:** Resolve PR review findings P7 + +| Category | Total | Fixed | Tasks | ADRs | Rules | Deferred | Discarded | +| --------- | ------ | ------ | ----- | ----- | ----- | -------- | --------- | +| [FIX] | 10 | 10 | -- | -- | -- | -- | -- | +| [TASK] | 3 | 1 | 2 | -- | -- | -- | -- | +| [ADR] | 0 | -- | -- | -- | -- | -- | -- | +| [RULE] | 1 | 1 | -- | -- | -- | -- | -- | +| **Total** | **14** | **12** | **2** | **0** | **0** | **0** | **0** | + +## Resolution Checklist + +- [x] All [FIX] findings resolved (10 items) +- [x] All [TASK] findings added to Steps.md or tracked (3 items) +- [x] All [ADR] findings have ADRs created or dismissed (0 items) +- [x] All [RULE] findings applied or dismissed (1 item) +- [x] Review verified by review-verify agent diff --git a/Context/Reviews/0007-pr65-progressive-disclosure-review.md b/Context/Reviews/0007-pr65-progressive-disclosure-review.md new file mode 100644 index 00000000..3a150f49 --- /dev/null +++ b/Context/Reviews/0007-pr65-progressive-disclosure-review.md @@ -0,0 +1,168 @@ +# PR Review: worktree-feat+progressive-disclosure -> develop + +**Date:** 2026-04-04 +**Feature:** Context/Features/010-Progressive-Disclosure/ +**Branch:** worktree-feat+progressive-disclosure +**PR:** #65 +**Reviewers:** code-reviewer, silent-failure-hunter, type-design-analyzer, comment-analyzer, code-simplifier +**Build:** TypeScript PASS, ESLint PASS (no new issues) +**Status:** 🟡 Partially resolved + +## Summary + +22 files changed across progressive disclosure and contextual help feature. Architecture is sound with data-driven visibility maps and centralized help content. 15 findings total: 7 fix-now, 4 missing tasks, 1 architectural concern, 3 convention gaps. Two critical items: silent save failure and JSX triplication. + +## Findings + +### Fix-Now + +#### [FIX] P7-001: `handleSave` silently no-ops when userId is empty + +- **File:** src/components/session-builder/session-template-form.tsx:192 +- **Severity:** Critical +- **Detail:** `if (!userId) return` exits without any user feedback. If auth state is lost or not yet loaded, user taps Save and nothing happens -- no error, no spinner, no indication. Should surface an error message via `setErrors()` and log with `[session-template-form]` prefix. +- **Status:** ✅ Fixed +- **Resolution:** Added console.error log with [session-template-form] prefix and setErrors() call for user-facing feedback + +#### [FIX] P7-002: Scoring/TimeCap JSX tripled across ternary branches + +- **File:** src/components/session-builder/session-template-form.tsx:312-406 +- **Severity:** Critical +- **Detail:** The Scoring ` handleNameChange(e.target.value)} - onBlur={() => setIsEditingName(false)} - onKeyDown={(e) => { - if (e.key === 'Enter') setIsEditingName(false) - }} - autoFocus - className="flex-1 border-0 border-b border-warm-ash/30 bg-transparent py-1 font-display text-sm font-medium text-bone-white focus:border-ember focus:outline-none" - aria-label="Block name" - /> - ) : ( - - )} - - - {block.blockType} - - - - {block.weeks.length} {block.weeks.length === 1 ? 'WEEK' : 'WEEKS'} - - - - - - - - {/* Expanded content */} - {expanded && ( -
-
- {BLOCK_TYPES.map((bt) => ( + + + {isEditingName ? ( + handleNameChange(e.target.value)} + onBlur={() => setIsEditingName(false)} + onKeyDown={(e) => { + if (e.key === 'Enter') setIsEditingName(false) + }} + autoFocus + className={`flex-1 border-0 border-b bg-transparent py-1 font-display text-sm font-medium text-bone-white focus:outline-none ${ + nameError + ? 'border-warning-flare focus:border-warning-flare' + : 'border-warm-ash/30 focus:border-ember' + }`} + aria-label="Block name" + aria-invalid={!!nameError} + /> + ) : ( - ))} -
+ )} + + + {block.blockType} + - {block.weeks.map((week, weekIndex) => ( - + {block.weeks.length} {block.weeks.length === 1 ? 'WEEK' : 'WEEKS'} + + + + + + + + setShowDeleteConfirm(true)}> + + Delete block + + + + + - ))} +
- + {/* Inline validation errors */} + {errors.length > 0 && ( +
+ {nameError &&

{nameError}

} + {weeksError &&

{weeksError}

} +
+ )} + + {/* Expanded content */} + +
+ + + {block.weeks.map((week, weekIndex) => { + const isCollapsible = collapsibleWeeks.has(week.clientId) + const isForceExpanded = manuallyExpanded.has(week.clientId) + + if (isCollapsible && !isForceExpanded) { + const refWeekNum = collapsibleWeeks.get(week.clientId)! + const sessionTypes = [...new Set(week.sessions.map((s) => s.sessionType))] + return ( +
+ + Week {weekIndex + 1} + + + same as Week {refWeekNum} + +
+ {sessionTypes.map((st) => ( + + {st} + + ))} +
+ +
+ ) + } + + return ( + + ) + })} + + +
+ - )} - + + + + ) } diff --git a/src/components/program-builder/block-list.tsx b/src/components/program-builder/block-list.tsx index a496305f..a449696b 100644 --- a/src/components/program-builder/block-list.tsx +++ b/src/components/program-builder/block-list.tsx @@ -1,4 +1,4 @@ -import { useCallback } from 'react' +import { useCallback, useEffect, useState } from 'react' import { DndContext, closestCenter, @@ -13,7 +13,7 @@ import { Button } from '@/components/ui/button' import { Icon } from '@/components/icon' import { BlockEditor } from './block-editor' import { addBlock, reorderBlocks } from './builder-state' -import type { ProgramDraft } from './builder-state' +import type { ProgramDraft, ValidationError } from './builder-state' import type { DayOfWeek } from './constants' // --------------------------------------------------------------------------- @@ -34,9 +34,20 @@ interface BlockListProps { onUpdate: (draft: ProgramDraft) => void onPickSession: (weekClientId: string, dayOfWeek: DayOfWeek) => void onCopyWeek?: (sourceWeekClientId: string) => void + showWeekends: boolean + fieldErrors?: ValidationError[] } -export function BlockList({ draft, onUpdate, onPickSession, onCopyWeek }: BlockListProps) { +export function BlockList({ + draft, + onUpdate, + onPickSession, + onCopyWeek, + showWeekends, + fieldErrors = [], +}: BlockListProps) { + const [newBlockId, setNewBlockId] = useState(null) + const sensors = useSensors( useSensor(PointerSensor, { activationConstraint: { distance: 8 }, @@ -59,9 +70,19 @@ export function BlockList({ draft, onUpdate, onPickSession, onCopyWeek }: BlockL ) const handleAddBlock = useCallback(() => { - onUpdate(addBlock(draft, 'ACCUMULATION')) + const updated = addBlock(draft, 'ACCUMULATION') + const newBlock = updated.blocks[updated.blocks.length - 1] + setNewBlockId(newBlock.clientId) + onUpdate(updated) }, [draft, onUpdate]) + useEffect(() => { + if (newBlockId) { + const t = setTimeout(() => setNewBlockId(null), 350) + return () => clearTimeout(t) + } + }, [newBlockId]) + const blockIds = draft.blocks.map((b) => b.clientId) return ( @@ -81,11 +102,20 @@ export function BlockList({ draft, onUpdate, onPickSession, onCopyWeek }: BlockL onUpdate={onUpdate} onPickSession={onPickSession} onCopyWeek={onCopyWeek} + showWeekends={showWeekends} + isNew={block.clientId === newBlockId} + errors={fieldErrors.filter((e) => e.blockClientId === block.clientId)} /> ))} + {draft.blocks.length === 0 && ( +

+ Start by adding your first training block. +

+ )} + + + {draft.blocks.map((block, blockIndex) => ( e.blockClientId === block.clientId)} /> ))} + {draft.blocks.length === 0 && ( +

+ Start by adding your first training block. +

+ )} + - )} - - - {block.blockType} - - - - {block.weeks.length} {block.weeks.length === 1 ? 'WK' : 'WKS'} - + useEffect(() => { + if (newWeekId) { + const t = setTimeout(() => setNewWeekId(null), 350) + return () => clearTimeout(t) + } + }, [newWeekId]) - - - - - - - - - - {/* Expanded content */} - {expanded && ( -
-
- {BLOCK_TYPES.map((bt) => ( +
+ + + + + {isEditingName ? ( + handleNameChange(e.target.value)} + onBlur={() => setIsEditingName(false)} + onKeyDown={(e) => { + if (e.key === 'Enter') setIsEditingName(false) + }} + autoFocus + className={`min-w-0 flex-1 border-0 border-b bg-transparent py-1 font-display text-sm font-medium text-bone-white focus:outline-none ${ + nameError + ? 'border-warning-flare focus:border-warning-flare' + : 'border-warm-ash/30 focus:border-ember' + }`} + aria-label="Block name" + aria-invalid={!!nameError} + /> + ) : ( - ))} + )} + + + {block.blockType} + + + + {block.weeks.length} {block.weeks.length === 1 ? 'WK' : 'WKS'} + + + + + + + + + + Move up + + + + Move down + + + setShowDeleteConfirm(true)}> + + Delete block + + +
- {block.weeks.map((week, weekIndex) => ( - - ))} - - + {/* Inline validation errors */} + {errors.length > 0 && ( +
+ {nameError &&

{nameError}

} + {weeksError &&

{weeksError}

} +
+ )} + + {/* Expanded content */} + +
+ + + {block.weeks.map((week, weekIndex) => { + const isCollapsible = collapsibleWeeks.has(week.clientId) + const isForceExpanded = manuallyExpanded.has(week.clientId) + + if (isCollapsible && !isForceExpanded) { + const refWeekNum = collapsibleWeeks.get(week.clientId)! + const sessionTypes = [...new Set(week.sessions.map((s) => s.sessionType))] + return ( +
+ + Week {weekIndex + 1} + + + same as Week {refWeekNum} + +
+ {sessionTypes.map((st) => ( + + {st} + + ))} +
+ +
+ ) + } + + return ( + + ) + })} + + +
+
- )} -
+ + + + ) } @@ -264,6 +437,8 @@ interface MobileWeekSectionProps { onUpdate: (draft: ProgramDraft) => void onPickSession: (weekClientId: string, dayOfWeek: DayOfWeek) => void onCopyWeek: (sourceWeekClientId: string) => void + showWeekends: boolean + isNew?: boolean } function MobileWeekSection({ @@ -275,12 +450,21 @@ function MobileWeekSection({ onUpdate, onPickSession, onCopyWeek, + showWeekends, + isNew, }: MobileWeekSectionProps) { + const [showWeekDeleteConfirm, setShowWeekDeleteConfirm] = useState(false) + // Build session lookup by dayOfWeek const sessionsByDay = new Map( sessions.filter((s) => s.dayOfWeek !== null).map((s) => [s.dayOfWeek!, s]), ) + const weekendSessionCount = useMemo(() => { + if (showWeekends) return 0 + return sessions.filter((s) => s.dayOfWeek === 0 || s.dayOfWeek === 6).length + }, [showWeekends, sessions]) + const handleRemoveWeek = useCallback(() => { onUpdate(removeWeekFromBlock(draft, blockClientId, weekClientId)) }, [draft, blockClientId, weekClientId, onUpdate]) @@ -289,10 +473,19 @@ function MobileWeekSection({ onCopyWeek(weekClientId) }, [weekClientId, onCopyWeek]) + const weekDeleteDescription = useMemo(() => { + if (sessions.length > 0) + return `This will remove ${sessions.length} ${sessions.length === 1 ? 'session' : 'sessions'}.` + return 'This week is empty.' + }, [sessions.length]) + return ( -
+
- + WEEK {weekIndex + 1}
- {DAY_ORDER.map((dayOfWeek) => { + {(showWeekends ? DAY_ORDER : WEEKDAY_ORDER).map((dayOfWeek) => { const session = sessionsByDay.get(dayOfWeek) return ( @@ -330,6 +523,20 @@ function MobileWeekSection({ ) })}
+ + {!showWeekends && weekendSessionCount > 0 && ( +

+ +{weekendSessionCount} weekend {weekendSessionCount === 1 ? 'session' : 'sessions'} +

+ )} + +
) } @@ -360,10 +567,18 @@ function MobileDayRow({ }, [weekClientId, dayOfWeek, onPickSession]) const handleRemove = useCallback( - (e: React.MouseEvent) => { + (e: React.MouseEvent | React.KeyboardEvent) => { e.stopPropagation() if (session) { + const previousDraft = draft onUpdate(removeSession(draft, weekClientId, session.clientId)) + toast('Session removed', { + action: { + label: 'Undo', + onClick: () => onUpdate(previousDraft), + }, + duration: 5000, + }) } }, [session, draft, weekClientId, onUpdate], @@ -374,14 +589,16 @@ function MobileDayRow({ ) } @@ -389,46 +606,46 @@ function MobileDayRow({ const isEvent = session.sessionType === 'EVENT' return ( - +
- + + ) } diff --git a/src/components/program-builder/program-form.tsx b/src/components/program-builder/program-form.tsx index 20e201c8..c5148398 100644 --- a/src/components/program-builder/program-form.tsx +++ b/src/components/program-builder/program-form.tsx @@ -1,3 +1,8 @@ +import { useState, useId } from 'react' +import { ToggleGroup, ToggleGroupItem } from '@/components/ui/toggle-group' +import { HelpTrigger } from '@/components/ui/help-trigger' +import { SOURCE_HELP } from '@/components/builders/help-content' +import { Icon } from '@/components/icon' import type { ProgramDraft } from './builder-state' import type { ProgramSource } from '@/domain/types' @@ -22,11 +27,16 @@ const PROGRAM_SOURCES: Array<{ value: ProgramSource; label: string }> = [ interface ProgramFormProps { draft: ProgramDraft onChange: (updates: Partial>) => void + error?: string } -export function ProgramForm({ draft, onChange }: ProgramFormProps) { +export function ProgramForm({ draft, onChange, error }: ProgramFormProps) { + const [detailsOpen, setDetailsOpen] = useState(false) + const detailsId = useId() + return (
+ {/* Program name -- always visible */}
PROGRAM NAME @@ -36,44 +46,83 @@ export function ProgramForm({ draft, onChange }: ProgramFormProps) { value={draft.name} onChange={(e) => onChange({ name: e.target.value })} placeholder="Program name" - className="w-full border-0 border-b border-warm-ash/30 bg-transparent py-3 font-display text-lg font-medium text-bone-white placeholder:text-warm-ash/40 focus:border-ember focus:outline-none" + className={`w-full border-0 border-b bg-transparent py-3 font-display text-lg font-medium text-bone-white placeholder:text-warm-ash/40 focus:outline-none ${ + error + ? 'border-warning-flare focus:border-warning-flare' + : 'border-warm-ash/30 focus:border-ember' + }`} aria-label="Program name" + aria-invalid={!!error} /> + {error &&

{error}

}
-
- - DESCRIPTION (OPTIONAL) - -