feat(desktop): v2 file editor — foundation, views, and stability pass#3526
Merged
Conversation
…gistry + CodeView
…veError/conflict/orphaned/hasExternalChange + banners + conflict dialog
…mageView, BinaryWarningView, FileViewToggle
…acking, external-change reload
…n via non-hook fileDocumentStore.getDocument
… + host-service useFileDocument
…renderHeaderExtras Also inline FileIcon inside renderTitle so the file icon keeps rendering — the default pane header uses `titleContent ?? (icon + title)`, and upstream 3dd1de2 introduced the custom renderTitle for italic name + dirty dot without including the icon, suppressing it.
…aceholder, contour selection, palette-native highlight - Fold markers now render as Lucide chevron SVGs via foldGutter's markerDOM, aligned per-line-box regardless of font metrics. Hidden at rest, fades in on gutter hover. - Fold placeholder (collapsed block) renders as a Lucide MoreHorizontal button via codeFolding's placeholderDOM, with rounded corners + subtle hover background. - Custom selection layer (contourSelectionLayer): per-line RectangleMarkers snug to each line's actual text width, extending 4px past the last character, with a half-line-height stub for empty middle lines. Consecutive rects abut exactly. CM's default .cm-selectionBackground hidden — our layer is the only painter. - Selection background and active-line highlight share one palette-derived token (foreground at ~3% alpha), so they read as the same visual weight. Active-line hidden while a selection is active (selectionClassTogglePlugin toggles .cm-hasSelection on the editor root). - Dropped the gutter/content separator border. Line numbers get more left padding, less right padding now that there's no vertical rule. - @replit/codemirror-css-color-picker adds inline swatches on CSS color literals. - Exported withAlpha from shared/themes so editor theme can derive low-alpha variants without hardcoded rgba.
…dupe view resolution Per AGENTS.md "one folder per module with barrel index.ts": - Extract inline helpers from CodeEditor.tsx into sibling extension folders: extensions/foldChevron, foldPlaceholder, contourSelectionLayer, selectionClassTogglePlugin. Hoists TRAILING_PAD / EMPTY_LINE_WIDTH_RATIO to module scope alongside their layer. - Move loose CodeMirror utility modules into their own folders: CodeEditorAdapter/, createCodeMirrorTheme/, loadLanguageSupport/ (with streamLanguages co-located as its only consumer), syntax-highlighting/. - Add registry/resolveActivePaneView/ — shared helper consumed by FilePane and FilePaneHeaderExtras to compute views + active view identically. Kills ~12 lines of duplication and removes one class of desync bugs. CodeEditor.tsx drops from 425 → 256 lines, focused purely on the React component.
AGENTS.md groups shared utility functions under utils/; relocate so the registry matches the convention.
…guards, conflict UX - Store passes trpcClient per-entry at acquire time so there's no global init race on hard reload; removes activeTrpcClient + requireClient. - Rename-safe acquire/release: iterate entries via snapshot to avoid mid-iteration map-insert loops; detect atomic-save renames (target matches open entry) and treat as content update; reuse handle when the entry was already migrated to the new path to avoid refcount leaks. - Preview-pane retarget: useSharedFileDocument now swaps handles on workspaceId/absolutePath prop changes via setState-during-render so the view never shows a stale file. - FilePane auto-follows renames by reconciling data.filePath from document.absolutePath, and auto-pins on first dirty transition. - Dirty state no longer mirrored into pane data; the tab title reads document.dirty live via a small FilePaneTabTitle component, which eliminates the store-cascade on first keystroke. Drop hasChanges from FilePaneData, and switch onBeforeClose / onBeforeCloseTab to read getDocument(ws, path)?.dirty. - Stable entry.id exposed on SharedFileDocument; CodeEditor keys on it so rename doesn't remount the editor and undo history is preserved. - Rename no longer flags hasExternalChange — a path change isn't a content change; disk conflicts continue to surface at save time. - Replace ExternalChangeBar + MultiFileDiff ConflictDialog with a VS Code-style alert (Save / Don't Save / Cancel) fired from FilePane. - CLOSE_TERMINAL usages in v2 swapped to CLOSE_PANE; the hotkey handler now runs the pane registry's onBeforeClose before closing. - Copy Path switches from electronTrpc to navigator.clipboard so it works when the host service is remote. - Active-line highlight bumped and switched to accent token; Cmd+Shift+/ vacated for the editor's built-in comment toggle on Cmd+/.
Dirty document entries never dispose (refCount <= 0 && !dirty rule), so choosing "Don't Save" on close left the entry floating in the store. Reopening the same file reattached to that stale dirty buffer, which looked like dirty state bleeding between files. Both close prompts now reload the buffer to disk before resolving, and the multi-file tab close's "Save All" actually saves each dirty pane via doc.save() instead of the previous TODO no-op.
- Images load via binary encoding (readAsBinary = isImageFile) so PNGs and friends don't go through a corrupting utf-8 decode path. - ImageView renders on a 1:1 checkerboard underlay (10% foreground mix) so transparency reads correctly against either theme. - Non-image binaries stay as utf-8 text so "Open Anyway" can actually render them in the code view. The isBinary flag is still used for view resolution. - binaryWarningView demoted from exclusive → default priority, and codeView no longer matches binary files, so an image's view toggle shows [Image] only rather than [Binary, Image], and unknown binaries default to the BinaryWarning with code as a toggle fallback. - Default load cap bumped 2 MB → 10 MB. too-large now offers an "Open anyway" button via a new document.loadUnlimited() method.
Matches VS Code's model — preview is a view of the source, not an editor. Avoids TipTap's markdown round-trip drift (parse → ProseMirror → serialize) that would spuriously mark files dirty and reformat on save. Edits continue to happen in the code view; preview re-renders from the shared document.
Contributor
📝 WalkthroughWalkthroughIntroduces a centralized, reference-counted shared file document store and provider, migrates panes to use the shared store and a file-view registry, removes legacy per-pane file hooks/renderers, and integrates a CodeMirror 6-based code editor with language loading and custom editor extensions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FilePane
participant SharedDocStore as Shared Document Store
participant TRPC as TRPC Client
participant WorkspaceEvent as Workspace Events
User->>FilePane: Open file
FilePane->>SharedDocStore: acquireDocument(workspaceId, filePath)
SharedDocStore->>TRPC: Fetch file content & metadata
TRPC-->>SharedDocStore: Content + revision
SharedDocStore-->>FilePane: SharedFileDocument handle
User->>FilePane: Edit content
FilePane->>SharedDocStore: document.setContent(newText)
SharedDocStore->>SharedDocStore: Update dirty flag
User->>FilePane: Save
FilePane->>SharedDocStore: document.save()
SharedDocStore->>TRPC: Write content with revision precondition
alt Conflict
TRPC-->>SharedDocStore: Conflict
SharedDocStore->>SharedDocStore: Set conflict + hasExternalChange
SharedDocStore-->>FilePane: SaveResult.conflict
FilePane->>User: Show conflict dialog
User->>FilePane: Resolve choice
FilePane->>SharedDocStore: document.resolveConflict(choice)
else Success
TRPC-->>SharedDocStore: Updated revision
SharedDocStore->>SharedDocStore: Clear dirty
SharedDocStore-->>FilePane: SaveResult.saved
end
WorkspaceEvent->>SharedDocStore: fs:events (file changed)
SharedDocStore->>SharedDocStore: Mark hasExternalChange or reload
SharedDocStore-->>FilePane: Notify subscribers
FilePane->>FilePane: Render banners or update content
sequenceDiagram
participant FilePane
participant ViewResolver as resolveActivePaneView
participant Registry as ALL_VIEWS
participant Views as View Components
FilePane->>ViewResolver: resolveActivePaneView(document, paneData)
ViewResolver->>Registry: resolveViews(filePath, meta)
alt forceViewId present
Registry-->>ViewResolver: forced view
else
Registry->>Registry: filter ALL_VIEWS by match
alt exclusive match exists
Registry-->>ViewResolver: exclusive view
else
Registry->>Registry: sort by priority
Registry-->>ViewResolver: ordered views
end
end
ViewResolver->>FilePane: activeView
FilePane->>Views: Render activeView.Renderer with ViewProps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (10)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/types.ts (1)
15-20: Numeric gap inPRIORITY_RANKis intentional?Values are
5, 4, 3, 1— ordering (exclusive > default > builtin > option) is correct and matches the comment, but the gap betweenbuiltin(3) andoption(1) is unusual. If it's reserved for a future tier (e.g., VS Code's implicituserpriority), a brief comment would help; otherwise making them contiguous (4, 3, 2, 1) avoids reader questions. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/types.ts around lines 15 - 20, PRIORITY_RANK currently uses values 5,4,3,1 which creates an unexplained gap between builtin and option; update the declaration for clarity by either making the ranks contiguous (e.g., exclusive=4, default=3, builtin=2, option=1) or add a short inline comment above PRIORITY_RANK explaining that the gap is intentional and reserved for a future tier; refer to the PRIORITY_RANK constant and the Priority type to locate and change the mapping or add the explanatory comment.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/SaveErrorBanner/SaveErrorBanner.tsx (1)
15-32: Use the sharedButtoncomponent for consistency and accessibility.
ErrorState.tsx(the sibling component in FilePane) usesButtonfrom@superset/ui/buttonwithvariant="outline"for its "Open anyway" action. SaveErrorBanner's native<button>elements with rawunderline/hover:no-underlinestyling create an inconsistency. UsingButtonwithvariant="link"would align the two components and automatically provide accessible focus rings and proper underline offset via the design system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/SaveErrorBanner/SaveErrorBanner.tsx around lines 15 - 32, Replace the native <button> elements in SaveErrorBanner.tsx with the shared Button component to match ErrorState.tsx; specifically, change the actions that use onRetry and onDismiss to render Button from `@superset/ui/button` with variant="link" (and preserve type="button" behavior via the Button props) so the "Retry" and "Dismiss" controls gain consistent styling, accessible focus rings, and underline offset from the design system.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FileViewToggle/FileViewToggle.tsx (1)
17-35: Add segmented-control a11y semantics.Visually this reads as a segmented toggle, but to assistive tech it's just a row of buttons with no indication of which is selected. Consider
role="group"on the wrapper andaria-pressed={view.id === activeViewId}(orrole="radiogroup"+role="radio"+aria-checked) on the buttons, plus anaria-labelfor the group.♿ Proposed fix
- <div className="inline-flex items-center gap-0.5 rounded bg-muted p-0.5 text-xs"> + <div + role="group" + aria-label="Select file view" + className="inline-flex items-center gap-0.5 rounded bg-muted p-0.5 text-xs" + > {views.map((view) => ( <button key={view.id} type="button" + aria-pressed={view.id === activeViewId} className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FileViewToggle/FileViewToggle.tsx around lines 17 - 35, The FileViewToggle render should expose segmented-control semantics: add a descriptive aria-label and role="group" to the wrapper div (where views is mapped) and mark each button with aria-pressed={view.id === activeViewId} (or alternatively use role="radiogroup" on the wrapper and role="radio" plus aria-checked on each button) so assistive tech knows which view is selected; update the JSX where views.map renders buttons (referencing FileViewToggle, activeViewId, onChange, and resolveViewLabel) to include these attributes.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx (1)
6-15: PreferURL.createObjectURL(Blob)over base64-encoding image bytes.
Array.from(uint8array).map(...).join("")allocates a JS array, a char array, and a ~N-byte intermediate string beforebtoaproduces another ~1.33×N base64 string — for a multi‑megabyte image, that's a significant peak-memory spike on every content change, and very large payloads can hit call-stack limits on older runtimes. ABlobURL skips the base64 round-trip entirely and is what<img>is optimized for.♻️ Proposed refactor
-import { useMemo } from "react"; +import { useEffect, useState } from "react"; import { getImageMimeType } from "shared/file-types"; import type { ViewProps } from "../../types"; export function ImageView({ document, filePath }: ViewProps) { - const dataUrl = useMemo(() => { - if (document.content.kind !== "bytes") return null; - const mimeType = getImageMimeType(filePath) ?? "image/png"; - const base64 = btoa( - Array.from(document.content.value) - .map((b) => String.fromCharCode(b)) - .join(""), - ); - return `data:${mimeType};base64,${base64}`; - }, [document.content, filePath]); + const [objectUrl, setObjectUrl] = useState<string | null>(null); + useEffect(() => { + if (document.content.kind !== "bytes") { + setObjectUrl(null); + return; + } + const mimeType = getImageMimeType(filePath) ?? "image/png"; + const url = URL.createObjectURL( + new Blob([document.content.value], { type: mimeType }), + ); + setObjectUrl(url); + return () => URL.revokeObjectURL(url); + }, [document.content, filePath]); - if (!dataUrl) { + if (!objectUrl) { return null; }And update
src={dataUrl}→src={objectUrl}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx around lines 6 - 15, Replace the base64 data URL creation in ImageView.tsx (the dataUrl useMemo that reads document.content and filePath) with a Blob URL produced via new Blob([document.content.value], { type: mimeType }) and URL.createObjectURL; return null if document.content.kind !== "bytes"; ensure you revoke the previous object URL with URL.revokeObjectURL when document.content or filePath changes or on unmount (useEffect cleanup), and update the <img> src usage from src={dataUrl} to src={objectUrl}; keep getImageMimeType(filePath) to supply the blob type.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldChevron/foldChevron.ts (1)
8-20: The function's return type should beNode, notHTMLElement.CodeMirror's
foldGutter.markerDOMis typed as(open: boolean) => Node, notHTMLElement. Sincedocument.createElementNS()returnsSVGSVGElement(a subtype ofNode), the function can return it directly without theas unknown as HTMLElementcast by changing the return type annotation toNode.Alternatively, the suggested refactor of wrapping the SVG in a
document.createElement("span")would also satisfy the currentHTMLElementreturn type and eliminate the cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldChevron/foldChevron.ts around lines 8 - 20, The returned SVG element for the fold chevron is being cast to HTMLElement even though CodeMirror's foldGutter.markerDOM expects a Node; update the function signature to return Node (not HTMLElement) and remove the "as unknown as HTMLElement" cast so the SVGSVGElement from document.createElementNS(...) can be returned directly, referencing the existing CHEVRON_DOWN_PATH / CHEVRON_RIGHT_PATH logic and the generated element (class "cm-foldChevron") used by foldGutter.markerDOM.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.ts (1)
56-61:getSelectionLinesnever returnsnulldespite the declared union.The interface (
EditorSelectionLines | null) suggests callers must handle a null case, but the implementation always returns an object (an empty selection still yieldsstartLine === endLineat the cursor). Either drop| nullfrom the interface, or add an explicitselection.emptycheck if callers genuinely care about the "no selection" case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.ts around lines 56 - 61, The getSelectionLines implementation always returns an object but its signature (EditorSelectionLines | null) implies it can return null; make the behavior and type consistent by either removing the union from the signature or returning null when there's no selection — e.g., in getSelectionLines() inspect view.state.selection.main.empty and return null for an empty selection, otherwise compute and return { startLine, endLine }; update any callers if you choose the type-change path.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts (2)
260-265:loadUnlimitedstate reset is inconsistent withreload.
reload()clearsconflict,hasExternalChange, andsaveErroralongsidesavedContentText, butloadUnlimited()clears onlysavedContentText. A user opening a large file via "Open anyway" after a conflict/save error may be left with stale error/conflict UI attached to freshly-loaded content. Mirrorreload()'s reset (or extract a sharedresetForLoad(entry)helper).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts around lines 260 - 265, loadUnlimited currently only clears savedContentText and sets entry.content to loading, which leaves stale conflict/saveError/hasExternalChange state; update loadUnlimited to mirror reload by resetting entry.conflict, entry.hasExternalChange, entry.saveError, and entry.savedContentText (and keep entry.content = { kind: "loading" } and notify(entry)) before calling await loadEntry(entry, { unlimited: true }), or extract a shared helper like resetForLoad(entry) used by both reload and loadUnlimited to perform these resets.
377-410: Nit:isContentMutationrename branch is unreachable.
isContentMutationincludes(event.kind === "rename" && event.absolutePath === entry.absolutePath), but the rename-matching entry is handled by the earlierevent.oldAbsolutePath === entry.absolutePathbranch whichcontinues, and any entry whoseabsolutePath === event.absolutePathat this point is unrelated to a rename. The rename contribution toisContentMutationis dead. Either drop it or intentionally treat post-rename as a content mutation (currently nothing refreshes the entry's content after a rename — you may want to).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts around lines 377 - 410, The isContentMutation condition includes a rename case that is never reached because renames where event.oldAbsolutePath === entry.absolutePath are handled earlier; either remove the redundant `(event.kind === "rename" && event.absolutePath === entry.absolutePath)` from isContentMutation, or intentionally handle post-rename-as-content-mutation by moving rename-post-processing into the rename branch: when a rename arrives with event.absolutePath === entry.absolutePath (i.e. this entry now matches the new path), clear entry.orphaned, run computeDirty(entry) and set entry.hasExternalChange/notify or call loadEntry(entry) as the existing content-mutation block does; update references to isContentMutation, the rename branch (event.oldAbsolutePath === entry.absolutePath), loadEntry, computeDirty, entry.hasExternalChange and entry.orphaned accordingly.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditor.tsx (1)
67-69: Nit:useRef(new Compartment())allocates a freshCompartmenton every render.
useRef(initialValue)evaluates its argument on every render (React only reads.currenton the first mount), so a throwawayCompartmentis constructed each render and discarded. Not a correctness bug, just wasted allocations. If you want to avoid it, use the lazy-init idiom:♻️ Proposed refactor
- const languageCompartment = useRef(new Compartment()).current; - const themeCompartment = useRef(new Compartment()).current; - const editableCompartment = useRef(new Compartment()).current; + const compartmentsRef = useRef<{ + language: Compartment; + theme: Compartment; + editable: Compartment; + } | null>(null); + if (!compartmentsRef.current) { + compartmentsRef.current = { + language: new Compartment(), + theme: new Compartment(), + editable: new Compartment(), + }; + } + const { language: languageCompartment, theme: themeCompartment, editable: editableCompartment } = compartmentsRef.current;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditor.tsx around lines 67 - 69, The Compartment instances (languageCompartment, themeCompartment, editableCompartment) are being constructed on every render because useRef(new Compartment()) evaluates its argument each time; change to lazy-init so you only allocate once: create each ref with useRef<Compartment | null>(null) (or an uninitialized ref), then on first render set ref.current = new Compartment() if it's null (e.g., in the component body before use), and use ref.current afterwards (refer to languageCompartment, themeCompartment, and editableCompartment to locate places to change). Ensure the rest of the code continues to read .current and no other behavior changes.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx (1)
204-208: "Don't Save" usesreload()to drop the dirty buffer — consider a dedicated discard API.Using
doc.reload()here works (it replaces the buffer with on-disk content socomputeDirtyreturns false andreleaseDocumentcan evict the entry), but it triggers an unnecessary filesystem round-trip on close and, if the file is now missing, will transition the entry to{ kind: "not-found" }and then release. A dedicateddiscard()onSharedFileDocumentthat simply resetscontent.value = savedContentText(or clears dirty state) would be clearer in intent and avoid the extra I/O — especially relevant when "Save All" / "Don't Save" runs across many files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx around lines 204 - 208, The current "Don't Save" handler calls getDocument(...).reload(), which forces an unnecessary filesystem round-trip and can flip the entry to { kind: "not-found" }; add a dedicated discard() method to SharedFileDocument that resets the in-memory buffer (e.g., set content.value = savedContentText or clear the dirty flag) without touching disk, update the SharedFileDocument implementation to expose discard() alongside reload() and computeDirty(), and change the onClick handler in usePaneRegistry to call doc.discard() (and then releaseDocument) instead of doc.reload() so you avoid I/O and accidental kind transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/plans/20260412-file-editor-v2-implementation.md`:
- Around line 493-503: The implementation in page.tsx implements the "Don't
Save" branch by calling doc.reload(), but the spec requires calling
document.discard() to drop the buffer and force refcount teardown; update the
code so onBeforeClose (or the handler in page.tsx that implements the dialog)
calls document.discard() on the FileDocument instance obtained via
fileDocumentStore.get(workspaceId, filePath) (or expose documentHandle from
FilePane) instead of doc.reload(), and add discard() to the document API and its
concrete implementation if missing (ensuring it drops in-memory buffer and
decrements refcount without doing I/O); alternatively, if you intend reload() to
be the new contract, update the spec and all callers to reflect that reload()
must behave like discard() in the "Don't Save" case and handle orphaned files
safely.
In `@apps/desktop/src/renderer/hooks/useCopyToClipboard.ts`:
- Around line 6-13: Wrap the async navigator.clipboard.writeText call inside the
copyToClipboard useCallback in a try/catch so promise rejections are handled; on
success keep the existing setCopied(true) and setTimeout reset behavior, and on
failure either set an error state (e.g., add a new error/useState variable) or
simply skip calling setCopied(true) and optionally log the error. Update the
copyToClipboard implementation (referenced by the useCallback named
copyToClipboard) to catch errors from navigator.clipboard.writeText and surface
or swallow the error accordingly, while preserving the timeout and dependency on
timeout.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.ts:
- Around line 65-131: The clipboard promise callbacks in CodeEditorAdapter.cut
and CodeEditorAdapter.paste can call view.dispatch after the adapter is
disposed; guard against dispatching into a destroyed view by checking the
adapter's disposed flag (the same one used elsewhere) before calling
view.dispatch inside the .then handlers and return early if disposed; also add
the same disposed guard to copy() and the synchronous paths in cut()/paste()
before using view or view.state to be defensive, referencing the cut, copy,
paste methods and the view and disposed identifiers to locate the changes.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/constants.ts:
- Around line 1-2: The font-family constant DEFAULT_CODE_EDITOR_FONT_FAMILY
currently lists Liberation Mono unquoted which is a multi-word font name and
will be ignored by browsers; update the string value for
DEFAULT_CODE_EDITOR_FONT_FAMILY to quote "Liberation Mono" (e.g., "Liberation
Mono") so the font-family list is valid and the fallback chain works as
intended.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/MarkdownPreviewView/MarkdownPreviewView.tsx:
- Around line 11-14: The TipTapMarkdownRenderer is rendered read-only here
(editable defaults to false) so the onSave prop passed (onSave={() => void
document.save()}) is dead; remove the onSave prop from the
<TipTapMarkdownRenderer> usage in MarkdownPreviewView to avoid misleading dead
code and rely on the EditorHotkeys isEditable guard in
createMarkdownExtensions.ts which prevents save when not editable.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:
- Around line 75-85: The CLOSE_PANE hotkey handler is re-entrant because it's
async and awaits definition.onBeforeClose; add an in-flight guard (e.g., a ref
like isClosingRef) in the useWorkspaceHotkeys hook to short-circuit if a close
is already pending, set it true before awaiting onBeforeClose and false after
completion or early return, and only call state.closePane when the guard allows;
reference the existing useHotkey("CLOSE_PANE", ...),
paneRegistry[active.pane.kind], definition.onBeforeClose, store.getState(), and
state.closePane to locate where to add the ref/guard.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 400-408: The "Don't Save" branch iterates dirtyPanes and calls
getDocument(...).reload(), which can throw or leave orphaned docs in error;
instead call the document.discard() API to drop buffers and clear dirty/orphaned
state. Locate the loop over dirtyPanes (where pane.data is cast to FilePaneData
and getDocument(workspaceId, filePath) is called) and replace any await
doc.reload() with doc.discard() (no re-read) before resolving so refcount
teardown can proceed cleanly.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts:
- Around line 125-129: The catch in loadEntry currently collapses all errors
into entry.content = { kind: "not-found" } which misrepresents transient
failures; update loadEntry to inspect the thrown error (e.g., tRPC error
code/reason or decode/permission indicators) and only set entry.content to {
kind: "not-found" } when the error is a true NotFound error, otherwise set a
retryable/failed state (e.g., a new ContentState like { kind: "error", error }
or populate the existing saveError-style field) and call notify(entry) so the UI
can show retry/saveError flows instead of “deleted” UX; look for symbols
loadEntry, entry.content, notify and the saveError handling logic to mirror
state shape.
- Around line 234-243: The save() path currently overwrites the live buffer by
replacing entry.content with the pre-await snapshot (currentValue); instead,
after writeFile.mutate succeeds only update the saved revision and
savedContentText and preserve the live buffer: update entry.content.revision =
result.revision (do NOT replace entry.content.value), set entry.savedContentText
= currentValue (the value actually persisted), clear entry.conflict and
entry.hasExternalChange as before, then recompute entry.dirty =
entry.content.value !== entry.savedContentText and call notify(entry); return
the saved status. Use the existing symbols save(), writeFile.mutate,
entry.content, entry.savedContentText, entry.dirty, and notify to locate and
implement the change.
- Around line 389-399: The rename handler currently overwrites any existing
entry at the target path which silently orphaned its subscribers; modify the
block in fileDocumentStore where it computes oldKey/newKey (the code that calls
key(entry.workspaceId, entry.absolutePath), entries.delete, entry.absolutePath =
event.absolutePath, entries.set(...), notify(entry)) to detect collisions:
compute const newKey = key(entry.workspaceId, event.absolutePath); if
entries.has(newKey) { const collided = entries.get(newKey)!; mark collided as
orphaned (e.g., collided.orphaned = true or set an appropriate flag/state), call
notify(collided) so its subscribers learn the orphaned state, and then proceed
to delete oldKey and set the renamed entry under newKey; ensure you use the same
notify(entry) behavior for the renamed entry as before.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts:
- Line 15: The effect that acquires the file-document handle uses trpcClient
from useWorkspaceClient but only lists [workspaceId, absolutePath] as deps;
update the effect and any preceding guard to include trpcClient (from
useWorkspaceClient) in their dependency arrays or add a guard that
returns/throws if trpcClient is not stable, so the effect re-runs whenever
trpcClient identity changes and the handle is recreated with the correct client.
- Around line 31-47: The current code calls acquireDocument during render
(inside the conditional that sets setState), which can bump refCount during a
render that may be discarded and leak leases; move the acquire/release logic
into a key-driven useEffect keyed on [workspaceId, absolutePath, trpcClient],
keeping the existing handleAlreadyPointsAtNewPath check but calling
acquireDocument(workspaceId, absolutePath, trpcClient) inside the effect, then
setState({ handle, workspaceId, absolutePath }) when acquisition completes, and
in the effect cleanup release the previously held handle (call
previousHandle.release()) to ensure refCount is decremented if the effect is
torn down or dependencies change.
In `@apps/desktop/src/shared/themes/editor-theme.ts`:
- Line 18: activeLine is currently set to withAlpha(theme.ui.accent, 0.5) which
can be too opaque for caret-line highlighting; change the alpha to a lower value
(suggest 0.15–0.25) and re-test activeLine across all built-in themes (light,
dark, monokai) and any user-imported themes to ensure the caret line does not
wash out syntax colors or tint text; update the value used in the activeLine
assignment (the withAlpha call referencing theme.ui.accent) and, if needed, make
the alpha configurable or theme-dependent to preserve readability for saturated
accents.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FileViewToggle/FileViewToggle.tsx:
- Around line 17-35: The FileViewToggle render should expose segmented-control
semantics: add a descriptive aria-label and role="group" to the wrapper div
(where views is mapped) and mark each button with aria-pressed={view.id ===
activeViewId} (or alternatively use role="radiogroup" on the wrapper and
role="radio" plus aria-checked on each button) so assistive tech knows which
view is selected; update the JSX where views.map renders buttons (referencing
FileViewToggle, activeViewId, onChange, and resolveViewLabel) to include these
attributes.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/components/SaveErrorBanner/SaveErrorBanner.tsx:
- Around line 15-32: Replace the native <button> elements in SaveErrorBanner.tsx
with the shared Button component to match ErrorState.tsx; specifically, change
the actions that use onRetry and onDismiss to render Button from
`@superset/ui/button` with variant="link" (and preserve type="button" behavior via
the Button props) so the "Retry" and "Dismiss" controls gain consistent styling,
accessible focus rings, and underline offset from the design system.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/types.ts:
- Around line 15-20: PRIORITY_RANK currently uses values 5,4,3,1 which creates
an unexplained gap between builtin and option; update the declaration for
clarity by either making the ranks contiguous (e.g., exclusive=4, default=3,
builtin=2, option=1) or add a short inline comment above PRIORITY_RANK
explaining that the gap is intentional and reserved for a future tier; refer to
the PRIORITY_RANK constant and the Priority type to locate and change the
mapping or add the explanatory comment.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditor.tsx:
- Around line 67-69: The Compartment instances (languageCompartment,
themeCompartment, editableCompartment) are being constructed on every render
because useRef(new Compartment()) evaluates its argument each time; change to
lazy-init so you only allocate once: create each ref with useRef<Compartment |
null>(null) (or an uninitialized ref), then on first render set ref.current =
new Compartment() if it's null (e.g., in the component body before use), and use
ref.current afterwards (refer to languageCompartment, themeCompartment, and
editableCompartment to locate places to change). Ensure the rest of the code
continues to read .current and no other behavior changes.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.ts:
- Around line 56-61: The getSelectionLines implementation always returns an
object but its signature (EditorSelectionLines | null) implies it can return
null; make the behavior and type consistent by either removing the union from
the signature or returning null when there's no selection — e.g., in
getSelectionLines() inspect view.state.selection.main.empty and return null for
an empty selection, otherwise compute and return { startLine, endLine }; update
any callers if you choose the type-change path.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldChevron/foldChevron.ts:
- Around line 8-20: The returned SVG element for the fold chevron is being cast
to HTMLElement even though CodeMirror's foldGutter.markerDOM expects a Node;
update the function signature to return Node (not HTMLElement) and remove the
"as unknown as HTMLElement" cast so the SVGSVGElement from
document.createElementNS(...) can be returned directly, referencing the existing
CHEVRON_DOWN_PATH / CHEVRON_RIGHT_PATH logic and the generated element (class
"cm-foldChevron") used by foldGutter.markerDOM.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx:
- Around line 6-15: Replace the base64 data URL creation in ImageView.tsx (the
dataUrl useMemo that reads document.content and filePath) with a Blob URL
produced via new Blob([document.content.value], { type: mimeType }) and
URL.createObjectURL; return null if document.content.kind !== "bytes"; ensure
you revoke the previous object URL with URL.revokeObjectURL when
document.content or filePath changes or on unmount (useEffect cleanup), and
update the <img> src usage from src={dataUrl} to src={objectUrl}; keep
getImageMimeType(filePath) to supply the blob type.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:
- Around line 204-208: The current "Don't Save" handler calls
getDocument(...).reload(), which forces an unnecessary filesystem round-trip and
can flip the entry to { kind: "not-found" }; add a dedicated discard() method to
SharedFileDocument that resets the in-memory buffer (e.g., set content.value =
savedContentText or clear the dirty flag) without touching disk, update the
SharedFileDocument implementation to expose discard() alongside reload() and
computeDirty(), and change the onClick handler in usePaneRegistry to call
doc.discard() (and then releaseDocument) instead of doc.reload() so you avoid
I/O and accidental kind transitions.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts:
- Around line 260-265: loadUnlimited currently only clears savedContentText and
sets entry.content to loading, which leaves stale
conflict/saveError/hasExternalChange state; update loadUnlimited to mirror
reload by resetting entry.conflict, entry.hasExternalChange, entry.saveError,
and entry.savedContentText (and keep entry.content = { kind: "loading" } and
notify(entry)) before calling await loadEntry(entry, { unlimited: true }), or
extract a shared helper like resetForLoad(entry) used by both reload and
loadUnlimited to perform these resets.
- Around line 377-410: The isContentMutation condition includes a rename case
that is never reached because renames where event.oldAbsolutePath ===
entry.absolutePath are handled earlier; either remove the redundant `(event.kind
=== "rename" && event.absolutePath === entry.absolutePath)` from
isContentMutation, or intentionally handle post-rename-as-content-mutation by
moving rename-post-processing into the rename branch: when a rename arrives with
event.absolutePath === entry.absolutePath (i.e. this entry now matches the new
path), clear entry.orphaned, run computeDirty(entry) and set
entry.hasExternalChange/notify or call loadEntry(entry) as the existing
content-mutation block does; update references to isContentMutation, the rename
branch (event.oldAbsolutePath === entry.absolutePath), loadEntry, computeDirty,
entry.hasExternalChange and entry.orphaned accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d307755-c94f-491a-ad52-a68e07b9790d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (75)
apps/desktop/package.jsonapps/desktop/plans/20260412-file-editor-v2-feature-audit.mdapps/desktop/plans/20260412-file-editor-v2-implementation.mdapps/desktop/src/renderer/hooks/host-service/useFileDocument/index.tsapps/desktop/src/renderer/hooks/host-service/useFileDocument/useFileDocument.tsapps/desktop/src/renderer/hooks/useCopyToClipboard.tsapps/desktop/src/renderer/hotkeys/registry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ErrorState/ErrorState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ErrorState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ExternalChangeBar/ExternalChangeBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ExternalChangeBar/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/FilePaneHeaderExtras.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FilePaneHeaderExtras/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FileViewToggle/FileViewToggle.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/FileViewToggle/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/LoadingState/LoadingState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/LoadingState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/OrphanedBanner/OrphanedBanner.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/OrphanedBanner/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/SaveErrorBanner/SaveErrorBanner.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/SaveErrorBanner/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/allViews.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/resolveViews.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/utils/resolveActivePaneView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/utils/resolveActivePaneView/resolveActivePaneView.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/BinaryWarningView/BinaryWarningView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/BinaryWarningView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/CodeView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditor.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/constants.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/createCodeMirrorTheme/createCodeMirrorTheme.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/createCodeMirrorTheme/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/contourSelectionLayer/contourSelectionLayer.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/contourSelectionLayer/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldChevron/foldChevron.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldChevron/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldPlaceholder/foldPlaceholder.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/foldPlaceholder/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/selectionClassTogglePlugin/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/extensions/selectionClassTogglePlugin/selectionClassTogglePlugin.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/loadLanguageSupport/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/loadLanguageSupport/loadLanguageSupport.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/loadLanguageSupport/streamLanguages.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/syntax-highlighting/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/syntax-highlighting/syntax-highlighting.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/MarkdownPreviewView/MarkdownPreviewView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/MarkdownPreviewView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/CodeRenderer/CodeRenderer.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/CodeRenderer/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/ImageRenderer/ImageRenderer.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/ImageRenderer/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/MarkdownRenderer/MarkdownRenderer.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/MarkdownRenderer/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/FileDocumentStoreProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.tsapps/desktop/src/shared/themes/editor-theme.tsapps/desktop/src/shared/themes/index.tspackages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx
💤 Files with no reviewable changes (10)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ExternalChangeBar/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ExternalChangeBar/ExternalChangeBar.tsx
- apps/desktop/src/renderer/hooks/host-service/useFileDocument/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/CodeRenderer/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/MarkdownRenderer/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/ImageRenderer/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/ImageRenderer/ImageRenderer.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/MarkdownRenderer/MarkdownRenderer.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/renderers/CodeRenderer/CodeRenderer.tsx
- apps/desktop/src/renderer/hooks/host-service/useFileDocument/useFileDocument.ts
Comment on lines
+493
to
+503
|
|
||
| 1. User clicks close on a tab with `data.hasChanges === true` | ||
| 2. `usePaneRegistry.onBeforeClose` reads `data.hasChanges`, returns an `alert(...)` Promise | ||
| 3. Dialog: Save / Don't Save / Cancel | ||
| 4. **Save** → calls `document.save()`; on success, resolves `true` (close proceeds); on conflict/error, resolves `false` (close blocked, banner shows) | ||
| 5. **Don't Save** → calls `document.discard()` which forces refcount to allow teardown; resolves `true` | ||
| 6. **Cancel** → resolves `false` | ||
|
|
||
| Blocker: `FilePane` holds the document; `onBeforeClose` runs on pane data only. Either expose a `documentHandle` via pane context, or have the store expose a non-hook `getDocument(workspaceId, filePath)` for non-React callers. | ||
|
|
||
| Decision: **non-hook store access** (`fileDocumentStore.get(workspaceId, filePath)`), used by `onBeforeClose`. Keeps the registry decoupled from React rendering. |
Contributor
There was a problem hiding this comment.
Spec and implementation diverge on "Don't Save".
The spec states the "Don't Save" path calls document.discard() which forces the refcount to allow teardown. The implementation in page.tsx (lines 400–408) calls doc.reload() instead. reload() reads the file back from disk, which (a) does unnecessary I/O on a path whose whole purpose is to throw the buffer away, and (b) fails if the file is orphaned — exactly the "delete externally, user has unsaved edits, now wants to close without saving" case the spec calls out in §6.6. Please reconcile: either add discard() to the document API as specified, or update the spec if reload() is now the intended contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/plans/20260412-file-editor-v2-implementation.md` around lines
493 - 503, The implementation in page.tsx implements the "Don't Save" branch by
calling doc.reload(), but the spec requires calling document.discard() to drop
the buffer and force refcount teardown; update the code so onBeforeClose (or the
handler in page.tsx that implements the dialog) calls document.discard() on the
FileDocument instance obtained via fileDocumentStore.get(workspaceId, filePath)
(or expose documentHandle from FilePane) instead of doc.reload(), and add
discard() to the document API and its concrete implementation if missing
(ensuring it drops in-memory buffer and decrements refcount without doing I/O);
alternatively, if you intend reload() to be the new contract, update the spec
and all callers to reflect that reload() must behave like discard() in the
"Don't Save" case and handle orphaned files safely.
Comment on lines
+65
to
+131
| cut() { | ||
| if (view.state.readOnly) return; | ||
| const clipboard = navigator.clipboard; | ||
| if (!clipboard) return; | ||
|
|
||
| const selection = view.state.selection.main; | ||
| if (selection.empty) return; | ||
|
|
||
| const text = view.state.sliceDoc(selection.from, selection.to); | ||
| void clipboard | ||
| .writeText(text) | ||
| .then(() => { | ||
| const currentSelection = view.state.selection.main; | ||
| if ( | ||
| currentSelection.from !== selection.from || | ||
| currentSelection.to !== selection.to | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| if (view.state.sliceDoc(selection.from, selection.to) !== text) { | ||
| return; | ||
| } | ||
|
|
||
| view.dispatch({ | ||
| changes: { from: selection.from, to: selection.to, insert: "" }, | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| console.error("[CodeEditor] Failed to cut selection:", error); | ||
| }); | ||
| }, | ||
| copy() { | ||
| const clipboard = navigator.clipboard; | ||
| if (!clipboard) return; | ||
|
|
||
| const selection = view.state.selection.main; | ||
| if (selection.empty) return; | ||
|
|
||
| void clipboard | ||
| .writeText(view.state.sliceDoc(selection.from, selection.to)) | ||
| .catch((error) => { | ||
| console.error("[CodeEditor] Failed to copy selection:", error); | ||
| }); | ||
| }, | ||
| paste() { | ||
| if (view.state.readOnly) return; | ||
| const clipboard = navigator.clipboard; | ||
| if (!clipboard) return; | ||
|
|
||
| void clipboard | ||
| .readText() | ||
| .then((text) => { | ||
| const selection = view.state.selection.main; | ||
| view.dispatch({ | ||
| changes: { | ||
| from: selection.from, | ||
| to: selection.to, | ||
| insert: text, | ||
| }, | ||
| selection: EditorSelection.cursor(selection.from + text.length), | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| console.error("[CodeEditor] Failed to paste from clipboard:", error); | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
Async clipboard callbacks can dispatch into a destroyed view.
cut() and paste() schedule view.dispatch(...) inside .then(...) after an awaited clipboard op. If the editor (and its adapter) gets disposed between the navigator.clipboard promise resolving and the .then firing — a real scenario when a pane is closed mid-edit — the dispatch targets a destroyed EditorView and will throw. The .catch only covers the clipboard promise itself, not the dispatch.
Consider guarding with the already-tracked disposed flag (and ideally, the synchronous methods too):
🛡️ Proposed guard
cut() {
- if (view.state.readOnly) return;
+ if (disposed || view.state.readOnly) return;
const clipboard = navigator.clipboard;
if (!clipboard) return;
@@
void clipboard
.writeText(text)
.then(() => {
+ if (disposed) return;
const currentSelection = view.state.selection.main;
@@
paste() {
- if (view.state.readOnly) return;
+ if (disposed || view.state.readOnly) return;
const clipboard = navigator.clipboard;
if (!clipboard) return;
void clipboard
.readText()
.then((text) => {
+ if (disposed) return;
const selection = view.state.selection.main;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.ts
around lines 65 - 131, The clipboard promise callbacks in CodeEditorAdapter.cut
and CodeEditorAdapter.paste can call view.dispatch after the adapter is
disposed; guard against dispatching into a destroyed view by checking the
adapter's disposed flag (the same one used elsewhere) before calling
view.dispatch inside the .then handlers and return early if disposed; also add
the same disposed guard to copy() and the synchronous paths in cut()/paste()
before using view or view.state to be defensive, referencing the cut, copy,
paste methods and the view and disposed identifiers to locate the changes.
Comment on lines
+1
to
+2
| export const DEFAULT_CODE_EDITOR_FONT_FAMILY = | ||
| "ui-monospace, Menlo, Consolas, Liberation Mono, monospace"; |
Contributor
There was a problem hiding this comment.
Quote multi-word font name in font-family list.
Liberation Mono contains a space and must be quoted in a CSS font-family value; unquoted, browsers will treat it as an invalid identifier and skip it in the fallback chain.
Proposed fix
export const DEFAULT_CODE_EDITOR_FONT_FAMILY =
- "ui-monospace, Menlo, Consolas, Liberation Mono, monospace";
+ 'ui-monospace, Menlo, Consolas, "Liberation Mono", monospace';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const DEFAULT_CODE_EDITOR_FONT_FAMILY = | |
| "ui-monospace, Menlo, Consolas, Liberation Mono, monospace"; | |
| export const DEFAULT_CODE_EDITOR_FONT_FAMILY = | |
| 'ui-monospace, Menlo, Consolas, "Liberation Mono", monospace'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/constants.ts
around lines 1 - 2, The font-family constant DEFAULT_CODE_EDITOR_FONT_FAMILY
currently lists Liberation Mono unquoted which is a multi-word font name and
will be ignored by browsers; update the string value for
DEFAULT_CODE_EDITOR_FONT_FAMILY to quote "Liberation Mono" (e.g., "Liberation
Mono") so the font-family list is valid and the fallback chain works as
intended.
Comment on lines
+389
to
+399
| if ( | ||
| event.kind === "rename" && | ||
| event.oldAbsolutePath === entry.absolutePath | ||
| ) { | ||
| const oldKey = key(entry.workspaceId, entry.absolutePath); | ||
| entries.delete(oldKey); | ||
| entry.absolutePath = event.absolutePath; | ||
| entries.set(key(entry.workspaceId, entry.absolutePath), entry); | ||
| notify(entry); | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Rename handler can silently overwrite a pre-existing entry at the new path.
If two files are open (e.g., a.txt and b.txt) and a.txt is renamed to b.txt, this code does entries.set(key(workspaceId, "b.txt"), a_entry) and wipes the existing b.txt entry from the map without orphaning it or notifying its subscribers. The old b.txt handle keeps serving stale state, and one of the tabs will end up pointing at the wrong document.
Consider either:
- marking a pre-existing collision as
orphaned(since its on-disk identity just got overwritten), or - bailing out of the re-key if the target key is already occupied and falling through to the content-mutation path instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/fileDocumentStore.ts
around lines 389 - 399, The rename handler currently overwrites any existing
entry at the target path which silently orphaned its subscribers; modify the
block in fileDocumentStore where it computes oldKey/newKey (the code that calls
key(entry.workspaceId, entry.absolutePath), entries.delete, entry.absolutePath =
event.absolutePath, entries.set(...), notify(entry)) to detect collisions:
compute const newKey = key(entry.workspaceId, event.absolutePath); if
entries.has(newKey) { const collided = entries.get(newKey)!; mark collided as
orphaned (e.g., collided.orphaned = true or set an appropriate flag/state), call
notify(collided) so its subscribers learn the orphaned state, and then proceed
to delete oldKey and set the renamed entry under newKey; ensure you use the same
notify(entry) behavior for the renamed entry as before.
| workspaceId, | ||
| absolutePath, | ||
| }: UseSharedFileDocumentParams): SharedFileDocument { | ||
| const { trpcClient } = useWorkspaceClient(); |
Contributor
There was a problem hiding this comment.
Minor: trpcClient from context is not in the effect deps.
L49-53's effect depends only on [workspaceId, absolutePath]. If trpcClient identity ever changes (e.g., after a workspace-client re-init), the acquired handle keeps using the stale client until the key changes. Either include trpcClient in the guard above and in this effect's deps, or assert/document that trpcClient is stable for a given workspaceId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts
at line 15, The effect that acquires the file-document handle uses trpcClient
from useWorkspaceClient but only lists [workspaceId, absolutePath] as deps;
update the effect and any preceding guard to include trpcClient (from
useWorkspaceClient) in their dependency arrays or add a guard that
returns/throws if trpcClient is not stable, so the effect re-runs whenever
trpcClient identity changes and the handle is recreated with the correct client.
Comment on lines
+31
to
+47
| if ( | ||
| state.workspaceId !== workspaceId || | ||
| state.absolutePath !== absolutePath | ||
| ) { | ||
| // Rename case: the entry behind our existing handle was migrated to | ||
| // match the new props. Reuse the handle — acquiring again would bump | ||
| // refCount a second time and release() of the old key no-ops (the | ||
| // entry isn't at that key anymore), which would leak one lease per | ||
| // rename. | ||
| const handleAlreadyPointsAtNewPath = | ||
| state.handle.workspaceId === workspaceId && | ||
| state.handle.absolutePath === absolutePath; | ||
| const handle = handleAlreadyPointsAtNewPath | ||
| ? state.handle | ||
| : acquireDocument(workspaceId, absolutePath, trpcClient); | ||
| setState({ handle, workspaceId, absolutePath }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React 18 concurrent rendering side effects during render body safety
💡 Result:
In React 18 concurrent rendering, side effects in the render body (component function body or hook calls) are unsafe and must be avoided. Components and hooks must remain pure: given the same inputs (props, state, context), they must always return the same JSX without mutating external state, objects, or variables. Concurrent rendering is interruptible—React may pause, resume, or discard in-progress renders for higher-priority updates (e.g., user input). Side effects during render (e.g., incrementing globals, DOM mutations, API calls, subscriptions) can cause bugs like duplicate executions, memory leaks, or inconsistent UI, as renders aren't guaranteed to commit to the DOM. Key rules from official docs: - Render phase (component bodies, render methods) must be pure—no side effects. - Side effects belong in event handlers (preferred) or useEffect/useLayoutEffect (last resort). - Props/state/context are read-only snapshots—never mutate them. - StrictMode double-invokes render functions in dev to detect impurities. Examples of unsafe render side effects: let count = 0; // Global mutation function MyComponent { count++; // Unsafe: increments on every render attempt return
Count: {count}
; } Safe alternatives: function MyComponent({ items }) { const [filter, setFilter] = useState(''); const filtered = useMemo( => items.filter(item => item.includes(filter)), [items, filter]); return ( <input value={filter} onChange={e => setFilter(e.target.value)} /> {filtered.map(item => {item}
)} ); } Concurrent features (createRoot, transitions, Suspense) opt into this behavior, but purity is required everywhere. Violations surface as warnings in StrictMode or inconsistent behavior in production.
Citations:
- 1: https://beta.reactjs.org/learn/keeping-components-pure
- 2: https://18.react.dev/reference/rules/components-and-hooks-must-be-pure
- 3: https://react.dev/reference/rules/components-and-hooks-must-be-pure
- 4: https://react.dev/learn/keeping-components-pure
- 5: https://beta.reactjs.org/blog/2022/03/29/react-v18
- 6: https://react.dev/blog/2022/03/29/react-v18
- 7: https://legacy.reactjs.org/docs/strict-mode.html
- 8: [On hold] React Concurrent mode: avoid side effects in render mobxjs/mobx-react-lite#53
🏁 Script executed:
fd -t f "useSharedFileDocument.ts" apps/desktop/Repository: superset-sh/superset
Length of output: 197
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts"Repository: superset-sh/superset
Length of output: 2467
🏁 Script executed:
# Find the acquireDocument and releaseDocument implementations
rg -t ts "export.*function.*(acquireDocument|releaseDocument)" apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/Repository: superset-sh/superset
Length of output: 389
🏁 Script executed:
# Broader search in the fileDocumentStore directory
fd -t f "." "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/state/fileDocumentStore/" | head -20Repository: superset-sh/superset
Length of output: 702
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/state/fileDocumentStore/fileDocumentStore.ts" | head -150Repository: superset-sh/superset
Length of output: 4890
🏁 Script executed:
# Get the full file size first
wc -l "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/state/fileDocumentStore/fileDocumentStore.ts"Repository: superset-sh/superset
Length of output: 197
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/state/fileDocumentStore/fileDocumentStore.ts" | sed -n '200,300p'Repository: superset-sh/superset
Length of output: 3326
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/state/fileDocumentStore/fileDocumentStore.ts" | sed -n '300,411p'Repository: superset-sh/superset
Length of output: 3852
🏁 Script executed:
# Check if there are tests for this hook
fd -t f "useSharedFileDocument" apps/desktop/ --type fRepository: superset-sh/superset
Length of output: 197
🏁 Script executed:
# Search for any usages of useSharedFileDocument to understand caller expectations
rg "useSharedFileDocument" apps/desktop/src/renderer --type ts --type tsx -A 3 | head -50Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check if there's any discussion or known issues about refCount balancing
rg "refCount" apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace --type ts --type tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for usages of useSharedFileDocument
rg "useSharedFileDocument" apps/desktop/src/renderer -A 3 | head -50Repository: superset-sh/superset
Length of output: 5438
🏁 Script executed:
# Check the refCount logic more carefully with context
rg "refCount" apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace -B 2 -A 2Repository: superset-sh/superset
Length of output: 4237
🏁 Script executed:
# Let me also check how React handles setState-during-render in terms of whether it actually discards the render
web_search
React setState during render committed render discard consequencesRepository: superset-sh/superset
Length of output: 158
🌐 Web query:
React setState during render committed render discard consequences
💡 Result:
Calling setState during render in React is strongly discouraged and leads to serious issues. Key consequences: - Unconditional setState in render triggers an additional render before the current render completes, creating an infinite loop that crashes the app (official React docs and ESLint rule). - React issues a warning: "Cannot update during an existing state transition (such as within render or another component's constructor). Render methods should be a pure function of props and state." - The render phase must be pure; side effects like setState cause unpredictable behavior. Regarding "committed render discard": - React's render process has two phases: Render (reconciliation, pure computation of new tree) and Commit (apply changes to DOM). - setState queues an update for after the current render/commit cycle. - If unconditional, the new render happens immediately after, but since it's during render, it interrupts, leading to loops. - No explicit "committed render discard" for setState in render; instead, it causes re-renders or warnings. In Strict Mode, renders may be intentionally discarded to detect side effects, revealing such bugs. - The ESLint rule explicitly notes it "triggers another render before the current one finishes." Best practices: - Keep render pure: derive values, don't set state. - Sync state to props conditionally using previous state refs (e.g., useEffect or updater functions). - Move logic to event handlers or useReducer for complex updates. - Use updater functions (setState(prev => ...)) for batched updates. Examples from docs: Invalid (infinite loop): function Bad({value}) { const [count, setCount] = useState(0); setCount(value); // Triggers loop return
{count}
; } Valid: function Good({max}) { const [count, setCount] = useState(0); const increment = => setCount(c => Math.min(c + 1, max)); return {count}; } Or conditional sync: const [prevItems, setPrevItems] = useState(items); if (items !== prevItems) { setPrevItems(items); // ... } This behavior is consistent across React 16+ with Fiber architecture.
Citations:
- 1: https://react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-render
- 2: https://beta.reactjs.org/reference/eslint-plugin-react-hooks/lints/set-state-in-render
- 3: https://uk.react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-render
- 4: https://beta.reactjs.org/learn/render-and-commit
- 5: http://react.dev/learn/render-and-commit
- 6: https://react.dev/learn/render-and-commit
- 7: https://stackoverflow.com/questions/37292874/cannot-update-during-an-existing-state-transition-not-using-any-illegal-setsta
- 8: https://stackoverflow.com/questions/42745176/cannot-update-during-an-existing-state-transition-error-in-react
- 9: https://www.codewall.co.uk/diagnosing-reacts-cannot-update-during-an-existing-state-transition-error/
- 10: http://react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-render
Move acquireDocument from render body to a key-driven useEffect to prevent refcount leaks under concurrent rendering.
React's render phase must be pure; calling acquireDocument (which mutates the store's refCount) during render violates this rule. In concurrent mode or StrictMode, a render can be discarded before commit, leaving the bumped refCount with no paired release in the cleanup effect—leaking one lease per discarded render. The setState-during-render pattern on L31–47 is unsafe even though it restarts the render synchronously; in concurrent scenarios, the restart can be interrupted and abandoned.
Move the conditional acquire/release into a single useEffect keyed by [workspaceId, absolutePath, trpcClient]. Accept one commit of latency before the first handle is available—callers already tolerate this via content.kind === "loading":
Suggested refactor
- const [state, setState] = useState<{
- handle: SharedFileDocument;
- workspaceId: string;
- absolutePath: string;
- }>(() => ({
- handle: acquireDocument(workspaceId, absolutePath, trpcClient),
- workspaceId,
- absolutePath,
- }));
-
- if (
- state.workspaceId !== workspaceId ||
- state.absolutePath !== absolutePath
- ) {
- const handleAlreadyPointsAtNewPath =
- state.handle.workspaceId === workspaceId &&
- state.handle.absolutePath === absolutePath;
- const handle = handleAlreadyPointsAtNewPath
- ? state.handle
- : acquireDocument(workspaceId, absolutePath, trpcClient);
- setState({ handle, workspaceId, absolutePath });
- }
-
- useEffect(() => {
- return () => {
- releaseDocument(workspaceId, absolutePath);
- };
- }, [workspaceId, absolutePath]);
+ const [handle, setHandle] = useState<SharedFileDocument | null>(null);
+ useEffect(() => {
+ const acquired = acquireDocument(workspaceId, absolutePath, trpcClient);
+ setHandle(acquired);
+ return () => releaseDocument(workspaceId, absolutePath);
+ }, [workspaceId, absolutePath, trpcClient]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( | |
| state.workspaceId !== workspaceId || | |
| state.absolutePath !== absolutePath | |
| ) { | |
| // Rename case: the entry behind our existing handle was migrated to | |
| // match the new props. Reuse the handle — acquiring again would bump | |
| // refCount a second time and release() of the old key no-ops (the | |
| // entry isn't at that key anymore), which would leak one lease per | |
| // rename. | |
| const handleAlreadyPointsAtNewPath = | |
| state.handle.workspaceId === workspaceId && | |
| state.handle.absolutePath === absolutePath; | |
| const handle = handleAlreadyPointsAtNewPath | |
| ? state.handle | |
| : acquireDocument(workspaceId, absolutePath, trpcClient); | |
| setState({ handle, workspaceId, absolutePath }); | |
| } | |
| const [handle, setHandle] = useState<SharedFileDocument | null>(null); | |
| useEffect(() => { | |
| const acquired = acquireDocument(workspaceId, absolutePath, trpcClient); | |
| setHandle(acquired); | |
| return () => releaseDocument(workspaceId, absolutePath); | |
| }, [workspaceId, absolutePath, trpcClient]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts
around lines 31 - 47, The current code calls acquireDocument during render
(inside the conditional that sets setState), which can bump refCount during a
render that may be discarded and leak leases; move the acquire/release logic
into a key-driven useEffect keyed on [workspaceId, absolutePath, trpcClient],
keeping the existing handleAlreadyPointsAtNewPath check but calling
acquireDocument(workspaceId, absolutePath, trpcClient) inside the effect, then
setState({ handle, workspaceId, absolutePath }) when acquisition completes, and
in the effect cleanup release the previously held handle (call
previousHandle.release()) to ensure refCount is decremented if the effect is
torn down or dependencies change.
| theme.ui.foreground, | ||
| theme.type === "dark" ? 0.04 : 0.06, | ||
| ), | ||
| activeLine: withAlpha(theme.ui.accent, 0.5), |
Contributor
There was a problem hiding this comment.
Verify activeLine at 0.5 alpha of the accent color isn't too opaque.
Previously activeLine was derived from foreground with a much smaller alpha; switching to withAlpha(theme.ui.accent, 0.5) produces a noticeably stronger tint that can wash out syntax colors on the caret line and, for themes where ui.accent is a saturated color, may visibly tint text through the highlight. Worth confirming against all built-in themes (light, dark, monokai) and any user-imported themes where accent is vivid. A smaller alpha (e.g. 0.15–0.25) is more typical for active-line highlights.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/shared/themes/editor-theme.ts` at line 18, activeLine is
currently set to withAlpha(theme.ui.accent, 0.5) which can be too opaque for
caret-line highlighting; change the alpha to a lower value (suggest 0.15–0.25)
and re-test activeLine across all built-in themes (light, dark, monokai) and any
user-imported themes to ensure the caret line does not wash out syntax colors or
tint text; update the value used in the activeLine assignment (the withAlpha
call referencing theme.ui.accent) and, if needed, make the alpha configurable or
theme-dependent to preserve readability for saturated accents.
Greptile SummaryThis PR builds the v2 file editor for the desktop app on a new refcounted The architecture is solid — the module-level Key changes:
Issue found:
Confidence Score: 4/5Safe to merge after fixing the save-overwrites-edits bug in fileDocumentStore.ts; all other machinery is well-designed. The architecture (refcounting store, useSyncExternalStore, rename tracking, conflict UX) is solid. One P1 correctness bug was found: save() resets entry.content.value to the value captured at save-start, silently discarding any edits the user typed during the async round-trip. The fix is a targeted 3-line change. Two P2 issues are non-blocking. No security concerns. apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts — specifically the save() success branch at lines 234–239.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts | Core store with refcounting and fs:events dispatch — solid overall, but save() overwrites entry.content.value with the stale captured value after a successful write, discarding edits typed during the async save round-trip (P1 bug at lines 234–239). |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts | Correctly handles the rename edge-case by detecting when the underlying entry has already migrated its path, reusing the handle rather than double-acquiring. useSyncExternalStore wiring is correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsx | Conflict alert, orphan/save-error banners, and rename-follow logic look correct; view resolution delegates cleanly to resolveActivePaneView. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx | onBeforeClose dirty-buffer alert correctly uses getDocument (not the hook) to avoid acquiring an extra reference at close time; Save/Don't Save/Cancel semantics look correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx | Checkerboard underlay and data-URL construction are correct; btoa(Array.from(bytes).map(...).join("")) works but builds a large intermediate string — may be slow for near-10 MB images. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/resolveViews.ts | View resolution logic mirrors VS Code's priority model correctly; exclusive views short-circuit, tie-breaking by stable ALL_VIEWS order is clear. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/FileDocumentStoreProvider.tsx | Minimal provider that bridges useWorkspaceEvent("fs:events") into dispatchFsEvent — correct and clean. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/MarkdownPreviewView/MarkdownPreviewView.tsx | Read-only preview passes original markdown content straight to TipTap, preventing reformatting; onSave wires Cmd+S correctly from the preview pane. |
| packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx | Module-level workspaceClientsCache Map correctly shares QueryClient and tRPC clients across re-mounts of the same workspace; bounded by the number of unique workspace host URLs. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | FileDocumentStoreProvider wraps the workspace correctly; onBeforeCloseTab iterates dirty panes via getDocument (not the hook) for Save All / Don't Save flow. |
Sequence Diagram
sequenceDiagram
participant User
participant CodeEditor
participant useSharedFileDocument
participant fileDocumentStore
participant WorkspaceFS
User->>CodeEditor: opens file pane
CodeEditor->>useSharedFileDocument: acquireDocument(workspaceId, path)
useSharedFileDocument->>fileDocumentStore: creates entry (refCount=1)
fileDocumentStore->>WorkspaceFS: readFile(path, utf-8)
WorkspaceFS-->>fileDocumentStore: content + revision
fileDocumentStore-->>CodeEditor: entry.content = text
User->>CodeEditor: types edits
CodeEditor->>fileDocumentStore: setContent(newValue)
fileDocumentStore->>fileDocumentStore: entry.content.value = newValue
User->>CodeEditor: Cmd+S
CodeEditor->>fileDocumentStore: save()
Note over fileDocumentStore: currentValue captured here
fileDocumentStore->>WorkspaceFS: writeFile(currentValue, ifMatch=revision)
User->>CodeEditor: types more during save
CodeEditor->>fileDocumentStore: setContent(newValue + extra)
fileDocumentStore->>fileDocumentStore: entry.content.value = newValue+extra
WorkspaceFS-->>fileDocumentStore: ok=true, revision=rev2
Note over fileDocumentStore: BUG - entry.content.value = currentValue (loses extra)
fileDocumentStore->>fileDocumentStore: notify
fileDocumentStore-->>CodeEditor: re-render value=currentValue
CodeEditor->>CodeEditor: sync effect resets editor, extra edits lost
Note over User,WorkspaceFS: Rename scenario
WorkspaceFS->>fileDocumentStore: fs rename event old to new
fileDocumentStore->>fileDocumentStore: migrate entry key to new path
fileDocumentStore->>fileDocumentStore: notify
CodeEditor->>useSharedFileDocument: absolutePath changed, updateData
useSharedFileDocument->>useSharedFileDocument: handleAlreadyPointsAtNewPath=true, reuse handle
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts
Line: 234-239
Comment:
**Edits typed during an in-flight save are silently discarded**
`currentValue` is captured at the top of `save()` (line 196). If the user types while the async `writeFile` round-trip is in progress, `setContent()` has already updated `entry.content.value` to newer text. When the save completes, this block overwrites `entry.content.value` back to the stale captured value and calls `notify()`. The subsequent re-render delivers the old value to `CodeEditor` as its `value` prop; the sync effect detects a mismatch against the CodeMirror document and dispatches a replace-all, permanently losing the intermediate edits.
The fix is to preserve the current editor content and only advance the `revision`:
```suggestion
// Only update the revision — the editor may have accumulated new edits
// since the save started; overwriting value would discard them.
if (entry.content.kind === "text") {
entry.content = {
...entry.content,
revision: result.revision,
};
}
entry.savedContentText = currentValue;
```
`savedContentText` still tracks what's on disk, so `computeDirty` correctly reflects any new edits as unsaved.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx
Line: 9-14
Comment:
**`btoa` via `Array.from + map + join` is O(n) allocations**
For a near-10 MB image (the default limit), this creates a 10 M-element array of single-character strings before joining, which is GC-heavy and can cause noticeable jank on the main thread. In Electron's renderer process, `Buffer` is available, which converts in one native call:
```suggestion
const bytes = document.content.value;
const mimeType = getImageMimeType(filePath) ?? "image/png";
let base64: string;
if (typeof Buffer !== "undefined") {
base64 = Buffer.from(bytes).toString("base64");
} else {
const binary = Array.from(bytes, (b) => String.fromCharCode(b)).join("");
base64 = btoa(binary);
}
return `data:${mimeType};base64,${base64}`;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts
Line: 49-53
Comment:
**Cleanup captures `absolutePath` from the outer closure, not the current render's path**
After a rename the pane's `filePath` prop changes from old → new. React will run the old effect's cleanup with the stale closure values (`workspaceId`, old `absolutePath`), calling `releaseDocument(workspaceId, oldPath)`. Because `dispatchFsEvent` already migrated the entry to the new key, `entries.get(oldKey)` returns `undefined` and the decrement is silently skipped — the refCount for the live entry is never decremented from that original acquire. The entry will still be released when the component unmounts (the new effect captures `newPath`), but only after the path changed twice via rename; a second rename before unmount would leave another dangling ref.
Consider tracking the handle's current path in state and releasing against it in the cleanup:
```ts
useEffect(() => {
const { handle } = state;
return () => {
releaseDocument(handle.workspaceId, handle.absolutePath);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.handle]); // release whichever handle we currently hold
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(desktop): make v2 markdown preview ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+234
to
+239
| entry.content = { | ||
| kind: "text", | ||
| value: currentValue, | ||
| revision: result.revision, | ||
| }; | ||
| entry.savedContentText = currentValue; |
There was a problem hiding this comment.
currentValue is captured at the top of save() (line 196). If the user types while the async writeFile round-trip is in progress, setContent() has already updated entry.content.value to newer text. When the save completes, this block overwrites entry.content.value back to the stale captured value and calls notify(). The subsequent re-render delivers the old value to CodeEditor as its value prop; the sync effect detects a mismatch against the CodeMirror document and dispatches a replace-all, permanently losing the intermediate edits.
The fix is to preserve the current editor content and only advance the revision:
Suggested change
| entry.content = { | |
| kind: "text", | |
| value: currentValue, | |
| revision: result.revision, | |
| }; | |
| entry.savedContentText = currentValue; | |
| // Only update the revision — the editor may have accumulated new edits | |
| // since the save started; overwriting value would discard them. | |
| if (entry.content.kind === "text") { | |
| entry.content = { | |
| ...entry.content, | |
| revision: result.revision, | |
| }; | |
| } | |
| entry.savedContentText = currentValue; |
savedContentText still tracks what's on disk, so computeDirty correctly reflects any new edits as unsaved.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts
Line: 234-239
Comment:
**Edits typed during an in-flight save are silently discarded**
`currentValue` is captured at the top of `save()` (line 196). If the user types while the async `writeFile` round-trip is in progress, `setContent()` has already updated `entry.content.value` to newer text. When the save completes, this block overwrites `entry.content.value` back to the stale captured value and calls `notify()`. The subsequent re-render delivers the old value to `CodeEditor` as its `value` prop; the sync effect detects a mismatch against the CodeMirror document and dispatches a replace-all, permanently losing the intermediate edits.
The fix is to preserve the current editor content and only advance the `revision`:
```suggestion
// Only update the revision — the editor may have accumulated new edits
// since the save started; overwriting value would discard them.
if (entry.content.kind === "text") {
entry.content = {
...entry.content,
revision: result.revision,
};
}
entry.savedContentText = currentValue;
```
`savedContentText` still tracks what's on disk, so `computeDirty` correctly reflects any new edits as unsaved.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+9
to
+14
| const base64 = btoa( | ||
| Array.from(document.content.value) | ||
| .map((b) => String.fromCharCode(b)) | ||
| .join(""), | ||
| ); | ||
| return `data:${mimeType};base64,${base64}`; |
There was a problem hiding this comment.
btoa via Array.from + map + join is O(n) allocations
For a near-10 MB image (the default limit), this creates a 10 M-element array of single-character strings before joining, which is GC-heavy and can cause noticeable jank on the main thread. In Electron's renderer process, Buffer is available, which converts in one native call:
Suggested change
| const base64 = btoa( | |
| Array.from(document.content.value) | |
| .map((b) => String.fromCharCode(b)) | |
| .join(""), | |
| ); | |
| return `data:${mimeType};base64,${base64}`; | |
| const bytes = document.content.value; | |
| const mimeType = getImageMimeType(filePath) ?? "image/png"; | |
| let base64: string; | |
| if (typeof Buffer !== "undefined") { | |
| base64 = Buffer.from(bytes).toString("base64"); | |
| } else { | |
| const binary = Array.from(bytes, (b) => String.fromCharCode(b)).join(""); | |
| base64 = btoa(binary); | |
| } | |
| return `data:${mimeType};base64,${base64}`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx
Line: 9-14
Comment:
**`btoa` via `Array.from + map + join` is O(n) allocations**
For a near-10 MB image (the default limit), this creates a 10 M-element array of single-character strings before joining, which is GC-heavy and can cause noticeable jank on the main thread. In Electron's renderer process, `Buffer` is available, which converts in one native call:
```suggestion
const bytes = document.content.value;
const mimeType = getImageMimeType(filePath) ?? "image/png";
let base64: string;
if (typeof Buffer !== "undefined") {
base64 = Buffer.from(bytes).toString("base64");
} else {
const binary = Array.from(bytes, (b) => String.fromCharCode(b)).join("");
base64 = btoa(binary);
}
return `data:${mimeType};base64,${base64}`;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+49
to
+53
| useEffect(() => { | ||
| return () => { | ||
| releaseDocument(workspaceId, absolutePath); | ||
| }; | ||
| }, [workspaceId, absolutePath]); |
There was a problem hiding this comment.
absolutePath from the outer closure, not the current render's path
After a rename the pane's filePath prop changes from old → new. React will run the old effect's cleanup with the stale closure values (workspaceId, old absolutePath), calling releaseDocument(workspaceId, oldPath). Because dispatchFsEvent already migrated the entry to the new key, entries.get(oldKey) returns undefined and the decrement is silently skipped — the refCount for the live entry is never decremented from that original acquire. The entry will still be released when the component unmounts (the new effect captures newPath), but only after the path changed twice via rename; a second rename before unmount would leave another dangling ref.
Consider tracking the handle's current path in state and releasing against it in the cleanup:
useEffect(() => {
const { handle } = state;
return () => {
releaseDocument(handle.workspaceId, handle.absolutePath);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.handle]); // release whichever handle we currently holdPrompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts
Line: 49-53
Comment:
**Cleanup captures `absolutePath` from the outer closure, not the current render's path**
After a rename the pane's `filePath` prop changes from old → new. React will run the old effect's cleanup with the stale closure values (`workspaceId`, old `absolutePath`), calling `releaseDocument(workspaceId, oldPath)`. Because `dispatchFsEvent` already migrated the entry to the new key, `entries.get(oldKey)` returns `undefined` and the decrement is silently skipped — the refCount for the live entry is never decremented from that original acquire. The entry will still be released when the component unmounts (the new effect captures `newPath`), but only after the path changed twice via rename; a second rename before unmount would leave another dangling ref.
Consider tracking the handle's current path in state and releasing against it in the cleanup:
```ts
useEffect(() => {
const { handle } = state;
return () => {
releaseDocument(handle.workspaceId, handle.absolutePath);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.handle]); // release whichever handle we currently hold
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritises the most important files to review.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsx:47">
P2: Use a cross-platform basename extraction instead of `split("/")` so Windows paths render the correct filename.
(Based on your team's feedback about using cross-platform path utilities instead of manual split parsing.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/plans/20260412-file-editor-v2-implementation.md">
<violation number="1" location="apps/desktop/plans/20260412-file-editor-v2-implementation.md:425">
P2: The spec has contradictory orphan-detection guidance (`no debounced probe` vs `100ms debounce`), which can cause inconsistent implementation of delete/orphan handling.</violation>
<violation number="2" location="apps/desktop/plans/20260412-file-editor-v2-implementation.md:538">
P1: `hasChanges` is driven by view callbacks instead of `document.dirty`, so tab dirty state can become stale after document-level transitions like save/discard/reload.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts:108">
P1: Save overwrites the live buffer with the pre-save snapshot, losing any edits the user made while the async write was in flight. Preserve `entry.content.value` (the current buffer) and only update the revision and `savedContentText`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/utils/resolveActivePaneView/resolveActivePaneView.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/utils/resolveActivePaneView/resolveActivePaneView.ts:26">
P2: Handle unknown `forceViewId` with a fallback; otherwise stale pane data can produce no views and incorrectly render `binary-unsupported`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts:22">
P1: `acquireDocument` is being called during render, which introduces render-phase side effects and can leak ref-counted document handles when renders are aborted or double-invoked.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/loadLanguageSupport/loadLanguageSupport.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/loadLanguageSupport/loadLanguageSupport.ts:40">
P2: `scss` and `less` currently load the plain CSS parser, which breaks dialect-specific syntax support. Route those cases to SCSS/LESS parsers instead of always returning `css()`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:81">
P2: Wrap the awaited `onBeforeClose` call in `try/catch` so a rejected close-guard promise does not become an unhandled rejection.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:179">
P1: Do not treat a missing document handle as "clean" in `onBeforeClose`; this can bypass the dirty prompt during rename races and close with unsaved changes.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:367">
P2: Use a cross-platform basename extraction instead of `split("/")` so Windows paths render correct file names in the close-confirmation alert.
(Based on your team's feedback about using cross-platform path utilities instead of manual `split`.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| [context.actions, data], | ||
| ); | ||
| const handleDirtyChange = useCallback( |
Contributor
There was a problem hiding this comment.
P1: hasChanges is driven by view callbacks instead of document.dirty, so tab dirty state can become stale after document-level transitions like save/discard/reload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260412-file-editor-v2-implementation.md, line 538:
<comment>`hasChanges` is driven by view callbacks instead of `document.dirty`, so tab dirty state can become stale after document-level transitions like save/discard/reload.</comment>
<file context>
@@ -0,0 +1,744 @@
+ },
+ [context.actions, data],
+ );
+ const handleDirtyChange = useCallback(
+ (dirty: boolean) => {
+ if (dirty !== data.hasChanges) {
</file context>
|
|
||
| if (result.kind === "bytes") { | ||
| entry.isBinary = true; | ||
| entry.content = { |
Contributor
There was a problem hiding this comment.
P1: Save overwrites the live buffer with the pre-save snapshot, losing any edits the user made while the async write was in flight. Preserve entry.content.value (the current buffer) and only update the revision and savedContentText.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts, line 108:
<comment>Save overwrites the live buffer with the pre-save snapshot, losing any edits the user made while the async write was in flight. Preserve `entry.content.value` (the current buffer) and only update the revision and `savedContentText`.</comment>
<file context>
@@ -0,0 +1,411 @@
+
+ if (result.kind === "bytes") {
+ entry.isBinary = true;
+ entry.content = {
+ kind: "bytes",
+ value: toBytes(result.content),
</file context>
| workspaceId: string; | ||
| absolutePath: string; | ||
| }>(() => ({ | ||
| handle: acquireDocument(workspaceId, absolutePath, trpcClient), |
Contributor
There was a problem hiding this comment.
P1: acquireDocument is being called during render, which introduces render-phase side effects and can leak ref-counted document handles when renders are aborted or double-invoked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/useSharedFileDocument.ts, line 22:
<comment>`acquireDocument` is being called during render, which introduces render-phase side effects and can leak ref-counted document handles when renders are aborted or double-invoked.</comment>
<file context>
@@ -0,0 +1,62 @@
+ workspaceId: string;
+ absolutePath: string;
+ }>(() => ({
+ handle: acquireDocument(workspaceId, absolutePath, trpcClient),
+ workspaceId,
+ absolutePath,
</file context>
| const data = pane.data as FilePaneData; | ||
| if (!data.hasChanges) return true; | ||
| const doc = getDocument(workspaceId, data.filePath); | ||
| if (!doc?.dirty) return true; |
Contributor
There was a problem hiding this comment.
P1: Do not treat a missing document handle as "clean" in onBeforeClose; this can bypass the dirty prompt during rename races and close with unsaved changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx, line 179:
<comment>Do not treat a missing document handle as "clean" in `onBeforeClose`; this can bypass the dirty prompt during rename races and close with unsaved changes.</comment>
<file context>
@@ -127,26 +157,26 @@ export function usePaneRegistry(
const data = pane.data as FilePaneData;
- if (!data.hasChanges) return true;
+ const doc = getDocument(workspaceId, data.filePath);
+ if (!doc?.dirty) return true;
const name = data.filePath.split("/").pop();
return new Promise<boolean>((resolve) => {
</file context>
Suggested change
| if (!doc?.dirty) return true; | |
| if (!doc) return false; | |
| if (!doc.dirty) return true; |
| const hasConflict = document.conflict !== null; | ||
| useEffect(() => { | ||
| if (!hasConflict) return; | ||
| const name = filePath.split("/").pop(); |
Contributor
There was a problem hiding this comment.
P2: Use a cross-platform basename extraction instead of split("/") so Windows paths render the correct filename.
(Based on your team's feedback about using cross-platform path utilities instead of manual split parsing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsx, line 47:
<comment>Use a cross-platform basename extraction instead of `split("/")` so Windows paths render the correct filename.
(Based on your team's feedback about using cross-platform path utilities instead of manual split parsing.) </comment>
<file context>
@@ -16,91 +18,129 @@ export function FilePane({ context, workspaceId }: FilePaneProps) {
+ const hasConflict = document.conflict !== null;
+ useEffect(() => {
+ if (!hasConflict) return;
+ const name = filePath.split("/").pop();
+ alert({
+ title: `Do you want to save the changes you made to ${name}?`,
</file context>
Suggested change
| const name = filePath.split("/").pop(); | |
| const name = filePath.split(/[\\/]/).pop(); |
| - ETag / revision tracking | ||
| - Dirty detection (`currentContent !== savedContent`) | ||
| - External-change detection (fs:events subscription) | ||
| - Orphan detection (delete probe with 100ms debounce) |
Contributor
There was a problem hiding this comment.
P2: The spec has contradictory orphan-detection guidance (no debounced probe vs 100ms debounce), which can cause inconsistent implementation of delete/orphan handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260412-file-editor-v2-implementation.md, line 425:
<comment>The spec has contradictory orphan-detection guidance (`no debounced probe` vs `100ms debounce`), which can cause inconsistent implementation of delete/orphan handling.</comment>
<file context>
@@ -0,0 +1,744 @@
+- ETag / revision tracking
+- Dirty detection (`currentContent !== savedContent`)
+- External-change detection (fs:events subscription)
+- Orphan detection (delete probe with 100ms debounce)
+- Save state machine (`dirty` → `pendingSave` → `saved` | `saveError` | `conflict`)
+- Refcount + lifetime rules (dispose blocked on dirty/orphaned)
</file context>
| isBinary: document.isBinary ?? undefined, | ||
| }; | ||
| const views = data.forceViewId | ||
| ? ALL_VIEWS.filter((v) => v.id === data.forceViewId) |
Contributor
There was a problem hiding this comment.
P2: Handle unknown forceViewId with a fallback; otherwise stale pane data can produce no views and incorrectly render binary-unsupported.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/utils/resolveActivePaneView/resolveActivePaneView.ts, line 26:
<comment>Handle unknown `forceViewId` with a fallback; otherwise stale pane data can produce no views and incorrectly render `binary-unsupported`.</comment>
<file context>
@@ -0,0 +1,31 @@
+ isBinary: document.isBinary ?? undefined,
+ };
+ const views = data.forceViewId
+ ? ALL_VIEWS.filter((v) => v.id === data.forceViewId)
+ : resolveViews(data.filePath, meta);
+ const activeView =
</file context>
| case "scss": | ||
| case "less": { | ||
| const { css } = await import("@codemirror/lang-css"); | ||
| return css(); |
Contributor
There was a problem hiding this comment.
P2: scss and less currently load the plain CSS parser, which breaks dialect-specific syntax support. Route those cases to SCSS/LESS parsers instead of always returning css().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/loadLanguageSupport/loadLanguageSupport.ts, line 40:
<comment>`scss` and `less` currently load the plain CSS parser, which breaks dialect-specific syntax support. Route those cases to SCSS/LESS parsers instead of always returning `css()`.</comment>
<file context>
@@ -0,0 +1,127 @@
+ case "scss":
+ case "less": {
+ const { css } = await import("@codemirror/lang-css");
+ return css();
+ }
+ case "markdown": {
</file context>
| return getDocument(workspaceId, filePath)?.dirty === true; | ||
| }); | ||
| const dirtyFileNames = dirtyPanes.map((p) => | ||
| (p.data as FilePaneData).filePath.split("/").pop(), |
Contributor
There was a problem hiding this comment.
P2: Use a cross-platform basename extraction instead of split("/") so Windows paths render correct file names in the close-confirmation alert.
(Based on your team's feedback about using cross-platform path utilities instead of manual split.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx, line 367:
<comment>Use a cross-platform basename extraction instead of `split("/")` so Windows paths render correct file names in the close-confirmation alert.
(Based on your team's feedback about using cross-platform path utilities instead of manual `split`.) </comment>
<file context>
@@ -350,19 +358,19 @@ function WorkspaceContent({
+ return getDocument(workspaceId, filePath)?.dirty === true;
+ });
+ const dirtyFileNames = dirtyPanes.map((p) =>
+ (p.data as FilePaneData).filePath.split("/").pop(),
+ );
+ if (dirtyPanes.length === 0) return true;
</file context>
Suggested change
| (p.data as FilePaneData).filePath.split("/").pop(), | |
| (p.data as FilePaneData).filePath.split(/[\\/]/).pop(), |
- save(): preserve live buffer when keystrokes arrive during an
in-flight write — only bump revision/savedContentText on success.
- loadEntry: distinguish ENOENT (not-found) from transient/permission
errors; new { kind: "error" } content state + load-failed ErrorState
with a Retry button.
- resetForLoad helper shared by reload() and loadUnlimited() so both
clear conflict/hasExternalChange/saveError consistently.
- useCopyToClipboard: navigator.clipboard primary + offscreen-textarea
+ execCommand fallback (matches VS Code's browserClipboardService);
preserves caller's selection range.
- PathActionsMenuItems uses toast.promise for copy feedback.
- CLOSE_PANE hotkey guarded with isClosingPaneRef against re-entrant
prompts when Cmd+W is pressed rapidly.
- ImageView switches from btoa(base64) data URLs to
URL.createObjectURL(Blob) with cleanup; ~3× lower peak memory.
- CodeEditorAdapter cut/paste async callbacks no-op after dispose.
- MarkdownPreviewView drops the now-dead onSave prop.
- Active-line and selection backgrounds share a theme.ui.accent @
0.5 alpha so the selection is actually visible against the theme.
- setup/steps.sh: Electric container now uses --restart on-failure:5
so a container stuck on a Neon session orphan doesn't loop
indefinitely without the setup-script cleanup path running again.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx (1)
37-37: Minor:altderivation won't handle Windows-style paths.
filePath.split("/").pop()returns the full path whenfilePathuses backslash separators (e.g.,C:\foo\bar.png), yielding a less usefulalttext. Consider splitting on/[\\/]/to cover both separators.Proposed tweak
- alt={filePath.split("/").pop() ?? ""} + alt={filePath.split(/[\\/]/).pop() ?? ""}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx at line 37, The alt text derivation in ImageView.tsx uses filePath.split("/").pop(), which fails for Windows backslashes; update the alt extraction used for the img alt prop (the filePath variable in the ImageView component) to handle both separators by splitting on a regex like /[\\/]/ or by using a path basename utility (e.g., path.basename or a small helper) so the alt contains only the filename regardless of OS path separators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsx:
- Line 37: The alt text derivation in ImageView.tsx uses
filePath.split("/").pop(), which fails for Windows backslashes; update the alt
extraction used for the img alt prop (the filePath variable in the ImageView
component) to handle both separators by splitting on a regex like /[\\/]/ or by
using a path basename utility (e.g., path.basename or a small helper) so the alt
contains only the filename regardless of OS path separators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08874eb2-ccf7-4880-b51d-fe864dcbf7e3
📒 Files selected for processing (12)
.superset/lib/setup/steps.shapps/desktop/src/renderer/hooks/useCopyToClipboard.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/components/WorkspaceFilesTreeItem/components/PathActionsMenuItems/PathActionsMenuItems.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ErrorState/ErrorState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/createCodeMirrorTheme/createCodeMirrorTheme.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/ImageView/ImageView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/MarkdownPreviewView/MarkdownPreviewView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/types.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/MarkdownPreviewView/MarkdownPreviewView.tsx
- apps/desktop/src/renderer/hooks/useCopyToClipboard.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/components/ErrorState/ErrorState.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/createCodeMirrorTheme/createCodeMirrorTheme.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/registry/views/CodeView/components/CodeEditor/CodeEditorAdapter/CodeEditorAdapter.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/FilePane/FilePane.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/state/fileDocumentStore/fileDocumentStore.ts
Contributor
There was a problem hiding this comment.
2 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/hooks/useCopyToClipboard.ts">
<violation number="1" location="apps/desktop/src/renderer/hooks/useCopyToClipboard.ts:7">
P2: Do not silently swallow clipboard API failures; handle the caught error before falling back so copy failures remain observable.
(Based on your team's feedback about avoiding silent catch blocks for async failures.) [FEEDBACK_USED]</violation>
</file>
<file name=".superset/lib/setup/steps.sh">
<violation number="1" location=".superset/lib/setup/steps.sh:262">
P2: `on-failure:5` weakens container persistence for a service started in one-time setup. After Docker/host restarts, Electric may stay down and break local dev until setup is rerun.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| await navigator.clipboard.writeText(text); | ||
| return; | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P2: Do not silently swallow clipboard API failures; handle the caught error before falling back so copy failures remain observable.
(Based on your team's feedback about avoiding silent catch blocks for async failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/hooks/useCopyToClipboard.ts, line 7:
<comment>Do not silently swallow clipboard API failures; handle the caught error before falling back so copy failures remain observable.
(Based on your team's feedback about avoiding silent catch blocks for async failures.) </comment>
<file context>
@@ -1,11 +1,51 @@
+ try {
+ await navigator.clipboard.writeText(text);
+ return;
+ } catch {}
+
+ const textarea = window.document.createElement("textarea");
</file context>
Suggested change
| } catch {} | |
| } catch (error) { | |
| console.warn("navigator.clipboard.writeText failed; falling back to execCommand copy", error); | |
| } |
| if ! docker run -d \ | ||
| --name "$ELECTRIC_CONTAINER" \ | ||
| --restart unless-stopped \ | ||
| --restart on-failure:5 \ |
Contributor
There was a problem hiding this comment.
P2: on-failure:5 weakens container persistence for a service started in one-time setup. After Docker/host restarts, Electric may stay down and break local dev until setup is rerun.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .superset/lib/setup/steps.sh, line 262:
<comment>`on-failure:5` weakens container persistence for a service started in one-time setup. After Docker/host restarts, Electric may stay down and break local dev until setup is rerun.</comment>
<file context>
@@ -259,7 +259,7 @@ step_start_electric() {
if ! docker run -d \
--name "$ELECTRIC_CONTAINER" \
- --restart unless-stopped \
+ --restart on-failure:5 \
$port_flag \
-e DATABASE_URL="$DIRECT_URL" \
</file context>
Suggested change
| --restart on-failure:5 \ | |
| --restart unless-stopped \ |
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
…-sh#3526) Cherry-pick PR4 slice of upstream c504a50. Scoped to the foundation layer only so existing v2 code continues to work unchanged; the registry/store is not wired yet. PR5 will do the FilePane / page.tsx / useWorkspaceHotkeys / useDefaultContextMenuActions adaptation. Scope: - Add @replit/codemirror-css-color-picker dep (CodeEditor CSS swatches) - Add `trpcClient` to WorkspaceClientContextValue so the store can acquire per-document tRPC handles - Export `withAlpha` from shared/themes (consumed by editor theme) - New v2 file editor subsystem (unwired): - state/fileDocumentStore/** — shared document store + provider + hook - hooks/usePaneRegistry/components/FilePane/registry/** — view registry (CodeView / MarkdownPreviewView / ImageView / BinaryWarningView) with CodeMirror editor stack - hooks/usePaneRegistry/components/FilePane/components/{ErrorState, FilePaneHeaderExtras, FileViewToggle, LoadingState, OrphanedBanner, SaveErrorBanner}/** — chrome building blocks - Extend FilePaneData with optional `viewId` / `forceViewId` so the new chrome can typecheck; existing `hasChanges` / `displayName` and BrowserPaneData fork fields are untouched (deferred to PR5) FORK NOTES: - upstream apps/desktop/package.json drop of fork-only deps (@xyflow/react, elkjs, exceljs, ansi_up, cron-parser, cronstrue, diff, dockerfile-*, @codemirror/merge, @taplo/lib, @superset/macos-window-blur) is NOT applied here — those deps are still in use. - upstream apps/desktop/src/shared/themes/editor-theme.ts token removals (docComment, controlKeyword, storageKeyword, escape, variableProperty, annotation, markdown*) are NOT applied here — fork's shiki-theme.ts (upstream-absent) reads them. Deferred to PR5 so editor-theme.ts and shiki-theme.ts can move together. - upstream's removal of hooks/host-service/useFileDocument/** and the old FilePane renderers/ folder is deferred to PR5, once the new registry is wired.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
…oundation upstream取り込み: v2 file editor foundation (superset-sh#3526 前半)
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
…-sh#3526, part 2) Complete the v2 file editor migration started in PR4. Wire the new fileDocumentStore + view registry into FilePane, usePaneRegistry, and page.tsx, then retire the old useFileDocument hook and the legacy FilePane renderers. Fork-specific features (SpreadsheetViewer, memo displayName, addMemoTab, SEARCH_IN_FILES / BROWSER_RELOAD hotkeys, right-sidebar width auto-adjust on file open) are preserved. Changes: * `v2-workspace/$workspaceId/page.tsx` — wrap with FileDocumentStoreProvider, switch onBeforeCloseTab to read dirty via getDocument, update useWorkspaceHotkeys() to pass paneRegistry (for onBeforeClose), relabel close-pane pane action with CLOSE_PANE. Fork features kept: openFilePane(filePath, displayName?) variant, openSidebarFilePane with rightSidebarOpenViewWidth auto-resize, addMemoTab, useCommandPalette hook, SEARCH_IN_FILES hotkey, BROWSER_RELOAD / BROWSER_HARD_RELOAD + browser-shortcut-event listener, presets bar toggle. * `v2-workspace/$workspaceId/types.ts` — drop `hasChanges`, keep fork's `displayName` and BrowserPaneData fields (`mode`, `reloadToken`, `hardReloadToken`). * `v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx` — new FilePaneTabTitle component reads dirty via useSharedFileDocument (with displayName fork fallback), renderHeaderExtras added, onBeforeClose rewritten to Save / Don't Save / Cancel flow using getDocument. * `FilePane/FilePane.tsx` — rewrite on top of useSharedFileDocument + view registry. Preserve fork SpreadsheetViewer fast-path (still bypasses the store). Memo displayName auto-derive effect kept in a fork-only useEffect that writes into FilePaneData.displayName. * `useWorkspaceHotkeys.ts` — CLOSE_TERMINAL → CLOSE_PANE with paneRegistry onBeforeClose await (upstream). * `useDefaultContextMenuActions.tsx` — close pane shortcut label now reads CLOSE_PANE. * `hotkeys/registry.ts` — SHOW_HOTKEYS moved off Cmd+/ onto Cmd+Shift+/ so CodeMirror can take Cmd+/ for the comment toggle. Fork-specific hotkeys (SEARCH_IN_FILES, BROWSER_RELOAD, BROWSER_HARD_RELOAD) kept. * `shared/themes/editor-theme.ts` — apply upstream's activeLine token change (accent at 0.5 alpha); KEEP fork's extended syntax tokens (docComment, controlKeyword, storageKeyword, escape, variableProperty, annotation, operator, punctuation, markdown*, meta) because fork-only shiki-theme.ts reads them. * `useCopyToClipboard.ts` / `PathActionsMenuItems.tsx` — switch to navigator.clipboard (upstream). * `.superset/lib/setup/steps.sh` — electric container restart policy: unless-stopped → on-failure:5 (upstream). * `WorkspaceFilePreviewContent.tsx` (fork-only FilesPane preview) — ported to useSharedFileDocument so it keeps working after the old useFileDocument is removed. * Deleted: `hooks/host-service/useFileDocument/**`, legacy `FilePane/components/ExternalChangeBar/**`, `FilePane/renderers/{CodeRenderer,ImageRenderer,MarkdownRenderer}/**`.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
…daptation upstream取り込み: v2 file editor adaptation (superset-sh#3526 後半 / FilePane registry 移行)
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
Upstream commits processed (cherry-picked, then adapted where needed): - 07c1ee0 fix(desktop): Cmd+O firing open-in twice on v1 workspace route (superset-sh#3511) → PR #302 (with fork tearoff-window adaptation for Cmd+O) - 4a1f41a chore(deps): upgrade tanstack/db + electric, drop durable-streams patch (superset-sh#3509) → PR #303 (fork keeps fstream patch) - a3df489 feat(desktop): v2 PR checkout via widened checkout procedure (superset-sh#3525) → PR #304 (clean) - c504a50 feat(desktop): v2 file editor — foundation, views, and stability pass (superset-sh#3526) → PR #310 (foundation: 58 files path-checkout) → PR #311 (adaptation: 20 files manual port with SpreadsheetViewer/memo/fork-hotkeys preserved) - 78b7dc8 feat(desktop): promote "Create Section Below" to top-level on workspace menu (superset-sh#3537) → PR #308 Record merge so upstream/main..main shows 0 behind.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fileDocumentStorewith shared buffers, save/dirty/conflict/orphan state,fs:eventssubscription, and rename tracking; wire three views (CodeView,MarkdownPreviewView,ImageView) + aBinaryWarningfallback behind a VS Code-style editor resolver.idso CodeEditor survives rename, "Don't Save" actually discards, Cmd+W respectsonBeforeClose, conflict UX replaces the heavy diff modal with a simple Save / Don't Save / Cancel alert.navigator.clipboard(host service may be remote), active-line usesaccenttoken, editor keymap keeps its default Cmd+/ comment toggle.Test plan
.png,.svg) → only "Image" in view toggle, checkerboard visible through transparency..binwith null bytes) → "Binary" warning; "Open Anyway" renders raw bytes in the code view.Pre-existing test failure (
packages/trpc/src/router/task/task.test.tsimportingdbWsfrompackages/db/src/client.ts) is unrelated to this branch; lint + typecheck green.Summary by cubic
Builds the v2 file editor on a shared, refcounted
fileDocumentStorewith a VS Code–style view resolver and stronger stability. Adds Code, Markdown preview, and Image views with better binary/large-file handling, rename‑safe buffers, clearer error states, and a simpler save/close flow.New Features
fileDocumentStorewithfs:events, dirty/save/conflict/orphan/external-change tracking, rename tracking, and stable document IDs.CodeView(CodeMirror 6 with theme polish, clearer selection/active‑line highlight, and@replit/codemirror-css-color-picker),MarkdownPreviewView(read-only),ImageView, plus aBinaryWarningfallback; view toggle lives in the tab header.navigator.clipboardwith an offscreen-textarea fallback and toast feedback.Bug Fixes
URL.createObjectURL(Blob)with cleanup to reduce memory use.Written for commit 227571c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation