Refactor ChatView into memoized UI subcomponents - #58
Conversation
- Extract header, error, approvals, and timeline into memoized components - Memoize `ChatMarkdown`, pickers, and handlers to reduce avoidable re-renders - Reduce running-time ticker frequency from 250ms to 1000ms
WalkthroughWrapped ChatMarkdown with React.memo. Refactored ChatView by extracting inline UI into multiple memoized subcomponents (ChatHeader, ThreadErrorBanner, PendingApprovalsPanel, MessagesTimeline, ModelPicker, ReasoningEffortPicker, OpenInPicker), adjusted callback wiring and timer interval. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ChatView as ChatView
participant Header as ChatHeader
participant Timeline as MessagesTimeline
participant Native as NativeApi
User->>ChatView: interact (send message / open image / pick model)
ChatView->>Header: render header, actions (onToggleDiff, OpenInPicker)
Header->>ChatView: emit action callbacks
ChatView->>Timeline: render messages, handle image expand / work-group toggle
Timeline->>Native: request image data / interaction
ChatView->>Native: session-scoped actions (respondToApproval, ensureSession)
Native-->>ChatView: responses / confirmations
ChatView-->>User: UI updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Refactor
|
Greptile SummaryThis PR refactors the monolithic
Confidence Score: 3/5
Important Files Changed
Flowchartflowchart TD
CV[ChatView] -->|"title, project, diffOpen"| CH[ChatHeader]
CV -->|"error string"| TEB[ThreadErrorBanner]
CV -->|"approvals, respondingIds, onRespond"| PAP[PendingApprovalsPanel]
CV -->|"activeThread, timelineEntries, nowIso, ..."| MT[MessagesTimeline]
MT -->|"text"| CM[ChatMarkdown]
CV -->|"model, onModelChange"| MP[ModelPicker]
CV -->|"effort, onEffortChange"| REP[ReasoningEffortPicker]
CH -->|"keybindings"| OIP[OpenInPicker]
CH -->|"api, gitCwd"| GAC[GitActionsControl]
style CH fill:#2d6a4f,stroke:#40916c
style TEB fill:#2d6a4f,stroke:#40916c
style PAP fill:#e76f51,stroke:#f4a261
style MT fill:#e76f51,stroke:#f4a261
style CM fill:#2d6a4f,stroke:#40916c
style MP fill:#2d6a4f,stroke:#40916c
style REP fill:#2d6a4f,stroke:#40916c
style OIP fill:#2d6a4f,stroke:#40916c
style GAC fill:#264653,stroke:#2a9d8f
Last reviewed commit: bc7eae3 |
| setRespondingRequestIds((existing) => existing.filter((id) => id !== requestId)); | ||
| } | ||
| }, | ||
| [activeThread?.id, activeThread?.session, api, dispatch], |
There was a problem hiding this comment.
activeThread?.session defeats memo on PendingApprovalsPanel
activeThread?.session is a ProviderSession object that gets replaced with a new reference on every provider event (via evolveSession in the store reducer). Since this callback only uses activeThread.session.sessionId (a string), the dependency should be narrowed to activeThread?.session?.sessionId. As-is, onRespondToApproval is recreated on every incoming event, which means PendingApprovalsPanel's memo wrapper is never effective during active streaming — exactly when memoization matters most.
| [activeThread?.id, activeThread?.session, api, dispatch], | |
| [activeThread?.id, activeThread?.session?.sessionId, api, dispatch], |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/components/ChatView.tsx (2)
500-508: Timer interval increase may affect streaming UX smoothness.The interval change from 250ms to 1000ms reduces render frequency, but elapsed time displays during streaming (used in
formatMessageMetaon lines 1599-1607) will now update only once per second instead of four times. This may make the elapsed time appear to "jump" rather than count smoothly.Consider whether this trade-off aligns with UX requirements, particularly for longer-running responses where users watch the elapsed time.
Based on learnings: "Preserve fast time-to-first-delta and smooth streaming in chat message updates."
1548-1556: Non-null assertion is safe but could be cleaner.The
image.previewUrl!assertion on line 1554 is safe because it's inside the truthy branch ofimage.previewUrl ?. TypeScript doesn't narrow types inside inline arrow functions, necessitating the assertion.An alternative would be to capture the value before the callback:
♻️ Optional: Avoid non-null assertion
- {image.previewUrl ? ( + {image.previewUrl ? (() => { + const previewUrl = image.previewUrl; + return ( <img src={image.previewUrl} alt={image.name} className="h-full max-h-[220px] w-full cursor-zoom-in object-cover" onClick={() => - onImageExpand({ src: image.previewUrl!, name: image.name }) + onImageExpand({ src: previewUrl, name: image.name }) } /> + ); + })() : ( - ) : (
- Derive `activeSessionId` and `activeThreadId` before approval callbacks - Avoid stale thread/session captures when submitting approval decisions - Pass `hasMessages` to `MessagesTimeline` instead of the full thread
Desktop Dev Perf Trace
Interaction Run
Input Event Metrics
Scheduler/Event Hotspots
Threshold Check
Heap Counters
Top User Timing Marks
Top Duration Events
|
…gdotgg#58) * feat(settings): build the Secrets administration page in T3 Replace the Secrets NotInT3Yet placeholder with a real panel: a Global and a Personal section, each listing its scope's secrets with an enable switch, edit, and delete, plus a create/edit dialog. Reads go through the shared moatless query helpers keyed under `secrets/{scope}`, so a write invalidates both scopes together. The row-derivation logic (kind labels, active-first ordering) lives in a pure `secretRows.ts` with co-located tests; the panel only reads atoms and renders T3's own settings primitives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(settings): build the Users administration page in T3 Replace the Users NotInT3Yet placeholder with a list and a detail page. The list shows everyone with an account — monogram, name, admin and bot tags — and leads to one user's page. The detail page edits name, email, and the global role with dirty tracking and an explicit save, and shows the read-only account identity beneath it. There is no single-user read on the wire, so the detail page reads the same list the index does and finds its login in it; the two share the `users` key so a role change refetches both at once. Row derivation (display name, monogram, humans-before-bots ordering) is a pure `userRows.ts` with co-located tests. The detail route uses the trailing-underscore file form so it does not nest under the list route, and is recorded in the merge inventory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(settings): build the Moatless Loops administration pages in T3 Add the Loops list and Loop detail settings surfaces, replacing the NotInT3Yet placeholders. The list shows each loop's state, git provenance and source summary, and links to a detail page that edits the loop's general fields, configuration and schedule with the workspace SaveBar pattern, exposes lifecycle actions (pause/resume/approve-and-activate), git override/restore, and delete. Source derivation and provenance live in a pure `loopRows.ts` module with co-located tests. Detail route uses the underscore form to avoid nested-route rendering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(settings): build the Moatless Integrations administration page in T3 Replace the NotInT3Yet placeholder on /settings/integrations with a panel of three sections: Connections (adapter connections a loop can subscribe to — listed, created, and opened to a detail page that removes them), Apps (the configured adapter apps and which of each app's secrets are set, read-only), and GitHub (registered GitHub apps, their installations and the default installation, read-only). Naming, ordering and secret fingerprint derivation live in a pure integrationRows.ts module with co-located tests. Connection detail route uses the underscore form to avoid nested-route rendering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(settings): administer skills plugins and their activation in T3 Adds the Skills administration surface: a plugin catalog and, per plugin, the skills it sources with a control for the deployment-wide default and the viewer's own override, side by side. The delivered badge is read straight from /plugins/effective rather than recomputed from the two controls — off-for-everyone-plus-on-for-you is the server's call to resolve, so a skill can read "Not set / Not set" and still be Delivered because its plugin is on. Precedence lives in skillRows.ts behind a test that pins the source-off-with-skill-on and unset-versus-off cases. Detail route uses the underscore form so it does not nest under the list panel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(spec): mark moatless-admin spec in review and correct stale claims All five administration surfaces are now built and under review as PRs pingdotgg#51–pingdotgg#55, so the spec no longer reads "in progress". Records the merge order the shared queries.ts forces, and the two follow-ups that land once the five merge (delete NotInT3Yet, move this spec to .plans/completed/). Also fixes two claims that drifted from what shipped: the acceptance criterion still said the not-built placeholder links into the SPA, which the placeholder decision earlier in the same doc deliberately reversed; and two seam-table module paths pointed at apps/web/src/moatless/ rather than where the pure modules actually landed under components/settings/moatless/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(spec): unbreak the surfaces table split by the route-convention note The trailing-underscore route-convention paragraph sat between two table rows, so Markdown ended the table at it and rendered every row below — loops through users, ten of the twelve surfaces — as literal pipes. Move the paragraph below the last row so the table stays one block. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(settings): retire the NotInT3Yet placeholder and archive the spec Every administration surface now has a real panel, so no route renders the placeholder. Its own comment said to delete the file when the last usage went; this is that moment. The merge-inventory row stops describing a build where some entries lead to a placeholder. Neither of these could be done while the surfaces were five separate branches — each still needed the placeholder for the other four. Combining them into one change is what makes both possible, so they land here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(fork): list the merge inventory as entries, not tables The inventory conflicted on every branch that touched it, and almost none of that was disagreement. The formatter re-pads a Markdown table across all of its rows when one cell changes width, so adding a single route path to the administration-pages row rewrote all 60 lines of the block; two branches editing two unrelated entries then conflicted on every line between them. Measured on this file, adding one route path to the administration pages: 60 changed lines as a table, 1 as a list. So the tables are gone. Every section is a list of entries: a title line, an optional nested `Paths:` list with one path per line, and the rule. The formatter leaves list items and prose alone — verified by running `vp fmt` over the result and getting a byte-identical file back. The header paragraph says so, so this does not get turned back into a table. Content is carried over unchanged. Verified by extracting every backticked path from both versions: 117 before, 117 after, none lost and none invented. Three deliberate edits beside the reformat: - "the three fork plans above" now says four; that entry has listed four plans since the administration spec was archived. - "row" means a table row, and there are no tables now, so policy entries and the skill's vocabulary say "entry" — including the `decide, then add an entry` policy value the skill defines. - `.agents/skills/fork-upstream-merge/**` gets a path-policy entry. It is fork-authored and was in no entry at all; upstream owns the other four skills under `.agents/skills/` and adds more, so a blanket `ours` there would be wrong. Checked against a shallow `upstream/main` fetch, which also confirmed the two counts this file quotes are still current: 32 of 36 `.plans/` files and 34 of 39 under `apps/web/src/browser/`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Brings in pingdotgg#58, the Moatless administration surfaces. The only conflict was docs/fork/upstream-merge-inventory.md, which pingdotgg#58 converted from tables to lists so an edit stops re-padding every row. Took that shape and re-stated this branch's four inventory changes in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…inspect MCP tools
The generation half, productized. A harness MCP toolkit (generate_3d,
generation_status, list_generations, inspect_generation) makes a 3D
generation project-owned state an agent can drive and inspect —
mirrors the preview toolkit's structure and registers the same way.
generate_3d returns a job handle immediately; the SERVER owns the Tripo
poll loop in a fiber forked into the service's OWN scope
(acquireRelease(Scope.make())+forkIn), so a job survives the MCP
request effect returning (the async model OpenCode's 65s timeout
forces). On success it downloads the GLB, inspects it server-side
(glTF accessor parse — no Unity; verified against a committed
5,996-tri barrel fixture), stores it under a project-scoped dir, and
records a GeneratedAsset. inspect_generation returns metadata + the
provider's preview image as an MCP image content block.
Contracts are fork-owned (packages/contracts/src/generation/) — ZERO
vendor-union edits (contracts diff = one barrel-export line, matching
the 5 existing unity-* precedents). Capability grant widened to
{preview, generation} (D2 resolved); the fork-owned
GenerationCapabilityUnavailableError translates at the gate.
Credential read once from ServerSecretStore, seeded from the owner's
key file, never logged.
Deferred to increment 2 (flagged, not skipped): the fork-owned
signed-asset route (its only consumer is the OUT-of-scope panel; server.ts
is at the 20-arg route ceiling pingdotgg#58), Unity import, the Generation panel.
deriveFbx has a minimal impl for inc-2, no coverage yet.
All red-proven (grant, gate, GLB inspection non-vacuous, job lifecycle
monotonic). Server suite 2255 passed; server+contracts typecheck clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Not sure why the compiler isn't doing it's magic but this manual memoization shows significant perf wins...
Before vs after (old 00:17 trace -> new 00:49 trace):
keypress avg: 24.66ms -> 3.60ms (-85.4%)
textInput avg: 24.58ms -> 3.50ms (-85.8%)
input avg: 23.97ms -> 2.99ms (-87.5%)
worst keypress spike: 68.73ms -> 5.50ms (-92.0%)
total EventDispatch time: 6467ms -> 816ms (-87.4%)
dispatchDiscreteEvent time: 2116ms -> 245ms (-88.4%)
EventDispatch >= 50ms: 24 -> 0
Summary
ChatViewinto focused memoized subcomponents (ChatHeader,ThreadErrorBanner,PendingApprovalsPanel,MessagesTimeline) to reduce render churn and improve maintainability.ChatMarkdown,ModelPicker,ReasoningEffortPicker,OpenInPicker).useCallback(approval responses, diff toggle, work-group expand, image expand) to support memoization.Testing
Summary by CodeRabbit