Review follow-ups: PR #584 — feature: Gambit G in label QR codes + full GCN printed under the QR #822

New Issue

2026-09-25T03:30:56Z

gambit-admin commented

2026-09-25 03:30:56 +00:00

Non-blocking findings from the auto code review of PR #584 (feature: Gambit G in label QR codes + full GCN printed under the QR) — captured for batch cleanup rather than blocking the PR.
Commit 2c66e21 · https://github.com/Gambit-Inc/gambit/pull/584

SHOULD-FIX:

mobile/rork-recon-card-scanner/expo/components/GambitQrLayer.tsx:5 (also lib/niimbot/gambitQr.test.ts:19, label.test.ts:16): the app now imports qrcode directly, but package.json only declares react-native-qrcode-svg. Its own comment concedes the dependency is transitive. If the package manager nests qrcode under react-native-qrcode-svg, or the version drifts away from 1.5.x, the app bundle and the mo_unit step in verify.sh (which CI installs with npm --legacy-peer-deps) fail to resolve it. Declare qrcode explicitly in package.json and pin it to the version the web app uses. I could not confirm the layout because node_modules is not installed here.
backend/app/services/zpl.py:233 (2x1 label): the price field has no width bound. At pf=52 a price like $12,345.67 is roughly 260 dots wide from x=6. That runs into the QR box, which starts at x=228 with the symbol at x≈243. The old layout had the QR further right. Cap the price with ^FB, or step the font down by length in more than two tiers.
backend/app/services/zpl.py:224 (2x1 label): the card-number field at ^FO6,155 also has no width limit. A long number (up to 30 characters at 18 dots) can run past x=228 into the QR's lowest rows and the GCN caption, which sit at y≈159–184.
backend/app/services/zpl.py:161,219,~297 (all three builders): when gambit_qr_zpl returns "" (payload too large for 2 dots per square), the label prints with no QR. The old short GCN text is also gone, because the branded branch has replaced it. Fall back to the legacy path when the result is empty, so the label still carries an identifier.
backend/app/services/gambit_mark.py:96-104: zxingcpp.create_barcode(..., ec_level=...) and to_image are called with no error handling. A failure other than ImportError (an API change or an encode error) raises straight through label generation instead of falling back to segno. The fallback only triggers on import failure. Catch exceptions around the zxing call and use segno in that case.

NITS:

backend/app/services/zpl.py:216-218: in build_tcg_label, the existing number_field is computed before the branded early return, and the branded branch recomputes its own number. This is duplicated dead work on that path. Reuse number_field instead.

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #584** (feature: Gambit G in label QR codes + full GCN printed under the QR) — captured for batch cleanup rather than blocking the PR. Commit `2c66e21` · https://github.com/Gambit-Inc/gambit/pull/584 SHOULD-FIX: - `mobile/rork-recon-card-scanner/expo/components/GambitQrLayer.tsx:5` (also `lib/niimbot/gambitQr.test.ts:19`, `label.test.ts:16`): the app now imports `qrcode` directly, but `package.json` only declares `react-native-qrcode-svg`. Its own comment concedes the dependency is transitive. If the package manager nests `qrcode` under `react-native-qrcode-svg`, or the version drifts away from 1.5.x, the app bundle and the `mo_unit` step in `verify.sh` (which CI installs with `npm --legacy-peer-deps`) fail to resolve it. Declare `qrcode` explicitly in `package.json` and pin it to the version the web app uses. I could not confirm the layout because `node_modules` is not installed here. - `backend/app/services/zpl.py:233` (2x1 label): the price field has no width bound. At `pf=52` a price like `$12,345.67` is roughly 260 dots wide from x=6. That runs into the QR box, which starts at x=228 with the symbol at x≈243. The old layout had the QR further right. Cap the price with `^FB`, or step the font down by length in more than two tiers. - `backend/app/services/zpl.py:224` (2x1 label): the card-number field at `^FO6,155` also has no width limit. A long number (up to 30 characters at 18 dots) can run past x=228 into the QR's lowest rows and the GCN caption, which sit at y≈159–184. - `backend/app/services/zpl.py:161,219,~297` (all three builders): when `gambit_qr_zpl` returns `""` (payload too large for 2 dots per square), the label prints with no QR. The old short GCN text is also gone, because the branded branch has replaced it. Fall back to the legacy path when the result is empty, so the label still carries an identifier. - `backend/app/services/gambit_mark.py:96-104`: `zxingcpp.create_barcode(..., ec_level=...)` and `to_image` are called with no error handling. A failure other than `ImportError` (an API change or an encode error) raises straight through label generation instead of falling back to segno. The fallback only triggers on import failure. Catch exceptions around the zxing call and use segno in that case. NITS: - `backend/app/services/zpl.py:216-218`: in `build_tcg_label`, the existing `number_field` is computed before the branded early return, and the branded branch recomputes its own `number`. This is duplicated dead work on that path. Reuse `number_field` instead. --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-25 03:30:56 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#822