From cab38f153ec4e02ae53d20cc456e88439a41dcd7 Mon Sep 17 00:00:00 2001 From: David Yonge-Mallo Date: Fri, 10 Jul 2026 09:11:47 +0200 Subject: [PATCH] refactor(view): swap `StandardView.cur_layout` atomically Publishing the incoming layout to `self.cur_layout` before `create` resolved let `update_files_impl`'s debounced `ensure_layout` trip `Layout:recover` mid-swap, leaving a stray `diffview://null` split next to the freshly opened diff after staging a resolved merge conflict. Stage the new layout via `_stage_layout` and assign `self.cur_layout` only after `create` returns. --- lua/diffview/scene/layout.lua | 6 +- .../scene/views/standard/standard_view.lua | 71 ++++++++++---- .../restore_focus_after_layout_swap_spec.lua | 96 ++++++++++++++++++- 3 files changed, 155 insertions(+), 18 deletions(-) diff --git a/lua/diffview/scene/layout.lua b/lua/diffview/scene/layout.lua index 52b26139..a39e611e 100644 --- a/lua/diffview/scene/layout.lua +++ b/lua/diffview/scene/layout.lua @@ -370,7 +370,11 @@ function Layout:is_nulled() return true end ----Validate the layout and recover if necessary. +---Validate the layout and recover if necessary. Recover is only ever +---triggered against the `StandardView.cur_layout`, and `use_entry` +---publishes a new layout to `cur_layout` only AFTER its `create` future +---has fully resolved (and the old layout has been destroyed), so a +---call to `ensure` cannot land on a layout that is still mid-swap. function Layout:ensure() local state = self:validate() diff --git a/lua/diffview/scene/views/standard/standard_view.lua b/lua/diffview/scene/views/standard/standard_view.lua index 86bd0c2f..e969b0d4 100644 --- a/lua/diffview/scene/views/standard/standard_view.lua +++ b/lua/diffview/scene/views/standard/standard_view.lua @@ -13,7 +13,7 @@ local oop = lazy.require("diffview.oop") ---@module "diffview.oop" local utils = lazy.require("diffview.utils") ---@module "diffview.utils" local api = vim.api -local await = async.await +local await, pawait = async.await, async.pawait local M = {} @@ -200,12 +200,22 @@ function StandardView:ensure_layout() end end +---Clone `layout`, cache the clone in `self.layouts`, and attach a +---`pivot_producer` to it — without touching `self.cur_layout`. Reuses +---an existing cache entry for the same class so pinned variants keep +---their file identities across swaps. ---@param layout Layout -function StandardView:use_layout(layout) - self.cur_layout = layout:clone() - self.layouts[layout.class] = self.cur_layout +---@return Layout +function StandardView:_stage_layout(layout) + local staged = self.layouts[layout.class] + if staged then + return staged + end + + staged = layout:clone() + self.layouts[layout.class] = staged - self.cur_layout.pivot_producer = function() + staged.pivot_producer = function() local was_open = self.panel:is_open() local was_only_win = was_open and #utils.tabpage_list_normal_wins(self.tabpage) == 1 self.panel:close() @@ -224,6 +234,13 @@ function StandardView:use_layout(layout) return pivot end + + return staged +end + +---@param layout Layout +function StandardView:use_layout(layout) + self.cur_layout = self:_stage_layout(layout) end ---Save the panel cursor position for later restoration. @@ -288,21 +305,43 @@ StandardView.use_entry = async.void(function(self, entry) self.cur_layout.emitter = entry.layout.emitter await(self.cur_layout:use_entry(entry)) else - if self.layouts[entry.layout.class] then - self.cur_layout = self.layouts[entry.layout.class] - self.cur_layout.emitter = entry.layout.emitter - else - self:use_layout(entry.layout) - self.cur_layout.emitter = entry.layout.emitter + -- Atomic swap: `self.cur_layout` must always expose a valid, + -- fully-created layout, since observers can read it at any point + -- (notably `update_files_impl`'s debounced `ensure_layout` and any + -- autocmd that `Window:close` fires synchronously during teardown). + -- So: build the incoming layout off to the side, publish it only + -- once `create` has resolved, and destroy the old one only after + -- publishing. A concurrent `Layout:ensure` then never sees a + -- half-built or half-destroyed layout and cannot trip `recover` + -- against one. + -- + -- Destroying the old layout AFTER the new create also lets + -- `find_pivot` split one of the old windows for the new layout's + -- pivot, anchoring the new windows inside the current diff area + -- rather than the null split `Panel:close` fabricates on a + -- last-window close. + local new_layout = self:_stage_layout(entry.layout) + new_layout.emitter = entry.layout.emitter + + -- Guard the staging phase: if `use_entry` or `create` errors + -- part-way through, any windows `create_wins` already made would + -- linger as orphans and the half-built `new_layout` would stay in + -- `self.layouts` for the next swap to reuse. Tear it down and drop + -- the cache entry before rethrowing so `_stage_layout` re-clones + -- next time. + local ok, err = pawait(async.void(function() + await(new_layout:use_entry(entry)) + await(new_layout:create()) + end)) + if not ok then + new_layout:destroy() + self.layouts[entry.layout.class] = nil + error(err) end - await(self.cur_layout:use_entry(entry)) - local future = self.cur_layout:create() + self.cur_layout = new_layout old_layout:destroy() - -- Wait for files to be created + opened - await(future) - if not vim.o.equalalways then vim.cmd("wincmd =") end diff --git a/lua/diffview/tests/functional/restore_focus_after_layout_swap_spec.lua b/lua/diffview/tests/functional/restore_focus_after_layout_swap_spec.lua index e0f99bea..04c9aa32 100644 --- a/lua/diffview/tests/functional/restore_focus_after_layout_swap_spec.lua +++ b/lua/diffview/tests/functional/restore_focus_after_layout_swap_spec.lua @@ -8,9 +8,10 @@ local Diff2Hor = require("diffview.scene.layouts.diff_2_hor").Diff2Hor local EventEmitter = require("diffview.events").EventEmitter local GitAdapter = require("diffview.vcs.adapters.git").GitAdapter local GitRev = require("diffview.vcs.adapters.git.rev").GitRev +local Layout = require("diffview.scene.layout").Layout local RevType = require("diffview.vcs.rev").RevType -local await = async.await +local await, pawait = async.await, async.pawait local eq = helpers.eq local run = helpers.run local cleanup_repo = helpers.cleanup_repo @@ -150,4 +151,97 @@ describe("StandardView.use_entry layout-swap focus (integration)", function() end end) ) + + -- The atomic-swap invariant: `self.cur_layout` always exposes a + -- valid layout to observers throughout the swap — the outgoing one + -- until `create` resolves, then the fully-created new one while + -- the old windows are torn down. A concurrent `ensure_layout` (the + -- debounced `update_files_impl` firing mid-swap, or any autocmd + -- that window teardown fires synchronously) therefore never sees a + -- half-built layout and cannot trip `Layout:recover`. + it( + "keeps cur_layout on a valid layout throughout the swap", + helpers.async_test(function() + config.setup({ + use_icons = false, + view = { + default = { layout = "diff2_horizontal", focus_diff = false }, + one_sided_layout = "raw", + }, + }) + + local repo = make_repo() + local view + + local ok, err = pcall(function() + local adapter = GitAdapter({ toplevel = repo, cpath = repo, path_args = {} }) + view = DiffView({ + adapter = adapter, + rev_arg = nil, + path_args = {}, + left = GitRev(RevType.STAGE, 0), + right = GitRev(RevType.LOCAL), + options = { show_untracked = true }, + }) + assert.is_true(view:is_valid()) + + view:open() + local loaded = vim.wait(3000, function() + return view.initialized + end, 10) + assert.is_true(loaded, "view did not finish loading within 3s") + + if view._set_file_in_flight then + await(view._set_file_in_flight) + end + + local modified_entry, raw_entry + for _, f in view.files:iter() do + if f.path == "existing.txt" then + modified_entry = f + elseif f.path == "newfile.txt" then + raw_entry = f + end + end + + -- Land on the Diff2Hor entry as the swap's outgoing layout. + await(view:set_file(modified_entry, false, false)) + eq(Diff2Hor, view.cur_layout.class) + + -- Spy on the invariant. Sample `self.cur_layout:is_valid()` on + -- every `Layout:destroy` entry that fires during the swap: at + -- each of those points the view must still expose a valid + -- layout to any observer (concurrent `ensure_layout`, autocmds + -- that window teardown fires synchronously, etc.). The specific + -- identity of that layout — outgoing vs. freshly-created — + -- doesn't matter to the invariant; only its validity does. + local snapshots = {} + local orig_destroy = Layout.destroy + Layout.destroy = function(layout_self) + snapshots[#snapshots + 1] = view.cur_layout and view.cur_layout:is_valid() + orig_destroy(layout_self) + end + + local swap_ok, swap_err = pawait(view.set_file, view, raw_entry, false, false) + Layout.destroy = orig_destroy + assert(swap_ok, swap_err) + + eq(Diff1Raw, view.cur_layout.class) + + -- At least one destroy fires during the swap (the outgoing + -- Diff2Hor). Every snapshot taken while the swap was in + -- flight must show a valid `cur_layout`. + assert.is_true(#snapshots > 0, "expected at least one Layout:destroy call during the swap") + for i, valid in ipairs(snapshots) do + assert.is_true(valid, "cur_layout was invalid at destroy call #" .. i) + end + end) + + close_view(view) + cleanup_repo(repo) + if not ok then + error(err) + end + end) + ) end)