[ynaamane]

A parent rename rebuilt its children's addresses from what it read before a child's save in flight had finished. MoveDescendants computes each new path in a recursive CTE and then updates the rows in the same statement. When a child's save holds that child's row, the UPDATE waits for it, but under READ COMMITTED PostgreSQL then re-checks only the updated row, not the CTE, so the path it writes is built from the child's old slug. Both saves succeed and the child's slug and address disagree.

moveDescendants now first locks every descendant with a new LockDescendants query, then runs MoveDescendants, which starts with a fresh snapshot and reads each child as it stands after its save. This is the order proposed in the issue: the rename waits for its children's saves, then builds their addresses from what they hold. The rows are locked from the top down (depth, then id), so a parent rename waiting on a child holds none of that child's descendants. The lock is taken in FOR NO KEY UPDATE mode, the mode the existing rewrite takes when the paths do not change, so an edit that keeps the address still does not wait for a page being filed under a child; it does conflict with a child's own save, which is the point.

Before, in the new test: the child saved under the slug team while its parent was renamed to manual ended at pages/manual/about. After: pages/manual/team.

Testing: three tests in content_nesting_lock_test.go. The first, run for a child save and for a grandchild save, fails on main in both cases. The second pins the top down order (it fails if the rows are locked by id only), the third the lock mode (it fails under FOR UPDATE). go test -race ./internal/postgres/ passes and the three tests pass 20 runs in a row. Against PostgreSQL 18, go test ./... shows no new failure: the packages that fail here fail on main too (two flaky same-timestamp checks and two tests that depend on the local machine). golangci-lint run (both modules) and go run ./cmd/doclint report nothing.

Not covered: a page filed under a child (created or moved there) during a parent rename can still keep the old address, since filing takes only a key share lock on the child. That window predates this change and concerns a page joining the subtree, not a descendant's own save, so I left it out.

Fixes #510

Summary by CodeRabbit

• Bug Fixes
  • Page renames now coordinate with saves to descendant pages, helping ensure those pages remain filed under the correct paths.
  • Parent edits can proceed without waiting for a page being filed beneath a child, reducing unnecessary delays during concurrent changes.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

• Source mode: Auto
• Repository: gopherium/gophenberg
• Branch: main
• File: .coderabbit.yaml
• Commit: 81fcebb
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a4776e06-d0fb-45ad-914f-16710ca484c8

📥 Commits

Reviewing files that changed from the base of the PR and between 7b0ea4d and dd0ffa8.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: gopherium/gophenberg/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1c9d81af-72f6-46ce-90dc-e16eaa06ad9f

📥 Commits

Reviewing files that changed from the base of the PR and between 58b460f and 7b0ea4d.

📒 Files selected for processing: 4

• internal/postgres/content_nesting_lock_test.go
• internal/postgres/db/content.sql.go
• internal/postgres/pagination.go
• internal/postgres/queries/content.sql

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Parent renames now lock descendant rows before moving descendants. Added concurrency tests check that child and grandchild saves complete before the rename updates their paths, and that editing an ancestor does not wait for a page being filed under its child.

Changes: Descendant locking during parent renames

Layer / File(s)	Summary
Recursive descendant locking internal/postgres/queries/content.sql, internal/postgres/db/content.sql.go	Adds LockDescendants, which recursively finds descendants, tracks cycles, excludes the root, and locks rows in depth-and-ID order.
Rename coordination and concurrency tests internal/postgres/pagination.go, internal/postgres/content_nesting_lock_test.go	moveDescendants locks descendants before moving them. Tests cover concurrent child and grandchild saves, lock ordering, and an ancestor edit while a page is being filed under its child.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant ContentStore
  participant moveDescendants
  participant Queries
  participant PostgreSQL
  ContentStore->>moveDescendants: Update renamed content
  moveDescendants->>Queries: LockDescendants(parent ID)
  Queries->>PostgreSQL: Lock descendant rows
  PostgreSQL-->>Queries: Lock operation result
  Queries-->>moveDescendants: Return result
  moveDescendants->>PostgreSQL: Move descendants after locks succeed

Loading

Suggested reviewers: erfouni, sirlouen

Merge Risk: ⚪ Minimal · up to 7b0ea

Parent renames now wait for in-flight child saves before rebuilding descendant addresses, which fixes the mismatch between a child's slug and its address. No actionable merge-blocking risk remains.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title accurately describes the main behavior change: descendant addresses use a child's slug after its save completes. It is specific and related to the PostgreSQL locking changes.
Linked Issues check		Issue [#510] requires a parent rename to wait for descendant saves and then rebuild descendant addresses from committed values. LockDescendants recursively locks descendants with FOR NO KEY UPDATE…
Out of Scope Changes check		The changed query, moveDescendants call order, and concurrency tests all support the descendant-address consistency requirement in [#510]. The test for lock mode verifies the required behavior witho…

Full details: Linked Issues check

Explanation

Issue [#510] requires a parent rename to wait for descendant saves and then rebuild descendant addresses from committed values. LockDescendants recursively locks descendants with FOR NO KEY UPDATE in depth and ID order before MoveDescendants. MoveDescendants then reads the post-save slugs. The added tests cover child and grandchild saves and verify the resulting paths. The tests also verify top-down locking and preservation of the non-blocking filing behavior.

Full details: Out of Scope Changes check

Explanation

The changed query, moveDescendants call order, and concurrency tests all support the descendant-address consistency requirement in [#510]. The test for lock mode verifies the required behavior without adding unrelated product changes. No unrelated change is shown in the reviewed evidence.

Full details: Docstring Coverage

Explanation

Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/postgres/pagination.go	50.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

The PR locks descendants before rebuilding their paths. This lets the rewrite read a child’s saved slug rather than the value seen before waiting.

• Parent renames rebuild child paths from the slugs they hold after saving.

Diagram

sequenceDiagram
    participant Child as Child save
    participant Parent as Parent rename
    participant DB as PostgreSQL
    Child->>DB: Save child's new slug
    Parent->>DB: Update parent
    Parent->>DB: Lock descendants top-down
    Note over Parent,DB: Wait for child save
    Child->>DB: Commit
    DB-->>Parent: Descendant locks acquired
    Parent->>DB: Rebuild paths with a fresh snapshot
    Parent->>DB: Commit

Loading

_{Reviews (1) · Last reviewed commit: "fix(postgres): build a child's address f..." · Reviewed by Greptile}

[gitar-bot]

Code Review ✅ Approved

🔴 High risk · Locking descendants before rebuilding paths changes stored page-address integrity; coordination issues could leave nested pages with stale or mismatched addresses.

Fixes a parent rename building child addresses from a stale slug by locking all descendants with LockDescendants before running MoveDescendants, so each child's address reflects its slug after its save. Adds three tests in content_nesting_lock_test.go covering child and grandchild saves plus lock order and mode. No issues found.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

[codescene-access]

Gates Failed
Enforce critical code health rules (1 file with Low Cohesion)

Our agent can fix these. Install it.

Gates Passed
2 Quality Gates Passed

Reason for failure

Enforce critical code health rules	Violations	Code Health Impact
content_nesting_lock_test.go	1 critical rule	10.00 → 8.82	Suppress

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.