Repository navigation
feat(wework): refine Codex session transcript display - #1572
Merged
qdaxb merged 2 commits intoJun 26, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds structured ChangesStructured blocks and chat rendering
Sequence Diagram(s)sequenceDiagram
participant ScrollableMessageArea
participant MessageList
participant AssistantMessage
participant ToolBlocksDisplay
participant ToolBlockItem
participant WebSearchSourcesChip
ScrollableMessageArea->>MessageList: conversationKey
MessageList->>AssistantMessage: message + conversationKey
AssistantMessage->>ToolBlocksDisplay: stateKey + processing blocks
ToolBlocksDisplay->>ToolBlockItem: file_changes / non-web-search blocks
AssistantMessage->>WebSearchSourcesChip: derived web-search sources
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)executor/src/runtime_work/transcript.rs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.14][ERROR]: unable to find a config; path executor/tests/app_runtime_work_file_changes_contract.rs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.21][ERROR]: unable to find a config; path packages/chat-core/src/index.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.17][ERROR]: unable to find a config; path
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/runtime_work/transcript.rs`:
- Around line 499-502: Preserve root-level fallback fields when handling the
websearchcall case in transcript.rs: the current branch in the match on the
action type returns the cloned action object as-is, which drops fallback fields
like query/url when action is partial. Update the logic in this branch to start
from the cloned action and merge in the root-level query and url values from the
item when they are missing, so callers like the UI still receive the fallback
data even if action only contains a partial payload.
In `@wework/src/components/chat/blocks/processingExpansionState.ts`:
- Around line 14-16: The `useEffect` in `processingExpansionState` is triggering
the `react-hooks/set-state-in-effect` lint rule and causing an extra render when
`key` changes. Replace the effect-driven `setExpanded(readExpansionState(key,
initialValue))` flow with the documented “adjust state while rendering” pattern
inside `useExpansionState`, so the state is synchronized during render instead
of after commit. Make sure the comparison is based on `key` (and include
`initialValue` too if it can vary in future callers) so the hook updates
correctly without the effect.
In `@wework/src/components/chat/blocks/toolBlockActivity.ts`:
- Around line 68-70: The special-case label in toolBlockActivity’s summary logic
is ignoring the group status and is incorrectly marking failed web-search groups
as completed. Update the isWebSearchActivityGroup(blocks) branch to only return
“已搜索网页” when the group has actually succeeded, otherwise fall through to the
existing failure-aware status summary path that respects block.status.
In `@wework/src/components/chat/blocks/webSearchActivity.ts`:
- Around line 178-180: The getFaviconUrl helper is constructing direct favicon
URLs for any transcript domain, which can trigger automatic third-party or
internal host requests when WebSearchFavicon loads them. Update this path so
webSearchActivity only returns icons for trusted/allowlisted domains or via a
safe proxy, and otherwise falls back to the generic icon unless the user
explicitly opts in. Keep the change localized around getFaviconUrl and the
WebSearchFavicon usage so arbitrary transcript domains are never fetched
directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95e25350-4014-4ce2-9727-0b0c4597a50c
📒 Files selected for processing (13)
executor/src/runtime_work/transcript.rsexecutor/tests/app_runtime_work_file_changes_contract.rswework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/ScrollableMessageArea.tsxwework/src/components/chat/blocks/ToolBlockItem.test.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/blocks/WebSearchSources.tsxwework/src/components/chat/blocks/processingExpansionState.tswework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/chat/blocks/webSearchActivity.ts
Comment on lines
+499
to
+502
| "websearchcall" => item | ||
| .get("action") | ||
| .cloned() | ||
| .unwrap_or_else(|| json!({"query": string_field(item, "query").unwrap_or_default()})), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve root-level fallback fields when action is partial.
Line 499 currently chooses the entire action object whenever it exists, so a payload like { action: { type: "search" }, query: "foo" } loses the query before it reaches the UI. Merge the fallback query/url fields into the cloned action object instead of treating action as all-or-nothing.
Proposed fix
- "websearchcall" => item
- .get("action")
- .cloned()
- .unwrap_or_else(|| json!({"query": string_field(item, "query").unwrap_or_default()})),
+ "websearchcall" => {
+ let mut action = item
+ .get("action")
+ .and_then(Value::as_object)
+ .cloned()
+ .unwrap_or_default();
+
+ if !action.contains_key("query") {
+ if let Some(query) = string_field(item, "query") {
+ action.insert("query".to_owned(), Value::String(query));
+ }
+ }
+ if !action.contains_key("url") {
+ if let Some(url) = string_field(item, "url") {
+ action.insert("url".to_owned(), Value::String(url));
+ }
+ }
+
+ Value::Object(action)
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "websearchcall" => item | |
| .get("action") | |
| .cloned() | |
| .unwrap_or_else(|| json!({"query": string_field(item, "query").unwrap_or_default()})), | |
| "websearchcall" => { | |
| let mut action = item | |
| .get("action") | |
| .and_then(Value::as_object) | |
| .cloned() | |
| .unwrap_or_default(); | |
| if !action.contains_key("query") { | |
| if let Some(query) = string_field(item, "query") { | |
| action.insert("query".to_owned(), Value::String(query)); | |
| } | |
| } | |
| if !action.contains_key("url") { | |
| if let Some(url) = string_field(item, "url") { | |
| action.insert("url".to_owned(), Value::String(url)); | |
| } | |
| } | |
| Value::Object(action) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/runtime_work/transcript.rs` around lines 499 - 502, Preserve
root-level fallback fields when handling the websearchcall case in
transcript.rs: the current branch in the match on the action type returns the
cloned action object as-is, which drops fallback fields like query/url when
action is partial. Update the logic in this branch to start from the cloned
action and merge in the root-level query and url values from the item when they
are missing, so callers like the UI still receive the fallback data even if
action only contains a partial payload.
Comment on lines
+68
to
+70
| if (isWebSearchActivityGroup(blocks)) { | ||
| return '已搜索网页' | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't summarize failed web-search groups as completed.
This shortcut ignores block.status, so an all-web_search group with only errors still collapses to 已搜索网页. Gate the special-case label on the group succeeding, or preserve the existing failure-aware summary path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/chat/blocks/toolBlockActivity.ts` around lines 68 - 70,
The special-case label in toolBlockActivity’s summary logic is ignoring the
group status and is incorrectly marking failed web-search groups as completed.
Update the isWebSearchActivityGroup(blocks) branch to only return “已搜索网页” when
the group has actually succeeded, otherwise fall through to the existing
failure-aware status summary path that respects block.status.
icycrystal4
force-pushed
the
codex/wework-web-search-display
branch
from
June 26, 2026 14:08
9b5fe52 to
fb8562a
Compare
icycrystal4
changed the title
icycrystal4
force-pushed
the
codex/wework-web-search-display
branch
from
June 26, 2026 14:18
fb8562a to
2bed6e2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
wework/src/features/workbench/WorkbenchProvider.tsx (1)
735-747: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPlease stop growing
WorkbenchProviderin place.Adding file-change normalization and startup-readiness wiring here pushes even more responsibility into an already oversized file/function. Extract the new readiness logic and block normalization into dedicated hooks/modules before this gets harder to test and change safely.
As per coding guidelines,
**/*.{ts,tsx}: Files should not exceed 1000 lines; split large files into multiple sub-modules for better maintainabilityandFunctions should not exceed 50 lines of code (preferred maximum)`.Also applies to: 1076-1294, 3247-3349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/workbench/WorkbenchProvider.tsx` around lines 735 - 747, WorkbenchProvider is accumulating too much normalization and readiness logic, so move the new file-change normalization and startup-readiness wiring out of the component into dedicated hooks or helper modules. Extract the block-handling logic around the file_changes branch that uses normalizeTurnFileChanges into a separate reusable unit, and similarly pull the readiness-related code out of WorkbenchProvider so the component stays smaller and easier to test. Keep the existing behavior intact while refactoring the relevant helpers and call sites so the oversized file and function boundaries are reduced.Source: Coding guidelines
packages/chat-core/src/workbench-message-reducer.ts (1)
117-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
reduceWorkbenchMessagesbefore threading more block variants through it.This reducer is already far past the preferred function-size limit, and the new generic
file_changespath makes the cross-action block invariants harder to audit. Extract per-action helpers so block creation/finalization logic stays local.As per coding guidelines,
**/*.{ts,tsx}: Functions should not exceed 50 lines of code (preferred maximum).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/chat-core/src/workbench-message-reducer.ts` around lines 117 - 281, The reduceWorkbenchMessages reducer is too large and the new block/file_changes handling is making the action-specific invariants hard to follow. Split the switch in reduceWorkbenchMessages into per-action helpers (for example for assistant_started, assistant_cached, assistant_chunk, assistant_done, assistant_error, block_created, and block_updated) so block creation/finalization stays local and each helper remains under the preferred size limit.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/runtime_work/transcript.rs`:
- Around line 57-59: The transcript turn currently emits file change data twice,
once via the new file_changes block in transcript.rs and again through the
existing aggregated fileChanges payload, which causes duplicate UI rendering in
AssistantMessage. Update the transcript loading path around the
fold_commentary/file_changes_block logic and the AssistantMessage render
contract so only one file-change source is shown per turn, either by gating one
renderer or by choosing a single payload shape consistently.
In `@wework/src/components/chat/blocks/ToolBlockItem.tsx`:
- Around line 103-148: Add stable data-testid attributes to the new interactive
toggle buttons in ToolBlockItem. The main expand/collapse control and each
per-file disclosure button currently lack E2E hooks; update the button elements
in the file-change summary and file row rendering to expose descriptive test ids
following the {action}-{element-type} pattern. Use the existing ToolBlockItem,
fileChangesSummaryLabel, and fileChangeRowLabel areas to locate the two toggles
and keep the selectors stable across UI changes.
In `@wework/src/components/chat/blocks/ToolBlocksDisplay.tsx`:
- Around line 170-172: The ToolBlocksDisplay component is setting state
synchronously in a useEffect, which triggers the react-hooks/set-state-in-effect
warning and adds an unnecessary re-render. Remove the effect that calls
setIsRendered from the expanded transition and instead derive the rendered state
directly from expanded in ToolBlocksDisplay so the block becomes visible without
a layout-cycle update. Keep the logic centered around the isRendered state used
in this component and update the rendering condition accordingly.
In `@wework/src/components/chat/ScrollableMessageArea.tsx`:
- Around line 363-365: The scroll state keying in scrollPositionKey currently
collapses numeric and string conversation identifiers into the same value, which
can mix state between distinct conversations. Update the key generation logic in
scrollPositionKey and its callers in ScrollableMessageArea so number and string
inputs are encoded distinctly while still returning null for missing keys; use a
type-aware prefix or equivalent disambiguation to keep 1 and '1' in separate
buckets.
---
Nitpick comments:
In `@packages/chat-core/src/workbench-message-reducer.ts`:
- Around line 117-281: The reduceWorkbenchMessages reducer is too large and the
new block/file_changes handling is making the action-specific invariants hard to
follow. Split the switch in reduceWorkbenchMessages into per-action helpers (for
example for assistant_started, assistant_cached, assistant_chunk,
assistant_done, assistant_error, block_created, and block_updated) so block
creation/finalization stays local and each helper remains under the preferred
size limit.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 735-747: WorkbenchProvider is accumulating too much normalization
and readiness logic, so move the new file-change normalization and
startup-readiness wiring out of the component into dedicated hooks or helper
modules. Extract the block-handling logic around the file_changes branch that
uses normalizeTurnFileChanges into a separate reusable unit, and similarly pull
the readiness-related code out of WorkbenchProvider so the component stays
smaller and easier to test. Keep the existing behavior intact while refactoring
the relevant helpers and call sites so the oversized file and function
boundaries are reduced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d518df94-9bec-41be-9dc7-7b757440d2f5
📒 Files selected for processing (22)
executor/src/runtime_work/transcript.rsexecutor/tests/app_runtime_work_file_changes_contract.rspackages/chat-core/src/index.tspackages/chat-core/src/workbench-message-reducer.tswework/src/components/chat/CodexTurnArtifacts.tsxwework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/ScrollableMessageArea.test.tsxwework/src/components/chat/ScrollableMessageArea.tsxwework/src/components/chat/blocks/ToolBlockItem.test.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/blocks/WebSearchSources.tsxwework/src/components/chat/blocks/processingExpansionState.tswework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/chat/blocks/webSearchActivity.tswework/src/components/chat/codexReferences.tswework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/chat.jsonwework/src/i18n/locales/zh-CN/chat.jsonwework/src/types/workbench.ts
✅ Files skipped from review due to trivial changes (2)
- wework/src/i18n/locales/zh-CN/chat.json
- wework/src/i18n/locales/en/chat.json
🚧 Files skipped from review as they are similar to previous changes (5)
- wework/src/components/chat/blocks/ToolBlockItem.test.tsx
- wework/src/components/chat/blocks/WebSearchSources.tsx
- wework/src/components/chat/blocks/toolBlockActivity.ts
- wework/src/components/chat/MessageList.tsx
- wework/src/components/chat/blocks/webSearchActivity.ts
Comment on lines
+103
to
+148
| <div className="min-w-0 overflow-hidden text-[13px]" data-testid="process-file-changes-block"> | ||
| <button | ||
| type="button" | ||
| aria-expanded={expanded} | ||
| onClick={() => setExpanded(value => !value)} | ||
| className="flex max-w-full items-center gap-1.5 text-text-muted hover:text-text-secondary" | ||
| > | ||
| <FileDiff className="h-4 w-4 shrink-0" strokeWidth={1.7} /> | ||
| <span className="min-w-0 truncate">{fileChangesSummaryLabel(summary, t)}</span> | ||
| <ChevronDown | ||
| className={`h-3.5 w-3.5 shrink-0 transition-transform ${expanded ? '' : '-rotate-90'}`} | ||
| strokeWidth={2} | ||
| /> | ||
| </button> | ||
| {expanded ? ( | ||
| <div className="mt-2 min-w-0 space-y-1.5"> | ||
| {summary.files.map(file => { | ||
| const previewLines = fileDiffPreviewLines(file, summary) | ||
| const fileExpanded = expandedFilePath === file.path && previewLines.length > 0 | ||
| return ( | ||
| <div key={`${file.old_path ?? ''}:${file.path}`} className="min-w-0"> | ||
| <button | ||
| type="button" | ||
| disabled={previewLines.length === 0} | ||
| onClick={() => | ||
| setExpandedFilePath(current => (current === file.path ? null : file.path)) | ||
| } | ||
| className="group flex max-w-full items-center gap-1.5 text-text-secondary disabled:cursor-default" | ||
| > | ||
| <span className="min-w-0 truncate">{fileChangeRowLabel(file, t)}</span> | ||
| {!file.binary ? ( | ||
| <span className="flex shrink-0 items-center gap-1.5 text-xs font-medium"> | ||
| <span className="text-green-600">+{file.additions}</span> | ||
| <span className="text-red-500">-{file.deletions}</span> | ||
| </span> | ||
| ) : null} | ||
| {previewLines.length > 0 ? ( | ||
| <ChevronDown | ||
| className={`h-3.5 w-3.5 shrink-0 text-text-muted transition-transform group-hover:text-text-secondary ${ | ||
| fileExpanded ? '' : '-rotate-90' | ||
| }`} | ||
| strokeWidth={2} | ||
| /> | ||
| ) : null} | ||
| </button> | ||
| {fileExpanded ? <InlineDiffPreview file={file} lines={previewLines} /> : null} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add stable data-testid hooks to the new file-change toggles.
These are newly added interactive buttons, but neither one exposes a stable E2E selector.
Suggested change
<button
type="button"
+ data-testid="toggle-file-changes-button"
aria-expanded={expanded}
onClick={() => setExpanded(value => !value)}
className="flex max-w-full items-center gap-1.5 text-text-muted hover:text-text-secondary"
>
@@
<button
type="button"
+ data-testid={`toggle-file-diff-button-${basename(file.path)}`}
disabled={previewLines.length === 0}
onClick={() =>
setExpandedFilePath(current => (current === file.path ? null : file.path))
}As per coding guidelines, "Preserve and add data-testid attributes to interactive elements (buttons, inputs, links, selects) for E2E testing, using descriptive naming pattern {action}-{element-type}."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="min-w-0 overflow-hidden text-[13px]" data-testid="process-file-changes-block"> | |
| <button | |
| type="button" | |
| aria-expanded={expanded} | |
| onClick={() => setExpanded(value => !value)} | |
| className="flex max-w-full items-center gap-1.5 text-text-muted hover:text-text-secondary" | |
| > | |
| <FileDiff className="h-4 w-4 shrink-0" strokeWidth={1.7} /> | |
| <span className="min-w-0 truncate">{fileChangesSummaryLabel(summary, t)}</span> | |
| <ChevronDown | |
| className={`h-3.5 w-3.5 shrink-0 transition-transform ${expanded ? '' : '-rotate-90'}`} | |
| strokeWidth={2} | |
| /> | |
| </button> | |
| {expanded ? ( | |
| <div className="mt-2 min-w-0 space-y-1.5"> | |
| {summary.files.map(file => { | |
| const previewLines = fileDiffPreviewLines(file, summary) | |
| const fileExpanded = expandedFilePath === file.path && previewLines.length > 0 | |
| return ( | |
| <div key={`${file.old_path ?? ''}:${file.path}`} className="min-w-0"> | |
| <button | |
| type="button" | |
| disabled={previewLines.length === 0} | |
| onClick={() => | |
| setExpandedFilePath(current => (current === file.path ? null : file.path)) | |
| } | |
| className="group flex max-w-full items-center gap-1.5 text-text-secondary disabled:cursor-default" | |
| > | |
| <span className="min-w-0 truncate">{fileChangeRowLabel(file, t)}</span> | |
| {!file.binary ? ( | |
| <span className="flex shrink-0 items-center gap-1.5 text-xs font-medium"> | |
| <span className="text-green-600">+{file.additions}</span> | |
| <span className="text-red-500">-{file.deletions}</span> | |
| </span> | |
| ) : null} | |
| {previewLines.length > 0 ? ( | |
| <ChevronDown | |
| className={`h-3.5 w-3.5 shrink-0 text-text-muted transition-transform group-hover:text-text-secondary ${ | |
| fileExpanded ? '' : '-rotate-90' | |
| }`} | |
| strokeWidth={2} | |
| /> | |
| ) : null} | |
| </button> | |
| {fileExpanded ? <InlineDiffPreview file={file} lines={previewLines} /> : null} | |
| <div className="min-w-0 overflow-hidden text-[13px]" data-testid="process-file-changes-block"> | |
| <button | |
| type="button" | |
| data-testid="toggle-file-changes-button" | |
| aria-expanded={expanded} | |
| onClick={() => setExpanded(value => !value)} | |
| className="flex max-w-full items-center gap-1.5 text-text-muted hover:text-text-secondary" | |
| > | |
| <FileDiff className="h-4 w-4 shrink-0" strokeWidth={1.7} /> | |
| <span className="min-w-0 truncate">{fileChangesSummaryLabel(summary, t)}</span> | |
| <ChevronDown | |
| className={`h-3.5 w-3.5 shrink-0 transition-transform ${expanded ? '' : '-rotate-90'}`} | |
| strokeWidth={2} | |
| /> | |
| </button> | |
| {expanded ? ( | |
| <div className="mt-2 min-w-0 space-y-1.5"> | |
| {summary.files.map(file => { | |
| const previewLines = fileDiffPreviewLines(file, summary) | |
| const fileExpanded = expandedFilePath === file.path && previewLines.length > 0 | |
| return ( | |
| <div key={`${file.old_path ?? ''}:${file.path}`} className="min-w-0"> | |
| <button | |
| type="button" | |
| data-testid={`toggle-file-diff-button-${basename(file.path)}`} | |
| disabled={previewLines.length === 0} | |
| onClick={() => | |
| setExpandedFilePath(current => (current === file.path ? null : file.path)) | |
| } | |
| className="group flex max-w-full items-center gap-1.5 text-text-secondary disabled:cursor-default" | |
| > | |
| <span className="min-w-0 truncate">{fileChangeRowLabel(file, t)}</span> | |
| {!file.binary ? ( | |
| <span className="flex shrink-0 items-center gap-1.5 text-xs font-medium"> | |
| <span className="text-green-600">+{file.additions}</span> | |
| <span className="text-red-500">-{file.deletions}</span> | |
| </span> | |
| ) : null} | |
| {previewLines.length > 0 ? ( | |
| <ChevronDown | |
| className={`h-3.5 w-3.5 shrink-0 text-text-muted transition-transform group-hover:text-text-secondary ${ | |
| fileExpanded ? '' : '-rotate-90' | |
| }`} | |
| strokeWidth={2} | |
| /> | |
| ) : null} | |
| </button> | |
| {fileExpanded ? <InlineDiffPreview file={file} lines={previewLines} /> : null} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/chat/blocks/ToolBlockItem.tsx` around lines 103 - 148,
Add stable data-testid attributes to the new interactive toggle buttons in
ToolBlockItem. The main expand/collapse control and each per-file disclosure
button currently lack E2E hooks; update the button elements in the file-change
summary and file row rendering to expose descriptive test ids following the
{action}-{element-type} pattern. Use the existing ToolBlockItem,
fileChangesSummaryLabel, and fileChangeRowLabel areas to locate the two toggles
and keep the selectors stable across UI changes.
Source: Coding guidelines
Comment on lines
+170
to
+172
| useEffect(() => { | ||
| if (expanded) setIsRendered(true) | ||
| }, [expanded]) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
This effect will trigger the react-hooks/set-state-in-effect error and cause an extra render.
setIsRendered(true) is called synchronously inside the effect whenever expanded changes to true, which ESLint flags as an error and forces an unnecessary re-render. Initialize isRendered directly from expanded in the rendering logic (e.g., const isRendered = expanded || isRendered) to remove the effect and the extra layout cycle.
🧰 Tools
🪛 ESLint
[error] 171-171: Error: Calling setState synchronously within an effect can trigger cascading renders
Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
- Update external systems with the latest state from React.
- Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
/home/jailuser/git/wework/src/components/chat/blocks/ToolBlocksDisplay.tsx:171:19
169 |
170 | useEffect(() => {
171 | if (expanded) setIsRendered(true)
| ^^^^^^^^^^^^^ Avoid calling setState() directly within an effect
172 | }, [expanded])
173 |
174 | useLayoutEffect(() => {
(react-hooks/set-state-in-effect)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/chat/blocks/ToolBlocksDisplay.tsx` around lines 170 -
172, The ToolBlocksDisplay component is setting state synchronously in a
useEffect, which triggers the react-hooks/set-state-in-effect warning and adds
an unnecessary re-render. Remove the effect that calls setIsRendered from the
expanded transition and instead derive the rendered state directly from expanded
in ToolBlocksDisplay so the block becomes visible without a layout-cycle update.
Keep the logic centered around the isRendered state used in this component and
update the rendering condition accordingly.
Comment on lines
+363
to
+365
| function scrollPositionKey(conversationKey: string | number | null | undefined): string | null { | ||
| return conversationKey == null ? null : String(conversationKey) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disambiguate numeric vs. string conversation keys.
String(conversationKey) maps 1 and '1' to the same bucket, so scroll state can bleed between distinct conversations when callers use both accepted key types.
Suggested change
function scrollPositionKey(conversationKey: string | number | null | undefined): string | null {
- return conversationKey == null ? null : String(conversationKey)
+ return conversationKey == null ? null : `${typeof conversationKey}:${conversationKey}`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function scrollPositionKey(conversationKey: string | number | null | undefined): string | null { | |
| return conversationKey == null ? null : String(conversationKey) | |
| } | |
| function scrollPositionKey(conversationKey: string | number | null | undefined): string | null { | |
| return conversationKey == null ? null : `${typeof conversationKey}:${conversationKey}` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/chat/ScrollableMessageArea.tsx` around lines 363 - 365,
The scroll state keying in scrollPositionKey currently collapses numeric and
string conversation identifiers into the same value, which can mix state between
distinct conversations. Update the key generation logic in scrollPositionKey and
its callers in ScrollableMessageArea so number and string inputs are encoded
distinctly while still returning null for missing keys; use a type-aware prefix
or equivalent disambiguation to keep 1 and '1' in separate buckets.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wework/src/components/chat/blocks/ToolBlockItem.tsx (1)
119-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid reparsing the unified diff once per file.
fileDiffPreviewLines(file, summary)calls intoparseUnifiedDiff(summary.diff)for every rendered file row, so large multi-file diffs get reparsed repeatedly during render. Precompute the parsed sections once persummary.diffand reuse them for row lookups.Also applies to: 251-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/chat/blocks/ToolBlockItem.tsx` around lines 119 - 120, Avoid reparsing the unified diff for each rendered file row in ToolBlockItem; fileDiffPreviewLines currently triggers parseUnifiedDiff(summary.diff) repeatedly inside the summary.files.map render path. Precompute the parsed diff sections once per summary.diff (for example in ToolBlockItem or a shared helper near fileDiffPreviewLines) and pass or reuse that cached result when computing row previews, so each file lookup reads from the same parsed structure instead of reprocessing the diff per row.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wework/src/components/chat/blocks/ToolBlockItem.tsx`:
- Around line 119-120: Avoid reparsing the unified diff for each rendered file
row in ToolBlockItem; fileDiffPreviewLines currently triggers
parseUnifiedDiff(summary.diff) repeatedly inside the summary.files.map render
path. Precompute the parsed diff sections once per summary.diff (for example in
ToolBlockItem or a shared helper near fileDiffPreviewLines) and pass or reuse
that cached result when computing row previews, so each file lookup reads from
the same parsed structure instead of reprocessing the diff per row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43e8c23c-910a-4c4c-979d-8903353cebd6
📒 Files selected for processing (20)
executor/src/runtime_work/transcript.rsexecutor/tests/app_runtime_work_file_changes_contract.rspackages/chat-core/src/index.tspackages/chat-core/src/workbench-message-reducer.tswework/src/components/chat/CodexTurnArtifacts.tsxwework/src/components/chat/MessageList.test.tsxwework/src/components/chat/MessageList.tsxwework/src/components/chat/ScrollableMessageArea.test.tsxwework/src/components/chat/ScrollableMessageArea.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/blocks/WebSearchSources.tsxwework/src/components/chat/blocks/processingExpansionState.tswework/src/components/chat/blocks/webSearchActivity.tswework/src/components/chat/codexReferences.tswework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/chat.jsonwework/src/i18n/locales/zh-CN/chat.jsonwework/src/types/workbench.ts
✅ Files skipped from review due to trivial changes (3)
- wework/src/i18n/locales/zh-CN/chat.json
- wework/src/i18n/locales/en/chat.json
- wework/src/types/workbench.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- wework/src/components/chat/ScrollableMessageArea.test.tsx
- wework/src/components/chat/blocks/ToolBlocksDisplay.test.tsx
- packages/chat-core/src/index.ts
- wework/src/components/chat/blocks/WebSearchSources.tsx
- wework/src/components/chat/CodexTurnArtifacts.tsx
- wework/src/components/chat/codexReferences.ts
- wework/src/features/workbench/WorkbenchProvider.tsx
- wework/src/components/chat/blocks/ToolBlocksDisplay.tsx
- wework/src/components/chat/MessageList.tsx
- wework/src/components/chat/MessageList.test.tsx
- packages/chat-core/src/workbench-message-reducer.ts
- executor/src/runtime_work/transcript.rs
- wework/src/components/chat/blocks/webSearchActivity.ts
- executor/tests/app_runtime_work_file_changes_contract.rs
- wework/src/components/chat/ScrollableMessageArea.tsx
This was referenced Jun 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
cargo fmt --manifest-path executor/Cargo.tomlcargo test --manifest-path executor/Cargo.toml --test app_runtime_work_file_changes_contractcargo test --manifest-path executor/Cargo.toml --test app_runtime_work_contractpnpm --dir wework exec tsc -b --pretty falsepnpm --dir wework test src/components/chat/ScrollableMessageArea.test.tsx src/components/chat/blocks/ToolBlocksDisplay.test.tsx src/components/chat/blocks/ToolBlockItem.test.tsx src/components/chat/MessageList.test.tsx src/components/chat/FileChangesCard.test.tsx src/test/theme-token-guard.test.tsgit diff --checkcargo fmt --check,cargo test --all-features,cargo clippySummary by CodeRabbit
New Features
Bug Fixes