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(api): allow ui refresh to deep links #1878

Merged
merged 2 commits into from
Apr 20, 2024
Merged

Conversation

zkhvan
Copy link
Contributor

@zkhvan zkhvan commented Apr 19, 2024

After #1833, installing the chart into a cluster will cause links to nested paths to return 404.

Reproduction

  • Install 0.6.0-unstable-20240419
  • Navigate to Kargo Dashboard
  • Create new project (kargo-demo)
  • Navigate to new project (https://host/project/kargo-demo)
  • Refresh and you'll see:
    404 page not found

Since assets are served through the API server, the router needs to redirect the URLs to index.html since the UI is a single page application.

@krancour I figured you'd have context on this, so tagging you. I'm not sure if this is something you've encountered and were thinking of an alternative solution.

Signed-off-by: Zhenya Khvan <zhenya.khvan@gmail.com>
@zkhvan zkhvan requested a review from a team as a code owner April 19, 2024 20:45
Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit eebf8a5
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66231830f2610f0008df84e0
😎 Deploy Preview https://deploy-preview-1878.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@krancour
Copy link
Member

Nice find. I'll review shortly!

@krancour
Copy link
Member

krancour commented Apr 19, 2024

A note about how this escaped my attention...

When we run Kargo from source with tilt up, the UI isn't served from the API server like it normally would be, because front-end development is faster if the UI is served from its own container running vite, which can hot-reload UI changes...

The embedded-ness of the UI was tested by running the kargo server command. I could not progress past the login page because the local server starts with no authn methods enabled and the UI doesn't support that yet. (#1728 is still WIP.)

So... being unable to progress past the login page, I never actually did see anything deeper than the login page served from the embedded FS. Especially since assets loaded, it didn't occur to me that anything was amiss.

Installing the unstable chart does indeed permit me to easily reproduce the problem.

internal/api/server.go Outdated Show resolved Hide resolved
internal/api/server.go Outdated Show resolved Hide resolved
@krancour
Copy link
Member

@zkhvan I proposed minor tweaks for readability and if they're cool with you then this PR lgtm.

Co-authored-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Zhenya Khvan <zhenya.khvan@gmail.com>
Copy link

codecov bot commented Apr 20, 2024

Codecov Report

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

Project coverage is 46.26%. Comparing base (fe958c8) to head (eebf8a5).
Report is 2 commits behind head on main.

Files Patch % Lines
internal/api/server.go 0.00% 39 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1878      +/-   ##
==========================================
- Coverage   46.38%   46.26%   -0.12%     
==========================================
  Files         215      215              
  Lines       14066    14101      +35     
==========================================
  Hits         6524     6524              
- Misses       7243     7278      +35     
  Partials      299      299              

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

@krancour krancour added this pull request to the merge queue Apr 20, 2024
Merged via the queue into akuity:main with commit 0ef2f50 Apr 20, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants