-
Notifications
You must be signed in to change notification settings - Fork 17
feat: use react-unipika library #3186
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
Conversation
|
@greptile-review |
8b78d04 to
fb22b0d
Compare
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.
Pull request overview
This PR migrates from a custom unipika implementation to the @gravity-ui/react-unipika library (v0.5.0), significantly simplifying the codebase by removing ~1000 lines of custom code. The migration affects the JsonViewer component and all its consumers across the application.
Key Changes:
- Replaced custom unipika implementation with
@gravity-ui/react-unipikalibrary - Refactored JsonViewer component to use ReactUnipika component as wrapper
- Added
scrollContainerRefprop to JsonViewer and threading it through component hierarchies
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added @gravity-ui/react-unipika v0.5.0 dependency |
| package-lock.json | Lock file update for new dependency |
| src/components/JsonViewer/JsonViewer.tsx | Major refactor to use ReactUnipika library, simplified from ~370 to ~140 lines |
| src/components/JsonViewer/JsonViewer.scss | Updated styles to work with library's class names (g-ru-cell) |
| src/components/JsonViewer/i18n/en.json | Removed unused i18n keys, added new error message key |
| src/components/JsonViewer/unipika/* | Deleted custom unipika implementation files (~500 lines) |
| src/components/JsonViewer/components/* | Deleted custom components replaced by library (Cell, ToggleCollapseButton, etc.) |
| src/components/JsonViewer/utils.ts | Deleted helper utilities no longer needed |
| src/containers/Tenant/Query/QueryResult/components/QueryJSONViewer/QueryJSONViewer.tsx | Added scrollRef for JsonViewer integration |
| src/containers/Tenant/Diagnostics/Describe/Describe.tsx | Passed scrollContainerRef to JsonViewer |
| src/containers/Tenant/Diagnostics/TopicData/* | Threading scrollContainerRef through TopicData component hierarchy |
| src/containers/Configs/components/Config/Config.tsx | Added wrapper structure and scrollContainerRef for JsonViewer |
| src/containers/Configs/components/Config/Config.scss | New styles for Config wrapper container |
| src/containers/Configs/Configs.scss | Removed unused CSS variable |
| src/containers/Tenant/Diagnostics/TopicData/FullValue.tsx | Deleted unused component |
src/containers/Tenant/Query/QueryResult/components/QueryJSONViewer/QueryJSONViewer.tsx
Outdated
Show resolved
Hide resolved
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.
24 files reviewed, 2 comments
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.
24 files reviewed, 1 comment
src/containers/Tenant/Diagnostics/TopicData/TopicMessageDetails/components/TopicMessage.tsx
Outdated
Show resolved
Hide resolved
7e7fc92 to
2830759
Compare
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.
24 files reviewed, 1 comment
| "description_search": "Search...", | ||
| "description_matched-rows": "Matched rows", | ||
| "description_full-value": "Full value", | ||
| "context_case-sensitive-search": "Case sensitive search enadled", |
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.
syntax: typo: "enadled" should be "enabled"
| "context_case-sensitive-search": "Case sensitive search enadled", | |
| "context_case-sensitive-search": "Case sensitive search enabled", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/JsonViewer/i18n/en.json
Line: 8:8
Comment:
**syntax:** typo: "enadled" should be "enabled"
```suggestion
"context_case-sensitive-search": "Case sensitive search enabled",
```
How can I resolve this? If you propose a fix, please make it concise.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.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
| scrollContainerRef={scrollRef} // key is used to reset JsonViewer state to collapsed due to performance issues on close fullscreen mode if nodes quantity is big enough https://github.com/ydb-platform/ydb-embedded-ui/issues/2265 | ||
| key={String(isFullscreen)} |
Copilot
AI
Dec 8, 2025
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.
The comment explaining the key prop is placed on the wrong line. It should be on line 24 (with the key prop) rather than line 23 (with the scrollContainerRef prop).
| scrollContainerRef={scrollRef} // key is used to reset JsonViewer state to collapsed due to performance issues on close fullscreen mode if nodes quantity is big enough https://github.com/ydb-platform/ydb-embedded-ui/issues/2265 | |
| key={String(isFullscreen)} | |
| scrollContainerRef={scrollRef} | |
| key={String(isFullscreen)} // key is used to reset JsonViewer state to collapsed due to performance issues on close fullscreen mode if nodes quantity is big enough https://github.com/ydb-platform/ydb-embedded-ui/issues/2265 |
| database: string; | ||
| databaseFullPath: string; | ||
| scrollContainerRef: React.RefObject<HTMLElement>; | ||
| scrollContainerRef: React.RefObject<HTMLDivElement>; |
Copilot
AI
Dec 8, 2025
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.
[nitpick] The scrollContainerRef type was changed from React.RefObject<HTMLElement> to React.RefObject<HTMLDivElement>, but this is passed to JsonViewer which expects React.RefObject<HTMLElement>. While HTMLDivElement extends HTMLElement and is technically compatible, this creates an unnecessary type narrowing that could cause issues if the ref is used in other contexts. Consider keeping the more general React.RefObject<HTMLElement> type for consistency with other components.
| scrollContainerRef: React.RefObject<HTMLDivElement>; | |
| scrollContainerRef: React.RefObject<HTMLElement>; |
| {renderToolbar()} | ||
| {renderTable()} | ||
| {renderFullValueModal()} | ||
| {/* @ts-ignore will be fixed shortly in library*/} |
Copilot
AI
Dec 8, 2025
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.
The @ts-ignore comment indicates a type issue with the ReactUnipika component. While the comment mentions "will be fixed shortly in library", it would be better to use @ts-expect-error instead of @ts-ignore so that TypeScript will error if the issue is already fixed in the library. Additionally, consider adding a more specific explanation of what type issue is being suppressed.
| {/* @ts-ignore will be fixed shortly in library*/} | |
| {/* @ts-expect-error Type mismatch: ReactUnipika props do not match expected types due to library issue. Remove when library is updated. */} |
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.
24 files reviewed, no comments
Stand
closes #1952
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 62.48 MB | Main: 62.34 MB
Diff: +0.14 MB (0.23%)
ℹ️ CI Information