Skip to content

[db] Guard activity overlap pairs - #2219

Merged
Asherlc merged 5 commits into
mainfrom
Asherlc/optimize-activity-overlap
Jul 27, 2026
Merged

Asherlc merged 5 commits into
mainfrom
Asherlc/optimize-activity-overlap

Conversation

@Asherlc

@Asherlc Asherlc commented Jul 27, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • require positive time overlap before activity deduplication evaluates either 80% overlap ratio
  • deploy the canonical fitness.v_activity update through migration 0059
  • cover contained and boundary-touching activities with a real-PostgreSQL plan regression
  • record the shared DOFEK-SERVER-5K/5M/5N production incident

Root cause

The recursive activity view compared every same-user activity pair globally before request-level user/date predicates could apply. The exact production plan rejected 6,941,004 time-disjoint pairs, held all five web-pool connections for 13.94–16.29 seconds under concurrent dashboard load, and caused queued session, insights, and PMC queries to hit the unchanged ten-second acquisition timeout.

The new predicates are logically implied by both existing strict overlap-ratio branches. They preserve current grouping semantics while giving PostgreSQL cheap necessary conditions for disjoint intervals. No pool size, timeout, retry, cache, or database resource limit changes.

Validation

  • production read-only benchmark: same 210 rows, 5.08s → 1.84s (64% faster)
  • pnpm typecheck
  • targeted Biome check
  • migration policy check
  • migration body is SQLFluff-formatted; its view semantics match the canonical source
  • uv tool run sqlfluff lint drizzle/0059_v_activity_positive_overlap.sql
  • pnpm exec cspell --no-progress
  • git diff --check
  • local PostgreSQL integration execution is blocked by Docker Desktop ENOSPC; PR integration CI is the executable green gate

Sentry

Refs DOFEK-SERVER-5K, DOFEK-SERVER-5M, and DOFEK-SERVER-5N.

Summary by Sourcery

Guard activity overlap detection with positive time-interval checks and document and test the production incident regression behavior.

Bug Fixes:

  • Prevent the activity overlap view from evaluating overlap ratios for time-disjoint activity pairs by requiring positive time overlap in the pair join.

Enhancements:

  • Introduce a canonical fitness.v_activity view definition and deploy the positive-overlap change via a forward migration.

Documentation:

  • Document the 2026-07-27 production incident where global activity overlap expansion exhausted web database pools.

Tests:

  • Add a PostgreSQL-backed integration test that validates both the deduplication behavior and the generated query plan’s use of positive-overlap guards for activity pairs.

Summary by cubic

Add positive-overlap guards to fitness.v_activity so time‑disjoint activity pairs are skipped. This speeds up overlap grouping and prevents the web DB pool exhaustion seen in production.

  • Bug Fixes

    • Require positive interval overlap in the pair join (c1.started_at < c2.ended_at and c1.ended_at > c2.started_at) before 80% overlap checks; grouping semantics unchanged. Align the canonical view with the migration and compute pair metrics once.
    • Production read-only benchmark: same 210 rows, 5.08s → 1.84s (~64% faster). Tests are alias‑independent and still assert both overlap guards. Incident documented; refs DOFEK-SERVER-5K/5M/5N.
  • Migration

    • Deploy canonical view via 0059; _views/01_v_activity.sql mirrors it. No changes to pool size, timeouts, retries, caches, or DB resource limits.

Written for commit dc8b132. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved activity overlap processing by excluding non-overlapping or boundary-touching activity intervals.
    • Reduced unnecessary database workload during activity merging and overlap analysis.
    • Improved reliability for activity-related requests, helping prevent connection timeout errors during high-load scenarios.
  • Tests

    • Added integration coverage to verify overlap results and query performance behavior.

Asherlc added 2 commits July 27, 2026 12:34
Deploy the canonical view update through migration 0059 so time-disjoint activity pairs are rejected by cheap interval predicates while preserving the existing overlap semantics.
Copilot AI review requested due to automatic review settings July 27, 2026 19:36
@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@cursor

cursor Bot commented Jul 27, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds a replacement fitness.v_activity view with strict positive-overlap candidate filtering, journals the migration, adds PostgreSQL integration and plan checks, and documents the production incident, root cause, mitigation, and validation.

Changes

Activity overlap mitigation

Layer / File(s) Summary
Prepare clusterable activity inputs
drizzle/0059_v_activity_positive_overlap.sql
Ranks activities, resolves effective tombstones, and normalizes activity intervals for clustering.
Filter pairs and build merged groups
drizzle/0059_v_activity_positive_overlap.sql, drizzle/_views/01_v_activity.sql
Requires mutual positive interval overlap before ratio checks, then builds clusters and merged canonical activity rows.
Register and validate the mitigation
drizzle/meta/_journal.json, src/db/activity-overlap-plan.integration.test.ts, docs/production-incident-baseline.md
Registers the migration, tests overlap results and query-plan predicates, and records incident evidence and follow-up validation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ActivityData
  participant fitness_v_activity
  participant OverlapQuery
  participant PostgreSQLPlan
  ActivityData->>fitness_v_activity: supply activity intervals
  fitness_v_activity->>fitness_v_activity: filter mutually overlapping pairs
  fitness_v_activity->>OverlapQuery: return merged activity groups
  OverlapQuery->>PostgreSQLPlan: inspect EXPLAIN plan predicates
Loading

Possibly related PRs

Suggested labels: area/db, type/bug

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is imperative, under 70 characters, correctly prefixed, and accurately describes the positive-overlap guard change.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Adds strict positive-interval overlap guards to the activity deduplication view, ships the canonical view via a new migration, documents the production incident, and adds a PostgreSQL-backed regression test that validates both query semantics and execution plan.

File-Level Changes

Change Details Files
Guard activity pair generation with cheap positive-overlap predicates to avoid evaluating overlap ratios for disjoint intervals.
  • Add started/ended timestamp overlap predicates to the clusterable self-join that feeds the pairs CTE in the v_activity view.
  • Ensure the new predicates are logically implied by existing 80% overlap-ratio conditions so grouping semantics remain unchanged.
drizzle/_views/01_v_activity.sql
drizzle/0059_v_activity_positive_overlap.sql
Introduce a canonical v_activity view migration wired into Drizzle metadata for consistent deployment.
  • Create migration 0059 with the full canonical fitness.v_activity definition, including the new positive-overlap guards.
  • Register the new migration in the Drizzle meta journal so fresh schemas and forwards migrations share the same source definition.
drizzle/0059_v_activity_positive_overlap.sql
drizzle/meta/_journal.json
Add a real-PostgreSQL regression that verifies both activity grouping behavior and the generated query plan’s join filter.
  • Seed three activities (containing, contained, and boundary-touching) for a test user in the test database.
  • Assert that v_activity groups only the overlapping pair and leaves the boundary-touching activity separate.
  • Run EXPLAIN (FORMAT JSON) on a v_activity query and walk the JSON to find the CTE pairs subplan, asserting the join filter contains both positive-overlap predicates.
src/db/activity-overlap-plan.integration.test.ts
Document the production incident caused by global activity overlap expansion and its fix.
  • Append a detailed incident postmortem describing symptoms, root cause, evidence, mitigation, and follow-up steps for the activity overlap query exhausting web DB pools.
  • Explicitly note that the fix adds positive-overlap guards without changing pooling or timeout settings and reference relevant PostgreSQL documentation links.
docs/production-incident-baseline.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Guard v_activity overlap pairs with positive time intersection

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add strict interval-overlap guards to avoid evaluating ratios for time-disjoint activity pairs.
• Deploy the updated canonical fitness.v_activity definition via migration 0059.
• Add a real-PostgreSQL plan regression test and document the related production incident.
Diagram

graph TD
  A["Dashboard/Insights query"] --> V["fitness.v_activity view"] --> P["CTE pairs join"]
  V --> DB[("fitness.activity table")]
  P --> DB
  M["Migration 0059"] --> V
  T["Plan integration test"] --> V
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use range overlap operator + GiST index
  • ➕ Could allow index-assisted overlap filtering (tsrange && tsrange).
  • ➕ Makes the overlap predicate more idiomatic Postgres.
  • ➖ Requires schema/index changes and careful handling of open-ended ended_at normalization.
  • ➖ Recursive CTE shape may still limit predicate pushdown benefits.
2. Convert v_activity to a parameterized function (user/date bounded)
  • ➕ Guarantees early bounding of work by user/date predicates.
  • ➕ Can tailor plans per-call and avoid global pair expansion.
  • ➖ Larger API change for callers and migrations.
  • ➖ Harder to use as a simple view across the codebase.
3. Materialize deduped activities
  • ➕ Removes expensive recursive work from request path.
  • ➕ More predictable performance under burst load.
  • ➖ Adds refresh complexity and staleness concerns.
  • ➖ More operational overhead than a necessary-condition guard.

Recommendation: Keep the PR’s approach: adding strict positive-overlap guards is the lowest-risk, semantics-preserving fix that gives Postgres a cheap necessary condition to prune disjoint pairs before ratio math. The alternatives (indexes/ranges, parameterization, or materialization) may yield further gains but introduce broader schema/API/operational complexity that isn’t necessary to resolve the immediate pool-exhaustion incident.

Files changed (5) +477 / -0

Bug fix (1) +2 / -0
01_v_activity.sqlGuard pairs join with strict interval overlap +2/-0

Guard pairs join with strict interval overlap

• Adds two join predicates that require positive time intersection between activities before overlap-ratio evaluation. This prevents evaluating expensive overlap arithmetic for time-disjoint pairs while preserving existing ratio semantics.

drizzle/_views/01_v_activity.sql

Tests (1) +100 / -0
activity-overlap-plan.integration.test.tsAdd Postgres integration test asserting overlap guards in executable plan +100/-0

Add Postgres integration test asserting overlap guards in executable plan

• Creates a PostgreSQL-backed integration test that inserts contained and boundary-touching activities, validates deduplication grouping behavior, and uses EXPLAIN (FORMAT JSON) to assert the pairs join filter includes both positive-overlap predicates.

src/db/activity-overlap-plan.integration.test.ts

Documentation (1) +50 / -0
production-incident-baseline.mdDocument 2026-07-27 DB pool exhaustion incident and root cause +50/-0

Document 2026-07-27 DB pool exhaustion incident and root cause

• Adds a detailed incident report covering symptoms, impact, query-plan evidence, and root cause tied to global activity pair expansion. Documents the chosen mitigation (positive-overlap guards), benchmark results, and validation/follow-up steps.

docs/production-incident-baseline.md

Other (2) +325 / -0
0059_v_activity_positive_overlap.sqlAdd canonical v_activity view definition with positive-overlap pair guards +318/-0

Add canonical v_activity view definition with positive-overlap pair guards

• Introduces the canonical CREATE OR REPLACE VIEW for fitness.v_activity used for fresh schemas and forward migrations. The pairs CTE join now requires strict interval overlap (c1.started_at < c2.ended_at and c2.started_at < c1.ended_at) before applying the existing 80% overlap ratio rules.

drizzle/0059_v_activity_positive_overlap.sql

_journal.jsonRegister migration 0059 in Drizzle journal +7/-0

Register migration 0059 in Drizzle journal

• Adds the 0059_v_activity_positive_overlap migration entry to the Drizzle migration journal so the view update is applied in deployed environments.

drizzle/meta/_journal.json

@github-actions

github-actions Bot commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

Storybook previews for 2afc0b1f are ready:

This comment updates automatically on each PR push.

@qodo-code-review

qodo-code-review Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 156 rules

Grey Divider


Remediation recommended

1. Migration marked mutable canonical ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new 0059 migration tells developers to "edit THIS file" as the canonical v_activity source, but
the migrator enforces applied migrations as immutable and will fail startup if this file is changed
later. This duplicates the same canonical guidance already present in drizzle/_views and increases
the risk someone edits the wrong (immutable) file in the future.
Code

drizzle/0059_v_activity_positive_overlap.sql[R1-7]

+-- Canonical definition of the fitness.v_activity view.
+-- This file is the source definition for fresh databases, local test schemas,
+-- and future forward migrations that need to update the deployed view.
+--
+-- To change v_activity: edit THIS file and add a forward migration when the
+-- deployed view definition must change.
+-- Git merge conflicts here force developers to reconcile concurrent changes.
Relevance

⭐⭐⭐ High

Team enforces migration immutability; restoring modified applied migrations was required (PRs #1848,
#1863).

PR-#1848
PR-#1863

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The migration file explicitly instructs editing itself in the future, but the migrator throws if an
applied migration’s contents change, making that guidance unsafe.

drizzle/0059_v_activity_positive_overlap.sql[1-7]
drizzle/_views/01_v_activity.sql[1-7]
src/db/postgres-migrator.ts[146-175]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`drizzle/0059_v_activity_positive_overlap.sql` is a numbered migration that will be applied and integrity-checked, but its header comment instructs future developers to edit it as the canonical view definition. This conflicts with the migrator’s immutability guarantees and risks future deployments failing due to migration integrity checks.

### Issue Context
The repository already has a canonical view definition in `drizzle/_views/01_v_activity.sql` with identical “edit THIS file” guidance. Numbered migration files should be treated as immutable after merge.

### Fix Focus Areas
- drizzle/0059_v_activity_positive_overlap.sql[1-7]
- drizzle/_views/01_v_activity.sql[1-7]
- src/db/postgres-migrator.ts[146-175]

### Suggested fix
- Update the header comment in `drizzle/0059_v_activity_positive_overlap.sql` to explicitly state it is an immutable forward migration (do not edit after merge).
- Point developers to `drizzle/_views/01_v_activity.sql` as the canonical/editable source of truth.
- Optionally add a brief note that future changes require a new forward migration generated from the canonical `_views` definition.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Incident lacks runbook reference ✓ Resolved 📘 Rule violation ➹ Performance
Description
This PR changes fitness.v_activity to address a dashboard-query slowdown, but the new incident
entry does not explicitly state that docs/performance/loading-performance-runbook.md was followed
or classify the slowdown per the runbook before the behavior change. This violates the requirement
to record the classification/evidence gate before modifying behavior in response to dashboard
slowdowns.
Code

docs/production-incident-baseline.md[R18957-19001]

+## 2026-07-27 — Global activity overlap expansion exhausted web DB pools
+
+- **Status:** Root cause confirmed from production query plans and direct
+  source fix prepared; merge, deployment, and production validation pending.
+- **Symptoms:** Dashboard bursts reported
+  [DOFEK-SERVER-5K](https://east-bay-software.sentry.io/issues/DOFEK-SERVER-5K),
+  [DOFEK-SERVER-5M](https://east-bay-software.sentry.io/issues/DOFEK-SERVER-5M),
+  and
+  [DOFEK-SERVER-5N](https://east-bay-software.sentry.io/issues/DOFEK-SERVER-5N)
+  as failures of session, insights, and PMC SQL statements.
+- **User impact:** Five session validations plus the insights and PMC SQL
+  calls could not acquire a database connection at 17:46 UTC. At 18:35 UTC,
+  insights and PMC failed while the surrounding dashboard batch took 47.7
+  seconds to finish.
+- **Evidence:** Every Sentry event's causal error was
+  `timeout exceeded when trying to connect` at the web process's unchanged
+  ten-second pool acquisition boundary; the displayed SQL had not started.
+  Web request logs and `pg_stat_statements` show the pool was instead occupied
+  by `fitness.v_activity` queries that took 13.94–16.29 seconds. A read-only
+  `EXPLAIN (ANALYZE, BUFFERS)` of the exact insights query took 5.08 seconds
+  from shared buffers: the recursive view built 2,635 activity rows globally,
+  compared the same-user cross-product, rejected 6,941,004 pairs, and produced
+  2,221 overlapping pairs before applying the request's user and date filters.
+  Current statistics were fresh, there were no lock waits or idle
+  transactions, and the server had 11 of 40 connections in use.
+- **Root cause:** `fitness.v_activity` evaluated the expensive overlap-ratio
+  arithmetic for every same-user activity pair, including millions of
+  time-disjoint pairs. The view's recursive grouping prevents the outer
+  request predicates from bounding that global work. PostgreSQL documents CTE
+  evaluation and recursive query behavior:
+  <https://www.postgresql.org/docs/current/queries-with.html>.
+- **Fix / mitigation:** Require strict positive interval overlap in the pair
+  join alongside the two 80% ratios. Both ratio branches already require
+  positive overlap, so the guards preserve deduplication, contained-activity,
+  cross-provider, and boundary-touching semantics while giving PostgreSQL
+  cheap necessary conditions for disjoint windows. The production read-only
+  benchmark returned the same 210 rows in 1.84 seconds, 64% faster. No pool
+  size, acquisition timeout, retry, cache, or database resource limit changed.
+- **Validation:** A real-PostgreSQL regression fixture covers a contained pair
+  and a boundary-touching non-pair, then inspects the database's executable
+  pairs plan for both positive-overlap guards. Local execution is blocked
+  because Docker Desktop's filesystem is full and PostgreSQL cannot create an
+  isolated test database; GitHub integration CI remains the executable green
+  gate. PostgreSQL documents executable plan inspection with `EXPLAIN`:
+  <https://www.postgresql.org/docs/current/using-explain.html>.
Relevance

⭐⭐⭐ High

Runbook/classification references were previously required and accepted for incident-baseline
performance changes (PRs #2036, #2038).

PR-#2036
PR-#2038

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR introduces a behavior change to the fitness.v_activity view to guard overlap pairs by
requiring positive interval overlap, which is explicitly motivated by a dashboard slowdown/DB pool
exhaustion incident. The corresponding new incident entry records symptoms and evidence but does not
explicitly state that the loading-performance runbook was followed or provide a runbook-style
classification entry, which the checklist requires before changing behavior.

Rule 1540813: Classify and record dashboard slowdowns before modifying behavior
drizzle/_views/01_v_activity.sql[149-156]
docs/production-incident-baseline.md[18957-19001]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new incident write-up for the 2026-07-27 dashboard slowdown does not explicitly document that the loading-performance runbook was followed and does not include an explicit slowdown classification (per the runbook’s taxonomy) before the behavior change.

## Issue Context
Compliance requires that behavior changes made in response to dashboard slowdowns be preceded by recorded classification and evidence per `docs/performance/loading-performance-runbook.md`.

## Fix Focus Areas
- docs/production-incident-baseline.md[18957-19001]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. testCtx.db.execute() bypasses executeWithSchema ✓ Resolved 📘 Rule violation ≡ Correctness
Description
New database SQL in the integration test is executed via testCtx.db.execute(...) (including
generic-typed execute<{...}>) instead of the required executeWithSchema() wrapper. This bypasses
Zod-based runtime validation and violates the raw-SQL execution policy.
Code

src/db/activity-overlap-plan.integration.test.ts[R41-92]

+    await testCtx.db.execute(
+      sql`INSERT INTO fitness.provider (id, name, user_id)
+          VALUES ('wahoo', 'Wahoo', ${TEST_USER_ID})
+          ON CONFLICT DO NOTHING`,
+    );
+    await testCtx.db.execute(
+      sql`INSERT INTO fitness.activity (
+            id, provider_id, user_id, external_id, activity_type, started_at, ended_at
+          ) VALUES
+          (
+            ${activityIds[0]}::uuid, 'wahoo', ${TEST_USER_ID}, 'overlap-plan-a', 'cycling',
+            TIMESTAMPTZ '2026-01-10 10:00:00+00',
+            TIMESTAMPTZ '2026-01-10 11:00:00+00'
+          ),
+          (
+            ${activityIds[1]}::uuid, 'wahoo', ${TEST_USER_ID}, 'overlap-plan-contained', 'cycling',
+            TIMESTAMPTZ '2026-01-10 10:05:00+00',
+            TIMESTAMPTZ '2026-01-10 10:55:00+00'
+          ),
+          (
+            ${activityIds[2]}::uuid, 'wahoo', ${TEST_USER_ID}, 'overlap-plan-touching', 'cycling',
+            TIMESTAMPTZ '2026-01-10 11:00:00+00',
+            TIMESTAMPTZ '2026-01-10 12:00:00+00'
+          )`,
+    );
+  });
+
+  afterAll(async () => {
+    await testCtx.cleanup();
+  });
+
+  it("requires positive overlap for candidate pairs", async () => {
+    const rows = await testCtx.db.execute<{ member_activity_ids: string[] }>(
+      sql`SELECT member_activity_ids::text[] AS member_activity_ids
+          FROM fitness.v_activity
+          WHERE user_id = ${TEST_USER_ID}
+            AND member_activity_ids && ARRAY[
+              ${activityIds[0]}::uuid,
+              ${activityIds[1]}::uuid,
+              ${activityIds[2]}::uuid
+            ]
+          ORDER BY started_at`,
+    );
+
+    expect(rows.map((row) => row.member_activity_ids.length).sort()).toEqual([1, 2]);
+
+    const explainRows = await testCtx.db.execute<{ "QUERY PLAN": unknown }>(
+      sql`EXPLAIN (FORMAT JSON)
+          SELECT count(*)
+          FROM fitness.v_activity
+          WHERE user_id = ${TEST_USER_ID}`,
+    );
Relevance

⭐⭐⭐ High

Similar reviews required replacing raw db.execute with executeWithSchema in tests; accepted in PR
#2215.

PR-#2215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 722086 requires executing raw SQL via executeWithSchema() with a Zod schema. In
src/db/activity-overlap-plan.integration.test.ts, multiple raw SQL statements are executed
directly via testCtx.db.execute(...) (including execute<{ member_activity_ids: string[] }> and
execute<{ "QUERY PLAN": unknown }>), which bypasses the required wrapper.

Rule 722086: Execute raw SQL via typed executeWithSchema wrapper
src/db/activity-overlap-plan.integration.test.ts[41-92]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test executes raw SQL using `testCtx.db.execute(...)` (and `execute<{...}>` generics) rather than `executeWithSchema()`, which is required for schema-validated SQL execution.

## Issue Context
The codebase provides `executeWithSchema()` (Zod-backed) to validate raw SQL result shapes at runtime and prevent schema drift/type mismatches that generics cannot catch.

## Fix Focus Areas
- src/db/activity-overlap-plan.integration.test.ts[41-92]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (2)
4. afterAll unguarded cleanup ✓ Resolved 🐞 Bug ☼ Reliability
Description
afterAll unconditionally calls testCtx.cleanup(), but testCtx is assigned inside beforeAll; if
setupTestDatabase throws before assignment, afterAll can throw a secondary error and obscure the
original failure. setupTestDatabase can throw early when TEST_DATABASE_URL is missing.
Code

src/db/activity-overlap-plan.integration.test.ts[R68-70]

+  afterAll(async () => {
+    await testCtx.cleanup();
+  });
Relevance

⭐⭐⭐ High

Repo previously accepted guarding/ensuring cleanup to avoid secondary failures/leaks in DB tests (PR
#1075).

PR-#1075

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test’s teardown dereferences a value only set during setup, and the setup helper can throw
before that assignment happens (e.g., missing env var).

src/db/activity-overlap-plan.integration.test.ts[36-70]
src/db/test-helpers.ts[263-269]
PR-#1075

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`afterAll` calls `testCtx.cleanup()` even though `testCtx` is only assigned in `beforeAll`. If `setupTestDatabase()` throws before assignment, teardown can throw a secondary error that hides the root cause.

### Issue Context
`setupTestDatabase()` throws when `TEST_DATABASE_URL` is not set, so this failure mode is realistic in local runs / misconfigured environments.

### Fix Focus Areas
- src/db/activity-overlap-plan.integration.test.ts[36-70]
- src/db/test-helpers.ts[263-269]

### Suggested fix
- Initialize `testCtx` as `TestContext | undefined` and guard in `afterAll`:
 - `if (testCtx) await testCtx.cleanup();`
- Optionally wrap `beforeAll` setup in a try/catch that rethrows after setting a flag, but the simple guard is usually sufficient.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. EXPLAIN assertion brittle ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new integration test asserts a specific EXPLAIN (FORMAT JSON) shape by reading only the "Join
Filter" field from a plan node identified as "CTE pairs" and matching exact rendered predicate
strings. Equivalent plans may place these predicates under different keys (or render them
differently) across PostgreSQL/planner changes, causing non-semantic test failures.
Code

src/db/activity-overlap-plan.integration.test.ts[R87-99]

+    const explainRows = await testCtx.db.execute<{ "QUERY PLAN": unknown }>(
+      sql`EXPLAIN (FORMAT JSON)
+          SELECT count(*)
+          FROM fitness.v_activity
+          WHERE user_id = ${TEST_USER_ID}`,
+    );
+    const pairsPlan = findPairsPlan(explainRows[0]?.["QUERY PLAN"]);
+    const joinFilter = pairsPlan?.["Join Filter"];
+
+    expect(joinFilter).toEqual(expect.any(String));
+    expect(joinFilter).toContain("(c1.started_at < c2.ended_at)");
+    expect(joinFilter).toContain("(c2.started_at < c1.ended_at)");
+  });
Relevance

⭐⭐ Medium

No strong historical evidence found about rejecting/accepting brittle EXPLAIN JSON predicate
assertions in tests.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test hard-codes both the plan node identifier and the specific JSON field used for predicates,
then asserts exact textual rendering, which is known to vary with planner decisions.

src/db/activity-overlap-plan.integration.test.ts[16-34]
src/db/activity-overlap-plan.integration.test.ts[87-99]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test currently depends on exact EXPLAIN JSON keys/strings:
- locating a node with `"Subplan Name" === "CTE pairs"`
- reading only `pairsPlan["Join Filter"]`
- substring-matching exact predicate text with parentheses

Planner output can legitimately change (e.g., predicates moved to `Hash Cond` / `Filter`, reordered, extra casts), breaking the test without any regression.

### Issue Context
This test is intended to ensure the positive-overlap guards remain part of the executable plan, but the assertion should be resilient to equivalent plan shapes.

### Fix Focus Areas
- src/db/activity-overlap-plan.integration.test.ts[16-34]
- src/db/activity-overlap-plan.integration.test.ts[87-99]

### Suggested fix options
- Collect *all* string fields in the plan JSON (walk the object tree) and assert the predicates appear somewhere, rather than requiring `Join Filter` on a specific node.
- Accept multiple fields (`Join Filter`, `Hash Cond`, `Filter`) when checking the join predicates.
- Consider asserting the view definition contains the guards (via `pg_get_viewdef`) in addition to a lighter plan assertion, to keep coverage while reducing dependence on planner formatting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/db/activity-overlap-plan.integration.test.ts Outdated
Comment thread docs/production-incident-baseline.md
Comment thread drizzle/0059_v_activity_positive_overlap.sql Outdated
Comment thread src/db/activity-overlap-plan.integration.test.ts Outdated
Comment thread src/db/activity-overlap-plan.integration.test.ts
Format the immutable migration for SQLFluff and make the database regression schema-validated and planner-shape tolerant.
@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@drizzle/0059_v_activity_positive_overlap.sql`:
- Around line 154-193: Update the pairs logic in both the canonical view and
migration so they share the same implementation, using the canonical view
definition as the source of truth. Remove the separate pair_metrics CTE from the
migration and align pairs with the canonical structure while preserving the
existing overlap criteria.

In `@src/db/activity-overlap-plan.integration.test.ts`:
- Around line 89-102: Update the row-size assertion to sort numerically using an
explicit comparator rather than default lexicographic sorting. In the EXPLAIN
assertions around collectPlanStrings, replace c1/c2 alias-dependent regexes with
checks for the relevant predicate columns and comparison operators, preserving
validation of the positive-overlap boundaries without coupling to view alias
rendering.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ac3cd75a-1d1a-43e5-b265-9e4ec3f4fcdd

📥 Commits

Reviewing files that changed from the base of the PR and between fcece97 and 7e43261.

📒 Files selected for processing (5)
  • docs/production-incident-baseline.md
  • drizzle/0059_v_activity_positive_overlap.sql
  • drizzle/_views/01_v_activity.sql
  • drizzle/meta/_journal.json
  • src/db/activity-overlap-plan.integration.test.ts

Comment thread drizzle/0059_v_activity_positive_overlap.sql
Comment thread src/db/activity-overlap-plan.integration.test.ts Outdated
Keep the canonical pair implementation structurally aligned with the linted migration and make the plan regression alias-independent.
@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@Asherlc
Asherlc merged commit a926444 into main Jul 27, 2026
104 checks passed
@Asherlc
Asherlc deleted the Asherlc/optimize-activity-overlap branch July 27, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants