IK-2461 Distort perspective improvements#8
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e33b570b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| const { distort, distortType, distortPerspective } = value | ||
| if (distort && distortType === "perspective" && distortPerspective) { | ||
| const perspective: PerspectiveObject = structuredClone(distortPerspective) |
There was a problem hiding this comment.
Replace structuredClone in perspective validation
The new validatePerspectiveDistort path now calls structuredClone before any checks, which will throw in runtimes that do not implement that API (for example older Safari/WebView builds and older Node-based test environments). In those environments, enabling perspective distortion causes validation to crash instead of returning form errors, so users cannot apply the transformation.
Useful? React with 👍 / 👎.
| Number(val) > field.fieldProps.max | ||
| Number(numSafeVal) > field.fieldProps.max | ||
| ) { | ||
| setValue(field.name, field.fieldProps.max) |
There was a problem hiding this comment.
Keep clamped text-slider values as strings
When a slider value exceeds max, this branch writes a number (field.fieldProps.max) into form state, but distortArcDegree now validates as a string regex. If validation runs before blur normalization (for example via immediate submit/trigger), the resolver reports a type error and blocks applying an otherwise valid clamped value. The min branch already stringifies, so max should be handled consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Improves distort-perspective handling and related UI/schema behaviors, including new perspective validation logic and several formatting/typing cleanups across the editor.
Changes:
- Added/updated schema handling for distort perspective + arc degree inputs (including support for
N-prefixed negatives). - Introduced
validatePerspectiveDistortand wired it into schema refinements. - Updated several UI components (slider behavior, zoom defaults, typed error props, lint suppressions) and reformatted code for consistency.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/imagekit-editor-dev/src/schema/transformation.ts | Refactors validators and tightens typings for refineUnsharpenMask and optional float preprocessing. |
| packages/imagekit-editor-dev/src/schema/index.ts | Updates transformation schemas/formatters, adds perspective validation helper, and changes distort arc to slider + string-based schema. |
| packages/imagekit-editor-dev/src/components/toolbar/toolbar.tsx | Adds Biome suppression comment for non-null assertion on image metadata access. |
| packages/imagekit-editor-dev/src/components/sidebar/transformation-config-sidebar.tsx | Adds typed error props for custom inputs; updates slider/input handling (incl. N negatives) and defaultValue plumbing. |
| packages/imagekit-editor-dev/src/components/sidebar/sortable-transformation-item.tsx | Formatting-only changes and minor handler cleanup. |
| packages/imagekit-editor-dev/src/components/header/index.tsx | Adds Biome suppression comments for non-null assertion usage. |
| packages/imagekit-editor-dev/src/components/editor/ListView.tsx | Adds Biome suppression comment for non-null assertion on image metadata access. |
| packages/imagekit-editor-dev/src/components/editor/GridView.tsx | Adds Biome suppression comment for non-null assertion on image metadata access. |
| packages/imagekit-editor-dev/src/components/common/ZoomInput.tsx | Refactors zoom math and hook usage; loosens defaultValue typing. |
| packages/imagekit-editor-dev/src/components/common/RadioCardField.tsx | Uses Chakra As typing for icons; adds a11y lint suppression comment. |
| packages/imagekit-editor-dev/src/components/common/PaddingInput.tsx | Introduces typed error structures and refactors formatting/props; adds a11y lint suppression comments. |
| packages/imagekit-editor-dev/src/components/common/Hover.tsx | Minor formatting and event listener string normalization. |
| packages/imagekit-editor-dev/src/components/common/GradientPicker.tsx | Formatting refactor; adds error logging when gradient parsing fails. |
| packages/imagekit-editor-dev/src/components/common/DistortPerspectiveInput.tsx | Rebuilds perspective input UI and introduces typed errors; normalizes input to uppercase. |
| packages/imagekit-editor-dev/src/components/common/CornerRadiusInput.tsx | Introduces typed error structures and refactors formatting/props; adds a11y lint suppression comments. |
| packages/imagekit-editor-dev/src/components/common/ColorPickerField.tsx | Imports ColorPickerProps as a type-only import. |
| packages/imagekit-editor-dev/src/components/common/CheckboxCardField.tsx | Uses Chakra As typing for icons; adds a11y lint suppression comments. |
| packages/imagekit-editor-dev/src/components/RetryableImage.tsx | Adjusts effect dependencies suppression and ref typing; changes beginLoad(0) to beginLoad(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| field.fieldProps?.inputType || | ||
| field.fieldProps?.autoOption | ||
| ? "text" | ||
| : "number" |
There was a problem hiding this comment.
This ternary condition has incorrect operator precedence: field.fieldProps?.inputType being any non-empty string makes the whole condition truthy, forcing the input type to always be \"text\" even if inputType was intended to be used (e.g., \"number\"). Parenthesize the condition and/or explicitly prioritize inputType (e.g., field.fieldProps?.inputType ?? (field.fieldProps?.autoOption ? \"text\" : \"number\")).
| field.fieldProps?.inputType || | |
| field.fieldProps?.autoOption | |
| ? "text" | |
| : "number" | |
| field.fieldProps?.inputType ?? | |
| (field.fieldProps?.autoOption ? "text" : "number") |
| const perspective: PerspectiveObject = JSON.parse(JSON.stringify(distortPerspective)) | ||
| const coords = Object.keys(perspective).reduce( | ||
| (acc, key) => { | ||
| const value = perspective[key as keyof typeof perspective] | ||
| if (!value) { | ||
| acc[key as keyof PerspectiveObject] = value | ||
| } | ||
| const numString = value.toUpperCase().replace(/^N/, "-") | ||
| acc[key as keyof PerspectiveObject] = parseInt(numString as string, 10) | ||
| return acc | ||
| }, |
There was a problem hiding this comment.
In the reducer, the if (!value) branch doesn't short-circuit, so empty strings still go through toUpperCase() and parseInt(), producing NaN and overwriting the earlier assignment. If the intent is to allow partial input without validation, you should return acc (or continue-equivalent) when value is empty, and only parse when value is non-empty.
| distortArcDegree: z | ||
| .string() | ||
| .regex(/^[-N]?\d+$/) | ||
| .optional(), |
There was a problem hiding this comment.
The previous numeric validation for distortArcDegree enforced a range (e.g., min/max). After switching to a string+regex, the schema no longer prevents out-of-range values like 9999. Consider adding a .refine(...) that parses the value (including N -> negative) and enforces the supported range.
| .optional(), | |
| .optional() | |
| .refine( | |
| (val) => { | |
| // Allow undefined due to .optional() | |
| if (val === undefined) { | |
| return true | |
| } | |
| // Handle "N" prefix as an alternative negative sign | |
| const isNNegative = val.startsWith("N") | |
| const isDashNegative = val.startsWith("-") | |
| const unsignedPart = isNNegative || isDashNegative ? val.slice(1) : val | |
| const parsed = Number.parseInt(unsignedPart, 10) | |
| if (Number.isNaN(parsed)) { | |
| return false | |
| } | |
| const signedValue = | |
| isNNegative || isDashNegative ? -parsed : parsed | |
| // Enforce supported range for arc distortion degrees | |
| return signedValue >= -360 && signedValue <= 360 | |
| }, | |
| { | |
| message: | |
| "distortArcDegree must be between -360 and 360 degrees", | |
| }, | |
| ), |
|
|
||
| export type PerspectiveErrors = Record<string, CornerErrors> | ||
|
|
||
| type DistorPerspectiveFieldProps = { |
There was a problem hiding this comment.
Typo in type name: DistorPerspectiveFieldProps should be DistortPerspectiveFieldProps for consistency with DistortPerspectiveInput and to avoid propagating the misspelling across imports/usages.
No description provided.