Fix: host mode does not indicate 404s well #3878
Conversation
Preview deployments |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dba330e2b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @action | ||
| loading(transition: Transition) { | ||
| transition.finally(() => { | ||
| // The loading template will be shown only during the initial load of the app | ||
| this.initialLoading = false; | ||
| }); | ||
|
|
||
| return this.initialLoading; |
There was a problem hiding this comment.
The initialLoading flag is only flipped to false from the loading action, which only runs when a transition actually enters the loading substate. If the initial app boot completes quickly (no loading substate), initialLoading never changes and later slow transitions will return true here, showing the global loading template even though the app is already running. That contradicts the intent in the comment and can reintroduce the “spinner covering errors” problem on later slow transitions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where error messages (specifically "card not found") were hidden behind a persistent loading spinner element in host mode. The solution leverages Ember's route substate pattern by introducing a global loading template and removing the static #host-loading element from index.html.
Changes:
- Introduced a new
loading.gtstemplate that displays a loading spinner using Ember's route loading substates - Added a
loadingaction to the index route that ensures the loading template only appears during the initial app load - Removed the static
#host-loadingelement and its associated styles from index.html - Enhanced the test for non-existent cards to verify that the loading indicator appears and disappears correctly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/host/app/templates/loading.gts | New loading template that replaces the static HTML loading indicator with a dynamic Ember template |
| packages/host/app/routes/index.gts | Implements the loading action to control when the loading template is shown |
| packages/host/app/index.html | Removes the static loading HTML and CSS that was previously blocking error messages |
| packages/host/tests/acceptance/host-mode-test.gts | Updates the test to verify loading indicator behavior and proper error display |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class='loading-indicator'> | ||
| <LoadingIndicator @color='#00FFBA' /> | ||
| </div> | ||
| <div class='loading-text'>Loading...</div> |
There was a problem hiding this comment.
The loading text uses three periods (...) instead of the Unicode ellipsis character (…) that was used in the original index.html. For consistency with the original implementation and better typography, consider using the Unicode ellipsis character.
| <div class='loading-text'>Loading...</div> | |
| <div class='loading-text'>Loading…</div> |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
Thanks for this PR--this issue was driving me nuts. this loader is also visible in the prerendered output which makes debugging really annoying.
This PR fixes an issue where the “card not found” error is not displayed because the
#host-loadingelement is never removed, causing the error to be covered by the loading spinner. The operator mode and host mode components work correctly because they are rendered above the#host-loadingelement. However, keeping#host-loadingin the DOM indefinitely is unexpected.In this PR, I leverage Ember route substates, introduce a global
loadingtemplate, and remove the#host-loadingelement fromindex.html. As a result, the loading spinner only appears during the initial loading of the Boxel app.