Table visualization re-write #11031
Table visualization re-write #11031
Conversation
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
abbyhu2000
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
raintygao,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
abbyhu2000
requested review from
TackAdam,
paulstn,
ps48,
ruchidh and
sumukhswamy
as code owners
WalkthroughParent table rendering logic was extracted into a new TableVisDynamicTable component with its own stylesheet; the parent now delegates rendering and interactions (sorting, pagination, filtering, resizing) to that component while passing title, table, visConfig, event, and uiState. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableVisComponent
participant TableVisDynamicTable
participant TableUiState as UI State / Handlers
User->>TableVisComponent: open visualization / interact (click header, page, resize, filter)
TableVisComponent->>TableVisDynamicTable: render(title, table, visConfig, event, uiState)
TableVisDynamicTable->>TableUiState: read/write sort, colWidth, pageIndex, pageSize
TableVisDynamicTable->>TableUiState: emit filterBucket(event) when filter action triggered
TableUiState-->>TableVisDynamicTable: updated uiState (sort/width/page)
TableVisDynamicTable-->>User: update rendered rows/headers/pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (8)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.scss (2)
26-30: Consider adding focus-visible for keyboard accessibility.The hover and focus states correctly show the sort icon, but using
:focusmay cause unwanted visual effects on mouse clicks. Consider using:focus-visiblefor better keyboard-only focus indication.// Show sort icon on hover of the th element &:hover &__sortIcon, - &:focus &__sortIcon { + &:focus-visible &__sortIcon { opacity: 1; }
97-101: Row separator class defined but inline styles also used in TSX.This class
.tableVis-row-separator-cellis defined here, but looking at the TSX file (lines 515-524), inline styles are used instead for the separator row. Consider using this class consistently.src/plugins/vis_type_table/public/components/table_vis_component.tsx (1)
7-7: Unused import:EuiFlexItem.
EuiFlexItemis imported but not used in this file. TheTableVisDynamicTablecomponent handles theEuiFlexItemwrapper internally.-import { EuiFlexItem } from '@elastic/eui';src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (5)
47-64: Measuring element created without table reference on initial render.
createMeasuringElementaccepts an optionaltableElementparameter to copy font styles, but at line 272 it's called without passingtableRef.current. This means the font style copying logic (lines 52-59) is never utilized.- const measuringDiv = createMeasuringElement(); + const measuringDiv = createMeasuringElement(tableElement);Also applies to: 269-272
73-74: Unused propfieldMapping.The
fieldMappingprop is declared in the component signature but never used withinTableCell. Remove it to clean up the interface.filterBucket: (rowIndex: number, columnIndex: number, negate: boolean) => void; columnId: string; - fieldMapping?: any; }) => {Also applies to: 81-81
241-246: Potential redundant state update in pagination effects.The second
useEffectadjustspageIndexwhenpageCountchanges, but sincepageCountis derived fromrows.length, the first effect (lines 237-239) should already reset to page 0 when data changes. Consider combining these or documenting why both are needed.
352-352: Dependency oncolumnWidths.lengthmay cause unnecessary re-renders.Including
columnWidths.lengthin the dependency array means this effect re-runs whenever the number of columns in the width array changes. Since this effect itself setscolumnWidths, it could trigger re-execution. Consider using a ref to track if initial measurement has occurred, or remove this dependency if the intent is only to run on data/column changes.- }, [formattedColumns, rows, columnWidths.length]); + }, [formattedColumns, rows]);
492-500: Inline styles duplicate SCSS definitions.These inline styles (
padding: '8px',verticalAlign: 'top',wordBreak: 'break-word') duplicate the styles already defined intable_vis_dynamic_table.scss(lines 90-95). Consider removing the redundant inline styles and relying on the CSS class.style={{ width: typeof columnWidths[colIndex] === 'number' ? `${columnWidths[colIndex]}px` : 'auto', - padding: '8px', - verticalAlign: 'top', - wordBreak: 'break-word', }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/plugins/vis_type_table/public/components/table_vis_component.tsx(2 hunks)src/plugins/vis_type_table/public/components/table_vis_dynamic_table.scss(1 hunks)src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
src/plugins/data/common/index_patterns/fields/index_pattern_field.ts (1)
filterable(135-142)
src/plugins/vis_type_table/public/components/table_vis_component.tsx (1)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
TableVisDynamicTable(218-552)
🪛 ast-grep (0.40.0)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
[warning] 88-88: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
[error] 89-89: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (5)
src/plugins/vis_type_table/public/components/table_vis_component.tsx (1)
23-38: Clean delegation pattern.The component now acts as a stable public interface while delegating all logic to
TableVisDynamicTable. This maintains backward compatibility and allows for implementation swaps in the future.src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (4)
30-37: Well-defined constants for configuration.Good practice extracting magic numbers into named constants. This improves readability and makes future adjustments easier.
85-90: XSS risk mitigated via DOMPurify sanitization.Static analysis flagged
dangerouslySetInnerHTMLas a security risk. However, the content is properly sanitized usingdompurify.sanitize()at line 487 before being passed to this component. This is the correct approach for rendering formatted HTML content safely.
481-482: Using array index as React key.Using
rowIndexas the key may cause rendering issues if rows have internal state, since sorted/paginated data reorders rows. If rows have a unique identifier, prefer using that. If not, this is acceptable but worth noting as a potential source of subtle bugs if cell components ever maintain state.
223-223:colWidthdestructured but unused.
colWidthis destructured fromuiStateon line 223 but never referenced in the component. ThesetWidthfunction is used forhandleColumnResize, butcolWidthitself is not applied when calculatingcolumnWidths. If persisted column widths should be restored on component load, the initialization logic needs to incorporatecolWidth.
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
Outdated
Show resolved
Hide resolved
| {rowIndex < paginatedRows.length - 1 && ( | ||
| <tr> | ||
| <td | ||
| colSpan={formattedColumns.length} | ||
| style={{ | ||
| borderBottom: '1px solid #e0e0e0', | ||
| height: '1px', | ||
| padding: 0, | ||
| }} | ||
| /> | ||
| </tr> | ||
| )} |
There was a problem hiding this comment.
Hardcoded border color won't adapt to theme changes.
The separator uses inline borderBottom: '1px solid #e0e0e0' instead of the defined .tableVis-row-separator-cell class that uses the theme-aware $euiBorderThin variable. This will break in dark mode.
{rowIndex < paginatedRows.length - 1 && (
<tr>
<td
colSpan={formattedColumns.length}
- style={{
- borderBottom: '1px solid #e0e0e0',
- height: '1px',
- padding: 0,
- }}
+ className="tableVis-row-separator-cell"
/>
</tr>
)}📝 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.
| {rowIndex < paginatedRows.length - 1 && ( | |
| <tr> | |
| <td | |
| colSpan={formattedColumns.length} | |
| style={{ | |
| borderBottom: '1px solid #e0e0e0', | |
| height: '1px', | |
| padding: 0, | |
| }} | |
| /> | |
| </tr> | |
| )} | |
| {rowIndex < paginatedRows.length - 1 && ( | |
| <tr> | |
| <td | |
| colSpan={formattedColumns.length} | |
| className="tableVis-row-separator-cell" | |
| /> | |
| </tr> | |
| )} |
🤖 Prompt for AI Agents
In src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
around lines 514 to 525, the separator td uses a hardcoded inline style with
borderBottom: '1px solid #e0e0e0' which won't adapt to themes; remove the inline
borderBottom (and move any present height/padding if needed) and instead apply
the existing theme-aware className 'tableVis-row-separator-cell' to the td
(keeping colSpan and accessibility unchanged), and if height/padding are
required, move those rules into the component stylesheet alongside
.tableVis-row-separator-cell so the separator uses the theme variable
$euiBorderThin and respects dark mode.
| feat: | ||
| - Table visualization re-write ([#11031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11031)) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other changelog fragments to compare format
echo "=== Sample changelog fragments ==="
fd --type f '\.yml$' changelogs/fragments/ | head -10 | xargs -I {} sh -c 'echo "--- {} ---" && head -3 {}'
# Check for format documentation
echo ""
echo "=== Checking for format documentation ==="
find . -maxdepth 3 -type f \( -name 'CHANGELOG*' -o -name 'CONTRIBUTING*' -o -name 'README*' \) | head -5 | xargs -I {} sh -c 'echo "--- {} ---" && grep -A 5 -i "changelog.*format\|fragment" {} 2>/dev/null | head -10'Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 224
🌐 Web query:
OpenSearch Dashboards changelog fragment format conventions yml
💡 Result:
OpenSearch Dashboards uses the common “changelog fragment” YAML pattern (one small .yml/.yaml file per PR placed in changelogs/fragments/) — fragments are simple YAML mappings that declare the change category and a short human-readable summary/description. Typical conventions you can follow:
-
File placement/name
- put one file per PR in changelogs/fragments/, filename including PR number (e.g. 10369-fix-thing.yml). [1]
-
Top‑level shape (two common variants)
-
Category list form (Ansible-style): top-level keys are section names (bugfixes, minor_changes, major_changes, deprecated_features, removed_features, security, etc.) with a YAML list of short entries:
bugfixes:
- scope - short description starting lowercase and ending with a period.
- another_scope - another short description.
[2]
-
Fragment-file-per-change form (change-log tool style): a mapping with explicit fields:
category: added|changed|fixed|deprecated|removed|security
summary: "one-line summary (RST-capable)"
description: |
Longer reStructuredText description (optional; preserves newlines)
pull requests:- 10369
issues: - 1234
(version is optional and usually omitted; unversioned fragments go into the next release). [3]
- 10369
-
-
Text formatting
- Summary and description are usually interpreted as reStructuredText (escape RST/YAML-sensitive characters as needed). Keep summary short; use scope - description format for scoped items. [2][3]
-
Rules
- Use a new fragment file per PR (don’t append to existing fragment files). [2]
- Choose category consistent with your release notes tooling (added/changed/fixed/etc.). [3]
Example (recommended, change-log style):
category: fixed
summary: "discover - fix crash when index pattern contains invalid mapping."
description: |
Fixes a crash in Discover when an index pattern contains fields with invalid mapping types.
pull requests:
- 10369
issues: - 9876
References:
- Example of OpenSearch-Dashboards repo having changelog fragments. [1]
- Ansible changelog-fragment conventions and scope/entry format (category-list form and entry formatting). [2]
- change-log (fragment) YAML schema (category/summary/description/pull requests/issues). [3]
Revise changelog category to use standard naming.
The changelog fragment uses feat: as the category, which does not align with OpenSearch Dashboards changelog conventions. Use a standard category from the documented set: minor_changes, major_changes, bugfixes, deprecated_features, removed_features, or security.
For example:
minor_changes:
- Table visualization re-write ([#11031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11031))🤖 Prompt for AI Agents
In changelogs/fragments/11031.yml lines 1-2, the fragment uses the nonstandard
category key "feat:"; update it to one of the repository's standard changelog
categories (e.g., "minor_changes") and move the entry under that key so the YAML
matches the documented format — e.g., replace "feat:" with "minor_changes:" and
ensure the list item remains properly indented under the new key.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11031 +/- ##
==========================================
- Coverage 60.80% 60.79% -0.01%
==========================================
Files 4532 4538 +6
Lines 122278 122703 +425
Branches 20502 20617 +115
==========================================
+ Hits 74353 74603 +250
- Misses 42686 42814 +128
- Partials 5239 5286 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
…ds into table_vis
Signed-off-by: abbyhu2000 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
520-529: Use theme-aware separator styling instead of hardcoded border.The separator row still uses an inline
borderBottom: '1px solid #e0e0e0', which won’t adapt to different themes (e.g., dark mode), even though a.tableVis-row-separator-cellclass already exists in SCSS using$euiBorderThin.You can simplify and make this theme-safe by relying on the class:
- <td - colSpan={formattedColumns.length} - style={{ - borderBottom: '1px solid #e0e0e0', - height: '1px', - padding: 0, - }} - /> + <td + colSpan={formattedColumns.length} + className="tableVis-row-separator-cell" + />
🧹 Nitpick comments (5)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.scss (2)
51-78: Cell filter controls are mouse-hover only (keyboard users can’t access).
.tableVisCell__filteris only displayed on.tableVisCell:hover, so the per-cell filter buttons are effectively unreachable via keyboard focus.Consider also showing the filter controls on focus (e.g.,
:focus-within) so users tabbing into the cell or its buttons can see and use them.
19-40: Sort/action affordances visible only on hover; consider focus styles.The sort icon and
&__actionButtonare revealed on:hover/:focusbut the<th>elements aren’t currently keyboard-focusable. Once headers become focusable, you may want explicit focus styles (e.g.,:focus-visible) to ensure sort affordances and action buttons are clearly discoverable for keyboard users.src/plugins/vis_type_table/public/components/table_vis_dynamic_table.test.tsx (2)
213-242: Column resize test doesn’t verify any visible effect.The “should handle column resize correctly” test only asserts that
mockUiState.setWidthis called. If the component fails to update its owncolumnWidthsstate (or ignoresuiState.colWidth), this test will still pass even though columns don’t visually resize.Consider asserting on the header or cell
style.widthafter the drag simulation, so regressions in actual resizing behavior are caught.
80-95: MockingconvertToFormattedDatamay be unnecessary.The test suite mocks
convert_to_formatted_dataand sets amockFormattedDatareturn value, but the currentTableVisDynamicTableimplementation doesn’t appear to call this utility.If the component no longer relies on
convertToFormattedData, you can simplify these tests by dropping the mock and related setup; or, if you plan to reintroduce it, add at least one assertion that depends on the formatted result.src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
269-305:createMeasuringElementparameter is unused; either use it or simplify.
createMeasuringElementaccepts an optionaltableElementand has logic to copy table cell font styles, but the only call site (const measuringDiv = createMeasuringElement();) never passes the table, so the style-copying branch is currently dead.Either pass
tableElementin:- const measuringDiv = createMeasuringElement(); + const measuringDiv = createMeasuringElement(tableElement);or remove the unused parameter and related branch to keep the code simple.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/plugins/vis_type_table/public/components/table_vis_component.test.tsx(3 hunks)src/plugins/vis_type_table/public/components/table_vis_component.tsx(2 hunks)src/plugins/vis_type_table/public/components/table_vis_dynamic_table.scss(1 hunks)src/plugins/vis_type_table/public/components/table_vis_dynamic_table.test.tsx(1 hunks)src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.test.tsx (1)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
TableVisDynamicTable(218-558)
src/plugins/vis_type_table/public/components/table_vis_component.tsx (1)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
TableVisDynamicTable(218-558)
src/plugins/vis_type_table/public/components/table_vis_component.test.tsx (2)
src/plugins/vis_type_table/public/components/table_vis_component.tsx (1)
TableVisComponent(22-38)src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
TableVisDynamicTable(218-558)
🪛 ast-grep (0.40.0)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
[warning] 88-88: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
[error] 89-89: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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). (70)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: lighthouse
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: linkchecker
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: dependency-review
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Lint and validate
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: bundle-analyzer
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (3)
src/plugins/vis_type_table/public/components/table_vis_component.tsx (1)
22-37: Wrapper delegation looks correct.
TableVisComponentnow cleanly delegates toTableVisDynamicTablewith all required props forwarded; this keeps the surface area stable while moving logic into the new component.src/plugins/vis_type_table/public/components/table_vis_component.test.tsx (1)
112-176: Prop-passthrough tests are solid.These tests give good confidence that
TableVisComponentcorrectly wires all props (includinguiStatevariations andshowTotal) intoTableVisDynamicTable. This aligns well with the new delegation model.src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx (1)
487-516: HTML injection is sanitized beforedangerouslySetInnerHTML(lint warning context).Static analysis flags
dangerouslySetInnerHTML, but in this implementation each cell’sformattedContentis passed throughdompurify.sanitizebefore being injected, and only the sanitized string reachesTableCell.sanitizedContent.Given table formatters may legitimately produce HTML, this pattern (DOMPurify +
dangerouslySetInnerHTML) is appropriate; just ensure any future changes continue to sanitize before injection.
| const handleSort = (columnIndex: number) => { | ||
| const currentSort = sort; | ||
| const newSort: ColumnSort = | ||
| currentSort.colIndex === columnIndex | ||
| ? { | ||
| colIndex: columnIndex, | ||
| direction: currentSort.direction === 'asc' ? 'desc' : 'asc', | ||
| } | ||
| : { | ||
| colIndex: columnIndex, | ||
| direction: 'asc', | ||
| }; | ||
|
|
||
| setSort(newSort); | ||
| // Reset to first page when sorting changes | ||
| setPageIndex(0); | ||
| }; |
There was a problem hiding this comment.
Header sorting is mouse-only; add keyboard / ARIA support.
Sorting is triggered inside the onMouseDown/onMouseUp path on <th>, but there’s no keyboard handler or tabIndex, so keyboard users can’t sort columns, and no aria-sort state is exposed.
Consider:
- Making the header (or an inner button) focusable (
tabIndex={0}or actual<button>). - Adding an
onKeyDownhandler to triggerhandleSortonEnter/Space. - Setting
aria-sorton<th>based onsort.colIndexandsort.direction.
This will restore accessibility closer to what users had with the previous data grid.
| const handleColumnResize = (columnIndex: number, width: number) => { | ||
| setWidth({ colIndex: columnIndex, width }); | ||
| }; | ||
|
|
||
| const handlePageChange = (newPageIndex: number) => { | ||
| setPageIndex(newPageIndex); | ||
| }; | ||
|
|
||
| const handlePageSizeChange = (newPageSize: number) => { | ||
| setPageSize(newPageSize); | ||
| // Reset to first page when page size changes | ||
| setPageIndex(0); | ||
| }; |
There was a problem hiding this comment.
Column resize updates external uiState but not local widths (no visual change).
handleColumnResize calls setWidth({ colIndex, width }) but never updates the component’s own columnWidths state, which is what <th> and <td> use for their style.width. As a result, dragging a column header will not actually resize the columns visually; only the external uiState is updated.
Also, initialWidth is read from columnWidths[index] but may be undefined on the initial render, which can lead to NaN widths during fast interactions/tests.
Consider something along these lines:
- const handleColumnResize = (columnIndex: number, width: number) => {
- setWidth({ colIndex: columnIndex, width });
- };
+ const handleColumnResize = (columnIndex: number, width: number) => {
+ // Update local state so the resize is reflected immediately
+ setColumnWidths((prev) => {
+ const next = [...prev];
+ next[columnIndex] = width;
+ return next;
+ });
+ // Persist to external uiState
+ setWidth({ colIndex: columnIndex, width });
+ };And defensively default initialWidth in the onMouseDown handler, e.g. const initialWidth = columnWidths[index] ?? MIN_COLUMN_WIDTH;.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/plugins/vis_type_table/public/components/table_vis_dynamic_table.tsx
around lines 395 to 407, handleColumnResize currently only updates external
uiState (setWidth) but does not update the component's local columnWidths state
or defend against undefined initial widths; change handleColumnResize to update
both the external uiState and the local columnWidths (e.g., setColumnWidths(prev
=> { ...update colIndex to width... })), and in the onMouseDown width
calculation use a safe default for initialWidth (e.g., const initialWidth =
columnWidths[index] ?? MIN_COLUMN_WIDTH) to avoid NaN during fast interactions
or initial renders.
|
Need to merge in opensearch-project/opensearch-dashboards-functional-test#1869 for the failed ciGroup2 cypress tests |
|
https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/20045713939/job/57605486876?pr=11031 <-- cypress test passed after functional repo PR merged in |
3 participants
Description
Re-implementation of table visualization using
<table>to replace the previous<OuiDataGrid>Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.