feat:[POC] HTML column type support in table #37603
Conversation
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Enhancement
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11934394632. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/widgets/TableWidgetV2/constants.ts (1)
149-149: Consider adding HTML to ReadOnlyColumnTypesIf HTML columns are meant to be read-only (which is typical for security reasons), consider adding the HTML type to the ReadOnlyColumnTypes enum.
Also applies to: 151-161
app/client/src/widgets/TableWidgetV2/component/Table.tsx (1)
333-335: Add documentation for virtual rendering conditions.The logic correctly disables virtual rendering for HTML columns, but consider adding a comment explaining why HTML columns require static rendering (e.g., variable height content).
shouldUseVirtual = props.serverSidePaginationEnabled && !props.columns.some( (column) => + // Disable virtual rendering for wrapped cells and HTML content due to variable heights !!column.columnProperties.allowCellWrapping || column.metaProperties?.type === ColumnTypes.HTML, );app/client/src/widgets/TableWidgetV2/component/cellComponents/HTMLCell.tsx (1)
68-68: Remove unused prop 'fontSize' from 'HTMLCellProps'The 'fontSize' prop is declared but not used in the component. Consider removing it to keep the interface clean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/client/src/widgets/TableWidgetV2/component/Table.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/HTMLCell.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/constants.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts(2 hunks)
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/constants.ts (2)
149-149: Evaluate if HTML columns should be filterable
Consider whether HTML columns should be included in FilterableColumnTypes. If the HTML content contains plain text that users might want to filter on, it could be valuable to add it to the FilterableColumnTypes array.
Also applies to: 163-170
149-149:
Verify HTML content sanitization implementation
Adding HTML column type support requires careful consideration of XSS vulnerabilities. Ensure that HTML content is properly sanitized before rendering.
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts (2)
45-48: LGTM! HTML column type option added correctly.
The implementation follows the existing pattern and maintains alphabetical ordering.
45-48: Verify HTML content sanitization implementation.
When rendering HTML content in table cells, ensure proper sanitization is implemented in the HTMLCell component to prevent XSS attacks.
Also applies to: 180-180
app/client/src/widgets/TableWidgetV2/component/Table.tsx (1)
33-37: LGTM! Clean import organization.
Import statement is well-structured with proper type imports.
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
142-142: LGTM!
Clean import of the HTMLCell component.
2521-2537: LGTM with suggestions for HTML sanitization.
The HTML column type implementation follows the established pattern and includes all necessary cell properties. However, ensure that the HTMLCell component implements proper HTML sanitization to prevent XSS attacks.
| const inteweaveCompatibleValue = useMemo(() => { | ||
| if (value === null || value === undefined) return ""; | ||
|
|
||
| return String(value); | ||
| }, [value]); |
There was a problem hiding this comment.
Fix typo in variable name 'inteweaveCompatibleValue'
The variable 'inteweaveCompatibleValue' seems to be misspelled. It should be 'interweaveCompatibleValue' to match the 'Interweave' library.
Apply this diff to correct the variable name:
- const inteweaveCompatibleValue = useMemo(() => {
+ const interweaveCompatibleValue = useMemo(() => {Also, update all usages of this variable accordingly.
Committable suggestion skipped: line range outside the PR's diff.
|
Deploy-Preview-URL: https://ce-37603.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
…o rahulbarwal/Issue13681/-Feature]--Support-HTML-as-a-column-type-in-the-table-widget
This commit adds cell wrapping support to the Table widget with the following changes: - Add allowCellWrapping property that controls text wrapping behavior in table cells - Update cell styling to use break-spaces and word-break when wrapping is enabled - Adjust cell height calculations to accommodate wrapped content - Disable virtual scrolling when any column has cell wrapping enabled for better rendering - Update inline cell editor height to be dynamic when wrapping is enabled Key changes: - TableStyledWrappers.tsx: Add conditional cell wrapping styles (lines 579-588) - InlineCellEditor.tsx: Update height calculation for wrapped cells (lines 49-62) - Table.tsx: Disable virtual scrolling when cell wrapping is enabled (lines 330-334) - PlainTextCell.tsx: Add content height detection for wrapped cells (lines 102-107) This improves content visibility in table cells by allowing text to wrap instead of being truncated with ellipsis, while maintaining performance optimizations where possible.
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12082842511. |
|
Deploy-Preview-URL: https://ce-37603.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12083755845. |
|
Deploy-Preview-URL: https://ce-37603.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12116632335. |
rahulbarwal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/client/src/components/propertyControls/HTMLTableComputeValue.tsx (2)
5-7: Add JSDoc comments to document HTML support capabilitiesThe interface should include documentation about HTML rendering capabilities and any restrictions.
+/** + * Props for HTML compute control in table columns + * @property {string} defaultValue - Default HTML content for the column + */ export interface HTMLComputeTablePropertyControlProps extends ControlProps { defaultValue?: string; }
9-25: Consider adding HTML content validationThe component accepts any string as HTML content without validation. Consider adding runtime checks for valid HTML structure.
class HTMLComputeTablePropertyControl extends ComputeTablePropertyControlV2 { + private validateHTML(content: string): boolean { + const parser = new DOMParser(); + const doc = parser.parseFromString(content, 'text/html'); + return !doc.querySelector('parsererror'); + } render() { + if (this.props.defaultValue && !this.validateHTML(this.props.defaultValue)) { + console.warn('Invalid HTML content provided'); + } return ( <ComputeTablePropertyControlV2 {...this.props} additionalControlData={{ ...this.props.additionalControlData, isExpectingHTML: true, }} /> ); }app/client/src/components/propertyControls/TableComputeValue.tsx (1)
16-16: Group related imports togetherThe normalizedMultilineValue import should be grouped with other utility imports.
-import { JSToString, normalizedMultilineValue, stringToJS } from "./utils"; +import { + JSToString, + normalizedMultilineValue, + stringToJS, +} from "./utils";app/client/src/components/propertyControls/index.ts (1)
Line range hint
142-177: Add HTMLComputeTablePropertyControl's props type to PropertyControlPropsType.The props type for the new HTMLComputeTablePropertyControl is missing from the PropertyControlPropsType union. This could cause TypeScript compilation issues.
Add the type to the union:
export type PropertyControlPropsType = | ControlProps | InputControlProps | DropDownControlProps + | HTMLComputeTablePropertyControlProps // ... other types
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/src/components/propertyControls/HTMLTableComputeValue.tsx(1 hunks)app/client/src/components/propertyControls/TableComputeValue.tsx(2 hunks)app/client/src/components/propertyControls/index.ts(2 hunks)app/client/src/components/propertyControls/utils.ts(1 hunks)
🔇 Additional comments (1)
app/client/src/components/propertyControls/index.ts (1)
80-80: LGTM on the HTMLComputeTablePropertyControl integration.
The import and export additions follow the existing patterns and maintain alphabetical ordering.
Also applies to: 105-105
| export const normalizedMultilineValue = (value: string) => { | ||
| return value.replace(/'/g, '"').replace(/\n/g, "\\n"); | ||
| }; |
There was a problem hiding this comment.
Add HTML sanitization to prevent XSS attacks
The normalizedMultilineValue function handles string normalization but doesn't sanitize HTML content. This could expose the application to XSS vulnerabilities when rendering HTML in table cells.
Consider using DOMPurify or a similar library to sanitize HTML content:
export const normalizedMultilineValue = (value: string) => {
- return value.replace(/'/g, '"').replace(/\n/g, "\\n");
+ return DOMPurify.sanitize(value.replace(/'/g, '"').replace(/\n/g, "\\n"));
};Committable suggestion skipped: line range outside the PR's diff.
| // Handle multiline strings by replacing newlines with spaces | ||
| let stringToEvaluate = ""; | ||
|
|
||
| if (this.props.additionalControlData?.isExpectingHTML) { | ||
| stringToEvaluate = stringToJS(normalizedMultilineValue(value)); | ||
| } else { | ||
| stringToEvaluate = stringToJS(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for HTML string processing
The HTML string processing lacks error handling for malformed content.
let stringToEvaluate = "";
if (this.props.additionalControlData?.isExpectingHTML) {
+ try {
stringToEvaluate = stringToJS(normalizedMultilineValue(value));
+ } catch (error) {
+ console.error('Failed to process HTML content:', error);
+ return value; // Fallback to original value
+ }
} else {
stringToEvaluate = stringToJS(value);
}📝 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.
| // Handle multiline strings by replacing newlines with spaces | |
| let stringToEvaluate = ""; | |
| if (this.props.additionalControlData?.isExpectingHTML) { | |
| stringToEvaluate = stringToJS(normalizedMultilineValue(value)); | |
| } else { | |
| stringToEvaluate = stringToJS(value); | |
| } | |
| // Handle multiline strings by replacing newlines with spaces | |
| let stringToEvaluate = ""; | |
| if (this.props.additionalControlData?.isExpectingHTML) { | |
| try { | |
| stringToEvaluate = stringToJS(normalizedMultilineValue(value)); | |
| } catch (error) { | |
| console.error('Failed to process HTML content:', error); | |
| return value; // Fallback to original value | |
| } | |
| } else { | |
| stringToEvaluate = stringToJS(value); | |
| } |
|
Deploy-Preview-URL: https://ce-37603.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
not required any more. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or
extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any
dependencies that are required for this change. Add links to Notion,
Figma or any other documents that might be relevant to the PR.
Fixes #13681
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD cd9422d yet
Mon, 02 Dec 2024 09:42:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
HTMLCellcomponent for rendering HTML content within table cells.Bug Fixes
Enhancements