bito-code-review[bot] commented on code in PR #44907: URL: https://github.com/apache/superset/pull/44907#discussion_r4170106861
########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/jsonCellRowHeight.ts: ########## @@ -0,0 +1,64 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +type RowHeightNode = { + setRowHeight: (height: number | null | undefined) => void; +}; + +type RowHeightApi = { + onRowHeightChanged: () => void; +}; + +const heightsByRow = new WeakMap<object, Map<string, number>>(); + +/** + * Track per-column JSON expansion height on a row. The row grows to the + * tallest expanded JSON cell and returns to the grid default once every + * expanded cell in that row is collapsed. A collapse for a column that was + * never expanded is ignored. + */ +export function syncJsonCellRowHeight( + node: RowHeightNode, + api: RowHeightApi, + colId: string, + height: number, +): void { + let heights = heightsByRow.get(node); + const hadEntry = heights?.has(colId) ?? false; + if (height <= 0 && !hadEntry) { + return; + } + if (!heights) { + heights = new Map(); + heightsByRow.set(node, heights); + } + if (height > 0) { + heights.set(colId, height); + } else { + heights.delete(colId); + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Row height tracking broken</b></div> <div id="fix"> `syncJsonCellRowHeight` keys the module `heightsByRow` WeakMap by its `node` arg, but both call sites in `JsonCellRenderer.tsx` (419-424, 474-479) pass a fresh `{ setRowHeight }` literal each call. `heightsByRow.get(node)` is always undefined, so `hadEntry` is always false: multi-column rows get only the last column's height (not the max), and collapse early-returns, never restoring the grid default height. Pass the stable AG Grid `node` as the key. </div> </div> <small><i>Code Review Run #1481d4</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-ag-grid-table/test/JsonCellRenderer.test.tsx: ########## @@ -0,0 +1,262 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import '@testing-library/jest-dom'; +import { + fireEvent, + render, + screen, + userEvent, + within, +} from '@superset-ui/core/spec'; +import { JsonCellRenderer } from '../src/renderers/JsonCellRenderer'; +import { + jsonCellPreview, + parseJsonCellValue, +} from '../src/renderers/parseJsonCellValue'; +import { syncJsonCellRowHeight } from '../src/renderers/jsonCellRowHeight'; +import { TextCellRenderer } from '../src/renderers/TextCellRenderer'; +import { isJsonCellActionTarget } from '../src/utils/isJsonCellActionTarget'; +import { CellRendererProps } from '../src/types'; + +const nestedJson = '{"user":"ada","address":{"city":"London"}}'; + +test('parseJsonCellValue accepts objects and arrays only', () => { + expect(parseJsonCellValue('{"a":1}')).toEqual({ a: 1 }); + expect(parseJsonCellValue(' [1, 2] ')).toEqual([1, 2]); + expect(parseJsonCellValue({ a: 1 })).toEqual({ a: 1 }); + expect(parseJsonCellValue('[1]')).toEqual([1]); + expect(parseJsonCellValue('plain')).toBeNull(); + expect(parseJsonCellValue('{not json')).toBeNull(); + expect(parseJsonCellValue('"just a string"')).toBeNull(); + expect(parseJsonCellValue('123')).toBeNull(); + expect(parseJsonCellValue(null)).toBeNull(); + expect(parseJsonCellValue(new Date('2024-01-01'))).toBeNull(); +}); + +test('jsonCellPreview collapses whitespace onto one line', () => { + expect(jsonCellPreview({ a: 1 }, '{\n "a": 1\n}')).toBe('{ "a": 1 }'); +}); + +test('syncJsonCellRowHeight keeps the tallest expanded cell', () => { + const node = { setRowHeight: jest.fn() }; + const api = { onRowHeightChanged: jest.fn() }; + + syncJsonCellRowHeight(node, api, 'a', 100); + syncJsonCellRowHeight(node, api, 'b', 40); + expect(node.setRowHeight).toHaveBeenLastCalledWith(100); + + syncJsonCellRowHeight(node, api, 'a', 0); + expect(node.setRowHeight).toHaveBeenLastCalledWith(40); + + syncJsonCellRowHeight(node, api, 'b', 0); + expect(node.setRowHeight).toHaveBeenLastCalledWith(null); +}); + +test('syncJsonCellRowHeight ignores a collapse that was never expanded', () => { + const node = { setRowHeight: jest.fn() }; + const api = { onRowHeightChanged: jest.fn() }; + syncJsonCellRowHeight(node, api, 'c', 0); + expect(node.setRowHeight).not.toHaveBeenCalled(); + expect(api.onRowHeightChanged).not.toHaveBeenCalled(); +}); + +test('isJsonCellActionTarget matches controls inside a JSON cell', () => { + document.body.innerHTML = + '<div data-json-cell-action="true"><span id="json-action"></span></div><span id="plain"></span>'; + expect(isJsonCellActionTarget(document.getElementById('json-action'))).toBe( + true, + ); + expect(isJsonCellActionTarget(document.getElementById('plain'))).toBe(false); + expect(isJsonCellActionTarget(null)).toBe(false); +}); + +test('collapsed JSON shows a one-line preview and hides nested keys', async () => { + render( + <JsonCellRenderer + value={{ user: 'ada', address: { city: 'London' } }} + rawText={nestedJson} + colId="payload" + autoHeight={false} + jsonInCell + />, + ); + + expect(screen.getByTestId('json-cell-preview')).toHaveTextContent(nestedJson); + expect( + screen.queryByRole('button', { name: 'Expand address' }), + ).not.toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Expand JSON' })); + + expect( + await screen.findByRole('button', { name: 'Expand address' }), + ).toBeInTheDocument(); + expect(screen.queryByTestId('json-cell-preview')).not.toBeInTheDocument(); + expect(screen.queryByText('London')).not.toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Expand address' })); + expect(screen.getByText('"London"')).toBeInTheDocument(); +}); + +test('JSON controls do not bubble clicks to the cell', async () => { + const onParentClick = jest.fn(); + const { container } = render( + <JsonCellRenderer + value={{ a: 1 }} + colId="payload" + autoHeight={false} + jsonInCell + />, + ); + container.addEventListener('click', onParentClick); + + await userEvent.click(screen.getByRole('button', { name: 'Expand JSON' })); + expect(onParentClick).not.toHaveBeenCalled(); +}); + +test('expanded JSON asks an auto-height grid to remeasure the row', async () => { + const resetRowHeights = jest.fn(); + render( + <JsonCellRenderer + value={{ a: 1 }} + colId="payload" + autoHeight + jsonInCell + api={{ resetRowHeights }} + />, + ); + + expect(resetRowHeights).not.toHaveBeenCalled(); + await userEvent.click(screen.getByRole('button', { name: 'Expand JSON' })); + expect( + await screen.findByRole('button', { name: 'Collapse JSON' }), + ).toBeInTheDocument(); + expect(resetRowHeights).toHaveBeenCalled(); +}); + +test('the default cell keeps the original JSON text, including line breaks', () => { + const raw = '{\n "user": "ada"\n}'; + render( + <JsonCellRenderer + value={{ user: 'ada' }} + rawText={raw} + colId="payload" + autoHeight={false} + />, + ); + + expect(screen.getByTestId('json-cell-preview').textContent).toBe(raw); + expect( + screen.queryByRole('button', { name: 'Expand JSON' }), + ).not.toBeInTheDocument(); +}); + +test('JSON stays the original text and opens on click', async () => { + const writeText = jest.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, 'clipboard', { + configurable: true, + value: { writeText }, + }); + + render( + <JsonCellRenderer + value={{ user: 'ada', address: { city: 'London' } }} + rawText={nestedJson} + colId="payload" + autoHeight={false} + />, + ); + + expect( + screen.queryByRole('button', { name: 'Expand JSON' }), + ).not.toBeInTheDocument(); + expect(screen.getByTestId('json-cell-preview').textContent).toBe(nestedJson); + await userEvent.click(screen.getByTestId('json-cell-preview')); + const dialog = await screen.findByRole('dialog'); + expect(dialog).toHaveTextContent('Cell content'); + expect(within(dialog).getByText('"ada"')).toBeInTheDocument(); + + await userEvent.click(within(dialog).getByRole('button', { name: 'Copy' })); + expect(writeText).toHaveBeenCalledWith(nestedJson); + expect( + await within(dialog).findByRole('button', { name: 'Copied' }), + ).toBeInTheDocument(); +}); + +test('a click on the arrow expands the cell and a second click opens the dialog', async () => { + render( + <JsonCellRenderer + value={{ user: 'ada', address: { city: 'London' } }} + rawText={nestedJson} + colId="payload" + autoHeight={false} + jsonInCell + />, + ); + + fireEvent.dblClick(screen.getByTestId('json-cell-preview')); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + + const arrow = screen.getByRole('button', { name: 'Expand JSON' }); + fireEvent.click(arrow); + fireEvent.click(arrow); + expect(await screen.findByRole('dialog')).toHaveTextContent('Cell content'); + expect( + screen.queryByRole('button', { name: 'Expand address' }), + ).not.toBeInTheDocument(); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Failing dialog assertion</b></div> <div id="fix"> The final assertion fails: after the second click opens the dialog, `screen.queryByRole('button', { name: 'Expand address' })` matches the button rendered inside the modal's `<JsonNode value={value} depth={0} initialOpen />` tree (the collapsed `address` node). `screen` queries the whole document including the portal, so `.not.toBeInTheDocument()` is false. Scope the query to the cell (`within(screen.getByTestId('json-cell'))`) to assert the cell body is not expanded. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion expect( within(screen.getByTestId('json-cell')).queryByRole('button', { name: 'Expand address', }), ).not.toBeInTheDocument(); ```` </div> </details> </div> <small><i>Code Review Run #1481d4</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
