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

BrowseDashboards: Add subpath to URLs on Browse Dashboards page #84992

Merged

Conversation

butkovv
Copy link
Contributor

@butkovv butkovv commented Mar 22, 2024

Fixes #74275

@butkovv butkovv requested a review from a team as a code owner March 22, 2024 12:08
@butkovv butkovv requested review from Clarity-89 and ashharrison90 and removed request for a team March 22, 2024 12:08
@CLAassistant
Copy link

CLAassistant commented Mar 22, 2024

CLA assistant check
All committers have signed the CLA.

@grafana-pr-automation grafana-pr-automation bot added area/frontend pr/external This PR is from external contributor labels Mar 22, 2024
@joshhunt joshhunt self-assigned this Mar 22, 2024
@joshhunt joshhunt added this to the 11.0.x milestone Mar 22, 2024
@joshhunt
Copy link
Contributor

Hi @butkovv - I believe that this problem is there (unfortunately it's fairly common - we need a better way to solve this), but I'm having trouble reproducing and would appreciate some tips for reproducing:

  • Are you able to tell me which link doesn't work correctly with the sub path?
  • Which Grafana version do you get this one?
  • Do you have the nestedFolders or newBrowseDashboards feature toggles enabled in your config.ini? (you can check the grafanaBootData.settings.featureToggles object in the browser JS console)

A screen recording would help. I've tried to follow reproduction in #74275 but wasn't able to reproduce

@joshhunt
Copy link
Contributor

Hmm. I can reproduce this in 10.1.5, but not in 10.2.0. We've made a lot of improvements to how folders and breadcrumbs work during this period, so it's hard to know what exactly fixed it.

@butkovv
Copy link
Contributor Author

butkovv commented Mar 22, 2024

Hi @butkovv - I believe that this problem is there (unfortunately it's fairly common - we need a better way to solve this), but I'm having trouble reproducing and would appreciate some tips for reproducing:

  • Are you able to tell me which link doesn't work correctly with the sub path?
  • Which Grafana version do you get this one?
  • Do you have the nestedFolders or newBrowseDashboards feature toggles enabled in your config.ini? (you can check the grafanaBootData.settings.featureToggles object in the browser JS console)

A screen recording would help. I've tried to follow reproduction in #74275 but wasn't able to reproduce

Yeah, sorry about that, kinda mislead by mentioning that issue because I thought it wasn't also fixed yet. There's another issue related to when you open a dashboard that is in nested directory while serving Grafana from subpath. See the attached video for reproduction on Grafana v11.0.0-pre.

Screen.recording.2024-03-22.17.42.08.mov

@joshhunt
Copy link
Contributor

Ahhh. I think the issue here is actually because you're serving it from the subpath of dashboards, which conflicts with the /dashboards/... route that the folder views are served from.

See how I get /grafana-subpath/dashboards/f/... here:

7418_2024-03-26-12-14_firefox

But the leading subpath is missing here:

7419_2024-03-26-12-14_firefox

I've been talking with @ashharrison90 about this, and I think we might want to solve this in another way. I'll leave this PR open in the meantime, but I will investigate another approach.

@ashharrison90 ashharrison90 modified the milestones: 11.0.x, 11.1.x Mar 26, 2024
@joshhunt
Copy link
Contributor

Okay - spent some time digging into this, and I think merging this one is the best approach so far. I'll give this a proper test tomorrow with the aim of merging it for Grafana 11 👍

@joshhunt joshhunt added the backport v11.0.x Mark PR for automatic backport to v11.0.x label Mar 27, 2024
@joshhunt joshhunt modified the milestones: 11.1.x, 11.0.x Mar 27, 2024
Copy link
Contributor

This PR must be merged before a backport PR will be created.

@joshhunt joshhunt force-pushed the dashboards/fix-urls-with-enabled-subpath branch from a4f5927 to e66b4f8 Compare March 28, 2024 12:09
Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

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

Thank you for your patience on this 👍

For context:

  • we often have this issue as most of us don't develop, or use grafana, with the subpath option
  • we've been talking about more automatic subprefixing for URLs, but turns out we already have then (when using react-router-dom Links with basename, which this does with)
  • BUT, it appears there's a bug or incomaptibility with react-router where it doesn't prefix the subpath if your subpath is the same as a route (such as your dashboards)
  • SO, the easiest thing to do here is just to prefix the subpath manually here, like they are in search results and dashboards

I also pushed up a minor change to use the already imported config object from @grafana/runtime - they're both the same object. And added a very brief comment explaining this.

Again, thanks for your patience and contribution 👍

@joshhunt joshhunt enabled auto-merge (squash) March 28, 2024 12:58
@joshhunt joshhunt merged commit b039995 into grafana:main Mar 28, 2024
12 checks passed
grafana-delivery-bot bot pushed a commit that referenced this pull request Mar 28, 2024
* BrowseDashboards: Add subpath to URLs on Browse Dashboards page

* comment

* use existing config

* comment

---------

Co-authored-by: joshhunt <josh@trtr.co>
(cherry picked from commit b039995)
joshhunt pushed a commit that referenced this pull request Apr 3, 2024
…page (#85354)

BrowseDashboards: Add subpath to URLs on Browse Dashboards page (#84992)

* BrowseDashboards: Add subpath to URLs on Browse Dashboards page

* comment

* use existing config

* comment

---------

Co-authored-by: joshhunt <josh@trtr.co>
(cherry picked from commit b039995)

Co-authored-by: butkovv <52626407+butkovv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend backport v11.0.x Mark PR for automatic backport to v11.0.x pr/external This PR is from external contributor type/bug
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

Breadcrumb: Parent folder link does not work when grafana is served from sub path /dashboard
5 participants