Duplicate requests to /api/card from Query Builder

[iethree]

Describe the bug

Regression between 45 and 46.

The query builder makes 2 identical request to GET /api/card/{id} on load.

To Reproduce

• Make any question or model
• open the network inspector
• refresh the page
• see 2 of the same GET request

Expected behavior

1 request to GET /api/card/{id}

Information about your Metabase installation

{
  "browser-info": {
    "language": "en-US",
    "platform": "MacIntel",
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36",
    "vendor": "Google Inc."
  },
  "system-info": {
    "file.encoding": "UTF-8",
    "java.runtime.name": "OpenJDK Runtime Environment",
    "java.runtime.version": "18.0.1+0",
    "java.vendor": "Homebrew",
    "java.vendor.url": "https://github.com/Homebrew/homebrew-core/issues",
    "java.version": "18.0.1",
    "java.vm.name": "OpenJDK 64-Bit Server VM",
    "java.vm.version": "18.0.1+0",
    "os.name": "Mac OS X",
    "os.version": "12.4",
    "user.language": "en",
    "user.timezone": "America/Denver"
  },
  "metabase-info": {
    "databases": [
      "h2",
      "postgres"
    ],
    "hosting-env": "unknown",
    "application-database": "h2",
    "application-database-details": {
      "database": {
        "name": "H2",
        "version": "2.1.212 (2022-04-09)"
      },
      "jdbc-driver": {
        "name": "H2 JDBC Driver",
        "version": "2.1.212 (2022-04-09)"
      }
    },
    "run-mode": "prod",
    "version": {
      "date": "2023-06-14",
      "tag": "v0.46.5",
      "branch": "release-x.46.x",
      "hash": "272e3e6"
    },
    "settings": {
      "report-timezone": null
    }
  }
}

Severity

not user visible, an under-the-hood performance issue

Additional context

No response

[WiNloSt]

The first call is a little hard to track where it originates from but the second request is from here.

metabase/frontend/src/metabase/query_builder/actions/core/initializeQB.ts

Lines 107 to 112 in e7363d9

	async function fetchAndPrepareSavedQuestionCards(
	cardId: number,
	dispatch: Dispatch,
	getState: GetState,
	) {
	const card = await loadCard(cardId, { dispatch, getState });

In loadCards we specify reload: true

metabase/frontend/src/metabase/lib/card.js

Line 29 in e7363d9

await dispatch(Questions.actions.fetch({ id: cardId }, { reload: true }));

It'd be quite complicated to make sure this second request doesn't fire.

I could think of one of the possible fix:

1. when specifying reload: true it won't always reload and will use the cache from the past x milliseconds, so in case we call reload: true in rapid succession, we will only call the API once.

I however don't think this is worth pursuing since there would be no visible change to users and the fix would be quite complicated + need more discussion on the exact number we want to delay before actually firing a new request. I don't think it's worth all the effort at the end of the day.

[deniskaber]

The first call is a little hard to track where it originates from but the second request is from here.

The first call comes from MainNavbar component. We need it to select an active collection in the left menu.
As @WiNloSt mentioned, the second call has reload: true. Removing it can break some other use cases we have - like this test, for instance:

metabase/e2e/test/scenarios/models/model-indexes.cy.spec.js

Line 174 in 83ae490

it("should not reload the model for record in the same model", () => {

There we navigate to home page, then search some stuff in search box. Search api call loads metadata about a model, however this data does not contain the full entity data needed to display a model. Because of that we cannot just remove reload flag.

So, it seems, we should make this cache smarter to understand in which cases we have partial data in state.

The logic for the first api request has been added here - https://github.com/metabase/metabase/pull/28057/files#diff-a6742c995fe25a651bfe9c2bf58296681d8440cc022c9569d338150364fdfb88R135 , it is a deliberate change.

Also, in my first naive attempt to fix the issue, @ranquild mentioned that we should not create implicit logical dependecy between the left nav menu and main content area - #33898 (review).

In the end, we cannot just remove either of api calls, and the second one has reload: true to cope with some other use cases. I think we can add some logic to do reload only when current cached entity is not complete.