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

fix: Broken breadcrumbs when using custom BASE_URL #861

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

ddl-rliu
Copy link
Contributor

@ddl-rliu ddl-rliu commented Apr 4, 2024

TL;DR

Typo – causes breadcrumbs to not appear in flyteconsole. To fix, use BASE_URL not BASE_PATH.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Missing breadcrumbs for custom BASE_URL like /custom-base/console

image

Expectation: breadcrumbs are present for custom BASE_URL Flyte instance.

image

This happens because when BASE_PATH is used instead of BASE_URL, the logic that generates the breadcrumbs at breadcrumbRegistry.breadcrumbBuilder ends up using the wrong BASE_URL value (this starts from makeUrlSegments). This causes it to parse the window location incorrectly, and the resulting generated breadcrumbs is missing the expected entries.

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Copy link

welcome bot commented Apr 4, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@ddl-rliu ddl-rliu changed the title Use BASE_URL [bug fix] Use BASE_URL Apr 4, 2024
Signed-off-by: ddl-rliu <[email protected]>
@@ -106,7 +106,7 @@ export class BreadcrumbRegistry {

static makeUrlSegments(location: Location, projectId = '', domainId = '') {
const pathName = location.pathname;
const basePath = process.env.BASE_PATH || '/console';
Copy link
Contributor Author

@ddl-rliu ddl-rliu Apr 4, 2024

Choose a reason for hiding this comment

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

I believe BASE_PATH may be a typo here, since the process.env object looks like

{
    NODE_ENV: "production",
    ADMIN_API: "//localhost:30080",
    ADMIN_API_URL: "localhost:30080",
    BASE_URL: "/console",
    BASE_HREF: "",
    DISABLE_CONSOLE_ROUTE_PREFIX: "",
    MAINTENANCE_MODE: ""
}

(in the case of a custom path, it looks like ... BASE_URL: "/custom-path/console"

And double checking the repo code, BASE_PATH has no other instance of usage, pointing to this being a potential typo.

@ddl-rliu ddl-rliu changed the title [bug fix] Use BASE_URL fix: Use BASE_URL not BASE_PATH Apr 4, 2024
@ddl-rliu ddl-rliu changed the title fix: Use BASE_URL not BASE_PATH fix: Broken breadcrumbs when using custom BASE_URL Apr 4, 2024
@eapolinario
Copy link
Contributor

@jsonporter , can you take a look?

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 54.76%. Comparing base (34edde1) to head (1db296c).

Files Patch % Lines
...nsole/src/components/Breadcrumbs/registry/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #861   +/-   ##
=======================================
  Coverage   54.76%   54.76%           
=======================================
  Files         578      578           
  Lines       15137    15137           
  Branches     2760     2760           
=======================================
  Hits         8290     8290           
  Misses       6847     6847           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ddl-ebrown
Copy link

Anything we can do to help get this typo fix merged @eapolinario / @jsonporter ?

@eapolinario eapolinario merged commit a24c4d3 into flyteorg:master Jun 13, 2024
11 of 12 checks passed
Copy link

welcome bot commented Jun 13, 2024

Congrats on merging your first pull request! 🎉

@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 1.15.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants