Add graph visualization for selected numbers #2884
Conversation
|
🚀 Thanks for opening this pull request! |
|
The label |
📝 WalkthroughWalkthroughA new graph visualization feature is introduced into the data browser interface. This includes a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataBrowser
participant BrowserToolbar
participant Toolbar
participant GraphPanel
User->>DataBrowser: Selects multiple cells
DataBrowser->>DataBrowser: handleCellClick()
DataBrowser->>DataBrowser: Set graphVisible=true (if multiple numeric cells)
DataBrowser->>GraphPanel: Pass selectedCells, order, data, columns, width
User->>Toolbar: Clicks "Show Graph"/"Hide Graph" button
Toolbar->>DataBrowser: Calls toggleGraphVisibility()
DataBrowser->>DataBrowser: Toggle graphVisible state
DataBrowser->>GraphPanel: Show/hide GraphPanel accordingly
User->>GraphPanel: Interacts with resizable panel
GraphPanel->>DataBrowser: handleGraphResizeDiv/handleGraphResizeStop()
DataBrowser->>GraphPanel: Update graphWidth prop
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code Graph Analysis (2)src/components/Chart/Chart.react.js (1)
src/components/GraphPanel/GraphPanel.js (1)
🪛 GitHub Check: Lintsrc/components/GraphPanel/GraphPanel.js[failure] 91-91: [failure] 89-89: [failure] 71-71: [failure] 60-60: ⏰ 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). (1)
🔇 Additional comments (8)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/components/GraphPanel/GraphPanel.js (1)
37-72: Consider extracting data processing logic into separate functions.The data processing logic for both time series and regular numeric data is quite complex and could benefit from extraction into smaller, more testable functions.
Consider refactoring into smaller functions:
+const processTimeSeriesData = (data, columnNames, rowStart, rowEnd) => { + const chartData = {}; + for (let j = 1; j < columnNames.length; j++) { + chartData[columnNames[j]] = { color: ChartColorSchemes[j - 1], points: [] }; + } + + for (let i = rowStart; i <= rowEnd; i++) { + const row = data[i]; + if (!row) continue; + const ts = parseDate(row.attributes[columnNames[0]]); + if (ts === null) continue; + for (let j = 1; j < columnNames.length; j++) { + const val = row.attributes[columnNames[j]]; + if (typeof val === 'number' && !isNaN(val)) { + chartData[columnNames[j]].points.push([ts, val]); + } + } + } + return chartData; +}; + +const processNumericData = (data, columnNames, columnTypes, rowStart, rowEnd) => { + const chartData = {}; + let seriesIndex = 0; + columnNames.forEach((col, idx) => { + if (columnTypes[idx] === 'Number') { + chartData[col] = { color: ChartColorSchemes[seriesIndex], points: [] }; + seriesIndex++; + } + }); + + let x = 0; + for (let i = rowStart; i <= rowEnd; i++, x++) { + const row = data[i]; + if (!row) continue; + columnNames.forEach(col => { + const val = row.attributes[col]; + if (typeof val === 'number' && !isNaN(val)) { + chartData[col].points.push([x, val]); + } + }); + } + return chartData; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/GraphPanel/GraphPanel.js(1 hunks)src/components/GraphPanel/GraphPanel.scss(1 hunks)src/components/Toolbar/Toolbar.react.js(4 hunks)src/components/Toolbar/Toolbar.scss(1 hunks)src/dashboard/Data/Browser/BrowserToolbar.react.js(2 hunks)src/dashboard/Data/Browser/DataBrowser.react.js(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/dashboard/Data/Browser/DataBrowser.react.js (1)
Learnt from: mtrezza
PR: parse-community/parse-dashboard#2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
🪛 GitHub Check: Lint
src/components/GraphPanel/GraphPanel.js
[failure] 64-64:
Expected { after 'if' condition
[failure] 45-45:
Expected { after 'if' condition
[failure] 43-43:
Expected { after 'if' condition
src/dashboard/Data/Browser/DataBrowser.react.js
[failure] 84-84:
Expected indentation of 4 spaces but found 6
⏰ 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). (2)
- GitHub Check: Node 18
- GitHub Check: Docker linux/amd64
🔇 Additional comments (19)
src/components/Toolbar/Toolbar.scss (1)
102-113: LGTM! Clean CSS implementation.The new
.graphclass follows the same styling pattern as the existing.statsclass with appropriate positioning to avoid overlap. The styles are consistent and well-structured.src/components/GraphPanel/GraphPanel.scss (1)
1-11: LGTM! Clean and focused styling.The stylesheet provides appropriate styling for the graph panel container and empty state. The styles are minimal and well-structured.
src/dashboard/Data/Browser/BrowserToolbar.react.js (2)
82-83: LGTM! Clean prop integration.The new props are properly destructured and follow the existing pattern for prop handling in this component.
282-283: LGTM! Proper prop forwarding.The props are correctly passed to the Toolbar component, maintaining consistency with the existing prop passing pattern.
src/components/Toolbar/Toolbar.react.js (4)
18-18: LGTM! Clean prop integration.The new props are properly added to the Stats component signature and follow the existing pattern.
111-125: LGTM! Well-implemented conditional rendering.The graph toggle button is correctly shown only when there are multiple data points (
data.length > 1), and the button text appropriately reflects the current visibility state.
162-163: LGTM! Proper prop forwarding.The props are correctly passed to the Stats component, maintaining consistency with the existing prop passing pattern.
194-195: LGTM! PropTypes correctly updated.The PropTypes are properly updated to include the new props with appropriate types.
src/components/GraphPanel/GraphPanel.js (1)
6-22: LGTM! Well-implemented date parsing utility.The
parseDatefunction handles various input formats correctly and includes proper null checking and validation.src/dashboard/Data/Browser/DataBrowser.react.js (10)
20-20: LGTM: GraphPanel import is properly added.The import statement follows the existing pattern and is correctly placed with other component imports.
103-104: LGTM: Graph state initialization is well-structured.The new state variables
graphVisibleandgraphWidthare properly initialized with sensible defaults (false and 300 respectively), consistent with the existingpanelWidthpattern.
115-117: LGTM: Method bindings follow existing patterns.The graph resize handler method bindings are consistent with the existing aggregation panel resize handlers and properly placed with other method bindings.
129-129: LGTM: Toggle method binding is appropriately placed.The
toggleGraphVisibilitymethod binding follows the existing pattern and is correctly positioned with other method bindings.
225-238: LGTM: Graph resize handlers mirror aggregation panel implementation.The graph resize handlers (
handleGraphResizeStart,handleGraphResizeStop,handleGraphResizeDiv) are well-implemented and follow the same pattern as the existing aggregation panel resize handlers, ensuring consistency in behavior.
289-291: LGTM: Toggle method implementation is clean and follows existing patterns.The
toggleGraphVisibilitymethod correctly usesprevStatecallback pattern, consistent with the existingtogglePanelVisibilitymethod implementation.
696-696: LGTM: Graph visibility logic is appropriately triggered.Setting
graphVisible: truewhen multiple cells are selected and all columns are numeric is the correct behavior for enabling graph visualization.
707-707: LGTM: Graph visibility reset is properly handled.Setting
graphVisible: falsewhen selection is cleared or single cell is selected maintains consistent state management.
789-811: LGTM: GraphPanel rendering implementation is well-structured.The ResizableBox wrapping the GraphPanel follows the exact same pattern as the aggregation panel, ensuring UI consistency. The component receives all necessary props (selectedCells, order, data, columns, width) for proper chart rendering.
841-842: LGTM: BrowserToolbar props are correctly configured.The new props
toggleGraphandisGraphVisibleproperly expose the graph control functionality to the toolbar, following the established pattern used for the aggregation panel.
| } | ||
| for (let i = rowStart; i <= rowEnd; i++) { | ||
| const row = data[i]; | ||
| if (!row) continue; |
There was a problem hiding this comment.
Fix linting issues: Add braces after if conditions.
The static analysis tool correctly identifies missing braces after if conditions, which violates the project's style guide.
Apply this diff to fix the linting issues:
- if (!row) continue;
+ if (!row) {
+ continue;
+ }
const ts = parseDate(row.attributes[columnNames[0]]);
- if (ts === null) continue;
+ if (ts === null) {
+ continue;
+ }
for (let j = 1; j < columnNames.length; j++) {
const val = row.attributes[columnNames[j]];
if (typeof val === 'number' && !isNaN(val)) {
chartData[columnNames[j]].points.push([ts, val]);
}
}
}
} else {
let seriesIndex = 0;
columnNames.forEach((col, idx) => {
if (columnTypes[idx] === 'Number') {
chartData[col] = { color: ChartColorSchemes[seriesIndex], points: [] };
seriesIndex++;
}
});
let x = 0;
for (let i = rowStart; i <= rowEnd; i++, x++) {
const row = data[i];
- if (!row) continue;
+ if (!row) {
+ continue;
+ }Also applies to: 45-45, 64-64
🧰 Tools
🪛 GitHub Check: Lint
[failure] 43-43:
Expected { after 'if' condition
🤖 Prompt for AI Agents
In src/components/GraphPanel/GraphPanel.js at lines 43, 45, and 64, the if
statements lack braces which causes linting errors. Add curly braces {} around
the statements following each if condition to comply with the project's style
guide and fix the linting issues.
| return null; | ||
| } | ||
| const { rowStart, rowEnd, colStart, colEnd } = selectedCells; | ||
| const columnNames = order.slice(colStart, colEnd + 1).map(o => o.name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for array slicing.
The code slices the order array without checking if colStart and colEnd are within bounds, which could cause runtime errors.
Add bounds checking:
+ if (colStart < 0 || colEnd >= order.length) {
+ return <div className={styles.empty}>Invalid column selection.</div>;
+ }
const columnNames = order.slice(colStart, colEnd + 1).map(o => o.name);📝 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.
| const columnNames = order.slice(colStart, colEnd + 1).map(o => o.name); | |
| if (colStart < 0 || colEnd >= order.length) { | |
| return <div className={styles.empty}>Invalid column selection.</div>; | |
| } | |
| const columnNames = order.slice(colStart, colEnd + 1).map(o => o.name); |
🤖 Prompt for AI Agents
In src/components/GraphPanel/GraphPanel.js at line 29, the slice operation on
the order array uses colStart and colEnd without verifying if these indices are
within the valid range of the array. To fix this, add bounds checking to ensure
colStart is at least 0 and colEnd does not exceed order.length - 1 before
slicing. Adjust colStart and colEnd accordingly to prevent runtime errors from
invalid indices.
| return null; | ||
| } | ||
|
|
||
| export default function GraphPanel({ selectedCells, order, data, columns, width }) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PropTypes validation.
The component is missing PropTypes validation, which would help catch type-related issues during development.
Add PropTypes validation:
+import PropTypes from 'prop-types';
+
+GraphPanel.propTypes = {
+ selectedCells: PropTypes.object,
+ order: PropTypes.array.isRequired,
+ data: PropTypes.array.isRequired,
+ columns: PropTypes.object.isRequired,
+ width: PropTypes.number.isRequired,
+};📝 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.
| export default function GraphPanel({ selectedCells, order, data, columns, width }) { | |
| import PropTypes from 'prop-types'; | |
| export default function GraphPanel({ selectedCells, order, data, columns, width }) { | |
| // ...existing implementation... | |
| } | |
| GraphPanel.propTypes = { | |
| selectedCells: PropTypes.object, | |
| order: PropTypes.array.isRequired, | |
| data: PropTypes.array.isRequired, | |
| columns: PropTypes.object.isRequired, | |
| width: PropTypes.number.isRequired, | |
| }; |
🤖 Prompt for AI Agents
In src/components/GraphPanel/GraphPanel.js at line 24, the GraphPanel component
lacks PropTypes validation. Add PropTypes import from 'prop-types' at the top of
the file, then define GraphPanel.propTypes after the component declaration
specifying the expected types for selectedCells, order, data, columns, and width
to ensure type checking during development.
| const chartData = {}; | ||
| if (timeSeries) { | ||
| for (let j = 1; j < columnNames.length; j++) { | ||
| chartData[columnNames[j]] = { color: ChartColorSchemes[j - 1], points: [] }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify ChartColorSchemes array has sufficient colors.
The code uses ChartColorSchemes[j - 1] and ChartColorSchemes[seriesIndex] without checking if the array has enough colors for the number of series.
Also applies to: 57-57
🏁 Script executed:
#!/bin/bash
# Search for ChartColorSchemes definition to verify available colors
ast-grep --pattern 'ChartColorSchemes = $_'Length of output: 45
🏁 Script executed:
#!/bin/bash
# Locate all references and the definition of ChartColorSchemes
rg -n "ChartColorSchemes"Length of output: 2197
🏁 Script executed:
#!/bin/bash
# Display the contents of ChartColorSchemes in lib/Constants.js
sed -n '1,100p' src/lib/Constants.js | sed -n '22,50p'Length of output: 515
🏁 Script executed:
#!/bin/bash
# Show context around ChartColorSchemes usage in GraphPanel.js
sed -n '1,200p' src/components/GraphPanel/GraphPanel.jsLength of output: 2548
Ensure ChartColorSchemes won’t be indexed out-of-bounds
ChartColorSchemes currently has 12 entries, but GraphPanel can request more if the user selects >12 series. To prevent undefined colors (or runtime errors), clamp or wrap your index and/or provide a fallback:
• File: src/components/GraphPanel/GraphPanel.js
– Line 39:
```diff
- chartData[columnNames[j]] = { color: ChartColorSchemes[j - 1], points: [] };
- const tsColorIndex = (j - 1) % ChartColorSchemes.length;
- chartData[columnNames[j]] = {
-
color: ChartColorSchemes[tsColorIndex], -
points: [], - };
– Line 57:
```diff
- chartData[col] = { color: ChartColorSchemes[seriesIndex], points: [] };
- const idx = seriesIndex % ChartColorSchemes.length;
- chartData[col] = {
-
color: ChartColorSchemes[idx], -
points: [], - };
Alternatively, check seriesCount <= ChartColorSchemes.length before rendering and display a warning or disable extra series.
📝 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.
| chartData[columnNames[j]] = { color: ChartColorSchemes[j - 1], points: [] }; | |
| // — at around line 39 in src/components/GraphPanel/GraphPanel.js — | |
| const tsColorIndex = (j - 1) % ChartColorSchemes.length; | |
| chartData[columnNames[j]] = { | |
| color: ChartColorSchemes[tsColorIndex], | |
| points: [], | |
| }; | |
| // — at around line 57 in src/components/GraphPanel/GraphPanel.js — | |
| const idx = seriesIndex % ChartColorSchemes.length; | |
| chartData[col] = { | |
| color: ChartColorSchemes[idx], | |
| points: [], | |
| }; |
🤖 Prompt for AI Agents
In src/components/GraphPanel/GraphPanel.js at line 39, the code indexes
ChartColorSchemes with j - 1, which can exceed the array length if more than 12
series are selected. To fix this, modify the index to wrap around using modulo
operation with ChartColorSchemes.length or clamp it to the maximum valid index
to avoid out-of-bounds access. Alternatively, add a check before rendering to
ensure seriesCount does not exceed ChartColorSchemes.length and handle excess
series by showing a warning or disabling them.
| @@ -80,7 +81,7 @@ | |||
| const storedRowNumber = | |||
| window.localStorage?.getItem(BROWSER_SHOW_ROW_NUMBER) === 'true'; | |||
|
|
|||
| this.state = { | |||
| this.state = { | |||
There was a problem hiding this comment.
Fix indentation to match coding standards.
The static analysis tool correctly identifies an indentation issue. The line should use 4 spaces instead of 6.
- this.state = {
+ this.state = {📝 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.
| this.state = { | |
| this.state = { |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 84-84:
Expected indentation of 4 spaces but found 6
🤖 Prompt for AI Agents
In src/dashboard/Data/Browser/DataBrowser.react.js at line 84, the indentation
of the line initializing this.state uses 6 spaces instead of the required 4
spaces. Adjust the indentation to use exactly 4 spaces to comply with the coding
standards.
Uffizzi Ephemeral Environment
|
Summary
Testing
npm testhttps://chatgpt.com/codex/tasks/task_e_686e8af4ea44832db20db0be1ce113e9
Summary by CodeRabbit
New Features
Style
Bug Fixes