Conversation
For a product with no abandoned-cart workflow, one toggle creates and publishes the standard cart workflow scoped to that product and toggling it back off pauses it. Reuses Workflow#publish!'s abandoned-cart exemption.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| belongs_to :utm_link, optional: true | ||
|
|
||
| enum :channel, Marketing::Channel::ALL.keys.index_by(&:itself), validate: true | ||
| enum :channel, Marketing::Channel.action_channels, validate: true |
There was a problem hiding this comment.
marketing_actions.channel is a string column whose existing posting-channel rows store names such as "x", but action_channels now maps those channels to integers. The new enum will no longer recognize or find existing rows by their stored channel, so recommendations can expose a nil channel and fail when execution checks it. Preserve the existing string values, or migrate the stored data and column type atomically.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/marketing/action.rb
Line: 17
Comment:
**Enum breaks existing channels**
`marketing_actions.channel` is a string column whose existing posting-channel rows store names such as `"x"`, but `action_channels` now maps those channels to integers. The new enum will no longer recognize or find existing rows by their stored channel, so recommendations can expose a nil channel and fail when execution checks it. Preserve the existing string values, or migrate the stored data and column type atomically.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| @covering_workflow ||= seller.workflows.alive.abandoned_cart_type.filter_map do |workflow| | ||
| workflow if workflow.abandoned_cart_products(only_product_and_variant_ids: true) | ||
| .any? { |product_id, _variant_ids| product_id == product.id } | ||
| end.max_by(&:id) |
There was a problem hiding this comment.
Multiple alive workflows may cover the same product, but max_by(&:id) keeps only one. Pausing unpublishes that workflow and then reports recovery as off, while the scheduler processes every other published matching workflow. As a result, abandoned-cart emails can continue after the seller turns recovery off. State and pause need to cover every matching workflow, or overlapping workflows must be prevented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/marketing/abandoned_cart.rb
Line: 62-65
Comment:
**Pause ignores overlapping workflows**
Multiple alive workflows may cover the same product, but `max_by(&:id)` keeps only one. Pausing unpublishes that workflow and then reports recovery as off, while the scheduler processes every other published matching workflow. As a result, abandoned-cart emails can continue after the seller turns recovery off. State and pause need to cover every matching workflow, or overlapping workflows must be prevented.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| ActiveRecord::Base.transaction do | ||
| workflow = seller.workflows.abandoned_cart_type.create!( | ||
| name: DefaultAbandonedCartWorkflowGeneratorService::WORKFLOW_NAME, | ||
| bought_products: [product.unique_permalink], | ||
| ) |
There was a problem hiding this comment.
Two concurrent enable requests can both observe that no covering workflow exists because the lookup and creation are not protected by a shared lock or uniqueness constraint. Each request can then publish its own workflow and installment, and the scheduler processes both, causing duplicate recovery emails. Serialize the lookup and creation on a stable seller or product row, or enforce an equivalent database-backed uniqueness invariant.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/marketing/abandoned_cart.rb
Line: 100-104
Comment:
**Concurrent enables duplicate workflows**
Two concurrent enable requests can both observe that no covering workflow exists because the lookup and creation are not protected by a shared lock or uniqueness constraint. Each request can then publish its own workflow and installment, and the scheduler processes both, causing duplicate recovery emails. Serialize the lookup and creation on a stable seller or product row, or enforce an equivalent database-backed uniqueness invariant.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| # Pausing is the workflow's own publish state: the workflow and its email are kept, so | ||
| # the seller can turn it back on or edit it in Workflows. | ||
| def pause | ||
| return :blocked unless available? |
There was a problem hiding this comment.
A seller can become ineligible after publishing a workflow, such as through suspension. This guard then rejects the seller's attempt to pause, while the scheduler only skips the still-published workflow. If eligibility later returns, email delivery resumes without the seller turning it back on. Eligibility should guard enabling and delivery, but unpublishing an existing workflow should remain allowed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/marketing/abandoned_cart.rb
Line: 51
Comment:
**Ineligible sellers cannot pause**
A seller can become ineligible after publishing a workflow, such as through suspension. This guard then rejects the seller's attempt to pause, while the scheduler only skips the still-published workflow. If eligibility later returns, email delivery resumes without the seller turning it back on. Eligibility should guard enabling and delivery, but unpublishing an existing workflow should remain allowed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| # The launch card's one-tap abandoned-cart workflow. Abandoned-cart email is the one | ||
| # automation a seller without a completed payout cannot use, and it is the channel | ||
| # exempt from the lifetime-sales half of the email gate. | ||
| # | ||
| # The workflow is seller-level — its recipient type is "abandoned_cart", not "product" — | ||
| # and the scheduler matches a carted product to a workflow through the workflow's own | ||
| # product filters, so "for this product" means a filter that covers exactly this product. |
There was a problem hiding this comment.
The repository requires comments to state only the non-obvious reason and generally stop within about three lines. This seven-line class comment narrates the domain and implementation, and the comments around account_wide and covering_workflow similarly repeat behavior visible in the code. Reduce them to only the invariant or trap the implementation cannot express. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/marketing/abandoned_cart.rb
Line: 3-9
Comment:
**Comments overexplain the implementation**
The repository requires comments to state only the non-obvious reason and generally stop within about three lines. This seven-line class comment narrates the domain and implementation, and the comments around `account_wide` and `covering_workflow` similarly repeat behavior visible in the code. Reduce them to only the invariant or trap the implementation cannot express. This repository requirement must be satisfied before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/antiwork/gumroad/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The channel column is a string, so mapping channels to integers would have written values nothing can read back.
gumclaw
added
the
run-all-specs
Three defects in the launch card's abandoned-cart toggle: - The controller authorized the Marketing::Action class with no explicit query, so Pundit picked the action name: show? reads record.user off a class (500) and update? does not exist on the policy. Both actions now ask for :index?, the same role permission the sibling marketing actions controller uses, and the ownership check stays in the controller. - `state` returned `enabled:` as a bare hash shorthand, which resolves to a method (or local) named `enabled` that this service does not have. - `enable`/`pause` left the `covering_workflow` memo pointing at the pre-call world, and the controller reads `state` in the same request, so a just-enabled toggle reported itself off (and a just-paused one on). The memo is dropped after the write.
Pausing unpublished only the newest covering workflow, so a seller with an account-wide default plus a product-scoped workflow saw the card read "off" while the scheduler kept emailing from the other one. Also: pause no longer needs eligibility, so a seller who becomes ineligible (suspended, payout reversed) can still stop emails that are already published; and enabling is serialized on the product so two taps cannot each create a workflow.
|
/tastelint |
TastelintFixed: nothing — both previous copy findings are still visible in these shots; trim the gated card's title repeat and the on-state's redundant Workflows sentence before ship. 1. Gated copy still repeats the card's own titleThe card heading says "Abandoned cart email" and the body starts with "Abandoned cart email turns on after your first payout" — the same phrase twice in a three-line card. The payout reason is good; the restated name is noise. Fix: Shorten the body to "Turns on after your first payout." 2. On-state sentence still restates the button below it"You can edit the email in Workflows" sits directly above an "Open in Workflows" button that says the same thing. One of them is enough. Fix: Drop the last sentence and let the button carry it — keep only "Sending “You left something in your cart” to anyone who leaves this product in their cart, 24 hours later." |
gumclaw
added
the
awaiting-human
|
/tastelint |
|
/tastelint |
3 participants
What
The launch card's one-tap cart recovery: for a product with no abandoned-cart workflow, a toggle that creates and publishes the standard abandoned-cart
Workflowwith its default single email, and toggling it back off pauses it (the workflow and its email are kept). The seller edits the email afterwards in Workflows.Tracks antiwork/gumroad-private#2720 (item 3 of #2556).
Why
Abandoned-cart email is the one automation the email gate's lifetime-sales bar does not block, so it is the lever that reaches the sellers the launch email cannot — and every primitive already exists.
How
Marketing::AbandonedCartresolves the state from the seller's own workflows. No gate is widened: enabling is offered wheneligible_for_abandoned_cart_workflows?holds, which is the cart email scheduler's own condition (ScheduleAbandonedCartEmailsJobskips workflows whose seller fails it), so the toggle cannot produce a workflow that will never fire.Workflow#publish!'s existing abandoned-cart exemption does the publishing.abandoned_cart), and the scheduler matches a carted product to a workflow throughWorkflow::AbandonedCartProducts; the service asks the workflow that same question instead of re-deriving coverage, so the card and the scheduler cannot disagree.unpublish!.DefaultAbandonedCartWorkflowGeneratorService::DEFAULT_NAME/.default_message, now also used byWorkflow::ManageService. Email body output is byte-identical (that literal was duplicated in both files). New default workflows are named “Abandoned cart email” to match the card; existing workflow names are not changed.Marketing::Actionchannelabandoned_cart— a non-posting channel: not in the posting picker, no executor,Marketing::Channel.live?answers false (so the web card and the v2 API both refuse to "execute" it). Its persisted channel names remain strings; the cart channel does not change existing posting-channel values.CartRecoveryCardsits beside the launch card on the Share tab, behind the same per-sellerauto_marketingflag and the Keep a frozen seller holdout for auto marketing #7727 holdout gate, with its own endpoint.Specs
GitHub CI is green at
228b44bc59d1778acf2b6dfcbdc11a361cecf6b6(Ruby/JS lint, TypeScript, Fast/Slow suites, Minitest andci/green). Local browser QA below exercised the real Share tab, toggle endpoint, workflow editor, pause/re-enable and duplicate enable; database readback confirmed one alive workflow and one email. The visual-review follow-up removes duplicate status badges/names, stale off-state copy and the button-like payout note. The five component tests, scoped ESLint, Prettier and full TypeScript check pass locally.bin/test-confidenceexited 0 at its 99% milestone, but classified several unrelated controller/email checks as pre-existing or unverified; it is not an all-tests-passed claim. The follow-up also makes off-state copy conditional and names newly generated workflows consistently. Ruby lint passes for all four touched Ruby files. Exact-head CI is re-running after these fixes.Covered: enable creates one published abandoned-cart workflow scoped to the product with the standard email and 24-hour rule; a second enable reuses it; the account-wide workflow is reused and reported as account-wide; a workflow covering a different product is left alone; pause unpublishes without deleting the workflow or its email; re-enabling turns the same workflow back on; a seller below the email gate can enable it (
eligible_to_send_emails?false, cart recovery on); a seller without a completed payout gets the reason and no workflow; the cart channel is absent from the posting picker and pinned to the workflow's own type; the controller's flag / ownership / unpublished-product / holdout gates.Evidence
See the QA evidence below.
Premerge review: clean @ d2fb350
QA evidence
Captured from the source committed as
d39fd0555cb2a6ace32b8c2c105cc113f4b494d6, booted locally withbin/dev-lane 2. No hosted preview or production data. Desktop is 1440 × 1100; mobile is 375 × 1000 (actualinnerWidthasserted). Paired images preserve the original pixels without scaling: desktop on the left, mobile on the right, except the two explicitly vertical pairs.Seed: fictional “Cart Recovery QA” seller and “Field Notes Workbook” ($12), a locally seeded completed payout, zero sales,
auto_marketingenabled, holdout false, and initially no alive workflow. Product publication was seeded directly, not a live payment-account onboarding test. Regular email eligibility remained false; abandoned-cart eligibility was true. Background email workers were not started and no email was sent. Local session was created through Rails' cookie-session middleware; no real user credentials. The local Bullet debug badge is hidden in capture, with application UI unchanged.Default
Default: one Abandoned cart email heading and unchecked switch. “Turn this on to send…” and “Once enabled…” describe future setup; there is no workflow link yet. Desktop left, mobile right.
Enabled
After one click: the same card has a checked switch and Open in Workflows. No duplicate state badge or off-state intro. Desktop left; mobile right.
Enabled dark
Enabled in dark mode: one heading, checked switch and workflow link, desktop left and mobile right.
Ineligible
Eligibility control after pausing and reversing only the seeded payout: disabled switch and plain “Abandoned cart email turns on after your first payout.” helper. Desktop left, mobile right.
Workflow
Top: the new workflow is named Abandoned cart email, with the Abandoned cart trigger selected and Field Notes Workbook as its filter. Bottom: its single default email, 24-hour preview delay and Unpublish control.
Repeat
Top: the card after pause/re-enable shows the checked switch and workflow link. Bottom: one Published abandoned-cart workflow, one email row, 24 Hours delay. Duplicate enabled=true returned 200 and the same workflow URL.
Before
Before comparison: ShareTab/index.tsx temporarily restored from origin/main
ff563b02072600e41be94d2af3e5d929be018ff1in this same local app, then restored to HEAD. TikTok is followed directly by Profile; the new Cart recovery section is absent. Only this rendered surface was swapped, not an entire baseline backend.Flag off
Flag off on the branch: Share controls lead to Profile without either the launch card or Cart recovery. The per-seller flag was restored after capture.
Persisted result and repeat behavior
The one-tap action created one alive
abandoned_cartWorkflow withfirst_published_atset to2026-09-17T03:29:55Z, one email, a product-scoped filter and a delay of 86400 seconds (24 hours). After the browser pause/re-enable and an extra identical enable request, database readback still showed the same workflow, same email and same first publication timestamp. The screenshots show the resulting UI; these persistence claims come from Rails readback, not pixels. Earlier failed capture attempts were reset locally; soft-deleted fixture workflows are excluded by the same alive scope the app uses.Local commands and repeatable QA
Working directory:
~/repos/gumroad-worktrees/gp2720. Capture scripts/logs are local-only undertmp/qa7735/; media is attached, never committed.A short local walkthrough is attached below. It shows the default card, one-tap enable, desktop/mobile and dark states, then the created workflow email. The recording changes viewport mid-clip; use the paired stills for layout inspection.
AI disclosure: GPT-6 Astra performed this local QA/evidence pass. Prompt: boot PR #7735 locally, capture default/enabled/ineligible/flag-off and workflow/repeat states at desktop and mobile widths, attach evidence, request Tastelint, and leave human review before merging. Earlier implementation/review attribution is unchanged.
Human review: Gianfranco must review before merge. Auto-merge is intentionally not armed: the repository requires zero approving reviews and has only
ci/greenas a required check, so the requested command could merge immediately rather than wait for his approval.walkthrough-v3.mp4
Review status
The final panel is clean at
d2fb35057ec700629d8d80a7e652368fbb45dc4e. Its only change after the captured source is a controller-test expectation; no rendered source changed. The controller suite was rerun directly withCI=1 DISABLE_SPRING=1 rbenv exec bundle exec rspec spec/controllers/products/marketing_abandoned_carts_controller_spec.rb --format documentationand passed. Tastelint remains neutral with two copy findings: shorten the gated helper to “Turns on after your first payout.” and remove the enabled-state Workflows sentence already expressed by the button. This PR remains draft until those changes, recapture and re-review are complete. It has not been marked ready or merged.