Persist inferred sessions, simplify feed, add session editing

[marcodejongh]

Summary

• Eliminates ungrouped ticks: Every tick now has either a session_id (party) or inferred_session_id (inferred). A SQL migration (0060) backfills all existing ungrouped ticks using the same UUIDv5 deterministic IDs as the TypeScript builder, so there's zero gap on deploy.
• Simplifies feed resolver: Removes ~300 lines of ungrouped session handling (complex LAG window functions, ug: synthetic IDs, resolveUngroupedSession()).
• Adds session editing: Users can rename inferred sessions, add/remove other climbers. Adding a user reassigns their overlapping ticks (±30 min buffer) into the session. Removing restores ticks to their original sessions via previousInferredSessionId — fully non-destructive.
• Improves inferred session builder: Replaces 48h-window processing with batched full-history builder. Also triggers after Aurora sync so imported ticks get sessions immediately.

Changes

Schema (packages/db)

• inferred_sessions: add name, description columns
• boardsesh_ticks: add previous_inferred_session_id column (for undo)
• New session_member_overrides table (tracks manual user additions)
• Migration 0059: schema changes
• Migration 0060: backfill all ungrouped ticks with inferred session IDs

Backend (packages/backend)

• Simplified session-feed.ts — removed ungrouped CTEs, UngroupedFilter, resolveUngroupedSession()
• New session-mutations.ts — updateInferredSession, addUserToSession, removeUserFromSession mutations
• Batched inferred session builder (processes all users, bulk updates)
• Removed ug: handling from entity validation
• Added backfill-sessions npm script

Shared Schema (packages/shared-schema)

• Added mutation types and input types for session editing

Frontend (packages/web)

• Session detail page: edit mode for renaming, add/remove participants
• New user search dialog for finding climbers to add
• Added mutation operations for session editing
• Post-Aurora-sync trigger for inferred session builder

Test plan

• Navigate to /session/fx-party-session-001 — should load party session correctly
• Navigate to an inferred session — should load with name/description fields
• Edit an inferred session name as a participant
• Add a user to an inferred session — their overlapping ticks should appear
• Remove a user from a session — their ticks should be restored
• Verify SELECT COUNT(*) FROM boardsesh_ticks WHERE session_id IS NULL AND inferred_session_id IS NULL returns 0 after migration
• Run npm run typecheck — all packages pass

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
boardsesh	Building	Preview, Comment	Feb 26, 2026 5:47pm

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Claude Review

✅ Ready to merge - Minor issues noted below, but nothing blocking.

The PR is well-structured with good test coverage for the core session builder logic. Documentation is added appropriately.

Issues

1. Missing transaction wrapper in session mutations (packages/backend/src/graphql/resolvers/social/session-mutations.ts:100-316)

   • addUserToSession and removeUserFromSession perform multiple DB operations (tick updates, override inserts, stats recalculations) without a transaction wrapper. If an operation fails midway, the database could be left in an inconsistent state.
   • Contrast with assignInferredSession which correctly uses db.transaction().

2. Code duplication between packages

   • packages/backend/src/jobs/inferred-session-builder.ts and packages/web/app/lib/data-sync/aurora/inferred-session-builder.ts duplicate the UUID namespace, 4h gap constant, and grouping logic. Consider extracting shared logic to @boardsesh/shared-schema to prevent future divergence.

3. No integration tests for mutation authorization

   • session-mutations-validation.test.ts only tests Zod schemas, not the actual requireSessionParticipant authorization logic or the tick reassignment behavior. Edge cases like "user removes themselves" or "concurrent session modifications" are not covered.

4. N+1 queries in session feed (session-feed.ts:186-192)

   • For each session, 3 parallel queries run (participants, grade distribution, session meta). With 20 sessions per page, this is ~60 queries. Consider batching these into single queries with IN clauses.

Documentation Check

• New docs/inferred-sessions.md is comprehensive and well-written.
• No updates needed to existing WebSocket documentation as this does not affect party mode session mechanics.

[claude]

Claude Review

✅ Ready to merge - Minor issues noted below, but nothing blocking. The architecture is solid, migrations are well-structured, and the core logic is correct.

Issues

1. Missing tests for session mutations - packages/backend/src/graphql/resolvers/social/session-mutations.ts introduces three new mutations (updateInferredSession, addUserToSession, removeUserFromSession) with no corresponding test coverage. These mutations handle authorization checks and complex tick reassignment logic that should be tested.

2. Potential race condition - packages/backend/src/jobs/inferred-session-builder.ts:139-237 (assignInferredSession) performs multiple sequential queries (find prev tick, insert session, update tick, recalculate stats) without a transaction. Concurrent tick assignments for the same user could produce inconsistent state.

3. Hardcoded CSS variables - packages/web/app/session/[sessionId]/session-detail-content.tsx:285,558 uses var(--neutral-200) and var(--neutral-100) instead of design tokens from theme-config.ts per project guidelines.

4. No documentation update - This PR significantly changes session architecture (eliminates ungrouped sessions, adds editing). Consider updating docs/ to document the new inferred session system and editing capabilities.

[claude]

Claude Review

✅ Ready to merge - Minor issues noted below, but nothing blocking.

---

Issues

1. Incorrect owner identification - packages/web/app/session/[sessionId]/session-detail-content.tsx:203

   • ownerUserId = participants[0]?.userId assumes the first participant is the owner, but participants are sorted by sends DESC (from fetchParticipants), not ownership. In multi-user sessions, the highest-sends user may not be the original owner. This could cause the remove button to show for the actual owner. The backend correctly rejects this, but UX is confusing.
   • Suggestion: Add ownerId field to SessionDetail GraphQL type from inferred_sessions.userId.

2. Missing tests for mutation edge cases - packages/backend/src/graphql/resolvers/social/session-mutations.ts

   • No tests for: adding a user with no overlapping ticks (throws "No ticks found..."), removing a user who wasn't added via overrides, concurrent session modifications.

3. Documentation not updated - The docs/social-features-plan.md mentions session as a future entity type but doesn't document the new inferred session system, session-grouped feed, or session editing mutations. Consider adding a dedicated doc or updating the existing one.

[claude]

Claude Review

✅ Ready to merge - Well-structured implementation with comprehensive tests. Minor issues noted below.

Issues

1. Transaction inconsistency in recalculateSessionStats - packages/backend/src/jobs/inferred-session-builder.ts:180,207 and packages/backend/src/graphql/resolvers/social/session-mutations.ts:186-191 call recalculateSessionStats which uses the global db client, not the transaction tx. This can lead to inconsistent reads during concurrent tick operations. Consider passing the transaction as an optional parameter.

2. Missing tests for web-side builder - packages/web/app/lib/data-sync/aurora/inferred-session-builder.ts duplicates the backend grouping logic but has no test coverage. The backend builder has thorough tests; the web version should too.

3. Mutation tests lack integration coverage - Tests in session-mutations.test.ts heavily mock the database, testing only authorization flow. Consider adding integration tests that verify actual tick reassignment and stats recalculation.

Documentation

The new docs/inferred-sessions.md appropriately documents the feature.

[claude]

Claude Review

✅ Ready to merge - Well-structured PR with good test coverage. Minor issues noted below, nothing blocking.

Issues

Performance

• session-feed.ts:186-225: N+1 query pattern - each session row triggers 3 parallel queries (fetchParticipants, fetchGradeDistribution, fetchSessionMeta). For 20 sessions, that's 60 extra queries. Consider batching these lookups.

Potential Race Condition

• session-mutations.ts:267-294: Orphaned tick handling calls assignInferredSession in a loop outside the main update path. If concurrent operations occur, ticks could be reassigned to different sessions. Consider wrapping in a transaction or using a FOR UPDATE lock.

Type Safety

• session-mutations.ts:340-347: Using unsafe type cast (result as unknown as { rows: ... }) for raw SQL results. Consider using a Zod schema to validate the query result shape.

Suggestions (non-blocking)

• session-detail-content.tsx:144-158: Shows "Session Not Found" immediately on initial render when session is null. Consider adding a loading state prop to distinguish between "loading" and "not found".
• Missing integration tests that verify actual tick reassignment against a real database - the mutation tests use mocks. The test file header mentions this can be run with DATABASE_URL set, which is a good approach.

Documentation

Appropriate new documentation added in docs/inferred-sessions.md covering the architecture.

[claude]

Claude Review

✅ Ready to merge - Minor issues noted below, but nothing blocking. The feature is well-designed with good test coverage and documentation.

Issues

1. Nested transaction issue - packages/backend/src/graphql/resolvers/social/session-mutations.ts:294-296
assignInferredSession creates its own db.transaction() but is called inside the outer tx transaction. This won't participate in the outer transaction, potentially causing inconsistencies if the outer transaction rolls back.

2. Missing transaction in addUserToSession - packages/backend/src/graphql/resolvers/social/session-mutations.ts:100-194
   Unlike removeUserFromSession, this mutation doesn't wrap operations in a transaction. Concurrent modifications could leave data inconsistent (tick reassignments, override inserts, and stats recalculation should be atomic).

3. Unused helper functions - packages/backend/src/graphql/resolvers/social/session-feed.ts:480-517, 550-587
fetchGradeDistribution and fetchSessionMeta (non-batched versions) are defined but unused - the batch versions are used instead. Consider removing to reduce code bloat.

Test Coverage

The tests are good overall - they cover:

• Authentication/authorization paths
• Input validation
• Error conditions (session not found, user not found, no overlapping ticks, owner removal protection)
• Pure functions for session grouping and ID generation

Missing:

• No integration tests for the happy path of addUserToSession/removeUserFromSession that verify tick reassignment and stats recalculation work end-to-end
• No concurrent modification tests

Documentation

Good - new docs/inferred-sessions.md properly documents the feature.

[claude]

Claude Review

✅ Ready to merge - Well-structured PR with comprehensive documentation and test coverage. Minor issues noted below, but nothing blocking.

Issues

1. Unused parameter (packages/backend/src/graphql/resolvers/social/session-feed.ts:1687,1738) - The sessionTypes parameter in fetchParticipantsBatch and fetchGradeDistributionBatch is passed but never used. The queries use COALESCE(session_id, inferred_session_id) instead of filtering by session type. Consider removing the unused parameter for clarity.

2. Fire-and-forget error handling (packages/backend/src/graphql/resolvers/ticks/mutations.ts:239-242) - assignInferredSession failures are silently caught and logged. If this fails, the tick remains unassigned until the background job picks it up (30 min interval). The error is logged, but consider adding monitoring/alerting for high failure rates in production.

Documentation

Documentation is properly updated:

• New docs/inferred-sessions.md added with comprehensive system documentation
• docs/social-features-plan.md updated with cross-references to the new doc

Test Coverage

Good test coverage added across multiple files:

• Backend: inferred-session-builder.test.ts, session-mutations.test.ts, session-feed-validation.test.ts
• Frontend: session-detail-content.test.tsx, session-feed-card.test.tsx, chart component tests
• Web-side builder: inferred-session-builder.test.ts

Tests use mocks which is appropriate for unit testing. The test file comments correctly note that integration tests with a real database can be run by setting DATABASE_URL.

[claude]

Claude Review

✅ Ready to merge - Well-structured PR with good test coverage. Minor issues noted below, but nothing blocking.

---

Issues Found

Memory Leak - UserSearchDialog (packages/web/app/session/[sessionId]/user-search-dialog.tsx:26-48)

• The debounce timeout (debounceRef) is not cleaned up on component unmount. If the dialog closes while a search is pending, this could cause a state update on an unmounted component.
• Fix: Add a useEffect cleanup:

useEffect(() => {
  return () => {
    if (debounceRef.current) clearTimeout(debounceRef.current);
  };
}, []);

Web-side session builder lacks batching (packages/web/app/lib/data-sync/aurora/inferred-session-builder.ts:72)

• buildInferredSessionsForUser fetches all unassigned ticks without a limit, unlike the backend version which has batchSize. For users with many historical ticks, this could be slow or cause memory issues.

---

Minor Observations (Non-blocking)

• Authorization allows any session participant to remove any non-owner participant - may be intentional for collaborative editing, but worth documenting the design decision
• Test coverage is good for unit tests; integration tests require DATABASE_URL as documented

Documentation

• New docs/inferred-sessions.md properly documents the architecture ✓
• docs/social-features-plan.md updated ✓

[claude]

Claude Review

✅ Ready to merge - Minor issues noted below, but nothing blocking. The implementation is well-structured with good test coverage and proper documentation.

---

Issues

Race condition in session stats calculation (medium)

• packages/backend/src/jobs/inferred-session-builder.ts:252-283 and packages/web/app/lib/data-sync/aurora/inferred-session-builder.ts:145-167
• The upsertSessionAndAssignTicks function uses ON CONFLICT DO UPDATE SET tick_count = tick_count + EXCLUDED.tick_count. If the web builder (post-Aurora sync) and backend builder (30-min interval) race for the same user's unassigned ticks, both will see the same ticks and both will add to the session stats, resulting in doubled counts. Consider adding a transaction or using SELECT FOR UPDATE when fetching unassigned ticks.

UserSearchDialog debounce timer not cleaned up on unmount

• packages/web/app/session/[sessionId]/user-search-dialog.tsx:63-87
• The debounceRef.current timeout is never cleared when the dialog unmounts. Add a useEffect cleanup: useEffect(() => () => { if (debounceRef.current) clearTimeout(debounceRef.current); }, [])

Unrelated change in room-manager.ts

• packages/backend/src/services/room-manager.ts:378
• The session status update changed from fire-and-forget to awaited. This adds latency to the join flow but isn't mentioned in the PR description. If intentional, consider documenting why this behavioral change was made.

[claude]

Claude Review

✅ Ready to merge - Well-structured PR with comprehensive test coverage and proper documentation. Minor issues noted below.

Issues

1. Authorization may be too permissive - packages/backend/src/graphql/resolvers/social/session-mutations.ts: Any session participant can add/remove other users via addUserToSession/removeUserFromSession. Consider restricting to session owner only, or document this as intentional behavior.

2. Missing test edge case - packages/backend/src/__tests__/session-mutations.test.ts: No test for a user trying to add themselves to a session (e.g., adding self via addUserToSession when already a participant).

3. Migration 0060 ON CONFLICT uses additive stats - packages/db/drizzle/0060_backfill_inferred_sessions.sql:65-70: The ON CONFLICT DO UPDATE adds to existing stats (tick_count = inferred_sessions.tick_count + EXCLUDED.tick_count). If this migration runs against a partially backfilled database, stats could be doubled. Low risk since temp table ensures atomicity within one run, but worth noting.

4. Fire-and-forget session assignment - packages/backend/src/graphql/resolvers/ticks/mutations.ts:95-98: assignInferredSession is called without awaiting. On failure, the tick stays unassigned until the 30-min background job. This is documented behavior but creates a brief consistency window.

Documentation

• ✅ New docs/inferred-sessions.md properly documents the system
• ✅ docs/social-features-plan.md updated with cross-references

Test Coverage

• ✅ Good coverage for validation, authorization, and core business logic
• ✅ groupTicksIntoSessions unit tests cover gap detection, multi-user isolation, status counting
• ✅ Session mutation tests cover auth enforcement and error paths