feat(editor): extract chat runtime#14937
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces AIProvider with AIAppEvents/AIRequestService, introduces AIChatRuntime and session strategies, refactors chat UI to use runtime snapshots and dispatch actions, moves transport/histories/context into AIRequestService (including BYOK), and adds runtime/request tests. ChangesAI chat runtime migration
Sequence Diagram(s)sequenceDiagram
participant UI as UI (ai-chat-input/toolbar/content)
participant Events as AIAppEvents
participant Runtime as AIChatRuntime
participant Request as AIRequestService
UI->>Events: requestOpenWithChat / requestSendWithChat
UI->>Runtime: dispatch(send/openSession/retry/stop)
Runtime->>Request: executeAction()/histories/context/forkChat
Request-->>Runtime: stream chunks, history, context updates
Runtime-->>UI: snapshot {messages,status,history,composer}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
|
Deploying blocksuite-docs with
|
| Latest commit: |
ee466ab
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef4fd2ba.blocksuite-docs.pages.dev |
| Branch Preview URL: | https://darksky-chat-panel-refactor.blocksuite-docs.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-toolbar.ts (1)
191-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential
TypeErrorintoggleHistoryMenuwhen toggled mid-refresh.The abort listener nulls
this.abortController, but the check afterawaitstill dereferences it:this.abortController.signal.addEventListener('abort', () => { this.abortController = null; // ← null'd by a second toggle click }); try { await this.runtime.dispatch({ type: 'refreshHistory' }); } catch (error) { console.error(error); } if (this.abortController.signal.aborted) { // ← TypeError if user clicked again return; }Race:
- First click: new
AbortControllerassigned,refreshHistoryawait suspends.- Second click (toggle):
this.abortController.abort()fires the listener →this.abortController = null.- First call resumes →
this.abortController.signal.abortedthrows "Cannot read properties of null".Also, the
createLitPortal(... abortController: this.abortController ...)call below dereferences the same field, so even without the explicit.signal.abortedcheck it can hit the same null path.🔒️ Suggested fix — capture the controller in a local
- this.abortController = new AbortController(); - this.abortController.signal.addEventListener('abort', () => { - this.abortController = null; - }); - - try { - await this.runtime.dispatch({ type: 'refreshHistory' }); - } catch (error) { - console.error(error); - } - if (this.abortController.signal.aborted) { - return; - } + const abortController = new AbortController(); + this.abortController = abortController; + abortController.signal.addEventListener('abort', () => { + if (this.abortController === abortController) { + this.abortController = null; + } + }); + + try { + await this.runtime.dispatch({ type: 'refreshHistory' }); + } catch (error) { + console.error(error); + } + if (abortController.signal.aborted) { + return; + } @@ - abortController: this.abortController, + abortController,🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-toolbar.ts` around lines 191 - 239, toggleHistoryMenu currently reads this.abortController after an await which can be nulled by a concurrent abort; fix by capturing the controller in a local constant and using that local for post-await checks and for passing into createLitPortal. Concretely: in toggleHistoryMenu, after creating the AbortController do "const controller = new AbortController(); this.abortController = controller;", add the abort listener to controller (and make the listener set this.abortController = null only if this.abortController === controller), await runtime.dispatch(...), then check controller.signal.aborted and pass controller to createLitPortal instead of this.abortController so the async path cannot hit a null this.abortController.
🧹 Nitpick comments (26)
packages/frontend/core/src/blocksuite/ai/runtime/chat/use-element.spec.tsx (2)
17-21: 💤 Low valueNo teardown between tests leaves containers on
document.body.
createContainerRefappends a fresh<div>todocument.bodyfor each test but nothing removes it. With more tests added later, residue accumulates and the next test'sdocument.bodywill contain N stale containers; happy-dom isolation in vitest mitigates this between files but not within adescribe. A smallafterEach(orcontainer.remove()in atry/finally) would future-proof.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/use-element.spec.tsx` around lines 17 - 21, Tests append a new div via createContainerRef which is never removed, leaving leftover containers on document.body across tests; update the test file to ensure cleanup by removing the created container after each test (for example add an afterEach that calls container.remove() or ensure createContainerRef returns the created element so each test can remove it in a finally/teardown). Reference the createContainerRef helper and ensure any variable holding the returned { current: container } is accessible to the afterEach/teardown so the DOM node is removed to prevent accumulation across tests.
27-90: ⚡ Quick winCoverage gaps worth filling:
enabled: false,onElementReady, and unmount semantics.The current two tests cover the happy paths well, but consider adding:
enabled: false→ no element is created/configured and no callback fires.onElementReady→ invoked exactly once per element, even after rerenders.- Unmount/cleanup behavior → encodes the (currently implicit) decision that the element persists in the container.
These would lock down contracts that downstream desktop chat pages rely on.
🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/use-element.spec.tsx` around lines 27 - 90, Add tests for three missing behaviors in useAIChatElement: (1) when enabled: false ensure no element is created/configured and callbacks like onElementReady are not invoked (use createElement and configureElement spies and containerRef to assert nothing appended); (2) verify onElementReady is called exactly once per element even after rerenders by rendering with onElementReady spy, rerendering, and asserting single invocation and that the same element is reused; (3) assert unmount/cleanup semantics by unmounting the hook and checking that the element remains (or is removed if intended) in containerRef to encode the persistence contract. Use the existing helpers (createElement, containerRef) and renderHook/cleanup patterns to implement these specs.packages/frontend/core/src/blocksuite/ai/runtime/request/message-transport.ts (1)
86-90: 💤 Low value
paramscast loses some of the type-narrowing benefit.You tightened
TextToTextOptions['params']fromRecord<string, any>toRecord<string, unknown>, which is good — but immediately casting back toParameters<CopilotClient['createMessage']>[0]['params']defeats most of the static-check benefit at this call site. IfCopilotClient['createMessage']accepts aRecord<string, unknown>(or compatible) directly, the cast can simply be dropped; otherwise consider narrowing/validating before the call. Non-blocking.🤖 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/frontend/core/src/blocksuite/ai/runtime/request/message-transport.ts` around lines 86 - 90, The params cast undermines the type safety gained by tightening TextToTextOptions['params']; in the call that builds options for CopilotClient.createMessage (variables: options, sessionId, content, params), either drop the explicit cast so the params value is passed with its narrowed type directly if CopilotClient['createMessage'] accepts Record<string, unknown>, or perform an explicit runtime/type-narrowing step on params (validate keys/values or map into the expected shape) before assigning to options.params so you don't revert to a wide any-like type; update the code around options and the createMessage call accordingly.packages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-session-history.ts (2)
25-42: 💤 Low value
truncateSessionTitlecan split surrogate pairs and emoji.
text.slice(0, TITLE_MAX_LENGTH)operates on UTF‑16 code units. A 28th code unit landing in the middle of a 4‑byte emoji (surrogate pair) or a combining-character cluster will produce a malformed glyph in the truncated title. Since session titles are user-authored text and may contain emoji, preferIntl.Segmenterfor grapheme-aware slicing, or at minimum back off one code unit when slicing lands on a high surrogate.♻️ Proposed grapheme-aware truncation
function truncateSessionTitle(text: string) { - if (text.length <= TITLE_MAX_LENGTH) return text; - return `${text.slice(0, TITLE_MAX_LENGTH).trimEnd()}…`; + if (text.length <= TITLE_MAX_LENGTH) return text; + const segmenter = new Intl.Segmenter(undefined, { granularity: 'grapheme' }); + let out = ''; + for (const { segment } of segmenter.segment(text)) { + if (out.length + segment.length > TITLE_MAX_LENGTH) break; + out += segment; + } + return `${out.trimEnd()}…`; }🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-session-history.ts` around lines 25 - 42, truncateSessionTitle uses text.slice(0, TITLE_MAX_LENGTH) which can split UTF‑16 surrogate pairs and break emoji/combining clusters; update truncateSessionTitle to perform grapheme-aware truncation (use Intl.Segmenter if available) or at minimum detect a trailing high surrogate at the cut boundary and back off one code unit before slicing, then trimEnd and append the ellipsis as before; keep deriveSessionTitle and TITLE_MAX_LENGTH unchanged but call the new grapheme-safe truncateSessionTitle so session titles never produce malformed glyphs.
362-370: 💤 Low valueFilter is doubly redundant — keep one source of truth for "current doc" exclusion.
otherSessionsis filtered with both!currentDocSessionIds.has(session.sessionId)and(!this.docId || session.docId !== this.docId). IfcurrentDocSessionsis the authoritative list of current-doc sessions (which the refactor assumes), the second clause is redundant; if it isn't,currentDocSessionIdsmembership is unreliable and should be replaced by thedocIdcheck. Picking one keeps intent clear and avoids future drift between the two predicates.🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-session-history.ts` around lines 362 - 370, The filter for otherSessions is using two redundant predicates—currentDocSessionIds membership and a docId equality check—so pick one source of truth and remove the other; for this refactor keep currentDocSessions/currentDocSessionIds as authoritative and change the recentSessions filter to only exclude sessions whose sessionId is in currentDocSessionIds (i.e., remove the "(!this.docId || session.docId !== this.docId)" clause), keeping the currentDocSessions, currentDocSessionIds and otherSessions names intact.packages/frontend/core/src/blocksuite/ai/runtime/chat/use-element.ts (3)
28-41: 💤 Low value
querySelectorAll+ reparenting can move a nested matching element.
container.querySelectorAll(selector)matches at any descendant depth. If a consumer happens to already have a matching element nested inside a wrapper inside the container, the hook will pick it up, thencontainer.append(nextElement)will reparent it as a direct child ofcontainer, which can break the surrounding layout/markup. For the current selectors (ai-chat-content,ai-chat-toolbar,ai-chat-tabs) wired by the desktop pages this likely won't trigger, but considercontainer.childrenfiltering or using:scope > ${selector}for safety:- const existingElements = Array.from( - container.querySelectorAll(selector) - ) as T[]; + const existingElements = Array.from( + container.querySelectorAll(`:scope > ${selector}`) + ) as T[];🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/use-element.ts` around lines 28 - 41, The hook currently uses container.querySelectorAll(selector) which can return nested descendants and later reparent them via container.append(nextElement); change the lookup to only direct children (e.g., filter container.children by matching selector or use :scope > ${selector}) so we only consider direct child elements for existingElements, then keep the same logic around nextElement, createElement, configureElement and appending to avoid moving nested elements and breaking layout.
24-59: 💤 Low valueEffect re-runs on every render when callbacks aren't memoized.
configureElement,createElement, andonElementReadyare in the dependency array, so unless every caller wraps them inuseCallback/stable refs, the effect re-runs on every parent render and re-invokesconfigureElement(nextElement). For the playground/peek-view consumers wiringai-chat-content/ai-chat-toolbar/ai-chat-tabs, that means a property write on every render — usually cheap, but it bypasses memoization and can trigger Lit reactive updates more than intended. Two options if it matters in practice:
- Document at the call site that callers must memoize, or
- Stash the latest callbacks in
useRefs inside the hook and depend only oncontainerRef,selector, andenabled.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/use-element.ts` around lines 24 - 59, The effect currently depends on configureElement, createElement, and onElementReady causing reruns when callers don't memoize; fix by stashing these callbacks in refs inside the hook (e.g., configureElementRef.current, createElementRef.current, onElementReadyRef.current) and update those refs whenever the props change, then change the useEffect dependency array to only depend on containerRef, selector, and enabled; inside the effect call the ref-held functions (configureElementRef.current(nextElement), createElementRef.current(), onElementReadyRef.current?.(nextElement)) and keep the existing logic around readyElementsRef and setElement unchanged.
24-41: 💤 Low valueAdd cleanup to remove element when
enabledtoggles false or on unmount.The effect creates/adopts an element but never returns a cleanup function. When
enabledtoggles fromtrue→false, the element remains attached. On component unmount, the element persists in the container DOM. While the container itself may be torn down if the parent component unmounts (e.g., chat page), intermediate toggles ofenabled(which occurs whenruntime,snapshot, or other conditions change) leave orphaned elements.Consider returning a cleanup function that removes the element when the effect re-runs with
enabled=falseor on unmount.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/use-element.ts` around lines 24 - 41, The effect in useEffect (using containerRef, selector, createElement, configureElement, nextElement, existingElements, enabled) mounts or reuses an element but never cleans it up; add and return a cleanup function from the effect that, on unmount or when the effect re-runs (including when enabled becomes false), removes the nextElement from its container (if it exists and is a child) and optionally removes any leftover existingElements that were created by this hook; ensure the cleanup only touches elements identified by selector or the nextElement to avoid removing unrelated DOM.packages/frontend/core/src/blocksuite/ai/runtime/request/byok-local-lease.ts (1)
58-96: 💤 Low valueTreat
isSupported()/getWorkspaceLeaseProvidersfailures distinctly from the GraphQL call.Right now the whole
tryblock wraps both local storage reads (isSupported,getWorkspaceLeaseProviders) and the remote mutation. Any throw from the local IPC will be reported as "Failed to create workspace BYOK local lease" and rethrown as aUserFriendlyError, aborting the streaming request entirely. If local BYOK storage is transiently broken, it would be friendlier to log and returnundefined(i.e. degrade to server-managed keys), reserving the rethrow for the actual lease mutation failure. Optional, but worth considering for resilience.🤖 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/frontend/core/src/blocksuite/ai/runtime/request/byok-local-lease.ts` around lines 58 - 96, Separate failures from local storage and the GraphQL mutation: wrap calls to storage.isSupported() and storage.getWorkspaceLeaseProviders(workspaceId) (and the subsequent provider mapping/early returns) in their own try/catch that logs the error with errorMetadata and returns undefined (so we gracefully degrade to server-managed keys) instead of rethrowing; keep the client.gql({ query: createWorkspaceByokLocalLeaseMutation, ... }) call in a separate try/catch so that only mutations errors are converted to UserFriendlyError.fromAny and rethrown, preserving the existing successful-return path for result.createWorkspaceByokLocalLease.leaseId and unchanged behavior for empty/no providers.packages/frontend/core/src/blocksuite/ai/runtime/request/service.ts (1)
265-292: 💤 Low valueConsider capping the attempts counter.
The exponential backoff calculation
Math.min(minInterval * Math.pow(1.5, attempts), maxInterval)will reachmaxIntervalafter about 25 attempts, after which the counter continues to increment indefinitely without affecting the interval. While this doesn't cause functional issues, cappingattemptsoncemaxIntervalis reached would make the logic clearer and avoid potential numeric overflow with extremely long-running polls.♻️ Optional refactor to cap attempts
while (!abortSignal.aborted) { const result = await this.client.getContextDocsAndFiles( workspaceId, sessionId, contextId ); onPoll(result); const interval = Math.min( minInterval * Math.pow(1.5, attempts), maxInterval ); - attempts++; + if (interval < maxInterval) attempts++; await new Promise(resolve => setTimeout(resolve, interval)); }🤖 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/frontend/core/src/blocksuite/ai/runtime/request/service.ts` around lines 265 - 292, In pollContextDocsAndFiles, cap the exponential backoff exponent so attempts stops growing once the interval reaches maxInterval: compute a maxAttempts value from minInterval, maxInterval and the 1.5 factor (e.g. ceil(log(maxInterval/minInterval)/log(1.5))) and use Math.min(attempts, maxAttempts) when calculating the wait interval (or clamp attempts after incrementing). This keeps the interval calculation stable (uses the cappedAttempts variable in place of attempts) and prevents unbounded attempts growth while preserving existing minInterval/maxInterval behavior.packages/frontend/core/src/blocksuite/ai/components/playground/content.ts (1)
144-160: 💤 Low valueOptional: reuse a single runtime for bootstrap + fork.
When no root session exists, this path creates one runtime (lines 144–159) to make the root session and then
forkSession()(line 153) constructs and disposes another runtime. Functionally correct, but you allocate/dispose two runtimes back-to-back for a single bootstrap flow. IfAIChatRuntimebecomes more expensive to set up (subscriptions, request bindings, etc.), consider a single runtime that supports bothcreateSessioncalls, or haveforkSessionaccept an existing runtime to avoid the second construction.🤖 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/frontend/core/src/blocksuite/ai/components/playground/content.ts` around lines 144 - 160, The bootstrap flow creates and immediately disposes two AIChatRuntime instances: one via createSessionRuntime/ runtime.createSession and then forkSession which constructs its own runtime; to fix, change forkSession to accept an optional existing runtime (or expose a helper method) and use the already-created runtime for the fork call, then dispose the single runtime once; update the call site here to pass the runtime into forkSession (or call the new helper) and remove the extra runtime construction/disposal so only one runtime is allocated for both createSession and forkSession operations.packages/frontend/core/src/blocksuite/ai/runtime/chat/use-runtime.ts (1)
6-10: 💤 Low valueHoist no-op fallbacks to module scope for stable references.
The fallbacks
() => () => {}and() => nullare recreated on every render. Whenruntimeis null, this meansuseSyncExternalStoresees newsubscribe/getSnapshotidentities each render and will re-run subscribe. Functionally harmless (no-op subscribe), but unnecessary churn.♻️ Suggested refactor
+const noopSubscribe = () => () => {}; +const noopGetSnapshot = () => null; + export function useAIChatRuntime(runtime: AIChatRuntime | null) { const snapshot = useSyncExternalStore( - runtime?.subscribe ?? (() => () => {}), - runtime?.getSnapshot ?? (() => null), - runtime?.getSnapshot ?? (() => null) + runtime?.subscribe ?? noopSubscribe, + runtime?.getSnapshot ?? noopGetSnapshot, + runtime?.getSnapshot ?? noopGetSnapshot );Also verify
runtime.subscribeandruntime.getSnapshotare bound methods (or arrow-function properties) with stable identity — if they're plain class methods, passing them directly will losethisbinding insideuseSyncExternalStore.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/use-runtime.ts` around lines 6 - 10, Hoist the no-op fallback functions used in the useSyncExternalStore call into module-scope constants so their identities remain stable across renders: replace the inline fallbacks `() => () => {}` and `() => null` with named constants (e.g., NOOP_SUBSCRIBE and NOOP_GET_SNAPSHOT) and use those in the call to useSyncExternalStore(runtime?.subscribe ?? NOOP_SUBSCRIBE, runtime?.getSnapshot ?? NOOP_GET_SNAPSHOT, runtime?.getSnapshot ?? NOOP_GET_SNAPSHOT); additionally, ensure that runtime.subscribe and runtime.getSnapshot are bound methods or arrow properties (or bind them before passing) so they retain the correct this when passed directly into useSyncExternalStore.packages/frontend/core/src/blocksuite/ai/components/ai-chat-content/ai-chat-content.spec.ts (1)
51-76: 💤 Low valueTest reaches into prototype + raw
updated— note the brittleness.
Object.create(AIChatContent.prototype)plus(content as any).updated(...)bypasses Lit's reactive lifecycle and Lit's@property/@stateaccessors. This is fine for an isolated unit test, but it means:
- Any future change to
AIChatContentthat moves logic intowillUpdate,firstUpdated, or property setters will silently break this contract without the test catching it.- The test does not verify
runtimeSnapshotsetter behavior, only theupdatedreaction.Consider either instantiating via
customElements/fixture(e.g.,@open-wc/testing-helpers) or adding a focused note that this is an intentional white-box test ofupdated().🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-content/ai-chat-content.spec.ts` around lines 51 - 76, The test directly creates an AIChatContent instance via Object.create(AIChatContent.prototype) and calls (content as any).updated(...), which bypasses Lit's reactive property accessors and lifecycle (so changes to willUpdate/firstUpdated/property setters could break silently); either change the test to instantiate the component through the custom element lifecycle (use customElements.define fixture/fixture from `@open-wc/testing-helpers` or similar) and set runtimeSnapshot/chatContextValue via the element's reactive properties so Lit triggers updated/willUpdate/firstUpdated, or—if you intentionally want a white-box unit test—add a clear comment in the spec explaining this is a deliberate direct test of AIChatContent.updated and keep the mocked properties (_initializeScrollListeners, runtimeSnapshot, chatContextValue) and the direct call to updated; ensure the chosen approach references AIChatContent, updated, runtimeSnapshot, chatContextValue, and _initializeScrollListeners so future readers know why the test manipulates prototype methods directly.packages/frontend/core/src/blocksuite/ai/utils/action-reporter.ts (1)
4-6: ⚡ Quick winTighten the
hostparameter type for better type safety.
host?: unknowndiscards type information at every call site. All observable callers inai-chat-input.ts,ai-panel.ts,edgeless-response.ts,doc-handler.ts, andchat-actions-handle.tspass anEditorHostinstance. Typing the parameter asEditorHost | undefinedpreserves intellisense and catches type mismatches at compile time.♻️ Suggested change
-import type { ActionEventType } from '../provider'; +import type { EditorHost } from '@blocksuite/affine/std'; +import type { ActionEventType } from '../provider'; import { getAIRequestService } from '../runtime/request'; -export function reportResponse(event: ActionEventType, host?: unknown) { +export function reportResponse(event: ActionEventType, host?: EditorHost) { getAIRequestService().reportLastAction(event, host); }Note: The underlying
AIRequestService.reportLastActionalso acceptshost?: unknown. Consider tightening that signature as well to complete the type propagation.🤖 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/frontend/core/src/blocksuite/ai/utils/action-reporter.ts` around lines 4 - 6, The host parameter on reportResponse should be typed more narrowly—change reportResponse(event: ActionEventType, host?: unknown) to use the concrete EditorHost type (e.g. host?: EditorHost) to preserve intellisense and compile-time checks; update the call into getAIRequestService().reportLastAction accordingly and also tighten AIRequestService.reportLastAction's signature from host?: unknown to host?: EditorHost so type safety propagates across reportResponse, getAIRequestService().reportLastAction, and any callers (ai-chat-input.ts, ai-panel.ts, edgeless-response.ts, doc-handler.ts, chat-actions-handle.ts).packages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.spec.ts (2)
61-99: 💤 Low value
createRequestmock omitsforkChatfrom the default surface.The base mock doesn't include
forkChat, but the fork/playground tests rely on it viaoverrides. This works today because the cast goes throughas unknown as AIRequestService, but a future test that forgets to supplyforkChatwon't fail at type-check time and will crash with a less obvious "is not a function" error at runtime. Worth adding a defaultforkChat: vi.fn().mockResolvedValue(...)(or asserting on the strategy invocation differently) to make the mock contract complete.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/runtime.spec.ts` around lines 61 - 99, The createRequest test helper currently omits the forkChat method from its returned AIRequestService mock; add a default forkChat: vi.fn().mockResolvedValue(...) entry to the returned object (e.g., resolving to a session or appropriate value used by tests) so the mock fully implements AIRequestService and prevents runtime "is not a function" errors when overrides forget to supply forkChat; update the returned object inside createRequest to include this forkChat mock alongside the other methods.
49-59: 💤 Low value
waitUntilhelper only yields microtasks.The current loop awaits
Promise.resolve()and retries up to 10 microtask turns. For purely synchronous promise chains this is fine, but for anything that crosses a real task boundary (setTimeout,queueMicrotasknesting beyond ~10, orsetImmediate-style flushes) this can mask timing bugs as test flakes. Consider usingvi.waitFor(built into Vitest and aware of fake timers) which gives more reliable retry semantics:♻️ Suggested replacement
-async function waitUntil(assertion: () => void) { - for (let i = 0; i < 10; i++) { - try { - assertion(); - return; - } catch { - await Promise.resolve(); - } - } - assertion(); -} +// Use vi.waitFor directly at call sites, or: +const waitUntil = (assertion: () => void) => + vi.waitFor(assertion, { timeout: 1000, interval: 0 });🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/runtime.spec.ts` around lines 49 - 59, The helper waitUntil only yields microtasks by awaiting Promise.resolve(), which can hide timing bugs that require real task ticks; replace its manual retry loop with Vitest's vi.waitFor (or call vi.waitFor(() => { ... }, { timeout, interval })) so the test harness handles retries across real task boundaries and fake timers; update any usages of waitUntil in runtime.spec.ts to use vi.waitFor and remove the custom waitUntil function (or implement it by delegating to vi.waitFor) so tests reliably wait for async conditions.packages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-tabs.ts (1)
239-254: 💤 Low valueScroll-into-view selector falls back to draft testid only when
activeSessionIdis null.
activeSessionId ? [data-session-id="${activeSessionId}"] : [data-testid="ai-chat-draft-tab"]works, but note that ifactiveTabIdstarts withdraft:whileactiveSessionIdis also defined (e.g. transitional state), the wrong element gets scrolled into view. Probably benign today, but worth keying offactiveTabIdinstead for consistency with_renderDraftTab'sdata-activederivation.🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-tabs.ts` around lines 239 - 254, The updated() method uses runtimeSnapshot?.activeSessionId to pick the element to scroll, which mis-selects when the active tab is a draft (e.g. id starts with "draft:"); change the selector to use runtimeSnapshot?.activeTabId (or activeTabId) instead of activeSessionId so it matches how _renderDraftTab derives data-active, and fall back to querying `[data-testid="ai-chat-draft-tab"]` when activeTabId is null/undefined; keep the same scrollIntoView call and options.packages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-toolbar.ts (1)
25-59: 💤 Low value
runtimeSnapshot!is asserted non-null but the property type allows it.
accessor runtimeSnapshot!: AIChatSnapshot;uses a definite-assignment assertion, while several call sites (runtimeSnapshot: snapshot ?? runtime.getSnapshot()) defensively fall back exactly becausesnapshotfromuseAIChatRuntimecan benull | undefined. The gettersisGeneratingandcanCreateNewSessionthen dereference.status/.uiPolicy.canCreateNewSessiondirectly.If any consumer forgets the
?? runtime.getSnapshot()fallback (or sets the property before the runtime has emitted a first snapshot), this will throw on render. Consider either:
- Widening the type to
AIChatSnapshot | null | undefinedand guarding in the getters, or- Initializing with
runtime.getSnapshot()inconnectedCallbackso the invariant is enforced inside the component.🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-toolbar.ts` around lines 25 - 59, The runtimeSnapshot property is asserted non-null but can actually be null/undefined (accessed by isGenerating and canCreateNewSession), which can throw at render; fix by either widening accessor runtimeSnapshot's type to AIChatSnapshot | null | undefined and guard in the getters (e.g., check runtimeSnapshot before reading .status or .uiPolicy), or ensure the invariant by initializing runtimeSnapshot in connectedCallback with runtime.getSnapshot() (or a safe fallback) so isGenerating and canCreateNewSession can dereference safely; update references to runtimeSnapshot, isGenerating, canCreateNewSession, and implement the chosen guard/initialization in connectedCallback or the setter that receives snapshots.packages/frontend/core/src/blocksuite/ai/_common/chat-actions-handle.ts (1)
381-440: 💤 Low valueLGTM on the runtime/strategy migration and
dispose()infinally.The
ForkAIChatSessionStrategyflow plus thefinally { runtime.dispose() }cleanup is the right shape, and the host-awarereportResponse(...)updates align with the broader PR changes.One small thing to consider: when
curMode !== 'edgeless', the code switches the editor mode and immediately notifies the user, then continues to create the session and add the chat block. If the in-flight mode switch hasn't yet produced anaffine-surfacereachable bygetAllModels()in the same tick,addAIChatBlockwill silently return without inserting (and the handler returnsfalsewithout surfacing an error). Worth confirming this still works as intended on first invocation from page mode.🤖 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/frontend/core/src/blocksuite/ai/_common/chat-actions-handle.ts` around lines 381 - 440, The flow can silently fail if the editor mode is switched to 'edgeless' but the new affine-surface/model isn’t yet available before calling addAIChatBlock; update the handler (the code that invokes runtime.createSession, constructRootChatBlockMessages and addAIChatBlock with ForkAIChatSessionStrategy and runtime.dispose) to await the mode switch completion (i.e., wait until the editor reports the new surface/model is present via getAllModels() or equivalent) before calling addAIChatBlock, and if addAIChatBlock still returns falsy, surface a clear error/notification and log the failure (include context like newSessionId and viewportCenter) instead of returning false silently.packages/frontend/core/src/blocksuite/ai/components/ai-chat-composer/ai-chat-composer.ts (1)
629-639: 💤 Low value
syncChipsFromRuntime()afterawait dispatch('startContextPolling'/'pollEmbeddingStatus')is effectively a no-op.Both runtime actions kick off long-lived polling/streaming in the background and resolve immediately (see
runtime.startContextPolling/pollEmbeddingStatus). The follow-upsyncChipsFromRuntime()runs before any polled response lands; actual chip updates arrive later through the runtimesubscribe→updated()→syncChipsFromRuntimeSnapshotflow. The post-await sync is dead and slightly misleading.🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-composer/ai-chat-composer.ts` around lines 629 - 639, The post-await calls to syncChipsFromRuntime() in pollContextDocsAndFiles and pollEmbeddingStatus are no-ops because startContextPolling and pollEmbeddingStatus spawn background polling and resolve immediately; remove the synchronous syncChipsFromRuntime() calls from both methods so updates rely on the runtime subscribe → updated() → syncChipsFromRuntimeSnapshot flow instead, leaving the await/runtime.dispatch(...) calls intact to start the background work.packages/frontend/core/src/blocksuite/ai/components/ai-chat-input/ai-chat-input.ts (1)
836-868: 💤 Low valueContext fields are cleared before the runtime dispatch awaits.
updateContext({ images: [], quote: '', markdown: '' })runs synchronously beforeawait this.runtime.dispatch(...). If the dispatch rejects (network or internal error insideruntime.send's catch handler still reports via snapshot, but anything outside that path), the user's selection / images are already gone with no easy way to recover from the input UI. Consider clearing only after a successful dispatch resolution, or restoring on failure.🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-input/ai-chat-input.ts` around lines 836 - 868, The call to updateContext({ images: [], quote: '', markdown: '' }) clears user selections before awaiting this.runtime.dispatch, so failures lose state; move the context-clearing to after a successful dispatch (e.g., after await this.runtime.dispatch resolves and after this.onChatSuccess?.()), and implement a failure path that restores the previous context if the dispatch rejects (capture the current context before dispatch via updateContext/getter, then on catch revert it and rethrow or surface the error). Ensure you reference updateContext, this.runtime.dispatch, and this.onChatSuccess when making the changes.packages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts (2)
137-139: 💤 Low value
retryaction carriesmessageIdthat the runtime never uses.The
AIChatActionretrydiscriminant accepts amessageId(callers inai-chat-messages.tsandchat-block-peek-view.tspopulate it), butAIChatRuntime.retry()only ever retries the last assistant message and forwards no message id toexecuteAction('chat', { ... retry: true }). Either threadmessageIdinto the underlyingexecuteActioncall (so callers can retry an earlier message) or drop the field from the action shape to avoid misleading consumers.Also applies to: 408-446
🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts` around lines 137 - 139, The retry action includes a messageId but AIChatRuntime.retry() currently ignores it and always retries the last assistant message; update AIChatRuntime.retry() to accept and pass the messageId through into the executeAction('chat', { retry: true }) call (e.g., executeAction('chat', { retry: true, messageId })) so callers can retry a specific message, and update all call sites that invoke retry() to forward the action.messageId; alternatively, if you prefer to remove this capability, remove messageId from the AIChatAction 'retry' discriminant and from callers in ai-chat-messages.ts and chat-block-peek-view.ts so the API is not misleading.
950-954: 💤 Low value
isEmbeddingCompletedreturnstruewhentotal === 0.
embedded === totalevaluates totruefor{ embedded: 0, total: 0 }. Depending on howpollEmbeddingStatusreports an empty / not-yet-known status, this can flipembeddingCompletedon prematurely the moment polling starts. Consider requiringtotal > 0before reporting completion:Proposed change
private isEmbeddingCompleted(status: unknown) { if (!status || typeof status !== 'object') return false; const { embedded, total } = status as EmbeddingStatus; - return embedded === total; + return total > 0 && embedded === total; }🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts` around lines 950 - 954, The isEmbeddingCompleted function currently returns true when embedded === total, which treats {embedded:0,total:0} as complete; update isEmbeddingCompleted (and any callers like pollEmbeddingStatus / the embeddingCompleted flag) to additionally require total > 0 before returning true (i.e., only return true when typeof status === 'object' and (status as EmbeddingStatus).total > 0 and embedded === total) so a zero-total/unknown status won't mark completion prematurely.packages/frontend/core/src/blocksuite/ai/components/ai-chat-messages/ai-chat-messages.ts (2)
510-517: 💤 Low valueEmpty
messageIdon retry when last item is an action.When
messagesends with anisChatActionitem (or is empty),messageIdis dispatched as''. This is fine today becauseAIChatRuntime.retryignores the field, but it leaves a misleading dispatch payload. Either dropmessageIdfrom the dispatch when unknown, or have the runtime consume/validate it.🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-messages/ai-chat-messages.ts` around lines 510 - 517, The retry handler currently always includes a possibly-empty messageId string in the dispatch payload; update the retry method so it only includes messageId when the last item is a chat message (use isChatMessage(this.messages.at(-1)) to detect) and omit the messageId property entirely otherwise before calling this.runtime.dispatch; target the retry function and the dispatch call to ensure the payload doesn't contain messageId: '' when the messages array is empty or ends with an action.
241-246: 💤 Low valueBrittle error discrimination in
toAIError.
'type' in errorwill treat any thrown object with atypeproperty as anAIError. If non-AIError instances ever carry atypefield (custom error classes, plain objects rejected from a Promise, third-party errors), they'll be cast throughas AIErrorand the downstream rendering may break. Prefer aninstanceof AIErrorcheck (or a tagged-class check).Proposed change
- private toAIError(error: Error | null): AIError | null { - if (!error) return null; - return 'type' in error - ? (error as AIError) - : new GeneralNetworkError(error.message); - } + private toAIError(error: Error | null): AIError | null { + if (!error) return null; + return error instanceof AIError + ? error + : new GeneralNetworkError(error.message); + }🤖 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/frontend/core/src/blocksuite/ai/components/ai-chat-messages/ai-chat-messages.ts` around lines 241 - 246, The toAIError helper is using a brittle "'type' in error" check; change it to use a proper class check (error instanceof AIError) so only real AIError instances are returned as AIError, and otherwise return a new GeneralNetworkError. Update the toAIError function to: if error is an instance of AIError return it, else wrap the message (or String(error) if non-Error thrown) in new GeneralNetworkError(error.message || String(error)); ensure AIError is imported/available in the scope so instanceof works.packages/frontend/core/src/blocksuite/ai/peek-view/chat-block-peek-view.ts (1)
159-181: 💤 Low value
initSessionallocates a runtime just to callloadInitialSessionand immediately disposes it.
loadInitialSessiononly needs the strategy + request service; constructing and disposing a fullAIChatRuntime(with its abort controllers and listener set) on every mount just to call one method is wasteful. Either exposeloadInitialSessionon the strategy directly (it already receives scope + request), or reuse the runtime created later inensureRuntimeinstead of throwing one away.🤖 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/frontend/core/src/blocksuite/ai/peek-view/chat-block-peek-view.ts` around lines 159 - 181, initSession currently creates a full AIChatRuntime via createRuntime() just to call loadInitialSession then immediately disposes it; change this so we don't allocate a runtime only to throw it away: either call the strategy's loadInitialSession directly (construct a ChatBlockAIChatSessionStrategy, call its loadInitialSession(scope, getAIRequestService()) using the same scope shape as createRuntime provides) or reuse the runtime from ensureRuntime (store the runtime produced by createRuntime in a reusable field and call runtime.loadInitialSession() there instead of creating a transient runtime). Update initSession to use one of these approaches and remove the short-lived AIChatRuntime allocation; keep references to AIChatRuntime, createRuntime, initSession, ChatBlockAIChatSessionStrategy, ensureRuntime and loadInitialSession in your changes so the reviewer can find the related code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8fecb4c-ee0e-4586-8a29-f27ae54dfbfc
📒 Files selected for processing (63)
packages/frontend/core/src/blocksuite/ai/_common/chat-actions-handle.tspackages/frontend/core/src/blocksuite/ai/_common/config.tspackages/frontend/core/src/blocksuite/ai/actions/doc-handler.tspackages/frontend/core/src/blocksuite/ai/actions/edgeless-handler.tspackages/frontend/core/src/blocksuite/ai/actions/edgeless-response.tspackages/frontend/core/src/blocksuite/ai/ai-panel.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-composer/ai-chat-composer.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-content/ai-chat-content.spec.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-content/ai-chat-content.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-input/ai-chat-input.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-messages/ai-chat-messages.spec.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-messages/ai-chat-messages.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-messages/preload-config.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-tabs.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-chat-toolbar.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/ai-session-history.tspackages/frontend/core/src/blocksuite/ai/components/ai-chat-toolbar/configure-ai-chat-toolbar.tspackages/frontend/core/src/blocksuite/ai/components/ai-history-clear/ai-history-clear.tspackages/frontend/core/src/blocksuite/ai/components/ask-ai-toolbar.tspackages/frontend/core/src/blocksuite/ai/components/playground/chat.tspackages/frontend/core/src/blocksuite/ai/components/playground/content.tspackages/frontend/core/src/blocksuite/ai/entries/edgeless/actions-config.tspackages/frontend/core/src/blocksuite/ai/entries/edgeless/index.tspackages/frontend/core/src/blocksuite/ai/entries/space/setup-space.tspackages/frontend/core/src/blocksuite/ai/index.tspackages/frontend/core/src/blocksuite/ai/messages/error.tspackages/frontend/core/src/blocksuite/ai/peek-view/chat-block-peek-view.tspackages/frontend/core/src/blocksuite/ai/provider/ai-app-events.tspackages/frontend/core/src/blocksuite/ai/provider/ai-provider.tspackages/frontend/core/src/blocksuite/ai/provider/event-source.tspackages/frontend/core/src/blocksuite/ai/provider/index.tspackages/frontend/core/src/blocksuite/ai/provider/request.spec.tspackages/frontend/core/src/blocksuite/ai/provider/setup-provider.spec.tspackages/frontend/core/src/blocksuite/ai/provider/setup-provider.tsxpackages/frontend/core/src/blocksuite/ai/provider/tracker.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/actions.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/index.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.spec.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/session-strategy.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/state.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/use-element.spec.tsxpackages/frontend/core/src/blocksuite/ai/runtime/chat/use-element.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/use-runtime.tspackages/frontend/core/src/blocksuite/ai/runtime/request/action-definitions.tspackages/frontend/core/src/blocksuite/ai/runtime/request/byok-local-lease.tspackages/frontend/core/src/blocksuite/ai/runtime/request/copilot-client.spec.tspackages/frontend/core/src/blocksuite/ai/runtime/request/copilot-client.tspackages/frontend/core/src/blocksuite/ai/runtime/request/index.tspackages/frontend/core/src/blocksuite/ai/runtime/request/message-transport.tspackages/frontend/core/src/blocksuite/ai/runtime/request/provider.tspackages/frontend/core/src/blocksuite/ai/runtime/request/service.spec.tspackages/frontend/core/src/blocksuite/ai/runtime/request/service.tspackages/frontend/core/src/blocksuite/ai/utils/action-reporter.tspackages/frontend/core/src/blocksuite/ai/widgets/edgeless-copilot/index.tspackages/frontend/core/src/components/providers/workspace-side-effects.tsxpackages/frontend/core/src/desktop/pages/workspace/chat-panel-utils.tspackages/frontend/core/src/desktop/pages/workspace/chat/index.tsxpackages/frontend/core/src/desktop/pages/workspace/detail-page/detail-page.tsxpackages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat-panel-session.spec.tspackages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat-panel-session.tspackages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat.tsxpackages/frontend/core/src/modules/peek-view/view/doc-preview/doc-peek-view.tsx
💤 Files with no reviewable changes (6)
- packages/frontend/core/src/blocksuite/ai/provider/request.spec.ts
- packages/frontend/core/src/blocksuite/ai/provider/setup-provider.spec.ts
- packages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat-panel-session.spec.ts
- packages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat-panel-session.ts
- packages/frontend/core/src/desktop/pages/workspace/chat-panel-utils.ts
- packages/frontend/core/src/blocksuite/ai/provider/ai-provider.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #14937 +/- ##
==========================================
- Coverage 58.82% 58.08% -0.74%
==========================================
Files 3209 3216 +7
Lines 176370 176748 +378
Branches 26105 26226 +121
==========================================
- Hits 103743 102663 -1080
- Misses 69184 70337 +1153
- Partials 3443 3748 +305
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts (2)
465-475: 💤 Low valueConsider not reporting cancellation as
success.When the user stops an in-flight stream, the assistant message is left with whatever partial content arrived and
statusis set to'success'. UI components keying offstatus === 'success'will then treat a truncated response as a completed one (e.g., enabling regenerate/copy/finalized rendering). An'idle'(or dedicated'stopped') terminal state would more accurately describe a user-initiated cancellation and avoid surprising downstream consumers.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts` around lines 465 - 475, The stop() method currently treats a user-initiated stream abort as a successful completion by incrementing requestSeq, aborting streamAbortController, and calling commit({ status: 'success' }); instead change the terminal status to reflect cancellation (e.g., 'idle' or a new 'stopped' state) so downstream consumers don't treat truncated content as finalized: in stop(), after streamAbortController?.abort() set streamAbortController = null and call commit({ status: 'idle' }) or commit({ status: 'stopped' }) when snapshot.status is 'loading' or 'transmitting' (update any related type/enum definitions and consumers if you add 'stopped'), and ensure requestSeq handling remains unchanged.
565-629: 🏗️ Heavy liftShared
contextRequestSeqlets background polling silently drop user add/remove updates.
addContextItem,removeContextItem,loadContext, andpollContextall bump and check the samecontextRequestSeq. IfpollContext(triggered bypollContextUntilIdlewhile embeddings are processing) fires during an in-flightaddContextItem, it invalidates that add — the persisted item never reachessnapshot.composer.context.items. BecausemergePolledContextItemsonly updates items already present in the snapshot (it iteratesthis.snapshot.composer.context.items), the new item then stays invisible until the nextloadContext. The same race exists forremoveContextItem(a still-present item won't be cleared).Two reasonable directions:
- Use a dedicated sequence per write op (e.g.,
contextWriteSeqfor add/remove,contextReadSeqfor poll/load) so background polling can't cancel user-initiated writes; or- Have
mergePolledContextItemsalso surface server items that aren't yet in the snapshot, so polling can reconcile the missing entry even after a write was cancelled.🤖 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/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts` around lines 565 - 629, The shared contextRequestSeq causes pollContext to cancel in-flight add/remove calls; split the sequence into separate read/write sequences and use them accordingly: introduce contextReadSeq and contextWriteSeq (replacing uses of contextRequestSeq in polling/load flows with contextReadSeq, and in addContextItem/removeContextItem with contextWriteSeq), increment and capture the appropriate seq at start of pollContext/loadContext (use contextReadSeq) and addContextItem/removeContextItem (use contextWriteSeq), and compare the matching seq before applying updates; alternatively, if you prefer reconciling in polling, update mergePolledContextItems to include server-side items not present in this.snapshot.composer.context.items so pollContext can't permanently hide newly persisted items—apply one of these fixes referencing addContextItem, removeContextItem, pollContext, loadContext, mergePolledContextItems, and the former contextRequestSeq.tests/affine-cloud-copilot/e2e/utils/settings-panel-utils.ts (1)
229-229: ⚡ Quick winWorkspace ID extraction is fragile; consider more robust parsing.
The expression
page.url().split('/').slice(-2)[0] || ''assumes a specific URL structure. If the URL format changes (e.g., additional path segments, query parameters, or different routing structure), this could silently fail by passing an empty string tocleanupWorkspace.🔧 Suggested refactor for robust workspace ID extraction
- await cleanupWorkspace(page.url().split('/').slice(-2)[0] || ''); + // Extract workspace ID from URL path segments + const url = new URL(page.url()); + const pathSegments = url.pathname.split('/').filter(Boolean); + const workspaceIndex = pathSegments.indexOf('workspace'); + const workspaceId = workspaceIndex !== -1 && pathSegments[workspaceIndex + 1] + ? pathSegments[workspaceIndex + 1] + : ''; + await cleanupWorkspace(workspaceId);Alternatively, if the workspace ID is always in a consistent position, use a more explicit extraction:
- await cleanupWorkspace(page.url().split('/').slice(-2)[0] || ''); + const workspaceId = page.url().match(/\/workspace\/([^\/]+)/)?.[1] || ''; + await cleanupWorkspace(workspaceId);🤖 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 `@tests/affine-cloud-copilot/e2e/utils/settings-panel-utils.ts` at line 229, The workspace ID extraction is fragile; replace the inline split with robust parsing using the URL API: call new URL(page.url()).pathname, split on '/' and filter out empty segments, then select the correct segment (e.g., last non-empty segment or the one following a known path like 'workspaces') before passing it to cleanupWorkspace(pageId). Update the call site where cleanupWorkspace is invoked and ensure you handle cases where no ID is found (pass '' or throw/log) so cleanupWorkspace receives a valid, deterministic workspaceId.
🤖 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 `@packages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts`:
- Around line 405-419: The post-success awaits (refreshLastMessageId and
bindActiveSessionToDoc) inside the same try block can reject and trigger the
outer catch, overwriting the just-committed success; in both send and retry
functions, move those calls out of the try/catch that sets status to 'success'
(or replace their awaits with non-throwing patterns) so failures cannot flip
status: after commit({status:'success', ...}) call
refreshLastMessageId(session.sessionId) and bindActiveSessionToDoc() via promise
chaining with .catch(console.error) (or wrap each in its own try/catch that logs
via this.toError/processLogger) so errors are surfaced only as logs and do not
commit status:'error'.
---
Nitpick comments:
In `@packages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.ts`:
- Around line 465-475: The stop() method currently treats a user-initiated
stream abort as a successful completion by incrementing requestSeq, aborting
streamAbortController, and calling commit({ status: 'success' }); instead change
the terminal status to reflect cancellation (e.g., 'idle' or a new 'stopped'
state) so downstream consumers don't treat truncated content as finalized: in
stop(), after streamAbortController?.abort() set streamAbortController = null
and call commit({ status: 'idle' }) or commit({ status: 'stopped' }) when
snapshot.status is 'loading' or 'transmitting' (update any related type/enum
definitions and consumers if you add 'stopped'), and ensure requestSeq handling
remains unchanged.
- Around line 565-629: The shared contextRequestSeq causes pollContext to cancel
in-flight add/remove calls; split the sequence into separate read/write
sequences and use them accordingly: introduce contextReadSeq and contextWriteSeq
(replacing uses of contextRequestSeq in polling/load flows with contextReadSeq,
and in addContextItem/removeContextItem with contextWriteSeq), increment and
capture the appropriate seq at start of pollContext/loadContext (use
contextReadSeq) and addContextItem/removeContextItem (use contextWriteSeq), and
compare the matching seq before applying updates; alternatively, if you prefer
reconciling in polling, update mergePolledContextItems to include server-side
items not present in this.snapshot.composer.context.items so pollContext can't
permanently hide newly persisted items—apply one of these fixes referencing
addContextItem, removeContextItem, pollContext, loadContext,
mergePolledContextItems, and the former contextRequestSeq.
In `@tests/affine-cloud-copilot/e2e/utils/settings-panel-utils.ts`:
- Line 229: The workspace ID extraction is fragile; replace the inline split
with robust parsing using the URL API: call new URL(page.url()).pathname, split
on '/' and filter out empty segments, then select the correct segment (e.g.,
last non-empty segment or the one following a known path like 'workspaces')
before passing it to cleanupWorkspace(pageId). Update the call site where
cleanupWorkspace is invoked and ensure you handle cases where no ID is found
(pass '' or throw/log) so cleanupWorkspace receives a valid, deterministic
workspaceId.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09758eaf-6d90-42ba-99cc-0ad0cc2f52d8
📒 Files selected for processing (8)
packages/frontend/core/src/blocksuite/ai/components/ai-history-clear/ai-history-clear.tspackages/frontend/core/src/blocksuite/ai/components/playground/content.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.spec.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/session-strategy.tspackages/frontend/core/src/blocksuite/ai/runtime/chat/use-runtime.tspackages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat.tsxtests/affine-cloud-copilot/e2e/utils/settings-panel-utils.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/frontend/core/src/blocksuite/ai/components/playground/content.ts
- packages/frontend/core/src/blocksuite/ai/components/ai-history-clear/ai-history-clear.ts
- packages/frontend/core/src/blocksuite/ai/runtime/chat/use-runtime.ts
- packages/frontend/core/src/blocksuite/ai/runtime/chat/session-strategy.ts
- packages/frontend/core/src/desktop/pages/workspace/detail-page/tabs/chat.tsx
- packages/frontend/core/src/blocksuite/ai/runtime/chat/runtime.spec.ts
1 participant
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit
New Features
Improvements