Conversation
…namicModal When a modal action had both showModalFooter: true (old setting) and footerButtons: 'custom' or 'none' (new setting), the legacy flag would override the explicit configuration, causing duplicate button rows.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughMoves a ConfigMigrations .shaconfig file from removal to embedded resource in the C# project and changes DynamicModal footer-button logic to prefer an explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![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)
shesha-reactjs/src/components/dynamicModal/index.tsx (1)
74-74:⚠️ Potential issue | 🔴 CriticalLogic bug: Default value in destructuring prevents fallback to
showModalFooter.The intent is to use
footerButtonswhen explicitly provided and fall back toshowModalFooterotherwise. However, sincefooterButtons = 'default'is set as a default value in the destructuring (line 74), the truthy checkfooterButtons ?on line 85 will always be true. TheshowModalFooterfallback path will never execute.This breaks backward compatibility for callers that rely on
showModalFooter={true}without explicitly providingfooterButtons.🐛 Proposed fix: Remove default from destructuring and use explicit undefined check
buttons = [], - footerButtons = 'default', + footerButtons, wrapper, showCloseIcon, } = props;Then update the logic to explicitly default when needed:
// `showModalFooter` for now is for backward compatibility // If `footerButtons` is explicitly set, it takes precedence over the legacy `showModalFooter` flag -const showDefaultSubmitButtons = footerButtons ? footerButtons === 'default' : (showModalFooter ?? false); +const showDefaultSubmitButtons = footerButtons !== undefined + ? footerButtons === 'default' + : (showModalFooter ?? false);Additionally, update line 153 to handle
undefined:-<Show when={footerButtons === 'custom' && Boolean(buttons?.length)}> +<Show when={footerButtons === 'custom' && Boolean(buttons?.length)}>This preserves the same behavior for the custom buttons check since
undefined === 'custom'is false.Also applies to: 84-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/dynamicModal/index.tsx` at line 74, Destructured default footerButtons = 'default' prevents falling back to showModalFooter; remove the default in the props destructuring (leave footerButtons possibly undefined) and change the footer selection logic in the component to explicitly check for undefined (e.g., if (footerButtons !== undefined) use footerButtons else use showModalFooter) and keep the existing custom-button check (footerButtons === 'custom') intact so undefined !== 'custom' still works; also update any later usage that assumed footerButtons is always a string to handle undefined appropriately.
🤖 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 `@shesha-reactjs/src/components/dynamicModal/index.tsx`:
- Line 74: Destructured default footerButtons = 'default' prevents falling back
to showModalFooter; remove the default in the props destructuring (leave
footerButtons possibly undefined) and change the footer selection logic in the
component to explicitly check for undefined (e.g., if (footerButtons !==
undefined) use footerButtons else use showModalFooter) and keep the existing
custom-button check (footerButtons === 'custom') intact so undefined !==
'custom' still works; also update any later usage that assumed footerButtons is
always a string to handle undefined appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: afc2cc2f-7707-4ebd-8b0a-8dde4e13fa49
📒 Files selected for processing (3)
shesha-core/src/Shesha.Application/ConfigMigrations/package20260316_1404.shaconfigshesha-core/src/Shesha.Application/Shesha.Application.csprojshesha-reactjs/src/components/dynamicModal/index.tsx
IvanIlyichev
left a comment
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @Lihlu. Please remove usage of globalState in cs-role-editor
2 participants
Summary by CodeRabbit
Bug Fixes
Chores