Repository navigation
fix(planned-purchases): Pause is visible + reversible (badge, toast, Resume) - #904
Conversation
cristim
added
triaged
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 45 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR implements pause/resume visibility and reversibility for planned purchases. The backend now includes ChangesPlanned purchase pause visibility and reversibility
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Comment |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…Resume) After #779/#772 added the paused DB status, clicking Pause made the scheduled purchase silently disappear from the list with no feedback. The planned-purchases list read GetPendingExecutions, whose status set (pending, notified) the scheduler relies on to decide what to FIRE and which excludes paused, so paused rows dropped out. Switch the list handler to GetExecutionsByStatuses with [pending, notified, paused] so paused executions stay listed (with the existing Paused badge and Resume button) while the scheduler keeps using the narrower GetPendingExecutions and never fires paused rows. Restore the soonest-first ordering. Add success toasts on Pause and Resume. Pause stays distinct from Disable plan (#774, whole plan) and Cancel (terminal): it is reversible and scoped to a single execution; the plan stays enabled. Resume (paused -> pending) and the 409 on ineligible transitions already existed from #772. Tests: backend asserts the paused status set is requested, paused rows stay returned, and ordering is ascending; frontend asserts the paused row renders visibly with a badge and that Pause/Resume fire success toasts.
ef451cf to
fcb2fab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/plans.test.ts (1)
645-647: ⚡ Quick winMake paused-label assertion less brittle
The
innerHTMLsubstring check ('>paused<') is tightly coupled to exact markup/casing and can fail on harmless rendering changes. Prefer DOM/text-based assertions.Suggested test hardening
- expect(container?.innerHTML).toContain('status-paused'); - expect(container?.innerHTML).toContain('>paused<'); + expect(container?.querySelector('.status-paused')).toBeTruthy(); + expect(container?.textContent?.toLowerCase()).toContain('paused');🤖 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 `@frontend/src/__tests__/plans.test.ts` around lines 645 - 647, Replace the brittle innerHTML substring check expect(container?.innerHTML).toContain('>paused<') with a DOM/text-based assertion: locate the paused label via a semantic query or selector (e.g. use screen.getByText(/paused/i) or within(container).getByText(/paused/i), or check container.querySelector('.status-paused')?.textContent.trim().toLowerCase() === 'paused') and assert it is present; keep the existing status-paused class assertion but remove the raw innerHTML match to avoid markup/casing brittleness.
🤖 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/api/handler_purchases.go`:
- Around line 101-110: Handler.getPlannedPurchases is fetching executions with
GetExecutionsByStatuses(plannedListStatuses, config.MaxListLimit) and then
re-sorting in-memory, but PostgresStore.GetExecutionsByStatuses currently uses
"ORDER BY scheduled_date DESC LIMIT" causing the DB to truncate the newest rows
before you re-sort; change the store/query to accept an order direction (or add
a dedicated method) so that when called from getPlannedPurchases it queries with
"ORDER BY scheduled_date ASC LIMIT $2" (or a parameterized ASC order) so the DB
returns the soonest rows before LIMIT, then remove the in-memory corrective sort
in Handler.getPlannedPurchases.
---
Nitpick comments:
In `@frontend/src/__tests__/plans.test.ts`:
- Around line 645-647: Replace the brittle innerHTML substring check
expect(container?.innerHTML).toContain('>paused<') with a DOM/text-based
assertion: locate the paused label via a semantic query or selector (e.g. use
screen.getByText(/paused/i) or within(container).getByText(/paused/i), or check
container.querySelector('.status-paused')?.textContent.trim().toLowerCase() ===
'paused') and assert it is present; keep the existing status-paused class
assertion but remove the raw innerHTML match to avoid markup/casing brittleness.
🪄 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
Run ID: 13db4f64-15f8-4b78-b2d9-29709055a242
📒 Files selected for processing (5)
frontend/src/__tests__/plans.test.tsfrontend/src/plans.tsinternal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/handler_test.go
GetExecutionsByStatuses uses ORDER BY scheduled_date DESC + LIMIT, which is correct for History (newest-first) but truncates the soonest rows when used for the Planned Purchases list and total pending/notified/paused rows exceed MaxListLimit. An in-memory ASC re-sort of the already-truncated subset cannot recover what LIMIT dropped at the DB. Add a dedicated GetPlannedExecutions method that mirrors the GetExecutionsByStatuses shape (same status filter, same limit clamping, same scan logic via queryExecutions) but flips the ORDER BY to scheduled_date ASC NULLS LAST, id ASC. The secondary id ASC sort keeps the ordering stable when multiple rows share a scheduled_date; NULLS LAST is defensive against a future schema relaxation (today scheduled_date is NOT NULL). GetExecutionsByStatuses is left untouched so its other callers (History queries that want newest-first) keep their semantics. pgxmock regression coverage anchors the SQL contract: - TestPGXMock_GetPlannedExecutions_UsesASCOrdering asserts the query has ASC + NULLS LAST + id ASC + LIMIT $2 via a strict regex matcher, so a regression to DESC or a dropped secondary sort fails the test. - TestPGXMock_GetPlannedExecutions_EmptyStatuses guards the short-circuit. - TestPGXMock_GetPlannedExecutions_LimitClamping covers the negative -> DefaultListLimit and over-MaxListLimit -> MaxListLimit clamps. All store mocks (api, purchase, scheduler, analytics, server health) gain a GetPlannedExecutions method so the StoreInterface contract is satisfied across the codebase. Refs CR on #904.
…est rows Handler.getPlannedPurchases was calling GetExecutionsByStatuses (ORDER BY scheduled_date DESC + LIMIT $2) and then re-sorting ASC in-memory. When the planned set (pending + notified + paused) exceeds MaxListLimit, the DB returns only the LATEST rows; the in-memory sort just re-orders that already-truncated subset. The soonest rows, exactly the ones the user has to act on, can be omitted from the response entirely. Switch the handler to the new GetPlannedExecutions store method (ASC at the SQL level so LIMIT keeps the soonest rows), and drop the post-fetch sort.SliceStable since the DB now returns rows in the correct order. The "sort" import stays (still used for tuple normalisation elsewhere in the file). End-to-end regression coverage: - TestHandler_getPlannedPurchases_SoonestRowsNotTruncated seeds 5 rows in ASC order, asserts all 5 reach the response in ASC order, and uses AssertNotCalled to guard against any future refactor re-introducing a parallel GetExecutionsByStatuses call on this code path. - TestHandler_getPlannedPurchases_PausedStaysVisible updated to reflect the new ASC-from-store contract (was returning DESC then re-sorting). Closes CR finding on #904.
|
Added GetPlannedExecutions store method with ASC ordering (vs the existing DESC GetExecutionsByStatuses). Handler.getPlannedPurchases now uses it; in-memory re-sort dropped. Regression test asserts the SQL uses ASC ordering AND a new handler test asserts the soonest rows are returned end-to-end. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
1 participant
Closes #903. Resolves QA Planned 6.1.
Problem
After #779/#772 added the
pausedstatus to thepurchase_executionsvalid_statusCHECK constraint, clicking Pause on a scheduled purchase made the row silently disappear from the Scheduled (Planned) Purchases list with no feedback. A Pause that looks like a delete.Root cause:
getPlannedPurchasesreadGetPendingExecutions, whose status set (pending,notified) the scheduler relies on to decide what to fire and which excludespaused. So paused rows dropped out of the list.Fix
GetExecutionsByStatuses(ctx, [pending, notified, paused], MaxListLimit)so paused executions stay listed. The scheduler keeps using the narrowerGetPendingExecutionsand never fires paused rows. Ordering restored to soonest-first (the store returns DESC).The Resume endpoint (
paused->pending, 409 on ineligible transitions) already existed from #772 -- no new endpoint needed.Tests
[pending, notified, paused]status set is requested, the paused row stays returned, and rows are ordered ascending. Existing list/error tests updated.status-pausedbadge (not the empty state); Pause and Resume each fire a success toast.go test ./internal/api/...and./internal/config/...pass;npx jest --testPathPattern='plan|purchase|history'passes;tsc --noEmitclean.Summary by CodeRabbit