chore: refactor into monorepo structure (wip) #295
Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRepository restructured into a monorepo: root package made private and simplified, zxing-cpp submodule moved under packages/zxing-wasm, a full zxing-wasm package added (with README and dynamic wasm locateFile), and a new packages/react package added containing a useScanner hook, worker, utils, stories, and build configs; editor/lint configs adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Consumer
participant Share as packages/zxing-wasm/src/share.ts
participant Resolver as import.meta.resolve
participant Runtime as Bundler/Runtime
rect rgb(245,245,255)
Note over Share: Previous (static)
App->>Share: locateFile(filename)
Share-->>App: return new URL("/src/" + filename, import.meta.url).toString()
end
rect rgb(245,255,245)
Note over Share: New (dynamic resolution)
App->>Share: locateFile(filename)
Share->>Resolver: import.meta.resolve(path)
Resolver->>Runtime: resolve to bundle/asset URL
Runtime-->>Resolver: resolved URL
Resolver-->>Share: URL string
Share-->>App: Resolved WASM path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the zxing-wasm project into a monorepo structure by moving the main package into packages/zxing-wasm/ subdirectory. The root package.json is transformed into a monorepo coordinator with minimal dependencies, while the original package configuration is preserved in the new subdirectory.
Key changes:
- Root package.json converted to monorepo root configuration
- Main zxing-wasm package moved to packages/zxing-wasm/ with its own package.json
- Submodule path updated to reflect new directory structure
- README split between root (placeholder) and package-specific documentation
Reviewed Changes
Copilot reviewed 7 out of 6617 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/zxing-wasm/package.json | Complete package.json moved to package directory with all exports, scripts, and dependencies preserved |
| package.json | Root package.json simplified to monorepo coordinator with minimal dev dependencies |
| packages/zxing-wasm/README.md | Full zxing-wasm documentation moved to package directory |
| README.md | Root README reduced to placeholder for monorepo |
| .vscode/settings.json | VS Code settings removed (likely to be reconfigured for monorepo) |
| .gitmodules | Submodule path updated to packages/zxing-wasm/zxing-cpp |
| packages/zxing-wasm/src/share.ts | WASM file resolution changed from hardcoded path to import.meta.resolve |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
7-9: WIP: Root README placeholder.The minimal README is appropriate for a work-in-progress monorepo refactor. Consider adding at minimum:
- Brief description of the monorepo structure
- Link to the main package documentation at
packages/zxing-wasm/README.md- Development setup instructions
packages/zxing-wasm/README.md (1)
1-483: Comprehensive package documentation - looks good!The package README provides thorough documentation covering installation, usage, configuration, and troubleshooting. The content is well-structured and informative.
Optional: Address minor grammar/style issues flagged by static analysis:
- Line 205: Use "custom-defined" (hyphenated)
- Line 340: Use "WeChat" (capital C)
- Line 456: Use "higher-level" (hyphenated)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks).gitmodules(1 hunks).vscode/settings.json(0 hunks)README.md(1 hunks)package.json(2 hunks)packages/zxing-wasm/README.md(1 hunks)packages/zxing-wasm/package.json(1 hunks)packages/zxing-wasm/src/share.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 LanguageTool
packages/zxing-wasm/README.md
[grammar] ~205-~205: Use a hyphen to join words.
Context: ...ons.html#overrides) object with a custom defined [locateFile](https://emscripte...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~340-~340: The official name of this popular chat service is spelled with a capital “C”.
Context: ...e this library in a WeChat mini program <img alt="wechat mini program icon" src="https://github....
(WECHAT)
[grammar] ~456-~456: Use a hyphen to join words.
Context: ...ursive ``` 3. **Are there any higher level libraries that can be used to simp...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (5)
.gitignore (1)
31-32: LGTM! Appropriate for monorepo structure.The broadened glob patterns correctly handle WASM and JS artifacts across multiple package directories in the monorepo.
package.json (2)
2-5: LGTM! Appropriate monorepo root configuration.The root package is correctly configured as a private monorepo coordinator with
private: trueand version0.0.0. The public package API has been moved topackages/zxing-wasm/package.json.
29-40: Clean monorepo root scripts and dependencies.The streamlined scripts and devDependencies are appropriate for a monorepo root. The
catalog:references suggest you're using PNPM workspace catalog features for centralized dependency management, which is a good practice.packages/zxing-wasm/package.json (1)
1-222: All build paths are correctly configured for the monorepo structure.The verification confirms:
src/cppdirectory exists at the expected location (packages/zxing-wasm/src/cpp)- Build output directories (
build,dist) are properly.gitignored- Relative paths in build scripts correctly resolve from the package root
No path corrections needed.
packages/zxing-wasm/src/share.ts (1)
123-130: Review comment is valid but no action required—version constraint already satisfied.The code change correctly uses
import.meta.resolve()only in the non-PROD branch (line 126). The repository already specifies Node.js version constraint via.node-versionfile set tolts/*, which resolves to v20+ or later—exceeding the v20.8.0 minimum you mentioned. The CI/CD setup action explicitly enforces this version vianode-version-file: .node-version, ensuring all development and test environments are compatible.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
packages/react/src/types.ts (2)
3-6: Well-designed interface with commented code to address.The
RequestData<T>interface correctly usesParameters<T>to extract function parameter types, ensuring type-safe request construction. However, the commentedid: ArrayBufferproperty (line 4) should be either implemented or removed to keep the codebase clean.If the
idproperty is planned for future use, consider adding a TODO comment explaining its purpose. Otherwise, remove the commented line:export interface RequestData<T extends (...args: never[]) => unknown> { - // id: ArrayBuffer; parameters: Parameters<T>; }
8-15: Excellent type safety with commented code to address.The
ResponseData<T, E>type correctly usesRequireExactlyOneto enforce that responses contain either a successfulreturnvalue or anerror, preventing ambiguous states. This is a robust pattern for worker communication.However, the commented
id: ArrayBufferproperty (line 10) should be addressed consistently with theRequestDatainterface.Apply the same fix as suggested for
RequestData:export type ResponseData<T = unknown, E = unknown> = RequireExactlyOne< { - // id: ArrayBuffer; return: T; error: E; }, "return" | "error" >;packages/react/src/workers/reader.ts (1)
15-18: Consider explicitly typing the caught error.The error caught on line 15 is implicitly
any. For better type safety and to avoid potential issues with error serialization across the worker boundary, consider explicitly typing it:} catch (error) { self.postMessage({ - error, + error: error instanceof Error ? error.message : String(error), } satisfies ReadBarcodesResponseData); }Note: Not all error types are serializable via
postMessage(e.g., DOMException in some contexts). Ensure the error can be successfully posted, or extract serializable properties likemessage,name, andstack.packages/react/src/utils/createCanvas.ts (1)
9-24: Refactor control flow to avoid throwing undefined.The current implementation uses
throw undefinedon line 18 purely for control flow, which is not idiomatic. Consider restructuring for clarity:export function createCanvas( width: number, height: number, ): OffscreenCanvas | HTMLCanvasElement { + // Validate dimensions + if (!Number.isFinite(width) || !Number.isFinite(height) || width <= 0 || height <= 0) { + throw new Error('Canvas dimensions must be positive finite numbers'); + } + try { const canvas = new OffscreenCanvas(width, height); - if ( - canvas.getContext("2d", { - willReadFrequently: true, - }) instanceof OffscreenCanvasRenderingContext2D - ) { + const ctx = canvas.getContext("2d", { willReadFrequently: true }); + if (ctx instanceof OffscreenCanvasRenderingContext2D) { return canvas; } - throw undefined; } catch { - const canvas = document.createElement("canvas"); - canvas.width = width; - canvas.height = height; - return canvas; + // Fallback to HTMLCanvasElement } + + const canvas = document.createElement("canvas"); + canvas.width = width; + canvas.height = height; + return canvas; }This approach:
- Validates input dimensions early
- Removes the
throw undefinedcontrol flow- Makes the fallback path explicit
- Improves readability
packages/react/src/index.tsx (1)
1-1: LGTM! Clean public API surface.The index file correctly re-exports the scanner hook and its options type, establishing a clear public API. This is a standard and clean pattern for package entry points.
If the utility functions (
createCanvas,isHTMLCanvasElement, etc.) are intended for public consumption, consider also exporting them from this index. Otherwise, keeping them internal is fine for a minimal API surface.packages/react/src/stories/useScanner.stories.tsx (1)
58-167: Consider extracting shared demo logic to reduce duplication.All three demo components (
ImageElementDemo,VideoElementDemo,CanvasElementDemo) share identical state management and error handling patterns. Consider extracting this into a shared wrapper component or custom hook to improve maintainability.Example approach:
const useScannerDemo = () => { const [results, setResults] = useState<ReadResult[]>([]); const [error, setError] = useState<string | null>(null); const scannerOptions: UseScannerOptions = { onScanResponse: (readResults) => { setResults(readResults); setError(null); }, onScanError: (err) => { const errorMessage = err instanceof Error ? err.message : String(err); setError(errorMessage); console.error("Scan error:", err); }, }; return { results, error, scannerOptions }; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/react/public/barcodes.movis excluded by!**/*.movpackages/react/public/wikipedia.pngis excluded by!**/*.png
📒 Files selected for processing (27)
.gitignore(1 hunks).lintstagedrc.json(1 hunks).prettierignore(0 hunks).prettierrc(1 hunks).vscode/project.code-snippets(1 hunks).vscode/settings.json(1 hunks)biome.json(1 hunks)package.json(2 hunks)packages/react/.gitignore(1 hunks)packages/react/.storybook/main.ts(1 hunks)packages/react/.storybook/preview.ts(1 hunks)packages/react/README.md(1 hunks)packages/react/package.json(1 hunks)packages/react/src/hooks/useScanner.ts(1 hunks)packages/react/src/index.css(1 hunks)packages/react/src/index.tsx(1 hunks)packages/react/src/stories/useScanner.stories.tsx(1 hunks)packages/react/src/types.ts(1 hunks)packages/react/src/utils/createCanvas.ts(1 hunks)packages/react/src/utils/isHTMLCanvasElement.ts(1 hunks)packages/react/src/utils/isHTMLImageElement.ts(1 hunks)packages/react/src/utils/isHTMLVideoElement.ts(1 hunks)packages/react/src/workers/reader.ts(1 hunks)packages/react/tsconfig.app.json(1 hunks)packages/react/tsconfig.json(1 hunks)packages/react/tsconfig.node.json(1 hunks)packages/react/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .prettierignore
✅ Files skipped from review due to trivial changes (4)
- packages/react/README.md
- packages/react/.gitignore
- .vscode/project.code-snippets
- packages/react/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
packages/react/src/stories/useScanner.stories.tsx (1)
packages/react/src/hooks/useScanner.ts (1)
useScanner(24-259)
packages/react/src/hooks/useScanner.ts (5)
packages/react/src/types.ts (2)
RequestData(3-6)ResponseData(8-15)packages/react/src/utils/isHTMLImageElement.ts (1)
isHTMLImageElement(7-16)packages/react/src/utils/createCanvas.ts (1)
createCanvas(5-25)packages/react/src/utils/isHTMLVideoElement.ts (1)
isHTMLVideoElement(7-16)packages/react/src/utils/isHTMLCanvasElement.ts (1)
isHTMLCanvasElement(7-16)
packages/react/src/workers/reader.ts (1)
packages/react/src/types.ts (2)
ResponseData(8-15)RequestData(3-6)
🔇 Additional comments (19)
packages/react/src/index.css (1)
1-1: LGTM! Correct Tailwind CSS v4 import syntax.The
@import "tailwindcss";directive is the correct import method for Tailwind CSS v4, replacing the older@tailwind base; @tailwind components; @tailwind utilities;directives..lintstagedrc.json (1)
2-5: LGTM! Consistent tooling separation.Moving JSX/TSX files from Biome processing to Prettier aligns with the corresponding exclusion patterns added to
biome.json. This creates a clear separation where Biome handles non-JSX/TSX files and Prettier handles JSX/TSX along with markup files..prettierrc (1)
4-4: LGTM! Appropriate plugin additions.The added Prettier plugins enhance the project's formatting capabilities:
prettier-plugin-jsdocensures consistent JSDoc formattingprettier-plugin-tailwindcssautomatically sorts Tailwind CSS classes, which is particularly useful with the new Tailwind v4 integration.gitignore (1)
27-28: LGTM! Appropriate pattern broadening for monorepo.The addition of
**/prefix correctly extends the ignore patterns to match generated files in any nestedsrc/directory, which is essential for the new monorepo structure where packages likepackages/zxing-wasm/src/will contain their own generated files.biome.json (1)
7-8: LGTM! Consistent with Prettier delegation.Excluding TSX and JSX files from Biome processing aligns with the
.lintstagedrc.jsonchanges that delegate these file types to Prettier. This establishes a clear tooling boundary for the newly introduced React package.packages/react/.storybook/preview.ts (1)
1-14: LGTM! Standard Storybook preview configuration.This is a conventional Storybook preview setup with appropriate control matchers for color and date properties. The configuration follows Storybook best practices for the React-Vite integration.
.vscode/settings.json (1)
2-2: LGTM! Appropriate simplification.Reducing the VSCode settings to just the CMake configuration is a good cleanup. The removed formatting settings are now properly handled by the centralized Prettier and Biome configurations in the monorepo, which provides better consistency across different editors.
packages/react/.storybook/main.ts (1)
1-14: LGTM! Basic Storybook configuration is correct.The configuration correctly sets up Storybook with React-Vite framework and disables telemetry. The story pattern will properly discover stories in the src directory.
package.json (2)
2-5: LGTM! Monorepo root configuration is correct.The conversion to a private monorepo root with
private: trueand version0.0.0follows standard monorepo practices. The updated name and description clearly indicate this is now a monorepo root.
34-43: PNPM catalog definitions verified and properly configured.All devDependencies in package.json reference valid entries in the
catalogsection ofpnpm-workspace.yaml. Each package has a corresponding version specification:
- @biomejs/biome: 2.3.4
- @changesets/cli: ^2.29.7
- lint-staged: ^16.2.6
- prettier: ^3.6.2
- prettier-plugin-jsdoc: ^1.5.0
- prettier-plugin-tailwindcss: ^0.7.1
- pretty-quick: ^4.2.2
- simple-git-hooks: ^2.13.1
No issues found.
packages/react/tsconfig.json (1)
1-7: LGTM! Project references structure is correct.This configuration follows the TypeScript project references pattern, delegating compilation to
tsconfig.app.json(application code) andtsconfig.node.json(build tooling). The emptyfilesarray is intentional, as this file serves only as a reference container.packages/react/tsconfig.node.json (1)
21-23: No action required — TypeScript version fully supports both compiler options.TypeScript 5.8 introduced
erasableSyntaxOnly, andnoUncheckedSideEffectImportswas introduced in TypeScript 5.6. The project's TypeScript version is 5.9.3, which includes both flags. No version compatibility issues exist.packages/react/src/utils/isHTMLCanvasElement.ts (1)
7-16: The review comment contains factual inaccuracies and mischaracterizes an intentional codebase pattern.
Non-existent code reference: The review states "The
throw undefinedon line 18 is used solely for control flow," but the actual code ends at line 16 with no throw statement. The try/catch block naturally handles errors.Established pattern: All three type guard functions (isHTMLCanvasElement, isHTMLImageElement, isHTMLVideoElement) use the identical
(el as Node)?.ownerDocument?.defaultView?.HTMLElementType as neverpattern consistently. This is not ad-hoc complexity but a deliberate cross-realm checking approach.Pattern correctness: The implementation correctly handles multiple realms (e.g., iframes, workers) where HTMLCanvasElement constructors differ. The optional chaining safely handles non-Node values by returning undefined, which then throws on instanceof and is caught.
The suggested refactoring alternatives would remove cross-realm support without justification.
Likely an incorrect or invalid review comment.
packages/react/src/workers/reader.ts (1)
1-1: Import pathzxing-wasm/readercorrectly resolves.The
zxing-wasmpackage properly exports the./readersubpath in itspackage.json, mapping to./dist/es/reader/index.jsand./dist/cjs/reader/index.js. The source file atpackages/zxing-wasm/src/reader/index.tsexports thereadBarcodesfunction, which is compiled into the dist output. Named package imports use the package's export field for resolution and work the same in worker files as in regular modules. No issues found.packages/react/src/utils/isHTMLVideoElement.ts (1)
7-16: LGTM! Cross-realm-safe type guard.The implementation correctly uses
ownerDocument.defaultViewto access the element's constructor from its own realm, making it safe for cross-iframe/cross-window scenarios. The try-catch properly handles edge cases whereownerDocumentordefaultViewmight be unavailable.packages/react/vite.config.ts (1)
10-18: Verify the necessity and impact of mutingcheckDeps.The replace plugin silences the
checkDepsfunction inuse-custom-compare.js, which could hide legitimate dependency tracking warnings during development. While this may be intentional to suppress noisy warnings, it could also mask real bugs in dependency arrays.Please confirm:
- Why is
checkDepsbeing muted? Is this to suppress development-only warnings?- Have you verified that this doesn't hide legitimate dependency tracking issues in your custom comparison hooks?
- Consider documenting the reason for this suppression in the comment on Line 9.
packages/react/src/utils/isHTMLImageElement.ts (1)
7-16: LGTM! Consistent with the video element guard.The implementation follows the same cross-realm-safe pattern as
isHTMLVideoElement, ensuring consistent and reliable type checking across different element types.packages/react/src/stories/useScanner.stories.tsx (1)
162-164: Canvas demo is marked as pending implementation.The canvas story includes a note that it's pending implementation. Ensure this is tracked and completed before release, or consider removing this story until it's fully functional.
packages/react/tsconfig.app.json (1)
23-25: TypeScript version compatibility verified.The project uses TypeScript 5.9.3, which fully supports both
erasableSyntaxOnlyandnoUncheckedSideEffectImportscompiler options (both available since TypeScript 5.5).
| const getImageElementFrameRequestCallback = useCallback< | ||
| ( | ||
| imageEl: HTMLImageElement, | ||
| byoc: HTMLCanvasElement | OffscreenCanvas, | ||
| ) => FrameRequestCallback | ||
| >( | ||
| (imageEl, byoc) => (_timestamp) => { | ||
| byoc.width = imageEl.naturalWidth; | ||
| byoc.height = imageEl.naturalHeight; | ||
|
|
||
| if (byoc.width === 0 || byoc.height === 0) { | ||
| // Image not loaded yet, try again on the next frame | ||
| requestAnimationFrame( | ||
| getImageElementFrameRequestCallback(imageEl, byoc), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const ctx = byoc.getContext("2d", { | ||
| willReadFrequently: true, | ||
| }); | ||
|
|
||
| if (!ctx) { | ||
| onScanError?.( | ||
| new DOMException( | ||
| "Failed to get the context from the canvas element.", | ||
| "InvalidStateError", | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| ctx.drawImage(imageEl, 0, 0); | ||
|
|
||
| try { | ||
| const imageData = ctx.getImageData(0, 0, byoc.width, byoc.height); | ||
|
|
||
| readerWorkerRef.current.postMessage( | ||
| { | ||
| // id: idRef.current, | ||
| parameters: [imageData, readerOptionsRef.current], | ||
| } satisfies ReadBarcodesRequestData, | ||
| { | ||
| transfer: [imageData.data.buffer], | ||
| }, | ||
| ); | ||
|
|
||
| requestAnimationFrame( | ||
| getImageElementFrameRequestCallback(imageEl, byoc), | ||
| ); | ||
| } catch (e) { | ||
| onScanError?.(e); | ||
| return; | ||
| } | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Critical: Stale closure and infinite loop issues.
The getImageElementFrameRequestCallback has two critical problems:
- Stale closure: The empty dependency array (Line 103) means
onScanErrorwill be stale if it changes - Infinite loop: The frame callback recursively schedules itself forever with no stop condition (Lines 60-62, 95-97), continuing even after the image is detached
The callback should:
- Include
onScanErrorin its dependencies to avoid stale closures - Stop scheduling frames when the element is detached or component unmounts
Consider this approach:
+ const abortControllerRef = useRef<AbortController | null>(null);
+
const getImageElementFrameRequestCallback = useCallback<
(
imageEl: HTMLImageElement,
byoc: HTMLCanvasElement | OffscreenCanvas,
) => FrameRequestCallback
>(
- (imageEl, byoc) => (_timestamp) => {
+ (imageEl, byoc, signal: AbortSignal) => (_timestamp) => {
+ if (signal.aborted) {
+ return;
+ }
// ... rest of the callback
requestAnimationFrame(
- getImageElementFrameRequestCallback(imageEl, byoc),
+ getImageElementFrameRequestCallback(imageEl, byoc, signal),
);
},
- [],
+ [onScanError],
);Then abort when the element changes:
const ref = useCallback<
RefCallback<HTMLImageElement | HTMLVideoElement | HTMLCanvasElement>
>((el) => {
cancelAnimationFrame(frameRequestCallbackIdRef.current);
+ abortControllerRef.current?.abort();
+ abortControllerRef.current = null;
if (el === null) {
return;
}
+ abortControllerRef.current = new AbortController();
if (isHTMLImageElement(el)) {
const { naturalWidth, naturalHeight } = el;
const byoc = createCanvas(naturalWidth, naturalHeight);
frameRequestCallbackIdRef.current = requestAnimationFrame(
- getImageElementFrameRequestCallback(el, byoc),
+ getImageElementFrameRequestCallback(el, byoc, abortControllerRef.current.signal),
);
}
// ...
}, [getImageElementFrameRequestCallback, ...]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react/src/hooks/useScanner.ts around lines 48 to 104, the
getImageElementFrameRequestCallback uses an empty dependency array causing
onScanError to become stale and recursively schedules requestAnimationFrame
forever even after the image is removed; fix by adding onScanError (and any
other external refs used) to the useCallback dependency list, switch from a
self-recursive closure to a loop driven by a tracked RAF id (store the id in a
ref) so you can cancel it, and before each next request check that the image
element is still connected (imageEl.isConnected) and that the component is
mounted (use an isMounted/ref set on mount/unmount) and if not, cancel the RAF
and stop scheduling further frames; also ensure any errors call the current
onScanError and that you clear the RAF on component unmount or when the image
element changes.
| const getVideoElementFrameRequestCallback = useCallback< | ||
| ( | ||
| videoEl: HTMLVideoElement, | ||
| byoc: HTMLCanvasElement | OffscreenCanvas, | ||
| ) => FrameRequestCallback | ||
| >( | ||
| (videoEl, byoc) => (_timestamp) => { | ||
| byoc.width = videoEl.videoWidth; | ||
| byoc.height = videoEl.videoHeight; | ||
|
|
||
| if (byoc.width === 0 || byoc.height === 0) { | ||
| // Video not ready yet, try again on the next frame | ||
| requestAnimationFrame( | ||
| getVideoElementFrameRequestCallback(videoEl, byoc), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const ctx = byoc.getContext("2d", { | ||
| willReadFrequently: true, | ||
| }); | ||
|
|
||
| if (!ctx) { | ||
| onScanError?.( | ||
| new DOMException( | ||
| "Failed to get the context from the canvas element.", | ||
| "InvalidStateError", | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| ctx.drawImage(videoEl, 0, 0); | ||
|
|
||
| try { | ||
| const imageData = ctx.getImageData(0, 0, byoc.width, byoc.height); | ||
|
|
||
| readerWorkerRef.current.postMessage( | ||
| { | ||
| // id: idRef.current, | ||
| parameters: [imageData, readerOptionsRef.current], | ||
| } satisfies ReadBarcodesRequestData, | ||
| { | ||
| transfer: [imageData.data.buffer], | ||
| }, | ||
| ); | ||
|
|
||
| requestAnimationFrame( | ||
| getVideoElementFrameRequestCallback(videoEl, byoc), | ||
| ); | ||
| } catch (e) { | ||
| onScanError?.(e); | ||
| return; | ||
| } | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Critical: Same stale closure and infinite loop issues as image callback.
This callback has identical problems to getImageElementFrameRequestCallback:
- Empty dependency array causes stale
onScanErrorclosure - Infinite requestAnimationFrame loop with no stop condition
Apply the same fix pattern as suggested for the image element callback (see comment on Lines 48-104).
🤖 Prompt for AI Agents
In packages/react/src/hooks/useScanner.ts around lines 106 to 162, the video
frame callback uses an empty dependency array causing a stale onScanError
closure and it recursively creates new requestAnimationFrame callbacks each
frame (infinite loop). Fix it by mirroring the image-callback pattern: capture
latest values via refs (e.g., onScanErrorRef, readerWorkerRef, readerOptionsRef,
and a scanning/isActive ref) or include the necessary deps in the useCallback;
make the frame callback a stable function reference (store it in a ref or define
once) instead of calling getVideoElementFrameRequestCallback(...) inside
requestAnimationFrame so you don’t recreate closures each frame, and add a clear
stop condition (check an isActive/scanning ref or videoEl.paused/ended) before
scheduling the next frame; also ensure you postMessage and transfer the
ImageData.buffer using the current readerWorkerRef and readerOptionsRef and call
onScanErrorRef.current on errors.
| const getCanvasElementFrameRequestCallback = useCallback< | ||
| (canvasEl: HTMLCanvasElement) => FrameRequestCallback | ||
| >( | ||
| (canvasEl) => (_timestamp) => { | ||
| const ctx = canvasEl.getContext("2d", { | ||
| willReadFrequently: true, | ||
| }); | ||
|
|
||
| if (!ctx) { | ||
| onScanError?.( | ||
| new DOMException( | ||
| "Failed to get the context from the canvas element.", | ||
| "InvalidStateError", | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const imageData = ctx.getImageData( | ||
| 0, | ||
| 0, | ||
| canvasEl.width, | ||
| canvasEl.height, | ||
| ); | ||
|
|
||
| readerWorkerRef.current.postMessage( | ||
| { | ||
| // id: idRef.current, | ||
| parameters: [imageData, readerOptionsRef.current], | ||
| } satisfies ReadBarcodesRequestData, | ||
| { | ||
| transfer: [imageData.data.buffer], | ||
| }, | ||
| ); | ||
|
|
||
| requestAnimationFrame(getCanvasElementFrameRequestCallback(canvasEl)); | ||
| } catch (e) { | ||
| onScanError?.(e); | ||
| } | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Critical: Same stale closure and infinite loop issues.
This callback also suffers from:
- Stale
onScanErrorclosure due to empty dependency array (Line 209) - Infinite requestAnimationFrame loop (Line 204) with no stop condition
Apply the same fix pattern as suggested for the image element callback (see comment on Lines 48-104).
🤖 Prompt for AI Agents
In packages/react/src/hooks/useScanner.ts around lines 168 to 210, the canvas
frame callback uses an empty dependency array causing a stale onScanError
closure and it unconditionally schedules requestAnimationFrame causing an
infinite loop; update the useCallback to reference the latest values via refs or
include needed deps (at minimum onScanError, readerWorkerRef, readerOptionsRef,
and any isActive/isStreaming ref) so the handler uses fresh closures, and
replace the unconditional requestAnimationFrame call with a guarded scheduling
(e.g., only request next frame when an isActive/isStreaming ref is true and
store the animationFrame id in a ref so it can be cancelled) so the loop can be
stopped cleanly on unmount/stop.
| const ref = useCallback< | ||
| RefCallback<HTMLImageElement | HTMLVideoElement | HTMLCanvasElement> | ||
| >((el) => { | ||
| cancelAnimationFrame(frameRequestCallbackIdRef.current); | ||
| if (el === null) { | ||
| return; | ||
| } | ||
| if (isHTMLImageElement(el)) { | ||
| const { naturalWidth, naturalHeight } = el; | ||
| const byoc = createCanvas(naturalWidth, naturalHeight); | ||
| frameRequestCallbackIdRef.current = requestAnimationFrame( | ||
| getImageElementFrameRequestCallback(el, byoc), | ||
| ); | ||
| } else if (isHTMLVideoElement(el)) { | ||
| const { videoWidth, videoHeight } = el; | ||
| const byoc = createCanvas(videoWidth, videoHeight); | ||
| frameRequestCallbackIdRef.current = requestAnimationFrame( | ||
| getVideoElementFrameRequestCallback(el, byoc), | ||
| ); | ||
| } else if (isHTMLCanvasElement(el)) { | ||
| frameRequestCallbackIdRef.current = requestAnimationFrame( | ||
| getCanvasElementFrameRequestCallback(el), | ||
| ); | ||
| } else { | ||
| return; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Stale closures in ref callback.
The empty dependency array (Line 240) means the callback will reference stale versions of the frame request callback generators. While this may work due to how refs are used, it's fragile and could break if the implementation changes.
Include all frame callback generators in the dependency array:
const ref = useCallback<
RefCallback<HTMLImageElement | HTMLVideoElement | HTMLCanvasElement>
>((el) => {
// ... callback body
- }, []);
+ }, [
+ getImageElementFrameRequestCallback,
+ getVideoElementFrameRequestCallback,
+ getCanvasElementFrameRequestCallback,
+ ]);🤖 Prompt for AI Agents
In packages/react/src/hooks/useScanner.ts around lines 214 to 240, the ref
callback is created with an empty dependency array causing it to capture stale
references to the frame callback generators; update the useCallback dependency
array to include getImageElementFrameRequestCallback,
getVideoElementFrameRequestCallback, getCanvasElementFrameRequestCallback and
createCanvas (and any other external helpers used inside the callback), or
alternatively wrap those helpers in stable refs/useCallback so the ref callback
can safely list them as dependencies.
| const ref = useCallback< | ||
| RefCallback<HTMLImageElement | HTMLVideoElement | HTMLCanvasElement> | ||
| >((el) => { | ||
| cancelAnimationFrame(frameRequestCallbackIdRef.current); |
There was a problem hiding this comment.
Missing cleanup for animation frame on unmount.
While Line 217 cancels the animation frame when the element changes, there's no cleanup when the component unmounts. This could leave a pending animation frame that tries to access unmounted component state.
Add cleanup in the main effect or a separate useEffect:
+ useEffect(() => {
+ return () => {
+ cancelAnimationFrame(frameRequestCallbackIdRef.current);
+ };
+ }, []);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react/src/hooks/useScanner.ts around line 217, the animation frame
is cancelled when the element changes but there is no cleanup on component
unmount; add a cleanup that cancels any pending animation frame by calling
cancelAnimationFrame(frameRequestCallbackIdRef.current) if present when the
effect unmounts (either by returning a cleanup function from the main effect
that watches the element or by adding a separate useEffect with an empty
dependency array that cancels and clears the ref), and ensure you null out
frameRequestCallbackIdRef.current after cancelling to avoid stale references.
| useEffect(() => { | ||
| readerWorkerRef.current.onmessage = ({ | ||
| data, | ||
| }: MessageEvent<ReadBarcodesResponseData>) => { | ||
| // // Ignore messages that are not from the current hook | ||
| // if (data.id !== idRef.current) { | ||
| // return; | ||
| // } | ||
| if ("error" in data) { | ||
| onScanError?.(data.error); | ||
| return; | ||
| } | ||
| onScanResponse?.(data.return); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Stale closures in worker message handler.
The empty dependency array (Line 256) causes onScanError and onScanResponse to be stale. If these callbacks change, the effect won't re-run, and the handler will call outdated versions.
Include the callbacks in the dependency array:
useEffect(() => {
readerWorkerRef.current.onmessage = ({
data,
}: MessageEvent<ReadBarcodesResponseData>) => {
if ("error" in data) {
onScanError?.(data.error);
return;
}
onScanResponse?.(data.return);
};
- }, []);
+ }, [onScanError, onScanResponse]);📝 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.
| useEffect(() => { | |
| readerWorkerRef.current.onmessage = ({ | |
| data, | |
| }: MessageEvent<ReadBarcodesResponseData>) => { | |
| // // Ignore messages that are not from the current hook | |
| // if (data.id !== idRef.current) { | |
| // return; | |
| // } | |
| if ("error" in data) { | |
| onScanError?.(data.error); | |
| return; | |
| } | |
| onScanResponse?.(data.return); | |
| }; | |
| }, []); | |
| useEffect(() => { | |
| readerWorkerRef.current.onmessage = ({ | |
| data, | |
| }: MessageEvent<ReadBarcodesResponseData>) => { | |
| // // Ignore messages that are not from the current hook | |
| // if (data.id !== idRef.current) { | |
| // return; | |
| // } | |
| if ("error" in data) { | |
| onScanError?.(data.error); | |
| return; | |
| } | |
| onScanResponse?.(data.return); | |
| }; | |
| }, [onScanError, onScanResponse]); |
🤖 Prompt for AI Agents
In packages/react/src/hooks/useScanner.ts around lines 242 to 256, the effect
that assigns readerWorkerRef.current.onmessage captures stale onScanError and
onScanResponse because the dependency array is empty; update the effect so it
includes onScanError and onScanResponse in the dependency array (or
alternatively store the latest callbacks in refs and reference those inside the
handler) and ensure you return a cleanup that clears or resets
readerWorkerRef.current.onmessage to avoid duplicate handlers when the effect
re-runs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
packages/react/src/hooks/useScanner.ts (6)
54-110: Critical: Stale closure and infinite animation loop.This callback has two critical problems:
- Stale closure: The empty dependency array (Line 109) means
onScanErrorwill be stale if the parent component re-renders with a new callback- Infinite loop: The frame callback recursively schedules itself forever (Lines 66-68, 101-103) with no stop condition, continuing even after the image element is detached or the component unmounts
The infinite loop will consume resources and may cause memory leaks or errors when trying to access a detached element.
Fix approach:
- Add
onScanErrorto the dependency array or use a ref pattern to capture the latest callback- Implement a cancellation mechanism:
- Check if the element is still connected (
imageEl.isConnected) before scheduling the next frame- Use an abort signal or a mounted flag to stop the loop on unmount
- Store a stable callback in a ref instead of creating new closures each frame
Example pattern using refs:
+ const onScanErrorRef = useRef(onScanError); + + useEffect(() => { + onScanErrorRef.current = onScanError; + }, [onScanError]); + + const isMountedRef = useRef(true); + + useEffect(() => { + return () => { + isMountedRef.current = false; + }; + }, []); const getImageElementFrameRequestCallback = useCallback< ( imageEl: HTMLImageElement, byoc: HTMLCanvasElement | OffscreenCanvas, ) => FrameRequestCallback >( (imageEl, byoc) => { - return (_timestamp) => { + const callback: FrameRequestCallback = (_timestamp) => { + if (!isMountedRef.current || !imageEl.isConnected) { + return; + } byoc.width = imageEl.naturalWidth; byoc.height = imageEl.naturalHeight; if (byoc.width === 0 || byoc.height === 0) { - requestAnimationFrame( - getImageElementFrameRequestCallback(imageEl, byoc), - ); + requestAnimationFrame(callback); return; } const ctx = byoc.getContext("2d", { willReadFrequently: true }); if (!ctx) { - onScanError?.( + onScanErrorRef.current?.( new DOMException( "Failed to get the context from the canvas element.", "InvalidStateError", ), ); return; } ctx.drawImage(imageEl, 0, 0); try { const imageData = ctx.getImageData(0, 0, byoc.width, byoc.height); readerWorkerRef.current.postMessage( { parameters: [imageData, readerOptionsRef.current], } satisfies ReadBarcodesRequestData, { transfer: [imageData.data.buffer], }, ); - requestAnimationFrame( - getImageElementFrameRequestCallback(imageEl, byoc), - ); + requestAnimationFrame(callback); } catch (e) { - onScanError?.(e); + onScanErrorRef.current?.(e); return; } - }, + }; + return callback; }, [], );
112-168: Critical: Same stale closure and infinite loop issues.This callback suffers from the identical problems as
getImageElementFrameRequestCallback:
- Stale closure: Empty dependency array (Line 167) causes stale
onScanError- Infinite loop: Unconditional
requestAnimationFrame(Lines 124-126, 159-161) with no stop conditionApply the same fix pattern suggested for Lines 54-110, using refs to capture the latest
onScanErrorand adding guard conditions to stop the loop when the video element is detached or the component unmounts.
174-216: Critical: Same stale closure and infinite loop issues.This callback also has:
- Stale closure: Empty dependency array (Line 215) causes stale
onScanError- Infinite loop: Unconditional
requestAnimationFrame(Line 210) with no stop conditionApply the same fix pattern suggested for Lines 54-110. Additionally, for canvas elements you should check
canvasEl.isConnectedbefore scheduling the next frame.
220-246: Stale closures in ref callback.The empty dependency array (Line 246) means this callback captures stale versions of the frame request callback generators. If those callbacks are updated (e.g., if their dependencies change), this ref callback will still use the old versions.
Include all frame callback generators in the dependency array:
const ref = useCallback< RefCallback<HTMLImageElement | HTMLVideoElement | HTMLCanvasElement> >((el) => { cancelAnimationFrame(frameRequestCallbackIdRef.current); if (el === null) { return; } if (isHTMLImageElement(el)) { const { naturalWidth, naturalHeight } = el; const byoc = createCanvas(naturalWidth, naturalHeight); frameRequestCallbackIdRef.current = requestAnimationFrame( getImageElementFrameRequestCallback(el, byoc), ); } else if (isHTMLVideoElement(el)) { const { videoWidth, videoHeight } = el; const byoc = createCanvas(videoWidth, videoHeight); frameRequestCallbackIdRef.current = requestAnimationFrame( getVideoElementFrameRequestCallback(el, byoc), ); } else if (isHTMLCanvasElement(el)) { frameRequestCallbackIdRef.current = requestAnimationFrame( getCanvasElementFrameRequestCallback(el), ); } else { return; } - }, []); + }, [ + getImageElementFrameRequestCallback, + getVideoElementFrameRequestCallback, + getCanvasElementFrameRequestCallback, + createCanvas, + ]);
218-223: Missing cleanup for animation frame on unmount.While Line 223 cancels the animation frame when the element changes, there's no cleanup when the component unmounts. A pending animation frame could attempt to access unmounted component state, causing errors or memory leaks.
Add cleanup on unmount:
+ useEffect(() => { + return () => { + cancelAnimationFrame(frameRequestCallbackIdRef.current); + frameRequestCallbackIdRef.current = 0; + }; + }, []);
248-262: Critical: Stale closures in worker message handler.The empty dependency array (Line 262) causes
onScanErrorandonScanResponseto become stale. If these callbacks change, the effect won't re-run, and the handler will invoke outdated versions.Include the callbacks in the dependency array:
useEffect(() => { readerWorkerRef.current.onmessage = ({ data, }: MessageEvent<ReadBarcodesResponseData>) => { if ("error" in data) { onScanError?.(data.error); return; } onScanResponse?.(data.return); }; - }, []); + }, [onScanError, onScanResponse]);Alternatively, use refs to capture the latest callbacks:
+ const onScanErrorRef = useRef(onScanError); + const onScanResponseRef = useRef(onScanResponse); + + useEffect(() => { + onScanErrorRef.current = onScanError; + }, [onScanError]); + + useEffect(() => { + onScanResponseRef.current = onScanResponse; + }, [onScanResponse]); useEffect(() => { readerWorkerRef.current.onmessage = ({ data, }: MessageEvent<ReadBarcodesResponseData>) => { if ("error" in data) { - onScanError?.(data.error); + onScanErrorRef.current?.(data.error); return; } - onScanResponse?.(data.return); + onScanResponseRef.current?.(data.return); }; }, []);packages/react/src/stories/useScanner.stories.tsx (1)
131-131: Replace Chinese text with English.The comment includes Chinese text "(待定)" meaning "pending" or "TBD". For codebase consistency and maintainability, use English throughout.
Apply this fix:
-// HTMLCanvasElement Story (待定) +// HTMLCanvasElement Story (pending implementation) const CanvasElementDemo = () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react/src/hooks/useScanner.ts(1 hunks)packages/react/src/stories/useScanner.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react/src/stories/useScanner.stories.tsx (1)
packages/react/src/hooks/useScanner.ts (1)
useScanner(24-265)
packages/react/src/hooks/useScanner.ts (5)
packages/react/src/types.ts (2)
RequestData(3-6)ResponseData(8-15)packages/react/src/utils/isHTMLImageElement.ts (1)
isHTMLImageElement(7-16)packages/react/src/utils/createCanvas.ts (1)
createCanvas(5-25)packages/react/src/utils/isHTMLVideoElement.ts (1)
isHTMLVideoElement(7-16)packages/react/src/utils/isHTMLCanvasElement.ts (1)
isHTMLCanvasElement(7-16)
🔇 Additional comments (2)
packages/react/src/hooks/useScanner.ts (1)
31-35: LGTM: Worker cleanup properly implemented.The worker termination on unmount correctly addresses the resource leak identified in previous reviews.
packages/react/src/stories/useScanner.stories.tsx (1)
59-189: LGTM: Demo components properly structured.The three demo components correctly wire up the
useScannerhook with appropriate state management and error handling. The pattern is consistent across all element types and suitable for Storybook demonstration.
| const ScanResults = ({ | ||
| const ScanResults = ({ |
There was a problem hiding this comment.
Critical: Duplicate declaration causes syntax error.
Line 8 duplicates the const ScanResults = ({ declaration from Line 7. This is a syntax error that will prevent the code from compiling.
Remove the duplicate line:
// Shared results display component
const ScanResults = ({
-const ScanResults = ({
results,
error,
}: {📝 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.
| const ScanResults = ({ | |
| const ScanResults = ({ | |
| // Shared results display component | |
| const ScanResults = ({ | |
| results, | |
| error, | |
| }: { |
🤖 Prompt for AI Agents
In packages/react/src/stories/useScanner.stories.tsx around lines 7 to 8, there
is a duplicate declaration of "const ScanResults = ({" on line 8 which causes a
syntax error; remove the duplicate line (keep a single "const ScanResults = ({")
and ensure the remaining declaration and its following block/closing brace
remain correctly paired so the file compiles.
Sec-ant
force-pushed
the
chore/monorepo
branch
from
797e012 to
6acde4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
packages/react/src/stories/useScanner.stories.tsx (1)
7-8: Critical: Remove duplicate declaration.Line 8 duplicates the declaration from Line 7, causing a syntax error. This was flagged in a previous review and must be fixed.
// Shared results display component const ScanResults = ({ -const ScanResults = ({ results, error, }: {packages/react/src/hooks/useScanner.ts (6)
54-110: Critical: Stale closure and infinite loop remain unresolved.The issues flagged in the previous review are still present:
- Empty dependency array (line 109) causes
onScanErrorto become stale if it changes- Recursive
requestAnimationFramecalls (lines 66-68, 101-103) create an infinite loop with no stop condition- The loop continues even after the image element is detached
112-168: Critical: Same stale closure and infinite loop issues.Identical problems to the image callback remain unresolved:
- Empty dependency array (line 167) causes stale
onScanErrorclosure- Infinite
requestAnimationFrameloop (lines 124-126, 159-161) with no stop condition
174-216: Critical: Same stale closure and infinite loop issues.The canvas callback has the same unresolved issues:
- Empty dependency array (line 215) causes stale
onScanErrorclosure- Infinite
requestAnimationFrameloop (line 210) with no stop condition
220-246: Major: Stale closures in ref callback remain unresolved.The empty dependency array (line 246) means the callback references stale versions of the frame request callback generators. While this may work currently due to how refs are used, it's fragile and will break when the frame callback dependencies are corrected.
223-223: Major: Missing cleanup for animation frame on unmount.While the animation frame is cancelled when the element changes (line 223), there's still no cleanup when the component unmounts. This could leave a pending animation frame that tries to access unmounted component state.
248-262: Critical: Stale closures in worker message handler remain unresolved.The empty dependency array (line 262) causes
onScanErrorandonScanResponseto be stale. If these callbacks change, the effect won't re-run, and the handler will call outdated versions.
🧹 Nitpick comments (4)
packages/zxing-wasm/README.md (3)
205-205: Minor: Hyphenate compound adjective.Grammar correction suggested by static analysis: "custom defined" should be "custom-defined" when used as a compound adjective.
-If you want to change the serve path to your own server or other CDNs, please use [`prepareZXingModule`](https://zxing-wasm.deno.dev/functions/full.prepareZXingModule.html) and pass an [`overrides`](http://localhost:4173/interfaces/full.PrepareZXingModuleOptions.html#overrides) object with a custom defined [`locateFile`](https://emscripten.org/docs/api_reference/module.html?highlight=locatefile#Module.locateFile) function before reading or writing barcodes. +If you want to change the serve path to your own server or other CDNs, please use [`prepareZXingModule`](https://zxing-wasm.deno.dev/functions/full.prepareZXingModule.html) and pass an [`overrides`](http://localhost:4173/interfaces/full.PrepareZXingModuleOptions.html#overrides) object with a custom-defined [`locateFile`](https://emscripten.org/docs/api_reference/module.html?highlight=locatefile#Module.locateFile) function before reading or writing barcodes.
340-340: Minor: Correct capitalization of WeChat.The official brand name uses a capital "C": "WeChat" (not "wechat").
-> [!NOTE] -> To use this library in a WeChat mini program <img alt="wechat mini program icon" src="https://github.com/user-attachments/assets/7d8f3337-dd9c-43ec-aab4-8d4e72d32867" width="16" height="16">, there are several things to keep in mind: +> [!NOTE] +> To use this library in a WeChat mini program <img alt="WeChat mini program icon" src="https://github.com/user-attachments/assets/7d8f3337-dd9c-43ec-aab4-8d4e72d32867" width="16" height="16">, there are several things to keep in mind:
456-456: Minor: Hyphenate compound adjective.Grammar correction: "higher level" should be "higher-level" when used as a compound adjective before a noun.
-3. **Are there any higher level libraries that can be used to simplify the usage of this library?** +3. **Are there any higher-level libraries that can be used to simplify the usage of this library?**packages/react/src/stories/useScanner.stories.tsx (1)
131-131: Translate Chinese comment to English.For codebase consistency, replace the Chinese comment with its English equivalent.
-// HTMLCanvasElement Story (待定) +// HTMLCanvasElement Story (pending) const CanvasElementDemo = () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/react/public/barcodes.movis excluded by!**/*.movpackages/react/public/wikipedia.pngis excluded by!**/*.png
📒 Files selected for processing (32)
.gitignore(1 hunks).gitmodules(1 hunks).lintstagedrc.json(1 hunks).prettierignore(0 hunks).prettierrc(1 hunks).vscode/project.code-snippets(1 hunks).vscode/settings.json(1 hunks)README.md(1 hunks)biome.json(1 hunks)package.json(2 hunks)packages/react/.gitignore(1 hunks)packages/react/.storybook/main.ts(1 hunks)packages/react/.storybook/preview.ts(1 hunks)packages/react/README.md(1 hunks)packages/react/package.json(1 hunks)packages/react/src/hooks/useScanner.ts(1 hunks)packages/react/src/index.css(1 hunks)packages/react/src/index.tsx(1 hunks)packages/react/src/stories/useScanner.stories.tsx(1 hunks)packages/react/src/types.ts(1 hunks)packages/react/src/utils/createCanvas.ts(1 hunks)packages/react/src/utils/isHTMLCanvasElement.ts(1 hunks)packages/react/src/utils/isHTMLImageElement.ts(1 hunks)packages/react/src/utils/isHTMLVideoElement.ts(1 hunks)packages/react/src/workers/reader.ts(1 hunks)packages/react/tsconfig.app.json(1 hunks)packages/react/tsconfig.json(1 hunks)packages/react/tsconfig.node.json(1 hunks)packages/react/vite.config.ts(1 hunks)packages/zxing-wasm/README.md(1 hunks)packages/zxing-wasm/package.json(1 hunks)packages/zxing-wasm/src/share.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .prettierignore
✅ Files skipped from review due to trivial changes (3)
- packages/react/src/index.css
- .gitmodules
- packages/react/README.md
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/react/tsconfig.json
- packages/react/src/utils/isHTMLVideoElement.ts
- packages/react/src/workers/reader.ts
- packages/react/.storybook/main.ts
- .prettierrc
- packages/react/package.json
- packages/react/src/utils/isHTMLImageElement.ts
- packages/zxing-wasm/src/share.ts
- packages/react/src/index.tsx
- packages/react/vite.config.ts
- .lintstagedrc.json
- packages/react/.storybook/preview.ts
- packages/react/src/utils/createCanvas.ts
- packages/react/tsconfig.app.json
- packages/react/tsconfig.node.json
- .vscode/project.code-snippets
- packages/react/src/types.ts
🧰 Additional context used
🪛 LanguageTool
packages/zxing-wasm/README.md
[grammar] ~205-~205: Use a hyphen to join words.
Context: ...ons.html#overrides) object with a custom defined [locateFile](https://emscripte...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~340-~340: The official name of this popular chat service is spelled with a capital “C”.
Context: ...e this library in a WeChat mini program <img alt="wechat mini program icon" src="https://github....
(WECHAT)
[grammar] ~456-~456: Use a hyphen to join words.
Context: ...ursive ``` 3. **Are there any higher level libraries that can be used to simp...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (13)
.vscode/settings.json (1)
1-3: Verify that alternative editor/formatter configurations exist elsewhere.This change removes all formatter and editor settings (TypeScript, JavaScript, JSON, JSONC, C/C++, and default formatter) while retaining only the CMake-specific setting. This is a significant deviation from the previous configuration and could impact developer experience and IDE consistency, especially if developers were relying on these settings.
Please confirm:
- Where are alternative formatter/editor configurations now defined? (e.g., in per-workspace folders under
packages/, root-level.prettierrc,tsconfig.json, or another location?)- Why is the CMake setting retained while all other formatter settings are removed?
- Does this change (marked as "wip") represent a complete transition, or is it a work-in-progress step?
If formatter configurations have moved to per-package locations, consider documenting this in the PR description or a migration guide for developers.
biome.json (1)
7-8: LGTM! TSX/JSX exclusion aligns with the new React package.The exclusion of TSX and JSX files from Biome analysis is appropriate given the addition of the React package with TSX-based components and stories. This allows different formatting/linting tools to handle React-specific syntax.
.gitignore (1)
27-28: LGTM! Pattern broadening supports monorepo structure.The updated patterns correctly ignore generated ZXing WASM and JS files across all package subdirectories, which is appropriate for the monorepo restructure where multiple packages may contain
src/directories.packages/react/.gitignore (1)
1-27: LGTM! Standard .gitignore patterns for a React package.The .gitignore correctly covers all common artifacts: logs, dependencies, build outputs, editor files, and Storybook-specific artifacts. The VSCode pattern appropriately preserves
extensions.jsonfor team-shared recommendations.README.md (1)
7-9: Documentation placeholder acknowledged.The "TBD" placeholder is appropriate for a WIP refactor. Before merging to main, ensure the README includes:
- Overview of the monorepo structure
- Links to individual package READMEs
- Build and development instructions
- Migration guide from the previous structure
packages/react/src/utils/isHTMLCanvasElement.ts (1)
7-16: LGTM! Robust cross-realm type guard.The implementation correctly handles cross-realm scenarios by accessing the
HTMLCanvasElementconstructor from the element's own window context. The try-catch appropriately guards against missing properties, and the pattern aligns with similar guards in the codebase (isHTMLImageElement, isHTMLVideoElement).packages/zxing-wasm/package.json (1)
1-221: LGTM! Well-structured package configuration.The package.json is comprehensive and correctly configured:
- Dual ESM/CJS exports with proper conditions
- WASM file exports include type definitions
typesVersionsensures correct type resolution across module formats- Scripts cover the full development lifecycle
- Dependencies and peer dependencies are appropriate
The configuration supports the documented usage patterns in the README and provides a solid foundation for the zxing-wasm package.
packages/react/src/hooks/useScanner.ts (2)
31-35: LGTM: Worker cleanup implemented correctly.The worker is now properly terminated when the component unmounts, preventing memory leaks.
37-52: LGTM: Efficient ref update pattern.Using deep comparison to update the readerOptions ref is a good approach to ensure the latest options are used without causing unnecessary re-renders.
package.json (4)
2-5: LGTM: Appropriate monorepo root metadata.The changes correctly mark the root package as private with version 0.0.0, which is standard practice for monorepo roots. The name and description clearly indicate this is the monorepo root.
29-33: LGTM: Streamlined scripts for monorepo root.Keeping only git hooks and submodule management scripts in the root is appropriate. Build and test scripts will be managed by individual packages.
44-44: LGTM: Package manager pinning.Pinning the pnpm version with an integrity hash ensures consistent behavior across all development environments.
34-43: Catalog dependencies are properly configured.Verification confirms the pnpm workspace catalog in
pnpm-workspace.yamlis correctly configured with all devDependencies from package.json (lines 34-43) properly defined with version specifications. Allcatalog:references resolve correctly.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.