Skip to content

refactor(app): extract session message comments#672

Merged
Astro-Han merged 4 commits into
devfrom
codex/message-comment-strip-contract
May 16, 2026
Merged

refactor(app): extract session message comments#672
Astro-Han merged 4 commits into
devfrom
codex/message-comment-strip-contract

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 #671 was squash merged.
Dependency status: true stack dependency on the message-timeline extraction context from #670/#671; this PR has been restacked to the latest dev.
Review order: review and merge after #671, before #674.
Latest head after restack and review follow-up: 4ec5e4cee.

Summary

Extracted the per-message comment parsing and comment strip rendering from MessageTimeline into session-message-comments.tsx, with focused tests for metadata parsing, fallback note parsing, filtering, and equality.

Why

This is a #601 message-flow slice in the frontend architecture governance stack. It reduces MessageTimeline ownership by moving the comment strip into a named helper boundary without changing turn rendering, scroll behavior, controller state, styling, or public package contracts.

Related Issue

Refs #601
Depends on #667, #669, #670, and #671, all now merged into dev.
Stack child: #674.

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

  • Whether SessionMessageComments preserves the previous comment strip DOM/content contract.
  • Whether extractMessageComments keeps the same synthetic metadata and formatted-note fallback behavior.
  • Whether this stays inside the [Task] UI rewrite v2 Area A: message flow modular rollout #601 message-flow boundary and avoids scroll, turn-change, or visual redesign work.

Risk Notes

Low. This is a behavior-preserving extraction from MessageTimeline. It touches a visible message-flow surface but does not intentionally change UI copy, layout, scroll behavior, persistence, IPC, platform behavior, or public exports. Review follow-ups only swap the comment strip card background to the registered bg-surface-base token and replace old text-13-* utilities with role typography.

e2e/app/session.spec.ts partially passed in earlier validation: the first two session smoke tests passed, and the width-contract test failed on an existing selector drift looking for [data-dock-surface="shell"], which is not present in the current composer source. The page snapshot still showed the timeline and composer loading surface rendered.

Review Follow-up

  • Gemini token thread fixed in 795a236e0 and resolved. The comment strip card now uses bg-surface-base instead of bg-bg-base.
  • Fresh-eyes/CI typography guard finding fixed in 4ec5e4cee; comment strip now uses role typography (text-body, font-emphasis) instead of old text-13-* utilities.

How To Verify

Focused tests: bun test --preload ./happydom.ts src/pages/session/session-message-comments.test.ts src/pages/session/session-timeline-staging.test.ts src/pages/session/session-timeline-scroll-intents.test.ts -> 16 pass, 0 fail
UI guard: bun test test/no-dead-tokens.test.ts from packages/ui -> 3 pass, 0 fail
Typecheck: bun run typecheck -> 8 successful, 8 total
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
GitHub required checks: rerunning after 4ec5e4cee

Screenshots or Recordings

Not applicable. This is a behavior-preserving extraction; no intentional visual change.

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

@Astro-Han Astro-Han added P2 Medium priority app Application behavior and product flows ui Design system and user interface task Maintainer or agent execution task tech-debt Internal cleanup and maintainability debt labels May 16, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

Warning

Rate limit exceeded

@Astro-Han has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 54 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ee3b90ed-72ea-4df2-a1a6-f8ded5f32e88

📥 Commits

Reviewing files that changed from the base of the PR and between 6b83048 and 4ec5e4c.

📒 Files selected for processing (3)
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/app/src/pages/session/session-message-comments.test.ts
  • packages/app/src/pages/session/session-message-comments.tsx
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/message-comment-strip-contract

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.

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 message comment parsing and rendering logic by extracting it from the MessageTimeline component into a new dedicated helper file, session-message-comments.tsx. The changes introduce the SessionMessageComments component, utility functions for comment extraction and equality checks, and a new test suite. A review comment identifies the use of an unregistered Tailwind color token, bg-bg-base, and suggests replacing it with bg-surface-base to ensure consistent styling across the repository.

Comment thread packages/app/src/pages/session/session-message-comments.tsx Outdated
@Astro-Han Astro-Han force-pushed the codex/timeline-scroll-intent-contract branch from 14265e0 to 1692f00 Compare May 16, 2026 05:51
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 16, 2026

Perf delta summary

Comparator: fail

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) 72 -> 72 (0) 60 -> 61 (+1) 10 -> 11 (+1) 33.3 -> 33.3 (0) 149.9 -> 100 (-49.9) 3 -> 3 (0) 0 -> 0 (0) pass
default / long-session-input-lag 48 -> 48 (0) 48 -> 48 (0) 0 -> 0 (0) 0 -> 0 (0) 16.8 -> 16.8 (0) 16.8 -> 16.8 (0) 0 -> 0 (0) 0 -> 0 (0) pass
default / session-streaming-long 40 -> 40 (0) 64 -> 64 (0) 0 -> 0 (0) 0 -> 0 (0) 16.8 -> 16.8 (0) 16.8 -> 16.8 (0) 0 -> 0 (0) 0 -> 0 (0) pass
default / tool-call-expand 16 -> 24 (+8) 24 -> 24 (0) 0 -> 0 (0) 0 -> 0 (0) 16.7 -> 16.7 (0) 16.7 -> 16.7 (0) 0 -> 0 (0) 0 -> 0 (0) pass
default / tool-default-open-heavy-bash 32 -> 32 (0) 32 -> 40 (+8) 64 -> 66 (+2) 14 -> 16 (+2) 16.8 -> 50 (+33.2) 166.6 -> 116.6 (-50) 1 -> 2 (+1) 0 -> 0 (0) fail: frame_gap_p95_ms
default / terminal-side-panel-open 56 -> 56 (0) 64 -> 56 (-8) 0 -> 0 (0) 0 -> 0 (0) 33.4 -> 33.3 (-0.1) 33.4 -> 33.3 (-0.1) 0 -> 0 (0) 0 -> 0 (0) pass
default / session-scroll-reading 32 -> 32 (0) 40 -> 32 (-8) 0 -> 0 (0) 0 -> 0 (0) 16.8 -> 16.7 (-0.1) 16.8 -> 16.7 (-0.1) 0 -> 0 (0) 0.505 -> 0.505 (0) warn: cls
low-end / session-scroll-reading-long 64 -> 56 (-8) 80 -> 56 (-24) 59 -> 66 (+7) 16 -> 18 (+2) 16.8 -> 16.8 (0) 166.7 -> 166.6 (-0.1) 3 -> 2 (-1) 0.011 -> 0.011 (0) pass
low-end / session-timeline-recompute 112 -> 112 (0) 136 -> 120 (-16) 100 -> 96 (-4) 151 -> 142 (-9) 83.3 -> 83.4 (+0.1) 166.6 -> 166.8 (+0.2) 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/message-comment-strip-contract branch from e1d2673 to d8088f8 Compare May 16, 2026 13:26
@Astro-Han Astro-Han changed the base branch from codex/timeline-scroll-intent-contract to dev May 16, 2026 13:26
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 user-path files (packages/app/src/pages/session/message-timeline.tsx, packages/app/src/pages/session/session-message-comments.test.ts, packages/app/src/pages/session/session-message-comments.tsx)).

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 merged commit 4dfea3f into dev May 16, 2026
28 checks passed
@Astro-Han Astro-Han deleted the codex/message-comment-strip-contract 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

app Application behavior and product flows P2 Medium priority task Maintainer or agent execution task 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