Repository navigation
fix: rollback of rollback should restore trashed pages - #147
Conversation
When undoing a rollback of a create operation, the system was incorrectly trying to trash the page again instead of restoring it. The bug was that `shouldBeTrashed = action === 'rollback'` didn't account for `rollingBackRollback`. When rolling back a rollback: - action is still 'rollback' - but rollingBackRollback is true - so we should RESTORE (shouldBeTrashed = false), not trash Fixed for both pages and drives. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThread undo-group activity IDs through preview and execute rollback flows; reorganize conflict detection and no-op handling (including AI-undo cases); standardize page-create rollback public shape to Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant RTP as rollback-to-point-service
participant RS as rollback-service
participant AI as ai-undo-service
participant DB as DataStore
participant SocketClient as web/socket-utils
participant RT as realtime server
UI->>RTP: request previewRollbackToPoint
RTP->>RS: previewRollback(activity, { undoGroupActivityIds? })
RS-->>RTP: preview (activitiesAffected, isNoOp, conflicts...)
RTP->>AI: filter AI-generated no-ops (skip isNoOp)
RTP->>RTP: derive/preserve undoGroupActivityIds
RTP->>RS: executeRollback(activity, { undoGroupActivityIds })
alt create op OR internal undo-group match
RS-->>RTP: treat as no-op / skip conflict gating
else potential conflict
RS->>DB: check external modifications (ignore undoGroupActivityIds)
DB-->>RS: externalMods? (yes/no)
alt externalMods == yes
RS-->>RTP: report blocking external conflicts
else
RS-->>RTP: clear internal-only conflicts -> proceed
end
end
RS->>DB: apply change (e.g., trash page -> restoredValues { isTrashed: true })
DB-->>RS: success
RS->>SocketClient: broadcastActivityEvent(payload) (debounced)
SocketClient->>RT: POST /api/broadcast -> emits activity:logged
RT-->>SocketClient: activity:logged
SocketClient->>UI: onActivityLogged (debounced) -> UI reloads activities
RS-->>RTP: execution result
RTP-->>UI: final result / summaries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
The restoredValues was using `trashed` and `pageId` fields, but currentValues uses `isTrashed`. This field name mismatch caused false conflict detection when undoing a rollback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/services/api/rollback-service.ts (1)
1871-1871: Critical: Inconsistent field name - same issue as pages but not fixed for drives.Line 1871 returns
{ trashed: true, ... }but drivecurrentValuesat lines 813-818 useisTrashed. This is the same field name mismatch that was fixed for pages at line 1765.This inconsistency will cause false conflict detection when
getConflictFieldscomparesrestoredValuesagainstcurrentValues, as the key names don't match.🔎 Proposed fix to align with page pattern
- return { trashed: true, driveId: activity.driveId, pagesTrashed: true }; + return { isTrashed: true };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/services/api/rollback-service.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/services/api/rollback-service.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/services/api/rollback-service.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/services/api/rollback-service.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/services/api/rollback-service.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (3)
apps/web/src/services/api/rollback-service.ts (3)
741-742: LGTM! Core bug fix correctly implemented.The updated logic properly handles rollback-of-rollback scenarios for page creation. When
rollingBackRollbackis true, the page will be restored rather than trashed again.
822-823: LGTM! Drive logic matches page fix.Consistent application of the rollback-of-rollback logic for drives.
1765-1765: LGTM! Field name aligned with database schema.Correctly changed from
trashedtoisTrashedto match the database field name and prevent false conflict detection when comparing restoredValues with currentValues.
When undoing an AI conversation, skip conflict detection for pages since all changes are from the same conversation - there can't be external conflicts by definition. This is simpler and more robust than the previous approach of querying for external modifications. - 'create' operations: skip conflict check (just trashing) - AI undo (undoGroupActivityIds provided): skip conflict check - Regular rollback: normal conflict detection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/services/api/rollback-service.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/services/api/rollback-service.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/services/api/rollback-service.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/services/api/rollback-service.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/services/api/rollback-service.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Tests
- GitHub Check: Lint & TypeScript Check
🔇 Additional comments (3)
apps/web/src/services/api/rollback-service.ts (3)
741-742: LGTM! Correct fix for rollback-of-rollback behavior.The logic now correctly handles the rollback-of-rollback scenario for page creation:
- Regular rollback of create:
shouldBeTrashed = true(trash the page)- Rollback of rollback of create:
shouldBeTrashed = false(restore the page)This directly addresses the bug described in the PR objectives.
842-843: LGTM! Consistent fix applied for drives.The same rollback-of-rollback fix is correctly applied for drive creation, ensuring consistent behavior between pages and drives.
1785-1785: LGTM! Correct field name for pages schema.Changed from
trashed: truetoisTrashed: trueto match the pages table schema. This ensures consistency with the database field naming.
Apply the same fix to all 7 resource types (page, drive, member, permission, role, agent, message) - skip conflict detection entirely when undoGroupActivityIds is provided since all changes are from the same AI conversation by definition. This removes ~130 lines of complex external-modifications queries that weren't working correctly, replacing with simple skip logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/services/api/rollback-service.ts (1)
752-798: Critical logic error: unreachable external modifications check for page conflicts.The outer condition at line 755 requires
undoGroupActivityIds.length === 0, but the inner condition at line 769 requiresundoGroupActivityIds.length > 0. These conditions are mutually exclusive, making the external modifications check (lines 770-796) unreachable.Impact: The AI undo feature cannot distinguish internal vs external conflicts for pages, contradicting the PR's stated intent.
Fix: Remove
undoGroupActivityIds.length === 0from line 755, keeping only theeffectiveOperation !== 'create'check. This mirrors the comment from the previous review.Based on past review comments indicating this was already identified.
🧹 Nitpick comments (1)
apps/web/src/services/api/rollback-service.ts (1)
853-856: Consider adding external modifications check for consistency.The drive conflict detection uses a simpler pattern (skip entirely when
undoGroupActivityIdsprovided) compared to the page logic's intent to distinguish internal vs external conflicts. While the page logic has a critical bug making its check unreachable, the intended design suggests drives (and other resource types) might also benefit from the external modifications check.Recommendation: Once the page conflict detection bug is fixed, consider whether drives, members, permissions, roles, agents, and messages should also implement the internal vs external conflict distinction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/services/api/rollback-service.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/services/api/rollback-service.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/services/api/rollback-service.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/services/api/rollback-service.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/services/api/rollback-service.ts
🧬 Code graph analysis (1)
apps/web/src/services/api/rollback-service.ts (3)
packages/lib/src/logging/logger-config.ts (1)
loggers(8-18)packages/db/src/index.ts (6)
db(20-20)and(8-8)eq(8-8)gt(9-9)not(8-8)inArray(8-8)packages/db/src/schema/monitoring.ts (1)
activityLogs(430-502)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Tests
- GitHub Check: Lint & TypeScript Check
🔇 Additional comments (4)
apps/web/src/services/api/rollback-service.ts (4)
739-750: LGTM! Rollback-of-rollback logic correctly restores pages.The logic properly handles the case where rolling back a rollback of a create operation should restore (not trash) the page. The condition
shouldBeTrashed = action === 'rollback' && !rollingBackRollbackensures that:
- Regular rollback of create: trashes the page
- Rollback of rollback (redo): restores the page
840-851: LGTM! Drive rollback-of-rollback logic is consistent with pages.The logic correctly implements the same behavior as pages, ensuring that rolling back a rollback of a drive create operation will restore (not trash) the drive.
955-958: Conflict detection pattern is consistent across resource types.The conflict detection for members, permissions, roles, agents, and messages consistently skips checks when
undoGroupActivityIdsis provided. This implements the AI undo behavior where changes from the same conversation are not treated as conflicts.Also applies to: 1060-1063, 1103-1106, 1189-1192, 1245-1248, 1292-1295
1676-1676: Bug fix: correct property name aligns with database schema.The change from
trashed: truetoisTrashed: truecorrectly uses the actual database schema field name, ensuring consistency with the rest of the codebase.
When undoing an AI conversation, individual operations may appear as no-ops (e.g., "Already at this version") but the resource will be affected by other operations in the same undo group (e.g., trashed by a create rollback). Skip no-op detection when undoGroupActivityIds is provided to avoid misleading "Cannot undo" warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pass undoGroupActivityIds to previewRollback and executeRollback in rollback-to-point service, matching the AI undo behavior. This skips conflict detection and no-op warnings for activities within the same rollback group. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/services/api/rollback-service.ts (1)
752-798: Critical: Unreachable external modifications check for pages (duplicate issue).The outer condition at line 755 requires
undoGroupActivityIds.length === 0, but the inner condition at line 769 requiresundoGroupActivityIds.length > 0. These are mutually exclusive, making the external modifications check (lines 770-796) unreachable.Impact: AI undo's ability to distinguish internal vs external conflicts for pages will never work. The drive conflict detection logic (lines 853-856) correctly handles this by not having the outer guard.
🔎 Proposed fix (from previous review)
- // For 'create' operations, skip conflict check - we're just trashing the page - // For AI undo (undoGroupActivityIds provided), skip conflict check since all - // changes are from the same conversation by definition - no "external" conflicts - if (effectiveOperation !== 'create' && undoGroupActivityIds.length === 0) { + // For 'create' operations, skip conflict check - we're just trashing the page + if (effectiveOperation !== 'create') { conflictFields = getConflictFields(activity.newValues, currentValues); // If there's a conflict but we have undo group context, check if the modifications // came from other activities in the same undo group (internal conflict vs external) if (conflictFields.length > 0) { loggers.api.debug('[Rollback:Preview] Page conflict detected', { activityId: activity.id, resourceId: activity.resourceId, timestamp: activity.timestamp?.toISOString(), conflictFields, undoGroupSize: undoGroupActivityIds.length, }); if (undoGroupActivityIds.length > 0) { const externalModifications = await db .select({ id: activityLogs.id }) .from(activityLogs) .where( and( eq(activityLogs.resourceId, activity.resourceId), eq(activityLogs.resourceType, 'page'), gt(activityLogs.timestamp, activity.timestamp), not(inArray(activityLogs.id, undoGroupActivityIds)) ) ) .limit(1); loggers.api.debug('[Rollback:Preview] External modifications check', { activityId: activity.id, externalModificationsFound: externalModifications.length, }); if (externalModifications.length === 0) { // All modifications came from activities in the undo group - not a real conflict loggers.api.debug('[Rollback:Preview] Page conflict is internal to undo group, ignoring', { activityId: activity.id, conflictFields, }); conflictFields = []; } } } }
🧹 Nitpick comments (1)
apps/web/src/services/api/rollback-service.ts (1)
853-856: Consider consistency: Drive conflict detection differs from pages.Drives (and other resources) use a simpler pattern: skip all conflict detection when
undoGroupActivityIds.length > 0. Pages attempt to distinguish internal vs external conflicts with an external modifications check, but the implementation is broken (see lines 752-798).Consider whether pages should follow this simpler, working pattern, or if the external modifications check should be properly fixed and potentially extended to other resources.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/services/api/rollback-service.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/services/api/rollback-service.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/services/api/rollback-service.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/services/api/rollback-service.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/services/api/rollback-service.ts
🧬 Code graph analysis (1)
apps/web/src/services/api/rollback-service.ts (1)
packages/db/src/schema/monitoring.ts (1)
activityLogs(430-502)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (5)
apps/web/src/services/api/rollback-service.ts (5)
741-742: LGTM: Correct fix for rollback-of-rollback restoring pages.The
shouldBeTrashedcomputation now correctly accounts forrollingBackRollback, ensuring that rolling back a rollback of a create operation restores the page instead of trashing it again.
842-843: LGTM: Consistent fix for drive rollback-of-rollback.The
shouldBeTrashedlogic matches the page fix, correctly restoring drives when rolling back a rollback of create.
955-958: LGTM: Consistent AI undo conflict detection across resources.All non-page resources (member, permission, role, agent, message) correctly skip conflict detection when
undoGroupActivityIds.length > 0, using a simple, consistent pattern.Also applies to: 1060-1063, 1103-1106, 1189-1192, 1245-1248, 1292-1295
1678-1678: LGTM: Correct field name for page trash state.Using
isTrashedinstead oftrashedaligns with the pages schema field name.
1318-1320: LGTM: No-op detection correctly skipped for AI undo operations.The logic correctly skips no-op detection when
undoGroupActivityIdsis provided, preventing false "Cannot undo" warnings for operations that may appear as no-ops individually but are part of a larger undo group.
Replace truncated warning lists that showed "...and X more" with scrollable lists using ScrollArea. Users need to see exactly what they're rolling back - truncation is unacceptable for transparency. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Apply the same fix to UndoAiChangesDialog - replace truncated lists with scrollable ScrollArea components for warnings, conflicts, and activities. Users need full transparency on what they're undoing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Skip create no-op check for AI undo operations - old activities from previous sessions may reference already-trashed pages that should be silently ignored (same aiConversationId spans all tool calls in thread) - Replace ScrollArea with native overflow-y-auto for warning lists - ScrollArea with max-h classes causes overflow instead of scrolling - Remove unused ScrollArea import from UndoAiChangesDialog 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add guard in execute path to skip rollback of create operations when the page is already trashed. This prevents: - Wasted database updates - Unnecessary revision increments on the page and its children - Confusing activity log entries for no-op operations The check happens early in executeRollbackPage before any mutations, making the operation truly idempotent for old activities from previous sessions that share the same aiConversationId. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/services/api/rollback-service.ts (1)
739-752: Fix inverted logic: condition and comment contradict.Line 739's comment says "Skip create no-op check for AI undo", but line 741 checks
undoGroupActivityIds.length === 0, which means the check runs only when NOT in AI undo mode (empty array = no undo group). Either:
- The comment should say "Skip create no-op check when NOT in AI undo", or
- The condition should be
undoGroupActivityIds.length > 0Line 744's
shouldBeTrashedlogic correctly fixes the rollback-of-rollback bug.🔎 Proposed fix for comment clarity
- // Skip create no-op check for AI undo - old activities from previous sessions - // may reference already-trashed pages that should be silently ignored - if (effectiveOperation === 'create' && undoGroupActivityIds.length === 0) { + // Only check for create no-ops when NOT in AI undo mode - during AI undo, + // old activities from previous sessions may reference already-trashed pages that should be silently ignored + if (effectiveOperation === 'create' && undoGroupActivityIds.length === 0) {
♻️ Duplicate comments (1)
apps/web/src/services/api/rollback-service.ts (1)
754-800: Critical logic error: unreachable code in AI undo conflict detection.The outer condition at line 757 checks
undoGroupActivityIds.length === 0, but the inner condition at line 771 checksundoGroupActivityIds.length > 0. These are mutually exclusive, making the external modifications check (lines 772-788) unreachable.Impact: The AI undo feature's ability to distinguish internal vs external conflicts for pages will never work, contradicting the PR's stated intent.
Inconsistency: The drive conflict detection logic (lines 855-858) correctly handles this by not having the outer
undoGroupActivityIds.length === 0guard.🔎 Proposed fix
- // For 'create' operations, skip conflict check - we're just trashing the page - // For AI undo (undoGroupActivityIds provided), skip conflict check since all - // changes are from the same conversation by definition - no "external" conflicts - if (effectiveOperation !== 'create' && undoGroupActivityIds.length === 0) { + // For 'create' operations, skip conflict check - we're just trashing the page + if (effectiveOperation !== 'create') { conflictFields = getConflictFields(activity.newValues, currentValues); // If there's a conflict but we have undo group context, check if the modifications // came from other activities in the same undo group (internal conflict vs external) if (conflictFields.length > 0) { loggers.api.debug('[Rollback:Preview] Page conflict detected', { activityId: activity.id, resourceId: activity.resourceId, timestamp: activity.timestamp?.toISOString(), conflictFields, undoGroupSize: undoGroupActivityIds.length, }); if (undoGroupActivityIds.length > 0) { const externalModifications = await db .select({ id: activityLogs.id }) .from(activityLogs) .where( and( eq(activityLogs.resourceId, activity.resourceId), eq(activityLogs.resourceType, 'page'), gt(activityLogs.timestamp, activity.timestamp), not(inArray(activityLogs.id, undoGroupActivityIds)) ) ) .limit(1); loggers.api.debug('[Rollback:Preview] External modifications check', { activityId: activity.id, externalModificationsFound: externalModifications.length, }); if (externalModifications.length === 0) { // All modifications came from activities in the undo group - not a real conflict loggers.api.debug('[Rollback:Preview] Page conflict is internal to undo group, ignoring', { activityId: activity.id, conflictFields, }); conflictFields = []; } } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/components/activity/RollbackToPointDialog.tsxapps/web/src/components/ai/shared/chat/UndoAiChangesDialog.tsxapps/web/src/services/api/rollback-service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/activity/RollbackToPointDialog.tsx
- apps/web/src/components/ai/shared/chat/UndoAiChangesDialog.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/services/api/rollback-service.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/services/api/rollback-service.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/services/api/rollback-service.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/services/api/rollback-service.ts
🧬 Code graph analysis (1)
apps/web/src/services/api/rollback-service.ts (2)
packages/db/src/schema/monitoring.ts (1)
activityLogs(430-502)packages/db/src/schema/core.ts (1)
pages(24-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Tests
- GitHub Check: Lint & TypeScript Check
🔇 Additional comments (6)
apps/web/src/services/api/rollback-service.ts (6)
290-290: LGTM - Optional pageMutationMeta aligns with idempotent behavior.The change to make
pageMutationMetaoptional correctly reflects that some rollback operations (like skipping already-trashed pages at line 1660) don't perform mutations and return undefined.
842-858: LGTM - Drive conflict detection implements correct pattern.Lines 855-858 correctly skip conflict detection for AI undo without the nested structure problem present in page conflict detection (lines 754-800). The
shouldBeTrashedlogic at line 845 correctly implements the rollback-of-rollback fix.This is the correct pattern that should be applied to page conflict detection as well (see previous comment).
957-960: LGTM - Consistent AI undo conflict detection across resource types.All non-page resource types (member, permission, role, agent, message) consistently use the correct pattern:
if (undoGroupActivityIds.length === 0) { conflictFields = getConflictFields(...) }. This matches the drive pattern and correctly skips conflict detection during AI undo.Only the page conflict detection (lines 754-800) deviates with the incorrect nested structure.
Also applies to: 1062-1065, 1105-1108, 1191-1194, 1247-1250, 1294-1297
1320-1322: LGTM - No-op detection correctly skipped for AI undo.Line 1322 correctly bypasses no-op detection when
undoGroupActivityIds.length > 0, as operations that appear isolated no-ops may be part of a larger undo group. This aligns with the AI undo handling pattern throughout the file.
1648-1688: LGTM - Idempotent rollback prevents redundant updates.Lines 1655-1661 correctly implement idempotency by checking if the page is already trashed before mutation. The early return with
pageMutationMeta: undefinedaligns with the interface change at line 290. Both paths correctly useisTrashed(nottrashed), consistent with the database schema.This prevents wasted DB updates, unnecessary revision increments, and noisy activity logs for AI undo of old activities.
744-744: Core fix correctly implements rollback-of-rollback restoration.The
shouldBeTrashedlogic at lines 744 and 845 correctly fixes the PR's stated bug:
- Regular rollback of create:
action='rollback' && !rollingBackRollback→ trash page/drive- Rollback of rollback (redo):
action='rollback' && rollingBackRollback=true→ restore page/driveThis ensures that undoing a rollback restores the resource instead of trashing it again.
Also applies to: 845-845
AI undo was showing 35 activities when only 9 actually happened in the conversation. The aiConversationId is the page's chat thread ID which persists across sessions, so old activities from previous sessions were being included. Changes: - rollback-service.ts: Restore no-op detection for create operations, but for AI undo return canExecute:true with isNoOp:true (silent skip) instead of an error - ai-undo-service.ts: Filter out activities where preview.isNoOp is true so only real changes are shown in the preview count This ensures the preview accurately reflects only the changes from the current AI conversation, not historical activities from past sessions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Apply the same no-op filtering to rollback-to-point-service that was added to ai-undo-service. This ensures consistent behavior across all rollback preview dialogs - only real changes are shown, not stale activities for already-trashed pages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The earlier fix bypassed conflict detection entirely for AI undo, which also bypassed the detection of external changes (edits by other users or manual edits after the AI change). This fix restores proper conflict detection for all resource types: - page, drive, member, permission, role, agent, message For each type: 1. Always run conflict detection (getConflictFields) 2. If conflicts found AND we have undo group context: - Query for modifications after this activity - Exclude modifications from activities in the undo group - If any EXTERNAL modifications exist, flag as conflict - If only INTERNAL modifications, ignore (not a real conflict) 3. External conflicts still require force=true to proceed This ensures external changes are protected while internal changes from the same AI conversation are still handled smoothly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixes issue where AI undo fails with "Message not found" immediately
after streaming completes, requiring a page refresh.
Root cause: The Vercel AI SDK generates assistant message IDs on the
client independently from the server. When the user clicks undo, the
client's ID doesn't match the server-saved ID in the database.
Solution: Generate the message ID on the server and send it to the
client via the stream protocol. Both client and server now use the
same ID.
- Page AI chat: Use writer.write({ type: 'start', messageId })
- Global Assistant: Use generateMessageId option in toUIMessageStreamResponse
References:
- https://ai-sdk.dev/docs/ai-sdk-ui/chatbot-message-persistence
- https://github.com/vercel/ai/discussions/3512
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add broadcastActivityEvent with 500ms debouncing to socket-utils - Add activity room handlers (join/leave) to realtime service - Create useActivitySocket hook for frontend subscription - Update SidebarActivityTab to auto-refresh on activity:logged events - Add setActivityBroadcastHook to activity-logger for dependency injection - Create instrumentation.ts to initialize broadcast hook at server startup - Fix collectConsecutiveRollbacks to respect changeGroupId Activity updates are now collaborative - all users viewing the same drive/page see each other's activities in real-time without refresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/instrumentation.ts (1)
8-18: Consider adding error handling for initialization failures.The dynamic imports and hook setup could fail silently. While this is fire-and-forget code, logging initialization failures would help with debugging production issues.
🔎 Proposed improvement
export async function register() { // Only run on the server (Node.js runtime) if (process.env.NEXT_RUNTIME === 'nodejs') { - // Initialize activity broadcast hook for real-time updates - const { setActivityBroadcastHook } = await import('@pagespace/lib'); - const { broadcastActivityEvent } = await import('@/lib/websocket/socket-utils'); - - setActivityBroadcastHook(broadcastActivityEvent); - - console.log('[Instrumentation] Activity broadcast hook initialized'); + try { + // Initialize activity broadcast hook for real-time updates + const { setActivityBroadcastHook } = await import('@pagespace/lib'); + const { broadcastActivityEvent } = await import('@/lib/websocket/socket-utils'); + + setActivityBroadcastHook(broadcastActivityEvent); + + console.log('[Instrumentation] Activity broadcast hook initialized'); + } catch (error) { + console.error('[Instrumentation] Failed to initialize activity broadcast hook:', error); + } } }apps/web/src/lib/websocket/socket-utils.ts (1)
19-30: Consider importing or re-exporting the payload type from@pagespace/lib.
ActivityEventPayloadmirrors the type defined inpackages/lib/src/monitoring/activity-logger.ts. While duplication is acceptable across package boundaries, consider re-exporting from a shared location to ensure they stay synchronized.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/realtime/src/index.tsapps/web/src/components/activity/utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsxapps/web/src/hooks/useActivitySocket.tsapps/web/src/instrumentation.tsapps/web/src/lib/websocket/socket-utils.tspackages/lib/src/monitoring/activity-logger.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/lib/websocket/socket-utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/activity/utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsxapps/realtime/src/index.tsapps/web/src/hooks/useActivitySocket.tspackages/lib/src/monitoring/activity-logger.tsapps/web/src/instrumentation.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/lib/websocket/socket-utils.tsapps/web/src/components/activity/utils.tsapps/realtime/src/index.tsapps/web/src/hooks/useActivitySocket.tspackages/lib/src/monitoring/activity-logger.tsapps/web/src/instrumentation.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/lib/websocket/socket-utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/activity/utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsxapps/realtime/src/index.tsapps/web/src/hooks/useActivitySocket.tspackages/lib/src/monitoring/activity-logger.tsapps/web/src/instrumentation.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/lib/websocket/socket-utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/activity/utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsxapps/web/src/hooks/useActivitySocket.tsapps/web/src/instrumentation.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/*.tsx: React component files should use PascalCase (e.g.,UserProfile.tsx)
Use @dnd-kit for drag-and-drop functionality
Use Zustand for client state management
Use SWR for server state management and caching
Files:
apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Tailwind CSS and shadcn/ui components for styling and UI
Files:
apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsx
apps/web/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/components/**/*.{ts,tsx}: When document editing, register editing state withuseEditingStore.getState().startEditing()to prevent UI refreshes, and clean up in return statement
When AI is streaming, register streaming state withuseEditingStore.getState().startStreaming()to prevent UI refreshes, and clean up in return statement
For SWR data fetching with editing protection, useisPaused: () => hasLoadedRef.current && isEditingActive()to allow initial fetch and only pause after, withrevalidateOnFocus: false
Use Zustand for client-side state management as the primary state solution
Use SWR for server state and caching with proper configuration includingrevalidateOnFocus: falsefor editing protection
Use TipTap rich text editor with markdown support for document editing
Use Monaco Editor for code editing features
Use @dnd-kit for drag-and-drop functionality instead of other libraries
Use Tailwind CSS with shadcn/ui components for all UI styling and components
Files:
apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/activity/utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsx
apps/realtime/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Socket.IO for real-time collaboration features
Files:
apps/realtime/src/index.ts
🧠 Learnings (4)
📚 Learning: 2025-12-22T20:04:40.910Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to apps/realtime/**/*.{ts,tsx} : Use Socket.IO for real-time collaboration features
Applied to files:
apps/web/src/lib/websocket/socket-utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/realtime/src/index.tsapps/web/src/hooks/useActivitySocket.ts
📚 Learning: 2025-12-23T18:49:41.966Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Applied to files:
apps/web/src/lib/websocket/socket-utils.tsapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/realtime/src/index.tsapps/web/src/hooks/useActivitySocket.ts
📚 Learning: 2025-12-23T18:49:41.966Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/components/**/*.{ts,tsx} : When AI is streaming, register streaming state with `useEditingStore.getState().startStreaming()` to prevent UI refreshes, and clean up in return statement
Applied to files:
apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/hooks/useActivitySocket.ts
📚 Learning: 2025-12-18T05:22:42.263Z
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 96
File: apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx:641-657
Timestamp: 2025-12-18T05:22:42.263Z
Learning: In apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx, the provider/model selector buttons are intentionally non-functional placeholders in the compact sidebar view. The `hideModelSelector={true}` prop is passed to ChatInput to hide the full ProviderModelSelector. Users are expected to use the full GlobalAssistantView for model selection. A settings link may be added in a future iteration.
Applied to files:
apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsx
🧬 Code graph analysis (7)
apps/web/src/lib/websocket/socket-utils.ts (2)
packages/lib/src/monitoring/activity-logger.ts (1)
ActivityOperation(47-89)apps/processor/src/logger.ts (1)
error(57-63)
apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsx (1)
apps/web/src/hooks/useActivitySocket.ts (2)
ActivityContext(4-4)useActivitySocket(16-95)
apps/web/src/components/activity/utils.ts (1)
apps/web/src/components/activity/types.ts (1)
ActivityLog(10-36)
apps/realtime/src/index.ts (2)
packages/lib/src/logging/logger-config.ts (1)
loggers(8-18)apps/processor/src/logger.ts (1)
error(57-63)
apps/web/src/hooks/useActivitySocket.ts (1)
apps/web/src/hooks/useSocket.ts (1)
useSocket(5-30)
packages/lib/src/monitoring/activity-logger.ts (1)
packages/db/src/index.ts (1)
db(20-20)
apps/web/src/instrumentation.ts (2)
packages/lib/src/monitoring/activity-logger.ts (1)
setActivityBroadcastHook(178-180)apps/web/src/lib/websocket/socket-utils.ts (1)
broadcastActivityEvent(383-452)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & TypeScript Check
- GitHub Check: Unit Tests
🔇 Additional comments (8)
apps/web/src/components/activity/utils.ts (1)
103-124: The nullchangeGroupIdbehavior is correct and intentional.The function correctly groups consecutive rollbacks by matching
changeGroupId, including when it is null. This is verified by the test suite at line 135-150, which explicitly expects three rollback activities with nullchangeGroupIdto be grouped together.This differs intentionally from
collectConsecutiveEditSession(line 152), which requires non-nullchangeGroupIdbecause edit sessions must have achangeGroupIdto be valid (enforced by theisEditSessionGroupablepredicate at line 96). Rollbacks have no such requirement—they can be meaningful with or without achangeGroupId, and the grouping logic correctly handles both cases by matching whatever value exists.No changes are needed.
apps/web/src/components/layout/right-sidebar/ai-assistant/__tests__/SidebarActivityTab.test.tsx (1)
14-29: LGTM!The added mocks correctly isolate the component from external dependencies:
useRoutermock provides the standard Next.js router functions needed for navigation tests.useActivitySocketmock prevents real socket connections during testing while providing the expected return shape.apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarActivityTab.tsx (1)
247-256: LGTM - Real-time activity integration is well-implemented.The socket integration correctly:
- Determines context from route params (page > drive > null for dashboard)
- Passes
loadActivitiesas the reload callback- Uses a type fallback for
contextthat's safe because the hook early-returns whencontextIdis nullThe inline comment clarifies the fallback intent.
apps/web/src/hooks/useActivitySocket.ts (1)
1-94: Well-structured hook implementation.The hook follows React best practices:
- Uses refs to maintain stable callback identity and avoid effect re-runs
- Properly manages room subscriptions with context tracking
- Implements client-side debouncing to complement server-side debouncing
- Cleans up subscriptions on context change and unmount
packages/lib/src/monitoring/activity-logger.ts (2)
157-180: Clean broadcast hook abstraction.The broadcast hook pattern:
- Provides clean decoupling between the logger and the broadcast mechanism
- Uses a module-level variable for the hook, allowing runtime configuration
- Type-safe payload definition with all necessary context
271-298: Note on transaction timing assumption.The comment states the broadcast "will fire after the transaction commits" due to debouncing. This relies on the 500ms debounce delay exceeding transaction duration, which is typically true but not guaranteed for long-running transactions.
This is acceptable since broadcast failures don't affect data consistency, but worth noting for future debugging if activity events appear before their transactions complete.
apps/web/src/lib/websocket/socket-utils.ts (1)
370-451: Well-implemented debounced broadcasting.The activity broadcast function:
- Correctly implements per-context debouncing with a 500ms window
- Handles both drive and page contexts when both IDs are present
- Follows the established error-handling pattern (log but don't throw)
- Properly cleans up the pending map entries after timeout fires
apps/realtime/src/index.ts (1)
273-322: LGTM - Activity channel handlers follow established patterns.The new handlers:
- Implement proper permission verification before joining rooms
- Use consistent room naming (
activity:drive:{id},activity:page:{id})- Follow the same error handling and logging patterns as existing handlers
- Don't disconnect on access denial (consistent with
join_drivebehavior)
Update cleanup guard in useActivitySocket to verify socket is connected before emitting leave event, avoiding emit on disconnected socket. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Problem
Scenario:
Root Cause
This didn't account for
rollingBackRollback. When rolling back a rollback:actionis still'rollback'rollingBackRollbackistrueSolution
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
UI
✏️ Tip: You can customize this high-level summary in your review settings.