From 4e907c6b3e00d22fa954b10a9fd97bfb1f601c7b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 9 Jul 2026 14:25:25 +0200 Subject: [PATCH] Compute list index conversions independently of rendering The model<->view index conversions were derived from arrays that only renderLines populated. That made them depend on the list having been rendered (so a conversion before the first render ignored the non-model items), and it made them go stale whenever the model changed after a render: converting an index then returned a wrong result, and once the model had grown past the last rendered length the conversion indexed a too-short array and panicked (seen in cherry_pick under -race). The conversion is a pure function of the current list length and the current non-model items, and needs none of the rendered display strings. Compute it directly and drop the cached arrays, so the result is always consistent with the current model and no longer depends on rendering. searchModelCommits converts every commit's index, and building the non-model items can be O(len) mid-rebase, so it would now be quadratic; snapshot the non-model items once via modelToViewIndexConverter instead of rebuilding them per index. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/gui/context/list_renderer.go | 96 ++++++++++++++---------- pkg/gui/context/list_renderer_test.go | 7 -- pkg/gui/context/local_commits_context.go | 2 +- pkg/gui/context/sub_commits_context.go | 2 +- 4 files changed, 60 insertions(+), 47 deletions(-) diff --git a/pkg/gui/context/list_renderer.go b/pkg/gui/context/list_renderer.go index e863045e0..b8d036778 100644 --- a/pkg/gui/context/list_renderer.go +++ b/pkg/gui/context/list_renderer.go @@ -32,34 +32,76 @@ type ListRenderer struct { getNonModelItems func() []*NonModelItem // The remaining fields are private and shouldn't be initialized by clients - numNonModelItems int - viewIndicesByModelIndex []int - modelIndicesByViewIndex []int - columnPositions []int + columnPositions []int } func (self *ListRenderer) GetList() types.IList { return self.list } -func (self *ListRenderer) ModelIndexToViewIndex(modelIndex int) int { - modelIndex = lo.Clamp(modelIndex, 0, self.list.Len()) - if self.viewIndicesByModelIndex != nil { - return self.viewIndicesByModelIndex[modelIndex] +func (self *ListRenderer) getNonModelItemList() []*NonModelItem { + if self.getNonModelItems == nil { + return nil } + return self.getNonModelItems() +} - return modelIndex +func (self *ListRenderer) ModelIndexToViewIndex(modelIndex int) int { + return modelIndexToViewIndex(self.list.Len(), self.getNonModelItemList(), modelIndex) } func (self *ListRenderer) ViewIndexToModelIndex(viewIndex int) int { - viewIndex = lo.Clamp(viewIndex, 0, self.list.Len()+self.numNonModelItems) - if self.modelIndicesByViewIndex != nil { - return self.modelIndicesByViewIndex[viewIndex] - } + return viewIndexToModelIndex(self.list.Len(), self.getNonModelItemList(), viewIndex) +} +// modelToViewIndexConverter returns a model-to-view index conversion that +// reuses a single snapshot of the non-model items. Callers that convert many +// indices in a row (e.g. search, which converts every commit) should use this +// rather than calling ModelIndexToViewIndex per index, which would rebuild the +// non-model items each time. +func (self *ListRenderer) modelToViewIndexConverter() func(modelIndex int) int { + listLength := self.list.Len() + nonModelItems := self.getNonModelItemList() + return func(modelIndex int) int { + return modelIndexToViewIndex(listLength, nonModelItems, modelIndex) + } +} + +// The view shows the model items with the non-model items (e.g. section +// headers) inserted at their model indices. The two conversions below are +// computed directly from the current list length and non-model items, so they +// don't depend on the list having been rendered, and they can never be stale +// with respect to a model that changed since the last render (which used to +// cause both wrong results and index-out-of-range panics). +// +// The non-model items are assumed to be ordered by their Index, which is how +// all producers build them; the i-th one therefore ends up at view index +// Index+i. +func modelIndexToViewIndex(listLength int, nonModelItems []*NonModelItem, modelIndex int) int { + modelIndex = lo.Clamp(modelIndex, 0, listLength) + // Each non-model item inserted at or before this model item pushes it down + // by one row in the view. + viewIndex := modelIndex + for _, item := range nonModelItems { + if item.Index <= modelIndex { + viewIndex++ + } + } return viewIndex } +func viewIndexToModelIndex(listLength int, nonModelItems []*NonModelItem, viewIndex int) int { + viewIndex = lo.Clamp(viewIndex, 0, listLength+len(nonModelItems)) + // Subtract the non-model items that appear before this view index. + modelIndex := viewIndex + for i, item := range nonModelItems { + if item.Index+i < viewIndex { + modelIndex-- + } + } + return modelIndex +} + func (self *ListRenderer) ColumnPositions() []int { return self.columnPositions } @@ -71,23 +113,18 @@ func (self *ListRenderer) renderLines(startIdx int, endIdx int) string { if self.getColumnAlignments != nil { columnAlignments = self.getColumnAlignments() } - nonModelItems := []*NonModelItem{} - self.numNonModelItems = 0 - if self.getNonModelItems != nil { - nonModelItems = self.getNonModelItems() - self.prepareConversionArrays(nonModelItems) - } + nonModelItems := self.getNonModelItemList() startModelIdx := 0 if startIdx == -1 { startIdx = 0 } else { - startModelIdx = self.ViewIndexToModelIndex(startIdx) + startModelIdx = viewIndexToModelIndex(self.list.Len(), nonModelItems, startIdx) } endModelIdx := self.list.Len() if endIdx == -1 { endIdx = endModelIdx + len(nonModelItems) } else { - endModelIdx = self.ViewIndexToModelIndex(endIdx) + endModelIdx = viewIndexToModelIndex(self.list.Len(), nonModelItems, endIdx) } lines, columnPositions := utils.RenderDisplayStrings( self.getDisplayStrings(startModelIdx, endModelIdx), @@ -97,23 +134,6 @@ func (self *ListRenderer) renderLines(startIdx int, endIdx int) string { return strings.Join(lines, "\n") } -func (self *ListRenderer) prepareConversionArrays(nonModelItems []*NonModelItem) { - self.numNonModelItems = len(nonModelItems) - viewIndicesByModelIndex := lo.Range(self.list.Len() + 1) - modelIndicesByViewIndex := lo.Range(self.list.Len() + 1) - offset := 0 - for _, item := range nonModelItems { - for i := item.Index; i <= self.list.Len(); i++ { - viewIndicesByModelIndex[i]++ - } - modelIndicesByViewIndex = slices.Insert( - modelIndicesByViewIndex, item.Index+offset, modelIndicesByViewIndex[item.Index+offset]) - offset++ - } - self.viewIndicesByModelIndex = viewIndicesByModelIndex - self.modelIndicesByViewIndex = modelIndicesByViewIndex -} - func (self *ListRenderer) insertNonModelItems( nonModelItems []*NonModelItem, endIdx int, startIdx int, lines []string, columnPositions []int, ) []string { diff --git a/pkg/gui/context/list_renderer_test.go b/pkg/gui/context/list_renderer_test.go index 24e4aa822..11398a995 100644 --- a/pkg/gui/context/list_renderer_test.go +++ b/pkg/gui/context/list_renderer_test.go @@ -254,9 +254,6 @@ func TestListRenderer_ModelIndexToViewIndex_and_back(t *testing.T) { getNonModelItems: getNonModelItems, } - // Need to render first so that it knows the non-model items - self.renderLines(-1, -1) - for i := range len(s.modelIndices) { assert.Equal(t, s.expectedViewIndices[i], self.ModelIndexToViewIndex(s.modelIndices[i])) } @@ -288,10 +285,6 @@ func TestListRenderer_IndexConversionsAreRenderIndependent(t *testing.T) { } // Deliberately convert without rendering first. - /* EXPECTED: assert.Equal(t, 2, self.ModelIndexToViewIndex(1)) assert.Equal(t, 1, self.ViewIndexToModelIndex(2)) - ACTUAL: */ - assert.Equal(t, 1, self.ModelIndexToViewIndex(1)) - assert.Equal(t, 2, self.ViewIndexToModelIndex(2)) } diff --git a/pkg/gui/context/local_commits_context.go b/pkg/gui/context/local_commits_context.go index d929aca88..a66c720c9 100644 --- a/pkg/gui/context/local_commits_context.go +++ b/pkg/gui/context/local_commits_context.go @@ -224,7 +224,7 @@ func (self *LocalCommitsContext) RefForAdjustingLineNumberInDiff() string { } func (self *LocalCommitsContext) ModelSearchResults(searchStr string, caseSensitive bool) []gocui.SearchPosition { - return searchModelCommits(caseSensitive, self.GetCommits(), self.ColumnPositions(), self.ModelIndexToViewIndex, searchStr) + return searchModelCommits(caseSensitive, self.GetCommits(), self.ColumnPositions(), self.modelToViewIndexConverter(), searchStr) } func (self *LocalCommitsViewModel) SetLimitCommits(value bool) { diff --git a/pkg/gui/context/sub_commits_context.go b/pkg/gui/context/sub_commits_context.go index fee5492ac..4e05c9594 100644 --- a/pkg/gui/context/sub_commits_context.go +++ b/pkg/gui/context/sub_commits_context.go @@ -223,7 +223,7 @@ func (self *SubCommitsContext) RefForAdjustingLineNumberInDiff() string { } func (self *SubCommitsContext) ModelSearchResults(searchStr string, caseSensitive bool) []gocui.SearchPosition { - return searchModelCommits(caseSensitive, self.GetCommits(), self.ColumnPositions(), self.ModelIndexToViewIndex, searchStr) + return searchModelCommits(caseSensitive, self.GetCommits(), self.ColumnPositions(), self.modelToViewIndexConverter(), searchStr) } func (self *SubCommitsContext) IndexForGotoBottom() int {