Skip to content

Prevent flashing during async message transitions#682

Closed
dv336699 wants to merge 2 commits into
axllent:developfrom
dv336699:fix/loading-flash
Closed

Prevent flashing during async message transitions#682
dv336699 wants to merge 2 commits into
axllent:developfrom
dv336699:fix/loading-flash

Conversation

@dv336699

@dv336699 dv336699 commented May 7, 2026

Copy link
Copy Markdown

Summary

  • replace the full-screen loading overlay with a fixed top loading bar
  • keep the message view mounted while loading the next message
  • stop remounting the HTML preview subtree on message-to-message navigation

Testing

  • npm run lint
  • npm run package
  • rebuilt the Docker image and verified the local container was healthy on :8025

Replace the full-screen loader overlay with a fixed top progress bar and keep the message detail view mounted while loading the next message. This preserves the existing loading and routing patterns while avoiding viewport flashes and iframe teardown in the HTML preview tab.

Constraint: Keep the UI fix minimal and aligned with existing Vue component patterns
Rejected: Add skeleton-loading components across the message view | broader UI churn than needed for the bug
Rejected: Introduce a new global loading store | unnecessary abstraction for a shared component styling fix
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Preserve the mounted message preview across route-driven message changes unless you also redesign tab and iframe state management
Tested: npm run lint; npm run package; rebuilt Docker image; verified container health and HTTP 200 on 127.0.0.1:8025
Not-tested: Cross-browser visual behavior outside the local Docker-backed manual flow
@axllent

axllent commented May 7, 2026

Copy link
Copy Markdown
Owner

Thanks @dv336699. I'm not seeing the loader - probably because it's the same colour as the navbar - was this intentional? Additionally (and I don't know if this is possible via CSS), it would be better if the loading overlay only started after maybe 200ms - otherwise there will always be a "flicker" or it appearing even with a very quick response.

Would you mind please looking into this for me and adjusting if possible? Thank you.

@dv336699

dv336699 commented May 7, 2026

Copy link
Copy Markdown
Author

hi @axllent sorry about that i ended leaving as $primary my mistake, i have patched the HTML check tab as well to prevent it from moving the other tab

to test the loading you might need to set the network to 3G, let me know if the loading animation looks good, happy to change it.

Screenshot 2026-05-07 at 22 22 56

thanks!

@axllent

axllent commented May 8, 2026

Copy link
Copy Markdown
Owner

Thanks. I've done some basic testing and found a few implementation issues:

  1. HTML source highlighting no longer works.
  2. The currently selected tab doesn't reset when opening a new message (this may be intentional).
  3. If I'm on the HTML preview tab and then click a plain-text message (which has no HTML), I get a blank screen with no tab selected.

These can be refactored and fixed of course, but I wanted to ask: was this functionality vibe-coded (i.e. written with AI)? It feels somewhat over-engineered and doesn't appear to have been visually tested.

@dv336699

dv336699 commented May 8, 2026

Copy link
Copy Markdown
Author

hi @axllent

thanks for getting back to me.

[sorry for the wall of text in advance, just wanted to make it clear re the points raised]

1 and 2. sorry, i will be honest and say i didnt check every tab and this is likely a side effect of keeping the same tab whilst moving from one email to another - what i did check was that as i navigate over the emails and the tab would stay in the same tab, e.g. if i'm on html check tab and wish to compare a few emails, it's nice to stay in the same tab instead of having to do an extra click from the current behaviour that reset to HTML tab

keeping the same tab whilst moving emails: yes intentional
breaking the html source format: not, again side effect likely caused by removing the :key

  1. it does get highlighted on first email, but not when moving from one email to another whilst the tab HTML stays in place, again apologies for not testing it properly

as for your concerns:

  • was this functionality vibe-coded: i did used Codex 5.4, but the syntax was checked and the available tests run (see below) and having been working with vue for many years i didn't find anything wrong with the syntax and all the tests didn't return any issues

before pushing the code i checked what was available using bun run

Screenshot 2026-05-08 083432

and the only one i run was bun run lint which does not catch the HTML source highlight regression...

so to clarify, was it vide-coded? i wouldn't call it that, assisted with AI, yes it was

  • doesn't appear to have been visually tested
    it was visually tested to the extent that would satisfy what i did, but not every tab and every button and every interaction - which is what most devs do when an app grows to a certain extent you can't keep testing every unrelated screen - i would normally use playwright or cypress to work on visual regressions but doesn't seem to be available in this repository

please find a visual test i carried out using both versions - the first version in the video is the current version and the second version is the one i updated - I'm on macbook m4 using Brave, so i think mailpit was too quick to load that the loading was causing the "flash effect" with loading

https://youtu.be/HaR_YDpjRcQ

i hope this clarifies your concerns and happy to continue working on this PR until you're happy that it aligns with the project or be closed - that won't stop me from making future contributions

many thanks!

@axllent

axllent commented May 8, 2026

Copy link
Copy Markdown
Owner

Thanks for the comprehensive feedback. It appears I may have offended you, this was not my intention. I am simply trying to "get a feel" for when things are vibe coded. Don't get me wrong, I'm not against AI-assisted development (in fact I have started doing this myself recently), but I have noted a surge in PRs which are completely vibe coded. Unfortunately the majority of those create more work than they solve, and unnecessarily over-complicate things at times. In some instances the "authors" aren't even programmers at all, and whilst they introduce a feature they also break a number of other things.

Anyway, moving on. You can leave this with me as I need to evaluate what you have done, and see if things can be done more simply (or alternatively what I may need to refactor). I see what you are trying to achieve here, I'm just not convinced (yet) it's the right approach.

@dv336699

dv336699 commented May 8, 2026

Copy link
Copy Markdown
Author

hi @axllent no offence taken, i appreciate the points raised and they are all valid, the reason the text was more extended was simply to make sure my points did get across and make sure it was a human being on the other side instead of another AI generated text.

please do feel free to close the PR if it doesn't align or you would rather keep the loading as is - it's really not a problem, honestly.

again i want to mention that this won't stop me from contributing to the project!

thanks!

@D-i-t-gh

D-i-t-gh commented May 8, 2026

Copy link
Copy Markdown

+1

This is definitely a lot easier on the eyes!

@axllent

axllent commented May 8, 2026

Copy link
Copy Markdown
Owner

I took a closer look at this to figure out how it could be simplified and properly fixed.

Reloading the MessageItem is partly necessary due to other related complications, namely the message tags functionality, the Bootstrap tabs, some manipulation of text links etc - it's genuinely easier to re-render that section of the page fresh rather than try to update the data dynamically. That said, this isn't actually what causes the "flash" your PR addresses. The real culprit is the (unnecessary) reset of the message data immediately before the AJAX call, which your PR correctly fixes.

Simply removing this.message = false in MessageView just before the AJAX call appears to be all that's required to prevent the intermittent blank screen in MessageItem while the next message loads. This preserves all existing (working) functionality and gives a smoother transition between messages.

The other significant change in your PR is replacing the AJAX loading animation - swapping the full-screen overlay with spinner for a horizontal bar at the top of the screen. The original reasoning behind the overlay was twofold: it prevents double-clicks (or, on slow connections, parallel HTTP requests that can produce unexpected behaviour when they start to resolve), and it makes it unmistakably clear that the app is busy and the user needs to wait - useful for things like pagination and search. That said, I'm open to hearing the case for why the less intrusive horizontal bar would be the better approach.

I'd appreciate any feedback from both of you (@dv336699 & @D-i-t-gh) as to whether you prefer the horizontal loader instead of the full screen one, and if so, why you think that is a better solution.

@D-i-t-gh

D-i-t-gh commented May 9, 2026

Copy link
Copy Markdown

These are just my thoughts, but written by AI. Generally I'm not a fan of artificial delays.

I have an alternative proposal that might give the best of both worlds:

Keep the overlay for click-blocking purposes only, but:

  • Remove the dark background (make it transparent)
  • Replace the spinner with the horizontal loader from this PR

Why this helps:

  • Prevents double-clicks/parallel requests
  • Much easier on the eyes than dark overlay (the original goal)
  • The horizontal loader remains clearly visible since there's no darkening to wash it out

Additional benefit: This approach likely eliminates the need for a 200ms delay. The original delay was needed because a full-screen dark overlay flashing on/off for fast responses is highly jarring. With no darkening and a subtle horizontal bar, that problem disappears - even if the loader appears for only 50ms, it's barely noticeable. This simplifies the implementation further.

Code impact: Actually less than removing the overlay entirely - mostly CSS changes to the overlay background plus keeping the PR's loader.

@axllent

axllent commented May 9, 2026

Copy link
Copy Markdown
Owner

Thanks for the feedback @D-i-t-gh . Just to clarify - the delay was not actually a delay fetching the data, but rather a delay before starting to show the loading animation (it prevents the brief "flicker" of the loading bar appearing on requests that take < 200ms).

As I mentioned earlier, the main point of the fullscreen overlay was to prevent user trying to click on multiple links while the application is loading. If the overlay is transparent then the user won't know why the mouse isn't working. The best solution I can think of here is to use a transparent overlay with cursor: wait; - this provides the user with a visual indicator that something is loading (horizontal loader) and that the cursor is "busy" loading something
(see video - note that I artificially added a delay to the API here to illustrate the loading screen).

I'm still curious to hear what @dv336699 has to say though.

@axllent

axllent commented May 14, 2026

Copy link
Copy Markdown
Owner

I have now included my change (based on this) in the latest release (v1.30.0). Hopefully no more (or at least less) "flashing" :) I'll close this PR now.

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