feat: resume builder, desktop analytics, windows signing & auto-launch, plus UI/perf fixes#1278
Conversation
…resume generation
…s, enhance report content handling
…eration tool in dashboard
…on states and display
…ccessibility and UI consistency
…s for consistency
…sume generation components
…esume generation tool
…replacing react-pdf with pdfjs-dist, enhancing loading states and error management
…component, introducing skeleton loader and improved state management
…t handling for Typst-based resumes
…d visibility management
…ge, public chat footer, and report panel
…il-panel The useState was declared but its value was never read in render (only the setter was called). Each setter call scheduled a re-render that changed nothing visible. The actual scroll/highlight behavior is driven by hasScrolledRef and setActiveChunkIndex; this state was effectively dead. Closes #1249
…d-state-v2 fix(new-chat): remove unused _hasScrolledToCited state in source-detail-panel
feat: resume builder
- Added event tracking for desktop app activation and quitting. - Introduced analytics bridge in preload script to handle user identification and event capturing. - Updated IPC channels to support analytics-related actions. - Enhanced analytics functionality in the main process to track user interactions and application updates. - Integrated analytics tracking for folder watching and deep link handling. - Improved connector setup tracking in the web application. This commit enhances the overall analytics capabilities of the application, ensuring better user behavior insights and event tracking across both desktop and web environments.
Closes #1247. toggleTheme's previous implementation read isDark from the closure via setIsDark(!isDark), which forced isDark into the useCallback dependency array. As a result toggleTheme's reference changed on every click, invalidating any downstream memoization. Switched to the functional updater setIsDark((prev) => !prev) and dropped isDark from the dependency list. The sibling setCrazyLightTheme and setCrazyDarkTheme callbacks already use this pattern (they pass concrete values to setIsDark without listing isDark in deps), so this keeps the three theme callbacks consistent. No observable behavior change — clicking the theme toggle still flips isDark. The callback reference is now stable between clicks, which is also safer under concurrent updates per React's standard guidance.
- Implemented IPC channels for managing auto-launch settings. - Enhanced main process to handle auto-launch behavior on startup. - Updated UI components to allow users to configure launch options. - Integrated analytics tracking for auto-launch events. This commit introduces the ability for users to enable or disable the application launching at system startup, along with options for starting minimized to the tray.
refactor(documents-sidebar): convert discarded isExportingKB state to ref
…o-onOpenChange fix(dialogs): move open-reset effects into onOpenChange handlers
fix(theme-toggle): use functional setIsDark in toggleTheme (#1247)
…nt-1242 perf: lazy-load DocumentTabContent to reduce initial dashboard bundle…
…effects-into-the-openTab-event-handlers-1252 refactor: move model selector reset logic to event handlers
feat: add startup for desktop app
- Added permissions for id-token to enable Windows signing. - Implemented logic to detect Windows signing eligibility based on production v* tags. - Integrated Azure login step for Windows signing if eligible. - Updated packaging command to include Windows signing options when applicable.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (2)
📝 WalkthroughWalkthroughAdds Typst-based resume generation (backend tool, DB persistence, PDF compilation, UI), introduces report content_type support and migrations, implements desktop auto-launch and analytics IPC, and many web UI/refactor changes including a client-side PDF viewer and analytics events. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebClient
participant Backend
participant LLM
participant TypstCompiler
participant DB
User->>WebClient: Request resume generation
WebClient->>Backend: POST generate_resume(user_info, user_instructions?, parent_report_id?)
Backend->>DB: Load parent report (if parent_report_id)
Backend->>LLM: Prompt for Typst body (generation or revision)
LLM-->>Backend: Typst body (raw)
Backend->>TypstCompiler: Compile Typst (header + body)
alt compile success
TypstCompiler-->>Backend: PDF bytes
else compile fails
Backend->>LLM: Fix prompt -> corrected Typst
LLM-->>Backend: Corrected Typst
Backend->>TypstCompiler: Recompile
TypstCompiler-->>Backend: PDF bytes
end
Backend->>DB: Persist Report (content_type="typst", source)
Backend-->>WebClient: {status:"ready", report_id, title, content_type}
WebClient->>User: Show preview / open report
User->>WebClient: Request preview PDF
WebClient->>Backend: GET /reports/{id}/preview
Backend->>TypstCompiler: Compile stored Typst -> PDF
TypstCompiler-->>Backend: PDF bytes
Backend-->>WebClient: Streaming PDF response
sequenceDiagram
participant User
participant Renderer
participant MainProcess
participant OS
User->>Renderer: Toggle "Launch on Startup" (UI)
Renderer->>MainProcess: IPC.invoke(SET_AUTO_LAUNCH, {enabled, openAsHidden})
MainProcess->>OS: Register/unregister login item or write freedesktop file
OS-->>MainProcess: Ack / error
MainProcess-->>Renderer: Return new state
MainProcess->>MainProcess: Persist state (electron-store)
Note over OS,MainProcess: On OS startup, app may be launched at login
OS->>MainProcess: App started at login
MainProcess->>MainProcess: wasLaunchedAtLogin(), shouldStartHidden()
alt start hidden
MainProcess->>MainProcess: stay in tray (no main window)
else start visible
MainProcess->>MainProcess: createMainWindow()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on b38a297..2703fd4
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (50)
• .github/workflows/desktop-release.yml
• surfsense_backend/alembic/versions/127_add_report_content_type.py
• surfsense_backend/alembic/versions/128_seed_build_resume_prompt.py
• surfsense_backend/app/agents/new_chat/system_prompt.py
• surfsense_backend/app/agents/new_chat/tools/registry.py
• surfsense_backend/app/agents/new_chat/tools/resume.py
• surfsense_backend/app/db.py
• surfsense_backend/app/prompts/system_defaults.py
• surfsense_backend/app/routes/public_chat_routes.py
• surfsense_backend/app/routes/reports_routes.py
• surfsense_backend/app/schemas/reports.py
• surfsense_backend/app/services/public_chat_service.py
• surfsense_backend/app/tasks/chat/stream_new_chat.py
• surfsense_desktop/src/ipc/channels.ts
• surfsense_desktop/src/ipc/handlers.ts
• surfsense_desktop/src/main.ts
• surfsense_desktop/src/modules/analytics.ts
• surfsense_desktop/src/modules/auto-launch.ts
• surfsense_desktop/src/modules/auto-updater.ts
• surfsense_desktop/src/modules/deep-links.ts
• surfsense_desktop/src/modules/folder-watcher.ts
• surfsense_desktop/src/modules/tray.ts
• surfsense_desktop/src/modules/window.ts
• surfsense_desktop/src/preload.ts
• surfsense_web/app/dashboard/[search_space_id]/new-chat/[[...chat_id]]/page.tsx
• surfsense_web/app/dashboard/[search_space_id]/user-settings/components/DesktopContent.tsx
• surfsense_web/app/dashboard/[search_space_id]/user-settings/components/PurchaseHistoryContent.tsx
• surfsense_web/atoms/chat/report-panel.atom.ts
• surfsense_web/components/assistant-ui/assistant-message.tsx
• surfsense_web/components/assistant-ui/connector-popup/constants/connector-constants.ts
• surfsense_web/components/assistant-ui/connector-popup/hooks/use-connector-dialog.ts
• surfsense_web/components/documents/CreateFolderDialog.tsx
• surfsense_web/components/documents/FolderPickerDialog.tsx
• surfsense_web/components/free-chat/anonymous-chat.tsx
• surfsense_web/components/free-chat/free-chat-page.tsx
• surfsense_web/components/free-chat/free-model-selector.tsx
• surfsense_web/components/layout/ui/shell/LayoutShell.tsx
• surfsense_web/components/layout/ui/sidebar/DocumentsSidebar.tsx
• surfsense_web/components/new-chat/model-selector.tsx
• surfsense_web/components/new-chat/source-detail-panel.tsx
• surfsense_web/components/public-chat/public-chat-footer.tsx
• surfsense_web/components/public-chat/public-thread.tsx
• surfsense_web/components/report-panel/pdf-viewer.tsx
• surfsense_web/components/report-panel/report-panel.tsx
• surfsense_web/components/shared/ExportMenuItems.tsx
• surfsense_web/components/theme/theme-toggle.tsx
• surfsense_web/components/tool-ui/generate-report.tsx
• surfsense_web/components/tool-ui/generate-resume.tsx
• surfsense_web/contracts/types/stripe.types.ts
• surfsense_web/instrumentation-client.ts
⏭️ Files skipped (4)
| Locations |
|---|
surfsense_web/lib/posthog/events.ts |
surfsense_web/package.json |
surfsense_web/pnpm-lock.yaml |
surfsense_web/types/window.d.ts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
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 (5)
surfsense_web/components/theme/theme-toggle.tsx (1)
588-599:⚠️ Potential issue | 🟠 MajorUse
resolvedThemefor theme toggle logic to handle the "system" case correctly.When
theme === "system"and the resolved theme is light, togglingisDarkwithout synchronously updating the actual theme creates a mismatch. The ternary at line 598 only checkstheme === "light", which fails whentheme === "system". UseresolvedThemeinstead, which is always concrete ("light" or "dark"), and update bothisDarkandsetThemeto the same target value. Also update the dependency array from[theme, ...]to[resolvedTheme, ...].🐛 Proposed fix
- const { theme, setTheme, resolvedTheme } = useTheme(); + const { setTheme, resolvedTheme } = useTheme(); const toggleTheme = useCallback(() => { - setIsDark((prev) => !prev); + const nextTheme = resolvedTheme === "dark" ? "light" : "dark"; + setIsDark(nextTheme === "dark"); const animation = createAnimation(variant, start, blur, gifUrl); updateStyles(animation.css); if (typeof window === "undefined") return; const switchTheme = () => { - setTheme(theme === "light" ? "dark" : "light"); + setTheme(nextTheme); }; if (!document.startViewTransition) { switchTheme(); return; } document.startViewTransition(switchTheme); - }, [theme, setTheme, variant, start, blur, gifUrl, updateStyles]); + }, [resolvedTheme, setTheme, variant, start, blur, gifUrl, updateStyles]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/theme/theme-toggle.tsx` around lines 588 - 599, The toggleTheme function currently flips setIsDark and then uses theme to decide setTheme, which breaks when theme === "system"; change the logic in toggleTheme to derive the new target from resolvedTheme (which is always "light" or "dark") instead of theme, update both setIsDark and setTheme to the same concrete target value, and update the useCallback dependency array to include resolvedTheme (replace theme) so createAnimation, updateStyles, setIsDark, and setTheme stay consistent with the resolved state.surfsense_web/components/new-chat/source-detail-panel.tsx (1)
132-133:⚠️ Potential issue | 🟠 MajorClear pending timers and track auto-scroll by
chunkId, not once per open panel.When
chunkIdchanges while the panel stays open, the booleanhasScrolledRefat line 282 prevents the new cited chunk from auto-scrolling, and any pending timers from the previous chunk can set the wrongactiveChunkIndexat line 328 because the dependency array (line 335) doesn't includechunkId.Proposed fix
const scrollAreaRef = useRef<HTMLDivElement>(null); -const hasScrolledRef = useRef(false); // Use ref to avoid stale closures +const autoScrolledChunkIdRef = useRef<number | null>(null); +const latestChunkIdRef = useRef(chunkId); const scrollTimersRef = useRef<ReturnType<typeof setTimeout>[]>([]); + +latestChunkIdRef.current = chunkId;- if (node && !hasScrolledRef.current && open) { - hasScrolledRef.current = true; // Mark immediately to prevent duplicate scrolls + if (node && autoScrolledChunkIdRef.current !== chunkId && open) { + scrollTimersRef.current.forEach(clearTimeout); + scrollTimersRef.current = []; + autoScrolledChunkIdRef.current = chunkId; // Mark immediately to prevent duplicate scrolls() => { - setActiveChunkIndex(citedChunkIndex); + if (latestChunkIdRef.current === chunkId && citedChunkIndex !== -1) { + setActiveChunkIndex(citedChunkIndex); + } },- [open, citedChunkIndex] + [open, citedChunkIndex, chunkId]- hasScrolledRef.current = false; + autoScrolledChunkIdRef.current = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/new-chat/source-detail-panel.tsx` around lines 132 - 133, The boolean hasScrolledRef and scrollTimersRef need to be reset/cleared when chunkId changes so a previous chunk's timers or the hasScrolled flag don't block or overwrite the new chunk's auto-scroll; update the effect that sets/clears timers (the one that sets activeChunkIndex and uses scrollTimersRef/setTimeout) to include chunkId in its dependency array, clear all timers in scrollTimersRef whenever chunkId changes or on unmount, and replace the single hasScrolledRef boolean with a per-chunk tracker (e.g., a Map or store keyed by chunkId) or reset hasScrolledRef when chunkId changes so each chunk can auto-scroll independently; ensure you reference hasScrolledRef, scrollTimersRef, chunkId, and activeChunkIndex in the updated logic.surfsense_web/components/layout/ui/sidebar/DocumentsSidebar.tsx (1)
481-580:⚠️ Potential issue | 🟡 MinorUse the export ref as an in-flight guard.
isExportingKBRef.currentis set/reset but never read, so rapid clicks can still start duplicate folder exports and duplicate backend work.Proposed fix
const handleExportWarningConfirm = useCallback(async () => { setExportWarningOpen(false); const ctx = exportWarningContext; if (!ctx?.folder) return; + if (isExportingKBRef.current) return; isExportingKBRef.current = true; try { @@ const handleExportFolder = useCallback( async (folder: FolderDisplay) => { + if (isExportingKBRef.current) return; + const folderPendingCount = getPendingCountInSubtree(folder.id); if (folderPendingCount > 0) { setExportWarningContext({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/layout/ui/sidebar/DocumentsSidebar.tsx` around lines 481 - 580, The in-flight guard is never read so rapid clicks can trigger duplicate exports; add an early-return check of isExportingKBRef.current at the start of both handleExportFolder and handleExportWarningConfirm (before any work or setExportWarningOpen calls) to no-op when true, keeping the existing isExportingKBRef.current = true and finally reset behavior; ensure doExport is only invoked when the guard allows execution so duplicate backend exports are prevented.surfsense_desktop/src/modules/deep-links.ts (1)
11-31:⚠️ Potential issue | 🟡 Minor
deepLinkUrlis never cleared when the window already exists, sohasPendingDeepLink()returns staletrue.
handleDeepLinkalways setsdeepLinkUrl = urlat Line 14, but in the path wheregetMainWindow()returns a window (Lines 16-30) the variable is never reset. After the very first successfully-handled deep link,hasPendingDeepLink()(Lines 76-78) will keep reportingtruefor the rest of the process lifetime, which defeats its purpose (main.tsuses it to decide whether to force-create the window). OnlyhandlePendingDeepLinkclears it (Line 69); the immediate path should too.🔧 Proposed fix
function handleDeepLink(url: string) { if (!url.startsWith(`${PROTOCOL}://`)) return; - deepLinkUrl = url; - const win = getMainWindow(); - if (!win) return; + if (!win) { + deepLinkUrl = url; + return; + } const parsed = new URL(url); trackEvent('desktop_deep_link_received', { host: parsed.hostname, path: parsed.pathname, }); if (parsed.hostname === 'auth' && parsed.pathname === '/callback') { const params = parsed.searchParams.toString(); win.loadURL(`http://localhost:${getServerPort()}/auth/callback?${params}`); } win.show(); win.focus(); + deepLinkUrl = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_desktop/src/modules/deep-links.ts` around lines 11 - 31, handleDeepLink currently sets the module-level deepLinkUrl unconditionally which leaves hasPendingDeepLink() returning true even after the link is immediately handled; update handleDeepLink so that after successfully obtaining getMainWindow() and handling the link (including loading the auth callback and showing/focusing the window) you clear deepLinkUrl (same variable cleared by handlePendingDeepLink) to avoid stale pending state—i.e., set deepLinkUrl = '' (or null per module convention) after the window-handling branch so hasPendingDeepLink() reflects reality.surfsense_backend/app/services/public_chat_service.py (1)
735-764:⚠️ Potential issue | 🟠 Major
clone_from_snapshotskipsgenerate_resumetool-calls — cloned resumes will point at the originalreport_idand losecontent_type.
create_snapshot(line 243) embeds resumes intosnapshot_data["reports"]by matching bothtoolName in ("generate_report", "generate_resume"), butclone_from_snapshotonly processestoolName == "generate_report". For a shared chat containing a resume:
- No new
Reportrow is created in the target search space, andreport_id_mappingis not populated for the resume.- The tool-call
result.report_idin the cloned message is left pointing at the source user's report, which the cloning user typically cannot access — the cloned resume card will fail to load.- Even if the branch is extended,
Report(...)at line 746 does not copycontent_type, so a cloned Typst resume will silently become a"markdown"report and the Typst-only preview/export paths won't engage.🔧 Proposed fix
- if ( - isinstance(part, dict) - and part.get("type") == "tool-call" - and part.get("toolName") == "generate_report" - ): + if ( + isinstance(part, dict) + and part.get("type") == "tool-call" + and part.get("toolName") in ("generate_report", "generate_resume") + ): result = part.get("result", {}) old_report_id = result.get("report_id") if old_report_id and old_report_id not in report_id_mapping: report_info = reports_lookup.get(old_report_id) if report_info: new_report = Report( title=report_info.get("title", "Cloned Report"), content=report_info.get("content"), + content_type=report_info.get("content_type", "markdown"), report_metadata=report_info.get("report_metadata"), search_space_id=target_search_space_id, thread_id=new_thread.id, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_backend/app/services/public_chat_service.py` around lines 735 - 764, clone_from_snapshot currently only handles tool-call parts with toolName == "generate_report", so "generate_resume" tool-calls are left pointing at the original report and cloned resumes lose content_type; update the processing block that checks part.get("toolName") to also include "generate_resume", create a new Report for resumes exactly like for reports but copy content_type from report_info (e.g., Report(..., content_type=report_info.get("content_type"), ...)), set new_report.report_group_id = new_report.id, add it to session and await session.flush(), populate report_id_mapping[old_report_id] with new_report.id, and then update part["result"]["report_id"] using that mapping so cloned tool-call results reference the new report and retain the original content_type.
♻️ Duplicate comments (1)
surfsense_web/components/documents/CreateFolderDialog.tsx (1)
32-41:⚠️ Potential issue | 🟠 MajorSame open-reset regression as
FolderPickerDialog.This dialog is also fully controlled by the parent (no
DialogTrigger). RadixDialogdoes not invokeonOpenChangewhen the parent flipsopentotrue, so thenext === truebranch — which clearsnameand focuses the input — won't run on programmatic opens. Users will see the previously typed folder name persist and the input will not auto-focus.Consider restoring a
useEffect(() => { if (open) { setName(""); inputRef.current?.focus(); } }, [open]), or rely onDialogContent's built-inonOpenAutoFocusfor focus and resetnamethere. Note also thatsetTimeout(..., 0)is fragile;requestAnimationFrameor Radix'sonOpenAutoFocusis more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/documents/CreateFolderDialog.tsx` around lines 32 - 41, The controlled dialog currently resets name and focuses the input only inside handleOpenChange(next) which isn't called when the parent sets open=true; update CreateFolderDialog to reset state and focus when the open prop changes instead: add a useEffect that watches the open prop and when open is true calls setName("") and inputRef.current?.focus() (or use requestAnimationFrame for focus), or move focus/reset logic to the DialogContent's onOpenAutoFocus handler; reference the existing handleOpenChange, setName, inputRef, open prop and DialogContent.onOpenAutoFocus when applying the fix.
🟡 Minor comments (12)
surfsense_web/components/public-chat/public-chat-footer.tsx-71-71 (1)
71-71:⚠️ Potential issue | 🟡 MinorFix invalid Tailwind utility causing hover animation to snap.
transition-alis not a valid Tailwind class—it should betransition-allto animate the hover effects (scale, shadow, brightness).Diff
- className="gap-2 rounded-full px-6 shadow-lg transition-al select-none duration-200 hover:scale-[1.02] hover:shadow-xl hover:brightness-110 hover:bg-primary" + className="gap-2 rounded-full px-6 shadow-lg transition-all select-none duration-200 hover:scale-[1.02] hover:shadow-xl hover:brightness-110 hover:bg-primary"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/public-chat/public-chat-footer.tsx` at line 71, The Tailwind class "transition-al" in the JSX className on the PublicChatFooter component is invalid and prevents hover animations from animating smoothly; update the className string used in the public-chat-footer.tsx JSX (the element with className="gap-2 rounded-full px-6 shadow-lg transition-al ...") to use "transition-all" instead so the hover transforms (scale, shadow, brightness) transition properly..github/workflows/desktop-release.yml-62-76 (1)
62-76:⚠️ Potential issue | 🟡 MinorHarden the production tag check.
refs/tags/v*will also enable signing for prerelease tags likev1.2.3-rc.1orv1.2.3-beta. If only final production releases should be signed, match exact release semver instead.Proposed guard refinement
- if [ "${{ matrix.os }}" = "windows-latest" ] \ - && [ "${{ github.event_name }}" = "push" ] \ - && [[ "$GITHUB_REF" == refs/tags/v* ]]; then + TAG="${GITHUB_REF#refs/tags/}" + if [ "${{ matrix.os }}" = "windows-latest" ] \ + && [ "${{ github.event_name }}" = "push" ] \ + && [[ "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/desktop-release.yml around lines 62 - 76, The tag check in the "Detect Windows signing eligibility" step (id: sign) is too broad and allows prerelease tags; replace the glob check [[ "$GITHUB_REF" == refs/tags/v* ]] with a strict semver regex that only matches final releases, e.g. use a bash regex test like [[ "$GITHUB_REF" =~ ^refs/tags/v[0-9]+\.[0-9]+\.[0-9]+$ ]] so only tags like vX.Y.Z enable signing and prerelease/build suffixes (e.g. -rc, -beta, +meta) are excluded.surfsense_web/app/dashboard/[search_space_id]/user-settings/components/PurchaseHistoryContent.tsx-120-132 (1)
120-132:⚠️ Potential issue | 🟡 MinorDon’t render failed purchase queries as an empty history.
If either request fails,
data?.purchases ?? []silently hides that purchase type; users can see “No purchases yet” or partial history instead of an error.🛠️ Proposed error handling
const [pagesQuery, tokensQuery] = results; const isLoading = pagesQuery.isLoading || tokensQuery.isLoading; + const hasError = pagesQuery.isError || tokensQuery.isError; const purchases = useMemo<UnifiedPurchase[]>(() => {+ if (hasError && purchases.length === 0) { + return ( + <div className="flex flex-col items-center justify-center gap-2 py-16 text-center"> + <ReceiptText className="h-8 w-8 text-muted-foreground" /> + <p className="text-sm font-medium">Failed to load purchases</p> + <p className="text-xs text-muted-foreground"> + Please refresh or try again later. + </p> + </div> + ); + } + if (purchases.length === 0) {Also applies to: 142-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/app/dashboard/`[search_space_id]/user-settings/components/PurchaseHistoryContent.tsx around lines 120 - 132, The current useMemo silently treats failed fetches as empty by using pagesQuery.data?.purchases ?? [] and tokensQuery.data?.purchases ?? []; update the logic in the PurchaseHistoryContent component to detect query failures (pagesQuery.isError or tokensQuery.isError) and surface an error state instead of rendering an empty purchase list: inside the hook that computes purchases (referencing pagesQuery, tokensQuery, normalizePagePurchase, normalizeTokenPurchase, useMemo) return/propagate an error flag or throw so the UI can render an error message (or the existing error UI) when either query failed, otherwise proceed to map and sort the combined purchases as before.surfsense_backend/app/schemas/reports.py-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorType
content_typeas a closed set.Using plain
strweakens the API contract and lets invalid report types flow to clients. Use a literal/enum matching the backend-supported values.🛠️ Proposed schema tightening
from datetime import datetime -from typing import Any +from typing import Any, Literal + +ReportContentType = Literal["markdown", "typst"]- content_type: str = "markdown" + content_type: ReportContentType = "markdown"- content_type: str = "markdown" + content_type: ReportContentType = "markdown"Also applies to: 44-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_backend/app/schemas/reports.py` at line 26, The content_type field is currently typed as a plain str (content_type: str = "markdown"); change it to a closed type using either typing.Literal (e.g., Literal["markdown", "html", ...]) or a small Enum to enumerate only backend-supported report types, update the default value to one of those members, and apply the same change to the other occurrence(s) of content_type in this file (the second block around lines 44-49) so the schema enforces valid report types at runtime and in generated clients.surfsense_web/app/dashboard/[search_space_id]/user-settings/components/PurchaseHistoryContent.tsx-75-80 (1)
75-80:⚠️ Potential issue | 🟡 MinorUse locale-aware currency formatting.
Hardcoding
$renders values like EUR/GBP as$12.00 EUR, which is misleading in billing history.💬 Proposed formatter update
function formatAmount(amount: number | null, currency: string | null): string { if (amount == null) return "—"; - const dollars = amount / 100; const code = (currency ?? "usd").toUpperCase(); - return `$${dollars.toFixed(2)} ${code}`; + try { + return new Intl.NumberFormat(undefined, { + style: "currency", + currency: code, + }).format(amount / 100); + } catch { + return `${(amount / 100).toFixed(2)} ${code}`; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/app/dashboard/`[search_space_id]/user-settings/components/PurchaseHistoryContent.tsx around lines 75 - 80, formatAmount currently hardcodes a leading "$" and appends the currency code, which mislabels non-USD amounts; update formatAmount to use Intl.NumberFormat with style: "currency" and the currency set to the uppercased currency code (fallback "USD"), format the value as amount/100 (since amount is cents), and return the formatted string (handle null amount by returning "—"); change the implementation inside formatAmount to create an Intl.NumberFormat (no hardcoded "$") and call .format(dollars) so currency symbols/locales are rendered correctly.surfsense_web/components/free-chat/free-chat-page.tsx-210-216 (1)
210-216:⚠️ Potential issue | 🟡 MinorAdd the missing dependencies to the
onNewcallback dependency array.The callback directly reads
modelSlug,anonMode.isAnonymous, andanonMode.uploadedDoc(lines 211–214), but onlymessagesanddoStreamare in the dependency list (line 294). This causes stale closures: ifmodelSlugoranonMode.uploadedDocchange after the component mounts, the analytics call continues to report outdated values.Also, simplify the ternary expression on line 214 from
? true : falseto just the boolean condition.Proposed fix
const onNew = useCallback( async (message: AppendMessage) => { let userQuery = ""; for (const part of message.content) { if (part.type === "text") userQuery += part.text; } if (!userQuery.trim()) return; trackAnonymousChatMessageSent({ modelSlug, messageLength: userQuery.trim().length, - hasUploadedDoc: - anonMode.isAnonymous && anonMode.uploadedDoc !== null ? true : false, + hasUploadedDoc: anonMode.isAnonymous && anonMode.uploadedDoc !== null, surface: "free_chat_page", }); // ...rest of callback - [messages, doStream] + [messages, doStream, modelSlug, anonMode.isAnonymous, anonMode.uploadedDoc] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/free-chat/free-chat-page.tsx` around lines 210 - 216, The onNew callback that calls trackAnonymousChatMessageSent reads modelSlug and anonMode.{isAnonymous, uploadedDoc} but only depends on messages and doStream, causing stale closures; update the onNew dependency array to include modelSlug, anonMode.isAnonymous, and anonMode.uploadedDoc so the callback re-creates when those values change, and simplify the hasUploadedDoc argument by replacing the ternary (anonMode.isAnonymous && anonMode.uploadedDoc !== null ? true : false) with the boolean expression (anonMode.isAnonymous && anonMode.uploadedDoc !== null).surfsense_desktop/src/modules/window.ts-83-88 (1)
83-88:⚠️ Potential issue | 🟡 MinorClose handler uses module-level
mainWindow, not the instance being closed.Inside the
closecallback,mainWindow.hide()dereferences the module-levelmainWindowvariable, butcreateMainWindow()reassigns that variable on every call. If this function is ever invoked to create a second window (as the adjacent comment on Lines 80-82 explicitly anticipates — "Applies to every instance — including the one created lazily after a launch-at-login boot"), closing the earlier window will hide the newer one rather than itself. Use the event target instead to keep the handler bound to its own window.🔒 Proposed fix
- mainWindow.on('close', (e) => { - if (!isQuitting && mainWindow) { - e.preventDefault(); - mainWindow.hide(); - } - }); + const thisWindow = mainWindow; + thisWindow.on('close', (e) => { + if (!isQuitting) { + e.preventDefault(); + thisWindow.hide(); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_desktop/src/modules/window.ts` around lines 83 - 88, The close handler currently uses the module-level mainWindow variable which can point to a different window; update the 'close' event handler installed in createMainWindow so it uses the event's target (or this/BrowserWindow.fromWebContents(e.sender)) to call hide() on the actual window being closed instead of mainWindow, and still respect the isQuitting check; locate the handler attached to mainWindow.on('close', ...) inside createMainWindow and replace references to mainWindow.hide() with a hide call on the event/window instance.surfsense_desktop/src/modules/tray.ts-58-64 (1)
58-64:⚠️ Potential issue | 🟡 MinorCall
shutdownAnalytics()beforeapp.exit(0)to ensure the quit event is flushed.
trackEvent()is synchronous and returns immediately after queuing the event with PostHog. The client will batch events and flush every 10 seconds or at 20 events, but a single quit event won't trigger immediate transmission. The process exits before the event is sent. Use the exportedshutdownAnalytics()function (which awaitsclient.shutdown()with a 3-second timeout) to ensure pending analytics are flushed before terminating:{ label: 'Quit', click: async () => { trackEvent('desktop_tray_quit_clicked'); await shutdownAnalytics(); app.exit(0); }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_desktop/src/modules/tray.ts` around lines 58 - 64, The Quit menu handler currently calls trackEvent('desktop_tray_quit_clicked') then immediately app.exit(0), which can terminate the process before analytics are flushed; change the click handler on the Quit menu entry to be async, call trackEvent(...) as before, then await shutdownAnalytics() (the exported helper that awaits client.shutdown() with timeout) before calling app.exit(0) so the event is flushed. Ensure you reference the existing trackEvent and shutdownAnalytics functions and update the click function signature to async so awaiting is possible.surfsense_web/app/dashboard/[search_space_id]/user-settings/components/DesktopContent.tsx-142-153 (1)
142-153:⚠️ Potential issue | 🟡 MinorApply the canonical auto-launch state returned by IPC.
Line 149 ignores the returned state, so the UI can stay optimistic if the desktop layer normalizes
openAsHiddenor reports unsupported behavior.🐛 Proposed fix
setAutoLaunchHidden(checked); try { - await api.setAutoLaunch(autoLaunchEnabled, checked); + const next = await api.setAutoLaunch(autoLaunchEnabled, checked); + if (next) { + setAutoLaunchEnabled(next.enabled); + setAutoLaunchHidden(next.openAsHidden); + setAutoLaunchSupported(next.supported); + } } catch { setAutoLaunchHidden(!checked); toast.error("Failed to update startup behavior");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/app/dashboard/`[search_space_id]/user-settings/components/DesktopContent.tsx around lines 142 - 153, The handler handleAutoLaunchHiddenToggle is optimistic and ignores the canonical state returned by the desktop IPC; change it to capture the result from api.setAutoLaunch(autoLaunchEnabled, checked), and then update UI with the returned canonical value (e.g., use the returned openAsHidden/normalized boolean) via setAutoLaunchHidden instead of blindly keeping the optimistic checked; on error revert to the previous state as before and show the toast. Ensure you reference api.setAutoLaunch, autoLaunchEnabled, setAutoLaunchHidden, and handleAutoLaunchHiddenToggle when making the change.surfsense_web/components/report-panel/pdf-viewer.tsx-64-86 (1)
64-86:⚠️ Potential issue | 🟡 Minor
first === 1sentinel is fragile; correct output depends onBUFFER_PAGES ≥ 1.
firstis initialized to1and then guarded byfirst === 1to "not overwrite once set". But whenscrollTop === 0the page-0 iteration setsfirst = i + 1 = 1, so the sentinel is still truthy on the next iteration andfirstgets overwritten to2. The finalMath.max(1, first - BUFFER_PAGES)happens to mask this becauseBUFFER_PAGES = 1; if anyone tunesBUFFER_PAGESto0in the future, the first page will silently stop being rendered while in view.Use an explicit "unset" sentinel so the intent is obvious and the behaviour is independent of the buffer value.
🔧 Proposed fix
- let cumTop = 16; - let first = 1; - let last = pageDimsRef.current.length; - - for (let i = 0; i < pageDimsRef.current.length; i++) { - const pageHeight = getScaledHeight(i); - const pageBottom = cumTop + pageHeight; - - if (pageBottom >= scrollTop && first === 1) { - first = i + 1; - } - if (cumTop > scrollBottom) { - last = i; - break; - } - - cumTop = pageBottom + PAGE_GAP; - } + let cumTop = 16; + let first: number | null = null; + let last = pageDimsRef.current.length; + + for (let i = 0; i < pageDimsRef.current.length; i++) { + const pageHeight = getScaledHeight(i); + const pageBottom = cumTop + pageHeight; + + if (first === null && pageBottom >= scrollTop) { + first = i + 1; + } + if (cumTop > scrollBottom) { + last = i; + break; + } + + cumTop = pageBottom + PAGE_GAP; + } + first = first ?? 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/report-panel/pdf-viewer.tsx` around lines 64 - 86, The loop uses first initialized to 1 as a sentinel which is fragile; change the sentinel to an explicit "unset" value (e.g., first = -1 or null) and update the condition that sets it from "first === 1" to check for the unset sentinel (e.g., first === -1 or first == null) so the first-visible-page calculation in the loop that iterates pageDimsRef.current with getScaledHeight(i), cumTop, PAGE_GAP and BUFFER_PAGES is independent of BUFFER_PAGES; finally, when returning, compute the clamped first using Math.max(1, (first unset ? 1 : first) - BUFFER_PAGES) (or handle the unset case to default to 1) and keep last calculation the same.surfsense_web/components/report-panel/pdf-viewer.tsx-89-128 (1)
89-128:⚠️ Potential issue | 🟡 MinorHarden canvas ref for async operations.
The canvas reference captured at line 91 can become stale if React unmounts the element during the
await pdf.getPage(pageNum)at line 103 (e.g., when the page scrolls out of the visible buffer or zoom changes). Re-check the canvas after the await before writing to it or passing it topage.render():Suggested fix
try { const page = await pdf.getPage(pageNum); + const currentCanvas = canvasRefs.current.get(pageNum); + if (!currentCanvas || currentCanvas !== canvas) { + page.cleanup(); + return; + } const viewport = page.getViewport({ scale: currentScale });Additionally, improve the error detection for cancelled renders. Replace
err.message?.includes("cancelled")witherr.name === 'RenderingCancelledException'per pdfjs-dist conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/report-panel/pdf-viewer.tsx` around lines 89 - 128, The renderPage function captures a canvas from canvasRefs.current before awaiting pdf.getPage, which can become stale if the component unmounts or the element is removed; after awaiting pdf.getPage (and before writing to canvas or calling page.render) re-read const canvas = canvasRefs.current.get(pageNum) and if it is missing abort/return (also ensure any pending renderTask for pageNum is cancelled and removed from renderTasksRef). Keep using renderTasksRef to track/cancel the active renderTask around page.render, and replace the cancelled check in the catch from err.message?.includes("cancelled") to err.name === 'RenderingCancelledException' to follow pdfjs-dist conventions.surfsense_desktop/src/modules/auto-launch.ts-286-293 (1)
286-293:⚠️ Potential issue | 🟡 MinorTrack visible login launches too.
For Linux/Windows,
wasLaunchedAtLogin()only detects--hidden. If the user enables auto-launch withopenAsHidden=false, login-started sessions are reported as manual launches. Consider adding a separate launch marker arg for all auto-launch registrations and keeping--hiddenonly for UI behavior.📊 Proposed direction
const STORE_KEY = 'launchAtLogin'; const HIDDEN_FLAG = '--hidden'; +const LAUNCHED_AT_LOGIN_FLAG = '--launched-at-login'; @@ - const args = openAsHidden ? ` ${HIDDEN_FLAG}` : ''; + const args = ` ${LAUNCHED_AT_LOGIN_FLAG}${openAsHidden ? ` ${HIDDEN_FLAG}` : ''}`; @@ openAtLogin: true, - args: openAsHidden ? [HIDDEN_FLAG] : [], + args: openAsHidden ? [LAUNCHED_AT_LOGIN_FLAG, HIDDEN_FLAG] : [LAUNCHED_AT_LOGIN_FLAG], @@ export function wasLaunchedAtLogin(): boolean { + if (process.argv.includes(LAUNCHED_AT_LOGIN_FLAG)) return true; if (process.argv.includes(HIDDEN_FLAG)) return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_desktop/src/modules/auto-launch.ts` around lines 286 - 293, wasLaunchedAtLogin currently only checks for HIDDEN_FLAG in process.argv and macOS login item settings, so Windows/Linux visible auto-launches aren't detected; add a distinct launch marker arg (e.g. AUTO_LAUNCH_FLAG) used when registering the app for auto-launch and update wasLaunchedAtLogin to return true if process.argv.includes(AUTO_LAUNCH_FLAG) || process.argv.includes(HIDDEN_FLAG) || (process.platform === 'darwin' && (app.getLoginItemSettings().wasOpenedAtLogin || app.getLoginItemSettings().wasOpenedAsHidden)); also update the auto-launch registration code (where you build the launch args) to include AUTO_LAUNCH_FLAG for all platforms while keeping HIDDEN_FLAG only to control UI behavior.
🧹 Nitpick comments (6)
surfsense_web/components/new-chat/model-selector.tsx (1)
329-330: Remove commented-out code.Leftover
// setOpen((prev) => !prev);(Line 329) and// onClick={() => setActiveTab(value)}(Line 978) should be removed now that the refactor is complete.🧹 Proposed cleanup
- // setOpen((prev) => !prev); handleOpenChange(!open);- // onClick={() => setActiveTab(value)} onClick={() => handleTabChange(value)}Also applies to: 978-979
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/new-chat/model-selector.tsx` around lines 329 - 330, Remove leftover commented-out code that remained after the refactor: delete the commented line "// setOpen((prev) => !prev);" adjacent to the call handleOpenChange(!open) and remove the commented "// onClick={() => setActiveTab(value)}" near where setActiveTab is used. Locate these comments by searching for handleOpenChange and setActiveTab in model-selector.tsx (e.g., inside the component surrounding the handleOpenChange(!open) call and the tab button/render logic) and simply delete the commented lines so only the active calls remain..github/workflows/desktop-release.yml (1)
23-25: Consider limitingid-token: writeto the Windows signing job.Currently, the workflow-level
id-token: writepermission (line 25) allows all matrix jobs (macOS, Ubuntu, Windows) to request OIDC tokens, even though only the Windows production signing path actually uses one. While theazure/loginstep (line 78) conditionally runs only for Windows, the capability to mint OIDC tokens exists for all jobs due to the workflow-level permission.To apply least-privilege principles, create separate jobs for each platform with
id-token: writescoped only to the Windows signing job:
- macOS build job (without
id-token)- Linux build job (without
id-token)- Windows signing job (with
id-token: write)This aligns with GitHub's OIDC guidance, which recommends job-level permissions when only specific jobs require token capability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/desktop-release.yml around lines 23 - 25, The workflow currently sets workflow-level permission "id-token: write" which grants OIDC token minting to all matrix jobs; restrict this by removing "id-token: write" from the top-level permissions block and instead add job-level permissions only to the Windows signing job that runs the azure/login step (the Windows signing job name or id you use), leaving the macOS and Linux build jobs without id-token; ensure the Windows job's job-level permissions include "id-token: write" and "contents: write" as needed while other jobs keep only "contents: write".surfsense_web/components/public-chat/public-thread.tsx (1)
21-21: Lazy-load the resume tool UI like the video tool to avoid bundling PDF viewer code unnecessarily.
GenerateResumeToolUIimportspdfjs-dist, which adds unnecessary weight to every public thread. Since this tool is only needed conditionally, convert it to dynamic import withssr: falseto match the video presentation pattern.⚡ Proposed lazy import
-import { GenerateResumeToolUI } from "@/components/tool-ui/generate-resume";const GenerateVideoPresentationToolUI = dynamic( () => import("@/components/tool-ui/video-presentation").then((m) => ({ default: m.GenerateVideoPresentationToolUI, })), { ssr: false } ); + +const GenerateResumeToolUI = dynamic( + () => + import("@/components/tool-ui/generate-resume").then((m) => ({ + default: m.GenerateResumeToolUI, + })), + { ssr: false } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/public-chat/public-thread.tsx` at line 21, The import of GenerateResumeToolUI pulls in pdfjs-dist and bloats the bundle; change the static import to a dynamic one like the video tool pattern so the PDF viewer code only loads when needed. Replace the top-level import of GenerateResumeToolUI with a dynamic import (using next/dynamic or your app's dynamic loader) with ssr: false and render the resulting dynamically-loaded component in the same place where GenerateResumeToolUI is used; keep the component name (GenerateResumeToolUI) as the reference so callers need no other changes.surfsense_web/atoms/chat/report-panel.atom.ts (1)
12-62: Consider a union type forcontentTypeinstead ofstring.The comment on Line 11 already enumerates the expected values (
"markdown" | "typst"). Using a string union would prevent typos at call sites (e.g.,surfsense_web/components/tool-ui/generate-resume.tsx) and let the compiler catch missed branches in panel/export switches that key offcontent_type.♻️ Proposed refinement
+export type ReportContentType = "markdown" | "typst"; + interface ReportPanelState { isOpen: boolean; reportId: number | null; title: string | null; wordCount: number | null; /** When set, uses public endpoints for fetching report data (public shared chat) */ shareToken: string | null; /** Content type of the report — "markdown" (default) or "typst" (resume) */ - contentType: string; + contentType: ReportContentType; }And apply the same type to
contentType?:in the action atom payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/atoms/chat/report-panel.atom.ts` around lines 12 - 62, Change the loose string typing for contentType to a string union to prevent typos and enable exhaustiveness checks: update the ReportPanelState definition so contentType is typed as "markdown" | "typst" (and set initialState.contentType to "markdown"), and update the openReportPanelAtom action payload signature so its contentType?: "markdown" | "typst" and the defaulting logic still uses "markdown". Ensure references to reportPanelAtom, initialState, ReportPanelState and openReportPanelAtom are updated consistently.surfsense_backend/app/agents/new_chat/tools/registry.py (1)
175-184: Nit: comment says "uses rendercv package" but the PR description and other comments describe the tool as Typst-based.Verify the comment accurately reflects the implementation in
tools/resume.py; otherwise update it to avoid confusing future contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_backend/app/agents/new_chat/tools/registry.py` around lines 175 - 184, The inline comment above the ToolDefinition for "generate_resume" incorrectly states "uses rendercv package"; inspect the implementation of create_generate_resume_tool in tools/resume.py and update the comment to match the actual implementation (e.g., "Typst-based" or the actual renderer used). Change the comment text near the ToolDefinition (the block creating the generate_resume tool) so it accurately describes the implementation used by create_generate_resume_tool and avoid referencing rendercv if it is not used.surfsense_web/components/shared/ExportMenuItems.tsx (1)
22-49:pdfOnlyis exposed on the sharedExportMenuItemsPropsbut onlyExportDropdownItemshonors it.
ExportContextItemsuses the same props interface yet silently ignorespdfOnly. If a caller passespdfOnlyto the context-menu variant (e.g., right-click on a Typst resume), it will still render every format. Either threadpdfOnlythroughExportContextItemsthe same way, or split the interfaces so the TypeScript surface reflects which component actually supports the flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@surfsense_web/components/shared/ExportMenuItems.tsx` around lines 22 - 49, The shared prop pdfOnly is defined on ExportMenuItemsProps and honored in ExportDropdownItems but ignored by ExportContextItems; update ExportContextItems to accept and respect the pdfOnly flag (or alternatively split the props so ExportContextItems uses a narrower interface) so behavior matches the type. Locate the ExportContextItems component and its prop typing, add pdfOnly?: boolean to its props (or create a new interface without pdfOnly), and gate rendering there the same way ExportDropdownItems does (only render the PDF item when pdfOnly is true, and still honor exporting state). Ensure both components’ prop types reflect the supported flags so callers get accurate TypeScript hints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e3a8e10-e3cf-4eb0-b2e2-93eb7d9af2af
⛔ Files ignored due to path filters (1)
surfsense_web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.github/workflows/desktop-release.ymlsurfsense_backend/alembic/versions/127_add_report_content_type.pysurfsense_backend/alembic/versions/128_seed_build_resume_prompt.pysurfsense_backend/app/agents/new_chat/system_prompt.pysurfsense_backend/app/agents/new_chat/tools/registry.pysurfsense_backend/app/agents/new_chat/tools/resume.pysurfsense_backend/app/db.pysurfsense_backend/app/prompts/system_defaults.pysurfsense_backend/app/routes/public_chat_routes.pysurfsense_backend/app/routes/reports_routes.pysurfsense_backend/app/schemas/reports.pysurfsense_backend/app/services/public_chat_service.pysurfsense_backend/app/tasks/chat/stream_new_chat.pysurfsense_desktop/src/ipc/channels.tssurfsense_desktop/src/ipc/handlers.tssurfsense_desktop/src/main.tssurfsense_desktop/src/modules/analytics.tssurfsense_desktop/src/modules/auto-launch.tssurfsense_desktop/src/modules/auto-updater.tssurfsense_desktop/src/modules/deep-links.tssurfsense_desktop/src/modules/folder-watcher.tssurfsense_desktop/src/modules/tray.tssurfsense_desktop/src/modules/window.tssurfsense_desktop/src/preload.tssurfsense_web/app/dashboard/[search_space_id]/new-chat/[[...chat_id]]/page.tsxsurfsense_web/app/dashboard/[search_space_id]/user-settings/components/DesktopContent.tsxsurfsense_web/app/dashboard/[search_space_id]/user-settings/components/PurchaseHistoryContent.tsxsurfsense_web/atoms/chat/report-panel.atom.tssurfsense_web/components/assistant-ui/assistant-message.tsxsurfsense_web/components/assistant-ui/connector-popup/constants/connector-constants.tssurfsense_web/components/assistant-ui/connector-popup/hooks/use-connector-dialog.tssurfsense_web/components/documents/CreateFolderDialog.tsxsurfsense_web/components/documents/FolderPickerDialog.tsxsurfsense_web/components/free-chat/anonymous-chat.tsxsurfsense_web/components/free-chat/free-chat-page.tsxsurfsense_web/components/free-chat/free-model-selector.tsxsurfsense_web/components/layout/ui/shell/LayoutShell.tsxsurfsense_web/components/layout/ui/sidebar/DocumentsSidebar.tsxsurfsense_web/components/new-chat/model-selector.tsxsurfsense_web/components/new-chat/source-detail-panel.tsxsurfsense_web/components/public-chat/public-chat-footer.tsxsurfsense_web/components/public-chat/public-thread.tsxsurfsense_web/components/report-panel/pdf-viewer.tsxsurfsense_web/components/report-panel/report-panel.tsxsurfsense_web/components/shared/ExportMenuItems.tsxsurfsense_web/components/theme/theme-toggle.tsxsurfsense_web/components/tool-ui/generate-report.tsxsurfsense_web/components/tool-ui/generate-resume.tsxsurfsense_web/contracts/types/stripe.types.tssurfsense_web/instrumentation-client.tssurfsense_web/lib/posthog/events.tssurfsense_web/package.jsonsurfsense_web/types/window.d.ts
| - name: Azure login (for Windows signing) | ||
| if: steps.sign.outputs.enabled == 'true' | ||
| uses: azure/login@v2 | ||
| with: | ||
| client-id: ${{ secrets.AZURE_CLIENT_ID }} | ||
| tenant-id: ${{ secrets.AZURE_TENANT_ID }} | ||
| subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect the desktop dependency versions/config paths that determine
# whether Azure Trusted Signing can authenticate from azure/login's Azure CLI context.
set -euo pipefail
echo "Electron builder / signing-related dependency entries:"
rg -n -C2 '"electron-builder"|@electron/windows-sign|azureSignOptions|TrustedSigning|Invoke-TrustedSigning' surfsense_desktop package.json pnpm-lock.yaml 2>/dev/null || true
echo
echo "Workflow signing auth/config references:"
rg -n -C3 'azure/login|enable-AzPSSession|AZURE_CLIENT|AZURE_TENANT|AZURE_FEDERATED|azureSignOptions|codeSigningAccountName' .github/workflows/desktop-release.ymlRepository: MODSetter/SurfSense
Length of output: 2430
🌐 Web query:
@electron/windows-sign Azure authentication credentials environment variables
💡 Result:
The @electron/windows-sign package, used by Electron Forge and electron-builder for Windows code signing, supports Azure Trusted Signing (now Artifact Signing) via Microsoft's Invoke-TrustedSigning PowerShell module or signtool.exe with specific DLLs. Authentication relies on Azure.Identity EnvironmentCredential, requiring specific environment variables. Required environment variables for service principal authentication (App Registration with secret, most common): - AZURE_TENANT_ID: Azure AD/Entra ID Tenant ID - AZURE_CLIENT_ID: Application (client) ID of the App Registration - AZURE_CLIENT_SECRET: Secret value created for the App Registration Other options (use one method): - AZURE_CLIENT_CERTIFICATE_PATH and AZURE_CLIENT_SEND_CERTIFICATE_CHAIN (client certificate) - AZURE_USERNAME and AZURE_PASSWORD (user account) These are set in CI/CD (e.g., GitHub Actions as secrets) or .env files (e.g., Electron Forge uses .env.trustedsigning with dotenv-cli). For electron-builder: Configure win.azureSignOptions in package.json (publisherName, codeSigningAccountName, certificateProfileName, endpoint), and provide env vars directly to the build process. For Electron Forge: Update @electron/windows-sign to 1.2.2+, create .env.trustedsigning, use signWithParams for signtool with /dlib and /dmdf. Sources confirm these vars are read by the TrustedSigning module for Entra ID authentication.
Citations:
- 1: https://hendrik-erz.de/post/code-signing-with-azure-trusted-signing-on-github-actions
- 2: https://www.electronforge.io/guides/code-signing/code-signing-windows
- 3: https://electron.build/code-signing-win
- 4: https://www.electron.build/code-signing-win.html
- 5: https://electron.build/code-signing-win.html
🌐 Web query:
azure/login GitHub action Node.js process credentials azure-cli
💡 Result:
The azure/login GitHub Action (latest v3.0.0 as of 2026-03-17) authenticates to Azure for use with Azure CLI (and optionally PowerShell) in GitHub Actions workflows. It is a Node.js/TypeScript action that internally executes az login using the Azure CLI after processing provided credentials. Key authentication methods: - Service Principal with secret: Use creds (JSON with clientSecret, subscriptionId, tenantId) or individual client-id, tenant-id, subscription-id, client-secret (via secrets). Deprecated in favor of OIDC. - OpenID Connect (OIDC, recommended): Provide client-id (Entra app or managed identity), tenant-id, subscription-id. Requires permissions: { id-token: write } in workflow. Exchanges GitHub OIDC JWT for Azure access token. The action (src/main.ts): - Initializes LoginConfig from inputs. - Calls AzureCliLogin.login, which runs az login with processed credentials (service principal details or OIDC token). For Node.js processes in subsequent steps: - Azure CLI login persists authentication in ~/.azure (CLI context). - Sets environment variables like AZURE_HTTP_USER_AGENT (via setUserAgent). - For OIDC, typically exchanges for access token and uses az login --service-principal with token as password, or sets env vars like AZURE_FEDERATION_TOKEN (inferred from standard OIDC flow, though exact vars not explicitly in snippets). - Subsequent Node.js scripts can use Azure SDKs with DefaultAzureCredential, which reads AZURE_CLIENT_ID, AZURE_TENANT_ID from env (set if provided) and CLI login state, or directly access process.env for CLI-set vars. Example workflow for Azure CLI + Node.js: permissions: id-token: write contents: read jobs: deploy: steps: - uses: azure/login@v3 with: client-id: ${{ secrets.AZURE_CLIENT_ID }} tenant-id: ${{ secrets.AZURE_TENANT_ID }} subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }} - run: az account show # Uses CLI login - run: node app.js # Node.js can use @azure/identity DefaultAzureCredential leveraging CLI auth No direct process.env credentials like raw secrets are exposed for security; auth is handled via CLI state and standard Azure env vars (e.g., AZURE_CLIENT_ID if set). Use Azure Identity library in Node.js for seamless integration post-login.
Citations:
- 1: https://github.com/Azure/login/
- 2: https://learn.microsoft.com/en-us/azure/developer/github/connect-from-azure
- 3: https://github.com/marketplace/actions/azure-cli-action
- 4: https://github.com/marketplace/actions/azure-login
- 5: https://github.com/Azure/login/blob/master/src/main.ts
- 6: https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-azure
🌐 Web query:
electron-builder Azure Trusted Signing authentication v26 v1.2.2
💡 Result:
To configure Azure Trusted Signing authentication in electron-builder version 26 (as of 2026-04-21, latest is around 26.8+), use the win.azureSignOptions property in your electron-builder configuration (e.g., package.json or electron-builder.yml). This feature was introduced in v25 and improved in v26. ### Configuration Example (package.json): { "build": { "win": { "azureSignOptions": { "publisherName": "Your Exact Certificate CN", "endpoint": "https://your-region.codesigning.azure.net/", "certificateProfileName": "your-profile-name", "codeSigningAccountName": "your-trusted-signing-account-name" // Additional Invoke-TrustedSigning params (optional): // "FileDigest": "SHA256", // "TimestampRfc3161": "http://timestamp.acs.microsoft.com", // "TimestampDigest": "SHA256" }, "publisherName": "Your Publisher Name (required for auto-updates)" } } } ### Authentication (Environment Variables): Set these in your CI/CD environment (e.g., GitHub Actions secrets). Use Service Principal (App Registration) method: - AZURE_TENANT_ID: Azure AD Tenant ID - AZURE_CLIENT_ID: App (client) ID from App Registration - AZURE_CLIENT_SECRET: Secret value from App Registration Assign "Trusted Signing Certificate Profile Signer" role to the App Registration in Azure portal. The Invoke-TrustedSigning PowerShell module (pinned to ~0.4.1-0.5.0) reads these directly. electron-builder installs it automatically during signing. ### Setup Prerequisites: 1. Create Azure Trusted Signing Account (now "Artifact Signing Account"). 2. Create Certificate Profile. 3. Create App Registration, generate secret, assign role. ### Notes for v26: - Full support since v26.0.0; fixes for concurrency, paths with spaces, pwsh detection. - Use signExts cautiously (potential module install issues). - No local .pfx needed; signing is remote. - For auto-updates, win.publisherName is required. Official docs: https://www.electron.build/code-signing-win.html#using-azure-trusted-signing-beta
Citations:
- 1: https://www.electron.build/code-signing-win.html
- 2: https://electron.build/code-signing-win.html
- 3: Support for Azure Trusted Signing in Electron Builder electron-userland/electron-builder#8276
- 4: feat(win): implement Azure Trusted Signing electron-userland/electron-builder#8458
- 5: Azure Trusted Signing when specifying signExts electron-userland/electron-builder#8700
- 6: Azure trusted signing Files param is malformed since v26.0.12 electron-userland/electron-builder#8985
- 7: chore(deploy): Release GA - v26.0.0 (electron-updater v6.4.0) electron-userland/electron-builder#8808
AZURE_CLIENT_SECRET environment variable is missing and the signing step will fail.
The workflow runs azure/login@v2 with client-id, tenant-id, and subscription-id, but @electron/windows-sign@1.2.2 (installed by electron-builder@26.8.1) requires three environment variables for Azure Trusted Signing authentication via the EnvironmentCredential path: AZURE_TENANT_ID, AZURE_CLIENT_ID, and AZURE_CLIENT_SECRET.
The workflow does not pass client-secret to azure/login, so AZURE_CLIENT_SECRET is never set in the environment. When the package step runs electron-builder, the Invoke-TrustedSigning module will fail to authenticate because the secret is missing. Either:
- Pass
client-secretto theazure/loginaction (if using service principal authentication), or - Set
AZURE_CLIENT_SECRETexplicitly as an environment variable before the package step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/desktop-release.yml around lines 78 - 84, The azure/login
step currently only supplies client-id, tenant-id, and subscription-id so
AZURE_CLIENT_SECRET is not exported and Windows signing (electron-builder /
`@electron/windows-sign`) will fail; update the Azure login step (the GitHub
Actions step named "Azure login (for Windows signing)" that uses azure/login@v2)
to include the client-secret parameter (client-secret: ${{
secrets.AZURE_CLIENT_SECRET }}) or alternatively set AZURE_CLIENT_SECRET as an
environment variable before the package/signing steps so the
Invoke-TrustedSigning flow can authenticate.
| def upgrade() -> None: | ||
| conn = op.get_bind() | ||
| columns = [c["name"] for c in sa.inspect(conn).get_columns("reports")] | ||
| if "content_type" in columns: | ||
| return | ||
| op.add_column( | ||
| "reports", | ||
| sa.Column( | ||
| "content_type", | ||
| sa.String(20), | ||
| nullable=False, | ||
| server_default="markdown", | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| op.drop_column("reports", "content_type") |
There was a problem hiding this comment.
Avoid silently claiming ownership of a pre-existing column.
If content_type already exists, upgrade() returns but Alembic still records revision 127 as applied; downgrade() can then drop a column this migration did not create. Prefer failing fast on unexpected schema drift, or track ownership before making downgrade destructive.
🛠️ Proposed fix: make the migration non-idempotent
def upgrade() -> None:
- conn = op.get_bind()
- columns = [c["name"] for c in sa.inspect(conn).get_columns("reports")]
- if "content_type" in columns:
- return
op.add_column(
"reports",
sa.Column(
"content_type",
sa.String(20),📝 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.
| def upgrade() -> None: | |
| conn = op.get_bind() | |
| columns = [c["name"] for c in sa.inspect(conn).get_columns("reports")] | |
| if "content_type" in columns: | |
| return | |
| op.add_column( | |
| "reports", | |
| sa.Column( | |
| "content_type", | |
| sa.String(20), | |
| nullable=False, | |
| server_default="markdown", | |
| ), | |
| ) | |
| def downgrade() -> None: | |
| op.drop_column("reports", "content_type") | |
| def upgrade() -> None: | |
| op.add_column( | |
| "reports", | |
| sa.Column( | |
| "content_type", | |
| sa.String(20), | |
| nullable=False, | |
| server_default="markdown", | |
| ), | |
| ) | |
| def downgrade() -> None: | |
| op.drop_column("reports", "content_type") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_backend/alembic/versions/127_add_report_content_type.py` around
lines 25 - 42, The migration currently returns early if "content_type" exists
which lets Alembic mark revision 127 as applied while the column may not be
owned by this revision; modify upgrade() to fail fast instead of returning:
after computing columns via sa.inspect(conn).get_columns("reports") check for
"content_type" and raise a clear exception (including current schema/column
info) so the migration aborts rather than silently skipping op.add_column; keep
downgrade() as destructive only when this migration actually created the column
by ensuring upgrade() either created it or raised an error (i.e., do not return
early), and reference upgrade(), downgrade(), op.add_column, and the
"content_type" check when making the change.
| def _build_header(template: dict[str, str], name: str) -> str: | ||
| """Build the template header with the person's name and current date.""" | ||
| now = datetime.now(tz=UTC) | ||
| return ( | ||
| template["header"] | ||
| .replace("{name}", name) | ||
| .replace("{year}", str(now.year)) | ||
| .replace("{month}", str(now.month)) | ||
| .replace("{day}", str(now.day)) | ||
| .replace("{month_name}", _MONTH_NAMES[now.month]) | ||
| ) |
There was a problem hiding this comment.
Escape the extracted name before inserting it into the Typst header.
name comes from LLM/user-controlled body content and is inserted inside Typst string literals. Quotes or backslashes in a real name can break compilation, and crafted text can alter the generated header.
🐛 Proposed fix
+def _typst_string(value: str) -> str:
+ """Escape a Python string for use inside a Typst string literal."""
+ return value.replace("\\", "\\\\").replace('"', '\\"')
+
+
def _build_header(template: dict[str, str], name: str) -> str:
"""Build the template header with the person's name and current date."""
now = datetime.now(tz=UTC)
+ escaped_name = _typst_string(name)
return (
template["header"]
- .replace("{name}", name)
+ .replace("{name}", escaped_name)
.replace("{year}", str(now.year))Also applies to: 553-555
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_backend/app/agents/new_chat/tools/resume.py` around lines 223 -
233, The _build_header function inserts unescaped user/LLM-provided name into a
Typst string literal, which can break compilation or allow injection; create a
small helper (e.g., escape_typst_literal(name: str) -> str) that escapes
backslashes, quotes, and Typst-special characters (at least \, ", {, }) and use
it to sanitize name before inserting in _build_header (and apply the same helper
at the other place that inserts a name into a Typst literal mentioned in the
review).
| async with shielded_async_session() as read_session: | ||
| if parent_report_id: | ||
| parent_report = await read_session.get(Report, parent_report_id) | ||
| if parent_report: | ||
| report_group_id = parent_report.report_group_id | ||
| parent_content = parent_report.content |
There was a problem hiding this comment.
Scope-check parent_report_id before reading report content.
read_session.get(Report, parent_report_id) accepts any report ID. A prompt/tool-call with another search space’s report ID can load that report’s content into the revision prompt and link the new report to its group. Validate search_space_id and content_type before using parent_content; if revisions are conversation-scoped, validate thread_id too.
🛡️ Proposed fix
if parent_report_id:
parent_report = await read_session.get(Report, parent_report_id)
- if parent_report:
+ if (
+ parent_report
+ and parent_report.search_space_id == search_space_id
+ and parent_report.content_type == "typst"
+ and (thread_id is None or parent_report.thread_id == thread_id)
+ ):
report_group_id = parent_report.report_group_id
parent_content = parent_report.content
logger.info(
f"[generate_resume] Revising from parent {parent_report_id} "
f"(group {report_group_id})"
)
+ elif parent_report:
+ error_msg = "The selected resume cannot be revised from this conversation."
+ report_id = await _save_failed_report(error_msg)
+ return {
+ "status": "failed",
+ "error": error_msg,
+ "report_id": report_id,
+ "title": "Resume",
+ "content_type": "typst",
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_backend/app/agents/new_chat/tools/resume.py` around lines 473 -
478, When loading a parent report inside the async with shielded_async_session()
block, don't blindly trust read_session.get(Report, parent_report_id); first
verify that parent_report.search_space_id equals the current search_space_id and
that parent_report.content_type matches the expected content_type (and if
revisions are conversation-scoped also verify parent_report.thread_id ==
thread_id) before assigning report_group_id or using parent_content; if any of
these checks fail, treat the parent as invalid (do not set report_group_id/use
parent_content and optionally log or raise a controlled error) so revisions
cannot link to or import content from a different scope.
| """Report model for storing generated reports (Markdown or Typst).""" | ||
|
|
||
| __tablename__ = "reports" | ||
|
|
||
| title = Column(String(500), nullable=False) | ||
| content = Column(Text, nullable=True) # Markdown body | ||
| content = Column(Text, nullable=True) | ||
| content_type = Column(String(20), nullable=False, server_default="markdown") |
There was a problem hiding this comment.
Constrain Report.content_type to supported values.
content_type currently accepts any string, but downstream rendering/preview paths only understand Markdown and Typst. Add a model-level/check constraint and mirror it in the migration so bad values cannot be persisted.
🛡️ Proposed model constraint
from sqlalchemy import (
ARRAY,
+ CheckConstraint,
JSON, class Report(BaseModel, TimestampMixin):
"""Report model for storing generated reports (Markdown or Typst)."""
__tablename__ = "reports"
+ __table_args__ = (
+ CheckConstraint(
+ "content_type IN ('markdown', 'typst')",
+ name="ck_reports_content_type",
+ ),
+ )
title = Column(String(500), nullable=False)
content = Column(Text, nullable=True)
- content_type = Column(String(20), nullable=False, server_default="markdown")
+ content_type = Column(
+ String(20),
+ nullable=False,
+ default="markdown",
+ server_default="markdown",
+ )📝 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.
| """Report model for storing generated reports (Markdown or Typst).""" | |
| __tablename__ = "reports" | |
| title = Column(String(500), nullable=False) | |
| content = Column(Text, nullable=True) # Markdown body | |
| content = Column(Text, nullable=True) | |
| content_type = Column(String(20), nullable=False, server_default="markdown") | |
| """Report model for storing generated reports (Markdown or Typst).""" | |
| __tablename__ = "reports" | |
| __table_args__ = ( | |
| CheckConstraint( | |
| "content_type IN ('markdown', 'typst')", | |
| name="ck_reports_content_type", | |
| ), | |
| ) | |
| title = Column(String(500), nullable=False) | |
| content = Column(Text, nullable=True) | |
| content_type = Column( | |
| String(20), | |
| nullable=False, | |
| default="markdown", | |
| server_default="markdown", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_backend/app/db.py` around lines 1201 - 1207, The Report model's
content_type Column currently allows any string but must be limited to
"markdown" or "typst"; add a SQLAlchemy CheckConstraint to the Report model
(e.g., on class Report / __tablename__ = "reports") enforcing content_type IN
('markdown','typst') and set server_default to 'markdown' if not already, and
then update the corresponding migration to create the same CHECK constraint on
the reports table so invalid values cannot be persisted; ensure the constraint
name is explicit (e.g., reports_content_type_check) to make future
migrations/rollbacks predictable.
| if (oauthConnector) { | ||
| trackConnectorSetupFailure( | ||
| Number(searchSpaceId), | ||
| oauthConnector.connectorType, | ||
| result.error, | ||
| "oauth_callback" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid sending raw connector error messages to analytics.
These new failure events forward result.error / Error.message into PostHog. Connector setup errors can include provider responses, backend details, or user-supplied config context; keep analytics to bounded reason codes and reserve full errors for local logs.
🛡️ Proposed direction
- result.error,
+ result.error === "duplicate_account" ? "duplicate_account" : "oauth_callback_failed",
"oauth_callback"
);- error instanceof Error ? error.message : "oauth_initiation_failed",
+ "oauth_initiation_failed",
"oauth_init"
);- error instanceof Error ? error.message : "webcrawler_create_failed",
+ "webcrawler_create_failed",
"webcrawler_quick_add"
);- error instanceof Error ? error.message : "connector_create_failed",
+ "connector_create_failed",
"non_oauth_form"
);Also applies to: 390-395, 473-478, 724-729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@surfsense_web/components/assistant-ui/connector-popup/hooks/use-connector-dialog.ts`
around lines 242 - 249, The code is currently passing raw error text
(result.error / Error.message) to analytics via trackConnectorSetupFailure (used
e.g. where oauthConnector is handled with searchSpaceId and
oauthConnector.connectorType); replace that by mapping the error to a bounded
reason code or sanitized enum (e.g., create/get a function
sanitizeConnectorError(result.error) or use an existing errorReasonCode) and
pass only the sanitized code to trackConnectorSetupFailure, while sending the
full error details to local logs (console/logger) for debugging; apply the same
replacement for the other occurrences that forward result.error (the other calls
of trackConnectorSetupFailure noted in the review).
| const handleOpenChange = useCallback( | ||
| (next: boolean) => { | ||
| if (next) { | ||
| setSelectedId(null); | ||
| setExpandedIds(new Set()); | ||
| } | ||
| onOpenChange(next); | ||
| }, | ||
| [onOpenChange] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Radix UI Dialog call onOpenChange when the controlled open prop is changed by the parent?
💡 Result:
No, the Radix UI Dialog component does not call onOpenChange when the controlled open prop is changed by the parent. Multiple developer reports and GitHub discussions confirm that onOpenChange is only triggered by internal state changes (e.g., user interactions like clicking Dialog.Close or overlay), not when the parent updates the open prop directly. For example, setting open to true via a button onClick outside Radix components does not fire onOpenChange, while closing via Radix components does. This behavior is described as intentional in discussions, to avoid infinite loops when using onOpenChange={setOpen}.
Citations:
- 1: onOpenChange callback only fires on close in Radix dialog radix-ui/primitives#3300
- 2: Radix Dialog - Events for open and close separately radix-ui/primitives#1678
🏁 Script executed:
fd -t f "FolderPickerDialog.tsx" --exec cat -n {} \;Repository: MODSetter/SurfSense
Length of output: 6353
🏁 Script executed:
rg "FolderPickerDialog" --type tsx --type ts -lRepository: MODSetter/SurfSense
Length of output: 90
🏁 Script executed:
rg "FolderPickerDialog" --type typescript --type jsx -lRepository: MODSetter/SurfSense
Length of output: 97
🏁 Script executed:
rg "FolderPickerDialog" -A 5 -B 2Repository: MODSetter/SurfSense
Length of output: 3850
Reset on open won't run for controlled dialogs—state will leak between sessions.
Radix Dialog's onOpenChange only fires in response to internal interactions (Escape, overlay click, close button). When the parent toggles open from false → true programmatically (as in DocumentsSidebar), onOpenChange is never invoked, so the reset in lines 42-43 never runs. This causes selectedId and expandedIds from the previous session to persist into the next open.
Fix this by:
- Adding
useEffect(() => { if (open) { setSelectedId(null); setExpandedIds(new Set()); } }, [open])to reset on prop change, or - Resetting on close (
next === falsein handleOpenChange), though this still misses the programmatic-close case
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_web/components/documents/FolderPickerDialog.tsx` around lines 39 -
48, The reset logic that clears selectedId and expandedIds inside
handleOpenChange will not run when the parent controls the Dialog via the open
prop, so add a useEffect that watches the open prop and when open becomes true
call setSelectedId(null) and setExpandedIds(new Set()) to ensure state is
cleared on programmatic opens; keep handleOpenChange as-is for internal
interactions but rely on the new useEffect (dependent on open) to perform the
reset so state doesn't leak between sessions.
| {reportContent?.content_type === "typst" ? ( | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => handleExport("pdf")} | ||
| disabled={isLoading || !reportContent?.content || exporting !== null} | ||
| className={`h-8 min-w-[100px] px-3.5 py-4 text-[15px] ${btnBg} select-none`} | ||
| > | ||
| <ExportDropdownItems | ||
| onExport={handleExport} | ||
| exporting={exporting} | ||
| showAllFormats={!shareToken} | ||
| /> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| {exporting === "pdf" ? <Spinner size="xs" /> : "Download"} | ||
| </Button> |
There was a problem hiding this comment.
Typst "Download" button is broken for public share-token viewers.
handleExport("pdf") on Line 230 unconditionally hits authenticatedFetch(.../api/v1/reports/${activeReportId}/export?format=...), which is the authenticated endpoint. A public viewer loading this panel via shareToken has no valid session and the request will 401. For non-Typst reports the dropdown falls back to client-side markdown export via showAllFormats={!shareToken}, but the Typst branch offers no such fallback — the visible Download button is the only action and it can't succeed.
Either route through a public export endpoint when shareToken is set (mirroring the /preview split just above on Line 399), or hide the Download button for shareToken users.
🛠 Example: hide Download for public viewers (or add a public export branch)
- {/* Export — plain button for resume (typst), dropdown for others */}
- {reportContent?.content_type === "typst" ? (
+ {/* Export — plain button for resume (typst), dropdown for others */}
+ {reportContent?.content_type === "typst" ? (
+ !shareToken && (
<Button
variant="outline"
size="sm"
onClick={() => handleExport("pdf")}
disabled={isLoading || !reportContent?.content || exporting !== null}
className={`h-8 min-w-[100px] px-3.5 py-4 text-[15px] ${btnBg} select-none`}
>
{exporting === "pdf" ? <Spinner size="xs" /> : "Download"}
</Button>
+ )
) : (The preferred fix is to add a public export route on the backend and branch handleExport on shareToken, so shared Typst reports remain downloadable.
📝 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.
| {reportContent?.content_type === "typst" ? ( | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| onClick={() => handleExport("pdf")} | |
| disabled={isLoading || !reportContent?.content || exporting !== null} | |
| className={`h-8 min-w-[100px] px-3.5 py-4 text-[15px] ${btnBg} select-none`} | |
| > | |
| <ExportDropdownItems | |
| onExport={handleExport} | |
| exporting={exporting} | |
| showAllFormats={!shareToken} | |
| /> | |
| </DropdownMenuContent> | |
| </DropdownMenu> | |
| {exporting === "pdf" ? <Spinner size="xs" /> : "Download"} | |
| </Button> | |
| {reportContent?.content_type === "typst" ? ( | |
| !shareToken && ( | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| onClick={() => handleExport("pdf")} | |
| disabled={isLoading || !reportContent?.content || exporting !== null} | |
| className={`h-8 min-w-[100px] px-3.5 py-4 text-[15px] ${btnBg} select-none`} | |
| > | |
| {exporting === "pdf" ? <Spinner size="xs" /> : "Download"} | |
| </Button> | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_web/components/report-panel/report-panel.tsx` around lines 312 -
321, The Typst "Download" button calls handleExport("pdf") which always uses the
authenticated export endpoint and 401s for public viewers; update handleExport
(and the report-panel rendering around reportContent/content_type, exporting and
the Button) to branch on shareToken: when shareToken is present call the public
preview export endpoint (e.g.,
/api/v1/reports/${activeReportId}/preview/export?format=pdf) instead of the
authenticated route, or alternatively hide/disable the Download Button for
shareToken users by checking shareToken before rendering (so the UI falls back
to the client-side export path used for non-typst reports); ensure you reference
handleExport, reportContent, shareToken, exporting and the Button rendering so
the fix is applied consistently.
| <button | ||
| type="button" | ||
| onClick={handleOpen} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| handleOpen(); | ||
| } | ||
| }} | ||
| className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:outline-none cursor-pointer" | ||
| > |
There was a problem hiding this comment.
Restore a visible keyboard focus state.
The card is now a real button, but focus:outline-none focus-visible:outline-none removes the visible focus indicator. Add a replacement ring so keyboard users can see where focus is.
Proposed fix
<button
type="button"
onClick={handleOpen}
- className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:outline-none cursor-pointer"
+ className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 cursor-pointer"
>📝 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.
| <button | |
| type="button" | |
| onClick={handleOpen} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| handleOpen(); | |
| } | |
| }} | |
| className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:outline-none cursor-pointer" | |
| > | |
| <button | |
| type="button" | |
| onClick={handleOpen} | |
| className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 cursor-pointer" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_web/components/tool-ui/generate-report.tsx` around lines 222 - 226,
The button element that triggers handleOpen currently strips all focus
indicators via the classes focus:outline-none and focus-visible:outline-none;
restore an accessible keyboard focus by removing the focus-visible:outline-none
(or at least not suppressing focus-visible) and add a visible ring on keyboard
focus (for example using focus-visible:ring-2 and a color class like
focus-visible:ring-primary or focus-visible:ring-indigo-500 and optionally
focus-visible:ring-offset-2) so the button remains visually highlighted when
focused while keeping existing hover styles; update the className on the button
around the handleOpen handler accordingly.
| <button | ||
| type="button" | ||
| onClick={handleOpen} | ||
| className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:outline-none cursor-pointer select-none" |
There was a problem hiding this comment.
Restore a visible keyboard focus state.
The card is keyboard-focusable, but focus-visible:outline-none removes the focus indicator without replacing it, making the action hard to operate via keyboard.
♿ Proposed fix
onClick={handleOpen}
- className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:outline-none cursor-pointer select-none"
+ className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2 cursor-pointer select-none"
>📝 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.
| <button | |
| type="button" | |
| onClick={handleOpen} | |
| className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:outline-none cursor-pointer select-none" | |
| <button | |
| type="button" | |
| onClick={handleOpen} | |
| className="w-full text-left transition-colors hover:bg-muted/50 focus:outline-none focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2 cursor-pointer select-none" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_web/components/tool-ui/generate-resume.tsx` around lines 239 - 242,
The button that triggers handleOpen currently strips the keyboard focus
indicator via the focus-visible:outline-none class; restore an accessible focus
state by removing focus-visible:outline-none and adding a visible focus style
(for example a focus-visible:ring, focus-visible:ring-offset, or
focus-visible:outline-* utility) so keyboard users see a clear focus ring on the
element when it receives focus; update the JSX button element that wraps
handleOpen to include the chosen focus-visible utility classes and keep other
classes unchanged.
- Updated the HTTP exception handler to sanitize 500 responses, replacing them with a generic message to prevent sensitive information leakage. - Preserved detailed messages for other 5xx statuses (e.g., 503, 502) to provide user-friendly feedback while logging the errors server-side. - Added unit tests to ensure that specific 5xx responses retain their detail for frontend rendering.
4 participants
Summary
This PR bundles a major new feature alongside several desktop app improvements and quality fixes across the web UI.
Features
content_typecolumn on reports,GenerateResumeToolUIcomponent, public report PDF preview endpoint, and updated system prompts (feat: resume builder #1240).Fixes
theme-toggle: use functionalsetIsDarkintoggleTheme(Use functionalsetIsDarkintoggleThemeto remove stale closure dependency #1247 / fix(theme-toggle): use functional setIsDark in toggleTheme (#1247) #1272).dialogs: move open-reset effects intoonOpenChangehandlers (fix(dialogs): move open-reset effects into onOpenChange handlers #1268).new-chat: remove unused_hasScrolledToCitedstate in source details (fix(new-chat): remove unused _hasScrolledToCited state in source-detail-panel #1255).Refactors / Performance
DocumentTabContentto reduce initial dashboard bundle size (perf: lazy-load DocumentTabContent to reduce initial dashboard bundle… #1273).isExportingKBstate to a ref indocuments-sidebar(refactor(documents-sidebar): convert discarded isExportingKB state to ref #1267).react-pdfforpdfjs-dist, improved thumbnail loading, public access handling, page-render perf).Test Plan
DocumentTabContentloads on demand.High-level PR Summary
This PR introduces a Typst-based resume builder tool that generates PDF resumes from user information, adds desktop app analytics tracking with PostHog, implements Windows code-signing for releases, enables auto-launch on system startup for the desktop app, and includes several UI/performance fixes: theme toggle now uses functional state updates, dialog reset logic moved to
onOpenChangehandlers,DocumentTabContentlazy-loaded to reduce bundle size, model selector reset logic moved to event handlers, unused state removed, PDF viewer streamlined by replacingreact-pdfwithpdfjs-dist, connector analytics tracking centralized, anonymous chat analytics added, and purchase history UI updated to show both page and token purchases.⏱️ Estimated Review Time: 1-3 hours
💡 Review Order Suggestion
surfsense_backend/alembic/versions/127_add_report_content_type.pysurfsense_backend/alembic/versions/128_seed_build_resume_prompt.pysurfsense_backend/app/db.pysurfsense_backend/app/prompts/system_defaults.pysurfsense_backend/app/schemas/reports.pysurfsense_backend/app/agents/new_chat/tools/resume.pysurfsense_backend/app/agents/new_chat/tools/registry.pysurfsense_backend/app/agents/new_chat/system_prompt.pysurfsense_backend/app/routes/reports_routes.pysurfsense_backend/app/routes/public_chat_routes.pysurfsense_backend/app/services/public_chat_service.pysurfsense_backend/app/tasks/chat/stream_new_chat.pysurfsense_web/components/tool-ui/generate-resume.tsxsurfsense_web/components/report-panel/pdf-viewer.tsxsurfsense_web/atoms/chat/report-panel.atom.tssurfsense_web/components/report-panel/report-panel.tsxsurfsense_web/components/assistant-ui/assistant-message.tsxsurfsense_web/components/public-chat/public-thread.tsxsurfsense_web/app/dashboard/[search_space_id]/new-chat/[[...chat_id]]/page.tsxsurfsense_web/package.jsonsurfsense_web/pnpm-lock.yaml.github/workflows/desktop-release.ymlsurfsense_desktop/src/modules/auto-launch.tssurfsense_desktop/src/modules/analytics.tssurfsense_desktop/src/ipc/channels.tssurfsense_desktop/src/ipc/handlers.tssurfsense_desktop/src/preload.tssurfsense_desktop/src/main.tssurfsense_desktop/src/modules/window.tssurfsense_desktop/src/modules/deep-links.tssurfsense_desktop/src/modules/tray.tssurfsense_desktop/src/modules/folder-watcher.tssurfsense_desktop/src/modules/auto-updater.tssurfsense_web/app/dashboard/[search_space_id]/user-settings/components/DesktopContent.tsxsurfsense_web/instrumentation-client.tssurfsense_web/lib/posthog/events.tssurfsense_web/components/assistant-ui/connector-popup/constants/connector-constants.tssurfsense_web/components/assistant-ui/connector-popup/hooks/use-connector-dialog.tssurfsense_web/types/window.d.tssurfsense_web/components/theme/theme-toggle.tsxsurfsense_web/components/layout/ui/shell/LayoutShell.tsxsurfsense_web/components/layout/ui/sidebar/DocumentsSidebar.tsxsurfsense_web/components/new-chat/model-selector.tsxsurfsense_web/components/new-chat/source-detail-panel.tsxsurfsense_web/components/documents/CreateFolderDialog.tsxsurfsense_web/components/documents/FolderPickerDialog.tsxsurfsense_web/components/free-chat/free-model-selector.tsxsurfsense_web/components/free-chat/free-chat-page.tsxsurfsense_web/components/free-chat/anonymous-chat.tsxsurfsense_web/app/dashboard/[search_space_id]/user-settings/components/PurchaseHistoryContent.tsxsurfsense_web/contracts/types/stripe.types.tssurfsense_web/components/shared/ExportMenuItems.tsxsurfsense_web/components/public-chat/public-chat-footer.tsxSummary by CodeRabbit
New Features
Improvements