fix(dashboard): Tab menu visible for urls trailing '/'

[CuriousCorrelation]

Description

This bug #10666 was due to this

gitpod/components/dashboard/src/Menu.tsx

Line 109 in 402894b

const isWorkspacesUI = ["/workspaces"].includes(location.pathname);

failing if url contained trailing '/'.

Here's a test

var url = new URL('https://gitpod.io/workspaces/')
>> ["/workspaces"].includes(url.pathname)
<- false

So if that was true, it should also happen to

gitpod/components/dashboard/src/Menu.tsx

Line 108 in 402894b

const isMinimalUI = ["/new", "/teams/new", "/open"].includes(location.pathname);

which was indeed the case, see https://gitpod.io/new/ (notice trailing '/').

A simple solution would have been to include another element ("workspaces/") in the array or discard trailing '/' and check with includes. Adding a new element can potentially cause similar problem if we were to forget duplicating for some new path. For the second option, I first thought of regex but it got really unreadable, so instead I chose split and filter

url.pathname.split('/').filter(p => p).join('/')

This would work but what if we are on a resource but not exactly on base? For example

(Browser console)

const url1 = new URL('https://gitpod.io/workspaces/testing')
var testingSubpath = url1.pathname.split('/').filter(p => p).join('/')
testingSubpath
"workspaces/testing"

var testing = ["workspaces", "teams/new", "open"]

Tab menu shouldn't be displayed in path workspace/testing because it is isWorkspacesUI (a subdirectory)

testing.includes(testingSubpath)
false

A better way would be to test "subpath" against "rootpath"

(Browser console)

var url1 = new URL("https://gitpod.io/teams/new/testing")
var trimmedPath = url1.pathname.split('/').filter(Boolean).join('/')
trimmedPath
"teams/new/testing"

Now if we were to test against

var testing = ["workspaces", "teams/new", "open"]

we would get an array

testing.map(r => trimmedPath.startsWith(r))
Array(3) [ false, true, false ]

on which we can do an exhaustive search like

testing.map(r => trimmedPath.startsWith(r)).some(Boolean)
true

This would also work for isAdminUI check

gitpod/components/dashboard/src/Menu.tsx

Line 110 in 402894b

const isAdminUI = window.location.pathname.startsWith("/admin");

(Browser console)

const url = new URL('https://gitpod.io/admin/testing')
const isAdminUI = url.pathname.startsWith("/admin")
isAdminUI
true
var testing = ["admin"]
var trimmedPath = url.pathname.split('/').filter(Boolean).join('/')
trimmedPath
"admin/testing"
testing.map(r => trimmedPath.startsWith(r)).some(Boolean)
true

---

If this subpath check behaviour isn't exactly what we need, we can swap positions and get exact results

var testing = ["admin"]
testing.map(r => r.startsWith(testingSubpath)).some(Boolean)
false
testing.map(r => testingSubpath.startsWith(r)).some(Boolean)
true

Related Issue(s)

Fixes #10666

How to test

Described above

Release Notes

Fixed Tab menu being visible for urls with trailing '/'

Documentation

Does this PR require updates to the documentation at www.gitpod.io/docs?

• No

• /werft with-preview

[werft-gitpod-dev-com]

annotations in the pull request changed, but user is not allowed to start a job

[werft-gitpod-dev-com]

annotations in the pull request changed, but user is not allowed to start a job

[werft-gitpod-dev-com]

annotations in the pull request changed, but user is not allowed to start a job

[gtsiolis]

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.0
(with .werft/ from main)

[gtsiolis]

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.1
(with .werft/ from main)

[werft-gitpod-dev-com]

started the job as gitpod-build-fix-dashboard-path.0 because the annotations in the pull request description changed
(with .werft/ from main)

[laushinka]

Added werft with-preview in the description

[laushinka]

Hi @CuriousCorrelation, sorry for the delay in reviewing. The build failed, and to make sure that we have everything up to date, would you mind rebasing to the latest in main? 🙏🏽

[CuriousCorrelation]

@laushinka No worries at all! Branch should be updated now

[CuriousCorrelation]

@laushinka I believe this PR should also resolve #10499

Would it be possible to test this via deployment?

[laushinka]

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.2
(with .werft/ from main)

[laushinka]

Thank you for this contribution, @CuriousCorrelation!
It indeed fixed #10666. It doesn't yet resolve #10499. You're very welcome to pick that up for your next PR, or include it in this one :)

Before doing that, would you mind writing a simple test for inResource()? It should be very similar and as straightforward as this test.
This will also make it much easier to test the #10499 cases should you or someone else pick that one :)

Thanks again 🙏🏽

[CuriousCorrelation]

Writing tests would be no trouble at all. I'll try to get it done as soon as possible.

[CuriousCorrelation]

@laushinka Great idea for adding tests! Not only did it help me fix a couple of edge issues, I believe it also fixed #10499

The problem was the trim function's application on first argument and not on second; the list of resources, because both of them can have a trailing or leading '/'.

A couple of changes from the last commit:

1. Moved inResource to utils.ts. I figured that'd be a better location since it's not technically a 'component'.
2. Which meant we could add a utils.test.ts for placing inResource's tests. Also easier to add more cases.

Let me know if these changes are fine, or if there's something I need to change.

[laushinka]

@laushinka Great idea for adding tests! Not only did it help me fix a couple of edge issues, I believe it also fixed #10499

That's awesome, @CuriousCorrelation! I'm on vacation right now, so let me tag my team @gitpod-io/engineering-webapp so someone could take over :)
Otherwise I will pick this up again next Monday.

[laushinka]

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.3
(with .werft/ from main)

[laushinka]

Hi @CuriousCorrelation, I'm back from vacation now - sorry that it's taking a long time.

The build is failing because we're missing a license header for utils.test.ts. Would you mind doing the following:

1. Run leeway run components:update-license-header in the terminal
2. Rebase to main.

[CuriousCorrelation]

@laushinka No worries at all, I totally understand how difficult it can get managing large projects.

We should be good to go now, but please feel free to let me know if there are any other changes I should look into!

[laushinka]

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.4
(with .werft/ from main)

[laushinka]

Connection got closed - running this again

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.5
(with .werft/ from main)

[laushinka]

Not sure why builds are failing. Trying again..

/werft run

👍 started the job as gitpod-build-fix-dashboard-path-fork.6
(with .werft/ from main)

[laushinka]

@CuriousCorrelation This looks good! I'm happy to approve this (finally).

#10499 is not yet fixed, which I believe is due to this line. All that needs to be done I believe is replacing that check with your new logic 😉

I don't want to block this from being merged any further though because you've done a lot of work. You're very welcome to work on #10499 with the hint above.

Thanks for your contribution 🔥

[laushinka]

(Oops I should've asked you to squash. Note for next time!)

[CuriousCorrelation]

#10499 is not yet fixed, which I believe is due to this line.

Ah makes sense, thanks for the heads up!