fix(revisions): queue refresh when log-batching stream is in flight#616
fix(revisions): queue refresh when log-batching stream is in flight#616cpennington wants to merge 1 commit intoidursun:mainfrom
Conversation
When a streaming refresh is already running and another refresh is triggered, queue it as a pending refresh to be fired once the stream completes, rather than dropping it. Also refactor streamer close logic to always nil the streamer field and move isLoading=false after the close block.
| if streamDone && m.pendingRefresh { | ||
| m.pendingRefresh = false | ||
| cmds = append(cmds, common.RefreshAndKeepSelections) | ||
| } |
There was a problem hiding this comment.
This is not debouncing but queueing the next refresh.
I don't see how this is helping with your situation. If a refresh is going on (say it takes 5 seconds each) and if you focus back 3 seconds later, instead of cancelling the ongoing one and saving 2 seconds, now you wait total of 10 seconds. Current implementation would settle in 8 seconds.
There was a problem hiding this comment.
The debouncing is in the change to the refresh function. However, if we only included that change, then we'd lose out on trailing updates. That said, maybe I've misunderstood the tea model, and this will still block the UI updates, as you say.
| return nil | ||
| case tea.FocusMsg: | ||
| return common.RefreshAndKeepSelections | ||
| return func() tea.Msg { return common.AutoRefreshMsg{} } |
There was a problem hiding this comment.
This change doesn't make sense to me. Why using auto refresh message here?
There was a problem hiding this comment.
The goal of this change was to take advantage of the AutoRefreshMsg behavior in revisions.go that only triggers a RefreshAndKeepSelections if the oplog has changed since the last update. Perhaps it would be better to put that check directly into the RefreshMsg handler instead?
| // Have enough rows — close the stream so the jj process doesn't linger | ||
| if m.streamer != nil { | ||
| m.streamer.Close() | ||
| m.streamer = nil | ||
| } | ||
| streamDone = true | ||
| } else { | ||
| if m.streamer != nil { | ||
| m.streamer.Close() | ||
| m.streamer = nil | ||
| } | ||
| streamDone = true | ||
| } |
There was a problem hiding this comment.
both of the branches are doing the same thing, so else branch is redundant here
There was a problem hiding this comment.
🤦 This PR in general was an off-cycle investigation w/ claude, and I clearly didn't look at the changes closely enough before submitting the PR.
|
Thanks but I don't think this PR is solving the issue you describe in your description. Debouncing simply means waiting x amount of time before starting the work, which is not happening here. This change is queueing refresh works but that wouldn't help with you issue. I would like to know how long |
cpennington
left a comment
cpennington
left a comment
There was a problem hiding this comment.
Anecdotally, this change has improved jjui responsiveness for me. I don't have hard metrics, though (I had added some debug logging to capture traces when it was behaving badly, but I don't know if I still have them). If it would be helpful, I can try to capture some traces with timing again, to help underscore what the problem I was seeing was.
| return nil | ||
| case tea.FocusMsg: | ||
| return common.RefreshAndKeepSelections | ||
| return func() tea.Msg { return common.AutoRefreshMsg{} } |
There was a problem hiding this comment.
The goal of this change was to take advantage of the AutoRefreshMsg behavior in revisions.go that only triggers a RefreshAndKeepSelections if the oplog has changed since the last update. Perhaps it would be better to put that check directly into the RefreshMsg handler instead?
| // Have enough rows — close the stream so the jj process doesn't linger | ||
| if m.streamer != nil { | ||
| m.streamer.Close() | ||
| m.streamer = nil | ||
| } | ||
| streamDone = true | ||
| } else { | ||
| if m.streamer != nil { | ||
| m.streamer.Close() | ||
| m.streamer = nil | ||
| } | ||
| streamDone = true | ||
| } |
There was a problem hiding this comment.
🤦 This PR in general was an off-cycle investigation w/ claude, and I clearly didn't look at the changes closely enough before submitting the PR.
| if streamDone && m.pendingRefresh { | ||
| m.pendingRefresh = false | ||
| cmds = append(cmds, common.RefreshAndKeepSelections) | ||
| } |
There was a problem hiding this comment.
The debouncing is in the change to the refresh function. However, if we only included that change, then we'd lose out on trailing updates. That said, maybe I've misunderstood the tea model, and this will still block the UI updates, as you say.
|
Hey, no worries.
I would like to see how long Alternatively, we can introduce a new configuration option to disable focus reporting (it is the mechanism that auto refresh when tab is gains focus) so that number of refreshes are reduced and becomes predictable. Third option is to change the default revset for this repo so that instead of loading everything, it can load only the main and your changes if that fits your workflow. Per repo config is implement but hasn't been released yet. Given that you can already compile the project, in theory, you can fine tune I also suggest that you create an issue with the actual problem and we can talk about possible solutions. |
2 participants
I've been using jjui in a large monorepo (with a relatively high number of git branches, and a lot of commits in the history), and had been getting hangs in the UI when I opened the repo or the bookmarks tab. After doing some debugging, the conclusion that the (relatively) expensive stream of commits from
jj logwas being interrupted every time switched away from and back to the tab, which made an already slow operation even slower. This change debounces the refreshes so that they don't get interrupted.