Added e2e test heatmap and coverage dashboard - #10
Merged
Conversation
added e2e test heatmap and coverage dashboard
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements end-to-end test run management: domain types and migrations, Postgres repositories, application use-cases (ingest/list/compare/get/heatmap), HTTP handlers/routes, CLI upload and Playwright normalization, frontend /e2e console and assets, and wiring in bootstrap/api. ChangesEnd-to-End Test Run Management Feature
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant IngestUC as IngestE2ERunUseCase
participant ProjectRepo as ProjectRepository
participant RunRepo as E2ETestRunRepository
participant SpecRepo as E2ESpecResultRepository
participant TX as TransactionManager
Client->>IngestUC: Execute(input)
IngestUC->>IngestUC: Validate input
IngestUC->>ProjectRepo: Get or Create project
ProjectRepo-->>IngestUC: project
IngestUC->>IngestUC: Build E2ETestRun & E2ESpecResult entities
IngestUC->>TX: Begin transaction
IngestUC->>RunRepo: Create E2ETestRun
RunRepo-->>IngestUC: run (with ID)
IngestUC->>SpecRepo: CreateBatch specs
SpecRepo-->>IngestUC: success
IngestUC->>TX: Commit
IngestUC->>RunRepo: Get baseline (latest on default branch)
RunRepo-->>IngestUC: baseline
IngestUC->>SpecRepo: List failed specs for comparison
SpecRepo-->>IngestUC: failed specs
IngestUC->>IngestUC: Compute deltas & pass rates
IngestUC-->>Client: IngestE2ERunOutput
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
internal/adapters/postgres/e2e_test_run_repository.go (1)
74-83: 💤 Low valueRedundant type cast on platform field.
The
platformcolumn is alreadyTEXTin the schema, soCOALESCE(platform::text, '')can be simplified toCOALESCE(platform, ''). The same pattern appears on lines 123 and 170.♻️ Optional simplification
- COALESCE(framework_version, ''), COALESCE(test_framework, ''), COALESCE(platform::text, ''), suite_description, suite_path, total_specs, passed_specs, + COALESCE(framework_version, ''), COALESCE(test_framework, ''), COALESCE(platform, ''), suite_description, suite_path, total_specs, passed_specs,Apply the same change on lines 123 and 170.
🤖 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/adapters/postgres/e2e_test_run_repository.go` around lines 74 - 83, The SQL queries in e2e_test_run_repository (the QueryRow and other query blocks that select platform) use a redundant platform::text cast; replace COALESCE(platform::text, '') with COALESCE(platform, '') in the query inside the function that calls q.QueryRow(...) (the e2e test run select), and apply the same change to the other two occurrences found later in the file (the other SELECTs around the blocks referenced at lines ~123 and ~170) so the platform column is coalesced without the unnecessary ::text cast.internal/adapters/postgres/e2e_spec_result_repository.go (1)
19-49: ⚖️ Poor tradeoffPrefer batch INSERT over loop of individual INSERTs.
The current implementation executes N individual
INSERTstatements. A single multi-rowINSERT(or pgxCopyFrom) would be more efficient and reduce round-trips.Example batch insert pattern
func (r *E2ESpecResultRepository) CreateBatch(ctx context.Context, specs []domain.E2ESpecResult) error { if len(specs) == 0 { return nil } q := getQuerier(ctx, r.pool) // Build multi-row INSERT values := make([]string, 0, len(specs)) args := make([]any, 0, len(specs)*9) for i, spec := range specs { offset := i * 9 values = append(values, fmt.Sprintf("($%d, $%d, $%d, $%d, $%d, $%d, $%d, $%d, $%d)", offset+1, offset+2, offset+3, offset+4, offset+5, offset+6, offset+7, offset+8, offset+9)) args = append(args, spec.ID, spec.E2ETestRunID, spec.SpecPath, spec.LeafNodeText, spec.State, spec.DurationMS, spec.FailureMessage, spec.FailureLocationFile, spec.FailureLocationLine, ) } sql := fmt.Sprintf(` INSERT INTO e2e_test_spec_results ( id, e2e_run_id, spec_path, leaf_node_text, state, duration_ms, failure_message, failure_location_file, failure_location_line ) VALUES %s `, strings.Join(values, ", ")) if _, err := q.Exec(ctx, sql, args...); err != nil { return fmt.Errorf("insert e2e spec results: %w", err) } return nil }Alternatively, consider
pgx.CopyFromfor even better performance with large batches.🤖 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/adapters/postgres/e2e_spec_result_repository.go` around lines 19 - 49, The CreateBatch method on E2ESpecResultRepository currently issues one INSERT per spec causing N round-trips; replace the loop in CreateBatch (which uses getQuerier and q.Exec) with a single multi-row INSERT or use pgx.CopyFrom: build a values slice of "(...)" placeholders for each spec, construct an args []any containing spec fields in the correct order (ID, E2ETestRunID, SpecPath, LeafNodeText, State, DurationMS, FailureMessage, FailureLocationFile, FailureLocationLine), join values into the SQL VALUES clause, then call q.Exec(ctx, sql, args...) and wrap/return any error (update the error message to "insert e2e spec results: %w"); alternatively implement pgx.CopyFrom for large batches for better perf.migrations/003_e2e_test_runs.sql (1)
51-52: ⚡ Quick winConsider a composite index on (e2e_run_id, state).
The standalone index on
statehas low selectivity (only 5 possible values) and may not be effective. Queries typically filter specs by state within a specific run, so a composite index(e2e_run_id, state)would better support that pattern and could replace both existing indexes.♻️ Proposed refactor for more efficient indexing
-CREATE INDEX IF NOT EXISTS e2e_test_spec_results_run_id_idx ON e2e_test_spec_results(e2e_run_id); -CREATE INDEX IF NOT EXISTS e2e_test_spec_results_state_idx ON e2e_test_spec_results(state); +CREATE INDEX IF NOT EXISTS e2e_test_spec_results_run_state_idx + ON e2e_test_spec_results(e2e_run_id, state);🤖 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 `@migrations/003_e2e_test_runs.sql` around lines 51 - 52, Replace the low-selectivity single-column state index with a composite index that matches common query patterns: create an index e2e_test_spec_results_run_id_state_idx on e2e_test_spec_results(e2e_run_id, state) and remove the standalone state index e2e_test_spec_results_state_idx (you can keep the run_id index e2e_test_spec_results_run_id_idx only if other queries need it); this ensures queries filtering by e2e_run_id and state use the composite index for better selectivity and performance.cmd/coveragecli/main.go (3)
571-571: ⚡ Quick winUse camelCase for local variables.
The variable
framework_versionuses snake_case. Go convention prefers camelCase for local variables.♻️ Proposed fix
func normalizePlaywrightReport(raw map[string]any) map[string]any { var suiteDescription string var suitePath string - var framework_version string + var frameworkVersion string result := make(map[string]any) testFramework := "playwright" config := firstMap(raw, "config") suites := firstSlice(raw, "suites") if config != nil { suitePath = firstString(config, "rootDir") - framework_version = firstString(config, "version") + frameworkVersion = firstString(config, "version") } if len(suites) > 0 { if first, ok := suites[0].(map[string]any); ok { suiteDescription = firstString(first, "title") } } result["suiteDescription"] = suiteDescription result["suitePath"] = suitePath result["reportType"] = &testFramework result["testFramework"] = &testFramework - result["frameworkVersion"] = framework_version + result["frameworkVersion"] = frameworkVersion result["platformType"] = "web"🤖 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 `@cmd/coveragecli/main.go` at line 571, The local variable framework_version uses snake_case instead of Go's camelCase convention; rename the identifier to frameworkVersion everywhere it's declared and referenced (look for the variable symbol framework_version in main.go, e.g., within the function scope where it is declared) and update all usages to the new name to maintain consistency and compile-time correctness.
592-592: ⚡ Quick winRemove redundant platformType assignment.
Line 592 hardcodes
platformTypeto"web", but line 486 overwrites it with the--platform-typeflag value. The hardcoded assignment is redundant and could cause confusion during maintenance.♻️ Proposed fix
result["suiteDescription"] = suiteDescription result["suitePath"] = suitePath result["reportType"] = &testFramework result["testFramework"] = &testFramework result["frameworkVersion"] = frameworkVersion - result["platformType"] = "web"🤖 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 `@cmd/coveragecli/main.go` at line 592, Remove the redundant hardcoded assignment result["platformType"] = "web" — the platformType is already set from the --platform-type flag (the platformType variable or earlier assignment around where result is populated), so delete that line in main (or the function that builds the result map) to avoid confusion and rely on the existing flag-derived value.
477-477: ⚡ Quick winRename variable for clarity.
The variable
normalizeReportholds a map result, not a function. Consider renaming tonormalizedReportto avoid confusion with the normalization functions below.♻️ Proposed fix
- var normalizeReport map[string]any + var normalizedReport map[string]any switch *reportType { case "playwright": - normalizeReport = normalizePlaywrightReport(report) + normalizedReport = normalizePlaywrightReport(report) case "appium": - normalizeReport = normalizeAppiumReport(report) + normalizedReport = normalizeAppiumReport(report) default: exitErr("validate input", fmt.Errorf("unsupported report type: %s", *reportType)) } - normalizeReport["platformType"] = *platformType + normalizedReport["platformType"] = *platformType payload := e2ePayload{ ProjectKey: *projectKey, ProjectName: *projectName, ProjectGroup: group, DefaultBranch: *defaultBranch, Branch: *branch, CommitSHA: *commitSHA, Author: *author, TriggerType: *triggerType, RunTimestamp: *runTimestamp, Environment: env, - TestReport: normalizeReport, + TestReport: normalizedReport, }🤖 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 `@cmd/coveragecli/main.go` at line 477, The local variable named normalizeReport is misleading because it suggests a function; rename it to normalizedReport everywhere it's declared and used in main.go (the variable declared as "var normalizeReport map[string]any") and update all references to match, ensuring you don't collide with any normalization function names (e.g., any functions named normalizeReport or normalize*). Adjust any subsequent code that reads/writes this map to use normalizedReport so identifiers remain clear and unambiguous.
🤖 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 `@cmd/frontend/web/assets/e2e.js`:
- Around line 638-646: The current UI computes pass rate as (passedRuns /
failedRuns) * 100 which produces misleading values (e.g., 900%); update the
logic around passedRuns, failedRuns, and e2ePassRate so you compute total =
passedRuns + failedRuns, return '-' if total === 0, otherwise set
e2ePassRate.textContent = `${((passedRuns / total) * 100).toFixed(2)}%` (this
makes 18 passed + 2 failed = 90.00%), and remove the special-case that outputs
'∞%' when failedRuns === 0; locate the calculation block referencing passedRuns,
failedRuns and e2ePassRate to implement this change.
- Around line 620-621: The platform query parameter being set in e2e.js is not
supported by the backend ListE2ERunsInput struct in
internal/application/e2e_usecase.go, causing the platform filter dropdown to be
ignored. To fix this, add a Platform field to the ListE2ERunsInput struct to
accept the platform parameter from the query string, update the repository query
method to filter by this platform field when it is provided, and ensure the
platform value flows through the entire chain from the HTTP handler through the
use case to the repository layer where the actual filtering logic is applied.
In `@internal/adapters/http/handlers.go`:
- Around line 177-181: Remove the premature slog.Info call that logs the
zero-value variable `in` (the slog.Info call with "name":"ingest_e2e_run" and
"stage":"decoding_input"); either delete that log line or move it to immediately
after the request body is decoded into `in` (e.g., after the call that decodes
into `in` succeeds) so that you only log the populated `in` and handle/avoid
logging on decode errors.
In `@internal/application/e2e_usecase_test.go`:
- Around line 1056-1084: In TestGetE2EHeatmapExecute remove the leftover debug
print by deleting the fmt.Println(out.Groups) statement inside the test; if you
need to record output for diagnostics use t.Logf or t.Log instead, but do not
leave fmt.Println in the TestGetE2EHeatmapExecute function (which constructs
NewGetE2EHeatmapUseCase and calls uc.Execute).
In `@internal/application/e2e_usecase.go`:
- Around line 501-520: The environment validation in ListE2ERunsUseCase.Execute
currently rejects "none" but the repository (e2e_test_run_repository.go) treats
"none" as a valid special filter for NULL environments; update the validation in
ListE2ERunsUseCase.Execute to accept "none" in addition to "test", "stage", and
"prod" and adjust the NewInvalidArgument message to reflect the allowed values
(e.g., "one of: test, stage, prod, none") so callers can filter for runs with no
environment set.
In `@migrations/003_e2e_test_runs.sql`:
- Line 45: Add a CHECK constraint on the spec result column duration_ms to
prevent negative values by altering the CREATE TABLE (or adding an ALTER TABLE)
in migrations/003_e2e_test_runs.sql so that the duration_ms BIGINT column
includes a non-negative check (e.g., ensure duration_ms >= 0) and give the
constraint a clear name (e.g., chk_spec_result_duration_ms_nonnegative) so it is
enforceable and identifiable.
- Around line 54-58: The rollback (Down) block contains duplicate DROP
statements for the tables e2e_test_spec_results and e2e_test_runs; remove the
redundant lines so each table is dropped only once (keep a single "DROP TABLE IF
EXISTS e2e_test_spec_results;" and a single "DROP TABLE IF EXISTS
e2e_test_runs;" in the Down section) to clean up the migration and avoid
copy-paste duplication.
- Line 24: Add a CHECK constraint to ensure duration_ms is non-negative: update
the migration that creates the e2e_test_runs table (column duration_ms) to
include a CHECK constraint such as CHECK (duration_ms >= 0) either inline on the
duration_ms column or as a table-level constraint; reference the duration_ms
column in the e2e_test_runs creation statement and add the constraint so
negative duration values cannot be inserted.
- Line 13: Add a CHECK constraint enforcing platform IN ('web','android','ios')
on the platform column (platform) of the e2e_test_runs table: update the column
definition in the migration (or add an ALTER TABLE ... ADD CONSTRAINT) to
include a CHECK that only allows 'web', 'android', or 'ios' values so invalid
platform strings are rejected at the DB level.
- Around line 16-21: Add SQL CHECK constraints to enforce non-negative values
and consistency: ensure total_specs, passed_specs, failed_specs, skipped_specs,
flaked_specs, and pending_specs are >= 0 and add a CHECK that passed_specs +
failed_specs + skipped_specs + flaked_specs + pending_specs = total_specs.
Modify the CREATE TABLE statement (or add an ALTER TABLE) to include checks like
CHECK (total_specs >= 0) and CHECK (passed_specs >= 0) ... for each column and a
single CHECK (passed_specs + failed_specs + skipped_specs + flaked_specs +
pending_specs = total_specs) referencing the existing column names total_specs,
passed_specs, failed_specs, skipped_specs, flaked_specs, pending_specs.
---
Nitpick comments:
In `@cmd/coveragecli/main.go`:
- Line 571: The local variable framework_version uses snake_case instead of Go's
camelCase convention; rename the identifier to frameworkVersion everywhere it's
declared and referenced (look for the variable symbol framework_version in
main.go, e.g., within the function scope where it is declared) and update all
usages to the new name to maintain consistency and compile-time correctness.
- Line 592: Remove the redundant hardcoded assignment result["platformType"] =
"web" — the platformType is already set from the --platform-type flag (the
platformType variable or earlier assignment around where result is populated),
so delete that line in main (or the function that builds the result map) to
avoid confusion and rely on the existing flag-derived value.
- Line 477: The local variable named normalizeReport is misleading because it
suggests a function; rename it to normalizedReport everywhere it's declared and
used in main.go (the variable declared as "var normalizeReport map[string]any")
and update all references to match, ensuring you don't collide with any
normalization function names (e.g., any functions named normalizeReport or
normalize*). Adjust any subsequent code that reads/writes this map to use
normalizedReport so identifiers remain clear and unambiguous.
In `@internal/adapters/postgres/e2e_spec_result_repository.go`:
- Around line 19-49: The CreateBatch method on E2ESpecResultRepository currently
issues one INSERT per spec causing N round-trips; replace the loop in
CreateBatch (which uses getQuerier and q.Exec) with a single multi-row INSERT or
use pgx.CopyFrom: build a values slice of "(...)" placeholders for each spec,
construct an args []any containing spec fields in the correct order (ID,
E2ETestRunID, SpecPath, LeafNodeText, State, DurationMS, FailureMessage,
FailureLocationFile, FailureLocationLine), join values into the SQL VALUES
clause, then call q.Exec(ctx, sql, args...) and wrap/return any error (update
the error message to "insert e2e spec results: %w"); alternatively implement
pgx.CopyFrom for large batches for better perf.
In `@internal/adapters/postgres/e2e_test_run_repository.go`:
- Around line 74-83: The SQL queries in e2e_test_run_repository (the QueryRow
and other query blocks that select platform) use a redundant platform::text
cast; replace COALESCE(platform::text, '') with COALESCE(platform, '') in the
query inside the function that calls q.QueryRow(...) (the e2e test run select),
and apply the same change to the other two occurrences found later in the file
(the other SELECTs around the blocks referenced at lines ~123 and ~170) so the
platform column is coalesced without the unnecessary ::text cast.
In `@migrations/003_e2e_test_runs.sql`:
- Around line 51-52: Replace the low-selectivity single-column state index with
a composite index that matches common query patterns: create an index
e2e_test_spec_results_run_id_state_idx on e2e_test_spec_results(e2e_run_id,
state) and remove the standalone state index e2e_test_spec_results_state_idx
(you can keep the run_id index e2e_test_spec_results_run_id_idx only if other
queries need it); this ensures queries filtering by e2e_run_id and state use the
composite index for better selectivity and performance.
🪄 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: bdd96296-2a7d-4b04-bd10-3d06fff09d94
📒 Files selected for processing (20)
cmd/api/main.gocmd/coveragecli/main.gocmd/coveragecli/main_test.gocmd/coveragecli/testdata/playwright-report-fail-dummy.jsoncmd/coveragecli/testdata/playwright-report-pass-dummy.jsoncmd/frontend/main.gocmd/frontend/web/assets/e2e.jscmd/frontend/web/e2eTest.htmlcmd/frontend/web/index.htmlinternal/adapters/http/handlers.gointernal/adapters/http/router.gointernal/adapters/postgres/e2e_spec_result_repository.gointernal/adapters/postgres/e2e_test_run_repository.gointernal/adapters/postgres/integration_test_run_repository.gointernal/application/e2e_usecase.gointernal/application/e2e_usecase_test.gointernal/application/mock_application.gointernal/application/ports.gointernal/domain/e2e.gomigrations/003_e2e_test_runs.sql
E2e report ingestion
arxdsilva
reviewed
Jun 8, 2026
arxdsilva
reviewed
Jun 8, 2026
arxdsilva
reviewed
Jun 8, 2026
arxdsilva
reviewed
Jun 8, 2026
arxdsilva
reviewed
Jun 8, 2026
arxdsilva
left a comment
arxdsilva
left a comment
Owner
There was a problem hiding this comment.
thanks @RaihanSultana for the contribution, lets just fix these small things and I'll merge it
replaced logs with slog and added check in e2e migration file
removed duplicate table drop query
Contributor
Author
|
Hey @arxdsilva, resolved the above pr commented. Replaced the print statements with log/slog and added constraints for platform field in the e2e migration file. Thanks! |
moved check constraint from the table
There was a problem hiding this comment.
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 `@internal/platform/bootstrap/bootstrap.go`:
- Around line 41-54: Wrap returned errors with context using fmt.Errorf to
indicate which startup step failed: when migrations.Up(ctx, cfg.DatabaseURL,
cfg.MigrationsDir) returns err, replace the bare return with returning
fmt.Errorf("migrations up failed: %w", err) (and still close pool before
returning); when pool.Ping(ctx) returns err, return fmt.Errorf("database ping
failed: %w", err); also add fmt to the imports. Ensure all returns that
previously returned err directly now wrap the error with a descriptive message
referencing the failing operation (e.g., "migrations up failed" and "database
ping failed").
- Around line 40-54: The DB startup code uses raw ctx for pgxpool.New,
migrations.Up and pool.Ping which can hang; wrap each DB operation in a context
with a sensible timeout (e.g. context.WithTimeout(ctx, <reasonable duration>))
and ensure defer cancel() is called, then pass the timeoutted context into
pgxpool.New, migrations.Up(cfg.DatabaseURL, cfg.MigrationsDir, ...) and
pool.Ping; if any operation times out or errors, close the pool (use
pool.Close()) and return the error. Ensure symbols referenced are pgxpool.New,
migrations.Up, pool.Ping, runMigrations, cfg.DatabaseURL, cfg.MigrationsDir and
pool.Close.
🪄 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: 8586cc53-cf50-4d87-9e7d-655c5bd3f9d2
📒 Files selected for processing (4)
cmd/api/main.gocmd/coveragecli/main.gointernal/platform/bootstrap/bootstrap.gomigrations/003_e2e_test_runs.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- migrations/003_e2e_test_runs.sql
- cmd/coveragecli/main.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@internal/platform/bootstrap/bootstrap.go`:
- Around line 41-54: Wrap returned errors with context using fmt.Errorf to
indicate which startup step failed: when migrations.Up(ctx, cfg.DatabaseURL,
cfg.MigrationsDir) returns err, replace the bare return with returning
fmt.Errorf("migrations up failed: %w", err) (and still close pool before
returning); when pool.Ping(ctx) returns err, return fmt.Errorf("database ping
failed: %w", err); also add fmt to the imports. Ensure all returns that
previously returned err directly now wrap the error with a descriptive message
referencing the failing operation (e.g., "migrations up failed" and "database
ping failed").
- Around line 40-54: The DB startup code uses raw ctx for pgxpool.New,
migrations.Up and pool.Ping which can hang; wrap each DB operation in a context
with a sensible timeout (e.g. context.WithTimeout(ctx, <reasonable duration>))
and ensure defer cancel() is called, then pass the timeoutted context into
pgxpool.New, migrations.Up(cfg.DatabaseURL, cfg.MigrationsDir, ...) and
pool.Ping; if any operation times out or errors, close the pool (use
pool.Close()) and return the error. Ensure symbols referenced are pgxpool.New,
migrations.Up, pool.Ping, runMigrations, cfg.DatabaseURL, cfg.MigrationsDir and
pool.Close.
🪄 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: 8586cc53-cf50-4d87-9e7d-655c5bd3f9d2
📒 Files selected for processing (4)
cmd/api/main.gocmd/coveragecli/main.gointernal/platform/bootstrap/bootstrap.gomigrations/003_e2e_test_runs.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- migrations/003_e2e_test_runs.sql
- cmd/coveragecli/main.go
🛑 Comments failed to post (2)
internal/platform/bootstrap/bootstrap.go (2)
40-54:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd timeouts for database operations during startup.
The database connection and ping operations use the raw context without a timeout. If the database is slow or unresponsive, the application startup could hang indefinitely. As per coding guidelines, sensible timeouts should be added for DB operations.
⏱️ Proposed fix to add startup timeouts
func New(ctx context.Context, cfg config.Config, runMigrations bool) (*App, error) { + connCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + - pool, err := pgxpool.New(ctx, cfg.DatabaseURL) + pool, err := pgxpool.New(connCtx, cfg.DatabaseURL) if err != nil { return nil, err } if runMigrations { if err := migrations.Up(ctx, cfg.DatabaseURL, cfg.MigrationsDir); err != nil { pool.Close() return nil, err } } - if err := pool.Ping(ctx); err != nil { + pingCtx, pingCancel := context.WithTimeout(ctx, 5*time.Second) + defer pingCancel() + if err := pool.Ping(pingCtx); err != nil { pool.Close() return nil, err }🤖 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/platform/bootstrap/bootstrap.go` around lines 40 - 54, The DB startup code uses raw ctx for pgxpool.New, migrations.Up and pool.Ping which can hang; wrap each DB operation in a context with a sensible timeout (e.g. context.WithTimeout(ctx, <reasonable duration>)) and ensure defer cancel() is called, then pass the timeoutted context into pgxpool.New, migrations.Up(cfg.DatabaseURL, cfg.MigrationsDir, ...) and pool.Ping; if any operation times out or errors, close the pool (use pool.Close()) and return the error. Ensure symbols referenced are pgxpool.New, migrations.Up, pool.Ping, runMigrations, cfg.DatabaseURL, cfg.MigrationsDir and pool.Close.Source: Coding guidelines
41-54:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap errors with context to aid debugging.
The function returns bare errors without context. When startup fails, operators cannot easily identify which step failed. As per coding guidelines, wrap errors with context using
fmt.Errorf("...: %w", err).🔧 Proposed fix to add error context
pool, err := pgxpool.New(ctx, cfg.DatabaseURL) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create connection pool: %w", err) } if runMigrations { if err := migrations.Up(ctx, cfg.DatabaseURL, cfg.MigrationsDir); err != nil { pool.Close() - return nil, err + return nil, fmt.Errorf("failed to run migrations: %w", err) } } if err := pool.Ping(ctx); err != nil { pool.Close() - return nil, err + return nil, fmt.Errorf("failed to ping database: %w", err) }Note: You'll need to add
"fmt"to the imports.🤖 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/platform/bootstrap/bootstrap.go` around lines 41 - 54, Wrap returned errors with context using fmt.Errorf to indicate which startup step failed: when migrations.Up(ctx, cfg.DatabaseURL, cfg.MigrationsDir) returns err, replace the bare return with returning fmt.Errorf("migrations up failed: %w", err) (and still close pool before returning); when pool.Ping(ctx) returns err, return fmt.Errorf("database ping failed: %w", err); also add fmt to the imports. Ensure all returns that previously returned err directly now wrap the error with a descriptive message referencing the failing operation (e.g., "migrations up failed" and "database ping failed").Source: Coding guidelines
arxdsilva
approved these changes
Jun 8, 2026
This was referenced Jun 16, 2026
Closed
Closed
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
3 participants
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.
Summary by CodeRabbit
New Features
Tests