Review follow-ups: PR #525 — feature: automatic sized binders, QR purchase requests and branded covers #771

New Issue

2026-09-21T20:44:14Z

gambit-admin commented

2026-09-21 20:44:14 +00:00

Non-blocking findings from the auto code review of PR #525 (feature: automatic sized binders, QR purchase requests and branded covers) — captured for batch cleanup rather than blocking the PR.
Commit 2253d45 · https://github.com/Gambit-Inc/gambit/pull/525

SHOULD-FIX:

frontend/app/binder/[token]/page.js:~603 (<BinderCover binder={profile?.binder} …/> on the legacy path): the cover now renders on every existing shared binder page, not only sized ones. Legacy binders have settings = null, so BinderCover shows a blue #1457e6 banner. It repeats the binder name and adds "A collection worth sharing." and the "Powered by Gambit" badge. This is a visible production change to every existing public binder. It also contradicts docs/binders-beta-ip.md, which says existing binders "keep legacy rendering until configured". Render it only when binder.settings is set, or drop this line, since the sized path already returns PublicSmartBinder.
backend/app/services/binder_automation.py assign_items (called at transactions.py:5054, intake.py:765, imports.py:1804): every finalize, intake commit and import chunk now runs an inventory/product join plus an automation SELECT … FOR UPDATE for each touched item, even when the vendor has no automations. Two consequences:
- Cost on every write path. It adds N×(2 to 6) queries to the POS sale, intake and import hot paths. Load the vendor's enabled automations once, and return early when there are none.
- Coupling to failures. There is no savepoint or guard around it, so an unexpected error in assignment rolls back the sale or the intake. Wrap it in db.begin_nested() with logging, or accept the coupling knowingly.
- Lock contention. The per-item FOR UPDATE on all of a vendor's enabled automations, held until commit, serializes concurrent finalizes for that vendor.
smart_binders.py create_automation: it calls db.run_sync(assign_inventory) once per item, up to 5,000, in a single HTTP request. Each call does 4 to 6 queries and takes the automation row lock. That is potentially 20k+ round trips inside one request while the vendor's POS finalizations block on the lock. Batch it: load the items and products once, then compute the assignments in memory. Alternatively, cap fill_existing far lower or move it to a background job.
binders.py:403 delete_binder: the delete now cascades through the new ON DELETE CASCADE FKs. It removes binder_customer_requests, which are the vendor's pull-request records with customer contact details. It also removes binder_cover_uploads and, through them, binder_cover_reports. That contradicts docs/binders-beta-ip.md, which says "audit records remain until workspace deletion or the adopted retention procedure". Decide deliberately:
- Make binder_id on requests and uploads SET NULL or RESTRICT.
- Or block deleting a binder that has requests or covers.
- Or correct the doc.
Separately, deleting an automated binder is allowed while all item edits on it return 409. Its stock is not re-filed until a new scan, because the fill_existing backfill only runs at rule creation.
core.py:1304 _auto_publish_to_binder (existing code, but this PR makes it unsafe): a manual binder that has opted into sized settings and has auto_publish_new_inventory=True still gets max_sort+10 with no capacity check. Items can land past pockets*sheets*2, and PublicSmartBinder only renders sheets*2 pages, so those cards silently never appear. add_binder_items got the capacity guard and this path did not. Either skip or guard when binder.settings is set, or reject enabling auto-publish on a sized binder.

NITS:

backend/app/routes/auth.py:352: the local import re-imports BinderCoverUpload, which is already imported at module level (line 46). It is redundant and shadows the module-level name inside the function.
frontend/app/binders/automatic/page.jsx, cover title/subtitle inputs: list={binder-filter-${key}} points at datalists that don't exist for title and subtitle. It is harmless but dead.
frontend/app/binders/automatic/page.jsx, preset <select defaultValue="custom">: choosing "Custom filters" resets rules to {}. That wipes any filters the user has already typed, and the control is uncontrolled.
frontend/app/binders/BindersPageDropdown.jsx:~367: setItemView(...) runs on every binder load. A view the user picked is reset after each refresh of a sized binder.
public_binder_items re-serializes the whole binder for every 300-item page. Sized binders can hold up to 7,200 slots, so PublicSmartBinder's paging loop does O(n²/300) work server-side.
frontend/app/components/binders/PublicSmartBinder.jsx, post(): res.json() runs before the res.ok check. A non-JSON error such as a proxy 502 or 429 body surfaces as a raw JSON parse error to the customer.

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #525** (feature: automatic sized binders, QR purchase requests and branded covers) — captured for batch cleanup rather than blocking the PR. Commit `2253d45` · https://github.com/Gambit-Inc/gambit/pull/525 SHOULD-FIX: 1. `frontend/app/binder/[token]/page.js:~603` (`<BinderCover binder={profile?.binder} …/>` on the legacy path): the cover now renders on every existing shared binder page, not only sized ones. Legacy binders have `settings = null`, so `BinderCover` shows a blue `#1457e6` banner. It repeats the binder name and adds "A collection worth sharing." and the "Powered by Gambit" badge. This is a visible production change to every existing public binder. It also contradicts `docs/binders-beta-ip.md`, which says existing binders "keep legacy rendering until configured". Render it only when `binder.settings` is set, or drop this line, since the sized path already returns `PublicSmartBinder`. 2. `backend/app/services/binder_automation.py` `assign_items` (called at `transactions.py:5054`, `intake.py:765`, `imports.py:1804`): every finalize, intake commit and import chunk now runs an inventory/product join plus an automation `SELECT … FOR UPDATE` for each touched item, even when the vendor has no automations. Two consequences: - **Cost on every write path.** It adds N×(2 to 6) queries to the POS sale, intake and import hot paths. Load the vendor's enabled automations once, and return early when there are none. - **Coupling to failures.** There is no savepoint or guard around it, so an unexpected error in assignment rolls back the sale or the intake. Wrap it in `db.begin_nested()` with logging, or accept the coupling knowingly. - **Lock contention.** The per-item `FOR UPDATE` on all of a vendor's enabled automations, held until commit, serializes concurrent finalizes for that vendor. 3. `smart_binders.py` `create_automation`: it calls `db.run_sync(assign_inventory)` once per item, up to 5,000, in a single HTTP request. Each call does 4 to 6 queries and takes the automation row lock. That is potentially 20k+ round trips inside one request while the vendor's POS finalizations block on the lock. Batch it: load the items and products once, then compute the assignments in memory. Alternatively, cap `fill_existing` far lower or move it to a background job. 4. `binders.py:403` `delete_binder`: the delete now cascades through the new `ON DELETE CASCADE` FKs. It removes `binder_customer_requests`, which are the vendor's pull-request records with customer contact details. It also removes `binder_cover_uploads` and, through them, `binder_cover_reports`. That contradicts `docs/binders-beta-ip.md`, which says "audit records remain until workspace deletion or the adopted retention procedure". Decide deliberately: - Make `binder_id` on requests and uploads `SET NULL` or `RESTRICT`. - Or block deleting a binder that has requests or covers. - Or correct the doc. Separately, deleting an automated binder is allowed while all item edits on it return 409. Its stock is not re-filed until a new scan, because the `fill_existing` backfill only runs at rule creation. 5. `core.py:1304` `_auto_publish_to_binder` (existing code, but this PR makes it unsafe): a manual binder that has opted into sized `settings` and has `auto_publish_new_inventory=True` still gets `max_sort+10` with no capacity check. Items can land past `pockets*sheets*2`, and `PublicSmartBinder` only renders `sheets*2` pages, so those cards silently never appear. `add_binder_items` got the capacity guard and this path did not. Either skip or guard when `binder.settings` is set, or reject enabling auto-publish on a sized binder. NITS: 1. `backend/app/routes/auth.py:352`: the local import re-imports `BinderCoverUpload`, which is already imported at module level (line 46). It is redundant and shadows the module-level name inside the function. 2. `frontend/app/binders/automatic/page.jsx`, cover title/subtitle inputs: `list={`binder-filter-${key}`}` points at datalists that don't exist for `title` and `subtitle`. It is harmless but dead. 3. `frontend/app/binders/automatic/page.jsx`, preset `<select defaultValue="custom">`: choosing "Custom filters" resets `rules` to `{}`. That wipes any filters the user has already typed, and the control is uncontrolled. 4. `frontend/app/binders/BindersPageDropdown.jsx:~367`: `setItemView(...)` runs on every binder load. A view the user picked is reset after each refresh of a sized binder. 5. `public_binder_items` re-serializes the whole binder for every 300-item page. Sized binders can hold up to 7,200 slots, so `PublicSmartBinder`'s paging loop does O(n²/300) work server-side. 6. `frontend/app/components/binders/PublicSmartBinder.jsx`, `post()`: `res.json()` runs before the `res.ok` check. A non-JSON error such as a proxy 502 or 429 body surfaces as a raw JSON parse error to the customer. --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-21 20:44:14 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#771