-
Notifications
You must be signed in to change notification settings - Fork 122
feat: new hive lab #7374
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
feat: new hive lab #7374
Conversation
Summary of ChangesHello @mskorokhodov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a brand new 'Hive Laboratory' feature, designed to be a powerful and interactive environment for GraphQL API exploration and testing. It provides a rich set of tools for building, executing, and managing GraphQL operations, along with features for history tracking, environment configuration, and preflight scripting. The new laboratory aims to significantly enhance the developer experience by centralizing various GraphQL interaction capabilities within the application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new 'Laboratory' feature, which is a powerful GraphQL IDE integrated into the application. The changes are extensive, adding many new components, hooks, and dependencies. The overall structure is well-organized.
A significant issue is the consistent misspelling of 'laboratory' as 'labaratory' throughout the codebase. This affects file names, directory names, component names, CSS classes, and more. This should be corrected globally for code quality and maintainability.
I have also provided specific feedback on a potential bug in parsing environment variables, a UI issue in the editor's theme, and an inconsistency in string replacement methods.
packages/web/app/src/index.css
Outdated
| --chart-2: 340 75% 55%; | ||
| } | ||
|
|
||
| .hive-labaratory { |
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.
| () => | ||
| Object.values( | ||
| ( | ||
| schema?.getType(props.field.type.toString().replace(/\[|\]|!/g, '')) as GraphQLObjectType |
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.
This line uses replace() with a global regex, while line 427 uses replaceAll() for the same purpose. For consistency and readability, it's better to use replaceAll() in both places.
| schema?.getType(props.field.type.toString().replace(/\[|\]|!/g, '')) as GraphQLObjectType | |
| schema?.getType(props.field.type.toString().replaceAll(/[\[\]!]/g, '')) as GraphQLObjectType |
packages/web/app/src/laboratory/components/laboratory/builder.tsx
Outdated
Show resolved
Hide resolved
| base: 'vs-dark', | ||
| inherit: true, | ||
| rules: [ | ||
| { token: '', foreground: 'F8F9FA', background: 'fffffe' }, |
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.
In the darkTheme definition, the default token ('') has a background color of 'fffffe', which is nearly white. This will likely clash with the dark editor background (#18181b). To fix this, you can remove the background property to let it inherit the editor's background color.
| { token: '', foreground: 'F8F9FA', background: 'fffffe' }, | |
| { token: '', foreground: 'F8F9FA' }, |
ee2488e to
abe1455
Compare
jdolle
left a comment
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.
Only minor comments.
Although some files are quite large, the components are well designed and make great use of memo, callbacks, etc to keep the code clean and efficient.
| You haven't selected any endpoint yet. Get started by selecting an endpoint. | ||
| </EmptyDescription> | ||
| </EmptyHeader> | ||
| {/* <EmptyContent> |
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.
minor: should this be removed?
| </CommandGroup> | ||
| <CommandSeparator /> | ||
| <CommandGroup heading="Schema"> | ||
| {/* <CommandItem |
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.
^
| variables: Object.fromEntries( | ||
| value | ||
| ?.split('\n') | ||
| .filter(line => line.trim() && !line.trim().startsWith('#')) |
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.
It looks like this is to support adding comments within env vars?
This is a new feature for us. Maybe we can update documentation examples to include comments to 1) show this feature, 2) add more inline context to our examples
| // </LaboratoryProvider> | ||
| // </div> | ||
| // ); | ||
| // }; |
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 this file be deleted?
| return ( | ||
| <Editor | ||
| value={JSON.stringify( | ||
| JSON.parse((historyItem as LaboratoryHistoryRequest)?.response ?? '{}'), |
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.
It's an edge case, but what if the response body is invalid JSON?
Should we show that it's invalid json or show the text content?
| <Query operation={operation} isReadOnly={isReadOnly} /> | ||
| </ResizablePanel> | ||
| <ResizableHandle /> | ||
| <ResizablePanel minSize={10} defaultSize={30} className="!overflow-visible"> |
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.
I assume your styling is right, but I expected overflow-auto to allow scrolling
| import { clsx, type ClassValue } from "clsx" | ||
| import { twMerge } from "tailwind-merge" | ||
|
|
||
| export function cn(...inputs: ClassValue[]) { |
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.
minor: we have a package for this already. But if we want to keep this independent with the plan to publish as a separate package then I'm fine with this.
|
|
||
| const getLocalStorageState = (key: string, defaultValue: any) => { | ||
| const value = localStorage.getItem(`${localStoragePrefix}${key}`); | ||
| return value ? JSON.parse(value) : defaultValue; |
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.
Something maybe to consider for the future is if the localstorage can be corrupted or if we want to migrate storage to another format, maybe we gracefully handle syntax errors
|
|
||
| const createOperation = useMemo( | ||
| () => | ||
| throttle((collection: LaboratoryCollection, operation: LaboratoryCollectionOperation) => { |
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.
It's difficult to follow the logic for these callbacks so maybe this is already handled, but in addition to throttling, I think this should only allow one request at a time so that if for some reason the request is taking longer, that duplicate operations aren't accidentally created by impatient users.
|
|
||
| setIntrospection(response.data as IntrospectionQuery); | ||
| } catch { | ||
| toast.error('Failed to fetch schema'); |
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.
Are there cases where this error will show up repetitively because the schema keeps trying to get fetched? If so, let's make sure not to spam the UI with alerts because that can be annoying
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7374.hive-storybook.pages.dev |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7374.hive-landing-page.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
|
@mskorokhodov Can you please take care of type-checking and linting failing? |
n1ru4l
left a comment
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.
Legit to merge after addressing failing checks
3c5577a to
86cdca5
Compare
Background
Replacement for GraphiQL
Description
New hive laboratory as replacement for new GraphiQL
Checklist