-
Notifications
You must be signed in to change notification settings - Fork 15
feat: migrate to gravity-ui/graph #2735
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
base: main
Are you sure you want to change the base?
Conversation
"@gravity-ui/components": "^4.4.0", | ||
"@gravity-ui/date-components": "^3.2.3", | ||
"@gravity-ui/date-utils": "^2.5.6", | ||
"@gravity-ui/graph": "^1.1.4", |
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.
lets also remove paranoid
}; | ||
|
||
export const QueryBlockComponent = ({className}: Props) => { | ||
return <div className={className} />; |
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.
should there really be no content here?
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.
yes, it's just an empty circle on schema
const content = useTooltipContent(block); | ||
|
||
return ( | ||
<Popover |
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.
If content is big enough, it is impossible to see all of it
Screen.Recording.2025-09-29.at.17.50.01.mov
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.
do we need actually? in this case for large graph block size will become too small for reading. I can implement simply canvas component but it's a bit complicated and I'm not sure there will be real user need
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.
do we need actually? in this case for large graph block size will become too small for reading. I can implement simply canvas component but it's a bit complicated and I'm not sure there will be real user need
What if we set maximum height to popup? Let it be scroll inside it. I think it's rare case, and no problem if it won't be beautiful enough...
return ( | ||
<div className={b('canvas-container')}> | ||
<YDBGraph key={theme} data={data} /> | ||
{true ? ( |
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.
lets remove it and get rid of @gravity-ui/paranoid
<div className={b('canvas-container')}> | ||
<YDBGraph key={theme} data={data} /> | ||
{true ? ( | ||
<GravityGraph data={data} theme={theme} key={theme} /> |
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.
Light theme is blinking when toggle tabs
Screen.Recording.2025-09-29.at.17.45.14.mov
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.
fixed
src/components/Graph/treeLayout.ts
Outdated
this.levels = []; | ||
} | ||
|
||
// Построение структуры дерева |
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.
lets or remove comments at all either translate it to english
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.
16 files reviewed, 6 comments
return ( | ||
<div className={b('canvas-container')}> | ||
<YDBGraph key={theme} data={data} /> | ||
{true ? ( |
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.
style: hardcoded true
condition for feature flag - consider using a configuration constant or feature flag instead
{true ? ( | |
{useGravityGraph ? ( |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/containers/Tenant/Query/QueryResult/components/Graph/Graph.tsx
Line: 36:36
Comment:
style: hardcoded `true` condition for feature flag - consider using a configuration constant or feature flag instead
```suggestion
{useGravityGraph ? (
```
How can I resolve this? If you propose a fix, please make it concise.
const onZoomInClick = () => { | ||
const cameraScale = graph.cameraService.getCameraScale(); | ||
graph.zoom({scale: cameraScale * ZOOM_STEP}); | ||
}; | ||
|
||
const onZoomOutClick = () => { | ||
const cameraScale = graph.cameraService.getCameraScale(); | ||
graph.zoom({scale: cameraScale / ZOOM_STEP}); | ||
}; | ||
|
||
const onResetZoomClick = () => { | ||
graph.zoom({scale: 1}); | ||
}; |
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.
style: event handlers should be wrapped with useCallback
to prevent unnecessary re-renders
const onZoomInClick = () => { | |
const cameraScale = graph.cameraService.getCameraScale(); | |
graph.zoom({scale: cameraScale * ZOOM_STEP}); | |
}; | |
const onZoomOutClick = () => { | |
const cameraScale = graph.cameraService.getCameraScale(); | |
graph.zoom({scale: cameraScale / ZOOM_STEP}); | |
}; | |
const onResetZoomClick = () => { | |
graph.zoom({scale: 1}); | |
}; | |
const onZoomInClick = useCallback(() => { | |
const cameraScale = graph.cameraService.getCameraScale(); | |
graph.zoom({scale: cameraScale * ZOOM_STEP}); | |
}, [graph]); | |
const onZoomOutClick = useCallback(() => { | |
const cameraScale = graph.cameraService.getCameraScale(); | |
graph.zoom({scale: cameraScale / ZOOM_STEP}); | |
}, [graph]); | |
const onResetZoomClick = useCallback(() => { | |
graph.zoom({scale: 1}); | |
}, [graph]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Graph/GraphControls.tsx
Line: 18:30
Comment:
style: event handlers should be wrapped with `useCallback` to prevent unnecessary re-renders
```suggestion
const onZoomInClick = useCallback(() => {
const cameraScale = graph.cameraService.getCameraScale();
graph.zoom({scale: cameraScale * ZOOM_STEP});
}, [graph]);
const onZoomOutClick = useCallback(() => {
const cameraScale = graph.cameraService.getCameraScale();
graph.zoom({scale: cameraScale / ZOOM_STEP});
}, [graph]);
const onResetZoomClick = useCallback(() => {
graph.zoom({scale: 1});
}, [graph]);
```
How can I resolve this? If you propose a fix, please make it concise.
const {graph, start} = useGraph(config); | ||
|
||
React.useEffect(() => { | ||
// на всякий случай, хотя маунт больше времени занимает, чем расчёт |
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.
style: Russian comment should be translated to English for consistency with project standards
// на всякий случай, хотя маунт больше времени занимает, чем расчёт | |
// Just in case, although mounting takes more time than calculation |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Graph/GravityGraph.tsx
Line: 69:69
Comment:
style: Russian comment should be translated to English for consistency with project standards
```suggestion
// Just in case, although mounting takes more time than calculation
```
How can I resolve this? If you propose a fix, please make it concise.
: block.name} | ||
{block.tables ? ( | ||
<div> | ||
<Text color="secondary">Tables: </Text> |
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.
style: hardcoded text 'Tables: ' should use i18n for internationalization
<Text color="secondary">Tables: </Text> | |
<Text color="secondary">{i18n('label_tables')}: </Text> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Graph/BlockComponents/StageBlockComponent.tsx
Line: 19:19
Comment:
style: hardcoded text 'Tables: ' should use i18n for internationalization
```suggestion
<Text color="secondary">{i18n('label_tables')}: </Text>
```
How can I resolve this? If you propose a fix, please make it concise.
const useTooltipContent = (block: ExtendedTBlock) => { | ||
const firstTab = block?.stats?.[0]?.group || ''; | ||
const [activeTab, setActiveTab] = useState(firstTab); | ||
|
||
return ( | ||
<TabProvider value={activeTab} onUpdate={setActiveTab}> | ||
<TabList className={b('tooltip-tabs')}> | ||
{block?.stats?.map((item) => ( | ||
<Tab value={item.group} key={item.group}> | ||
{item.group} | ||
</Tab> | ||
))} | ||
</TabList> | ||
{block?.stats?.map((item) => ( | ||
<TabPanel value={item.group} key={item.group}> | ||
{item.stats?.map(getStatsContent)} | ||
</TabPanel> | ||
))} | ||
</TabProvider> | ||
); | ||
}; |
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.
style: useTooltipContent
hook creates new JSX on every render - should be memoized for performance
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Graph/TooltipComponent.tsx
Line: 42:62
Comment:
style: `useTooltipContent` hook creates new JSX on every render - should be memoized for performance
How can I resolve this? If you propose a fix, please make it concise.
src/components/Graph/treeLayout.ts
Outdated
this.blocks = new Map(blocks.map((block: any) => [block.id, {...block}])); | ||
this.connections = connections; | ||
|
||
// Настройки отступов |
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.
style: Russian comment should be translated to English for consistency
// Настройки отступов | |
// Layout settings |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Graph/treeLayout.ts
Line: 17:17
Comment:
style: Russian comment should be translated to English for consistency
```suggestion
// Layout settings
```
How can I resolve this? If you propose a fix, please make it concise.
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔽
Current: 84.41 MB | Main: 85.61 MB
Diff: 1.20 MB (-1.40%)
✅ Bundle size decreased.
ℹ️ CI Information