fix(ui): suppress initial loading flash and drop stale file-list responses#117
Merged
fix(ui): suppress initial loading flash and drop stale file-list responses#117
Conversation
…onses Bubble Tea delivered the first WindowSizeMsg before filesLoadedMsg, so between ready=true and the async ChangedFiles returning, View() painted the populated two-pane layout with an empty tree and "no file selected" — a 100-500 ms flash on larger repos. Gate View() on a new filesLoaded flag: show "loading..." while !ready, then "loading files..." while ready && !filesLoaded. handleFilesLoaded flips the flag first thing, including on error, so the loading screen always exits. Also tag filesLoadedMsg with a seq captured at load time and drop mismatched messages in handleFilesLoaded. toggleUntracked (and any future reload site) bumps filesLoadSeq so an older in-flight ChangedFiles can't overwrite the tree after the user toggles state.
There was a problem hiding this comment.
Pull request overview
This PR adjusts the UI’s file-list loading state machine to prevent an initial “empty two-pane” flash during startup and to ensure late/stale file-list responses can’t overwrite newer state (e.g., after toggling untracked visibility).
Changes:
- Add a
filesLoadedview gate soView()renders a loading string until the first acceptedfilesLoadedMsgis handled. - Add
filesLoadSeq/filesLoadedMsg.seqto drop stale file-list responses and bump the seq ontoggleUntracked. - Update tests (and architecture docs) to lock in the new loading/seq behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/ARCHITECTURE.md | Documents the two-stage View() gating and the file-load lifecycle. |
| app/ui/view.go | Adds a filesLoaded gate to suppress rendering the empty two-pane layout before file data arrives. |
| app/ui/model.go | Introduces filesLoaded and filesLoadSeq, and tags filesLoadedMsg with seq. |
| app/ui/loaders.go | Captures filesLoadSeq in loadFiles() and drops stale filesLoadedMsg in handleFilesLoaded. |
| app/ui/model_test.go | Adds regression test for the startup flash and updates testModel() helper for the new state. |
| app/ui/loaders_test.go | Adds tests for stale-response dropping, seq bumping, and ensures error path exits loading state. |
| app/ui/diffview_test.go | Updates setup to bypass the new loading gate and exercise the real render path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+261
to
+263
| `m.ready && !m.filesLoaded` (after resize, before `filesLoadedMsg`). `filesLoaded` | ||
| flips to true on every `filesLoadedMsg`, including the error path, so the loading | ||
| screen always exits. |
There was a problem hiding this comment.
This paragraph says filesLoaded “flips to true on every filesLoadedMsg”, but stale responses are intentionally dropped when msg.seq != m.filesLoadSeq, so not every filesLoadedMsg flips the flag. Suggest rewording to “on every accepted/latest filesLoadedMsg” (still including the error path) to reflect the seq-based drop logic.
Suggested change
| `m.ready && !m.filesLoaded` (after resize, before `filesLoadedMsg`). `filesLoaded` | |
| flips to true on every `filesLoadedMsg`, including the error path, so the loading | |
| screen always exits. | |
| `m.ready && !m.filesLoaded` (after resize, before an accepted/latest | |
| `filesLoadedMsg`). `filesLoaded` flips to true on every accepted/latest | |
| `filesLoadedMsg`, including the error path, so the loading screen always exits. |
| Model.Init() → loadFiles cmd | ||
| → Renderer.ChangedFiles() → filesLoadedMsg | ||
| → tree.Rebuild(entries) | ||
| → tree.Rebuild(entries) + m.filesLoaded = true |
There was a problem hiding this comment.
ARCH diagram line implies m.filesLoaded is set as part of tree.Rebuild(entries), but in code it’s set at the start of handleFilesLoaded (and also on the error / early-return paths). Consider splitting this into its own step (e.g., handleFilesLoaded: m.filesLoaded = true) so the diagram matches the actual state machine.
Suggested change
| → tree.Rebuild(entries) + m.filesLoaded = true | |
| → handleFilesLoaded: m.filesLoaded = true | |
| → tree.Rebuild(entries) |
Address Copilot review feedback on PR #117. The previous wording said filesLoaded flips "on every filesLoadedMsg", which is inaccurate after the seq-check was added — stale messages are dropped and do not flip the flag. The diagram also coupled tree.Rebuild with the flag flip, but the flag is set before the error check while Rebuild only runs on non-error accepted messages.
Deploying revdiff with
|
| Latest commit: |
ee25d27
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bf66ef16.revdiff.pages.dev |
| Branch Preview URL: | https://fix-initial-loading-flash.revdiff.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Opening revdiff on a repo with changes briefly shows an empty two-pane layout with "no file selected" before the real content appears. On small repos it's unnoticeable, on larger ones it's a 100-500 ms flash.
Cause:
Model.Init()returnsloadFiles()as an asynctea.Cmd. Bubble Tea delivers the firstWindowSizeMsgwell beforefilesLoadedMsg, so for the gap betweenready=trueand the goroutine returning,View()paints the two-pane layout against a nil-entry tree and emptym.file.name.Fix
Two related changes to the file-list loading state machine.
1. Suppress the empty frame. Add a
filesLoadedflag onModel.View()returns"loading..."while!ready, then"loading files..."whileready && !filesLoaded, then the populated layout.handleFilesLoadedflips the flag first thing, including on the error path, so the loading screen always exits.2. Drop stale file-list responses. Tag
filesLoadedMsgwith aseq uint64captured at load time (same patternfileLoadedMsg.seqalready uses).handleFilesLoadeddrops messages whose seq doesn't matchm.filesLoadSeq.toggleUntrackedbumpsfilesLoadSeqbefore issuing a new load, so if the user hitsuwhile an older load is in flight, the late response can't overwrite the tree with stale data that contradictsm.modes.showUntracked.Tests
TestModel_InitialLoadingState_NoEmptyFlashdrives the realready=false → WindowSizeMsg → filesLoadedMsgsequence and asserts the intermediateView()is"loading files...", not the empty two-pane layout.TestModel_FilesLoaded_DropsStaleResponsesverifies a stale seq response is dropped (doesn't flipfilesLoaded, doesn't populate the tree) while a fresh seq response is accepted.TestModel_ToggleUntrackedBumpsFilesLoadSeqlocks in the bump invariant.TestModel_FilesLoadedErrornow assertsfilesLoaded=trueon the error path, so a future refactor can't leave the loading screen pinned forever.testModel()helper andTestModel_PlainStylesupdated to also setfilesLoaded = trueso they exercise the render path rather than the loading string.docs/ARCHITECTURE.mdFile Loading section updated with the two-stage View gating.