-
Notifications
You must be signed in to change notification settings - Fork 172
run introspection queries lazy and handle resulting errors better #1800
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
#1768 Bundle Size — 2.01MiB (+0.07%).3988b89(current) vs 8961f38 main#1758(baseline) Warning Bundle contains 14 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch pr/lazy-introspection Project dashboard Generated by RelativeCI Documentation Report issue |
commit: |
|
Open problem: clicking the "run in explorer" button will open the explorer and fill in the query, but if navigating to the explorer this way, the "We could not introspect your schema." screen will not open. I suspect that it opens and immediately closes because another command comes in to overwrite it. Okay, this is addressed |
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 pull request fixes error handling in introspection queries and defers their execution until the Explorer tab is visible. The main changes include:
Purpose: Fix uncaught promise rejections in introspection queries and defer their execution until needed.
Changes:
- Added
errorPolicy: "all"to query execution in v3 and v4 handlers to prevent promise rejections - Deferred introspection query execution until Explorer tab is visible (via
isVisiblecheck) - Refactored Explorer iframe management from state-based to ref-based using
useImperativeHandle - Updated TypeScript to 5.9.3 and added
ES2024.Promiselib forPromise.withResolverssupport
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Added ES2024.Promise lib to support Promise.withResolvers |
| src/extension/tab/v4/handler.ts | Added errorPolicy: "all" to executeQuery to prevent promise rejections |
| src/extension/tab/v3/handler.ts | Added errorPolicy: "all" to executeQuery to prevent promise rejections |
| src/application/components/Explorer/Explorer.tsx | Refactored to use ref-based iframe management and deferred introspection query |
| src/application/components/Queries/RunInExplorerButton.tsx | Updated to use explorerRef instead of embeddedExplorerIFrame |
| src/application/components/Queries/Queries.tsx | Updated prop from explorerIFrame to explorerRef |
| src/application/components/Mutations/Mutations.tsx | Updated prop from explorerIFrame to explorerRef |
| src/application/App.tsx | Changed from useState to useRef for explorer management |
| src/application/components/Explorer/GraphRefModal.tsx | Changed from receiving iframe to receiving postMessage function |
| src/testUtils/testMessageAdapter.ts | Exported TestAdapter interface for testing |
| src/application/components/Select/Option.tsx | Exported OptionProps interface for testing |
| package.json | Updated TypeScript from 5.5.4 to 5.9.3 |
| Test files | Updated to use explorerRef pattern instead of explorerIFrame |
| Development files | Added ErrorLink for debugging purposes |
Files not reviewed (2)
- development/client/package-lock.json: Language not supported
- development/server/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const postMessage = (message: OutgoingMessageEvent) => { | ||
| iframeRef.current.promise.then((embeddedExplorerIFrame) => { | ||
| postMessageToEmbed({ | ||
| embeddedExplorerIFrame, | ||
| message, | ||
| }); | ||
| }); | ||
| }; |
Copilot
AI
Jan 29, 2026
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 postMessage function should be wrapped in useCallback to ensure a stable reference. While it's defined outside useImperativeHandle, having it recreated on every render could cause issues if consumers of the ref hold onto the function reference. Additionally, it captures iframeRef from closure which is problematic since iframeRef.current is being recreated on every render (see separate comment).
| useEffect, | ||
| useImperativeHandle, | ||
| useRef, | ||
| useEffectEvent, |
Copilot
AI
Jan 29, 2026
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.
useEffectEvent is being imported from React but this hook was experimental and under RFC as of React 18. Verify that this hook is available and stable in React 19.2.1 (the version used in this project). If it's not available, this will cause a runtime error. Consider using a standard useCallback with appropriate dependencies or implementing a custom hook pattern to achieve the same effect.
| useEffectEvent, |
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 is stable.
| <Button | ||
| variant="secondary" | ||
| size="sm" | ||
| className="ml-auto" | ||
| disabled={!embeddedExplorerIFrame} | ||
| icon={<IconRun />} | ||
| onClick={() => { | ||
| if (embeddedExplorerIFrame) { | ||
| // send a post message to the embedded explorer to fill the operation | ||
| postMessageToEmbed({ | ||
| message: { | ||
| name: SET_OPERATION, | ||
| operation, | ||
| variables: JSON.stringify(variables), | ||
| }, | ||
| embeddedExplorerIFrame, | ||
| }); | ||
| currentScreen(Screens.Explorer); | ||
| } | ||
| explorerRef.current?.postMessage({ | ||
| name: SET_OPERATION, | ||
| operation, | ||
| variables: JSON.stringify(variables), | ||
| }); | ||
| currentScreen(Screens.Explorer); | ||
| }} |
Copilot
AI
Jan 29, 2026
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 disabled prop was removed from the Button component. Previously, the button was disabled when embeddedExplorerIFrame was null to prevent users from clicking before the iframe was ready. Now the button can be clicked at any time, and while explorerRef.current?.postMessage uses optional chaining to handle null/undefined, clicking the button when the explorer isn't ready will silently fail without user feedback. Consider adding the disabled prop back with a check like disabled={!explorerRef.current}, or handle this scenario with user feedback (e.g., a loading state or error message).
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.
We won't care about that first render that's the for a split second if at all.
| }); | ||
| } | ||
| }); | ||
| }, |
Copilot
AI
Jan 29, 2026
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 observer.subscribe call only provides a next callback but doesn't handle error or complete callbacks. If the observable errors (e.g., network failure, parsing error), the error will be silently ignored. Consider adding error and complete handlers to properly handle all observable states:
observer.subscribe({
next: (response) => { /* existing code */ },
error: (error) => { /* handle error */ },
complete: () => { /* optional cleanup */ }
});| }, | |
| }, | |
| error: (error) => { | |
| // Handle observable-level errors (e.g., network failures) similarly to | |
| // GraphQL-level introspection errors. | |
| const graphRefFromLocalStorage = getGraphRefFromLocalStorage(); | |
| if (graphRefFromLocalStorage) { | |
| setGraphRef(graphRefFromLocalStorage); | |
| } else { | |
| // Delay opening the modal for a moment. If the "Run in Explorer" button | |
| // navigates here, this might cause the HeadlessUI Dialog to call `onClose` | |
| // immediately because of the tab change | |
| setTimeout(() => { | |
| setShowGraphRefModal("triggeredByIntrospectionFailure"); | |
| }, 100); | |
| } | |
| postMessage({ | |
| name: SCHEMA_ERROR, | |
| errors: undefined, | |
| error: | |
| error instanceof Error | |
| ? error.message | |
| : String(error), | |
| }); | |
| }, | |
| complete: () => { | |
| // No-op: placeholder for any future cleanup when introspection completes. | |
| }, |
| setPreviousClientId(clientId); | ||
| setSchema(null); | ||
| } | ||
| const iframeRef = useRef(Promise.withResolvers<HTMLIFrameElement>()); |
Copilot
AI
Jan 29, 2026
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 iframeRef is initialized with useRef(Promise.withResolvers<HTMLIFrameElement>()) on every render, creating a new Promise instance each time the component re-renders. This breaks the ref pattern because:
- The
postMessagefunction referencesiframeRef.current.promisewhich will be a different promise after each render - Any messages sent via
postMessageduring one render will be waiting on a promise that gets replaced on the next render - The
resolvecall iniframeRefFnwill resolve the wrong promise instance if it happens after a re-render
The initialization should only happen once. Consider using lazy initialization:
const iframeRef = useRef<{promise: Promise<HTMLIFrameElement>; resolve: (iframe: HTMLIFrameElement) => void; reject: (reason?: any) => void} | null>(null);
if (!iframeRef.current) {
iframeRef.current = Promise.withResolvers<HTMLIFrameElement>();
}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.
What's wrong with Copilot today? Of course that value will only be used the first time.
This fixes two things:
executeQuerywould callclient.query(without specifying anerrorPolicy, which could cause the promise to reject - our logic expected it to resolve with theerror, though. This did lead to a number of uncaught promise rejections.Also, the whole Explorer iframe logic was very complicated and added a lot of code to deal with the
nullishness of the iframe - which only played a role in the initial render and would become irrelevant already on the second render. I moved this to anuseImperativeHandle-managed ref instead.Some of these changes also required a TS version bump, so I did that too.
I also bumped the ESLint rules for react-hooks by 3 major versions (the current ones didn't know
useEffectEvent) and also removed that other non-officialeslint-plugin-reactsince it seems seriously outdated.