Skip to content

feat(app): copy full file contents from review - #59785

Open
lancy69 wants to merge 5 commits into
anomalyco:devfrom
lancy69:review-copy-content
Open

feat(app): copy full file contents from review#59785
lancy69 wants to merge 5 commits into
anomalyco:devfrom
lancy69:review-copy-content

Conversation

@lancy69

@lancy69 lancy69 commented Oct 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Issue for this PR

Closes #58878

Design approval requested on the issue; still pending. Implementation prepared with OpenAI Codex assistance.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Copy complete file text from the review header rather than diff hunks. Provide success/error feedback and leave the clipboard unchanged for unavailable or binary content. Available reviewed snapshots take precedence over current disk contents, including empty snapshots. When no snapshot is supplied, non-deleted files fall back to a fresh disk read. Deleted files use complete old text when available.

Updated against current upstream dev; release-managed changelog remains unchanged. Uses the current shared clipboard helper, prefers diff.after snapshots (including empty files), and keeps the simplified first E2E test without replacing navigator.clipboard. Removed the feature-specific README section requested in the latest review.

How did you verify your code works?

Latest update: 844be2752c, merging upstream dev 06aa6da22e.

  • Removed the feature-specific README section requested in the latest review.
  • Followed upstream's clipboard helper move to components/copy, retaining the new review/comment controller and placeholder behavior.
  • bun run typecheck:tsc in packages/session-ui and packages/gui-plugins: passed.
  • Both clipboard component regressions passed with 2 workers and no retries.
  • All 3 production-built app clipboard E2E regressions passed with full parallelism, 3 workers, and no retries.
  • Direct Chromium inspection of the production preview component confirmed that copying uses the reviewed snapshot rather than later disk text, and an empty snapshot copies an empty string without a reader.

The installed Bun 1.4.3 cannot run the new --no-script typecheck flag, so focused typechecks used the repository's pinned official compiler. The mandatory pre-push bun run check then passed with Bun 1.4.4: lint completed (unrelated warnings remain) and 34/34 typecheck tasks succeeded. App builds still emit upstream CSS/chunk-size/deprecation warnings.

Screenshots / recordings

Before: upstream preview component from f29e2813 mounted in the merged app through an isolated Vite override, not a complete historical app build. Both before and after use the in-memory E2E API harness.

Before: review header without copy action
After: copy action and success feedback
After: forced RTL

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Visual

<svg xmlns="http://www.w3.org/2000/svg" width="240" height="72" viewBox="0 0 240 72">
  <g fill="none" stroke="currentColor" stroke-width="1.5">
    <rect x="8" y="8" width="224" height="56" rx="6" opacity=".3"/>
    <path d="M20 25h135M20 38h112M20 51h145" opacity=".5"/>
    <rect x="197" y="22" width="14" height="18" rx="2"/>
    <path d="M202 18h14v18" opacity=".5"/>
  </g>
  <path d="m196 51 5 5 10-11" fill="none" stroke="currentColor" stroke-width="2">
    <animate attributeName="opacity" values="1;.25;1" dur="2s" repeatCount="indefinite"/>
  </path>
</svg>

@lancy69
lancy69 marked this pull request as ready for review October 30, 2026 07:12
@lancy69

lancy69 commented Oct 30, 2026

Copy link
Copy Markdown
Author

Merged current dev and resolved conflicts in d3db361; adopted the shared clipboard helper. Local check and all clipboard regressions pass; before/after evidence is in the description. Three other review-suite failures reproduced at the same assertions on unmodified upstream f29e281, so they are documented rather than changed here. Could a maintainer approve the full CI workflow runs? Design approval requested in #58878 remains pending.

opencodejr[bot]
opencodejr Bot reviewed Oct 30, 2026
View reviewed changes

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works as described. On the base branch the review file header has no copy action. With this PR, the new button copied the whole current file (not just the hunk) and showed the success toast. The three new e2e tests pass, and session-ui and gui-plugins type-check. The image and audio previews that share readFile still get binary content, so the switch to returning text for text-like files doesn't affect them.

Public API: unchanged (UI-only, plus four new English i18n keys).

Points worth addressing, details inline:

  • Please drop the CHANGELOG.md entry. Only the release automation writes that file.
  • The button always copies the file as it is on disk now, even in the "turn" and "branch" review modes, where that can differ from the version the panel shows.
  • The first e2e test is harder to follow than it needs to be.

Before

After

Comment thread CHANGELOG.md Resolved
Comment thread packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx Resolved
Comment thread packages/app/e2e/regression/review.spec.ts Resolved
opencodejr[bot]
opencodejr Bot reviewed Nov 2, 2026
View reviewed changes

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The CHANGELOG.md entry is gone, so that point is addressed. Otherwise the push only merges dev: the feature code and tests are unchanged since the last review. On the new head, the three new e2e tests still pass and session-ui and gui-plugins type-check.

Public API: unchanged.

Two earlier points are still open in their threads:

  • The button copies the file as it is on disk now in the "turn" and "branch" modes.
  • The first e2e test still swaps navigator.clipboard out and back in.

opencodejr[bot]
opencodejr Bot reviewed Nov 2, 2026
View reviewed changes

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This commit fixes both earlier points. The copy button now uses the version of the file being reviewed when the diff includes it, even when that file is empty, and only reads from disk otherwise. The first e2e test no longer swaps navigator.clipboard in and out. On this head, the new component tests (2) and the review copy e2e tests (3) pass, and session-ui type-checks.

Public API: unchanged.

One new point: the paragraph added to packages/app/README.md isn't needed (see inline).

Comment thread packages/app/README.md Resolved
opencodejr[bot]
opencodejr Bot reviewed Nov 10, 2026
View reviewed changes

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README section is gone, so that point is resolved. After the dev merge, the copy code only changed where it imports the clipboard helper from (components/copy). It is independent of the new loading placeholder: for changed files the button still copies the reviewed version or reads the file from disk, and for deleted files with only a partial diff it stays disabled. On this commit, the two component copy tests and the three review e2e copy tests pass, and session-ui and gui-plugins type-check.

Public API: unchanged.

No further concerns.

@ESwordCn ESwordCn mentioned this pull request Nov 11, 2026
Open
1 task done

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

Reviewers

@opencodejr opencodejr[bot] opencodejr[bot] left review comments
@Hona Hona Awaiting requested review from Hona
@Brendonovich Brendonovich Awaiting requested review from Brendonovich

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE]: web review-panel copy content button

1 participant

@lancy69