Cell inspector data display #8702
Conversation
…ortcut The toggle function was overwriting the store's value (set by hover's updateValue call) with a potentially stale local value from the CellInspector component. Now when opening, toggle() preserves the store's existing value if one was set via updateValue(), falling back to the passed value only if the store's value is empty. Fixes: APP-693 Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
|
Cursor Agent can help with this pull request. Just |
- Updated cell-inspector-store to accept null values (string | null type) - Changed toggle to use nullish coalescing (??) to preserve null values - Updated CellInspector to show 'null' instead of 'No value' for null cells - Updated subscription to sync null values when inspector is open - Modified all cell components to call updateValue even for null values: - LeaderboardCell.svelte - Cell.svelte (virtualized-table) - VirtualTableCell.svelte - FlatTable.svelte - NestedTable.svelte - RegularTable.svelte - KPI.svelte - MeasureBigNumber.svelte Fixes: APP-693 Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
The nullish coalescing operator (??) was treating null as a fallback case, causing the inspector to show a stale value instead of null. Changed to check for empty string (the initial state) instead, so that null values from hovering over null cells are properly preserved when toggling the inspector open. Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ? state.value | ||
| : state.value !== "" | ||
| ? state.value | ||
| : value, |
There was a problem hiding this comment.
Empty string cell values display stale data
Medium Severity
The toggle function and subscription use "" (empty string) as a sentinel value to detect "no value set", but empty string is also a valid cell value. When a user hovers over a cell containing an empty string and presses Space, the check state.value !== "" fails, causing the function to fall back to the passed value parameter (which is stale from a previous session) instead of using the actual empty string from the hover. This causes the cell inspector to display incorrect data for empty string cells.
Additional Locations (1)
Changed sentinel value from empty string to undefined: - undefined = no value set yet (initial state / not hovered) - null = cell contains null value - string (including '') = cell contains a string value Also added distinct display for empty strings showing '(empty string)' to distinguish from null or missing values. Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
| cellInspectorStore.updateValue(value.toString()); | ||
| } | ||
| // Always update the value in the store, but don't change visibility | ||
| cellInspectorStore.updateValue( |
There was a problem hiding this comment.
There seems to be a lot of duplication for null check. Lets move all of it to cellInspectorStore.updateValue
| isOpen: boolean; | ||
| value: string; | ||
| // undefined = no value set yet, null = cell contains null, string = cell value (including empty string) | ||
| value: string | null | undefined; |
There was a problem hiding this comment.
We should probably have a separate field, hasValue instead of messing about with null/undefined values.
- Added hasValue boolean field instead of using undefined as sentinel - Moved null check logic into updateValue() which now accepts unknown - Simplified all cell components to just pass raw value to updateValue() - normalizeValue() helper converts null/undefined to null, else to string Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
Fixes APP-693: Cell inspector shows incorrect data on certain UX paths.
The cell inspector was displaying stale or "No value" data when opened via the Spacebar after hovering over a cell. This happened because the
togglefunction incell-inspector-store.tswas overwriting the correctly updated store value (from hover) with a stale local value fromCellInspector.sveltewhen the inspector was closed.The fix modifies the
togglefunction to prioritize the existingstate.valuewhen opening the inspector, ensuring that the value set by a preceding hover event is preserved and displayed correctly. The passedvalueis now only used as a fallback.Checklist:
Linear Issue: APP-693
Note
Resolves incorrect/stale values shown in the cell inspector and standardizes null handling.
cell-inspector-store.tsto allowstring | null, and modifiestoggleto prefer existing store value on open;open/updateValue/toggleaccept nullsCellInspector.svelte, only syncs when open/not locked and ignores initial empty string; rendersnullexplicitlycellInspectorStore.updateValue, passingnullfor null/undefinedWritten by Cursor Bugbot for commit 1f8b58e. This will update automatically on new commits. Configure here.