Copilot commented on code in PR #15647:
URL: https://github.com/apache/pinot/pull/15647#discussion_r2060407626
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/AddRealtimeTableOp.tsx:
##########
@@ -163,6 +170,14 @@ export default function AddRealtimeTableOp({
useEffect(()=>{
setTableObj({...tableObj,"tableType":tableType})
},[])
+ // Sync state when toggling between simple and JSON view
+ useEffect(() => {
Review Comment:
The useEffect dependency array only includes 'editView' even though it
references 'tableObj' and 'jsonTableObj'. Consider adding these dependencies to
prevent potential stale state issues during view toggling.
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/AddOfflineTableOp.tsx:
##########
@@ -255,13 +287,32 @@ const checkFields = (tableObj,fields) => {
open={true}
handleClose={hideModal}
handleSave={handleSave}
- title={`Add ${tableType} Table`}
+ title={
+ <div style={{ display: 'flex', alignItems: 'center', justifyContent:
'space-between' }}>
+ <span>Add {tableType} Table</span>
+ <ButtonGroup size="small" color="primary">
+ <Button
+ variant={editView === EditView.SIMPLE ? 'contained' : 'outlined'}
+ onClick={() => setEditView(EditView.SIMPLE)}
+ >
+ Simple
+ </Button>
+ <Button
+ variant={editView === EditView.JSON ? 'contained' : 'outlined'}
+ onClick={() => setEditView(EditView.JSON)}
+ >
+ Json
Review Comment:
[nitpick] Consider using consistent casing for the JSON view label (e.g.,
'JSON' instead of 'Json') to match the enum value.
```suggestion
JSON
```
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/AddRealtimeTableOp.tsx:
##########
@@ -267,13 +300,32 @@ const checkFields = (tableObj,fields) => {
open={true}
handleClose={hideModal}
handleSave={handleSave}
- title={`Add ${tableType} Table`}
+ title={
+ <div style={{ display: 'flex', alignItems: 'center', justifyContent:
'space-between' }}>
+ <span>Add {tableType} Table</span>
+ <ButtonGroup size="small" color="primary">
+ <Button
+ variant={editView === EditView.SIMPLE ? 'contained' : 'outlined'}
+ onClick={() => setEditView(EditView.SIMPLE)}
+ >
+ Simple
+ </Button>
+ <Button
+ variant={editView === EditView.JSON ? 'contained' : 'outlined'}
+ onClick={() => setEditView(EditView.JSON)}
+ >
+ Json
Review Comment:
[nitpick] Consider using consistent casing for the JSON view label (e.g.,
'JSON' instead of 'Json') to match the enum value.
```suggestion
JSON
```
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/AddOfflineTableOp.tsx:
##########
@@ -150,6 +156,14 @@ export default function AddOfflineTableOp({
useEffect(()=>{
setTableObj({...tableObj,"tableType":tableType})
},[])
+ // Sync state when toggling between simple and JSON view
+ useEffect(() => {
Review Comment:
The dependency array for the state synchronization effect only lists
'editView' though it also uses 'tableObj' and 'jsonTableObj'. Including these
in the dependencies can help avoid potential stale state issues.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
