Repository navigation
feat(app): let the row side panel choose property key order #3126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d87458c
3661b9e
f9b7fa4
3ae9a6b
0c757f1
3a0f1e9
048b8d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| '@hyperdx/app': patch | ||
| --- | ||
|
|
||
| feat: choose how property keys are ordered in the row side panel | ||
|
|
||
| The JSON viewer sorts keys alphabetically, which is the right default for wide | ||
| ClickHouse `Map(...)` columns like `ProfileEvents` but hides the stored column | ||
| order. The properties view options menu now offers "Sort keys A–Z", "Sort keys | ||
| Z–A", and "Original order"; the choice persists with the viewer's other options. | ||
| Array elements keep their index order in every mode. In "Original order", | ||
| integer-like keys (e.g. HTTP status codes) still list first in ascending order, | ||
| since parsed JavaScript objects order keys that way. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,15 +40,30 @@ export type FormatLeafValue = (arg0: { | |
| value: unknown; | ||
| }) => React.ReactNode | undefined; | ||
|
|
||
| /** | ||
| * How object keys are ordered at every level of the tree. | ||
| * | ||
| * `original` keeps the object's own key order — for ClickHouse `Map(...)` | ||
| * columns, physical storage order — except that integer-like keys (`"200"`, | ||
| * `Map(UInt64, …)` keys) always come first in ascending numeric order. JS | ||
| * objects order keys that way, so the true order is already gone once the | ||
| * query result is parsed. | ||
| */ | ||
| export type KeyOrder = 'asc' | 'desc' | 'original'; | ||
|
|
||
| export const DEFAULT_KEY_ORDER: KeyOrder = 'asc'; | ||
|
|
||
| // Store common state in an atom so that it can be shared between components | ||
| // to avoid prop drilling | ||
| type HyperJsonAtom = { | ||
| normallyExpanded: boolean; | ||
| keyOrder: KeyOrder; | ||
| getLineActions?: GetLineActions; | ||
| formatLeafValue?: FormatLeafValue; | ||
| }; | ||
| const hyperJsonAtom = atom<HyperJsonAtom>({ | ||
| normallyExpanded: false, | ||
| keyOrder: DEFAULT_KEY_ORDER, | ||
| }); | ||
|
|
||
| const ValueRenderer = React.memo( | ||
|
|
@@ -273,6 +288,7 @@ const Line = React.memo( | |
| <div | ||
| ref={ref} | ||
| data-testid="json-viewer-line" | ||
| data-depth={nestedLevel} | ||
| onClick={handleToggle} | ||
| className={cx(styles.line, { | ||
| [styles.nestedLine]: nestedLevel > 0, | ||
|
|
@@ -358,6 +374,8 @@ function TreeNode({ | |
|
|
||
| const originalLength = React.useMemo(() => Object.keys(data).length, [data]); | ||
|
|
||
| const { keyOrder } = useAtomValue(hyperJsonAtom); | ||
|
|
||
| // ClickHouse hands back `Map(...)` keys in physical storage order, which reads | ||
| // as random for wide maps like `ProfileEvents`. Sorting here (rather than | ||
| // upstream) covers every nesting level for free, since TreeNode recurses, and | ||
|
|
@@ -366,13 +384,15 @@ function TreeNode({ | |
| const entries = React.useMemo(() => { | ||
| const raw = Object.entries(data); | ||
| // Arrays are index-keyed — reordering them would change the data. | ||
| if (isArray(data)) { | ||
| if (isArray(data) || keyOrder === 'original') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — "Original order" is not the stored order when keys look like integers
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid. The integer-key reordering happens when the ClickHouse JSON result is parsed into an object, before |
||
| return raw; | ||
| } | ||
| return raw.sort(([a], [b]) => | ||
| a.localeCompare(b, undefined, { numeric: true }), | ||
| const direction = keyOrder === 'desc' ? -1 : 1; | ||
| return raw.sort( | ||
| ([a], [b]) => | ||
| direction * a.localeCompare(b, undefined, { numeric: true }), | ||
| ); | ||
| }, [data]); | ||
| }, [data, keyOrder]); | ||
|
|
||
| const visibleLines = React.useMemo(() => { | ||
| return isExpanded ? entries : entries.slice(0, MAX_TREE_NODE_ITEMS); | ||
|
|
@@ -430,6 +450,7 @@ type HyperJsonProps = { | |
| normallyExpanded?: boolean; | ||
| tabulate?: boolean; | ||
| whiteSpace?: 'pre' | 'pre-wrap'; | ||
| keyOrder?: KeyOrder; | ||
| getLineActions?: GetLineActions; | ||
| formatLeafValue?: FormatLeafValue; | ||
| }; | ||
|
|
@@ -439,16 +460,20 @@ const HyperJson = ({ | |
| normallyExpanded = false, | ||
| tabulate = false, | ||
| whiteSpace = 'pre-wrap', | ||
| keyOrder = DEFAULT_KEY_ORDER, | ||
| getLineActions, | ||
| formatLeafValue, | ||
| }: HyperJsonProps) => { | ||
| const isEmpty = React.useMemo(() => Object.keys(data).length === 0, [data]); | ||
|
|
||
| const initialValues = React.useMemo( | ||
| () => ({ normallyExpanded, keyOrder, getLineActions, formatLeafValue }), | ||
| [normallyExpanded, keyOrder, getLineActions, formatLeafValue], | ||
| ); | ||
|
|
||
| return ( | ||
| <Provider> | ||
| <HydrateAtoms | ||
| initialValues={{ normallyExpanded, getLineActions, formatLeafValue }} | ||
| > | ||
| <HydrateAtoms initialValues={initialValues}> | ||
| <div | ||
| className={cx(styles.container, { | ||
| [styles.withTabulate]: tabulate, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import React from 'react'; | ||
| import { Provider } from 'jotai'; | ||
| import { MantineProvider } from '@mantine/core'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
|
|
||
| import { DBRowJsonViewer } from '@/components/DBRowJsonViewer'; | ||
| import { RowSidePanelContext } from '@/components/DBRowSidePanel'; | ||
|
|
||
| jest.mock('next/router', () => ({ | ||
| __esModule: true, | ||
| default: { push: jest.fn() }, | ||
| })); | ||
|
|
||
| jest.mock('@/useFormatTime', () => ({ | ||
| useFormatTime: () => (time: unknown) => String(time), | ||
| FormatTime: jest.fn(() => null), | ||
| })); | ||
|
|
||
| const VIEWER_OPTIONS_KEY = 'hdx_json_viewer_options'; | ||
|
|
||
| // Options written before the sort setting existed — no `keyOrder` key. | ||
| const LEGACY_STORED_OPTIONS = { | ||
| normallyExpanded: true, | ||
| whiteSpace: 'pre-wrap', | ||
| tabulate: true, | ||
| filterBlanks: false, | ||
| }; | ||
|
|
||
| const data = { zebra: 'z', alpha: 'a', Mango: 'm' }; | ||
|
|
||
| // env="test" mounts the menu dropdown without a transition to wait on. The | ||
| // viewer options atom is module-scoped, so each render gets its own jotai store; | ||
| // the atom then reads localStorage when it mounts in that store. | ||
| const renderViewer = () => | ||
| render( | ||
| <Provider> | ||
| <MantineProvider env="test"> | ||
| <RowSidePanelContext value={{}}> | ||
| <DBRowJsonViewer data={data} /> | ||
| </RowSidePanelContext> | ||
| </MantineProvider> | ||
| </Provider>, | ||
| ); | ||
|
|
||
| const renderedKeys = (container: HTMLElement) => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — The same Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not changing here: it's a two-line query, and pulling it into a shared test util is the kind of hoisting the review guidance says to keep out of a feature PR. |
||
| Array.from(container.querySelectorAll('.keyContainer > .key')).map(el => | ||
| el.textContent?.trim(), | ||
| ); | ||
|
|
||
| const openOptionsMenu = async () => { | ||
| await userEvent.click(screen.getByTestId('json-viewer-options-menu')); | ||
| await screen.findByRole('menu'); | ||
| }; | ||
|
|
||
| const selectKeyOrder = async (value: 'asc' | 'desc' | 'original') => { | ||
| await openOptionsMenu(); | ||
| await userEvent.click(screen.getByTestId(`json-viewer-key-order-${value}`)); | ||
| }; | ||
|
|
||
| const storedKeyOrder = () => | ||
| JSON.parse(localStorage.getItem(VIEWER_OPTIONS_KEY) ?? '{}').keyOrder; | ||
|
|
||
| describe('DBRowJsonViewer key order', () => { | ||
| beforeEach(() => { | ||
| localStorage.clear(); | ||
| }); | ||
|
|
||
| it('sorts alphabetically when stored options predate the setting', async () => { | ||
| localStorage.setItem( | ||
| VIEWER_OPTIONS_KEY, | ||
| JSON.stringify(LEGACY_STORED_OPTIONS), | ||
| ); | ||
| const { container } = renderViewer(); | ||
|
|
||
| expect(renderedKeys(container)).toEqual(['alpha', 'Mango', 'zebra']); | ||
|
|
||
| await openOptionsMenu(); | ||
| expect( | ||
| screen.getByTestId('json-viewer-key-order-asc').querySelector('.ps-2'), | ||
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('restores a stored order on render', async () => { | ||
| localStorage.setItem( | ||
| VIEWER_OPTIONS_KEY, | ||
| JSON.stringify({ ...LEGACY_STORED_OPTIONS, keyOrder: 'original' }), | ||
| ); | ||
| const { container } = renderViewer(); | ||
|
|
||
| expect(renderedKeys(container)).toEqual(['zebra', 'alpha', 'Mango']); | ||
| }); | ||
|
|
||
| it('switches between the stored order and reverse alphabetical', async () => { | ||
| const { container } = renderViewer(); | ||
|
|
||
| expect(renderedKeys(container)).toEqual(['alpha', 'Mango', 'zebra']); | ||
|
|
||
| await selectKeyOrder('original'); | ||
| expect(renderedKeys(container)).toEqual(['zebra', 'alpha', 'Mango']); | ||
| expect(storedKeyOrder()).toBe('original'); | ||
|
|
||
| await selectKeyOrder('desc'); | ||
| expect(renderedKeys(container)).toEqual(['zebra', 'Mango', 'alpha']); | ||
| expect(storedKeyOrder()).toBe('desc'); | ||
|
|
||
| await selectKeyOrder('asc'); | ||
| expect(renderedKeys(container)).toEqual(['alpha', 'Mango', 'zebra']); | ||
| expect(storedKeyOrder()).toBe('asc'); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — 'Original order' isn't the stored order when keys look like integers
In JS,
Object.entriesalways lists integer-like keys (e.g."200","404","0") first, in ascending numeric order, no matter how they were inserted. Rows reach the viewer throughJSON.parse, so a ClickHouseMap(...)with numeric-string keys (status codes, ports, shard IDs) comes out sorted under 'original' and doesn't match the physical storage order that the KeyOrder doc comment and the menu label promise. Two fixes: keep the ordered entry list from the raw response (e.g. aMapor an entries array), or say in the doc comment and the changeset that integer-like keys are still listed numerically first.Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond