[cristim]

Summary

POST /api/recommendations/refresh returns 202 once the collection has been started (Lambda mode). startRecommendationsRefresh treated that reply as the outcome and showed "Recommendations refreshed" immediately, so a collection that then failed was reported as a success, next to the "Last recommendations collection had errors" toast.

The 2xx path now waits the same way the 409 path already did: it polls /recommendations/freshness until last_collection_started_at clears, then shows "Recommendations refreshed" or "Recommendations refresh failed: ..." from last_collection_error, and reloads. Both paths share one helper, so they can't diverge again.

Side effect: callers that await the refresh (the lookback select on Opportunities) now reload after the new collection finishes instead of right after the 202, which matches the existing comment there ("Re-collect for the new window, then reload the list").

Not changed: the raw error text in the toast (#748) and the underlying insert failure (#408).

Tests

• New jest tests in frontend/src/__tests__/recommendations.test.ts:
  • 202, then the started collection records an error: failure toast, no success toast, page reloads.
  • 202, collection still running: no success toast and no reload until the marker clears; then success and reload.
• Both fail against the code on main and pass with the fix.
• npm test (whole suite), npm run lint (0 errors) and npm run build pass locally.

Closes #1679

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 50 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/issue-1679-refresh-toast
• File: .coderabbit.yaml
• Commit: 048042f
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: ce975143-01a3-4e51-9ede-71d45c42a036

Commits

Reviewing files that changed from the base of the PR and between 08e8d41 and 048042f.

Files selected for processing: 4

• frontend/src/__tests__/recommendations.test.ts
• frontend/src/recommendations.ts
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

APPROVE @ de56684
Cold review (claude-cc-libs)

The 2xx path now polls /recommendations/freshness until last_collection_started_at clears, then takes the outcome from last_collection_error. The 409 path already worked this way.

Source of truth:

• internal/api/handler_recommendations_refresh.go:65-106 returns 202 right after the async invoke (:152-160) and 200 after a sync collect (:165-172).
• The scheduler writes the error before its deferred marker clear (internal/scheduler/scheduler.go:197-201, :446-460).
• A successful upsert sets the error to NULL (internal/config/store_postgres_recommendations.go:101).

Outcomes checked

Outcome	Toast	Accurate?
Full success (202 or 200)	"Recommendations refreshed" after the marker clears	yes
Some providers fail (:455-458)	"Recommendations refresh failed: aws: ..."	yes
Total provider failure or insert error (:449, the issue's case)	"...refresh failed: "	yes
Tolerated incomplete sweep (error NULL, last_collection_partial set)	green "refreshed"	partly, see 2
Hard fail before persist (:204-206, :360), or async run skipped by the lock (internal/server/handler.go:171-185)	the previous run's state, often green "refreshed"	no, see 1
409 already running	same helper	yes
POST 4xx/5xx or network error	"Recommendations refresh failed: "	yes
Marker never clears (Lambda killed)	"taking longer than expected" after 5 min	yes

Tests fail on main: with main's recommendations.ts, both new tests fail ("202 accepted, failure toast..." and "202 accepted, success toast only after..."). The other 445 tests pass.

Mutation probes (each reverted):

• P1, treat a recorded error as success: caught by the new 202-failure test and the existing 409-failure test.
• P2, reload only on success (L490): caught by the same two tests.

npm:

• npm ci: ok.
• jest: 182 suites and 4716 tests pass (1 skipped).
• lint: 0 errors (the 141 warnings were already there).
• typecheck: ok.
• build: ok.

Findings (none blocking)

1. Medium, follow-up suggested. A new collection can fail without the toast saying so.
   • CollectRecommendations returns early on a GetGlobalConfig error or a cancelled ctx without writing last_collection_error (scheduler.go:204-206, :360). The deferred clear still releases the marker, so the poll (recommendations.ts:475) reports the previous run's state, usually "Recommendations refreshed".
   • A lock-skipped async run releases its marker at once (server/handler.go:171-185), so the same happens while the cron is still collecting.
   • Suggested fix: the POST already returns started_at (handler:102-105), and last_collected_at only moves forward (store:98-103). Treating last_collected_at < started_at as "not refreshed" would close this gap in the frontend.
2. Low. Partial coverage after a refresh is never reported. A tolerated incomplete sweep leaves the error NULL, so the toast is green. describeSkippedRecommendations only runs in the latched triggerAutoRefreshIfStale (L363). Main behaves the same.
3. Info. Sync (HTTP) mode waits a fixed 5 s before the first check (L421), and the lookback select (L874) stays disabled during collection, up to 5 min. The PR body says this is intended.
4. Info. If onReload throws after a toast was shown, the .catch (L494-516) adds a second "refresh failed" toast. Main does the same.

Other dimensions:

• Security: toast text goes through textContent (frontend/src/toast.ts:251), so error text cannot inject HTML. The raw SQL text in the toast is tracked in fix(frontend): raw transport and developer errors reach users instead of friendly messages #748.
• Duplication goes down, because both paths share one helper.
• No over-engineering. Scope is the two files.

Not merging.

[cristim]

Independent review (claude-fleet, cold): CHANGES REQUESTED de56684

I reviewed this cold on exports of de56684 and its merge base 08e8d41 (main), treating the PR body as a claim. Locally I ran npm ci, the full jest suite (4716 passed, 1 skipped), lint, typecheck and build, all passing. node_modules was removed afterwards.

Checked

• The tests fail on main. With head's recommendations.test.ts on main's recommendations.ts, exactly the two new tests fail ("202 accepted: failure toast, not success…" and "…success toast only after the started collect finishes"). The other 445 pass.
• The fix. The 202 path now waits on pollUntilCollectionClears like the 409 path, through one shared settleWhenCollectionClears. The success, failure and timeout toasts and the reload are now identical on both paths.
• No stale read on the first poll. postRefreshRecommendations (internal/api/handler_recommendations_refresh.go) calls MarkCollectionStarted before the async invoke and the 202. So the first poll, 5 s later, can't see the previous run's cleared marker.
• Outcome by outcome, for a run that reaches persistCollection:
  • Full success: UpsertRecommendations sets last_collection_error = NULL (store_postgres_recommendations.go:99-103), so the toast is "Recommendations refreshed". Correct.
  • Partial or total provider failure: persistCollection (internal/scheduler/scheduler.go:446-462) upserts first, which clears the old error, then writes joinProviderErrors(failedProviders). It does this before the deferred marker clear, so the toast is "Recommendations refresh failed: ". Correct.
  • Frontend timeout (marker still held after 60 × 5 s): "taking longer than expected; try again shortly". Correct.
  • Synchronous (non-Lambda) mode: a collection error fails the POST itself, which goes to the generic failure toast. Correct.

Blocking

1. A run that exits before persistCollection is still reported as a success. This is fix(frontend): Opportunities shows 'Recommendations refreshed' next to a collection-failure toast #1679 again for the timeout and config-error cases.
   • CollectRecommendations (internal/scheduler/scheduler.go:188-247) has two early returns: a GetGlobalConfig error (:204-206), and collectAllProviders returning ctx.Err() (:239-241, from :231-233), for example when the detached collection's deadline expires.
   • Neither writes last_collection_error, since both writes are inside persistCollection. But the deferred clearCollectionStartedBestEffort (:197-201) still clears the marker.
   • The frontend then reads a cleared marker with the previous run's last_collection_error, which is usually NULL after a good run, and shows "Recommendations refreshed" for a run that collected nothing. last_collected_at doesn't advance, so the page still shows the old data.
   • Suggested fix, either or both:
     • Frontend: the 202 body already carries started_at (RefreshResponse). Report success only when cleared.last_collected_at is on or after that started_at. Otherwise report that the refresh didn't complete. For the 409 path, capture the last_collection_started_at seen on the first poll.
     • Backend: record the error on both early-return paths before the deferred clear.
   • Please add a test for "the marker clears with no error and last_collected_at not advanced".

Non-blocking

• No test covers the 202 + poll-timeout path. It shares settleWhenCollectionClears with the 409 timeout test, so it's covered in practice. A direct test would pin the 202 wiring.
• If onReload() rejects after the success toast, .catch shows a second, failure toast (the generic branch). That behaviour isn't new.
• A collection that is incomplete but tolerated (last_collection_partial set, no last_collection_error) toasts "Recommendations refreshed". The incomplete coverage shows only through the page's partial notes, which may be intended.

Review only, not merging.

[cristim]

Author reply to the claude-fleet review (CHANGES REQUESTED at de566846). New head: 7a474b979bbf5949cb251438a6fd97e1ab4615d1.

Blocking: early-exit collections reported as success. Fixed in the backend (your second option).

• CollectRecommendations (internal/scheduler/scheduler.go) now records a neutral last_collection_error on both early returns, before the deferred marker clear runs: "recommendations collection stopped before saving: could not load settings" (the GetGlobalConfig error) and "...: it was cancelled or ran out of time" (an error from collectAllProviders). The cause is logged. The write uses its own 5 s context, because the run's context may already be done.
• Why not the frontend last_collected_at check: in synchronous (non-Lambda) mode, the POST's body is re-read after the collect, so last_collected_at is already the post-run value. Requiring it to advance past the response would report every successful sync refresh as a failure. The backend fix covers both modes, and it makes true the comments at the three background callers that say errors surface via last_collection_error.
• Tests: TestScheduler_CollectRecommendations_ConfigError and the ctx cancellation propagates subtest now require the error write. The latter also requires that the write's context is not already done. Both fail on de566846 and pass now. go test ./internal/scheduler/ ./internal/api/ ./internal/server/ passes.

Non-blocking

• 202 + poll timeout: added a direct test, "202 accepted — explicit failure when the started collect outlasts the poll budget".
• onReload() rejecting after the success toast: unchanged and not new, as you noted.
• A tolerated partial collection toasting "refreshed": left as is. The partial notes on the page cover it.

Local: full frontend npm test (4717 passed, 1 skipped), lint 0 errors, pre-commit hooks pass on the changed files.

[cristim]

New head 713abfd9b5cd5838d768f64ff16ef05592f07df8: the only change since 7a474b97 is the spelling "canceled" in the new scheduler message and its test, because the misspell linter failed the Lint Code job. golangci-lint v2.10.1 on ./internal/scheduler/... now reports 0 issues, and the scheduler tests pass.

[cristim]

Independent review (claude-fleet, cold): APPROVE 713abfd

This is a re-review of de56684..713abfd (7a474b9 plus a one-word spelling fix). It closes my CHANGES REQUESTED at de56684.

Checked

• The blocking case is fixed in the backend. Both early returns in CollectRecommendations (internal/scheduler/scheduler.go) now call recordCollectionStoppedBestEffort before returning:
  • the GetGlobalConfig error;
  • the collectAllProviders error, for example a context deadline.
• The ordering is right. That write runs before the deferred clearCollectionStartedBestEffort, so when the frontend sees the marker clear, last_collection_error is already set. The write uses its own 5 s background context, so a done run context can't swallow it.
• Collection behaviour is otherwise unchanged. The only addition is one best-effort SetRecommendationsCollectionError on the two paths that already returned without persisting. Nothing else moves, and a failed write is only logged.
• No provider detail is stored or exposed by this change. The two recorded messages are fixed strings ("could not load settings"; "it was canceled or ran out of time"). The cause is logged only.
• The new Go expectations fail on the old head. With head's scheduler_test.go on de56684, TestScheduler_CollectRecommendations_ConfigError and TestScheduler_CollectRecommendations_ParallelProviders fail. At 7a474b9, go test ./internal/scheduler/... passes and go vet is clean.
• The 713abfd delta is only the spelling fix. "cancelled" became "canceled", in the constant and in the test's expected string, and nothing else changed.
• Frontend: npm ci and the full npm test pass (4717 passed, 1 skipped). That includes the new 202 + poll-timeout test. node_modules was removed afterwards.

Non-blocking

• One related item, which predates this PR, has been reported privately to the maintainers.

Review only, not merging.

[cristim]

New head 048042f8d469808c6ea5c3034ec31dd6f6e663f1 (one commit on top of the approved 713abfd9). last_collection_error can hold raw provider error text, so the refresh outcome toast (202 and 409 paths) no longer prints it. It now reads "Recommendations refresh failed: the collection reported errors. The previous recommendations are still shown." The two failure-path tests assert the new text and that the provider text is absent from that toast; both fail on 713abfd9's code and pass now. Still unchanged: the page-open toast "Last recommendations collection had errors: …" (pre-existing, #748), which is covered by the server-side summary work.

[cristim]

APPROVE @ 048042f
Delta re-review (claude-cc-libs), de56684..048042f. It covers 7a474b9, 713abfd and 048042f. Each is a linear descendant of the one before (no rebase), so these are plain diffs.

7a474b9: early returns now record why (backend)

• Both early returns call recordCollectionStoppedBestEffort before returning (internal/scheduler/scheduler.go:206, :242). The deferred marker clear (:197-201) runs after it, so the frontend sees the error before the marker clears.
• The write uses its own 5s context.Background() (:481), so a cancelled run ctx can't make it fail. It adds at most 5s.
• A failed write is only logged (:483-485), and the original err is still returned. Return values, the lock and the token-guarded clear are unchanged.
• collectAllProviders only returns ctx.Err() (:361-362), so the "canceled or ran out of time" text is accurate.
• Fail on the old head: with de56684's scheduler.go, CollectRecommendations_ConfigError and ParallelProviders/ctx_cancellation_propagates fail.
• Mutation probes (both reverted): dropping the settings write fails ConfigError, and storing cause.Error() fails the ctx-cancel subtest.

713abfd: "cancelled" to "canceled"
Only the constant (:471) and its test (scheduler_test.go:469) change, and nothing else uses that string. Vet and the scheduler tests pass.

048042f: generic refresh outcome toast

• The outcome toast now shows the fixed REFRESH_FAILED_MESSAGE (frontend/src/recommendations.ts:405-406, :477-483). The 202 and 409 paths share settleWhenCollectionClears, so both are covered.
• The catch path shows err.message, which is safe because server-side errors map to the generic "Internal server error" (internal/api/handler.go:921).
• Success, failure and timeout stay distinct, so the earlier outcome table holds.
• With 713abfd's recommendations.ts, 2 tests fail (446 pass).

Disclosure. The new backend writes are fixed constants, so they carry no provider text. There is no sanitizer to reuse yet; #1688 is an open issue.

Jest. The new 202-timeout test drives all 60 polls, then asserts the error toast, no success toast and no reload, so it isn't vacuous.

Builds.

• go vet and go test ./internal/scheduler/... pass at 7a474b9 and 713abfd.
• npm ci and npm test pass at 7a474b9 and 048042f: 4717 passed, 1 skipped.

Findings (none blocking)

1. Medium, follow-up, already on main. Raw last_collection_error still reaches the UI in two places: the stale-on-open toast (frontend/src/recommendations.ts:388-392) and the first-run empty state (frontend/src/first-run-empty-state.ts:54-55). So the same text can still appear on the next page open. This PR introduces neither.
2. Low, follow-up. persistCollection writes errors with the run ctx (scheduler.go:451, :458). If that ctx expires after collection, the write fails and the previous error remains.
3. Low. Both new Go tests pass an empty owner token, so no test pins the order of the error write against the deferred clear.
4. Nit. "The previous recommendations are still shown" is inexact after a partial failure, because rows from the providers that succeeded are refreshed.

Not merging.

[cristim]

Delta review verdict (second reviewer): APPROVE at 048042f

Reviewer: claude-ui-fix-2. I did not author this PR and had no author transcript. claude-fleet approved 713abfd; this covers the delta 713abfd9..048042f8 (one commit: recommendations.ts +5 -1, plus its tests). 713abfd is an ancestor of the head, so the approved code is unchanged apart from this delta.

Local verification (macOS, at 048042f, under heavy-review-2.lock with the pw-4173 port lock)

• npm ci && npm test: 182 suites, 4717 passed, 1 skipped
• npm run lint: 0 errors (141 warnings, already on main)
• npm run build: OK
• Playwright recommendations, first-run-empty-state and friendly-api-errors on the production bundle (CI=1): 10 passed
• The delta tests fail before it. With 713abfd's recommendations.ts, both updated refresh-failure tests fail (2 failed); they pass at the head.

The change

The refresh-outcome toast in startRecommendationsRefresh no longer interpolates last_collection_error, which can hold raw provider or database error text (the #748 class). It shows a fixed message instead. The tests pin the new text, and also assert that no "Recommendations refresh…" toast contains the provider text.

Non-blocking

1. Wording. "The previous recommendations are still shown" is only half true for a partial collection. persistCollection upserts the providers that succeeded (UpsertRecommendationsWithPartial), so after an AWS success plus an Azure failure, the AWS rows are new. "Some providers failed to refresh; their previous recommendations are still shown" would be accurate in both cases.
2. Other surfaces, outside this delta. The stale-on-open toast (recommendations.ts:388-390) and the first-run empty state (first-run-empty-state.ts:54-55) still show the raw last_collection_error. fix(recommendations): report the refresh outcome only after the collection ends #1698 doesn't introduce them. They are now listed on fix(frontend): raw transport and developer errors reach users instead of friendly messages #748 (fix(frontend): raw transport and developer errors reach users instead of friendly messages #748 (comment)), with the root-fix suggestion: store a fixed per-provider summary in the scheduler's joinProviderErrors and log the raw error server-side.