Skip to content

perf(shared): avoid quadratic file link labels - #34340

Open
voltcrash wants to merge 1 commit into
pingdotgg:mainfrom
voltcrash:perf/file-link-parent-suffixes
Open

voltcrash wants to merge 1 commit into
pingdotgg:mainfrom
voltcrash:perf/file-link-parent-suffixes

Conversation

@voltcrash

Copy link
Copy Markdown
Contributor

Problem

Reports containing many different paths named index.ts make file-chip disambiguation scan the whole basename group for each path. Both synchronous Markdown rendering and the first server Find parse call this helper. The exact 4,000-path reproduction took 2,180 ms on the original code in this session.

Change

Count parent suffixes once per basename group and look up collisions, preserving the shortest unique suffix, two-parent minimum, path normalization, and indistinguishable-path fallback.

Scope and approval

This is a small performance fix with no product behavior change, submitted under the focused obvious-bug exception.

Verification

  • Added a deterministic complexity regression: 128 paths constructed 16,768 suffixes before (test fails against a 2,560 bound), and 896 after. No wall-clock assertion.
  • vp test run packages/shared/src/fileLinks.test.ts packages/shared/src/threadFindText.test.ts: 44 passed. POSIX, Windows, UNC, duplicate, shallow, and colliding paths covered; 1,000 additional seeded mixed-path groups matched the original helper.
  • Five same-process helper benchmark pairs for /repo/packages/pkg-${i}/src/index.ts (4,000 paths): before 3731/3570/3623/3785/4334 ms; after 48/49/29/22/20 ms. Every output map matched. These are isolated helper timings, not end-to-end render or Find speedups; runtime/load affects them. Work is linear in path count for fixed path depth; suffix strings still cost with path depth.
  • Isolated .t3 dev server and real Browser panel: a replayed 128-link assistant report produced all expected index.ts · pkg-N/src chips; Find for pkg-127/src returned 1/1. Preview screenshots/recordings timed out, so no visual timing evidence is claimed. The scenarios runbook explicitly excludes timing regressions from replay proof; no artificial timeout scenario was added.
  • vp run check:changed: passed format, lint, knip, affected-package typechecks, and adjacent tests.

Overlap: freshly inspected open upstream PRs, including #27866 (Markdown parsing), #33794 (completed-block rerenders), #29310 (Find cache), and #28644 (parentheses parsing); none fixes this helper's pairwise scan.

Reviewed with review-loop: 1 round with Claude Opus 5.5 on xhigh. Reviewer verdict: approved. Optional nits declined to keep the fix focused (deep-path optimization, trimming compatibility cases, alternative complexity instrumentation).

Created with GPT-6.1 Sol (xhigh) in the Codex harness, through T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 11, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 04083af

Macroscope's review found this PR approvable — This optimizes parent-directory suffix computation for file-link labels without changing link destinations. The assessed change is confined to display-label generation and presents no obvious material risk.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

buildFileLinkParentSuffixByPath now counts parent suffix occurrences to select unique displayed suffixes. New tests cover duplicate basenames, path formats, and the number of Array.join calls.

Changes: File link suffixes

Layer / File(s) Summary
Count suffixes and select unique labels
packages/shared/src/fileLinks.ts, packages/shared/src/fileLinks.test.ts
The builder counts parent suffixes and selects the first unique depth, while retaining the minimum and available-depth limits. Tests cover varied paths and assert fewer than 20 Array.join calls per path for 128 paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Possibly related PRs

Suggested labels: size:M

Suggested reviewers: noojuno, macroscopeapp

Merge Risk: Minimal · up to 04083

This change speeds up file link label disambiguation while keeping the same displayed labels. No actionable merge-blocking risk was identified.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly identifies a performance improvement in shared file-link label generation and matches the main change.
Description check Passed The description includes all required sections. It explains the problem, implementation, scope rationale, focused verification, benchmark results, limitations, and test outcomes.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between d806251 and 04083af.

Files selected for processing: 2

  • packages/shared/src/fileLinks.test.ts
  • packages/shared/src/fileLinks.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Verified in a real dev environment

Head 04083af, compared against main (d2c2595), each run with vp run dev and identical copied .t3 state.

Setup, all through the app: a real git repo with 1,000 packages/pkg-N/src/index.ts files, added as a project. Claude claude-haiku-5-5 turns produced (1) a reply listing all 1,000 paths as inline code and (2) a mixed-path reply. Headless Chrome (Playwright) clicked the threads in the sidebar, typed in the composer and used Find. A CDP CPU profile measured time spent inside buildFileLinkParentSuffixByPath.

Measurement (CPU inside the helper) main PR
Open the 1,000-link thread (3 runs) 128.6 / 193.2 / 142.1 ms 21.4 / 9.9 / 14.0 ms
Composer follow-up that streams another 1,000-link reply 128.7 ms 8.1 ms
  • Chip labels are identical: index.ts · pkg-N/src for all 1,000 paths.
  • Mixed paths (packages/pkg-1/src, apps/pkg-1/src, packages/pkg-2/src, packages/pkg-2/test, README.md) render the same on both: packages/pkg-1/src, apps/pkg-1/src, pkg-2/src, pkg-2/test, and plain README.md. This covers deeper collisions, the two-parent minimum and the no-suffix case.
  • Find in thread (server ThreadFind path) for pkg-777/src returns 1/1 with the highlight on the right chip on both.
  • Context: total long-task time while streaming barely changed (main 5.4 s, PR 4.9 s). At 1,000 links, rendering the chips costs more than the helper did. The PR removes the helper's share without changing output.

1,000-link thread end, main then PR:

main: last rows of the 1,000-link reply, index.ts · pkg-N/src chips

PR: same last rows, identical chip labels

Mixed-path labels, main then PR:

main: mixed index.ts labels thread

PR: mixed index.ts labels thread, same labels

Find pkg-777/src, main then PR:

main: Find in thread shows 1/1 with pkg-777/src highlighted

PR: Find in thread shows 1/1 with pkg-777/src highlighted

Not verified

  • Desktop (Electron) and mobile clients. Both use the same shared helper, but neither was run on this Linux VM.
  • Windows and UNC paths in a live thread. The PR's unit table covers them; no Windows host was available.
  • I did not profile the server-side Find parse separately. Only its end-to-end result (1/1) was checked.

This branch has not been deployed

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

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants