feat(assets): vector QR label export (PDF sheet + label-printer files), paid - #2595
Open
carlosvirreira wants to merge 11 commits into
Open
feat(assets): vector QR label export (PDF sheet + label-printer files), paid#2595carlosvirreira wants to merge 11 commits into
carlosvirreira wants to merge 11 commits into
Conversation
…), paid
Replace the blurry raster QR download with a single vector pipeline used
everywhere: bulk + single asset, download + on-screen preview + print.
Export (asset index, Actions > Export QR labels), two device-keyed journeys:
- "Print on a regular printer": react-to-print PDF sheet on plain paper, with
paper (Letter/A4), size (S/M/L with mm), cut guides, an on-screen print-dialog
tip, and a fit-to-width preview on small screens.
- "Use a label printer or sticker sheets": a zip of vector SVGs + manifest.csv
+ a plain-language README.txt with step-by-step instructions.
Implementation:
- QR rendered as inline vector (qrcode-generator), error-correction L to keep
modules large and scannable on small labels; lowest viable version.
- Identifier text comes from resolveDisplayCode so labels match list views.
- Single-asset code preview: download, preview and print are vector too (one
renderer). Barcode path unchanged (bwip-js raster).
- Paid: gated by the asset-export entitlement (assertUserCanExportAssets server
side; canExportAssets surfaced on the index loader for the in-dialog upsell).
Single-asset QR stays free as the freemium ramp.
- Branding ("Powered by shelf.nu") re-resolved against the tier server-side so
the export can't be used to strip it.
- Remove the old html-to-image/changedpi/.jpg raster bulk path and the 100-item
cap (replaced by a generous safety bound).
Tests: pure label builders incl. a sharp -> jsQR decode-roundtrip proving the
printed QR encodes the right asset URL; loader wiring (resolver text, branding
tier-gate, cross-org IDOR, add-on leak, no cap, paywall); the printable sheet.
Adds jsqr as a test-only devDependency.
🩺 React Doctor — webappFindings on the files changed by this PR:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0b65dcbde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
Closing — opened prematurely. Branch kept; will reopen when ready. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces raster QR exports with vector SVG label generation, adds print-ready label sheets, and rewires bulk export, preview, and download flows to use the new label data and entitlement checks. ChangesQR Bulk Export Redesign
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Neutralize spreadsheet formula injection in manifest.csv — asset names starting with =, +, -, @ are apostrophe-prefixed before quoting. - Bound the export query with take = cap + 1, so a huge select-all isn't fully loaded (with relations) just to be rejected by the limit. - Guard the export dialog against the loader's error payload (e.g. select-all over the limit): show the message instead of crashing on data.assets. Adds tests for the CSV neutralization and the query bound.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/routes/api+/assets.get-assets-for-bulk-qr-download.ts (1)
122-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
statuson overflow error — will return 500 instead of 400.The
ShelfErrorat line 97–103 correctly includesstatus: 400, but this one doesn't. Without an explicit status, the error will likely surface as a 500 to the client, which misrepresents what is a user-input validation error.Proposed fix
if (rows.length > MAX_BULK_QR_EXPORT) { throw new ShelfError({ cause: null, + status: 400, label: "Assets", shouldBeCaptured: false, message: `QR export is limited to ${MAX_BULK_QR_EXPORT} assets at a time. Please narrow your selection.`, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/routes/api`+/assets.get-assets-for-bulk-qr-download.ts around lines 122 - 129, The overflow check that throws a ShelfError when rows.length > MAX_BULK_QR_EXPORT is missing a status property so it will surface as a 500 instead of a 400; update the thrown ShelfError (in the assets.get-assets-for-bulk-qr-download handler where the if (rows.length > MAX_BULK_QR_EXPORT) check occurs) to include status: 400 along with the existing message/label/shouldBeCaptured/cause fields so the client receives a proper 400 validation response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/webapp/app/routes/api`+/assets.get-assets-for-bulk-qr-download.ts:
- Around line 122-129: The overflow check that throws a ShelfError when
rows.length > MAX_BULK_QR_EXPORT is missing a status property so it will surface
as a 500 instead of a 400; update the thrown ShelfError (in the
assets.get-assets-for-bulk-qr-download handler where the if (rows.length >
MAX_BULK_QR_EXPORT) check occurs) to include status: 400 along with the existing
message/label/shouldBeCaptured/cause fields so the client receives a proper 400
validation response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48a2a921-d77f-4db7-bd75-316894846ec5
📒 Files selected for processing (5)
apps/webapp/app/components/assets/bulk-download-qr-dialog.tsxapps/webapp/app/modules/qr/label.test.tsapps/webapp/app/modules/qr/label.tsapps/webapp/app/routes/api+/assets.get-assets-for-bulk-qr-download.tsapps/webapp/test/routes-tests/qr-label-export.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/webapp/app/components/assets/bulk-download-qr-dialog.tsx
- apps/webapp/app/modules/qr/label.test.ts
- apps/webapp/test/routes-tests/qr-label-export.test.ts
- apps/webapp/app/modules/qr/label.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/webapp/app/components/assets/qr-label-sheet.tsx (1)
197-197: 💤 Low valueConsider using
transform: scale()instead ofzoomfor better standards compliance.The
zoomCSS property is non-standard (though widely supported). Usingtransform: scale()would be more standards-compliant and explicitly supported across browsers.♻️ Proposed alternative using transform
- <div style={{ zoom: scale }}> + <div style={{ transform: `scale(${scale})`, transformOrigin: 'top left' }}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/assets/qr-label-sheet.tsx` at line 197, Replace the non-standard CSS usage of zoom on the div (the element using style={{ zoom: scale }}) with a standards-compliant transform: use style={{ transform: `scale(${scale})`, transformOrigin: '0 0' }} and adjust container sizing as needed (e.g., wrap the scaled content in an overflow-hidden container or set width/height to account for the scale) so layout and print/export behavior remain correct; update any references to the old zoom styling in the QRLabelSheet component accordingly.apps/webapp/app/components/code-preview/code-preview.tsx (1)
1-17: ⚡ Quick winAdd the required JSDoc header and exported-type docs.
The updated public QR types still rely on inline field comments only, and the file still starts without the required JSDoc block. Please add proper JSDoc for the file and for these exported types. As per coding guidelines, "every file must start with a JSDoc block explaining its purpose, and every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors".
Also applies to: 27-37, 461-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/code-preview/code-preview.tsx` around lines 1 - 17, This file is missing the required file-level JSDoc header and several exported types/components lack JSDoc; add a top-of-file JSDoc block describing the module purpose and usage, then add JSDoc comments for every exported type, function and component in this file (including the CodePreview component and the public QR types referenced in the file), describing parameters, return values and possible thrown errors; ensure each exported type (the public QR type declarations) and exported functions/components (e.g., CodePreview, any helpers near the later exports) have concise `@param/`@returns/@throws tags as appropriate and follow the project's JSDoc style guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/app/components/code-preview/code-preview.tsx`:
- Around line 255-270: The downloadCode path computes idText locally (using
organization.qrIdDisplayPreference and sequentialId) which can diverge from the
centralized resolver; instead, stop recomputing it inside downloadCode and
either accept a pre-resolved idText prop on CodePreview or call the shared
resolver from apps/webapp/app/modules/ (the same helper used by bulk label flow)
before rendering. Locate the downloadCode function and buildLabelSvg call (and
the similar logic at the other spot around lines 499-503) and replace the local
idText logic with the resolvedIdText passed in (or a single import of the
central resolver) so both single-item and bulk exports use the same identifier
resolution.
In `@apps/webapp/app/utils/svg-to-png.ts`:
- Around line 25-28: The current viewBox parsing silently falls back to 1x1
using the vb variable and then computes pxHeight, which can produce distorted
PNGs; update the viewBox handling in svg-to-png.ts to more robustly parse and
validate the viewBox (accept different attribute ordering/whitespace and numeric
formats) and fail fast: when the regex against svg (viewBox) does not produce
valid numeric width/height values for vb[1] and vb[2], throw a descriptive error
instead of defaulting to 1, and only compute w, h, and pxHeight after successful
validation of those parsed numbers (referencing svg, vb, w, h, pxWidth, pxHeight
and the viewBox attribute).
In `@apps/webapp/test/routes-tests/qr-label-export.test.ts`:
- Around line 69-72: Replace the current hard-coded mock of getAssetsWhereInput
with a spy/mock function that captures its arguments and assert it receives the
current query string (currentSearchParams) when the loader handles the
ALL_SELECTED path; specifically, change the mocked getAssetsWhereInput to a
vi.fn/spy, invoke the loader with ALL_SELECTED_KEY active and a known
querystring, and add assertions that getAssetsWhereInput was called with an
object containing that query string (preserving the ALL_SELECTED_KEY pattern).
Apply the same change/assertions for the other test block referenced (lines
~249-275) so both paths verify the helper receives the serialized filters.
---
Nitpick comments:
In `@apps/webapp/app/components/assets/qr-label-sheet.tsx`:
- Line 197: Replace the non-standard CSS usage of zoom on the div (the element
using style={{ zoom: scale }}) with a standards-compliant transform: use
style={{ transform: `scale(${scale})`, transformOrigin: '0 0' }} and adjust
container sizing as needed (e.g., wrap the scaled content in an overflow-hidden
container or set width/height to account for the scale) so layout and
print/export behavior remain correct; update any references to the old zoom
styling in the QRLabelSheet component accordingly.
In `@apps/webapp/app/components/code-preview/code-preview.tsx`:
- Around line 1-17: This file is missing the required file-level JSDoc header
and several exported types/components lack JSDoc; add a top-of-file JSDoc block
describing the module purpose and usage, then add JSDoc comments for every
exported type, function and component in this file (including the CodePreview
component and the public QR types referenced in the file), describing
parameters, return values and possible thrown errors; ensure each exported type
(the public QR type declarations) and exported functions/components (e.g.,
CodePreview, any helpers near the later exports) have concise
`@param/`@returns/@throws tags as appropriate and follow the project's JSDoc style
guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e0de931-2428-438e-a3b8-6f4656c608e9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/webapp/app/components/assets/bulk-actions-dropdown.tsxapps/webapp/app/components/assets/bulk-download-qr-dialog.tsxapps/webapp/app/components/assets/qr-label-sheet.test.tsxapps/webapp/app/components/assets/qr-label-sheet.tsxapps/webapp/app/components/assets/qr-svg.tsxapps/webapp/app/components/code-preview/code-preview.tsxapps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/qr/label.test.tsapps/webapp/app/modules/qr/label.tsapps/webapp/app/modules/qr/utils.server.tsapps/webapp/app/routes/api+/assets.get-assets-for-bulk-qr-download.tsapps/webapp/app/utils/svg-to-png.tsapps/webapp/package.jsonapps/webapp/test/routes-tests/qr-label-export.test.ts
added 2 commits
June 4, 2026 13:53
- Single-item code preview now derives the QR id text via the shared resolveDisplayCode (incl. per-asset preferredBarcodeId), so single-item and bulk label text can't diverge for barcode-preference / override assets. - Over-limit QR export now returns 400 instead of 500 (free-tier was already 403). - svgToPngBlob throws when the SVG viewBox is missing instead of silently emitting a distorted 1x1-derived PNG. - Test: getAssetsWhereInput is now a spy asserting the active filters are forwarded on the select-all path.
…wnload) Collapse the three drifted label layouts onto a single source, buildLabelSvg, so the preview, print, single download, SVG zip and PDF sheet are byte-identical (QR on top -> name -> id -> branding). - New <QrLabelCard> renders buildLabelSvg as a vector <img>; used by the asset-page preview/print and the PDF sheet cells. - Single download + SVG zip already used buildLabelSvg, so everything now matches. Remove the old QrLabel component and QrSvg (both now unused). - buildLabelSvg truncates long titles with an ellipsis (SVG text can't wrap). - Rename "Label size" -> "QR size": the mm is measured across the QR (incl. its white quiet-zone border), not the whole sticker. Tests updated for the <img>-of-svg cards; full suite green (2166).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/webapp/app/components/assets/qr-label-sheet.tsx (1)
55-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpose the active segmented option to assistive tech.
These buttons only communicate the selected Paper/QR size through color. Screen readers won't get the current state unless each option exposes it (
aria-pressedor radio semantics) and the group has an accessible label. As per coding guidelines, "Use WCAG 2.1 AA as the minimum accessibility standard: ensure ... keyboard accessibility for all interactive elements ... and visible focus indicators."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/assets/qr-label-sheet.tsx` around lines 55 - 84, Segmented currently only uses color to indicate selection; update the component to expose the active option to assistive tech by adding an accessible group label and per-button state: give the container an accessible role and label (e.g., role="radiogroup" plus an aria-label or accept a label prop) and on each button add aria-pressed={value === o.value} (or role="radio" with aria-checked) and ensure the selected item is keyboard-focusable (set tabIndex= value === o.value ? 0 : -1) so screen readers and keyboard users can perceive and navigate the selected option; locate the Segmented function and modify the container div and each button accordingly (props: value, options, onChange).apps/webapp/app/components/code-preview/code-preview.tsx (1)
338-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrint the canonical label node, not the preview wrapper.
useReactToPrintprintscaptureDivRef, and in the QR branch that ref is attached to the bordered/padded preview container. That makes the single-label printout include extra chrome and different physical dimensions than the shared SVG used for download and sheet export.Suggested fix
{selectedCode?.type === "qr" ? ( - <div - ref={captureDivRef} - className="flex w-[260px] flex-col items-center rounded border bg-white p-4" - > - <QrLabelCard - url={selectedCode.qrData?.url ?? ""} - title={item.name} - idText={resolvedIdText || selectedCode.id} - showBranding={resolvedShowShelfBranding} - width="220px" - /> - </div> + <div className="flex w-[260px] flex-col items-center rounded border bg-white p-4"> + <div ref={captureDivRef}> + <QrLabelCard + url={selectedCode.qrData?.url ?? ""} + title={item.name} + idText={resolvedIdText || selectedCode.id} + showBranding={resolvedShowShelfBranding} + width="220px" + /> + </div> + </div> ) : selectedCode?.type === "barcode" ? (Also applies to: 423-434
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/code-preview/code-preview.tsx` around lines 338 - 345, The print setup is targeting captureDivRef (the padded preview wrapper) so prints include extra chrome; change the useReactToPrint contentRef to point to the canonical label DOM node used for download/sheet export (the same shared SVG/label node used by your download/export logic) instead of captureDivRef, and ensure the onBeforePrint still awaits images on that canonical node (i.e., replace references to captureDivRef in the onBeforePrint callback with the canonical label ref). Apply the same change to the other useReactToPrint instance mentioned in the comment (the second block).
🧹 Nitpick comments (3)
apps/webapp/app/components/code-preview/code-preview.tsx (1)
1-25: ⚡ Quick winAdd the required JSDoc for the changed exports.
This file still starts without the required module docblock, and the changed exports (
CodeType,CodePreview,BarcodeLabel) are undocumented. That breaks the repo's TS/TSX documentation contract. As per coding guidelines, "All code must include inline documentation with JSDoc comments; every file must start with a JSDoc block explaining its purpose, and every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."Also applies to: 28-44, 121-134, 482-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/code-preview/code-preview.tsx` around lines 1 - 25, Add the missing JSDoc comments: insert a module-level JSDoc block at the top of the file describing the component's purpose and exports, then add JSDoc blocks for the exported type CodeType, the React component CodePreview, and the exported component/function BarcodeLabel; each JSDoc should summarize the symbol, list parameters/props (with types and meaning), describe the return value (e.g., JSX.Element or type), and note any thrown errors or side effects (e.g., uses print/download, external libs like html-to-image). Ensure the docblocks follow the repo style used elsewhere (see similar blocks at lines 28-44, 121-134, 482-533) and place them immediately above the corresponding export declarations.apps/webapp/test/components/code-preview/labels.test.tsx (1)
1-11: ⚡ Quick winDocument this mock and add the missing file docblock.
The
lottie-reactmock is missing the required// why:rationale, and this TSX test file still starts without a JSDoc header. Both are repo standards for changed test files. As per coding guidelines, "every file must start with a JSDoc block explaining its purpose" and "Every mock in tests must be accompanied by a// why:comment explaining the reason for mocking."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/test/components/code-preview/labels.test.tsx` around lines 1 - 11, Add a top-of-file JSDoc block describing the purpose of this test file (e.g., verifies BarcodeLabel rendering/behavior) and why it exists, and annotate the test-local mock for "lottie-react" with a `// why:` comment immediately above the vi.mock call explaining why the module is mocked (e.g., to avoid rendering lottie animations in unit tests and keep snapshot stability); ensure references to BarcodeLabel (and the comment about QrLabelCard/buildLabelSvg if helpful) remain intact so reviewers can locate the behavior being tested.apps/webapp/app/components/assets/qr-label-sheet.test.tsx (1)
18-32: ⚡ Quick winReplace the inline asset literals with a small factory/helper.
ASSETSis hardcoded in the module now, which makes edge-case setup noisier as this suite grows. A local factory keeps each test focused on the behavior it actually cares about. Based on learnings, "Avoid hardcoding data within tests; use factories to keep tests clean and maintainable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/assets/qr-label-sheet.test.tsx` around lines 18 - 32, Replace the hardcoded ASSETS array with a small factory/helper and update renderSheet to use it: remove the module-level ASSETS constant, add a createAsset(overrides?) helper that returns an asset object with default fields (id, title, qrId, idText) and an optional createAssets(count or list of overrides) helper to build arrays; then update renderSheet to call these helpers to supply assets to the QrLabelSheet component (referencing QrLabelSheet, renderSheet, and the former ASSETS symbol to locate the code). Ensure tests can pass overrides to the factory for edge cases so individual tests only specify the fields they care about.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/webapp/app/components/assets/qr-label-sheet.tsx`:
- Around line 55-84: Segmented currently only uses color to indicate selection;
update the component to expose the active option to assistive tech by adding an
accessible group label and per-button state: give the container an accessible
role and label (e.g., role="radiogroup" plus an aria-label or accept a label
prop) and on each button add aria-pressed={value === o.value} (or role="radio"
with aria-checked) and ensure the selected item is keyboard-focusable (set
tabIndex= value === o.value ? 0 : -1) so screen readers and keyboard users can
perceive and navigate the selected option; locate the Segmented function and
modify the container div and each button accordingly (props: value, options,
onChange).
In `@apps/webapp/app/components/code-preview/code-preview.tsx`:
- Around line 338-345: The print setup is targeting captureDivRef (the padded
preview wrapper) so prints include extra chrome; change the useReactToPrint
contentRef to point to the canonical label DOM node used for download/sheet
export (the same shared SVG/label node used by your download/export logic)
instead of captureDivRef, and ensure the onBeforePrint still awaits images on
that canonical node (i.e., replace references to captureDivRef in the
onBeforePrint callback with the canonical label ref). Apply the same change to
the other useReactToPrint instance mentioned in the comment (the second block).
---
Nitpick comments:
In `@apps/webapp/app/components/assets/qr-label-sheet.test.tsx`:
- Around line 18-32: Replace the hardcoded ASSETS array with a small
factory/helper and update renderSheet to use it: remove the module-level ASSETS
constant, add a createAsset(overrides?) helper that returns an asset object with
default fields (id, title, qrId, idText) and an optional createAssets(count or
list of overrides) helper to build arrays; then update renderSheet to call these
helpers to supply assets to the QrLabelSheet component (referencing
QrLabelSheet, renderSheet, and the former ASSETS symbol to locate the code).
Ensure tests can pass overrides to the factory for edge cases so individual
tests only specify the fields they care about.
In `@apps/webapp/app/components/code-preview/code-preview.tsx`:
- Around line 1-25: Add the missing JSDoc comments: insert a module-level JSDoc
block at the top of the file describing the component's purpose and exports,
then add JSDoc blocks for the exported type CodeType, the React component
CodePreview, and the exported component/function BarcodeLabel; each JSDoc should
summarize the symbol, list parameters/props (with types and meaning), describe
the return value (e.g., JSX.Element or type), and note any thrown errors or side
effects (e.g., uses print/download, external libs like html-to-image). Ensure
the docblocks follow the repo style used elsewhere (see similar blocks at lines
28-44, 121-134, 482-533) and place them immediately above the corresponding
export declarations.
In `@apps/webapp/test/components/code-preview/labels.test.tsx`:
- Around line 1-11: Add a top-of-file JSDoc block describing the purpose of this
test file (e.g., verifies BarcodeLabel rendering/behavior) and why it exists,
and annotate the test-local mock for "lottie-react" with a `// why:` comment
immediately above the vi.mock call explaining why the module is mocked (e.g., to
avoid rendering lottie animations in unit tests and keep snapshot stability);
ensure references to BarcodeLabel (and the comment about
QrLabelCard/buildLabelSvg if helpful) remain intact so reviewers can locate the
behavior being tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce0d17c5-b9c8-43ab-96b0-5bf8402f840a
📒 Files selected for processing (7)
apps/webapp/app/components/assets/qr-label-card.tsxapps/webapp/app/components/assets/qr-label-sheet.test.tsxapps/webapp/app/components/assets/qr-label-sheet.tsxapps/webapp/app/components/code-preview/code-preview.tsxapps/webapp/app/modules/qr/label.test.tsapps/webapp/app/modules/qr/label.tsapps/webapp/test/components/code-preview/labels.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/modules/qr/label.ts
- apps/webapp/app/modules/qr/label.test.ts
A select-all export over MAX_BULK_QR_EXPORT returns an error payload that useApiQuery surfaces as a truthy `data` with no `assets` array. The `count` computation read `data?.assets.length`, which only guards a nullish `data` and threw on `.length` for the error payload — white-screening the dialog before the existing hasAssets/apiErrorMessage guard could render the cap message. Compute `count` from the hasAssets guard instead, so the dialog shows the 'limited to 1500 assets' message as intended.
# Conflicts: # apps/webapp/app/components/assets/bulk-download-qr-dialog.tsx
- Remove bulk-download-qr-dialog.test.tsx: it covered the deleted raster download path (html-to-image, fetch-based request superseding, old loader payload shape). The redesign uses useApiQuery keyed on the search params and a click-driven zip build, so the stale-cache and superseded-response races it guarded can no longer occur; the new design is covered by qr-label-export, label, and qr-label-sheet tests. - Add handledClientError to the Logger mock: http.server's error() path now calls it, so the loader's 4xx/error branches threw under the partial mock. - Give the sharp+jsQR decode-roundtrip tests a 20s timeout so they don't flake past the 5s default under the fully-parallel suite.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new vector-based QR label export flow for assets (paid, tier-gated), replacing the legacy rasterized bulk QR JPG export. This introduces two export “journeys” (printable sheet + label-printer zip bundle) and aligns single-asset QR preview/download/print to use the same vector label renderer.
Changes:
- Introduces a pure vector QR label generator (
buildLabelSvg, manifest CSV, zip entry builder) and uses it across bulk + single-asset surfaces. - Reworks the bulk export loader to return resolver-driven label data (QR id + idText), tier-gated branding, and a higher export cap with a safety bound.
- Updates UI to present the two export journeys and adds test coverage (including QR decode round-trip via
sharp+jsQR).
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks jsqr addition for tests. |
| apps/webapp/package.json | Adds jsqr devDependency for QR decode tests. |
| apps/webapp/test/routes-tests/qr-label-export.test.ts | New behavioral tests for the bulk QR export loader (tier gate, resolver text, branding, bounds). |
| apps/webapp/test/components/code-preview/labels.test.tsx | Removes legacy QrLabel tests; notes coverage moved to new label tests. |
| apps/webapp/app/utils/svg-to-png.ts | Adds browser helper to rasterize generated SVG labels into sharp PNGs. |
| apps/webapp/app/routes/api+/assets.get-assets-for-bulk-qr-download.ts | Replaces raster bulk export loader with vector-label data + tier-gated branding + higher capped export. |
| apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx | Passes preferredBarcodeId for consistent display-code resolution. |
| apps/webapp/app/modules/qr/utils.server.ts | Adds url to generated QR payload for vector re-rendering on the client. |
| apps/webapp/app/modules/qr/label.ts | New “single source of truth” vector label builder + manifest CSV + zip entries/README. |
| apps/webapp/app/modules/qr/label.test.ts | New unit tests including QR decode round-trip and CSV/zip contract checks. |
| apps/webapp/app/modules/asset/data.server.ts | Surfaces canExportAssets in asset index loader data for UI gating/upsell. |
| apps/webapp/app/components/code-preview/code-preview.tsx | Migrates single-asset QR preview/download to vector label rendering + sharp PNG export. |
| apps/webapp/app/components/assets/qr-label-sheet.tsx | New printable sheet UI using react-to-print and mm-based sizing presets. |
| apps/webapp/app/components/assets/qr-label-sheet.test.tsx | RTL tests for sheet rendering and print CSS. |
| apps/webapp/app/components/assets/qr-label-card.tsx | New shared <img> wrapper for the vector label (data URL) used across journeys. |
| apps/webapp/app/components/assets/bulk-download-qr-dialog.tsx | Replaces old raster zip flow with two-journey hub (PDF sheet + SVG zip) and premium upsell. |
| apps/webapp/app/components/assets/bulk-download-qr-dialog.test.tsx | Removes legacy dialog regression tests tied to the old fetch/raster flow. |
| apps/webapp/app/components/assets/bulk-actions-dropdown.tsx | Renames action label to “Export QR labels”. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- use-api-query: guard against out-of-order responses. The dialog stays mounted while selection/filters change, so a slow earlier request could resolve last and overwrite a newer one. Mark superseded runs stale and abort their fetch so only the latest request sets state; update the hook tests for the signal arg. - bulk QR dialog: a token bumped on close stops an in-flight zip build that resolves after dismissal from triggering a stray download or flipping to done. - label csv: neutralize leading tab/CR/LF too, not just the = + - @ formula triggers, matching the documented intent; add a control-char injection test. - qr-label-export test: the select-all filter-forwarding test sliced away the filter param, so it never proved forwarding. Send a real filter and assert it reaches getAssetsWhereInput.
- label svg: build the QR <rect> markup in an array and join once instead of repeated string concatenation in the nested loop — avoids a needless CPU/memory hotspot across a bulk export (up to 1500 labels). - qr-label-sheet: add aria-pressed to the segmented paper/size controls so screen readers convey the active option (was styling-only). - use-api-query: clear any prior error when a new request starts, so a successful refetch no longer leaves a stale error visible; add a test. - Add an aria-pressed assertion to the sheet test.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (1)
apps/webapp/app/routes/api+/assets.get-assets-for-bulk-qr-download.ts:97
- When
assetIdsis an explicit list (non select-all), the API can still receive an arbitrarily large number ofassetIdsquery params. Even thoughtakelimits returned rows, Prisma will still build a hugeWHERE id IN (...)list, which is avoidable work and can become a DoS vector. Consider rejecting early whenassetIds.lengthexceedsMAX_BULK_QR_EXPORT(for non-select-all).
const searchParams = new URL(request.url).searchParams;
const assetIds = searchParams.getAll("assetIds");
if (assetIds.length === 0) {
throw new ShelfError({
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
|
I found some issues and fixed them. I also tested both the print PDF and the export. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Need
Customers can't reliably print Shelf QR labels. The bulk "Download QR Codes" export
rasterized a ~116px QR bitmap into a fixed 300×300 box via
html-to-imageand saved itas
.jpg— unscannable at small label sizes (a real customer demonstrated this on aBrother QL-810W with 0.66"×2.1" labels). The clean SVG path existed but was admin-only and
emitted unlinked codes, so support was hand-emailing SVG batches per account.
Hypothesis
If we render the QR as vector and ship two opinionated, device-keyed journeys — a
print-and-cut PDF sheet and a label-printer file bundle — most customers self-serve a
sharp, scannable label with no support contact.
What ships
Asset index → Actions ▸ Export QR labels, two journeys:
react-to-printPDF sheet (same mechanism as everyother Shelf PDF). Plain paper, paper (Letter/A4) × size (S/M/L, shown in mm with a
per-page count), cut guides, an on-screen print-dialog tip (Scale 100% / Margins None),
and a fit-to-width preview on small screens (print stays real-mm).
.svgper asset +manifest.csv(asset id, name, QR id, scan URL) + a plain-languageREADME.txt.Single-asset code preview (asset/kit overview) is now vector too — download, preview and
print — so there is one renderer everywhere. Barcode path unchanged.
Design decisions (and non-goals)
Lon purpose: higher EC ⇒ more modules ⇒ smaller modules ⇒ worseon a small/low-DPI label. The lever is minimize modules (short URL + lowest version).
Final EC pending a physical print-scan (see Open items).
(
assertUserCanExportAssetsserver-side;canExportAssetssurfaced on the index loaderfor the in-dialog upsell). Single-asset QR stays free as the freemium ramp.
server-side, so the export can't be used to strip it.
resolveDisplayCodeso labels match list views.html-to-image/changedpi/.jpgraster bulk path and the 100-item cap(replaced by a generous safety bound). Deferred: bulk PNG (no demand), Avery-template
alignment, barcode-graphic-on-label.
Key files
app/modules/qr/label.ts— pure vector label builders (SVG, manifest, zip entries, README).app/components/assets/qr-svg.tsx,qr-label-sheet.tsx— vector QR + PDF sheet.app/components/assets/bulk-download-qr-dialog.tsx— the two-journey hub + upsell.app/routes/api+/assets.get-assets-for-bulk-qr-download.ts— loader: resolver text,tier-gated branding, cap lifted, no per-asset
sharp.app/components/code-preview/code-preview.tsx,app/modules/qr/utils.server.ts,app/utils/svg-to-png.ts— single-asset vector (download/preview/print).app/modules/asset/data.server.ts— surfacecanExportAssets.Test plan & tests
buildLabelSvgdecode-roundtrip — rasterize withsharp, decodewith
jsQR, assert it reads back the exact asset URL (proves the printed QR is correct,not just that the lib ran); module-minimization; manifest columns/escaping; zip entries.
tier-gate incl. free-tier-can't-strip, cross-org IDOR (foreign asset absent), add-on leak,
150-asset no-cap, paywall (free → non-200), graceful QR-less skip.
@page+break-inside-avoid, size preset changes density.jsqras a test-only devDependency. No new runtime dependency.pnpm webapp:validateclean; full suite 2164 tests pass, typecheck 0, lint clean.Verification
Driven live in-browser (Playwright) end-to-end across desktop / tablet / mobile:
both journeys, single-asset download/preview/print, the paywall path, and the downloaded
artifacts (SVG decodes to the right URL; zip contains README/manifest). Also walked as
low/mid/high-IQ personas to de-risk support overhead (device-keyed names, in-zip README,
on-screen print tip). Screenshots available on request.
Open items
(default
L). This is the only thing software can't verify.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
manifest.csv,README.txt, and higher export limit (100 → 1,500).UI Updates