Yash code - #3
Conversation
latest widget code and read me
added code to generate current and timestamps 24 hours ago in read a…
There was a problem hiding this comment.
Code Review
This pull request introduces a new D3-based BarChartWidget component, updates chart themes, adds a StreamWidget for data streaming, and integrates the bar chart into the test application. Key feedback highlights a critical case-sensitivity bug in StreamWidget (NewNode vs newNode) and a validation key mismatch in BarChartWidget. Other recommendations include avoiding syncing props to state, removing unused imports and helper functions, cleaning up debug console logs, handling dummy data regeneration when limits change, and renaming the Bar Chart folder to eliminate spaces.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| const anedya = (client as any)._anedya as Anedya; | ||
|
|
||
| const node = anedya.NewNode(client, nodeId); |
There was a problem hiding this comment.
There is a case-sensitivity mismatch when calling NewNode. In other components (like ChartWidget.tsx and BarChartWidgets.tsx), the method is called newNode (lowercase 'n'). Calling anedya.NewNode will likely result in a runtime TypeError because the method does not exist on the SDK client.
| const node = anedya.NewNode(client, nodeId); | |
| const node = anedya.newNode(client, nodeId); |
| validateRequiredProps( | ||
| "Bar Chart Widget", | ||
| { client, nodeId, variable, from, to }, | ||
| ["client", "nodeId", "variable", "from time", "to time"] | ||
| ); |
There was a problem hiding this comment.
The keys in the object passed to validateRequiredProps do not match the strings in the required array. The object has keys from and to, but the required array lists "from time" and "to time". This will cause the validation helper to always report these props as missing even when they are correctly provided.
| validateRequiredProps( | |
| "Bar Chart Widget", | |
| { client, nodeId, variable, from, to }, | |
| ["client", "nodeId", "variable", "from time", "to time"] | |
| ); | |
| validateRequiredProps( | |
| "Bar Chart Widget", | |
| { client, nodeId, variable, from, to }, | |
| ["client", "nodeId", "variable", "from", "to"] | |
| ); |
| useEffect(() => { | ||
| if (dataProp) { | ||
| setData(dataProp); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Syncing dataProp to state data inside useEffect is an anti-pattern in React. Since dataProp is an array, passing it directly in the dependency array of useEffect can cause infinite render loops or unnecessary effect executions if the parent component passes an unmemoized array literal. Consider deriving data directly during render (e.g., const data = dataProp ?? fetchedData;) instead of duplicating it in state.
| const toDateSafe = (ts: number): Date => | ||
| ts < 1e12 ? new Date(ts * 1000) : new Date(ts); |
| import { | ||
| defaultDateFormatter, | ||
| formatDate, | ||
| formatNumber, | ||
| } from "../../helpers/formatDate"; |
| console.log("Stream Node : ", node); | ||
| console.log(Object.getOwnPropertyNames(Object.getPrototypeOf(node))); |
| setData(dataProp); | ||
| return; | ||
| } | ||
| if (!usingRealClient || !node) return; |
There was a problem hiding this comment.
When usingRealClient is false (i.e., using dummy data), the useEffect returns early because of if (!usingRealClient || !node) return;. This means that if the limit prop changes, the dummy data will not be regenerated to match the new limit. Consider handling dummy data regeneration when limit changes.
| LatestDataGauge, | ||
| } from "../../../src/index"; | ||
|
|
||
| import { BarChartWidget } from "../../../src/components/Bar Chart/BarChartWidgets"; |
There was a problem hiding this comment.
No description provided.