Merged
Conversation
Contributor
WalkthroughAdds structured row styling props (rowDimensions, rowStylingBox, rowBorderStyle), keeps legacy flat props as deprecated, adds converters and a runtime mapping layer, and updates designer settings UI to nested collapsible panels bound to the new structured properties. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
shesha-reactjs/src/designer-components/dataTable/table/models.ts(1 hunks)shesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx(1 hunks)shesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts(1 hunks)shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx(4 hunks)shesha-reactjs/src/designer-components/dataTable/table/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CodeRabbitReview-Project-Specific-Guidelines.md)
**/*.{ts,tsx}: Eliminate theanytype; useunknowntype instead for values with unknown types, forcing explicit type checking
Prefer type guards over type casting for type checking
Avoid monolithic types; use discriminated unions with a discriminator property instead
Leverage TypeScript to its full potential as a type system, not merely as a linter
Files:
shesha-reactjs/src/designer-components/dataTable/table/models.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/utils.tsshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
**/*{Component,Editor}.{ts,tsx}
📄 CodeRabbit inference engine (CodeRabbitReview-Project-Specific-Guidelines.md)
**/*{Component,Editor}.{ts,tsx}: Default values must be part of model initialization logic, not in the editor component; controlled editors should follow the standardvalueandonChangecontract
Remove thedefaultValuepattern entirely from controlled editor components
Do not useuseEffectfor resetting values to defaults in controlled editor components; this causes unnecessary re-renders
Files:
shesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx
🧠 Learnings (11)
📚 Learning: 2025-12-02T14:32:10.827Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4280
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:205-227
Timestamp: 2025-12-02T14:32:10.827Z
Learning: In shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts, the mockup components (filterButtonMockup and viewSelectorMockup) intentionally use hardcoded colors (#d9d9d9, #fafafa, #8c8c8c) to maintain consistent appearance across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/models.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/utils.tsshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
📚 Learning: 2025-10-02T11:25:33.370Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3926
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:138-166
Timestamp: 2025-10-02T11:25:33.370Z
Learning: In the shesha-framework repository, the hint popover components (sha-quick-search-hint-popover, sha-table-view-selector-hint-popover, sha-table-pager-hint-popover) in shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts intentionally use hard-coded color `rgb(214, 214, 214)` with `!important` to impose a specific consistent color across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/models.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/utils.tsshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/models.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/utils.tsshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
📚 Learning: 2025-10-28T14:21:55.782Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4084
File: shesha-reactjs/src/components/quickView/styles/styles.ts:41-94
Timestamp: 2025-10-28T14:21:55.782Z
Learning: In shesha-reactjs/src/components/quickView/styles/styles.ts, the quickViewContent styles intentionally use multiple `!important` flags and broad selectors (including spans, form items, inputs, links, and labels) to ensure proper targeting and prevent layout breakage in the quick view component.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx
📚 Learning: 2025-11-05T08:12:08.149Z
Learnt from: Lihlu
Repo: shesha-io/shesha-framework PR: 4132
File: shesha-reactjs/src/providers/layersProvider/index.tsx:35-39
Timestamp: 2025-11-05T08:12:08.149Z
Learning: In shesha-reactjs LayerGroupConfiguratorProvider (src/providers/layersProvider/index.tsx), the provider uses a unidirectional data flow pattern after initialization: props.items provide initial state to the reducer (line 35-38), user actions modify the reducer state internally, and changes sync back to the parent via onChange callbacks in consuming components (e.g., modal.tsx line 55-57 uses useDeepCompareEffect). Do NOT add a useEffect to sync props.items back to the reducer state, as this creates a feedback loop that breaks layer updates in the Calendar UI. The pattern is intentionally "controlled initialization, uncontrolled updates" where the provider manages state internally after the initial mount.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-12-03T09:43:09.622Z
Learnt from: HackGenesis
Repo: shesha-io/shesha-framework PR: 4283
File: shesha-reactjs/src/components/dataTable/index.tsx:881-882
Timestamp: 2025-12-03T09:43:09.622Z
Learning: In the shesha-framework DataTable/ReactTable components as of PR #4283, checkbox visibility is controlled by selectionMode: checkboxes are only shown when selectionMode is 'multiple'. For selectionMode 'single', rows can be selected via clicking without displaying checkbox columns. The useMultiSelect prop calculation reflects this: it evaluates to true only for 'multiple' mode (or when selectionMode is undefined and the legacy useMultiSelect prop is true).
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-06-26T19:59:22.872Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/utils.tsshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
📚 Learning: 2025-06-26T20:00:58.574Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/chartControl.tsx:51-51
Timestamp: 2025-06-26T20:00:58.574Z
Learning: In shesha-reactjs chart components (ChartControl), filters are intentionally passed as props rather than obtained from context. This allows the query builder and other external components to dynamically update filters. The pattern is: most chart configuration comes from useChartDataStateContext(), but filters are passed as props from the chart factories (bar.tsx, line.tsx, pie.tsx, polarArea.tsx) for dynamic updates.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-05-28T07:55:21.036Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx
📚 Learning: 2025-06-26T20:01:48.838Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:171-192
Timestamp: 2025-06-26T20:01:48.838Z
Learning: In shesha-reactjs chart settings forms, duplicate input configurations may be intentionally included as part of UI/UX experiments and should be flagged for review during UI/UX review phases rather than immediately removed as errors.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
📚 Learning: 2025-11-20T14:51:46.152Z
Learnt from: AlexStepantsov
Repo: shesha-io/shesha-framework PR: 4235
File: shesha-reactjs/src/components/modelConfigurator/propertiesEditor/provider/reducer.ts:149-149
Timestamp: 2025-11-20T14:51:46.152Z
Learning: In shesha-reactjs model configurator reducer (src/components/modelConfigurator/propertiesEditor/provider/reducer.ts), when handling ArrayFormats.childObjects in the UpdateItem action, itemsType.dataFormat should be set to undefined (not ObjectFormats.object). This is the correct logic for child object array configuration.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-attempt
🔇 Additional comments (6)
shesha-reactjs/src/designer-components/dataTable/table/tableComponent.tsx (1)
141-153: LGTM!The property filter correctly extends the allowed properties to include both deprecated (rowHeight, rowPadding, rowBorder) and new structured properties (rowDimensions, rowStylingBox, rowBorderStyle). The inline comments provide clear guidance on the deprecation path.
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx (2)
70-90: Approve the backward-compatibility pattern.The conversion pattern correctly prioritizes new structured properties (rowDimensions, rowStylingBox, rowBorderStyle) while falling back to deprecated properties (rowHeight, rowPadding, rowBorder). The useMemo hooks appropriately optimize the conversions.
113-115: LGTM!The effective values (effectiveRowHeight, effectiveRowPadding, effectiveRowBorder) are used consistently in both the useStyles hook and the DataTable component, ensuring uniform behavior across styling and rendering.
Also applies to: 384-386
shesha-reactjs/src/designer-components/dataTable/table/models.ts (1)
48-81: LGTM!The type definitions properly introduce structured properties (rowDimensions, rowStylingBox, rowBorderStyle) with appropriate TypeScript types while maintaining backward compatibility. The JSDoc @deprecated comments clearly document the migration path for developers.
shesha-reactjs/src/designer-components/dataTable/table/utils.ts (1)
256-321: LGTM!The padding and border conversion utilities correctly handle the transformation from structured objects to CSS shorthand strings. The padding conversion appropriately optimizes shorthand formats, and the border conversion acknowledges the limitation of returning only one border value for custom configurations (which is acceptable given the legacy property constraint).
shesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts (1)
558-646: LGTM!The UI restructuring into collapsible panels (Row Dimensions, Row Padding, Row Border) provides better organization and aligns with the new structured properties. The implementation follows existing patterns and correctly binds to the new property paths (rowDimensions.height, rowStylingBox, rowBorderStyle).
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+226
to
+234
| const addPxUnit = (value?: string | number): string => { | ||
| if (!value && value !== 0) return '0px'; | ||
| const strValue = String(value); | ||
| // If it's just a number (with optional decimal), add 'px' | ||
| if (/^-?\d+\.?\d*$/.test(strValue)) { | ||
| return `${strValue}px`; | ||
| } | ||
| return strValue; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider edge cases in numeric detection.
The regex pattern /^-?\d+\.?\d*$/ handles common numeric formats but would not match scientific notation (e.g., '1e10') or explicit positive signs (e.g., '+5'). While these are uncommon in CSS contexts, you may want to document this limitation or enhance the pattern if broader numeric support is needed.
If broader support is desired:
const addPxUnit = (value?: string | number): string => {
if (!value && value !== 0) return '0px';
const strValue = String(value);
- // If it's just a number (with optional decimal), add 'px'
- if (/^-?\d+\.?\d*$/.test(strValue)) {
+ // If it's just a number (with optional decimal or scientific notation), add 'px'
+ if (/^[+-]?\d+\.?\d*([eE][+-]?\d+)?$/.test(strValue)) {
return `${strValue}px`;
}
return strValue;
};📝 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
| const addPxUnit = (value?: string | number): string => { | |
| if (!value && value !== 0) return '0px'; | |
| const strValue = String(value); | |
| // If it's just a number (with optional decimal), add 'px' | |
| if (/^-?\d+\.?\d*$/.test(strValue)) { | |
| return `${strValue}px`; | |
| } | |
| return strValue; | |
| }; | |
| const addPxUnit = (value?: string | number): string => { | |
| if (!value && value !== 0) return '0px'; | |
| const strValue = String(value); | |
| // If it's just a number (with optional decimal or scientific notation), add 'px' | |
| if (/^[+-]?\d+\.?\d*([eE][+-]?\d+)?$/.test(strValue)) { | |
| return `${strValue}px`; | |
| } | |
| return strValue; | |
| }; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dataTable/table/utils.ts around lines
226 to 234, the numeric-detection regex excludes values like "+5" and scientific
notation ("1e10"); either broaden the regex to accept an optional leading "+"
and exponent notation (e.g. allow /^\+?-?\d+(\.\d+)?([eE][+-]?\d+)?$/ or
equivalent) so those strings get "px" appended, or explicitly document the
current limitation in a brief comment above addPxUnit if you prefer to keep the
simpler pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
shesha-reactjs/src/designer-components/dataTable/table/utils.ts (1)
226-234: Edge case limitation already flagged in previous review.The regex pattern limitation for scientific notation and explicit positive signs has already been comprehensively documented in the previous review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/dataTable/table/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CodeRabbitReview-Project-Specific-Guidelines.md)
**/*.{ts,tsx}: Eliminate theanytype; useunknowntype instead for values with unknown types, forcing explicit type checking
Prefer type guards over type casting for type checking
Avoid monolithic types; use discriminated unions with a discriminator property instead
Leverage TypeScript to its full potential as a type system, not merely as a linter
Files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
📚 Learning: 2025-12-02T14:32:10.827Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4280
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:205-227
Timestamp: 2025-12-02T14:32:10.827Z
Learning: In shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts, the mockup components (filterButtonMockup and viewSelectorMockup) intentionally use hardcoded colors (#d9d9d9, #fafafa, #8c8c8c) to maintain consistent appearance across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
📚 Learning: 2025-10-02T11:25:33.370Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3926
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:138-166
Timestamp: 2025-10-02T11:25:33.370Z
Learning: In the shesha-framework repository, the hint popover components (sha-quick-search-hint-popover, sha-table-view-selector-hint-popover, sha-table-pager-hint-popover) in shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts intentionally use hard-coded color `rgb(214, 214, 214)` with `!important` to impose a specific consistent color across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
🔇 Additional comments (1)
shesha-reactjs/src/designer-components/dataTable/table/utils.ts (1)
251-259: JSDoc documentation addresses previous feedback.The extensive JSDoc documentation now clearly explains the limitation that minHeight and maxHeight are intentionally discarded for backward compatibility, and recommends accessing
rowDimensionsdirectly when min/max height support is needed.
Comment on lines
+251
to
+340
| export const convertRowDimensionsToHeight = (rowDimensions?: { | ||
| height?: string; | ||
| minHeight?: string; | ||
| maxHeight?: string; | ||
| }): string | undefined => { | ||
| if (!rowDimensions?.height) return undefined; | ||
| // Add px unit if it's just a number | ||
| return addPxUnit(rowDimensions.height); | ||
| }; | ||
|
|
||
| /** | ||
| * Converts styling box padding to a single padding string | ||
| * @param rowStylingBox - Styling box configuration | ||
| * @returns Padding string for backward compatibility | ||
| */ | ||
| export const convertRowStylingBoxToPadding = (rowStylingBox?: { | ||
| margin?: { | ||
| top?: string; | ||
| right?: string; | ||
| bottom?: string; | ||
| left?: string; | ||
| }; | ||
| padding?: { | ||
| top?: string; | ||
| right?: string; | ||
| bottom?: string; | ||
| left?: string; | ||
| }; | ||
| }): string | undefined => { | ||
| if (!rowStylingBox?.padding) return undefined; | ||
|
|
||
| const { top, right, bottom, left } = rowStylingBox.padding; | ||
|
|
||
| // Add px units to values if needed | ||
| const topPx = addPxUnit(top); | ||
| const rightPx = addPxUnit(right); | ||
| const bottomPx = addPxUnit(bottom); | ||
| const leftPx = addPxUnit(left); | ||
|
|
||
| // If all values are the same, return single value | ||
| if (topPx === rightPx && rightPx === bottomPx && bottomPx === leftPx) { | ||
| return topPx; | ||
| } | ||
|
|
||
| // If top/bottom and left/right pairs are the same | ||
| if (topPx === bottomPx && leftPx === rightPx) { | ||
| return `${topPx} ${leftPx}`; | ||
| } | ||
|
|
||
| // Return all four values | ||
| return `${topPx} ${rightPx} ${bottomPx} ${leftPx}`; | ||
| }; | ||
|
|
||
| /** | ||
| * Converts border value object to a single border string | ||
| * @param rowBorderStyle - Border style configuration | ||
| * @returns Border string for backward compatibility | ||
| */ | ||
| export const convertRowBorderStyleToBorder = (rowBorderStyle?: { | ||
| borderType?: string; | ||
| border?: { | ||
| all?: { width?: string | number; style?: string; color?: string }; | ||
| top?: { width?: string | number; style?: string; color?: string }; | ||
| right?: { width?: string | number; style?: string; color?: string }; | ||
| bottom?: { width?: string | number; style?: string; color?: string }; | ||
| left?: { width?: string | number; style?: string; color?: string }; | ||
| }; | ||
| }): string | undefined => { | ||
| if (!rowBorderStyle?.border) return undefined; | ||
|
|
||
| const { borderType, border } = rowBorderStyle; | ||
|
|
||
| // If borderType is 'all', use the all configuration | ||
| if (borderType === 'all' && border.all) { | ||
| const { width, style, color } = border.all; | ||
| if (!width || !style || !color) return undefined; | ||
| return `${width} ${style} ${color}`; | ||
| } | ||
|
|
||
| // For custom borders, we can only return one border string | ||
| // Let's use the top border or the first available border | ||
| const firstBorder = border.top || border.right || border.bottom || border.left; | ||
| if (firstBorder) { | ||
| const { width, style, color } = firstBorder; | ||
| if (!width || !style || !color) return undefined; | ||
| return `${width} ${style} ${color}`; | ||
| } | ||
|
|
||
| return undefined; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting inline type definitions.
The three conversion functions (convertRowDimensionsToHeight, convertRowStylingBoxToPadding, convertRowBorderStyleToBorder) use inline type definitions for their parameters. Consider extracting these to named type definitions or interfaces for better reusability and maintainability, especially if these types are used elsewhere in the codebase.
As per coding guidelines, leveraging TypeScript's type system more fully by defining named types improves code clarity and reduces duplication.
Example:
// Type definitions for row styling conversion
export interface IRowDimensions {
height?: string;
minHeight?: string;
maxHeight?: string;
}
export interface ISpacing {
top?: string;
right?: string;
bottom?: string;
left?: string;
}
export interface IRowStylingBox {
margin?: ISpacing;
padding?: ISpacing;
}
export interface IBorderConfig {
width?: string | number;
style?: string;
color?: string;
}
export interface IRowBorderStyle {
borderType?: string;
border?: {
all?: IBorderConfig;
top?: IBorderConfig;
right?: IBorderConfig;
bottom?: IBorderConfig;
left?: IBorderConfig;
};
}
// Then use in function signatures
export const convertRowDimensionsToHeight = (rowDimensions?: IRowDimensions): string | undefined => {
// ...
};🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dataTable/table/utils.ts around lines
251 to 340, the three conversion functions use inline parameter type literals;
extract and export named interfaces (e.g., IRowDimensions, ISpacing,
IRowStylingBox, IBorderConfig, IRowBorderStyle) at the top of the file and
replace the inline types in the function signatures with these interfaces;
ensure the border width type remains string | number, export the interfaces for
reuse, and update any imports/exports if other modules will consume them.
Comment on lines
+266
to
+302
| export const convertRowStylingBoxToPadding = (rowStylingBox?: { | ||
| margin?: { | ||
| top?: string; | ||
| right?: string; | ||
| bottom?: string; | ||
| left?: string; | ||
| }; | ||
| padding?: { | ||
| top?: string; | ||
| right?: string; | ||
| bottom?: string; | ||
| left?: string; | ||
| }; | ||
| }): string | undefined => { | ||
| if (!rowStylingBox?.padding) return undefined; | ||
|
|
||
| const { top, right, bottom, left } = rowStylingBox.padding; | ||
|
|
||
| // Add px units to values if needed | ||
| const topPx = addPxUnit(top); | ||
| const rightPx = addPxUnit(right); | ||
| const bottomPx = addPxUnit(bottom); | ||
| const leftPx = addPxUnit(left); | ||
|
|
||
| // If all values are the same, return single value | ||
| if (topPx === rightPx && rightPx === bottomPx && bottomPx === leftPx) { | ||
| return topPx; | ||
| } | ||
|
|
||
| // If top/bottom and left/right pairs are the same | ||
| if (topPx === bottomPx && leftPx === rightPx) { | ||
| return `${topPx} ${leftPx}`; | ||
| } | ||
|
|
||
| // Return all four values | ||
| return `${topPx} ${rightPx} ${bottomPx} ${leftPx}`; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify or remove unused margin parameter.
The function signature includes a margin property in the rowStylingBox parameter, but this property is never referenced in the function body. Only the padding property is used for the conversion.
If margin is included for future extensibility or to match a shared type definition, consider adding a comment explaining this. Otherwise, remove it to improve clarity.
Apply this diff if margin is not needed:
export const convertRowStylingBoxToPadding = (rowStylingBox?: {
- margin?: {
- top?: string;
- right?: string;
- bottom?: string;
- left?: string;
- };
padding?: {
top?: string;
right?: string;
bottom?: string;
left?: string;
};
}): string | 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
| export const convertRowStylingBoxToPadding = (rowStylingBox?: { | |
| margin?: { | |
| top?: string; | |
| right?: string; | |
| bottom?: string; | |
| left?: string; | |
| }; | |
| padding?: { | |
| top?: string; | |
| right?: string; | |
| bottom?: string; | |
| left?: string; | |
| }; | |
| }): string | undefined => { | |
| if (!rowStylingBox?.padding) return undefined; | |
| const { top, right, bottom, left } = rowStylingBox.padding; | |
| // Add px units to values if needed | |
| const topPx = addPxUnit(top); | |
| const rightPx = addPxUnit(right); | |
| const bottomPx = addPxUnit(bottom); | |
| const leftPx = addPxUnit(left); | |
| // If all values are the same, return single value | |
| if (topPx === rightPx && rightPx === bottomPx && bottomPx === leftPx) { | |
| return topPx; | |
| } | |
| // If top/bottom and left/right pairs are the same | |
| if (topPx === bottomPx && leftPx === rightPx) { | |
| return `${topPx} ${leftPx}`; | |
| } | |
| // Return all four values | |
| return `${topPx} ${rightPx} ${bottomPx} ${leftPx}`; | |
| }; | |
| export const convertRowStylingBoxToPadding = (rowStylingBox?: { | |
| padding?: { | |
| top?: string; | |
| right?: string; | |
| bottom?: string; | |
| left?: string; | |
| }; | |
| }): string | undefined => { | |
| if (!rowStylingBox?.padding) return undefined; | |
| const { top, right, bottom, left } = rowStylingBox.padding; | |
| // Add px units to values if needed | |
| const topPx = addPxUnit(top); | |
| const rightPx = addPxUnit(right); | |
| const bottomPx = addPxUnit(bottom); | |
| const leftPx = addPxUnit(left); | |
| // If all values are the same, return single value | |
| if (topPx === rightPx && rightPx === bottomPx && bottomPx === leftPx) { | |
| return topPx; | |
| } | |
| // If top/bottom and left/right pairs are the same | |
| if (topPx === bottomPx && leftPx === rightPx) { | |
| return `${topPx} ${leftPx}`; | |
| } | |
| // Return all four values | |
| return `${topPx} ${rightPx} ${bottomPx} ${leftPx}`; | |
| }; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dataTable/table/utils.ts around lines
266 to 302, the rowStylingBox parameter declares an unused margin property;
remove margin from the parameter type if it isn't required (change the type to
only include padding) and update any callers/types if necessary OR, if margin is
kept for type compatibility, add a one-line comment above the function
explaining margin is intentionally present for future/compatibility reasons and
leave the implementation as-is; ensure TypeScript types and imports remain
consistent after the change.
Comment on lines
+330
to
+337
| // For custom borders, we can only return one border string | ||
| // Let's use the top border or the first available border | ||
| const firstBorder = border.top || border.right || border.bottom || border.left; | ||
| if (firstBorder) { | ||
| const { width, style, color } = firstBorder; | ||
| if (!width || !style || !color) return undefined; | ||
| return `${width} ${style} ${color}`; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the limitation of discarding custom border configurations.
When borderType is not 'all', the function only returns the first available border (trying top, right, bottom, left in order) and discards any other custom border configurations. This is a backward-compatibility limitation similar to the minHeight/maxHeight case in convertRowDimensionsToHeight.
Add JSDoc to document this limitation and recommend using rowBorderStyle directly when per-side borders are needed.
/**
* Converts border value object to a single border string
+ *
+ * IMPORTANT LIMITATION: When borderType is not 'all', this function only returns
+ * the first available border (top, right, bottom, or left) and discards other
+ * custom border configurations. This is a backward-compatibility limitation for
+ * the legacy `rowBorder` property, which only supports a single border value.
+ *
+ * If you need per-side border configurations, access the `rowBorderStyle` property
+ * directly instead of using this conversion helper.
+ *
* @param rowBorderStyle - Border style configuration
* @returns Border string for backward compatibility
*/Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dataTable/table/utils.ts around lines
330 to 337, the code picks only the first available side when constructing a
custom border and silently discards other side-specific configurations; add a
JSDoc comment above this block explaining this limitation (that when borderType
!== 'all' we return only one border string and ignore other side borders),
mention that this is a backward-compatibility constraint similar to
convertRowDimensionsToHeight, and recommend using rowBorderStyle directly when
per-side borders are required so callers know to avoid relying on per-side
border preservation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shesha-reactjs/src/designer-components/dataTable/table/utils.ts (1)
266-279: Clarify or drop unusedmarginfromrowStylingBox.
marginis declared onrowStylingBoxbut never read; this can confuse future maintainers.If
marginis not required for compatibility, remove it from the parameter type. If it is required to match a shared type, keep it but add a brief comment stating thatmarginis intentionally ignored in this backward‑compat conversion./** * Converts styling box padding to a single padding string - * @param rowStylingBox - Styling box configuration + * @param rowStylingBox - Styling box configuration (margin is accepted for + * type compatibility but intentionally ignored here) * @returns Padding string for backward compatibility */ export const convertRowStylingBoxToPadding = (rowStylingBox?: { - margin?: { - top?: string; - right?: string; - bottom?: string; - left?: string; - }; + // Note: margin is intentionally ignored by this helper; callers needing + // margin should read rowStylingBox directly. + margin?: { + top?: string; + right?: string; + bottom?: string; + left?: string; + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/dataTable/table/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CodeRabbitReview-Project-Specific-Guidelines.md)
**/*.{ts,tsx}: Eliminate theanytype; useunknowntype instead for values with unknown types, forcing explicit type checking
Prefer type guards over type casting for type checking
Avoid monolithic types; use discriminated unions with a discriminator property instead
Leverage TypeScript to its full potential as a type system, not merely as a linter
Files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
📚 Learning: 2025-12-02T14:32:10.827Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4280
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:205-227
Timestamp: 2025-12-02T14:32:10.827Z
Learning: In shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts, the mockup components (filterButtonMockup and viewSelectorMockup) intentionally use hardcoded colors (#d9d9d9, #fafafa, #8c8c8c) to maintain consistent appearance across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
📚 Learning: 2025-10-02T11:25:33.370Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3926
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:138-166
Timestamp: 2025-10-02T11:25:33.370Z
Learning: In the shesha-framework repository, the hint popover components (sha-quick-search-hint-popover, sha-table-view-selector-hint-popover, sha-table-pager-hint-popover) in shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts intentionally use hard-coded color `rgb(214, 214, 214)` with `!important` to impose a specific consistent color across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
📚 Learning: 2025-11-25T07:17:44.449Z
Learnt from: CR
Repo: shesha-io/shesha-framework PR: 0
File: CodeRabbitReview-Project-Specific-Guidelines.md:0-0
Timestamp: 2025-11-25T07:17:44.449Z
Learning: Applies to **/*.{ts,tsx} : Avoid monolithic types; use discriminated unions with a discriminator property instead
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
📚 Learning: 2025-11-25T07:17:44.449Z
Learnt from: CR
Repo: shesha-io/shesha-framework PR: 0
File: CodeRabbitReview-Project-Specific-Guidelines.md:0-0
Timestamp: 2025-11-25T07:17:44.449Z
Learning: Applies to **/*.{ts,tsx} : Leverage TypeScript to its full potential as a type system, not merely as a linter
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
📚 Learning: 2025-06-26T19:59:22.872Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/utils.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-attempt
🔇 Additional comments (1)
shesha-reactjs/src/designer-components/dataTable/table/utils.ts (1)
223-234: addPxUnit helper is consistent and safe for the current CSS use cases.The handling of numeric values (including
0) and already‑unitized strings is correct, and the default'0px'fallback integrates cleanly with the padding/border converters. No changes needed here.
Comment on lines
+304
to
+337
| /** | ||
| * Converts border value object to a single border string | ||
| * @param rowBorderStyle - Border style configuration | ||
| * @returns Border string for backward compatibility | ||
| */ | ||
| export const convertRowBorderStyleToBorder = (rowBorderStyle?: { | ||
| borderType?: string; | ||
| border?: { | ||
| all?: { width?: string | number; style?: string; color?: string }; | ||
| top?: { width?: string | number; style?: string; color?: string }; | ||
| right?: { width?: string | number; style?: string; color?: string }; | ||
| bottom?: { width?: string | number; style?: string; color?: string }; | ||
| left?: { width?: string | number; style?: string; color?: string }; | ||
| }; | ||
| }): string | undefined => { | ||
| if (!rowBorderStyle?.border) return undefined; | ||
|
|
||
| const { borderType, border } = rowBorderStyle; | ||
|
|
||
| // If borderType is 'all', use the all configuration | ||
| if (borderType === 'all' && border.all) { | ||
| const { width, style, color } = border.all; | ||
| if (!width || !style || !color) return undefined; | ||
| return `${addPxUnit(width)} ${style} ${color}`; | ||
| } | ||
|
|
||
| // For custom borders, we can only return one border string | ||
| // Let's use the top border or the first available border | ||
| const firstBorder = border.top || border.right || border.bottom || border.left; | ||
| if (firstBorder) { | ||
| const { width, style, color } = firstBorder; | ||
| if (!width || !style || !color) return undefined; | ||
| return `${addPxUnit(width)} ${style} ${color}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Border conversion: allow width = 0 and document the “first side only” limitation.
Two small improvements will make this helper safer and clearer:
widthis treated as missing when it is0because of the falsy check, so a legitimate0width border will be discarded.- The JSDoc doesn’t mention that for custom borders you only return the first available side and drop the rest, which is an intentional legacy limitation.
Recommend:
- Switch the width check to explicit
null/undefined. - Extend the JSDoc to document the first‑side‑only behavior.
-/**
- * Converts border value object to a single border string
- * @param rowBorderStyle - Border style configuration
- * @returns Border string for backward compatibility
- */
+/**
+ * Converts border value object to a single border string.
+ *
+ * IMPORTANT LIMITATION: When `borderType` is not `'all'`, this helper returns
+ * only the first available side (top, right, bottom, or left) and discards
+ * other side-specific border settings. This mirrors the legacy `rowBorder`
+ * prop, which only supports a single border value.
+ *
+ * Callers that need full per-side configuration should use `rowBorderStyle`
+ * directly instead of this legacy conversion helper.
+ *
+ * @param rowBorderStyle - Border style configuration
+ * @returns Border string for backward compatibility
+ */
export const convertRowBorderStyleToBorder = (rowBorderStyle?: {
@@
// If borderType is 'all', use the all configuration
if (borderType === 'all' && border.all) {
const { width, style, color } = border.all;
- if (!width || !style || !color) return undefined;
+ if (width === undefined || width === null || !style || !color) return undefined;
return `${addPxUnit(width)} ${style} ${color}`;
}
@@
const firstBorder = border.top || border.right || border.bottom || border.left;
if (firstBorder) {
const { width, style, color } = firstBorder;
- if (!width || !style || !color) return undefined;
+ if (width === undefined || width === null || !style || !color) return undefined;
return `${addPxUnit(width)} ${style} ${color}`;
}🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dataTable/table/utils.ts around lines
304 to 337, the helper treats width === 0 as missing due to falsy checks and the
JSDoc omits that for non-'all' borders we return only the first available side;
change the width existence checks from falsy checks to explicit null/undefined
checks (e.g. width === null || width === undefined) so a zero width is allowed,
and update the JSDoc comment to document that for custom borders the function
returns only the first available side (top → right → bottom → left) for backward
compatibility.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
shesha-reactjs/src/components/dataTable/index.tsx(3 hunks)shesha-reactjs/src/components/reactTable/index.tsx(2 hunks)shesha-reactjs/src/components/reactTable/interfaces.ts(1 hunks)shesha-reactjs/src/components/reactTable/styles/styles.ts(3 hunks)shesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts(1 hunks)shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CodeRabbitReview-Project-Specific-Guidelines.md)
**/*.{ts,tsx}: Eliminate theanytype; useunknowntype instead for values with unknown types, forcing explicit type checking
Prefer type guards over type casting for type checking
Avoid monolithic types; use discriminated unions with a discriminator property instead
Leverage TypeScript to its full potential as a type system, not merely as a linter
Files:
shesha-reactjs/src/components/reactTable/styles/styles.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/components/reactTable/index.tsxshesha-reactjs/src/components/dataTable/index.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.tsshesha-reactjs/src/components/reactTable/interfaces.ts
🧠 Learnings (10)
📚 Learning: 2025-06-26T19:59:22.872Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Applied to files:
shesha-reactjs/src/components/reactTable/styles/styles.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/components/reactTable/index.tsxshesha-reactjs/src/components/dataTable/index.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.tsshesha-reactjs/src/components/reactTable/interfaces.ts
📚 Learning: 2025-10-28T14:21:55.782Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4084
File: shesha-reactjs/src/components/quickView/styles/styles.ts:41-94
Timestamp: 2025-10-28T14:21:55.782Z
Learning: In shesha-reactjs/src/components/quickView/styles/styles.ts, the quickViewContent styles intentionally use multiple `!important` flags and broad selectors (including spans, form items, inputs, links, and labels) to ensure proper targeting and prevent layout breakage in the quick view component.
Applied to files:
shesha-reactjs/src/components/reactTable/styles/styles.ts
📚 Learning: 2025-10-02T11:25:33.370Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3926
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:138-166
Timestamp: 2025-10-02T11:25:33.370Z
Learning: In the shesha-framework repository, the hint popover components (sha-quick-search-hint-popover, sha-table-view-selector-hint-popover, sha-table-pager-hint-popover) in shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts intentionally use hard-coded color `rgb(214, 214, 214)` with `!important` to impose a specific consistent color across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/components/reactTable/styles/styles.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/components/reactTable/index.tsxshesha-reactjs/src/components/dataTable/index.tsx
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/components/reactTable/styles/styles.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
📚 Learning: 2025-12-02T14:32:10.827Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 4280
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:205-227
Timestamp: 2025-12-02T14:32:10.827Z
Learning: In shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts, the mockup components (filterButtonMockup and viewSelectorMockup) intentionally use hardcoded colors (#d9d9d9, #fafafa, #8c8c8c) to maintain consistent appearance across themes, overriding theme tokens by design.
Applied to files:
shesha-reactjs/src/components/reactTable/styles/styles.tsshesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsxshesha-reactjs/src/components/reactTable/index.tsxshesha-reactjs/src/components/dataTable/index.tsxshesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
📚 Learning: 2025-11-05T07:46:22.081Z
Learnt from: Lihlu
Repo: shesha-io/shesha-framework PR: 4132
File: shesha-reactjs/src/components/layerEditor/modal.tsx:55-57
Timestamp: 2025-11-05T07:46:22.081Z
Learning: In shesha-reactjs layer editor modal (src/components/layerEditor/modal.tsx), the useDeepCompareEffect that syncs items to the parent via onChange should only depend on [items], not [items, onChange]. Adding onChange to the dependency array causes the effect to fire on every parent render and breaks functionality. The guard for optional onChange is needed but the function reference should not be in the dependency array.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-12-03T09:43:09.622Z
Learnt from: HackGenesis
Repo: shesha-io/shesha-framework PR: 4283
File: shesha-reactjs/src/components/dataTable/index.tsx:881-882
Timestamp: 2025-12-03T09:43:09.622Z
Learning: In the shesha-framework DataTable/ReactTable components as of PR #4283, checkbox visibility is controlled by selectionMode: checkboxes are only shown when selectionMode is 'multiple'. For selectionMode 'single', rows can be selected via clicking without displaying checkbox columns. The useMultiSelect prop calculation reflects this: it evaluates to true only for 'multiple' mode (or when selectionMode is undefined and the legacy useMultiSelect prop is true).
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-06-26T20:00:58.574Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/chartControl.tsx:51-51
Timestamp: 2025-06-26T20:00:58.574Z
Learning: In shesha-reactjs chart components (ChartControl), filters are intentionally passed as props rather than obtained from context. This allows the query builder and other external components to dynamically update filters. The pattern is: most chart configuration comes from useChartDataStateContext(), but filters are passed as props from the chart factories (bar.tsx, line.tsx, pie.tsx, polarArea.tsx) for dynamic updates.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-05-28T07:55:21.036Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
📚 Learning: 2025-06-26T20:01:48.838Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:171-192
Timestamp: 2025-06-26T20:01:48.838Z
Learning: In shesha-reactjs chart settings forms, duplicate input configurations may be intentionally included as part of UI/UX experiments and should be flagged for review during UI/UX review phases rather than immediately removed as errors.
Applied to files:
shesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts
🔇 Additional comments (5)
shesha-reactjs/src/components/reactTable/interfaces.ts (1)
245-246: LGTM! Clean backward-compatible migration.The deprecation comment on
rowBorderclearly guides users to the newrowBorderStyleproperty, and both remain optional to ensure backward compatibility. The new property uses the structuredIBorderValuetype rather than a simple string, enabling richer border configuration.shesha-reactjs/src/components/reactTable/index.tsx (1)
107-107: LGTM! Property correctly threaded through.The new
rowBorderStyleprop is properly destructured and passed to the styling pipeline viauseMainStyles, aligning with the pattern used for other styling properties.shesha-reactjs/src/components/reactTable/styles/styles.ts (1)
244-254: LGTM! Solid conversion logic with proper fallback.The IIFE correctly prioritizes
rowBorderStyleover the legacyrowBorderstring, usinggetBorderStyleto convert the structured border object to CSS properties. The camelCase-to-kebab-case conversion ensures valid CSS output, and the fallback maintains backward compatibility.shesha-reactjs/src/components/dataTable/index.tsx (1)
93-93: LGTM! Consistent propagation through component hierarchy.The new
rowBorderStyleproperty is properly defined in the interface, destructured from props, and passed to the underlyingReactTablecomponent, maintaining consistency with the established pattern for styling properties.Also applies to: 153-153, 948-948
shesha-reactjs/src/designer-components/dataTable/table/tableSettings.ts (1)
558-646: LGTM! Well-organized UI structure with proper property bindings.The migration from flat inputs to nested collapsible panels improves the settings UI organization:
- Row Dimensions panel groups height, minHeight, and maxHeight with clear icons and tooltips
- Row Padding panel uses the style box control for visual padding configuration
- Row Border panel leverages existing border input utilities (getBorderInputs, getCornerInputs) for consistent border configuration
The property paths correctly bind to the new structured props (rowDimensions, rowStylingBox, rowBorderStyle).
Comment on lines
+70
to
+96
| // Convert new property structures to old format for backward compatibility | ||
| const effectiveRowHeight = useMemo(() => { | ||
| // Prefer new rowDimensions over old rowHeight | ||
| const converted = convertRowDimensionsToHeight(props?.rowDimensions); | ||
| if (isDesignMode) { | ||
| console.warn('Row Height - rowDimensions:', props?.rowDimensions, 'converted:', converted, 'fallback:', props?.rowHeight); | ||
| } | ||
| return converted || props?.rowHeight; | ||
| }, [props?.rowDimensions, props?.rowHeight, isDesignMode]); | ||
|
|
||
| const effectiveRowPadding = useMemo(() => { | ||
| // Prefer new rowStylingBox over old rowPadding | ||
| const converted = convertRowStylingBoxToPadding(props?.rowStylingBox); | ||
| if (isDesignMode) { | ||
| console.warn('Row Padding - rowStylingBox:', props?.rowStylingBox, 'converted:', converted, 'fallback:', props?.rowPadding); | ||
| } | ||
| return converted || props?.rowPadding; | ||
| }, [props?.rowStylingBox, props?.rowPadding, isDesignMode]); | ||
|
|
||
| const effectiveRowBorder = useMemo(() => { | ||
| // Prefer new rowBorderStyle over old rowBorder | ||
| const converted = convertRowBorderStyleToBorder(props?.rowBorderStyle); | ||
| if (isDesignMode) { | ||
| console.warn('Row Border - rowBorderStyle:', props?.rowBorderStyle, 'converted:', converted, 'fallback:', props?.rowBorder); | ||
| } | ||
| return converted || props?.rowBorder; | ||
| }, [props?.rowBorderStyle, props?.rowBorder, isDesignMode]); |
Contributor
There was a problem hiding this comment.
Remove console.warn statements from production code.
While these warnings are conditional on isDesignMode, they remain in the production bundle and can execute at runtime. Debug logging should be removed or replaced with a proper logging framework that can be disabled in production builds.
Apply this diff to remove the console warnings:
const effectiveRowHeight = useMemo(() => {
- // Prefer new rowDimensions over old rowHeight
const converted = convertRowDimensionsToHeight(props?.rowDimensions);
- if (isDesignMode) {
- console.warn('Row Height - rowDimensions:', props?.rowDimensions, 'converted:', converted, 'fallback:', props?.rowHeight);
- }
return converted || props?.rowHeight;
- }, [props?.rowDimensions, props?.rowHeight, isDesignMode]);
+ }, [props?.rowDimensions, props?.rowHeight]);
const effectiveRowPadding = useMemo(() => {
- // Prefer new rowStylingBox over old rowPadding
const converted = convertRowStylingBoxToPadding(props?.rowStylingBox);
- if (isDesignMode) {
- console.warn('Row Padding - rowStylingBox:', props?.rowStylingBox, 'converted:', converted, 'fallback:', props?.rowPadding);
- }
return converted || props?.rowPadding;
- }, [props?.rowStylingBox, props?.rowPadding, isDesignMode]);
+ }, [props?.rowStylingBox, props?.rowPadding]);
const effectiveRowBorder = useMemo(() => {
- // Prefer new rowBorderStyle over old rowBorder
const converted = convertRowBorderStyleToBorder(props?.rowBorderStyle);
- if (isDesignMode) {
- console.warn('Row Border - rowBorderStyle:', props?.rowBorderStyle, 'converted:', converted, 'fallback:', props?.rowBorder);
- }
return converted || props?.rowBorder;
- }, [props?.rowBorderStyle, props?.rowBorder, isDesignMode]);
+ }, [props?.rowBorderStyle, props?.rowBorder]);🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dataTable/table/tableWrapper.tsx
around lines 70 to 96, remove the console.warn statements that log
rowDimensions/rowStylingBox/rowBorderStyle when isDesignMode is true; instead
either delete those console.warn lines entirely or replace them with a
design-mode-only logger that is stripped from production builds (e.g. call a
provided debugLogger guarded by a build-time DEV flag or a no-op in production).
Ensure the rest of the useMemo bodies return the converted value or fallback as
before and do not introduce new side effects.
This was referenced Dec 11, 2025
Merged
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
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.