fix(import): abort commit on stale schedule diff - #484
Open
chiptus wants to merge 3 commits into
Open
Conversation
diff-schedule now computes a watermark over the edition's sets and returns it with the plan; the client threads it through unchanged to commit-schedule, which re-validates it inside the commit transaction and aborts (applying nothing) if the edition changed since Analyse, instead of silently applying a stale plan. Closes #42
…x its ordering - Prefix commit_schedule's watermark-mismatch exception with a stable edition_changed_since_analyse marker, and have the review step key off it to show a dedicated "start over, re-run Analyse" message instead of a generic failure with a doomed Retry button. - Compute diff-schedule's watermark before (not alongside, via Promise.all) the sets read it certifies, so a concurrent edit can no longer land in the gap between the two reads and slip past the Commit-time re-check.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Deploy →
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The watermark computation currently depends on timestamptz text formatting, which can vary by session timezone and lead to false commit aborts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the schedule import flow against stale Analyse-time diffs by introducing an optimistic-concurrency “watermark” that is captured during Analyse, round-tripped through the client, and re-validated inside the commit_schedule transaction before applying any changes.
Changes:
- Add a DB-side watermark computation function and require a matching watermark parameter in
commit_schedule, aborting the transaction on mismatch. - Thread the watermark through the
diff-scheduleandcommit-scheduleEdge Functions and the frontend schedule import service payload. - Update the admin UI review step to show a dedicated “schedule changed” message and disable Commit, with new unit/UI tests covering the behavior.
File summaries
| File | Description |
|---|---|
| supabase/migrations/20260901000000_commit_schedule_watermark.sql | Adds watermark computation + enforces watermark re-check inside commit_schedule. |
| supabase/functions/diff-schedule/types.ts | Extends Analyse response type to include the watermark. |
| supabase/functions/diff-schedule/index.ts | Fetches watermark via RPC and includes it in the response. |
| supabase/functions/commit-schedule/index.ts | Requires watermark in request schema and forwards it to commit_schedule. |
| supabase/functions/commit-schedule/commit-schedule.test.ts | Updates tests to supply watermark; adds stale-watermark abort test. |
| src/services/scheduleImport/types.ts | Updates client schemas/types and adds marker detection helper for watermark mismatch. |
| src/services/scheduleImport/types.test.ts | Tests the watermark-mismatch marker detection helper. |
| src/services/scheduleImport/buildCommitPayload.ts | Includes watermark in the commit payload built from the diff result. |
| src/services/scheduleImport/buildCommitPayload.test.ts | Tests that watermark is passed through unchanged. |
| src/components/Admin/ScheduleImport/DiffReviewStep.tsx | Shows dedicated “schedule changed” error UI and disables Commit on watermark mismatch. |
| src/components/Admin/ScheduleImport/DiffReviewStep.test.tsx | Verifies the dedicated message + disabled Commit behavior vs generic failures. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Playwright test results
Details
|
- Split DiffPlan (computeDiff's pure-function output) from DiffResult (DiffPlan + watermark) so computeDiff.ts type-checks again — it has no DB access and can't produce the watermark itself; index.ts still merges it in before responding. This is what broke the "Run Edge Function Tests" CI job (Deno type-checks tests before running them). - Compute the watermark via EXTRACT(EPOCH FROM ...) instead of a plain ::TEXT cast, per Copilot review: the text cast depends on the session TimeZone GUC, so Analyse and Commit could see different strings for the same value; epoch extraction on a timestamptz doesn't. - Capture and assert the setup insert's error/data in the new stale- watermark test instead of silently swallowing it, per Copilot review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
commit_schedule applied a stale Analyse-time diff with no re-validation at Commit, so a concurrent edit made after Analyse could be silently overridden (most visibly,
setIdsToArchivearchiving sets based on an orphan list that had gone stale). commit-schedule now re-checks a watermark inside the commit transaction and aborts, applying nothing, if the edition changed since Analyse.Closes #42
Verification
pnpm run typecheck,pnpm run lint,pnpm exec vitest run, andpnpm run buildall pass.Generated by Claude Code