add page-save-origin-semantics skill and update CLAUDE.md with architectural patterns

.claude/skills/learned/page-save-origin-semantics/SKILL.md

@@ -0,0 +1,279 @@
+---
+name: page-save-origin-semantics
+description: Auto-invoked when working on page save, revision, or update operations. Explains the two-stage origin check mechanism for conflict detection and its separation from diff detection.
+---
+
+# 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
+
+- `Origin.View = "view"` - Save from view mode
+- `Origin.Editor = "editor"` - Save from editor mode (collaborative editing via Yjs)
+- `undefined` - API-based saves or legacy pages
+
+## 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.

apps/app/CLAUDE.md

@@ -1 +1,25 @@
 @AGENTS.md
+
+# apps/app Specific Knowledge
+
+## Critical Architectural Patterns
+
+### Page Save Origin Semantics
+
+**IMPORTANT**: When working on page save, update, or revision operations, always consult the **page-save-origin-semantics** skill (auto-invoked) for understanding the two-stage origin check mechanism.
+
+**Key Concept**: Origin-based conflict detection uses a two-stage check (frontend + backend) to determine when revision validation should be enforced vs. bypassed for Yjs collaborative editing.
+
+**Critical Rule**: **Conflict detection (revision check)** and **other revision-based features (diff detection, history, etc.)** serve different purposes and require separate logic. Do NOT conflate them.
+
+**Quick Reference**:
+- Frontend checks `currentPage?.revision?.origin === undefined` to decide if `revisionId` should be sent
+- Backend checks `(origin === Editor) && (latestOrigin === Editor || View)` to bypass revision validation
+- When `revisionId` is not provided, use server-side fallback: fetch from `currentPage.revision` for non-conflict-detection purposes
+
+**Documentation**:
+- Skill (auto-invoked): `.claude/skills/learned/page-save-origin-semantics/SKILL.md`
+- Official docs: https://dev.growi.org/651a6f4a008fee2f99187431#origin-%E3%81%AE%E5%BC%B7%E5%BC%B1
+
+**Common Pitfall**: Assuming `revisionId` is always available or forcing frontend to always send it will break Yjs collaborative editing.
+