UnderlineNav doesn't update its tabs when the items change

[jaked]

Description

If you change the items in an UnderlineNav, it doesn't update the tabs.

In the video, I'm generating an UnderlineNav from numItems; if I change the useState initializer, the whole component remounts and the UnderlineNav tabs update; but if I change numItems via setState it causes a rerender but not a remount, and the tabs do not update.

CleanShot.2024-09-03.at.14.29.18.mp4

As far as I can see, this is because the list of children is kept in React state (responsiveProps) that is not updated when the children change. It looks like this was fixed in #2618 but broken again in #3361.

See https://github.com/jaked/primer-react-UnderlineNav-bug for a repro.

Steps to reproduce

as above

Version

v36.27.0

Browser

No response

[lesliecdubs]

👋🏻 Thanks for reporting.

For the Primer First Responder who picks this up: please investigate and add an appropriate size: label. If it's <30 min to execute, it would be great to pick up as part of the FR queue; otherwise, you can move it back to the Inbox for further review.

[camertron]

Primer first responder here 👋 I did a semi-deep dive into this today. The bottom line is that the component just isn't architected to support adding or removing tabs. Supporting such behavior will involve significant refactoring work, so I've sized it at rock.

Additional details

I was actually able to get things 90% working with a few minor changes, but the component exhibited a number of curious behaviors as a result, mostly around the calculations for remaining horizontal space.

The component calculates the width of each child item and stores the widths in an array in the parent. To do this, it wraps the component in an instance of <UnderlineNavContext.Provider> and passes a callback for setting the widths array. When an Item renders, it calculates its width and calls the callback to add the width to the array. Unfortunately the width array is never cleared, so re-rendering causes the adding of additional widths, i.e. beyond the number of actual tabs. In other words, if there are 3 existing tabs and therefore 3 entries in the widths array, adding a new tab will result in 4 tabs but 7 widths. I should caveat this by saying that only items that have changed will cause a new width to be added, so 7 is the max possible. The point is that the array is append-only and can easily get out-of-sync as tabs are added, removed, or updated.

[lesliecdubs]

Thanks for investigating @camertron.

@jaked can you tell us more about your use case for changing the items in UnderlineNav on the fly?

[jaked]

Thanks @camertron / @lesliecdubs.

We're using it in a tabbed file editor, where files can be added / removed / renamed.

[lesliecdubs]

@jaked is this use case in dotcom or another production app? Can you share a little more about the business goals of the UI you're working on? More info will help us make better decisions about how to prioritize further action. Thanks!

[jaked]

This is for a GitHub Next prototype called GitHub Spark. Tagging @drifkin for followup.

[lesliecdubs]

Thanks. @dipree given the described use case above, what do you think about where this sits in priorities for Primer? Given this assessment, we propose it's a size: rock which corresponds to multiple weeks of engineering work.

[broccolinisoup]

Just to add additional context that we had another issue reported - seems like it got closed due to being stale- related to the same problem and as @camertron mentioned that it is an intentional decision (some more context on the design decision) and requires refactoring.

I'd be happy to do a quick spike about how much work it requires to support this if we need - just let me know 🙌🏻

[lesliecdubs]

@broccolinisoup it would be helpful if you could timebox a spike (<1hr) to determine how much work it might take to support this. I'll assign to you as a next step, but give a shout if you don't have capacity to fit this in.