MM-69268 - Page tree CRUD + URL API: spaces, pages, move, duplicate#3
Open
catalintomai wants to merge 62 commits into
Open
MM-69268 - Page tree CRUD + URL API: spaces, pages, move, duplicate#3catalintomai wants to merge 62 commits into
catalintomai wants to merge 62 commits into
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a space-scoped ChangesDocs Plugin Core Implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Build Metadata and Translations
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 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: 12
🧹 Nitpick comments (2)
server/model/page_test.go (1)
216-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for
PagePatch.IsValid.The store tests exercise this contract, but the model package should fail close to the source if nil/empty patches or Body/SearchText mismatches regress.
Suggested test coverage
+func TestPagePatchIsValid(t *testing.T) { + t.Run("nil patch rejected", func(t *testing.T) { + var patch *model.PagePatch + aerr := patch.IsValid() + require.NotNil(t, aerr) + require.Equal(t, "model.page.patch.nothing_to_update.app_error", aerr.Id) + }) + + t.Run("empty patch rejected", func(t *testing.T) { + aerr := (&model.PagePatch{}).IsValid() + require.NotNil(t, aerr) + require.Equal(t, "model.page.patch.nothing_to_update.app_error", aerr.Id) + }) + + t.Run("body and search text must be patched together", func(t *testing.T) { + body := "body" + aerr := (&model.PagePatch{Body: &body}).IsValid() + require.NotNil(t, aerr) + require.Equal(t, "model.page.patch.search_text_body_mismatch.app_error", aerr.Id) + }) +}🤖 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_test.go` around lines 216 - 247, Add direct unit coverage for PagePatch.IsValid in the model tests so the validation contract is checked at the source. Extend TestPagePatch with cases for a valid patch, nil/empty patch rejection, and Body/SearchText mismatch rejection, using PagePatch and IsValid to verify the expected pass/fail behavior.server/model/draft_test.go (1)
57-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a self-parent validation test.
server/model/draft.gorejectsParentId == PageId, but this suite never assertsmodel.draft.is_valid.parent_self.app_error. That leaves the new circular-reference guard untested.Suggested test
t.Run("empty ParentId accepted", func(t *testing.T) { d := validDraft() d.ParentId = "" require.Nil(t, d.IsValid()) }) + + t.Run("ParentId equal to PageId rejected", func(t *testing.T) { + d := validDraft() + d.ParentId = d.PageId + aerr := d.IsValid() + require.NotNil(t, aerr) + require.Equal(t, "model.draft.is_valid.parent_self.app_error", aerr.Id) + })🤖 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_test.go` around lines 57 - 69, Add a new unit test in draft_test.go alongside the existing ParentId cases to verify that Draft.IsValid rejects a draft whose ParentId matches its PageId. Use validDraft() to build the draft, set ParentId equal to PageId, and assert the returned AppError is not nil with Id model.draft.is_valid.parent_self.app_error; keep the existing non-empty invalid and empty ParentId tests 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 `@go.mod`:
- Around line 5-10: The go.mod replace directive for
github.com/mattermost/mattermost/server/public uses a developer-local absolute
path, which is not portable. Remove the local replace from go.mod and keep any
branch-specific override in local-only configuration such as go.work, using the
module reference in go.mod so clean CI and other checkouts resolve correctly.
In `@plugin.json`:
- Around line 2-8: The plugin manifest is still advertising a server version
that predates the new ChannelTypeSpace dependency. Update the min_server_version
field in the plugin manifest to the first Mattermost server release that
includes that core change, and if that release is not available yet, keep the
plugin unreleased until it is. Use the existing manifest entry in plugin.json to
locate and adjust the version gate.
In `@README.md`:
- Around line 23-28: The README’s Test and lint section currently implies make
test works without setup, but the new store/service tests require a reachable
Postgres DSN. Update this section to explicitly mention TEST_DATABASE_DSN or the
expected local Postgres configuration next to make test, so contributors know
the prerequisite before running the test target.
In `@server/api.go`:
- Around line 21-27: The router in initRouter currently applies only
MattermostAuthorizationRequired, which leaves all API endpoints open to any
authenticated user across spaces/pages. Add per-space and per-page authorization
checks before handlers are reachable, and apply the same enforcement
consistently across the route registrations in initRouter and the related route
handlers that perform read, move, duplicate, or delete actions. Use the existing
Plugin router setup and any space/page access helpers to gate each request based
on the requested resource, not just the Mattermost-User-ID.
- Around line 34-39: The space router currently registers
create/get/update/delete in the Space CRUD block but omits the restore endpoint,
so add the missing restore route alongside the existing registrations in the API
setup. Use the same routing pattern as the other space handlers and wire it to
the appropriate restore handler method (for example the space restore handler on
the same router object) so archived spaces can be unarchived via HTTP.
In `@server/app/page_hierarchy.go`:
- Around line 131-134: The initial page lookup in page_hierarchy flow is too
broad because it uses GetPage by pageID only, so the pre-flight validation can
operate on a page outside the caller’s route space. Update the lookup in the
page move path to use GetPageInSpace first, and in the later validation block
replace the pageID-only fetch with the same route-scoped lookup so both checks
are anchored to the validated source space. Use the existing page_hierarchy
methods and pageID/page source space values already available in the move logic,
and make the team comparison against that validated source space rather than an
unscoped page record.
In `@server/app/page.go`:
- Around line 229-240: The preflight read in RestorePage is not scoped to the
caller’s space, so a valid page ID from another space can return not_deleted or
not_restorable instead of a scoped 404. Update the RestorePage flow in
server/app/page.go to use the spaceID when calling the page lookup (the
s.store.GetPage preflight before the OriginalId/DeleteAt checks), or otherwise
ensure the preflight enforces the same space scoping as store.RestorePage. Keep
the existing restore-state checks, but only after the lookup is constrained to
the requested space.
- Around line 105-110: `CreatePage` is conflating two different ErrNotFound
cases from `s.store.CreatePage(page)`: a missing space and a vanished/moved
parent. Update the error handling in this path so it distinguishes the missing
parent case from the space row case, using the page’s parent/locked-parent
context before returning the app error. Keep `space_not_found` only for the
actual missing space scenario, and return the earlier `invalid_parent` contract
when the parent disappears after the pre-check.
In `@server/model/props.go`:
- Around line 37-73: deepCloneAny currently only deep-copies a few concrete
shapes, so Clone() is still aliasing nested typed slices/maps like []int or
map[string][]string. Update deepCloneAny (and deepCloneStringInterface if
needed) to either recursively handle all supported nested map/slice forms used
by StringInterface, or explicitly narrow the contract/documentation so only the
handled shapes are allowed. Keep the fix centered around deepCloneAny and
deepCloneStringInterface so the clone behavior is consistently deep for the
supported data model.
In `@server/model/space.go`:
- Around line 144-150: The Space.IsValid validation currently only checks TeamId
format when present, but TeamId must be required for all persisted spaces while
CreatorId remains optional. Update the Space.IsValid method in space.go so it
rejects empty TeamId as well as invalid IDs, and keep the existing CreatorId
validation conditional so the userID == "" path still works. Use the existing
mmmodel.NewAppError flow and the Space.IsValid symbol to locate and adjust the
validation logic.
In `@server/plugin.go`:
- Around line 49-57: The store created in plugin activation is wrapping the
server-owned master DB, so its cleanup path must not close that shared handle.
Update the plugin lifecycle around GetMasterDB, store.New, and Store.Close so
the plugin only releases its own store state while leaving the master *sql.DB
open; adjust the deferred cleanup and OnDeactivate logic accordingly, and apply
the same ownership fix in the other activation paths mentioned in the diff.
In `@server/store/page_store.go`:
- Around line 163-179: The depth-cap recheck in page duplication only runs
inside the root.ParentId != "" branch, so duplicating a subtree to the space
root can bypass maxDepth. Update the logic in PageStore’s duplication flow
around the root page handling to validate the subtree depth even when ParentId
is empty, using the same pageDepth/localSubtreeMaxDepth check and
ErrLimitExceeded path as the existing parent-locked validation.
---
Nitpick comments:
In `@server/model/draft_test.go`:
- Around line 57-69: Add a new unit test in draft_test.go alongside the existing
ParentId cases to verify that Draft.IsValid rejects a draft whose ParentId
matches its PageId. Use validDraft() to build the draft, set ParentId equal to
PageId, and assert the returned AppError is not nil with Id
model.draft.is_valid.parent_self.app_error; keep the existing non-empty invalid
and empty ParentId tests unchanged.
In `@server/model/page_test.go`:
- Around line 216-247: Add direct unit coverage for PagePatch.IsValid in the
model tests so the validation contract is checked at the source. Extend
TestPagePatch with cases for a valid patch, nil/empty patch rejection, and
Body/SearchText mismatch rejection, using PagePatch and IsValid to verify the
expected pass/fail 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: 29f118e4-b81e-4717-b5ec-626e897ebe57
⛔ Files ignored due to path filters (4)
assets/icon.svgis excluded by!**/*.svggo.sumis excluded by!**/*.sumgo.tools.sumis excluded by!**/*.sumwebapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
.github/workflows/ci.yml.gitignore.golangci.ymlMakefileREADME.mdassets/i18n/en.jsongo.modgo.tools.modplugin.jsonpublic/hello.htmlserver/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/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/command/command.goserver/command/command_test.goserver/command/mocks/mock_commands.goserver/internal/testutil/testdb.goserver/job.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/plugin_test.goserver/store/draft_store.goserver/store/export_test.goserver/store/kvstore/kvstore.goserver/store/kvstore/startertemplate.goserver/store/migrations/000001_create_spaces.down.sqlserver/store/migrations/000001_create_spaces.up.sqlserver/store/migrations/000002_create_pages.down.sqlserver/store/migrations/000002_create_pages.up.sqlserver/store/migrations/000003_create_drafts.down.sqlserver/store/migrations/000003_create_drafts.up.sqlserver/store/page_hierarchy.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.gowebapp/babel.config.jswebapp/i18n/en.jsonwebapp/package.jsonwebapp/src/react_fragment.test.tsx
💤 Files with no reviewable changes (8)
- public/hello.html
- server/job.go
- server/command/command.go
- server/store/kvstore/kvstore.go
- server/store/kvstore/startertemplate.go
- server/command/mocks/mock_commands.go
- server/command/command_test.go
- webapp/src/react_fragment.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/api.go (1)
148-164: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
decodeJSONBodydoesn't reject trailing data after the JSON value.
json.Decoder.Decodeonly consumes the first JSON value in the stream; anything appended after it (e.g., a concatenated second object) is silently ignored rather than rejected. This is a well-knownencoding/jsonfootgun — "Users often write json.NewDecoder(r).Decode(v), which is incorrect since it does not reject trailing junk at the end of the payload". Consider a follow-upDecode(&struct{}{})call expectingio.EOFto reject trailing data.♻️ Proposed fix
func decodeJSONBody(w http.ResponseWriter, r *http.Request, maxBytes int64, v any, where string, allowEmptyBody bool) bool { r.Body = http.MaxBytesReader(w, r.Body, maxBytes) dec := json.NewDecoder(r.Body) if err := dec.Decode(v); err != nil { if allowEmptyBody && errors.Is(err, io.EOF) { return true } var maxBytesErr *http.MaxBytesError if errors.As(err, &maxBytesErr) { writeAppError(w, mmmodel.NewAppError(where, "api.request_too_large.app_error", map[string]any{"MaxBytes": maxBytes}, "", http.StatusRequestEntityTooLarge)) return false } writeAppError(w, mmmodel.NewAppError(where, "api.invalid_json.app_error", nil, "", http.StatusBadRequest)) return false } + // Reject trailing data after the first JSON value (e.g. two concatenated objects). + if err := dec.Decode(&struct{}{}); !errors.Is(err, io.EOF) { + writeAppError(w, mmmodel.NewAppError(where, "api.invalid_json.app_error", nil, "", http.StatusBadRequest)) + return false + } return true }🤖 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.go` around lines 148 - 164, `decodeJSONBody` currently accepts extra bytes after the first JSON value because it only calls `json.Decoder.Decode(v)`. Update this helper to verify the request body is fully consumed after a successful decode by performing a follow-up decode into a discard value and requiring `io.EOF`; if any trailing data remains, return the same invalid JSON app error. Keep the existing `allowEmptyBody`, `http.MaxBytesReader`, and `writeAppError` behavior intact while tightening validation in `decodeJSONBody`.
🤖 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 `@plugin.json`:
- Around line 5-8: The install gate in plugin.json is too low for the
ChannelTypeSpace dependency. Update the min_server_version in plugin.json to the
first server release that actually includes ChannelTypeSpace so it matches the
server/public commit pinned in go.mod; use the min_server_version field as the
source of truth and keep it aligned with the plugin’s channel-creation flow.
In `@server/api_space.go`:
- Around line 60-125: The space handlers currently trust only authentication, so
any logged-in user can access or mutate an arbitrary space by `space_id`. Add a
per-space authorization/membership check in each of `handleGetSpace`,
`handleUpdateSpace`, `handleDeleteSpace`, `handleRestoreSpace`, and
`handleGetSpacePages`, ideally by resolving the current actor and verifying
access before calling `p.service.GetSpace`, `UpdateSpace`, `DeleteSpace`,
`RestoreSpace`, or `GetSpacePages`. If there is an existing service/helper for
space membership/permission validation, use it consistently from these handlers
so unauthorized requests are rejected early.
---
Nitpick comments:
In `@server/api.go`:
- Around line 148-164: `decodeJSONBody` currently accepts extra bytes after the
first JSON value because it only calls `json.Decoder.Decode(v)`. Update this
helper to verify the request body is fully consumed after a successful decode by
performing a follow-up decode into a discard value and requiring `io.EOF`; if
any trailing data remains, return the same invalid JSON app error. Keep the
existing `allowEmptyBody`, `http.MaxBytesReader`, and `writeAppError` behavior
intact while tightening validation in `decodeJSONBody`.
🪄 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: 67d691ad-f56f-42dc-9bce-ef6d4bb3d0eb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
README.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_internal_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/model/draft_test.goserver/model/page.goserver/model/page_test.goserver/model/space.goserver/plugin.goserver/store/draft_store.goserver/store/migrations/000004_add_hierarchy_move_indexes.down.sqlserver/store/migrations/000004_add_hierarchy_move_indexes.up.sqlserver/store/page_hierarchy.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
calebroseland
added a commit
that referenced
this pull request
Jul 8, 2026
…t route
Align URL scheme with API spec (GET /api/v1/teams/{team_id}/spaces):
- DOCS_BASE_URL /docs → /spaces
- Route params constrained to 26-char lowercase-alphanumeric (MM_ID),
so malformed/truncated links are rejected browser-side before any fetch
- Add DOCS_DRAFT_ROUTE + draftPath() builder for /:spaceId/drafts/:pageId
- Expose goToDraft() and paths.draft in useDocsNavigation
TODO comment: /{teamName} prefix requires core registerProduct() support
for team-scoped baseURLs (tracked separately).
Prompt: align URL routing with mattermost-plugin-docs PR #3 which uses
/api/v1/teams/{team_id}/spaces; spec says 'spaces' replaces old 'wiki' term
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api_page.go (1)
20-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWebSocket broadcasts are inconsistent across structural page mutations.
handleMovePage,handleDuplicatePage, andhandleMovePageToSpacepublish WS events (Lines 185-187, 235-239, 304-306) so other clients refresh the tree, buthandleCreatePage,handleDeletePage, andhandleRestorePage— which also change the visible page tree — publish nothing. Other users viewing the same space won't see a live update when a page is created, deleted, or restored, only when it's moved/duplicated/relocated.Consider extending
publishToChannelscalls to create/delete/restore for consistency, reusing the samespaceChannelID(space)pattern already established here.Also applies to: 125-152
🤖 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.go` around lines 20 - 77, WebSocket broadcasts are missing for some structural page-tree mutations, so clients don’t get live refreshes after create/delete/restore. Update the relevant handlers in this file—especially handleCreatePage, handleDeletePage, and handleRestorePage—to call publishToChannels with the same page-tree event pattern used by handleMovePage, handleDuplicatePage, and handleMovePageToSpace. Reuse spaceChannelID(space) (and any other affected space/channel IDs) so broadcasts stay consistent, deduplicated, and best-effort without changing the primary mutation response.
🤖 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/space.go`:
- Around line 141-155: The membership bypass is too permissive when the
pluginapi client is missing: CheckSpaceMembership currently returns success for
authenticated requests, which can leak direct space reads and unfiltered
listings. Update CheckSpaceMembership and the related team-space listing path to
fail closed whenever s.client is nil, reserving the nil-return bypass only for
the system caller case where userID == "". Keep the existing identifiers
CheckSpaceMembership and the listing helper used in the 201-234 range aligned so
both paths reject authenticated access without a wired client.
---
Nitpick comments:
In `@server/api_page.go`:
- Around line 20-77: WebSocket broadcasts are missing for some structural
page-tree mutations, so clients don’t get live refreshes after
create/delete/restore. Update the relevant handlers in this file—especially
handleCreatePage, handleDeletePage, and handleRestorePage—to call
publishToChannels with the same page-tree event pattern used by handleMovePage,
handleDuplicatePage, and handleMovePageToSpace. Reuse spaceChannelID(space) (and
any other affected space/channel IDs) so broadcasts stay consistent,
deduplicated, and best-effort without changing the primary mutation response.
🪄 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: 96e0fdd5-7882-4731-8d66-f4a591eaecab
📒 Files selected for processing (31)
.github/workflows/ci.ymlassets/i18n/en.jsongo.modserver/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_internal_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/model/page.goserver/model/props.goserver/model/space_test.goserver/plugin.goserver/store/migrations/000004_add_page_originalid_index.down.sqlserver/store/migrations/000004_add_page_originalid_index.up.sqlserver/store/page_hierarchy.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
💤 Files with no reviewable changes (2)
- server/store/migrations/000004_add_page_originalid_index.up.sql
- server/store/migrations/000004_add_page_originalid_index.down.sql
✅ Files skipped from review due to trivial changes (1)
- server/model/props.go
🚧 Files skipped from review as they are similar to previous changes (13)
- server/app/pagination_internal_test.go
- server/app/page_reorder_test.go
- go.mod
- server/plugin.go
- server/api_space.go
- server/store/page_move_test.go
- server/app/page_move_to_space_test.go
- server/app/page_duplicate_test.go
- server/app/page_move_test.go
- server/store/page_hierarchy.go
- server/app/page_hierarchy.go
- server/store/store_test.go
- server/app/service_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
go.mod (1)
3-17: 🔒 Security & Privacy | 🔵 TrivialConfirm CI vulnerability gates tolerate the dev-only pre-release pins.
server/public/server/v8are intentionally pinned to unreleased pre-release commits (documented rationale at Lines 5-11), and static analysis surfaces a large number of HIGH-severity advisories against theserver/v8pseudo-version. Per the comment,server/v8is test-harness-only and contributes no runtime symbols, so most of these are likely non-actionable noise for this plugin's actual attack surface — but worth confirming the CI security-scanning step (in.github/workflows/ci.yml) doesn't hard-fail on this pin, and that there's a tracked follow-up to bump both pins once the corresponding release tags ship (as the comment already promises).🤖 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 - 17, Verify the CI security-scanning step in ci.yml handles the intentional pre-release pins for server/public and server/v8 without failing on non-runtime advisories, especially the HIGH findings from the test-only server/v8 dependency. Adjust scan scope or exclusions only as needed, preserving runtime dependency coverage, and add a tracked follow-up issue to bump both modules to release tags when available.Source: Linters/SAST tools
🤖 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/api.go`:
- Around line 95-106: Update EnableDocsRequired to return the same structured
JSON AppError response as other API error paths by using writeAppError instead
of http.Error, with the appropriate 501 status and docs-not-enabled error
identifier/message. Add the matching api.docs_not_enabled.app_error translation
entry to assets/i18n/en.json.
In `@server/app/space.go`:
- Around line 145-181: Check s.client for nil in CheckSpaceMembership before
calling Team.GetMember or Channel.GetMember; return the same fail-closed
internal-server-error AppError used by CreateSpace. Place the guard after space
retrieval and before either membership lookup, preserving existing behavior when
a client is available.
- Around line 194-231: Add an early s.client == nil guard in GetSpacesForTeam
before any userID-dependent calls, returning an appropriate
internal-server-error AppError; ensure both s.client.Team.GetMember and
s.client.Channel.ListForTeamForUser are protected without changing the
system-caller path.
In `@server/store/page_move_test.go`:
- Around line 243-258: Update TestMovePage_StoreReindexClampSingleSibling to
cover a negative sibling index that could underflow, either by adding a sub-case
using a negative index and asserting the child remains correctly ordered, or by
revising the doc comment to describe only the existing positive-index clamping
behavior.
---
Nitpick comments:
In `@go.mod`:
- Around line 3-17: Verify the CI security-scanning step in ci.yml handles the
intentional pre-release pins for server/public and server/v8 without failing on
non-runtime advisories, especially the HIGH findings from the test-only
server/v8 dependency. Adjust scan scope or exclusions only as needed, preserving
runtime dependency coverage, and add a tracked follow-up issue to bump both
modules to release tags when available.
🪄 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: 81ea6d29-b3a9-4fac-9bc2-02c81b7e0280
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.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_internal_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/model/draft_test.goserver/model/page.goserver/model/page_test.goserver/model/props.goserver/model/space_test.goserver/plugin.goserver/store/draft_store.goserver/store/migrations/000004_add_page_originalid_index.down.sqlserver/store/migrations/000004_add_page_originalid_index.up.sqlserver/store/page_hierarchy.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
catalintomai
changed the base branch from
MM-69267-foundations-data-model
to
master
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
server/plugin.go (1)
58-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose() errors are now silently dropped.
Both
_ = s.Close()on migration failure and_ = p.store.Close()inOnDeactivatediscard the error entirely, whereas the previous implementation logged close failures viap.API.LogError. Sincep.client.Logis already available and passed intostore.New, logging the close error would preserve observability of connection-pool cleanup failures (e.g., stuck connections across deactivate/reactivate cycles) without changing control flow.♻️ Suggested fix
if migErr := s.RunMigrations(); migErr != nil { - _ = s.Close() + if closeErr := s.Close(); closeErr != nil { + p.client.Log.Warn("failed to close store after migration failure", "err", closeErr) + } return errors.Wrap(migErr, "failed to run docs migrations") }func (p *Plugin) OnDeactivate() error { if p.store != nil { - _ = p.store.Close() + if closeErr := p.store.Close(); closeErr != nil { + p.client.Log.Warn("failed to close store on deactivate", "err", closeErr) + } } return nil }Also applies to: 73-77
🤖 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/plugin.go` around lines 58 - 61, Preserve observability for store cleanup failures instead of discarding Close errors. In the migration-failure path of the startup method containing RunMigrations, capture s.Close() errors and log them through the available client logger before returning the wrapped migration error; likewise, update OnDeactivate to capture p.store.Close() errors and log them via p.client.Log, without changing the existing control flow.
🤖 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 `@go.mod`:
- Line 3: Update the Go version declaration in go.mod from 1.26.3 to 1.26.5 so
the CI workflow uses the patched toolchain version.
In `@server/api_page.go`:
- Around line 15-18: The maxPageBodyBytes constant underestimates worst-case
JSON size because string escaping and other accepted fields add overhead.
Calculate the cap from the worst-case JSON-encoded sizes of PageBodyMaxBytes and
PageSearchTextMaxBytes, including escaping, field names, envelope, and all other
request fields accepted by the page create/update handlers, so valid payloads
reach model validation without premature 413 responses.
In `@server/api.go`:
- Around line 182-185: The trailing-data validation in the JSON decoding flow
currently maps all non-EOF errors to 400, losing 413 for oversized input. Update
the second Decode check to detect *http.MaxBytesError, as in the initial decode
handling, and call writeAppError with http.StatusRequestEntityTooLarge; retain
the existing 400 response for other trailing-data errors.
In `@server/app/service.go`:
- Around line 145-150: Prevent overflow in paginationOffsetLimit by validating
or clamping page before multiplying it by the normalized perPage value, or by
using checked multiplication; ensure the returned offset is safely bounded
rather than negative or wrapped for excessively large page parameters.
In `@server/app/space_test.go`:
- Around line 222-258: RestoreSpace currently leaves the space marked live after
backing-channel restoration fails, preventing reliable retries. Update
RestoreSpace to compensate the database state by keeping or reverting the space
as deleted when RestoreChannel fails, or make already-live spaces retry channel
restoration; ensure the existing
TestServiceRestoreSpace_ChannelRestoreFailurePropagates scenario remains
retryable and preserves consistent space/channel state.
In `@server/store/migrations/000004_add_page_originalid_index.up.sql`:
- Around line 1-2: The migration should remove any existing invalid or partial
index before creation. In the nontransactional migration, add a drop for the
named index idx_docs_page_originalid, then recreate it with CREATE INDEX
CONCURRENTLY without IF NOT EXISTS, preserving the existing table, column, and
predicate.
---
Nitpick comments:
In `@server/plugin.go`:
- Around line 58-61: Preserve observability for store cleanup failures instead
of discarding Close errors. In the migration-failure path of the startup method
containing RunMigrations, capture s.Close() errors and log them through the
available client logger before returning the wrapped migration error; likewise,
update OnDeactivate to capture p.store.Close() errors and log them via
p.client.Log, without changing the existing control flow.
🪄 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: 3066c84a-19c1-4879-909a-f83a39577e44
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.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_internal_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/model/draft_test.goserver/model/page.goserver/model/page_test.goserver/model/props.goserver/model/space_test.goserver/plugin.goserver/store/draft_store.goserver/store/migrations/000004_add_page_originalid_index.down.sqlserver/store/migrations/000004_add_page_originalid_index.up.sqlserver/store/page_hierarchy.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
Collaborator
Author
|
@coderabbitai full review. Also , can you resolve all previous comments that have been addressed already? |
|
On resolving comments: the ✅ Action performedComments resolved and changes approved. Full review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go.mod (1)
17-17: 🔒 Security & Privacy | 🔵 TrivialConfirm the
server/v8dev-pin will be bumped before a stable release.The pinned pseudo-version (
8.0.0-20260623200446-ba033eae4704) triggers dozens of historical OSV advisories againstmattermost-server/mattermost-plugin-playbooks. The file's own comment (lines 5-11) explains this is a deliberate temporary pin forChannelTypeSpace/GetSpaceBackingChannelahead of a releasedserver/public, so this is not a new defect — just flagging so the bump-to-release-tag follow-up isn't dropped, since a stale dev pin left in a shipped release would carry these advisories forward.🤖 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` at line 17, Track the temporary Mattermost server/v8 pseudo-version pin for replacement before the stable release; update the dependency to the released server/public-compatible version while preserving support for ChannelTypeSpace and GetSpaceBackingChannel.Source: Linters/SAST tools
🤖 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 `@go.mod`:
- Line 17: Track the temporary Mattermost server/v8 pseudo-version pin for
replacement before the stable release; update the dependency to the released
server/public-compatible version while preserving support for ChannelTypeSpace
and GetSpaceBackingChannel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 233ddde8-f9e8-4b7e-b4fa-d455c7c5ce7b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.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_internal_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_test.goserver/model/draft_test.goserver/model/page.goserver/model/page_test.goserver/model/props.goserver/model/space_test.goserver/plugin.goserver/store/draft_store.goserver/store/migrations/000004_add_page_originalid_index.down.sqlserver/store/migrations/000004_add_page_originalid_index.up.sqlserver/store/page_hierarchy.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
Collaborator
Author
|
@coderabbitai The restore-depth finding is already handled — you can withdraw it.
parentDepth, depthErr := s.pageDepth(tx, target.ParentID)
if parentDepth+1 > maxDepth {
restoreParentID = "" // fall back to space root
}Landed in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
sed -n '1250,1320p' server/store/page_store.go
echo "---test---"
rg -n "TestRestorePageFallsBackToRootWhenParentTooDeep" server/store/store_test.go
sed -n '1090,1140p' server/store/store_test.goLength of output: 5623 Confirmed — the code and test verify the fix is in place.
|
Collaborator
Author
|
@coderabbitai - re: #3 (review) Acknowledged — no change needed here. The |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
sed -n '1,20p' go.modLength of output: 1254
|
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
1 participant
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: full space CRUD (create/get/patch/delete), page CRUD scoped to a space, and tree operations ; move (with reorder and circular-reference/depth guards), duplicate (optionally with subtree), and move-to-space (whole-subtree relocation across spaces on the same team).
Core PR:
mattermost/mattermost#37321
Ticket Link
https://mattermost.atlassian.net/browse/MM-69268