feat(word-addin): align chat UI with web app - #299
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
amal66
left a comment
amal66
left a comment
There was a problem hiding this comment.
Multi-agent review — 6 analysts + 6 adversarial verifiers
Each dimension (backend API, security/privacy, UI alignment with the web app, Office.js/tracked edits, chat runtime/streaming, tests/tooling) was analyzed by a dedicated agent, and every finding was then re-checked against the code by an independent verifier instructed to refute it. Of 38 raw findings, none were refuted, one is marked uncertain, two were downgraded, and cross-analyst duplicates were merged, leaving 34 findings: 2 high, 10 medium, 16 low, 6 nit. No critical or security-breaking issues — auth, ownership checks, and RLS on the new word-chat tables held up.
Overall: solid, shippable feature with two high-severity fixes needed first:
- Header pills have no background/hover state —
LiquidActionRowusesapp-surfaceutilities whose tokens don't exist in the add-in bundle. - Section navigation destroys the live conversation while keeping its
chatId, so later sends are appended to a chat the user can no longer see.
Recurring themes: the vendor/ → src/shared/ move turns the shared design system into a declared local fork with drift already visible; wordChat.ts duplicates ~150 lines of chat.ts streaming orchestration (drift already started); debug console.logs ship in the production pane (including full user message content and a literal "This is not supposed to run"); and the document-identity UUID embedded in the .docx travels with every copy of the file.
Findings not anchorable to a diff line (4)
[Medium] Backend CORS allowlist has no entry for a deployed add-in origin — backend/src/app.ts:118
The API's CORS allowlist contains exactly one origin (FRONTEND_URL, default http://localhost:3000). This PR introduces a production build path for the add-in served from WORD_ADDIN_PUBLIC_URL — a different HTTPS origin — whose bundle calls REACT_APP_API_BASE_URL directly (the webpack same-origin proxy exists only in dev). A deployed task pane's fetches to /word-chat will be blocked by the browser because the response carries no Access-Control-Allow-Origin for the add-in's origin. Neither the PR's backend changes nor word-addin/README's 'Production build' section addresses this, which pressures operators toward the unsafe workaround of wildcarding CORS instead of adding a second explicit allowed origin (e.g. a WORD_ADDIN_URL env var).
Suggested fix: Add an optional WORD_ADDIN_URL (or comma-separated ALLOWED_ORIGINS) env var to the allowedOrigins set, and document in word-addin/README's Production section that the API must either share the add-in's origin behind a reverse proxy or explicitly allowlist WORD_ADDIN_PUBLIC_URL.
[Medium] E2E hermeticity depends on enumerating eager endpoints; no catch-all guard — word-addin/e2e/support/fixtures.ts:169
The fixture registers default routes for exactly the four requests known to fire on mount (user/api-keys, models/ollama, user/profile, GET word-chat?*), each with a comment explaining it exists because an unmocked request escapes to a developer's live backend where "a 401 would clear the seeded session mid-test". That failure mode was clearly hit repeatedly, yet there is still no last-resort route: the next component that fetches on mount will silently leak to localhost:3001/54321 again and produce environment-dependent flakes rather than a loud failure.
Suggested fix: Register one catch-all route first (so it matches last in Playwright's newest-first order) that aborts or fulfills-with-500 any request to the API/Supabase origins, making an unmocked call fail the test deterministically.
[Low] Office mock cannot catch missing load()/sync() or Word search semantics, and the PR removes the only real-Word run — word-addin/e2e/support/office-mock.ts:315
In the shim, context.sync() is () => Promise.resolve() and every load() is a no-op, so all proxy properties are always readable. Real Word throws PropertyNotLoaded/GeneralException when a property is read without load()+sync or when a range handle goes stale outside the seeded opt-in knobs — a whole class of Word.run misuse passes the 4,750-line suite and fails only in the host. body.search is plain case-folded substring matching (lines 533-549), so Word's ^-control-code parsing and its ~255-char search failure never occur in tests (the app's guard at useWordDoc.ts:830 is itself only validated against the mock). At the same time the PR deletes e2e-live/live-demo.spec.ts and playwright.live.config.ts, leaving no automated execution against real Office at all — only manual-session.mjs.
Suggested fix: Cheap fidelity upgrade: have mock ranges/collections throw on property reads unless load() was called and a sync() has since resolved — this catches the most common real-Word breakage class. Document in the README that redline/tracked-edit changes require a manual pass via e2e-live/manual-session.mjs.
[Low] Fixed-delay timing patterns in streaming and scroll tests are flake/weak-assertion risks — word-addin/e2e/chat.spec.ts:833
The stop-control test holds the /word-chat response for a fixed 1500ms (holdMs: 1500) and must complete ~8 assertions including two expect.poll calls inside that window; on a loaded CI worker the stream can resolve mid-assertion and the Stop button disappears (masked locally by retries: 2 on CI only). The scroll-spacer test (lines 372-391) uses two fixed 750ms waits with a <2px pixel tolerance on animated scroll positions. Separately, several negative assertions are gated on 50-100ms settles (chat.spec.ts:191, 1277; history.spec.ts:228, 342), which only prove the regression didn't appear within that window.
Suggested fix: Replace holdMs with an explicitly released gate promise inside the route handler — auth.spec.ts already demonstrates this pattern with refreshGate in the sign-out-during-refresh test — and poll for positional stability instead of sleeping twice.
🤖 Generated with Claude Code
| return ( | ||
| <div | ||
| className={cn( | ||
| "flex shrink-0 items-center gap-2 rounded-full border border-white/70 bg-app-surface px-1 py-1 shadow-[0_8px_24px_rgba(15,23,42,0.06)] backdrop-blur-2xl", |
There was a problem hiding this comment.
[High] LiquidActionRow uses app-surface utilities whose tokens are not defined in the add-in, so header pills have no background and no hover/active feedback
UI alignment · independently verified
LiquidActionRow/LiquidIconButton/LiquidTextButton (the floating header's menu bubble, New chat, Chat history, workflow Use/Back controls) use bg-app-surface, hover:bg-app-surface-hover, and active:bg-app-surface-active. The web app defines --color-app-surface / -hover / -active in frontend/src/app/globals.css @theme inline, but the add-in's copied token sheet (word-addin/src/shared/styles/tokens.css) omits every --app-* token. Under Tailwind v4, utilities referencing unknown theme colors are silently not generated, so the pill row renders transparent (border+shadow only, unlike the web's near-white #fdfdfe surface) and none of the header buttons show any hover or pressed state. Verified empirically: compiling word-addin/src/taskpane/styles.css through @tailwindcss/postcss emits no rule for bg-app-surface or app-surface-hover while bg-primary is emitted.
Suggested fix: Add the --app-background/--app-surface/--app-surface-hover/--app-surface-active/--app-floating custom properties and their @theme inline --color-app-* mappings to word-addin/src/shared/styles/tokens.css (copy the block from globals.css), or switch LiquidActionRow to tokens that exist in the add-in sheet.
Evidence
Compiled the add-in stylesheet with its own @tailwindcss/postcss: .bg-app-surface generated: false, app-surface-hover generated: false, .bg-primary generated: true. grep confirms --app-surface is defined nowhere under word-addin/; frontend/src/app/globals.css defines --app-surface: #fdfdfe etc. and the web's liquid-dropdown/AppSidebar rely on those same classes.
|
|
||
| {/* Active tab content */} | ||
| <div className="flex flex-1 flex-col overflow-hidden">{renderTab()}</div> | ||
| {renderSection()} |
There was a problem hiding this comment.
[High] Section navigation destroys the live conversation while keeping its chatId, silently appending later sends to the invisible chat
Chat runtime · independently verified
App renders exactly one section via renderSection(), so ChatPanel (and useWordAssistantChat, which owns the messages array in hook state) unmounts whenever the user opens Quick Actions, Workflows, History, or Settings from the header menu. changeSection() intentionally does NOT reset chatId/initialMessages (only startNewChat/openSelectedChat do). On returning to the Assistant section, ChatPanel remounts and the hook re-seeds messages from the stale initialMessages prop — [] for any chat started in this session, or the original history snapshot for a chat opened from history. The user sees a blank initial view (or stale snapshot), yet chatId still points at the old chat: the next send posts history=[newUserMessage] to the same chat_id, so (1) the model loses all prior conversational context (backend builds context only from client-sent messages), and (2) in cloud/local storage the new turn is appended to a chat the user believes is gone — history later shows one chat mixing both 'conversations'. Also, if a stream is in flight when the section switches, the unmount cleanup aborts it with no warning. This is an everyday flow (peek at Quick Actions or History mid-chat, or the designed Workflows → 'Use' → chat path) — users will hit it. The e2e suite (chat.spec.ts section-menu tests) exercises the menu but never asserts conversation survival across a round trip.
Suggested fix: Either keep ChatPanel mounted (render all sections, hide inactive ones), lift messages state up to App next to chatId, or on ChatPanel remount with a non-null chatId re-fetch the chat (getCloudWordChat/getLocalWordChat) into initialMessages before allowing a send.
Evidence
App.tsx:90-137 renderSection() returns a single section component; :153-159 changeSection() only clears workflow state, never chatId/initialMessages/sessionKey; useWordAssistantChat.ts:59 messages live in hook state, :69-78 unmount aborts the stream, :80-95 mount effect re-seeds from initialMessages. No code path reloads messages for a non-null chatId on remount (getCloudWordChat/getLocalWordChat are only called from ChatHistoryList on explicit selection).
| assistantMessageId, | ||
| })}\n\n`, | ||
| ); | ||
| const { fullText, events, citations } = await runLLMStream({ |
There was a problem hiding this comment.
[Medium] ask_inputs tool is exposed to /word-chat but the route and add-in cannot handle it
Backend · independently verified
runLLMStream always includes the full TOOLS set (streaming.ts:201 baseTools = [...TOOLS, ...]), which contains ask_inputs (toolSchemas.ts:125) whose description tells the model to use it 'when required documents have not been attached'. The word route only disables research tools (includeResearchTools: false). Unlike chat.ts, wordChat.ts never parses ask_inputs_response and the add-in has no ask_inputs UI (grep of word-addin/src returns zero hits; wordPrompt.ts never mentions it). If the model calls ask_inputs in a Word chat — plausible, since the composer supports document sources — the stream pauses (AssistantStreamAskInputsPause), persists an event the client can't render, and the user sees the assistant silently stop with no way to answer the question. The messageTable parameter added to appendAskInputsResponseToLastAssistantMessage suggests this was anticipated but never wired up.
Suggested fix: Either filter ask_inputs out of the tool set for /word-chat (add an option like includeAskInputs to runLLMStream), or instruct against it in buildWordChatSystemPrompt and add ask_inputs_response support to the route + add-in.
Evidence
wordChat.ts POST body parsing has no parseOptionalAskInputsResponse (lines 204-229); streaming.ts:199-201 builds tools as [...TOOLS, ...researchTools, ...WORKFLOW_TOOLS] with no way to exclude ask_inputs; grep -rn ask_inputs word-addin/src → no matches.
| }); | ||
|
|
||
| // POST /word-chat — Word-specific streaming endpoint. | ||
| wordChatRouter.post("/", requireAuth, async (req, res) => { |
There was a problem hiding this comment.
[Medium] POST /word-chat duplicates ~150 lines of chat.ts streaming orchestration — drift already visible
Backend · independently verified
The reservation insert, 3-attempt updateAssistantMessage retry helper, SSE header block, abort-close wiring, AssistantStreamError partial-persistence, error-event persistence, and title-truncation logic in wordChat.ts:359-514 are near-verbatim copies of the same code added to chat.ts:526-720 in this same PR (updateReservedAssistantMessage, identical retry loop, identical catch structure). Any future fix to one (e.g., backoff in the retry loop, changed error semantics, [DONE] ordering) must be manually mirrored. Drift has already started: chat.ts hydrates edit statuses on read and handles ask_inputs; wordChat.ts bumps updated_at while chat.ts does not — differences that are intentional today but indistinguishable from bugs tomorrow.
Suggested fix: Extract a shared helper (e.g., persistStreamedAssistantTurn({ db, table, chatId, assistantMessageId, write }) plus a shared runStreamedChatTurn wrapper) parameterized by message table, the same way contextBuilders.ts was parameterized with messageTable in this PR.
Evidence
Compare wordChat.ts:384-400 with chat.ts updateReservedAssistantMessage (identical 3-iteration retry over .update().eq(id).eq(chat_id)), and the catch blocks wordChat.ts:466-510 vs chat.ts's abort/error persistence — same structure, different table name and log prefix.
| * the web app still keeps its own copy until the shared package lands. Keep | ||
| * values in sync with globals.css. Import AFTER `@import "tailwindcss";` so | ||
| * the `@theme` mappings and base layer apply. | ||
| * A local subset of the web app's tokens (frontend/src/app/globals.css), |
There was a problem hiding this comment.
[Medium] PR converts the shared design system from a keep-in-sync vendored copy into a declared local fork, and drift is already visible
UI alignment · independently verified
main aliased word-addin/src/vendor/shared as @mike/shared with a header saying 'A verbatim subset of the web app's tokens... Keep values in sync'; this PR renames it to src/shared and rewrites the header to 'A local subset... adapted for the Word add-in', dropping the @mike/shared alias from webpack.config.js and tsconfig.json entirely. Given the project rule that all client UI/UX must mirror the main web app and the intended single-shared-package direction, this formalizes a fork, and divergence has already begun: shared/chat/ChatInput.tsx is a new composer shell with a leftSlot/rightSlot/attachments API the web's frontend/src/app/components/assistant/ChatInput.tsx does not have; shared/ui/button.tsx dropped the web button's cva variant/size API (frontend/src/app/components/ui/button.tsx) down to a single hardcoded style; the model catalog is re-declared as STATIC_MODELS in the add-in ModelToggle instead of sharing the web's exported MODELS list, so the next model launch must be edited in two places or the surfaces silently disagree.
Suggested fix: Keep the 'keep in sync with ' provenance comments on every duplicated file (tokens.css lost its sync mandate; button.tsx and ChatInput.tsx have none), and track the shared-package extraction so the fork is explicitly temporary. At minimum restore a sync checklist (tokens, MODELS list, composer classes) in word-addin/README.md.
Evidence
git diff main...HEAD tokens.css comment change; webpack.config.js diff removes the '@mike/shared' alias block; word-addin ModelToggle.tsx:25-35 duplicates frontend ModelToggle.tsx:25-36 verbatim; word-addin/src/shared/ui/button.tsx (21 lines, no variants) vs frontend ui/button.tsx (cva with 6 variants/6 sizes).
| attachments, | ||
| className, | ||
| }: ChatInputProps) { | ||
| const textareaRef = React.useRef<HTMLTextAreaElement>(null); |
There was a problem hiding this comment.
[Nit] Minor copy and affordance divergences from the web composer/initial view
UI alignment · independently verified
Placeholder is 'Ask Mike…' vs the web's 'How can I help?'; the greeting reads 'Hello, {name}' vs the web's 'Hi, {username}' and falls back to 'there' instead of the email prefix; the add-in ModelToggle omits the Check glyph the web renders next to the selected available model (relying on the gray selected-row background instead); and the web's workflow slash-commands (typing '/', with combobox aria wiring) have no counterpart in the pane composer.
Suggested fix: Align the two copy strings, add the selected-item Check to the add-in DropdownItem, and note slash commands as a known scope cut (or port them, since WorkflowModal already loads the workflow list).
Evidence
shared/chat/ChatInput.tsx:46 placeholder default; InitialView.tsx:89 'Hello, {name}' vs frontend InitialView.tsx:179 'Hi, {username}'; frontend ModelToggle.tsx:131-133 renders Check for the selected item, add-in ModelToggle.tsx renders none; frontend ChatInput.tsx:26-34 slash-command imports have no add-in equivalent.
| * All functions return Promises and must be called in a component context | ||
| * where Office.js has already initialised (i.e. inside Office.onReady). | ||
| */ | ||
| export function useWordDoc() { |
There was a problem hiding this comment.
[Nit] useWordDoc returns new function identities every render, defeating downstream useCallback memoization
Office.js · independently verified
readDocumentText and applyTrackedEdits are re-created on each render (no useCallback/useMemo). useWordTrackedEdits.applyStreamedEdit lists applyTrackedEdits as a dependency (useWordTrackedEdits.ts:244), so applyStreamedEdit and processLiveRedlines get new identities every render as well. No correctness impact today because stream callbacks capture them at send time, but it silently disables the memoization the consumers wrote and will bite if these ever land in an effect dependency list.
Suggested fix: Hoist readDocumentText/applyTrackedEdits to module scope (they use no hook state) or wrap them in useCallback with empty deps.
Evidence
useWordDoc.ts:758-1082 — plain const arrow functions returned from the hook body; both are stateless wrappers over module-level functions.
| } | ||
|
|
||
| /** True when a thrown value is a transport failure rather than an HTTP status. */ | ||
| export function isNetworkFailure(error: unknown): boolean { |
There was a problem hiding this comment.
[Nit] isNetworkFailure is dead code and would misclassify the client's own wrapped transport errors
Chat runtime · independently verified
isNetworkFailure tests error instanceof TypeError, but api/client.ts sendRequest (lines 73-88) re-wraps every transport failure into a plain Error (with the TypeError as cause), so any future caller passing an error thrown by the API client would get false for genuine network failures. No code currently calls it (grep finds zero callers outside the module), making it a misleading exported API.
Suggested fix: Delete the unused export, or make it cause-chain aware (walk error.cause looking for TypeError) if it is meant to be used.
Evidence
networkError.ts:49-51 return error instanceof TypeError;; client.ts:79-87 throw new Error(describeNetworkFailure(...), { cause: error }) replaces the TypeError before callers see it.
| await expect(modal.locator('img[src*="/icons/pdf."]')).toBeVisible(); | ||
| const uploadedRow = modal.getByRole("button", { name: /agreement\.pdf/ }); | ||
| await expect(uploadedRow).toHaveCSS( | ||
| "background-color", |
There was a problem hiding this comment.
[Nit] Assertions pin exact oklch computed colors, mirroring Tailwind palette constants
Tests & tooling · independently verified
17 toHaveCSS assertions in chat.spec.ts check literal resolved colors such as oklch(0.928 0.006 264.531) for selected/unselected document rows. These encode Tailwind's current palette math rather than behavior: any design-token or Tailwind version bump fails the suite with no functional change, and the assertion says nothing about what the color means.
Suggested fix: Assert selection state via aria-selected/aria-pressed or a data-state attribute, and keep at most one CSS assertion where the visual itself is the contract.
Evidence
chat.spec.ts lines 975-986: three consecutive toHaveCSS("background-color", "oklch(...)") calls toggling a row's selection.
| "typecheck": "tsc --noEmit", | ||
| "build": "webpack --mode production && node scripts/build-manifest.js", | ||
| "predev": "node scripts/clear-sideload.js", | ||
| "dev": "office-addin-debugging start manifest.xml --dev-server \"bun run dev:server\" --dev-server-port 3000", |
There was a problem hiding this comment.
[Nit] dev script hardcodes bun; loadEnvFile needs newer Node than errors will explain
Tests & tooling · independently verified
"dev": "office-addin-debugging start ... --dev-server \"bun run dev:server\"" requires bun on PATH while every sibling script (including the otherwise-identical start) uses npm — a bun-less contributor's npm run dev fails inside office-addin-debugging with an opaque spawn error. Relatedly, webpack.config.js:17 calls process.loadEnvFile (Node >=20.12); engines declares >=22 but nothing enforces it, so older Nodes die with process.loadEnvFile is not a function instead of a version message.
Suggested fix: Make dev use npm run dev:server (or detect bun), and guard loadEnvFile with a typeof check that prints the required Node version.
Evidence
package.json scripts block: dev uses bun run dev:server, start uses npm run dev:server; webpack.config.js line 17 process.loadEnvFile(localEnvPath).
…roll fix(word-addin): WKWebView completion scroll jump + streaming performance (stacked on #299)
7fd0f92 to
ab4726a
Compare
|
Follow-up to the four unanchored findings in the review body (implemented in
Verification for the review-fix commit: add-in typecheck passed, backend build passed, 35 focused backend route/CORS tests passed, the full backend suite passed (555 tests), and the full add-in browser suite passed (112 tests). |
|
Flagging one piece of tech debt we're accepting in this PR: the shared UI is now hand-forked from the web app. With We're not blocking this PR on it. The plan is a follow-up that extracts the import-clean leaf components (mike-icon, toggle-switch, tab-pill-button, input, PillButton at minimum) into a local shared package both apps consume, drawing on the shared-UI package extraction indexed in #205. Until then, please avoid growing the forked set further — genuinely pane-specific adaptations (ChatView, EditCard) stay local, everything else should wait for the package. |
fix(word-addin): review fixes for #299 — WebKit e2e gate, streaming perf, and correctness edges
…efend it against pane resizes
WHY THIS MATTERS
When you send a message in the assistant pane, the transcript scrolls so your
turn "pins" near the top while the answer streams in below it. Users reported
the pinned turn drifting to different heights per turn and sliding when the
response's activity strip changed size.
WHAT IS A PIN LINE (and what was wrong)
The pin is built from two cooperating pieces:
1. a scroll target — where the turn should sit (`element.offsetTop - N`), and
2. a reserved spacer under the turn (min-height on the assistant row) that
guarantees the scroll range can actually reach that position.
The old code targeted N=24px but sized the spacer against the container's
80px top padding, so the maximum scroll only reached `offsetTop - 80`. The
turn's resting position became a function of the answer's height:
`max(24, 80 - (answerHeight - spacer))` — short answers rested at 80, long
ones climbed to 24, and any shrink slid the turn back down. The web assistant
avoids this by keeping the target and the container padding on the same line;
the add-in had copied the constant but not the invariant.
HOW IT WORKS NOW
- `PIN_TOP_OFFSET = 80` — one constant, documented as "must equal the
container's pt-20" — is used by the live pin scroll, the restored-history
scroll, and `measureSpacerPx`. With both pieces on the same line, the
minimum scroll range lands exactly on the pin position for every answer
height, so post-completion shrinking can never clamp the turn away.
- Two ResizeObserver watchdogs (container box + active assistant row)
re-assert the anchored position when the pane or the streaming row resizes,
and restore the user's own position (clamped to the new range) once the
user has scrolled away. Scroll "ownership" is tracked in refs
(`anchorActiveRef`, `desiredScrollTopRef`): the app owns the anchor until
wheel/touch/pointer/keyboard input hands it to the user.
- `UserMessage` clamps long prompts from the first painted frame so the
spacer measurement and the painted layout can never disagree.
VERIFICATION TOOLING
- e2e/chat-layout.spec.ts gains completion-transition and bottom-arrow specs.
- playwright.webkit.temp.config.ts runs the same suite under WebKit — the
engine the Office task pane actually uses (WKWebView) — where Chromium-only
runs cannot observe engine scroll adjustments.
- docs/word-addin-chat-scroll-report.md records the full investigation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… position, and cut streaming render cost
WHY THIS MATTERS
In real Word (not in our Chromium e2e), the moment a response finished — the
activity strip flipping from "Working" to "Completed in N steps" — the whole
transcript jumped away from the pinned turn. Streaming also felt sluggish.
Both had the same underlying theme: work and behavior that only shows up in
the Office WebView under real streaming load.
WHAT IS SCROLL ANCHORING (and why the pane fought it)
Browsers try to keep what you're reading still when content above or around
it changes size, by silently adjusting the scroller's scrollTop — "scroll
anchoring". The pane opts out with `overflow-anchor: none` because it manages
its own pin geometry. Chromium honors the opt-out; WebKit (the engine inside
Word's task pane) never implemented the property. When a response completes,
the strip's streamed rows unmount in the same React commit that flips its
label — and WebKit, which had latched onto one of those DOM nodes as its
anchor, resets scrollTop to 0. Instrumentation showed the reset arrives with
no JavaScript write and zero geometry change (the min-height spacer holds the
row's size), so no ResizeObserver watchdog can see it. Under a WebKit
Playwright run the pinned turn measured 80px -> 2228px at completion; under
Chromium the bug is unobservable, which is why the suite was green while
users saw the jump.
HOW THE FIX WORKS — explicit scroll ownership (ChatView.tsx)
Scroll position now always has an owner, and scroll events are audited
against that owner:
1. Every position the app writes (pin animation frames included, via
animateScrollTo's onFrame callback) is mirrored into desiredScrollTopRef
BEFORE the write, so the app's own scroll events match the record.
2. User input opens an ownership window before its scroll events land:
wheel/keyboard grant a short grace, pointer/touch hold ownership while
pressed, and touch release keeps it through momentum. During the window,
desiredScrollTopRef simply follows the user.
3. Any other scroll event matches neither owner — it can only be
engine-initiated — and is snapped back to the owned position.
This kills the completion reset, and also a second WebKit habit the tests
exposed: a scroller resting exactly at the bottom gets dragged along as
streamed content grows ("bottom-follow"), which made the view creep after
pressing the scroll-to-bottom arrow.
STABLE EVENT IDENTITIES (wordChatEvents.ts, AssistantMessage.tsx)
React keys for streamed rows were derived from event-array indices. At
completion, completeAssistantEvents() filters out transient rows (trailing
"thinking", stuck "reading"), shifting every later index — so surviving rows
remounted, destroying exactly the DOM nodes WebKit anchored to and flashing
the reasoning block open for one frame. Events are now stamped with a
creation-time `key` (a module counter) that survives streaming mutations, and
render keys prefer it: `event.key ?? index`. The field is inert in storage;
unit specs assert it with expect.any(String).
STREAMING PERFORMANCE — why it was quadratic
1. projectRedlineStream() re-parsed the FULL accumulated answer on every
chunk, and ran >=3x per chunk (edit controller + renderer + per-event
map): O(n^2) over a stream. A single-entry memo (same text + same flag
returns the cached projection) collapses those to one parse per change,
with zero call-site churn (redline.ts).
2. Every SSE event committed React state, re-rendering the whole transcript
far more often than the screen paints. Publishes now coalesce onto one
requestAnimationFrame, flushed synchronously at stream end/error so
terminal UI state never lags (useWordAssistantChat.ts).
3. Nothing was memoized: every settled message re-rendered — and
react-markdown re-parsed its full text — on every chunk. AssistantMessage,
UserMessage, and Markdown are now React.memo boundaries; Markdown's
plugins/components props are hoisted to module scope (an inline object
defeats react-markdown's own memoization); ChatView passes stable
useCallback handlers; and handleChat reads messages via a render-synced
ref instead of depending on them, so its identity stops churning per
chunk (which was re-rendering the composer).
4. Edit cards/sections painted a backdrop-blur behind a fully opaque
bg-white — invisible, but a compositing layer per card in the Office
WebView. Removed (messageStyles.ts).
VERIFICATION
- New WebKit regression spec (chat-layout.spec.ts) streams a doc-read plus a
multi-step reasoning strip and asserts the pinned turn holds through
completion: fails on the parent commit (80 -> 2228px, scrollTop 2148 -> 0),
passes here. The bottom-arrow spec now asserts the settled position, since
the corrector is deliberately eventually-consistent within a frame.
- Full Chromium suite: 107/107. WebKit chat-layout suite: 4/4. Typecheck clean.
- Deferred (tracked in docs/word-addin-chat-scroll-report.md): batching the
~9 serialized context.sync round trips Office needs per tracked edit — the
dominant remaining wall-clock on edit turns — and the header/glass blur
stack, which is a deliberate design choice asserted by e2e.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS The task pane's headline fix defends the transcript's scroll position against WKWebView, but the assertions that prove it only fail under WebKit. WebKit's scroll anchoring ignores `overflow-anchor: none` and rewrites scrollTop whenever a descendant (e.g. the collapsing "Working -> Completed in N steps" strip) resizes; Chromium honours the opt-out. A chromium-only suite therefore stays green even with the fix fully reverted -- the tests pass vacuously. WHAT IS A VACUOUS TEST A test that asserts a property the environment can never violate. It looks like coverage but can't fail, so regressions ship silently. The cure is to run the assertion in the environment where the property is actually at risk -- here, a WebKit browser, the same engine Word on macOS embeds. HOW IT WORKS - playwright.config.ts gains a `webkit` project (Desktop Safari), so `npm run test:e2e` runs every spec in both engines by default; a `test:e2e:webkit` script exists for targeted debugging. - The orphaned playwright.webkit.temp.config.ts (referenced by no script, doc, or CI, and invisible to every tsconfig) is deleted -- its only non-duplicated content was the webkit device entry. - A new path-filtered .github/workflows/word-addin.yml gates typecheck + the two-browser suite on every change under word-addin/. The webServer timeout rises 180s->300s because in CI that command performs a cold typecheck + production webpack build, and a webServer timeout aborts the run un-retried. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
…ge lookups
WHY THIS MATTERS
The chat routes reserve the assistant row with `content: null` BEFORE
the LLM stream runs, so the row id can be sent to the client early. If
the stream dies before its save path (process crash, deploy restart) --
or while a concurrent POST to the same chat is still streaming -- that
empty reservation is the newest assistant row in the table. Any query
for "the last assistant message" then finds a husk instead of the real
last turn.
WHAT BREAKS
Two context builders used order-by-created_at-desc + limit(1):
- enrichWithPriorEvents bails when content is not an array, so an
orphaned reservation silently hides the prior turn's doc_created /
doc_edited events -- the model loses its references to documents it
just generated.
- appendAssistantEventsToLastAssistantMessage could append
ask_inputs_response events onto the empty reservation, where the
eventual stream save would overwrite them.
HOW IT WORKS
Both queries now add `.not("content", "is", null)`, so they resolve to
the most recent COMPLETED assistant message; reservations are invisible
to reads while the reservation design itself stays unchanged. The only
other assistant-row query (buildDocContext) selects all rows and
already skips non-array content per row, so it needed no change.
Tests pin the behavior at two levels: unit tests drive a fake
chat_messages table through the real filter chain (prior turn's events
still surface past a newer null row; ask-inputs append targets the real
message), and a route-level test proves a POST /chat ask-inputs
continuation never updates the reservation row. Removing either filter
fails three tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
…racked-edits runtime Three fixes to the same runtime, all about respecting WKWebView's single main thread: work done per SSE chunk or per Word round-trip competes directly with paint, scroll handling, and typing. 1. PARSE STREAMED REDLINES ONCE PER FRAME, NOT PER CHUNK WHY: projectRedlineStream re-parses the accumulated answer from index zero, so invoking it synchronously on every SSE chunk makes a stream O(n^2) -- the longer the answer, the more each keystroke-sized chunk costs, exactly while the transcript is animating. HOW: the chunk handler now only flags redlineParsePending; the projection runs inside the same requestAnimationFrame callback that already coalesces the transcript publish, i.e. once per painted frame with the latest snapshot. Terminal paths stay exact: success flushes synchronously and still runs the streamComplete pass, and the catch/abort path flushes before markIncompleteRedlines, so a sealed edit in a trailing un-flushed chunk still applies on cancel. A sendIsCurrent() predicate (currency minus the abort bit) guards the deferred parse so a rAF firing after a session switch cannot schedule Word edits under the new generation -- deliberately not the stricter requestIsCurrent, because an aborted-but-current stream must keep the edits it already received. 2. STOP THE EDIT CONTROLLER FROM RECREATING handleChat MID-STREAM WHY: the hook returned a fresh object literal carrying editStateByKey, and handleChat listed that controller in its deps -- so every receiving->applying->pending transition recreated handleChat and re-rendered everything holding it, defeating the message-ref mirroring built precisely to keep it stable. HOW: the controller now exposes a useMemo'd streamController (processLiveRedlines / markIncompleteRedlines / waitForMessageEdits, all useCallback-stable) separate from editStateByKey. The chat hook receives only the stable streamController, so handleChat's identity survives the whole stream; components that render edit state still consume editStateByKey and re-render on real state changes. 3. RESTORE A CHAT'S TRACKED EDITS IN ONE Word.run BATCH WHY: opening a chat ran one serialized Word.run (~4 context.sync() host round-trips) per stored edit behind the global mutation queue -- pane readiness was linear in chat history, and every user action queued behind the backlog. Each sync is a WKWebView<->host hop. HOW: restoreTrackedEdits(descriptors) performs one Word.run for the whole set: all getBookmarkRangeOrNullObject lookups load before one sync, then items, verification, stale-bookmark deletes, and tracking -- a constant ~4 syncs total. Missing bookmarks are null objects, never batch failures; per-edit classification (not-found / resolved / view-only / restored) is preserved verbatim; if Word fails the shared batch outright, each edit retries sequentially via restoreTrackedEditNow so one bad object cannot sink the rest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
…ent flashes WHY THIS MATTERS loadMore talked to the fetch effect solely through setOffset(chats.length). Offset pagination over a list ordered by updated_at shifts whenever a chat is bumped to the top, so a whole fetched page can be deduplicated away -- leaving chats.length equal to the current offset. The offset-keyed effect then never re-fires, and the requestPending guard plus the loadingMore spinner stay stuck forever: pagination is dead until the pane reloads. WHAT IS AN OFFSET-PAGINATION SHIFT Page N is defined as "rows N*size..N*size+size at query time". If a row moves ahead of the cursor between requests (updated_at reordering), page N+1 re-serves rows you already have; dedupe correctly drops them, but any signal derived from list length no longer moves. HOW IT WORKS A monotonic requestId now accompanies the offset: loadMore bumps both, and the fetch effect keys on the requestId, so every loadMore performs exactly one fetch even when the numeric offset is unchanged. Dedupe semantics and the history-changed subscription are untouched. Also fixed here: switching document or storage scope while offset != 0 used to early-return after setOffset(0) without clearing state, so the previous document's chats stayed on screen until the new fetch resolved. The list and hasMore now reset before that early return. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
WHY THIS MATTERS The add-in identifies a document by a UUID stored in Office.context.document.settings -- and Office embeds those settings in the .docx itself. "Save As" or a file copy therefore carries the UUID into the copy, which silently inherits the original's ENTIRE chat history (cloud and local storage are both keyed by this ID) and its tracked-edit anchor registry. Chats about one contract surface inside a different file; anchors point at revisions that don't exist there. WHAT IS THE FAILURE MODE Document settings are the right place for identity (they survive renames and moves), but they conflate "same document" with "same file lineage". A second identity signal is needed to tell a moved original apart from a spawned copy. HOW IT WORKS A companion setting stores the normalized document URL next to the UUID. On load: - both stored and current URL known and different -> this is a copy: mint a fresh UUID (createSecureUuid), persist it with the new URL, and clear the stale anchor registry -- all batched into one saveAsync; - stored URL missing (first open after upgrade) -> keep identity, adopt the current URL; - current URL empty or unavailable (unsaved doc) -> keep identity, store nothing. Normalization is trim + trailing-slash strip + lowercase, deliberately without SharePoint URL canonicalization: an over-eager "copy" verdict would orphan real chat history, so ambiguity always resolves to keeping the existing identity. The e2e Office mock gains a seedable document.url; new specs cover the Save As path (fresh document_id, updated URL setting, anchor registry cleared) and both keep-identity paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
…ecting stale views WHY THIS MATTERS The prompt editor auto-saves on an 800ms debounce. Three lifecycle bugs hid in that convenience: 1. Navigating away inside the debounce window cleared the timer without firing it -- the user's typed instructions silently vanished. 2. If an updateWorkflow call was in flight when the user deselected, its .then called onSelectedWorkflowChange, which the App wires straight into page navigation -- re-opening the detail view for a workflow the user had just left. 3. The "Saved -> idle" status timer was never tracked, so it could stamp "idle" over a newer "Saving..." and fire after unmount. WHAT IS A DEBOUNCE FLUSH A debounce trades latency for batching, which is only safe while the component lives. At any teardown boundary (unmount, deselect, switch) the pending work must either flush or be knowingly discarded; silently dropping it turns an optimization into data loss. HOW IT WORKS - pendingSaveRef holds the latest unsaved edit; flushPendingSave() fires it (fire-and-forget) from the effect cleanup, which runs on deselect, workflow switch, and unmount. - selectedIdRef is nulled in cleanup and re-set synchronously by the next effect run; the save's .then/.catch check it, so a late resolution for a departed workflow can no longer navigate or write status. - The status-reset timer lives in statusResetTimerRef and is cleared on every new edit and in cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
WHY THIS MATTERS
Pressing Escape with a select dropdown open inside the workflow modals
closed BOTH the dropdown and the modal, discarding everything typed
into the form. Escape should peel one layer at a time: first the
dropdown, then (on a second press) the modal.
WHAT IS THE EVENT-ORDER MECHANISM
Radix's dismissable layer registers a capture-phase keydown listener on
document; the Modal listens bubble-phase on window. Capture on document
runs before the event bubbles back out to window, so stopping
propagation inside the Radix handler kills the event before the Modal
ever sees it -- no timing hacks, just DOM event phases.
HOW IT WORKS
ModalSelect passes onEscapeKeyDown={(e) => e.stopPropagation()} to its
DropdownContent. The dropdown still closes (no preventDefault), and a
second Escape -- with the Radix layer unmounted -- bubbles to window
and closes the modal as before. The behavior is opt-in at the
ModalSelect call site rather than baked into the shared Dropdown
primitive, because that primitive also serves non-modal surfaces
(header menu, history, document source, model toggle) where swallowing
Escape could collide with the prompt editor's document-level handler.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
…masked layer WHY THIS MATTERS The floating header's progressive-blur scrim stacked four full-width backdrop-blur layers (1/2/4/8px). backdrop-filter cannot be cached: the compositor must re-sample whatever is behind the layer every time it changes -- and behind this scrim is the transcript, which moves on every scroll frame and repaints on every streamed token. Four stacked layers meant four full-width re-samples per frame over the hottest region of the pane, in WKWebView where main-thread headroom is already the constraint (the same cost class messageStyles.ts documents for text blurs). WHAT IS A MASKED PROGRESSIVE BLUR The standard single-layer approximation: one blur at the maximum strength whose mask-image alpha ramps from opaque to transparent, so the blurred copy cross-fades into the sharp content underneath. The eye reads the fade-out of an 8px blur as the blur itself easing off -- visually equivalent to stacked increasing blurs at a quarter of the sampling cost. HOW IT WORKS One backdrop-blur-[8px] layer with a multi-stop mask ramp (black 0-16%, 0.55 @46%, 0.2 @72%, transparent 100%) replaces the four layers; both mask-image and -webkit-mask-image are set (WKWebView needs the prefix), and the gradient overlay above it is unchanged. The layout spec now asserts exactly one masked blur layer instead of the old stack, and a leftover WEBKIT_COMPLETION_DIAGNOSTIC debug console.log in the same spec was removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW
d6b1315 to
28acdbd
Compare
3 participants
Summary
Testing
npm run typechecknpm run build:e2enpm run test:e2e— 59 passed, 7 skipped (Projects is intentionally not exposed)Build note
The deployment production build requires
REACT_APP_API_BASE_URL,REACT_APP_SUPABASE_URL,REACT_APP_SUPABASE_ANON_KEY, andREACT_APP_WEB_APP_URL; the environment-free verification uses the repository’sbuild:e2econfiguration.