fix: replace insert with upsert to prevent duplicate daily_activity_logs on re-save#185
fix: replace insert with upsert to prevent duplicate daily_activity_logs on re-save#185Varadraj75 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
…duplicate daily_activity_logs (AOSSIE-Org#184)
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRestores an index on Changes
Sequence Diagram(s)sequenceDiagram
participant TherapistApp as Therapist App
participant Repo as Supabase Repository
participant DB as Supabase DB
TherapistApp->>Repo: save daily activity (activity_id, date, patient_id, items)
Repo->>DB: upsert into daily_activity_logs (onConflict: activity_id,date,patient_id)
alt row exists
DB-->>Repo: update existing row
else no row
DB-->>Repo: insert new row
end
DB-->>Repo: enforce unique constraint uq_activity_date_patient
Repo-->>TherapistApp: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes duplicate daily_activity_logs rows created when re-saving/editing a daily activity by switching the write path to an upsert and adding a DB-level uniqueness guard.
Changes:
- Replaced
daily_activity_logs.insert()withupsert(..., onConflict: ...)in the therapist Supabase repository. - Added a
UNIQUE (activity_id, date, patient_id)constraint todaily_activity_logsin the Supabase schema.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| therapist/lib/repository/supabase_therapy_repository.dart | Writes daily activity logs via upsert to avoid duplicates on re-save. |
| supabase/schemas/schema.sql | Enforces uniqueness for activity logs at the database layer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| -- Unique constraint to prevent duplicate activity logs | ||
| ALTER TABLE daily_activity_logs | ||
| ADD CONSTRAINT uq_activity_date_patient | ||
| UNIQUE (activity_id, date, patient_id); |
There was a problem hiding this comment.
If schema.sql is intended to be a declarative schema (recreated from scratch), it's more maintainable to define the uniqueness directly in the CREATE TABLE daily_activity_logs statement (or as a CREATE UNIQUE INDEX) rather than appending an ALTER TABLE at the end. This keeps table constraints co-located with the table definition and avoids ordering issues when the schema file evolves.
There was a problem hiding this comment.
Acknowledged. This schema.sql is a declarative file that will be recreated from scratch, so moving the constraint inline into the CREATE TABLE would be cleaner. However, since PR #156 (which restructures the schema) is currently open and not yet merged, I've kept the ALTER TABLE approach here to avoid conflicts. Will consolidate inline once #156 is merged.
| 'activity_items': dailyActivity.activityList, | ||
| 'patient_id': dailyActivity.patientId, | ||
| }); | ||
| }, onConflict: 'activity_id, date, patient_id'); |
There was a problem hiding this comment.
onConflict is passed as 'activity_id, date, patient_id' with spaces. PostgREST expects a comma-separated column list (e.g., activity_id,date,patient_id) and the spaces can be interpreted as part of the column names (e.g., date), causing the upsert request to fail at runtime.
| }, onConflict: 'activity_id, date, patient_id'); | |
| }, onConflict: 'activity_id,date,patient_id'); |
There was a problem hiding this comment.
Fixed in the latest commit — removed spaces: onConflict: 'activity_id,date,patient_id'
| 'activity_id': dailyActivity.id, | ||
| 'date': date.toString(), | ||
| 'activity_items': dailyActivity.activityList, | ||
| 'patient_id': dailyActivity.patientId, |
There was a problem hiding this comment.
date is being stored via date.toString() while other queries in the codebase use toIso8601String() and normalize date ranges to midnight. Because the uniqueness/upsert conflict key includes date (TIMESTAMPTZ), differences in timezone/DST or time-of-day can still produce multiple rows for the same calendar day. Consider normalizing to a day boundary (e.g., UTC midnight) before writing, or changing the column to a DATE (or using a generated date::date column) and keying conflicts on that.
There was a problem hiding this comment.
Fixed in the latest commit — normalized to calendar day using DateTime(date.year, date.month, date.day).toIso8601String().substring(0, 10) which produces a YYYY-MM-DD string, ensuring same-day re-saves always hit the same conflict key.
| -- Unique constraint to prevent duplicate activity logs | ||
| ALTER TABLE daily_activity_logs | ||
| ADD CONSTRAINT uq_activity_date_patient | ||
| UNIQUE (activity_id, date, patient_id); |
There was a problem hiding this comment.
Adding a UNIQUE constraint will fail to apply if the database already contains duplicates (which is the scenario this PR is fixing). Consider adding a deduplication step/migration (delete or merge duplicate rows) before creating the constraint, or create the constraint/index in a way that can be validated after cleanup.
There was a problem hiding this comment.
Fixed in the latest commit — added a WITH ranked ... DELETE deduplication step in schema.sql that removes all but the oldest row per (activity_id, date, patient_id) group before the ALTER TABLE runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/schemas/schema.sql`:
- Around line 172-173: The UNIQUE constraint uq_activity_date_patient on
daily_activity_logs uses the TIMESTAMPTZ column date so uniqueness is at the
instant level; change the schema to enforce day-level uniqueness by normalizing
to a DATE value — either alter daily_activity_logs.date to type DATE (and
migrate existing timestamps to their calendar date in UTC) or add a generated
column (e.g., date_day := date_trunc('day', date)::date) and replace the
constraint with one on (activity_id, date_day, patient_id); update any code that
inserts/queries this table to use the normalized date field and create a
migration to backfill and validate existing rows before replacing
uq_activity_date_patient.
- Around line 171-173: Before adding the UNIQUE constraint
uq_activity_date_patient on daily_activity_logs, remove existing duplicate rows
for the key (activity_id, date, patient_id): identify duplicates by grouping on
activity_id, date, patient_id and keep one row per group (e.g., the smallest id
or latest updated_at), delete the others, and only then run ALTER TABLE ... ADD
CONSTRAINT uq_activity_date_patient UNIQUE (activity_id, date, patient_id);
include this deduplication step in the migration so the ALTER will not fail on
environments that already contain duplicates.
In `@therapist/lib/repository/supabase_therapy_repository.dart`:
- Around line 317-322: The upsert is using date.toString(), which includes time
and allows same-day duplicates; normalize the date to a calendar-day
representation before upserting (e.g., construct a date-only value like
YYYY-MM-DD or a DateTime with time zeroed: year/month/day) and use that
normalized value instead of date.toString() in the
_supabaseClient.from('daily_activity_logs').upsert payload (alongside
activity_id, patient_id, activity_items). Also ensure the DB column and the
onConflict constraint ('activity_id, date, patient_id') expect this normalized
day format so the uniqueness check works as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f81ae5-c960-410f-b241-1f7b7af74140
📒 Files selected for processing (2)
supabase/schemas/schema.sqltherapist/lib/repository/supabase_therapy_repository.dart
…p migration before unique constraint
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
therapist/lib/repository/supabase_therapy_repository.dart (2)
284-299:⚠️ Potential issue | 🟡 Minor
_addDailyActivityLogis called withoutawait- errors will be silently lost.Line 299 calls
_addDailyActivityLog(updatedDailyActivity)withoutawait. If the upserts fail, the exception won't propagate toaddOrUpdateDailyActivity, causing silent failures. The method will return success even if logging failed.🔧 Suggested fix
- _addDailyActivityLog(updatedDailyActivity); + await _addDailyActivityLog(updatedDailyActivity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@therapist/lib/repository/supabase_therapy_repository.dart` around lines 284 - 299, The call to _addDailyActivityLog(updatedDailyActivity) inside addOrUpdateDailyActivity is not awaited so any exceptions from logging are lost; change the call to await _addDailyActivityLog(updatedDailyActivity) so the Future is awaited and exceptions propagate to addOrUpdateDailyActivity (and adjust any return flow if needed), ensuring the async method addOrUpdateDailyActivity properly awaits the logging step after the insert/update of DailyActivityResponse.
286-299:⚠️ Potential issue | 🟠 MajorPre-existing bug: activity_id mismatch on update path causes foreign key violation.
On the update path (when
dailyActivity.id.isNotEmpty), line 288 assigns a new UUID toupdatedDailyActivity, but theUPDATEquery on line 291-293 targets the originaldailyActivity.id. The row indaily_activitiesretains its original id, but_addDailyActivityLogreceivesupdatedDailyActivitywith the new UUID. This causesactivity_idindaily_activity_logsto reference a non-existent row, violating the foreign key constraint.While this bug predates this PR, the upsert changes here won't achieve deduplication on re-save until it's fixed.
🐛 Suggested fix: preserve original id on update
try { - final updatedDailyActivity = dailyActivity.copyWith( - therapistId: _supabaseClient.auth.currentUser!.id, - id: const Uuid().v4(), - ); if(dailyActivity.id.isNotEmpty) { + final updatedDailyActivity = dailyActivity.copyWith( + therapistId: _supabaseClient.auth.currentUser!.id, + ); await _supabaseClient.from('daily_activities').update( updatedDailyActivity.toMap() ).eq('id', dailyActivity.id); + await _addDailyActivityLog(updatedDailyActivity); } else { + final updatedDailyActivity = dailyActivity.copyWith( + therapistId: _supabaseClient.auth.currentUser!.id, + id: const Uuid().v4(), + ); await _supabaseClient.from('daily_activities').insert( updatedDailyActivity.toMap() ); + await _addDailyActivityLog(updatedDailyActivity); } - _addDailyActivityLog(updatedDailyActivity); return ActionResultSuccess(data: 'Daily Activity Added Successfully', statusCode: 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@therapist/lib/repository/supabase_therapy_repository.dart` around lines 286 - 299, The update path incorrectly assigns a new UUID to updatedDailyActivity causing activity_id mismatches; change the logic so that when dailyActivity.id.isNotEmpty you preserve the original id (use dailyActivity.id) and only generate const Uuid().v4() for the insert path, while still setting therapistId from _supabaseClient.auth.currentUser!.id; update the code that builds updatedDailyActivity and the branches that call _supabaseClient.from('daily_activities').update(...) / .insert(...) and then call _addDailyActivityLog(updatedDailyActivity) so the logged object's id matches the row in the DB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@therapist/lib/repository/supabase_therapy_repository.dart`:
- Around line 284-299: The call to _addDailyActivityLog(updatedDailyActivity)
inside addOrUpdateDailyActivity is not awaited so any exceptions from logging
are lost; change the call to await _addDailyActivityLog(updatedDailyActivity) so
the Future is awaited and exceptions propagate to addOrUpdateDailyActivity (and
adjust any return flow if needed), ensuring the async method
addOrUpdateDailyActivity properly awaits the logging step after the
insert/update of DailyActivityResponse.
- Around line 286-299: The update path incorrectly assigns a new UUID to
updatedDailyActivity causing activity_id mismatches; change the logic so that
when dailyActivity.id.isNotEmpty you preserve the original id (use
dailyActivity.id) and only generate const Uuid().v4() for the insert path, while
still setting therapistId from _supabaseClient.auth.currentUser!.id; update the
code that builds updatedDailyActivity and the branches that call
_supabaseClient.from('daily_activities').update(...) / .insert(...) and then
call _addDailyActivityLog(updatedDailyActivity) so the logged object's id
matches the row in the DB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59479e4f-39fb-4d87-9f6b-ccc66e04d64f
📒 Files selected for processing (2)
supabase/schemas/schema.sqltherapist/lib/repository/supabase_therapy_repository.dart
…og to prevent FK violation and silent errors
Closes #184
📝 Description
The daily activity log insert loop in
supabase_therapy_repository.dartunconditionally inserted a new row into
daily_activity_logsfor everydate in the activity's range with no deduplication check. Re-saving or
editing a daily activity created duplicate logs for the same
activity_id+date+patient_idcombination.This was also the root cause of the
PGRST116crash fixed in #167 —duplicate logs for the same day caused
.maybeSingle()to throw"Results contain more than one row".
🔧 Changes Made
therapist/lib/repository/supabase_therapy_repository.dart: Replaced.insert()with.upsert(onConflict: 'activity_id, date, patient_id')so re-saving an activity updates existing logs instead of creating
duplicates.
supabase/schemas/schema.sql: Added aUNIQUEconstraint on(activity_id, date, patient_id)to enforce deduplication at thedatabase level as a defence-in-depth measure.
📷 Screenshots or Visual Changes (if applicable)
N/A — data integrity fix, no visual changes.
🤝 Collaboration
Collaborated with: N/A
✅ Checklist
Summary by CodeRabbit