[Harpoon2] Calling remove_at to manipulate elements when using Telescope UI causes items list to become malformed

[PedroBinotto]

• May be fixed by: [BUGFIX] Calling remove_at to manipulate elements when using Telescope UI causes items list to become malformed #628;

---

• Possibly related:
  • Cannot delete or re-arrange files (using telescope ui) #477;
  • [Harpoon2] Remove files from list when using telescope picker (and possibly also reorder them..?) #499;
  • fix(encode): use a for-loop, instead of next() or ipairs(), to prevent loss of data #573;

---

• What issue are you having that you need harpoon to solve?

When using Harpoon via the Telescope UI, I would like to be able to remove items from the Harpoon list; currently, there are some ways to do it, such as extracting the index in selected_entry.index from telescope.actions.state, and using that to index the item to be removed from the list:

-- excerpt from my neovim config

-- alias definitions:
local harpoon = require("user.harpoon")
local telescope_conf = require("telescope.config").values
local telescope_state = require("telescope.actions.state")
local telescope_finders = require("telescope.finders")
local telescope_pickers = require("telescope.pickers")

-- [...]

function M.toggle_telescope(harpoon_files)
	local file_paths = harpoon_get_paths(harpoon_files)

	telescope_pickers
		.new({}, {
			prompt_title = "Harpoon",
			finder = harpoon_make_finder(file_paths),
			previewer = telescope_conf.file_previewer({}),
			sorter = telescope_conf.generic_sorter({}),
			attach_mappings = function(prompt_buffer_number, map)
				map("i", "<M-d>", function()
				
				        -- HERE
					local selected_entry = telescope_state.get_selected_entry()
					local current_picker = telescope_state.get_current_picker(prompt_buffer_number)
					harpoon:list():remove_at(selected_entry.index)
					current_picker:refresh(harpoon_make_finder(harpoon_get_paths(harpoon:list())))
				        -- HERE

				end)

				return true
			end,
		})
		:find()
end

-- aux functions:

local harpoon_get_paths = function(files)
	local paths = {}
	for _, item in ipairs(files.items) do
		table.insert(paths, item.value)
	end
	return paths
end

local function harpoon_make_finder(paths)
	return telescope_finders.new_table({ results = paths })
end

• Why doesn't the current config help?

This method leads to problems, however. As it turns out, calling remove_at and passing it an index will leave a list with a "hole" where the removed item once was, and every item that comes after will be explicitly indexed, like so:

{  -- BEFORE
  {
    context = {
      col = 0,
      row = 1
    },
    value = "file_1"
  }, {
    context = {
      col = 0,
      row = 1
    },
    value = "file_2"
  }, {
    context = {
      col = 1,
      row = 2
    },
    value = "file_3"
  }, {
    context = {
      col = 0,
      row = 1
    },
    value = "file_4"
  }, {
    context = {
      col = 0,
      row = 1
    },
    value = "file_5"
  }
}

-- harpoon:list():remove_at(3)

{  -- AFTER
  {
    context = {
      col = 0,
      row = 1
    },
    value = "file_1"
  }, {
    context = {
      col = 0,
      row = 1
    },
    value = "file_2"
  },
  [4] = {
    context = {
      col = 0,
      row = 1
    },
    value = "file_4"
  },
  [5] = {
    context = {
      col = 0,
      row = 1
    },
    value = "file_5"
  }
}

This alone wouldn't be a problem, but whenever an operation requires looping over the list, the iterator will halt once it reaches the "missing" element. That includes the Telecope UI rendering, which would only display elements up to that position, and, for whatever reason, this also seems to impact the "persistence" of the harpooned items on exit; harpoon:list().items only contains items up to that index once you restart vim (possibly caused by the use of ipairs() over self.items in HarpoonList:encode, which halts once it reaches the "hole"); this means that the items after the removed element are LOST once you exit vim:

{
  {
    context = {
      col = 0,
      row = 1
    },
    value = "file_1"
  }, {
    context = {
      col = 0,
      row = 1
    },
    value = "file_2"
  }
}

There is a possible workaround if you want to make it work nonetheless, as suggested by @Ocholla-T in this comment (#499); it works by manually removing the item from the table, instead of making the acutal API call:

function M.toggle_telescope(harpoon_files)
	local file_paths = harpoon_get_paths(harpoon_files)

	telescope_pickers
		.new({}, {
			prompt_title = "Harpoon",
			finder = harpoon_make_finder(file_paths),
			previewer = telescope_conf.file_previewer({}),
			sorter = telescope_conf.generic_sorter({}),
			attach_mappings = function(prompt_buffer_number, map)
				map("i", "<M-d>", function()
					local selected_entry = telescope_state.get_selected_entry()
					local current_picker = telescope_state.get_current_picker(prompt_buffer_number)
-                                       harpoon:list():remove_at(selected_entry.index)
+					table.remove(harpoon:list().items, selected_entry.index)
					current_picker:refresh(harpoon_make_finder(harpoon_get_paths(harpoon:list())))
				end)

				return true
			end,
		})
		:find()
end

• What proposed API changes are you suggesting?

This works! But it seems as if there's a bug in the actual list implementation, though it isn't caught by the unit tests nor seems to cause any other apparent problems.

I'm still doing some debugging over this issue, but I was able to roughly identify the point where the "bug" occurs:

---@return HarpoonList
function HarpoonList:remove_at(index)
    if self.items[index] then
        Logger:log(
            "HarpoonList:removeAt",
            { item = self.items[index], index = index }
        )
        self.items[index] = nil
---     ^^^^^^^^^^^^^^^^^^^^^^^ from this point onwards, the full items list is no longer functional
        if index == self._length then
            self._length = determine_length(self.items, self._length)
        end
        Extensions.extensions:emit(
            Extensions.event_names.REMOVE,
            { list = self, item = self.items[index], idx = index }
        )
    end
    return self
end

from: harpoon/lua/harpoon/list.lua#L205

[PedroBinotto]

Fix in the making #628

[PedroBinotto]

I've been using my "fixed" version of remove_at for the last couple days; in order to make Telescope play nice with it, we need a way to represent the nil values in between the populated indexes of the list (since the list only collapses back to its "compact" length once there are no "holes" in between items, in order to preserve the list indexes); something like this:

-- excerpt from my neovim config

local harpoon_get_paths = function(files)
  local paths = {}
  local items = files.items
  local len = files._length

  for i = 1, len do
    paths[i] = ''
    local item = items[i]
    if item ~= nil then
      paths[i] = item.value
    end
  end

  return paths
end

Then you can use remove_at directly and it will work as expected:

function M.toggle_telescope(harpoon_files)
    ...
        attach_mappings = function(prompt_buffer_number, map)
          map("i", "<M-d>", function()
            local selected_entry = telescope_state.get_selected_entry()
            local current_picker = telescope_state.get_current_picker(prompt_buffer_number)
-            table.remove(harpoon:list().items, selected_entry.index)
+            harpoon:list():remove_at(selected_entry.index)
            current_picker:refresh(harpoon_make_finder(harpoon_get_paths(harpoon:list())))
          end)
    ...
end

Asciinema demo of the fixed behaviour:

[ThePrimeagen]

I am officially back, I'll get this through shortly

[PedroBinotto]

👀