Conversation
|
@Praashh is attempting to deploy a commit to the william Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughRemoved direct contenteditable toggles from the diff plugin and delegated editability to the Editor component; removed client-side validation for version-fork and allow forking from the latest version; stopped auto-committing to the latest version on pointer leave. Server-side validation and fork request flow remain. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VersionRail
participant DiffPlugin
participant Editor
participant Server
User->>VersionRail: triggerFork(idx, forkFromTimestamp)
VersionRail-->>DiffPlugin: dispatch version-fork event (detail includes idx, timestamp)
DiffPlugin->>Server: POST /api/document { action: 'fork', versionIndex, forkFromTimestamp, ... }
Server-->>DiffPlugin: 200 { id: newDocId } / error
DiffPlugin->>User: navigate to newDocId or show error
rect #EFEFEF
Note over DiffPlugin,Editor: Preview/cancel/apply flow\n(Editability delegated to Editor)
end
User->>DiffPlugin: preview-document-update
DiffPlugin->>Editor: emit preview request (no contenteditable toggles)
Editor-->>User: handles editability and UI state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
apps/saru/lib/editor/diff-plugin.ts (3)
26-27: Good call removing direct contenteditable flips; add a non-invasive preview signal for Editor/UI.Directly mutating
contenteditableoneditorView.domcan desync ProseMirror’s internaleditablestate. To help the Editor component react (styling, toolbar state, etc.) without reintroducing that risk, set a data attribute when preview starts.Please confirm the Editor component already toggles
editablein sync with preview/apply/cancel; otherwise users could still edit while preview is shown.Apply this small addition within the same block:
- // Commented out: editorView.dom.setAttribute('contenteditable', 'false'); - // Let the Editor component handle editability + // Let the Editor component handle editability; expose non-invasive signal for UI/logic. + editorView.dom.setAttribute('data-preview-active', 'true');
68-69: On cancel: clear preview signal and restore focus for better UX.This keeps the Editor in charge of
editablewhile ensuring UI state and caret are restored.- // Commented out: editorView.dom.setAttribute('contenteditable', 'true'); - // Let the Editor component handle editability + // Clear preview signal; Editor controls editability. + editorView.dom.removeAttribute('data-preview-active'); + // Optionally restore caret/focus. + editorView.focus();
112-116: On apply: clear preview signal, restore focus, and avoid dispatch after unmount by tracking the timeout.Store the timeout id and clear it in
destroy()to prevent a latedispatchif the view unmounts during the animation window.- setTimeout(finalizeApply, animationDuration); - // Commented out: editorView.dom.setAttribute('contenteditable', 'true'); - // Let the Editor component handle editability + applyTimeoutId = window.setTimeout(finalizeApply, animationDuration); + // Clear preview signal; Editor controls editability. + editorView.dom.removeAttribute('data-preview-active'); + editorView.focus();Additions outside this hunk (for clarity):
// After lastPreviewContentRef declaration: let applyTimeoutId: number | null = null;// Inside destroy(): if (applyTimeoutId !== null) { clearTimeout(applyTimeoutId); applyTimeoutId = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/saru/lib/editor/diff-plugin.ts(3 hunks)
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/saru/components/document/version-rail.tsx (1)
29-31: Fix long‑press detection: stale state captured in timeout prevents reliable triggeringThe timeout closure captures an out-of-date
pressStartvalue from the render when the timeout was scheduled. This can cause missed or spurious forks. Track press start with a ref and clear it on pointer up/leave.Apply these diffs:
- const [pressStart, setPressStart] = React.useState<number | null>(null); + const pressStartRef = React.useRef<number | null>(null);const handlePointerDown = (e: React.PointerEvent<HTMLDivElement>) => { maybeRefresh(); const rect = e.currentTarget.getBoundingClientRect(); const x = e.clientX - rect.left; const fraction = x / rect.width; const idx = Math.min(Math.max(Math.round(fraction * (versions.length - 1)), 0), versions.length - 1); - setPressStart(Date.now()); - - setTimeout(() => { - if (pressStart !== null && Date.now() - pressStart >= 800) { - triggerFork(idx); - setPressStart(null); - } - }, 800); + const start = Date.now(); + pressStartRef.current = start; + setTimeout(() => { + if (pressStartRef.current !== null && Date.now() - pressStartRef.current >= 800) { + triggerFork(idx); + pressStartRef.current = null; + } + }, 800); };const handlePointerUp = () => { - setPressStart(null); + pressStartRef.current = null; };const handlePointerLeave = () => { - setPressStart(null); + pressStartRef.current = null; if (hoverIndex !== null) { if (isViewingHistory && selectedIndex !== null && selectedIndex < versions.length - 1) { const v = versions[selectedIndex]; window.dispatchEvent( new CustomEvent('preview-document-update', { detail: { documentId: baseDocumentId, newContent: v.content }, }) ); } else { window.dispatchEvent( new CustomEvent('cancel-document-update', { detail: { documentId: baseDocumentId }, }) ); } setHoverIndex(null); }Optional: also store the timeout id in a ref and clear it on unmount/pointer cancel to avoid late triggers if the component unmounts within 800ms.
Also applies to: 171-186, 188-190, 192-194
🧹 Nitpick comments (3)
apps/saru/components/document/version-rail.tsx (3)
122-135: Fork payload timestamp: normalize to ISO and prefer updatedAt for latest versionNow that forking from the latest is allowed,
createdAtmay be stale. Also,createdAtis typedDate | string; passing aDateacross the event boundary can break consumers expecting strings.Apply this diff to compute a safe timestamp:
window.dispatchEvent( new CustomEvent('version-fork', { - detail: { - originalDocumentId: baseDocumentId, - versionIndex: idx, - forkFromTimestamp: version.createdAt, - }, + detail: { + originalDocumentId: baseDocumentId, + versionIndex: idx, + forkFromTimestamp: (() => { + const t = idx === versions.length - 1 + ? ((version as any).updatedAt ?? version.createdAt) + : version.createdAt; + return typeof t === 'string' ? t : (t as Date).toISOString(); + })(), + }, }) );Please confirm the server expects an ISO string and whether it relies on this timestamp for validation or snapshot lookup.
230-239: Clear hover state when returning to latest to avoid immediate re-previewsAfter cancelling preview, also clear
hoverIndexso moving the pointer inside the rail doesn’t re-dispatch a preview based on stale hover.const goToLatest = () => { const latestIndex = versions.length - 1; setSelectedIndex(latestIndex); onIndexChange(latestIndex); + setHoverIndex(null); window.dispatchEvent( new CustomEvent('cancel-document-update', { detail: { documentId: baseDocumentId }, }) ); };
71-77: Remove redundant branch in effectBoth branches set
selectedIndex(currentIndex). Simplify and reduce re-renders.- React.useEffect(() => { - if (currentIndex === versions.length - 1) { - setSelectedIndex(currentIndex); - } else { - setSelectedIndex(currentIndex); - } - }, [currentIndex, versions.length]); + React.useEffect(() => { + setSelectedIndex(currentIndex); + }, [currentIndex]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/saru/components/document/document-workspace.tsx(0 hunks)apps/saru/components/document/version-rail.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/saru/components/document/document-workspace.tsx
🔇 Additional comments (1)
apps/saru/components/document/version-rail.tsx (1)
192-212: Pointer-leave behavior change looks good; verify editability resets end-to-endDropping auto-commit to latest on leave should help UX. Please verify that
cancel-document-updatereliably restores editor editability after hovering history, including when versions refresh mid-hover.
2 participants
This PR fixes the uneditable editor input reported here: https://x.com/studio_hungry/status/1962794756439716321
Fix Screen recording
Screen.Recording.2025-09-02.at.4.00.52.PM.mov
Summary by CodeRabbit