Simplify node setup ui#215
Conversation
Signed-off-by: Pavlenex <36959754+pavlenex@users.noreply.github.com>
pavlenex
requested review from
GitGab19 and
lucasbalieiro
and removed request for
lucasbalieiro
GitGab19
left a comment
GitGab19
left a comment
There was a problem hiding this comment.
A minor patch suggested by my clanker.
Co-Authored-By: Gabriele Vernetti <62447440+GitGab19@users.noreply.github.com>
|
@GitGab19 did you test it locally. were you able to get past this step? |
|
@GitGab19 nvm it's unrelated to this PR. It's been a while since I tried running it locally, so perhaps we changed something there and automatic setup is not expected to work locally? cc @lucasbalieiro |
|
Yes, I was able to proceed without any issues. Are you running your node on the same machine? |
|
I am, I replicated same issue on master, so it doesn't seem it's related to this PR. But something I have to look into, could be Docker issue.. |
about #215 (comment) It should be working when running with I'll check it on main after reviewing this PR. |
| if (browserPlatform.includes('linux')) return 'linux'; | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
I would suggest not relying on this, as it could introduce misleading behavior.
navigator.platform is a legacy API with inconsistent behavior. Different browsers running on the same operating system may report different values, especially since some browsers intentionally reduce fingerprinting by exposing less accurate platform information.
When I introduced the HOST_OS environment variable, its purpose was to assist the Umbrel setup process. It is intended to be set on the server running the application, which does not necessarily mean that the browser is running on the same machine.
Take Umbrel as an example: the server runs on Linux, but I may access the UI from a browser running on macOS (or an iPad, android etc). In that case, deriving HOST_OS from the browser would result in the wrong platform being detected.
This environment variable is primarily intended for cases such as local testing:
export HOST_OS=Linux && npm run devIt is also set through the Umbrel app's Docker Compose configuration:
https://github.com/getumbrel/umbrel-apps/blob/61c7b4fc3e96103b5029f554fb0039469b591eb7/sv2-ui/docker-compose.yml#L50
Another source of truth for this env is the node auto-discovery process, where we can make an educated guess about the host system based on the RPC information we receive.
It could also be useful in installation examples on the website, allowing users to explicitly configure it in the startup command, for example:
docker run --rm \
--name sv2-ui \
-p 8080:8080 \
-e HOST_HOME=$HOME \
-e HOST_OS=Linux \
-v /var/run/docker.sock:/var/run/docker.sock \
-v sv2-config:/app/data/config \
stratumv2/sv2-ui:latest| const normalized = hostOs.toLowerCase(); | ||
| if (normalized === 'linux') return 'linux'; | ||
| if (normalized === 'macos') return 'macos'; | ||
| if (normalized === 'macos' || normalized === 'darwin' || normalized === 'mac') return 'macos'; |
There was a problem hiding this comment.
I guess this is being introduced because of the detection of the OS based on the navigator.platform.
If my argumentation on https://github.com/stratum-mining/sv2-ui/pull/215/changes#r3589708089 made some sense, this should be dropped as well
Signed-off-by: Pavlenex <36959754+pavlenex@users.noreply.github.com>
|
Thanks for review @lucasbalieiro pushed 9672013 to address the concern. Overall if you feel the rest of the changes don't fit well are or too much, I'm happy to leave the whole bottom part as it was and just address design changes, up to you, let me know however you prefer us to proceed. |
3 participants
This PR simplifies the node setup flow by reducing duplicated copy, consolidating the instructions into a clearer three-step layout, and showing only one relevant readiness status at a time. It also reuses the existing Bitcoin Network selector across both setup screens, improves the manual configuration fallback, and fixes host OS detection so macOS/Linux settings and socket paths are prefilled correctly.
Before PR
After PR
closes #190