fix: Enhance URL handling in table by rendering URL column types with <a> tag.
#37179
Conversation
… table cell components
…nhancing URL handling
…entProps and StyledEditIcon
WalkthroughThe changes introduced in this pull request focus on enhancing type safety and improving the rendering logic of components within the Table Widget. Key modifications include updating property types from 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
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11624484613. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.test.tsx (1)
8-24: Consider adding validation cases for URL propsWhile the default props are comprehensive, consider adding test cases for:
- Invalid URL formats
- Empty URL handling
- URL encoding scenarios
// Example additional test props: const urlTestCases = [ { url: "https://example.com/path with spaces", expected: "https://example.com/path%20with%20spaces" }, { url: "", expected: "#" }, { url: "invalid-url", expected: "#" } ];app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx (1)
Line range hint
78-93: Address TODO comment regarding any typeWhile the type improvements are good, there's a TODO comment about fixing the
anytype that should be addressed.Would you like me to help create a more specific type for the
valueprop?app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (1)
165-171: Remove unnecessary break after returnThe break statement after the return is unreachable and should be removed.
case ColumnTypes.BUTTON: if (props.title) { return content; } - break;app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (1)
Line range hint
65-101: Consider splitting SelectProps interfaceThe SelectProps interface has grown quite large with many responsibilities. Consider splitting it into smaller, focused interfaces:
- ISelectEditProps for edit-mode properties
- ISelectViewProps for view-mode properties
- ISelectCommonProps for shared properties
Example structure:
interface ISelectCommonProps extends BaseCellComponentProps { columnType: ColumnTypes; value: string; // ... other common props } interface ISelectEditProps extends ISelectCommonProps { onFilterChange: (text: string, rowIndex: number, ...) => void; onItemSelect: (value: string | number, ...) => void; // ... other edit-specific props } interface ISelectViewProps extends ISelectCommonProps { // ... view-specific props } type SelectProps = ISelectEditProps & ISelectViewProps;app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2)
Line range hint
89-99: Consider adding URL validationWhile the URL handling logic is correct, consider adding URL format validation to prevent invalid URLs from being rendered as links.
if (value && columnType === ColumnTypes.URL && displayText) { + try { + new URL(value.toString()); text = displayText; + } catch (e) { + text = value.toString(); + } } else if (columnType === ColumnTypes.CURRENCY) {
Line range hint
235-244: Add default case to switch statementConsider adding a default case to handle any future column types that might be added to the ColumnTypes enum.
switch (columnType) { case ColumnTypes.NUMBER: return [InputTypes.NUMBER, "NUMBER"]; case ColumnTypes.CURRENCY: return [InputTypes.CURRENCY, "NUMBER"]; default: return [InputTypes.TEXT, "TEXT"]; + // Add comment explaining why TEXT is the default choice }app/client/src/widgets/TableWidgetV2/component/Constants.ts (1)
549-549: Type enhancement looks good.The change from
stringtoCompactModeTypesenum improves type safety by restricting possible values.Consider adding JSDoc comments to document the properties of
BaseCellComponentPropsinterface, especially describing the purpose and impact of eachCompactModeTypesvalue.Example:
/** * Base properties for table cell components */ export interface BaseCellComponentProps { /** Controls the cell's display mode. Affects row height and styling. */ compactMode: CompactModeTypes; // ... other properties }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/client/src/widgets/TableWidgetV2/component/Constants.ts(1 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.test.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx(5 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx(2 hunks)
🔇 Additional comments (14)
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.test.tsx (2)
1-7: LGTM: Imports are well-organized and complete
All necessary testing utilities and component dependencies are properly imported.
30-42:
Add tests for right-click functionality
The linked issue #24769 specifically mentions right-click functionality on URL columns, but this isn't covered in the current test suite.
Add test case for right-click functionality:
it("handles right-click on URL links", () => {
render(
<BasicCell
{...defaultProps}
columnType={ColumnTypes.URL}
url="http://example.com"
/>,
);
const link = screen.getByRole("link");
fireEvent.contextMenu(link);
// Add assertions for context menu behavior
});app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx (3)
2-2: LGTM: Import changes align with new requirements
The added imports support the enhanced type safety and performance optimizations.
Also applies to: 5-5, 10-10
59-59: Type change validation needed for StyledEditIcon
The compactMode prop type has been updated from string to CompactModeTypes enum.
✅ Verification successful
Type change is safe and correctly implemented
The change from string to CompactModeTypes is type-safe because:
- CompactModeTypes is properly defined as an enum with values "SHORT", "DEFAULT", "TALL"
- StyledEditIcon in BasicCell.tsx already uses CompactModeTypes in its styled-component props interface
- All usages pass the compactMode prop directly without string literals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing usage of StyledEditIcon to verify compatibility
rg -l "StyledEditIcon" | xargs rg "compactMode.*=.*['\"].*['\"]"
Length of output: 68
Script:
#!/bin/bash
# Let's first check where StyledEditIcon is defined
rg -l "StyledEditIcon"
# Then search for all usages of compactMode with StyledEditIcon
rg "StyledEditIcon.*compactMode" -A 2 -B 2
# Also check the enum definition
rg "enum.*CompactModeTypes"
# And check all places where compactMode is used with any component
rg "compactMode={" -A 1 -B 1
Length of output: 661
Script:
#!/bin/bash
# Let's check the actual usage of StyledEditIcon with proper escaping
rg "compactMode\s*=" app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx app/client/src/widgets/TableWidgetV2/component/cellComponents/HeaderCell.tsx
# Check the enum definition to understand valid values
cat app/client/src/widgets/TableWidgetV2/component/Constants.ts
# Check if StyledEditIcon is using compactMode in its styled-components definition
rg "const StyledEditIcon.*=.*styled" -A 5 app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx app/client/src/widgets/TableWidgetV2/component/cellComponents/HeaderCell.tsx
Length of output: 18233
169-169: Verify content ref usage with URL rendering
The content ref is now being applied to a wrapper around the URL anchor tag. Ensure this doesn't affect any existing functionality that relies on the ref.
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (5)
140-140: LGTM: Type safety improvement
The change from string to ColumnTypes enum enhances type checking and prevents invalid column type assignments.
162-164: LGTM: Clean separation of concerns
The isHyperLink flag improves code readability and reusability.
172-183: LGTM: Enhanced URL rendering
The URL case properly handles both the link text and icon, improving visual feedback for users.
213-213: LGTM: Clean content rendering
The contentToRender variable is properly used here, maintaining consistency with the switch statement logic above.
199-208: 🛠️ Refactor suggestion
Consider URL sanitization and handler optimization
Two suggestions for improvement:
- URLs should be sanitized before opening in a new window to prevent potential security issues.
- The onClick handler could be memoized to prevent unnecessary re-renders.
Consider applying this optimization:
const handleUrlClick = React.useCallback(
(e: React.MouseEvent<HTMLDivElement>) => {
e.stopPropagation();
// Add URL sanitization here
window.open(props.url, "_blank");
},
[props.url]
);app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (2)
14-14: LGTM: Type safety improvement
The change from string to ColumnTypes enum improves type safety and maintains consistency across the table components.
Also applies to: 65-65
Line range hint 236-259: Verify URL handling in BasicCell
Since this component delegates rendering to BasicCell, we should verify that URL columns are properly rendered as clickable links.
✅ Verification successful
Let me analyze the actual file location and URL handling implementation before making any conclusions about the verification.
URL handling is properly implemented in BasicCell
The code correctly renders URL columns as clickable links using an anchor tag when columnType is ColumnTypes.URL. The implementation is clean and follows React best practices with proper memoization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL handling logic in BasicCell
ast-grep --pattern 'columnType === $_ { $$$' BasicCell.tsx
Length of output: 121
Script:
#!/bin/bash
# First, let's find the correct path to BasicCell
fd -e tsx -e ts "BasicCell"
# Then search for URL handling in those files
rg -l "columnType.*url" $(fd -e tsx -e ts "BasicCell")
# Get the implementation details with context
rg -A 5 "columnType.*url" $(fd -e tsx -e ts "BasicCell")
Length of output: 1281
Script:
#!/bin/bash
# Let's get more context around the URL handling implementation
rg -B 10 -A 10 "columnType.*url" app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx
# Also check for any URL-related rendering logic
ast-grep --pattern 'const $_ = useMemo(() => {
$$$
url
$$$
}, [$_]);' app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx
Length of output: 843
app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2)
42-42: LGTM: Type safety improvement
The change from string to ColumnTypes enum enhances type safety and prevents invalid column type assignments.
Line range hint 315-316: LGTM: Clean URL handling implementation
The separation of URL value and display text through dedicated props provides a clean solution for the right-click functionality requirement.
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.test.tsx
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11625124500. |
…f table widget Fixes #24769
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11625241932. |
|
Deploy-Preview-URL: https://ce-37179.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx (1)
Line range hint
1-150: Comprehensive test coverage for URL handling.The test suite effectively covers the core functionality including URL handling, tooltip behavior, and text truncation. However, given the PR's focus on URL column types and
<a>tag rendering, consider adding these test cases:
- Verify proper rendering of URL columns with the
<a>tag- Test URL validation
- Test accessibility attributes for URL links
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx(1 hunks)
🔇 Additional comments (1)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx (1)
57-59: Verify the empty tooltip test case behavior.
The test case now uses "Empty button" instead of an empty string. Let's verify this aligns with the component's expected behavior.
✅ Verification successful
Test case modification is appropriate
The change from empty string to "Empty button" in the test case is correct. Looking at the test file, it's clear this is a behavioral test verifying that tooltips don't appear for non-truncated button text, regardless of the title value. The test maintains its original intent while using a more descriptive test value that better represents real-world usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for empty string handling in the component implementation
ast-grep --pattern 'function AutoToolTipComponent($$$) {
$$$
if (title === "") {
$$$
}
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's check the actual implementation of AutoToolTipComponent
rg -A 10 "export (default )?function AutoToolTipComponent" --type typescript
# Also check for any empty string handling in the component
rg "title(\s+)?==(\s+)?['\"]\"" --type typescript
# Let's also check the test file content for context
cat "app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx"
Length of output: 4498
Script:
#!/bin/bash
# Let's check the implementation file
cat "app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.tsx"
# Also check for any related files that might define the component's behavior
fd AutoTooltipComponent.tsx -x cat {}
Length of output: 253
… interaction in table widget
…o rahulbarwal/issue24769
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11663305463. |
|
Deploy-Preview-URL: https://ce-37179.dp.appsmith.com |
… and maintainability in table widget
…o rahulbarwal/issue24769
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11677499937. |
|
Deploy-Preview-URL: https://ce-37179.dp.appsmith.com |
… to streamline the test suite This can be removed because, its only usage in `BasicCell` was sending URL type, which now handles it itself by rendering `<a>` tag instead of a div tag.
…tes, improving URL navigation verification in widget tests
rahulbarwal
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Url_Column_spec.ts (1)
Line range hint 1-42: Consider adding more comprehensive test coverage.
The test only verifies URL attributes but doesn't test:
- URL click behavior
- Error states
- Different URL formats
Let's check if there are other URL-related tests:
| agHelper | ||
| .GetElement(`${table._tableRowColumnData(0, 0, "v2")} a`) | ||
| .should( | ||
| "have.attr", | ||
| "href", | ||
| "https://randomuser.me/api/portraits/med/women/39.jpg", | ||
| ) | ||
| .should("have.attr", "target", "_blank"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor selectors and reduce code duplication.
- Use data-* attributes instead of complex selectors.
- Extract duplicate assertion logic into a reusable helper function.
- Move test URLs to constants.
Consider refactoring to:
const TEST_URLS = {
WOMAN_PROFILE: "https://randomuser.me/api/portraits/med/women/39.jpg",
MAN_PROFILE: "https://randomuser.me/api/portraits/med/men/52.jpg"
};
const verifyUrlColumn = (rowIndex: number, expectedUrl: string) => {
agHelper
.GetElement(`[data-table-url-cell="${rowIndex}"]`)
.should("have.attr", "href", expectedUrl)
.should("have.attr", "target", "_blank");
};
// Usage in test
verifyUrlColumn(0, TEST_URLS.WOMAN_PROFILE);
verifyUrlColumn(3, TEST_URLS.MAN_PROFILE);Also applies to: 32-39
Description
Problem
URLs in table were not being rendered as links, resulting in inconsistent user experience(missing context menus.
Root cause
URLs were rendered in
<div>instead of<a>, making the component lack links related features..Solution
This PR handles...
Rendering URLs as links in BasicCell for a better user experience.
Adding specific types for column properties for more robust data validation and type checking.
Adding unit tests for BasicCell functionality to ensure accurate rendering and behavior.
Simplifies the AutoToolTipComponent by removing unncessary
LinkWrappercomponentFixes #24769
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="@tag.Table"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11681339029
Commit: b7c5d17
Cypress dashboard.
Tags:
@tag.TableSpec:
Tue, 05 Nov 2024 10:23:38 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
compactModeandcolumnTypeproperties across various components.AutoToolTipComponentfor better control based oncolumnType.BasicCellcomponent usinguseMemo.Bug Fixes
BasicCell,PlainTextCell, andSelectCellcomponents.AutoToolTipComponentto ensure accurate rendering.Tests
BasicCellcomponent, ensuring proper rendering and interaction behaviors.AutoToolTipComponentto verify accurate rendering under various conditions.