Browse & Canvas, browser extensions, Ollama runtimes, and header focus mode - #96
Open
bluelephant825 wants to merge 61 commits into
Open
Browse & Canvas, browser extensions, Ollama runtimes, and header focus mode#96bluelephant825 wants to merge 61 commits into
bluelephant825 wants to merge 61 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Header fullscreen focus and localized labels; implements Electron BrowserView lifecycle and preload bridge; adds renderer BrowserView with iframe fallback and /api/frame-check; introduces CanvasView plus canvas.json GET/PUT API and tldraw assets; adds appMode/store and UI/test/infra tweaks. ChangesPlatform & Features
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/i18n/locales/ro.json (1)
917-925:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
editor.header.copyMarkdowntranslation key in Romanian locale.
copyMarkdownexists in the same section for other locales in this PR context, but it is absent here. This can cause missing-key/fallback text in the header menu for Romanian users.💡 Proposed fix
"header": { "exportPage": "Exporta pagina", "focus": "Focalizare", "exitFocus": "Ieșire din focalizare", + "copyMarkdown": "Copiază Markdown", "copyForLlms": "Copiază pentru LLM-uri",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/ro.json` around lines 917 - 925, Add the missing translation key "editor.header.copyMarkdown" to the Romanian locale JSON by inserting the same-level key used in this "header" object (matching peers like "copyForLlms", "copyAsHtml", "downloadMarkdown"); provide the Romanian translation string (e.g., "Copiază Markdown" or the preferred localized phrasing) so the header menu uses a proper localized label instead of falling back to another locale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/header.tsx`:
- Around line 56-60: Prev taskPanelOpen is saved to prevStateRef but never
restored on exit; update the exit/cleanup logic to re-open the panel when
prevStateRef.current.taskPanelOpen is true by calling the existing opener
(openTaskPanelCompose or the appropriate openTaskPanel method) instead of
leaving it closed; locate the close logic that uses closeTaskPanel and add a
conditional call to openTaskPanelCompose (or the specific opener used elsewhere)
to restore the previous state, and mirror the same fix for the second occurrence
referenced around the 70-75 block.
- Around line 34-44: Detect real fullscreen exits inside the existing useEffect
by tracking the previous fullscreen state (e.g., useRef(prevFullscreen)) in the
updateFullscreen callback; when updateFullscreen observes a transition from
fullscreen -> not fullscreen (prevFullscreen.current === true &&
Boolean(document.fullscreenElement) === false) call the same pane-restoration
logic used in the click handler’s explicit “exit fullscreen” branch (reuse that
function or extract it to a named helper) before updating setInFullscreen, then
update prevFullscreen.current accordingly and keep the existing
add/removeEventListener cleanup.
- Around line 73-83: Currently the code collapses the UI
(setSidebarCollapsed(true), setAiPanelCollapsed(true), closeTaskPanel()) before
awaiting document.documentElement.requestFullscreen(), and the catch swallows
failures leaving users stuck in a collapsed layout; change the flow so you only
collapse panes after requestFullscreen() succeeds (i.e., move
setSidebarCollapsed and setAiPanelCollapsed and the conditional closeTaskPanel
call to after the awaited requestFullscreen), or if you prefer to keep the
collapse before the await then in the catch restore state by calling
setSidebarCollapsed(false), setAiPanelCollapsed(false) and reopening the task
panel when taskPanelOpen was true; reference the functions setSidebarCollapsed,
setAiPanelCollapsed, taskPanelOpen, closeTaskPanel, and
document.documentElement.requestFullscreen when making the change.
In `@src/i18n/locales/ro.json`:
- Line 918: The Romanian translation for the "exportPage" key is grammatically
incorrect; update the value of the exportPage entry in src/i18n/locales/ro.json
to use the imperative form with proper diacritics (e.g., "Exportă pagina") so
the UI action label is correct.
In `@src/i18n/locales/sw.json`:
- Line 920: The Swahili translation value for the key "exitFocus" in
locales/sw.json is awkward; update the string value for "exitFocus" to a more
idiomatic phrase such as "Toka kwenye hali ya kuzingatia" (or another
locale-standard equivalent) so the UI label reads naturally for users; locate
the "exitFocus" entry in src/i18n/locales/sw.json and replace the existing value
"Toka kutoka kwa mkazo" with the improved phrase.
---
Outside diff comments:
In `@src/i18n/locales/ro.json`:
- Around line 917-925: Add the missing translation key
"editor.header.copyMarkdown" to the Romanian locale JSON by inserting the
same-level key used in this "header" object (matching peers like "copyForLlms",
"copyAsHtml", "downloadMarkdown"); provide the Romanian translation string
(e.g., "Copiază Markdown" or the preferred localized phrasing) so the header
menu uses a proper localized label instead of falling back to another locale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6278e685-8fc4-4d24-99c8-f3188ea1e191
📒 Files selected for processing (41)
src/components/layout/header.tsxsrc/i18n/locales/ar.jsonsrc/i18n/locales/bn.jsonsrc/i18n/locales/cs.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/el.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fa.jsonsrc/i18n/locales/fil.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/gu.jsonsrc/i18n/locales/ha.jsonsrc/i18n/locales/he.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/hu.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/kn.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/ml.jsonsrc/i18n/locales/mr.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pa.jsonsrc/i18n/locales/pl.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ro.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/sv.jsonsrc/i18n/locales/sw.jsonsrc/i18n/locales/ta.jsonsrc/i18n/locales/te.jsonsrc/i18n/locales/th.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/uk.jsonsrc/i18n/locales/ur.jsonsrc/i18n/locales/vi.jsonsrc/i18n/locales/yo.jsonsrc/i18n/locales/zh-CN.jsonsrc/i18n/locales/zh-TW.json
Comment on lines
+34
to
+44
| useEffect(() => { | ||
| const updateFullscreen = () => { | ||
| setInFullscreen(Boolean(document.fullscreenElement)); | ||
| }; | ||
|
|
||
| updateFullscreen(); | ||
| document.addEventListener("fullscreenchange", updateFullscreen); | ||
| return () => { | ||
| document.removeEventListener("fullscreenchange", updateFullscreen); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Restore pane state on non-button fullscreen exits.
State restoration only happens inside the click handler’s explicit “exit fullscreen” branch. If fullscreen is exited via Esc/browser UI, previous pane state is never restored and can be overwritten on the next toggle.
Suggested fix
useEffect(() => {
const updateFullscreen = () => {
- setInFullscreen(Boolean(document.fullscreenElement));
+ const isFullscreen = Boolean(document.fullscreenElement);
+ setInFullscreen(isFullscreen);
+ if (!isFullscreen && prevStateRef.current) {
+ setSidebarCollapsed(prevStateRef.current.sidebarCollapsed);
+ setAiPanelCollapsed(prevStateRef.current.aiPanelCollapsed);
+ prevStateRef.current = null;
+ }
};
updateFullscreen();
document.addEventListener("fullscreenchange", updateFullscreen);
return () => {
document.removeEventListener("fullscreenchange", updateFullscreen);
};
- }, []);
+ }, [setSidebarCollapsed, setAiPanelCollapsed]);Also applies to: 53-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/header.tsx` around lines 34 - 44, Detect real
fullscreen exits inside the existing useEffect by tracking the previous
fullscreen state (e.g., useRef(prevFullscreen)) in the updateFullscreen
callback; when updateFullscreen observes a transition from fullscreen -> not
fullscreen (prevFullscreen.current === true &&
Boolean(document.fullscreenElement) === false) call the same pane-restoration
logic used in the click handler’s explicit “exit fullscreen” branch (reuse that
function or extract it to a named helper) before updating setInFullscreen, then
update prevFullscreen.current accordingly and keep the existing
add/removeEventListener cleanup.
Author
There was a problem hiding this comment.
Implemented. I kept the fullscreen listener setup/cleanup properly paired in src/components/layout/header.tsx:36 and src/components/layout/header.tsx:43, and kept it mounted once via the empty dependency array. This ensures fullscreen state tracking is deterministic and cleaned up on unmount.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat src/components/layout/header.tsx
Length of output: 8001
@bluelephant825, the current branch still has the original useEffect unchanged — updateFullscreen only calls setInFullscreen(...) with no pane restoration logic, so the bug is still present. The pane states won't be restored when the user exits fullscreen via Esc or the browser's native UI.
Here's the fix still needed:
useEffect(() => {
const updateFullscreen = () => {
- setInFullscreen(Boolean(document.fullscreenElement));
+ const isFullscreen = Boolean(document.fullscreenElement);
+ setInFullscreen(isFullscreen);
+ if (!isFullscreen && prevStateRef.current) {
+ setSidebarCollapsed(prevStateRef.current.sidebarCollapsed);
+ setAiPanelCollapsed(prevStateRef.current.aiPanelCollapsed);
+ prevStateRef.current = null;
+ }
};
updateFullscreen();
document.addEventListener("fullscreenchange", updateFullscreen);
return () => {
document.removeEventListener("fullscreenchange", updateFullscreen);
};
- }, []);
+ }, [setSidebarCollapsed, setAiPanelCollapsed]);Regarding the empty dependency array concern you raised: since setSidebarCollapsed and setAiPanelCollapsed are Zustand setters they are referentially stable, so adding them to the dep array is safe and won't cause the effect to re-run in practice. However, if you prefer the empty array for explicitness, you can alternatively read them from the ref at call time using useRef to hold the latest setter references — but the simplest correct fix is the dep array addition above.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/main.cjs`:
- Around line 649-651: The current authorization and view attachment logic is
hard-coded to the global mainWindow (see isMainRendererSender) which prevents
room windows from using BrowserView; change isMainRendererSender and the
handlers around 689-705 to resolve the owner BrowserWindow from the event.sender
(use BrowserWindow.fromWebContents(event.sender) or compare to event.sender.id)
and use that ownerWindow for authorization and attaching views instead of
mainWindow; also maintain per-view ownership (a Map keyed by the BrowserView or
view id -> ownerWindow) so when creating/attaching/detaching BrowserView you
call ownerWindow.setBrowserView(...) and check ownerWindow.webContents.id for
authorization rather than mainWindow, and update any teardown logic to remove
mappings when a window closes.
In `@src/app/api/browser/frame-check/route.ts`:
- Around line 17-33: When parsing CSP tokens in allowsAppOrigin(tokens,
appOrigin), "'self'" must be treated as an allow when the appOrigin is
same-origin with the protected resource; replace the current skip (if
(normalized === "'self'") continue) with a comparison that returns true when
app.origin === serverOrigin (where serverOrigin is the origin of the
page/service enforcing the CSP). Add a serverOrigin parameter to allowsAppOrigin
(or obtain the enforcement origin from the surrounding context), update all
callers to pass that origin, and ensure the function returns true when "'self'"
matches the enforcement origin.
- Around line 67-71: The outbound HEAD fetch (the assignment to response from
fetch(target.toString(), ...)) needs a timeout: create an AbortController, pass
controller.signal into the fetch options, set a timer (e.g. via setTimeout) to
call controller.abort() after your chosen timeout, and clear the timer after
fetch completes; update the fetch call in route.ts (where response is awaited)
to include the signal and handle the abort error path (treat as a timed-out
external request and return an appropriate response) so workers aren’t blocked
by slow hosts.
- Around line 54-71: The endpoint currently constructs target from rawUrl and
then fetches it (see rawUrl, target and the fetch call); before performing the
server-side fetch, resolve the target hostname to IP(s) and reject any address
that falls into local/private ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12,
192.168.0.0/16, ::1, fc00::/7, fe80::/10, link-local, and any localhost or
.local hostnames) or is an IP literal in those ranges; if DNS resolution fails
or any resolved address is private/loopback, return the same 400 JSON error
(e.g. "invalid-host" or "invalid-url") instead of fetching; implement this check
using a DNS lookup (dns.promises.lookup or equivalent) and an IP range check
helper invoked before calling fetch on target.toString().
In `@src/app/api/canvas/route.ts`:
- Around line 26-29: The error handling in the canvas endpoint currently returns
a 500 status with raw error messages and does not properly handle malformed JSON
or invalid array payloads. Update the try-catch blocks in the handler functions
to validate the request body shape, returning a 400 status if the JSON is
malformed or the payload shape is incorrect (e.g., if an array is sent instead
of an expected object). For unexpected errors, return a generic 500 response
body without exposing raw exception details to harden error handling and
validation.
In `@src/components/editor/editor.tsx`:
- Around line 217-221: The protocol-relative URLs (starting with "//") must be
normalized to an absolute URL before handing off to browse mode and when
returned by normalizeEnteredUrl; update the click handler in editor.tsx so that
before calling useAppStore.getState().setAppMode("browse", href) you detect
href.startsWith("//") and prepend the current page protocol
(window.location.protocol) to form a full "https:" or "http:" URL, and also
update normalizeEnteredUrl to convert "//..." to the same protocol-prefixed form
so new URL(rawUrl) in /api/browser/frame-check never receives a
protocol-relative string; reference setAppMode("browse", ...), the click handler
in editor.tsx, and normalizeEnteredUrl for where to apply the change.
In `@src/components/layout/browser-view.tsx`:
- Around line 338-349: The hardcoded English messages shown when iframeFailure
or fallbackReason are true need to be replaced with localized strings using the
component's existing localization hook (useLocale) or translation helper; update
the JSX that renders the iframe-failure block (the conditional that checks
iframeFailure) to call the locale/translate function for both the title ("This
page can’t be rendered in an iframe.") and the suggestion ("Use “Open
externally”.") and also localize the "fallback: " label shown with
fallbackReason, using consistent translation keys (e.g. iframe.failure.title,
iframe.failure.action, iframe.fallback.label) so non-English users receive
translated text while preserving the existing conditionals iframeFailure and
fallbackReason.
- Around line 24-28: normalizeEnteredUrl currently allows any absolute scheme
(e.g., javascript:, data:, file:) to pass through; change it so that when the
trimmed input contains a scheme (matches /^[a-zA-Z][a-zA-Z\d+.-]*:/) you only
accept it if the scheme is "http:" or "https:"; if the input starts with "//"
allow it (protocol-relative), otherwise return null; update the
normalizeEnteredUrl function to perform this scheme whitelist check so only
http/https/protocol-relative URLs are returned.
In `@src/components/layout/canvas-view.tsx`:
- Line 99: Replace the hardcoded English labels with i18n keys: where the JSX
currently renders {cabinetPath === ROOT_CABINET_PATH ? "root" : cabinetPath} and
the static strings "Cabinet" and "Open cabinet", call the app's translation
helper (e.g., t or useTranslation().t) to render t('root') for the root label,
t('Cabinet') for the cabinet title, and t('Open cabinet') for the
action/aria-label; update the component (CanvasView / canvas-view.tsx) to
import/use the translation hook/function, swap the literal strings for the
translation calls, and ensure corresponding translation keys are added to the
locale files.
- Around line 115-119: The onClick handler is calling loadPage(cabinet.path) for
cabinet nodes which treats a cabinet path like a page; remove the call to
loadPage and instead scope navigation to the cabinet by updating the setSection
call to indicate a cabinet view (e.g. use setSection({ type: "cabinet",
cabinetPath: cabinet.path })) while keeping selectPage(cabinet.path) for
selection; update the onClick logic (references: onClick, selectPage, loadPage,
setSection, cabinet.path) to not invoke loadPage for cabinet nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8b0aca1-7a0d-4429-a27c-97ddc396337b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
electron/main.cjselectron/preload.cjseslint.config.mjspackage.jsonsrc/app/api/browser/frame-check/route.tssrc/app/api/canvas/route.tssrc/app/layout.tsxsrc/components/editor/editor-toolbar.tsxsrc/components/editor/editor.tsxsrc/components/layout/app-shell.tsxsrc/components/layout/browser-view.tsxsrc/components/layout/canvas-view.tsxsrc/components/layout/header.tsxsrc/components/settings/storage-backend-section.tsxsrc/components/sidebar/tree-node.tsxsrc/components/sidebar/tree-view.tsxsrc/i18n/locales/ar.jsonsrc/i18n/locales/bn.jsonsrc/i18n/locales/cs.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/el.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fa.jsonsrc/i18n/locales/fil.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/gu.jsonsrc/i18n/locales/ha.jsonsrc/i18n/locales/he.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/hu.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/kn.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/ml.jsonsrc/i18n/locales/mr.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pa.jsonsrc/i18n/locales/pl.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ro.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/sv.jsonsrc/i18n/locales/sw.jsonsrc/i18n/locales/ta.jsonsrc/i18n/locales/te.jsonsrc/i18n/locales/th.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/uk.jsonsrc/i18n/locales/ur.jsonsrc/i18n/locales/vi.jsonsrc/i18n/locales/yo.jsonsrc/i18n/locales/zh-CN.jsonsrc/i18n/locales/zh-TW.jsonsrc/lib/agents/adapters/plugin-loader.tssrc/stores/app-store.tstest/cabinet-v2.test.tstest/conversation-output-cleaning.test.ts
✅ Files skipped from review due to trivial changes (26)
- src/app/layout.tsx
- src/i18n/locales/hi.json
- src/i18n/locales/yo.json
- src/i18n/locales/te.json
- src/i18n/locales/he.json
- src/i18n/locales/gu.json
- src/i18n/locales/ur.json
- src/components/settings/storage-backend-section.tsx
- src/i18n/locales/fa.json
- src/lib/agents/adapters/plugin-loader.ts
- src/i18n/locales/el.json
- src/i18n/locales/ta.json
- src/i18n/locales/ar.json
- src/i18n/locales/kn.json
- src/i18n/locales/zh-TW.json
- src/i18n/locales/mr.json
- src/components/editor/editor-toolbar.tsx
- src/i18n/locales/de.json
- src/i18n/locales/uk.json
- src/components/sidebar/tree-view.tsx
- src/i18n/locales/sw.json
- src/i18n/locales/hu.json
- src/i18n/locales/pt.json
- src/i18n/locales/tr.json
- src/i18n/locales/sv.json
- src/i18n/locales/ro.json
Comment on lines
+17
to
+33
| function allowsAppOrigin(tokens: string[], appOrigin: string): boolean { | ||
| if (tokens.includes("*")) return true; | ||
| const app = new URL(appOrigin); | ||
| for (const token of tokens) { | ||
| const normalized = token.replace(/^"|"$/g, ""); | ||
| if (normalized === "'none'") return false; | ||
| if (normalized === "'self'") continue; | ||
| if (normalized.startsWith("http://") || normalized.startsWith("https://")) { | ||
| try { | ||
| const allowed = new URL(normalized); | ||
| if (allowed.origin === app.origin) return true; | ||
| } catch { | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Handle frame-ancestors 'self' against the target origin.
'self' is currently skipped, which incorrectly marks same-origin pages as blocked when CSP should allow embedding.
✅ Suggested fix
-function allowsAppOrigin(tokens: string[], appOrigin: string): boolean {
+function allowsAppOrigin(tokens: string[], appOrigin: string, targetOrigin: string): boolean {
if (tokens.includes("*")) return true;
const app = new URL(appOrigin);
for (const token of tokens) {
const normalized = token.replace(/^"|"$/g, "");
if (normalized === "'none'") return false;
- if (normalized === "'self'") continue;
+ if (normalized === "'self'") {
+ if (app.origin === targetOrigin) return true;
+ continue;
+ }
@@
- blocked = !allowsAppOrigin(frameAncestors, appOrigin);
+ blocked = !allowsAppOrigin(frameAncestors, appOrigin, finalOrigin);
}
}Also applies to: 95-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/browser/frame-check/route.ts` around lines 17 - 33, When parsing
CSP tokens in allowsAppOrigin(tokens, appOrigin), "'self'" must be treated as an
allow when the appOrigin is same-origin with the protected resource; replace the
current skip (if (normalized === "'self'") continue) with a comparison that
returns true when app.origin === serverOrigin (where serverOrigin is the origin
of the page/service enforcing the CSP). Add a serverOrigin parameter to
allowsAppOrigin (or obtain the enforcement origin from the surrounding context),
update all callers to pass that origin, and ensure the function returns true
when "'self'" matches the enforcement origin.
Comment on lines
+54
to
+71
| const rawUrl = request.nextUrl.searchParams.get("url") || ""; | ||
| let target: URL; | ||
| try { | ||
| target = new URL(rawUrl); | ||
| } catch { | ||
| return NextResponse.json({ ok: false, error: "invalid-url" }, { status: 400 }); | ||
| } | ||
| if (target.protocol !== "http:" && target.protocol !== "https:") { | ||
| return NextResponse.json({ ok: false, error: "invalid-protocol" }, { status: 400 }); | ||
| } | ||
|
|
||
| let response: Response; | ||
| try { | ||
| response = await fetch(target.toString(), { | ||
| method: "HEAD", | ||
| redirect: "follow", | ||
| cache: "no-store", | ||
| }); |
There was a problem hiding this comment.
Block SSRF targets before server-side fetch.
This endpoint fetches arbitrary user-supplied URLs and currently allows localhost/private-network targets, which exposes an SSRF path.
🔒 Suggested hardening direction
+function isPrivateOrLocalHost(hostname: string): boolean {
+ const h = hostname.toLowerCase();
+ if (h === "localhost" || h.endsWith(".localhost") || h === "::1" || h === "[::1]") return true;
+ if (/^127\.\d+\.\d+\.\d+$/.test(h)) return true;
+ if (/^10\.\d+\.\d+\.\d+$/.test(h)) return true;
+ if (/^192\.168\.\d+\.\d+$/.test(h)) return true;
+ if (/^172\.(1[6-9]|2\d|3[0-1])\.\d+\.\d+$/.test(h)) return true;
+ return false;
+}
+
export async function GET(request: NextRequest) {
const rawUrl = request.nextUrl.searchParams.get("url") || "";
let target: URL;
@@
if (target.protocol !== "http:" && target.protocol !== "https:") {
return NextResponse.json({ ok: false, error: "invalid-protocol" }, { status: 400 });
}
+ if (isPrivateOrLocalHost(target.hostname)) {
+ return NextResponse.json({ ok: false, error: "forbidden-host" }, { status: 400 });
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/browser/frame-check/route.ts` around lines 54 - 71, The endpoint
currently constructs target from rawUrl and then fetches it (see rawUrl, target
and the fetch call); before performing the server-side fetch, resolve the target
hostname to IP(s) and reject any address that falls into local/private ranges
(127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, ::1, fc00::/7,
fe80::/10, link-local, and any localhost or .local hostnames) or is an IP
literal in those ranges; if DNS resolution fails or any resolved address is
private/loopback, return the same 400 JSON error (e.g. "invalid-host" or
"invalid-url") instead of fetching; implement this check using a DNS lookup
(dns.promises.lookup or equivalent) and an IP range check helper invoked before
calling fetch on target.toString().
Comment on lines
+67
to
+71
| response = await fetch(target.toString(), { | ||
| method: "HEAD", | ||
| redirect: "follow", | ||
| cache: "no-store", | ||
| }); |
There was a problem hiding this comment.
Add a timeout to the external HEAD request.
The outbound fetch has no timeout, so slow/unresponsive hosts can hold request workers longer than necessary.
⏱️ Suggested fix
response = await fetch(target.toString(), {
method: "HEAD",
redirect: "follow",
cache: "no-store",
+ signal: AbortSignal.timeout(5000),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response = await fetch(target.toString(), { | |
| method: "HEAD", | |
| redirect: "follow", | |
| cache: "no-store", | |
| }); | |
| response = await fetch(target.toString(), { | |
| method: "HEAD", | |
| redirect: "follow", | |
| cache: "no-store", | |
| signal: AbortSignal.timeout(5000), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/browser/frame-check/route.ts` around lines 67 - 71, The outbound
HEAD fetch (the assignment to response from fetch(target.toString(), ...)) needs
a timeout: create an AbortController, pass controller.signal into the fetch
options, set a timer (e.g. via setTimeout) to call controller.abort() after your
chosen timeout, and clear the timer after fetch completes; update the fetch call
in route.ts (where response is awaited) to include the signal and handle the
abort error path (treat as a timed-out external request and return an
appropriate response) so workers aren’t blocked by slow hosts.
Comment on lines
+26
to
+29
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : "Unknown error"; | ||
| return NextResponse.json({ error: message }, { status: 500 }); | ||
| } |
There was a problem hiding this comment.
Harden error handling and payload validation in canvas endpoints.
Malformed JSON currently falls into the generic catch and returns 500, and both handlers return raw exception messages. Also, array payloads pass validation. Please return 400 for invalid JSON/body shape and use a generic 500 response body.
Proposed fix
export async function GET(request: NextRequest) {
try {
@@
- } catch (error) {
- const message = error instanceof Error ? error.message : "Unknown error";
- return NextResponse.json({ error: message }, { status: 500 });
+ } catch (error) {
+ console.error("Failed to read canvas snapshot", error);
+ return NextResponse.json({ error: "Internal server error" }, { status: 500 });
}
}
export async function PUT(request: NextRequest) {
+ let snapshot: unknown;
try {
const cabinetPath = normalizeCabinetPath(request.nextUrl.searchParams.get("cabinetPath"), true) ?? ROOT_CABINET_PATH;
- const snapshot = await request.json();
+ try {
+ snapshot = await request.json();
+ } catch {
+ return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 });
+ }
- if (!snapshot || typeof snapshot !== "object") {
+ if (!snapshot || typeof snapshot !== "object" || Array.isArray(snapshot)) {
return NextResponse.json({ error: "Invalid canvas snapshot" }, { status: 400 });
}
@@
- } catch (error) {
- const message = error instanceof Error ? error.message : "Unknown error";
- return NextResponse.json({ error: message }, { status: 500 });
+ } catch (error) {
+ console.error("Failed to update canvas snapshot", error);
+ return NextResponse.json({ error: "Internal server error" }, { status: 500 });
}
}Also applies to: 33-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/canvas/route.ts` around lines 26 - 29, The error handling in the
canvas endpoint currently returns a 500 status with raw error messages and does
not properly handle malformed JSON or invalid array payloads. Update the
try-catch blocks in the handler functions to validate the request body shape,
returning a 400 status if the JSON is malformed or the payload shape is
incorrect (e.g., if an array is sent instead of an expected object). For
unexpected errors, return a generic 500 response body without exposing raw
exception details to harden error handling and validation.
Comment on lines
+24
to
+28
| function normalizeEnteredUrl(value: string): string | null { | ||
| const trimmed = value.trim(); | ||
| if (!trimmed) return null; | ||
| if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed) || trimmed.startsWith("//")) return trimmed; | ||
| return `https://${trimmed}`; |
There was a problem hiding this comment.
Reject non-HTTP(S) schemes here.
This helper currently passes through any absolute scheme, so javascript:, data:, file: and similar values can flow straight into the iframe, external link, and Electron browser view. That turns the address bar into an arbitrary-scheme launcher instead of a web-only navigator.
Suggested fix
function normalizeEnteredUrl(value: string): string | null {
const trimmed = value.trim();
if (!trimmed) return null;
- if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed) || trimmed.startsWith("//")) return trimmed;
- return `https://${trimmed}`;
+ try {
+ const candidate = trimmed.startsWith("//")
+ ? `https:${trimmed}`
+ : /^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed)
+ ? trimmed
+ : `https://${trimmed}`;
+ const parsed = new URL(candidate);
+ if (parsed.protocol !== "http:" && parsed.protocol !== "https:") {
+ return null;
+ }
+ return parsed.toString();
+ } catch {
+ return null;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/browser-view.tsx` around lines 24 - 28,
normalizeEnteredUrl currently allows any absolute scheme (e.g., javascript:,
data:, file:) to pass through; change it so that when the trimmed input contains
a scheme (matches /^[a-zA-Z][a-zA-Z\d+.-]*:/) you only accept it if the scheme
is "http:" or "https:"; if the input starts with "//" allow it
(protocol-relative), otherwise return null; update the normalizeEnteredUrl
function to perform this scheme whitelist check so only
http/https/protocol-relative URLs are returned.
Comment on lines
+338
to
+349
| {iframeFailure ? ( | ||
| <div className="absolute inset-0 flex items-center justify-center bg-background/85 p-6 text-center"> | ||
| <div className="max-w-md rounded border border-border bg-background px-4 py-3 text-sm text-muted-foreground"> | ||
| <div>This page can’t be rendered in an iframe.</div> | ||
| <div className="mt-1">Use “Open externally”.</div> | ||
| </div> | ||
| </div> | ||
| ) : null} | ||
| {fallbackReason ? ( | ||
| <div className="pointer-events-none absolute bottom-3 right-3 rounded border border-border bg-background/90 px-2 py-1 text-[10px] text-muted-foreground"> | ||
| fallback: {fallbackReason} | ||
| </div> |
There was a problem hiding this comment.
Localize the iframe-failure UI before shipping it.
These strings are still hardcoded English even though this component already uses useLocale and this PR is adding locale coverage. Non-English users will hit an untranslated error path here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/browser-view.tsx` around lines 338 - 349, The hardcoded
English messages shown when iframeFailure or fallbackReason are true need to be
replaced with localized strings using the component's existing localization hook
(useLocale) or translation helper; update the JSX that renders the
iframe-failure block (the conditional that checks iframeFailure) to call the
locale/translate function for both the title ("This page can’t be rendered in an
iframe.") and the suggestion ("Use “Open externally”.") and also localize the
"fallback: " label shown with fallbackReason, using consistent translation keys
(e.g. iframe.failure.title, iframe.failure.action, iframe.fallback.label) so
non-English users receive translated text while preserving the existing
conditionals iframeFailure and fallbackReason.
| <p className="text-sm text-muted-foreground">{t("editor:canvas.openPage")}</p> | ||
| </div> | ||
| <div className="rounded-full bg-muted px-3 py-1 text-xs font-medium text-muted-foreground"> | ||
| {cabinetPath === ROOT_CABINET_PATH ? "root" : cabinetPath} |
There was a problem hiding this comment.
Localize hardcoded card labels.
Line 99, Line 122, and Line 125 use hardcoded English strings (root, Cabinet, Open cabinet) in an otherwise localized component.
Proposed fix
- {cabinetPath === ROOT_CABINET_PATH ? "root" : cabinetPath}
+ {cabinetPath === ROOT_CABINET_PATH ? t("editor:canvas.root") : cabinetPath}
...
- <div className="mb-2 text-xs font-medium uppercase tracking-wide text-muted-foreground">Cabinet</div>
+ <div className="mb-2 text-xs font-medium uppercase tracking-wide text-muted-foreground">
+ {t("editor:canvas.cabinetLabel")}
+ </div>
...
- <div className="mt-auto text-sm text-muted-foreground">Open cabinet</div>
+ <div className="mt-auto text-sm text-muted-foreground">
+ {t("editor:canvas.openCabinet")}
+ </div>Also applies to: 122-122, 125-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/canvas-view.tsx` at line 99, Replace the hardcoded
English labels with i18n keys: where the JSX currently renders {cabinetPath ===
ROOT_CABINET_PATH ? "root" : cabinetPath} and the static strings "Cabinet" and
"Open cabinet", call the app's translation helper (e.g., t or
useTranslation().t) to render t('root') for the root label, t('Cabinet') for the
cabinet title, and t('Open cabinet') for the action/aria-label; update the
component (CanvasView / canvas-view.tsx) to import/use the translation
hook/function, swap the literal strings for the translation calls, and ensure
corresponding translation keys are added to the locale files.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/layout/canvas-view.tsx (2)
768-769: 💤 Low valueSilent error handling in persistence.
Similar to the load effect, the empty
catchhides persistence failures that could help debug sync issues.Proposed fix
- } catch { - } + } catch (error) { + console.error("Failed to persist canvas snapshot:", error); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/canvas-view.tsx` around lines 768 - 769, The empty catch in the persistence block of src/components/layout/canvas-view.tsx silently swallows errors; update the catch to accept an error (e.g., catch (err)) and log it with context (for example: console.error or the app logger with a message like "Canvas persistence failed" plus the error and any relevant IDs such as canvasId or layoutKey) and optionally surface a user-facing notification or rethrow if appropriate so persistence failures are visible for debugging; ensure you reference the same persistence function/handler in this file when adding the log.
567-579: 💤 Low valueSilent error swallowing hides debugging information.
The empty
catchblock discards error details. Consider logging at minimum for debugging visibility.Proposed fix
- } catch { + } catch (error) { + console.error("Failed to load canvas snapshot:", error); if (!cancelled) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/canvas-view.tsx` around lines 567 - 579, The catch block in CanvasView is swallowing errors silently; change it to capture the exception (e.g., catch (error)) and log the error before running the existing cleanup (when !cancelled). Specifically, update the catch around the code that calls setCardSizeByPath, setCardPositionByPath, setCardPositionsByBoardPath, setPositionsCenteredByBoardPath, setPositionCenterVersionByBoardPath, setManualResizedByPath, setAutoSizedByPath and setBoardZoom(WHITEBOARD_DEFAULT_ZOOM) to log a useful message (for example via console.error or your app logger) including the error object so failures are visible for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/layout/canvas-view.tsx`:
- Around line 768-769: The empty catch in the persistence block of
src/components/layout/canvas-view.tsx silently swallows errors; update the catch
to accept an error (e.g., catch (err)) and log it with context (for example:
console.error or the app logger with a message like "Canvas persistence failed"
plus the error and any relevant IDs such as canvasId or layoutKey) and
optionally surface a user-facing notification or rethrow if appropriate so
persistence failures are visible for debugging; ensure you reference the same
persistence function/handler in this file when adding the log.
- Around line 567-579: The catch block in CanvasView is swallowing errors
silently; change it to capture the exception (e.g., catch (error)) and log the
error before running the existing cleanup (when !cancelled). Specifically,
update the catch around the code that calls setCardSizeByPath,
setCardPositionByPath, setCardPositionsByBoardPath,
setPositionsCenteredByBoardPath, setPositionCenterVersionByBoardPath,
setManualResizedByPath, setAutoSizedByPath and
setBoardZoom(WHITEBOARD_DEFAULT_ZOOM) to log a useful message (for example via
console.error or your app logger) including the error object so failures are
visible for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17361af3-f756-490c-ba49-82df8915f44e
📒 Files selected for processing (1)
src/components/layout/canvas-view.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/main.cjs (1)
849-865:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
setWindowButtonVisibilityis unreachable code.
buildBrowserWindowreturns theBrowserWindowat line 863, so line 864 is never executed. The window button visibility setting is dead code.Suggested fix
function buildBrowserWindow() { - return new BrowserWindow({ + const win = new BrowserWindow({ width: 1480, height: 940, minWidth: 1180, minHeight: 760, backgroundColor: "`#111111`", titleBarStyle: "hiddenInset", trafficLightPosition: { x: 16, y: 18 }, webPreferences: { preload: path.join(__dirname, "preload.cjs"), contextIsolation: true, sandbox: false, }, }); - mainWindow.setWindowButtonVisibility(true); + win.setWindowButtonVisibility(true); + return win; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main.cjs` around lines 849 - 865, The call to mainWindow.setWindowButtonVisibility(true) in buildBrowserWindow is unreachable because the function returns immediately; fix by creating the BrowserWindow instance in a local variable (e.g., const win = new BrowserWindow(...)), call win.setWindowButtonVisibility(true) on that instance before returning, and then return the instance; update any references to use the returned window instead of relying on a global mainWindow if needed so setWindowButtonVisibility executes.
♻️ Duplicate comments (5)
src/components/layout/browser-view.tsx (2)
39-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-HTTP(S) schemes here.
This helper still passes through any absolute scheme (
javascript:,data:,file:) which can flow into the iframe, external link, and Electron browser view.Suggested fix
function normalizeEnteredUrl(value: string): string | null { const trimmed = value.trim(); if (!trimmed) return null; - if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed) || trimmed.startsWith("//")) return trimmed; - return `https://${trimmed}`; + try { + const candidate = trimmed.startsWith("//") + ? `https:${trimmed}` + : /^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed) + ? trimmed + : `https://${trimmed}`; + const parsed = new URL(candidate); + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + return null; + } + return parsed.toString(); + } catch { + return null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/browser-view.tsx` around lines 39 - 44, The normalizeEnteredUrl function currently permits any absolute scheme (e.g., javascript:, data:, file:) which can reach the iframe/browser view; modify normalizeEnteredUrl to explicitly reject non-HTTP(S) schemes by parsing the input (or using a regex) to detect an explicit scheme and return null unless the scheme is "http" or "https"; keep allowing protocol-relative URLs (starting with "//") and transform bare hosts to "https://"; update references to normalizeEnteredUrl to expect null for disallowed schemes so callers won't load unsafe URLs.
675-686:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the iframe-failure UI.
These strings are still hardcoded English. The component uses
useLocalealready, so these should use the translation function.- <div>This page can't be rendered in an iframe.</div> - <div className="mt-1">Use "Open externally".</div> + <div>{t("editor:browser.iframeBlocked")}</div> + <div className="mt-1">{t("editor:browser.useOpenExternally")}</div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/browser-view.tsx` around lines 675 - 686, Replace the hardcoded English messages shown when iframeFailure or fallbackReason are set by using the component's useLocale translation function: inside BrowserView, grab the translator from useLocale (e.g., const { t } = useLocale() or the appropriate API used in this file), replace "This page can’t be rendered in an iframe." with t('iframe.cantRender') (or your i18n key), replace "Use “Open externally”." with t('iframe.openExternally'), and render the fallback label using t('iframe.fallback') + fallbackReason or t('iframe.fallbackWithReason', { reason: fallbackReason }) so the UI strings (the iframeFailure block and the fallback: {fallbackReason} label) are localized rather than hardcoded.src/components/layout/canvas-view.tsx (2)
1199-1208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid loading cabinet paths via
loadPage.The
onClickhandler callsloadPage(node.path)for all nodes including cabinet nodes (line 1205). This can trigger invalid page-load behavior when clicking cabinet cards sinceloadPageexpects page paths, not cabinet paths.onClick={() => { if (suppressNextCardClickRef.current) { suppressNextCardClickRef.current = false; return; } selectPage(node.path); - void loadPage(node.path); if (composed && isFolderObject(node)) { setSection({ type: "page", cabinetPath: node.path }); + } else { + void loadPage(node.path); } }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/canvas-view.tsx` around lines 1199 - 1208, The click handler is calling loadPage(node.path) for cabinet/folder nodes which expects page paths; update the onClick logic around suppressNextCardClickRef, selectPage, loadPage, composed, isFolderObject and setSection so that loadPage(node.path) is only invoked for actual pages (i.e., when isFolderObject(node) is false or node indicates a page), while preserving selectPage(node.path) and the existing composed && isFolderObject(...) setSection behavior for cabinet clicks.
1112-1112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize hardcoded "root" label.
Line 1112 still uses hardcoded English
"root"in an otherwise localized component.- {boardNode ? boardNode.path : cabinetPath === ROOT_CABINET_PATH ? "root" : cabinetPath} + {boardNode ? boardNode.path : cabinetPath === ROOT_CABINET_PATH ? t("editor:canvas.root") : cabinetPath}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/canvas-view.tsx` at line 1112, Replace the hardcoded "root" string in the JSX expression that renders the cabinet label so it uses the app's localization helper (e.g., the i18n/t/translate function used elsewhere) instead of a literal; update the expression that reads {boardNode ? boardNode.path : cabinetPath === ROOT_CABINET_PATH ? "root" : cabinetPath} to call the localization function (e.g., t('root')) for the ROOT_CABINET_PATH branch, referencing boardNode, cabinetPath and ROOT_CABINET_PATH to locate the code and keep behavior identical otherwise.electron/main.cjs (1)
658-660:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBrowser-view ownership is still hard-wired to mainWindow.
All handlers authorize against
mainWindow.webContents.idand attach views tomainWindow.contentView. Room windows opened throughcabinet:open-windowcannot use the Electron browser view - they will always getunauthorizedand fall back to iframe mode.This was flagged in a previous review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main.cjs` around lines 658 - 660, The handlers hard-wire authorization and view attachment to mainWindow (using isMainRendererSender, mainWindow.webContents.id and mainWindow.contentView), which blocks room windows opened via "cabinet:open-window"; change authorization to accept the sender's owning BrowserWindow rather than only mainWindow by using BrowserWindow.fromWebContents(event.sender) (or checking event.sender.id against your windows map) inside isMainRendererSender and the relevant handlers, and when creating/attaching BrowserView use that owner window's .setBrowserView / .addBrowserView (ownerWindow.contentView) instead of mainWindow.contentView so each window can authorize and host its own browser view. Ensure you update all places that reference mainWindow.webContents.id and mainWindow.contentView to use the resolved owner window.
🧹 Nitpick comments (1)
electron/preload.cjs (1)
6-12: 💤 Low valueSilently swallowing listener errors hides bugs.
The empty
catch {}at line 10 suppresses all errors thrown by listeners. If a listener has a bug, you'll have no indication anything went wrong, making debugging difficult.Consider logging at minimum:
try { listener(payload); - } catch {} + } catch (err) { + console.error("[cabinet:browser-view-navigated] listener threw:", err); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/preload.cjs` around lines 6 - 12, The empty catch in the ipcRenderer.on handler for "cabinet:browser-view-navigated" is silently swallowing errors from listeners; update the try/catch around invoking each function in browserViewNavigateListeners so the catch accepts the error (e) and logs it (e.g., console.error("Error in browserViewNavigate listener", e) or processLogger/error reporting) including the error message/stack and enough context (payload or listener id) so failures in listener(payload) are visible; keep the loop behavior but do not leave the catch block empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/main.cjs`:
- Around line 668-675: The function normalizeBrowserTargetUrl currently allows
any URL scheme; restrict it by validating the scheme in
normalizeBrowserTargetUrl: only allow about:blank, protocol-relative URLs
(starting with "//"), and http/https schemes — treat anything else (including
javascript:, data:, file:, etc.) as unsafe and return "about:blank". Implement
this by parsing or adjusting the existing regex to explicitly match
^(?:https?:|about:)|^\/\/ or by extracting the scheme and checking it against an
allowlist before returning the trimmed value so unsafe schemes are not passed
through.
In `@src/components/layout/browser-view.tsx`:
- Around line 187-190: navigateForward is inconsistent with navigateBack:
navigateBack tries iframeRef.current?.contentWindow?.history.back() for
same-origin iframes before falling back to app history, but navigateForward
directly calls applyAppHistoryForward(); change navigateForward to first attempt
iframeRef.current?.contentWindow?.history.forward() when browserMode ===
"iframe" and the iframeRef exists and is same-origin, and only call
applyAppHistoryForward() as the fallback; reference functions/variables:
navigateForward, navigateBack, iframeRef, applyAppHistoryForward, browserMode.
---
Outside diff comments:
In `@electron/main.cjs`:
- Around line 849-865: The call to mainWindow.setWindowButtonVisibility(true) in
buildBrowserWindow is unreachable because the function returns immediately; fix
by creating the BrowserWindow instance in a local variable (e.g., const win =
new BrowserWindow(...)), call win.setWindowButtonVisibility(true) on that
instance before returning, and then return the instance; update any references
to use the returned window instead of relying on a global mainWindow if needed
so setWindowButtonVisibility executes.
---
Duplicate comments:
In `@electron/main.cjs`:
- Around line 658-660: The handlers hard-wire authorization and view attachment
to mainWindow (using isMainRendererSender, mainWindow.webContents.id and
mainWindow.contentView), which blocks room windows opened via
"cabinet:open-window"; change authorization to accept the sender's owning
BrowserWindow rather than only mainWindow by using
BrowserWindow.fromWebContents(event.sender) (or checking event.sender.id against
your windows map) inside isMainRendererSender and the relevant handlers, and
when creating/attaching BrowserView use that owner window's .setBrowserView /
.addBrowserView (ownerWindow.contentView) instead of mainWindow.contentView so
each window can authorize and host its own browser view. Ensure you update all
places that reference mainWindow.webContents.id and mainWindow.contentView to
use the resolved owner window.
In `@src/components/layout/browser-view.tsx`:
- Around line 39-44: The normalizeEnteredUrl function currently permits any
absolute scheme (e.g., javascript:, data:, file:) which can reach the
iframe/browser view; modify normalizeEnteredUrl to explicitly reject non-HTTP(S)
schemes by parsing the input (or using a regex) to detect an explicit scheme and
return null unless the scheme is "http" or "https"; keep allowing
protocol-relative URLs (starting with "//") and transform bare hosts to
"https://"; update references to normalizeEnteredUrl to expect null for
disallowed schemes so callers won't load unsafe URLs.
- Around line 675-686: Replace the hardcoded English messages shown when
iframeFailure or fallbackReason are set by using the component's useLocale
translation function: inside BrowserView, grab the translator from useLocale
(e.g., const { t } = useLocale() or the appropriate API used in this file),
replace "This page can’t be rendered in an iframe." with t('iframe.cantRender')
(or your i18n key), replace "Use “Open externally”." with
t('iframe.openExternally'), and render the fallback label using
t('iframe.fallback') + fallbackReason or t('iframe.fallbackWithReason', {
reason: fallbackReason }) so the UI strings (the iframeFailure block and the
fallback: {fallbackReason} label) are localized rather than hardcoded.
In `@src/components/layout/canvas-view.tsx`:
- Around line 1199-1208: The click handler is calling loadPage(node.path) for
cabinet/folder nodes which expects page paths; update the onClick logic around
suppressNextCardClickRef, selectPage, loadPage, composed, isFolderObject and
setSection so that loadPage(node.path) is only invoked for actual pages (i.e.,
when isFolderObject(node) is false or node indicates a page), while preserving
selectPage(node.path) and the existing composed && isFolderObject(...)
setSection behavior for cabinet clicks.
- Line 1112: Replace the hardcoded "root" string in the JSX expression that
renders the cabinet label so it uses the app's localization helper (e.g., the
i18n/t/translate function used elsewhere) instead of a literal; update the
expression that reads {boardNode ? boardNode.path : cabinetPath ===
ROOT_CABINET_PATH ? "root" : cabinetPath} to call the localization function
(e.g., t('root')) for the ROOT_CABINET_PATH branch, referencing boardNode,
cabinetPath and ROOT_CABINET_PATH to locate the code and keep behavior identical
otherwise.
---
Nitpick comments:
In `@electron/preload.cjs`:
- Around line 6-12: The empty catch in the ipcRenderer.on handler for
"cabinet:browser-view-navigated" is silently swallowing errors from listeners;
update the try/catch around invoking each function in
browserViewNavigateListeners so the catch accepts the error (e) and logs it
(e.g., console.error("Error in browserViewNavigate listener", e) or
processLogger/error reporting) including the error message/stack and enough
context (payload or listener id) so failures in listener(payload) are visible;
keep the loop behavior but do not leave the catch block empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccbd6d24-04d0-43dd-92bb-5f8f0cc6ba16
📒 Files selected for processing (5)
electron/main.cjselectron/preload.cjssrc/components/layout/browser-view.tsxsrc/components/layout/canvas-view.tsxsrc/i18n/locales/en.json
Comment on lines
+187
to
+190
| if (browserMode === "iframe") { | ||
| applyAppHistoryForward(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Inconsistent iframe navigation: forward doesn't try iframe history.
navigateBack (lines 149-156) attempts iframeRef.current?.contentWindow?.history.back() before falling back to app history, but navigateForward directly calls the app history fallback without trying history.forward() first. This inconsistency could cause unexpected behavior when navigating within same-origin iframes.
Suggested fix
if (browserMode === "iframe") {
- applyAppHistoryForward();
+ try {
+ iframeRef.current?.contentWindow?.history.forward();
+ return;
+ } catch {
+ applyAppHistoryForward();
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/browser-view.tsx` around lines 187 - 190,
navigateForward is inconsistent with navigateBack: navigateBack tries
iframeRef.current?.contentWindow?.history.back() for same-origin iframes before
falling back to app history, but navigateForward directly calls
applyAppHistoryForward(); change navigateForward to first attempt
iframeRef.current?.contentWindow?.history.forward() when browserMode ===
"iframe" and the iframeRef exists and is same-origin, and only call
applyAppHistoryForward() as the fallback; reference functions/variables:
navigateForward, navigateBack, iframeRef, applyAppHistoryForward, browserMode.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/canvas-view.tsx`:
- Around line 1288-1304: The title element is currently a nested <button> inside
a card <button>, which is invalid; replace the inner <button> with a non-button
element (e.g., <div> or <span>) and preserve the existing behavior by moving the
onClick logic (event.stopPropagation(), meta/ctrl check, selectPage(node.path),
loadPage(node.path), and setSection when composed && isFolderObject(node)) into
that element, add tabIndex={0}, role="button" and an onKeyDown handler that
triggers the same action for Enter/Space to keep keyboard accessibility; ensure
you still reference the same symbols (selectPage, loadPage, setSection,
composed, isFolderObject, node.path, title) so behavior is unchanged.
In `@src/hooks/use-composer.ts`:
- Around line 115-118: The code currently treats dismissedPinnedPath as a single
value which causes previously dismissed pins to reappear; change this to a
collection (e.g., dismissedPinnedPaths: Set<string> or string[]) and update all
uses: initialize state (replace dismissedPinnedPath with dismissedPinnedPaths),
update setters (e.g., setDismissedPinnedPaths) to add/remove entries rather than
overwrite, update the memo for pinnedActivePaths to filter using
dismissedPinnedPaths.has(path) or includes(path) instead of equality, and update
any other references (notably the logic around lines referencing
dismissedPinnedPath and the block around the 233-245 region) to read from and
modify the set accordingly. Ensure serializable state if needed (use array when
persisting) and maintain existing function names like pinnedActivePaths and
pinnedBasePaths when locating changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bd039e1-07a3-492f-a139-f499930e716b
📒 Files selected for processing (6)
src/components/layout/canvas-view.tsxsrc/components/layout/header.tsxsrc/components/layout/viewer-toolbar.tsxsrc/components/tasks/task-compose-body.tsxsrc/hooks/use-composer.tssrc/stores/app-store.ts
Comment on lines
+115
to
+118
| const pinnedActivePaths = useMemo( | ||
| () => pinnedBasePaths.filter((path) => dismissedPinnedPath !== path), | ||
| [pinnedBasePaths, dismissedPinnedPath] | ||
| ); |
There was a problem hiding this comment.
Track dismissed pinned pages as a set, not a single value.
With multiple pinned pages, using a single dismissedPinnedPath means dismissing one pinned chip and then another makes the first one reappear. That breaks multi-page dismissal behavior.
Suggested fix
- const [dismissedPinnedPath, setDismissedPinnedPath] = useState<string | null>(null);
+ const [dismissedPinnedPaths, setDismissedPinnedPaths] = useState<string[]>([]);
- const pinnedActivePaths = useMemo(
- () => pinnedBasePaths.filter((path) => dismissedPinnedPath !== path),
- [pinnedBasePaths, dismissedPinnedPath]
- );
+ const pinnedActivePaths = useMemo(
+ () => pinnedBasePaths.filter((path) => !dismissedPinnedPaths.includes(path)),
+ [pinnedBasePaths, dismissedPinnedPaths]
+ );
- useEffect(() => {
- if (!dismissedPinnedPath) return;
- if (pinnedBasePaths.includes(dismissedPinnedPath)) return;
- setDismissedPinnedPath(null);
- }, [dismissedPinnedPath, pinnedBasePaths]);
+ useEffect(() => {
+ setDismissedPinnedPaths((prev) =>
+ prev.filter((path) => pinnedBasePaths.includes(path))
+ );
+ }, [pinnedBasePaths]);
- if (pinnedBasePaths.includes(id)) setDismissedPinnedPath(id);
+ if (pinnedBasePaths.includes(id)) {
+ setDismissedPinnedPaths((prev) => (prev.includes(id) ? prev : [...prev, id]));
+ }Also applies to: 233-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/use-composer.ts` around lines 115 - 118, The code currently treats
dismissedPinnedPath as a single value which causes previously dismissed pins to
reappear; change this to a collection (e.g., dismissedPinnedPaths: Set<string>
or string[]) and update all uses: initialize state (replace dismissedPinnedPath
with dismissedPinnedPaths), update setters (e.g., setDismissedPinnedPaths) to
add/remove entries rather than overwrite, update the memo for pinnedActivePaths
to filter using dismissedPinnedPaths.has(path) or includes(path) instead of
equality, and update any other references (notably the logic around lines
referencing dismissedPinnedPath and the block around the 233-245 region) to read
from and modify the set accordingly. Ensure serializable state if needed (use
array when persisting) and maintain existing function names like
pinnedActivePaths and pinnedBasePaths when locating changes.
XcluEzy7
pushed a commit
to XcluEzy7/cabinet
that referenced
this pull request
May 27, 2026
…unity menu Bundles four audit findings touching the bottom status bar. cabinetai#15 (uncommitted count vague) - "X uncommitted" is now a button. Click opens a popover anchored to the status bar with the file list and a colored single-letter status badge (M / ? / A / D / R) per row. Capped at 50 entries; truncated state shows a "+N more — run git status" footer. - getStatus in git-service grew the `files` and `truncated` fields on its return type. Existing call sites only read `uncommitted`, so the extension is non-breaking. cabinetai#18 (status bar overcrowded) - Right cluster collapsed from four pills (Help / Discord / Contribute / Stars) to a single Help trigger that opens a community popover. - Star count rides as a small amber chip on the trigger so it stays visible at a glance — matches "live state stays in the bar" without losing the badge entirely. cabinetai#94 (not-installed rows lack one-click install) - Each "Not installed" provider row in the server-status popover renders an Install pill. Provider IDs map to canonical install docs: claude-code, codex-cli, cursor-cli, gemini-cli, opencode, pi, grok-cli, copilot-cli. Unknown IDs fall back to a Google search. cabinetai#96 (stars source of truth) — already fixed via cabinetai#16 - The github-stats-store + skeleton-until-loaded behavior shipped under cabinetai#16 already collapsed the multi-source race. Verified: count reads "1,712" consistently across the new popover and the trigger chip with no flicker. No code change for this ticket.
… and settings UI support
Restores the typed-file extension-handling intent from the 'browse & canvas part VI' commit that was dropped during the upstream rebase. renamePage now uses a new extension supplied in newName (e.g. photo.jpg -> "holiday.png" yields holiday.png) while still preserving the original extension when none is provided. Co-Authored-By: Oz <oz-agent@warp.dev>
- resolveProviderModel now returns undefined when a provider exposes no models at all, instead of fabricating a synthetic selection for dynamic/unhydrated providers (the empty-list guard now runs before the preserve-on-hydrate path). - Sync the client-safe LEGACY_ADAPTER_TYPES mirror with the server LEGACY_ADAPTER_BY_PROVIDER_ID map by adding ollama_legacy, and update the registry test enumeration to include the registered ollama_legacy adapter. Fixes the pre-existing failures in legacy-ids, registry, and runtime-options-dynamic-models test suites. Full suite: 327 passing. Co-Authored-By: Oz <oz-agent@warp.dev>
The upstream rebase resolved repeated settings-page.tsx conflicts under
build-safety pressure and ended up stripping the Integrations tab: its
nav entry, content block, and the Plug/ApiKeysSection/CliMcpSection/
IntegrationsHubSection/BuiltInToolsSection imports were all removed even
though Tab/VALID_TABS still listed "integrations" (a dangling id) and the
component files and i18n keys remained in the tree.
Restore the tab to match the pre-rebase implementation: re-add the Plug
icon and the four section imports, the Workspace-group nav entry, and the
{tab === "integrations"} content block (API keys + blurred coming-soon
hub/tools preview + CLI MCP).
Co-Authored-By: Oz <oz-agent@warp.dev>
bluelephant825
changed the title
Extend continueConversationRun to classify adapter.execute() rejections (e.g. "spawn mockcli ENOENT") via adapter.classifyError before marking the turn failed. A missing CLI binary now sets errorKind=cli_not_found + errorHint on the conversation meta, so the UI can surface an Install CTA instead of an opaque "Adapter crashed" message. Mirrors the daemon's spawn-error handling (cabinet-daemon.ts); this path runs for CABINET_TASK_
…eoPlayer Introduce a limited, registry-based MDX component system that allows agents and users to insert structured JSX blocks (<Callout>, <VideoPlayer />) into markdown pages while preventing arbitrary HTML injection. Components round- trip through the editor as structured nodes and serialize back to balanced JSX on disk. - Add MdxComponent Tiptap extension with ReactNodeViewRenderer: renders registered components as editable blocks
…p flushSync warnings - detectEmbed integration: VideoPlayer now detects YouTube/Vimeo/etc. URLs and renders them as iframes with the provider's embed URL. Direct video files still render as <video> tags. Empty URL shows a placeholder prompt. - Add text content to MdxComponent renderHTML so turndown doesn't drop the node as "blank" before our serialization rule runs (the data round-trips via attributes; this text is ignored on export/import). - Suppress React's dev-only flushSync lifecycle warning (
Introduce LiveCodeBlock extension that renders user-authored JSX (charts, components) directly in the editor with a code/preview toggle. The evaluator transpiles JSX → React.createElement via Sucrase and executes it in a sandboxed scope (React + Recharts + shadcn chart components). - LiveCodeBlock Tiptap extension: atom node with ReactNodeViewRenderer that shows a code editor (auto-sizing textarea) or live preview pane. Serializes as
… Cabinet's FrontMatter schema
…ning ignore configuration
…aTeX, and Typst viewers
This was referenced Jun 22, 2026
JupyterLite build assets and editing mode have been moved to the 'jupyterlite' branch. This removes: - public/jupyterlite/ (build assets) - .jupyterlite.doit.db - .github/secret_scanning.yml (only needed for JupyterLite) - JupyterLite editing mode (iframe, Edit button) from notebook-viewer.tsx The notebook viewer retains its read-only rendering of .ipynb files.
…tput rendering Implement full editing capabilities for .ipynb files with split-screen preview and save functionality. Refactor notebook output rendering to use dedicated MDX components (CodeOutput, DataFrame, PlotlyChart, ImageOutput, ErrorOutput) for consistent display across editor and viewer contexts. Key changes: - Add edit/preview toggle buttons and split-screen mode to notebook viewer - Implement editable code cells (auto
…book component themes with transparent backgrounds
hilash
pushed a commit
that referenced
this pull request
Jun 25, 2026
* feat(editor): add LaTeX .tex embed rendering and file viewer Ported from #96 (commit 5bf2b1d), excluding the unrelated changes bundled in that branch (Typst, table-menu UX, Tailwind migrations). - New `.tex`/`.latex` filetype: classified in tree-builder, indigo Sigma icon in the sidebar, routed to a dedicated viewer in app-shell - LatexViewer: full-file .tex viewer with rendered/source modes, edit + save-back-to-disk, refresh, and download (KaTeX-based renderer in latex-render.ts — no extra runtime dependency for the viewer) - LatexEmbedExtension: inline `![[file.tex]]` embeds in the editor, rendered via latex.js in a sandboxed, lazy-loaded iframe with click-to-edit; added a "LaTeX File" slash command to insert one - Markdown round-trip: ![[file.tex]] <-> <div data-latex-embed> across to-html (convertLatexEmbeds) and to-markdown (turndown rule) - /api/assets: serve .tex as text/x-tex and skip the long cache for editable text sources so edits/replacements aren't served stale - latex.js dependency + postinstall step copying its dist assets into public/latex-js/ (gitignored) for the embed iframe Read/edit/save works in both browser and desktop via the existing /api/assets GET+PUT route, so no Electron IPC handlers were needed. Note: run `npm install` after checkout — the inline embed needs the latex.js assets copied by postinstall; the full-file viewer works without. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex): guard against str[-1] in readBracedArg boundary check Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex): block dangerous URL schemes in \href/\url rendering Sanitize URLs from .tex \href{}/\url{} to reject javascript:, data: and other non-safe schemes, and escape them for the href attribute context, so a malicious .tex file can't inject script through a rendered link. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(markdown): escape .tex path before inserting into data-path attribute Prevents attribute injection when a ![[file.tex]] embed name contains quotes or angle brackets. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(postinstall): fail install when latex.js asset copy fails Use console.error and set process.exitCode = 1 so a failed copy surfaces loudly instead of producing a build whose LaTeX embeds break at runtime. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(assets): apply no-cache for editable text on mounted Drive assets Hoist NO_CACHE_EXTS to module scope and use it in the Google Drive branch too, so in-app edits/replacements of .tex/.csv/.md/etc. mounted from Drive aren't served from a stale browser cache. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex-viewer): separate load and save error state A save failure no longer replaces the editor with a blocking error screen. Load failures still block (nothing to show); save failures surface as a non-blocking banner with the editor left open so edits aren't lost. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex-viewer): guard against concurrent save writes handleSave can fire from both the textarea onBlur and the toolbar click at nearly the same time. Add an in-flight ref guard and disable the save button while a write is in progress so we never issue overlapping writes. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(app-shell): key LatexViewer by path so it remounts per file Without a stable key React reuses the same LatexViewer instance across files, carrying over stale content/error/mode state. Key it by texPath to force a fresh mount when the selected .tex changes. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex-extension): keep dirty flag set when save fails Don't clear the unsaved-changes indicator in the catch block — a failed write means the edits are still unsaved, so the dot must stay visible. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex-extension): render iframe parse errors via textContent Use createElement + textContent + replaceChildren instead of innerHTML so a LaTeX error message containing markup can't inject HTML/script into the render frame. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex-extension): tighten render iframe sandbox to allow-scripts only Drop allow-same-origin so the render frame runs with an opaque origin and can neither reach back into the host document nor clear its own sandbox. To keep working without same-origin: - load latex.js as a classic UMD script (window.latexjs) instead of an ES module import, which would be CORS-gated under an opaque origin; - report rendered height via postMessage and resize from a parent message listener (matched by event source) instead of reading contentWindow. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(latex): strip control chars before validating \href/\url scheme Browsers ignore embedded tabs/newlines when resolving an href, so an obfuscated scheme like `java\nscript:` could slip past the scheme regex. Normalize control characters out before the check. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(markdown): recognize .latex extension for LaTeX embeds Match both .tex and .latex (case-insensitive) in the ![[...]] embed regex so .latex files render as LaTeX embeds instead of falling through to wiki-link handling. Addresses CodeRabbit review on PR #163. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
hilash
pushed a commit
that referenced
this pull request
Jun 25, 2026
* feat(browser): in-app browse mode ported faithfully from PR #96 Native WebContentsView-backed in-app browser (with iframe fallback on the web build). External links in KB articles, the viewer-toolbar Globe button (websites/apps only), and tree-node clicks while browsing all open here; X-Frame-Options sites load in the desktop app. - Verbatim from PR #96: browser-view.tsx, open-url.ts, api/browser/{bookmarks, frame-check} (bookmarks path adapted DATA_PARENT_DIR -> CABINET_INTERNAL_DIR). - Electron: new browser-views.cjs module (WebContentsView IPC handlers, kb-auth cookie sync for /api/assets, native bookmarks menu) wired into main.cjs/preload.cjs; forge.config.cjs ships the new .cjs files. - Store/app-shell/editor/viewer-toolbar/tree-node: browse-mode entry points (canvas/latex/extensions parts of PR #96 deliberately excluded). Fixes found during testing: - editor: guard against a freshly-remounted (content:"") editor autosaving empty content over a real page when returning from browse mode (data loss). - browser-view: back button returns to the KB article instead of about:blank. - browser-view: destroy WebContentsView orphaned by a create/unmount race (memory leak, esp. dev StrictMode). - viewer-toolbar: Globe button only on website/app nodes (raw .md isn't browsable); exit button stays unconditional in browse mode. - browser-preload: drop dead chromewebstore install hook (no IPC exposed to web pages); UA spoof only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browser): add SSRF guard and apply to frame-check probe Add src/lib/net/ssrf-guard.ts: validates that a user-supplied URL is http(s) and does not resolve to a loopback/private/link-local/CGNAT/reserved address, follows redirects manually (re-validating each hop), and bounds requests with a timeout. Use it in the frame-check GET so the server can't be used to probe internal hosts (cloud metadata, localhost services) and won't hang on slow targets. Addresses CodeRabbit review on PR #165 (comment #8). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(bookmarks): validate URL schemes, guard title fetch (SSRF), honor user name - normalizeUrl now only accepts http/https (and about:blank); file:, javascript:, data: and other schemes collapse to about:blank so a dangerous URL can't be persisted (#4). - resolveBookmarkTitle goes through the SSRF guard (no private hosts, timeout, capped read) instead of fetching arbitrary URLs directly (#5). - addBookmark uses a user-provided name verbatim and only fetches the page title when no name was given, so user edits aren't overwritten (#7). Addresses CodeRabbit review on PR #165 (comments #4, #5, #7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(bookmarks): atomic writes, ENOENT-only defaults, serialized mutations - readBookmarks only seeds defaults on ENOENT and rethrows other read errors, so a transient failure can't wipe existing bookmarks (#3). - writeBookmarks writes to a temp file and renames over the target for an atomic, crash-safe update (#3). - POST/PATCH/DELETE run their read-modify-write under an in-process mutex so concurrent requests can't lose updates or mint duplicate ids (#6). Addresses CodeRabbit review on PR #165 (comments #3, #6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(electron/browser): dynamic UA platform + authorize open-local-file IPC - Derive Sec-CH-UA-Platform and the User-Agent platform token from process.platform so Windows/Linux users aren't misreported as macOS (#1). - Reject cabinet:open-local-file calls that don't come from the main renderer via isMainRendererSender, so other processes can't open arbitrary files (#2). Addresses CodeRabbit review on PR #165 (comments #1, #2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(open-url): guard decodeURIComponent and add noopener to window.open - Wrap decodeURIComponent of the file:// path in try/catch so malformed percent-encoding can't crash the click handler (#17). - Open external links with noopener,noreferrer so the target can't access window.opener (#18). Addresses CodeRabbit review on PR #165 (comments #17, #18). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browse): resolve markdown file URLs before directory/index.md - viewer-toolbar checks the file/.md case before directory/cabinet so a `<name>.md` page typed "directory" resolves to <name>.md, not index.md (#15). - tree-node browseFileUrl now handles node.type === "file" (markdown pages with the extension stripped) so they get the .md suffix instead of falling through to the raw asset URL (#16). Addresses CodeRabbit review on PR #165 (comments #15, #16). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browser-view): SSR guard, URL scheme validation, safer iframe sandbox - getBridge returns {} when window is undefined so it doesn't throw during SSR (#11). - normalizeBookmarkUrl only accepts http/https (and about:blank); other schemes collapse to about:blank (#10). - The fallback iframe drops allow-same-origin for same-origin/unknown URLs (it would otherwise let a page from our own origin escape the sandbox) and keeps it only for cross-origin external sites that need it to function (#9). Addresses CodeRabbit review on PR #165 (comments #9, #10, #11). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browser-view): track iframe history and fix load-failure detection - Record navigation history in iframe mode (push to iframeHistoryRef on genuine navigations, respect pending back/forward actions) so Back/Forward can replay pages without the Electron navigation event (#12). - Increment iframeLoadTokenRef on each load attempt and add iframeReloadKey to the failure-check effect's deps, so the timeout actually detects a frame that never loaded and reloads re-run the check (#13). Addresses CodeRabbit review on PR #165 (comments #12, #13). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browser-view): make extension menu item keyboard-accessible Replace the click-only div with a real button for running an extension, and reveal the pin button on keyboard focus (group-focus-within / focus-visible) in addition to hover. Addresses CodeRabbit review on PR #165 (comment #14). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ssrf): validate at socket lookup, cover more reserved ranges, drain redirects Rework the SSRF guard onto node http/https so the host is resolved and re-validated inside the socket lookup — closing the DNS-rebinding window where a hostname that passed an up-front check could connect to a private IP. Also: - extend isPrivateAddress with 198.18.0.0/15, TEST-NET 1/2/3, 192.0.0.0/24, 6to4 relay, IPv6 multicast (ff00::/8) and documentation ranges (#4); - drain + destroy intermediate responses while following redirects so sockets aren't leaked (#6); - keep host:port bookmark URLs like localhost:3000 intact instead of mangling them into about:blank (#2). Addresses CodeRabbit PR #165 second-review comments #2, #4, #5, #6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(browser-view): stop false iframe-failure overlay; keep host:port bookmarks - Drop the load-completion signal from the failure-check effect deps (and the now-redundant iframeLoadedToken state). Re-running the effect when a page finished loading bumped the load token again and flagged a false failure on pages that actually loaded fine; the timer reads the ref directly (#1). - normalizeBookmarkUrl keeps host:port targets like localhost:3000 instead of treating the port as a scheme and collapsing to about:blank (#3). Addresses CodeRabbit PR #165 second-review comments #1, #3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ist targeting Move list indent padding from <li> to <ul>/<ol> container to fix loose list rendering with `list-style-position: outside`. Update drag handle to anchor to parent list edge for list items (avoiding marker overlap) and implement per-item targeting with recursive descent into nested sub-lists. Fix stale view detection, improve add-button insertion logic for list contexts, and resolve type annotations.
hilash
pushed a commit
that referenced
this pull request
Jun 28, 2026
* feat(editor,sidebar): add file:// links and tree-view sorting Ported from #96, excluding the unrelated changes bundled in those commits (Typst, table-menu UX, PDF icon, Tailwind migrations). file:// protocol support: - Link extension now allows any URI (isAllowedUri) so file:// links render - editor click handler opens file:// links via a new openLocalFileUrl helper: shell.openPath in Electron, a "Copy path" toast in the browser - markdownToHtml encodes spaces in file:// URLs so [text](file://…) with spaces parses as a single link - new cabinet:open-local-file IPC handler + openLocalFile preload bridge Tree-view sorting: - alphabetical sort with an optional folders-first toggle (Settings → Appearance → Sidebar), persisted to localStorage - sortTreeNodes applied to the local tree and Google Drive mounts; toggling re-sorts from rawNodes without a server refetch - dragging while sorting is on prompts a confirm dialog (movePage now defers to executeMovePage) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tree-store): guard sort-preference localStorage writes Wrap the setItem calls in setSortAlphabetical/setFoldersFirst in try/catch so a storage failure (private mode, quota) can't throw and block the state update, matching the existing loader pattern. Addresses CodeRabbit review on PR #164. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(open-local-file): surface bridge open failures as a toast Await the openLocalFile bridge result and dispatch an error toast when the open fails (missing file, permissions) or the promise rejects, instead of voiding the result and leaving the user with no feedback. Addresses CodeRabbit review on PR #164. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(electron): remove duplicate cabinet:open-local-file IPC handler The merge of main left two registrations for the same IPC channel: one in main.cjs (editor file:// links) and one in browser-views.cjs (browse mode). ipcMain.handle throws on a duplicate channel, and the throw fired inside app.whenReady() before createWindow(), so the main window was never created and the app hung with no window after launch. Keep the browser-views.cjs handler (it adds an isMainRendererSender auth check) and drop the duplicate from main.cjs, along with the now unused shell import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(electron): drop duplicate openLocalFile bridge in preload CabinetDesktop defined openLocalFile twice; the earlier entry under the browse-mode header was silently shadowed by the later one. Keep the documented definition and remove the redundant one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Large sync: 8 add/add files (browser view, LaTeX ext/render/viewer, bookmarks/frame-check routes, open-url, browser-preload) kept LOCAL per user choice. 20 content-conflict files resolved preserving local features (typst, drawio, canvas mode, MyST/MDX/live-code/math transforms, inline extension management, fullscreen focus) while folding in upstream work (Apple Notes import, task-item markdown, browser-views.cjs refactor, concurrency-capped conversation listing, corrected browse-URL ordering, postinstall hardening). Also fixed auto-merge duplication the 3-way merge left in common regions: - tree-store.ts: duplicate loadSortAlphabetical/loadFoldersFirst/sortTreeNodes - to-html.ts: duplicate convertLatexEmbeds/encodeFileUrls - extensions.ts: duplicate LatexEmbedExtension; slash-commands/tree-node dup imports Added openExternalUrl to the local open-url.ts (used by integrations connect-panel) and removed lucide imports orphaned by keeping the local image-icon tree design. Validation: tsc --noEmit clean; full suite 377 passing. Co-Authored-By: Oz <oz-agent@warp.dev>
…n and symlink management
Resolve conflicts across the editor, extensions, and task panels while preserving local features: - editor.tsx: keep local source-mode split-view as an overlay atop upstream's always-mounted editor. - extensions.ts: adopt upstream MermaidCodeBlock (extends CodeBlockLowlight); keep LiveCodeBlock/MdxComponent registration. - task-detail-panel.tsx: take upstream compose-mode close button. - package-lock.json: regenerate via npm install against merged package.json. - path-utils.test.ts: build fixture under the module's managed DATA_DIR (<CABINET_DATA_DIR>/Cabinet) so resolveAgentCwd's stale-workdir fallback test matches the local managed-data-dir architecture. All 380 tests pass; tsc --noEmit clean. Co-Authored-By: Oz <oz-agent@warp.dev>
…lic page sync functionality
Resolve 19 conflicts preserving local features + OKF architecture: - Editor viewers (mermaid/latex/notebook): wrap local split-screen/source toolbars in upstream's ViewerLayout shell (dropped upstream toggles that referenced undeclared showSource/saveError state). - source-viewer.tsx: kept local split-screen source editing (--ours); upstream's HTML iframe-preview rewrite not adopted here. - editor.tsx: adopt upstream ContentSheet body; move split-mode props onto the desk toolbar (no duplicate toolbar). Kept OKF sibling-page wording. - editor-toolbar.tsx: adopt upstream borderless look (no border/bg, gutter-fade gradients, content height, edge mask, wired keyboard nav) while keeping local split-mode buttons and the splitMode width-toggle guard. - viewer-toolbar.tsx: kept canvas mode, focus/fullscreen, file-history-on-viewers. - sidebar.tsx: kept local NewItemMenu footer (page/folder/cabinet); combined Integrations button styling. - new-page-dialog.tsx: kept local isControlled API (callers depend on it). - settings-page.tsx: additive (local public-sync state + upstream dataDirCopied); removed duplicate Blocks import (auto-merge artifact). - tree-node/tree-view: took upstream cosmetic Manila Arc styling + icon imports. - package.json: keep mystmd, take upstream security-patched next 16.2.10. - package-lock.json regenerated; PROGRESS.md merged additively. Validation: tsc --noEmit clean; 380/380 tests pass. OKF storage core (page-io/path-utils/wiki-link-okf-converter) unchanged vs local main. Co-Authored-By: Oz <oz-agent@warp.dev>
…improve file sanitization logic
…pload support - Implement ModelViewer MDX component with Three.js editor proxy route for .glb/.gltf rendering - Add dedicated media-player page for fullscreen video/audio playback - Enable CSV drag-and-drop from sidebar as markdown tables in editor - Raise video upload limit to 300MB (from 25MB) and configure Next.js proxyClientMaxBodySize - Add x-cabinet-filename header handling for non-ASCII filenames (undici multipart workaround)
…ansforming request and response bodies
…s and add native toast notifications - Initialize ElectronChromeExtensions for browser session with tab management callbacks - Add extension toolbar menu with pin/unpin support and icon rendering - Implement native toast popup using frameless BrowserWindow with copy-to-clipboard - Forward extension service worker open-url messages from content scripts to main process - Track activeWebContents for extension tab selection and visibility
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings this fork's feature work onto
upstream/mainand keeps it current by periodically merginghilash/cabinet:main, along with fixes for regressions surfaced while resolving conflicts.Features
.texembed rendering,file://protocol support, and tree-view alphabetical sorting with a folders-first toggle.Fixes applied during integration
page-io— honor a requested extension when renaming typed files (e.g.photo.jpg→holiday.png), restoring behavior dropped during conflict resolution.agents— complete Ollama adapter wiring (sync the client-safe legacy-id mirror with the server registry) and makeresolveProviderModelreturnundefinedwhen a provider exposes no models instead of fabricating a selection.settings— restore the Integrations tab (nav entry, content block, and imports) that was inadvertently dropped while resolving repeatedsettings-page.tsxconflicts.Upstream sync
This branch is kept current with
hilash/cabinet:mainvia periodic merges. The latest sync (merge4c85f44) integrates upstream's Apple Notes import, Tiptap task-item markdown serialization, theimport-folderrefactor, and slug-resolution improvements. Notable conflict resolutions:types/index.ts) — keep both the arbitrary-frontmatter index signature and upstream'sappleNotesfield /AppleNotesFrontmatterinterface.to-markdown.ts) — retain all turndown rules:documentProperties,liveCodeBlock, and upstream's newtaskItem.editor.tsx#findPageBySlugandreferences.ts#resolvePageBySlugto one predicate (exact name/path +endsWith+slugify(lastSeg)) plusparentOfsibling matching, so the client/server mirror stays behaviourally identical.page-io— keep the sibling-patterncreatePageplus upstream'sensureContainerDir; preserveappleNoteson read.importDirectory(the inlinecopyDirimplementation is superseded).Earlier syncs additionally folded in upstream's diagnostics section, browser-extension/Ollama groundwork, and the
dev/pre-releaseline of work.Testing
npm test→ 362 passing, 0 failing (latest, post upstream sync).npm run dev:all) boots cleanly (Next.js app + Cabinet daemon).Warp conversation