Skip to content

fix(jj): enable unchanged-context expansion - #890

Merged
benvinegar merged 2 commits into
modem-dev:mainfrom
samuela:feat/jj-gap-expansion
Aug 30, 2026
Merged

benvinegar merged 2 commits into
modem-dev:mainfrom
samuela:feat/jj-gap-expansion

Conversation

@samuela

@samuela samuela commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Problem

Git-backed reviews can load complete file contents and expand context omitted from a patch. The Jujutsu adapter returned only patch text, so the same gaps appeared as static ··· N unchanged lines ··· rows in hunk show and hunk diff.

Approach

  • Resolve single-revision JJ reviews to immutable commit IDs before generating the patch, so lazy source reads cannot race a moving @ or bookmark.
  • Load complete files with a bounded, path-safe jj file show --ignore-working-copy reader and expose it through the existing VCS extension contract.
  • Preserve rename, addition, deletion, source-cache, and custom-executable behavior.
  • Keep aggregate multi-revision patches reviewable without attaching a guessed source pair.
  • For merges, expose the exact new side used by gap expansion without pretending that any one parent is JJ's virtual merged-parent baseline.

The resulting JJ review uses the existing expandable row and z interaction:

Before: ··· 6 unchanged lines ···
After:  ▾ 6 unchanged lines

And here's some screenshot of it in action:

image image

Validation

  • bun test src/extensions/default/vcs/jujutsu/commands.test.ts src/extensions/default/vcs/jujutsu/source.test.ts src/extensions/default/vcs/jujutsu/index.test.ts — 24 passed
  • bun run test — 1627 passed, 3 skipped
  • bun run typecheck
  • bun run format:check
  • bun run lint
  • bun run deps:check
  • git diff --check
  • Manually verified hunk show mq in a pure JJ workspace: the previously static gap expanded and collapsed with z.

Tested on NixOS/Linux with JJ 0.41.0. macOS and Windows were not tested manually.

@vercel

vercel Bot commented Aug 28, 2026

Copy link
Copy Markdown

@samuela is attempting to deploy a commit to the Modem Team on Vercel.

A member of the Team first needs to authorize it.

@samuela
samuela marked this pull request as ready for review August 28, 2026 03:23
@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@benvinegar

Copy link
Copy Markdown
Member

Awesome thank you!

Comment thread src/extensions/default/vcs/jujutsu/index.ts Outdated
Comment thread src/extensions/default/vcs/jujutsu/commands.ts Outdated
Comment thread src/extensions/default/vcs/jujutsu/source.ts
@samuela
samuela force-pushed the feat/jj-gap-expansion branch 3 times, most recently from 1743fe4 to b9d9005 Compare August 28, 2026 19:28
@samuela

samuela commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@benvinegar thanks for the review, i believe i have addressed all your comments, lmkwyt

@benvinegar benvinegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the three earlier threads are addressed. I found two additional user-config interactions that still make the new “exact source” path unsafe, plus the Windows CI failure:

  1. commands.ts:23,228-259: use an alias-insensitive commit-ID template. JJ template aliases can shadow the bare commit_id keyword. With a valid template-aliases.commit_id that emits an older full commit ID, hunk show @ pins the patch, source reader, and cache key to that older commit. I reproduced the adapter showing an old add-file patch instead of the current @ diff. self.commit_id() bypassed the alias in the same repository.

  2. source.ts:45-47,74-86: neutralize fileset and file_show template customization. root-file:<value> is itself shadowable through fileset-aliases; I reproduced root-file:"a.txt" selecting both a.txt and b.txt. Separately, jj file show defaults to user-configured templates.file_show, which can prepend metadata to the source. Together, the reader returned PREFIX:a.txt\none\nPREFIX:b.txt\nbee\n as one file's attested source. A bare JSON-quoted path from repoRoot plus an explicit empty -T template (-T '""') avoided both customizations in my local JJ 0.41.0 test. Please add regression coverage for both configurations.

  3. source.test.ts:66: make the integration filename Windows-valid. The double quote in -note [exact] "file".txt is illegal on Windows, and the Windows compatibility job is currently red with ENOENT at the following writeFileSync. The quote case can stay in the mocked argv test; the real filesystem fixture should use only cross-platform-valid metacharacters.

The focused source tests, typecheck, lint, dependency check, and tracked-file format check otherwise pass locally. I also exercised the real branch TUI under tmux against local JJ repositories: z expanded the omitted source correctly, and a nested-cwd show @ -- file.txt selected only sub/file.txt and expanded its source.

This comment was generated by Pi using OpenAI GPT-5.6-sol

@samuela
samuela force-pushed the feat/jj-gap-expansion branch from b9d9005 to 3359e45 Compare August 29, 2026 01:46
@samuela
samuela force-pushed the feat/jj-gap-expansion branch from 3359e45 to 9d999ab Compare August 29, 2026 01:51
@samuela

samuela commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

done in 9d999ab

@benvinegar benvinegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requested fixes are addressed, including the Windows fileset regression, and the full CI matrix is green.

This comment was generated by Pi using OpenAI GPT-5.6-sol

@benvinegar
benvinegar merged commit 5012b2f into modem-dev:main Aug 30, 2026
11 of 12 checks passed
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.

2 participants