MM-69893: Mount the host WYSIWYG editor at the page route with draft autosave - #13
Draft
nang2049 wants to merge 3 commits into
Draft
MM-69893: Mount the host WYSIWYG editor at the page route with draft autosave#13nang2049 wants to merge 3 commits into
nang2049 wants to merge 3 commits into
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/page_editor/page_editor.module.scss (1)
1-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the blank line inside
.root.Stylelint reports
declaration-empty-line-beforeat Line 7. The blank Line 6 betweenmin-width: 0;andmin-height: 0;triggers this.🧹 Proposed fix
.root { display: flex; flex: 1 1 0; flex-direction: column; min-width: 0; - min-height: 0; height: 100%; background: var(--center-channel-bg); }🤖 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 `@webapp/src/components/page_editor/page_editor.module.scss` around lines 1 - 10, Remove the empty line between the min-width and min-height declarations in the .root style rule, leaving the declaration order and values unchanged.Source: Linters/SAST tools
🧹 Nitpick comments (8)
webapp/src/client/presence_events.ts (1)
21-23: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueIsolate listener errors during publish.
publishPagePresencecalls each listener directly. If one listener throws, the remaining listeners never receive the event, and the exception propagates into the WebSocket handler registered inwebapp/src/index.tsx. Wrap each call so one failing subscriber cannot block delivery.♻️ Proposed isolation of listener errors
export function publishPagePresence(event: PagePresenceEvent): void { - listeners.forEach((listener) => listener(event)); + listeners.forEach((listener) => { + try { + listener(event); + } catch { + // A single subscriber must not block delivery to the others. + } + }); }🤖 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 `@webapp/src/client/presence_events.ts` around lines 21 - 23, Update publishPagePresence so each listener invocation is isolated with per-listener error handling, ensuring an exception from one subscriber does not stop iteration or propagate into the WebSocket handler; preserve delivery of the event to all remaining listeners.webapp/src/hooks/draft_autosave.ts (1)
112-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe debounce has no maximum wait, so continuous typing never saves.
Every
queuecall clears the timer and starts a new one. While a user types without a pause ofAUTOSAVE_DEBOUNCE_MS, no write occurs. A long uninterrupted editing session therefore holds all content in memory. A tab crash or a forced reload loses that work.Consider tracking the time of the first pending edit and forcing a write once a maximum interval elapses.
🤖 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 `@webapp/src/hooks/draft_autosave.ts` around lines 112 - 124, The queue function’s debounce can be postponed indefinitely during continuous edits. Add tracking for when the current pending batch first starts, and in queue enforce a maximum wait interval that triggers write even when the debounce timer keeps resetting; reset that tracking when the pending changes are written or cleared, while preserving the existing debounce behavior for shorter pauses.webapp/src/hooks/draft_autosave.test.tsx (1)
266-280: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd baseline coverage to the page-change test.
This test omits
baseEditAt, so it asserts only that the patch reachespage1. It cannot detect whichbase_edit_ataccompanies that patch. Two gaps remain untested, and both correspond to issues raised onwebapp/src/hooks/draft_autosave.ts:
- Rerender with a different
baseEditAttogether with the newpageId, then assert the flushed patch carries the previous page'sbase_edit_at.- Add a case with
baseEditAt: 0and assertbase_edit_at: 0is still sent.🤖 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 `@webapp/src/hooks/draft_autosave.test.tsx` around lines 266 - 280, Expand the page-change coverage around setup and the “flushes the pending patch…” test to provide an initial baseEditAt, rerender with both a new pageId and different baseEditAt, and assert the flushed page1 patch includes the previous page’s base_edit_at. Add a separate case using baseEditAt: 0 and verify the emitted patch preserves base_edit_at: 0.webapp/src/components/page_editor/toolbar_controls.tsx (2)
138-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
CALLOUT_LABELSaboveCalloutControland store plain message descriptors.
CalloutControlreadsCALLOUT_LABELSat line 128, but the constant is declared here at line 140. The reference resolves at render time, so it works today. Declaring the constant before its use removes the forward reference. StoringMessageDescriptorvalues instead of functions also removes theFormatterindirection.♻️ Proposed refactor
-type Formatter = ReturnType<typeof useIntl>['formatMessage']; - -const CALLOUT_LABELS: Record<CalloutType, (f: Formatter) => string> = { - info: (f) => f({id: 'docs.editor.calloutInfo', defaultMessage: 'Info'}), - note: (f) => f({id: 'docs.editor.calloutNote', defaultMessage: 'Note'}), - success: (f) => f({id: 'docs.editor.calloutSuccess', defaultMessage: 'Success'}), - warning: (f) => f({id: 'docs.editor.calloutWarning', defaultMessage: 'Warning'}), - error: (f) => f({id: 'docs.editor.calloutError', defaultMessage: 'Error'}), -};Add above
CalloutControl:const CALLOUT_LABELS: Record<CalloutType, MessageDescriptor> = { info: {id: 'docs.editor.calloutInfo', defaultMessage: 'Info'}, note: {id: 'docs.editor.calloutNote', defaultMessage: 'Note'}, success: {id: 'docs.editor.calloutSuccess', defaultMessage: 'Success'}, warning: {id: 'docs.editor.calloutWarning', defaultMessage: 'Warning'}, error: {id: 'docs.editor.calloutError', defaultMessage: 'Error'}, };Then render with
{formatMessage(CALLOUT_LABELS[type])}.🤖 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 `@webapp/src/components/page_editor/toolbar_controls.tsx` around lines 138 - 146, Move CALLOUT_LABELS above CalloutControl and change its values from formatter functions to MessageDescriptor objects. Remove the Formatter indirection, and update CalloutControl to pass CALLOUT_LABELS[type] directly to formatMessage when rendering the label.
112-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd arrow-key navigation to the callout menu.
The container declares
role='menu'and the items declarerole='menuitem'. A screen reader then announces a menu, and the user expects arrow keys to move between items. The current code provides no arrow-key handling and no rovingtabindex. Tab still reaches each button, so the task stays completable, but the announced interaction model does not match the behaviour.Either implement arrow-key navigation with a roving
tabindex, or drop therole='menu'androle='menuitem'attributes and let the buttons present as a plain group.🤖 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 `@webapp/src/components/page_editor/toolbar_controls.tsx` around lines 112 - 133, Align the callout menu’s accessibility semantics with its behavior in the toolbar component: either implement arrow-key navigation and roving tabindex for the `menu` and `menuitem` elements around `CALLOUT_TYPES.map`, or remove those role attributes so the controls remain a plain button group. Preserve the existing `insert(type)` activation behavior.webapp/src/hooks/caret_anchored_suggestions.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
.suggestion-listselector couples this hook to a host-owned class name.The hook positions a DOM node that the host web application renders. If the host renames the class, then this code silently stops positioning the list and no error appears. Add a comment that records the host version this selector targets, so a future reader can trace the dependency.
🤖 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 `@webapp/src/hooks/caret_anchored_suggestions.ts` around lines 6 - 8, Add a concise comment next to the SELECTOR constant documenting the host version associated with the `.suggestion-list` class, preserving the existing selector and positioning behavior.webapp/src/components/page_editor/publish_conflict_dialog.tsx (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the conflict reason exactly instead of by substring.
Line 23 uses
reason.includes('concurrent_autosave'). Substring matching also matches an unrelated future reason such asnot_concurrent_autosaveor a reason that embeds the token in a longer message. Typereasonas a union of the server reason codes and compare with===. The only consequence today is the wrong explanatory paragraph, so this is a robustness improvement rather than a defect.#!/bin/bash # Find the reason values that the server and the draft client produce. rg -n 'concurrent_autosave|PublishConflictError|reason' --type=ts --type=tsx -g '!**/*.test.*' rg -n 'concurrent_autosave' --type=go🤖 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 `@webapp/src/components/page_editor/publish_conflict_dialog.tsx` around lines 17 - 23, Update isConcurrentAutosave to accept the server’s reason-code union rather than a generic string, and compare the value exactly with === 'concurrent_autosave'. Reuse the existing reason-code type or define the union from the server/draft-client reason values, preserving the current explanatory-paragraph behavior for the exact concurrent_autosave code.webapp/src/components/page_editor/page_editor.module.scss (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the repeated
.columnselector.
.columnis declared three times (Lines 28-32, 62-73, 88-153). Merging these into a single block groups the editor-surface and callout styling with the layout rules, and makes future edits less likely to miss one of the declarations.Also applies to: 62-73, 88-153
🤖 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 `@webapp/src/components/page_editor/page_editor.module.scss` around lines 28 - 32, Consolidate the three `.column` declarations into one selector block, combining the layout, editor-surface, and callout rules while preserving all existing properties and responsive behavior.
🤖 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 `@webapp/src/components/docs_root/docs_main_content.tsx`:
- Around line 24-44: Update DocsMainContent and the PageEditor loading flow to
explicitly validate that the requested space exists before rendering an editor
for spaceId/pageId. Preserve the existing 404 handling that allows valid new
drafts, but prevent invalid spaceId requests from rendering an empty editor when
both requests return 404; reuse the existing space state and error-handling
symbols.
In `@webapp/src/components/page_editor/apply_formatting.ts`:
- Around line 21-43: The selectWordUnderCaret function uses textContent, which
omits inline leaf nodes while parentOffset counts them. Replace the parent text
extraction with parent.textBetween(0, parent.content.size, undefined, '\ufffc')
so text indexing and caret offsets share one coordinate space; preserve the
existing word-boundary and selection logic.
In `@webapp/src/components/page_editor/callout_extension.ts`:
- Around line 48-53: Update toggleCallout in addCommands so an active callout
with a different type uses commands.updateAttributes(this.name, {type}) instead
of nesting via wrapIn; retain commands.toggleWrap(this.name, {type}) for all
other cases.
In `@webapp/src/components/page_editor/floating_formatting_bar.module.scss`:
- Around line 6-9: Remove the empty line between the z-index declaration and the
width declaration in the floating formatting bar styles so the declarations are
contiguous and satisfy Stylelint.
In `@webapp/src/components/page_editor/page_byline.tsx`:
- Around line 22-27: Update the component’s missing-author branch around getUser
and the author null check to dispatch getMissingProfilesByIds([userId]) when
author is unavailable before returning null. Add useEffect to the React imports
and use it to trigger this profile-loading dispatch when userId or author state
requires it, while preserving the existing rendering behavior once the profile
is available.
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 154-156: Update the early-return paths in the publish and
save-draft flows around autosave.flush() to call setActionError before returning
when the flush resolves false, using the existing error message pattern. Ensure
saveDraftAndLeave also catches rejected flush promises and sets actionError so
the exit dialog reports the failure instead of leaving an unhandled rejection.
- Around line 88-92: Update the page-change reset effect keyed by spaceId and
pageId to also clear conflict and showExitDialog, ensuring any publish-conflict
or exit dialog closes before actions can target the newly selected page.
- Around line 147-209: Replace the callback-captured busy guard in publish,
discard, and saveDraftAndLeave with a shared synchronous in-flight ref that is
checked and set before any await, then cleared when each action finishes.
Continue updating the existing busy state for rendering, and ensure all early
returns and finally paths release the ref.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 86-90: Guard the toolbar_controls.tsx insert callback so it
verifies the editor chain provides toggleCallout before invoking it, while
preserving focus, execution, and menu-closing behavior when supported. In
webapp/src/webapp_globals.ts lines 82-87, document the host build that
introduced contentType, extensions, and onContentError, or add a JSON-content
capability probe that page_editor.tsx can use for the legacy notice instead of
relying solely on hostSupportsDocumentEditor/getEditor.
In `@webapp/src/hooks/caret_anchored_suggestions.ts`:
- Around line 85-95: Update the effect around the MutationObserver in the
caret-anchored suggestions hook to schedule repositioning when the editor’s
data-docs-scroll container scrolls and when the window resizes. Register both
listeners while enabled, reuse the existing schedule callback, and remove them
in the cleanup alongside the observer and selectionchange listener.
- Around line 30-38: Update the caret-anchored positioning logic to clamp the
computed left offset before assigning `list.style.left`, using the surface width
and suggestion-list width so the list remains within the surface. Follow the
existing `maxLeft` clamping approach in `floating_formatting_bar.tsx` while
preserving the current caret-relative positioning when it fits.
In `@webapp/src/hooks/draft_autosave.ts`:
- Around line 140-143: Update the autosave flow around the Pending type, write,
and error-requeue logic to store baseEditAt alongside each queued patch, using
the value captured for that page rather than reading latest.current during
cleanup. Preserve the stored baseEditAt when requeueing failed patches, and add
coverage that switches between pages with different baselines and verifies each
patch uses its own baseline.
In `@webapp/src/hooks/page_draft.ts`:
- Around line 70-71: Update the draft field initialization in the page draft
hook to use nullish coalescing for both title and body, preserving empty-string
values while still falling back to page values only when the draft fields are
nullish.
In `@webapp/src/hooks/page_presence.ts`:
- Around line 54-57: Update the expired-snapshot branch in the page presence
effect to refresh the `now` state before exiting, rather than returning with the
mount-time value. Keep the existing timer scheduling for unexpired snapshots
unchanged so the memoized active-editor calculation receives the current
timestamp and clears stale editors.
---
Outside diff comments:
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 1-10: Remove the empty line between the min-width and min-height
declarations in the .root style rule, leaving the declaration order and values
unchanged.
---
Nitpick comments:
In `@webapp/src/client/presence_events.ts`:
- Around line 21-23: Update publishPagePresence so each listener invocation is
isolated with per-listener error handling, ensuring an exception from one
subscriber does not stop iteration or propagate into the WebSocket handler;
preserve delivery of the event to all remaining listeners.
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 28-32: Consolidate the three `.column` declarations into one
selector block, combining the layout, editor-surface, and callout rules while
preserving all existing properties and responsive behavior.
In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx`:
- Around line 17-23: Update isConcurrentAutosave to accept the server’s
reason-code union rather than a generic string, and compare the value exactly
with === 'concurrent_autosave'. Reuse the existing reason-code type or define
the union from the server/draft-client reason values, preserving the current
explanatory-paragraph behavior for the exact concurrent_autosave code.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 138-146: Move CALLOUT_LABELS above CalloutControl and change its
values from formatter functions to MessageDescriptor objects. Remove the
Formatter indirection, and update CalloutControl to pass CALLOUT_LABELS[type]
directly to formatMessage when rendering the label.
- Around line 112-133: Align the callout menu’s accessibility semantics with its
behavior in the toolbar component: either implement arrow-key navigation and
roving tabindex for the `menu` and `menuitem` elements around
`CALLOUT_TYPES.map`, or remove those role attributes so the controls remain a
plain button group. Preserve the existing `insert(type)` activation behavior.
In `@webapp/src/hooks/caret_anchored_suggestions.ts`:
- Around line 6-8: Add a concise comment next to the SELECTOR constant
documenting the host version associated with the `.suggestion-list` class,
preserving the existing selector and positioning behavior.
In `@webapp/src/hooks/draft_autosave.test.tsx`:
- Around line 266-280: Expand the page-change coverage around setup and the
“flushes the pending patch…” test to provide an initial baseEditAt, rerender
with both a new pageId and different baseEditAt, and assert the flushed page1
patch includes the previous page’s base_edit_at. Add a separate case using
baseEditAt: 0 and verify the emitted patch preserves base_edit_at: 0.
In `@webapp/src/hooks/draft_autosave.ts`:
- Around line 112-124: The queue function’s debounce can be postponed
indefinitely during continuous edits. Add tracking for when the current pending
batch first starts, and in queue enforce a maximum wait interval that triggers
write even when the debounce timer keeps resetting; reset that tracking when the
pending changes are written or cleared, while preserving the existing debounce
behavior for shorter pauses.
🪄 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: b73604e5-f7a9-4c16-9257-acc82cd46454
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
webapp/i18n/en.jsonwebapp/package.jsonwebapp/src/client/drafts.tswebapp/src/client/pages.tswebapp/src/client/presence_events.tswebapp/src/client/rest.tswebapp/src/components/docs_root/docs_main_content.tsxwebapp/src/components/page_editor/apply_formatting.tswebapp/src/components/page_editor/autosave_indicator.module.scsswebapp/src/components/page_editor/autosave_indicator.tsxwebapp/src/components/page_editor/callout_extension.tswebapp/src/components/page_editor/docs_extensions.tswebapp/src/components/page_editor/exit_editor_dialog.module.scsswebapp/src/components/page_editor/exit_editor_dialog.tsxwebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_byline.module.scsswebapp/src/components/page_editor/page_byline.tsxwebapp/src/components/page_editor/page_editor.module.scsswebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/publish_conflict_dialog.module.scsswebapp/src/components/page_editor/publish_conflict_dialog.tsxwebapp/src/components/page_editor/toolbar_controls.module.scsswebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/data/fixtures.tswebapp/src/hooks/caret_anchored_suggestions.tswebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/page_draft.tswebapp/src/hooks/page_presence.tswebapp/src/hooks/pinned_toolbar.tswebapp/src/hooks/user.tswebapp/src/index.tsxwebapp/src/store/test_fixtures.tswebapp/src/types/docs.tswebapp/src/types/drafts.tswebapp/src/webapp_globals.ts
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR replaces the Docs page placeholder with a functional Tiptap editor. It adds draft APIs, autosave, publishing, conflict handling, presence updates, formatting controls, editor dialogs, page metadata, and localized strings. ChangesDocs editor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
webapp/src/hooks/page_draft.ts (1)
73-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve empty draft fields with nullish coalescing.
Draft.titleandDraft.bodyare required strings, and autosave sends empty strings. With||, a cleared title or a cleared body falls back to the published page content on the next load. The user then sees content that the draft no longer contains.Use
??so the fallback applies only when the draft field isnullorundefined.🐛 Proposed fix to preserve cleared draft fields
- title: draft?.title || page?.title || '', - body: draft?.body || page?.body || '', + title: draft?.title ?? page?.title ?? '', + body: draft?.body ?? page?.body ?? '',🤖 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 `@webapp/src/hooks/page_draft.ts` around lines 73 - 74, Update the draft initialization around title and body to use nullish coalescing instead of falsy coalescing, so empty-string Draft.title and Draft.body values are preserved while fallback to page values occurs only for null or undefined.webapp/src/components/page_editor/page_editor.tsx (1)
159-162: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA failed
autosave.flush()still produces no user-visible message.Line 160 and Line 211 return early when
flush()resolvesfalse. Neither path callssetActionError. Thefinallyblock clearsbusy, so the button becomes active and nothing else changes. The user sees no result and no reason.
saveDraftAndLeaveat Line 203 also has nocatch. Ifflush()rejects, then the rejection is unhandled and the exit dialog shows no error, becausefailed={actionError != null}staysfalseat Line 418.🐛 Proposed fix
try { if (!await autosave.flush()) { + setActionError(new Error('autosave_flush_failed')); return; }Apply the same change at Line 211, and add a
catchblock tosaveDraftAndLeavethat callssetActionError(error).🤖 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 `@webapp/src/components/page_editor/page_editor.tsx` around lines 159 - 162, Update both autosave.flush() call paths to surface failures through setActionError: when flush() resolves false, set the action error before returning, and add a catch block to saveDraftAndLeave that passes the rejected error to setActionError. Preserve the existing finally cleanup and successful save/navigation behavior.
🧹 Nitpick comments (9)
webapp/src/hooks/draft_autosave.test.tsx (1)
172-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an assertion for recovery after a failed save.
The test verifies the retry patch content, but it does not verify that the status returns to
savedafter the retry succeeds. That transition drives the autosave indicator inpage_editor.tsx.Add the assertion at the end of the test.
♻️ Proposed additional assertion
await act(async () => { await result.current.flush(); }); expect(patchesSent()[1]).toEqual({body: 'lost'}); + expect(result.current.status).toBe('saved');🤖 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 `@webapp/src/hooks/draft_autosave.test.tsx` around lines 172 - 189, Add an assertion at the end of the “keeps the patch for retry when a save fails” test verifying that result.current.status transitions to “saved” after flush() successfully retries the preserved patch.webapp/src/hooks/pinned_toolbar.ts (2)
6-6: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the storage key to the user.
STORAGE_KEYis a single global key. If two accounts use the same browser profile, they share one toolbar preference. The PR description lists the toolbar preference storage as an open question, so this choice is worth confirming now.Two options exist:
- Append the current user id to the key.
useCurrentUserIdinwebapp/src/hooks/user.tsalready provides the id.- Store the preference as a Mattermost user preference, so it follows the user across devices.
🤖 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 `@webapp/src/hooks/pinned_toolbar.ts` at line 6, Update the pinned-toolbar persistence around STORAGE_KEY to scope the stored preference to the authenticated user, using the existing useCurrentUserId symbol from the user hook when constructing the key. Ensure different users receive separate storage entries and preserve the existing toolbar preference behavior.
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
writeStoredreturns a value that no caller uses.
writeStoredreturnstrueorfalse, but line 36 discards the result. A failed write is therefore silent, and the toolbar state and the stored state diverge without any signal.Either drop the return type, or use the result to surface the 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 `@webapp/src/hooks/pinned_toolbar.ts` around lines 30 - 37, Update the pinned toolbar effect around writeStored so its boolean result is no longer silently discarded: either remove the unused return value from writeStored or handle false by surfacing the storage failure, while preserving the first-render skip behavior.webapp/src/client/presence_events.ts (1)
21-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate listener failures during dispatch.
If one listener throws,
publishPagePresencestops and the exception propagates to the caller. The caller is the WebSocket handler registered inwebapp/src/index.tsx, so a single failing subscriber can affect host event dispatch and can block the remaining subscribers.Wrap each invocation so that one failure does not stop the others.
♻️ Proposed fix to isolate listener errors
export function publishPagePresence(event: PagePresenceEvent): void { - listeners.forEach((listener) => listener(event)); + listeners.forEach((listener) => { + try { + listener(event); + } catch { + // A failing subscriber must not block the remaining subscribers. + } + }); }🤖 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 `@webapp/src/client/presence_events.ts` around lines 21 - 23, Update publishPagePresence to invoke each listener within its own failure boundary, ensuring an exception from one listener is contained and does not propagate to the WebSocket caller or prevent remaining listeners from receiving the event.webapp/src/components/page_editor/exit_editor_dialog.module.scss (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the copyright header.
floating_formatting_bar.module.scssandtoolbar_controls.module.scssin the same directory both start with the two-line Mattermost copyright comment. This file does not.♻️ Proposed addition
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + .actions { display: flex;🤖 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 `@webapp/src/components/page_editor/exit_editor_dialog.module.scss` at line 1, Add the standard two-line Mattermost copyright comment at the beginning of the stylesheet containing the .actions rule, matching the header used by floating_formatting_bar.module.scss and toolbar_controls.module.scss.webapp/src/components/page_editor/toolbar_controls.module.scss (1)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
.swatchclass.
toolbar_controls.tsxusescontrol,active,menuWrapper,menu, andmenuItem. It does not useswatch. The menu items render Compass icons, not colour swatches.♻️ Proposed removal
- -.swatch { - width: 12px; - height: 12px; - border-radius: 2px; - flex-shrink: 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 `@webapp/src/components/page_editor/toolbar_controls.module.scss` around lines 66 - 71, Remove the unused .swatch style rule from the toolbar controls stylesheet; retain the styles for the classes used by toolbar_controls.tsx, including control, active, menuWrapper, menu, and menuItem.webapp/src/components/page_editor/floating_formatting_bar.tsx (1)
85-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe scroll listener attaches only if
editorRef.currentis set on the first effect run.Line 86 resolves the scroll container once. The effect depends on
[schedule, editorRef].editorRefis a stable ref object, so the effect does not re-run wheneditorRef.currentchanges later. If the referenced element mounts after this effect runs,scrollerstaysnulland the bar never repositions on scroll.Bind the scroll listener at the document level with capture, which does not depend on resolving the container.
♻️ Proposed change
useEffect(() => { - const scroller = editorRef.current?.closest('[data-docs-scroll]'); - document.addEventListener('selectionchange', schedule); window.addEventListener('resize', schedule); - scroller?.addEventListener('scroll', schedule); + document.addEventListener('scroll', schedule, true); return () => { document.removeEventListener('selectionchange', schedule); window.removeEventListener('resize', schedule); - scroller?.removeEventListener('scroll', schedule); + document.removeEventListener('scroll', schedule, true); if (frameRef.current) { cancelAnimationFrame(frameRef.current); frameRef.current = 0; } }; }, [schedule, editorRef]);🤖 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 `@webapp/src/components/page_editor/floating_formatting_bar.tsx` around lines 85 - 100, Update the scroll handling in the useEffect so it no longer resolves a container from editorRef.current or attaches to scroller; register the scroll listener on document with capture enabled and remove it using the same capture configuration during cleanup, while preserving the existing selection, resize, and animation-frame cleanup behavior.webapp/src/components/page_editor/page_editor.module.scss (1)
62-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicate
.columnselectors.
.columnis declared at Line 28, Line 62, and Line 88. Three separate blocks for the same class make the cascade harder to follow. Merge the two:globalblocks into one block under a single.columnrule.🤖 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 `@webapp/src/components/page_editor/page_editor.module.scss` around lines 62 - 73, Merge the duplicate .column selectors in the stylesheet into one rule, combining both :global blocks under it while preserving all existing declarations and cascade behavior. Use the existing .column rule as the single location for these styles.webapp/src/components/page_editor/publish_conflict_dialog.tsx (1)
23-27: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCompare the conflict reason exactly instead of using a substring match.
reasonis a discrete server reason code.includesmatches any future code that embeds theconcurrent_autosavetoken and selects the wrong message. Use an exact comparison against a shared constant.♻️ Proposed change
-const isConcurrentAutosave = (reason: string): boolean => reason.includes('concurrent_autosave'); +const CONCURRENT_AUTOSAVE = 'concurrent_autosave'; + +const isConcurrentAutosave = (reason: string): boolean => reason === CONCURRENT_AUTOSAVE;Export the constant from
client/draftsif the reason codes are already defined there.🤖 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 `@webapp/src/components/page_editor/publish_conflict_dialog.tsx` around lines 23 - 27, Update isConcurrentAutosave to compare reason by exact equality with the shared concurrent-autosave reason constant, reusing and exporting that constant from client/drafts if the reason codes are defined there; remove the substring-based includes check while preserving the existing autosave conflict selection.
🤖 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 `@webapp/i18n/en.json`:
- Around line 27-31: Update the user-facing strings in the English translations
so docs.editor.autosave.saving, docs.editor.bodyPlaceholder, and the
corresponding string at line 49 use the same ellipsis convention, and change
docs.editor.callout to use “Callout” as the noun while preserving the existing
meaning.
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 6-7: Remove the blank line immediately before the min-height
declaration in the SCSS file. Update the Stylelint configuration used for CSS
Modules files to ignore the global pseudo-class, reusing or extending the
existing .module.scss-specific rules after confirming whether other module
styles are already covered.
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 394-406: Update the WysiwygEditor usage in the page editor to set
useCtrlSend={true}, ensuring onPublish is triggered only through the intended
modified-key shortcut rather than an unmodified Enter in a paragraph.
In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx`:
- Around line 39-54: The force-publish action in the conflict dialog lacks
loading and failure feedback. Update the dialog component’s props and the
`PrimaryButton` using `onForcePublish` to accept and mirror the `busy` and
`failed` handling used by `ExitEditorDialog`, so failed forced publishes are
surfaced within the open dialog.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 114-135: Update the menu item selection handler in the
CALLOUT_TYPES map to restore focus to triggerRef after insert(type) closes the
menu, unless the successful editor command has already moved focus to the editor
surface. Match the existing Escape-path focus behavior while preserving the
insert command flow.
---
Duplicate comments:
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 159-162: Update both autosave.flush() call paths to surface
failures through setActionError: when flush() resolves false, set the action
error before returning, and add a catch block to saveDraftAndLeave that passes
the rejected error to setActionError. Preserve the existing finally cleanup and
successful save/navigation behavior.
In `@webapp/src/hooks/page_draft.ts`:
- Around line 73-74: Update the draft initialization around title and body to
use nullish coalescing instead of falsy coalescing, so empty-string Draft.title
and Draft.body values are preserved while fallback to page values occurs only
for null or undefined.
---
Nitpick comments:
In `@webapp/src/client/presence_events.ts`:
- Around line 21-23: Update publishPagePresence to invoke each listener within
its own failure boundary, ensuring an exception from one listener is contained
and does not propagate to the WebSocket caller or prevent remaining listeners
from receiving the event.
In `@webapp/src/components/page_editor/exit_editor_dialog.module.scss`:
- Line 1: Add the standard two-line Mattermost copyright comment at the
beginning of the stylesheet containing the .actions rule, matching the header
used by floating_formatting_bar.module.scss and toolbar_controls.module.scss.
In `@webapp/src/components/page_editor/floating_formatting_bar.tsx`:
- Around line 85-100: Update the scroll handling in the useEffect so it no
longer resolves a container from editorRef.current or attaches to scroller;
register the scroll listener on document with capture enabled and remove it
using the same capture configuration during cleanup, while preserving the
existing selection, resize, and animation-frame cleanup behavior.
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 62-73: Merge the duplicate .column selectors in the stylesheet
into one rule, combining both :global blocks under it while preserving all
existing declarations and cascade behavior. Use the existing .column rule as the
single location for these styles.
In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx`:
- Around line 23-27: Update isConcurrentAutosave to compare reason by exact
equality with the shared concurrent-autosave reason constant, reusing and
exporting that constant from client/drafts if the reason codes are defined
there; remove the substring-based includes check while preserving the existing
autosave conflict selection.
In `@webapp/src/components/page_editor/toolbar_controls.module.scss`:
- Around line 66-71: Remove the unused .swatch style rule from the toolbar
controls stylesheet; retain the styles for the classes used by
toolbar_controls.tsx, including control, active, menuWrapper, menu, and
menuItem.
In `@webapp/src/hooks/draft_autosave.test.tsx`:
- Around line 172-189: Add an assertion at the end of the “keeps the patch for
retry when a save fails” test verifying that result.current.status transitions
to “saved” after flush() successfully retries the preserved patch.
In `@webapp/src/hooks/pinned_toolbar.ts`:
- Line 6: Update the pinned-toolbar persistence around STORAGE_KEY to scope the
stored preference to the authenticated user, using the existing useCurrentUserId
symbol from the user hook when constructing the key. Ensure different users
receive separate storage entries and preserve the existing toolbar preference
behavior.
- Around line 30-37: Update the pinned toolbar effect around writeStored so its
boolean result is no longer silently discarded: either remove the unused return
value from writeStored or handle false by surfacing the storage failure, while
preserving the first-render skip behavior.
🪄 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: d019f109-4109-4213-a227-e285f66c7f60
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
webapp/i18n/en.jsonwebapp/package.jsonwebapp/src/client/drafts.tswebapp/src/client/pages.tswebapp/src/client/presence_events.tswebapp/src/client/rest.tswebapp/src/components/docs_root/docs_main_content.tsxwebapp/src/components/page_editor/apply_formatting.tswebapp/src/components/page_editor/autosave_indicator.module.scsswebapp/src/components/page_editor/autosave_indicator.tsxwebapp/src/components/page_editor/callout_extension.tswebapp/src/components/page_editor/docs_extensions.tswebapp/src/components/page_editor/exit_editor_dialog.module.scsswebapp/src/components/page_editor/exit_editor_dialog.tsxwebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_byline.module.scsswebapp/src/components/page_editor/page_byline.tsxwebapp/src/components/page_editor/page_editor.module.scsswebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/publish_conflict_dialog.module.scsswebapp/src/components/page_editor/publish_conflict_dialog.tsxwebapp/src/components/page_editor/toolbar_controls.module.scsswebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/data/fixtures.tswebapp/src/hooks/caret_anchored_suggestions.tswebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/page_draft.tswebapp/src/hooks/page_presence.tswebapp/src/hooks/pinned_toolbar.tswebapp/src/hooks/user.tswebapp/src/index.tsxwebapp/src/store/test_fixtures.tswebapp/src/types/docs.tswebapp/src/types/drafts.tswebapp/src/webapp_globals.ts
Collaborator
|
@asaadmahmood - for (some of) the "Open Questions" in #13 (comment) |
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
requested changes
Aug 3, 2026
calebroseland
added a commit
that referenced
this pull request
Aug 3, 2026
Pre-resolves the parts of the PR #13 (MM-69893) merge that are worth keeping whichever order the two PRs land in, so the editor branch can be merged, reverted and re-merged without redoing them. - webapp_globals: adopt the document-mode editor contract — contentType, extensions, onContentError, plus getEditor/hasContentError on the handle and a hostSupportsDocumentEditor probe. A Docs page body is structured content, so the editor is useless to us in markdown mode. Also onChange's param rename and ReactNodeArray -> ReactNode[], which drops a deprecation warning. - rest: thrown errors become RestError, a ClientError subclass that keeps the parsed payload and the server error id. Draft publish answers a 409 with the current page in the body, which ClientError alone discards. Existing instanceof ClientError / status_code callers are unaffected. - rest: add a path-relative doFetch over the same request path, for client modules that describe a whole endpoint family. - types: drop the stale note on Page — user_id and last_modified_by are here. The editor branch's client layer compiles against this without edits, so it no longer needs its own transport (which hand-rolled headers and relied on the deprecated XMLHttpRequest CSRF fallback). Prompt: merge PR#13 for preview with a revert-marked commit; conflict resolution in a separate commit that stays, to reduce conflicts when both PRs merge
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/page_editor/page_editor.tsx (1)
153-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winIgnore action results after the route changes.
If a publish for page A remains in flight after navigation to page B, Line 175 can open page A's conflict dialog on page B. The dialog then calls the current
publishcallback and force-publishes page B.
discardandsaveDraftAndLeavecan also callleave()for page A after navigation to page B.Capture a route generation or immutable page key when each action starts. Before each post-await state update, navigation, or conflict update, return when that key no longer matches the current route.
Proposed guard
+ const routeKeyRef = useRef(`${spaceId}:${pageId}`); + useEffect(() => { + routeKeyRef.current = `${spaceId}:${pageId}`; setContentError(false); // ... }, [spaceId, pageId]); const publish = useCallback(async (force: boolean, exitAfter = false) => { + const actionRouteKey = `${spaceId}:${pageId}`; // ... try { if (!await autosave.flush()) { return; } + if (routeKeyRef.current !== actionRouteKey) { + return; + } const published = await publishPageDraft(spaceId, pageId, force); + if (routeKeyRef.current !== actionRouteKey) { + return; + } // state updates and leave()Also applies to: 185-222
🤖 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 `@webapp/src/components/page_editor/page_editor.tsx` around lines 153 - 183, Guard publish, discard, and saveDraftAndLeave against stale results after navigation by capturing the current route generation or immutable page key when each action begins. Before every post-await state update, conflict/dialog update, or leave() call, verify the captured key still matches the current route and return otherwise; ensure stale page A actions cannot affect page B.
🤖 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.
Outside diff comments:
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 153-183: Guard publish, discard, and saveDraftAndLeave against
stale results after navigation by capturing the current route generation or
immutable page key when each action begins. Before every post-await state
update, conflict/dialog update, or leave() call, verify the captured key still
matches the current route and return otherwise; ensure stale page A actions
cannot affect page B.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d83bd77-8081-44df-b8b8-f242e7a2d7c4
📒 Files selected for processing (13)
webapp/i18n/en.jsonwebapp/src/components/page_editor/apply_formatting.tswebapp/src/components/page_editor/autosave_indicator.tsxwebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/publish_conflict_dialog.module.scsswebapp/src/components/page_editor/publish_conflict_dialog.tsxwebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/page_draft.test.tsxwebapp/src/hooks/page_draft.ts
💤 Files with no reviewable changes (1)
- webapp/src/components/page_editor/floating_formatting_bar.module.scss
🚧 Files skipped from review as they are similar to previous changes (9)
- webapp/src/components/page_editor/publish_conflict_dialog.module.scss
- webapp/src/components/page_editor/autosave_indicator.tsx
- webapp/src/hooks/page_draft.ts
- webapp/i18n/en.json
- webapp/src/components/page_editor/toolbar_controls.tsx
- webapp/src/components/page_editor/floating_formatting_bar.tsx
- webapp/src/hooks/draft_autosave.test.tsx
- webapp/src/hooks/draft_autosave.ts
- webapp/src/components/page_editor/apply_formatting.ts
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
4 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
POC for the Docs page editor. Mounts the core webapp's
WysiwygEditorat the page route in document mode and wires it to the draft API added in MM-69271.Built on the plugin surface published in MM-69912 (core). Uses all five additions from that contract:
contentType='json',extensions,onContentError,getEditor()andhasContentError().base_edit_atsent on every write for optimistic locking. Writes are serialized so publish cannot race an in-flight save, and a pending patch is flushed to the page you are leaving rather than dropped.concurrent_editfromconcurrent_autosave.page_presence_updatedwebsocket events, currently rendered as a count.FormattingBar, pinned to the top by default with a toggle to a floating bar over the selection.calloutnode already on the server allowlist.** Not in this PR **
Read/view mode, the in-page outline sidebar, presence avatars, and the text-colour / comment / AI toolbar controls.
are untouched.
Testing
Requires
MM_FEATUREFLAGS_ENABLEDOCS=true, otherwise every plugin API call is a 501.webapp/src/hooks/draft_autosave.test.tsxcovers debouncing, coalescing,base_edit_at, cancellation, in-flight flushing, and failure reporting.Manually: type and confirm the indicator settles on Saved, go offline and publish, confirming the editor stays open with an error rather than closing and losing the text; type and immediately switch pages, confirming the text is on the page you left.
Open questions for the team
localStorage, which is per-browser, should it be a real user preference?CommandProvideron any text starting with/, so typing/in a page currently autocompletes channel slash commands. Should/instead open a docs block-insert menu? This needs a decision before it can be fixed properly in core.Known issues that need fixing in Core
RangeErrorfromwysiwyg_editor.tsxand crashed the whole webapp. Guarded locally.isOpenfalse but the next keystroke re-runs the providers and reopens it.WysiwygSuggestionListhardcodesposition='top', worked around here with aMutationObserverthat re-anchors the popup to the caret.Ticket Link
https://mattermost.atlassian.net/browse/MM-69893