engineering/make_post_sell

Fork 0

feat: integrate design tokens from www.makepostsell.com styleguide #94

Merged

russellballestrini merged 31 commits from explore/design-tokens into master

2026-03-07 23:28:24 -05:00

russellballestrini commented

2026-03-07 23:28:02 -05:00

(Migrated from git2.unturf.com)

Add tokens.css as foundation layer (loaded before common.css)
- Color system: brand, surface, text, border, state, alert tokens
- Typography: major third scale with 14 semantic classes
- Spacing: 4px base, 12-step scale with utility classes
- Shape: 7 radius levels from none to pill
- Elevation: 5 shadow levels with dual-shadow technique
- Motion: 4 easing curves, 7 durations, entrance animations
- Loading: skeleton shimmer + spinner (3 sizes)
- Scroll reveal, state layers, ripple effect, focus rings
- Accessibility: prefers-reduced-motion, :focus-visible
Rewrite styleguide.j2 to comprehensive design system reference
- New sections: tokens, elevation, motion, spacing, shape, states,
  loading, status notices, cart buttons, checkout layout, toggle
- All app components documented with live demos
- TOC navigation for all sections
Add STYLEGUIDE rule to CLAUDE.md
Wire tokens.css into base.j2 before common.css

Exploration branch — tokens.css provides new variables alongside
existing common.css variables. No visual regressions expected as
common.css values take precedence for shared property names.

Summary by CodeRabbit

New Features
- Gift cards: purchase, apply discounts at checkout, check balance, and manage inventory
- Bring Your Own Bucket (BYOB): shops can configure custom S3 storage for media uploads
- Shop environments: production, staging, and development designation options
- 21-day free trial for newly created shops
- Enhanced design system with tokens, components, and typography utilities
Improvements
- Updated media delivery to use shop-specific CDN endpoints
- Enhanced cart with gift card discount support alongside coupons
- Discovery search respects shop environment settings
Documentation
- Added comprehensive design system documentation
- Updated architecture reference with new features
- Enhanced development guidelines and test coverage requirements

- Add tokens.css as foundation layer (loaded before common.css) - Color system: brand, surface, text, border, state, alert tokens - Typography: major third scale with 14 semantic classes - Spacing: 4px base, 12-step scale with utility classes - Shape: 7 radius levels from none to pill - Elevation: 5 shadow levels with dual-shadow technique - Motion: 4 easing curves, 7 durations, entrance animations - Loading: skeleton shimmer + spinner (3 sizes) - Scroll reveal, state layers, ripple effect, focus rings - Accessibility: prefers-reduced-motion, :focus-visible - Rewrite styleguide.j2 to comprehensive design system reference - New sections: tokens, elevation, motion, spacing, shape, states, loading, status notices, cart buttons, checkout layout, toggle - All app components documented with live demos - TOC navigation for all sections - Add STYLEGUIDE rule to CLAUDE.md - Wire tokens.css into base.j2 before common.css Exploration branch — tokens.css provides new variables alongside existing common.css variables. No visual regressions expected as common.css values take precedence for shared property names.  ## Summary by CodeRabbit * **New Features** * Gift cards: purchase, apply discounts at checkout, check balance, and manage inventory * Bring Your Own Bucket (BYOB): shops can configure custom S3 storage for media uploads * Shop environments: production, staging, and development designation options * 21-day free trial for newly created shops * Enhanced design system with tokens, components, and typography utilities * **Improvements** * Updated media delivery to use shop-specific CDN endpoints * Enhanced cart with gift card discount support alongside coupons * Discovery search respects shop environment settings * **Documentation** * Added comprehensive design system documentation * Updated architecture reference with new features * Enhanced development guidelines and test coverage requirements

russellballestrini (Migrated from git2.unturf.com) merged commit df8f4993c6 into master

2026-03-07 23:28:24 -05:00

russellballestrini commented

2026-03-07 23:28:27 -05:00

(Migrated from git2.unturf.com)

mentioned in commit df8f4993c6

mentioned in commit df8f4993c675258465b9610a3df513a4ac1b5efc

coderabbitai commented

2026-03-07 23:28:43 -05:00

(Migrated from git2.unturf.com)

📝 Walkthrough

Walkthrough

This PR introduces a comprehensive gift card system (purchase, redemption, management), shop environment classification (production/staging/development), a 21-day trial feature, and Bring Your Own Bucket (BYOB) S3 support. Includes new models, migrations, request helpers, templates, routes, and extensive documentation updates across design system, architecture, and ticket specifications.

Changes

Cohort / File(s)	Summary
Gift Card Core Models `make_post_sell/models/gift_card.py`, `gift_card_transaction.py`, `cart_gift_card.py`	New ORM models for gift cards, transactions, and cart-to-gift-card associations. Includes code generation, validation, deduction logic, and retrieval helpers.
Gift Card Cart Integration `make_post_sell/models/cart.py`	Extends Cart with `json_gift_cards` column, gift card association proxy, purchase tracking, discount deductions per gift card, and validation methods.
Gift Card Purchase & Routes `make_post_sell/routes.py`, `templates/gift_card.j2`, `static/js/gift_card.js`	New routes for gift card page, add-to-cart, apply, and removal. Template with slider/input controls for amount selection and recipient details. Route handling not shown in diff.
Gift Card Management UI `templates/gift_card_manage.j2`, `gift_card_detail.j2`	Admin templates for listing gift cards with totals, status badges, toggle controls, and transaction history display.
Shop Model Extensions `make_post_sell/models/shop.py`	Adds gift card settings, environment classification (production/staging/development), trial tracking (started, ended, active), and BYOB primary S3 configuration with CDN endpoint.
BYOB S3 Implementation `make_post_sell/request_methods.py`, `lib/karaoke.py`, `lib/s3_mirror.py`	Shop-aware S3 client selection via `shop_uploads_client` and `shop_bucket_name` request methods. Defers S3 client creation to post-shop-load in backfill operations. Adds domain check for 127.0.0.1.
Design System & Guidelines `docs/design-system.md`, `CLAUDE.md`	Comprehensive design system documentation covering tokens, typography, spacing, components, and CSS conventions. CLAUDE.md reinforces design/test/BYOB guidelines with post-work checklists.
Architecture & Ticket Documentation `docs/architecture.md`, `docs/tickets/mps-10.md` through `mps-16.md`	Updates architecture diagram for gift cards and BYOB. Detailed specifications for gift card models, purchase flow, redemption, admin settings, shop environment, trial system, and BYOB feature.
Database Migrations `make_post_sell/scripts/alembic/versions/f8201a9ba045_...`, `9884324a48e3_...`	Gift card tables (`mps_gift_card`, `mps_gift_card_transaction`, `mps_cart_gift_card`), cart `json_gift_cards` column, shop settings, and environment/trial/primary S3 columns with idempotent guards.
Template CDN Endpoint Updates `templates/base.j2`, `cart.j2`, `content.j2`, `home.j2`, `product_edit.j2`, `shop.j2`, `shop_about.j2`	Replaces `request.app["bucket.secure_uploads.get_endpoint"]` with `request.shop_cdn_endpoint` for all media/thumbnail URLs. Adds environment and trial banners to base template.
Email & Service Updates `make_post_sell/lib/mail.py`, `lib/digest_sender.py`	New `send_gift_card_email()` function with amount, code, message, and redemption instructions. Digest sender filters to production shops only (`Shop.environment == 0`).
Frontend State Management `make_post_sell/static/js/watch.js`	Adds ring state staleness detection and server-seeding on startup; fetches current product from server if localStorage ring data is empty; fallback to existing preloadNext on fetch failure.
Model & Meta Updates `make_post_sell/models/__init__.py`, `meta.py`, `product.py`	Exports new gift card model classes. Adds CLASS_TO_TABLE mappings for GiftCard, GiftCardTransaction, CartGiftCard. Updates product search to exclude non-production shops via environment filter.
Admin UI `templates/actions_view.j2`	Conditional "Gift Cards" management link when `request.shop.gift_card_enabled`.

Sequence Diagram

sequenceDiagram
    participant Client as Client (Browser)
    participant Server as Server (Routes/Cart)
    participant DB as Database
    participant Checkout as Checkout View
    participant Email as Email Service

    Client->>Server: POST /gift-card/apply<br/>(gift_card_code, cart_id)
    Server->>DB: Fetch GiftCard by code
    DB-->>Server: GiftCard (balance, validity check)
    Server->>Server: Validate (enabled, balance > 0, shop match)
    Server->>DB: Add CartGiftCard entry
    DB-->>Server: Success
    Server->>Server: Recalculate cart.discounted_totals<br/>(apply after coupons)
    Server-->>Client: Updated cart with gift card deduction

    Client->>Server: POST /checkout (finalize)
    Server->>Checkout: Process payment
    Checkout->>DB: Create GiftCardTransaction
    Checkout->>DB: Deduct balance from GiftCard
    DB-->>Checkout: Transaction & balance updated
    Checkout->>Email: Send redemption confirmation
    Email-->>Client: Email to purchaser
    Server-->>Client: Order confirmation

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🎁 A gift card hops with joy today,
Through shop and cart in every way,
With buckets bright and trials new,
The MPS burrows made all true!
Five features strong, the rabbit sings—
What wonderful commerce spring brings! 🐰

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the primary change: integrating design tokens from the styleguide into the codebase. It is specific, concise, and directly relates to the main objective of establishing tokens.css as a foundation layer.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings: use the @coderabbitai generate docstrings command.

💡 TIP
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

<details> <summary>📝 Walkthrough</summary> ## Walkthrough This PR introduces a comprehensive gift card system (purchase, redemption, management), shop environment classification (production/staging/development), a 21-day trial feature, and Bring Your Own Bucket (BYOB) S3 support. Includes new models, migrations, request helpers, templates, routes, and extensive documentation updates across design system, architecture, and ticket specifications. ## Changes |Cohort / File(s)|Summary| |---|---| |**Gift Card Core Models** `make_post_sell/models/gift_card.py`, `gift_card_transaction.py`, `cart_gift_card.py`|New ORM models for gift cards, transactions, and cart-to-gift-card associations. Includes code generation, validation, deduction logic, and retrieval helpers.| |**Gift Card Cart Integration** `make_post_sell/models/cart.py`|Extends Cart with `json_gift_cards` column, gift card association proxy, purchase tracking, discount deductions per gift card, and validation methods.| |**Gift Card Purchase & Routes** `make_post_sell/routes.py`, `templates/gift_card.j2`, `static/js/gift_card.js`|New routes for gift card page, add-to-cart, apply, and removal. Template with slider/input controls for amount selection and recipient details. Route handling not shown in diff.| |**Gift Card Management UI** `templates/gift_card_manage.j2`, `gift_card_detail.j2`|Admin templates for listing gift cards with totals, status badges, toggle controls, and transaction history display.| |**Shop Model Extensions** `make_post_sell/models/shop.py`|Adds gift card settings, environment classification (production/staging/development), trial tracking (started, ended, active), and BYOB primary S3 configuration with CDN endpoint.| |**BYOB S3 Implementation** `make_post_sell/request_methods.py`, `lib/karaoke.py`, `lib/s3_mirror.py`|Shop-aware S3 client selection via `shop_uploads_client` and `shop_bucket_name` request methods. Defers S3 client creation to post-shop-load in backfill operations. Adds domain check for 127.0.0.1.| |**Design System & Guidelines** `docs/design-system.md`, `CLAUDE.md`|Comprehensive design system documentation covering tokens, typography, spacing, components, and CSS conventions. CLAUDE.md reinforces design/test/BYOB guidelines with post-work checklists.| |**Architecture & Ticket Documentation** `docs/architecture.md`, `docs/tickets/mps-10.md` through `mps-16.md`|Updates architecture diagram for gift cards and BYOB. Detailed specifications for gift card models, purchase flow, redemption, admin settings, shop environment, trial system, and BYOB feature.| |**Database Migrations** `make_post_sell/scripts/alembic/versions/f8201a9ba045_...`, `9884324a48e3_...`|Gift card tables (`mps_gift_card`, `mps_gift_card_transaction`, `mps_cart_gift_card`), cart `json_gift_cards` column, shop settings, and environment/trial/primary S3 columns with idempotent guards.| |**Template CDN Endpoint Updates** `templates/base.j2`, `cart.j2`, `content.j2`, `home.j2`, `product_edit.j2`, `shop.j2`, `shop_about.j2`|Replaces `request.app["bucket.secure_uploads.get_endpoint"]` with `request.shop_cdn_endpoint` for all media/thumbnail URLs. Adds environment and trial banners to base template.| |**Email & Service Updates** `make_post_sell/lib/mail.py`, `lib/digest_sender.py`|New `send_gift_card_email()` function with amount, code, message, and redemption instructions. Digest sender filters to production shops only (`Shop.environment == 0`).| |**Frontend State Management** `make_post_sell/static/js/watch.js`|Adds ring state staleness detection and server-seeding on startup; fetches current product from server if localStorage ring data is empty; fallback to existing preloadNext on fetch failure.| |**Model & Meta Updates** `make_post_sell/models/__init__.py`, `meta.py`, `product.py`|Exports new gift card model classes. Adds CLASS_TO_TABLE mappings for GiftCard, GiftCardTransaction, CartGiftCard. Updates product search to exclude non-production shops via environment filter.| |**Admin UI** `templates/actions_view.j2`|Conditional "Gift Cards" management link when `request.shop.gift_card_enabled`.| ## Sequence Diagram ```mermaid sequenceDiagram participant Client as Client (Browser) participant Server as Server (Routes/Cart) participant DB as Database participant Checkout as Checkout View participant Email as Email Service Client->>Server: POST /gift-card/apply (gift_card_code, cart_id) Server->>DB: Fetch GiftCard by code DB-->>Server: GiftCard (balance, validity check) Server->>Server: Validate (enabled, balance > 0, shop match) Server->>DB: Add CartGiftCard entry DB-->>Server: Success Server->>Server: Recalculate cart.discounted_totals (apply after coupons) Server-->>Client: Updated cart with gift card deduction Client->>Server: POST /checkout (finalize) Server->>Checkout: Process payment Checkout->>DB: Create GiftCardTransaction Checkout->>DB: Deduct balance from GiftCard DB-->>Checkout: Transaction & balance updated Checkout->>Email: Send redemption confirmation Email-->>Client: Email to purchaser Server-->>Client: Order confirmation ``` ## Estimated code review effort 🎯 4 (Complex) | ⏱️ ~50 minutes --- ## Poem > 🎁 A gift card hops with joy today, > Through shop and cart in every way, > With buckets bright and trials new, > The MPS burrows made all true! > Five features strong, the rabbit sings— > What wonderful commerce spring brings! 🐰 </details>   <details> <summary>🚥 Pre-merge checks | ✅ 2 | ❌ 1</summary> ### ❌ Failed checks (1 warning) | Check name | Status | Explanation | Resolution | | :----------------: | :--------- | :------------------------------------------------------------------------------------ | :--------------------------------------------------------------------------------- | | Docstring Coverage | ⚠️ Warning | Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | <details> <summary>✅ Passed checks (2 passed)</summary> | Check name | Status | Explanation | | :---------------: | :------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | | Title check | ✅ Passed | The PR title accurately describes the primary change: integrating design tokens from the styleguide into the codebase. It is specific, concise, and directly relates to the main objective of establishing tokens.css as a foundation layer. | </details> ✏️ Tip: You can configure your own custom pre-merge checks in the settings. </details>   <details> <summary>✨ Finishing Touches</summary> 📝 Generate docstrings: use the `@coderabbitai generate docstrings` command. </details>   > 💡 **TIP** > Try [Coding Plans](https://www.coderabbit.ai/issue-planner). Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). > Share your feedback on [Discord](https://discord.com/invite/coderabbit).   --- Comment `@coderabbitai help` to get the list of available commands and usage tips.

coderabbitai commented

2026-03-07 23:30:13 -05:00

(Migrated from git2.unturf.com)

changed the description

coderabbitai commented

2026-03-07 23:38:50 -05:00

(Migrated from git2.unturf.com)

Actionable comments posted: 14

📝 NOTE
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🔥 CAUTION
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
make_post_sell/models/cart.py (1)
51-67: ⚠️ Potential issue | 🟠 Major

Wire the new gift-card state into cart reset and merge flows.

After this change a cart carries both json_gift_cards and attached gift_cards, but empty() still only clears json_cart and merge_in_cart() still only merges products/coupons. That leaves stale gift cards in an “empty” cart and drops them during cart merges.

Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/cart.py` around lines 51 - 67, The cart now has both
json_gift_cards and association_proxy gift_cards but empty() and merge_in_cart()
ignore them; update empty() to clear json_gift_cards and remove all associated
CartGiftCard entries (via the cart_gift_cards relationship or by clearing the
gift_cards proxy) so an "empty" cart has no gift cards, and update
merge_in_cart() (and the equivalent logic referenced at lines ~79-80) to also
transfer/merge gift cards: merge json_gift_cards arrays and attach/deduplicate
CartGiftCard objects to the destination cart (using the CartGiftCard/ gift_cards
proxy) just like you do for coupons/products so gift cards aren’t left behind or
dropped during merges.

🟡 Minor comments (9)

docs/design-system.md-9-52 (1)
9-52: ⚠️ Potential issue | 🟡 Minor

Add languages to the fenced examples.

markdownlint is already flagging these blocks with MD040. Using text for diagrams/tables and the concrete language for actual snippets will keep docs checks clean and improve syntax highlighting.

Also applies to: 67-77, 81-94, 98-114, 118-126, 130-137, 141-151, 155-163, 167-173, 179-187, 304-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/design-system.md` around lines 9 - 52, The fenced code blocks in the
design-system.md examples (e.g., the ASCII diagram showing tokens.css →
common.css → templates/ and other snippet blocks) lack language identifiers and
trigger markdownlint MD040; update each triple-backtick fence to include an
appropriate language tag (use text for diagrams/tables and the concrete language
like css, jinja, or html for actual snippets) across the affected example blocks
so syntax highlighting and linting pass (apply this to the diagram block
containing tokens.css/common.css/templates and the other listed example blocks).
make_post_sell/static/js/watch.js-221-228 (1)
221-228: ⚠️ Potential issue | 🟡 Minor

Preserve the current item when resetting a stale ring.

This clears ringHistory immediately after markWatched(initialId), so a stale-ring load forgets that the current product was already watched. The visible effect is an incorrect 0 / N progress state until the user loops back to this item.
Suggested fix
 if (ringProductIds.length && ringProductIds.indexOf(initialId) === -1) {
 ringProductIds = [];
 ringPosition = 0;
 ringHistory = {};
+ ringHistory[initialId] = true;
 ringLoops = 0;
 saveRingState();
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/static/js/watch.js` around lines 221 - 228, When you detect a
stale ring, don't wipe out the watched state for the currently viewed product
(initialId). Modify the reset so that ringHistory preserves the entry for
initialId (e.g. set ringHistory = ringHistory[initialId] ? { [initialId]:
ringHistory[initialId] } : {}), or alternatively clear ringHistory then
immediately call markWatched(initialId) before saveRingState(); keep resetting
ringProductIds, ringPosition and ringLoops as before and then call
saveRingState().
make_post_sell/templates/gift_card_detail.j2-59-59 (1)
59-59: ⚠️ Potential issue | 🟡 Minor

Render a human-readable transaction date here.

txn.created_timestamp is the raw millisecond DB value, so this will show a 13-digit epoch in the UI instead of a usable date/time. Pass a formatted string from the view or expose a display property on the model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/templates/gift_card_detail.j2` at line 59, The template is
rendering txn.created_timestamp (a raw millisecond epoch) which will show a
13-digit number; change the rendering to use a human-readable string by either
formatting the timestamp in the view and passing a new property (e.g.,
txn.created_at_display) or add a display property on the model that returns a
formatted datetime, then update the template to render that property instead of
txn.created_timestamp; ensure the formatter uses your app's timezone/locale
conventions so the UI shows a readable date/time.
docs/tickets/mps-14.md-10-10 (1)
10-10: ⚠️ Potential issue | 🟡 Minor

Clarify whether the two extra shops require a paid production seat.

Line 10 and Line 105 say the allowance is tied to a paid production shop, but the counting example grants it for any production shop. MPS-15 also says trial shops can create two dev/stage shops, so this needs one consistent rule before implementation/tests are written.

Also applies to: 103-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-14.md` at line 10, The statement "Every paid production shop
seat includes 2 free dev/stage shops." is inconsistent with the counting example
and MPS-15; update the policy text and all examples to state a single rule:
either (A) the two extra dev/stage shops are granted only for paid production
seats, or (B) they are granted for any production shop including trials — choose
one and apply it consistently. Specifically, edit the sentence "Every paid
production shop seat includes 2 free dev/stage shops." and the related "counting
example" and the MPS-15 reference so they all use the same rule and adjust any
example calculations that assume the opposite behavior; ensure the document
explicitly calls out whether trial production shops qualify and remove
conflicting phrasing in the passages currently using "paid" vs "any".
docs/tickets/mps-13.md-25-26 (1)
25-26: ⚠️ Potential issue | 🟡 Minor

These route examples don't match the URLs used by the new templates.

make_post_sell/templates/gift_card_detail.j2 already links to /s/{{ request.shop.uuid_str }}/gift-cards/..., so keeping /shop/{slug}/... here will send implementation and tests toward the wrong route shape.
💡 Proposed fix
-New route: `/shop/{slug}/gift-cards/manage` (shop owner only)
+New route: `/s/{shop_id}/gift-cards/manage` (shop owner only)
@@
-`/shop/{slug}/gift-cards/{card_id}` (shop owner only)
+`/s/{shop_id}/gift-cards/{card_id}` (shop owner only)
@@
-On the gift card purchase page (`/shop/{slug}/gift-card`), add a "Check Balance"
+On the gift card purchase page (`/s/{shop_id}/gift-card`), add a "Check Balance"
Also applies to: 40-40, 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-13.md` around lines 25 - 26, The documented route examples
use /shop/{slug}/gift-cards/manage but the templates
(make_post_sell/templates/gift_card_detail.j2) and links use /s/{{
request.shop.uuid_str }}/gift-cards/..., so update the docs to the template URL
shape (e.g., /s/{shop_uuid}/gift-cards/manage) and adjust the example
routes/tests/implementation guidance to that shape; also fix the other two
occurrences of the incorrect /shop/{slug}/... examples mentioned in the comment
so all examples match the template URL format.
docs/tickets/mps-15.md-116-129 (1)
116-129: ⚠️ Potential issue | 🟡 Minor

Use uuid_str in the banner links.

The rest of this PR builds shop URLs with request.shop.uuid_str, so documenting request.shop.id here will propagate the wrong pattern into base.j2 and tests.
💡 Proposed fix
 {% if request.shop and request.shop.is_trial_active and request.user in request.shop.owners %}
 <div class="trial-banner">
 Free trial: {{ request.shop.trial_days_remaining }} days remaining.
- <a href="/s/{{ request.shop.id }}/settings#plan">Choose a plan</a>
+ <a href="/s/{{ request.shop.uuid_str }}/settings#plan">Choose a plan</a>
 </div>
 {% endif %}
 
 {% if request.shop and request.shop.is_trial_expired and not request.shop.plan_active %}
 <div class="trial-banner trial-expired">
 Your 21-day trial has expired.
- <a href="/s/{{ request.shop.id }}/settings#plan">Choose a plan to keep selling</a>
+ <a href="/s/{{ request.shop.uuid_str }}/settings#plan">Choose a plan to keep selling</a>
 </div>
 {% endif %}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-15.md` around lines 116 - 129, The banner link URLs use
request.shop.id but should use request.shop.uuid_str to match the rest of the
codebase; update both anchor hrefs in the trial banner blocks to build URLs with
request.shop.uuid_str (the conditional checks request.shop,
request.shop.is_trial_active, request.shop.owners,
request.shop.is_trial_expired, and request.shop.plan_active remain unchanged) so
templates and tests use the UUID string pattern consistently.
docs/tickets/mps-16.md-31-49 (1)
31-49: ⚠️ Potential issue | 🟡 Minor

Document the final BYOB schema, not both alternatives.

This ticket still presents mutually exclusive designs (mirror_s3_* reuse vs dedicated primary_s3_* columns), but the implementation chose the dedicated columns. Leaving both here makes the reference ambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-16.md` around lines 31 - 49, The docs currently show two
mutually exclusive BYOB schema designs (reusing mirror_s3_* vs new primary_s3_*
columns); update the document to remove the alternative and document only the
chosen schema: list and describe the final primary_s3_* columns
(primary_s3_endpoint, primary_s3_region, primary_s3_bucket,
primary_s3_access_key, primary_s3_secret_key, primary_s3_cdn_endpoint) and the
primary_s3_enabled flag, clarifying semantics (when primary_s3_enabled is true
these primary_* fields are used as the primary bucket) and remove any mention of
reusing mirror_s3_* to avoid ambiguity.
docs/tickets/mps-11.md-31-32 (1)
31-32: ⚠️ Potential issue | 🟡 Minor

Drop the “Option A” wording now that the schema is fixed.

This ticket already names json_gift_cards in the changed files, so keeping the storage format as an option makes the reference doc look undecided.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-11.md` around lines 31 - 32, Remove the "Option A" hedging
and make the storage format definitive: replace the "Option A:" prefix and any
language suggesting alternatives with a declarative statement that Cart now
stores gift cards in the json_gift_cards column, and keep the format example
`[{"shop_id": "...", "amount_in_cents": 2500, "gift_email": "...",
"gift_message": "..."}]` as the canonical schema; ensure references to Cart and
json_gift_cards are updated to reflect the fixed schema and not presented as
optional.
docs/tickets/mps-10.md-36-37 (1)
36-37: ⚠️ Potential issue | 🟡 Minor

Align the sample code format with the actual generator.

secrets.token_hex(8).upper() only yields hex characters, so GC-A1B2C3D4E5F6G7H8 can never be produced. Either switch the generator or make the example hex-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-10.md` around lines 36 - 37, The example ticket code format
in the docs doesn't match the generator: secrets.token_hex(8).upper() produces
16 hex characters (0-9A-F) so remove the impossible alphanumeric letters from
the example or change the generator; either update the sample to "GC-" followed
by 16 hex chars (e.g., GC-<16_HEX_CHARS>) to match secrets.token_hex(8).upper(),
or replace secrets.token_hex(8).upper() with a generator that picks 16 chars
from string.ascii_uppercase + string.digits (e.g., using secrets.choice) so it
can produce values like GC-A1B2C3D4E5F6G7H8.

🧹 Nitpick comments (7)

make_post_sell/models/product.py (1)

680-685: Move the import outside the loop for efficiency.

The from .shop import Shop import (line 683) is inside the for keyword in keywords: loop, so it executes for every keyword when shop is None. While Python caches imports, the lookup still adds unnecessary overhead.

♻️ Proposed fix to move import outside loop

 def get_products_by_keywords(dbsession, keywords, shop=None):
     scores = {}
     hits = {}
+    
+    # Import Shop here to avoid circular import at module level
+    if not shop:
+        from .shop import Shop

     if shop:
         product_query = shop.products
     else:
         product_query = dbsession.query(Product)

     for keyword in keywords:
         keyword_filter = Product.title.ilike(f"%{keyword}%")
         # the product _must_ be public (1).
         query = product_query.filter(keyword_filter).filter(Product.visibility == 1)
         # Exclude non-production shops from search results (MPS-14)
         if not shop:
-            from .shop import Shop
             query = query.join(Shop, Product.shop_id == Shop.id).filter(Shop.environment == 0)
         products = query.all()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/product.py` around lines 680 - 685, The import of Shop
is inside the keywords loop which causes repeated lookups; move the "from .shop
import Shop" import out of the loop (place it above where product_query is used
or before the loop) and keep the existing logic that uses Shop only when shop is
falsy (the block that sets query = query.join(Shop, Product.shop_id ==
Shop.id).filter(Shop.environment == 0)); ensure references to
Product.visibility, product_query, shop, and keywords continue to work
unchanged.

make_post_sell/lib/s3_mirror.py (1)

232-248: Consider adding region fallback for consistency.

The _make_src helper uses shop.primary_s3_region directly (line 237), but this field may be None since has_primary_s3 doesn't check for it. The fallback path (lines 242-248) uses app_settings["bucket.secure_uploads.region"] which is guaranteed to exist, and _make_mirror_client uses region or "us-east-1" as a fallback (line 23).

For consistency with the mirror client pattern:

♻️ Proposed fix to add region fallback

             if shop and shop.has_primary_s3:
                 return boto3.session.Session().client(
                     "s3",
-                    region_name=shop.primary_s3_region,
+                    region_name=shop.primary_s3_region or "us-east-1",
                     endpoint_url=shop.primary_s3_endpoint,
                     aws_access_key_id=shop.primary_s3_access_key,
                     aws_secret_access_key=shop.primary_s3_secret_key,
                 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/lib/s3_mirror.py` around lines 232 - 248, The _make_src helper
uses shop.primary_s3_region directly even though has_primary_s3 may be true
while primary_s3_region is None; update _make_src to compute region =
shop.primary_s3_region or app_settings["bucket.secure_uploads.region"] and pass
that region into boto3.session.Session().client(...) so the source client falls
back consistently (matching the region fallback pattern used in
_make_mirror_client); reference _make_src, shop.primary_s3_region,
app_settings["bucket.secure_uploads.region"], and _make_mirror_client when
making the change.

make_post_sell/templates/gift_card.j2 (1)

16-34: Step value mismatch between slider and input may cause minor UX friction.

The range slider uses step="1" (line 20) while the numeric input uses step="0.01" (line 30). When the user types a decimal value like 25.50, the slider can only snap to 25 or 26, potentially confusing users expecting precise synchronization.

Consider aligning the step values or documenting this as intentional (slider for quick selection, input for precise amounts).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/templates/gift_card.j2` around lines 16 - 34, The slider (id
"gift-card-slider") uses step="1" while the numeric input (id
"gift-card-amount") uses step="0.01", causing desynchronization when users enter
decimal amounts; update the slider's step to "0.01" to match the input (or
change the input to "1" if intentional), and ensure any JS that syncs values
between gift-card-slider and gift-card-amount handles decimals consistently so
both controls reflect the same precision.

make_post_sell/lib/karaoke.py (1)

364-380: Consider restructuring _make_s3 to receive shop as a parameter.

The nested _make_s3 function references the outer shop variable, which creates an implicit dependency on execution order. The function is called both at line 388 (after shop is loaded) and at line 433 within worker threads. While this works because shop is loaded before any calls, passing shop as an explicit parameter would make the dependency clear and prevent accidental misuse.

♻️ Proposed refactor

-        def _make_s3():
-            # BYOB: use shop's own bucket if configured (MPS-16)
-            if shop and shop.has_primary_s3:
+        def _make_s3(shop_obj):
+            # BYOB: use shop's own bucket if configured (MPS-16)
+            if shop_obj and shop_obj.has_primary_s3:
                 return boto3.session.Session().client(
                     "s3",
-                    region_name=shop.primary_s3_region,
-                    endpoint_url=shop.primary_s3_endpoint,
-                    aws_access_key_id=shop.primary_s3_access_key,
-                    aws_secret_access_key=shop.primary_s3_secret_key,
+                    region_name=shop_obj.primary_s3_region,
+                    endpoint_url=shop_obj.primary_s3_endpoint,
+                    aws_access_key_id=shop_obj.primary_s3_access_key,
+                    aws_secret_access_key=shop_obj.primary_s3_secret_key,
                 )
             return boto3.session.Session().client(

Then update call sites:

-            s3 = _make_s3()
+            s3 = _make_s3(shop)

-                thread_s3 = _make_s3()
+                thread_s3 = _make_s3(shop)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/lib/karaoke.py` around lines 364 - 380, The nested helper
_make_s3 currently captures the outer shop variable; change its signature to
accept shop as an explicit parameter (e.g., def _make_s3(shop):) and update its
body to use that parameter instead of the outer variable while still falling
back to app_settings when shop is falsy; then update every call site that
invokes _make_s3 (including the post-shop-load call and the worker/thread
invocation) to pass the appropriate shop object (or None if using app_settings),
ensuring no code relies on the outer lexical capture of shop.

make_post_sell/lib/mail.py (2)

549-565: Remove extraneous f prefixes from strings without placeholders.

Several strings have f prefixes but contain no interpolated values. This triggers Ruff F541 errors and adds unnecessary overhead.

🔧 Proposed fix

     message_parts = [
         f"You received a {amount} gift card for {shop_name}!",
-        f"",
+        "",
         f"Your gift card code: {code}",
-        f"",
+        "",
     ]
     if gift_message:
         message_parts.append(f"Message: {gift_message}")
         message_parts.append("")
     message_parts.extend([
         f"To redeem, enter the code at checkout when shopping at {shop_name}.",
-        f"",
+        "",
         f"Visit: {shop_url}",
-        f"",
-        f"This gift card never expires.",
+        "",
+        "This gift card never expires.",
     ])

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/lib/mail.py` around lines 549 - 565, The message_parts
construction uses unnecessary f-string prefixes on literals without
interpolation (e.g., f"", f"This gift card never expires.", f"To redeem..."
etc.); update the list building around the message_parts variable so only
strings that actually include placeholders remain f-strings (keep f on lines
with {amount}, {shop_name}, {code}, {shop_url}, {gift_message}), and remove the
f prefix from empty strings and static sentences before joining into
message_text to eliminate Ruff F541 warnings.

567-577: Remove extraneous f prefixes in HTML parts.

Same issue in the HTML generation section.

🔧 Proposed fix

     html_parts = [
         f"<h2>You received a {amount} gift card for {shop_name}!</h2>",
-        f"<p><strong>Your gift card code:</strong></p>",
+        "<p><strong>Your gift card code:</strong></p>",
         f"<p style='font-size: 24px; font-family: monospace; background: `#f0f0f0`; padding: 12px; display: inline-block;'>{code}</p>",
     ]
     if gift_message:
         html_parts.append(f"<p><em>{gift_message}</em></p>")
     html_parts.extend([
         f"<p>To redeem, enter the code at checkout when shopping at <a href='{shop_url}'>{shop_name}</a>.</p>",
-        f"<p><small>This gift card never expires.</small></p>",
+        "<p><small>This gift card never expires.</small></p>",
     ])

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/lib/mail.py` around lines 567 - 577, The HTML parts list uses
unnecessary f-string prefixes for static strings; update html_parts so only
strings that interpolate variables use f-strings (keep f for entries that
reference amount, shop_name, code, shop_url, gift_message) and remove the
extraneous f prefix from static entries such as the "<p><small>This gift card
never expires.</small></p>" item (locate and edit the html_parts construction
and any nearby HTML generation that currently uses f"" without interpolation).

make_post_sell/models/cart_gift_card.py (1)

15-18: Consider adding a unique constraint on (cart_id, gift_card_id).

While the documentation mentions that validation happens at the view level (one gift card per shop), a database-level unique constraint would provide an additional safeguard against duplicate gift card applications to the same cart.

🔧 Proposed enhancement

+from sqlalchemy import UniqueConstraint
+
 class CartGiftCard(RBase, Base):
     """
     Many to many, Carts to GiftCards.
     A relationship signifies the application of a gift card to a cart.
     """
+    __table_args__ = (
+        UniqueConstraint('cart_id', 'gift_card_id', name='uq_cart_gift_card'),
+    )

     id = Column(UUIDType, primary_key=True, index=True)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/cart_gift_card.py` around lines 15 - 18, Add a DB-level
unique constraint to prevent duplicate gift-card entries per cart by updating
the CartGiftCard model: import and use SQLAlchemy's UniqueConstraint (e.g.,
UniqueConstraint("cart_id", "gift_card_id", name="uq_cart_gift_card")) in the
model's __table_args__ so the combination of cart_id and gift_card_id is
enforced as unique at the database level; ensure the import for UniqueConstraint
is added alongside Column/UUIDType and keep the existing columns (id, cart_id,
gift_card_id, created_timestamp) unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/tickets/mps-15.md`:
- Around line 27-30: The is_active logic will leave legacy shops inactive
because NULL `trial_started_timestamp` is treated as false and `plan_active`
defaults to 0; update the activation rule or migration so legacy shops remain
active: either change the boolean expression (used where `is_active` is computed
around lines 66-69) to treat NULL `trial_started_timestamp` as "paid" (e.g.,
`plan_active OR is_trial_active OR trial_started_timestamp IS NULL`) or add an
idempotent backfill/migration on `mps_shop` to set `plan_active=true` for
existing rows; reference the column names `trial_started_timestamp`,
`trial_ended`, and `plan_active` and the `is_active` computation when making the
change.

In `@make_post_sell/models/cart.py`:
- Around line 468-485: The validate_attached_gift_cards method currently doesn't
reject duplicate attachments, so the same gift card can be applied multiple
times; update validate_attached_gift_cards to detect duplicate gift_card.id
(and/or gift_card.code) across self.gift_cards and add an error like "Gift card
'<code>' is attached multiple times" when duplicates are found (in addition to
the existing disabled/balance/shop checks), returning these errors; also note to
add a unique (cart_id, gift_card_id) constraint later in
make_post_sell/models/cart_gift_card.py to enforce this at the DB level.

In `@make_post_sell/models/gift_card_transaction.py`:
- Around line 3-4: Reject non-positive redemption amounts by adding a guard in
the GiftCardTransaction constructor and a schema-level constraint: in the
GiftCardTransaction class, update its __init__ to validate that amount_in_cents
> 0 and raise a ValueError if not (refer to the amount_in_cents parameter), and
add a SQLAlchemy CheckConstraint like CheckConstraint("amount_in_cents > 0") on
the model so the DB enforces it as well; ensure imports are added
(CheckConstraint) and adjust any existing code paths that construct
GiftCardTransaction to expect the ValueError on invalid input.

In `@make_post_sell/models/gift_card.py`:
- Around line 80-84: The deduct method currently allows negative amounts and
performs a non-atomic read/modify/write on balance_in_cents; change deduct to
first validate amount_in_cents > 0 and raise a ValueError for non-positive
inputs, then perform the deduction using a DB-level atomic operation instead of
in-memory math: either run the update inside a transaction with a SELECT ... FOR
UPDATE on the GiftCard row and adjust balance_in_cents, or issue an atomic
UPDATE gift_cards SET balance_in_cents = balance_in_cents - :amt WHERE id = :id
AND balance_in_cents >= :amt and check affected rows to determine the actual
deducted amount; keep the method name deduct and its contract of returning the
actual deducted cents.

In `@make_post_sell/models/shop.py`:
- Around line 842-850: The async rebuild bypasses the non-production guard:
update reforge_discovery_ring_async to mirror reforge_discovery_ring by checking
shop.environment and shop.is_non_production before calling
compute_discovery_ring(), setting shop.discovery_ring = [] and returning [] for
non-production shops (or alternatively add the same guard at the start of
compute_discovery_ring()); ensure you reference and modify
reforge_discovery_ring_async (and/or compute_discovery_ring) so background
rebuilds do not repopulate discovery for staging/development shops.
- Around line 173-175: The trial_ended boolean is stored but never used; update
the shop state helpers to respect it: modify is_trial_active, is_trial_expired,
and is_active to check trial_ended (and trial_started_timestamp) so that when
trial_ended is True the trial is treated as ended/expired and the shop is
considered not active via trial. Ensure is_trial_active returns False if
trial_ended is True, is_trial_expired returns True if trial_ended is True, and
is_active uses those updated helpers so early-ended trials no longer grant
access.
- Around line 339-348: The has_primary_s3 property currently omits
primary_s3_region, so a shop can pass readiness while add_shop_uploads_client()
later supplies shop.primary_s3_region to boto3 and fail; update the
has_primary_s3 boolean check to also require self.primary_s3_region (include
primary_s3_region in the AND list) so the readiness check matches what
add_shop_uploads_client() expects and prevents creating a misconfigured S3
client.

In `@make_post_sell/request_methods.py`:
- Around line 158-185: The helpers add_shop_uploads_client,
add_shop_bucket_name, and add_shop_cdn_endpoint currently treat any truthy
shop.has_primary_s3 as enough and silently fall back to the platform bucket when
one or more BYOB fields are missing; instead validate that all required BYOB
fields are present before returning BYOB values (for add_shop_uploads_client
check primary_s3_region, primary_s3_endpoint, primary_s3_access_key,
primary_s3_secret_key; for add_shop_bucket_name check primary_s3_bucket; for
add_shop_cdn_endpoint check primary_s3_cdn_endpoint), and if validation fails
raise an explicit error (or log and abort) so misconfiguration is surfaced
rather than quietly returning request.secure_uploads_client or
request.app["bucket.secure_uploads"].

In
`@make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py`:
- Around line 73-83: The migration currently adds plaintext credential columns
primary_s3_access_key and primary_s3_secret_key to the mps_shop table via
op.add_column (guarded by _column_exists); instead, change the migration to add
a single encrypted_secret_reference (or primary_s3_secret_id) column (e.g.,
VARCHAR/UUID) that stores a reference/ID to a secrets vault or an
envelope-encrypted payload, and remove the direct plaintext columns; update any
code that will write to/read from primary_s3_access_key/primary_s3_secret_key to
use the new reference and fetch/decrypt secrets via your secret manager
(KMS/Vault) at runtime, and include a migration note to securely rotate/migrate
existing plaintext data to the secret store before dropping old columns if
needed.
- Around line 98-109: The downgrade() currently unconditionally calls
op.drop_column for many columns (e.g., "primary_s3_enabled",
"primary_s3_cdn_endpoint", "primary_s3_secret_key", "primary_s3_access_key",
"primary_s3_bucket", "primary_s3_region", "primary_s3_endpoint", "plan_active",
"trial_ended", "trial_started_timestamp", "environment"), which can break on
partially applied schemas; update downgrade() to mirror the tolerant behavior of
upgrade() by checking for each column's existence before dropping it (use the
Alembic op.get_bind()/sqlalchemy.inspect Inspector or a helper like has_column
to query the table schema), and only call op.drop_column for columns that
actually exist to avoid dropping columns that this revision didn't create.

In
`@make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py`:
- Around line 36-100: The upgrade() migration is missing the required
pre-migration SQLite backup; add a call at the very start of upgrade() to invoke
the project's backup helper (e.g., call a function like ensure_sqlite_backup()
or run_sqlite_backup()) before any DDL runs, and make that helper a no-op on
non-SQLite backends; update or create a small function (ensure_sqlite_backup /
run_sqlite_backup) that checks the current DB URL/driver, performs the
filesystem copy/backup when driver == "sqlite", and raises/logs on failure so
the migration aborts safely if the backup cannot be made.
- Around line 102-108: The downgrade() fails to fully reverse upgrade() (it
never removes mps_cart.json_gift_cards) and naively drops tables/columns which
will error on partially-applied DBs; update downgrade() to drop the
mps_cart.json_gift_cards column and guard each drop with an existence check (use
SQLAlchemy Inspector or context.get_bind() to verify table/column existence)
before calling op.drop_table or op.drop_column for mps_cart_gift_card,
mps_gift_card_transaction, mps_gift_card, and the mps_shop columns
gift_card_enabled, gift_card_min_in_cents, gift_card_max_in_cents so the
rollback is idempotent and fully reverses upgrade().

In `@make_post_sell/static/js/watch.js`:
- Around line 1841-1856: The async fetchWatchData callback can overwrite
ringProductIds when the user navigates before the response arrives; to fix,
capture the product id at request time (e.g., const requestedId =
currentProductId) before calling fetchWatchData and, inside the .then handler,
verify the live currentProductId still equals requestedId (or that data is
tagged for requestedId) before assigning ringProductIds, calling
syncRingPosition, saveRingState, or updateProgressDisplay; if it doesn't match,
ignore the stale response and still call preloadNext as needed.

In `@make_post_sell/templates/content.j2`:
- Around line 42-45: The media URL generation in watch.py currently returns
presigned S3 URLs (via generate_presigned_url()) for media_url,
instrumentals_url, and vocals_url causing a scheme mismatch with templates that
expect persistent CDN URLs; update watch.py to construct these three URLs using
request.shop_cdn_endpoint the same way file_url and thumbnail_url are built
(i.e., combine request.shop_cdn_endpoint with product.s3_path and the
appropriate path/filename pattern used for product media) instead of calling
generate_presigned_url(), ensuring media_url, instrumentals_url, and vocals_url
are consistent CDN endpoints that won’t expire.

---

Outside diff comments:
In `@make_post_sell/models/cart.py`:
- Around line 51-67: The cart now has both json_gift_cards and association_proxy
gift_cards but empty() and merge_in_cart() ignore them; update empty() to clear
json_gift_cards and remove all associated CartGiftCard entries (via the
cart_gift_cards relationship or by clearing the gift_cards proxy) so an "empty"
cart has no gift cards, and update merge_in_cart() (and the equivalent logic
referenced at lines ~79-80) to also transfer/merge gift cards: merge
json_gift_cards arrays and attach/deduplicate CartGiftCard objects to the
destination cart (using the CartGiftCard/ gift_cards proxy) just like you do for
coupons/products so gift cards aren’t left behind or dropped during merges.

---

Minor comments:
In `@docs/design-system.md`:
- Around line 9-52: The fenced code blocks in the design-system.md examples
(e.g., the ASCII diagram showing tokens.css → common.css → templates/ and other
snippet blocks) lack language identifiers and trigger markdownlint MD040; update
each triple-backtick fence to include an appropriate language tag (use text for
diagrams/tables and the concrete language like css, jinja, or html for actual
snippets) across the affected example blocks so syntax highlighting and linting
pass (apply this to the diagram block containing tokens.css/common.css/templates
and the other listed example blocks).

In `@docs/tickets/mps-10.md`:
- Around line 36-37: The example ticket code format in the docs doesn't match
the generator: secrets.token_hex(8).upper() produces 16 hex characters (0-9A-F)
so remove the impossible alphanumeric letters from the example or change the
generator; either update the sample to "GC-" followed by 16 hex chars (e.g.,
GC-<16_HEX_CHARS>) to match secrets.token_hex(8).upper(), or replace
secrets.token_hex(8).upper() with a generator that picks 16 chars from
string.ascii_uppercase + string.digits (e.g., using secrets.choice) so it can
produce values like GC-A1B2C3D4E5F6G7H8.

In `@docs/tickets/mps-11.md`:
- Around line 31-32: Remove the "Option A" hedging and make the storage format
definitive: replace the "Option A:" prefix and any language suggesting
alternatives with a declarative statement that Cart now stores gift cards in the
json_gift_cards column, and keep the format example `[{"shop_id": "...",
"amount_in_cents": 2500, "gift_email": "...", "gift_message": "..."}]` as the
canonical schema; ensure references to Cart and json_gift_cards are updated to
reflect the fixed schema and not presented as optional.

In `@docs/tickets/mps-13.md`:
- Around line 25-26: The documented route examples use
 /shop/{slug}/gift-cards/manage but the templates
(make_post_sell/templates/gift_card_detail.j2) and links use /s/{{
request.shop.uuid_str }}/gift-cards/..., so update the docs to the template URL
shape (e.g., /s/{shop_uuid}/gift-cards/manage) and adjust the example
routes/tests/implementation guidance to that shape; also fix the other two
occurrences of the incorrect /shop/{slug}/... examples mentioned in the comment
so all examples match the template URL format.

In `@docs/tickets/mps-14.md`:
- Line 10: The statement "Every paid production shop seat includes 2 free
dev/stage shops." is inconsistent with the counting example and MPS-15; update
the policy text and all examples to state a single rule: either (A) the two
extra dev/stage shops are granted only for paid production seats, or (B) they
are granted for any production shop including trials — choose one and apply it
consistently. Specifically, edit the sentence "Every paid production shop seat
includes 2 free dev/stage shops." and the related "counting example" and the
MPS-15 reference so they all use the same rule and adjust any example
calculations that assume the opposite behavior; ensure the document explicitly
calls out whether trial production shops qualify and remove conflicting phrasing
in the passages currently using "paid" vs "any".

In `@docs/tickets/mps-15.md`:
- Around line 116-129: The banner link URLs use request.shop.id but should use
request.shop.uuid_str to match the rest of the codebase; update both anchor
hrefs in the trial banner blocks to build URLs with request.shop.uuid_str (the
conditional checks request.shop, request.shop.is_trial_active,
request.shop.owners, request.shop.is_trial_expired, and request.shop.plan_active
remain unchanged) so templates and tests use the UUID string pattern
consistently.

In `@docs/tickets/mps-16.md`:
- Around line 31-49: The docs currently show two mutually exclusive BYOB schema
designs (reusing mirror_s3_* vs new primary_s3_* columns); update the document
to remove the alternative and document only the chosen schema: list and describe
the final primary_s3_* columns (primary_s3_endpoint, primary_s3_region,
primary_s3_bucket, primary_s3_access_key, primary_s3_secret_key,
primary_s3_cdn_endpoint) and the primary_s3_enabled flag, clarifying semantics
(when primary_s3_enabled is true these primary_* fields are used as the primary
bucket) and remove any mention of reusing mirror_s3_* to avoid ambiguity.

In `@make_post_sell/static/js/watch.js`:
- Around line 221-228: When you detect a stale ring, don't wipe out the watched
state for the currently viewed product (initialId). Modify the reset so that
ringHistory preserves the entry for initialId (e.g. set ringHistory =
ringHistory[initialId] ? { [initialId]: ringHistory[initialId] } : {}), or
alternatively clear ringHistory then immediately call markWatched(initialId)
before saveRingState(); keep resetting ringProductIds, ringPosition and
ringLoops as before and then call saveRingState().

In `@make_post_sell/templates/gift_card_detail.j2`:
- Line 59: The template is rendering txn.created_timestamp (a raw millisecond
epoch) which will show a 13-digit number; change the rendering to use a
human-readable string by either formatting the timestamp in the view and passing
a new property (e.g., txn.created_at_display) or add a display property on the
model that returns a formatted datetime, then update the template to render that
property instead of txn.created_timestamp; ensure the formatter uses your app's
timezone/locale conventions so the UI shows a readable date/time.

---

Nitpick comments:
In `@make_post_sell/lib/karaoke.py`:
- Around line 364-380: The nested helper _make_s3 currently captures the outer
shop variable; change its signature to accept shop as an explicit parameter
(e.g., def _make_s3(shop):) and update its body to use that parameter instead of
the outer variable while still falling back to app_settings when shop is falsy;
then update every call site that invokes _make_s3 (including the post-shop-load
call and the worker/thread invocation) to pass the appropriate shop object (or
None if using app_settings), ensuring no code relies on the outer lexical
capture of shop.

In `@make_post_sell/lib/mail.py`:
- Around line 549-565: The message_parts construction uses unnecessary f-string
prefixes on literals without interpolation (e.g., f"", f"This gift card never
expires.", f"To redeem..." etc.); update the list building around the
message_parts variable so only strings that actually include placeholders remain
f-strings (keep f on lines with {amount}, {shop_name}, {code}, {shop_url},
{gift_message}), and remove the f prefix from empty strings and static sentences
before joining into message_text to eliminate Ruff F541 warnings.
- Around line 567-577: The HTML parts list uses unnecessary f-string prefixes
for static strings; update html_parts so only strings that interpolate variables
use f-strings (keep f for entries that reference amount, shop_name, code,
shop_url, gift_message) and remove the extraneous f prefix from static entries
such as the "<p><small>This gift card never expires.</small></p>" item (locate
and edit the html_parts construction and any nearby HTML generation that
currently uses f"" without interpolation).

In `@make_post_sell/lib/s3_mirror.py`:
- Around line 232-248: The _make_src helper uses shop.primary_s3_region directly
even though has_primary_s3 may be true while primary_s3_region is None; update
_make_src to compute region = shop.primary_s3_region or
app_settings["bucket.secure_uploads.region"] and pass that region into
boto3.session.Session().client(...) so the source client falls back consistently
(matching the region fallback pattern used in _make_mirror_client); reference
_make_src, shop.primary_s3_region, app_settings["bucket.secure_uploads.region"],
and _make_mirror_client when making the change.

In `@make_post_sell/models/cart_gift_card.py`:
- Around line 15-18: Add a DB-level unique constraint to prevent duplicate
gift-card entries per cart by updating the CartGiftCard model: import and use
SQLAlchemy's UniqueConstraint (e.g., UniqueConstraint("cart_id", "gift_card_id",
name="uq_cart_gift_card")) in the model's __table_args__ so the combination of
cart_id and gift_card_id is enforced as unique at the database level; ensure the
import for UniqueConstraint is added alongside Column/UUIDType and keep the
existing columns (id, cart_id, gift_card_id, created_timestamp) unchanged.

In `@make_post_sell/models/product.py`:
- Around line 680-685: The import of Shop is inside the keywords loop which
causes repeated lookups; move the "from .shop import Shop" import out of the
loop (place it above where product_query is used or before the loop) and keep
the existing logic that uses Shop only when shop is falsy (the block that sets
query = query.join(Shop, Product.shop_id == Shop.id).filter(Shop.environment ==
0)); ensure references to Product.visibility, product_query, shop, and keywords
continue to work unchanged.

In `@make_post_sell/templates/gift_card.j2`:
- Around line 16-34: The slider (id "gift-card-slider") uses step="1" while the
numeric input (id "gift-card-amount") uses step="0.01", causing
desynchronization when users enter decimal amounts; update the slider's step to
"0.01" to match the input (or change the input to "1" if intentional), and
ensure any JS that syncs values between gift-card-slider and gift-card-amount
handles decimals consistently so both controls reflect the same precision.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ffa4c162-a0ca-4566-b877-4c0c156a88e9

📥 Commits

Reviewing files that changed from the base of the PR and between 32a19a339c and cca3172c9b.

📒 Files selected for processing (62)

CLAUDE.md
GIT_HASH
docs/architecture.md
docs/design-system.md
docs/tickets/mps-10.md
docs/tickets/mps-11.md
docs/tickets/mps-12.md
docs/tickets/mps-13.md
docs/tickets/mps-14.md
docs/tickets/mps-15.md
docs/tickets/mps-16.md
make_post_sell/GIT_HASH
make_post_sell/lib/digest_sender.py
make_post_sell/lib/karaoke.py
make_post_sell/lib/mail.py
make_post_sell/lib/s3_mirror.py
make_post_sell/models/__init__.py
make_post_sell/models/cart.py
make_post_sell/models/cart_gift_card.py
make_post_sell/models/gift_card.py
make_post_sell/models/gift_card_transaction.py
make_post_sell/models/meta.py
make_post_sell/models/product.py
make_post_sell/models/shop.py
make_post_sell/request_methods.py
make_post_sell/routes.py
make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py
make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py
make_post_sell/static/css/common.css
make_post_sell/static/css/tokens.css
make_post_sell/static/js/watch.js
make_post_sell/templates/actions_view.j2
make_post_sell/templates/base.j2
make_post_sell/templates/cart.j2
make_post_sell/templates/content.j2
make_post_sell/templates/gift_card.j2
make_post_sell/templates/gift_card_detail.j2
make_post_sell/templates/gift_card_manage.j2
make_post_sell/templates/home.j2
make_post_sell/templates/join-or-log-in.j2
make_post_sell/templates/product.j2
make_post_sell/templates/product_edit.j2
make_post_sell/templates/shop.j2
make_post_sell/templates/shop_about.j2
make_post_sell/templates/shop_new.j2
make_post_sell/templates/shop_settings.j2
make_post_sell/templates/snippets/footer.j2
make_post_sell/templates/snippets/related_content.j2
make_post_sell/templates/styleguide.j2
make_post_sell/templates/user_purchases.j2
make_post_sell/tests/test_functional.py
make_post_sell/tests/test_integration.py
make_post_sell/tests/test_models.py
make_post_sell/views/__init__.py
make_post_sell/views/cart.py
make_post_sell/views/content.py
make_post_sell/views/feeds.py
make_post_sell/views/gift_card.py
make_post_sell/views/player.py
make_post_sell/views/product.py
make_post_sell/views/shop.py
make_post_sell/views/watch.py

**Actionable comments posted: 14** > 📝 **NOTE** > Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments. > 🔥 **CAUTION** > Some comments are outside the diff and can’t be posted inline due to platform limitations. > > > > <details> > <summary>⚠️ Outside diff range comments (1)</summary><blockquote> > > <details> > <summary>make_post_sell/models/cart.py (1)</summary><blockquote> > > `51-67`: _⚠️ Potential issue_ | _🟠 Major_ > > **Wire the new gift-card state into cart reset and merge flows.** > > After this change a cart carries both `json_gift_cards` and attached `gift_cards`, but `empty()` still only clears `json_cart` and `merge_in_cart()` still only merges products/coupons. That leaves stale gift cards in an “empty” cart and drops them during cart merges. > > > > Also applies to: 79-80 > > <details> > <summary>🤖 Prompt for AI Agents</summary> > > ``` > Verify each finding against the current code and only fix it if needed. > > In `@make_post_sell/models/cart.py` around lines 51 - 67, The cart now has both > json_gift_cards and association_proxy gift_cards but empty() and merge_in_cart() > ignore them; update empty() to clear json_gift_cards and remove all associated > CartGiftCard entries (via the cart_gift_cards relationship or by clearing the > gift_cards proxy) so an "empty" cart has no gift cards, and update > merge_in_cart() (and the equivalent logic referenced at lines ~79-80) to also > transfer/merge gift cards: merge json_gift_cards arrays and attach/deduplicate > CartGiftCard objects to the destination cart (using the CartGiftCard/ gift_cards > proxy) just like you do for coupons/products so gift cards aren’t left behind or > dropped during merges. > ``` > > </details> > > </blockquote></details> > > </blockquote></details> <details> <summary>🟡 Minor comments (9)</summary><blockquote> <details> <summary>docs/design-system.md-9-52 (1)</summary><blockquote> `9-52`: _⚠️ Potential issue_ | _🟡 Minor_ **Add languages to the fenced examples.** markdownlint is already flagging these blocks with MD040. Using `text` for diagrams/tables and the concrete language for actual snippets will keep docs checks clean and improve syntax highlighting. Also applies to: 67-77, 81-94, 98-114, 118-126, 130-137, 141-151, 155-163, 167-173, 179-187, 304-309 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/design-system.md` around lines 9 - 52, The fenced code blocks in the design-system.md examples (e.g., the ASCII diagram showing tokens.css → common.css → templates/ and other snippet blocks) lack language identifiers and trigger markdownlint MD040; update each triple-backtick fence to include an appropriate language tag (use text for diagrams/tables and the concrete language like css, jinja, or html for actual snippets) across the affected example blocks so syntax highlighting and linting pass (apply this to the diagram block containing tokens.css/common.css/templates and the other listed example blocks). ``` </details> </blockquote></details> <details> <summary>make_post_sell/static/js/watch.js-221-228 (1)</summary><blockquote> `221-228`: _⚠️ Potential issue_ | _🟡 Minor_ **Preserve the current item when resetting a stale ring.** This clears `ringHistory` immediately after `markWatched(initialId)`, so a stale-ring load forgets that the current product was already watched. The visible effect is an incorrect `0 / N` progress state until the user loops back to this item. <details> <summary>Suggested fix</summary> ```diff if (ringProductIds.length && ringProductIds.indexOf(initialId) === -1) { ringProductIds = []; ringPosition = 0; ringHistory = {}; + ringHistory[initialId] = true; ringLoops = 0; saveRingState(); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/static/js/watch.js` around lines 221 - 228, When you detect a stale ring, don't wipe out the watched state for the currently viewed product (initialId). Modify the reset so that ringHistory preserves the entry for initialId (e.g. set ringHistory = ringHistory[initialId] ? { [initialId]: ringHistory[initialId] } : {}), or alternatively clear ringHistory then immediately call markWatched(initialId) before saveRingState(); keep resetting ringProductIds, ringPosition and ringLoops as before and then call saveRingState(). ``` </details> </blockquote></details> <details> <summary>make_post_sell/templates/gift_card_detail.j2-59-59 (1)</summary><blockquote> `59-59`: _⚠️ Potential issue_ | _🟡 Minor_ **Render a human-readable transaction date here.** `txn.created_timestamp` is the raw millisecond DB value, so this will show a 13-digit epoch in the UI instead of a usable date/time. Pass a formatted string from the view or expose a display property on the model. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/templates/gift_card_detail.j2` at line 59, The template is rendering txn.created_timestamp (a raw millisecond epoch) which will show a 13-digit number; change the rendering to use a human-readable string by either formatting the timestamp in the view and passing a new property (e.g., txn.created_at_display) or add a display property on the model that returns a formatted datetime, then update the template to render that property instead of txn.created_timestamp; ensure the formatter uses your app's timezone/locale conventions so the UI shows a readable date/time. ``` </details> </blockquote></details> <details> <summary>docs/tickets/mps-14.md-10-10 (1)</summary><blockquote> `10-10`: _⚠️ Potential issue_ | _🟡 Minor_ **Clarify whether the two extra shops require a paid production seat.** Line 10 and Line 105 say the allowance is tied to a **paid** production shop, but the counting example grants it for **any** production shop. MPS-15 also says trial shops can create two dev/stage shops, so this needs one consistent rule before implementation/tests are written. Also applies to: 103-115 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-14.md` at line 10, The statement "Every paid production shop seat includes 2 free dev/stage shops." is inconsistent with the counting example and MPS-15; update the policy text and all examples to state a single rule: either (A) the two extra dev/stage shops are granted only for paid production seats, or (B) they are granted for any production shop including trials — choose one and apply it consistently. Specifically, edit the sentence "Every paid production shop seat includes 2 free dev/stage shops." and the related "counting example" and the MPS-15 reference so they all use the same rule and adjust any example calculations that assume the opposite behavior; ensure the document explicitly calls out whether trial production shops qualify and remove conflicting phrasing in the passages currently using "paid" vs "any". ``` </details> </blockquote></details> <details> <summary>docs/tickets/mps-13.md-25-26 (1)</summary><blockquote> `25-26`: _⚠️ Potential issue_ | _🟡 Minor_ **These route examples don't match the URLs used by the new templates.** `make_post_sell/templates/gift_card_detail.j2` already links to `/s/{{ request.shop.uuid_str }}/gift-cards/...`, so keeping `/shop/{slug}/...` here will send implementation and tests toward the wrong route shape. <details> <summary>💡 Proposed fix</summary> ```diff -New route: `/shop/{slug}/gift-cards/manage` (shop owner only) +New route: `/s/{shop_id}/gift-cards/manage` (shop owner only) @@ -`/shop/{slug}/gift-cards/{card_id}` (shop owner only) +`/s/{shop_id}/gift-cards/{card_id}` (shop owner only) @@ -On the gift card purchase page (`/shop/{slug}/gift-card`), add a "Check Balance" +On the gift card purchase page (`/s/{shop_id}/gift-card`), add a "Check Balance" ``` </details> Also applies to: 40-40, 55-56 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-13.md` around lines 25 - 26, The documented route examples use /shop/{slug}/gift-cards/manage but the templates (make_post_sell/templates/gift_card_detail.j2) and links use /s/{{ request.shop.uuid_str }}/gift-cards/..., so update the docs to the template URL shape (e.g., /s/{shop_uuid}/gift-cards/manage) and adjust the example routes/tests/implementation guidance to that shape; also fix the other two occurrences of the incorrect /shop/{slug}/... examples mentioned in the comment so all examples match the template URL format. ``` </details> </blockquote></details> <details> <summary>docs/tickets/mps-15.md-116-129 (1)</summary><blockquote> `116-129`: _⚠️ Potential issue_ | _🟡 Minor_ **Use `uuid_str` in the banner links.** The rest of this PR builds shop URLs with `request.shop.uuid_str`, so documenting `request.shop.id` here will propagate the wrong pattern into `base.j2` and tests. <details> <summary>💡 Proposed fix</summary> ```diff {% if request.shop and request.shop.is_trial_active and request.user in request.shop.owners %} <div class="trial-banner"> Free trial: {{ request.shop.trial_days_remaining }} days remaining. - <a href="/s/{{ request.shop.id }}/settings#plan">Choose a plan</a> + <a href="/s/{{ request.shop.uuid_str }}/settings#plan">Choose a plan</a> </div> {% endif %} {% if request.shop and request.shop.is_trial_expired and not request.shop.plan_active %} <div class="trial-banner trial-expired"> Your 21-day trial has expired. - <a href="/s/{{ request.shop.id }}/settings#plan">Choose a plan to keep selling</a> + <a href="/s/{{ request.shop.uuid_str }}/settings#plan">Choose a plan to keep selling</a> </div> {% endif %} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-15.md` around lines 116 - 129, The banner link URLs use request.shop.id but should use request.shop.uuid_str to match the rest of the codebase; update both anchor hrefs in the trial banner blocks to build URLs with request.shop.uuid_str (the conditional checks request.shop, request.shop.is_trial_active, request.shop.owners, request.shop.is_trial_expired, and request.shop.plan_active remain unchanged) so templates and tests use the UUID string pattern consistently. ``` </details> </blockquote></details> <details> <summary>docs/tickets/mps-16.md-31-49 (1)</summary><blockquote> `31-49`: _⚠️ Potential issue_ | _🟡 Minor_ **Document the final BYOB schema, not both alternatives.** This ticket still presents mutually exclusive designs (`mirror_s3_*` reuse vs dedicated `primary_s3_*` columns), but the implementation chose the dedicated columns. Leaving both here makes the reference ambiguous. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-16.md` around lines 31 - 49, The docs currently show two mutually exclusive BYOB schema designs (reusing mirror_s3_* vs new primary_s3_* columns); update the document to remove the alternative and document only the chosen schema: list and describe the final primary_s3_* columns (primary_s3_endpoint, primary_s3_region, primary_s3_bucket, primary_s3_access_key, primary_s3_secret_key, primary_s3_cdn_endpoint) and the primary_s3_enabled flag, clarifying semantics (when primary_s3_enabled is true these primary_* fields are used as the primary bucket) and remove any mention of reusing mirror_s3_* to avoid ambiguity. ``` </details> </blockquote></details> <details> <summary>docs/tickets/mps-11.md-31-32 (1)</summary><blockquote> `31-32`: _⚠️ Potential issue_ | _🟡 Minor_ **Drop the “Option A” wording now that the schema is fixed.** This ticket already names `json_gift_cards` in the changed files, so keeping the storage format as an option makes the reference doc look undecided. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-11.md` around lines 31 - 32, Remove the "Option A" hedging and make the storage format definitive: replace the "Option A:" prefix and any language suggesting alternatives with a declarative statement that Cart now stores gift cards in the json_gift_cards column, and keep the format example `[{"shop_id": "...", "amount_in_cents": 2500, "gift_email": "...", "gift_message": "..."}]` as the canonical schema; ensure references to Cart and json_gift_cards are updated to reflect the fixed schema and not presented as optional. ``` </details> </blockquote></details> <details> <summary>docs/tickets/mps-10.md-36-37 (1)</summary><blockquote> `36-37`: _⚠️ Potential issue_ | _🟡 Minor_ **Align the sample code format with the actual generator.** `secrets.token_hex(8).upper()` only yields hex characters, so `GC-A1B2C3D4E5F6G7H8` can never be produced. Either switch the generator or make the example hex-only. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-10.md` around lines 36 - 37, The example ticket code format in the docs doesn't match the generator: secrets.token_hex(8).upper() produces 16 hex characters (0-9A-F) so remove the impossible alphanumeric letters from the example or change the generator; either update the sample to "GC-" followed by 16 hex chars (e.g., GC-<16_HEX_CHARS>) to match secrets.token_hex(8).upper(), or replace secrets.token_hex(8).upper() with a generator that picks 16 chars from string.ascii_uppercase + string.digits (e.g., using secrets.choice) so it can produce values like GC-A1B2C3D4E5F6G7H8. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (7)</summary><blockquote> <details> <summary>make_post_sell/models/product.py (1)</summary><blockquote> `680-685`: **Move the import outside the loop for efficiency.** The `from .shop import Shop` import (line 683) is inside the `for keyword in keywords:` loop, so it executes for every keyword when `shop is None`. While Python caches imports, the lookup still adds unnecessary overhead. <details> <summary>♻️ Proposed fix to move import outside loop</summary> ```diff def get_products_by_keywords(dbsession, keywords, shop=None): scores = {} hits = {} + + # Import Shop here to avoid circular import at module level + if not shop: + from .shop import Shop if shop: product_query = shop.products else: product_query = dbsession.query(Product) for keyword in keywords: keyword_filter = Product.title.ilike(f"%{keyword}%") # the product _must_ be public (1). query = product_query.filter(keyword_filter).filter(Product.visibility == 1) # Exclude non-production shops from search results (MPS-14) if not shop: - from .shop import Shop query = query.join(Shop, Product.shop_id == Shop.id).filter(Shop.environment == 0) products = query.all() ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/product.py` around lines 680 - 685, The import of Shop is inside the keywords loop which causes repeated lookups; move the "from .shop import Shop" import out of the loop (place it above where product_query is used or before the loop) and keep the existing logic that uses Shop only when shop is falsy (the block that sets query = query.join(Shop, Product.shop_id == Shop.id).filter(Shop.environment == 0)); ensure references to Product.visibility, product_query, shop, and keywords continue to work unchanged. ``` </details> </blockquote></details> <details> <summary>make_post_sell/lib/s3_mirror.py (1)</summary><blockquote> `232-248`: **Consider adding region fallback for consistency.** The `_make_src` helper uses `shop.primary_s3_region` directly (line 237), but this field may be `None` since `has_primary_s3` doesn't check for it. The fallback path (lines 242-248) uses `app_settings["bucket.secure_uploads.region"]` which is guaranteed to exist, and `_make_mirror_client` uses `region or "us-east-1"` as a fallback (line 23). For consistency with the mirror client pattern: <details> <summary>♻️ Proposed fix to add region fallback</summary> ```diff if shop and shop.has_primary_s3: return boto3.session.Session().client( "s3", - region_name=shop.primary_s3_region, + region_name=shop.primary_s3_region or "us-east-1", endpoint_url=shop.primary_s3_endpoint, aws_access_key_id=shop.primary_s3_access_key, aws_secret_access_key=shop.primary_s3_secret_key, ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/lib/s3_mirror.py` around lines 232 - 248, The _make_src helper uses shop.primary_s3_region directly even though has_primary_s3 may be true while primary_s3_region is None; update _make_src to compute region = shop.primary_s3_region or app_settings["bucket.secure_uploads.region"] and pass that region into boto3.session.Session().client(...) so the source client falls back consistently (matching the region fallback pattern used in _make_mirror_client); reference _make_src, shop.primary_s3_region, app_settings["bucket.secure_uploads.region"], and _make_mirror_client when making the change. ``` </details> </blockquote></details> <details> <summary>make_post_sell/templates/gift_card.j2 (1)</summary><blockquote> `16-34`: **Step value mismatch between slider and input may cause minor UX friction.** The range slider uses `step="1"` (line 20) while the numeric input uses `step="0.01"` (line 30). When the user types a decimal value like `25.50`, the slider can only snap to `25` or `26`, potentially confusing users expecting precise synchronization. Consider aligning the step values or documenting this as intentional (slider for quick selection, input for precise amounts). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/templates/gift_card.j2` around lines 16 - 34, The slider (id "gift-card-slider") uses step="1" while the numeric input (id "gift-card-amount") uses step="0.01", causing desynchronization when users enter decimal amounts; update the slider's step to "0.01" to match the input (or change the input to "1" if intentional), and ensure any JS that syncs values between gift-card-slider and gift-card-amount handles decimals consistently so both controls reflect the same precision. ``` </details> </blockquote></details> <details> <summary>make_post_sell/lib/karaoke.py (1)</summary><blockquote> `364-380`: **Consider restructuring `_make_s3` to receive `shop` as a parameter.** The nested `_make_s3` function references the outer `shop` variable, which creates an implicit dependency on execution order. The function is called both at line 388 (after shop is loaded) and at line 433 within worker threads. While this works because shop is loaded before any calls, passing `shop` as an explicit parameter would make the dependency clear and prevent accidental misuse. <details> <summary>♻️ Proposed refactor</summary> ```diff - def _make_s3(): - # BYOB: use shop's own bucket if configured (MPS-16) - if shop and shop.has_primary_s3: + def _make_s3(shop_obj): + # BYOB: use shop's own bucket if configured (MPS-16) + if shop_obj and shop_obj.has_primary_s3: return boto3.session.Session().client( "s3", - region_name=shop.primary_s3_region, - endpoint_url=shop.primary_s3_endpoint, - aws_access_key_id=shop.primary_s3_access_key, - aws_secret_access_key=shop.primary_s3_secret_key, + region_name=shop_obj.primary_s3_region, + endpoint_url=shop_obj.primary_s3_endpoint, + aws_access_key_id=shop_obj.primary_s3_access_key, + aws_secret_access_key=shop_obj.primary_s3_secret_key, ) return boto3.session.Session().client( ``` Then update call sites: ```diff - s3 = _make_s3() + s3 = _make_s3(shop) ``` ```diff - thread_s3 = _make_s3() + thread_s3 = _make_s3(shop) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/lib/karaoke.py` around lines 364 - 380, The nested helper _make_s3 currently captures the outer shop variable; change its signature to accept shop as an explicit parameter (e.g., def _make_s3(shop):) and update its body to use that parameter instead of the outer variable while still falling back to app_settings when shop is falsy; then update every call site that invokes _make_s3 (including the post-shop-load call and the worker/thread invocation) to pass the appropriate shop object (or None if using app_settings), ensuring no code relies on the outer lexical capture of shop. ``` </details> </blockquote></details> <details> <summary>make_post_sell/lib/mail.py (2)</summary><blockquote> `549-565`: **Remove extraneous `f` prefixes from strings without placeholders.** Several strings have `f` prefixes but contain no interpolated values. This triggers Ruff F541 errors and adds unnecessary overhead. <details> <summary>🔧 Proposed fix</summary> ```diff message_parts = [ f"You received a {amount} gift card for {shop_name}!", - f"", + "", f"Your gift card code: {code}", - f"", + "", ] if gift_message: message_parts.append(f"Message: {gift_message}") message_parts.append("") message_parts.extend([ f"To redeem, enter the code at checkout when shopping at {shop_name}.", - f"", + "", f"Visit: {shop_url}", - f"", - f"This gift card never expires.", + "", + "This gift card never expires.", ]) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/lib/mail.py` around lines 549 - 565, The message_parts construction uses unnecessary f-string prefixes on literals without interpolation (e.g., f"", f"This gift card never expires.", f"To redeem..." etc.); update the list building around the message_parts variable so only strings that actually include placeholders remain f-strings (keep f on lines with {amount}, {shop_name}, {code}, {shop_url}, {gift_message}), and remove the f prefix from empty strings and static sentences before joining into message_text to eliminate Ruff F541 warnings. ``` </details> --- `567-577`: **Remove extraneous `f` prefixes in HTML parts.** Same issue in the HTML generation section. <details> <summary>🔧 Proposed fix</summary> ```diff html_parts = [ f"<h2>You received a {amount} gift card for {shop_name}!</h2>", - f"Your gift card code:", + "Your gift card code:", f"{code}", ] if gift_message: html_parts.append(f"{gift_message}") html_parts.extend([ f"To redeem, enter the code at checkout when shopping at <a href='{shop_url}'>{shop_name}</a>.", - f"This gift card never expires.", + "This gift card never expires.", ]) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/lib/mail.py` around lines 567 - 577, The HTML parts list uses unnecessary f-string prefixes for static strings; update html_parts so only strings that interpolate variables use f-strings (keep f for entries that reference amount, shop_name, code, shop_url, gift_message) and remove the extraneous f prefix from static entries such as the "This gift card never expires." item (locate and edit the html_parts construction and any nearby HTML generation that currently uses f"" without interpolation). ``` </details> </blockquote></details> <details> <summary>make_post_sell/models/cart_gift_card.py (1)</summary><blockquote> `15-18`: **Consider adding a unique constraint on `(cart_id, gift_card_id)`.** While the documentation mentions that validation happens at the view level (one gift card per shop), a database-level unique constraint would provide an additional safeguard against duplicate gift card applications to the same cart. <details> <summary>🔧 Proposed enhancement</summary> ```diff +from sqlalchemy import UniqueConstraint + class CartGiftCard(RBase, Base): """ Many to many, Carts to GiftCards. A relationship signifies the application of a gift card to a cart. """ + __table_args__ = ( + UniqueConstraint('cart_id', 'gift_card_id', name='uq_cart_gift_card'), + ) id = Column(UUIDType, primary_key=True, index=True) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/cart_gift_card.py` around lines 15 - 18, Add a DB-level unique constraint to prevent duplicate gift-card entries per cart by updating the CartGiftCard model: import and use SQLAlchemy's UniqueConstraint (e.g., UniqueConstraint("cart_id", "gift_card_id", name="uq_cart_gift_card")) in the model's __table_args__ so the combination of cart_id and gift_card_id is enforced as unique at the database level; ensure the import for UniqueConstraint is added alongside Column/UUIDType and keep the existing columns (id, cart_id, gift_card_id, created_timestamp) unchanged. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` Verify each finding against the current code and only fix it if needed. Inline comments: In `@docs/tickets/mps-15.md`: - Around line 27-30: The is_active logic will leave legacy shops inactive because NULL `trial_started_timestamp` is treated as false and `plan_active` defaults to 0; update the activation rule or migration so legacy shops remain active: either change the boolean expression (used where `is_active` is computed around lines 66-69) to treat NULL `trial_started_timestamp` as "paid" (e.g., `plan_active OR is_trial_active OR trial_started_timestamp IS NULL`) or add an idempotent backfill/migration on `mps_shop` to set `plan_active=true` for existing rows; reference the column names `trial_started_timestamp`, `trial_ended`, and `plan_active` and the `is_active` computation when making the change. In `@make_post_sell/models/cart.py`: - Around line 468-485: The validate_attached_gift_cards method currently doesn't reject duplicate attachments, so the same gift card can be applied multiple times; update validate_attached_gift_cards to detect duplicate gift_card.id (and/or gift_card.code) across self.gift_cards and add an error like "Gift card '<code>' is attached multiple times" when duplicates are found (in addition to the existing disabled/balance/shop checks), returning these errors; also note to add a unique (cart_id, gift_card_id) constraint later in make_post_sell/models/cart_gift_card.py to enforce this at the DB level. In `@make_post_sell/models/gift_card_transaction.py`: - Around line 3-4: Reject non-positive redemption amounts by adding a guard in the GiftCardTransaction constructor and a schema-level constraint: in the GiftCardTransaction class, update its __init__ to validate that amount_in_cents > 0 and raise a ValueError if not (refer to the amount_in_cents parameter), and add a SQLAlchemy CheckConstraint like CheckConstraint("amount_in_cents > 0") on the model so the DB enforces it as well; ensure imports are added (CheckConstraint) and adjust any existing code paths that construct GiftCardTransaction to expect the ValueError on invalid input. In `@make_post_sell/models/gift_card.py`: - Around line 80-84: The deduct method currently allows negative amounts and performs a non-atomic read/modify/write on balance_in_cents; change deduct to first validate amount_in_cents > 0 and raise a ValueError for non-positive inputs, then perform the deduction using a DB-level atomic operation instead of in-memory math: either run the update inside a transaction with a SELECT ... FOR UPDATE on the GiftCard row and adjust balance_in_cents, or issue an atomic UPDATE gift_cards SET balance_in_cents = balance_in_cents - :amt WHERE id = :id AND balance_in_cents >= :amt and check affected rows to determine the actual deducted amount; keep the method name deduct and its contract of returning the actual deducted cents. In `@make_post_sell/models/shop.py`: - Around line 842-850: The async rebuild bypasses the non-production guard: update reforge_discovery_ring_async to mirror reforge_discovery_ring by checking shop.environment and shop.is_non_production before calling compute_discovery_ring(), setting shop.discovery_ring = [] and returning [] for non-production shops (or alternatively add the same guard at the start of compute_discovery_ring()); ensure you reference and modify reforge_discovery_ring_async (and/or compute_discovery_ring) so background rebuilds do not repopulate discovery for staging/development shops. - Around line 173-175: The trial_ended boolean is stored but never used; update the shop state helpers to respect it: modify is_trial_active, is_trial_expired, and is_active to check trial_ended (and trial_started_timestamp) so that when trial_ended is True the trial is treated as ended/expired and the shop is considered not active via trial. Ensure is_trial_active returns False if trial_ended is True, is_trial_expired returns True if trial_ended is True, and is_active uses those updated helpers so early-ended trials no longer grant access. - Around line 339-348: The has_primary_s3 property currently omits primary_s3_region, so a shop can pass readiness while add_shop_uploads_client() later supplies shop.primary_s3_region to boto3 and fail; update the has_primary_s3 boolean check to also require self.primary_s3_region (include primary_s3_region in the AND list) so the readiness check matches what add_shop_uploads_client() expects and prevents creating a misconfigured S3 client. In `@make_post_sell/request_methods.py`: - Around line 158-185: The helpers add_shop_uploads_client, add_shop_bucket_name, and add_shop_cdn_endpoint currently treat any truthy shop.has_primary_s3 as enough and silently fall back to the platform bucket when one or more BYOB fields are missing; instead validate that all required BYOB fields are present before returning BYOB values (for add_shop_uploads_client check primary_s3_region, primary_s3_endpoint, primary_s3_access_key, primary_s3_secret_key; for add_shop_bucket_name check primary_s3_bucket; for add_shop_cdn_endpoint check primary_s3_cdn_endpoint), and if validation fails raise an explicit error (or log and abort) so misconfiguration is surfaced rather than quietly returning request.secure_uploads_client or request.app["bucket.secure_uploads"]. In `@make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py`: - Around line 73-83: The migration currently adds plaintext credential columns primary_s3_access_key and primary_s3_secret_key to the mps_shop table via op.add_column (guarded by _column_exists); instead, change the migration to add a single encrypted_secret_reference (or primary_s3_secret_id) column (e.g., VARCHAR/UUID) that stores a reference/ID to a secrets vault or an envelope-encrypted payload, and remove the direct plaintext columns; update any code that will write to/read from primary_s3_access_key/primary_s3_secret_key to use the new reference and fetch/decrypt secrets via your secret manager (KMS/Vault) at runtime, and include a migration note to securely rotate/migrate existing plaintext data to the secret store before dropping old columns if needed. - Around line 98-109: The downgrade() currently unconditionally calls op.drop_column for many columns (e.g., "primary_s3_enabled", "primary_s3_cdn_endpoint", "primary_s3_secret_key", "primary_s3_access_key", "primary_s3_bucket", "primary_s3_region", "primary_s3_endpoint", "plan_active", "trial_ended", "trial_started_timestamp", "environment"), which can break on partially applied schemas; update downgrade() to mirror the tolerant behavior of upgrade() by checking for each column's existence before dropping it (use the Alembic op.get_bind()/sqlalchemy.inspect Inspector or a helper like has_column to query the table schema), and only call op.drop_column for columns that actually exist to avoid dropping columns that this revision didn't create. In `@make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py`: - Around line 36-100: The upgrade() migration is missing the required pre-migration SQLite backup; add a call at the very start of upgrade() to invoke the project's backup helper (e.g., call a function like ensure_sqlite_backup() or run_sqlite_backup()) before any DDL runs, and make that helper a no-op on non-SQLite backends; update or create a small function (ensure_sqlite_backup / run_sqlite_backup) that checks the current DB URL/driver, performs the filesystem copy/backup when driver == "sqlite", and raises/logs on failure so the migration aborts safely if the backup cannot be made. - Around line 102-108: The downgrade() fails to fully reverse upgrade() (it never removes mps_cart.json_gift_cards) and naively drops tables/columns which will error on partially-applied DBs; update downgrade() to drop the mps_cart.json_gift_cards column and guard each drop with an existence check (use SQLAlchemy Inspector or context.get_bind() to verify table/column existence) before calling op.drop_table or op.drop_column for mps_cart_gift_card, mps_gift_card_transaction, mps_gift_card, and the mps_shop columns gift_card_enabled, gift_card_min_in_cents, gift_card_max_in_cents so the rollback is idempotent and fully reverses upgrade(). In `@make_post_sell/static/js/watch.js`: - Around line 1841-1856: The async fetchWatchData callback can overwrite ringProductIds when the user navigates before the response arrives; to fix, capture the product id at request time (e.g., const requestedId = currentProductId) before calling fetchWatchData and, inside the .then handler, verify the live currentProductId still equals requestedId (or that data is tagged for requestedId) before assigning ringProductIds, calling syncRingPosition, saveRingState, or updateProgressDisplay; if it doesn't match, ignore the stale response and still call preloadNext as needed. In `@make_post_sell/templates/content.j2`: - Around line 42-45: The media URL generation in watch.py currently returns presigned S3 URLs (via generate_presigned_url()) for media_url, instrumentals_url, and vocals_url causing a scheme mismatch with templates that expect persistent CDN URLs; update watch.py to construct these three URLs using request.shop_cdn_endpoint the same way file_url and thumbnail_url are built (i.e., combine request.shop_cdn_endpoint with product.s3_path and the appropriate path/filename pattern used for product media) instead of calling generate_presigned_url(), ensuring media_url, instrumentals_url, and vocals_url are consistent CDN endpoints that won’t expire. --- Outside diff comments: In `@make_post_sell/models/cart.py`: - Around line 51-67: The cart now has both json_gift_cards and association_proxy gift_cards but empty() and merge_in_cart() ignore them; update empty() to clear json_gift_cards and remove all associated CartGiftCard entries (via the cart_gift_cards relationship or by clearing the gift_cards proxy) so an "empty" cart has no gift cards, and update merge_in_cart() (and the equivalent logic referenced at lines ~79-80) to also transfer/merge gift cards: merge json_gift_cards arrays and attach/deduplicate CartGiftCard objects to the destination cart (using the CartGiftCard/ gift_cards proxy) just like you do for coupons/products so gift cards aren’t left behind or dropped during merges. --- Minor comments: In `@docs/design-system.md`: - Around line 9-52: The fenced code blocks in the design-system.md examples (e.g., the ASCII diagram showing tokens.css → common.css → templates/ and other snippet blocks) lack language identifiers and trigger markdownlint MD040; update each triple-backtick fence to include an appropriate language tag (use text for diagrams/tables and the concrete language like css, jinja, or html for actual snippets) across the affected example blocks so syntax highlighting and linting pass (apply this to the diagram block containing tokens.css/common.css/templates and the other listed example blocks). In `@docs/tickets/mps-10.md`: - Around line 36-37: The example ticket code format in the docs doesn't match the generator: secrets.token_hex(8).upper() produces 16 hex characters (0-9A-F) so remove the impossible alphanumeric letters from the example or change the generator; either update the sample to "GC-" followed by 16 hex chars (e.g., GC-<16_HEX_CHARS>) to match secrets.token_hex(8).upper(), or replace secrets.token_hex(8).upper() with a generator that picks 16 chars from string.ascii_uppercase + string.digits (e.g., using secrets.choice) so it can produce values like GC-A1B2C3D4E5F6G7H8. In `@docs/tickets/mps-11.md`: - Around line 31-32: Remove the "Option A" hedging and make the storage format definitive: replace the "Option A:" prefix and any language suggesting alternatives with a declarative statement that Cart now stores gift cards in the json_gift_cards column, and keep the format example `[{"shop_id": "...", "amount_in_cents": 2500, "gift_email": "...", "gift_message": "..."}]` as the canonical schema; ensure references to Cart and json_gift_cards are updated to reflect the fixed schema and not presented as optional. In `@docs/tickets/mps-13.md`: - Around line 25-26: The documented route examples use /shop/{slug}/gift-cards/manage but the templates (make_post_sell/templates/gift_card_detail.j2) and links use /s/{{ request.shop.uuid_str }}/gift-cards/..., so update the docs to the template URL shape (e.g., /s/{shop_uuid}/gift-cards/manage) and adjust the example routes/tests/implementation guidance to that shape; also fix the other two occurrences of the incorrect /shop/{slug}/... examples mentioned in the comment so all examples match the template URL format. In `@docs/tickets/mps-14.md`: - Line 10: The statement "Every paid production shop seat includes 2 free dev/stage shops." is inconsistent with the counting example and MPS-15; update the policy text and all examples to state a single rule: either (A) the two extra dev/stage shops are granted only for paid production seats, or (B) they are granted for any production shop including trials — choose one and apply it consistently. Specifically, edit the sentence "Every paid production shop seat includes 2 free dev/stage shops." and the related "counting example" and the MPS-15 reference so they all use the same rule and adjust any example calculations that assume the opposite behavior; ensure the document explicitly calls out whether trial production shops qualify and remove conflicting phrasing in the passages currently using "paid" vs "any". In `@docs/tickets/mps-15.md`: - Around line 116-129: The banner link URLs use request.shop.id but should use request.shop.uuid_str to match the rest of the codebase; update both anchor hrefs in the trial banner blocks to build URLs with request.shop.uuid_str (the conditional checks request.shop, request.shop.is_trial_active, request.shop.owners, request.shop.is_trial_expired, and request.shop.plan_active remain unchanged) so templates and tests use the UUID string pattern consistently. In `@docs/tickets/mps-16.md`: - Around line 31-49: The docs currently show two mutually exclusive BYOB schema designs (reusing mirror_s3_* vs new primary_s3_* columns); update the document to remove the alternative and document only the chosen schema: list and describe the final primary_s3_* columns (primary_s3_endpoint, primary_s3_region, primary_s3_bucket, primary_s3_access_key, primary_s3_secret_key, primary_s3_cdn_endpoint) and the primary_s3_enabled flag, clarifying semantics (when primary_s3_enabled is true these primary_* fields are used as the primary bucket) and remove any mention of reusing mirror_s3_* to avoid ambiguity. In `@make_post_sell/static/js/watch.js`: - Around line 221-228: When you detect a stale ring, don't wipe out the watched state for the currently viewed product (initialId). Modify the reset so that ringHistory preserves the entry for initialId (e.g. set ringHistory = ringHistory[initialId] ? { [initialId]: ringHistory[initialId] } : {}), or alternatively clear ringHistory then immediately call markWatched(initialId) before saveRingState(); keep resetting ringProductIds, ringPosition and ringLoops as before and then call saveRingState(). In `@make_post_sell/templates/gift_card_detail.j2`: - Line 59: The template is rendering txn.created_timestamp (a raw millisecond epoch) which will show a 13-digit number; change the rendering to use a human-readable string by either formatting the timestamp in the view and passing a new property (e.g., txn.created_at_display) or add a display property on the model that returns a formatted datetime, then update the template to render that property instead of txn.created_timestamp; ensure the formatter uses your app's timezone/locale conventions so the UI shows a readable date/time. --- Nitpick comments: In `@make_post_sell/lib/karaoke.py`: - Around line 364-380: The nested helper _make_s3 currently captures the outer shop variable; change its signature to accept shop as an explicit parameter (e.g., def _make_s3(shop):) and update its body to use that parameter instead of the outer variable while still falling back to app_settings when shop is falsy; then update every call site that invokes _make_s3 (including the post-shop-load call and the worker/thread invocation) to pass the appropriate shop object (or None if using app_settings), ensuring no code relies on the outer lexical capture of shop. In `@make_post_sell/lib/mail.py`: - Around line 549-565: The message_parts construction uses unnecessary f-string prefixes on literals without interpolation (e.g., f"", f"This gift card never expires.", f"To redeem..." etc.); update the list building around the message_parts variable so only strings that actually include placeholders remain f-strings (keep f on lines with {amount}, {shop_name}, {code}, {shop_url}, {gift_message}), and remove the f prefix from empty strings and static sentences before joining into message_text to eliminate Ruff F541 warnings. - Around line 567-577: The HTML parts list uses unnecessary f-string prefixes for static strings; update html_parts so only strings that interpolate variables use f-strings (keep f for entries that reference amount, shop_name, code, shop_url, gift_message) and remove the extraneous f prefix from static entries such as the "This gift card never expires." item (locate and edit the html_parts construction and any nearby HTML generation that currently uses f"" without interpolation). In `@make_post_sell/lib/s3_mirror.py`: - Around line 232-248: The _make_src helper uses shop.primary_s3_region directly even though has_primary_s3 may be true while primary_s3_region is None; update _make_src to compute region = shop.primary_s3_region or app_settings["bucket.secure_uploads.region"] and pass that region into boto3.session.Session().client(...) so the source client falls back consistently (matching the region fallback pattern used in _make_mirror_client); reference _make_src, shop.primary_s3_region, app_settings["bucket.secure_uploads.region"], and _make_mirror_client when making the change. In `@make_post_sell/models/cart_gift_card.py`: - Around line 15-18: Add a DB-level unique constraint to prevent duplicate gift-card entries per cart by updating the CartGiftCard model: import and use SQLAlchemy's UniqueConstraint (e.g., UniqueConstraint("cart_id", "gift_card_id", name="uq_cart_gift_card")) in the model's __table_args__ so the combination of cart_id and gift_card_id is enforced as unique at the database level; ensure the import for UniqueConstraint is added alongside Column/UUIDType and keep the existing columns (id, cart_id, gift_card_id, created_timestamp) unchanged. In `@make_post_sell/models/product.py`: - Around line 680-685: The import of Shop is inside the keywords loop which causes repeated lookups; move the "from .shop import Shop" import out of the loop (place it above where product_query is used or before the loop) and keep the existing logic that uses Shop only when shop is falsy (the block that sets query = query.join(Shop, Product.shop_id == Shop.id).filter(Shop.environment == 0)); ensure references to Product.visibility, product_query, shop, and keywords continue to work unchanged. In `@make_post_sell/templates/gift_card.j2`: - Around line 16-34: The slider (id "gift-card-slider") uses step="1" while the numeric input (id "gift-card-amount") uses step="0.01", causing desynchronization when users enter decimal amounts; update the slider's step to "0.01" to match the input (or change the input to "1" if intentional), and ensure any JS that syncs values between gift-card-slider and gift-card-amount handles decimals consistently so both controls reflect the same precision. ``` </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `ffa4c162-a0ca-4566-b877-4c0c156a88e9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 32a19a339c8614bbda2ea122a99f8a42af93d454 and cca3172c9b7eaa88c41a661a641bf45bc6f56fd0. </details> <details> <summary>📒 Files selected for processing (62)</summary> * `CLAUDE.md` * `GIT_HASH` * `docs/architecture.md` * `docs/design-system.md` * `docs/tickets/mps-10.md` * `docs/tickets/mps-11.md` * `docs/tickets/mps-12.md` * `docs/tickets/mps-13.md` * `docs/tickets/mps-14.md` * `docs/tickets/mps-15.md` * `docs/tickets/mps-16.md` * `make_post_sell/GIT_HASH` * `make_post_sell/lib/digest_sender.py` * `make_post_sell/lib/karaoke.py` * `make_post_sell/lib/mail.py` * `make_post_sell/lib/s3_mirror.py` * `make_post_sell/models/__init__.py` * `make_post_sell/models/cart.py` * `make_post_sell/models/cart_gift_card.py` * `make_post_sell/models/gift_card.py` * `make_post_sell/models/gift_card_transaction.py` * `make_post_sell/models/meta.py` * `make_post_sell/models/product.py` * `make_post_sell/models/shop.py` * `make_post_sell/request_methods.py` * `make_post_sell/routes.py` * `make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py` * `make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py` * `make_post_sell/static/css/common.css` * `make_post_sell/static/css/tokens.css` * `make_post_sell/static/js/watch.js` * `make_post_sell/templates/actions_view.j2` * `make_post_sell/templates/base.j2` * `make_post_sell/templates/cart.j2` * `make_post_sell/templates/content.j2` * `make_post_sell/templates/gift_card.j2` * `make_post_sell/templates/gift_card_detail.j2` * `make_post_sell/templates/gift_card_manage.j2` * `make_post_sell/templates/home.j2` * `make_post_sell/templates/join-or-log-in.j2` * `make_post_sell/templates/product.j2` * `make_post_sell/templates/product_edit.j2` * `make_post_sell/templates/shop.j2` * `make_post_sell/templates/shop_about.j2` * `make_post_sell/templates/shop_new.j2` * `make_post_sell/templates/shop_settings.j2` * `make_post_sell/templates/snippets/footer.j2` * `make_post_sell/templates/snippets/related_content.j2` * `make_post_sell/templates/styleguide.j2` * `make_post_sell/templates/user_purchases.j2` * `make_post_sell/tests/test_functional.py` * `make_post_sell/tests/test_integration.py` * `make_post_sell/tests/test_models.py` * `make_post_sell/views/__init__.py` * `make_post_sell/views/cart.py` * `make_post_sell/views/content.py` * `make_post_sell/views/feeds.py` * `make_post_sell/views/gift_card.py` * `make_post_sell/views/player.py` * `make_post_sell/views/product.py` * `make_post_sell/views/shop.py` * `make_post_sell/views/watch.py` </details> </details>

coderabbitai commented

2026-03-07 23:38:55 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Grandfathered shops stay inactive with this is_active example.

Lines 27-30 define NULL trial_started_timestamp as “pre-trial era, treated as paid,” but Line 69 only returns plan_active or is_trial_active. For those legacy shops both branches are false, so the example would lock out existing stores unless some separate backfill sets plan_active=True.

💡 Proposed fix

 `@property`
 def is_active(self):
     """Shop can operate: either paid plan or active trial."""
-    return self.plan_active or self.is_trial_active
+    return (
+        self.plan_active
+        or self.is_trial_active
+        or self.trial_started_timestamp is None
+    )

Also applies to: 66-69

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/tickets/mps-15.md` around lines 27 - 30, The is_active logic will leave
legacy shops inactive because NULL `trial_started_timestamp` is treated as false
and `plan_active` defaults to 0; update the activation rule or migration so
legacy shops remain active: either change the boolean expression (used where
`is_active` is computed around lines 66-69) to treat NULL
`trial_started_timestamp` as "paid" (e.g., `plan_active OR is_trial_active OR
trial_started_timestamp IS NULL`) or add an idempotent backfill/migration on
`mps_shop` to set `plan_active=true` for existing rows; reference the column
names `trial_started_timestamp`, `trial_ended`, and `plan_active` and the
`is_active` computation when making the change.

_⚠️ Potential issue_ | _🟠 Major_ **Grandfathered shops stay inactive with this `is_active` example.** Lines 27-30 define `NULL` `trial_started_timestamp` as “pre-trial era, treated as paid,” but Line 69 only returns `plan_active or is_trial_active`. For those legacy shops both branches are false, so the example would lock out existing stores unless some separate backfill sets `plan_active=True`. <details> <summary>💡 Proposed fix</summary> ```diff `@property` def is_active(self): """Shop can operate: either paid plan or active trial.""" - return self.plan_active or self.is_trial_active + return ( + self.plan_active + or self.is_trial_active + or self.trial_started_timestamp is None + ) ``` </details> Also applies to: 66-69 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tickets/mps-15.md` around lines 27 - 30, The is_active logic will leave legacy shops inactive because NULL `trial_started_timestamp` is treated as false and `plan_active` defaults to 0; update the activation rule or migration so legacy shops remain active: either change the boolean expression (used where `is_active` is computed around lines 66-69) to treat NULL `trial_started_timestamp` as "paid" (e.g., `plan_active OR is_trial_active OR trial_started_timestamp IS NULL`) or add an idempotent backfill/migration on `mps_shop` to set `plan_active=true` for existing rows; reference the column names `trial_started_timestamp`, `trial_ended`, and `plan_active` and the `is_active` computation when making the change. ``` </details>

coderabbitai commented

2026-03-07 23:38:55 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Reject duplicate gift cards before applying deductions.

The same card can be attached more than once (make_post_sell/models/cart_gift_card.py:8-33 has no (cart_id, gift_card_id) uniqueness guard), and discounted_shop_totals_in_cents() will apply its balance once per attachment. Add a duplicate-id/code check here, and ideally back it with a DB constraint later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/cart.py` around lines 468 - 485, The
validate_attached_gift_cards method currently doesn't reject duplicate
attachments, so the same gift card can be applied multiple times; update
validate_attached_gift_cards to detect duplicate gift_card.id (and/or
gift_card.code) across self.gift_cards and add an error like "Gift card '<code>'
is attached multiple times" when duplicates are found (in addition to the
existing disabled/balance/shop checks), returning these errors; also note to add
a unique (cart_id, gift_card_id) constraint later in
make_post_sell/models/cart_gift_card.py to enforce this at the DB level.

_⚠️ Potential issue_ | _🟠 Major_ **Reject duplicate gift cards before applying deductions.** The same card can be attached more than once (`make_post_sell/models/cart_gift_card.py:8-33` has no `(cart_id, gift_card_id)` uniqueness guard), and `discounted_shop_totals_in_cents()` will apply its balance once per attachment. Add a duplicate-id/code check here, and ideally back it with a DB constraint later. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/cart.py` around lines 468 - 485, The validate_attached_gift_cards method currently doesn't reject duplicate attachments, so the same gift card can be applied multiple times; update validate_attached_gift_cards to detect duplicate gift_card.id (and/or gift_card.code) across self.gift_cards and add an error like "Gift card '<code>' is attached multiple times" when duplicates are found (in addition to the existing disabled/balance/shop checks), returning these errors; also note to add a unique (cart_id, gift_card_id) constraint later in make_post_sell/models/cart_gift_card.py to enforce this at the DB level. ``` </details>

coderabbitai commented

2026-03-07 23:38:55 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Reject non-positive redemption amounts.

This constructor accepts 0 or negative cents, which would either create meaningless rows or add balance back once downstream code subtracts the “deduction.” Enforce amount_in_cents > 0 here and ideally at the schema level too.

💡 Proposed fix

-from sqlalchemy import Column, BigInteger
+from sqlalchemy import Column, BigInteger, CheckConstraint
 from sqlalchemy.orm import relationship
@@
 class GiftCardTransaction(RBase, Base):
     """Tracks each time a gift card balance is used at checkout."""
+    __table_args__ = (
+        CheckConstraint(
+            "amount_in_cents > 0",
+            name="ck_gift_card_transaction_amount_positive",
+        ),
+    )
@@
     def __init__(self, gift_card, invoice, amount_in_cents):
+        if amount_in_cents <= 0:
+            raise ValueError("amount_in_cents must be positive")
         self.id = uuid.uuid1()
         self.gift_card = gift_card
         self.invoice = invoice

Also applies to: 18-24, 33-38

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/gift_card_transaction.py` around lines 3 - 4, Reject
non-positive redemption amounts by adding a guard in the GiftCardTransaction
constructor and a schema-level constraint: in the GiftCardTransaction class,
update its __init__ to validate that amount_in_cents > 0 and raise a ValueError
if not (refer to the amount_in_cents parameter), and add a SQLAlchemy
CheckConstraint like CheckConstraint("amount_in_cents > 0") on the model so the
DB enforces it as well; ensure imports are added (CheckConstraint) and adjust
any existing code paths that construct GiftCardTransaction to expect the
ValueError on invalid input.

_⚠️ Potential issue_ | _🟠 Major_ **Reject non-positive redemption amounts.** This constructor accepts `0` or negative cents, which would either create meaningless rows or add balance back once downstream code subtracts the “deduction.” Enforce `amount_in_cents > 0` here and ideally at the schema level too. <details> <summary>💡 Proposed fix</summary> ```diff -from sqlalchemy import Column, BigInteger +from sqlalchemy import Column, BigInteger, CheckConstraint from sqlalchemy.orm import relationship @@ class GiftCardTransaction(RBase, Base): """Tracks each time a gift card balance is used at checkout.""" + __table_args__ = ( + CheckConstraint( + "amount_in_cents > 0", + name="ck_gift_card_transaction_amount_positive", + ), + ) @@ def __init__(self, gift_card, invoice, amount_in_cents): + if amount_in_cents <= 0: + raise ValueError("amount_in_cents must be positive") self.id = uuid.uuid1() self.gift_card = gift_card self.invoice = invoice ``` </details> Also applies to: 18-24, 33-38 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/gift_card_transaction.py` around lines 3 - 4, Reject non-positive redemption amounts by adding a guard in the GiftCardTransaction constructor and a schema-level constraint: in the GiftCardTransaction class, update its __init__ to validate that amount_in_cents > 0 and raise a ValueError if not (refer to the amount_in_cents parameter), and add a SQLAlchemy CheckConstraint like CheckConstraint("amount_in_cents > 0") on the model so the DB enforces it as well; ensure imports are added (CheckConstraint) and adjust any existing code paths that construct GiftCardTransaction to expect the ValueError on invalid input. ``` </details>

coderabbitai commented

2026-03-07 23:38:55 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🔴 Critical

Make redemption atomic and reject non-positive amounts.

min(amount_in_cents, self.balance_in_cents) turns negative inputs into balance increases, and this read/modify/write update can double-spend the same card under concurrent checkouts. This needs a positive-amount guard plus DB-level locking or an atomic UPDATE ... WHERE balance_in_cents >= ... in the redemption path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/gift_card.py` around lines 80 - 84, The deduct method
currently allows negative amounts and performs a non-atomic read/modify/write on
balance_in_cents; change deduct to first validate amount_in_cents > 0 and raise
a ValueError for non-positive inputs, then perform the deduction using a
DB-level atomic operation instead of in-memory math: either run the update
inside a transaction with a SELECT ... FOR UPDATE on the GiftCard row and adjust
balance_in_cents, or issue an atomic UPDATE gift_cards SET balance_in_cents =
balance_in_cents - :amt WHERE id = :id AND balance_in_cents >= :amt and check
affected rows to determine the actual deducted amount; keep the method name
deduct and its contract of returning the actual deducted cents.

_⚠️ Potential issue_ | _🔴 Critical_ **Make redemption atomic and reject non-positive amounts.** `min(amount_in_cents, self.balance_in_cents)` turns negative inputs into balance increases, and this read/modify/write update can double-spend the same card under concurrent checkouts. This needs a positive-amount guard plus DB-level locking or an atomic `UPDATE ... WHERE balance_in_cents >= ...` in the redemption path. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/gift_card.py` around lines 80 - 84, The deduct method currently allows negative amounts and performs a non-atomic read/modify/write on balance_in_cents; change deduct to first validate amount_in_cents > 0 and raise a ValueError for non-positive inputs, then perform the deduction using a DB-level atomic operation instead of in-memory math: either run the update inside a transaction with a SELECT ... FOR UPDATE on the GiftCard row and adjust balance_in_cents, or issue an atomic UPDATE gift_cards SET balance_in_cents = balance_in_cents - :amt WHERE id = :id AND balance_in_cents >= :amt and check affected rows to determine the actual deducted amount; keep the method name deduct and its contract of returning the actual deducted cents. ``` </details>

coderabbitai commented

2026-03-07 23:38:56 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

trial_ended never changes the shop’s effective access state.

The new flag is stored, but is_trial_active, is_trial_expired, and is_active ignore it. An early-ended trial will still look active until the timestamp expires.

Also applies to: 303-335

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/shop.py` around lines 173 - 175, The trial_ended
boolean is stored but never used; update the shop state helpers to respect it:
modify is_trial_active, is_trial_expired, and is_active to check trial_ended
(and trial_started_timestamp) so that when trial_ended is True the trial is
treated as ended/expired and the shop is considered not active via trial. Ensure
is_trial_active returns False if trial_ended is True, is_trial_expired returns
True if trial_ended is True, and is_active uses those updated helpers so
early-ended trials no longer grant access.

_⚠️ Potential issue_ | _🟠 Major_ **`trial_ended` never changes the shop’s effective access state.** The new flag is stored, but `is_trial_active`, `is_trial_expired`, and `is_active` ignore it. An early-ended trial will still look active until the timestamp expires. Also applies to: 303-335 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/shop.py` around lines 173 - 175, The trial_ended boolean is stored but never used; update the shop state helpers to respect it: modify is_trial_active, is_trial_expired, and is_active to check trial_ended (and trial_started_timestamp) so that when trial_ended is True the trial is treated as ended/expired and the shop is considered not active via trial. Ensure is_trial_active returns False if trial_ended is True, is_trial_expired returns True if trial_ended is True, and is_active uses those updated helpers so early-ended trials no longer grant access. ``` </details>

coderabbitai commented

2026-03-07 23:38:56 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd /repo && find . -type f -name "*.py" | head -20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 140

🏁 Script executed:

cd /repo && rg "add_shop_uploads_client" --type py -A 5 -B 2

Repository: engineering/make-post-sell/make_post_sell

Length of output: 140

🏁 Script executed:

cd /repo && grep -n "primary_s3_region" make_post_sell/models/shop.py | head -20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 140

🏁 Script executed:

cd /repo && grep -n "has_primary_s3" make_post_sell/models/shop.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 140

🏁 Script executed:

find . -type f -name "*.py" | grep -E "(shop|upload)" | head -20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1544

🏁 Script executed:

rg "add_shop_uploads_client" --type py -A 10

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1910

🏁 Script executed:

rg "primary_s3_region" make_post_sell/models/shop.py -n

Repository: engineering/make-post-sell/make_post_sell

Length of output: 147

🏁 Script executed:

rg "def has_primary_s3" make_post_sell/models/shop.py -A 15

Repository: engineering/make-post-sell/make_post_sell

Length of output: 542

🏁 Script executed:

rg "region_name" make_post_sell/request_methods.py -B 5 -A 5

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1019

🏁 Script executed:

rg "has_primary_s3" make_post_sell --type py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 2302

🏁 Script executed:

rg "test_has_primary_s3_requires_all_fields" --type py -A 20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1861

🏁 Script executed:

rg "test_has_primary_s3_when_enabled_and_configured" --type py -A 15

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1349

🏁 Script executed:

grep -n "primary_s3_region" make_post_sell/tests/test_models.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 151

🏁 Script executed:

grep -n "primary_s3_region" make_post_sell/tests/test_integration.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 299

🏁 Script executed:

rg "test_has_primary_s3_missing_field" --type py -A 25 make_post_sell/tests/test_integration.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1281

🏁 Script executed:

cat -n make_post_sell/tests/test_models.py | sed -n '3595,3620p'

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1526

Include primary_s3_region in the has_primary_s3 validation.

add_shop_uploads_client() always passes shop.primary_s3_region to the boto3 S3 client, but has_primary_s3 doesn't require that field. This allows a shop to pass the BYOB readiness check while primary_s3_region is None or empty, resulting in a misconfigured client.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/shop.py` around lines 339 - 348, The has_primary_s3
property currently omits primary_s3_region, so a shop can pass readiness while
add_shop_uploads_client() later supplies shop.primary_s3_region to boto3 and
fail; update the has_primary_s3 boolean check to also require
self.primary_s3_region (include primary_s3_region in the AND list) so the
readiness check matches what add_shop_uploads_client() expects and prevents
creating a misconfigured S3 client.

_⚠️ Potential issue_ | _🟠 Major_ <details> <summary>🧩 Analysis chain</summary> 🏁 Script executed: ```shell cd /repo && find . -type f -name "*.py" | head -20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 140 --- 🏁 Script executed: ```shell cd /repo && rg "add_shop_uploads_client" --type py -A 5 -B 2 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 140 --- 🏁 Script executed: ```shell cd /repo && grep -n "primary_s3_region" make_post_sell/models/shop.py | head -20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 140 --- 🏁 Script executed: ```shell cd /repo && grep -n "has_primary_s3" make_post_sell/models/shop.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 140 --- 🏁 Script executed: ```shell find . -type f -name "*.py" | grep -E "(shop|upload)" | head -20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1544 --- 🏁 Script executed: ```shell rg "add_shop_uploads_client" --type py -A 10 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1910 --- 🏁 Script executed: ```shell rg "primary_s3_region" make_post_sell/models/shop.py -n ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 147 --- 🏁 Script executed: ```shell rg "def has_primary_s3" make_post_sell/models/shop.py -A 15 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 542 --- 🏁 Script executed: ```shell rg "region_name" make_post_sell/request_methods.py -B 5 -A 5 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1019 --- 🏁 Script executed: ```shell rg "has_primary_s3" make_post_sell --type py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 2302 --- 🏁 Script executed: ```shell rg "test_has_primary_s3_requires_all_fields" --type py -A 20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1861 --- 🏁 Script executed: ```shell rg "test_has_primary_s3_when_enabled_and_configured" --type py -A 15 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1349 --- 🏁 Script executed: ```shell grep -n "primary_s3_region" make_post_sell/tests/test_models.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 151 --- 🏁 Script executed: ```shell grep -n "primary_s3_region" make_post_sell/tests/test_integration.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 299 --- 🏁 Script executed: ```shell rg "test_has_primary_s3_missing_field" --type py -A 25 make_post_sell/tests/test_integration.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1281 --- 🏁 Script executed: ```shell cat -n make_post_sell/tests/test_models.py | sed -n '3595,3620p' ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1526 --- </details> **Include `primary_s3_region` in the `has_primary_s3` validation.** `add_shop_uploads_client()` always passes `shop.primary_s3_region` to the boto3 S3 client, but `has_primary_s3` doesn't require that field. This allows a shop to pass the BYOB readiness check while `primary_s3_region` is `None` or empty, resulting in a misconfigured client. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/shop.py` around lines 339 - 348, The has_primary_s3 property currently omits primary_s3_region, so a shop can pass readiness while add_shop_uploads_client() later supplies shop.primary_s3_region to boto3 and fail; update the has_primary_s3 boolean check to also require self.primary_s3_region (include primary_s3_region in the AND list) so the readiness check matches what add_shop_uploads_client() expects and prevents creating a misconfigured S3 client. ``` </details>

coderabbitai commented

2026-03-07 23:38:57 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

The non-production discovery guard is bypassed by async rebuilds.

reforge_discovery_ring() now empties the ring for staging/development shops, but reforge_discovery_ring_async() still calls compute_discovery_ring() directly. Any background rebuild will repopulate discovery for shops that were supposed to stay hidden.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/models/shop.py` around lines 842 - 850, The async rebuild
bypasses the non-production guard: update reforge_discovery_ring_async to mirror
reforge_discovery_ring by checking shop.environment and shop.is_non_production
before calling compute_discovery_ring(), setting shop.discovery_ring = [] and
returning [] for non-production shops (or alternatively add the same guard at
the start of compute_discovery_ring()); ensure you reference and modify
reforge_discovery_ring_async (and/or compute_discovery_ring) so background
rebuilds do not repopulate discovery for staging/development shops.

_⚠️ Potential issue_ | _🟠 Major_ **The non-production discovery guard is bypassed by async rebuilds.** `reforge_discovery_ring()` now empties the ring for staging/development shops, but `reforge_discovery_ring_async()` still calls `compute_discovery_ring()` directly. Any background rebuild will repopulate discovery for shops that were supposed to stay hidden. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/models/shop.py` around lines 842 - 850, The async rebuild bypasses the non-production guard: update reforge_discovery_ring_async to mirror reforge_discovery_ring by checking shop.environment and shop.is_non_production before calling compute_discovery_ring(), setting shop.discovery_ring = [] and returning [] for non-production shops (or alternatively add the same guard at the start of compute_discovery_ring()); ensure you reference and modify reforge_discovery_ring_async (and/or compute_discovery_ring) so background rebuilds do not repopulate discovery for staging/development shops. ``` </details>

coderabbitai commented

2026-03-07 23:38:57 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Don't silently fall back to the platform bucket when BYOB is enabled.

If primary_s3_enabled is true but one field is missing, all three helpers quietly return the MPS client/bucket/CDN instead of surfacing a misconfiguration. That breaks the BYOB contract and can send shop media back to the shared bucket.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/request_methods.py` around lines 158 - 185, The helpers
add_shop_uploads_client, add_shop_bucket_name, and add_shop_cdn_endpoint
currently treat any truthy shop.has_primary_s3 as enough and silently fall back
to the platform bucket when one or more BYOB fields are missing; instead
validate that all required BYOB fields are present before returning BYOB values
(for add_shop_uploads_client check primary_s3_region, primary_s3_endpoint,
primary_s3_access_key, primary_s3_secret_key; for add_shop_bucket_name check
primary_s3_bucket; for add_shop_cdn_endpoint check primary_s3_cdn_endpoint), and
if validation fails raise an explicit error (or log and abort) so
misconfiguration is surfaced rather than quietly returning
request.secure_uploads_client or request.app["bucket.secure_uploads"].

_⚠️ Potential issue_ | _🟠 Major_ **Don't silently fall back to the platform bucket when BYOB is enabled.** If `primary_s3_enabled` is true but one field is missing, all three helpers quietly return the MPS client/bucket/CDN instead of surfacing a misconfiguration. That breaks the BYOB contract and can send shop media back to the shared bucket. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/request_methods.py` around lines 158 - 185, The helpers add_shop_uploads_client, add_shop_bucket_name, and add_shop_cdn_endpoint currently treat any truthy shop.has_primary_s3 as enough and silently fall back to the platform bucket when one or more BYOB fields are missing; instead validate that all required BYOB fields are present before returning BYOB values (for add_shop_uploads_client check primary_s3_region, primary_s3_endpoint, primary_s3_access_key, primary_s3_secret_key; for add_shop_bucket_name check primary_s3_bucket; for add_shop_cdn_endpoint check primary_s3_cdn_endpoint), and if validation fails raise an explicit error (or log and abort) so misconfiguration is surfaced rather than quietly returning request.secure_uploads_client or request.app["bucket.secure_uploads"]. ``` </details>

coderabbitai commented

2026-03-07 23:38:58 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Avoid storing raw BYOB credentials in mps_shop.

Adding primary_s3_access_key and primary_s3_secret_key as plain text columns means a DB leak exposes every connected bucket. This should be an encrypted secret reference or envelope-encrypted payload, not directly readable key material.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py`
around lines 73 - 83, The migration currently adds plaintext credential columns
primary_s3_access_key and primary_s3_secret_key to the mps_shop table via
op.add_column (guarded by _column_exists); instead, change the migration to add
a single encrypted_secret_reference (or primary_s3_secret_id) column (e.g.,
VARCHAR/UUID) that stores a reference/ID to a secrets vault or an
envelope-encrypted payload, and remove the direct plaintext columns; update any
code that will write to/read from primary_s3_access_key/primary_s3_secret_key to
use the new reference and fetch/decrypt secrets via your secret manager
(KMS/Vault) at runtime, and include a migration note to securely rotate/migrate
existing plaintext data to the secret store before dropping old columns if
needed.

_⚠️ Potential issue_ | _🟠 Major_ **Avoid storing raw BYOB credentials in `mps_shop`.** Adding `primary_s3_access_key` and `primary_s3_secret_key` as plain text columns means a DB leak exposes every connected bucket. This should be an encrypted secret reference or envelope-encrypted payload, not directly readable key material. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py` around lines 73 - 83, The migration currently adds plaintext credential columns primary_s3_access_key and primary_s3_secret_key to the mps_shop table via op.add_column (guarded by _column_exists); instead, change the migration to add a single encrypted_secret_reference (or primary_s3_secret_id) column (e.g., VARCHAR/UUID) that stores a reference/ID to a secrets vault or an envelope-encrypted payload, and remove the direct plaintext columns; update any code that will write to/read from primary_s3_access_key/primary_s3_secret_key to use the new reference and fetch/decrypt secrets via your secret manager (KMS/Vault) at runtime, and include a migration note to securely rotate/migrate existing plaintext data to the secret store before dropping old columns if needed. ``` </details>

coderabbitai commented

2026-03-07 23:38:59 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Mirror the existence checks in downgrade().

upgrade() is intentionally tolerant of already-present columns, but downgrade() drops everything unconditionally. On a partially applied or hand-repaired schema, rollback will fail or remove columns this revision never created.

💡 Proposed fix

 def downgrade():
-    op.drop_column("mps_shop", "primary_s3_enabled")
-    op.drop_column("mps_shop", "primary_s3_cdn_endpoint")
-    op.drop_column("mps_shop", "primary_s3_secret_key")
-    op.drop_column("mps_shop", "primary_s3_access_key")
-    op.drop_column("mps_shop", "primary_s3_bucket")
-    op.drop_column("mps_shop", "primary_s3_region")
-    op.drop_column("mps_shop", "primary_s3_endpoint")
-    op.drop_column("mps_shop", "plan_active")
-    op.drop_column("mps_shop", "trial_ended")
-    op.drop_column("mps_shop", "trial_started_timestamp")
-    op.drop_column("mps_shop", "environment")
+    if _column_exists("mps_shop", "primary_s3_enabled"):
+        op.drop_column("mps_shop", "primary_s3_enabled")
+    if _column_exists("mps_shop", "primary_s3_cdn_endpoint"):
+        op.drop_column("mps_shop", "primary_s3_cdn_endpoint")
+    if _column_exists("mps_shop", "primary_s3_secret_key"):
+        op.drop_column("mps_shop", "primary_s3_secret_key")
+    if _column_exists("mps_shop", "primary_s3_access_key"):
+        op.drop_column("mps_shop", "primary_s3_access_key")
+    if _column_exists("mps_shop", "primary_s3_bucket"):
+        op.drop_column("mps_shop", "primary_s3_bucket")
+    if _column_exists("mps_shop", "primary_s3_region"):
+        op.drop_column("mps_shop", "primary_s3_region")
+    if _column_exists("mps_shop", "primary_s3_endpoint"):
+        op.drop_column("mps_shop", "primary_s3_endpoint")
+    if _column_exists("mps_shop", "plan_active"):
+        op.drop_column("mps_shop", "plan_active")
+    if _column_exists("mps_shop", "trial_ended"):
+        op.drop_column("mps_shop", "trial_ended")
+    if _column_exists("mps_shop", "trial_started_timestamp"):
+        op.drop_column("mps_shop", "trial_started_timestamp")
+    if _column_exists("mps_shop", "environment"):
+        op.drop_column("mps_shop", "environment")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

def downgrade():
    if _column_exists("mps_shop", "primary_s3_enabled"):
        op.drop_column("mps_shop", "primary_s3_enabled")
    if _column_exists("mps_shop", "primary_s3_cdn_endpoint"):
        op.drop_column("mps_shop", "primary_s3_cdn_endpoint")
    if _column_exists("mps_shop", "primary_s3_secret_key"):
        op.drop_column("mps_shop", "primary_s3_secret_key")
    if _column_exists("mps_shop", "primary_s3_access_key"):
        op.drop_column("mps_shop", "primary_s3_access_key")
    if _column_exists("mps_shop", "primary_s3_bucket"):
        op.drop_column("mps_shop", "primary_s3_bucket")
    if _column_exists("mps_shop", "primary_s3_region"):
        op.drop_column("mps_shop", "primary_s3_region")
    if _column_exists("mps_shop", "primary_s3_endpoint"):
        op.drop_column("mps_shop", "primary_s3_endpoint")
    if _column_exists("mps_shop", "plan_active"):
        op.drop_column("mps_shop", "plan_active")
    if _column_exists("mps_shop", "trial_ended"):
        op.drop_column("mps_shop", "trial_ended")
    if _column_exists("mps_shop", "trial_started_timestamp"):
        op.drop_column("mps_shop", "trial_started_timestamp")
    if _column_exists("mps_shop", "environment"):
        op.drop_column("mps_shop", "environment")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py`
around lines 98 - 109, The downgrade() currently unconditionally calls
op.drop_column for many columns (e.g., "primary_s3_enabled",
"primary_s3_cdn_endpoint", "primary_s3_secret_key", "primary_s3_access_key",
"primary_s3_bucket", "primary_s3_region", "primary_s3_endpoint", "plan_active",
"trial_ended", "trial_started_timestamp", "environment"), which can break on
partially applied schemas; update downgrade() to mirror the tolerant behavior of
upgrade() by checking for each column's existence before dropping it (use the
Alembic op.get_bind()/sqlalchemy.inspect Inspector or a helper like has_column
to query the table schema), and only call op.drop_column for columns that
actually exist to avoid dropping columns that this revision didn't create.

_⚠️ Potential issue_ | _🟠 Major_ **Mirror the existence checks in `downgrade()`.** `upgrade()` is intentionally tolerant of already-present columns, but `downgrade()` drops everything unconditionally. On a partially applied or hand-repaired schema, rollback will fail or remove columns this revision never created. <details> <summary>💡 Proposed fix</summary> ```diff def downgrade(): - op.drop_column("mps_shop", "primary_s3_enabled") - op.drop_column("mps_shop", "primary_s3_cdn_endpoint") - op.drop_column("mps_shop", "primary_s3_secret_key") - op.drop_column("mps_shop", "primary_s3_access_key") - op.drop_column("mps_shop", "primary_s3_bucket") - op.drop_column("mps_shop", "primary_s3_region") - op.drop_column("mps_shop", "primary_s3_endpoint") - op.drop_column("mps_shop", "plan_active") - op.drop_column("mps_shop", "trial_ended") - op.drop_column("mps_shop", "trial_started_timestamp") - op.drop_column("mps_shop", "environment") + if _column_exists("mps_shop", "primary_s3_enabled"): + op.drop_column("mps_shop", "primary_s3_enabled") + if _column_exists("mps_shop", "primary_s3_cdn_endpoint"): + op.drop_column("mps_shop", "primary_s3_cdn_endpoint") + if _column_exists("mps_shop", "primary_s3_secret_key"): + op.drop_column("mps_shop", "primary_s3_secret_key") + if _column_exists("mps_shop", "primary_s3_access_key"): + op.drop_column("mps_shop", "primary_s3_access_key") + if _column_exists("mps_shop", "primary_s3_bucket"): + op.drop_column("mps_shop", "primary_s3_bucket") + if _column_exists("mps_shop", "primary_s3_region"): + op.drop_column("mps_shop", "primary_s3_region") + if _column_exists("mps_shop", "primary_s3_endpoint"): + op.drop_column("mps_shop", "primary_s3_endpoint") + if _column_exists("mps_shop", "plan_active"): + op.drop_column("mps_shop", "plan_active") + if _column_exists("mps_shop", "trial_ended"): + op.drop_column("mps_shop", "trial_ended") + if _column_exists("mps_shop", "trial_started_timestamp"): + op.drop_column("mps_shop", "trial_started_timestamp") + if _column_exists("mps_shop", "environment"): + op.drop_column("mps_shop", "environment") ``` </details>  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion:-11+0 def downgrade(): if _column_exists("mps_shop", "primary_s3_enabled"): op.drop_column("mps_shop", "primary_s3_enabled") if _column_exists("mps_shop", "primary_s3_cdn_endpoint"): op.drop_column("mps_shop", "primary_s3_cdn_endpoint") if _column_exists("mps_shop", "primary_s3_secret_key"): op.drop_column("mps_shop", "primary_s3_secret_key") if _column_exists("mps_shop", "primary_s3_access_key"): op.drop_column("mps_shop", "primary_s3_access_key") if _column_exists("mps_shop", "primary_s3_bucket"): op.drop_column("mps_shop", "primary_s3_bucket") if _column_exists("mps_shop", "primary_s3_region"): op.drop_column("mps_shop", "primary_s3_region") if _column_exists("mps_shop", "primary_s3_endpoint"): op.drop_column("mps_shop", "primary_s3_endpoint") if _column_exists("mps_shop", "plan_active"): op.drop_column("mps_shop", "plan_active") if _column_exists("mps_shop", "trial_ended"): op.drop_column("mps_shop", "trial_ended") if _column_exists("mps_shop", "trial_started_timestamp"): op.drop_column("mps_shop", "trial_started_timestamp") if _column_exists("mps_shop", "environment"): op.drop_column("mps_shop", "environment") ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/scripts/alembic/versions/9884324a48e3_add_environment_trial_and_primary_s3_.py` around lines 98 - 109, The downgrade() currently unconditionally calls op.drop_column for many columns (e.g., "primary_s3_enabled", "primary_s3_cdn_endpoint", "primary_s3_secret_key", "primary_s3_access_key", "primary_s3_bucket", "primary_s3_region", "primary_s3_endpoint", "plan_active", "trial_ended", "trial_started_timestamp", "environment"), which can break on partially applied schemas; update downgrade() to mirror the tolerant behavior of upgrade() by checking for each column's existence before dropping it (use the Alembic op.get_bind()/sqlalchemy.inspect Inspector or a helper like has_column to query the table schema), and only call op.drop_column for columns that actually exist to avoid dropping columns that this revision didn't create. ``` </details>

coderabbitai commented

2026-03-07 23:39:01 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Missing the required pre-migration backup step.

This script starts applying DDL immediately, but the repo rule for Alembic migrations requires taking a SQLite backup first. Please wire that into the migration path before this ships.

As per coding guidelines, "Always create a backup of the database before any database operations (migrations, schema changes, etc.)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py`
around lines 36 - 100, The upgrade() migration is missing the required
pre-migration SQLite backup; add a call at the very start of upgrade() to invoke
the project's backup helper (e.g., call a function like ensure_sqlite_backup()
or run_sqlite_backup()) before any DDL runs, and make that helper a no-op on
non-SQLite backends; update or create a small function (ensure_sqlite_backup /
run_sqlite_backup) that checks the current DB URL/driver, performs the
filesystem copy/backup when driver == "sqlite", and raises/logs on failure so
the migration aborts safely if the backup cannot be made.

_⚠️ Potential issue_ | _🟠 Major_ **Missing the required pre-migration backup step.** This script starts applying DDL immediately, but the repo rule for Alembic migrations requires taking a SQLite backup first. Please wire that into the migration path before this ships. As per coding guidelines, "Always create a backup of the database before any database operations (migrations, schema changes, etc.)". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py` around lines 36 - 100, The upgrade() migration is missing the required pre-migration SQLite backup; add a call at the very start of upgrade() to invoke the project's backup helper (e.g., call a function like ensure_sqlite_backup() or run_sqlite_backup()) before any DDL runs, and make that helper a no-op on non-SQLite backends; update or create a small function (ensure_sqlite_backup / run_sqlite_backup) that checks the current DB URL/driver, performs the filesystem copy/backup when driver == "sqlite", and raises/logs on failure so the migration aborts safely if the backup cannot be made. ``` </details>

coderabbitai commented

2026-03-07 23:39:02 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Downgrade does not restore the pre-migration schema.

upgrade() adds mps_cart.json_gift_cards, but downgrade() never removes it. The unguarded drops will also fail on partially-applied or partially-rolled-back databases.

Suggested rollback fix

 def downgrade():
-    op.drop_table("mps_cart_gift_card")
-    op.drop_table("mps_gift_card_transaction")
-    op.drop_table("mps_gift_card")
-    op.drop_column("mps_shop", "gift_card_enabled")
-    op.drop_column("mps_shop", "gift_card_min_in_cents")
-    op.drop_column("mps_shop", "gift_card_max_in_cents")
+    if _table_exists("mps_cart_gift_card"):
+        op.drop_table("mps_cart_gift_card")
+    if _table_exists("mps_gift_card_transaction"):
+        op.drop_table("mps_gift_card_transaction")
+    if _table_exists("mps_gift_card"):
+        op.drop_table("mps_gift_card")
+    if _column_exists("mps_cart", "json_gift_cards"):
+        op.drop_column("mps_cart", "json_gift_cards")
+    if _column_exists("mps_shop", "gift_card_enabled"):
+        op.drop_column("mps_shop", "gift_card_enabled")
+    if _column_exists("mps_shop", "gift_card_min_in_cents"):
+        op.drop_column("mps_shop", "gift_card_min_in_cents")
+    if _column_exists("mps_shop", "gift_card_max_in_cents"):
+        op.drop_column("mps_shop", "gift_card_max_in_cents")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py`
around lines 102 - 108, The downgrade() fails to fully reverse upgrade() (it
never removes mps_cart.json_gift_cards) and naively drops tables/columns which
will error on partially-applied DBs; update downgrade() to drop the
mps_cart.json_gift_cards column and guard each drop with an existence check (use
SQLAlchemy Inspector or context.get_bind() to verify table/column existence)
before calling op.drop_table or op.drop_column for mps_cart_gift_card,
mps_gift_card_transaction, mps_gift_card, and the mps_shop columns
gift_card_enabled, gift_card_min_in_cents, gift_card_max_in_cents so the
rollback is idempotent and fully reverses upgrade().

_⚠️ Potential issue_ | _🟠 Major_ **Downgrade does not restore the pre-migration schema.** `upgrade()` adds `mps_cart.json_gift_cards`, but `downgrade()` never removes it. The unguarded drops will also fail on partially-applied or partially-rolled-back databases. <details> <summary>Suggested rollback fix</summary> ```diff def downgrade(): - op.drop_table("mps_cart_gift_card") - op.drop_table("mps_gift_card_transaction") - op.drop_table("mps_gift_card") - op.drop_column("mps_shop", "gift_card_enabled") - op.drop_column("mps_shop", "gift_card_min_in_cents") - op.drop_column("mps_shop", "gift_card_max_in_cents") + if _table_exists("mps_cart_gift_card"): + op.drop_table("mps_cart_gift_card") + if _table_exists("mps_gift_card_transaction"): + op.drop_table("mps_gift_card_transaction") + if _table_exists("mps_gift_card"): + op.drop_table("mps_gift_card") + if _column_exists("mps_cart", "json_gift_cards"): + op.drop_column("mps_cart", "json_gift_cards") + if _column_exists("mps_shop", "gift_card_enabled"): + op.drop_column("mps_shop", "gift_card_enabled") + if _column_exists("mps_shop", "gift_card_min_in_cents"): + op.drop_column("mps_shop", "gift_card_min_in_cents") + if _column_exists("mps_shop", "gift_card_max_in_cents"): + op.drop_column("mps_shop", "gift_card_max_in_cents") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/scripts/alembic/versions/f8201a9ba045_add_gift_card_tables_and_shop_settings.py` around lines 102 - 108, The downgrade() fails to fully reverse upgrade() (it never removes mps_cart.json_gift_cards) and naively drops tables/columns which will error on partially-applied DBs; update downgrade() to drop the mps_cart.json_gift_cards column and guard each drop with an existence check (use SQLAlchemy Inspector or context.get_bind() to verify table/column existence) before calling op.drop_table or op.drop_column for mps_cart_gift_card, mps_gift_card_transaction, mps_gift_card, and the mps_shop columns gift_card_enabled, gift_card_min_in_cents, gift_card_max_in_cents so the rollback is idempotent and fully reverses upgrade(). ``` </details>

coderabbitai commented

2026-03-07 23:39:04 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🟠 Major

Guard the initial ring seed against stale async responses.

This callback writes ringProductIds using the mutable currentProductId. If the user navigates before the fetch resolves, the old response can overwrite the new ring state and persist the wrong ring in localStorage.

Suggested fix

     // Seed ring from server if localStorage is empty (first visit, cache cleared)
     if (!ringProductIds.length && currentProductId) {
-        fetchWatchData(currentProductId).then(function(data) {
+        var seedProductId = currentProductId;
+        fetchWatchData(seedProductId).then(function(data) {
+            if (currentProductId !== seedProductId) return;
             if (data.ring && data.ring.length) {
                 ringProductIds = data.ring;
-                syncRingPosition(currentProductId);
+                syncRingPosition(seedProductId);
                 saveRingState();
                 updateProgressDisplay();
             }
             preloadNext();
         }).catch(function() {
+            if (currentProductId !== seedProductId) return;
             preloadNext();
         });
     } else {
         preloadNext();
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    // Seed ring from server if localStorage is empty (first visit, cache cleared)
    if (!ringProductIds.length && currentProductId) {
        var seedProductId = currentProductId;
        fetchWatchData(seedProductId).then(function(data) {
            if (currentProductId !== seedProductId) return;
            if (data.ring && data.ring.length) {
                ringProductIds = data.ring;
                syncRingPosition(seedProductId);
                saveRingState();
                updateProgressDisplay();
            }
            preloadNext();
        }).catch(function() {
            if (currentProductId !== seedProductId) return;
            preloadNext();
        });
    } else {
        preloadNext();
    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/static/js/watch.js` around lines 1841 - 1856, The async
fetchWatchData callback can overwrite ringProductIds when the user navigates
before the response arrives; to fix, capture the product id at request time
(e.g., const requestedId = currentProductId) before calling fetchWatchData and,
inside the .then handler, verify the live currentProductId still equals
requestedId (or that data is tagged for requestedId) before assigning
ringProductIds, calling syncRingPosition, saveRingState, or
updateProgressDisplay; if it doesn't match, ignore the stale response and still
call preloadNext as needed.

_⚠️ Potential issue_ | _🟠 Major_ **Guard the initial ring seed against stale async responses.** This callback writes `ringProductIds` using the mutable `currentProductId`. If the user navigates before the fetch resolves, the old response can overwrite the new ring state and persist the wrong ring in localStorage. <details> <summary>Suggested fix</summary> ```diff // Seed ring from server if localStorage is empty (first visit, cache cleared) if (!ringProductIds.length && currentProductId) { - fetchWatchData(currentProductId).then(function(data) { + var seedProductId = currentProductId; + fetchWatchData(seedProductId).then(function(data) { + if (currentProductId !== seedProductId) return; if (data.ring && data.ring.length) { ringProductIds = data.ring; - syncRingPosition(currentProductId); + syncRingPosition(seedProductId); saveRingState(); updateProgressDisplay(); } preloadNext(); }).catch(function() { + if (currentProductId !== seedProductId) return; preloadNext(); }); } else { preloadNext(); } ``` </details>  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion:-15+0 // Seed ring from server if localStorage is empty (first visit, cache cleared) if (!ringProductIds.length && currentProductId) { var seedProductId = currentProductId; fetchWatchData(seedProductId).then(function(data) { if (currentProductId !== seedProductId) return; if (data.ring && data.ring.length) { ringProductIds = data.ring; syncRingPosition(seedProductId); saveRingState(); updateProgressDisplay(); } preloadNext(); }).catch(function() { if (currentProductId !== seedProductId) return; preloadNext(); }); } else { preloadNext(); } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/static/js/watch.js` around lines 1841 - 1856, The async fetchWatchData callback can overwrite ringProductIds when the user navigates before the response arrives; to fix, capture the product id at request time (e.g., const requestedId = currentProductId) before calling fetchWatchData and, inside the .then handler, verify the live currentProductId still equals requestedId (or that data is tagged for requestedId) before assigning ringProductIds, calling syncRingPosition, saveRingState, or updateProgressDisplay; if it doesn't match, ignore the stale response and still call preloadNext as needed. ``` </details>

coderabbitai commented

2026-03-07 23:39:06 -05:00

(Migrated from git2.unturf.com)

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if watch.js constructs media URLs that need updating
rg -n "shop_cdn_endpoint|secure_uploads|endpoint.*product" make_post_sell/static/js/watch.js || echo "watch.js not found or no matches"

# Check the watch.py endpoint for how it provides URLs
rg -n "cdn_endpoint|secure_uploads" make_post_sell/views/watch.py || echo "watch.py not found or no matches"

Repository: engineering/make-post-sell/make_post_sell

Length of output: 261

🏁 Script executed:

# Find watch.js in the repository (may be in different location)
fd -t f "watch\.js" .

# Check the structure of the static directory
fd -t f "\.js$" make_post_sell/static/js/ | head -20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 414

🏁 Script executed:

# Look at the specific lines in watch.py to understand what they do
sed -n '90,105p' make_post_sell/views/watch.py
sed -n '130,145p' make_post_sell/views/watch.py
sed -n '170,185p' make_post_sell/views/watch.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 1636

🏁 Script executed:

# Check if there's an updatePageContent function anywhere
rg -n "updatePageContent" make_post_sell/

Repository: engineering/make-post-sell/make_post_sell

Length of output: 505

🏁 Script executed:

# Look at the updatePageContent function in watch.js
sed -n '662,750p' make_post_sell/static/js/watch.js

Repository: engineering/make-post-sell/make_post_sell

Length of output: 3793

🏁 Script executed:

# Check the full watch.py to see what JSON it returns
# First, let's see the return statement and structure
rg -n "return.*json|jsonify|return {" make_post_sell/views/watch.py | head -20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 206

🏁 Script executed:

# Get a broader view of watch.py to understand the endpoint structure
wc -l make_post_sell/views/watch.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 118

🏁 Script executed:

# Let's look at the return statements in watch.py more carefully
grep -n "return\|jsonify" make_post_sell/views/watch.py | head -20

Repository: engineering/make-post-sell/make_post_sell

Length of output: 442

🏁 Script executed:

# Let's get the full structure of the response - look near the end of watch.py where it builds the response
tail -100 make_post_sell/views/watch.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 3800

🏁 Script executed:

# Check if media_url, instrumentals_url, vocals_url are mentioned in watch.py
rg -n "media_url|instrumentals_url|vocals_url" make_post_sell/views/watch.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 425

🏁 Script executed:

# Look at where media_url is created in watch.py
sed -n '62,75p' make_post_sell/views/watch.py

Repository: engineering/make-post-sell/make_post_sell

Length of output: 630

Update watch.py to return CDN endpoint URLs for media, matching the template pattern.

The template now uses request.shop_cdn_endpoint for the video URL, but watch.py returns presigned S3 URLs via generate_presigned_url() for media_url, instrumentals_url, and vocals_url. This creates a scheme mismatch: the initial page load uses persistent CDN URLs, but navigation in watch mode switches to temporary presigned URLs (ExpiresIn=900), which will expire after 15 minutes and break playback.

watch.py already uses request.shop_cdn_endpoint for file_url and thumbnail_url. Update media_url, instrumentals_url, and vocals_url to follow the same pattern for consistency across watch mode lifecycle.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@make_post_sell/templates/content.j2` around lines 42 - 45, The media URL
generation in watch.py currently returns presigned S3 URLs (via
generate_presigned_url()) for media_url, instrumentals_url, and vocals_url
causing a scheme mismatch with templates that expect persistent CDN URLs; update
watch.py to construct these three URLs using request.shop_cdn_endpoint the same
way file_url and thumbnail_url are built (i.e., combine
request.shop_cdn_endpoint with product.s3_path and the appropriate path/filename
pattern used for product media) instead of calling generate_presigned_url(),
ensuring media_url, instrumentals_url, and vocals_url are consistent CDN
endpoints that won’t expire.

_⚠️ Potential issue_ | _🔴 Critical_ <details> <summary>🧩 Analysis chain</summary> 🏁 Script executed: ```shell #!/bin/bash # Check if watch.js constructs media URLs that need updating rg -n "shop_cdn_endpoint|secure_uploads|endpoint.*product" make_post_sell/static/js/watch.js || echo "watch.js not found or no matches" # Check the watch.py endpoint for how it provides URLs rg -n "cdn_endpoint|secure_uploads" make_post_sell/views/watch.py || echo "watch.py not found or no matches" ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 261 --- 🏁 Script executed: ```shell # Find watch.js in the repository (may be in different location) fd -t f "watch\.js" . # Check the structure of the static directory fd -t f "\.js$" make_post_sell/static/js/ | head -20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 414 --- 🏁 Script executed: ```shell # Look at the specific lines in watch.py to understand what they do sed -n '90,105p' make_post_sell/views/watch.py sed -n '130,145p' make_post_sell/views/watch.py sed -n '170,185p' make_post_sell/views/watch.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 1636 --- 🏁 Script executed: ```shell # Check if there's an updatePageContent function anywhere rg -n "updatePageContent" make_post_sell/ ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 505 --- 🏁 Script executed: ```shell # Look at the updatePageContent function in watch.js sed -n '662,750p' make_post_sell/static/js/watch.js ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 3793 --- 🏁 Script executed: ```shell # Check the full watch.py to see what JSON it returns # First, let's see the return statement and structure rg -n "return.*json|jsonify|return {" make_post_sell/views/watch.py | head -20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 206 --- 🏁 Script executed: ```shell # Get a broader view of watch.py to understand the endpoint structure wc -l make_post_sell/views/watch.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 118 --- 🏁 Script executed: ```shell # Let's look at the return statements in watch.py more carefully grep -n "return\|jsonify" make_post_sell/views/watch.py | head -20 ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 442 --- 🏁 Script executed: ```shell # Let's get the full structure of the response - look near the end of watch.py where it builds the response tail -100 make_post_sell/views/watch.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 3800 --- 🏁 Script executed: ```shell # Check if media_url, instrumentals_url, vocals_url are mentioned in watch.py rg -n "media_url|instrumentals_url|vocals_url" make_post_sell/views/watch.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 425 --- 🏁 Script executed: ```shell # Look at where media_url is created in watch.py sed -n '62,75p' make_post_sell/views/watch.py ``` Repository: engineering/make-post-sell/make_post_sell Length of output: 630 --- </details> **Update watch.py to return CDN endpoint URLs for media, matching the template pattern.** The template now uses `request.shop_cdn_endpoint` for the video URL, but watch.py returns presigned S3 URLs via `generate_presigned_url()` for `media_url`, `instrumentals_url`, and `vocals_url`. This creates a scheme mismatch: the initial page load uses persistent CDN URLs, but navigation in watch mode switches to temporary presigned URLs (ExpiresIn=900), which will expire after 15 minutes and break playback. watch.py already uses `request.shop_cdn_endpoint` for `file_url` and `thumbnail_url`. Update `media_url`, `instrumentals_url`, and `vocals_url` to follow the same pattern for consistency across watch mode lifecycle. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@make_post_sell/templates/content.j2` around lines 42 - 45, The media URL generation in watch.py currently returns presigned S3 URLs (via generate_presigned_url()) for media_url, instrumentals_url, and vocals_url causing a scheme mismatch with templates that expect persistent CDN URLs; update watch.py to construct these three URLs using request.shop_cdn_endpoint the same way file_url and thumbnail_url are built (i.e., combine request.shop_cdn_endpoint with product.s3_path and the appropriate path/filename pattern used for product media) instead of calling generate_presigned_url(), ensuring media_url, instrumentals_url, and vocals_url are consistent CDN endpoints that won’t expire. ``` </details>