chore: Local environment + UX #163
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the seeder to use per-row SAVEPOINTs for COPY inserts with centralized duplicate-skip logic; updates tests to expect per-row savepoints; restructures Caddy routing to a /api/* handler and moves CORS to Caddy; removes rs/cors from go.mod and Go-level CORS; adjusts docker-compose ports and renames Makefile Caddy commands. Changes
Sequence Diagram(s)sequenceDiagram
participant Importer as COPY Importer
participant DB as Database
participant Checker as Duplicate Checker
loop each COPY row
Importer->>DB: CREATE SAVEPOINT importer_copy_row_X
Importer->>DB: INSERT row
alt insert succeeds
Importer->>DB: RELEASE SAVEPOINT importer_copy_row_X
else insert fails
Importer->>Checker: shouldSkipCopyInsertError(table, err)
alt should skip (duplicate)
Importer->>DB: ROLLBACK TO SAVEPOINT importer_copy_row_X
Importer->>DB: RELEASE SAVEPOINT importer_copy_row_X
Checker->>Importer: log skip, continue
else hard error
Importer->>DB: ROLLBACK TO SAVEPOINT importer_copy_row_X
Importer->>Importer: return error
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-17T09:16:09.377ZApplied to files:
🔇 Additional comments (2)
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.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
Makefile(1 hunks)database/seeder/importer/runner.go(3 hunks)database/seeder/importer/runner_sql_test.go(4 hunks)database/seeder/importer/runner_test.go(1 hunks)docker-compose.yml(1 hunks)go.mod(0 hunks)infra/caddy/Caddyfile.local(2 hunks)infra/caddy/Caddyfile.prod(1 hunks)pkg/endpoint/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
database/seeder/importer/runner_test.go (1)
database/seeder/importer/runner.go (1)
SeedFromFile(22-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.25.3)
🔇 Additional comments (12)
database/seeder/importer/runner_test.go (1)
409-441: LGTM! Well-structured test for COPY duplicate handling.The test properly validates idempotent behavior for COPY operations by:
- Including duplicate rows (version 42) in the COPY data
- Running the seed process twice to verify cross-execution idempotency
- Asserting that only a single migration row persists
This mirrors the INSERT-based test effectively and ensures the per-row savepoint logic works correctly.
database/seeder/importer/runner_sql_test.go (2)
112-118: LGTM! Test expectations correctly reflect per-row savepoint handling.The test now expects savepoint creation and release around each COPY row insert, which aligns with the implementation changes.
227-272: LGTM! Comprehensive test for duplicate handling in COPY operations.The test validates the per-row savepoint rollback and skip logic:
- First row inserts successfully with savepoint create/release
- Second duplicate row triggers error, rollback to savepoint, and release
- The COPY operation completes without error despite the duplicate
This correctly exercises the new error handling path in
executeCopy.database/seeder/importer/runner.go (3)
646-678: LGTM! Per-row savepoint handling is correctly implemented.The implementation properly handles duplicate rows during COPY operations:
- Creates a savepoint before each row insert
- On error: checks if the error should be skipped, rolls back to the savepoint, releases it, and continues
- On success: releases the savepoint
This ensures idempotent behavior for COPY operations, particularly for
schema_migrationsduplicates.
731-752: LGTM! Well-designed helper functions for duplicate detection.The new helpers centralize duplicate-skip logic:
shouldSkipCopyInsertErrorhandles COPY-specific error checking (23505 duplicate key)shouldSkipDuplicateSchemaMigrationInsertis shared between COPY and INSERT paths- Proper identifier normalization ensures consistent behavior
This refactoring eliminates code duplication and improves maintainability.
714-716: LGTM! Good refactoring to eliminate code duplication.The refactoring consolidates duplicate-migration skip logic into a shared helper, ensuring consistency between INSERT and COPY execution paths.
Makefile (1)
106-107: LGTM: Consistent naming with plural form.The help text updates correctly reflect plural naming (certs instead of cert), improving consistency.
docker-compose.yml (1)
398-400: LGTM: Helpful documentation.The added comments clearly explain the port exposure strategy for local versus production environments, improving configuration clarity.
infra/caddy/Caddyfile.local (2)
45-54: LGTM: API path handling properly structured.The new
handle_path /api/*block correctly:
- Strips the
/apiprefix before forwarding to the backend- Blocks access to protected paths like
/metrics- Proxies requests to the API service
This architecture aligns with the production configuration and provides consistent routing behavior across environments.
62-75: LGTM: Dedicated metrics endpoint for Prometheus.The
:9180listener correctly exposes only the/metricsendpoint by proxying to the admin API (localhost:2019), with a fallback 404 for all other paths. This provides secure, isolated metrics scraping for Prometheus without exposing the full admin API.pkg/endpoint/server.go (1)
84-98: LGTM: CORS properly delegated to Caddy and fully configured.Verification confirms all required CORS scenarios are properly configured in both Caddy files:
- Preflight handling:
@preflightmatchers and handlers present in both Caddyfile.local and Caddyfile.prod- Origin validation: Hardcoded per environment (localhost:5173 for local, oullin.io for prod) with dynamic fallback
- Methods & headers: GET, POST, PUT, DELETE, OPTIONS allowed; all custom headers configured
The application-level delegation to Caddy is complete and appropriate.
infra/caddy/Caddyfile.prod (1)
4-4: Global metrics directive syntax is valid.The global
metricsdirective is valid Caddy v2 syntax and can be enabled at the top-level in a global options block. The change moves metrics configuration to global scope, which is a documented and supported pattern in Caddy v2. No syntax issues identified.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
gocanto
force-pushed
the
fix/db-importter
branch
from
5473b46 to
c34a096
Compare
Summary by CodeRabbit
Bug Fixes
Chores
Refactor