Feat: Add Docker setup, CMake and cross-platform build scripts - #139
Conversation
- Replace rm -rf with rimraf in clean-js script for cross-platform compatibility - Add comprehensive Windows Compatibility section to WASM setup docs - Document which scripts work natively on Windows vs require WSL - Remove HELP WANTED admonition (closes #80) - Add missing help description in docs/scripts/help.js Scripts now working natively on Windows: - dev, dev:all, preview, build-js, clean-js - lint, lint:fix, lint:style, format, format-js, format-wasm - docs, help, release WASM scripts still require WSL (Emscripten recommendation)
Replace Unix-specific Makefiles with CMake for full cross-platform Windows, macOS, and Linux support: - Add root CMakeLists.txt that auto-discovers WASM modules - Add per-module CMakeLists.txt with Emscripten configuration - Create cross-platform Node.js build script (scripts/build-wasm.js) - Update npm scripts to use new CMake-based build - Update documentation with CMake installation and usage instructions - Add CMake build artifacts to .gitignore WASM development now works natively on Windows without WSL. Addresses feedback from PR #93 review.
- Add proper error handling to clean() with force: false - Surface Emscripten check errors (not just ENOENT) - Add try/catch to mkdirSync with helpful diagnostics - Log error context (status, signal, message) in run() - Validate CLI arguments and reject unknown ones - Dynamically discover modules for clean operation - Remove legacy Makefiles (replaced by CMake) Addresses PR review feedback on error handling.
WalkthroughAdds a containerized development environment and cross-platform CLI wrappers, replaces Makefile-based WASM builds with a CMake + Emscripten workflow driven by a Node build script, removes old Makefiles, and updates docs/config to reflect the new build and developer workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (CLI)
participant Wrapper as img2num (Bash/PS)
participant Compose as Docker Compose
participant Container as Dev Container
participant NPM as npm (in container)
participant BuildJS as scripts/build-wasm.js
participant CMake as CMake / Emscripten
Dev->>Wrapper: img2num build-wasm [--debug]
Wrapper->>Compose: ensure dev service running
alt container not present
Compose->>Container: create & start service
else container stopped
Compose->>Container: start service
end
Wrapper->>Container: docker compose exec npm run build-wasm
Container->>NPM: npm run build-wasm
NPM->>BuildJS: node scripts/build-wasm.js [--debug]
BuildJS->>BuildJS: validate args, discover modules
BuildJS->>CMake: emcmake cmake (configure)
BuildJS->>CMake: cmake --build --parallel (per-module)
CMake-->>BuildJS: build artifacts (index.js, index.wasm)
BuildJS-->>NPM: exit status
NPM-->>Wrapper: command result
Wrapper-->>Dev: output/exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-12-17T22:39:25.704ZApplied to files:
📚 Learning: 2025-12-20T20:11:28.389ZApplied to files:
📚 Learning: 2025-12-17T22:39:25.704ZApplied to files:
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
img2num.bat (1)
1-10: Minor: Redundant comments on lines 2 and 4.The wrapper logic is correct—
%~dp0properly resolves the script directory, and-ExecutionPolicy Bypassensures the PowerShell script can run regardless of the user's policy settings. Lines 2 and 4 have nearly identical comments; consider removing one.Suggested cleanup
@echo off REM img2num.bat - CMD wrapper for img2num.ps1 (PowerShell script) -REM img2num.bat – Windows wrapper for img2num.ps1 - REM Get the directory of the batch file SET "SCRIPT_DIR=%~dp0"img2num (1)
64-67: Consider deriving the image name dynamically.The image name
img2num-dev:latestis hardcoded. If the image name indocker-compose.ymlchanges, this would silently fail. Consider usingdocker compose imagesor a variable for consistency.src/wasm/modules/image/CMakeLists.txt (1)
16-19: Consider explicitly listing source files instead ofGLOB_RECURSE.CMake's
file(GLOB_RECURSE ...)won't detect newly added source files until the next explicit reconfiguration. For small modules this is manageable, but it can cause build confusion when contributors add files and the build silently ignores them.If the module is expected to grow, consider listing sources explicitly or documenting the need to re-run
cmakeafter adding files.scripts/build-wasm.js (1)
100-111: Considerforce: truefor more robust cleanup.Using
force: falsemay fail on read-only files or locked files (e.g., if an editor has a file open on Windows). For build artifact cleanup,force: trueis typically preferred.🔎 Suggested change
- rmSync(dir, { recursive: true, force: false }); + rmSync(dir, { recursive: true, force: true });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.devcontainer/entrypoint.sh(1 hunks).dockerignore(1 hunks).gitignore(1 hunks)Dockerfile.dev(1 hunks)docker-compose.yml(1 hunks)docs/docs/reference/wasm/how-to-add-a-module.md(1 hunks)docs/docs/reference/wasm/setup-and-dependencies.md(1 hunks)docs/package.json(1 hunks)docs/scripts/help.js(1 hunks)img2num(1 hunks)img2num.bat(1 hunks)img2num.ps1(1 hunks)package.json(2 hunks)scripts/build-wasm.js(1 hunks)src/data/contributor-credits.json(1 hunks)src/wasm/CMakeLists.txt(1 hunks)src/wasm/Makefile(0 hunks)src/wasm/modules/image/CMakeLists.txt(1 hunks)src/wasm/modules/image/Makefile(0 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/wasm/modules/image/Makefile
- src/wasm/Makefile
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, test documentation should be properly organized in the docs/docs/ folder structure, either as a dedicated testing category or integrated into existing categories like project-scripts.
Applied to files:
img2num
📚 Learning: 2025-12-17T21:35:30.135Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.135Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
docs/docs/reference/wasm/setup-and-dependencies.mdDockerfile.dev
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/wasm/how-to-add-a-module.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (32)
src/data/contributor-credits.json (2)
23-36: The two new contributors have been verified through the repository's commit history. Doug Silkstone (dougwithseismic) has multiple commits in this repository, and Rashmi Joshi (Rashmijoshi18) has made at least one commit. The contribution count of 1 for Rashmijoshi18 and the user data (IDs, URLs) are accurate.Consider automating the maintenance of this file by creating a script that fetches contributor data directly from the GitHub API to ensure accuracy and reduce manual update errors in future contributions.
2-15: Verify updated contribution counts against GitHub API data.The contribution counts for Ryan-Millard (now 55) and dependabot[bot] (now 16) are correctly reflected in the file, but their accuracy against GitHub's actual contribution statistics should be confirmed via GitHub API or the repository's contributors graph.
docs/package.json (1)
8-8: LGTM!The
--host 0.0.0.0binding enables the dev server to be accessible from outside the container, and--poll 1000provides reliable file watching in Docker volume mounts where inotify events may not propagate. These are appropriate changes for containerized development.Also applies to: 13-13
.gitignore (1)
30-34: LGTM!The CMake build artifact patterns are correctly scoped to
src/wasm/and cover the standard files generated by CMake (cmake-build/,CMakeCache.txt,CMakeFiles/).docs/scripts/help.js (1)
9-9: LGTM!Good fix—adding the self-referential description for the
helpscript ensures it doesn't display "No description" when listing available scripts..devcontainer/entrypoint.sh (1)
1-12: LGTM! Well-structured entrypoint script.The entrypoint correctly implements strict error handling, conditionally sources the EMSDK environment, and properly replaces the shell process with the provided command using
exec "$@"..dockerignore (1)
1-8: LGTM! Appropriate Docker ignore patterns.The ignore patterns effectively exclude build artifacts, dependencies, and metadata from the Docker build context, which improves build performance and reduces image size.
vite.config.js (1)
43-45: LGTM! Correctly configured for Docker development.The server configuration properly binds to all network interfaces (
0.0.0.0) and sets the port to5173, which aligns with the Docker Compose port mapping and enables access to the Vite dev server from outside the container.src/wasm/CMakeLists.txt (3)
7-20: LGTM! Well-structured CMake setup with proper Emscripten validation.The CMake configuration appropriately requires version 3.16, enforces C++17, and includes a clear error message guiding users to build with Emscripten. This prevents common misconfiguration issues.
22-28: LGTM! Sensible build type defaults.Defaulting to Release builds is appropriate for production use, and the status messages provide helpful feedback about the build configuration.
30-38: LGTM! Elegant module auto-discovery.The automatic module discovery correctly identifies subdirectories with
CMakeLists.txtand includes them in the build. The status messages provide useful feedback during configuration.Dockerfile.dev (5)
9-20: LGTM! Appropriate build dependencies.The installed packages include all necessary tools for building WASM modules with Emscripten and handling image processing. The cleanup of apt lists reduces the final image size.
22-31: LGTM! Proper EMSDK installation.The EMSDK installation correctly clones, checks out a specific version for reproducibility, and updates the PATH. The single
RUNcommand minimizes Docker layers.
35-42: LGTM! Proper container configuration.The working directory, CHOKIDAR polling configuration for file watching, exposed ports, and default command are all correctly configured for the containerized development environment.
33-33: npm@11 is a valid and stable version—no action needed.npm 11 was released on December 16, 2024 with the goal of improving security, reliability, and usability for JavaScript package management. The current latest stable release is v11.7.0. The Dockerfile instruction
RUN npm install -g npm@11will correctly install a supported version.
4-7: EMSDK version 4.0.10 is valid and available.The specified version exists in the official emscripten-core/emsdk repository and is appropriate for use.
docker-compose.yml (2)
1-20: LGTM! Well-configured development service.The Docker Compose service is properly configured with:
- Appropriate volume mounts (project root + isolated node_modules)
- Correct port mappings for Vite and Docusaurus dev servers
- CHOKIDAR polling for reliable file watching in containers
- Interactive terminal support
22-24: LGTM! Named volumes for dependency isolation.The named volumes for
node_modulesprevent conflicts between host and container dependencies, which is a best practice for Node.js development in Docker.package.json (3)
17-18: LGTM! Cross-platform WASM build scripts.The migration from
maketo Node.js-based build scripts enables cross-platform development, particularly for Windows users who don't have Make installed by default.
20-21: LGTM! Cross-platform cleanup scripts.The use of
rimrafforclean-jsand the Node.js script forclean-wasmensure these commands work consistently across Windows, macOS, and Linux.
54-54: rimraf@6.1.2 is available and appropriate for the project.The latest version of rimraf is 6.1.2, last published a month ago. The caret version constraint (^6.1.2) allows patch and minor updates within the v6 series, which is an appropriate approach for a widely-maintained package. The package has a healthy maintenance status with no known vulnerabilities.
docs/docs/reference/wasm/how-to-add-a-module.md (3)
9-20: LGTM! Clear CMake-based module instructions.The updated instructions accurately reflect the new CMake-based workflow and provide clear guidance for adding new WASM modules.
24-94: LGTM! Comprehensive and well-documented CMake template.The CMakeLists.txt template provides excellent guidance with:
- Automatic module naming from directory
- Appropriate Emscripten flags for web environments
- Reasonable memory settings with clear comments indicating they should be adjusted per module
- Separate Debug and Release configurations
This will significantly help developers add new WASM modules.
96-121: LGTM! Clear directory structure and build instructions.The directory structure example and build commands clearly demonstrate how to use the new CMake-based workflow across all platforms.
src/wasm/modules/image/CMakeLists.txt (1)
33-48: LGTM!The Emscripten configuration is well-structured with appropriate modularization flags, ES6 export, and memory growth settings. The dynamic export name generation (
create${CAP_MODULE_NAME}Module) is a clean approach for module discovery.docs/docs/reference/wasm/setup-and-dependencies.md (2)
41-92: LGTM!The OS-specific tabs for CMake and Emscripten installation are well-organized and provide clear instructions for Windows, Linux, and macOS users. The separation of environment setup commands per platform is helpful.
132-148: Clear architecture documentation.The directory structure diagram effectively communicates the CMake-based build system layout and the auto-discovery mechanism for modules.
scripts/build-wasm.js (5)
22-31: LGTM!Clean argument validation with helpful error messages listing valid options.
41-62: LGTM!Good use of
execFileSync(no shell injection risk) with proper error handling including exit codes and signals.
67-88: LGTM!Good platform-aware Emscripten detection with appropriate handling for
ENOENTvs other errors.
169-219: LGTM!The build function is well-structured with clear phases (Emscripten check, directory creation, CMake configure, CMake build). The error messages with installation instructions are helpful for onboarding.
221-229: LGTM!The main execution logic correctly handles the three cases: clean-only, clean-then-build, and build-only.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
img2num (2)
29-29: Samecleancommand conflict as in previous review.Line 29 includes
cleanin the npm scripts pattern, but line 64 also handlesclean|destroyfor Docker maintenance. Since Bashcasematches the first pattern,cleanwill always runnpm run cleanrather thandocker compose down --volumes.Based on the usage text (line 107 lists
cleanunder "Docker Maintenance"), removecleanfrom the npm scripts pattern on line 29.Suggested fix
- dev|dev:all|dev:debug|dev:all:debug|build|build-js|build-wasm|build-wasm:debug|preview|docs|lint|lint:fix|lint:style|format|format-js|format-wasm|clean|clean-js|clean-wasm|help) + dev|dev:all|dev:debug|dev:all:debug|build|build-js|build-wasm|build-wasm:debug|preview|docs|lint|lint:fix|lint:style|format|format-js|format-wasm|clean-js|clean-wasm|help)
73-78: Same exit code and UX issues as in previous review.Two issues persist:
- Exit code 1 is returned even for explicit
-h/--helprequests.- When no arguments are passed (
MODEis empty), the script shows "Unknown command used." which is misleading.Suggested fix
-h|--help|*) echo - if [ "$MODE" != "-h" ] && [ "$MODE" != "--help" ]; then + if [ "$MODE" != "-h" ] && [ "$MODE" != "--help" ] && [ -n "$MODE" ]; then echo "Unknown command used." echo fi cat <<EOF ... EOF - exit 1 + # Exit 0 for help requests, 1 for unknown commands + if [ "$MODE" = "-h" ] || [ "$MODE" = "--help" ] || [ -z "$MODE" ]; then + exit 0 + else + exit 1 + fi ;;Also applies to: 111-111
🧹 Nitpick comments (1)
img2num (1)
42-44: Consider removing-eflag from echo.The
-eflag interprets backslash escapes, but the color variables usetputoutput (terminal control sequences), not backslash escapes. Plainechosuffices here.Suggested change
- echo -e "${YELLOW}[INFO] Docusaurus is running inside the container, listening on all interfaces (0.0.0.0).${RESET}" - echo -e "${YELLOW}[INFO] You cannot use the 0.0.0.0 link directly.${RESET}" - echo -e "${YELLOW}[INFO] Access the site in your browser via: ${MAGENTA}http://localhost:3000/Img2Num/info/${RESET}" + echo "${YELLOW}[INFO] Docusaurus is running inside the container, listening on all interfaces (0.0.0.0).${RESET}" + echo "${YELLOW}[INFO] You cannot use the 0.0.0.0 link directly.${RESET}" + echo "${YELLOW}[INFO] Access the site in your browser via: ${MAGENTA}http://localhost:3000/Img2Num/info/${RESET}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
img2num(1 hunks)
🔇 Additional comments (6)
img2num (6)
1-6: LGTM! Solid initialization.The shebang, error handling with
set -e, and argument capture with safe shift are all correct.
7-14: LGTM!The container check and startup logic is correct.
50-58: LGTM!Both npm passthrough and shell access commands are correctly implemented.
60-63: LGTM!Docker maintenance commands are correctly implemented with proper conditional logic for
destroy.Also applies to: 66-67
69-70: LGTM!Logs command correctly tails container logs.
16-24: The hardcoded container name filter on line 19 actually works correctly due to Docker's partial substring matching behavior for thedocker ps --filter name=option. The filterimg2num-devwill match auto-generated names likeimg2num-dev-1regardless of whether Docker Compose v1 (underscores) or v2 (dashes) naming conventions are used, as long as the hardcoded prefix is present. The concern about a mismatch preventing theexecbranch from executing is invalid.
Ryan-Millard
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile.dev (2)
23-28: Consider optimizing the EMSDK clone.The current approach uses
--depth 1for a shallow clone but then runsgit fetch --tags, which negates the shallow clone benefit by fetching additional git objects.🔎 Proposed optimization
-RUN git clone --depth 1 https://github.com/emscripten-core/emsdk.git $EMSDK_DIR \ -&& cd $EMSDK_DIR \ -&& git fetch --tags \ -&& git checkout $EMSDK_VERSION \ +RUN git clone --depth 1 --branch $EMSDK_VERSION https://github.com/emscripten-core/emsdk.git $EMSDK_DIR \ +&& cd $EMSDK_DIR \ && ./emsdk install $EMSDK_VERSION \ && ./emsdk activate $EMSDK_VERSIONThis clones only the specific tag needed, reducing both clone time and image layer size.
4-4: Node.js 22 is in Maintenance LTS; consider upgrading to Node.js 24 for longer Active LTS support.Node.js 22.x moved to Maintenance LTS on October 21, 2025 and will reach end-of-life on April 30, 2027, while Node.js 24.x is in Active LTS and supported through October 20, 2026. For a development environment with sustained use, Active LTS offers guaranteed support timeline, critical security updates, and enhanced functionality. The
node:22-bullseyeimage is secure and functional, butnode:24-bullseyewould provide extended Active LTS coverage.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile.dev(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:35:30.135Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.135Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
Dockerfile.dev
🔇 Additional comments (3)
Dockerfile.dev (3)
9-20: LGTM!The system dependencies are appropriate for building WASM modules with Emscripten. Good use of
--no-install-recommendsand cleanup of apt lists to keep the image size down.
35-42: LGTM!The workdir, Chokidar polling configuration (necessary for file watching in Docker on some host systems), port exposure (5173 for Vite, 3000 for docs), and bash default command are all appropriate for a development container.
33-33: No changes needed — npm version 11 is available and appropriate.npm version 11.7.0 is the latest stable release, confirming that npm version 11 was released on December 16, 2024 and is actively maintained. Installing npm@11 globally in your Dockerfile.dev is valid and appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/docs/reference/wasm/development-workflow.md (1)
22-94: LGTM — comprehensive CMakeLists.txt template.The template is well-structured and covers all essential aspects: module name extraction, capitalization for export naming, source discovery, Emscripten flags (MODULARIZE, EXPORT_ES6, memory, export naming), build-type branching (Debug vs. Release), and output configuration. The comments are clear and guide users through the setup.
Optional note for future enhancement: The template uses
file(GLOB_RECURSE)for source discovery (line 39–41), which is a common simplification for templates but is generally discouraged in production CMake builds (non-deterministic ordering; doesn't detect new files without reconfiguration). For an introductory template, this is acceptable, but advanced users may want to enumerate sources explicitly for reproducibility.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.editorconfig(1 hunks)README.md(2 hunks)docs/docs/guidelines/CONTRIBUTING.md(1 hunks)docs/docs/guidelines/coding-style.md(1 hunks)docs/docs/project-scripts/build.md(2 hunks)docs/docs/project-scripts/clean.md(1 hunks)docs/docs/project-scripts/overview.md(1 hunks)docs/docs/reference/wasm/development-workflow.md(1 hunks)docs/docs/reference/wasm/modules/image/overview.md(1 hunks)docs/docs/reference/wasm/overview.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docs/project-scripts/build.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, do not create multiple markdown files at the repository root. The README.md at root should remain brief and point to the Docusaurus site.
Applied to files:
docs/docs/guidelines/CONTRIBUTING.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, avoid creating strangely-named markdown files (like COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, etc.) at the root level.
Applied to files:
docs/docs/guidelines/CONTRIBUTING.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, all documentation must be placed in the docs/docs/ folder following the Docusaurus structure, not at the repository root.
Applied to files:
docs/docs/guidelines/CONTRIBUTING.md
📚 Learning: 2025-12-17T21:35:30.135Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.135Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
docs/docs/guidelines/CONTRIBUTING.mddocs/docs/reference/wasm/overview.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, test documentation should be properly organized in the docs/docs/ folder structure, either as a dedicated testing category or integrated into existing categories like project-scripts.
Applied to files:
docs/docs/guidelines/CONTRIBUTING.md
🔇 Additional comments (11)
.editorconfig (1)
28-46: CMake configuration additions are well-structured and consistent.The new sections for
CMakeLists.txtand*.cmakefiles follow the established pattern in the file and maintain consistency with related language-specific blocks (particularly the C++ section). All formatting rules—indent_style, indent_size, charset, end_of_line, trim_trailing_whitespace, insert_final_newline, and max_line_length—are appropriately aligned with the project's conventions and the PR objective of migrating to CMake-based WASM builds.README.md (2)
1-8: Clarify the TODO comment and its relationship to PR #139.The TODO references PR #93's merge deadline, but per the PR objectives, PR #139 is replacing PR #93. If PR #139 merges instead of (or independently of) PR #93, this TODO logic may need adjustment. Additionally, the CAUTION block still references PR #93 and its breaking changes—consider whether this notice remains accurate if PR #139 is the primary change vehicle.
Could you clarify:
- Will PR #93 be merged, or is PR #139 the definitive replacement?
- Should the TODO deadline and/or the CAUTION block be updated to reflect PR #139's scope?
55-55: Verify Make prerequisite given CMake migration.Line 55 now states "CMake for WASM builds" (updated from Makefile). However, Line 70 still lists
Makeas a required tool. Given the migration to CMake:
- Is
Makestill required, or does CMake replace it?- Should the Prerequisites section mention CMake alongside or instead of Make?
The PR objectives mention "Preserve existing Makefile architecture," which suggests both may coexist, but this should be clarified in the Prerequisites for developer onboarding.
Also applies to: 70-70
docs/docs/guidelines/CONTRIBUTING.md (1)
35-35: LGTM — consistent Makefile → CMakeLists.txt migration.The link update correctly reflects the shift from Makefile-based WASM orchestration to CMake-based orchestration.
docs/docs/guidelines/coding-style.md (1)
9-9: LGTM — simplified indentation rule.Consolidating to a uniform 2-space indentation is cleaner and easier to enforce across the codebase.
docs/docs/project-scripts/overview.md (1)
12-17: LGTM — CMakeLists.txt reference + new script categories.The update correctly reflects the CMake-based build orchestration, and the addition of Linting and Documentation script categories aligns with the PR's expanded script organization.
docs/docs/reference/wasm/modules/image/overview.md (1)
14-14: LGTM — module structure reflects CMakeLists.txt.The directory structure correctly shows
CMakeLists.txtin place ofMakefile, aligning with the per-module CMake build configuration.docs/docs/project-scripts/clean.md (1)
22-22: LGTM — cross-platform clean scripts.The shift from
rm -rftorimrafenables Windows compatibility, and delegation toscripts/build-wasm.jscentralizes the CMake-based build cleanup. The documentation clearly explains the flow and updated CMakeLists.txt reference.Also applies to: 30-35
docs/docs/reference/wasm/overview.md (1)
31-33: LGTM — CMakeLists.txt references updated.The documentation correctly describes the root
CMakeLists.txtorchestration and delegation pattern, reflecting the Makefile→CMake migration.docs/docs/reference/wasm/development-workflow.md (2)
17-20: LGTM — root CMakeLists.txt contract clearly documented.The new contract description concisely explains that the root
CMakeLists.txtimplementsbuildanddebugtargets, with submodules supporting at least these targets.
96-101: LGTM — helpful guidance note for template customization.The note provides actionable guidance: code organization (include/src/), exported function usage (ccall/cwrap), memory configuration, and where to place module-specific options. This helps users adapt the template to their needs.
…this usage Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/reference/wasm/setup-and-dependencies.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:35:30.135Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.135Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
docs/docs/reference/wasm/setup-and-dependencies.md
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/wasm/setup-and-dependencies.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
docs/docs/reference/wasm/setup-and-dependencies.md (2)
26-26: ✓ Node.js version requirement is correctly updated.Line 26 now specifies Node.js v20.11+ (or v22 LTS), which correctly aligns with the use of
import.meta.dirnameinscripts/build-wasm.js(introduced in Node.js 20.11.0). This addresses the prior review concern about version compatibility.
22-35: Verify cross-platform WASM support claim against PR objectives.Line 24 claims WASM development "works natively on all platforms including Windows," but the PR objectives state that WASM-related scripts (build-wasm, build-wasm:debug, clean-wasm, build, clean, dev:debug, dev:all:debug) require WSL per Emscripten. This appears to be a positive outcome—if CMake + Emscripten now support true Windows natives—but deserves verification to ensure the documentation accurately reflects the actual capability and doesn't overstate Windows native support when WSL is still required for some workflows.
Can you confirm whether WASM scripts now run truly natively on Windows without WSL, or if WSL is still needed for certain workflows? If the latter, the doc should clarify which scripts require WSL (if any) on Windows.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/docs/reference/wasm/setup-and-dependencies.md (1)
39-103: Cross-platform setup instructions are well-presented.The use of Tabs component for CMake installation (lines 41–57) and Emscripten environment setup (lines 73–92) provides clear OS-specific guidance for Windows (CMD and PowerShell), Linux, and macOS. All code blocks have proper language identifiers, including the
cmdandPowerShellvariants on Windows. This effectively implements the Windows Compatibility requirement from the PR objectives.The Step 3 verification commands (lines 100–103) are shown in bash only, but since
cmake --versionandemcc --versionare universally applicable (just run in the platform's shell), this is acceptable. For maximum consistency with the other steps, consider wrapping these in a Tabs component as well, though it is not required.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/reference/wasm/setup-and-dependencies.md(1 hunks)img2num(1 hunks)img2num.ps1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- img2num
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:35:30.135Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.135Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
docs/docs/reference/wasm/setup-and-dependencies.md
🔇 Additional comments (5)
docs/docs/reference/wasm/setup-and-dependencies.md (5)
1-5: Frontmatter ID typo fixed correctly.The id change from
wasm-setup-depencenciesstowasm-setup-dependenciesresolves the typo mentioned in the PR objectives.
10-35: Heading hierarchy and cross-platform structure are correct.The prerequisites section now properly uses h2 for main headings (
## Prerequisitesat line 10) with h3 subsections (### Frontend-Only Developmentand### Full Development). This addresses the MD001 violation flagged in the previous review. The split between frontend-only and full (WASM-inclusive) development is clear and helpful for users with different needs.
26-26: Node.js requirement correctly updated.The requirement now specifies Node.js v20.11+ or v22 LTS, which aligns with the use of
import.meta.dirnameinscripts/build-wasm.js. This resolves the previous review comment about version compatibility.
105-125: Build system integration explanation is clear.Lines 107–125 effectively explain how the npm scripts integrate with the new CMake-based build system via
scripts/build-wasm.js. The documentation correctly describes the build pipeline steps and output location, providing users with a mental model of what happens when they runnpm run build-wasm.
132-148: Build System Architecture diagram is helpful and language-specified correctly.The ASCII directory tree at lines 136–146 uses the
txtlanguage identifier (addressing the earlier MD040 linting issue) and clearly shows the new CMake-based structure (CMakeLists.txt, module directories,build/outputs) versus the previous Makefile approach. The reference to PR #93 provides historical context.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
docs/docs/index.md (1)
43-43: Use HTTPS for consistency and security.The GitHub link uses
http://while other links in the repository usehttps://. Use HTTPS consistently.🔎 Proposed fix
-- [Blank Issue](https://github.com/Ryan-Millard/Img2Num/issues/new) +- [Blank Issue](https://github.com/Ryan-Millard/Img2Num/issues/new)img2num.bat (1)
2-8: Simplify comments to reduce redundancy.The batch file contains multiple REM comments that describe straightforward operations. Consider keeping only the file-level comment (line 2) and removing the overly descriptive comments on lines 4 and 7, as the code is self-explanatory.
🔎 Proposed simplification
@echo off REM img2num.bat - CMD wrapper for img2num.ps1 (PowerShell script) -REM Get the directory of the batch file SET "SCRIPT_DIR=%~dp0" -REM Call the PowerShell script with all arguments using Windows PowerShell powershell -ExecutionPolicy Bypass -File "%SCRIPT_DIR%img2num.ps1" %*Dockerfile.dev (1)
6-6: Consider updating EMSDK to a newer stable version.EMSDK version 4.0.10 may be outdated compared to the latest stable releases in the 4.0.x series. Using a more recent version ensures access to bug fixes, security improvements, and the latest WebAssembly features.
What is the latest stable version of Emscripten EMSDK in the 4.0.x series?docs/docs/reference/wasm/how-to-add-a-module.md (1)
22-22: Fix heading level to follow Markdown hierarchy.The heading should be level 2 (
##) rather than level 1 (#) to maintain proper heading hierarchy.🔎 Proposed fix
-# Minimal module CMakeLists.txt (copy/paste) +## Minimal module CMakeLists.txt (copy/paste)src/wasm/modules/image/CMakeLists.txt (1)
52-56: Replace deprecated -g4 flag with -g or -gsource-map.The
-g4flag is deprecated in recent Emscripten versions. Use-g(equivalent to-g3, preserves DWARF for interactive debugging) or-gsource-mapif you specifically need source maps for broader browser compatibility.🔎 Proposed fix
if(CMAKE_BUILD_TYPE STREQUAL "Debug") - target_compile_options(${MODULE_NAME}_wasm PRIVATE -O0 -g4) + target_compile_options(${MODULE_NAME}_wasm PRIVATE -O0 -g) target_link_options(${MODULE_NAME}_wasm PRIVATE "SHELL:-s ASSERTIONS=2" - -g4 + -g )docs/docs/reference/wasm/setup-and-dependencies.md (2)
39-41: Document CMake reconfiguration requirement for GLOB_RECURSE.Using
file(GLOB_RECURSE ...)means CMake won't automatically detect newly added source files—developers must manually reruncmake(or deletecmake-build/) after adding new.cppfiles. Consider either explicitly listing sources or adding a note in the template warning developers about this limitation.📝 Suggested documentation addition
Add a warning comment in the template:
# Collect source files (recursively) +# NOTE: CMake will not auto-detect new .cpp files added after initial +# configuration. Re-run cmake or delete cmake-build/ to pick up new files. file(GLOB_RECURSE SRC_FILES "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" )Or reference it in the note section at lines 96-101.
100-103: Useforce: truein rmSync for better Windows compatibility.Setting
force: falsemeansrmSyncwill throw on permission errors or read-only files, which are more common on Windows. Usingforce: truemakes cleanup more robust and prevents unnecessary build failures due to locked or read-only artifacts.🔎 Suggested fix
try { - rmSync(dir, { recursive: true, force: false }); + rmSync(dir, { recursive: true, force: true }); console.log(` Removed: ${dir}`);
🧹 Nitpick comments (5)
src/data/contributor-credits.json (1)
23-36: New contributor entries look good.The JSON structure is valid and the new contributor data is properly formatted with all required fields.
If this file is manually maintained, would you like help generating a script to automatically fetch and update contributor data from the GitHub API? This would reduce the risk of manual entry errors and keep the data synchronized.
src/wasm/CMakeLists.txt (1)
31-38: Consider the limitation offile(GLOB ...)for module discovery.Using
file(GLOB ...)for module auto-discovery is convenient, but CMake won't automatically detect newly added module directories until you manually reconfigure (reruncmake). If a developer creates a new module with aCMakeLists.txt, the build system won't pick it up until reconfiguration.Consider one of these approaches:
- Option 1 (recommended for CHILL review): Document this limitation in a comment above the glob, noting that developers must rerun
cmakeafter adding new modules.- Option 2: Maintain an explicit list of modules (e.g.,
set(MODULES image ...)) for more predictable builds at the cost of manual updates.🔎 Example documentation comment
+# Note: file(GLOB) does not trigger automatic reconfiguration. +# After adding a new module directory, rerun: emcmake cmake .. # Auto-discover all modules with CMakeLists.txt file(GLOB MODULE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/modules/*")docs/docs/reference/wasm/how-to-add-a-module.md (1)
39-41: Consider explicitly listing source files instead of using GLOB_RECURSE.Using
file(GLOB_RECURSE ...)means CMake won't detect newly added source files until you manually rerun cmake. For better build reliability, either explicitly list source files or document that developers must reconfigure CMake when adding new.cppfiles to the module.💡 Alternative approaches
Option 1: Explicit file list
set(SRC_FILES "${CMAKE_CURRENT_SOURCE_DIR}/src/main.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/src/utils.cpp" # Add new files here )Option 2: Keep GLOB but add documentation
# Collect source files # Note: If you add new .cpp files, you must rerun cmake to detect them file(GLOB_RECURSE SRC_FILES "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" )src/wasm/modules/image/CMakeLists.txt (1)
17-19: Avoid file(GLOB_RECURSE) for source file collection.CMake won't detect newly added source files until you manually reconfigure. This means adding a new
.cppfile to thesrc/directory won't trigger a rebuild or show up in the build untilcmakeis rerun. For more reliable builds, explicitly list source files or document the reconfiguration requirement.💡 Recommended alternatives
Option 1: Explicit source list (preferred)
# Collect source files set(SRC_FILES "${CMAKE_CURRENT_SOURCE_DIR}/src/main.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/src/color.cpp" # Add new source files here )Option 2: Keep GLOB with clear documentation
# Collect source files # IMPORTANT: Adding new .cpp files requires running cmake again to detect them file(GLOB_RECURSE SRC_FILES "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" )docs/docs/reference/wasm/setup-and-dependencies.md (1)
76-84: Improve UX for invocation without arguments.When users run
./img2numwith no arguments (MODE is empty), they see "Unknown command used." before the help text and the script exits with code 1. This is unfriendly—users who simply want to see available commands shouldn't get an error message.🔎 Suggested fix
-h|--help|*) EXIT_CODE=0 echo - if [ "$MODE" != "-h" ] && [ "$MODE" != "--help" ]; then + if [ "$MODE" != "-h" ] && [ "$MODE" != "--help" ] && [ -n "$MODE" ]; then EXIT_CODE=1 echo "Unknown command used." echo fiThis way:
./img2num(empty) → shows help, exits 0./img2num --help→ shows help, exits 0./img2num badcommand→ shows error + help, exits 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
docs/docs/introduction/img/docker-desktop-homepage.jpgis excluded by!**/*.jpgdocs/docs/introduction/img/docker-desktop-resources-button-location.jpgis excluded by!**/*.jpgdocs/docs/introduction/img/docker-desktop-settings-button-location.jpgis excluded by!**/*.jpgdocs/docs/introduction/img/docker-desktop-wsl-integration-button-location.jpgis excluded by!**/*.jpgdocs/docs/introduction/img/docker-desktop-wsl-integration-setup.jpgis excluded by!**/*.jpgdocs/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.devcontainer/entrypoint.sh(1 hunks).dockerignore(1 hunks).editorconfig(2 hunks).gitignore(1 hunks)Dockerfile.dev(1 hunks)README.md(2 hunks)docker-compose.yml(1 hunks)docs/docs/guidelines/CONTRIBUTING.md(1 hunks)docs/docs/guidelines/coding-style.md(1 hunks)docs/docs/index.md(2 hunks)docs/docs/introduction/getting-started.md(4 hunks)docs/docs/introduction/usage.md(0 hunks)docs/docs/project-scripts/build.md(2 hunks)docs/docs/project-scripts/clean.md(1 hunks)docs/docs/project-scripts/overview.md(1 hunks)docs/docs/reference/wasm/development-workflow.md(1 hunks)docs/docs/reference/wasm/how-to-add-a-module.md(1 hunks)docs/docs/reference/wasm/modules/image/overview.md(1 hunks)docs/docs/reference/wasm/overview.md(2 hunks)docs/docs/reference/wasm/setup-and-dependencies.md(1 hunks)docs/package.json(1 hunks)docs/scripts/help.js(1 hunks)img2num(1 hunks)img2num.bat(1 hunks)img2num.ps1(1 hunks)package.json(2 hunks)scripts/build-wasm.js(1 hunks)src/data/contributor-credits.json(1 hunks)src/wasm/CMakeLists.txt(1 hunks)src/wasm/Makefile(0 hunks)src/wasm/modules/image/CMakeLists.txt(1 hunks)src/wasm/modules/image/Makefile(0 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (3)
- src/wasm/Makefile
- src/wasm/modules/image/Makefile
- docs/docs/introduction/usage.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, do not create multiple markdown files at the repository root. The README.md at root should remain brief and point to the Docusaurus site.
Applied to files:
docs/docs/index.mddocs/docs/guidelines/CONTRIBUTING.md
📚 Learning: 2025-12-20T20:11:28.389Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-20T20:11:28.389Z
Learning: In the Img2Num repository, all documentation should be properly organized in the docs/docs/ folder structure following the Docusaurus conventions, either as a dedicated category or integrated into existing categories like project-scripts.
Applied to files:
docs/docs/index.mdimg2numdocs/docs/guidelines/CONTRIBUTING.mddocs/docs/introduction/getting-started.mddocs/docs/reference/wasm/overview.mdimg2num.ps1docs/docs/project-scripts/overview.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, all documentation must be placed in the docs/docs/ folder following the Docusaurus structure, not at the repository root.
Applied to files:
docs/docs/index.mddocs/docs/guidelines/CONTRIBUTING.mddocs/docs/introduction/getting-started.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, avoid creating strangely-named markdown files (like COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, etc.) at the root level.
Applied to files:
docs/docs/index.mddocs/docs/guidelines/CONTRIBUTING.md
📚 Learning: 2025-12-17T21:35:30.135Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.135Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
Dockerfile.devdocs/docs/guidelines/CONTRIBUTING.mddocs/docs/reference/wasm/setup-and-dependencies.mddocs/docs/reference/wasm/overview.md
🔇 Additional comments (30)
docs/docs/guidelines/coding-style.md (1)
1-100: Coding style guidelines appropriately simplified.The removal of the Makefile-specific exception from indentation rules and deletion of the Makefiles section aligns well with the PR's broader architectural shift toward CMake-based builds. The indentation rule is now uniform across all supported languages/file types, which improves clarity and matches the
.editorconfigreference on line 8.src/data/contributor-credits.json (1)
7-7: Verify the accuracy of updated contribution counts.Please confirm that the contribution count increases for Ryan-Millard (+6, from 49→55) and dependabot[bot] (+2, from 14→16) reflect actual commit/PR activity in the repository.
Also applies to: 14-14
docs/package.json (1)
8-8: LGTM! Docker-friendly server configuration.Binding to
0.0.0.0allows the Docusaurus dev server to be accessible from outside the Docker container, and the--poll 1000flag ensures file watching works reliably in containerized and WSL environments where native file system events may not propagate correctly.Also applies to: 13-13
.dockerignore (1)
1-8: LGTM! Standard Docker ignore patterns.The ignore patterns appropriately exclude build artifacts, dependencies, and metadata to reduce Docker build context size and improve build performance.
.gitignore (1)
31-34: LGTM! Appropriate CMake artifact ignores.The new ignore patterns correctly exclude CMake-generated build artifacts, aligning with the repository's shift from Makefile-based to CMake-based WASM workflows.
docs/scripts/help.js (1)
9-9: LGTM! Helpful self-documenting entry.Adding the description for the
helpscript itself improves discoverability and completes the help documentation..editorconfig (1)
28-46: LGTM! Comprehensive cross-platform editor configuration.The new CMake and shell script blocks follow EditorConfig best practices with:
- Appropriate line endings (LF for Unix-based, CRLF for Windows scripts)
- Consistent formatting rules across all file types
- Proper character encoding and whitespace handling
Also applies to: 149-192
docs/docs/index.md (1)
22-22: LGTM! Improved documentation structure.The simplified Getting Started link and the new issue reporting guidance with template links improve the onboarding experience and make it easier for contributors to report issues.
Also applies to: 33-35, 37-42
docs/docs/introduction/getting-started.md (2)
10-14: LGTM! Comprehensive Docker-first onboarding flow.The new tabbed structure effectively guides users through Docker installation across different operating systems with clear, actionable steps. The tip recommending Docker as the preferred route helps users make informed decisions about their setup path.
Also applies to: 22-29, 34-163
253-376: Complex but functional tabbed dependency and runtime sections.The multi-level nested tabs provide comprehensive coverage of different installation routes (Docker/Local), operating systems, and shell environments. While the structure is complex, it serves the goal of platform-agnostic development well.
Based on learnings, all documentation is properly organized in docs/docs/ following Docusaurus conventions.
Also applies to: 383-518
img2num.ps1 (3)
11-36: LGTM! Well-structured container orchestration functions.The
Ensure-ContainerandRun-InContainerfunctions properly handle container lifecycle management and command execution, checking for both existence and running state before executing commands.
54-67: LGTM! User-friendly docs command with helpful output.The color-capable guidance for the
docscommand helpfully explains the container networking behavior and provides the correct localhost URL, improving the developer experience when working with containerized Docusaurus.
101-142: LGTM! Comprehensive and well-organized help documentation.The usage message clearly documents all available commands with appropriate grouping and helpful descriptions, making the tool easy to discover and use.
docs/docs/guidelines/CONTRIBUTING.md (1)
35-35: LGTM!The documentation update correctly reflects the migration from Makefile to CMake-based orchestration for WASM builds.
.devcontainer/entrypoint.sh (1)
4-7: Verify EMSDK_ROOT is always defined in the container.With
set -uenabled (line 2), the script will fail ifEMSDK_ROOTis unset when evaluating the condition[ -f "${EMSDK_ROOT}/emsdk_env.sh" ]. Ensure thatEMSDK_ROOTis always exported in the Dockerfile or Docker Compose environment, or adjust the check to handle unset variables gracefully.🔎 Alternative approach for safer unset variable handling
# Source emsdk environment -if [ -f "${EMSDK_ROOT}/emsdk_env.sh" ]; then +if [ -n "${EMSDK_ROOT:-}" ] && [ -f "${EMSDK_ROOT}/emsdk_env.sh" ]; then source "${EMSDK_ROOT}/emsdk_env.sh" fidocs/docs/project-scripts/overview.md (1)
12-12: LGTM!The documentation correctly reflects the new CMake-based WASM build orchestration.
README.md (1)
55-55: LGTM!The tech stack update correctly reflects the migration from Makefile-based to CMake-based WASM builds.
vite.config.js (1)
44-45: LGTM!The server configuration correctly enables Docker container access by binding to all network interfaces (
0.0.0.0) and fixing the port to align with the Docker Compose setup. This is standard practice for containerized development environments.docs/docs/reference/wasm/modules/image/overview.md (1)
14-14: LGTM!The documentation correctly updates the module structure to reflect the CMake-based build configuration.
docs/docs/project-scripts/build.md (1)
48-48: LGTM!The documentation references have been correctly updated to point to the CMake-based orchestrator, reflecting the repository-wide migration from Makefile to CMake workflows.
Also applies to: 58-58
src/wasm/CMakeLists.txt (1)
7-28: LGTM!The CMake project setup is well-structured with appropriate guards:
- Reasonable minimum CMake version requirement (3.16)
- Proper C++17 standard enforcement
- Clear Emscripten verification with helpful error messaging
- Sensible Release build type default
docs/docs/project-scripts/clean.md (1)
22-35: LGTM! Documentation accurately reflects the cross-platform build migration.The updates correctly document the shift from
rm -rftorimraffor cross-platform compatibility and the migration from Makefile-based to CMake-based WASM cleanup workflow. The explanations are clear and align with the broader PR objectives.Dockerfile.dev (1)
1-42: Well-structured development container configuration.The Dockerfile follows best practices:
- Uses a stable Node base image
- Properly caches EMSDK installation in Docker layers
- Installs necessary build dependencies
- Configures appropriate environment variables for containerized development
- Exposes the correct ports for Vite and Docusaurus
docker-compose.yml (1)
1-25: LGTM! Solid Docker Compose development setup.The configuration is well-designed:
- Named volumes isolate node_modules to prevent host/container filesystem conflicts
- Appropriate port mappings for all development servers (Vite dev, preview, and Docusaurus)
- CHOKIDAR polling enabled for reliable file watching in containerized environments
- Interactive terminal support for development workflows
docs/docs/reference/wasm/how-to-add-a-module.md (1)
7-122: Clear documentation for CMake-based WASM module workflow.The migration from Makefile to CMake is well-documented with:
- Step-by-step instructions for creating a new module
- Complete CMakeLists.txt template with proper Emscripten flags
- Directory structure example
- Build commands for both Release and Debug configurations
This will help contributors add new WASM modules consistently.
package.json (1)
17-21: LGTM! Scripts successfully migrated to cross-platform approach.The script updates achieve the PR's cross-platform goals:
- WASM build/clean scripts now delegate to a Node.js orchestrator instead of Makefile
rimrafreplacesrm -rffor Windows compatibility- All changes are consistent with the documented CMake-based build workflow
Also applies to: 54-54
src/wasm/modules/image/CMakeLists.txt (1)
1-71: Well-structured CMake configuration for WASM module.The CMakeLists.txt properly:
- Derives module name from directory structure
- Configures Emscripten-specific flags for web environment
- Separates Debug and Release build configurations
- Sets appropriate memory limits and growth settings
- Generates correctly named output artifacts
This provides a solid template for the CMake-based WASM build workflow.
docs/docs/reference/wasm/overview.md (1)
31-44: LGTM! Documentation correctly reflects the CMake migration.The updates accurately document the transition from Makefile-based to CMakeLists.txt-based WASM builds. The references to the root CMake configuration and the simplified command explanations align with the broader architectural changes in this PR.
docs/docs/reference/wasm/setup-and-dependencies.md (2)
1-148: Excellent documentation improvements!The migration from Makefile to CMake-based workflow is well-documented with clear cross-platform instructions. The use of Docusaurus Tabs for OS-specific commands greatly improves usability, and the prerequisites section now clearly distinguishes between frontend-only and full WASM development requirements. Previous linting issues (MD001, MD040) and the Node.js version requirement have been properly addressed.
1-231: Well-structured cross-platform build script.The script demonstrates excellent practices: comprehensive error handling, clear diagnostics, Windows-specific handling (
emcc.bat,emcmake.bat), dynamic module discovery, and proper argument validation. The migration from Makefile to this Node.js orchestrator with CMake significantly improves the cross-platform developer experience.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/index.md (1)
26-29: Lines 28-29 contain incomplete placeholder links pointing to the current page.Lines 28-29 both reference
./(the current page) instead of actual documentation targets. No Examples or FAQ pages exist in the docs structure yet. Additionally, line 26 duplicates the installation link already provided on line 22 by both linking to./introduction/getting-started.md.Create the missing Examples and FAQ pages, or remove these items until the pages are ready.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/index.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, do not create multiple markdown files at the repository root. The README.md at root should remain brief and point to the Docusaurus site.
Applied to files:
docs/docs/index.md
📚 Learning: 2025-12-20T20:11:28.389Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-20T20:11:28.389Z
Learning: In the Img2Num repository, all documentation should be properly organized in the docs/docs/ folder structure following the Docusaurus conventions, either as a dedicated category or integrated into existing categories like project-scripts.
Applied to files:
docs/docs/index.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, all documentation must be placed in the docs/docs/ folder following the Docusaurus structure, not at the repository root.
Applied to files:
docs/docs/index.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, avoid creating strangely-named markdown files (like COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, etc.) at the root level.
Applied to files:
docs/docs/index.md
🔇 Additional comments (1)
docs/docs/index.md (1)
33-44: Good fixes to past documentation issues.The grammatical error from line 38 has been corrected ("an something" → "something"), and all GitHub URLs now consistently use HTTPS. These updates improve documentation quality and security.
ea5040c to
44819f7
Compare
Ryan-Millard
added
the
BREAKING CHANGE
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/index.md (1)
28-29: Remove or fix self-referential documentation links.Lines 28–29 use
./which resolves according to the page's URL location rather than its file system location, making both[Examples](./)and[FAQ](./)link back to the current page. Since no Examples or FAQ documentation pages exist in the repository, either remove these entries or replace them with links to relevant existing content (e.g., introduction/getting-started.md or project-scripts/overview.md).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/index.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, do not create multiple markdown files at the repository root. The README.md at root should remain brief and point to the Docusaurus site.
Applied to files:
docs/docs/index.md
📚 Learning: 2025-12-20T20:11:28.389Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-20T20:11:28.389Z
Learning: In the Img2Num repository, all documentation should be properly organized in the docs/docs/ folder structure following the Docusaurus conventions, either as a dedicated category or integrated into existing categories like project-scripts.
Applied to files:
docs/docs/index.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, all documentation must be placed in the docs/docs/ folder following the Docusaurus structure, not at the repository root.
Applied to files:
docs/docs/index.md
📚 Learning: 2025-12-17T22:39:25.704Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T22:39:25.704Z
Learning: In the Img2Num repository, avoid creating strangely-named markdown files (like COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, etc.) at the root level.
Applied to files:
docs/docs/index.md
🔇 Additional comments (1)
docs/docs/index.md (1)
33-43: All GitHub issue template URLs are correct. The referenced template files (bug_report.yml and refactor.yml) exist in .github/ISSUE_TEMPLATE/, and the documentation links use the proper GitHub format with the ?template= query parameter pointing to the correct repository.
@CodeRabbit, is this still relevant after my force push? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 554 @Ryan-Millard, yes, the comment is still relevant. I can see the Quick Links section in - [Examples](./) – Ready-to-use snippets and demos
- [FAQ](./) – Common questions answeredBoth of these links use
The other Quick Links (Overview and Changelog) are working correctly since they point to specific paths. 🧠 Learnings used |
44819f7 to
9a24995
Compare
My apolocheese, @CodeRabbit. I forgot to stage the changes before force pushing. How is it now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1540 @Ryan-Millard, perfect! 🎉 The issue is now resolved. The Quick Links section in
Both of these point to actual content, so there are no more self-referential link issues. Great job fixing that after staging the changes! |
2 participants
✨ Feature Pull Request
📌 Description
Improves developer experience by making the entire app platform-agnostic during development.
🔗 Issue
Fixes #80
Fixes #127
Closes #107
Closes #109
Replaces #93
Changes
Changes in #93 / #80
rm -rfwithrimrafinclean-jsscript for cross-platform compatibilitywasm-setup-depencenciess→wasm-setup-dependencies)Scripts now working natively on Windows:
WASM scripts still require WSL (Emscripten recommendation):
Changes in #127
./img2num) to help with onboarding new devs in:🧪 How Has This Been Tested?
#93
npm run clean-jsworks with rimraf (exits 0 on non-existent dir)npm run lint,npm run format-js,npm run format-wasmall pass#127
🧩 Checklist
📸 Screenshots / Demo (if applicable)
N/A - Infrastructure/documentation change
💬 Additional Context
The approach preserves the existing Makefile architecture for WASM builds (extensible for new modules) while documenting that WSL is required for WASM development on Windows. This aligns with Emscripten's recommendations.
Frontend-only development now works fully natively on Windows without any Unix tools.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.