Skip to content

batches: reimplement tabs with static order

Warren Gifford requested to merge kr/static-tabs into main

Created by: courier-new

Potentially closes https://github.com/sourcegraph/sourcegraph/issues/40105. 👀

I couldn't reproduce a scenario where the height of the preview page on the spec file tab would stay too tall, but what I did notice was that in the Chromatic snapshots that were too tall, they reported the exact same height as the height of the preview page with the changesets tab active. I also noticed that when navigating directly to the preview page with the spec file tab focused via the URL, the page would initially load the changesets tab and switch over to the spec file tab only after half a second or so. So my theory is that the transition is just happening too slowly in Chromatic -- the tab contents would change in time for the snapshot, but the total page size might not.

The delay is due to the way we sync the active tab with the URL parameters. We have a generalized Tabs wrapper component which infers the names and order of all of its children TabPanels via dispatching actions to a reducer when each TabPanel first mounts. That reducer state is then shared via React.context with the parent Tabs, which in turn uses it to read and write from the URL parameters.

This had the advantage that we could add, remove, and switch up the order of the Tabs dynamically and still be able to map from tab name <> index at any point along the way. But, it had the disadvantage that we would consume as many render cycles as there were tabs to build up the initial ordering, since each tab would need to dispatch an action announcing itself in the order before the parent Tabs would know the whole arrangement. This was what caused that half second delay and flash of the wrong tab when loading a page with the URL parameter set for a tab other than the default one.

I think we've also seen this on the BatchChangeDetailsPage, which would make sense given it used the same URL-syncing tabs implementation.

So, this PR re-implemented the tabs as static configurations, since we weren't taking advantage of the dynamic abilities anyway. As a result, we're able to render the right initial tab right away, which is better UX anyway but also hopefully means we see less of the screen height flakes.

Net negative lines of code (thanks, React context boilerplate!) isn't a bad thing, either. 🙂

Test plan

Tested manually for regressions in the URL parameter syncing on both the preview and details pages. Time will tell if Chromatic sees the height problem go away...

App preview:

Check out the client app preview documentation to learn more.

Merge request reports

Loading