MM-69268 - Page tree CRUD + URL API: spaces, pages, move, duplicate - #4
Merged
Conversation
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>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 13, 2026
catalintomai
requested review from
JulienTant,
Willyfrog,
calebroseland and
jgheithcock
July 13, 2026 14:12
|
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 (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds Mattermost Docs APIs for spaces and pages, including membership checks, CRUD, pagination, hierarchy movement, duplication, optimistic locking, WebSocket events, feature-flag gating, persistence safeguards, and extensive integration tests. ChangesSpaces and pages API
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/store/page_hierarchy.go (1)
66-98: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExclude version snapshots from the descendants CTE.
Unlike the new ancestry and subtree templates, this CTE traverses rows where
OriginalId != ''. Snapshots can consequently appear in page trees, consume the descendant limit, and be treated as duplicateable descendants.Proposed fix
- FROM DOCS_Page WHERE Id = $1 AND DeleteAt = 0 + FROM DOCS_Page WHERE Id = $1 AND DeleteAt = 0 AND OriginalId = '' ... - WHERE p.DeleteAt = 0 AND d.depth < %d + WHERE p.DeleteAt = 0 AND p.OriginalId = '' AND d.depth < %d ... - WHERE d.Id != $1 AND p.DeleteAt = 0 + WHERE d.Id != $1 AND p.DeleteAt = 0 AND p.OriginalId = ''🤖 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_hierarchy.go` around lines 66 - 98, Update computeDescendantsCTE to exclude version snapshot rows by requiring OriginalId = '' in both the recursive page traversal and the root seed, matching the ancestry and subtree CTE behavior. Ensure snapshots cannot enter the descendants result or consume the hierarchy-depth limit.
🧹 Nitpick comments (3)
go.mod (1)
3-18: 🧹 Nitpick | 🔵 TrivialTrack the temporary core-branch pin to avoid silent breakage.
Pinning
server/publicto the head of an unreleased core branch (rather than a tag) means a rebase/force-push on that branch invalidates the pseudo-version+commit-hash pair, breakinggo mod/go.sumresolution for anyone re-fetching. The comment already documents this as temporary, which is good — consider also tracking it with a CI check or ticket link so it isn't forgotten once the core branch ships a release tag.🤖 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 `@go.mod` around lines 3 - 18, Track the temporary server/public pseudo-version pin in an automated CI check or an existing issue/ticket reference, using the dependency declaration and its accompanying temporary-pin comment as the integration points. Ensure the tracking mechanism detects or reminds maintainers to replace the unreleased core-branch pin with a release tag once the space-channel changes ship.server/app/ws_events_test.go (2)
139-152: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the total number of cross-space broadcasts.
AssertCalledpermits an additional broad or full-payload event, so this privacy contract could regress without failing. Require exactly two publications.Proposed test hardening
mockAPI.AssertCalled(t, "PublishWebSocketEvent", "page_moved_to_space", map[string]any{ "page_id": moved.Id, "target_space_id": spaceB.Id, "new_parent_id": "", }, &mmmodel.WebsocketBroadcast{ChannelId: chB}) +mockAPI.AssertNumberOfCalls(t, "PublishWebSocketEvent", 2)🤖 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/ws_events_test.go` around lines 139 - 152, Strengthen the assertions in the cross-space move test around the two PublishWebSocketEvent calls to require exactly two total publications, while preserving the existing source-space and target-space payload checks. Use the mockAPI call-count assertion for PublishWebSocketEvent so extra broad or full-payload broadcasts fail the test.
239-258: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the fallback publication before channel archival.
The test checks both calls occurred but not their order. A regression that publishes after
DeleteChannelwould still pass, although an archived-channel broadcast reaches nobody. Add an ordering assertion or sequence recorder.🤖 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/ws_events_test.go` around lines 239 - 258, Update TestServiceDeleteSpace_SnapshotFailureFallsBackToChannelBroadcast to record the relative order of PublishWebSocketEvent and DeleteChannel, then assert the fallback publication occurs before channel archival. Keep the existing argument assertions and failure-path setup unchanged.
🤖 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.go`:
- Around line 182-215: Update Service.RestorePage and the underlying
store.RestorePage operation to return the restored page row directly from the
atomic transaction. Remove the readBackAfterRestore/GetPageInSpace call and use
the returned page for publishing and the method result, while preserving
existing error mapping and validation.
In `@server/store/space_store.go`:
- Around line 368-417: Update withSpaceMembershipLock so any advisory unlock
failure marks the database connection for discard rather than returning it to
the pool. Track unlock errors from both the best-effort unlock after acquisition
failure and the deferred unlock after fn, then close or discard the connection
through the pool’s supported mechanism while preserving the existing warning
logs and error behavior.
In `@server/store/store_test.go`:
- Around line 906-911: Update the cycle-handling test around
FetchDescendantRowsForTest to assert that descendants is empty, rather than
discarding it with _ = descendants. Keep the existing no-error assertion and
verify the cyclic root is excluded, including preventing any returned root or
duplicate rows.
---
Outside diff comments:
In `@server/store/page_hierarchy.go`:
- Around line 66-98: Update computeDescendantsCTE to exclude version snapshot
rows by requiring OriginalId = '' in both the recursive page traversal and the
root seed, matching the ancestry and subtree CTE behavior. Ensure snapshots
cannot enter the descendants result or consume the hierarchy-depth limit.
---
Nitpick comments:
In `@go.mod`:
- Around line 3-18: Track the temporary server/public pseudo-version pin in an
automated CI check or an existing issue/ticket reference, using the dependency
declaration and its accompanying temporary-pin comment as the integration
points. Ensure the tracking mechanism detects or reminds maintainers to replace
the unreleased core-branch pin with a release tag once the space-channel changes
ship.
In `@server/app/ws_events_test.go`:
- Around line 139-152: Strengthen the assertions in the cross-space move test
around the two PublishWebSocketEvent calls to require exactly two total
publications, while preserving the existing source-space and target-space
payload checks. Use the mockAPI call-count assertion for PublishWebSocketEvent
so extra broad or full-payload broadcasts fail the test.
- Around line 239-258: Update
TestServiceDeleteSpace_SnapshotFailureFallsBackToChannelBroadcast to record the
relative order of PublishWebSocketEvent and DeleteChannel, then assert the
fallback publication occurs before channel archival. Keep the existing argument
assertions and failure-path setup unchanged.
🪄 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: 78af1742-8c39-4596-bcb0-3574e422e8e0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
.github/workflows/ci.ymlREADME.mdassets/i18n/en.jsongo.modplugin.jsonserver/api.goserver/api_handler_test.goserver/api_page.goserver/api_params_test.goserver/api_space.goserver/app/page.goserver/app/page_duplicate_test.goserver/app/page_hierarchy.goserver/app/page_move_test.goserver/app/page_move_to_space_test.goserver/app/page_reorder_test.goserver/app/pagination.goserver/app/pagination_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/configuration.goserver/internal/testutil/fixtures.goserver/model/draft.goserver/model/draft_test.goserver/model/page.goserver/model/page_test.goserver/model/props.goserver/model/space.goserver/model/space_test.goserver/plugin.goserver/store/draft_store.goserver/store/export_test.goserver/store/migrations/000004_add_page_originalid_index.down.sqlserver/store/migrations/000004_add_page_originalid_index.up.sqlserver/store/page_duplicate.goserver/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
jgheithcock
approved these changes
Jul 13, 2026
jgheithcock
left a comment
jgheithcock
left a comment
Collaborator
There was a problem hiding this comment.
I went through this fairly thoroughly with AI and the consensus was LGTM!
JulienTant
approved these changes
Jul 16, 2026
JulienTant
left a comment
JulienTant
left a comment
Member
There was a problem hiding this comment.
LGTM, I actually didn't know about github.com/mattermost/mattermost/server/public/plugin/plugintest and now I realize maybe making you extract logger last time was not necessary if you can abstract any call to the plugin api... oops.
Thanks!
edgarbellot
reviewed
Jul 17, 2026
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
@catalintomai LGTM, I just have one request.
| } | ||
|
|
||
| s.logDebug("Creating page", "space_id", spaceID, "parent_id", parentID, "user_id", userID) | ||
| s.log.Debug("Creating page", "space_id", spaceID, "parent_id", parentID, "user_id", userID) |
There was a problem hiding this comment.
The models implement Auditable - can we wire up audit records for the mutating operations? Right now they rely on s.log.Debug calls.
The Playbooks plugin has a good example of the pattern here:
auditRec := plugin.MakeAuditRecord("createPage", mmmodel.AuditStatusFail)
defer s.api.LogAuditRec(auditRec)
mmmodel.AddEventParameterToAuditRec(auditRec, "userID", userID)
mmmodel.AddEventParameterAuditableToAuditRec(auditRec, "page", *page)
// ... do the work ...
auditRec.Success()Having this in place would make incident response much easier.
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
Adds the page-tree CRUD + URL API surface under
/api/v1: space CRUD and membership, page CRUD scoped to a space, tree operations (move/reorder, duplicate, move-to-space), WebSocket events, and pagination.{space_id, page_id}, closing the race where a page is relocated out of its space mid-request; children listing.Supersedes #3.
Core PR:
mattermost/mattermost#37321
Ticket Link
https://mattermost.atlassian.net/browse/MM-69268