MM-69271: Editor authoring – page drafts, TipTap content handling, and presence - #5
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis change adds page-draft CRUD and publishing APIs, transactional draft storage, optimistic conflict handling, server-side TipTap content normalization, derived search text, active-editor presence events, hierarchy and quota validation, and extensive integration and store test coverage. ChangesDraft lifecycle and page content
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
server/store/store_test.go (1)
2684-2685: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific publish conflict reasons.
These tests pass even if
PublishDraftdrops the reason used by the app layer to distinguish concurrent edits from concurrent autosaves.Proposed assertions
_, err = s.PublishDraft(false, &edit, userID, space.Id, false, testDefaultMaxDepth, draft.UpdateAt) require.True(t, store.IsErrConflict(err), "a stale baseline must conflict, got %v", err) +require.Equal(t, store.ReasonConcurrentEdit, store.ConflictReason(err)) _, err = s.PublishDraft(false, &edit, userID, space.Id, false, testDefaultMaxDepth, stale.UpdateAt) require.True(t, store.IsErrConflict(err), "publishing stale draft content must conflict, got %v", err) +require.Equal(t, store.ReasonConcurrentAutosave, store.ConflictReason(err))Also applies to: 2743-2744
🤖 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 `@server/store/store_test.go` around lines 2684 - 2685, Strengthen the assertions around the PublishDraft calls in the stale-baseline tests near the shown assertion and the corresponding case at 2743-2744. In addition to verifying store.IsErrConflict(err), assert that the conflict error preserves the specific expected reason for each scenario, distinguishing concurrent edits from concurrent autosaves as consumed by the application layer.server/model/draft.go (1)
170-187: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
EditBaselineconflates "absent" with "unparseable type."If
original_page_edit_atis present but notfloat64/int64/int(e.g. a client sends a string), this silently returns(0, false)— identical to the key being absent entirely.SanitizePropsonly checks key presence, not value type, so a malformed value survives sanitization and could cause a previously-valid baseline to read back as "no baseline" on a later call, changing the optimistic-lock/new-page branching in whatever caller consumes this (not in this review batch). Consider surfacing "present but malformed" distinctly (e.g. a third return value, or reject atIsValid/write time) so a bad value fails loudly instead of silently downgrading to "no baseline."💡 Sketch: distinguish malformed from absent
-func (d *Draft) EditBaseline() (int64, bool) { +// EditBaseline extracts the optimistic-lock baseline. ok=false means "absent"; malformed=true means +// the key was present but not a recognized numeric type, so callers can reject it explicitly instead +// of treating it as "no baseline." +func (d *Draft) EditBaseline() (value int64, ok bool, malformed bool) { v, ok := d.GetProps()[DraftPropsOriginalPageEditAt] if !ok { - return 0, false + return 0, false, false } switch n := v.(type) { case float64: - return int64(n), n != 0 + return int64(n), n != 0, false case int64: - return n, n != 0 + return n, n != 0, false case int: - return int64(n), n != 0 + return int64(n), n != 0, false } - return 0, false + return 0, false, true }Also worth adding direct unit tests in draft_test.go for
EditBaseline's type-switch branches andSanitizeProps's allowlist behaviour — currently only exercised indirectly via app-layer integration tests.🤖 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 `@server/model/draft.go` around lines 170 - 187, Update Draft.EditBaseline so a present but unsupported original_page_edit_at value is distinguished from an absent or zero baseline, rather than returning the same (0, false) result; use an explicit error/status return or reject malformed values during validation/write handling. Preserve valid float64, int64, and int conversions, and add direct unit coverage for EditBaseline type branches and SanitizeProps allowlist 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 `@server/app/page_draft.go`:
- Around line 287-291: Ensure successful draft discards cannot be resurrected by
in-flight autosaves. Add a server-side tombstone or generation/order marker in
the discard flow and make UpsertDraft reject autosaves issued before the
discard, including unpublished drafts without a page row; preserve normal
autosave behavior for drafts that were not discarded.
- Around line 145-154: Update the presence limiter around
presenceBroadcastLast.LoadOrStore and CompareAndSwap to key entries by both page
ID and editor user, and apply the same page-and-user key to publish/delete
cleanup. In server/app/ws_events_test.go lines 350-389, add a second user and
verify that their first autosave broadcasts within the interval; retain the
existing repeated-save coverage for the original user.
- Around line 91-108: Update the unpublished-page validation in UpdatePageDraft
so a missing caller-owned draft is rejected even when another user has reserved
the page ID. Remove the AnyDraftExistsForPageInSpace-based allowance and return
the existing page-not-found error whenever PageExistsInSpace reports no live
page; retain the live-page path and existing error handling.
In `@server/app/page_presence.go`:
- Around line 28-40: The presence flow must distinguish a failed active-editor
query from a successful empty result. Update getActiveEditors and its callers,
including broadcastPagePresence, to return or propagate a success flag, and skip
publishing the snapshot or fresh as_of when the query fails; preserve the
originating request’s best-effort behavior and continue broadcasting valid empty
results.
In `@server/app/page.go`:
- Around line 316-328: Update clonePageFields so Props is recursively
deep-cloned rather than copied with maps.Clone. Preserve all existing fields and
use the project’s established recursive JSON-value cloning approach so nested
maps and slices in src.Props cannot alias the cloned page.
In `@server/app/service.go`:
- Around line 154-165: Make the ReasonDraftCycle and ReasonDraftTooDeep mappings
in storeAppError operation-aware so CreateSpaceDraft never receives the
update-only app.page_draft.update.* translation keys. Use the appropriate
create-safe or shared translation keys for create operations while preserving
the existing update mappings for update operations.
In `@server/store/draft_store.go`:
- Line 231: Run gofmt on the Go source containing the parentIDParam declaration
and ensure the resulting formatting passes the repository’s formatting check.
- Around line 490-529: Update DeleteDraftReparenting to reparent child drafts
only when the deleted draft’s page is no longer live in that draft’s space.
Before executing the reparentQ update, check the page’s live status using the
existing page/space lookup symbols; skip the update for published-page edit
drafts while preserving deletion and transaction behavior.
- Around line 188-197: The live-ancestor depth check in the draft validation
flow uses MaxPageHierarchyDepth instead of the application’s draft/publishing
limit. Update the comparison in pageDepth handling to use
draftCycleCheckMaxDepth (or the explicitly propagated publishing limit),
ensuring the computed live depth plus draft chain and new leaf cannot exceed the
10-level limit.
In `@server/store/page_move.go`:
- Around line 430-457: Update server/store/page_move.go lines 430-457 in
rewriteSubtreeSpace to compute the complete transitive draft closure, including
nested new-page descendants, before counting mover drafts and enforcing the
target-space quota; update lines 496-508 in rewriteSubtreeSpace to delete or
reparent every affected non-mover descendant from that same closure rather than
only direct children.
---
Nitpick comments:
In `@server/model/draft.go`:
- Around line 170-187: Update Draft.EditBaseline so a present but unsupported
original_page_edit_at value is distinguished from an absent or zero baseline,
rather than returning the same (0, false) result; use an explicit error/status
return or reject malformed values during validation/write handling. Preserve
valid float64, int64, and int conversions, and add direct unit coverage for
EditBaseline type branches and SanitizeProps allowlist behavior.
In `@server/store/store_test.go`:
- Around line 2684-2685: Strengthen the assertions around the PublishDraft calls
in the stale-baseline tests near the shown assertion and the corresponding case
at 2743-2744. In addition to verifying store.IsErrConflict(err), assert that the
conflict error preserves the specific expected reason for each scenario,
distinguishing concurrent edits from concurrent autosaves as consumed by the
application layer.
🪄 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: 0afb0479-a2c3-4178-8055-82d0f2445b95
📒 Files selected for processing (31)
assets/i18n/en.jsonserver/api.goserver/api_handler_test.goserver/api_page.goserver/api_page_drafts.goserver/api_page_drafts_test.goserver/api_page_presence.goserver/app/page.goserver/app/page_content.goserver/app/page_content_test.goserver/app/page_draft.goserver/app/page_draft_test.goserver/app/page_duplicate_test.goserver/app/page_hierarchy.goserver/app/page_presence.goserver/app/service.goserver/app/service_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/model/draft.goserver/model/draft_test.goserver/model/page_content.goserver/model/page_content_test.goserver/store/draft_store.goserver/store/migrations/000005_add_draft_lastactiveat.down.sqlserver/store/migrations/000005_add_draft_lastactiveat.up.sqlserver/store/page_move.goserver/store/page_move_test.goserver/store/page_store.goserver/store/store.goserver/store/store_test.go
Drop the hello-world scaffolding inherited from the plugin starter template: the /hello API route and handler, the hello slash command and its mocks, the demo background job, the KV store sample, public/hello.html, and the placeholder webapp test. The router, auth middleware, and configuration plumbing the Docs feature builds on are kept. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
catalintomai
requested review from
JulienTant,
Willyfrog,
calebroseland and
nang2049
July 23, 2026 15:16
jgheithcock
reviewed
Jul 24, 2026
| // broadcast is needed. | ||
| if !pageWasLive { | ||
| return nil | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggest calling publishToUser(wsEventPagePresenceUpdated with empty editor list or alternately, making a helper similar to publishSelfPresence
jgheithcock
reviewed
Jul 24, 2026
| return draft, nil | ||
| } | ||
|
|
||
| // DeletePageDraft removes the calling user's draft for the given page (on publish or discard). |
Collaborator
There was a problem hiding this comment.
Line 513 calls out that PublishPageDraft bypasses "the app-level DeletePageDraft" so this "(on publish or discard)" is misleading.
jgheithcock
reviewed
Jul 24, 2026
| nil, "", http.StatusConflict) | ||
| } | ||
| s.clearThrottleAndBroadcastPagePresence(pageID, userID, spaceID, existing.ChannelId) | ||
| return existing, false, nil |
Collaborator
There was a problem hiding this comment.
Instead of returning existing, suggestion is to refetch to handle any concurrency issues. Not sure if that is actually possible (but might be later?)
current, getErr := s.GetPage(pageID)
if getErr != nil {
return nil, false, getErr
}
s.clearThrottleAndBroadcastPagePresence(pageID, userID, spaceID, current.ChannelId)
return current, false, nil
jgheithcock
approved these changes
Jul 24, 2026
jgheithcock
left a comment
jgheithcock
left a comment
Collaborator
There was a problem hiding this comment.
Other than my nits, looks good.
JulienTant
requested changes
Jul 24, 2026
JulienTant
left a comment
JulienTant
left a comment
Member
There was a problem hiding this comment.
Also worth noting that I ask AI to review the indices and it returned those:
-
Missing composite index (PageId, SpaceId, LastActiveAt) for GetPageActiveEditors — a per-autosave hot path currently
scanning+re-filtering on the PageId-only index
File:Line: store/migrations/000005_*.up.sql, store/draft_store.go:625-650 -
Page-move draft-rehoming cascade filters/joins on unindexed SpaceId/ParentId while holding both space locks (lower urgency —
admin-triggered, not per-autosave)
File:Line: store/page_move.go:525-543
Source(s): rev-db-arch
Collaborator
Author
re: "composite (PageId, SpaceId, LastActiveAt):" re: "unindexed SpaceId/ParentId in the move cascade" |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/store/page_store.go (1)
781-795: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNil-patch guard missing in
PublishPageEditDraft.
PublishNewPageDraftguardspage == nilbefore use, but herepatch.IsValid()is called without a nil check. Add a symmetric guard so a store-level caller cannot panic.🛡️ Proposed guard
if userID == "" { return nil, &ErrInvalidInput{Entity: "Draft", Field: "userID", Value: userID} } + if patch == nil { + return nil, &ErrInvalidInput{Entity: "Page", Field: "Patch", Value: nil} + }🤖 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 `@server/store/page_store.go` around lines 781 - 795, Add a nil check for patch in PublishPageEditDraft before calling patch.IsValid(), returning the same store-level invalid-input error used for invalid patches. Preserve the existing validation order so nil or invalid patches are rejected before opening the transaction.
🤖 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.
Nitpick comments:
In `@server/store/page_store.go`:
- Around line 781-795: Add a nil check for patch in PublishPageEditDraft before
calling patch.IsValid(), returning the same store-level invalid-input error used
for invalid patches. Preserve the existing validation order so nil or invalid
patches are rejected before opening the transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 409536e9-ec0c-433b-9780-27580ff56182
📒 Files selected for processing (22)
assets/i18n/en.jsonserver/api_page_drafts.goserver/api_page_drafts_test.goserver/app/page_content.goserver/app/page_content_test.goserver/app/page_draft.goserver/app/page_draft_internal_test.goserver/app/page_draft_test.goserver/app/page_presence.goserver/app/service_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/model/draft.goserver/model/page_content.goserver/model/page_content_test.goserver/store/draft_store.goserver/store/draft_store_test.goserver/store/page_move.goserver/store/page_move_test.goserver/store/page_store.goserver/store/store.goserver/store/store_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- server/api_page_drafts.go
- server/model/draft.go
- assets/i18n/en.json
- server/store/page_move_test.go
- server/store/store.go
- server/store/page_move.go
- server/model/page_content.go
- server/api_page_drafts_test.go
- server/app/page_content.go
- server/model/page_content_test.go
- server/store/draft_store.go
Willyfrog
added a commit
that referenced
this pull request
Jul 27, 2026
Renumber the import migration and fix seven inspector/model hardening issues that are independent of PR #5: - Migration renumbered 000005 -> 000006 to avoid colliding with PR #5's 000005_add_draft_lastactiveat_baseeditat (morph keys on the version number, so two 000005s would block plugin activation). Updated the model comment and the implementation plan references. archive.go: - Validate mode, encryption, and compression method for every file entry, including data/ payloads that are never opened (previously method/encryption were checked only for import.jsonl/import-manifest.json). - Genuinely normalize entry names via path.Clean before duplicate detection (after the raw ".." check), so "data//x" and "data/x" collide as duplicates instead of the "normalized" map being a no-op alias of the raw map. inspect.go: - Reject a manifest with trailing data after its JSON object (decoder stopped at the first value). - Reject a JSONL line that carries a payload not matching its declared type (e.g. type:"page" also carrying a "space" payload). - Reject a bundle whose manifest source has no space key, since it becomes the ImportSource's required ExternalSpaceKey. - Use attachments_not_imported (plan section 20.2) for the attachment-records issue, distinct from the attachment_placeholder_not_imported link code. - Judge future timestamps against InspectOptions.Now + a skew allowance when supplied (fixed year-2100 ceiling as the pure-function fallback). - Include the manifest advisory target team in the aggregate team-mismatch check. model/import.go: - Require BundleSha256 to be a valid 64-hex digest (never empty) at the model boundary; a persisted job always has it from inspection. Added unit tests for each. go test ./server/... , go build ./... , and golangci-lint on the changed packages all pass; the renamed migration applies cleanly via the store test harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
calebroseland
approved these changes
Jul 27, 2026
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (6)
server/model/page_content_test.go (1)
131-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "denylist"/case-insensitive comments.
The sanitizer now uses a case-sensitive allowlist, so
SCRIPT/IFrame/MActionare rejected because they are absent from the allowlist, not because of case-insensitive denylist matching. The assertions still hold; only the annotations mislead.Also applies to: 154-155, 186-186
🤖 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 `@server/model/page_content_test.go` around lines 131 - 134, Update the comments in TestParseTipTapDocumentRejectsForbiddenTypes and the referenced assertion locations to describe the sanitizer’s case-sensitive allowlist, removing stale denylist and case-insensitive matching language while preserving the existing rejection assertions.server/app/page_content_test.go (1)
118-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo direct coverage for
normalizeContentBody.The draft autosave path goes through
normalizeContentBody, which is the only entry point that skips SearchText derivation. A small test (empty → no-op, valid TipTap → normalized, invalid → 400) would pin that branch alongside the page path.🤖 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 `@server/app/page_content_test.go` around lines 118 - 138, Add focused coverage for normalizeContentBody alongside the existing normalizePageContent tests: verify empty input is a no-op, valid TipTap content is normalized without SearchText derivation, and invalid content returns an HTTP 400 application error. Use the existing test helpers and assertions, and keep the tests scoped to the draft autosave normalization branch.server/model/page_content.go (2)
256-291: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSanitized values are written back under the original (untrimmed) key.
m[key] = sanitizeURL(v)re-keys nothing, so" href"stays" href"while a sibling"href"may also exist. Both are sanitized, so this is safe today, but a renderer resolving the trimmed name could see two entries. Consider deleting the padded variant instead of rewriting it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/model/page_content.go` around lines 256 - 291, The stripDangerousKeys function currently retains whitespace-padded attribute keys after sanitizing their values, allowing both padded and canonical names to coexist. When trimBrowserIgnoredChars changes a key, remove the original padded entry rather than writing the sanitized value back under it, while preserving the existing handling for canonical keys and dangerous attributes.
347-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttrs depth restarts at 0 while flat-key values inherit node depth.
sanitizeAttrs(attrs, 0)on Line 353 gives the attrs subtree its own 100-level budget, but Line 361 passes the nodedepthfor flat keys, so flat-key containers get100 - depth. Both fail closed, but the asymmetry is worth a short comment so a future reader doesn't read it as a bug.🤖 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 `@server/model/page_content.go` around lines 347 - 366, The depth behavior in sanitizeObjAttrsAndFlatKeys is intentional but undocumented: sanitizeAttrs should continue restarting the attrs subtree at depth 0, while sanitizeAttrValue should continue using the current node depth for flat-key values. Add a brief comment near these calls documenting this distinction without changing the existing depth handling.server/api_page_drafts_test.go (1)
120-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments say
PUT, but the requests arePATCH.Lines 121, 128 (comment), 135 and 159/168 describe the draft-update route as
PUTwhile every call useshttp.MethodPatch. Worth aligning so the comments stay usable as the contract description.♻️ Suggested wording fix
-// TestHandler_UpdatePageDraftRequiresExistingDraft confirms the update-only guard: PUT on a page id +// TestHandler_UpdatePageDraftRequiresExistingDraft confirms the update-only guard: PATCH on a page id // that has no existing draft must return 404 rather than silently creating one. @@ -// distinction: sending parent_id: "" in the PUT body must clear an existing parent (set it to +// distinction: sending parent_id: "" in the PATCH body must clear an existing parent (set it to // root), while omitting parent_id entirely must leave the parent unchanged.🤖 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 `@server/api_page_drafts_test.go` around lines 120 - 137, Align the draft-update test comments with the implemented HTTP method by replacing references to PUT with PATCH in TestHandler_UpdatePageDraftRequiresExistingDraft and the adjacent parent-clearing test comments, while leaving the http.MethodPatch requests unchanged.server/app/page_draft_test.go (1)
805-815: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
chainLenfrommodel.MaxPageDepthinstead of hard-coding10.The whole point of the test is the depth boundary, but the literal silently decouples from the constant if it ever changes — the test would then pass without exercising the limit.
♻️ Suggested change
- const chainLen = 10 + const chainLen = model.MaxPageDepth🤖 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 `@server/app/page_draft_test.go` around lines 805 - 815, Update the chain-length setup in the draft depth-boundary test to derive chainLen from model.MaxPageDepth instead of hard-coding 10, preserving the existing loop and parent-chain construction so the test continues to exercise the configured maximum depth.
🤖 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.
Nitpick comments:
In `@server/api_page_drafts_test.go`:
- Around line 120-137: Align the draft-update test comments with the implemented
HTTP method by replacing references to PUT with PATCH in
TestHandler_UpdatePageDraftRequiresExistingDraft and the adjacent
parent-clearing test comments, while leaving the http.MethodPatch requests
unchanged.
In `@server/app/page_content_test.go`:
- Around line 118-138: Add focused coverage for normalizeContentBody alongside
the existing normalizePageContent tests: verify empty input is a no-op, valid
TipTap content is normalized without SearchText derivation, and invalid content
returns an HTTP 400 application error. Use the existing test helpers and
assertions, and keep the tests scoped to the draft autosave normalization
branch.
In `@server/app/page_draft_test.go`:
- Around line 805-815: Update the chain-length setup in the draft depth-boundary
test to derive chainLen from model.MaxPageDepth instead of hard-coding 10,
preserving the existing loop and parent-chain construction so the test continues
to exercise the configured maximum depth.
In `@server/model/page_content_test.go`:
- Around line 131-134: Update the comments in
TestParseTipTapDocumentRejectsForbiddenTypes and the referenced assertion
locations to describe the sanitizer’s case-sensitive allowlist, removing stale
denylist and case-insensitive matching language while preserving the existing
rejection assertions.
In `@server/model/page_content.go`:
- Around line 256-291: The stripDangerousKeys function currently retains
whitespace-padded attribute keys after sanitizing their values, allowing both
padded and canonical names to coexist. When trimBrowserIgnoredChars changes a
key, remove the original padded entry rather than writing the sanitized value
back under it, while preserving the existing handling for canonical keys and
dangerous attributes.
- Around line 347-366: The depth behavior in sanitizeObjAttrsAndFlatKeys is
intentional but undocumented: sanitizeAttrs should continue restarting the attrs
subtree at depth 0, while sanitizeAttrValue should continue using the current
node depth for flat-key values. Add a brief comment near these calls documenting
this distinction without changing the existing depth handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0887b40-fba8-4190-afd5-619f0fac7691
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
README.mdassets/i18n/en.jsongo.modserver/api.goserver/api_handler_test.goserver/api_page.goserver/api_page_drafts.goserver/api_page_drafts_test.goserver/api_page_presence.goserver/app/page.goserver/app/page_content.goserver/app/page_content_test.goserver/app/page_draft.goserver/app/page_draft_internal_test.goserver/app/page_draft_test.goserver/app/page_duplicate_test.goserver/app/page_hierarchy.goserver/app/page_move_test.goserver/app/page_move_to_space_test.goserver/app/page_presence.goserver/app/page_presence_test.goserver/app/pagination.goserver/app/service.goserver/app/service_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/model/draft.goserver/model/draft_test.goserver/model/page.goserver/model/page_content.goserver/model/page_content_test.goserver/model/page_presence.goserver/model/props.goserver/model/props_test.goserver/model/space.goserver/store/draft_store.goserver/store/draft_store_test.goserver/store/migrations/000005_add_draft_lastactiveat_baseeditat.down.sqlserver/store/migrations/000005_add_draft_lastactiveat_baseeditat.up.sqlserver/store/page_hierarchy.goserver/store/page_move.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
Contributor
|
a bit late, but approved ;) |
This was referenced Aug 3, 2026
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
6 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
This PR adds the editor-authoring layer on top of the page tree CRUD foundation (MM-69268). It introduces per-user page drafts with autosave semantics, TipTap document sanitization and content extraction, and a lightweight presence system derived from draft activity.
Page Drafts (autosave)
Six new endpoints manage the full draft lifecycle:
POST/spaces/{space_id}/draftsGET/spaces/{space_id}/draftsPATCH/spaces/{space_id}/pages/{page_id}/draftGET/spaces/{space_id}/pages/{page_id}/draftDELETE/spaces/{space_id}/pages/{page_id}/draftPOST/spaces/{space_id}/pages/{page_id}/draft/publishMerge-on-write autosave:
PATCH /draftmerges rather than replaces — only fields the editor touched need to be sent. Partial heartbeats from different editor panels cannot clobber each other.Optimistic-lock conflict detection: the client stores
original_page_edit_atin draft props when opening an existing page. On publish, the server rejects with409 Conflictif another user has since saved the page, prompting a merge flow.Draft migration (
000005_add_draft_lastactiveat): addsLastActiveAtto theDOCS_Drafttable, stamped on every autosave and used as the presence heartbeat.TipTap Content Handling
model.ParseTipTapDocument— parses and sanitizes TipTap JSON (strips disallowed node types, base64-decodes image src, enforces depth/size limits).model.BuildSearchText— extracts plain text + mention labels for full-text indexing.app.normalizePageContent— shared normalization path used by both draft autosave and page publish, so stored content is always sanitized before it lands in the DB.Page Presence (active editors)
GET /spaces/{space_id}/pages/{page_id}/active-editorsreturns user IDs actively editing the page.DOCS_Draft.LastActiveAt: any user with a draft updated within the last 5 minutes counts as active.WebSocket Events
page_presence_updated— the active-editors snapshot (active_editors,as_of,active_timeout_ms). Broadcast channel-wide on autosave (rate-limited to 1 per 30 s per page per editor) and unconditionally on discard and publish; for a new-page draft (no live page yet) it is sent only to the author to avoid disclosing the reserved page id.page_created— fired when a publish creates the live page for a new-page draft.page_updated— fired when a publish updates an already-live page.