[8023] Image cropper dialog does not open when using XB#4843
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImage restriction validation is centralized and wired into editor, preview, and guest upload flows. New dialog actions and subtitle rendering support opening the image editor on validation failure, and edited images are uploaded through the guest epic flow. ChangesImage restriction validation and editor upload flow
Sequence Diagram(s)sequenceDiagram
actor User
participant PreviewConcierge
participant ExperienceBuilder
participant GuestEpic
participant validateImageRestrictions
participant ImageEditorDialog
participant uploadDataUrl
User->>PreviewConcierge: request image edit
PreviewConcierge->>ImageEditorDialog: open dialog with restrictions
User->>ImageEditorDialog: crop or cancel
ImageEditorDialog->>ExperienceBuilder: imageEdited / imageEditCancelled
ExperienceBuilder->>GuestEpic: dispatch action
GuestEpic->>validateImageRestrictions: validate path or dataUrl
alt restrictions fail
validateImageRestrictions-->>GuestEpic: false
GuestEpic->>ImageEditorDialog: show editor flow
else restrictions pass
validateImageRestrictions-->>GuestEpic: true
GuestEpic->>uploadDataUrl: upload image or edited blob
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx`:
- Around line 1133-1165: Add an onClose/cancel path to the ImageEditorDialog
push so cancelling the editor clears the dialog and notifies the guest to stop
the pending upload; specifically, in the showImageEditorDialog.type branch
(where pushDialog and createComponentId('ImageEditorDialog') are used and onCrop
currently sends hostToGuest$.next with imageEdited.type and then
dispatch(popDialog({ id }))), add an onClose/onCancel prop that dispatches
popDialog({ id }) and sends a hostToGuest$.next message (e.g.,
imageEditCancelled.type or a desktopAssetUploadFailed/ended message) with the
original fileName/recordId/uploadPath so the guest side can clean up the started
upload/drag state. Ensure the new message type aligns with the guest epic's
expected cancellation/failure event.
In `@ui/app/src/state/actions/dialogs.ts`:
- Around line 395-397: Update the action payload typings to match actual usage:
widen showImageEditorDialog (created by createAction in the symbol
showImageEditorDialog) from Partial<ImageEditorDialogBaseProps> to include
fileName, recordId, and uploadPath (or replace with a new interface that extends
ImageEditorDialogBaseProps plus these fields) so callers in the guest epic no
longer produce excess property errors; likewise broaden imageEdited (created by
createAction in the symbol imageEdited) from { blob: Blob; path?: string | null
} to include newPath, fileName, recordId, and uploadPath (or define and use a
richer ImageEditedPayload interface) so the host/guest flow and the
destructuring in PreviewConcierge.tsx align with the declared contract.
In `@ui/guest/src/store/epics/root.ts`:
- Around line 293-302: Remove the premature desktopAssetUploadStarted emission
from the branches that call showImageEditorDialog (the block posting
showImageEditorDialog with path/restrictions/fileName/recordId/uploadPath) and
instead dispatch the action that ends the drag state (the existing
desktop-drag-end action used elsewhere in this file) so the UI exits drag mode
when the editor opens; then add desktopAssetUploadStarted({ record }) into the
imageEdited epic's upload path so the upload-start is emitted only when the user
actually commits the crop.
- Around line 538-541: The policy validation is constructing the target path
using blob.name but blob is a Blob (no .name) causing `/undefined`; update the
call to validateActionPolicy to use the carried imageFileName from the action
payload instead of blob.name (i.e. target:
ensureSingleSlash(`${path}/${imageFileName}`)), keeping state.activeSite and
ensureSingleSlash as-is so the policy target is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b653b4a-75cb-4bba-a58d-1b413f55d4f4
📒 Files selected for processing (7)
ui/app/src/components/FormsEngine/controls/ImagePicker.tsxui/app/src/components/PreviewConcierge/PreviewConcierge.tsxui/app/src/services/content.tsui/app/src/state/actions/dialogs.tsui/app/src/utils/content.tsui/guest/src/react/ExperienceBuilder.tsxui/guest/src/store/epics/root.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/utils/content.ts`:
- Around line 1304-1307: The current validateImageRestrictions function bypasses
validation when isImage(path) returns false, but isImage only checks file
extensions so blob: and data:image/ URLs (created by URL.createObjectURL and
FileReader.readAsDataURL) slip through; update validateImageRestrictions to
treat blob: and data:image/* URLs as images by checking path.startsWith('blob:')
or path.startsWith('data:image/') (or extend isImage to include those schemes),
and only skip validation for non-image schemes; ensure the dimension/crop checks
still run for blob/data URLs when restrictions are provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fafcfab-76f3-4ecb-8ce3-daa48bd39bf4
📒 Files selected for processing (1)
ui/app/src/utils/content.ts
…o bugfix/8023 # Conflicts: # ui/guest/src/react/ExperienceBuilder.tsx
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Diff ==="
gh pr diff 4843 --repo craftercms/studio-ui
echo ""
echo "=== PR Files Changed ==="
gh pr view 4843 --repo craftercms/studio-ui --json files --jq '.files[].path'Length of output: 29794
🐛 Bug 1 (Critical) —
|
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Files Changed ==="
gh pr view 4843 --repo craftercms/studio-ui --json files --jq '.files[].path'
echo ""
echo "=== Current PR Diff ==="
gh pr diff 4843 --repo craftercms/studio-uiLength of output: 34071
✅ Fixed from previous review
🐛 Still unresolved — Critical
In // `iceRecord` is declared above (line ~264), but `record` never is
post(
showImageEditorDialog({
path,
restrictions,
fileName: name,
recordId: record.id, // 🐛 ReferenceError at runtime — `record` is not in scope
uploadPath
})
);Fix: recordId: iceRecord.id, // ✅
|
| Item | Status |
|---|---|
record.id → iceRecord.id in PLACING_DETACHED_ASSET |
❌ Still needs fix |
ImageRestrictionSubtitle with undefined restrictions guard |
|
| Blob URL leaks | ✅ Fixed |
| Subtitle conditional messages | ✅ Fixed |
createFileUpload blob fallback |
✅ Correct |
imageEditCancelled epic |
✅ Correct |
imageEdited epic structure |
✅ Correct |
| XB whitelist entries | ✅ Correct |
One-line fix remaining before this is ready for human review.
|
@coderabbitai about this item |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR Diff ==="
gh pr diff 4843 --repo craftercms/studio-ui
echo ""
echo "=== root.ts PLACING_DETACHED_ASSET section ==="
gh api repos/craftercms/studio-ui/contents/ui/guest/src/store/epics/root.ts \
-H "Accept: application/vnd.github.raw" \
--jq '.' 2>/dev/null | head -n 600 || trueLength of output: 33815
✅ Final Review — All Clear
The PR looks good to hand over for human review. 🐇 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current diff ==="
gh pr diff 4843 --repo craftercms/studio-ui
echo ""
echo "=== PLACING_DETACHED_ASSET block in root.ts ==="
gh api repos/craftercms/studio-ui/contents/ui/guest/src/store/epics/root.ts \
-H "Accept: application/vnd.github.raw" \
--jq . 2>/dev/null | head -400 || trueLength of output: 33807
✅ Final Review — Everything looks good
The PR is ready for human review. 🐇 ✨ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/app/src/components/FormsEngine/lib/controlHelpers.tsx`:
- Line 405: The ImageRestrictionSubtitle component is being rendered
unconditionally at line 405 even though the restrictions parameter can be
undefined based on the showImageCropDialog function signature at line 393. Since
ImageRestrictionSubtitle dereferences restrictions at lines 447-449, this will
cause a runtime error when restrictions is not provided. Add a conditional check
to only render the subtitle property with ImageRestrictionSubtitle when
restrictions is defined, otherwise render nothing or null for the subtitle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4535ad13-2fad-4617-bc86-d242d79546ef
📒 Files selected for processing (8)
ui/app/src/components/FormsEngine/lib/controlHelpers.tsxui/app/src/components/ImageEditorDialog/ImageEditorDialogContainer.tsxui/app/src/components/PreviewConcierge/PreviewConcierge.tsxui/app/src/services/content.tsui/app/src/state/actions/dialogs.tsui/app/src/utils/content.tsui/guest/src/react/ExperienceBuilder.tsxui/guest/src/store/epics/root.ts
💤 Files with no reviewable changes (2)
- ui/app/src/state/actions/dialogs.ts
- ui/app/src/utils/content.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/app/src/services/content.ts
- ui/guest/src/react/ExperienceBuilder.tsx
- ui/guest/src/store/epics/root.ts
…trictions presence
|
@coderabbitai, don't see your review of d003023. Please look through this PR and report findings and if ready for merge |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="craftercms/studio-ui"
pr="4843"
echo "=== PR metadata ==="
gh pr view "$pr" --repo "$repo" --json number,title,headRefName,baseRefName,author,mergeStateStatus,reviewDecision,statusCheckRollup,commits \
--jq '{
number,
title,
headRefName,
baseRefName,
author: .author.login,
mergeStateStatus,
reviewDecision,
checks: (.statusCheckRollup // [] | map({name: .name, status: .status, conclusion: .conclusion})),
commits: [.commits[].oid[0:7]]
}'
echo
echo "=== PR commit list ==="
gh pr view "$pr" --repo "$repo" --json commits \
--jq '.commits[] | {oid: .oid, messageHeadline: .messageHeadline, committedDate: .committedDate}'
echo
echo "=== Specific commit d003023 (if present) ==="
gh api "repos/$repo/commits/d003023" --jq '{sha: .sha, commit: .commit.message, files: [.files[].filename]}' || true
echo
echo "=== Patch for commit d003023 ==="
gh api "repos/$repo/commits/d003023" -H "Accept: application/vnd.github.patch" | sed -n '1,260p' || true
echo
echo "=== Current PR files changed ==="
gh pr view "$pr" --repo "$repo" --json files --jq '.files[].path'
echo
echo "=== Current PR diff ==="
gh pr diff "$pr" --repo "$repo" | sed -n '1,420p'
echo
echo "=== Outline: ui/guest/src/store/epics/root.ts ==="
ast-grep outline ui/guest/src/store/epics/root.ts --view expanded || true
echo
echo "=== root.ts relevant slice ==="
sed -n '180,520p' ui/guest/src/store/epics/root.ts || true
echo
echo "=== PreviewConcierge relevant slice ==="
sed -n '1,260p' ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx || true
echo
echo "=== controlHelpers relevant slice ==="
sed -n '1,260p' ui/app/src/components/FormsEngine/lib/controlHelpers.tsx || true
echo
echo "=== ImageEditorDialogContainer relevant slice ==="
sed -n '1,220p' ui/app/src/components/ImageEditorDialog/ImageEditorDialogContainer.tsx || true
echo
echo "=== utils/content.ts relevant slice ==="
sed -n '1,260p' ui/app/src/utils/content.ts || true
echo
echo "=== services/content.ts relevant slice ==="
sed -n '1,220p' ui/app/src/services/content.ts || true
echo
echo "=== dialogs.ts relevant slice ==="
sed -n '1,220p' ui/app/src/state/actions/dialogs.ts || true
echo
echo "=== ExperienceBuilder relevant slice ==="
sed -n '1,260p' ui/guest/src/react/ExperienceBuilder.tsx || trueLength of output: 102624
What I checked
Findings
Merge readinessFrom a code review perspective, this looks ready for merge. That said, the PR is currently not mergeable yet because GitHub reports:
So my final status is:
If you want, I can also do a fresh formal PR pass now. ✅ Action performedReview finished.
|
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review. Please do not consider the state of TravisCI |
|
Final review summary:
So, from a code review perspective, ✅ Action performedFull review finished. |
2 participants
craftercms/craftercms#8023
Summary by CodeRabbit