fix: add ellipsis tooltip component on mui tables#266
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a shared ChangesMUI text truncation
Sequence Diagram(s)sequenceDiagram
participant User
participant TruncateText
participant Element
participant HTMLElement
participant Tooltip
User->>TruncateText: hover
TruncateText->>Element: read scrollWidth
TruncateText->>HTMLElement: read offsetWidth
TruncateText->>TruncateText: compare widths or apply charLimit
alt truncated output
TruncateText->>Tooltip: show full text
else text fits
TruncateText->>Tooltip: hide tooltip title
end
Tooltip->>User: display tooltip state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/editable-table/mui-table-editable.js`:
- Line 322: The EllipsisTooltip title currently uses String(row[col.columnKey]
?? "") which yields unhelpful results for objects/arrays; update the cell
rendering around EllipsisTooltip (where title is set) to use a safe formatter
(e.g., a small helper function like formatTooltipValue or reuse the base-table
formatter) that: checks for null/undefined, returns primitives via String(...),
and for objects/arrays attempts JSON.stringify with a try/catch (falling back to
Object.prototype.toString or an empty string) so the tooltip shows useful text
for complex values; update the title prop to call that formatter instead of
String(row[col.columnKey]).
- Around line 321-324: The tooltip currently always uses raw data
(String(row[col.columnKey])) while the cell shows col.render(row), causing a
mismatch; update the EllipsisTooltip title to mirror the rendered cell by using
col.render(row) when col.render is present (and falling back to
String(row[col.columnKey] ?? "") otherwise), converting any React node to text
the same way the base table fix does (e.g., via the existing helper or
ReactDOMServer.renderToStaticMarkup) so the tooltip content matches the
displayed cell; change the usage around EllipsisTooltip/title near col.render
and row[col.columnKey].
In `@src/components/mui/sortable-table/mui-table-sortable.js`:
- Line 223: The EllipsisTooltip title currently uses String(row[col.columnKey]
?? "") which yields unhelpful "[object Object]" for non-primitive values; update
the tooltip value handling in the component that renders EllipsisTooltip (look
for the JSX containing EllipsisTooltip and the variables row and col.columnKey)
to guard against objects by converting primitives with String(...) but for
objects/arrays attempt a safe JSON.stringify (with a try/catch and a fallback
like "(object)") so the title shows meaningful text for non-string values;
optionally extract this logic to a small helper (e.g., toSafeTitle(value)) and
use that when passing title to EllipsisTooltip.
- Around line 222-228: The tooltip currently uses the raw row[col.columnKey]
value instead of the rendered cell content; change the render so the
EllipsisTooltip title uses the same displayed value as the cell by computing a
displayedValue = col.render?.(row) || row[col.columnKey] (or similar) and then
pass title={String(displayedValue ?? "")} to EllipsisTooltip; update the code
paths that render the cell (the branch with EllipsisTooltip and the non-ellipsis
branch) to use this displayedValue so the tooltip matches the rendered output
(refer to col.render, row, columnKey, and EllipsisTooltip in
mui-table-sortable.js).
In `@src/components/mui/table/mui-table.js`:
- Line 172: The tooltip is using raw row[col.columnKey] instead of the displayed
rendered cell; update the EllipsisTooltip usage in mui-table.js to prefer a
column-provided tooltip string (e.g., col.tooltipContent?.(row) or
col.tooltipContent) and otherwise derive text from the rendered cell by calling
col.render(row) and converting the resulting React node to plain text
(strip/serialize children or use a small helper like renderToString/flattenText)
so the tooltip matches the visible cell; ensure you reference EllipsisTooltip,
col.render, col.tooltipContent and row when implementing the fallback.
- Line 172: The tooltip title currently uses String(row[col.columnKey] ?? "")
which yields "[object Object]" for objects/arrays; update the EllipsisTooltip
title logic (in the component rendering the table row/col — locate
EllipsisTooltip usage with row and col.columnKey) to first inspect the value: if
it's string/number/boolean use String(value), if it's object/array attempt
JSON.stringify(value) inside a try/catch and fall back to a short placeholder
like "" on error/too-large result; ensure the final value passed to
EllipsisTooltip.title is always a string.
- Around line 157-179: renderCell currently passes the raw value
row[col.columnKey] into EllipsisTooltip causing mismatched tooltip vs rendered
cell when col.render transforms data; update renderCell to derive the tooltip
text from the rendered content when col.render exists (or support an explicit
col.tooltipContent function) — e.g., compute tooltipTitle = col.tooltipContent ?
col.tooltipContent(row) : (col.render ?
extractTextFromRenderedOutput(col.render(row)) : String(row[col.columnKey] ??
"")) and pass tooltipTitle to EllipsisTooltip, keeping the visible cell content
as col.render(row) or content; touch renderCell, col.render, col.ellipsis and
EllipsisTooltip usage to implement this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 873100c9-90af-4584-a713-72ca0a5e28c9
📒 Files selected for processing (6)
src/components/mui/__tests__/ellipsis-tooltip.test.jssrc/components/mui/__tests__/mui-table.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/ellipsis-tooltip.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/mui-table.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
hey @tomrndom this looks good but I think we can make it a little bit better if we call the col prop truncateText and allow passing true when you want to just use the col width, or a number of characters . wdyt ?
@santipalenque I'll rename the col prop to truncateText.
I took a closer look at this, and it seems a bit messy to add a character number to truncate since the rendered content is always wrapped in a html span tag, this would required to pass to the ellipsis tooltip component new props as raw content and if the column has a render function, internally get the result of the render function, evaluate if it's a string and then truncate the text to display and get the tooltip with the full content. I think that with the current approach using the width of the column we can achieve a similar result without making this component more complex and condition the posibble render based on the content of the cell let me know what you think |
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/sortable-table/mui-table-sortable.js (1)
213-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse existence check for
col.render, not truthiness of its result.
col.render?.(row) || <span>…</span>falls back to the rawrow[col.columnKey]wheneverrenderreturns a falsy node (e.g.0,"",null,false). A custom renderer that intentionally returns one of those will be silently overridden with raw data. The editable table at line 294 already guards on the function's existence (col.render ? col.render(row) : …); align this variant for consistency.🐛 Proposed fix
- const cellContent = col.render?.(row) || <span>{row[col.columnKey]}</span>; + const cellContent = col.render + ? col.render(row) + : <span>{row[col.columnKey]}</span>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/sortable-table/mui-table-sortable.js` around lines 213 - 214, The sortable table cell rendering in the columns.map block is using the truthiness of col.render(row) to decide the fallback, which incorrectly overrides valid falsy render outputs. Update the rendering logic in mui-table-sortable.js to check whether col.render exists before calling it, matching the existing pattern used by the editable table, and only fall back to row[col.columnKey] when no renderer is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/mui/sortable-table/mui-table-sortable.js`:
- Around line 213-214: The sortable table cell rendering in the columns.map
block is using the truthiness of col.render(row) to decide the fallback, which
incorrectly overrides valid falsy render outputs. Update the rendering logic in
mui-table-sortable.js to check whether col.render exists before calling it,
matching the existing pattern used by the editable table, and only fall back to
row[col.columnKey] when no renderer is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 297ca81f-1d97-4716-b8b2-b026f9461b33
📒 Files selected for processing (5)
src/components/mui/__tests__/ellipsis-tooltip.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/ellipsis-tooltip.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/mui-table.js
✅ Files skipped from review due to trivial changes (1)
- src/components/mui/tests/ellipsis-tooltip.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/mui/table/mui-table.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/mui/table/mui-table.js (1)
158-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve custom boolean renderers before default icon mapping.
Line 158 short-circuits on boolean values before
col.render, so any column that intentionally renders boolean-backed data (e.g., custom labels/chips) is now ignored.Suggested fix
const renderCell = (row, col) => { - if (isBoolean(row[col.columnKey])) { - return row[col.columnKey] ? ( - <CheckIcon fontSize="large" /> - ) : ( - <CloseIcon fontSize="large" /> - ); - } + if (col.render) { + return col.render(row); + } + + if (isBoolean(row[col.columnKey])) { + return row[col.columnKey] ? ( + <CheckIcon fontSize="large" /> + ) : ( + <CloseIcon fontSize="large" /> + ); + } if (col.truncateText) { return ( <TruncateText charLimit={col.truncateText}> - {col.render ? col.render(row) : row[col.columnKey]} + {row[col.columnKey]} </TruncateText> ); } - - const content = col.render - ? col.render(row) - : <span style={{ fontWeight: "normal" }}>{row[col.columnKey]}</span>; - - return content; + return <span style={{ fontWeight: "normal" }}>{row[col.columnKey]}</span>; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/table/mui-table.js` around lines 158 - 164, The boolean short-circuit in MUITable’s cell rendering is bypassing any custom column renderer, so boolean-backed columns can no longer use their own labels/chips. Update the rendering flow in MUITable so col.render is checked and used before the default boolean icon mapping, and only fall back to CheckIcon/CloseIcon when no custom renderer is provided. Preserve the existing boolean default behavior for columns that do not define a renderer.src/components/mui/sortable-table/mui-table-sortable.js (1)
214-214: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse nullish fallback for rendered cell content.
Line 214 uses
||, so valid rendered values like0,false, or""are treated as empty and replaced with raw row data.Suggested fix
-const cellContent = col.render?.(row) || <span>{row[col.columnKey]}</span>; +const cellContent = col.render?.(row) ?? <span>{row[col.columnKey]}</span>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/sortable-table/mui-table-sortable.js` at line 214, The cell rendering fallback in mui-table-sortable.js uses a truthy check, so valid rendered values like 0, false, or empty strings get replaced by raw row data. Update the cellContent assignment in the sortable table render path to use a nullish fallback instead of a logical OR, keeping col.render?.(row) when it returns any defined value and only falling back to row[col.columnKey] when it is null or undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/truncate-text.js`:
- Around line 31-33: The TruncateText component’s children prop is too
restrictive because it is typed as PropTypes.string even though table renderers
can pass React elements from col.render?.(row). Update TruncateText.propTypes so
children uses PropTypes.node, keeping the existing truncation logic in
TruncateText unchanged since it already only truncates when children is a
string. Ensure the change is made on the TruncateText propTypes definition so
non-string nodes no longer trigger console warnings.
---
Outside diff comments:
In `@src/components/mui/sortable-table/mui-table-sortable.js`:
- Line 214: The cell rendering fallback in mui-table-sortable.js uses a truthy
check, so valid rendered values like 0, false, or empty strings get replaced by
raw row data. Update the cellContent assignment in the sortable table render
path to use a nullish fallback instead of a logical OR, keeping
col.render?.(row) when it returns any defined value and only falling back to
row[col.columnKey] when it is null or undefined.
In `@src/components/mui/table/mui-table.js`:
- Around line 158-164: The boolean short-circuit in MUITable’s cell rendering is
bypassing any custom column renderer, so boolean-backed columns can no longer
use their own labels/chips. Update the rendering flow in MUITable so col.render
is checked and used before the default boolean icon mapping, and only fall back
to CheckIcon/CloseIcon when no custom renderer is provided. Preserve the
existing boolean default behavior for columns that do not define a renderer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff33730b-2da1-4196-a364-8ac451486118
📒 Files selected for processing (6)
src/components/mui/__tests__/mui-table.test.jssrc/components/mui/__tests__/truncate-text.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/mui-table.jssrc/components/mui/truncate-text.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/mui/tests/mui-table.test.js
| TruncateText.propTypes = { | ||
| children: PropTypes.string, | ||
| charLimit: PropTypes.oneOfType([PropTypes.bool, PropTypes.number]), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
Change children PropTypes to PropTypes.node to support table renderers.
Table components pass the result of col.render?.(row) (which may be a React element/JSX) to TruncateText. The current PropTypes.string restriction will cause console warnings when non-string nodes are passed.
The component's internal logic already handles non-string children safely (only truncating if typeof children === "string"), so changing the prop type to node allows for more flexible usage without breaking existing behavior.
Suggested fix
TruncateText.propTypes = {
- children: PropTypes.string,
+ children: PropTypes.node,
charLimit: PropTypes.oneOfType([PropTypes.bool, PropTypes.number]),
};📝 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.
| TruncateText.propTypes = { | |
| children: PropTypes.string, | |
| charLimit: PropTypes.oneOfType([PropTypes.bool, PropTypes.number]), | |
| TruncateText.propTypes = { | |
| children: PropTypes.node, | |
| charLimit: PropTypes.oneOfType([PropTypes.bool, PropTypes.number]), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/mui/truncate-text.js` around lines 31 - 33, The TruncateText
component’s children prop is too restrictive because it is typed as
PropTypes.string even though table renderers can pass React elements from
col.render?.(row). Update TruncateText.propTypes so children uses
PropTypes.node, keeping the existing truncation logic in TruncateText unchanged
since it already only truncates when children is a string. Ensure the change is
made on the TruncateText propTypes definition so non-string nodes no longer
trigger console warnings.
…djust tests, render cell function for all tables Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/mui/table/mui-table.js (1)
120-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
wordBreak: "break-all"may split words mid-character.
break-allbreaks between any two characters, which can produce awkward wrapping for normal text (e.g. emails/URLs aside).break-word/overflow-wrap: anywhereonly breaks long unbreakable tokens. Consider whether the gentler option is preferable for these data cells, since the same value is also applied in the editable and sortable variants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/table/mui-table.js` at line 120, The text wrapping in the table cell styles uses wordBreak in the MUI table component, which can split normal words mid-character; update the shared cell styling in mui-table.js to use a gentler wrapping approach like break-word or overflow-wrap:anywhere instead, and make sure the change applies consistently to the editable and sortable variants that reuse the same style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/table/utils.js`:
- Around line 7-18: The renderCell helper now returns col.render(row)
immediately, which skips the truncateText behavior for columns that define both
render and truncateText. Update renderCell to preserve the original priority
order by checking isBoolean first, then applying col.render output through the
truncateText wrapper when col.truncateText is set, and only otherwise returning
the raw rendered value.
---
Nitpick comments:
In `@src/components/mui/table/mui-table.js`:
- Line 120: The text wrapping in the table cell styles uses wordBreak in the MUI
table component, which can split normal words mid-character; update the shared
cell styling in mui-table.js to use a gentler wrapping approach like break-word
or overflow-wrap:anywhere instead, and make sure the change applies consistently
to the editable and sortable variants that reuse the same style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa5525cb-9b1e-4b08-8ce0-b0fcc39dca4d
📒 Files selected for processing (7)
src/components/mui/__tests__/mui-table.test.jssrc/components/mui/__tests__/truncate-text.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/mui-table.jssrc/components/mui/table/utils.jssrc/components/mui/truncate-text.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/mui/sortable-table/mui-table-sortable.js
- src/components/mui/truncate-text.js
- src/components/mui/tests/truncate-text.test.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
2 participants
ref: https://app.clickup.com/t/86bacrq3e
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit