Skip to content

Comments

refactor+test(rust): unify resize behavior, centralize errors, and add e2e tests#2

Merged
2YH02 merged 8 commits intomainfrom
develop
Feb 9, 2026
Merged

refactor+test(rust): unify resize behavior, centralize errors, and add e2e tests#2
2YH02 merged 8 commits intomainfrom
develop

Conversation

@2YH02
Copy link
Owner

@2YH02 2YH02 commented Feb 9, 2026

Summary

  • Refactored the Rust image pipeline for more consistent resize behavior and cleaner internal error handling.
  • Added Rust e2e-style tests to cover the real resize/encode flow and unsupported-format handling.
  • This was needed to reduce path divergence in resize logic, align default behavior across layers, and improve regression safety.

Key Changes

  • Unified resize path in src/lib.rs by using DynamicImage::resize_exact/resize (removed explicit RGBA resize branch).
  • Aligned Rust default quality to 0.7 to match the TypeScript wrapper default.
  • Refactored format parsing to use ASCII-insensitive matching without to_lowercase allocation.
  • Introduced typed internal Rust errors (ToolkitError) and centralized wasm boundary conversion to JsValue.
  • Added Rust tests for pipeline-level behavior:
    • exact resize + JPEG output
    • single-dimension resize with aspect ratio preservation
    • unsupported output format rejection

Breaking Changes

  • No breaking changes
  • Yes (describe below)

Validation

  • cargo test
  • npm run build
  • Example app manual check

Release Checklist (if applicable)

  • package.json version is correct
  • CHANGELOG.md updated
  • develop -> main merge ready
  • Tag push plan ready (e.g. v2.1.0)

Notes

  • Rust test coverage is now stronger for core pipeline paths, but TS wrapper unit tests are still pending.
  • Follow-up: add TS unit tests for ts-wrapper/resizeImage.ts branching (webp lossy/fallback, option sanitization, error paths).

@qodo-code-review
Copy link

Review Summary by Qodo

Unify resize behavior, centralize errors, and add e2e tests

✨ Enhancement 🧪 Tests 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Unified resize behavior using DynamicImage::resize_exact instead of explicit RGBA branch
• Introduced typed internal ToolkitError enum with centralized wasm boundary conversion
• Changed default quality from 0.8 to 0.7 to match TypeScript wrapper
• Refactored format parsing to use ASCII-insensitive matching without allocation
• Added three e2e-style Rust tests covering resize pipeline and error handling
Diagram
flowchart LR
  A["Resize Logic"] -->|"Unified via resize_exact"| B["DynamicImage"]
  C["Error Handling"] -->|"Typed ToolkitError"| D["JsValue Conversion"]
  E["Format Parsing"] -->|"ASCII-insensitive"| F["ImageFormat"]
  G["Tests"] -->|"E2E Coverage"| H["Pipeline Validation"]
Loading

Grey Divider

File Changes

1. src/lib.rs Enhancement, tests, bug fix +135/-24

Refactor resize pipeline and centralize error handling

• Removed unused imageops::resize import and unified resize path using
 DynamicImage::resize_exact for exact dimensions
• Introduced ToolkitError enum with From<ToolkitError> impl for JsValue to centralize error
 handling across all encoding/decoding operations
• Refactored parse_format() to use eq_ignore_ascii_case() instead of to_lowercase() allocation
• Changed default quality from 0.8 to 0.7 in encode_image() function
• Extracted resize_image_impl() and resize_image_with_options() helper functions to enable
 testability
• Added three integration tests: exact resize to JPEG, single-dimension resize with aspect ratio
 preservation, and unsupported format rejection
• Added test helper functions make_test_png() and decode_image() for pipeline-level testing

src/lib.rs


Grey Divider

Qodo Logo

@2YH02 2YH02 added the enhancement New feature or request label Feb 9, 2026
@qodo-code-review
Copy link

qodo-code-review bot commented Feb 9, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📎 Requirement gaps (0)

Grey Divider


Action required

✅ 1. ToolkitError exposes internal e 📘 Rule violation ⛨ Security
Description
ToolkitError is converted into JsValue using interpolated internal error strings (e.g.,
  Decode failed: {e}), which can surface implementation details to JS callers.
• This can leak library/internal state and reduces security by exposing low-level failure reasons
  directly to end-users.
Code

src/lib.rs[R36-53]

+impl From<ToolkitError> for JsValue {
+    fn from(error: ToolkitError) -> Self {
+        match error {
+            ToolkitError::InvalidOptions(e) => JsValue::from_str(&format!("Invalid options: {}", e)),
+            ToolkitError::FormatGuessFailed(e) => {
+                JsValue::from_str(&format!("Format guess failed: {}", e))
+            }
+            ToolkitError::DecodeFailed(e) => JsValue::from_str(&format!("Decode failed: {}", e)),
+            ToolkitError::UnsupportedFormat => JsValue::from_str("Unsupported format"),
+            ToolkitError::JpegEncodeFailed(e) => {
+                JsValue::from_str(&format!("JPEG encode failed: {}", e))
+            }
+            ToolkitError::PngEncodeFailed(e) => {
+                JsValue::from_str(&format!("PNG encode failed: {}", e))
+            }
+            ToolkitError::WebpEncodeFailed => JsValue::from_str("Image encoding failed"),
+            ToolkitError::WriteFailed(e) => JsValue::from_str(&format!("Write failed: {}", e)),
+        }
Evidence
PR Compliance ID 4 requires user-facing errors to be generic and not expose internal details. The
From for JsValue implementation embeds internal e strings into user-visible JsValue messages.

Rule 4: Generic: Secure Error Handling
src/lib.rs[36-53]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ToolkitError` is converted into user-facing `JsValue` strings that include internal error details via `{e}` interpolation. This violates secure error handling requirements for external-facing error messages.
## Issue Context
This code runs at the wasm boundary (`resize_image` returns `Result&amp;lt;_, JsValue&amp;gt;`). Errors returned as `JsValue` are observable by JS consumers.
## Fix Focus Areas
- src/lib.rs[36-53]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


✅ 2. Console logs raw encode error 📘 Rule violation ⛨ Security
Description
encode_as_webp logs the raw underlying encoder error string to the browser console via
  console::error_1, exposing internal failure details to end users.
• The log is also unstructured text, reducing auditability and increasing the risk of inadvertently
  logging sensitive/internal data.
Code

src/lib.rs[R190-194]

       .encode(&rgba, width, height, ExtendedColorType::Rgba8)
       .map_err(|e| {
           console::error_1(&JsValue::from_str(&format!("WebP encode failed: {}", e)));
-            JsValue::from_str("Image encoding failed")
+            ToolkitError::WebpEncodeFailed
       })
Evidence
PR Compliance ID 4 disallows exposing internal implementation details to end users, and PR
Compliance ID 5 requires safe/structured logging. The console::error_1 call prints a formatted
message containing the underlying error e in unstructured form.

Rule 4: Generic: Secure Error Handling
Rule 5: Generic: Secure Logging Practices
src/lib.rs[188-194]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`encode_as_webp` logs a raw underlying error string to the browser console, which is user-visible and unstructured.
## Issue Context
The wasm module runs in a client context; console logs are effectively user-facing and should not include internal error details.
## Fix Focus Areas
- src/lib.rs[188-194]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


✅ 3. NaN quality becomes 0 🐞 Bug ✓ Correctness
Description
quality is deserialized as Option and is not validated for finiteness; NaN is a valid f32
  value and can flow through.
• The current computation clamps on f32 and then casts to u8; for NaN the clamp still yields
  NaN, and the cast yields 0, which is outside the intended 1–100 JPEG quality range.
• The TS wrapper’s clamp() also doesn’t guard against NaN, so even wrapper users can inadvertently
  pass NaN through to WASM.
Code

src/lib.rs[141]

+    let quality = (options.quality.unwrap_or(0.7) * 100.0).round().clamp(1.0, 100.0) as u8;
Evidence
Rust computes the encoder quality via float math + clamp() and then casts to u8 without checking
for non-finite values; the TS wrapper similarly clamps without a finiteness check before calling
into WASM.

src/lib.rs[135-148]
ts-wrapper/resizeImage.ts[120-145]
ts-wrapper/resizeImage.ts[242-244]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`quality` can be `NaN` when coming from JS. The current float clamp + `as u8` cast can yield `0`, which is outside the intended JPEG quality range and can lead to unexpected output or encoder failure.
### Issue Context
The TS wrapper clamps via `Math.min/Math.max` but does not guard non-finite values; Rust similarly does not validate finiteness before casting.
### Fix Focus Areas
- src/lib.rs[135-148]
- ts-wrapper/resizeImage.ts[120-145]
- ts-wrapper/resizeImage.ts[242-244]
### Suggested fix (Rust)
- Extract `quality_f = options.quality.unwrap_or(0.7)`.
- If `!quality_f.is_finite()` then either:
- return `Err(ToolkitError::InvalidOptions(&amp;quot;quality must be a finite number&amp;quot;.into()))`, or
- set `quality_f = 0.7`.
- Then compute `quality_u8 = (quality_f * 100.0).round().clamp(1.0, 100.0) as u8`.
### Suggested fix (TS)
- Update `clamp()` to handle `NaN`/non-finite:
- `if (!Number.isFinite(value)) return min;` (or return a provided default).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@2YH02 2YH02 merged commit a9e67d1 into main Feb 9, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant