Repository navigation
Conversation
…unning GetDiffReviewStatus now resolves the diff/hunk structure even before a review completes, and the webui polls batch events to merge in streamed comments live, instead of hiding Findings behind the Events tab until the whole review finishes. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Notify the person who triggered a review, even with the tab or browser closed, when the first finding is ready and again when the review completes or fails - via Web Push + a service worker, opt-in from a toast shown right after triggering a review. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Catches up schema.sql with every migration already applied to the dev database but never dumped (push notifications, plus the pre-existing user time_zone migration), after fixing the local pg_dump/server version mismatch by running pg_dump inside the matching-version Postgres container. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
dbmate dump refuses to run when the host's pg_dump is older than the Postgres server (ours runs Postgres 18 in Docker, hosts often still have an older postgresql-client). scripts/dbmate-dump.sh shims pg_dump to run inside the livereview_pg container instead, so the dump always matches the server without requiring a matching client install on every machine. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…ia Docker dbmate-dump.sh previously forwarded pg_dump into the livereview_pg container to sidestep a host/server version mismatch. That made schema regeneration depend on Docker being up. Replace it with a version check that fails fast with install instructions (scripts/install-pgdg-client.sh, installing the matching major from the official PGDG apt repo) when the host's pg_dump is older than the server, so the fix is a one-time local client upgrade rather than a per-dump workaround. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
shrsv
changed the title
Add Push Notifications and Stream Review FindingsOverviewThis change introduces browser push notifications for review events. It also enables real-time streaming of findings into the UI. New database tables, API endpoints, and a service worker support these features. Technical Highlights
Impact
|
| CREATE TABLE IF NOT EXISTS push_subscriptions ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| endpoint TEXT NOT NULL UNIQUE, |
There was a problem hiding this comment.
Severity: warning
Ensure endpoint, p256dh, and auth_key are handled securely. While standard for web push, review overall data-at-rest encryption policies.
Suggestions:
- Confirm if these fields are encrypted at rest or if the push service provides sufficient security guarantees.
- Ensure these values are not logged excessively.
| default_org_id bigint, | ||
| review_level integer DEFAULT 1 NOT NULL | ||
| review_level integer DEFAULT 1 NOT NULL, | ||
| time_zone text |
There was a problem hiding this comment.
Severity: info
The time_zone column was added to the users table. This is a good addition for internationalization and personalized time display.
Suggestions:
- Consider how existing users will be migrated or assigned a default time zone.
- Ensure application logic correctly uses this time zone for display and operations.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: critical
Removing this block changes the API contract for GetDiffReviewStatus. Previously, it returned a 200 OK with status and metadata for non-completed reviews.
Suggestions:
- Reintroduce the status check and early return for non-completed reviews if previous behavior is desired.
- Update API documentation to reflect this breaking change if the new behavior is intentional.
- Consider returning an appropriate HTTP error (e.g.,
404 Not Foundor400 Bad Request) for non-completed reviews if the endpoint is now strictly for completed reviews.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: critical
Without the early return, the code will now attempt to load diffFileRecords and findingRecords for non-completed reviews, which might be empty or incomplete.
Suggestions:
- Add a check for
reviewRecord.Status == "completed"before attempting to load full review data. - Ensure subsequent logic handles incomplete data gracefully if this new behavior is intended.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: warning
The logic for applying envelope usage, operation ID, and idempotency key from metadata for non-completed reviews is lost.
Suggestions:
- Re-evaluate if billing/accounting metadata should still be applied for non-completed review statuses.
- Restore the metadata application logic if it is still required for auditing or billing purposes.
| if _, baseURL, parseErr := validateAndParseURL(reviewRecord.PrMrURL); parseErr == nil { | ||
| if token, tokenErr := s.findIntegrationToken(baseURL, orgID); tokenErr == nil { | ||
| connectorID = &token.ID | ||
| if updErr := rm.UpdateReviewConnector(reviewRecord.ID, token.ID); updErr != nil { |
There was a problem hiding this comment.
Severity: info
The UpdateReviewConnector call is a side effect within a function primarily designed for reading/resolving diffs. Consider if this backfill logic could be triggered elsewhere, or if resolvePreloadedChanges should be renamed to reflect its mutation aspect.
Suggestions:
- Extract the
UpdateReviewConnectorlogic into a separate, explicit backfill or migration function that runs once per review, perhaps triggered by the initial review creation or a background job. - Rename
resolvePreloadedChangesto indicate it performs data mutations, e.g.,ResolveAndBackfillReviewChanges.
| } | ||
|
|
||
| userAgent := c.Request().UserAgent() | ||
| _, err = h.db.ExecContext(c.Request().Context(), ` |
There was a problem hiding this comment.
Severity: warning
The ON CONFLICT (endpoint) DO UPDATE SET user_id = EXCLUDED.user_id clause is a critical security and correctness feature. It ensures that if an endpoint is re-registered, it's correctly associated with the currently authenticated user, preventing a stale subscription from being tied to the wrong user.
| return JSONErrorWithEnvelope(c, http.StatusBadRequest, "endpoint query param is required") | ||
| } | ||
|
|
||
| if _, err := h.db.ExecContext(c.Request().Context(), ` |
There was a problem hiding this comment.
Severity: warning
Deleting by endpoint AND user_id is crucial for security, preventing one user from deleting another's push subscription.
| // a failed push must never fail the review itself. | ||
| func (n *Notifier) NotifyReviewEvent(ctx context.Context, reviewID, orgID int64, kind string) { | ||
| go func() { | ||
| dispatchCtx, cancel := context.WithTimeout(context.Background(), 20*time.Second) |
There was a problem hiding this comment.
Severity: warning
Consider adding the reviewID and kind to the log message for better traceability if the context times out.
Suggestions:
- Modify the log message to include
reviewIDandkindfrom the outer scope.
| } | ||
|
|
||
| type reviewInfo struct { | ||
| userEmail string |
There was a problem hiding this comment.
Severity: warning
The orgID parameter is passed to NotifyReviewEvent but never used in dispatch or any other function within this package. Remove it.
Suggestions:
- Remove the
orgIDparameter fromNotifyReviewEventsignature and its calls.
| return "Your review" | ||
| } | ||
|
|
||
| func buildPayload(kind Kind, reviewID int64, info *reviewInfo, prodURL string) pushPayload { |
There was a problem hiding this comment.
Severity: info
The prodURL parameter is retrieved from the database in dispatch then passed here. Consider passing n *Notifier directly or caching prodURL if it's static.
Suggestions:
- Pass the
Notifierinstance tobuildPayloadinstead ofprodURLto allow internal access togetProductionURL. - Alternatively, consider caching
prodURLin theNotifierstruct if it's expected to be static.
|
|
||
| func (n *Notifier) getProductionURL(ctx context.Context) string { | ||
| var url sql.NullString | ||
| if err := n.db.QueryRowContext(ctx, `SELECT livereview_prod_url FROM instance_details LIMIT 1`).Scan(&url); err != nil { |
There was a problem hiding this comment.
Severity: warning
This function queries livereview_prod_url on every dispatch. If this URL is static, cache it in the Notifier struct to avoid repeated DB lookups.
Suggestions:
- Cache the
livereview_prod_urlin theNotifierstruct after the first successful retrieval.
| return nil | ||
| } | ||
|
|
||
| vapidPublic, vapidPrivate, err := loadOrCreateVAPIDKeys(ctx, n.db) |
There was a problem hiding this comment.
Severity: critical
The loadOrCreateVAPIDKeys function is not defined in this file. Ensure it's part of the overall change and properly handles secure storage of the private key.
Suggestions:
- Ensure
loadOrCreateVAPIDKeysis implemented in a separate, secure utility file and that the VAPID private key is stored encrypted or with appropriate access controls.
| } | ||
|
|
||
| options := &webpush.Options{ | ||
| Subscriber: "mailto:support@hexmos.com", |
There was a problem hiding this comment.
Severity: warning
The Subscriber email mailto:REDACTED_EMAIL is hardcoded. Consider making this configurable or deriving it from system settings.
Suggestions:
- Move the subscriber email to a configuration setting or retrieve it from
instance_detailstable.
| log.Printf("[WARN] push send failed for subscription %d: %v", sub.id, sendErr) | ||
| continue | ||
| } | ||
| resp.Body.Close() |
There was a problem hiding this comment.
Severity: critical
The resp.Body.Close() call is inside the if sendErr != nil block. It should be deferred right after webpush.SendNotificationWithContext to ensure it's always closed.
Suggestions:
- Move
defer resp.Body.Close()immediately after thewebpush.SendNotificationWithContextcall to ensure the response body is always closed, preventing potential resource leaks.
| t.Cleanup(func() { _, _ = db.Exec(`DELETE FROM system_settings WHERE name = 'push_vapid'`) }) | ||
| _, _ = db.Exec(`DELETE FROM system_settings WHERE name = 'push_vapid'`) | ||
|
|
||
| first, err := GetVAPIDPublicKey(context.Background(), db) |
There was a problem hiding this comment.
Severity: critical
The GetVAPIDPublicKey function is not defined in the provided pushnotify.go file. This test implies its existence and functionality.
Suggestions:
- Ensure
GetVAPIDPublicKeyis defined and available, likely in a related VAPID utility file. If it's part ofloadOrCreateVAPIDKeys, consider exposing it properly or testingloadOrCreateVAPIDKeysdirectly.
|
|
||
| // vapidKeys is stored as the 'push_vapid' row in system_settings, the same | ||
| // name/data JSONB pattern internal/api/system_settings.go already uses for | ||
| // SMTP credentials — no new secret-storage mechanism invented here. |
There was a problem hiding this comment.
Severity: info
Clarify that system_settings is for system-wide settings. VAPID keys are global, not user-specific.
Suggestions:
- Add 'system-wide' to the comment for clarity.
|
|
||
| func readVAPIDKeys(ctx context.Context, db *sql.DB) (*vapidKeys, error) { | ||
| var data []byte | ||
| err := db.QueryRowContext(ctx, `SELECT data FROM system_settings WHERE name = 'push_vapid'`).Scan(&data) |
There was a problem hiding this comment.
Severity: warning
Consider adding a FOR UPDATE clause to the SELECT query if strong consistency is needed to prevent another transaction from inserting keys between the SELECT and a potential INSERT. However, given the ON CONFLICT DO NOTHING and re-read pattern, this might be overkill and could lead to deadlocks if not carefully managed.
Suggestions:
- Evaluate if
FOR UPDATEis necessary for stronger consistency, or if the currentON CONFLICTand re-read is sufficient given thepush_vapidkey is immutable once set.
| } | ||
| var keys vapidKeys | ||
| if err := json.Unmarshal(data, &keys); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Severity: warning
Add context to the error message for JSON unmarshalling failures.
Suggestions:
- Change to
fmt.Errorf("unmarshal VAPID keys: %w", err).
| } | ||
| data, err := json.Marshal(vapidKeys{PublicKey: newPublic, PrivateKey: newPrivate}) | ||
| if err != nil { | ||
| return "", "", err |
There was a problem hiding this comment.
Severity: warning
Add context to the error message for JSON marshalling failures.
Suggestions:
- Change to
fmt.Errorf("marshal VAPID keys: %w", err).
| // concurrently, so which one is actually "first" is decided by | ||
| // pushnotify.Notifier's atomic claim, not by this call site. See | ||
| // reviewprocessor.SetNotifier's doc comment. | ||
| if status == "completed" && hasComments(comments) { |
There was a problem hiding this comment.
Severity: warning
If comments is always []*models.ReviewComment, consider passing a len(comments) > 0 boolean directly to CreateBatchEvent instead of using reflection. This improves type safety and performance.
Suggestions:
- Refactor
CreateBatchEventto accept ahasCommentsboolean directly, avoiding reflection. - Alternatively, if the concrete type is always
[]*models.ReviewComment, pass the slice directly and perform a type assertion insidehasComments.
| if comments == nil { | ||
| return false | ||
| } | ||
| v := reflect.ValueOf(comments) |
There was a problem hiding this comment.
Severity: warning
Using reflect.ValueOf(comments) on an interface{} can be slower and less type-safe than direct type checks. Ensure comments is consistently a slice type.
Suggestions:
- Add a comment explaining the expected types for
commentsto prevent misuse. - Consider if a more specific interface or generic type could avoid reflection if
commentsis always a known slice type.
| // NotifyReviewEvent should be non-blocking (fire-and-forget) — it is | ||
| // called from hot paths (batch-completion event persistence, review | ||
| // status updates) that must never wait on a push send. kind is one of | ||
| // "first_finding", "completed", "failed". |
There was a problem hiding this comment.
Severity: info
Consider adding ctx to NotifyReviewEvent signature to allow for context-aware logging or tracing within the notifier implementation.
Suggestions:
- Update
ReviewNotifierinterface andNotifyReviewEventto acceptcontext.Context.
| // each of those, guarantees every terminal transition fires it exactly once. | ||
| // See reviewprocessor.SetNotifier's doc comment for why this is a | ||
| // package-level hook rather than a constructor parameter. | ||
| if status == "completed" || status == "failed" { |
There was a problem hiding this comment.
Severity: warning
Exactly-once is not guaranteed. There is no status guard, so a completed review can later be updated to failed, or concurrent terminal updates can both notify and the last write wins. Enforce terminal state in the UPDATE.
Suggestions:
- Add AND status = 'in_progress' to terminal UPDATEs so only the first terminal transition wins.
- Introduce a state machine guard that rejects transitions out of a terminal state.
| if err := rm.store.QueryRow(query, args...).Scan(&orgID); err != nil { | ||
| return fmt.Errorf("failed to update review status: %w", err) | ||
| } | ||
| NotifyReviewEvent(context.Background(), reviewID, orgID, status) |
There was a problem hiding this comment.
Severity: warning
NotifyReviewEvent is called with context.Background(); if the hook does I/O and blocks, the caller hangs indefinitely. Use a timeout context or make it asynchronous.
Suggestions:
- Create context.WithTimeout(context.Background(), 5*time.Second) for the notification call.
- Or make NotifyReviewEvent non-blocking/best-effort.
| cd "$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
|
|
||
| if [ -f .env ]; then | ||
| export $(grep -v '^#' .env | xargs) |
There was a problem hiding this comment.
Severity: warning
export $(grep -v '^#' .env | xargs) breaks on values with spaces, quotes, or an export prefix and can load the wrong DATABASE_URL. Use a safe shell source instead.
Suggestions:
- Use set -a; . ./.env; set +a if .env is shell-compatible.
- Or parse the file with a small helper that handles quotes and spaces.
| } | ||
| dsn := os.Getenv("DATABASE_URL") | ||
| if dsn == "" { | ||
| dsn = "postgres://livereview:livereview_password_123@localhost:5432/livereview?sslmode=disable" |
There was a problem hiding this comment.
Severity: warning
Hardcoded DB password in the fallback DSN is a credential in source. Remove the fallback or require DATABASE_URL/TEST_DATABASE_URL from the environment.
Suggestions:
- Drop the fallback and fail with a clear message when DATABASE_URL is unset.
- If a local default is needed, read it from a git-ignored env file.
| } | ||
| t.Cleanup(func() { _ = db.Close() }) | ||
| if err := db.Ping(); err != nil { | ||
| t.Skipf("db not reachable: %v", err) |
There was a problem hiding this comment.
Severity: warning
Ping failure always skips the test, even when DATABASE_URL is explicitly set. CI can silently pass without a DB. Fail when DATABASE_URL is set; skip only for the built-in local fallback.
Suggestions:
- Track whether dsn came from DATABASE_URL and use t.Fatalf on Ping error when it did.
- Only t.Skipf when using the hardcoded local dev DSN.
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13528
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 10 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13523
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 9 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
…rives Root cause (traced against review 13528's actual server log, which showed the backend streamed and kept all 26 comments correctly): the disappearing comments were a client-side bug. DiffViewerPanel defaults to the risk-flat sort, which dissolves every file's hunks into synthetic single-hunk entries the instant any hunk anywhere in the diff gets a blast-radius score (arrives asynchronously from a separate server-side job, after the AI review already streamed its findings). flattenFilesByRisk assigned each comment to the one hunk whose line range contained it and silently dropped any comment that fell in the gap between two hunks, or outside every hunk's range - a comment that was visible under the plain per-file view vanished the moment the view switched to risk-flat. flattenFilesByRisk now partitions each file's comments across its hunks in original order and queues any unmatched comment onto the file's last hunk instead of dropping it, so the total comment count is always conserved. Adds a fast-check property test proving conservation across arbitrary hunk/comment-line combinations, deterministic unit tests for the exact gap scenario, and a DiffViewerPanel regression test simulating blast-radius data arriving after comments are already rendered - both fail against the old filter-only implementation and pass after the fix. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…Events tab Root causes (found via live-testing a real review on prod): batch worker count defaulted to runtime.NumCPU() (2 on prod) regardless of batch count, starving later batches until the 10-minute review timeout killed them; the Events tab had no way to distinguish a batch's terminal failure from a transient retry error, so a stale fileCount rendered as fake "Generated N comments" for batches that produced zero. - Replace the fixed worker pool with goroutine-per-batch + a semaphore sized to a configurable per-review ceiling (default 16, no longer CPU-derived), and add a global, self-tuning (AIMD) per-AI-connector concurrency limiter shared across concurrently-running reviews (internal/llm/adaptivelimiter, internal/llm/errclass). - Fix a retry-loop bug where a cancelled context only broke out of the inner select, not the retry loop, letting it burn through every retry near-instantly instead of bailing immediately. - Emit an explicit terminal batch-failure event distinct from a retry log, and stop using a batch's file count as a comment-count fallback in the frontend, so a failed batch always renders as Failed. - Cut review timeout 10m->4m and per-batch LLM call timeout 5-8m->90s. - Add instance-wide (system_settings) and per-org (org_review_ai_settings) configuration for the concurrency ceiling and review timeout, clamped so an org can only turn these down, never above the instance limit. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
|
Error generating summary: request timeout: API call exceeded deadline This review completed with partial results: 6 out of 7 batches processed successfully. Failed batches: [batch-1] The comments and analysis shown are based only on the files that were successfully reviewed. Some files may have been skipped due to batch processing errors. |
| func TestAIReviewPerformanceSettings_DefaultsWhenNoRowSaved(t *testing.T) { | ||
| db := perfSettingsTestDB(t) | ||
| // Clean slate - delete any row a prior run left behind. | ||
| if _, err := db.Exec(`DELETE FROM system_settings WHERE name = 'ai_review_performance'`); err != nil { |
There was a problem hiding this comment.
Severity: critical
DELETE removes an existing ai_review_performance row and nothing restores it. A failed or interrupted test run can wipe real admin config from a shared dev DB. Capture the prior value and restore in t.Cleanup, or use a dedicated test database.
Suggestions:
- Read the existing system_settings row before deleting and restore it in t.Cleanup.
- Run these integration tests against a disposable database instead of a shared dev DB.
|
|
||
| // Restore defaults so other tests (and a shared dev DB) aren't left | ||
| // with this test's ceiling. | ||
| if err := reviewperf.Save(putC.Request().Context(), db, reviewperf.DefaultSettings()); err != nil { |
There was a problem hiding this comment.
Severity: warning
Defaults are restored only if the test reaches this line. Any earlier failure leaves the test ceiling in the shared DB. Use t.Cleanup so restoration runs on both success and failure.
Suggestions:
- Register reviewperf.Save(... DefaultSettings()) in t.Cleanup instead of calling it inline.
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": "Failed to save review AI settings: " + err.Error()}) | ||
| } | ||
|
|
||
| perfSettings, err := reviewperf.Load(c.Request().Context(), s.db) |
There was a problem hiding this comment.
Severity: warning
Perf settings are loaded after Upsert. If Load fails, the handler returns 500 after the preference was already persisted, so the client sees failure for a successful mutation. Load perf settings before Upsert, or make the write conditional on a successful load.
Suggestions:
- Move reviewperf.Load before store.Upsert.
- If a Load error occurs after Upsert, still return the saved state or explicitly indicate the save succeeded.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: warning
Removing this branch sends non-completed reviews through the completed-review path. Ensure the remaining code branches on reviewRecord.Status and preserves failure_reason/message; otherwise pending/failed reviews are returned as completed-shaped responses. Keep an explicit status branch or add a regression test for queued/running/failed states.
Suggestions:
- Add an explicit status switch in GetDiffReviewStatus for non-completed and completed reviews.
- Add handler tests for pending, running, and failed reviews to verify response shape.
| switch { | ||
| case err == nil: | ||
| t.limiter.ReportSuccess() | ||
| case errclass.Classify(t.providerType, err) == errclass.RateLimited: |
There was a problem hiding this comment.
Severity: warning
Rate-limit classification depends on providerType, but SetLimiter does not enforce SetProviderType. An empty value silently skews limiter self-tuning.
Suggestions:
- Validate in Execute that providerType is non-empty whenever limiter is non-nil, or default to a safe classification.
- Document SetProviderType as required before SetLimiter is used.
| // iteration near-instantly once ctx was done, racking up well beyond | ||
| // what a properly-bailing loop would attempt. A generous upper bound | ||
| // catches the regression without being timing-flaky. | ||
| if got := started.Load(); got > 6 { |
There was a problem hiding this comment.
Severity: warning
The cancel goroutine is independent of the retry loop. If the scheduler delays it past 10ms, even fixed code can exceed 6 attempts and fail on a correct implementation.
Suggestions:
- Trigger cancellation deterministically, e.g. from a watcher that cancels once started reaches 3.
- Or make retry delay and attempts injectable with a fake clock so no real time is needed.
| // Bounded by the single slowest task (300ms) plus scheduling slop, not | ||
| // by any serialization - this is the ~2x+ measured improvement over the | ||
| // old model's >= 450ms floor. | ||
| if elapsed > 400*time.Millisecond { |
There was a problem hiding this comment.
Severity: warning
The 400ms wall-clock bound is tight for a shared CI runner. A scheduling pause can push elapsed time over the limit without a code regression.
Suggestions:
- Assert concurrency deterministically, e.g. record max simultaneous Execute calls and require it to reach 7.
- Or use a larger margin combined with a minimum-concurrency check.
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13539
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 9 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
…tale push-worker icon Review-detail page: - Elapsed timer no longer resets on page refresh (seed from review.startedAt instead of mount time). - Add a live "Elapsed" header stat alongside "Last activity". - Capture and show commit messages (new review_commits.message column, GitHub/GitLab providers now keep the message instead of discarding it). - Events tab label shows a live event-count badge. - Group duplicate circuit-breaker pills into one expandable pill per cause. - Raw Events tab gets a "Tail Log" auto-scroll toggle. - Findings tab shows total comment count above the summary panel. - Fix "Batches: 0" getting stuck - the review summary now refreshes on the existing 15s poll instead of only fetching once on mount. Reviews/PR tables: - Add opt-in TanStack column resizing to ClientTable, used by the Reviews and Merge Requests tables, without changing existing visuals/layout. Push notifications: - Service worker now calls skipWaiting()/clients.claim() on install/activate so existing subscribers pick up sw.js changes (like the notification icon fix) without needing to fully close every open tab first. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
|
Error generating summary: request timeout: API call exceeded deadline This review completed with partial results: 5 out of 7 batches processed successfully. Failed batches: [batch-3 batch-6] The comments and analysis shown are based only on the files that were successfully reviewed. Some files may have been skipped due to batch processing errors. |
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13542
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ❌ | no diff classification available |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20); llm: no diff classification available |
|
Error generating summary: request timeout: API call exceeded deadline |
| // Collect batch results - TOLERATE PARTIAL FAILURES. One bad batch (a | ||
| // transient LLM error, a parse failure, ...) must not discard every other | ||
| // batch's already-good comments and fail the whole review; only abort if | ||
| // every batch failed. Mirrors internal/ai/langchain/provider.go's |
There was a problem hiding this comment.
Severity: warning
This behavior mirrors the langchain provider instead of reusing shared code; the two implementations can drift.
Suggestions:
- Extract partial-failure batch aggregation into a shared helper used by both gemini and langchain providers.
| return nil, fmt.Errorf("batch %s not found in results", batchID) | ||
| fmt.Printf("[PARTIAL FAILURE] Batch %s not found in results, skipping\n", batchID) | ||
| } else { | ||
| fmt.Printf("[PARTIAL FAILURE] Batch %s failed: %v, skipping\n", batchID, taskResult.Error) |
There was a problem hiding this comment.
Severity: warning
Per-batch error text is printed to stdout then dropped; the final warning/error only knows batch IDs. Callers cannot see why a batch failed.
Suggestions:
- Store per-batch failures as a slice of {ID, Err} and include the error text in the partial-failure warning and all-failed error.
- If a batch failed due to context cancellation, wrap/return ctx.Err so callers can distinguish cancellation from other failures.
|
|
||
| // If NO batches succeeded, fail the entire review - there's nothing to aggregate. | ||
| if len(batchResults) == 0 { | ||
| return nil, fmt.Errorf("all %d batches failed: %v", len(batchInput.Batches), failedBatches) |
There was a problem hiding this comment.
Severity: warning
If batchInput.Batches is empty, this returns "all 0 batches failed" when there was nothing to process. Empty input should not be reported as a failure.
Suggestions:
- Handle len(batchInput.Batches) == 0 before this check, e.g., return an empty result or a clear no-op result.
| switch { | ||
| case err == nil: | ||
| t.limiter.ReportSuccess() | ||
| case errclass.Classify(t.providerType, err) == errclass.RateLimited: |
There was a problem hiding this comment.
Severity: warning
An empty providerType silently reaches this classification when SetLimiter is used without SetProviderType. That can mislabel rate limits and skew limiter adaptation.
Suggestions:
- Validate in Execute that limiter != nil implies providerType is set, or log a warning once.
- Make providerType a required parameter of NewBatchTask or SetLimiter instead of a separate optional setter.
| t.logger.Info("Processing batch task %s with %d files", t.id, len(t.batch)) | ||
| } | ||
|
|
||
| if t.limiter != nil { |
There was a problem hiding this comment.
Severity: warning
No test covers the new limiter path. Add a fake limiter covering Acquire failure, deferred release, and all three report branches.
Suggestions:
- Add a fake limiter capturing Acquire/Release/Report calls.
- Assert release happens on processor success and error, and that the correct report branch is chosen.
| // Bounded by the single slowest task (300ms) plus scheduling slop, not | ||
| // by any serialization - this is the ~2x+ measured improvement over the | ||
| // old model's >= 450ms floor. | ||
| if elapsed > 400*time.Millisecond { |
There was a problem hiding this comment.
Severity: warning
A 400ms wall-clock ceiling for 300ms of work is timing-flaky on loaded CI. Prefer deterministic concurrency assertions.
Suggestions:
- Assert that all tasks started before any slow task finishes, using sync primitives rather than elapsed time.
- If wall-clock timing is kept, increase the margin substantially or use a controllable fake clock.
| // iteration near-instantly once ctx was done, racking up well beyond | ||
| // what a properly-bailing loop would attempt. A generous upper bound | ||
| // catches the regression without being timing-flaky. | ||
| if got := started.Load(); got > 6 { |
There was a problem hiding this comment.
Severity: warning
This upper-bound check passes vacuously if cancellation happens before ProcessAll starts the task. Require evidence the task actually ran.
Suggestions:
- Assert started.Load() >= 1 to ensure the task began before cancellation.
- Use a channel to signal the first Execute, then cancel only after receiving it.
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13543
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 9 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
🔍 LiveReview
⚙️ Customize risk-based approval policy for this repository or your organization.
This PR bundles two related review-visibility improvements, a dev-tooling fix found along the way, and a round of production bug fixes found while verifying the push-notification feature end to end.
Part 1: Stream findings into the Findings tab while a review is running
Use case
Today, when you hit Trigger Review or Start a Review, the webui bounces you to the Events tab (a log/timeline view) and keeps the Findings tab empty until the entire review finishes. For a large diff spanning many files, that means staring at a scrolling log with no sense of which files have problems until everything is done.
This PR makes the Findings tab usable the whole time a review is running:
Out of scope for this PR: posting comments to GitHub/GitLab/etc. PR threads is untouched and still happens as a single batch at the end. Making that incremental too is a separate, larger change (it would need to handle the helper "polish" stage potentially rewording a comment after it's already been posted) and is intentionally left for a future PR.
Algorithm / data structures
The backend has quietly been capable of this for a while: every time a batch of files finishes its LLM call,
internal/ai/langchain/provider.goemits abatch/completedevent carrying that batch's raw[]*models.ReviewCommentinto thereview_eventstable (EmitBatchComplete→DatabaseEventSink.EmitBatchEvent→CreateBatchEvent). The webui simply never read that payload — the Events tab only ever rendered it as a log line. git-lrc's local review UI already solved exactly this by polling the same events endpoint and merging streamed comments into its file view; this PR ports that logic into LiveReview's hosted webui.New module:
ui/src/components/reviews/diffviewer/reviewStreamState.ts(ported ~1:1 fromgit-lrc:internal/staticserve/static/components/review_stream_state.mjs, unit-tested inreviewStreamState.test.ts):extractNewEvents): maintains aSet<string>of seen event ids across polls (falling back to a stable composite key — type/time/batchId/stringified-data — when the server omits an id), so re-polling with overlap never double-applies a batch's comments.inferReviewStatusFromEvents): scans events backwards for the lateststatusevent or acompletionevent, to detectcompleted/failedfrom the event stream itself without waiting on a separateGET /reviews/:idpoll.extractExternalCommentsFromEvents/normalizeStreamedComment): filters fortype === 'batch' && data.status === 'completed', readsdata.comments[], and normalizes field casing —models.ReviewCommenthas no JSON tags so it marshals PascalCase (FilePath,Line, ...) over the wire, whileDiffReviewComment(the shapeGetDiffReviewStatusreturns) is lower camelCase. DropsIsInternalcomments, which are synthesis-only and never meant to reach a user.appendStreamedCommentsToFiles): an O(files + comments) merge — builds aMap<file_path, index>once, then appends each incoming comment to its file'scomments[]by path lookup, immutably (returns new file objects, doesn't mutate the input array react state depends on).DiffViewerPanel.tsxwires this into two effects:getDiffReviewfetch on mount/reviewId change (unchanged).data.statusiscreated/in_progress: pollsGetReviewEvents(reviewId, since, limit=1000)every 1s (matching git-lrc's cadence), runs the response through dedup → extract → merge, and accumulates streamed comments in state. The moment inferred status flips tocompleted/failed, it fires one finalgetDiffReviewcall and clears the streamed-comment accumulator — that response already carries the real (possibly helper-polished) comments, so there's nothing left to merge on top of it. Rendered files =appendStreamedCommentsToFiles(data.files, streamedComments), memoized.Deeper technical details
Backend change was required, not just a frontend port.
GetDiffReviewStatus(internal/api/diff_review.go) used to early-return{status, review_id}with nofileskey at all wheneverreviewRecord.Status != "completed"— the diff/hunk structure (fetchPreloadedChanges+buildDiffFiles) was only ever computed in the completed-path below. That meant there was nothing for the frontend to merge streamed comments into before completion. Fixed by extracting the trigger-type-aware diff-resolution logic (blob storage lookup, falling back to a live re-fetch from the PR/MR or fromscheduled-trigger metadata depending onTriggerType) into a new helper,resolvePreloadedChanges, called unconditionally before branching on completion status. The non-completed response path now also returnsfiles: buildDiffFiles(preloaded, nil)— hunks with emptycomments, to be filled in by the frontend's event-driven merge. The completed path is otherwise unchanged (still callsbuildDiffFiles(preloaded, result.Comments)with the real, final comments fromreview_resultmetadata).Why the final reconciliation fetch is safe, not just convenient. The helper "polish" stage (
internal/review/service.golines ~259-264) only ever rewords findings the leader model already produced — per the code's own comment, it never adds, drops, or relocates one. So a comment shown early via the batch-event stream is already correct in substance and position; the only thing the finalgetDiffReviewfetch can change is wording. That's why clearingstreamedCommentsand trusting the final fetch wholesale, rather than trying to diff/patch it against what was already shown, is the right trade — verified by reading git-lrc's own reason for being "fast": its local CLI-diff reviews skip PR-posting entirely (Provider.Type == "cli") and normally run without the helper stage at all, so there was nothing to reconcile there; LiveReview's hosted reviews can have the helper stage, hence the explicit reconciliation step this PR adds that git-lrc's UI never needed.Tab-switching changes in
ReviewDetail.tsx. Removed the one-time-per-review effect that force-switchedactiveTabto'events'wheneverreview.status !== 'completed'— Findings is now reachable (and useful) the entire time a review runs. The existing effect that jumps back to Findings on completion is kept as a courtesy. The existingkey={review?.status}on<DiffViewerPanel>(forcing a clean remount on each status transition) is left untouched and composes fine with the new polling — each remount just starts a fresh poll cycle for whatever status it mounts under.Gating/empty-states in
DiffViewerPanel.tsxwere restructured aroundfiles.lengthinstead ofdata.status === 'completed': no diff data at all → unchanged "no diff data" empty state; a failure with zero streamed findings → unchanged failure empty state; a failure with some findings already streamed in → now shown with a warning banner instead of being discarded; still-running with no files yet (diff not resolved server-side yet) → new "Preparing the diff…" empty state; still-running or failed with files present → full diff/findings view, with an info or warning banner noting the in-progress/failed state and the running finding count.Part 2: Browser push notifications for review progress
Use case
Once you trigger a review, the only way to know it's progressing or done used to be keeping the tab open (helped by Part 1 above). There was no way to walk away and get pinged. This adds real OS-level browser notifications — via the Web Push API + a service worker, so they arrive even if the tab or browser is closed — for two moments per review: the first finding becoming available, and the review completing (or failing).
PushNotificationBanner) on the review detail page — not a toast, which turned out to never render action buttons at all (see Part 4) — with an inline "Enable notifications" button, a "Settings" link, and guidance if the browser has already blocked notifications for the site.Algorithm / data structures
Two different trigger points, for a good reason:
First finding available. Batches run concurrently (
internal/batch/task_queue.goworker pool), so "first" needs an atomic claim, not an in-memory flag, or a goroutine race would double-notify.pushnotify.ClaimFirstFindingrunsUPDATE reviews SET first_finding_notified_at = now() WHERE id = $1 AND first_finding_notified_at IS NULL— only the caller that gets a row back (verified under 20-goroutine concurrency inpushnotify_test.go) proceeds to notify. This is wired into both of the codebase's independent event-sink implementations (internal/review_processor/events.go'sCreateBatchEvent, used by the async job-queue path, andinternal/api/polling_event_service.go'sCreateBatchEvent, used by the synchronous CLI-diff API path) — missing either one would silently drop notifications for half of LiveReview's review-triggering paths.Completion/failure. Terminal-status writes are scattered across
internal/jobqueue/review_worker.go,scheduled_review_worker.go, andinternal/review_processor/manual.go— too many call sites to safely hook individually. Centralized instead insideReviewManager.UpdateReviewStatus(internal/review_processor/reviews.go): after the status row update succeeds for a terminal status, it calls a package-level notifier hook (review_processor.SetNotifier(n)/ a nil-safe package var), set once at server startup ininternal/api/server.go. A package-level hook, not a constructor param, becauseNewReviewManager(db)is called fresh at many unrelated call sites — threading a notifier through all of them would recreate the same "touch everything" problem this avoids.Delivery (
internal/pushnotify): a VAPID keypair is generated once at first use and stored insystem_settings(same pattern as SMTP credentials).NotifyReviewEventresolvesreviews.user_email→ ausersrow (skips silently if empty — the "no identifiable human" case), checks that user'suser_notification_preferencesrow (missing row = both enabled, matching the "on by default" decision with no backfill needed), loads theirpush_subscriptionsrows (one per browser/device), and sends viawebpush-go; a 410/404 response prunes that subscription inline.Deeper technical details
New schema (
db/migrations/20261030134721_create_push_notifications.sql):push_subscriptions(id, user_id FK, endpoint UNIQUE, p256dh, auth_key, user_agent, created_at, last_used_at),user_notification_preferences(user_id PK FK, notify_first_finding, notify_completion, updated_at), andreviews.first_finding_notified_at TIMESTAMPTZ(the claim-once marker above).New endpoints, session-only (
authMiddleware.RequireAuth(), never API-key — a push subscription is tied to one specific browser's real endpoint, not something automation should manage on a user's behalf):GET /push/vapid-public-key,POST/DELETE /push/subscriptions,GET/PUT /users/me/notification-preferences. The three mutating routes are wired throughs.audited(...)with newauditlog.Specentries/action constants, documented indocs/audit-events.mdper this repo's audit-logging rules.user_idis always read from the authenticated session context, never the request body.Frontend: a minimal push-only service worker (
ui/public/sw.js— justpush/notificationclick, not a full PWA/offline cache, usingnotification-icon.png—favicon.svgrasterized at 192x192 viavl-convert svg2png, since.icoisn't reliably rendered as a notification icon),pushNotifications.tswrappingpushManager.subscribe/getSubscription,PushNotificationBanneron the review detail page, and a new Settings → Notifications tab — plus the required mega-menu entry androutes_guidedoc per this repo's navigation/RAG-doc conventions.Part 3: Fix
db/schema.sqldrift and the dump tooling behind itWhile working on Part 2, found
db/schema.sqlhad silently drifted from the actual migration history —dbmate dumpwas failing locally because the host'spg_dump(16.13, from Ubuntu's own repo) was older than the dev Postgres server (18.6), andpg_dumprefuses to dump a server newer than itself. This wasn't new or caused by this PR; it meant at least one prior migration (users.time_zone, already merged in an earlier PR) had also never made it intoschema.sql.Fixed properly rather than routed around:
scripts/install-pgdg-client.shinstalls a matching-majorpostgresql-clientfrom the official PGDG apt repo (one-time, needs sudo).scripts/dbmate-dump.sh(wired tomake db-dump) now checks the localpg_dumpmajor version against the live server'sserver_version_numbefore dumping, and fails fast with the exact fix command instead of a confusing dbmate error or a silent workaround.db/schema.sqlregenerated and verified byte-identical to a known-good Docker-container-produced dump except for the dumper-package label — confirming the catch-up is accurate, not just "a diff happened."Part 4: Bugs found verifying Part 2 against a real deployment
reviewprocessor.SetNotifier(...)was only called insideNewServer()(theapiprocess). The backgroundworkerprocess (ecosystem.config.js'slivereview-worker/-2,cmd/worker.go→api.WorkerContext()) is where queued reviews actually run and where batch-completion/status events fire — so in that process the notifier stayednil, and everyNotifyReviewEventcall silently no-op'd with no error or log line. A subscribed user's completed review with findings never setfirst_finding_notified_atand never sent a push. Fixed by movingSetNotifierinto the sharedappContext()bothNewServerandWorkerContextcall.GET /reviews/:id/accounting500'd on every freshly-triggered review.GetReviewAccountingTotals'stoken_tracked_opscolumn is a bareSUM(CASE WHEN ... THEN 1 ELSE 0 END), unliketotal_billable_loc'sCOALESCE(SUM(...), 0)right next to it —SUM()over zero matchingloc_usage_ledgerrows (every review, until its first LLM call is accounted) isNULL, and scanning that into a non-nullableint64failed the whole query. Fixed with a matchingCOALESCE; regression test confirmed to fail against the old query and pass against the fix.#<id>,owner/repo, PR/MR title) in the body instead of a bare repository URL, a proper PNG icon instead of.ico, and "Review completed with errors" instead of the blunter "Review failed" (most failures here are one erroring batch, not total loss).PUT /users/time-zone) — couldn't reproduce a bad zone name server-side, but this guards against stray whitespace some browser/OS combinations are known to append.GET /github-app/demo404s are expected on boxes withoutGITHUB_APP_IDconfigured, not a bug —useGitHubAppConnectedalready documents and handles a 404 there as "not connected."Testing
go build ./...andmake testall(full repo Go test suite, matching CI'slivereview-tests.yml) pass clean, includinginternal/pushnotify(claim-once race under concurrency, preference-filtering, empty-user-email skip, the new notification-content/label tests) and the newstorage/licenseaccounting regression test.npx tsc --noEmitpasses; full jest suite passes, includingPushNotificationBanner's tests (eligibility/blocked-state/subscribe-failure-retry).make security-osv,make security-govulncheck,make security-semgrep,make security-gitleaks.livereview.hexmos.com): triggered a review, confirmed the notification arrives with the tab closed, confirmed the icon/body/click-through fixes in Part 4, confirmed the accounting-poll 500 is gone.LiveReview auto-approval decisions