fix(word-addin): review fixes for #299 — WebKit e2e gate, streaming perf, and correctness edges - #320
Merged
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Review follow-up for #299, targeting its branch so the fixes land inside that PR. Eight commits, each with a self-contained educational message:
overflow-anchor: none, WebKit ignores it), so the orphanedplaywright.webkit.temp.config.tsis folded intoplaywright.config.tsas a second project,test:e2eruns both engines, and a new path-filteredword-addin.ymlworkflow gates typecheck + the two-browser suite in CI.enrichWithPriorEvents, ask-inputs append) now skip null-content streaming reservations, so an orphaned reservation (crash/restart/concurrent POST) can no longer shadow the prior turn'sdoc_created/doc_editedevents. Tests are mutation-checked.streamControllersohandleChatsurvives a whole stream; chat-open restore batches all bookmark lookups into oneWord.run(~4 syncs total instead of ~4 per edit) with per-edit failure isolation.Also flagged separately on #299: the hand-forked shared UI components are accepted for now, with a shared-package follow-up planned (see the review comment).
Testing
npm run typecheck(word-addin, app + e2e) — cleannpx tsc --noEmit(backend) — cleannpx vitest runon the touched suites — 40/40 passed; removing the reservation filters fails 3 tests (fix is pinned)npx playwright test— 228 passed (112 chromium + 112 webkit) in 2.5m, including the scroll assertions running under WebKit for the first time and new copy-identity specs🤖 Generated with Claude Code
https://claude.ai/code/session_015U1iPxsADQ2yuksNmDuvvW