diff --git a/.claude/commands/learn.md b/.claude/commands/learn.md index be695d87e6b..183aca1cc3f 100644 --- a/.claude/commands/learn.md +++ b/.claude/commands/learn.md @@ -25,7 +25,18 @@ Focus on four key areas: ## Skill File Structure -Extracted patterns are saved in `.claude/skills/learned/{topic-name}/SKILL.md` with: +Extracted patterns are saved in **appropriate skill directories** based on the scope of the pattern: + +**Workspace-specific patterns**: +- `apps/{workspace}/.claude/skills/learned/{topic-name}/SKILL.md` +- `packages/{package}/.claude/skills/learned/{topic-name}/SKILL.md` +- Examples: patterns specific to a single app or package + +**Global patterns** (monorepo-wide): +- `.claude/skills/learned/{topic-name}/SKILL.md` +- Examples: patterns applicable across all workspaces + +### File Template ```yaml --- @@ -49,12 +60,19 @@ description: Brief description (auto-invoked when working on related code) ## GROWI-Specific Examples Topics commonly learned in GROWI development: -- `virtualized-tree-patterns` — @headless-tree + @tanstack/react-virtual optimizations + +**Apps/app-specific** (`apps/app/.claude/skills/learned/`): +- `page-save-origin-semantics` — Origin-based conflict detection for collaborative editing - `socket-jotai-integration` — Real-time state synchronization patterns - `api-v3-error-handling` — RESTful API error response patterns -- `jotai-atom-composition` — Derived atoms and state composition - `mongodb-query-optimization` — Mongoose indexing and aggregation patterns +**Global monorepo patterns** (`.claude/skills/learned/`): +- `virtualized-tree-patterns` — @headless-tree + @tanstack/react-virtual optimizations (if used across apps) +- `jotai-atom-composition` — Derived atoms and state composition (if shared pattern) +- `turborepo-cache-invalidation` — Build cache debugging techniques +- `pnpm-workspace-dependencies` — Workspace dependency resolution issues + ## Quality Guidelines **Extract:** @@ -74,7 +92,25 @@ Topics commonly learned in GROWI development: 1. User triggers `/learn` after solving a complex problem 2. Review the session to identify valuable patterns 3. Draft skill file(s) with clear structure -4. Save to `.claude/skills/learned/{topic-name}/SKILL.md` +4. **Autonomously determine the appropriate directory**: + - Analyze the pattern's scope (which files/modules were involved) + - If pattern is specific to a workspace in `apps/*` or `packages/*`: + - Save to `{workspace}/.claude/skills/learned/{topic-name}/SKILL.md` + - If pattern is applicable across multiple workspaces: + - Save to `.claude/skills/learned/{topic-name}/SKILL.md` 5. Skills automatically apply in future sessions when working on related code +### Directory Selection Logic + +**Workspace-specific** (save to `{workspace}/.claude/skills/learned/`): +- Pattern involves workspace-specific concepts, models, or APIs +- References files primarily in one `apps/*` or `packages/*` directory +- Example: Page save logic in `apps/app` → `apps/app/.claude/skills/learned/` + +**Global** (save to `.claude/skills/learned/`): +- Pattern applies across multiple workspaces +- Involves monorepo-wide tools (Turborepo, pnpm, Biome, Vitest) +- Shared coding patterns or architectural principles +- Example: Turborepo caching pitfall → `.claude/skills/learned/` + Learned skills are automatically invoked based on their description when working on related code. diff --git a/.claude/skills/learned/.gitkeep b/.claude/skills/learned/.gitkeep deleted file mode 100644 index 11c1f0b4c5a..00000000000 --- a/.claude/skills/learned/.gitkeep +++ /dev/null @@ -1,27 +0,0 @@ -# Learned Skills Directory - -This directory contains Skills learned from development sessions using the `/learn` command. - -Each learned skill is automatically applied when working on related code based on its description. - -## Structure - -``` -learned/ -├── {topic-name-1}/ -│ └── SKILL.md -├── {topic-name-2}/ -│ └── SKILL.md -└── {topic-name-3}/ - └── SKILL.md -``` - -## How Skills Are Created - -Use the `/learn` command after completing a feature or solving a complex problem: - -``` -/learn -``` - -Claude will extract reusable patterns and save them as Skills in this directory. diff --git a/apps/app/.claude/skills/learned/page-save-origin-semantics/SKILL.md b/apps/app/.claude/skills/learned/page-save-origin-semantics/SKILL.md new file mode 100644 index 00000000000..c560553cda2 --- /dev/null +++ b/apps/app/.claude/skills/learned/page-save-origin-semantics/SKILL.md @@ -0,0 +1,302 @@ +--- +name: page-save-origin-semantics +description: Auto-invoked when modifying origin-based conflict detection, revision validation logic, or isUpdatable() method. 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 + +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. diff --git a/apps/app/CLAUDE.md b/apps/app/CLAUDE.md index 43c994c2d36..f98b92ea413 100644 --- a/apps/app/CLAUDE.md +++ b/apps/app/CLAUDE.md @@ -1 +1,19 @@ @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 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. + +**Documentation**: +- Skill (auto-invoked): `.claude/skills/learned/page-save-origin-semantics/SKILL.md` + +**Common Pitfall**: Assuming `revisionId` is always available or forcing frontend to always send it will break Yjs collaborative editing. + diff --git a/apps/app/src/client/components/PageHistory/PageRevisionTable.tsx b/apps/app/src/client/components/PageHistory/PageRevisionTable.tsx index 8caa9b79953..871766da454 100644 --- a/apps/app/src/client/components/PageHistory/PageRevisionTable.tsx +++ b/apps/app/src/client/components/PageHistory/PageRevisionTable.tsx @@ -44,8 +44,8 @@ export const PageRevisionTable = ( const { data, size, error, setSize, isValidating } = swrInifiniteResponse; - const revisions = data && data[0].revisions; - const oldestRevision = revisions && revisions[revisions.length - 1]; + const revisions = data?.[0].revisions; + const oldestRevision = revisions?.[revisions.length - 1]; // First load const isLoadingInitialData = !data && !error; @@ -166,34 +166,30 @@ export const PageRevisionTable = ( - {(hasDiff || revisionId === sourceRevision?._id) && ( -
- setSourceRevision(revision)} - /> -
- )} +
+ setSourceRevision(revision)} + /> +
- {(hasDiff || revisionId === targetRevision?._id) && ( -
- setTargetRevision(revision)} - /> -
- )} +
+ setTargetRevision(revision)} + /> +
); diff --git a/apps/app/src/client/components/PageHistory/Revision.tsx b/apps/app/src/client/components/PageHistory/Revision.tsx index aa2b9d58a2d..8d9a64d1f05 100644 --- a/apps/app/src/client/components/PageHistory/Revision.tsx +++ b/apps/app/src/client/components/PageHistory/Revision.tsx @@ -45,13 +45,18 @@ export const Revision = (props: RevisionProps): JSX.Element => { return (
-
{pic}
-
- - {t('No diff')} - +
+
{pic}
+
+ + + +
+
+
+ {t('No diff')}
); @@ -75,7 +80,7 @@ export const Revision = (props: RevisionProps): JSX.Element => {
- + {isLatestRevision && ( {t('Latest')} diff --git a/apps/app/src/server/routes/apiv3/page/update-page.ts b/apps/app/src/server/routes/apiv3/page/update-page.ts index b4de092d96a..1c62408f9fd 100644 --- a/apps/app/src/server/routes/apiv3/page/update-page.ts +++ b/apps/app/src/server/routes/apiv3/page/update-page.ts @@ -161,11 +161,11 @@ export const updatePageHandlersFactory = (crowi: Crowi): RequestHandler[] => { 'update', option, ); - results.forEach((result) => { + for (const result of results) { if (result.status === 'rejected') { logger.error('Create user notification failed', result.reason); } - }); + } } catch (err) { logger.error('Create user notification failed', err); } @@ -298,7 +298,36 @@ export const updatePageHandlersFactory = (crowi: Crowi): RequestHandler[] => { options.grant = grant; options.userRelatedGrantUserGroupIds = userRelatedGrantUserGroupIds; } - previousRevision = await Revision.findById(sanitizeRevisionId); + + // Priority 1: Use provided revisionId (for conflict detection) + previousRevision = null; + if (sanitizeRevisionId != null) { + try { + previousRevision = await Revision.findById(sanitizeRevisionId); + } catch (error) { + logger.error('Failed to fetch previousRevision by revisionId', { + revisionId: sanitizeRevisionId, + pageId: currentPage._id, + error, + }); + } + } + + // Priority 2: Fallback to currentPage.revision (for diff detection) + if (previousRevision == null && currentPage.revision != null) { + try { + previousRevision = await Revision.findById(currentPage.revision); + } catch (error) { + logger.error( + 'Failed to fetch previousRevision by currentPage.revision', + { + pageId: currentPage._id, + revisionId: currentPage.revision, + error, + }, + ); + } + } // There are cases where "revisionId" is not required for revision updates // See: https://dev.growi.org/651a6f4a008fee2f99187431#origin-%E3%81%AE%E5%BC%B7%E5%BC%B1