Skip to content

refactor(ui): extract session turn diffs#677

Merged
Astro-Han merged 3 commits into
devfrom
codex/session-turn-diff-summary
May 16, 2026
Merged

refactor(ui): extract session turn diffs#677
Astro-Han merged 3 commits into
devfrom
codex/session-turn-diff-summary

Conversation

@Astro-Han
Copy link
Copy Markdown
Owner

@Astro-Han Astro-Han commented May 16, 2026

Topology Update (2026-05-16)

Final base: dev after #676 was squash merged.
Dependency status: true short stack dependency. #676 and #677 both modify packages/ui/src/components/session-turn.tsx; #677 is now restacked on dev after #676 landed.
Review order: final PR in the session-turn UI stack. This stack is independent from the message-timeline stack, whose lower PRs have already landed.
Latest head after fresh-eyes follow-up: c0fa4e99e.


Summary

Extracted the legacy SessionTurn diff summary accordion into packages/ui/src/components/session-turn-diffs.tsx and preserved the existing SessionTurn fallback wiring.

Why

This continues the #601 message-flow architecture stack after #676. The previous PR separated the turn-change panel but left the legacy diff summary inside session-turn.tsx. This PR moves that summary into its own owner component while keeping the fallback behavior unchanged.

Canonical manifest: .github/frontend-architecture-manifest.md

Related Issue

Human Review Status

Fresh-eyes review clean after c0fa4e99e. The first fresh-eyes pass found a P2 keyboard-focus visibility issue; c0fa4e99e fixed it, and the second fresh-eyes review found no P0-P3 issues.

Review Focus

  • Confirm the extracted diff summary preserves the existing accordion, show-all toggle, delayed diff mount, and data-slot names.
  • Confirm SessionTurn still owns assistant rendering, retry/thinking state, message lookup, and fallback gating.
  • Confirm the interactive show-all/more controls are accessible native buttons without visual drift.
  • Confirm stack base is correct: base is now dev after refactor(ui): extract session turn changes panel #676 was squash merged.

Risk Notes

Behavior is intended unchanged. Main risk is a legacy diff summary regression because the accordion state moved into a child component. No dependency, persistence, platform, or public export change.

Architecture Boundary

Owner lane: #601 message flow.

Base/depends on: #667 -> #669 -> #670 -> #671 -> #672 -> #674 -> #675 -> #676. This PR is now restacked on dev.

Touched files:

  • packages/ui/src/components/session-turn.tsx
  • packages/ui/src/components/session-turn-diffs.tsx
  • packages/ui/src/components/session-turn.css
  • packages/ui/src/components/session-turn-parent.test.ts

Architecture effect:

  • owner extracted
  • legacy diff fallback remains wired through SessionTurnDiffs
  • session-turn.tsx sheds the inline diff accordion code

Review Follow-up

  • Gemini accessibility threads fixed and resolved. The show-all and more controls are now button type="button", with existing data-slot styling preserved through button reset rules, and the i18n count is passed as a number.
  • Restack follow-up: after refactor(ui): extract session turn changes panel #676 was squash merged, the original refactor(ui): extract session turn diffs #677 commit only retained the new file; the follow-up restores the intended SessionTurn wiring so the component is actually used.
  • Fresh-eyes P2 fixed in c0fa4e99e: keyboard focus now reveals the show-all toggle with a visible outline, and the more button explicitly remains left-aligned.

How To Verify

Focused UI tests: bun test src/components/session-turn-turn-changes.test.ts src/components/session-turn-parent.test.ts -> 7 pass, 0 fail
bun run typecheck -> 8 successful, 8 total
bun run lint -> pass
Targeted #600 timeline perf: bun script/e2e-local.ts -- e2e/perf/perf-probe.spec.ts --grep "long-session-input-lag|session-timeline-recompute" -> 1 passed, 1 skipped
git diff --check origin/dev...HEAD && git diff --check -> pass
Review threads: all resolved
Fresh-eyes review: clean after c0fa4e99e
GitHub required checks: all pass except perf-probe-baseline

Current Blocker

perf-probe-baseline is not green on c0fa4e99e, so this PR is not mergeable yet.

Evidence:

  • First run 25964690195 / job 76325995214 failed after 30m7s. Confirmed compare failed on default:homepage-cold:frame_gap_max_ms_delta; diagnostic then failed in base checkout on low-end session-scroll-reading-long coverage.
  • One rerun was attempted per CI rule. Rerun job 76327884487 failed after 30m28s with different confirmed failures: initial compare default:terminal-side-panel-open:interaction_ms_median, confirmed compare default:session-streaming-long:* and default:tool-default-open-heavy-bash:frame_gap_max_ms_delta; diagnostic was cancelled during base low-end trace after timeout.
  • The failures are inconsistent across runs and include base diagnostic failures/cancellation, which points to perf-gate flake or infra noise rather than a stable refactor(ui): extract session turn diffs #677 regression. Because the required check remains failed, this PR should not be merged until the perf gate is green or the gate owner manually clears/reruns it.

Screenshots or Recordings

Not included. This PR is intended as a behavior-preserving component extraction with no visible copy change; Electron manual visual verification was not run.

Checklist

  • Human review status is stated above as pending, approved, or not required
  • I linked the related issue, or stated why there is no issue
  • This PR has type, primary area, and priority labels, or I requested maintainer labeling
  • I described the review focus and any meaningful risks
  • I listed the relevant verification steps and the key result for each
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant
  • I reviewed the final diff for unrelated changes and suspicious dependency changes
  • I am targeting dev after the merged stack base, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

Release Notes

  • Refactor

    • Reorganized diff rendering functionality for improved code organization and maintainability.
  • Style

    • Enhanced accessibility of diff controls with improved focus visibility states.
    • Refined button styling for cleaner diff toggle appearance.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fa60056d-e199-4b16-9cb2-f069c8adb412

📥 Commits

Reviewing files that changed from the base of the PR and between 3d35426 and 0cfe254.

📒 Files selected for processing (4)
  • packages/ui/src/components/session-turn-diffs.tsx
  • packages/ui/src/components/session-turn-parent.test.ts
  • packages/ui/src/components/session-turn.css
  • packages/ui/src/components/session-turn.tsx

📝 Walkthrough

Walkthrough

This PR extracts per-file diff accordion rendering from session-turn.tsx into a new reusable SessionTurnDiffs component. The refactoring removes local diff UI state, helpers, and imports from session-turn.tsx, replaces inline accordion rendering with a delegated component call, and adds button styling and focus accessibility improvements to the CSS.

Changes

Diff Rendering Extraction

Layer / File(s) Summary
SessionTurnDiffs component implementation
packages/ui/src/components/session-turn-diffs.tsx
New exported component that manages diff accordion state (showAll, expanded), computes edited count and overflow detection, renders header with toggle controls, and uses deferred Solid effects to conditionally render per-diff views after panels become active.
session-turn.tsx refactoring to use SessionTurnDiffs
packages/ui/src/components/session-turn.tsx
Removes diff-specific imports, the useFileComponent() call, and local diff UI state/helpers; replaces inline accordion rendering with a SessionTurnDiffs invocation, wiring onShowAllToggle callback to pause auto-scrolling.
Styling and test updates
packages/ui/src/components/session-turn.css, packages/ui/src/components/session-turn-parent.test.ts
Adds button appearance reset (appearance: none, border/background removal), padding reset, and :focus-visible outline styling for diffs toggle and more controls; updates test assertion from edited() > 0 to diffs().length > 0.

Possibly Related PRs

  • Astro-Han/pawwork#676: Concurrent refactoring of session-turn.tsx extracting the per-file turn-change panel into SessionTurnChangesPanel component alongside this PR's diff extraction.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A diff once lived inside the turn,
Now extracted clean, a lesson to learn,
With state and toggles, accordion bright,
SessionTurnDiffs renders just right!
New homes for old code—refactor's delight! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'refactor(ui): extract session turn diffs' clearly summarizes the main change—extracting the diff summary accordion into a new component.
Description check ✅ Passed The description comprehensively covers all template sections: Summary, Why, Related Issues, Human Review Status, Review Focus, Risk Notes, Architecture Boundary, Review Follow-up, How To Verify, and a detailed Checklist.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/session-turn-diff-summary

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Astro-Han Astro-Han added P2 Medium priority ui Design system and user interface app Application behavior and product flows tech-debt Internal cleanup and maintainability debt labels May 16, 2026
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 refactors the SessionTurn component by extracting the diff summary accordion into a new standalone component, SessionTurnDiffs. This change reduces the complexity of SessionTurn and updates the frontend architecture manifest to track the resolution of large files. Feedback focuses on improving accessibility by replacing generic <span> and <div> elements with native <button> elements for interactive toggles, as well as removing a redundant String() conversion in an i18n translation call.

Comment thread packages/ui/src/components/session-turn-diffs.tsx Outdated
Comment thread packages/ui/src/components/session-turn-diffs.tsx Outdated
@Astro-Han Astro-Han force-pushed the codex/session-turn-change-panel branch from 77d1b82 to f0958b4 Compare May 16, 2026 06:33
@Astro-Han Astro-Han force-pushed the codex/session-turn-diff-summary branch from b03c549 to 7b36fd1 Compare May 16, 2026 06:33
@Astro-Han Astro-Han changed the base branch from codex/session-turn-change-panel to dev May 16, 2026 06:34
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 16, 2026

Perf delta summary

Comparator: pass

Profile / Scenario interaction median interaction worst long task max tbt frame gap p95 frame gap max jank count cls status
default / homepage-cold 40 -> 32 (-8) 56 -> 48 (-8) 74 -> 67 (-7) 24 -> 17 (-7) 16.8 -> 16.8 (0) 133.3 -> 100 (-33.3) 3 -> 3 (0) 0 -> 0 (0) pass
default / long-session-input-lag 48 -> 40 (-8) 64 -> 48 (-16) 0 -> 0 (0) 0 -> 0 (0) 16.7 -> 16.7 (0) 16.8 -> 16.8 (0) 0 -> 0 (0) 0 -> 0 (0) pass
default / session-streaming-long 48 -> 48 (0) 72 -> 72 (0) 116 -> 0 (-116) 66 -> 0 (-66) 33.4 -> 16.8 (-16.6) 100.1 -> 33.4 (-66.7) 1 -> 0 (-1) 0 -> 0 (0) pass
default / tool-call-expand 16 -> 24 (+8) 24 -> 24 (0) 0 -> 0 (0) 0 -> 0 (0) 16.8 -> 16.7 (-0.1) 16.8 -> 16.7 (-0.1) 0 -> 0 (0) 0 -> 0 (0) pass
default / tool-default-open-heavy-bash 24 -> 24 (0) 24 -> 32 (+8) 68 -> 67 (-1) 18 -> 17 (-1) 50 -> 49.9 (-0.1) 100 -> 133.4 (+33.4) 3 -> 3 (0) 0 -> 0 (0) pass
default / terminal-side-panel-open 48 -> 56 (+8) 56 -> 56 (0) 0 -> 0 (0) 0 -> 0 (0) 33.3 -> 33.3 (0) 33.3 -> 33.3 (0) 0 -> 0 (0) 0 -> 0 (0) pass
default / session-scroll-reading 24 -> 16 (-8) 32 -> 24 (-8) 0 -> 0 (0) 0 -> 0 (0) 16.7 -> 16.7 (0) 16.7 -> 16.7 (0) 0 -> 0 (0) 0.505 -> 0.505 (0) warn: cls
low-end / session-scroll-reading-long 56 -> 56 (0) 64 -> 88 (+24) 70 -> 65 (-5) 29 -> 28 (-1) 16.8 -> 16.8 (0) 66.7 -> 200 (+133.3) 2 -> 3 (+1) 0.011 -> 0.011 (0) warn: frame_gap_max_ms_delta
low-end / session-timeline-recompute 120 -> 120 (0) 136 -> 128 (-8) 112 -> 110 (-2) 170 -> 179 (+9) 83.4 -> 83.4 (0) 183.3 -> 183.4 (+0.1) 3 -> 3 (0) 0.081 -> 0.081 (0) pass
low-end / concurrent-shimmer-extreme 0 -> 0 (0) 0 -> 0 (0) 0 -> 0 (0) 0 -> 0 (0) 16.8 -> 16.7 (-0.1) 16.8 -> 16.8 (0) 0 -> 0 (0) 0 -> 0 (0) pass

@Astro-Han Astro-Han force-pushed the codex/session-turn-diff-summary branch from 7b36fd1 to 7b309a6 Compare May 16, 2026 06:42
@github-actions github-actions Bot removed the app Application behavior and product flows label May 16, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@Astro-Han Astro-Han force-pushed the codex/session-turn-diff-summary branch from c0fa4e9 to 0cfe254 Compare May 16, 2026 15:50
@Astro-Han Astro-Han merged commit 656a58c into dev May 16, 2026
28 checks passed
@Astro-Han Astro-Han deleted the codex/session-turn-diff-summary branch May 16, 2026 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium priority tech-debt Internal cleanup and maintainability debt ui Design system and user interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant