Skip to content

feat: file context expansion + collapse/expand nav button#33

Merged
dastratakos merged 12 commits into
mainfrom
dastratakos/file-context-expansion
May 5, 2026
Merged

feat: file context expansion + collapse/expand nav button#33
dastratakos merged 12 commits into
mainfrom
dastratakos/file-context-expansion

Conversation

@dastratakos
Copy link
Copy Markdown
Contributor

@dastratakos dastratakos commented May 5, 2026

Summary

Enable expandable context lines in diff views by bundling full file contents alongside the patch in the diff API response. Also adds a collapse/expand all files button to the sticky tab nav bar, matching hosted stage's styling.

Changes

  • New DiffResponse type (packages/types/src/diff.ts) bundles patch + per-file old/new content
  • Diff route rewritten from streaming plain text to JSON response with file contents fetched via git show
  • Frontend enriches FileDiffMetadata with full file arrays and remaps hunk indices so Pierre's expandUnchanged works correctly
  • Collapse/expand all files button in sticky nav via a context that child pages register into
  • Nav bar matches hosted stage styling: solid background, responsive text labels at @7xl, line counts at @5xl

Testing

  • All 149 tests pass (including updated + new diff route tests for JSON response, added/deleted file edge cases)
  • Typecheck and lint clean
  • Manually tested with stage-cli show — context expansion works on modified files, button toggles collapse state on both Files and Chapter Detail pages

Open in Stage

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request transitions the diff API from streaming plain text to returning a structured JSON response containing both the patch and full file contents, enabling context expansion and accurate line indexing in the web UI. Frontend updates include a new CollapseActionsProvider for managing file visibility, addition/deletion counts in the layout, and a "Collapse/Expand all" feature. Feedback focuses on improving the robustness of the file path parsing regex, increasing the buffer size for fetching file contents, and addressing potential scalability issues related to memory consumption and process concurrency when handling large diffs.

Comment thread packages/cli/src/routes/diff.ts
Comment thread packages/cli/src/routes/diff.ts Outdated
Comment thread packages/cli/src/routes/diff.ts Outdated
Comment thread packages/cli/src/routes/diff.ts
@dastratakos dastratakos force-pushed the dastratakos/file-context-expansion branch from a4eb483 to ff97243 Compare May 5, 2026 02:21
Comment thread packages/web/src/routes/pull-request-layout.tsx
Comment thread packages/web/src/lib/split-with-newlines.ts
Change the diff endpoint from streaming plain text to returning JSON
with both the patch and full file contents, enabling Pierre's
expandUnchanged feature for context line expansion in diff views.
…ntent

Pierre's hunk metadata has deletionLineIndex/additionLineIndex pointing
into the partial (patch-only) line arrays. When switching to full-file
arrays (isPartial=false), remap these indices to the actual file line
positions derived from the hunk's deletionStart/additionStart headers.
Add a button in the sticky tab navigation that toggles collapse/expand
for all file diffs. Visible on both the Files Changed and Chapter Detail
pages via a context that child pages register their collapse state into.
The provider must wrap both the nav (where the button reads context)
and the Outlet (where pages register their collapse state).
…ve text

- Remove backdrop-blur and opacity from sticky nav (use solid bg-background)
- Add @container to layout wrapper for container query breakpoints
- Add total +additions/-deletions display (visible at @5XL)
- Collapse/expand and Display buttons show text labels at @7XL
- Right action area uses responsive gap (@XL:gap-6)
The collapse/expand all button compared collapsed files against the
total diff file count, but on chapter pages the collapse state only
tracks that chapter's files. Include fileCount in the context
registration so each page provides its own count.
- Replace manual spawn+buffer with execFileAsync for collecting git diff
- Merge getOldRef/getNewRef into single getContentRefs returning both
- Extract fetchContent helper to eliminate nested ternary
- Extract MAX_FILE_BYTES constant
@dastratakos dastratakos force-pushed the dastratakos/file-context-expansion branch from c94fd0f to 34418e8 Compare May 5, 2026 03:04
Comment thread packages/web/src/lib/parse-diff.ts
Pierre requires all hunks to be present when isPartial=false (trailing
context must match on both sides). Chapter pages show a subset of hunks,
so enrichment is only correct on the Files page where the full per-file
diff is used.
Comment thread packages/cli/src/routes/diff.ts
Port the hosted stage's approach: apply non-chapter hunks to the old
file content to produce an intermediate, then diff intermediate vs new
using parseDiffFromFile. This produces a clean isPartial=false diff
showing only the chapter's changes while supporting full context
expansion.
Comment thread packages/web/src/lib/filter-files-for-chapter.ts Outdated
Comment thread packages/web/src/lib/filter-files-for-chapter.ts
- Use original @@ header line in fallback path instead of reconstructing
  with hardcoded +0,0
- Strip trailing empty strings from parsed hunk lines (split artifact)
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c54519e. Configure here.

Comment thread packages/web/src/lib/filter-files-for-chapter.ts
@dastratakos dastratakos marked this pull request as ready for review May 5, 2026 05:06
@dastratakos dastratakos merged commit 18a1850 into main May 5, 2026
5 checks passed
@dastratakos dastratakos deleted the dastratakos/file-context-expansion branch May 5, 2026 05:06
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c54519edf1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +125 to +129
const resolved = path.resolve(repoRoot, filePath);
const rel = path.relative(repoRoot, resolved);
if (rel.startsWith("..") || path.isAbsolute(rel)) return null;
try {
return await fs.readFile(resolved, "utf8");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep file-content reads inside repo after symlink resolution

readFileContent checks the lexical path with path.resolve/path.relative, but then reads through fs.readFile, which follows symlinks. In working-tree runs, a changed symlink inside the repo (for example, foo -> /etc/passwd) passes this check and causes /diff.patch to return data from outside repoRoot in fileContents. This introduces a local file disclosure path that did not exist before; resolve and validate the real path (or refuse symlinks) before reading.

Useful? React with 👍 / 👎.

Comment on lines +168 to +176
const entries = await Promise.all(
files.map(async ({ oldPath, newPath, isBinary }) => {
const key = newPath ?? oldPath;
if (!key || isBinary) return null;

const [oldContent, newContent] = await Promise.all([
oldPath ? fetchContent(repoRoot, oldRef, oldPath) : Promise.resolve(null),
newPath ? fetchContent(repoRoot, newRef, newPath) : Promise.resolve(null),
]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bound git-show fanout when building fileContents

buildFileContents uses nested Promise.all and can start up to two git show subprocesses per changed file at once. On large diffs, this unbounded fanout can exhaust process or file-descriptor limits (EAGAIN/EMFILE) and make the endpoint return 500 even though git diff succeeded. Use a bounded concurrency pool (or batching) for per-file content fetches.

Useful? React with 👍 / 👎.

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