Skip to content

refactor(chat): move ChatPage message-content helpers to their own module - #8978

Merged
iamwhatever merged 1 commit into
mainfrom
refactor/chat-page-message-content
Sep 6, 2026
Merged

refactor(chat): move ChatPage message-content helpers to their own module#8978
iamwhatever merged 1 commit into
mainfrom
refactor/chat-page-message-content

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Stack: 1 of 4 (bottom). This is the base of the ChatPage split stack: #9072 (session controller) targets this branch, #9078 (resources controller) targets #9072's branch, #9079 (pre-virtualizer transcript state) targets #9078's branch. Merge order 1 (#8978) → 2 (#9072) → 3 (#9078) → 4 (#9079). Each later PR targets the previous PR's branch, so read every diff relative to its own base.

Problem / Motivation

website/src/pages/ChatPage.tsx is a 10,169-line page. Its header menu, row-key helpers, mintSendId, the knowledge chip and the user-bubble content renderers sit at the top of that file even though none of them read page state. Finding them means scrolling through a monolith.

Why it matters

The whole-page split in #7255 (92 files, +12.6k/-8.7k) grew past the point where CI's Opus review lane can finish one pass (it fails closed above 15k reviewable lines), and it collided with main's chat-core transport work. Cutting the split into slices that each fit one review pass is the only way that refactor lands. This is the first slice: the part with zero dependency on the send path.

What changed (motivation → approach → change)

The moved code is pure: components and functions that take their inputs as arguments. So it can leave the page without changing who calls it.

ChatPage.tsx lines 387–1117 move byte-for-byte into pages/chat/ChatPageMessageContent.tsx. The only edits to the moved text are three added export keywords (mintSendId, msgIdentityKey, KnowledgeBubbleChip) so the page can import them back. NO_AUTO_DENIED, which sat between two moved blocks, stays in the page because only the page reads it.

The page imports the eleven names it still uses from the new module and drops the imports that only the moved code needed. It does not re-export the moved names: every consumer is a test in this repo, so each of the 13 tests now imports pages/chat/ChatPageMessageContent directly. The feature map's Sessions row names the new module the same way it already names TranscriptScrollShell.tsx.

main's recent DirChip change (the useRevealFailure + ErrorNotice reveal-failure notice) moves with the code it belongs to.

Tests

  • website/src/test/ChatPageMessageContent.test.ts (new): pins mintSendId() to the legacy s-<ts36>-<nonce36> wire format with mocked Date.now and Math.random.
  • 13 existing tests re-point their import to the owning module; their assertions are unchanged.
  • Full website vitest suite with coverage: see the run summary in the first comment.
  • tsc -b, eslint (0 warnings), jscpd (0 clones), i18n:check, lint:phantom-classes, check_feature_map.py, check_brand_name.py, docs-lint.sh all exit 0.

Manual verification

N/A — this is a byte-preserving move of pure functions and components; the existing renderUserContent / ChatHeaderMenu / row-key tests exercise every moved export.

Screenshots / video

Why no screenshot: pure move of functions and components; nothing renders differently.

Related Issues

Part of the split of #7255. no linked issue: the parent PR has no tracking issue.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@bolichen97
bolichen97 requested a review from a team September 6, 2026 09:55
@bolichen97
bolichen97 requested a review from a team as a code owner September 6, 2026 09:56
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Local gate run for 299601bd1 (rebased on d4c2cbf22):

  • Full website vitest suite with coverage: 1876 files, 29473 passed, 1 expected fail, 2 skipped, exit 0.
  • Project frontend line-rate 92.49% (floor 90).
  • scripts/check_per_file_coverage.py (floor 80, frontend baseline): exit 0. src/pages/ChatPage.tsx 80.60%, src/pages/chat/ChatPageMessageContent.tsx 90.31% (de-duplicated line counts, the gate's own measure).
  • tsc -b: 0 errors outside src/stories/ (the storybook type errors are environment-only — the same 15 appear on a clean main checkout without the storybook packages installed).
  • eslint 0 warnings; jscpd 0 clones; i18n:check PASS; lint:phantom-classes, check_feature_map.py (FEATURE_MAP_BASE_REF=origin/main), check_brand_name.py, docs-lint.sh all exit 0.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of c5e170e005108dd7bac3feb0e9220fd516baeb35 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. The three newly exported symbols each have a real consumer inside ChatPage.tsx (grep: mintSendId ×2 call sites, msgIdentityKey ×3, KnowledgeBubbleChip ×1 in the composer's knowledge chip), no non-test file imports the moved names from the old path, and the feature-map edit is forced by scripts/check_feature_map.py's new-page-file trigger.

First-Principles-Verdict: PASS

A byte-preserving slice of the ChatPage monolith, motivated by a measured CI limit, with every new export consumed and every side edit mandated or declared.

What this change ships

Intent: land the ChatPage split (#7255) in review-sized slices by moving its pure message-content helpers into their own module — a refactor (a MOVE, not a capability change).

  1. ~730 lines (header menu, row-key helpers, user-bubble renderers) move byte-for-byte to pages/chat/ChatPageMessageContent.tsx — justified: derived from the stated review-lane cap (15k reviewable lines) that killed the 92-file parent PR.
  2. mintSendId, msgIdentityKey, KnowledgeBubbleChip become exported — justified: required by the move; each has a counted real consumer in ChatPage.tsx (2, 3, and 1 use sites respectively).
  3. Old import path pages/ChatPage stops exporting the moved names; 13 tests re-point — justified: no re-export layer added, and grep shows zero non-test consumers of the old named imports.
  4. New 14-line test pins mintSendId's s-<ts36>-<nonce36> wire format — declared; guards a server-echoed correlation-id shape (fix(dashboard): stop reporting other products' memory as this gateway's #2845/Stranded thinking chip when a steer races chat_done onto the new-turn path #6075) at the moment the symbol becomes independently importable.
  5. Feature-map Sessions row names the new module — justified: mandated by check_feature_map.py, which fails on a new file under the pages tree.

Nothing is undeclared: the description accounts for NO_AUTO_DENIED staying, the DirChip reveal-failure code travelling with its owner, and the no-re-export decision. No item has a zero-cost zero option, no existing mechanism does this job, and no surface ships without a counted consumer.

[FIRST-PRINCIPLES-REVIEWED] c5e170e

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of c5e170e005108dd7bac3feb0e9220fd516baeb35 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change verifies as described: ~730 lines of self-contained helpers moved from the 10k-line ChatPage into pages/chat/ChatPageMessageContent.tsx, page re-imports eleven names, thirteen tests re-point directly, feature map updated in the same commit, no dangling consumers of the moved names remain, and the move is trivially reversible.

Design-Verdict: PASS

A byte-preserving, review-sized slice of a tracked monolith split, at the established pages/chat/ seam, with all consumers re-pointed and no new surface.

Suggestions

  • The module is grouped by "doesn't touch the send path," not by theme — ChatHeaderMenu and mintSendId are not message content; when later refactor(chat): split chat page controllers #7255 slices land, re-home or rename before the feature-map entry cements this as the permanent home.

[DESIGN-REVIEWED] c5e170e

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed c5e170e005108dd7bac3feb0e9220fd516baeb35 — this comment is updated in place on each push.

Review details

All moved symbols used in ChatPage.tsx are imported from the new module; the removed imports have no remaining code references. The extraction is verbatim and the consumers/tests are repointed correctly. Nothing groundable to a reachable defect.

No findings.

[OPUS-REVIEWED] c5e170e

Verdict parsed from the review's SHA-scoped output markers for commit c5e170e005108dd7bac3feb0e9220fd516baeb35.

False positive or not applicable? A repository writer can comment:
/ai-review override fable c5e170e005108dd7bac3feb0e9220fd516baeb35: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of c5e170e005108dd7bac3feb0e9220fd516baeb35 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c5e170e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt c5e170e005108dd7bac3feb0e9220fd516baeb35: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of c5e170e005108dd7bac3feb0e9220fd516baeb35 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The check is clean: every unbalanced literal is an import path (../../../), double-quoted literals and JSX strings balance exactly, and the only lines added back to ChatPage.tsx are the import block. This is a genuine byte-preserving move of pure helpers with no user-visible surface touched — the PR's <!-- no-visual-delta --> claim holds, so the absent blind read and screenshots leave no evidence gap.

UX-Verdict: PASS

Pure code move — no string, control, state, or pixel changes; nothing a user could experience differently.

[UX-REVIEWED] c5e170e

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Sep 6, 2026
…dule

ChatPage.tsx is a 10k-line page. The header menu, the row-key helpers
(msgIdentityKey, turnLeadKey, anchorAltIdFor, stableAnchorIdFor,
virtualKeyFor, uniqueRowKeys, messageRowKey), mintSendId, the knowledge
bubble chip and the user-bubble content renderers (renderUserContent and
its file/dir segment helpers) do not read page state. They move byte-for-
byte into pages/chat/ChatPageMessageContent.tsx; the page imports them
back. No behaviour, string, or wire format changes.

The page no longer re-exports the moved names. Every consumer of those
names is a test in this repository, so each one now imports the owning
module directly. The one remaining default export and the two pre-
existing re-exports (isBrowseCommand, PREFILL_STORAGE_KEY) are untouched.

First slice of the ChatPage split proposed in #7255, cut small enough
for one review pass and re-based on main's chat-core transport work.

Co-authored-by: Kiro Crew <noreply@kiro.dev>
@bolichen97
bolichen97 force-pushed the refactor/chat-page-message-content branch from 299601b to c5e170e Compare September 6, 2026 16:42
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Rebased onto d7fb9c55e (42 commits; the only conflict was the feature-map Sessions row, which main had reworded — re-applied on top of main's text). main did not touch the moved region (ChatPage.tsx 387–1117), so the extraction is unchanged; head is now c5e170e00.

Local gates on the new head: full website vitest suite with coverage 1886 files, 29703 passed, 1 expected fail, 2 skipped, exit 0; frontend line-rate 92.54%; check_per_file_coverage.py exit 0 (ChatPage.tsx 80.60%, ChatPageMessageContent.tsx 90.31%); tsc -b 0 errors outside src/stories/; eslint 0 warnings.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 6, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@iamwhatever
iamwhatever merged commit 4aa07f9 into main Sep 6, 2026
106 of 108 checks passed
@iamwhatever
iamwhatever deleted the refactor/chat-page-message-content branch September 6, 2026 18:16
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 6, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

The module is grouped by "doesn't touch the send path," not by theme — ChatHeaderMenu and mintSendId are not message contentaccepted; re-homed by the slices that own them, not deferred to nowhere.

when later #7255 slices land, re-home or rename before the feature-map entry cements this as the permanent home.

Agreed on the diagnosis. The two symbols have owners further up the stack, and each moves when its owner lands, so nothing is left to a rename later:

  • mintSendId is the optimistic-bubble correlation id minted by the send and steer paths. It moves into the actions controller slice (the one rebuilt on sendTurn), which is the only production consumer once the page's send()/steer leave ChatPage.tsx.
  • ChatHeaderMenu is view chrome. It moves into the ChatPageView slice, next to the header it renders in.

Each of those PRs updates the feature-map Sessions row for the symbol it takes, so the entry added here is transitional, not cemented. Tracked in the stack plan on #7255 (comment linked from each stack PR); the current stack (#9072#9078#9079) does not touch either symbol, so the re-homing is not in scope there.

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