Skip to content

refactor(app): unify the file-collapse predicate - #524

Open
ceffo wants to merge 1 commit into
agavra:mainfrom
ceffo:unify-file-collapse-predicate
Open

refactor(app): unify the file-collapse predicate#524
ceffo wants to merge 1 commit into
agavra:mainfrom
ceffo:unify-file-collapse-predicate

Conversation

@ceffo

@ceffo ceffo commented Jul 30, 2026

Copy link
Copy Markdown

Whether a file's diff body is hidden was decided independently in five places: the annotation builder, both diff renderers, and two scroll-height helpers. They have to agree or the cursor lands on rows that were never drawn, which is the failure mode behind several past scroll fixes.

This extracts App::is_file_collapsed and routes all five through it. No behavior change — it's a consolidation, and the tests that existed before still pass unchanged.

Why the predicate says nothing about single-file view

The call sites genuinely disagree: file_render_height ignores is_single_file_view while the renderers honor it. Folding the check into the predicate would have silently changed file_render_height for its two callers, so each site keeps its own gate and the asymmetry is preserved rather than accidentally "fixed". There's a test locking that in (is_file_collapsed_ignores_single_file_view) so a future change can't quietly fold it in.

No added per-frame cost

This is called once per file per frame, so I checked the lookup count rather than assuming.

Three sites had cached a reviewed bool and used it to decide two different things — whether to collapse, and whether to draw the "Marked reviewed" banner — which a naive predicate call would have turned into a second map lookup per file per frame. Those two decisions are mutually exclusive on is_single_file_view, so they're now one if/else instead of two guarded conditions. Each branch performs exactly one file-state lookup, matching the previous count:

before after
multi-file file_header_prefix_text + collapse gate (2) unchanged (2)
single-file is_file_reviewed (1) unchanged (1)
hunk_positions one lookup per file (1) unchanged (1)

Uses of is_file_reviewed that drive reviewed-specific decoration — the tree checkbox, the header mark, the banner — are left alone. Only the collapse gates moved.

Testing

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test (1199 passing) are clean on top of 5675284. Dogfooded per CONTRIBUTING by reviewing this diff in tuicr itself.

Note

This is the first of two: #525 builds on it to collapse files marked generated in .gitattributes, which is what motivated the refactor. This one stands on its own merits either way — five copies of one predicate is a latent bug regardless of whether the follow-up is wanted.

🤖 Generated with Claude Code

Whether a file's diff body is hidden was decided independently in five
places: the annotation builder, both diff renderers, and two scroll-height
helpers. They have to agree or the cursor lands on rows that were never
drawn, which is the failure mode behind several past scroll fixes.

Extract `App::is_file_collapsed` and route all five through it. No
behavior change.

The predicate deliberately says nothing about single-file view, because
the call sites genuinely disagree: `file_render_height` ignores it while
the renderers honor it. Folding the check in would have silently changed
`file_render_height` for its two direct callers, so each site keeps its
own gate and the asymmetry is preserved rather than accidentally "fixed".

No added per-frame cost. Three sites had cached a reviewed bool and used
it to decide two different things -- whether to collapse, and whether to
draw the "Marked reviewed" banner -- which a predicate call would have
turned into a second map lookup per file per frame. Those two decisions
are mutually exclusive on `is_single_file_view`, so they are now one
if/else instead of two guarded conditions. Each branch performs exactly
one file-state lookup, matching the previous count:

  multi-file:  file_header_prefix_text + is_file_collapsed  (2, unchanged)
  single-file: is_file_reviewed                             (1, unchanged)
  hunk_positions: one lookup per file                       (1, unchanged)

Uses of `is_file_reviewed` that drive reviewed-specific decoration -- the
tree checkbox, the `✓` header mark, and the banner -- are left alone; only
the collapse gates moved.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ceffo

ceffo commented Jul 30, 2026

Copy link
Copy Markdown
Author

The red X here is not this change. Update command (macos-latest) failed in the live-network step:

Error: failed to fetch update metadata: http status: 403

The 25 update:: unit tests in that same job all passed — the 403 is the GitHub API rate-limiting a shared runner IP. fail-fast then cancelled the Windows leg mid-compile, which is why it also shows red (##[error]The operation was canceled with no test having run).

Two pieces of evidence that it is environmental:

  1. The identical failure happens on main — e.g. run 30514923294, same http status: 403. 12 of the last 25 ci.yml runs are red, several of them pushes to main.
  2. feat: collapse generated files marked in .gitattributes #525 contains this exact commit as its first commit and its macOS and Windows legs both passed, in a run that started about a minute after this one.

I do not have rerun rights, so I closed/reopened to trigger a fresh run rather than force-pushing a no-op commit. If it lands red again on the same step, that is the rate limit rather than anything here.

Not part of this PR, but if the flake is bothersome: that step reaches the releases API unauthenticated, and passing GITHUB_TOKEN to it would raise the limit from 60/hr per IP to 1000/hr. fail-fast: false on the matrix would also stop one leg's network hiccup from masking the other two. Happy to send that as a separate PR if it would be useful.

@ceffo

ceffo commented Jul 30, 2026

Copy link
Copy Markdown
Author

Re-run is green on all seven checks, including Update command on macOS and Windows. The commit SHA is unchanged, so GitHub's rollup now reports SUCCESS (it takes the latest run per check name); the superseded red entries from the first run are still visible in the Checks tab if you scroll, but they are stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant