Skip to content

MobX crop store for Image Cropper#2321

Open
rahmanunver wants to merge 4 commits into
feat/image-cropper-polish-rotate-bwfrom
experiment/image-cropper-mobx-store
Open

MobX crop store for Image Cropper#2321
rahmanunver wants to merge 4 commits into
feat/image-cropper-polish-rotate-bwfrom
experiment/image-cropper-mobx-store

Conversation

@rahmanunver

@rahmanunver rahmanunver commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Pull request type

Refactoring (e.g. file rename, variable rename, etc.)


Description

Stacked on top of the Image Cropper PR (feat/image-cropper-polish-rotate-bw) — base is set to that branch, so this diff shows only the MobX refactor, not the base PR's changes.

Why. Review feedback flagged that the widget's state had become too coupled to React. ImageCropperContainer carried five state-mirroring refs (committedCropRef, zoomRef, grayscaleRef, liveCropRef, zoomAnchorRef) plus two callback mirrors (markInternalRef, showPreviewRef). They existed only because applyCrop was a stable-identity useCallback (kept stable for the 400ms auto-apply debouncer) and therefore couldn't read current state through its stale closure. A MobX store method reads this.zoom directly, so the refs are no longer needed.

Moved into ImageCropperStore (MobX): the crop-domain working state — liveCrop, committedCrop, zoomAnchor (observable.ref), zoom, grayscale (observable) — plus aspect (computed) and the crop operations as actions (setZoom, commitCrop, toggleGrayscale, rotate, reset, initFromImageLoad, onImageChanged). The store also owns the debounce + auto-apply gate (absorbing useAutoApplyCrop). Crop ops are now plain calls like store.setZoom(v) / store.rotate(90).

Stayed normal React state (imperative / DOM-bound): the imageRef DOM node, usePreviewSrc (blob object-URL lifecycle), useOriginalImage (fetch + unmount cleanup + internal-change flag), and CropArea's local DOM-measurement state. These are injected into the store via setDeps. Config props are read live through the mobx-kit gate, not copied into observables.

Controlled/uncontrolled unchanged. The only externally controlled value is props.image (Mendix EditableValue); zoom/rotation/grayscale/crop are internal. Inbound image changes sync through one uri-keyed effect calling store.onImageChanged(); outbound writes go markInternalChange()setValue(), with the internal-change flag absorbing the resulting uri change so there's no feedback loop. No reactions used for prop syncing.

Preview stays an async action, not a computedcropImage/rotateImage are async canvas operations reading a live DOM <img>, so they can't be a pure synchronous computed.

Removed: the 5 mirror refs + 2 callback mirrors, and the useAutoApplyCrop / useImageCropperState hooks (subsumed by the store). ImageCropperContainer is now an observer wired via the shared widget-plugin-mobx-kit gate pattern (GateProvider + useConst + useSetup), matching custom-chart-web.

Public API unchanged — no XML/props changes; widget behavior is identical.

@rahmanunver rahmanunver requested a review from a team as a code owner July 13, 2026 10:16
@github-actions

This comment has been minimized.

@rahmanunver rahmanunver changed the title Experiment: MobX crop store for Image Cropper MobX crop store for Image Cropper Jul 13, 2026
…bx-kit

Dependencies for the MobX crop store: mobx 6.12.3 and mobx-react-lite
4.0.7 (pinned to match the rest of the monorepo) plus the shared
widget-plugin-mobx-kit gate/setup utilities.
Centralize crop-domain working state (live/committed crop, zoom,
grayscale, zoom anchor) and its operations in a MobX store. Store
methods read observable state directly, so the debounced auto-apply no
longer needs a stable-identity callback reading state-mirroring refs.

The store owns the debounce + armed/user-interacted apply gate
(previously useAutoApplyCrop) and exposes crop operations as actions:
setZoom, commitCrop, toggleGrayscale, rotate, reset, initFromImageLoad,
onImageChanged. aspect is a computed derived from gated props.

Imperative React-owned dependencies (live img element, blob-URL
preview, original-image capture, internal-change flag) are injected via
setDeps and not owned by the store. Preview generation stays an async
action, not a computed, because cropImage/rotateImage are async canvas
operations with side effects.

Add headless unit tests covering the commit gate, debounce coalescing,
no-stale-reads, rotate/reset/grayscale flows, inbound image sync, and
read-only/no-crop guards.
Wire ImageCropperContainer to the MobX store via the mobx-kit gate
pattern (GateProvider + useConst + useSetup) and wrap it in observer.
Crop operations become plain store method calls.

Remove the five state-mirroring refs (committedCropRef, zoomRef,
grayscaleRef, liveCropRef, zoomAnchorRef) and the two callback mirrors
(markInternalRef, showPreviewRef); the store reads its own observable
state, so stable-identity closures no longer need them. Config and the
controlled image value are read live through the gate; the bound uri is
synced inbound via a single effect calling store.onImageChanged.

usePreviewSrc, useOriginalImage and the DOM image ref stay in React and
are injected into the store through setDeps. Delete useAutoApplyCrop and
useImageCropperState, now subsumed by the store.
@rahmanunver rahmanunver force-pushed the experiment/image-cropper-mobx-store branch from fb16fae to edfa936 Compare July 13, 2026 10:39
@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/image-cropper-web/package.json Added mobx, mobx-react-lite, @mendix/widget-plugin-mobx-kit dependencies
packages/pluggableWidgets/image-cropper-web/src/components/CropArea.tsx Import reorder only
packages/pluggableWidgets/image-cropper-web/src/components/ImageCropperContainer.tsx Replaced React-state + ref-mirror pattern with MobX store + observer
packages/pluggableWidgets/image-cropper-web/src/stores/ImageCropperStore.ts New MobX store absorbing state, debounce, original-image capture, and preview blob lifecycle
packages/pluggableWidgets/image-cropper-web/src/stores/__tests__/ImageCropperStore.spec.ts New comprehensive store spec
packages/pluggableWidgets/image-cropper-web/src/hooks/useAutoApplyCrop.ts Deleted — absorbed into store
packages/pluggableWidgets/image-cropper-web/src/hooks/useImageCropperState.ts Deleted — absorbed into store
packages/pluggableWidgets/image-cropper-web/src/hooks/useOriginalImage.ts Deleted — absorbed into store
packages/pluggableWidgets/image-cropper-web/src/hooks/usePreviewSrc.ts Deleted — absorbed into store
packages/pluggableWidgets/image-cropper-web/src/hooks/__tests__/useImageCropperState.spec.ts Deleted — replaced by store spec
packages/pluggableWidgets/image-cropper-web/src/hooks/__tests__/useOriginalImage.spec.ts Deleted — replaced by store spec
packages/pluggableWidgets/image-cropper-web/src/hooks/__tests__/usePreviewSrc.spec.ts Deleted — replaced by store spec

Skipped (out of scope): pnpm-lock.yaml

CI check status could not be retrieved in this environment — verify checks are green before merging.


Findings

⚠️ Low — PR title does not follow conventional commits

File: PR metadata
Note: AGENTS.md enforces type(scope): description conventional commits. "MobX crop store for Image Cropper" has no prefix. A title like refactor(image-cropper-web): extract MobX store for crop domain state would satisfy the format.


⚠️ Low — makeImageProp manual mock misses the Loading/Unavailable status guard

File: packages/pluggableWidgets/image-cropper-web/src/stores/__tests__/ImageCropperStore.spec.ts line 1244
Note: makeImageProp constructs the image prop by hand (status, setValue, setThumbnailSize, …) instead of using EditableValueBuilder from @mendix/widget-plugin-test-utils. The checklist flags this as brittle — it won't warn if the Mendix API shape changes. More concretely, the applyCrop guard image.status !== ValueStatus.Available has no test: readOnly and committedCrop undefined are covered, but a Loading or Unavailable image producing a no-op is not. Consider adding one:

it("does nothing when the image is Loading", async () => {
    const { store, gate, dispose } = makeStore();
    gate.setProps(makeProps({ image: makeImageProp({ status: ValueStatus.Loading, value: undefined }) }));
    store.markUserDragged();
    store.commitCrop(PIXEL_CROP);
    await flush();
    expect(cropImage).not.toHaveBeenCalled();
    dispose();
});

And swap makeImageProp for a builder where it fits:

import { EditableValueBuilder } from "@mendix/widget-plugin-test-utils";
const imageValue = new EditableValueBuilder<WebImage>().withValue({ uri: "http://…", name: "img.png" }).build();

Positives

  • makeObservable type-param correctly lists private method names so TypeScript accepts the annotations on private members — not a commonly known requirement, done right.
  • All post-await observable mutations in rotate and captureOriginal are wrapped in runInAction, which is the correct pattern for async MobX actions in this codebase.
  • fetchGeneration token pattern is a clean and testable analog of the cancelled boolean from the old hook; the stale-fetch cancellation test covers it directly.
  • FakeGate in the spec uses observable.ref + action exactly as the real GateProvider does — the store's uri reaction and aspect computed will re-track correctly in tests.
  • fireImmediately: true on the uri reaction correctly handles the initial image without a separate useEffect.
  • The two no-dep useEffect calls in ImageCropperContainer (gate props + deps) are intentional and correct for the mobx-kit GateProvider pattern; the comments explain why.
  • Debounce abort and reaction disposal are both covered in the setup() teardown, matching the blob-revoke cleanup — no leaks left behind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant