Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(frontend): implement project-based routing #13474

Merged
merged 52 commits into from
Jan 15, 2024
Merged

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Dec 23, 2022

Problem

Alternative to #13453

Changes

  • Update all frontend routes to use /project/:id in front of the URL
  • Make sure they all old routes keep working
  • Fix the tests that are failing because getCurrentTeamId() is not mocked
  • Make a backend middleware that switches the project if needed
  • Add tests
  • Fixes the project switcher to simply use the url based switching
  • Link detects urls in other projects and forces a full reload
  • Keep the most of the path when changing project

TODO

  • Fix issue if you go to a team that doesn't exist / you don't have access to
  • Indicate in the frontend if you are on a project url that you don't have access to

Out of scope:

  • Create a short ID to use instead of the real ID to prevent increment-based attacks. --> should we?
  • Deprecate current_team and always use the team ID in URLs --> out of scope? what's missing?

How did you test this code?

@mariusandra mariusandra changed the title feat routs feat(frontend): implement project-based routing Dec 23, 2022
@mariusandra
Copy link
Collaborator Author

@benjackwhite I added the changes into kea-router, and a todo list to the top of the PR.

@benjackwhite
Copy link
Contributor

@benjackwhite I added the changes into kea-router, and a todo list to the top of the PR.

Super cool. If you haven't done it by the new year I will be happy to take on that TODO list when I'm back 😉

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Size Change: +667 B (0%)

Total Size: 2 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 2 MB +667 B (0%)

compressed-size-action

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Size Change: +5.98 kB (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB +5.98 kB (0%)

compressed-size-action

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Size Change: +5.98 kB (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB +5.98 kB (0%)

compressed-size-action

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Size Change: +5.98 kB (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB +5.98 kB (0%)

compressed-size-action

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Size Change: +5.98 kB (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB +5.98 kB (0%)

compressed-size-action

@Twixes Twixes added the waiting Prevents stale-bot from marking the PR as stale. label Jan 4, 2024
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only remaining issue that I can see is the choice of project id vs short_id

Hard to decide if this is worth doing now. If we do it later we would need to back support converting an ID to a short_id (probably not too hard, just another part of the AutoProjectMiddleware's job - if it is a number, look it up and redirect to the short_id version).

The tricky bit will be the conversion throughout the project to use short_id. If we only do it for the URL, then the question is have we actually gained anything given you can easily find out your own projectId via the api or settings.

If we fully convert to it, then we have a lot of complexity in the code as we will be constantly swapping from the short_id version to the real ID given it is used as the reference pretty much all of the time.

We could do it just for the URL now which at least makes it way less obvious.
@mariusandra you wrote the original PR mentioning increment-based attacks. Any thoughts?

@mariusandra
Copy link
Collaborator Author

mariusandra commented Jan 5, 2024

I personally think it's fine.

Wore case: You can learn exactly how many teams we have and our growth by periodically making new teams and calculating the delta in between.

Counter: Were transparent enough that we'll probably tell you if you ask or read James's tweets 🤷

Plus, indeed, hiding it completely is going to be a huge undertaking, and even then you can use other proxies (e.g. feature flag id-s, etc)

I played around and it seems to work. I'm happy to just merge it in, and if there are any errors that we missed, fix them as they come up.

@Twixes
Copy link
Member

Twixes commented Jan 5, 2024

I really like this, though I'm still gonna advocate for using organization.slug (which already is a thing) instead of /project. This way we'll water two plants with one hose. It looks like around 20% of users with multiple projects are also in multiple orgs. It's not a massive number (insight), but if we're doing this, I think it's worth a bit of extra effort.

@posthog-contributions-bot
Copy link
Contributor

This issue has 2009 words at 48 comments. Issues this long are hard to read or contribute to, and tend to take very long to reach a conclusion. Instead, why not:

  1. Write some code and submit a pull request! Code wins arguments
  2. Have a sync meeting to reach a conclusion
  3. Create a Request for Comments and submit a PR with it to the meta repo or product internal repo

Is this issue intended to be sprawling? Consider adding label epic or sprint to indicate this.

@benjackwhite
Copy link
Contributor

I really like this, though I'm still gonna advocate for using organization.slug (which already is a thing) instead of /project. This way we'll water two plants with one hose. It looks like around 20% of users with multiple projects are also in multiple orgs. It's not a massive number (insight), but if we're doing this, I think it's worth a bit of extra effort.

The question I guess is why would we need it? You have to be in the org to access the team. The downside is that we then have an even longer URL and have to handle cases such as the org id being different to the team ID.

Essentially the team "slug" (id) is both org and team. If you click a link to a project in a different org you are swapped to that team and organization so we have all the functionality without any additional complexity.

Copy link
Contributor

github-actions bot commented Jan 8, 2024

Size Change: +5.96 kB (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB +5.96 kB (0%)

compressed-size-action

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/organization/billing would benefit from this directly, but it's true that otherwise auto-switching would not be aided by use of the organization slug instead of project. I think the actual benefit is in the URLs being more obvious. With just the project ID in there, we achieve auto-switching, which is the most important thing – but the project ID is totally opaque, so the URL isn't actually more readable. This article: "Examples of Great URL Design", is an interesting read on the topic of making URLs a good experience in themselves.
But there are two problems to be solved: 1. what you mentioned about org slug and project ID mismatches 2. potential collisions of slugs with scenes from the legacy system.
So let's just ship this now.

Copy link
Contributor

Size Change: +667 B (0%)

Total Size: 2 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 2 MB +667 B (0%)

compressed-size-action

@benjackwhite benjackwhite merged commit 8ccdde7 into master Jan 15, 2024
100 checks passed
@benjackwhite benjackwhite deleted the kea-router-projects branch January 15, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Prevents stale-bot from marking the PR as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants