Allow GraphiQL apps control over closing tabs

[klippx]

Allow GraphiQL apps control over closing tabs

This feature is about closing tabs.

Current functionality:

• When closing a tab, it just happens, the tab is removed and all state is gone

Requested functionality

• Grant possibility to give control to the user (i.e. the app that mounts the GraphiQL component) if they are OK with closing a particular tab
• This could for instance be an dialogue "Are you sure you want to close the tab?"

In our case, we are working on a Collection plugin that you can click to read saved queries / variables / headers from a DB, and this opens it in a new tab. So we need to sync tabState with the list of Collection items. And if a user starts to change a Collection item, we want to warn the user that changes will be lost.

It is the same as if the user selected a file in a code editor, started to change it, and then tries to close the tab.

Mock screenshots

[changeset-bot]

🦋 Changeset detected

Latest commit: be49600

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
graphiql	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov]

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 67.30%. Comparing base (1c1d2b5) to head (be49600).
Report is 9 commits behind head on main.

Files	Patch %	Lines
packages/graphiql/src/components/GraphiQL.tsx	68.42%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3563      +/-   ##
==========================================
- Coverage   67.60%   67.30%   -0.31%     
==========================================
  Files         121      121              
  Lines        6968     7016      +48     
  Branches     2251     2243       -8     
==========================================
+ Hits         4711     4722      +11     
- Misses       2240     2277      +37     
  Partials       17       17              

Files	Coverage Δ
packages/graphiql/src/components/GraphiQL.tsx	78.41% <68.42%> (-0.03%)	⬇️

... and 2 files with indirect coverage changes

[acao]

i think we should move all of this into closeTab itself, even what was directly in the handler here before, making it async. i see no reason to let this be an implementation detail for sdk users, as if the context they are using has this logic they would expect this as as an internal detail of the method if it's exposed to the user yeah?

  const closeTab = useCallback<EditorContextType['closeTab']>(
    async index => {
      if (await closeTabConfirmation(index)) {
      // this could be a breaking change for users who are already stopping 
      // execution manually like graphiql does, however stopping execution twice should not
      // cause any errors?
        if (index === tabState.activeTabIndex) {
          stop();
        }
        setTabState(current => {
          const updated = {
            tabs: current.tabs.filter((_tab, i) => index !== i),
            activeTabIndex: Math.max(current.activeTabIndex - 1, 0),
          };
          storeTabs(updated);
          setEditorValues(updated.tabs[updated.activeTabIndex]);
          onTabChange?.(updated);
          return updated;
        });
      }
    },
    [
      onTabChange,
      setEditorValues,
      storeTabs,
      closeTabConfirmation,
      tabState.activeTabIndex,
    ],
  );

and then here in graphiql:

 <Tab.Close
    onClick={async () => editorContext.closeTab(index)}
/>

so then sdk users have this in const { openTab, closeTab } = useEditorContext()

[dimaMachina]

the reason why it was not inside closeTab in ExecutionContextProvider is that ExecutionContextProvider is children of EditorContextProvider and can't be accessible from EditorContextProvider

[acao]

this is awesome, and makes a lot of sense! i'm glad you're able to achieve this with your app. just the one change,,I hope that's ok!

[klippx]

this is awesome, and makes a lot of sense! i'm glad you're able to achieve this with your app. just the one change,,I hope that's ok!

Absolutely, I don't mind a little boy scouting, looks much cleaner now overall. I think this looks good to go now.

PS:

i'm glad you're able to achieve this with your app.

Yea, it totally works - but... Since the plugin is handling its own state management and its own data fetching, it is a bit tricky, since I need the top level component that mounts <GraphiQL /> to essentially get the confirmation from a component further down the component tree. I got it to work using event emitters and listeners, which I think is not React-kosher but still fine, since I don't want to lift state further up, that price is much higher to pay since I want my plugin to manage its own state.

• Just sharing my solution FYI, like I said I am fine with this.

[klippx]

@acao Are we good to go?

[acao]

I think this is awesome, I would love to hear from my colleauges first as well, but if not I think we can merge soon!

[klippx]

@thomasheyenbrock can you take a look at this as well? 🙏

[dimaMachina]

this now adds confirmation on netlify preview, so it will be annoying

[dimaMachina]

it is better to add independent test for confirmation, and not merge it with existent one

[dimaMachina]

Why we need to pass this function to context? It’s used only in this component

[dimaMachina]

this will never be called, because we can't use execution context inside the editor context, because we already use editor context inside execution context. to reproduce it simply change

Suggested change

	executionContext?.stop();
	executionContext!.stop();

and use useExecutionContext({ notNull: true });

[dimaMachina]

I added e2e tests, some refactoring of moving executionContext.stop() can be done after moving to zustand