Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThreads two new props, actionIconSize and actionIconColor, from designer defaults through TableWrapper → DataTable → ReactTable into styling hooks; augments row interaction contexts with a selectedRow object; adjusts header/body alignment, vertical separators, background composition, and row/cell layout. Changes
Sequence Diagram(s)sequenceDiagram
participant Designer as Designer (UI/Defaults)
participant TableWrapper as TableWrapper
participant DataTable as DataTable
participant ReactTable as ReactTable
participant Styles as StylingHooks
Designer->>TableWrapper: provide settings (actionIconSize, actionIconColor)
TableWrapper->>DataTable: render with actionIconSize/actionIconColor
DataTable->>ReactTable: pass tableProps including actionIconSize/actionIconColor
ReactTable->>Styles: call useMainStyles/useStyles(actionIconSize, actionIconColor)
ReactTable->>ReactTable: render rows/cells with styled icons and separators
Note over ReactTable: Row interaction flow
ReactTable->>ReactTable: onRowHover/onRowDoubleClick -> build selectedRow {index,row,id}
ReactTable->>DataTable: dispatchRowEvent(payload + selectedRow)
DataTable->>Designer: evaluate actions with new context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 3
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/designer-components/dataTable/table/tableWrapper.tsx (1)
231-305:⚠️ Potential issue | 🟠 Major
borderPropis returned as a non-CSS key, dropping the border.Destructuring
borderintoborderPropand then returning{ borderProp }creates an invalid style key, so the border is lost whenprops.borderis falsy.🛠️ Suggested fix
- return { - borderProp, + return { + border: borderProp, borderTop, borderRight, borderBottom, borderLeft, borderWidth, borderStyle: borderStyleProp, borderColor, borderRadius: borderRadiusProp,shesha-reactjs/src/components/reactTable/styles/styles.ts (1)
445-505:⚠️ Potential issue | 🟠 MajorNormalize numeric
actionIconSizeto CSS units.
actionIconSizeisstring | number, but template-literal CSS (antd-stylecreateStyles) requires units forfont-size,width, andheight. If a number is passed, it produces unitless values likefont-size: 16;, which is invalid CSS. Apply the normalization across all three icon styling blocks (.sha-linkouter,.sha-linkwithin.sha-crud-cell, and.sha-action-button).🛠️ Suggested fix
+ const resolvedActionIconSize = + typeof actionIconSize === 'number' ? `${actionIconSize}px` : actionIconSize; ... - font-size: ${actionIconSize || bodyFontSize || '16px'}; - width: ${actionIconSize || bodyFontSize || '16px'}; - height: ${actionIconSize || bodyFontSize || '16px'}; - min-width: ${actionIconSize || bodyFontSize || '16px'}; - min-height: ${actionIconSize || bodyFontSize || '16px'}; + font-size: ${resolvedActionIconSize || bodyFontSize || '16px'}; + width: ${resolvedActionIconSize || bodyFontSize || '16px'}; + height: ${resolvedActionIconSize || bodyFontSize || '16px'}; + min-width: ${resolvedActionIconSize || bodyFontSize || '16px'}; + min-height: ${resolvedActionIconSize || bodyFontSize || '16px'};Apply the same
resolvedActionIconSizereplacement for all three icon blocks in this file.
🤖 Fix all issues with AI agents
In `@shesha-reactjs/src/components/reactTable/styles/styles.ts`:
- Around line 739-759: The flex container currently forces horizontal centering
with "justify-content: center" which ignores the bodyTextAlign prop; update the
auto rowHeight block (the styles around rowHeight and the cell flex rules) to
map bodyTextAlign to the flex "justify-content" value (e.g., left->flex-start,
center->center, right->flex-end, justify->space-between) while keeping vertical
centering via "align-items: center" (or "align-self: center" semantics used),
and retain the existing min-height/height/display rules and the vertical
separator ::after pseudo-element; use the rowHeight and bodyTextAlign variables
to compute the correct justify-content instead of the hardcoded center.
In `@shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx`:
- Around line 154-189: The background-size handling in effectiveBackground can
emit an invalid "/ size" token when bgStyles.backgroundSize exists but
bgStyles.backgroundPosition is absent; update the logic in the useMemo (function
effectiveBackground using getBackgroundStyle and bgStyles) to ensure a default
position is inserted before the "/" separator (e.g., use a default like "0% 0%"
or "center" when backgroundPosition is missing) and then build the combined
token as "<position> / <size>" instead of pushing "/ <size>" separately; keep
the early-return for color intact and only apply the default position when
composing image/position/size/repeat strings.
In `@shesha-reactjs/src/designer-components/dataTable/table/utils.ts`:
- Line 137: The return type in utils.ts declares actionIconSize as string but
must match the model ITableComponentBaseProps which defines actionIconSize as
string | number; update the returned type/signature and any related default
value or inferred type in the function or object that includes actionIconSize so
it is string | number (or cast the default to that union) to maintain type
consistency with ITableComponentBaseProps.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
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/designer-components/dataTable/table/tableWrapper.tsx (1)
231-305:⚠️ Potential issue | 🟠 MajorBorder is dropped when
props.borderis falsy due toborderPropkey.
The return object usesborderProp(notborder), so the border is lost even when it should be preserved.🛠️ Suggested fix
return { - borderProp, + border: borderProp, borderTop, borderRight, borderBottom, borderLeft, borderWidth,shesha-reactjs/src/components/reactTable/styles/styles.ts (1)
450-510:⚠️ Potential issue | 🟡 MinorNormalize numeric icon sizes to valid CSS units.
actionIconSize/bodyFontSizeallow numbers; interpolating them directly yields unitlessfont-size/width/heightand can be ignored by CSS. Normalize topxbefore use.In CSS, do font-size/width/height accept unitless numbers, or must they include units like px?🛠️ Suggested fix
+ const normalizeSize = (value?: string | number): string | undefined => + typeof value === 'number' ? `${value}px` : value; + + const actionIconSizeValue = + normalizeSize(actionIconSize) ?? normalizeSize(bodyFontSize) ?? '16px'; ... .sha-link { .${iconPrefixCls} { - font-size: ${actionIconSize || bodyFontSize || '16px'}; - width: ${actionIconSize || bodyFontSize || '16px'}; - height: ${actionIconSize || bodyFontSize || '16px'}; - min-width: ${actionIconSize || bodyFontSize || '16px'}; - min-height: ${actionIconSize || bodyFontSize || '16px'}; + font-size: ${actionIconSizeValue}; + width: ${actionIconSizeValue}; + height: ${actionIconSizeValue}; + min-width: ${actionIconSizeValue}; + min-height: ${actionIconSizeValue}; ${actionIconColor ? `color: ${actionIconColor};` : ''} } } ... .${iconPrefixCls} { - font-size: ${actionIconSize || bodyFontSize || '16px'}; - width: ${actionIconSize || bodyFontSize || '16px'}; - height: ${actionIconSize || bodyFontSize || '16px'}; - min-width: ${actionIconSize || bodyFontSize || '16px'}; - min-height: ${actionIconSize || bodyFontSize || '16px'}; + font-size: ${actionIconSizeValue}; + width: ${actionIconSizeValue}; + height: ${actionIconSizeValue}; + min-width: ${actionIconSizeValue}; + min-height: ${actionIconSizeValue}; ${actionIconColor ? `color: ${actionIconColor};` : ''} display: inline-flex; align-items: center; justify-content: center; } ... .${iconPrefixCls} { - font-size: ${actionIconSize || bodyFontSize || '16px'}; - width: ${actionIconSize || bodyFontSize || '16px'}; - height: ${actionIconSize || bodyFontSize || '16px'}; - min-width: ${actionIconSize || bodyFontSize || '16px'}; - min-height: ${actionIconSize || bodyFontSize || '16px'}; + font-size: ${actionIconSizeValue}; + width: ${actionIconSizeValue}; + height: ${actionIconSizeValue}; + min-width: ${actionIconSizeValue}; + min-height: ${actionIconSizeValue}; ${actionIconColor ? `color: ${actionIconColor};` : ''} display: inline-flex; align-items: center; justify-content: center; }
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 (1)
shesha-reactjs/src/components/reactTable/index.tsx (1)
201-219: 🧹 Nitpick | 🔵 TrivialConsider stronger typing for
overrideSelectedRowparameter.The
overrideSelectedRowparameter usesanyfor therowandidproperties. Per coding guidelines, preferunknownor a proper type overanyto enforce explicit type checking.♻️ Suggested type improvement
+interface ISelectedRowContext { + index: number; + row: unknown; + id: unknown; +} + const dispatchRowEvent = ( actionConfig: IConfigurableActionConfiguration | undefined, rowData: any, rowIndex: number, - overrideSelectedRow?: { index: number; row: any; id: any }, + overrideSelectedRow?: ISelectedRowContext, ): void => {
🤖 Fix all issues with AI agents
In `@shesha-reactjs/src/components/reactTable/index.tsx`:
- Around line 840-845: Replace the nested ternary that sets justifyContent using
effectiveHeaderTextAlign with a clear mapping object to improve readability:
create an alignMap (e.g., { center: 'center', right: 'flex-end', justify:
'space-between', left: 'flex-start' }) and use
alignMap[effectiveHeaderTextAlign] ?? 'flex-start' when assigning justifyContent
inside the component (where effectiveHeaderTextAlign and justifyContent are used
in the react table rendering). Ensure you reference effectiveHeaderTextAlign and
replace the ternary expression with the map lookup to keep behavior identical.
In `@shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx`:
- Around line 278-305: The return object mistakenly emits a property named
borderProp instead of restoring the original CSS property; change the returned
key from borderProp to border: borderProp so the original `border` value
(renamed to `borderProp` during destructuring) is reattached; update the object
in tableWrapper.tsx where `borderProp` is spread alongside other border fields
(the same place returning borderTop/Right/Bottom/Left etc.) so the `border` CSS
prop is preserved when props.border was falsy.
Comment on lines
+840
to
+845
| // Map headerTextAlign to justify-content for flex container | ||
| justifyContent: | ||
| effectiveHeaderTextAlign === 'center' ? 'center' | ||
| : effectiveHeaderTextAlign === 'right' ? 'flex-end' | ||
| : effectiveHeaderTextAlign === 'justify' ? 'space-between' | ||
| : 'flex-start', // default for 'left' or undefined |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting alignment mapping for readability.
The nested ternary is functional but could be clearer with a mapping object.
♻️ Optional refactor
+const alignmentToJustify: Record<string, string> = {
+ center: 'center',
+ right: 'flex-end',
+ justify: 'space-between',
+};
+
// In the style object:
-justifyContent:
- effectiveHeaderTextAlign === 'center' ? 'center'
- : effectiveHeaderTextAlign === 'right' ? 'flex-end'
- : effectiveHeaderTextAlign === 'justify' ? 'space-between'
- : 'flex-start',
+justifyContent: alignmentToJustify[effectiveHeaderTextAlign] ?? 'flex-start',🤖 Prompt for AI Agents
In `@shesha-reactjs/src/components/reactTable/index.tsx` around lines 840 - 845,
Replace the nested ternary that sets justifyContent using
effectiveHeaderTextAlign with a clear mapping object to improve readability:
create an alignMap (e.g., { center: 'center', right: 'flex-end', justify:
'space-between', left: 'flex-start' }) and use
alignMap[effectiveHeaderTextAlign] ?? 'flex-start' when assigning justifyContent
inside the component (where effectiveHeaderTextAlign and justifyContent are used
in the react table rendering). Ensure you reference effectiveHeaderTextAlign and
replace the ternary expression with the map lookup to keep behavior identical.
Comment on lines
+278
to
+305
| return { | ||
| borderProp, | ||
| borderTop, | ||
| borderRight, | ||
| borderBottom, | ||
| borderLeft, | ||
| borderWidth, | ||
| borderStyle: borderStyleProp, | ||
| borderColor, | ||
| borderRadius: borderRadiusProp, | ||
| borderTopWidth, | ||
| borderRightWidth, | ||
| borderBottomWidth, | ||
| borderLeftWidth, | ||
| borderTopStyle, | ||
| borderRightStyle, | ||
| borderBottomStyle, | ||
| borderLeftStyle, | ||
| borderTopColor, | ||
| borderRightColor, | ||
| borderBottomColor, | ||
| borderLeftColor, | ||
| borderTopLeftRadius, | ||
| borderTopRightRadius, | ||
| borderBottomLeftRadius, | ||
| borderBottomRightRadius, | ||
| ...styleWithoutBorderAndBackground, | ||
| }; |
Contributor
There was a problem hiding this comment.
Bug: Property renamed incorrectly when re-spreading border properties.
Line 279 uses borderProp, which creates a property named borderProp instead of restoring the original border property. The destructuring at line 234 renames border to borderProp, but when re-adding it to the object, it should be border: borderProp,.
This causes the border CSS property to be lost when props.border is falsy.
🐛 Proposed fix
// Remove background properties but keep border properties
return {
- borderProp,
+ border: borderProp,
borderTop,
borderRight,
borderBottom,
borderLeft,
borderWidth,
borderStyle: borderStyleProp,
borderColor,
borderRadius: borderRadiusProp,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| borderProp, | |
| borderTop, | |
| borderRight, | |
| borderBottom, | |
| borderLeft, | |
| borderWidth, | |
| borderStyle: borderStyleProp, | |
| borderColor, | |
| borderRadius: borderRadiusProp, | |
| borderTopWidth, | |
| borderRightWidth, | |
| borderBottomWidth, | |
| borderLeftWidth, | |
| borderTopStyle, | |
| borderRightStyle, | |
| borderBottomStyle, | |
| borderLeftStyle, | |
| borderTopColor, | |
| borderRightColor, | |
| borderBottomColor, | |
| borderLeftColor, | |
| borderTopLeftRadius, | |
| borderTopRightRadius, | |
| borderBottomLeftRadius, | |
| borderBottomRightRadius, | |
| ...styleWithoutBorderAndBackground, | |
| }; | |
| return { | |
| border: borderProp, | |
| borderTop, | |
| borderRight, | |
| borderBottom, | |
| borderLeft, | |
| borderWidth, | |
| borderStyle: borderStyleProp, | |
| borderColor, | |
| borderRadius: borderRadiusProp, | |
| borderTopWidth, | |
| borderRightWidth, | |
| borderBottomWidth, | |
| borderLeftWidth, | |
| borderTopStyle, | |
| borderRightStyle, | |
| borderBottomStyle, | |
| borderLeftStyle, | |
| borderTopColor, | |
| borderRightColor, | |
| borderBottomColor, | |
| borderLeftColor, | |
| borderTopLeftRadius, | |
| borderTopRightRadius, | |
| borderBottomLeftRadius, | |
| borderBottomRightRadius, | |
| ...styleWithoutBorderAndBackground, | |
| }; |
🤖 Prompt for AI Agents
In `@shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx`
around lines 278 - 305, The return object mistakenly emits a property named
borderProp instead of restoring the original CSS property; change the returned
key from borderProp to border: borderProp so the original `border` value
(renamed to `borderProp` during destructuring) is reattached; update the object
in tableWrapper.tsx where `borderProp` is spread alongside other border fields
(the same place returning borderTop/Right/Bottom/Left etc.) so the `border` CSS
prop is preserved when props.border was falsy.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx`:
- Around line 201-203: The code casts props.allStyles.fontStyles.fontSize to
string before parseInt which can hide non-string values; update the fontSize
assignment in tableWrapper.tsx to first read const fs =
props?.allStyles?.fontStyles?.fontSize and then use a type guard: if typeof fs
=== 'string' parseInt(fs, 10), else if typeof fs === 'number' use fs, otherwise
fall back to props?.font?.size; replace the existing
parseInt(props.allStyles.fontStyles.fontSize as string, 10) expression with this
guarded logic to avoid NaN and unsafe casts.
- Around line 150-156: In the useMemo block that computes effectiveBodyTextAlign
(the memoized function in tableWrapper.tsx), remove the debug console.log("table
styles", props?.allStyles) so no debug output is emitted; keep the rest of the
useMemo logic that returns props?.allStyles?.fontStyles?.textAlign ??
props?.font?.align and retain the dependency array
[props?.allStyles?.fontStyles?.textAlign, props?.font?.align].
- Around line 529-531: The bodyFontSize prop in tableWrapper.tsx currently
force-casts props.allStyles.fontStyles.fontSize to string which is unsafe;
update the expression (used alongside useStyles) to use a type guard: check if
props?.allStyles?.fontStyles?.fontSize is a string then use it, else if it's a
number convert it to `${value}px`, otherwise fall back to props?.font?.size
similarly converted; mirror the same safe handling logic used in useStyles so
fontSize is consistently and safely resolved without blind type assertions.
Comment on lines
+201
to
+203
| fontSize: props?.allStyles?.fontStyles?.fontSize | ||
| ? parseInt(props.allStyles.fontStyles.fontSize as string, 10) | ||
| : props?.font?.size, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer type guard over type cast for fontSize.
The as string cast bypasses type checking. If fontSize isn't a string at runtime, parseInt may produce NaN. Per coding guidelines, use a type guard to safely check the value before parsing.
♻️ Suggested refactor
- fontSize: props?.allStyles?.fontStyles?.fontSize
- ? parseInt(props.allStyles.fontStyles.fontSize as string, 10)
- : props?.font?.size,
+ fontSize: typeof props?.allStyles?.fontStyles?.fontSize === 'string'
+ ? parseInt(props.allStyles.fontStyles.fontSize, 10)
+ : typeof props?.allStyles?.fontStyles?.fontSize === 'number'
+ ? props.allStyles.fontStyles.fontSize
+ : props?.font?.size,🤖 Prompt for AI Agents
In `@shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx`
around lines 201 - 203, The code casts props.allStyles.fontStyles.fontSize to
string before parseInt which can hide non-string values; update the fontSize
assignment in tableWrapper.tsx to first read const fs =
props?.allStyles?.fontStyles?.fontSize and then use a type guard: if typeof fs
=== 'string' parseInt(fs, 10), else if typeof fs === 'number' use fs, otherwise
fall back to props?.font?.size; replace the existing
parseInt(props.allStyles.fontStyles.fontSize as string, 10) expression with this
guarded logic to avoid NaN and unsafe casts.
Comment on lines
+529
to
+531
| bodyFontSize={props?.allStyles?.fontStyles?.fontSize | ||
| ? (props.allStyles.fontStyles.fontSize as string) | ||
| : (props?.font?.size ? `${props.font.size}px` : undefined)} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Apply consistent type handling for fontSize.
Same type cast issue as in useStyles. Use a type guard for consistency and safety.
♻️ Suggested refactor
- bodyFontSize={props?.allStyles?.fontStyles?.fontSize
- ? (props.allStyles.fontStyles.fontSize as string)
- : (props?.font?.size ? `${props.font.size}px` : undefined)}
+ bodyFontSize={typeof props?.allStyles?.fontStyles?.fontSize === 'string'
+ ? props.allStyles.fontStyles.fontSize
+ : typeof props?.allStyles?.fontStyles?.fontSize === 'number'
+ ? `${props.allStyles.fontStyles.fontSize}px`
+ : (props?.font?.size ? `${props.font.size}px` : undefined)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bodyFontSize={props?.allStyles?.fontStyles?.fontSize | |
| ? (props.allStyles.fontStyles.fontSize as string) | |
| : (props?.font?.size ? `${props.font.size}px` : undefined)} | |
| bodyFontSize={typeof props?.allStyles?.fontStyles?.fontSize === 'string' | |
| ? props.allStyles.fontStyles.fontSize | |
| : typeof props?.allStyles?.fontStyles?.fontSize === 'number' | |
| ? `${props.allStyles.fontStyles.fontSize}px` | |
| : (props?.font?.size ? `${props.font.size}px` : undefined)} |
🤖 Prompt for AI Agents
In `@shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx`
around lines 529 - 531, The bodyFontSize prop in tableWrapper.tsx currently
force-casts props.allStyles.fontStyles.fontSize to string which is unsafe;
update the expression (used alongside useStyles) to use a type guard: check if
props?.allStyles?.fontStyles?.fontSize is a string then use it, else if it's a
number convert it to `${value}px`, otherwise fall back to props?.font?.size
similarly converted; mirror the same safe handling logic used in useStyles so
fontSize is consistently and safely resolved without blind type assertions.
James-Baloyi
force-pushed
the
james/b/table-styles
branch
from
8dd2ca3 to
7467639
Compare
Merged
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.
Summary by CodeRabbit
New Features
Style
Behavior