[8685] Fix issues when saving, and repeat group validator#4905
[8685] Fix issues when saving, and repeat group validator#4905jvega190 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFormsEngine now uses ChangesFormsEngine save path and validator fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/src/components/FormsEngine/FormsEngine.tsx (1)
908-914: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHeader still displays the raw, unresolved
create?.path.The save flow now derives
createPathfrompathInSite(the macro-resolved path), per the comment on Line 816, butCreateModeHeaderat Line 911 still receives the rawcreate?.path. If the configured create path contains macros, the header will show the unresolved macro string while the form actually saves to the resolvedpathInSite, creating a discrepancy between what's displayed and where the content lands.💡 Suggested fix
- <CreateModeHeader path={create?.path} collapse={collapseHeader} /> + <CreateModeHeader path={pathInSite} collapse={collapseHeader} />🤖 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 `@ui/app/src/components/FormsEngine/FormsEngine.tsx` around lines 908 - 914, The CreateModeHeader is still rendering the unresolved create path, causing a mismatch with the macro-resolved save target. Update the create-mode branch in FormsEngine so it passes the resolved create path derived from pathInSite instead of create?.path, keeping the displayed header aligned with the actual destination used by the save flow. Use the existing createPath/pathInSite logic in FormsEngine and ensure CreateModeHeader receives the resolved value.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/FormsEngine.tsx (1)
811-819: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
isCreateModeinstead of recomputing the same condition.
isCreateMode(Line 634) is alreadyBoolean(create?.path). Recomputing it inline here is redundant.♻️ Proposed simplification
- createPath: Boolean(create?.path) ? pathInSite : undefined, // pathInSite is the result of processing the create path with macros. + createPath: isCreateMode ? pathInSite : undefined, // pathInSite is the result of processing the create path with macros.🤖 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 `@ui/app/src/components/FormsEngine/FormsEngine.tsx` around lines 811 - 819, `FormsEngine` is recomputing the create-mode check inline when building `saveFn`, even though `isCreateMode` already captures `Boolean(create?.path)`. Update the `useSaveForm` call to pass `isCreateMode` for `createPath` instead of repeating the condition, keeping the logic centralized and consistent with the existing `isCreateMode` value.
🤖 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 `@ui/app/src/components/FormsEngine/FormsEngine.tsx`:
- Around line 908-914: The CreateModeHeader is still rendering the unresolved
create path, causing a mismatch with the macro-resolved save target. Update the
create-mode branch in FormsEngine so it passes the resolved create path derived
from pathInSite instead of create?.path, keeping the displayed header aligned
with the actual destination used by the save flow. Use the existing
createPath/pathInSite logic in FormsEngine and ensure CreateModeHeader receives
the resolved value.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/FormsEngine.tsx`:
- Around line 811-819: `FormsEngine` is recomputing the create-mode check inline
when building `saveFn`, even though `isCreateMode` already captures
`Boolean(create?.path)`. Update the `useSaveForm` call to pass `isCreateMode`
for `createPath` instead of repeating the condition, keeping the logic
centralized and consistent with the existing `isCreateMode` value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b1be6a4-ef45-4b4c-9da0-3c5673e9ddf2
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/FormsEngine.tsxui/app/src/components/FormsEngine/lib/validators.ts
|
Outside diff range comment addressed |
|
@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
gh pr view 4905 --repo craftercms/studio-ui --json title,body,commits,files,additions,deletions,changedFiles
echo "----DIFF----"
gh pr diff 4905 --repo craftercms/studio-uiLength of output: 4869 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd $(git rev-parse --show-toplevel 2>/dev/null || pwd) || true
echo "---- getValidationValue / getPropertyValue defs ----"
rg -n "export function getValidationValue|export function getPropertyValue" ui/app/src/components/FormsEngine/lib/validators.ts -A 8
echo "---- pathInSite in ItemMetaContext ----"
rg -n "pathInSite" ui/app/src/components/FormsEngine/FormsEngine.tsx -B3 -A3
echo "---- ItemMetaContext type def ----"
rg -n "ItemMetaContext" ui/app/src/components/FormsEngine -l
echo "---- usage of minOccurs/maxOccurs elsewhere ----"
rg -n "minOccurs|maxOccurs" ui/app/src/components/FormsEngine -g '*.ts*'Length of output: 1006 🏁 Script executed: #!/bin/bash
set -e
pwd
find / -maxdepth 3 -iname "studio-ui*" 2>/dev/null
lsLength of output: 439 🏁 Script executed: #!/bin/bash
set -e
cd /home/jailuser/git
echo "---- getValidationValue / getPropertyValue defs ----"
rg -n "export function getValidationValue|export function getPropertyValue|function getValidationValue|function getPropertyValue" ui/app/src/components/FormsEngine -A 10
echo "---- pathInSite in FormsEngine.tsx ----"
rg -n "pathInSite" ui/app/src/components/FormsEngine/FormsEngine.tsx -B5 -A5
echo "---- ItemMetaContext def ----"
rg -n "ItemMetaContext" ui/app/src/components/FormsEngine -l
echo "---- minOccurs/maxOccurs usage ----"
rg -n "minOccurs|maxOccurs" ui/app/src/components/FormsEngineLength of output: 8046 🏁 Script executed: #!/bin/bash
set -e
cd /home/jailuser/git
echo "---- imports in validators.ts ----"
rg -n "^import|getPropertyValue|getValidationValue" ui/app/src/components/FormsEngine/lib/validators.ts
echo "---- check other usages of getPropertyValue in validators.ts file (ensure still used elsewhere or remove unused import) ----"
rg -n "getPropertyValue" ui/app/src/components/FormsEngine/lib/validators.tsLength of output: 3246 Both changes look clean and consistent with how validations are used elsewhere in the file (e.g.,
One minor, non-blocking note: the inline comment I don't see any functional issues — this looks ready for human review. [approve] ᕦ(ᐛ)ᕤ |
1 participant
craftercms/craftercms#8685
Summary by CodeRabbit