page-save-origin-semantics

# Page Save Origin Semantics ## Problem When modifying page save logic, it's easy to accidentally break the carefully designed origin-based conflict detection system. The system uses a two-stage check mechanism (frontend + backend) to determine when revision validation should be enforced vs. bypassed for collaborative editing (Yjs). **Key Insight**: **Conflict detection (revision check)** and **diff detection (hasDiffToPrev)** serve different purposes and require separate logic. ## Solution Understanding the two-stage origin check mechanism: ### Stage 1: Frontend Determines revisionId Requirement ```typescript // apps/app/src/client/components/PageEditor/PageEditor.tsx:158 const isRevisionIdRequiredForPageUpdate = currentPage?.revision?.origin === undefined; // lines 308-310 const revisionId = isRevisionIdRequiredForPageUpdate ? currentRevisionId : undefined; ``` **Logic**: Check the **latest revision's origin** on the page: - If `origin === undefined` (legacy/API save) → Send `revisionId` - If `origin === "editor"` or `"view"` → Do NOT send `revisionId` ### Stage 2: Backend Determines Conflict Check Behavior ```javascript // apps/app/src/server/models/obsolete-page.js:167-172 const ignoreLatestRevision = origin === Origin.Editor && (latestRevisionOrigin === Origin.Editor || latestRevisionOrigin === Origin.View); if (ignoreLatestRevision) { return true; // Bypass revision check } // Otherwise, enforce strict revision matching if (revision != previousRevision) { return false; // Reject save } return true; ``` **Logic**: Check **current request's origin** AND **latest revision's origin**: - If `origin === "editor"` AND latest is `"editor"` or `"view"` → Bypass revision check - Otherwise → Enforce strict revision ID matching ## Origin Values Three types of page update methods (called "origin"): - **`Origin.Editor = "editor"`** - Save from editor mode (collaborative editing via Yjs) - **`Origin.View = "view"`** - Save from view mode - Examples: HandsontableModal, DrawioModal editing - **`undefined`** - API-based saves or legacy pages ## Origin Strength (強弱) **Basic Rule**: Page updates require the previous revision ID in the request. If the latest revision doesn't match, the server rejects the request. **Exception - Editor origin is stronger than View origin**: - **UX Goal**: Avoid `Posted param "revisionId" is outdated` errors when multiple members are using the Editor and View changes interrupt them - **Special Case**: When the latest revision's origin is View, Editor origin requests can update WITHOUT requiring revision ID ### Origin Strength Matrix | | Latest Revision: Editor | Latest Revision: View | Latest Revision: API | | ------ | ----------------------- | --------------------- | -------------------- | | **Request: Editor** | ⭕️ Bypass revision check | ⭕️ Bypass revision check | ❌ Strict check | | **Request: View** | ❌ Strict check | ❌ Strict check | ❌ Strict check | | **Request: API** | ❌ Strict check | ❌ Strict check | ❌ Strict check | **Reading the table**: - ⭕️ = Revision check bypassed (revisionId not required) - ❌ = Strict revision check required (revisionId must match) ## Behavior by Scenario | Latest Revision Origin | Request Origin | revisionId Sent? | Revision Check | Use Case | |------------------------|----------------|------------------|----------------|----------| | `editor` or `view` | `editor` | ❌ No | ✅ Bypassed | Normal Editor use (most common) | | `undefined` | `editor` | ✅ Yes | ✅ Enforced | Legacy page in Editor | | `undefined` | `undefined` (API) | ✅ Yes (required) | ✅ Enforced | API save | ## Example: Server-Side Logic Respecting Origin Semantics When adding server-side functionality that needs previous revision data: ```typescript // ✅ CORRECT: Separate concerns - conflict detection vs. diff detection let previousRevision: IRevisionHasId | null = null; // Priority 1: Use provided revisionId (for conflict detection) if (sanitizeRevisionId != null) { previousRevision = await Revision.findById(sanitizeRevisionId); } // Priority 2: Fallback to currentPage.revision (for other purposes like diff detection) if (previousRevision == null && currentPage.revision != null) { previousRevision = await Revision.findById(currentPage.revision); } const previousBody = previousRevision?.body ?? null; // Continue with existing conflict detection logic (unchanged) if (currentPage != null && !(await currentPage.isUpdatable(sanitizeRevisionId, origin))) { // ... return conflict error } // Use previousBody for diff detection or other purposes updatedPage = await crowi.pageService.updatePage( currentPage, body, previousBody, // ← Available regardless of conflict detection logic req.user, options, ); ``` ```typescript // ❌ WRONG: Forcing frontend to always send revisionId const revisionId = currentRevisionId; // Always send, regardless of origin // This breaks Yjs collaborative editing semantics! ``` ```typescript // ❌ WRONG: Changing backend conflict detection logic // Don't modify isUpdatable() unless you fully understand the implications // for collaborative editing ``` ## When to Apply **Always consider this pattern when**: - Modifying page save/update API handlers - Adding functionality that needs previous revision data - Working on conflict detection or revision validation logic - Implementing features that interact with page history - Debugging save operation issues **Key Principles**: 1. **Do NOT modify frontend revisionId logic** unless explicitly required for conflict detection 2. **Do NOT modify isUpdatable() logic** unless fixing conflict detection bugs 3. **Separate concerns**: Conflict detection ≠ Other revision-based features (diff detection, history, etc.) 4. **Server-side fallback**: If you need previous revision data when revisionId is not provided, fetch from `currentPage.revision` ## Detailed Scenario Analysis ### Scenario A: Normal Editor Mode (Most Common Case) **Latest revision has `origin=editor`**: 1. **Frontend Logic**: - `isRevisionIdRequiredForPageUpdate = false` (latest revision origin is not undefined) - Does NOT send `revisionId` in request - Sends `origin: Origin.Editor` 2. **API Layer**: ```typescript previousRevision = await Revision.findById(undefined); // → null ``` Result: No previousRevision fetched via revisionId 3. **Backend Conflict Check** (`isUpdatable`): ```javascript ignoreLatestRevision = (Origin.Editor === Origin.Editor) && (latestRevisionOrigin === Origin.Editor || latestRevisionOrigin === Origin.View) // → true (latest revision is editor) return true; // Bypass revision check ``` Result: ✅ Save succeeds without revision validation 4. **Impact on Other Features**: - If you need previousRevision data (e.g., for diff detection), it won't be available unless you implement server-side fallback - This is where `currentPage.revision` fallback becomes necessary ### Scenario B: Legacy Page in Editor Mode **Latest revision has `origin=undefined`**: 1. **Frontend Logic**: - `isRevisionIdRequiredForPageUpdate = true` (latest revision origin is undefined) - Sends `revisionId` in request - Sends `origin: Origin.Editor` 2. **API Layer**: ```typescript previousRevision = await Revision.findById(sanitizeRevisionId); // → revision object ``` Result: previousRevision fetched successfully 3. **Backend Conflict Check** (`isUpdatable`): ```javascript ignoreLatestRevision = (Origin.Editor === Origin.Editor) && (latestRevisionOrigin === undefined) // → false (latest revision is undefined, not editor/view) // Strict revision check if (revision != sanitizeRevisionId) { return false; // Reject if mismatch } return true; ``` Result: ✅ Save succeeds only if revisionId matches 4. **Impact on Other Features**: - previousRevision data is available - All revision-based features work correctly ### Scenario C: API-Based Save **Request has `origin=undefined` or omitted**: 1. **Frontend**: Not applicable (API client) 2. **API Layer**: - API client MUST send `revisionId` in request - `previousRevision = await Revision.findById(sanitizeRevisionId)` 3. **Backend Conflict Check** (`isUpdatable`): ```javascript ignoreLatestRevision = (undefined === Origin.Editor) && ... // → false // Strict revision check if (revision != sanitizeRevisionId) { return false; } return true; ``` Result: Strict validation enforced ## Root Cause: Why This Separation Matters **Historical Context**: At some point, the frontend stopped sending `previousRevision` (revisionId) for certain scenarios to support Yjs collaborative editing. This broke features that relied on previousRevision data being available. **The Core Issue**: - **Conflict detection** needs to know "Is this save conflicting with another user's changes?" (Answered by revision check) - **Diff detection** needs to know "Did the content actually change?" (Answered by comparing body) - **Current implementation conflates these**: When conflict detection is bypassed, previousRevision is not fetched, breaking diff detection **The Solution Pattern**: ```typescript // Separate the two concerns: // 1. Fetch previousRevision for data purposes (diff detection, history, etc.) let previousRevision: IRevisionHasId | null = null; if (sanitizeRevisionId != null) { previousRevision = await Revision.findById(sanitizeRevisionId); } else if (currentPage.revision != null) { previousRevision = await Revision.findById(currentPage.revision); // Fallback } // 2. Use previousRevision data for your feature const previousBody = previousRevision?.body ?? null; // 3. Conflict detection happens independently via isUpdatable() if (currentPage != null && !(await currentPage.isUpdatable(sanitizeRevisionId, origin))) { // Return conflict error } ``` ## Reference **Official Documentation**: - https://dev.growi.org/651a6f4a008fee2f99187431#origin-%E3%81%AE%E5%BC%B7%E5%BC%B1 **Related Files**: - Frontend: `apps/app/src/client/components/PageEditor/PageEditor.tsx` (lines 158, 240, 308-310) - Backend: `apps/app/src/server/models/obsolete-page.js` (lines 159-182, isUpdatable method) - API: `apps/app/src/server/routes/apiv3/page/update-page.ts` (lines 260-282, conflict check) - Interface: `packages/core/src/interfaces/revision.ts` (lines 6-11, Origin definition) ## Common Pitfalls 1. **Assuming revisionId is always available**: It's not! Editor mode with recent editor/view saves omits it by design. 2. **Conflating conflict detection with other features**: They serve different purposes and need separate logic. 3. **Breaking Yjs collaborative editing**: Forcing revisionId to always be sent breaks the bypass mechanism. 4. **Ignoring origin values**: The system behavior changes significantly based on origin combinations. ## Lessons Learned This pattern was identified during the "improve-unchanged-revision" feature implementation, where the initial assumption was that frontend should always send `revisionId` for diff detection. Deep analysis revealed: - The frontend logic is correct for conflict detection and should NOT be changed - Server-side fallback is the correct approach to get previous revision data - Two-stage checking is intentional and critical for Yjs collaborative editing - Conflict detection and diff detection must be separated **Key Takeaway**: Always understand the existing architectural patterns before proposing changes. What appears to be a "fix" might actually break carefully designed functionality.