Skip to content

Conversation

@jonas-meyer
Copy link
Contributor

--- PR TEMPLATE INSTRUCTIONS (2) ---

This pull request allows the user to run the hamilton-ui on a custom subpath for a domain, e.g https://domain.com/hamilton

Changes

  • Add basename parameter to BrowserRouter
  • Expose environment variable with the following name: REACT_APP_HAMILTON_SUB_PATH
  • If environment variable is not set, set as empty string.

How I tested this

Tested manually by setting the following in the docker-compose.yml under the environment section in the frontend service:

- REACT_APP_HAMILTON_SUB_PATH=/hamilton

Notes

N/A

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 215cdf4 in 1 minute and 3 seconds

More details
  • Looked at 37 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. docs/hamilton-ui/ui.rst:121
  • Draft comment:
    Clarify that REACT_APP_HAMILTON_SUB_PATH should begin with '/' if set.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
2. ui/frontend/src/App.tsx:31
  • Draft comment:
    Consider sanitizing REACT_APP_HAMILTON_SUB_PATH to ensure it has a leading '/' (if non-empty) for BrowserRouter basename.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
3. docs/hamilton-ui/ui.rst:121
  • Draft comment:
    Consider adding a note that REACT_APP_HAMILTON_SUB_PATH must start with a '/' (e.g., '/hamilton') to ensure proper routing.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. ui/frontend/src/App.tsx:31
  • Draft comment:
    Good integration of subPath. Consider normalizing the value so that it always starts with a '/' to avoid routing issues when used as the basename in BrowserRouter.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. ui/frontend/src/App.tsx:58
  • Draft comment:
    Typographical error: In the comment on line 58, 'oseparate' should be corrected to 'separate'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_hl2qxDygutE58iH2


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@elijahbenizzy
Copy link
Contributor

This looks great/backwards compatible. Thanks! Will take another look then we can merge/release.

Copy link
Contributor

@elijahbenizzy elijahbenizzy 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! Looks good.

@elijahbenizzy elijahbenizzy merged commit 370c289 into apache:main Feb 28, 2025
23 of 24 checks passed
@legout
Copy link

legout commented Mar 24, 2025

Hi guys,

sorry for stepping in here. But I wonder how to correctly configure nginx to work properly with a subpath defined as

- REACT_APP_HAMILTON_SUB_PATH=/hamilton

Probably, this could also be included into the docs.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants