Open
Conversation
- Updated getGhostStyleOverrides to accept fontStyles for better customization. - Adjusted ghost button style handling in various components to utilize the new fontStyles parameter. - Modified DynamicModal to ensure the closable property defaults to true for backward compatibility.
…ation logic - Updated getGhostStyleOverrides to accept optional fontStyles for enhanced customization. - Refined ghost button style application in AdvancedFilterButton component. - Cleaned up formatting in DynamicModalProvider's migration logic for better readability.
- Ensured the closable property in DynamicModal respects the showCloseIcon prop. - Improved getGhostStyleOverrides to conditionally apply color from fontStyles for better customization.
- Updated getGhostStyleOverrides to use a default color of '#000' when fontStyles.color is not provided, enhancing style consistency.
Contributor
WalkthroughUpdated ghost-button styling by changing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsx (1)
103-106:⚠️ Potential issue | 🟡 MinorPotential null value passed to
doFetchItems.
getUrlFromValue(props.value)can returnnull(line 72-73), butdebouncedFetchItemsis called unconditionally. This passesnulltodoFetchItems(term: string, verb: string), which expects astringforterm. While TypeScript may not flag this at compile time due to the intermediate debounced callback, passingnullto the API query could cause unexpected behavior.Consider adding a guard consistent with
handleSearch:🛡️ Proposed fix to add null check
useEffect(() => { const url = getUrlFromValue(props.value); - debouncedFetchItems(url, currentVerb); + if (url) { + debouncedFetchItems(url, currentVerb); + } }, [currentVerb]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsx` around lines 103 - 106, getUrlFromValue(props.value) can return null but the effect always calls debouncedFetchItems, which ultimately passes a null term into doFetchItems(term: string, verb: string); update the useEffect to guard like handleSearch does: compute const url = getUrlFromValue(props.value) and only call debouncedFetchItems(url, currentVerb) when url is non-null (or fallback to an empty string if that’s intended), referencing getUrlFromValue, debouncedFetchItems, doFetchItems, handleSearch, props.value, and currentVerb to locate the change.shesha-reactjs/src/designer-components/button/configurableButton/index.tsx (1)
82-107: 🧹 Nitpick | 🔵 TrivialConsider simplifying style composition to avoid redundant spread.
For both ghost and non-ghost cases,
props.styleis spread twice in the final style object:
- At line 103:
...props?.style- At line 105:
...ghostOverrides(which containsprops.stylefor non-ghost, or{...props.style, ...}for ghost)While functionally harmless (same values overwrite), this is redundant. Consider simplifying:
♻️ Optional: Simplify style composition
- // Ghost buttons: only foreground color, no background/border/shadow - const ghostOverrides = isGhostType ? getGhostStyleOverrides(props.style) : props.style; + // Ghost buttons: only foreground color, no background/border/shadow + const ghostOverrides = isGhostType ? getGhostStyleOverrides(props.style) : {}; return ( <Button ... style={{ ...props?.style, ...(isSameUrl && !isGhostType && { background: theme.application.primaryColor, color: theme.text.default }), ...ghostOverrides, ...(buttonDisabled && { pointerEvents: "none" }), }}This preserves the original behavior where non-ghost buttons don't re-spread
props.styleviaghostOverrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/button/configurableButton/index.tsx` around lines 82 - 107, The style composition redundantly spreads props.style twice; update the style merge in the ConfigurableButton render so props.style is only included once by making ghostOverrides represent only the ghost-specific overrides (use getGhostStyleOverrides(props.style) to return just the extra rules) or by conditionally spreading either props.style or ghostOverrides but not both; adjust references in the JSX where ghostOverrides is used and ensure the existing logic for isSameUrl, buttonDisabled, and theme.application.primaryColor remains applied to the final style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@shesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsx`:
- Around line 91-93: The doFetchItems function calls endpointsFetcher.refetch
which returns a Promise but currently has no error handling; restore the pattern
used in referenceListAutocomplete by appending a .catch to
endpointsFetcher.refetch({ queryParams: { term, verb } }) that logs the error
(e.g., console.error('Failed to fetch items', error)) and rethrows the error to
avoid unhandled promise rejections and preserve upstream error behavior.
- Around line 135-141: The change removed verb validation causing API calls with
null/undefined verbs; restore the isValidVerb helper and add checks before
calling debouncedFetchItems/refetch: update doFetchItems to validate its verb
argument with isValidVerb before calling refetch, update the useEffect that
calls debouncedFetchItems(currentVerb) to only call when
isValidVerb(currentVerb) is true (and fall back to props.httpVerb validation),
and change handleSearch to check isValidVerb(currentVerb) before invoking
debouncedFetchItems(localValue, currentVerb); reference functions/vars:
isValidVerb, doFetchItems, debouncedFetchItems, refetch, useEffect,
getVerbFromValue, handleSearch, currentVerb, props.httpVerb.
---
Outside diff comments:
In
`@shesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsx`:
- Around line 103-106: getUrlFromValue(props.value) can return null but the
effect always calls debouncedFetchItems, which ultimately passes a null term
into doFetchItems(term: string, verb: string); update the useEffect to guard
like handleSearch does: compute const url = getUrlFromValue(props.value) and
only call debouncedFetchItems(url, currentVerb) when url is non-null (or
fallback to an empty string if that’s intended), referencing getUrlFromValue,
debouncedFetchItems, doFetchItems, handleSearch, props.value, and currentVerb to
locate the change.
In `@shesha-reactjs/src/designer-components/button/configurableButton/index.tsx`:
- Around line 82-107: The style composition redundantly spreads props.style
twice; update the style merge in the ConfigurableButton render so props.style is
only included once by making ghostOverrides represent only the ghost-specific
overrides (use getGhostStyleOverrides(props.style) to return just the extra
rules) or by conditionally spreading either props.style or ghostOverrides but
not both; adjust references in the JSX where ghostOverrides is used and ensure
the existing logic for isSameUrl, buttonDisabled, and
theme.application.primaryColor remains applied to the final style.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97ada373-e24e-4144-b029-f39b134649e1
📒 Files selected for processing (9)
shesha-reactjs/src/components/buttonGroupConfigurator/buttonGroupItem.tsxshesha-reactjs/src/components/dynamicModal/index.tsxshesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsxshesha-reactjs/src/components/formDesigner/components/styles.tsshesha-reactjs/src/designer-components/button/buttonGroup/buttonGroup.tsxshesha-reactjs/src/designer-components/button/configurableButton/index.tsxshesha-reactjs/src/designer-components/dataTable/advancedFilterButton/advancedFilterButton.tsxshesha-reactjs/src/providers/dynamicModal/index.tsxshesha-reactjs/src/utils/style.ts
- Implemented a validation check in the doFetchItems function to ensure that requests are only made with valid verbs, enhancing the robustness of the component.
Contributor
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/endpointsAutocomplete/endpointsAutocomplete.tsx (1)
91-91: 🛠️ Refactor suggestion | 🟠 MajorReplace
anywithunknownto maintain type safety.The
isValidVerbtype guard usesanyfor its parameter, which bypasses TypeScript's type checking. Since the function accepts values of unknown types and performs runtime validation, useunknowninstead to enforce explicit type checking. As per coding guidelines, eliminate theanytype in favor ofunknownfor values with unknown types.♻️ Proposed fix
- const isValidVerb = (verbValue: any): verbValue is string => { + const isValidVerb = (verbValue: unknown): verbValue is string => { return typeof verbValue === 'string' && verbValue.trim() !== ''; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsx` at line 91, The isValidVerb type guard currently declares its parameter as any; change the parameter type to unknown to restore type safety, then ensure the function performs explicit runtime checks (e.g., typeof verbValue === 'string' and any existing string validations) before narrowing to string. Update the signature const isValidVerb = (verbValue: unknown): verbValue is string => and keep the same validation logic inside so TypeScript can safely use the type guard elsewhere.
🤖 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/endpointsAutocomplete/endpointsAutocomplete.tsx`:
- Line 91: The isValidVerb type guard currently declares its parameter as any;
change the parameter type to unknown to restore type safety, then ensure the
function performs explicit runtime checks (e.g., typeof verbValue === 'string'
and any existing string validations) before narrowing to string. Update the
signature const isValidVerb = (verbValue: unknown): verbValue is string => and
keep the same validation logic inside so TypeScript can safely use the type
guard elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 057b0f0b-fb1f-46ca-a23b-8564a114f4a0
📒 Files selected for processing (1)
shesha-reactjs/src/components/endpointsAutocomplete/endpointsAutocomplete.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#4749
Summary by CodeRabbit
Bug Fixes
Chores