Use persistent staging for Apple Health imports#197
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR replaces temporary-table staging for Apple Health XML imports with persistent staging tables ( ChangesPersistent Import Staging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant ImportSession
participant StageTables as StageTables (import_stage_points/workouts)
participant PromotionTx
participant ImportRuns
Parser->>ImportSession: AddPoints/AddWorkouts
ImportSession->>StageTables: CopyFrom (import_run_id)
Parser->>ImportSession: Commit()
ImportSession->>PromotionTx: db.pool.Begin()
PromotionTx->>StageTables: promoteCoverage/promotePoints/promoteWorkouts
PromotionTx->>ImportRuns: markCommitted(tx)
PromotionTx->>StageTables: deleteStageRows(tx)
PromotionTx-->>ImportSession: Commit()
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be6f1b903f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := s.cleanupAbandonedImportStages(ctx, abandonedRunningImportAge); err != nil { | ||
| return nil, fmt.Errorf("cleanup import staging: %w", err) |
There was a problem hiding this comment.
When the standalone cmd/import path is run against a schema that has not already been migrated by server startup, BeginAppleHealthXMLImport now calls cleanup before doing anything else, and that cleanup references the new import_stage_* tables. I checked the import CLI path: it opens storage.New but does not call EnsureIndexes, so a fresh/just-upgraded CLI import can fail immediately with relation "import_stage_points" does not exist instead of creating the new persistent staging tables. Ensure the stage-table DDL is run in the CLI/Begin path before this cleanup.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/storage/import_run.go (1)
174-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated rollback/fail/cleanup block in
Commit().The 4-line
tx.Rollback→markImportFailedWithCounters→cleanupImportStageRows→returnsequence is duplicated verbatim across all four error branches (promoteCoverage, promotePoints, promoteWorkouts, markCommitted, deleteStageRows — 5 occurrences). A future change to this failure path (e.g., adding a log line, changing arguments) risks being applied inconsistently. Also, the errors frommarkImportFailedWithCounters/cleanupImportStageRowsare silently discarded (_ =), so a failure in the failure-handling path itself is invisible.♻️ Proposed refactor
+func (s *ImportSession) failCommit(ctx context.Context, tx pgx.Tx, cause error) (ImportCounters, error) { + _ = tx.Rollback(ctx) + if err := s.db.markImportFailedWithCounters(s.runID, cause.Error(), s.counters); err != nil { + log.Printf("mark import %d failed: %v", s.runID, err) + } + if err := s.db.cleanupImportStageRows(s.runID); err != nil { + log.Printf("cleanup stage rows for import %d: %v", s.runID, err) + } + return s.counters, cause +} + func (s *ImportSession) Commit() (ImportCounters, error) { ... if err := s.promoteCoverage(ctx, tx); err != nil { - _ = tx.Rollback(ctx) - _ = s.db.markImportFailedWithCounters(s.runID, err.Error(), s.counters) - _ = s.db.cleanupImportStageRows(s.runID) - return s.counters, err + return s.failCommit(ctx, tx, err) } // ...repeat for promotePoints, promoteWorkouts, markCommitted, deleteStageRows🤖 Prompt for 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. In `@internal/storage/import_run.go` around lines 174 - 221, Refactor the duplicated failure path in ImportSession.Commit by extracting the repeated tx.Rollback, markImportFailedWithCounters, cleanupImportStageRows, and return sequence into a single helper used by the promoteCoverage, promotePoints, promoteWorkouts, markCommitted, and deleteStageRows error branches. Make sure the helper preserves the current behavior while centralizing any future changes, and consider surfacing or at least logging errors from markImportFailedWithCounters and cleanupImportStageRows instead of discarding them so failures in the error-handling path are visible.
🤖 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 `@internal/storage/import_run.go`:
- Around line 65-68: The cleanup call in BeginAppleHealthXMLImport currently
turns abandoned-stage cleanup failures into a hard error, which blocks starting
new imports. Make the cleanup best-effort here, matching EnsureIndexes, by
handling the error from cleanupAbandonedImportStages with logging only and
continuing the import flow instead of returning fmt.Errorf. Keep the existing
cleanupAbandonedImportStages call and adjust the surrounding error handling so
transient failures do not abort BeginAppleHealthXMLImport.
- Around line 184-213: The rollback paths in the import transaction flow are
using the request context, so if ctx is canceled or times out the transaction
rollback may not execute cleanly. Update the rollback calls in the
promoteCoverage, promotePoints, promoteWorkouts, markCommitted, and
deleteStageRows error branches in importRun to use a cancellation-independent
context such as context.Background(), matching the approach used by
execStartupDDL, while keeping the existing failure marking and cleanup behavior
unchanged.
---
Nitpick comments:
In `@internal/storage/import_run.go`:
- Around line 174-221: Refactor the duplicated failure path in
ImportSession.Commit by extracting the repeated tx.Rollback,
markImportFailedWithCounters, cleanupImportStageRows, and return sequence into a
single helper used by the promoteCoverage, promotePoints, promoteWorkouts,
markCommitted, and deleteStageRows error branches. Make sure the helper
preserves the current behavior while centralizing any future changes, and
consider surfacing or at least logging errors from markImportFailedWithCounters
and cleanupImportStageRows instead of discarding them so failures in the
error-handling path are visible.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7df2661-f4f5-4d33-9f1c-a84e4df0fc8b
📒 Files selected for processing (11)
AGENTS.mdCLAUDE.mdREADME.mddocs/ai-plans/2026-07-03-healthkit-import-dedup-batching.htmldocs/ai-plans/2026-07-05-healthkit-persistent-import-staging.htmlinternal/storage/db.gointernal/storage/import_run.gointernal/storage/import_run_test.gointernal/storage/import_schema.gointernal/storage/indexes.gointernal/storage/indexes_test.go
1 participant
Summary
import_run_id-keyed staging tables.Test Plan
HEALTH_DB_TESTS=1 GOCACHE=/tmp/health-go-build-cache go test ./internal/storage -run "TestAppleHealthXMLImport|TestEnsureIndexesIntegration" -count=1 -vGOCACHE=/tmp/health-go-build-cache go test ./internal/storage -run 'AppleHealthXMLImport|Import|Bulk|Workout|EnsureIndexesIntegration' -count=1GOCACHE=/tmp/health-go-build-cache go test ./internal/applehealth ./cmd/import ./internal/ui -run 'ExportDate|Import' -count=1GOCACHE=/tmp/health-go-build-cache make test-unitSummary by CodeRabbit
New Features
Bug Fixes
Documentation