feat(desktop): add diagnostics, database explorer, and workspace UX improvements#44
feat(desktop): add diagnostics, database explorer, and workspace UX improvements#44
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a language‑services subsystem (providers, manager, diagnostics store, LSP/stdio client), Problems UI and editor diagnostics, search-and-replace (regex/case) with replace operation, a Database Explorer (SQLite/Postgres), GitHub workflow/issue integrations, browser bookmarks, hotkeys/browser shortcuts, and macOS agent sleep prevention. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer Process
participant TRPC as tRPC Router
participant Manager as LanguageService Manager (Main)
participant Provider as Language Provider (tsserver/json/taplo/dart)
participant Store as Diagnostics Store
Renderer->>TRPC: openDocument(workspaceId, document)
TRPC->>Manager: openDocument(document)
Manager->>Provider: openDocument(document)
Provider->>Provider: analyze / run LSP/tsserver
Provider->>Store: setFileDiagnostics(file, diagnostics)
Store->>Store: update version & emit workspace event
Renderer->>TRPC: subscribeDiagnostics(workspaceId)
TRPC-->>Renderer: diagnostics update notification
sequenceDiagram
participant UI as Search UI (Renderer)
participant TRPC as tRPC Router
participant FSHost as workspace-fs host
participant Search as search.ts
participant FS as Filesystem
UI->>TRPC: searchContent(query, options)
TRPC->>FSHost: searchContent(...)
FSHost->>Search: searchContent(options)
Search->>FS: glob & read files
Search-->>TRPC: matches grouped by file
TRPC-->>UI: results
UI->>TRPC: replaceContent(query,replacement,paths?)
TRPC->>FSHost: replaceContent(...)
FSHost->>Search: replaceContent(...)
Search->>FS: read, write (with precondition)
Search-->>TRPC: updated/conflicts/failed
TRPC-->>UI: replace result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/filesystem/index.ts (1)
243-256:⚠️ Potential issue | 🟠 MajorDon't trim the query at the API boundary.
Trimming here changes the operation itself:
" "becomes a no-op, and literals/patterns like" foo"or"bar "can never be found or replaced. Onlyquery === ""should short-circuit; otherwise pass the raw query through unchanged. The mirrored host-service router should stay aligned with the same behavior.Suggested fix
- const trimmedQuery = input.query.trim(); - if (!trimmedQuery) { + if (input.query.length === 0) { return { matches: [] }; } … - query: trimmedQuery, + query: input.query, … - const trimmedQuery = input.query.trim(); - if (!trimmedQuery) { + if (input.query.length === 0) { return { replacements: 0, filesUpdated: 0, updated: [], conflicts: [], failed: [], }; } … - query: trimmedQuery, + query: input.query,Also applies to: 263-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 243 - 256, The code trims the query at the API boundary (creating trimmedQuery from input.query.trim()) which alters search semantics; instead, only short-circuit when input.query === "" and pass the raw input.query into getServiceForWorkspace(...).searchContent (and the mirrored host-service router) so leading/trailing spaces are preserved for literal/pattern searches—remove use of trimmedQuery, use input.query directly for the query param, and apply the same change to the other block referenced (the one around lines 263-283).
🟡 Minor comments (13)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx-333-347 (1)
333-347:⚠️ Potential issue | 🟡 MinorAllow empty diff snapshots to hydrate the document state.
""is a validdiffData.modifiedvalue for deletions and truncations. Thelength === 0guard skipsapplyLoadedDocumentContentin that case, so the backing document never gets updated to the empty snapshot and can keep stale state/diagnostics.🩹 Proposed fix
useEffect(() => { if ( viewMode !== "diff" || isDirty || - !diffData || - diffData.modified.length === 0 + !diffData ) { return; } applyLoadedDocumentContent( documentKey, diffData.modified, workingCopyRevision ?? rawRevision ?? null, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx` around lines 333 - 347, The current effect early-returns when diffData.modified.length === 0, which prevents applying valid empty-string snapshots (deletions/truncations); update the guard so it only returns when diffData is falsy or diffData.modified is null/undefined (not when it's an empty string or empty array). Concretely, in the useEffect that checks viewMode/isDirty/diffData, remove the length===0 check and replace it with a nullish check (e.g., diffData?.modified == null) so applyLoadedDocumentContent(documentKey, diffData.modified, workingCopyRevision ?? rawRevision ?? null) runs for empty-string snapshots as well.apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/RepositoryPanel/RepositoryPanel.tsx-257-265 (1)
257-265:⚠️ Potential issue | 🟡 MinorLabel the icon-only remove buttons.
These controls render only
LuX, so assistive tech has no accessible name for them. Add anaria-label(and ideally atitle) so users can tell whether the button removes a tracked run or an attached asset.Also applies to: 950-958
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/RepositoryPanel/RepositoryPanel.tsx` around lines 257 - 265, The remove Buttons in RepositoryPanel render only the LuX icon so they lack accessible names; update the Button that calls onRemove(tracked.workflowId) (and the similar Button used for attached assets) to include an appropriate aria-label (e.g., aria-label={`Remove tracked run ${tracked.workflowId}`} or aria-label={`Remove attached asset ${asset.name}`}) and also add a title attribute with the same brief text; locate the Buttons in RepositoryPanel (the one wrapping <LuX /> and any other icon-only remove Buttons around the repository/asset UI) and add these attributes so assistive tech and hover tooltips convey the action and target.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/components/BookmarkBarItem/components/EditBookmarkDialog/EditBookmarkDialog.tsx-42-47 (1)
42-47:⚠️ Potential issue | 🟡 MinorTrim and validate the URL before saving.
The current submit path forwards
urlas-is, so this dialog can persist empty or whitespace-padded bookmark URLs. Trim beforeonSaveand block submit when the URL is blank.✂️ Proposed fix
<form className="space-y-4" onSubmit={(event) => { event.preventDefault(); - onSave({ title, url }); + const nextTitle = title.trim(); + const nextUrl = url.trim(); + if (!nextUrl) { + return; + } + onSave({ title: nextTitle, url: nextUrl }); }} > @@ - <Button type="submit">Save</Button> + <Button type="submit" disabled={!url.trim()}> + Save + </Button>Also applies to: 71-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/components/BookmarkBarItem/components/EditBookmarkDialog/EditBookmarkDialog.tsx` around lines 42 - 47, The form submit handler currently calls onSave({ title, url }) with the raw url; update the onSubmit in EditBookmarkDialog to trim and validate the URL first by computing const trimmedUrl = url.trim(), then block submission (e.g., set an error state or return early) when trimmedUrl === "" and only call onSave({ title, url: trimmedUrl }) when valid; apply the same trim-and-validate change to the other submit handler in this component that currently forwards title and url as-is.apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts-493-513 (1)
493-513:⚠️ Potential issue | 🟡 MinorAdd the missing supported-language keywords here.
searchSettings()only indexestitle,description, andkeywords, but this entry omitsjavascript,jsx, andjsonc. Searches for those providers will not surface the new diagnostics toggle even though this PR adds them.➕ Suggested keyword additions
keywords: [ "diagnostics", "problems", "errors", "warnings", + "javascript", "typescript", + "jsx", "tsx", "json", + "jsonc", "toml", "dart", "flutter", "language server",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts` around lines 493 - 513, The keywords array for the settings entry identified by SETTING_ITEM_ID.BEHAVIOR_LANGUAGE_DIAGNOSTICS is missing some supported-language tokens so searchSettings() won't index them; update the keywords array in that object to include the additional language identifiers (e.g., "javascript", "jsx", "jsonc") so title/description/keywords indexing by searchSettings() will surface this diagnostics toggle for those providers.apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/CodeEditor/CodeEditor.tsx-601-613 (1)
601-613:⚠️ Potential issue | 🟡 MinorMemoize
editorThemeto prevent unnecessary effect reconfiguration on every render.The
getEditorTheme()function returns a new object reference on every call. Without memoization at line 450,editorThemewill have a different reference on each render, causing the effect at lines 601–613 to re-run unnecessarily even when the theme hasn't changed. Wrap the call withuseMemokeyed onactiveTheme:const editorTheme = useMemo(() => getEditorTheme(activeTheme), [activeTheme]);This pattern is already correctly implemented in
DiffScrollbarDecorations.tsx(line 161).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/components/CodeEditor/CodeEditor.tsx` around lines 601 - 613, The effect reconfigures diagnostics because editorTheme is a new object each render; memoize the result of getEditorTheme so its reference only changes when activeTheme changes. Replace direct calls to getEditorTheme() that set editorTheme with a useMemo wrapper keyed on activeTheme (e.g., const editorTheme = useMemo(() => getEditorTheme(activeTheme), [activeTheme])) so the diagnostics effect (which uses diagnosticsCompartment.reconfigure and buildDiagnosticDecorations) only runs when the actual theme changes.apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/utils/searchPattern/searchPattern.tsx-74-97 (1)
74-97:⚠️ Potential issue | 🟡 MinorSkip zero-width matches instead of marking the next character.
Patterns like
\b,^,$, or lookaheads producematch[0].length === 0. The currentmatchLength = 1branch highlights the following character, so the preview becomes wrong for valid regex searches. AdvancelastIndexto avoid the loop, but don't synthesize a one-character<mark>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/utils/searchPattern/searchPattern.tsx` around lines 74 - 97, The loop currently treats zero-length regex matches by faking a one-character match and highlighting the next character; instead, detect when matchText (match[0]) has length 0 before creating the <mark>, do NOT set matchLength = 1 or push a synthesized <mark>, and simply advance regex.lastIndex (or set it to match.index + 1 for global regex) and continue the loop so we skip the zero-width match without modifying cursor or inserting a mark; adjust the logic around matchText/matchLength, regex.lastIndex, nodes, and cursor in the while (match) loop inside searchPattern.tsx accordingly.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/DatabaseExplorerPane/DatabaseExplorerPane.tsx-70-76 (1)
70-76:⚠️ Potential issue | 🟡 MinorReset the pane auto-title when the selected connection disappears.
The
!currentConnectionbranch returns early, so clearing/removing the connection leaves the oldDB: …title behind. Please clear the auto title in that branch using whatever “no auto title” valuesetPaneAutoTitleexpects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/DatabaseExplorerPane/DatabaseExplorerPane.tsx` around lines 70 - 76, The early-return branch in the useEffect (watching currentConnection, paneId, setPaneAutoTitle) leaves the previous "DB: …" title intact; update that !currentConnection branch to clear the auto-title by calling setPaneAutoTitle(paneId, <the component's "no auto title" sentinel>) so the pane title is removed when the connection disappears (use the project's expected sentinel such as undefined/null/empty string as appropriate).apps/desktop/src/renderer/stores/browser-bookmarks.ts-110-135 (1)
110-135:⚠️ Potential issue | 🟡 MinorClear stale favicons when a bookmark points to a new URL.
updateBookmarkalways carries forward the previousfaviconUrlwhen the caller doesn't supply one. If a user edits a bookmark from one site to another, the bar can keep showing the old site's icon.💡 Suggested change
const updatedBookmark: BrowserBookmark = { ...targetBookmark, url: normalizedUrl, title: bookmark.title.trim() || normalizedUrl, - faviconUrl: bookmark.faviconUrl ?? targetBookmark.faviconUrl, + faviconUrl: + bookmark.faviconUrl ?? + (normalizeBookmarkUrl(targetBookmark.url) === normalizedUrl + ? targetBookmark.faviconUrl + : undefined), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/browser-bookmarks.ts` around lines 110 - 135, The updateBookmark handler (updateBookmark) currently always preserves targetBookmark.faviconUrl via "faviconUrl: bookmark.faviconUrl ?? targetBookmark.faviconUrl", which leaves a stale icon when the URL was changed but no new favicon was provided. Change the favicon assignment so that if bookmark.faviconUrl is provided use it; otherwise if normalizeBookmarkUrl(targetBookmark.url) !== normalizedUrl then clear the favicon (set faviconUrl to undefined/null) else keep targetBookmark.faviconUrl. Use the existing symbols normalizeBookmarkUrl, targetBookmark, normalizedUrl and bookmark.faviconUrl to implement this conditional.apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ProblemsView/ProblemsView.tsx-462-467 (1)
462-467:⚠️ Potential issue | 🟡 MinorReplace array index with stable diagnostic fields in list key.
The current key includes
${index}, which shifts when diagnostics reorder after filtering or refreshing, breaking React's component state tracking. Use only the stable diagnostic fields to build the key.♻️ Suggested change
- key={`${groupKey}:${problem.code ?? "no-code"}:${problem.line ?? 0}:${index}`} + key={[ + groupKey, + problem.providerId, + problem.source, + problem.code ?? "no-code", + problem.line ?? 0, + problem.column ?? 0, + problem.endLine ?? 0, + problem.endColumn ?? 0, + problem.message, + ].join(":")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ProblemsView/ProblemsView.tsx` around lines 462 - 467, The list key currently uses the array index in ProblemsView's problems.map (see the ContextMenu key template `${groupKey}:${problem.code ?? "no-code"}:${problem.line ?? 0}:${index}`), which causes instability when diagnostics reorder; remove the index and build the key from only stable diagnostic fields such as problem.id (or fallback to `${problem.relativePath ?? "no-path"}:${problem.line ?? 0}:${problem.character ?? 0}:${problem.code ?? "no-code"}` if id is unavailable), and update the ContextMenu key to use that stable composite string so React can reliably track items.apps/desktop/src/main/lib/language-services/providers/json/JsonLanguageProvider.ts-67-80 (1)
67-80:⚠️ Potential issue | 🟡 MinorRemote schema fetching lacks timeout and error handling.
The
schemaRequestServiceusesfetchwithout a timeout, which could hang indefinitely if a schema server is unresponsive. Consider adding anAbortControllerwith a reasonable timeout.🛡️ Proposed fix with timeout
schemaRequestService: async (uri) => { if (uri.startsWith("file://")) { return await fs.readFile(new URL(uri), "utf8"); } - const response = await fetch(uri); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10_000); + try { + const response = await fetch(uri, { signal: controller.signal }); + if (!response.ok) { + throw new Error(`Failed to load schema: ${uri} (${response.status})`); + } + return await response.text(); + } finally { + clearTimeout(timeoutId); + } - if (!response.ok) { - throw new Error(`Failed to load schema: ${uri} (${response.status})`); - } - - return await response.text(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/providers/json/JsonLanguageProvider.ts` around lines 67 - 80, The schemaRequestService inside the jsonService uses fetch without a timeout and no abort handling; update schemaRequestService to create an AbortController, start a short timeout (e.g. 3–10s) that calls controller.abort(), pass controller.signal into fetch(uri, { signal }), and clear the timeout on success; wrap the fetch and response handling in try/catch to translate fetch/abort errors into a clear Error (including uri and status or abort reason), while leaving the file:// branch using fs.readFile unchanged; reference jsonService and schemaRequestService when making these changes.apps/desktop/src/lib/trpc/routers/diagnostics/index.ts-319-319 (1)
319-319:⚠️ Potential issue | 🟡 MinorSynchronous TypeScript compilation may block the event loop.
The
getTypeScriptProblemsprocedure uses.query()and performs synchronousts.createProgramcalls. For large workspaces with multipletsconfig.jsonfiles, this can block the main thread significantly.Consider using
.mutation()(which better signals a heavy operation) or offloading to a worker thread for large projects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/diagnostics/index.ts` at line 319, The getTypeScriptProblems procedure currently defined with .query() runs synchronous ts.createProgram calls which can block the event loop for large workspaces; change the router entry for getTypeScriptProblems to use .mutation() instead of .query() to signal a heavy operation and/or move the ts.createProgram work into a worker thread (using worker_threads or a child process) so the synchronous compilation does not run on the main thread; update the procedure implementation (getTypeScriptProblems) to dispatch the heavy compilation task to the worker and return results asynchronously once the worker completes.apps/desktop/src/main/lib/language-services/providers/typescript/TypeScriptLanguageProvider.ts-687-712 (1)
687-712:⚠️ Potential issue | 🟡 Minor
sendRequesthas no timeout for unresponsive tsserver.If tsserver becomes unresponsive (infinite loop, deadlock), the Promise will never resolve. Consider adding a timeout that rejects the Promise after a reasonable duration (e.g., 30 seconds).
🛡️ Proposed fix with timeout
+const REQUEST_TIMEOUT_MS = 30_000; + private async sendRequest( session: WorkspaceSession, command: string, args?: unknown, ): Promise<TsServerResponse> { const seq = ++session.seq; const payload: TsServerRequest = { seq, type: "request", command, arguments: args, }; const content = `${JSON.stringify(payload)}\n`; return await new Promise<TsServerResponse>((resolve, reject) => { + const timeoutId = setTimeout(() => { + session.requestResolvers.delete(seq); + reject(new Error(`tsserver request timed out: ${command}`)); + }, REQUEST_TIMEOUT_MS); + - session.requestResolvers.set(seq, { resolve, reject }); + session.requestResolvers.set(seq, { + resolve: (value) => { + clearTimeout(timeoutId); + resolve(value); + }, + reject: (error) => { + clearTimeout(timeoutId); + reject(error); + }, + }); session.process.stdin.write(content, "utf8", (error) => { if (!error) { return; } + clearTimeout(timeoutId); session.requestResolvers.delete(seq); reject(error); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/providers/typescript/TypeScriptLanguageProvider.ts` around lines 687 - 712, The sendRequest Promise can hang if tsserver never responds; add a timeout (e.g., 30s) inside sendRequest that rejects the Promise and cleans up the resolver if no response arrives. Specifically, when creating the Promise in sendRequest (for WorkspaceSession), start a setTimeout that calls session.requestResolvers.delete(seq) and rejects with a TimeoutError after the duration; ensure you clear that timer when the resolver is resolved or rejected (both in the stdin.write callback error branch and where the corresponding resolver.resolve is invoked elsewhere). Use the existing seq, session.requestResolvers and session.process.stdin.write locations to wire up the timer and cleanup so unresolved requests cannot leak.apps/desktop/src/main/lib/language-services/providers/dart/DartLanguageProvider.ts-304-311 (1)
304-311:⚠️ Potential issue | 🟡 MinorDon't fall back to
workspaceErrorsfor an active session.After a failed
dart/reanalyze, the map keeps the old message. Lines 320-321 then null-coalesce into that stale entry even after a later success setssession.lastError = null, so the provider stays stuck inerroruntil the workspace is recreated.Patch sketch
try { await session.client.request("dart/reanalyze"); session.lastError = null; + this.workspaceErrors.delete(args.workspaceId); } catch (error) { session.lastError = error instanceof Error ? error.message : String(error); this.workspaceErrors.set(args.workspaceId, session.lastError); } @@ - const lastError = - session?.lastError ?? this.workspaceErrors.get(args.workspaceId) ?? null; + const lastError = session + ? session.lastError + : this.workspaceErrors.get(args.workspaceId) ?? null;Also applies to: 319-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/providers/dart/DartLanguageProvider.ts` around lines 304 - 311, The provider falls back to this.workspaceErrors for an active session, which leaves stale errors in the map and causes the provider to stay in error; update the dart/reanalyze success path (where session.client.request("dart/reanalyze") sets session.lastError = null) to also remove any stale entry by calling this.workspaceErrors.delete(args.workspaceId), and ensure the catch still sets both session.lastError and this.workspaceErrors.set(args.workspaceId, ...). Also change any lookup logic that computes the current error for an active session to prefer session.lastError exclusively (do not null-coalesce into this.workspaceErrors when session exists).
🧹 Nitpick comments (11)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/BookmarkBar.tsx (1)
43-46: Skip no-op bookmark moves when dropped on the same item.At Line 45, you can avoid unnecessary store updates/rerenders by returning early when
active.id === over.id.♻️ Proposed tweak
const handleDragEnd = ({ active, over }: DragEndEvent) => { if (!over) return; + if (active.id === over.id) return; moveBookmark(String(active.id), String(over.id)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/BookmarkBar.tsx` around lines 43 - 46, The drag-end handler handleDragEnd currently always calls moveBookmark even when the item is dropped on itself; update handleDragEnd to return early when there's no meaningful move by checking that over exists and that the active and over ids are not equal (e.g., compare String(active.id) !== String(over.id)) before calling moveBookmark, so moveBookmark is only invoked for real reorders.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx (1)
209-222: Move diagnostics subscription out of per-file viewer instances.At Line 209, each mounted
FileViewerContentcreates its own diagnostics subscription and invalidates the same workspace query. With many open panes, this multiplies invalidations and refetch churn for each diagnostics event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx` around lines 209 - 222, FileViewerContent is creating a per-instance diagnostics subscription using electronTrpc.languageServices.subscribeDiagnostics.useSubscription which causes duplicate invalidations of trpcUtils.languageServices.getWorkspaceDiagnostics.invalidate for the same workspaceId; lift this subscription out of FileViewerContent into a single higher-level owner (e.g., WorkspaceView or TabsContent) so only one subscription exists per workspaceId, enable it only when workspaceId is present, and have that single subscriber call trpcUtils.languageServices.getWorkspaceDiagnostics.invalidate({ workspaceId }) on data events; remove the subscription code from FileViewerContent and instead either provide the subscription via context or manage it in the parent component that already knows workspaceId.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/CodeMirrorDiffViewer/CodeMirrorDiffViewer.tsx (1)
139-198: Extract shared diagnostic helpers to reduce duplication.
createDiagnosticsThemeandbuildDiagnosticDecorationsare duplicated verbatim fromCodeEditor.tsx. Consider extracting these to a shared utility module (e.g.,utils/codemirror-diagnostics.ts) to maintain DRY principles and ensure consistent behavior across both editors.Example extraction
Create a new file:
// utils/codemirror-diagnostics.ts import { EditorState } from "@codemirror/state"; import { Decoration, EditorView } from "@codemirror/view"; import type { getEditorTheme } from "shared/themes"; export type DiagnosticEntry = { line: number | null; column: number | null; endLine: number | null; endColumn: number | null; severity: "error" | "warning" | "info" | "hint"; }; export function createDiagnosticsTheme(theme: ReturnType<typeof getEditorTheme>) { // ... existing implementation } export function buildDiagnosticDecorations( doc: EditorState["doc"], diagnostics: DiagnosticEntry[], ) { // ... existing implementation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/CodeMirrorDiffViewer/CodeMirrorDiffViewer.tsx` around lines 139 - 198, The two functions createDiagnosticsTheme and buildDiagnosticDecorations in CodeMirrorDiffViewer.tsx are duplicated from CodeEditor.tsx; extract them into a shared module (e.g., utils/codemirror-diagnostics.ts) that exports the functions and a DiagnosticEntry type, update both CodeEditor.tsx and CodeMirrorDiffViewer.tsx to import createDiagnosticsTheme and buildDiagnosticDecorations from that new module, and ensure imports for EditorState, Decoration, EditorView and getEditorTheme are adjusted in the shared file so both consumers compile unchanged.apps/desktop/src/renderer/providers/LanguageServicesProvider/LanguageServicesProvider.tsx (1)
174-219: Consider adding error handling for document lifecycle mutations.The
openDocument,changeDocument, andcloseDocumentmutations are fire-and-forget (voidprefix). If these fail, diagnostics won't work for those documents, but users won't receive any feedback. Consider adding.catch()handlers to log errors, at minimum for debugging purposes.Example error logging
if (!prev) { - void electronTrpcClient.languageServices.openDocument.mutate({ + electronTrpcClient.languageServices.openDocument.mutate({ workspaceId: tracked.workspaceId, absolutePath: tracked.absolutePath, languageId: tracked.languageId, content: tracked.content, version: tracked.version, - }); + }).catch((err) => { + console.error("[LanguageServicesProvider] Failed to open document:", tracked.absolutePath, err); + }); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/providers/LanguageServicesProvider/LanguageServicesProvider.tsx` around lines 174 - 219, The document lifecycle mutations (electronTrpcClient.languageServices.openDocument.mutate, changeDocument.mutate, closeDocument.mutate) are currently invoked fire-and-forget and lack error handling; update the logic in the useEffect that iterates over trackedDocuments/previousRef to attach .catch handlers (or await in an async wrapper) to each mutate call and log failures with contextual info (documentKey, workspaceId, absolutePath, operation name) so errors are observable for debugging and diagnostics.apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/DiagnosticsSettings/DiagnosticsSettings.tsx (1)
55-58: Unknown providers should not render as live toggles.For unknown
providerIds,checkedfalls back toprovider.enabled, butonCheckedChangereturns immediately, so the control looks interactive and then snaps back on the next render. Either disable unsupported rows explicitly or key the preference state byproviderIdso new providers do not require touching this component.Also applies to: 76-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/DiagnosticsSettings/DiagnosticsSettings.tsx` around lines 55 - 58, The toggle appears interactive for unknown providers because checked falls back to provider.enabled while onCheckedChange is a no-op; change behavior so unknown providers render disabled and derive checked only from the preferences map. Specifically, in DiagnosticsSettings use isKnownProviderId(...) to set disabled={true} when false, compute checked as !!enabledProviders[providerId] (not provider.enabled) and keep onCheckedChange early-return, and apply the same change to the other toggle block (lines handling the other provider rendering around where onCheckedChange is defined) so unknown provider rows look and behave non-interactive.apps/desktop/src/main/lib/language-services/manager.ts (2)
183-183: Consider adding adisposemethod for application shutdown.The manager singleton maintains stateful providers with external processes (tsserver). For clean application shutdown, a
dispose()method that callsdisposeWorkspacefor all known workspaces would ensure proper cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/manager.ts` at line 183, Add a dispose method to LanguageServiceManager that cleanly shuts down all workspace resources: implement LanguageServiceManager.dispose() to iterate over the manager's tracked workspaces (the internal collection used by methods like disposeWorkspace and getWorkspace/getOrCreateWorkspace) and call disposeWorkspace for each, then clear the collection; expose this on the exported singleton languageServiceManager so the application shutdown path can call languageServiceManager.dispose() to terminate tsserver processes and other external resources.
27-35: Method name mismatch:syncDocumentcallschangeDocument.The
syncDocumentmethod internally callsprovider.changeDocument. This semantic mismatch may confuse consumers. If "sync" implies creating-if-not-exists behavior, consider having the provider'schangeDocumentcheck if the document is open first (whichTypeScriptLanguageProvider.changeDocumentalready does at line 263-266).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/manager.ts` around lines 27 - 35, The syncDocument method calls provider.changeDocument which is a semantic mismatch; update syncDocument to either call a provider API named syncDocument/openDocument (if providers implement that) or ensure provider.changeDocument performs create-if-not-exists behavior like TypeScriptLanguageProvider.changeDocument (lines referencing TypeScriptLanguageProvider.changeDocument). Concretely: modify syncDocument to invoke the provider method that expresses "sync/create-if-missing" (e.g., provider.syncDocument or provider.openDocument) or add the existence check and creation logic before calling provider.changeDocument so providers receive consistent semantics.apps/desktop/src/lib/trpc/routers/diagnostics/index.ts (1)
91-105: Duplicate utility:normalizeRelativePathalready exists in utils.ts.This function duplicates
toRelativeWorkspacePathfromapps/desktop/src/main/lib/language-services/utils.ts. Consider importing and reusing the shared utility to maintain DRY.♻️ Suggested refactor
+import { toRelativeWorkspacePath } from "@/main/lib/language-services/utils"; // ... -function normalizeRelativePath( - workspacePath: string, - fileName: string, -): string | null { - const relativePath = path.relative(workspacePath, fileName); - if ( - !relativePath || - relativePath.startsWith("..") || - path.isAbsolute(relativePath) - ) { - return null; - } - - return relativePath.split(path.sep).join("/"); -} +// Use toRelativeWorkspacePath from utils instead🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/diagnostics/index.ts` around lines 91 - 105, The function normalizeRelativePath duplicates existing utility toRelativeWorkspacePath; remove the local normalizeRelativePath implementation and import toRelativeWorkspacePath from the shared utils module, then replace uses of normalizeRelativePath with toRelativeWorkspacePath (ensure argument order/workspace semantics match); keep return types and error/null behavior consistent and update any call sites in this file to handle null/undefined the same way.apps/desktop/src/main/lib/language-services/providers/toml/TomlLanguageProvider.ts (1)
232-265: All TOML diagnostics reported as "error" severity.The
mapDiagnosticfunction hardcodesseverity: "error"(line 262). If Taplo'sLintErrorincludes severity information in the future, this won't adapt. Consider checking if the error object has severity metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/providers/toml/TomlLanguageProvider.ts` around lines 232 - 265, The mapDiagnostic function currently hardcodes severity: "error" causing all Taplo LintError entries to be reported as errors; update mapDiagnostic (in TomlLanguageProvider) to inspect the incoming LintError for any severity/level field (e.g., error.severity, error.level or similar) and map it to the LanguageServiceDiagnostic severity (e.g., "error" | "warning" | "info"); default to "error" if no metadata exists and ensure the returned object uses that mapped value instead of the hardcoded string.apps/desktop/src/main/lib/language-services/providers/typescript/TypeScriptLanguageProvider.ts (1)
158-172: Duplicate utility:toRelativeWorkspacePathalready exists in utils.ts.This function duplicates the implementation from
apps/desktop/src/main/lib/language-services/utils.ts. Import the shared utility instead.♻️ Suggested refactor
+import { toRelativeWorkspacePath } from "../../utils"; // ... -function toRelativeWorkspacePath( - workspacePath: string, - absolutePath: string, -): string | null { - const relativePath = path.relative(workspacePath, absolutePath); - // ... -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/providers/typescript/TypeScriptLanguageProvider.ts` around lines 158 - 172, The local toRelativeWorkspacePath implementation in TypeScriptLanguageProvider.ts duplicates the one in language-services/utils.ts; remove the local function and import the shared utility instead by adding an import for toRelativeWorkspacePath from the utils module and updating any local references to use the imported symbol (keep existing call sites in TypeScriptLanguageProvider like any uses inside methods such as resolvePath or where toRelativeWorkspacePath is referenced). Ensure the import path points to apps/desktop/src/main/lib/language-services/utils (or the module's relative path) and delete the duplicate function block to avoid shadowing.apps/desktop/src/main/lib/language-services/utils.ts (1)
52-70: Potential off-by-one in CRLF handling.When encountering
\r\n, incrementingindexinside the loop (line 62) causes theforloop'sindex += 1to skip the character after\n. This is intentional to skip the\nthat's part of CRLF, but the column calculation after a\r(not followed by\n) resets to 1 correctly.However, if the offset falls exactly on the
\nof a\r\npair, the position calculation may be slightly off. Consider adding a comment clarifying this edge case behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/language-services/utils.ts` around lines 52 - 70, The CRLF branch inside the loop (variables boundedOffset, content, index, line, column) increments index to skip the '\n' of a '\r\n' pair which can cause an off-by-one when the target offset equals that '\n'; update the code by adding a clear comment above the '\r' handling that states: we intentionally increment index to consume the '\n' of a CRLF pair so the '\n' is not processed separately, and note the edge case where an offset landing exactly on the '\n' will be treated as part of the preceding CRLF (explain expected behavior), and optionally add a test or adjust logic to explicitly handle offset === index+1 if you want a different semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdbd0a13-fb60-43ae-840e-bae8e0540360
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
apps/desktop/docs/LANGUAGE_SERVICES.mdapps/desktop/package.jsonapps/desktop/src/lib/trpc/routers/databases/index.tsapps/desktop/src/lib/trpc/routers/diagnostics/index.tsapps/desktop/src/lib/trpc/routers/filesystem/index.tsapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/language-services/index.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.tsapps/desktop/src/main/lib/language-services/diagnostics-store.tsapps/desktop/src/main/lib/language-services/lsp/StdioJsonRpcClient.tsapps/desktop/src/main/lib/language-services/manager.tsapps/desktop/src/main/lib/language-services/providers/dart/DartLanguageProvider.tsapps/desktop/src/main/lib/language-services/providers/json/JsonLanguageProvider.tsapps/desktop/src/main/lib/language-services/providers/toml/TomlLanguageProvider.tsapps/desktop/src/main/lib/language-services/providers/typescript/TypeScriptLanguageProvider.tsapps/desktop/src/main/lib/language-services/types.tsapps/desktop/src/main/lib/language-services/utils.tsapps/desktop/src/renderer/components/MarkdownRenderer/MarkdownRenderer.tsxapps/desktop/src/renderer/providers/LanguageServicesProvider/LanguageServicesProvider.tsxapps/desktop/src/renderer/providers/LanguageServicesProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/DiagnosticsSettings/DiagnosticsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/DiagnosticsSettings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/BookmarkBar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/components/BookmarkBarItem/BookmarkBarItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/components/BookmarkBarItem/components/EditBookmarkDialog/EditBookmarkDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/components/BookmarkBarItem/components/EditBookmarkDialog/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/components/BookmarkBarItem/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BookmarkBar/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/DatabaseExplorerPane/DatabaseExplorerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/DatabaseExplorerPane/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/CodeMirrorDiffViewer/CodeMirrorDiffViewer.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/RepositoryPanel/RepositoryPanel.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/RepositoryPanel/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ProblemsView/ProblemsView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ProblemsView/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/SearchView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/components/SearchFileGroup/SearchFileGroup.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/components/SearchFileGroup/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/components/SearchMatchItem/SearchMatchItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/components/SearchMatchItem/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/components/SearchToolbar/SearchToolbar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/components/SearchToolbar/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/hooks/useContentSearch/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/hooks/useContentSearch/useContentSearch.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/utils/searchPattern/searchPattern.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/components/CodeEditor/CodeEditor.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/components/CodeEditor/loadLanguageSupport.tsapps/desktop/src/renderer/stores/browser-bookmarks.tsapps/desktop/src/renderer/stores/database-sidebar.tsapps/desktop/src/renderer/stores/language-service-preferences.tsapps/desktop/src/renderer/stores/sidebar-state.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/detect-language.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/shared/tabs-types.tsbunfig.tomlpackages/host-service/src/trpc/router/filesystem/filesystem.tspackages/workspace-fs/src/client/index.tspackages/workspace-fs/src/core/service.tspackages/workspace-fs/src/host/service.tspackages/workspace-fs/src/search.tspackages/workspace-fs/src/types.ts
| SELECT | ||
| t.table_schema, | ||
| t.table_name, | ||
| t.table_type, | ||
| c.column_name, | ||
| c.data_type, | ||
| c.is_nullable, | ||
| c.ordinal_position | ||
| FROM information_schema.tables t | ||
| JOIN information_schema.columns c | ||
| ON t.table_schema = c.table_schema | ||
| AND t.table_name = c.table_name | ||
| WHERE t.table_schema NOT IN ('pg_catalog', 'information_schema') | ||
| ORDER BY t.table_schema, t.table_name, c.ordinal_position |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/desktop/src/lib/trpc/routers/databases/index.tsRepository: MocA-Love/superset
Length of output: 118
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '310,340p'Repository: MocA-Love/superset
Length of output: 1100
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '535,550p'Repository: MocA-Love/superset
Length of output: 719
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '575,590p'Repository: MocA-Love/superset
Length of output: 824
🏁 Script executed:
rg "ctid" apps/desktop/src/lib/trpc/routers/databases/index.ts -nRepository: MocA-Love/superset
Length of output: 468
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '295,335p'Repository: MocA-Love/superset
Length of output: 1386
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '520,560p'Repository: MocA-Love/superset
Length of output: 1777
🏁 Script executed:
rg "inspectPostgres|previewData|getRow" apps/desktop/src/lib/trpc/routers/databases/index.ts -B 2 -A 1 | head -80Repository: MocA-Love/superset
Length of output: 115
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '1,50p'Repository: MocA-Love/superset
Length of output: 1706
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '500,520p'Repository: MocA-Love/superset
Length of output: 832
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '565,585p'Repository: MocA-Love/superset
Length of output: 958
🏁 Script executed:
rg "previewPostgres|getPostgresRow" apps/desktop/src/lib/trpc/routers/databases/index.ts -B 5 | head -50Repository: MocA-Love/superset
Length of output: 223
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '496,510p'Repository: MocA-Love/superset
Length of output: 548
🏁 Script executed:
rg "table_type.*VIEW|table_type.*TABLE" apps/desktop/src/lib/trpc/routers/databases/index.tsRepository: MocA-Love/superset
Length of output: 44
🌐 Web query:
PostgreSQL ctid column views system columns
💡 Result:
The ctid is a system column in PostgreSQL present in every table, representing the physical location of a row version as a tuple identifier (tid) of type tid, formatted as (block_number, tuple_offset). It changes on updates or VACUUM FULL and should not be used as a permanent row identifier. System columns like ctid are implicitly defined for tables but are not part of user-defined column lists. In information_schema.columns (which covers both tables and views), system columns such as ctid are explicitly excluded. For views, which have no physical storage or rows of their own, system columns do not exist. When querying a view, SELECT * returns only the columns defined in the view's query (user columns from underlying tables). The ctid column from underlying base tables can be accessed explicitly if the view's SELECT includes it (e.g., CREATE VIEW v AS SELECT ctid, * FROM t;), but it is not automatically included and is not a property of the view itself. In view update operations (UPDATE/DELETE on updatable views), PostgreSQL internally uses ctid from underlying tables or wholerow references, but this is not exposed in standard SELECT output from the view. Examples: - On a table: SELECT ctid, * FROM my_table; shows ctid. - On a standard view without ctid in definition: SELECT * FROM my_view; does not show ctid. - To include: CREATE VIEW my_view AS SELECT ctid, col1 FROM my_table;
Citations:
- 1: https://www.postgresql.org/docs/current/ddl-system-columns.html
- 2: https://www.postgresql.org/docs/current/infoschema-columns.html
- 3: https://www.postgresql.org/docs/current/rules-views.html
- 4: https://postgresql.org/docs/9.0/ddl-system-columns.html
- 5: https://postgrespro.com/docs/postgrespro/17/ddl-system-columns
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '330,380p'Repository: MocA-Love/superset
Length of output: 1646
🏁 Script executed:
grep -n "table_type\|table_schema\|TABLE\|VIEW" apps/desktop/src/lib/trpc/routers/databases/index.ts | head -30Repository: MocA-Love/superset
Length of output: 1279
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '395,415p'Repository: MocA-Love/superset
Length of output: 808
🏁 Script executed:
rg "inspectPostgres" apps/desktop/src -lRepository: MocA-Love/superset
Length of output: 223
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/databases/index.ts | sed -n '390,410p'Repository: MocA-Love/superset
Length of output: 709
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx | head -100Repository: MocA-Love/superset
Length of output: 3029
🏁 Script executed:
rg "inspectPostgres|previewPostgres|getPostgresRow" apps/desktop/src/renderer -A 3 -B 3 | head -80Repository: MocA-Love/superset
Length of output: 5074
🏁 Script executed:
rg "type.*view|type.*table" apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx -i | head -20Repository: MocA-Love/superset
Length of output: 161
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx | grep -A 10 "selectedTable"Repository: MocA-Love/superset
Length of output: 14195
🏁 Script executed:
rg "WHERE type = 'table'" apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx -B 5 -A 5Repository: MocA-Love/superset
Length of output: 306
🏁 Script executed:
rg "type.*==.*view|type.*===.*view|\.filter.*type" apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx -i -A 2 -B 2Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx | sed -n '2300,2430p'Repository: MocA-Love/superset
Length of output: 6086
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx | sed -n '1050,1080p'Repository: MocA-Love/superset
Length of output: 1290
🏁 Script executed:
rg "selectedTable\.type|table\.type" apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsxRepository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
rg "activeTables|activeSchemaQuery" apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx -B 3 -A 3 | head -50Repository: MocA-Love/superset
Length of output: 1328
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/DatabasesView/DatabasesView.tsx | sed -n '850,900p'Repository: MocA-Love/superset
Length of output: 1967
Fix the inspection query to exclude views, or handle them separately in preview/detail paths.
The inspectPostgres query returns both tables and views from information_schema.tables, but previewPostgresTable and getPostgresRowDetail require ctid—a system column that exists only on tables, not views. When users select a view from the explorer, preview and detail operations will fail with an error about missing ctid. Either filter the inspection query to include only tables (WHERE t.table_type = 'TABLE'), or modify the preview/detail paths to handle views separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/databases/index.ts` around lines 317 - 330,
The inspection query currently returns both tables and views causing
previewPostgresTable and getPostgresRowDetail to fail (ctid only exists on real
tables); update the inspectPostgres query to only return real tables by adding a
filter on t.table_type (e.g., WHERE t.table_type = 'BASE TABLE' in addition to
the existing NOT IN clause), or alternatively update previewPostgresTable and
getPostgresRowDetail to detect views (via table_type) and handle them separately
(skip ctid-dependent logic or fetch rows via a view-safe primary-key-based
path).
| function workflowSupportsDispatch({ | ||
| repoPath, | ||
| workflowPath, | ||
| }: { | ||
| repoPath: string; | ||
| workflowPath?: string; | ||
| }): boolean { | ||
| if (!workflowPath) { | ||
| return false; | ||
| } | ||
|
|
||
| const absolutePath = path.join(repoPath, workflowPath); | ||
| if (!existsSync(absolutePath)) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| const content = readFileSync(absolutePath, "utf8"); | ||
| return ( | ||
| /^\s*workflow_dispatch\s*:/m.test(content) || | ||
| /^\s*on\s*:\s*workflow_dispatch\s*$/m.test(content) || | ||
| /^\s*on\s*:\s*\[[^\]]*\bworkflow_dispatch\b[^\]]*\]/m.test(content) | ||
| ); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "git-status.ts" --path "apps/desktop/**"Repository: MocA-Love/superset
Length of output: 295
🏁 Script executed:
fd -t f "git-status.ts" apps/desktop/Repository: MocA-Love/superset
Length of output: 131
🏁 Script executed:
wc -l apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsRepository: MocA-Love/superset
Length of output: 136
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '204,230p'Repository: MocA-Love/superset
Length of output: 814
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '500,520p'Repository: MocA-Love/superset
Length of output: 818
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '1,50p'Repository: MocA-Love/superset
Length of output: 1856
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '450,510p'Repository: MocA-Love/superset
Length of output: 2475
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '380,470p'Repository: MocA-Love/superset
Length of output: 2821
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '330,410p'Repository: MocA-Love/superset
Length of output: 2647
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '60,120p'Repository: MocA-Love/superset
Length of output: 2093
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '260,330p'Repository: MocA-Love/superset
Length of output: 2448
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | sed -n '520,550p'Repository: MocA-Love/superset
Length of output: 910
Dispatch support is being inferred from the local checkout, not the target repo/ref.
getGitHubRepositoryOverview() fetches the workflow list from GitHub API (line 446), but lines 509-512 filter these workflows by checking local file existence. If the worktree is on an older branch or lacks a workflow file, workflowSupportsDispatch() returns false and the workflow is hidden from the UI even though the repository on GitHub can still dispatch it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts` around
lines 204 - 230, The code hides workflows by checking the local checkout via
workflowSupportsDispatch (which uses existsSync/readFileSync) instead of using
the workflow data fetched from GitHub (getGitHubRepositoryOverview); update the
logic to determine dispatch support from the remote workflow info or the
workflow file content returned by the GitHub API rather than the local
filesystem. Concretely: stop using existsSync/readFileSync in
workflowSupportsDispatch for filtering, either (a) change
workflowSupportsDispatch to accept the workflow file content (string) or a
boolean flag from the API and detect workflow_dispatch from that content, or (b)
replace the local-file check where workflows are filtered (the code that calls
workflowSupportsDispatch) to consult the workflows returned by
getGitHubRepositoryOverview (or fetch the workflow content via the GitHub API)
and base the dispatch-supported filter on that remote data. Ensure references to
symbols: workflowSupportsDispatch and getGitHubRepositoryOverview are updated so
UI filtering uses remote/ API-derived information, not the local worktree.
| const trimmedQuery = query.trim(); | ||
| const validationError = useMemo( | ||
| () => getSearchValidationError(trimmedQuery, isRegex), | ||
| [trimmedQuery, isRegex], | ||
| ); | ||
| const debouncedQuery = useDebouncedValue(trimmedQuery, 150); | ||
| const isDebouncing = | ||
| trimmedQuery.length > 0 && trimmedQuery !== debouncedQuery; |
There was a problem hiding this comment.
Preserve whitespace in the search input.
query.trim() changes the term itself, so users cannot search for indentation, a single space, or literals/regexes with meaningful leading/trailing whitespace. Treat only "" as empty here; otherwise the search request and the UI state drift from user intent.
Suggested fix
- const trimmedQuery = query.trim();
+ const rawQuery = query;
const validationError = useMemo(
- () => getSearchValidationError(trimmedQuery, isRegex),
- [trimmedQuery, isRegex],
+ () => getSearchValidationError(rawQuery, isRegex),
+ [rawQuery, isRegex],
);
- const debouncedQuery = useDebouncedValue(trimmedQuery, 150);
- const isDebouncing =
- trimmedQuery.length > 0 && trimmedQuery !== debouncedQuery;
+ const debouncedQuery = useDebouncedValue(rawQuery, 150);
+ const isDebouncing = rawQuery.length > 0 && rawQuery !== debouncedQuery;
…
- hasQuery: trimmedQuery.length > 0,
+ hasQuery: rawQuery.length > 0,Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/hooks/useContentSearch/useContentSearch.ts`
around lines 30 - 37, Don't trim the user's search term; preserve
leading/trailing whitespace for validation, debouncing, and isDebouncing logic.
Replace trimmedQuery with the original query string for calls to
getSearchValidationError, useDebouncedValue, and the isDebouncing calculation
(e.g., remove the const trimmedQuery = query.trim() and change validationError,
debouncedQuery, and isDebouncing to use query), but still treat only query ===
"" as empty when gating empty-state behavior; also apply the same change to the
other occurrence around the symbols at lines 74-77 (the other use of
useDebouncedValue/getSearchValidationError/isDebouncing).
| const runReplace = useCallback( | ||
| async (paths?: string[]) => { | ||
| if (!workspaceId || !query.trim() || validationError) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const result = await replaceMutation.mutateAsync({ | ||
| workspaceId, | ||
| query, | ||
| replacement, | ||
| includeHidden: false, | ||
| includePattern, | ||
| excludePattern, | ||
| isRegex, | ||
| caseSensitive, | ||
| paths, | ||
| }); |
There was a problem hiding this comment.
Trim the query before calling replaceContent.
This callback treats query.trim() as the canonical search text, but the mutation still receives raw query. A query like " foo " can therefore produce visible matches and then replace nothing.
💡 Suggested change
async (paths?: string[]) => {
- if (!workspaceId || !query.trim() || validationError) {
+ const normalizedQuery = query.trim();
+ if (!workspaceId || !normalizedQuery || validationError) {
return;
}
try {
const result = await replaceMutation.mutateAsync({
workspaceId,
- query,
+ query: normalizedQuery,
replacement,
includeHidden: false,📝 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.
| const runReplace = useCallback( | |
| async (paths?: string[]) => { | |
| if (!workspaceId || !query.trim() || validationError) { | |
| return; | |
| } | |
| try { | |
| const result = await replaceMutation.mutateAsync({ | |
| workspaceId, | |
| query, | |
| replacement, | |
| includeHidden: false, | |
| includePattern, | |
| excludePattern, | |
| isRegex, | |
| caseSensitive, | |
| paths, | |
| }); | |
| const runReplace = useCallback( | |
| async (paths?: string[]) => { | |
| const normalizedQuery = query.trim(); | |
| if (!workspaceId || !normalizedQuery || validationError) { | |
| return; | |
| } | |
| try { | |
| const result = await replaceMutation.mutateAsync({ | |
| workspaceId, | |
| query: normalizedQuery, | |
| replacement, | |
| includeHidden: false, | |
| includePattern, | |
| excludePattern, | |
| isRegex, | |
| caseSensitive, | |
| paths, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/SearchView/SearchView.tsx`
around lines 166 - 183, The runReplace callback uses query.trim() for validation
but still passes the raw query to replaceMutation.mutateAsync; create a
trimmedQuery (e.g., const trimmedQuery = query.trim()) inside runReplace and
pass trimmedQuery (not query) into replaceMutation.mutateAsync (and any
downstream call sites like replaceContent if present) so replacements use the
canonical trimmed search text; keep the existing validation using trimmedQuery
to avoid double work.
| export const useDatabaseSidebarStore = create<DatabaseSidebarState>()( | ||
| devtools( | ||
| persist( | ||
| (set, get) => ({ |
There was a problem hiding this comment.
Don't persist raw database secrets in this store.
Because this store is wrapped in persist, every Postgres connectionString is written to disk in plaintext, and queryHistory.sql may retain sensitive literals too. That is a real credential/privacy leak for the new database explorer. Keep secrets in secure OS/Electron storage and persist only non-sensitive metadata or opaque references here.
Also applies to: 92-100, 163-183, 203-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/database-sidebar.ts` around lines 67 - 70,
The store created by useDatabaseSidebarStore (type DatabaseSidebarState) is
wrapped in persist and currently persists sensitive fields like connectionString
and queryHistory.sql; change the state shape and persistence so that secrets are
never stored in this persisted slice: remove or replace connectionString and any
queryHistory.sql literals with non-sensitive metadata or opaque references/IDs,
and move actual secrets to secure OS/Electron storage (e.g.,
secureStore.get/set) accessed at runtime by functions in the store (e.g.,
loadConnection, getQueryHistory) so persisted data only contains non-sensitive
metadata/ids and not raw credentials or SQL literals.
| @@ -1,3 +1,3 @@ | |||
| [install] | |||
| minimumReleaseAge = 604800 | |||
| #minimumReleaseAge = 604800 | |||
There was a problem hiding this comment.
Re-enable minimumReleaseAge to avoid weakening supply-chain protection.
Commenting this out removes the 7-day quarantine safeguard for newly published packages. Keep this policy enabled and resolve blocked installs by selecting an older compliant version instead of disabling the guard.
Suggested fix
[install]
-#minimumReleaseAge = 604800
+minimumReleaseAge = 604800
linker = "isolated" # Prevent hoisting from resolving `path-scurry@2` to `lru-cache@6` (missing `LRUCache`), which breaks the `@superset/desktop` postinstall native rebuild.Based on learnings: When a package install/update/add command fails due to age security policy (7-day minimum release age), choose an older version that satisfies the policy instead of disabling the policy or using bypass flags.
📝 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.
| #minimumReleaseAge = 604800 | |
| [install] | |
| minimumReleaseAge = 604800 | |
| linker = "isolated" # Prevent hoisting from resolving `path-scurry@2` to `lru-cache@6` (missing `LRUCache`), which breaks the `@superset/desktop` postinstall native rebuild. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bunfig.toml` at line 2, Uncomment and re-enable the minimumReleaseAge setting
(restore minimumReleaseAge = 604800) in bunfig.toml so the 7-day quarantine
policy is enforced; do not remove or bypass this guard—if installs fail due to
the age policy, resolve by selecting an older package version that meets
minimumReleaseAge rather than disabling or commenting out the setting.
| function compileSearchPattern({ | ||
| query, | ||
| isRegex = false, | ||
| caseSensitive, | ||
| }: Pick< | ||
| SearchContentOptions, | ||
| "query" | "isRegex" | "caseSensitive" | ||
| >): CompiledSearchPattern { | ||
| const resolvedCaseSensitive = resolveCaseSensitive( | ||
| query, | ||
| caseSensitive, | ||
| isRegex, | ||
| ); | ||
| const flags = resolvedCaseSensitive ? "gu" : "giu"; | ||
| const source = isRegex ? query : escapeRegExp(query); | ||
|
|
||
| return { | ||
| isRegex, | ||
| caseSensitive: resolvedCaseSensitive, | ||
| regex: new RegExp(source, flags), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are best practices for preventing ReDoS attacks when accepting user-provided regex patterns in Node.js?
💡 Result:
Best practices for preventing ReDoS attacks when accepting user-provided regex patterns in Node.js focus on avoiding direct execution of untrusted patterns with the native RegExp engine, which is vulnerable to catastrophic backtracking. 1. Do not execute user-provided regex patterns directly. Instead, reject them or use safe alternatives. Native Node.js RegExp can be exploited with evil regexes like /^([a-zA-Z]+)+$/ combined with crafted input, freezing the single-threaded event loop {1,2,3,6,10,13}. 2. Use the 're2' package, a drop-in replacement using Google's RE2 engine that guarantees linear-time matching and prevents backtracking-based ReDoS. Wrap user patterns in try-catch to fallback if unsupported features (backreferences, lookaheads) are present: const RE2 = require('re2'); try { const re = new RE2(userPattern); re.exec(input); } catch (e) { // Reject or sanitize } RE2 supports most RegExp syntax but rejects ReDoS-prone features {13,14,6}. 3. Validate/sanitize user patterns before use: - Check with 'safe-regex' (or forks like safe-regex2): detects nested quantifiers but has false positives/negatives. const safe = require('safe-regex'); if (!safe(userPattern)) reject; {1,18} - Restrict pattern complexity: limit length, forbid nested quantifiers (+*, ? inside repeats), backrefs. 4. Always limit input string length (e.g., express.json({limit: '10kb'})) and rate-limit requests to prevent amplification {2,3,6}. 5. Run regex in worker threads or with timeouts using child_process to isolate from main event loop {7}. 6. Prefer schema validators (Joi, Zod) over raw regex for common cases like email/username {2,5}. 7. Monitor CPU/performance; use WAF for extra protection {1}. Node.js official docs do not cover user-provided regex specifically, as per threat model it's app-level {11,12}.
Citations:
- 1: https://medium.com/@arunangshudas/5-tips-to-avoid-regex-dos-attacks-in-node-js-ce1c1d7004a6
- 2: https://www.herodevs.com/blog-posts/preventing-redos-regular-expression-denial-of-service-attacks-in-express
- 3: https://www.aikido.dev/code-quality/rules/guard-against-slow-regular-expressions-preventing-redos-attacks
- 4: https://www.nodejs-security.com/blog/regex-gone-wrong-parse-duration-npm-package-vulnerability
- 5: https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- 6: https://nodejs.dev/en/learn/getting-started/security-best-practices
- 7: https://nodejs.org/api/all.html
- 8: https://github.com/uhop/node-re2
- 9: https://www.npmjs.com/package/re2?activeTab=code
- 10: https://registry.npmjs.org/safe-regex
🏁 Script executed:
# First, let's examine the search.ts file around the mentioned lines
head -n 380 packages/workspace-fs/src/search.ts | tail -n 50Repository: MocA-Love/superset
Length of output: 1028
🏁 Script executed:
# Get a broader view of the file to understand the context
wc -l packages/workspace-fs/src/search.tsRepository: MocA-Love/superset
Length of output: 102
🏁 Script executed:
# Check how compileSearchPattern is called and where user input comes from
rg "compileSearchPattern" packages/workspace-fs/src/search.ts -B 5 -A 5Repository: MocA-Love/superset
Length of output: 600
🏁 Script executed:
# Look for any input validation or safety checks in the file
rg "regex|RegExp|pattern" packages/workspace-fs/src/search.ts -i | head -20Repository: MocA-Love/superset
Length of output: 733
🏁 Script executed:
# Find the SearchContentOptions interface definition
rg "interface SearchContentOptions|type SearchContentOptions" packages/workspace-fs/src/search.ts -A 10Repository: MocA-Love/superset
Length of output: 311
🏁 Script executed:
# Find the searchContent and replaceContent function signatures
rg "export.*function (searchContent|replaceContent)" packages/workspace-fs/src/search.ts -A 8Repository: MocA-Love/superset
Length of output: 408
🏁 Script executed:
# Check if there's any input validation or length checks
rg "query.*length|validate|sanitize|trim" packages/workspace-fs/src/search.ts -B 2 -A 2Repository: MocA-Love/superset
Length of output: 2159
🏁 Script executed:
# Look at the calling context - where is this used from?
rg "searchContent|replaceContent" packages/workspace-fs/src/search.ts | head -5Repository: MocA-Love/superset
Length of output: 283
ReDoS vulnerability: User-supplied regex patterns compiled without validation.
The compileSearchPattern function at line 372 directly constructs a RegExp from user input when isRegex is true: const source = isRegex ? query : escapeRegExp(query). The exported searchContent and replaceContent functions accept this query parameter directly from users with only basic trimming and empty checks—no regex complexity validation. Malicious patterns like (a+)+$ combined with crafted input can trigger catastrophic backtracking, freezing the single-threaded Node.js event loop and causing denial of service.
Recommended mitigations:
- Use the
re2package (Google's RE2 engine) as a drop-in replacement to guarantee linear-time matching - Validate patterns with
safe-regexbefore compilation - Add query length limits and request rate-limiting
- Run regex operations in worker threads with timeout mechanisms to isolate from the event loop
🧰 Tools
🪛 ast-grep (0.42.0)
[warning] 372-372: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(source, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workspace-fs/src/search.ts` around lines 354 - 375,
compileSearchPattern currently constructs a RegExp directly from user input (via
the isRegex branch) which can lead to ReDoS; update compileSearchPattern and the
callers searchContent and replaceContent to validate or sandbox user regexes
before compilation: run the raw query through a safe-regex check (or limit
allowed quantifier constructs), enforce a strict max query length, and if
isRegex is true either compile with the re2 engine instead of RegExp or perform
validation and refuse/escape unsafe patterns; also consider executing regex work
in a worker with a timeout for extra safety. Ensure you update references to
escapeRegExp use for non-regex queries and add clear error handling for
rejected/unsafe patterns so callers get deterministic failures instead of
compiling dangerous regexes.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx (1)
539-548:⚠️ Potential issue | 🟠 MajorScope
SEARCH_IN_FILEShotkey to active workspace pages.The
SEARCH_IN_FILEShotkey is registered withundefinedoptions, bypassing theisActivegating used by other hotkeys in this component. This means the shortcut can trigger from inactive workspace pages. Either add anisActiveguard insidehandleSearchInFilesor passhotkeyOptionsto the registration.🔧 Proposed fix: Pass hotkeyOptions to scope the hotkey
- useAppHotkey("SEARCH_IN_FILES", handleSearchInFiles, undefined, [ + useAppHotkey("SEARCH_IN_FILES", handleSearchInFiles, hotkeyOptions, [ handleSearchInFiles, ]);🤖 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/workspace/`$workspaceId/page.tsx around lines 539 - 548, The SEARCH_IN_FILES hotkey is currently registered without scoping and can fire on inactive workspace pages; fix by either adding an early isActive check inside handleSearchInFiles or, preferably, pass hotkeyOptions to useAppHotkey so the hotkey is only active when the workspace page is active (e.g., useAppHotkey("SEARCH_IN_FILES", handleSearchInFiles, { isActive: /* same active predicate used by other hotkeys */ }, [handleSearchInFiles])). Ensure the guard uses the same active predicate as other hotkeys in this component and keep the existing behavior in handleSearchInFiles (setSidebarOpen, setSidebarMode(SidebarMode.Tabs), setRightSidebarTab(RightSidebarTab.Search)).
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.sh (1)
31-63: Consider extracting the sleep inhibition logic into a reusable function.This sleep inhibition block is duplicated verbatim across
gemini-hook.template.sh,cursor-hook.template.sh, andcopilot-hook.template.sh. Thenotify-hook.template.shalready extracts this into a_superset_manage_sleep_inhibitorfunction, which improves readability.While template files are deployed independently (so some duplication is acceptable), the inline approach here makes future maintenance harder—any bug fix or enhancement needs to be applied to 4 files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.sh` around lines 31 - 63, The duplicated sleep-inhibition block in gemini-hook.template.sh (guarded by SUPERSET_WRAPPER_PID, SUPERSET_PREVENT_AGENT_SLEEP and EVENT_TYPE) should be extracted into a reusable function named like _superset_manage_sleep_inhibitor (matching notify-hook.template.sh) and invoked from the case statement; move the logic that creates _superset_sleep_dir, reads/writes the _superset_pid_file, starts/stops caffeinate, and checks kill -0 into that function, preserve the checks for Darwin and command -v caffeinate, and replace the inline block in gemini-hook.template.sh with a single call to _superset_manage_sleep_inhibitor so future changes only need to be applied once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx:
- Around line 539-548: The SEARCH_IN_FILES hotkey is currently registered
without scoping and can fire on inactive workspace pages; fix by either adding
an early isActive check inside handleSearchInFiles or, preferably, pass
hotkeyOptions to useAppHotkey so the hotkey is only active when the workspace
page is active (e.g., useAppHotkey("SEARCH_IN_FILES", handleSearchInFiles, {
isActive: /* same active predicate used by other hotkeys */ },
[handleSearchInFiles])). Ensure the guard uses the same active predicate as
other hotkeys in this component and keep the existing behavior in
handleSearchInFiles (setSidebarOpen, setSidebarMode(SidebarMode.Tabs),
setRightSidebarTab(RightSidebarTab.Search)).
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.sh`:
- Around line 31-63: The duplicated sleep-inhibition block in
gemini-hook.template.sh (guarded by SUPERSET_WRAPPER_PID,
SUPERSET_PREVENT_AGENT_SLEEP and EVENT_TYPE) should be extracted into a reusable
function named like _superset_manage_sleep_inhibitor (matching
notify-hook.template.sh) and invoked from the case statement; move the logic
that creates _superset_sleep_dir, reads/writes the _superset_pid_file,
starts/stops caffeinate, and checks kill -0 into that function, preserve the
checks for Darwin and command -v caffeinate, and replace the inline block in
gemini-hook.template.sh with a single call to _superset_manage_sleep_inhibitor
so future changes only need to be applied once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8a1e54b-3a15-486e-a425-ceb1be7e703d
📒 Files selected for processing (27)
apps/desktop/src/lib/trpc/routers/menu.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-common.tsapps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/main/lib/menu-events.tsapps/desktop/src/main/lib/menu.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/renderer/lib/browser-shortcut-events.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/PaneViewer.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/pane-viewer.model.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsxapps/desktop/src/renderer/stores/hotkeys/store.tsapps/desktop/src/shared/browser-shortcuts.tsapps/desktop/src/shared/constants.tsapps/desktop/src/shared/hotkeys.tspackages/local-db/drizzle/0038_groovy_sister_grimm.sqlpackages/local-db/drizzle/meta/0038_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
✅ Files skipped from review due to trivial changes (9)
- apps/desktop/src/main/lib/menu-events.ts
- apps/desktop/src/shared/constants.ts
- packages/local-db/drizzle/0038_groovy_sister_grimm.sql
- apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/pane-viewer.model.ts
- apps/desktop/src/main/lib/menu.ts
- packages/local-db/drizzle/meta/_journal.json
- packages/local-db/drizzle/meta/0038_snapshot.json
- apps/desktop/src/main/lib/terminal/env.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx
- apps/desktop/src/shared/hotkeys.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts
1 participant
close #45
close #46
概要
apps/desktopに集中しており、一部workspace-fs/ host-service 側に検索・置換 API の拡張が入っています。主な変更
Problemsタブを追加し、ワークスペース全体のエラー・警告を一覧、絞り込み、再取得、該当ファイルへのジャンプできるようにしました。Searchタブを追加し、ワークスペース全文検索、include/exclude glob、正規表現検索、Match Case、結果のファイル単位グルーピング、行・列ジャンプ、ファイル単位置換、ワークスペース全体置換、Cmd/Ctrl+Shift+Fを追加しました。workspace-fs/ filesystem TRPC に、正規表現対応の検索・置換 API を追加しました。補足
mainです。packages/workspace-fsとpackages/host-serviceの typecheck です。apps/desktopの全体 typecheck は、この差分外の既存 TypeScript エラーがあるため現状では通っていません。Summary by CodeRabbit
New Features
Documentation