revert: unrevert "fix: unsubscribe global emitter listeners on View close to release memory (#24)" by dlyongemallo · Pull Request #28 · dlyongemallo/diffview.nvim

dlyongemallo · 2026-02-28T19:56:16Z

This reverts the revert in 886640d, re-applying the original fix from 0ad7f36 for debugging. Adds additional tests covering the close sequence: local listeners, on_any forwarding, reentrant emit, and multi-view isolation.

…lose to release memory (#24)" This reverts the revert in 886640d, re-applying the original fix from 0ad7f36 for debugging. Adds additional tests covering the close sequence: local listeners, on_any forwarding, reentrant emit, and multi-view isolation.

Copilot

Pull request overview

Re-applies the View-close memory leak fix by unsubscribing View-registered global emitter listeners on close, and adds functional regression tests to exercise the View close sequence and listener isolation.

Changes:

Track global-emitter callback wrappers per-View and unsubscribe them during View:close().
Clear the View’s local emitter on close to remove local listeners.
Add a new functional spec covering EventEmitter basics plus View init/close regression cases (local listeners, on_any, reentrant emit, and multi-view isolation).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
lua/diffview/scene/view.lua	Tracks global emitter callbacks per View and unsubscribes them on close to prevent leaked references; clears local emitter.
lua/diffview/tests/functional/event_emitter_spec.lua	Adds functional tests for listener cleanup and View close lifecycle regressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-28T20:00:37Z

+      local orig_emitter = DiffviewGlobal.emitter
+      DiffviewGlobal.emitter = EventEmitter()
+
+      local inner_fired = false
+
+      local view = View({ default_layout = {} })
+
+      -- Register a view_closed listener on the local emitter that
+      -- emits another event during the close sequence.
+      view.emitter:on("view_closed", function()
+        view.emitter:emit("custom_event")
+      end)
+      view.emitter:on("custom_event", function()
+        inner_fired = true
+      end)
+
+      view:close()
+
+      -- The nested emit should have fired before clear().
+      eq(true, inner_fired)
+      eq(nil, view.emitter:get("view_closed"))
+      eq(nil, view.emitter:get("custom_event"))
+
+      DiffviewGlobal.emitter = orig_emitter
+    end)


This test swaps DiffviewGlobal.emitter but cleanup is not guaranteed if the test aborts early due to an assertion failure. That can cause cross-test contamination. Consider extracting an with_temp_global_emitter helper or using before_each/after_each to ensure DiffviewGlobal.emitter is always restored.

Copilot · 2026-02-28T20:00:37Z

+      DiffviewGlobal.emitter = EventEmitter()
+
+      local inner_fired = false
+
+      local view = View({ default_layout = {} })
+
+      -- Register a view_closed listener on the local emitter that
+      -- emits another event during the close sequence.
+      view.emitter:on("view_closed", function()
+        view.emitter:emit("custom_event")
+      end)
+      view.emitter:on("custom_event", function()
+        inner_fired = true
+      end)
+
+      view:close()
+
+      -- The nested emit should have fired before clear().
+      eq(true, inner_fired)
+      eq(nil, view.emitter:get("view_closed"))
+      eq(nil, view.emitter:get("custom_event"))
+
+      DiffviewGlobal.emitter = orig_emitter
+    end)
+
+    -- Verifies that when two views exist, closing one does not affect
+    -- the other's listeners.
+    it("closing one view does not remove another view's global listeners", function()
+      local orig_emitter = DiffviewGlobal.emitter
+      DiffviewGlobal.emitter = EventEmitter()
+
+      local view_a = View({ default_layout = {} })
+      local view_b = View({ default_layout = {} })
+
+      -- Both views should have registered a view_closed wrapper.
+      eq(2, #(DiffviewGlobal.emitter:get("view_closed") or {}))
+
+      view_a:close()
+
+      -- Only view_a's wrapper should have been removed.
+      eq(1, #(DiffviewGlobal.emitter:get("view_closed") or {}))
+
+      -- View B's wrapper should still work.
+      local forwarded = false
+      view_b.emitter:on("view_closed", function()
+        forwarded = true
+      end)
+      DiffviewGlobal.emitter:emit("view_closed", view_b)
+      eq(true, forwarded)
+
+      view_b:close()
+      eq(0, #(DiffviewGlobal.emitter:get("view_closed") or {}))
+
+      DiffviewGlobal.emitter = orig_emitter


This test changes DiffviewGlobal.emitter and restores it at the end, but a failure anywhere before the restore will leave the global emitter in the modified state and potentially break later tests. Use after_each or wrap the body in pcall so the restore always runs.

Suggested change

DiffviewGlobal.emitter = EventEmitter()

local inner_fired = false

local view = View({ default_layout = {} })

-- Register a view_closed listener on the local emitter that

-- emits another event during the close sequence.

view.emitter:on("view_closed", function()

view.emitter:emit("custom_event")

end)

view.emitter:on("custom_event", function()

inner_fired = true

end)

view:close()

-- The nested emit should have fired before clear().

eq(true, inner_fired)

eq(nil, view.emitter:get("view_closed"))

eq(nil, view.emitter:get("custom_event"))

DiffviewGlobal.emitter = orig_emitter

end)

-- Verifies that when two views exist, closing one does not affect

-- the other's listeners.

it("closing one view does not remove another view's global listeners", function()

local orig_emitter = DiffviewGlobal.emitter

DiffviewGlobal.emitter = EventEmitter()

local view_a = View({ default_layout = {} })

local view_b = View({ default_layout = {} })

-- Both views should have registered a view_closed wrapper.

eq(2, #(DiffviewGlobal.emitter:get("view_closed") or {}))

view_a:close()

-- Only view_a's wrapper should have been removed.

eq(1, #(DiffviewGlobal.emitter:get("view_closed") or {}))

-- View B's wrapper should still work.

local forwarded = false

view_b.emitter:on("view_closed", function()

forwarded = true

end)

DiffviewGlobal.emitter:emit("view_closed", view_b)

eq(true, forwarded)

view_b:close()

eq(0, #(DiffviewGlobal.emitter:get("view_closed") or {}))

DiffviewGlobal.emitter = orig_emitter

local ok, err = pcall(function()

DiffviewGlobal.emitter = EventEmitter()

local inner_fired = false

local view = View({ default_layout = {} })

-- Register a view_closed listener on the local emitter that

-- emits another event during the close sequence.

view.emitter:on("view_closed", function()

view.emitter:emit("custom_event")

end)

view.emitter:on("custom_event", function()

inner_fired = true

end)

view:close()

-- The nested emit should have fired before clear().

eq(true, inner_fired)

eq(nil, view.emitter:get("view_closed"))

eq(nil, view.emitter:get("custom_event"))

end)

DiffviewGlobal.emitter = orig_emitter

if not ok then

error(err)

end

end)

-- Verifies that when two views exist, closing one does not affect

-- the other's listeners.

it("closing one view does not remove another view's global listeners", function()

local orig_emitter = DiffviewGlobal.emitter

local ok, err = pcall(function()

DiffviewGlobal.emitter = EventEmitter()

local view_a = View({ default_layout = {} })

local view_b = View({ default_layout = {} })

-- Both views should have registered a view_closed wrapper.

eq(2, #(DiffviewGlobal.emitter:get("view_closed") or {}))

view_a:close()

-- Only view_a's wrapper should have been removed.

eq(1, #(DiffviewGlobal.emitter:get("view_closed") or {}))

-- View B's wrapper should still work.

local forwarded = false

view_b.emitter:on("view_closed", function()

forwarded = true

end)

DiffviewGlobal.emitter:emit("view_closed", view_b)

eq(true, forwarded)

view_b:close()

eq(0, #(DiffviewGlobal.emitter:get("view_closed") or {}))

end)

DiffviewGlobal.emitter = orig_emitter

if not ok then

error(err)

end

Copilot · 2026-02-28T20:00:37Z

+      -- Swap in a fresh global emitter so the test is isolated.
+      local orig_emitter = DiffviewGlobal.emitter
+      DiffviewGlobal.emitter = EventEmitter()
+
+      for _ = 1, 10 do
+        local view = View({ default_layout = {} })
+        view:close()
+      end
+
+      eq(0, #(DiffviewGlobal.emitter:get("view_closed") or {}))
+
+      DiffviewGlobal.emitter = orig_emitter
+    end)


This test mutates global state (DiffviewGlobal.emitter) but only restores it at the end of the test body. If an assertion fails or an error is thrown before the restore line, the global emitter will remain swapped and can cascade failures into later tests. Use a protected cleanup pattern (e.g., wrap the body in pcall and restore in a finally section, or use before_each/after_each for this describe block) so restoration happens even on failure.

Copilot · 2026-02-28T20:00:38Z

+      local orig_emitter = DiffviewGlobal.emitter
+      DiffviewGlobal.emitter = EventEmitter()
+
+      local call_log = {}
+
+      local view = View({ default_layout = {} })
+
+      -- Register listeners similar to those from diff/listeners.lua.
+      view.emitter:on("tab_enter", function()
+        table.insert(call_log, "tab_enter")
+      end)
+      view.emitter:on("tab_leave", function()
+        table.insert(call_log, "tab_leave")
+      end)
+
+      view:close()
+
+      -- After close, the local emitter should be empty.
+      eq(nil, view.emitter:get("tab_enter"))
+      eq(nil, view.emitter:get("tab_leave"))
+
+      -- Emitting on a cleared emitter should be a no-op, not a crash.
+      view.emitter:emit("tab_enter")
+
+      DiffviewGlobal.emitter = orig_emitter
+    end)


This test mutates global state (DiffviewGlobal.emitter) but restoration only happens at the end of the test. If any assertion fails before the restore, subsequent tests may run with the wrong global emitter and produce misleading failures. Prefer after_each (or a pcall/finally pattern) to guarantee cleanup.

Copilot · 2026-02-28T20:00:38Z

+      local orig_emitter = DiffviewGlobal.emitter
+      DiffviewGlobal.emitter = EventEmitter()
+
+      local any_events = {}
+      DiffviewGlobal.emitter:on_any(function(e, args)
+        table.insert(any_events, e.id)
+      end)
+
+      local view = View({ default_layout = {} })
+      view:close()
+
+      -- The on_any listener should have seen "view_closed".
+      assert(vim.tbl_contains(any_events, "view_closed"),
+        "on_any listener should see view_closed event")
+
+      -- No listeners should remain on the global emitter.
+      eq(0, #(DiffviewGlobal.emitter:get("view_closed") or {}))
+
+      DiffviewGlobal.emitter = orig_emitter
+    end)


This test replaces DiffviewGlobal.emitter and restores it only on the happy path. If the test errors before the final assignment, the altered global emitter can leak into the rest of the suite. Use after_each or a guaranteed cleanup mechanism so global state is always restored.

Copilot AI review requested due to automatic review settings February 28, 2026 19:56

dlyongemallo merged commit 397db14 into main Feb 28, 2026
4 checks passed

Copilot started reviewing on behalf of dlyongemallo February 28, 2026 19:56 View session

dlyongemallo deleted the add_tests_for_memory_fix branch February 28, 2026 19:56

Copilot AI reviewed Feb 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert: unrevert "fix: unsubscribe global emitter listeners on View close to release memory (#24)"#28

revert: unrevert "fix: unsubscribe global emitter listeners on View close to release memory (#24)"#28
dlyongemallo merged 1 commit intomainfrom
add_tests_for_memory_fix

dlyongemallo commented Feb 28, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 28, 2026

Uh oh!

Copilot AI Feb 28, 2026

Uh oh!

Copilot AI Feb 28, 2026

Uh oh!

Copilot AI Feb 28, 2026

Uh oh!

Copilot AI Feb 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dlyongemallo commented Feb 28, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants