[8723] Handle errors when trying to paste an item in an invalid location for its content type#4890
[8723] Handle errors when trying to paste an item in an invalid location for its content type#4890jvega190 wants to merge 5 commits into
Conversation
…ion for its content type
WalkthroughThis PR removes the dedicated UI blocker dialog infrastructure from the dialog system and enhances error handling for paste operations. The ChangesUI Error Handling and Dialog Management
Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 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
# Get the full PR diff
gh pr diff 4890 --repo craftercms/studio-uiLength of output: 2375 🏁 Script executed: #!/bin/bash
# Read the full content of the three changed files
echo "=== ErrorState.tsx ==="
cat ui/app/src/components/ErrorState/ErrorState.tsx
echo ""
echo "=== content.ts (epics) ==="
cat ui/app/src/state/epics/content.ts
echo ""
echo "=== mappedDialogs.ts ==="
cat ui/app/src/state/epics/mappedDialogs.tsLength of output: 33895 🏁 Script executed: #!/bin/bash
# Check where extractErrorPayload is used in content.ts
rg -n "extractErrorPayload" ui/app/src/state/epics/content.ts
# Check how blockUI is used elsewhere in the codebase
echo "=== blockUI usages ==="
rg -n "blockUI\|unblockUI" --type ts -C2 ui/app/src/
# Check if there's another handler for blockUI
echo "=== blockUI action/slice ==="
rg -rn "blockUI" --type ts ui/app/src/state/Length of output: 2869
✅ What looks good
|
| File | Status |
|---|---|
ErrorState.tsx |
✅ Good to go |
content.ts |
extractErrorPayload import |
mappedDialogs.ts |
blockUI / unblockUI imports |
Two small cleanups and this is ready for human review. 🐇
|
@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
gh pr diff 4890 --repo craftercms/studio-uiLength of output: 2267 🏁 Script executed: #!/bin/bash
# Check current state of content.ts - look for extractErrorPayload and pasteItem error handling
rg -n "extractErrorPayload\|pasteItem\|pushErrorDialog\|catchAjaxError" ui/app/src/state/epics/content.tsLength of output: 456 🏁 Script executed: #!/bin/bash
# Find the correct path for content.ts epics file
fd "content.ts" ui/app/src/state/epics/
echo "=== Check imports section ==="
head -60 ui/app/src/state/epics/content.ts
echo "=== Check pasteItem area ==="
rg -n "paste" ui/app/src/state/epics/content.ts | head -30Length of output: 2709 🏁 Script executed: #!/bin/bash
# Read the pasteItem epic section
sed -n '410,520p' ui/app/src/state/epics/content.tsLength of output: 3535
The diff is minimal and focused. Ready to hand off for human review. 🐇 |
| // region UIBlocker | ||
| (action$, state$) => | ||
| action$.pipe( | ||
| ofType(blockUI.type), | ||
| withLatestFrom(state$), | ||
| map(([{ payload, type }]) => { | ||
| return pushDialog({ | ||
| id: blockUI.type, | ||
| component: dialogsMap[type], | ||
| props: payload | ||
| }); | ||
| }) | ||
| ), | ||
| (action$, state$) => | ||
| action$.pipe( | ||
| ofType(unblockUI.type), | ||
| withLatestFrom(state$), | ||
| map(() => { | ||
| return popDialog({ id: blockUI.type }); | ||
| }) | ||
| ) | ||
| // end region |
There was a problem hiding this comment.
What's this got to do with paste?
Why is this being removed?
Where is it handled if not here?
There was a problem hiding this comment.
The actions were added to the dialogs system, but we left some existing dialogs in the GlobalDialogManager (ItemMegaMenu, AuthMonitor, Laucher, etc) including the UI Blocker. So it was duplicated
There was a problem hiding this comment.
Haven't gotten an answer to where the UI blocker via dispatch is handled
There was a problem hiding this comment.
The UIBlocker is still in the GlobalDialogManager. When dispatching the action there's a uiBlocker reducer that updates the state (opens the blocker), and the component in the GlobalDialogManager gets the updated state and shows/hides according to the action dispatched
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/state/epics/content.ts`:
- Around line 443-444: The code unconditionally accesses error.response.code at
line 443 without first verifying that error.response exists, which will throw an
exception for network/offline/CORS failures where the response property is
absent, breaking the error handling flow. Add a guard clause to check if
error.response is defined before attempting to access its code property. This
pattern also needs to be fixed at the second location mentioned around line 470.
Use optional chaining or an explicit null/undefined check to safely access the
code property only when the response object exists.
- Around line 457-464: The sourceContentType variable passed to the
formatMessage interpolation in the remedialAction block can be undefined when
the clipboard source item is not found in itemsByPath, causing the message to
display "Content type "undefined"...". Add a fallback value for
sourceContentType before passing it to the formatMessage call using a logical OR
operator or ternary expression to provide a default string like "Unknown" when
sourceContentType is undefined.
🪄 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: 87af99c7-91f7-44d4-8d03-2d22de7c19b8
📒 Files selected for processing (2)
ui/app/src/state/epics/content.tsui/app/src/state/epics/mappedDialogs.ts
💤 Files with no reviewable changes (1)
- ui/app/src/state/epics/mappedDialogs.ts
…improve error payload extraction
|
@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
# Check the current PR status and latest commits
gh pr view 4890 --repo craftercms/studio-ui --json commits,headRefName,updatedAt | jq '.commits[-3:], .updatedAt'
# Get current diff to verify state
gh pr diff 4890 --repo craftercms/studio-uiLength of output: 5875 🏁 Script executed: #!/bin/bash
echo "=== AjaxError import check in content.ts ==="
head -55 ui/app/src/state/epics/content.ts | grep -n "AjaxError\|ajax\|rxjs"
echo ""
echo "=== remedialAction usage across codebase ==="
rg -n "remedialAction" --type ts --type tsx -C3
echo ""
echo "=== ErrorDialog / pushErrorDialog props type ==="
rg -n "remedialAction\|ErrorDialogProps\|ErrorDialogStateProps" --type ts -C2 ui/app/src/
echo ""
echo "=== Check ErrorState component for remedialAction ==="
cat ui/app/src/components/ErrorState/ErrorState.tsxLength of output: 4138 🏁 Script executed: #!/bin/bash
echo "=== Full imports section of content.ts ==="
sed -n '1,60p' ui/app/src/state/epics/content.ts
echo ""
echo "=== remedialAction in all files ==="
rg -rn "remedialAction" ui/app/src/ -C3
echo ""
echo "=== ErrorDialog component and its props ==="
fd "ErrorDialog" ui/app/src/ --extension ts --extension tsx
echo ""
echo "=== pushErrorDialog action definition ==="
rg -rn "pushErrorDialog" ui/app/src/state/actions/ -C3
echo ""
echo "=== Error type/interface used in dialogs ==="
rg -rn "interface.*Error\b|type.*Error\b" ui/app/src/models/ -C2Length of output: 10996 🏁 Script executed: #!/bin/bash
echo "=== ErrorDialog component full content ==="
cat ui/app/src/components/ErrorDialog/ErrorDialog.tsx
echo ""
echo "=== ApiResponse model ==="
cat ui/app/src/models/ApiResponse.ts
echo ""
echo "=== AjaxError import check — more of content.ts imports ==="
sed -n '55,80p' ui/app/src/state/epics/content.ts
echo ""
echo "=== extractErrorPayload function signature ==="
rg -n "extractErrorPayload" ui/app/src/utils/ajax.ts -A10Length of output: 5006 🏁 Script executed: #!/bin/bash
echo "=== Search for AjaxError import in content.ts ==="
rg -n "AjaxError" ui/app/src/state/epics/content.ts
echo ""
echo "=== ApiResponseErrorState component — how it uses remedialAction ==="
cat ui/app/src/components/ApiResponseErrorState/ApiResponseErrorState.tsxLength of output: 3783
The two new commits refined the error handling nicely — the optional chaining and the use of |
| textAlign: 'center', | ||
| marginBottom: (theme) => theme.spacing(1), | ||
| wordBreak: 'break-all', | ||
| wordBreak: 'break-word', |
There was a problem hiding this comment.
This style might have been in place for super long exceptions. Why change?
There was a problem hiding this comment.
break-all causes it to break at any point of the text, causing it to look like this:
by setting it to break-word, it won't split words, like this:
| // region UIBlocker | ||
| (action$, state$) => | ||
| action$.pipe( | ||
| ofType(blockUI.type), | ||
| withLatestFrom(state$), | ||
| map(([{ payload, type }]) => { | ||
| return pushDialog({ | ||
| id: blockUI.type, | ||
| component: dialogsMap[type], | ||
| props: payload | ||
| }); | ||
| }) | ||
| ), | ||
| (action$, state$) => | ||
| action$.pipe( | ||
| ofType(unblockUI.type), | ||
| withLatestFrom(state$), | ||
| map(() => { | ||
| return popDialog({ id: blockUI.type }); | ||
| }) | ||
| ) | ||
| // end region |
There was a problem hiding this comment.
Haven't gotten an answer to where the UI blocker via dispatch is handled
2 participants
The error handler was correct, but the block/unblock UI actions were added to the new dialogs system, so the showing/removal wasn't working as expected in this scenario (and probably others).
Added error handling for this scenario, for the error message to be clear to the user.
This PR craftercms/studio#3956 is needed for the error code handling to work properly.
craftercms/craftercms#8723
Summary by CodeRabbit
Summary by CodeRabbit
Style
Bug Fixes
Refactor