[8872] Fix XB RTE edits not working - #8887
Conversation
|
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:
WalkthroughThe PR adds shared TinyMCE picker utilities, a guest-host picker protocol, and host-side data-source action selection. The guest inserts returned URLs and names. The host supports direct actions, grouped dialogs, cancellation, and errors. ChangesRTE data-source picker
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuestTinyMCE
participant PreviewConcierge
participant DataSourceActions
GuestTinyMCE->>PreviewConcierge: Request picker with field and file type
PreviewConcierge->>DataSourceActions: Resolve compatible actions
DataSourceActions-->>PreviewConcierge: Return available actions
PreviewConcierge-->>GuestTinyMCE: Return selected URL and name
GuestTinyMCE->>GuestTinyMCE: Insert URL and alt text
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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 `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx`:
- Line 549: Update the request handling around expandPath and processPathMacros
to validate the optional request.objectId before macro expansion. Reject or
explicitly handle requests without an objectId, and only pass a defined objectId
to processPathMacros so missing values never become "undefined" or trigger
{objectGroupId2} failures.
🪄 Autofix
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: 53ace2eb-e378-4c7f-8843-60f4e5d7beaf
📒 Files selected for processing (4)
studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.tsstudio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsxstudio-ui/ui/app/src/state/actions/dialogs.tsstudio-ui/ui/guest/src/controls/rte.ts
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
✅ Action performedFull review finished. |
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 `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx`:
- Line 566: In the grouped-request handling flow around setRteDataSourcePicker,
reply to the currently active picker request with a null result before replacing
its state with the new requestId, actions, and context. Preserve the existing
picker setup for the incoming request so prior subscriptions complete before the
active request is overwritten.
🪄 Autofix
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: ead35ab0-4046-4af2-9ffa-ca4446be4be6
📒 Files selected for processing (4)
studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.tsstudio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsxstudio-ui/ui/app/src/state/actions/dialogs.tsstudio-ui/ui/guest/src/controls/rte.ts
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
✅ Action performedFull review finished. |
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 `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx`:
- Around line 558-569: In the request handling flow around buildActionGroups,
cancel the existing rteDataSourcePicker before branching into direct or grouped
actions by replying with null, then clear its state. Preserve the direct
invokeActionChoice path, and only setRteDataSourcePicker for requests that enter
the grouped-action branch.
🪄 Autofix
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: adab3df1-88ab-4ed0-882e-e97919b16a55
📒 Files selected for processing (4)
studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.tsstudio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsxstudio-ui/ui/app/src/state/actions/dialogs.tsstudio-ui/ui/guest/src/controls/rte.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx (1)
523-526: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake picker cancellation safe for overlapping requests.
openRteDataSourcePickerawaitsresolveFieldDataSourcesbefore checking the render-capturedrteDataSourcePicker. Overlapping requests can therefore replace an active picker without cancelling itsrequestId, or invoke a direct action while an older grouped picker remains open. Track the active picker and a request-generation token in refs, or serialize requests. Ignore stale completions before updating picker state or invoking an action. The guest replaces its previous result subscription, so an uncancelled request can remain in picking state.🤖 Prompt for 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. In `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx` around lines 523 - 526, Make the openRteDataSourcePicker flow safe for overlapping requests by tracking the active picker and request generation in refs (or serializing requests). After resolveFieldDataSources completes, ignore stale completions before updating picker state or invoking any direct action; when replacing or cancelling a picker, cancel its prior requestId so the guest cannot remain in picking state.
🤖 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.
Outside diff comments:
In `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx`:
- Around line 523-526: Make the openRteDataSourcePicker flow safe for
overlapping requests by tracking the active picker and request generation in
refs (or serializing requests). After resolveFieldDataSources completes, ignore
stale completions before updating picker state or invoking any direct action;
when replacing or cancelling a picker, cancel its prior requestId so the guest
cannot remain in picking state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce2f3a3c-9936-4ee0-9c24-2fd0ba576847
📒 Files selected for processing (1)
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx
|
Outside diff range comment addressed |
|
@coderabbitai how does this work look to be merged? Any issues you can point out? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR metadata ---'
gh pr view 8887 --repo craftersoftware/craftercms \
--json number,title,state,isDraft,mergeable,reviewDecision,statusCheckRollup,headRefName,baseRefName \
--jq '{number,title,state,isDraft,mergeable,reviewDecision,headRefName,baseRefName,checks:[.statusCheckRollup[]? | {name,conclusion,status}]}'
echo '--- Changed files ---'
git diff --name-only "origin/develop...HEAD"
echo '--- Final diff ---'
git diff --find-renames --find-copies --unified=80 "origin/develop...HEAD" -- \
studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.ts \
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx \
studio-ui/ui/app/src/state/actions/dialogs.ts \
studio-ui/ui/guest/src/controls/rte.ts
echo '--- Symbol outlines ---'
ast-grep outline studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.ts --items all --view expanded
ast-grep outline studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx --items all --view expanded
ast-grep outline studio-ui/ui/app/src/state/actions/dialogs.ts --items all --view expanded
ast-grep outline studio-ui/ui/guest/src/controls/rte.ts --items all --view expanded
echo '--- Protocol call sites and tests ---'
rg -n -C 4 --glob '*.{ts,tsx}' \
'showRteDataSourcePicker|rteDataSourcePickerResult|rteSelectionToUrl|getRteDataSourcePropertyNames|filePickerCallback' \
studio-uiLength of output: 119035 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Locate related implementation and test files ---'
fd -t f -E node_modules -E dist -E build \
'actionAdapters|GroupedDataSourceActionMenuItems|communicator|iceRegistry|InContextEditing|rte.*test|rte.*spec|PreviewConcierge.*test|PreviewConcierge.*spec' \
studio-ui
echo '--- Action adapter implementation ---'
adapter=$(fd -t f -E node_modules -E dist -E build 'actionAdapters.ts' studio-ui | head -n 1)
[ -n "$adapter" ] && {
ast-grep outline "$adapter" --items all --view expanded
rg -n -C 20 'function invokeActionChoice|const invokeActionChoice|export.*invokeActionChoice|function buildActionGroups|const buildActionGroups' "$adapter"
}
echo '--- Grouped menu implementation ---'
menu=$(fd -t f -E node_modules -E dist -E build 'GroupedDataSourceActionMenuItems.tsx' studio-ui | head -n 1)
[ -n "$menu" ] && {
ast-grep outline "$menu" --items all --view expanded
sed -n '1,280p' "$menu"
}
echo '--- Guest communication implementation ---'
comm=$(fd -t f -E node_modules -E dist -E build 'communicator.ts' studio-ui/ui/guest | head -n 1)
[ -n "$comm" ] && {
ast-grep outline "$comm" --items all --view expanded
sed -n '1,240p' "$comm"
}
echo '--- Referential entry types and content-type lookup ---'
rg -n -C 12 --glob '*.{ts,tsx}' \
'getReferentialEntries\s*[:(]|contentTypeId|function getField|const getField' \
studio-ui/ui/guest/src studio-ui/ui/app/src/utils/contentType.ts
echo '--- Focused test coverage and package scripts ---'
rg -n -C 3 --glob '*.{ts,tsx,json}' \
'getTinyMceInitOptions|initTinyMCE|PreviewConcierge|rteDataSourcePickerResult|showRteDataSourcePicker|\"test\"|\"typecheck\"' \
studio-ui/ui/app studio-ui/ui/guest | head -n 500Length of output: 131169 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Guest picker lifecycle ---'
nl -ba studio-ui/ui/guest/src/controls/rte.ts | sed -n '120,155p;235,280p'
echo '--- Host picker cancellation lifecycle ---'
nl -ba studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx | sed -n '510,625p;1318,1338p;1530,1650p'
echo '--- All host picker cancellation call sites ---'
rg -n -C 3 'cancelRteDataSourcePicker\s*\(|clearRteDataSourcePickerState\s*\(|closeRteDataSourcePicker\s*\(' \
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx
echo '--- Current PR status ---'
gh pr view 8887 --repo craftersoftware/craftercms \
--json isDraft,mergeable,reviewDecision,statusCheckRollup \
--jq '{isDraft,mergeable,reviewDecision,checks:[.statusCheckRollup[]? | {name,conclusion,status,detailsUrl}]}'Length of output: 1190 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered_range() {
local file="$1"
local start="$2"
local end="$3"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo '--- Guest picker lifecycle ---'
numbered_range studio-ui/ui/guest/src/controls/rte.ts 120 155
numbered_range studio-ui/ui/guest/src/controls/rte.ts 235 280
echo '--- Host picker cancellation lifecycle ---'
numbered_range studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx 510 625
numbered_range studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx 1318 1338
numbered_range studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx 1530 1650
echo '--- All host picker cancellation call sites ---'
rg -n -C 3 'cancelRteDataSourcePicker\s*\(|clearRteDataSourcePickerState\s*\(|closeRteDataSourcePicker\s*\(' \
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx
echo '--- Current PR status ---'
gh pr view 8887 --repo craftersoftware/craftercms \
--json isDraft,mergeable,reviewDecision,statusCheckRollup \
--jq '{isDraft,mergeable,reviewDecision,checks:[.statusCheckRollup[]? | {name,conclusion,status,detailsUrl}]}'Length of output: 18040
Please add a guest-to-host cancellation action, or cancel active RTE picker requests when the guest checks out or reloads. Add coverage for selection, dismissal, overlapping requests, and guest teardown. I found no other code-level blocker in the reviewed paths. The PR is still a draft. Travis CI is still in progress. GitHub also reports You are interacting with an AI system. |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="craftersoftware/craftercms"
pr="8887"
echo '--- Pull request status ---'
gh pr view "$pr" --repo "$repo" \
--json number,title,state,isDraft,mergeable,reviewDecision,headRefName,baseRefName,statusCheckRollup \
--jq '{number,title,state,isDraft,mergeable,reviewDecision,headRefName,baseRefName,checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}]}'
echo '--- Changed files ---'
git diff --name-only "origin/develop...HEAD"
echo '--- Final diff ---'
git diff --find-renames --find-copies --unified=60 "origin/develop...HEAD" -- \
studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.ts \
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx \
studio-ui/ui/app/src/state/actions/dialogs.ts \
studio-ui/ui/guest/src/controls/rte.ts
echo '--- Relevant symbol outlines ---'
ast-grep outline studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx --items all --view expanded
ast-grep outline studio-ui/ui/guest/src/controls/rte.ts --items all --view expanded
ast-grep outline studio-ui/ui/app/src/state/actions/dialogs.ts --items all --view expanded
ast-grep outline studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.ts --items all --view expanded
echo '--- Picker protocol, cancellation, and teardown call sites ---'
rg -n -C 5 --glob '*.{ts,tsx}' \
'showRteDataSourcePicker|rteDataSourcePickerResult|cancelRteDataSourcePicker|RteDataSourcePicker|filePickerCallback|focusout|destroy\s*\(' \
studio-ui/ui/app studio-ui/ui/guest
echo '--- Focused test files and test references ---'
fd -t f -E node_modules -E dist -E build \
'*(rte|PreviewConcierge)*(test|spec)*.*' studio-ui || true
rg -n -C 3 --glob '*.{ts,tsx}' \
'showRteDataSourcePicker|rteDataSourcePickerResult|cancelRteDataSourcePicker|getTinyMceInitOptions' \
studio-ui --glob '*.{test,spec}.{ts,tsx}' || trueLength of output: 164475 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Guest communicator ---'
comm=$(fd -t f -E node_modules -E dist -E build '^communicator\.ts$' studio-ui/ui/guest | head -n 1)
if [ -n "$comm" ]; then
ast-grep outline "$comm" --items all --view expanded
sed -n '1,260p' "$comm"
fi
echo '--- TinyMCE option merge and guest picker types ---'
sed -n '150,310p' studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.ts
sed -n '120,205p' studio-ui/ui/guest/src/controls/rte.ts
echo '--- Type declarations for TinyMCE file picker metadata ---'
rg -n -C 4 --glob '*.{ts,d.ts}' \
'file_picker_callback|filetype.*image|interface.*File.*Picker|type.*File.*Picker' \
studio-ui/ui | head -n 300
echo '--- Focused test files ---'
fd -t f -E node_modules -E dist -E build \
--glob '*{rte,Rte,PreviewConcierge}*{test,spec}*.{ts,tsx}' studio-ui || true
echo '--- Existing test conventions for guest controls and PreviewConcierge ---'
fd -t f -E node_modules -E dist -E build --glob '*.{test,spec}.{ts,tsx}' studio-ui/ui/guest studio-ui/ui/app | head -n 160
rg -n -C 3 --glob '*.{test,spec}.{ts,tsx}' \
'initTinyMCE|PreviewConcierge|fromTopic\s*\(|getTinyMceInitOptions' \
studio-ui/ui/guest studio-ui/ui/app || true
echo '--- Package scripts ---'
fd -t f -E node_modules -E dist -E build '^package\.json$' studio-ui | while IFS= read -r file; do
echo "--- $file ---"
jq '{scripts}' "$file"
doneLength of output: 26866
The prior blocking picker-lifecycle issue is addressed. The guest now sends I found no remaining code-level blocker in the changed paths. Before human handover, wait for these conditions:
No focused RTE or ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx (1)
560-620: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd picker lifecycle tests.
Add focused tests for selection, dismissal, overlapping requests, and guest teardown. These paths depend on request IDs and generation invalidation.
🤖 Prompt for 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. In `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx` around lines 560 - 620, Add focused lifecycle tests for openRteDataSourcePicker and its related picker handlers, covering successful selection, dismissal/error responses, overlapping requests with generation invalidation, and guest teardown. Assert request IDs are responded to correctly, stale requests cannot update state or respond, and active picker state is cleared.
🤖 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.
Nitpick comments:
In `@studio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx`:
- Around line 560-620: Add focused lifecycle tests for openRteDataSourcePicker
and its related picker handlers, covering successful selection, dismissal/error
responses, overlapping requests with generation invalidation, and guest
teardown. Assert request IDs are responded to correctly, stale requests cannot
update state or respond, and active picker state is cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5f782e8-67b6-48b0-b595-e706b0ac62fe
📒 Files selected for processing (4)
studio-ui/ui/app/src/components/FormsEngine/lib/rteUtils.tsstudio-ui/ui/app/src/components/PreviewConcierge/PreviewConcierge.tsxstudio-ui/ui/app/src/state/actions/dialogs.tsstudio-ui/ui/guest/src/controls/rte.ts
3 participants
#8872
Summary by CodeRabbit