Skip to content

Color the row numbers for partial addition/deletions on schema proposals #7468

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.

Sign up for GitHub

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

Merged
jdolle merged 1 commit into from
Jan 7, 2026
Merged

Color the row numbers for partial addition/deletions on schema proposals #7468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions packages/web/app/src/components/target/proposals/schema-diff/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */
import { createContext, Fragment, ReactElement, ReactNode, useContext, useState } from 'react';
import {
createContext,
Fragment,
ReactElement,
ReactNode,
useContext,
useEffect,
useState,
} from 'react';
import {
astFromValue,
ConstArgumentNode,
Expand Down Expand Up @@ -35,6 +43,7 @@ import { SeverityLevelType } from '@/gql/graphql';
import { cn } from '@/lib/utils';
import { ExclamationTriangleIcon } from '@radix-ui/react-icons';
import { compareLists, diffArrays, matchArrays } from './compare-lists';
import { ChangeRowContext } from './context';

type RootFieldsType = {
query: GraphQLField<any, any>;
Expand Down Expand Up @@ -120,14 +129,20 @@ export function ChangeRow(props: {
ctx.annotatedCoordinates?.add(props.coordinate!);
}

// if the children include any additions or subtractions
const [added, setAdded] = useState(false);
const [removed, setRemoved] = useState(false);

return (
<>
<ChangeRowContext.Provider
value={{ change: { addition: added, removal: removed }, setAdded, setRemoved }}
>
<tr style={{ counterIncrement: incrementCounter }}>
<td
className={cn(
'schema-doc-row-old w-[42px] min-w-fit select-none bg-gray-900 p-1 pr-3 text-right text-gray-600',
props.className,
props.type === 'removal' && 'bg-red-900/30',
(props.type === 'removal' || removed) && 'bg-red-900/30',
props.type === 'addition' && 'invisible',
)}
/>
Expand All @@ -136,7 +151,7 @@ export function ChangeRow(props: {
'schema-doc-row-new w-[42px] min-w-fit select-none bg-gray-900 p-1 pr-3 text-right text-gray-600',
props.className,
props.type === 'removal' && 'invisible',
props.type === 'addition' && 'bg-green-900/30',
(props.type === 'addition' || added) && 'bg-green-900/30',
)}
/>
<td
Expand Down Expand Up @@ -182,7 +197,7 @@ export function ChangeRow(props: {
<td colSpan={3}>{annotation}</td>
</tr>
)}
</>
</ChangeRowContext.Provider>
);
}

Expand All @@ -191,6 +206,13 @@ function Keyword(props: { term: string }) {
}

function Removal(props: { children: ReactNode | string; className?: string }): ReactNode {
const { setRemoved, change } = useContext(ChangeRowContext);
useEffect(() => {
if (!change.removal) {
setRemoved(true);
}
}, [change.removal]);
Comment on lines +210 to +214
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This useEffect triggers a re-render of the parent component, which can impact performance and goes against our style guide rule to 'avoid setting state from useEffect' (line 19). The same logic is duplicated in the Addition component.

Please consider if there's an alternative to this pattern that avoids the extra render cycle. If the pattern is kept, the duplicated logic should be extracted into a custom hook for better maintainability.

References
  1. The style guide advises to 'avoid setting state from useEffect instead of deriving it'. The current implementation uses useEffect to set state on a parent component, which causes an extra render and is a pattern to be avoided if a better alternative exists. (link)

Copy link
Collaborator Author

@jdolle jdolle Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only trigger once on the parent and although this can be calculated in the parent, it's much more complicated since which row something ends up on is based on how we render it.


return (
<span
className={cn(
Expand All @@ -204,6 +226,12 @@ function Removal(props: { children: ReactNode | string; className?: string }): R
}

function Addition(props: { children: ReactNode; className?: string }): ReactNode {
const { setAdded, change } = useContext(ChangeRowContext);
useEffect(() => {
if (!change.addition) {
setAdded(true);
}
}, [change.addition]);
return (
<span className={cn('bg-[#11362b] hover:bg-green-900', props.className)}>{props.children}</span>
);
Expand Down
10 changes: 10 additions & 0 deletions packages/web/app/src/components/target/proposals/schema-diff/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { createContext } from 'react';

export const ChangeRowContext = createContext({
change: {
addition: false,
removal: false,
},
setAdded(_: boolean) {},
setRemoved(_: boolean) {},
});
Loading