[8547] Workflow states options should only display valid options#4808
[8547] Workflow states options should only display valid options#4808jvega190 wants to merge 2 commits intocraftercms:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a new public prop Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
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/WorkflowStateManagement/WorkflowStateManagement.tsx (1)
154-163:⚠️ Potential issue | 🟡 MinorMissing error handler in
fetchPublishingTargetssubscription causes silent failure.If
fetchPublishingTargetsfails (network error, 5xx, etc.),hasStagingsilently staysfalse. On sites with staging actually configured, the staging workflow controls in the dialog and the "staged" filter checkbox will permanently be absent for that page load — a silent UX regression with no indication to the user.🛡️ Suggested fix: add an error handler
useMount(() => { const sub = fetchPublishingTargets(siteId).subscribe({ next({ publishingTargets: targets }) { setHasStaging(targets.some((target) => target.name === 'staging')); - } + }, + error() { + // Fail open: leave hasStaging as false; staging controls remain hidden. + // Optionally dispatch an error notification here if desired. + } }); return () => { sub.unsubscribe(); }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/WorkflowStateManagement/WorkflowStateManagement.tsx` around lines 154 - 163, The subscription to fetchPublishingTargets inside useMount lacks an error handler so failures are silent; update the subscribe call in WorkflowStateManagement (the useMount block that calls fetchPublishingTargets(siteId).subscribe) to provide an error callback that logs the error (e.g., console.error or the app logger) and explicitly sets setHasStaging(false) or otherwise maintains a safe default state, ensuring the returned teardown still unsubscribes the subscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/src/components/WorkflowStateManagement/WorkflowStateManagement.tsx`:
- Around line 154-163: The subscription to fetchPublishingTargets inside
useMount lacks an error handler so failures are silent; update the subscribe
call in WorkflowStateManagement (the useMount block that calls
fetchPublishingTargets(siteId).subscribe) to provide an error callback that logs
the error (e.g., console.error or the app logger) and explicitly sets
setHasStaging(false) or otherwise maintains a safe default state, ensuring the
returned teardown still unsubscribes the subscription.
|
Outside diff range comments addressed |
craftercms/craftercms#8547
Summary by CodeRabbit
New Features
Bug Fixes / Improvements