Conversation
Summary of ChangesHello @jdolle, 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 refactors the mechanism for displaying row numbers in the schema proposal diff view. By transitioning from CSS-driven counters to a state-managed approach using React Context, the system can now accurately maintain and display row numbers even when certain rows are hidden or collapsed, significantly improving the user experience for reviewing complex schema changes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/apollo |
0.47.0-alpha-20260108014240-2cbf494fef8bf337c81f3c0aa00cff5e335dcffd |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/cli |
0.57.1-alpha-20260108014240-2cbf494fef8bf337c81f3c0aa00cff5e335dcffd |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/core |
0.20.0-alpha-20260108014240-2cbf494fef8bf337c81f3c0aa00cff5e335dcffd |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/envelop |
0.40.2-alpha-20260108014240-2cbf494fef8bf337c81f3c0aa00cff5e335dcffd |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/yoga |
0.47.0-alpha-20260108014240-2cbf494fef8bf337c81f3c0aa00cff5e335dcffd |
npm ↗︎ unpkg ↗︎ |
hive |
8.14.1-alpha-20260108014240-2cbf494fef8bf337c81f3c0aa00cff5e335dcffd |
npm ↗︎ unpkg ↗︎ |
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7473.hive-storybook.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the schema diff component to track row numbers using React context instead of CSS counters, which is a necessary change to support collapsing unchanged rows. While the approach is directionally correct, the current implementation introduces critical bugs related to state management in React. Specifically, the row counters are not reset on re-renders, and state is mutated directly during the render phase using incorrect hooks. My review includes suggestions to fix these issues to ensure the component behaves predictably and correctly.
| const [context, _] = useState({ | ||
| annotatedCoordinates: new Set<string>(), | ||
| oldDocumentRowNumber: 1, | ||
| newDocumentRowNumber: 1, | ||
| }); |
There was a problem hiding this comment.
Using useState here creates a critical bug. The context object is created once and then mutated by child components (ChangeRow). Because useState preserves this object across re-renders of AnnotatedProvider, the row counters are never reset. This will cause row numbers to increment indefinitely on subsequent re-renders, leading to incorrect numbering. The context value must be reset for each render of the diff.
| const [context, _] = useState({ | |
| annotatedCoordinates: new Set<string>(), | |
| oldDocumentRowNumber: 1, | |
| newDocumentRowNumber: 1, | |
| }); | |
| const context = { | |
| annotatedCoordinates: new Set<string>(), | |
| oldDocumentRowNumber: 1, | |
| newDocumentRowNumber: 1, | |
| }; | |
| const newDocumentRowNumber = useRef(ctx.newDocumentRowNumber); | ||
| const oldDocumentRowNumber = useRef(ctx.oldDocumentRowNumber); | ||
| if (props.type === 'addition') { | ||
| ctx.newDocumentRowNumber += 1; | ||
| } else if (props.type === 'removal') { | ||
| ctx.oldDocumentRowNumber += 1; | ||
| } else { | ||
| ctx.oldDocumentRowNumber += 1; | ||
| ctx.newDocumentRowNumber += 1; | ||
| } |
There was a problem hiding this comment.
This implementation has two critical issues:
-
Incorrect
useRefUsage:useRefpreserves its value across renders. It will not be updated with the new value fromctxif this component re-renders, leading to stale row numbers. You should use simple variables to capture the value for the current render pass. -
Mutation During Render: Modifying context during render (
ctx.newDocumentRowNumber += 1) is a side effect and a React anti-pattern. This will break in React's Strict Mode and is incompatible with concurrent features, leading to unpredictable behavior.
To fix the immediate bug with useRef, you should use simple variables. You will also need to update the JSX to access them directly (e.g., {oldDocumentRowNumber}) instead of via .current.
| const newDocumentRowNumber = useRef(ctx.newDocumentRowNumber); | |
| const oldDocumentRowNumber = useRef(ctx.oldDocumentRowNumber); | |
| if (props.type === 'addition') { | |
| ctx.newDocumentRowNumber += 1; | |
| } else if (props.type === 'removal') { | |
| ctx.oldDocumentRowNumber += 1; | |
| } else { | |
| ctx.oldDocumentRowNumber += 1; | |
| ctx.newDocumentRowNumber += 1; | |
| } | |
| const newDocumentRowNumber = ctx.newDocumentRowNumber; | |
| const oldDocumentRowNumber = ctx.oldDocumentRowNumber; | |
| if (props.type === 'addition') { | |
| ctx.newDocumentRowNumber += 1; | |
| } else if (props.type === 'removal') { | |
| ctx.oldDocumentRowNumber += 1; | |
| } else { | |
| ctx.oldDocumentRowNumber += 1; | |
| ctx.newDocumentRowNumber += 1; | |
| } | |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7473.hive-landing-page.pages.dev |
|
This change is not necessary if we go the route of determining the lines/words first, then render. |
1 participant
Background
We want to be able to collapse unchanged rows to reduce noise on schema proposal diff. The current way of adding row numbers is by counting the classes using css. This doesn't work when rows get hidden.
Description
This adds logic to count the rows using context instead of relying on css. this allows us to show the row numbers even if rows are collapsed.
This may still yet change again depending on how rows are hidden.