Sidebar: layoutFor counts open recent projects as rendered rows, corrupting section height, scroll clamp, and hit testing

[coneilen]

Summary

Sidebar.layoutFor() counts every entry in model.recent_projects as a rendered 24px project row, but appendRows() skips rendering any recent project that is already open. Whenever an open project also appears in recent_projects, the layout/scroll math disagrees with the rows actually drawn.

Found by wiring Sidebar.zig's previously-unexecuted tests into CI under #424. These are pre-existing failures that had never run, not regressions.

Evidence (verified on main)

The row producer skips open projects:

Sidebar.zig:507   if (project.isRemote() or isProjectOpen(model, project.path)) continue;
Sidebar.zig:518   if (!project.isRemote() or isProjectOpen(model, project.path)) continue;

The layout consumer does not:

Sidebar.zig:588   .project_count = model.recent_projects.items.len,

and that count drives geometry directly:

Sidebar.zig:485   return @as(i32, @intCast((self.project_count + self.project_heading_count) * 24));

So project_count overcounts by exactly the number of recent projects that are currently open. Section height, every offset derived from it, and scroll clamping are all inflated.

Note :647 and :652 (projectHeadingCount helpers) do apply !isProjectOpen(...), so heading visibility already respects the skip. Only the row count does not, which is why this reads as an oversight rather than an intentional model.

Observed failures

Three tests in Sidebar.zig fail on first execution:

test	result
shared sidebar layout routes every loop row after project rows and scroll	expected 0, found 1
sidebar scroll clamps overflow, shrink, and resize	expected 334, found 410
recent project rows exclude folders already open in the projects list	expected .open_project, found .quick_chat_overview

The 334 vs 410 delta is 76px, and the third failure shows hit-testing resolving to the wrong row kind - a click lands on a different row than the one drawn. These are consistent with an overcounted project section, not with three unrelated defects.

App.zig imports Sidebar.zig, so the same three failures surface in the App test binary too.

User-visible impact

With at least one open project also present in recents: the sidebar reserves vertical space for rows that are never drawn, sections below the project list sit lower than the content they label, scroll range over-extends past the last real row, and hit testing can resolve to the wrong row. The third failing test indicates a click intended for a project row can be attributed to a different row kind entirely.

Expected

layoutFor() counts only rows appendRows() will actually render - i.e. apply the same isProjectOpen (and remote/local partition) filtering used at :507/:518, consistent with how :647/:652 already work.

Fix guidance

The three failing tests encode the correct expectations; make them pass rather than adjusting them. Deriving the count from the same predicate used by appendRows (rather than duplicating the condition) would prevent the two from drifting apart again.

Sidebar.zig has 22 tests total and is being wired into the harness by #426, which quarantines these three rather than silently deleting them. Once fixed, remove the quarantine so all 22 gate normally.

[coneilen]

Correction: this issue conflated two independent defects

The original report claimed all three failures shared one root cause (the overcounted project section) and cited the 76px scroll delta as supporting evidence. That inference was wrong. Verified directly against the code on main:

Defect 1 - real product bug (this issue). layoutFor() at :588 counts every recent_projects entry while appendRows() at :507/:518 skips already-open projects. This causes:

• shared sidebar layout routes every loop row after project rows and scroll (expected 0, found 1)
• recent project rows exclude folders already open in the projects list (expected .open_project, found .quick_chat_overview)

Both now pass with a source fix. This remains the substance of #428.

Defect 2 - stale test, NOT a product bug. sidebar scroll clamps overflow, shrink, and resize (expected 334, found 410) is unrelated to project overcount. The 76px delta is exactly the Activity block:

Sidebar.zig:817   if (model.activity.items.len != 0) {
Sidebar.zig:818       bottom += 18 + 24 + 34;      // = 76

paint() at :282 renders that block - header, "Attention only" filter, prev/next controls, and cards - whenever activity is non-empty, so contentBottom is correct to reserve the space. The test appends 3 activity events at :1326, but its oracle at :1340 is:

const expected_short = sidebarSectionBottom(&model, &inspection, null) + 30 + 4 * 34 - 400;

which accounts for the Needs You block and omits the Activity term. 410 - 334 = 76.

Changing the product to satisfy that expectation would introduce a bug, excluding visibly-rendered content from the scroll extent and clipping the Activity block. The expectation is being corrected to include the missing term, written as literals rather than as a call to contentBottom() so the oracle stays independent of the implementation it checks.

Credit where due: this was caught by actually executing the tests rather than reasoning about them, which is precisely why #424 (wiring the ~91 orphaned tests) matters. Two defects of genuinely different kinds were sitting behind one silent gap.

[coneilen]

Fixed by #430, merged as ece5935.

Verified on main rather than taking the fix on trust. The repair is structural, not a point patch: a single shared predicate projectIsVisibleInSection (Sidebar.zig:669) now drives all three consumers that previously disagreed -- row emission (:507, :518), section-heading presence (:655, :662), and the count feeding layout geometry (:681, via visibleProjectCount). Because the counter and the renderer are now the same code path, the count-vs-render divergence that caused this bug cannot silently reappear at one call site.

Recording the correction noted earlier in this thread, since it matters for anyone reading this later: only two of the three failures were this product defect. The third (sidebar scroll clamps overflow, shrink, and resize, expected 334 / found 410) was a stale test oracle -- the 76px delta is exactly the Activity block that contentBottom reserves and paint() renders, which the test's expected value omitted. Changing the product to satisfy it would have clipped visible content. That oracle was corrected to literals in #430 instead.