feat(extensions): implement iframe-based isolation with TinyBase sync#1957
feat(extensions): implement iframe-based isolation with TinyBase sync#1957
Conversation
- Add TinyBase postMessage synchronizer utilities for iframe communication - Create /app/ext-host route for extension iframe hosting - Update TabContentExtension to render extensions in isolated iframes - Expose TinyBase hooks to extensions via extension-globals - Update build script to handle tinybase/ui-react imports - Update hello-world extension to demonstrate TinyBase store access and sync This implements a VS Code-like extension isolation pattern where extension UIs run in sandboxed iframes with TinyBase state synchronized via postMessage. Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughReplaces in-process extension UIs with an iframe-hosted ext-host route, adds parent↔iframe TinyBase synchronizers over postMessage, exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Parent as Desktop App (parent)
participant Iframe as Embedded <iframe>
participant ExtHost as /app/ext-host (iframe page)
participant Store as TinyBase Mergeable Store
Parent->>Iframe: create <iframe src="/app/ext-host?extensionId&scriptUrl">
Parent->>Parent: createIframeSynchronizer(store, iframe) — start
Iframe->>ExtHost: ext-host route loads with search params
ExtHost->>Store: create mergeable store
ExtHost->>ExtHost: createParentSynchronizer(store) — start
ExtHost->>ExtHost: fetch scriptUrl → validate JS → inject script
ExtHost->>ExtHost: read __hypr_panel_exports.default → mount inside TinyBaseProvider
note over ExtHost,Parent: Bidirectional TinyBase sync via postMessage (tinybase-sync envelope)
ExtHost->>Parent: postMessage(sync)
Parent->>Parent: iframe synchronizer applies → update parent store
Parent->>Iframe: postMessage(sync update)
ExtHost->>ExtHost: parent synchronizer applies incoming updates
Parent->>Parent: destroy iframe synchronizer on parent unmount
ExtHost->>ExtHost: destroy parent synchronizer on iframe unload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/desktop/src/components/main/body/extensions/index.tsx (1)
106-119: Consider adding error handling forstartSync().If synchronization fails to start, the user won't see any feedback. Consider handling potential errors.
const synchronizer = createIframeSynchronizer<Schemas>( store, iframeRef.current, ); synchronizerRef.current = synchronizer; - synchronizer.startSync(); + synchronizer.startSync().catch((err) => { + console.error("Failed to start iframe sync:", err); + }); }, [store]);extensions/hello-world/ui.tsx (1)
46-52: Consider disabling the synced button when store is not connected.If the store is not connected, clicking "Synced +1" will update state on an undefined store, which may silently fail or cause unexpected behavior.
<Button onClick={handleIncrementSynced} variant="outline" size="sm" className="bg-green-50 hover:bg-green-100 border-green-200" + disabled={!storeConnected} > Synced +1 </Button>apps/desktop/src/routes/app/ext-host.tsx (2)
29-45: Potential stale closure:loadExtensionScriptcapturesentryPathbut effect has empty deps.If
entryPathchanges without remounting the component, the effect won't re-run with the new value. Consider addingentryPathto the dependency array, or if remounting is guaranteed, add a comment documenting this assumption.useEffect(() => { initExtensionGlobals(); const store = createMergeableStore(); storeRef.current = store; const synchronizer = createParentSynchronizer(store); synchronizerRef.current = synchronizer; synchronizer.startSync().then(() => { loadExtensionScript(); }); return () => { synchronizer.destroy(); }; - }, []); + }, [entryPath]);Note: You'll need to move
loadExtensionScriptinside the effect or wrap it withuseCallbackwithentryPathas a dependency to satisfy the linter.
38-40: Missing error handling forstartSync()failure.If synchronization fails, the extension won't load but no error will be displayed to the user.
- synchronizer.startSync().then(() => { - loadExtensionScript(); - }); + synchronizer.startSync() + .then(() => { + loadExtensionScript(); + }) + .catch((err) => { + setError(`Failed to start sync: ${err instanceof Error ? err.message : String(err)}`); + });apps/desktop/src/store/tinybase/iframe-sync.ts (3)
17-26: Consider stricter validation in the type guard.The type guard only validates that
payloadis an array but doesn't check its length or element types. Malformed messages could pass validation and cause runtime errors during destructuring (lines 61, 112).Consider adding stricter validation:
function isTinybaseSyncEnvelope(data: unknown): data is TinybaseSyncEnvelope { return ( typeof data === "object" && data !== null && "kind" in data && data.kind === "tinybase-sync" && "payload" in data && - Array.isArray(data.payload) + Array.isArray(data.payload) && + data.payload.length === 4 && + typeof data.payload[0] === "string" && + (data.payload[1] === null || typeof data.payload[1] === "string") && + typeof data.payload[2] === "number" ); }
74-74: Extract magic number to named constant.The timeout value
5(seconds) is hardcoded and duplicated in both functions. This makes it harder to maintain and understand the timeout behavior.+const SYNC_TIMEOUT_SECONDS = 5; + /** * Creates a TinyBase synchronizer for the parent (main webview) side * that syncs with an iframe via postMessage. */ export function createIframeSynchronizer<Schemas extends [object, object]>( store: MergeableStore<Schemas>, iframe: HTMLIFrameElement, targetOrigin = "*", ): Synchronizer<Schemas> { // ... - 5, // request timeout in seconds + SYNC_TIMEOUT_SECONDS, );
32-129: Consider extracting shared synchronizer logic.The two functions share ~80% of their implementation. While the context-specific differences (parent vs. iframe) may justify some duplication, extracting common logic would improve maintainability.
Consider a helper that accepts send/receive strategy:
function createPostMessageSynchronizer<Schemas extends [object, object]>( store: MergeableStore<Schemas>, config: { send: (envelope: TinybaseSyncEnvelope, origin: string) => void; shouldAcceptMessage: (event: MessageEvent) => boolean; targetOrigin: string; }, ): Synchronizer<Schemas> { const clientId = getUniqueId(); let handler: ((event: MessageEvent) => void) | null = null; const synchronizer = createCustomSynchronizer( store as MergeableStore<[object, object]>, (_toClientId, requestId, message, body) => { const payload: TinybaseSyncPayload = [clientId, requestId, message, body]; config.send({ kind: "tinybase-sync", payload }, config.targetOrigin); }, (receive) => { handler = (event: MessageEvent) => { if (!config.shouldAcceptMessage(event)) return; if (!isTinybaseSyncEnvelope(event.data)) return; const [fromClientId, requestId, message, body] = event.data.payload; receive(fromClientId, requestId, message, body); }; window.addEventListener("message", handler); }, () => { if (handler) { window.removeEventListener("message", handler); handler = null; } }, SYNC_TIMEOUT_SECONDS, ); return synchronizer as unknown as Synchronizer<Schemas>; }Then implement the public functions as thin wrappers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/components/main/body/extensions/index.tsx(3 hunks)apps/desktop/src/extension-globals.ts(3 hunks)apps/desktop/src/routes/app/ext-host.tsx(1 hunks)apps/desktop/src/store/tinybase/iframe-sync.ts(1 hunks)extensions/build.mjs(2 hunks)extensions/hello-world/ui.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/desktop/src/extension-globals.tsapps/desktop/src/store/tinybase/iframe-sync.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/extension-globals.tsapps/desktop/src/store/tinybase/iframe-sync.tsextensions/hello-world/ui.tsxapps/desktop/src/components/main/body/extensions/index.tsxapps/desktop/src/routes/app/ext-host.tsx
🪛 GitHub Actions: .github/workflows/desktop_ci.yaml
apps/desktop/src/routes/app/ext-host.tsx
[error] 11-11: TS2345: Argument of type '"/app/ext-host"' is not assignable to parameter of type 'keyof FileRoutesByPath | undefined'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
🔇 Additional comments (8)
extensions/build.mjs (1)
132-135: LGTM!The TinyBase UI React aliasing follows the established pattern for external dependencies. The resolution and loading hooks are consistent with existing handlers for React, ReactDOM, and
@hypr/*packages.Also applies to: 178-184
apps/desktop/src/extension-globals.ts (1)
4-4: LGTM!The global bridge for TinyBase UI React is properly typed and initialized, aligning with the build-time aliasing in
build.mjs.Also applies to: 17-17, 26-26
apps/desktop/src/components/main/body/extensions/index.tsx (2)
152-159: LGTM!The iframe implementation with
sandbox="allow-scripts allow-same-origin"provides appropriate isolation while enabling the required postMessage communication for TinyBase sync.
98-99: I'll help you verify this review comment about the type casting issue. Let me investigate the type definitions and usage patterns in the codebase.
<function_calls>
#!/bin/bashFirst, find where useStores is defined and how it's typed
rg -nP --type=ts "useStores(" -C5 | head -100
</function_calls>This will help me understand the return type of
useStores()and why the double cast might be necessary. Let me also search for the Store type definition.
<function_calls>
#!/bin/bashFind Store type/interface definition
rg -nP --type=ts "type Store|interface Store" -C3 | head -150
</function_calls>Let me also look at the specific file in question to see the context around lines 98-99:
<function_calls>
#!/bin/bashRead the extensions/index.tsx file around lines 95-110
find . -path "*/extensions/index.tsx" -type f | head -5
</function_calls>And check how
useStoreshook is defined:
<function_calls>
#!/bin/bashFind the hook definition
rg -nP --type=ts "export.*useStores|function useStores|const useStores" -C10 | head -150
</function_calls>extensions/hello-world/ui.tsx (1)
25-36: LGTM!The TinyBase integration correctly uses
useStore,useRow, anduseSetRowCallbackhooks for managing extension-specific persistent state. The type assertion onuseRowis a common pattern for TinyBase.apps/desktop/src/routes/app/ext-host.tsx (2)
62-86: Script injection pattern is functional but consider usingnew Functionor dynamic import alternatives.The current approach works but has limitations (e.g., CSP restrictions). The backup/restore of
__hypr_panel_exportsis a good safeguard for concurrent loads.
11-17: Route is correctly registered in TanStack Router file-based routing.The file
apps/desktop/src/routes/app/ext-host.tsxis properly placed for TanStack Router's file-based routing system. The path mapping is correct:
- File location:
apps/desktop/src/routes/app/ext-host.tsx→ Route path:/app/ext-host- Parent route
/appis defined inapps/desktop/src/routes/app/route.tsxwith properOutlet- Root route is configured in
apps/desktop/src/routes/__root.tsxThe route definition uses the correct
createFileRoutesyntax matching all other routes in the codebase. TanStack Router v1.139.3 automatically generates the route tree from the file structure, making this configuration valid. No TS2345 error exists.apps/desktop/src/store/tinybase/iframe-sync.ts (1)
97-103: Verify window.parent context.The code assumes
window.parentexists and is different fromwindow. In a top-level window,window.parent === window, which would create a messaging loop or fail the source check at line 109.Please verify that this synchronizer will only be called from within an actual iframe context, and consider adding a guard:
if (window === window.parent) { throw new Error("createParentSynchronizer must be called from within an iframe"); }
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/components/main/body/extensions/index.tsx (1)
95-96: Double type casting suggests type mismatch.The
as unknown as Storepattern indicates the types fromuseStores()don't align with your localStoretype. This could hide runtime issues if the actual store shape differs.Consider either:
- Properly typing the stores registry upstream, or
- Adding runtime validation before using the store.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/desktop/src/routeTree.gen.tsis excluded by!**/*.gen.ts
📒 Files selected for processing (2)
apps/desktop/src/components/main/body/extensions/index.tsx(3 hunks)apps/desktop/src/store/tinybase/iframe-sync.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/store/tinybase/iframe-sync.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/main/body/extensions/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
🔇 Additional comments (4)
apps/desktop/src/components/main/body/extensions/index.tsx (4)
103-116: Synchronizer not recreated ifstorechanges after iframe loads.The
handleIframeLoadcallback capturesstorevia dependency, but ifstorereference changes after the iframe has already loaded, no new synchronizer will be created with the updated store. The iframe'sonLoadonly fires once.If
storecan change during the component's lifetime, consider adding an effect that watchesstorechanges:+ useEffect(() => { + if (iframeRef.current && store) { + if (synchronizerRef.current) { + synchronizerRef.current.destroy(); + } + const synchronizer = createIframeSynchronizer( + store as unknown as MergeableStore, + iframeRef.current, + ); + synchronizerRef.current = synchronizer; + synchronizer.startSync(); + } + }, [store]);If the store reference is guaranteed stable, this can be safely ignored.
142-145: Clean URL construction with proper encoding.Good use of
URLSearchParamsfor building the iframe src - this properly encodes the parameters and prevents injection issues.
118-125: Cleanup effect correctly handles unmount.The empty dependency array ensures cleanup only runs on unmount, and the null assignment after destroy prevents stale reference issues.
149-156: I'll help you verify the security concern about the iframe sandbox and postMessage targetOrigin. Let me start by locating the relevant files and examining the postMessage implementation.
<function_calls>
#!/bin/bashFind iframe-sync.ts and related synchronizer files
fd -e ts -e tsx | grep -i 'iframe|sync' | head -20
</function_calls>Now let me search for postMessage and targetOrigin usage across the codebase:
<function_calls>
#!/bin/bashSearch for postMessage and targetOrigin usage
rg -n "postMessage|targetOrigin" --type=ts --type=tsx -C3
</function_calls>Let me also check the extensions index file structure to understand the context better:
<function_calls>
#!/bin/bashRead the extensions index file to see full context
wc -l apps/desktop/src/components/main/body/extensions/index.tsx
</function_calls><function_calls>
#!/bin/bashRead relevant portions of the extensions file around lines 149-156
sed -n '140,170p' apps/desktop/src/components/main/body/extensions/index.tsx
</function_calls>
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/desktop/src/routes/app/ext-host.tsx (2)
10-16: TightenvalidateSearchto fail fast on missing/invalid params
validateSearchcurrently just castsextensionIdandscriptUrltostring, so malformed or missing values silently becomeundefined as stringand only surface later (e.g., “No script URL provided”). Consider validating types and throwing early to keep route state consistent and errors easier to reason about.For example:
validateSearch: (search: Record<string, unknown>) => { const { extensionId, scriptUrl } = search; if (typeof extensionId !== "string" || typeof scriptUrl !== "string") { throw new Error("Missing or invalid extensionId/scriptUrl in search params"); } return { extensionId, scriptUrl }; },This keeps the Route contract tight and avoids passing bad props into the host.
47-92: Script loading flow looks solid; consider clarifying assumptions around__hypr_panel_exportsThe script loading logic (fetch, inject inline
<script>, snapshot/restore__hypr_panel_exports, and wiring the default export into React state) is coherent and minimal.Two small, optional improvements:
- Document the assumption that extension bundles synchronously set
window.__hypr_panel_exports.defaultduring evaluation; otherwise authors might be surprised that async initializers aren’t supported.- Optionally log (or surface) a more specific hint when
exportsis missing but the script evaluated, to help distinguish “no default export set” from runtime errors inside the extension script.No blockers here from a correctness standpoint.
apps/desktop/src/components/main/body/extensions/index.tsx (2)
96-103: Defensively handle missing TinyBase store forSTORE_ID
storeis derived as:const stores = useStores(); const store = stores[STORE_ID] as unknown as Store | undefined;If, for any reason,
stores[STORE_ID]is undefined (e.g., store not registered yet or misconfigured), the iframe will render buthandleIframeLoadwill no-op, so the extension UI appears but never syncs and no explicit error is shown.If
STORE_IDbeing present is a hard invariant, consider asserting early:if (!store) { throw new Error(`TinyBase store "${STORE_ID}" not found`); }Or, if you want graceful handling:
if (!store) { return ( <StandardTabWrapper> {/* simple “Store unavailable” UI */} </StandardTabWrapper> ); }Either path makes this failure mode much easier to diagnose.
143-147: Iframe src construction is safe and readableUsing
convertFileSrc(panelInfo.entry_path)plusURLSearchParamsforextensionIdandscriptUrlis a good balance of clarity and safety (proper encoding for arbitrary paths/IDs).If you ever need to extend this, consider centralizing host URL construction into a helper, but there’s no immediate need.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/main/body/extensions/index.tsx(3 hunks)apps/desktop/src/routes/app/ext-host.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/routes/app/ext-host.tsxapps/desktop/src/components/main/body/extensions/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : Use `motion/react` instead of `framer-motion`.
Applied to files:
apps/desktop/src/components/main/body/extensions/index.tsx
🧬 Code graph analysis (2)
apps/desktop/src/routes/app/ext-host.tsx (4)
extensions/hello-world/ui.tsx (1)
ExtensionViewProps(14-17)apps/desktop/src/store/tinybase/iframe-sync.ts (1)
createParentSynchronizer(92-142)apps/desktop/src/extension-globals.ts (1)
initExtensionGlobals(21-32)plugins/local-llm/src/ext/plugin.rs (1)
store(320-320)
apps/desktop/src/components/main/body/extensions/index.tsx (5)
plugins/local-llm/src/ext/plugin.rs (1)
store(320-320)apps/desktop/src/store/tinybase/main.ts (2)
STORE_ID(36-36)Store(73-73)apps/desktop/src/components/main/body/extensions/registry.ts (1)
getPanelInfoByExtensionId(71-76)apps/desktop/src/store/tinybase/iframe-sync.ts (1)
createIframeSynchronizer(33-84)apps/desktop/src/components/main/body/index.tsx (1)
StandardTabWrapper(349-368)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
🔇 Additional comments (2)
apps/desktop/src/routes/app/ext-host.tsx (1)
94-120: Error and loading UI are straightforward and adequateThe error and loading branches are simple, read well, and correctly gate on both
ComponentandstoreRef.current. Wrapping the extension view inTinyBaseProviderwith the in-memory store is exactly what the iframe host needs.No changes needed here.
apps/desktop/src/components/main/body/extensions/index.tsx (1)
119-126: Synchronizer cleanup on unmount is correct and minimalThe
useEffectcleanup that destroys any existing synchronizer and nulls the ref is exactly what’s needed to avoid dangling message listeners when the tab unmounts.No changes needed.
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/routes/__root.tsx (1)
32-40: Prefer TanStack Router hooks over directwindow.locationaccess.Using
window.location.pathnamedirectly is non-idiomatic for TanStack Router. Consider using the router's state via hooks for better type safety and consistency:+import { + createRootRouteWithContext, + type LinkProps, + Outlet, + useNavigate, + useRouterState, +} from "@tanstack/react-router"; function Component() { - // ext-host route runs in iframe without Tauri access, so skip auth/billing providers - // and navigation events (which use Tauri APIs) - const isExtHost = - typeof window !== "undefined" && - window.location.pathname.startsWith("/app/ext-host"); + // ext-host route runs in iframe without Tauri access, so skip auth/billing providers + // and navigation events (which use Tauri APIs) + const pathname = useRouterState({ select: (s) => s.location.pathname }); + const isExtHost = pathname.startsWith("/app/ext-host"); if (isExtHost) { return <Outlet />; } return <MainAppLayout />; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/__root.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/routes/__root.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: fmt
🔇 Additional comments (1)
apps/desktop/src/routes/__root.tsx (1)
45-55: LGTM!Clean extraction of the main app layout logic into a separate component. The wrapping of
AuthProvider,BillingProvider, and navigation event handling is correct and maintains the existing behavior for non-ext-host routes.
…ing in iframe Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
…s errors Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/desktop/src/main.tsx (1)
89-91: Consider guardinginitWindowsPluginso it never runs in iframe/ext-host contextsRight now
initWindowsPlugin()runs unconditionally, relying on the__TAURI_INTERNALS__polyfill to absorb calls inside iframes. To avoid any accidental Tauri calls or noisy rejected Promises in the extension-host iframes, it’s safer to align this with the iframe guard you added inauth.tsxand only initialize the windows plugin in the main webview.For example:
-// Initialize plugins - the polyfill in index.html handles iframe context -initWindowsPlugin(); -initExtensionGlobals(); +// Initialize plugins - only run the Windows plugin in the main (non-iframe) webview. +const isIframeContext = + typeof window !== "undefined" && window.self !== window.top; + +if (!isIframeContext) { + initWindowsPlugin(); +} + +initExtensionGlobals();This keeps the main window behavior unchanged while making the iframe story more explicit.
apps/desktop/src/components/main-app-layout.tsx (1)
29-60: Handle failures from plugin.listencalls to avoid unhandled rejectionsThe event wiring looks correct and you clean up listeners properly, but the
.listen(...)calls return Promises that aren’t error‑handled. If the plugins are unavailable or fail to register, this can produce unhandled rejections.You can add minimal logging while keeping behavior the same when things work:
useEffect(() => { @@ - windowsEvents - .navigate(webview) - .listen(({ payload }) => { - navigate({ to: payload.path, search: payload.search ?? undefined }); - }) - .then((fn) => { - unlistenNavigate = fn; - }); + windowsEvents + .navigate(webview) + .listen(({ payload }) => { + navigate({ to: payload.path, search: payload.search ?? undefined }); + }) + .then((fn) => { + unlistenNavigate = fn; + }) + .catch((error) => { + console.error("[MainAppLayout] Failed to bind windows navigate events", error); + }); @@ - deeplink2Events.deepLinkEvent - .listen(({ payload }) => { - navigate({ to: payload.to, search: payload.search }); - }) - .then((fn) => { - unlistenDeepLink = fn; - }); + deeplink2Events.deepLinkEvent + .listen(({ payload }) => { + navigate({ to: payload.to, search: payload.search }); + }) + .then((fn) => { + unlistenDeepLink = fn; + }) + .catch((error) => { + console.error("[MainAppLayout] Failed to bind deeplink events", error); + });This keeps the hook’s semantics but makes it more resilient when the desktop plugins aren’t present or misbehave.
apps/desktop/index.html (1)
17-31: Iframe Tauri polyfill looks good; consider documenting the “fail-closed” semanticsThe early, iframe‑only
__TAURI_INTERNALS__stub is a solid way to keep@tauri-apps/apiand plugins from throwing in extension iframes, while making any actual Tauri calls reliably fail (invokerejects,transformCallbackis a no‑op,convertFileSrcis identity).To make future usage clearer, I’d suggest a brief inline note that this polyfill is intentionally “fail‑closed” (no real Tauri side effects) and must not be relied on by extensions for real filesystem or window operations. That will help avoid someone later leaning on
convertFileSrchere as if it were the real thing.apps/desktop/src/auth.tsx (1)
195-202: FixuseAuthguard to correctly detect missing provider
AuthContextis initialized withnull, butuseAuthonly throws whencontext === undefined, so callinguseAuth()outside anAuthProviderwill quietly returnnullinstead of giving the intended error.You can tighten this up with:
export function useAuth() { const context = useContext(AuthContext); - if (context === undefined) { + if (!context) { throw new Error("'useAuth' must be used within an 'AuthProvider'"); } return context; }This makes misuse fail fast with a clear message instead of a later
nulldereference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/index.html(1 hunks)apps/desktop/src/auth.tsx(1 hunks)apps/desktop/src/components/main-app-layout.tsx(1 hunks)apps/desktop/src/main.tsx(1 hunks)apps/desktop/src/routes/__root.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/__root.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/main-app-layout.tsxapps/desktop/src/main.tsxapps/desktop/src/auth.tsx
🧬 Code graph analysis (1)
apps/desktop/src/components/main-app-layout.tsx (3)
apps/desktop/src/auth.tsx (1)
AuthProvider(74-193)apps/desktop/src/billing.tsx (1)
BillingProvider(43-120)plugins/windows/src/ext.rs (1)
navigate(19-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: fmt
🔇 Additional comments (2)
apps/desktop/src/components/main-app-layout.tsx (1)
11-27: Good separation of main app vs iframe/ext-host concernsDynamic
MainAppLayoutthat wraps routes withAuthProvider/BillingProviderand callsuseNavigationEvents()cleanly keeps auth/billing and Tauri-dependent bits out of the iframe/ext-host path while preserving behavior for the main window.apps/desktop/src/auth.tsx (1)
22-45: Iframe-aware auth initialization is consistent and avoids Tauri usage in extension hostsThe
isIframeContextguard plustauriStorage: SupportedStorage | nulland the conditionalsupabasecreation cleanly prevent Tauri storage and Supabase from being instantiated in iframe/ext-host contexts. Downstream checks (if (!supabase) return;) in the effects and methods mean iframe imports would safely no-op rather than touching Tauri APIs.This aligns well with the new layout split and the iframe polyfill strategy.
Also applies to: 47-53
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
- Use window.location.origin as default targetOrigin instead of '*' - Add origin validation in message handlers for both iframe and parent synchronizers - Handle missing contentWindow explicitly with error logging - Add .catch handlers for startSync() rejections in extensions/index.tsx - Add mounted ref and error handling for startSync() in ext-host.tsx - Fix pathname check in __root.tsx to use exact match or subpath - Remove allow-same-origin from iframe sandbox attribute Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/desktop/src/routes/__root.tsx (1)
31-34: Pathname matching is now precise.The exact match (
pathname === "/app/ext-host") combined with the prefix check (startsWith("/app/ext-host/")) properly addresses the previous concern about matching unintended routes like/app/ext-host-debug.Based on past review comments indicating the need for more precise pathname matching.
apps/desktop/src/routes/app/ext-host.tsx (1)
27-27: Unmount guard and error handling properly implemented.The
isMountedRefpattern correctly prevents state updates after unmount, and the.catch()handler onstartSync()ensures sync failures are logged and surfaced to the user. This addresses the previous review concern about unhandled promise rejections and post-unmount updates.Based on past review comments requesting unmount guards and error handling for startSync().
Also applies to: 29-60
apps/desktop/src/components/main/body/extensions/index.tsx (2)
104-122: Sync error handling properly implemented.The
.catch()handler onstartSync()now logs errors with context (extension ID). This addresses the previous concern about silent failures when the iframe sync handshake fails.Based on past review comments requesting error handling for startSync().
156-163: Sandbox attribute correctly restricts iframe privileges.The
sandbox="allow-scripts"(withoutallow-same-origin) prevents extension code from accessingwindow.parentand manipulating the parent DOM. This addresses the security concern raised in previous reviews while still allowingpostMessagecommunication.Based on past review comments about removing
allow-same-originfor better isolation.
🧹 Nitpick comments (3)
apps/desktop/src/routes/__root.tsx (1)
40-44: Consider adding a loading indicator instead of null fallback.The
Suspense fallback={null}will show nothing duringMainAppLayoutloading. While this may be brief, a minimal loading indicator (spinner or skeleton) would provide better UX feedback during the initial load.- <Suspense fallback={null}> + <Suspense fallback={<div className="flex items-center justify-center h-screen"><div className="animate-spin rounded-full h-8 w-8 border-b-2 border-neutral-900" /></div>}> <MainAppLayout /> </Suspense>apps/desktop/src/routes/app/ext-host.tsx (1)
11-13: Validate search parameters to prevent runtime errors.The
validateSearchfunction casts parameters without validation. IfextensionIdorscriptUrlare missing or malformed, the component will fail at runtime rather than at the route level.validateSearch: (search: Record<string, unknown>) => ({ - extensionId: search.extensionId as string, - scriptUrl: search.scriptUrl as string, + extensionId: + typeof search.extensionId === "string" && search.extensionId + ? search.extensionId + : (() => { + throw new Error("Missing or invalid extensionId"); + })(), + scriptUrl: + typeof search.scriptUrl === "string" && search.scriptUrl + ? search.scriptUrl + : (() => { + throw new Error("Missing or invalid scriptUrl"); + })(), }),apps/desktop/src/components/main/body/extensions/index.tsx (1)
96-97: Type assertion chain is fragile.The double cast
stores[STORE_ID] as unknown as Store | undefinedbypasses TypeScript's type checking. IfuseStores()returns a different structure or ifSTORE_IDis incorrect, this will silently fail at runtime.Consider a runtime guard:
const stores = useStores(); - const store = stores[STORE_ID] as unknown as Store | undefined; + const store = stores[STORE_ID]; + if (!store) { + console.error(`[extensions] Store "${STORE_ID}" not found`); + return ( + <StandardTabWrapper> + <div className="flex items-center justify-center h-full"> + <p className="text-red-500">Store initialization error</p> + </div> + </StandardTabWrapper> + ); + }Then update the subsequent checks to handle the typed
storesafely.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/main/body/extensions/index.tsx(3 hunks)apps/desktop/src/routes/__root.tsx(2 hunks)apps/desktop/src/routes/app/ext-host.tsx(1 hunks)apps/desktop/src/store/tinybase/iframe-sync.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/store/tinybase/iframe-sync.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/routes/__root.tsxapps/desktop/src/components/main/body/extensions/index.tsxapps/desktop/src/routes/app/ext-host.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : Use `motion/react` instead of `framer-motion`.
Applied to files:
apps/desktop/src/components/main/body/extensions/index.tsx
🧬 Code graph analysis (3)
apps/desktop/src/routes/__root.tsx (2)
apps/desktop/src/components/main-app-layout.tsx (1)
MainAppLayout(17-27)plugins/windows/src/events.rs (2)
window(14-14)window(32-32)
apps/desktop/src/components/main/body/extensions/index.tsx (4)
apps/desktop/src/store/tinybase/main.ts (2)
STORE_ID(36-36)Store(73-73)apps/desktop/src/components/main/body/extensions/registry.ts (1)
getPanelInfoByExtensionId(71-76)apps/desktop/src/store/tinybase/iframe-sync.ts (1)
createIframeSynchronizer(41-102)apps/desktop/src/components/main/body/index.tsx (1)
StandardTabWrapper(349-368)
apps/desktop/src/routes/app/ext-host.tsx (5)
apps/desktop/src/routes/__root.tsx (1)
Route(21-25)extensions/hello-world/ui.tsx (1)
ExtensionViewProps(14-17)apps/desktop/src/store/tinybase/iframe-sync.ts (1)
createParentSynchronizer(113-165)apps/desktop/src/extension-globals.ts (1)
initExtensionGlobals(21-32)plugins/local-llm/src/ext/plugin.rs (1)
store(320-320)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (macos, depot-macos-14)
🔇 Additional comments (5)
apps/desktop/src/routes/__root.tsx (1)
6-6: LGTM! Lazy loading prevents Tauri imports in iframe context.The lazy loading of
MainAppLayoutis appropriate given thatauth.tsxinstantiates a Supabase client at module level using Tauri APIs. This prevents those imports from executing in the iframe context where Tauri is unavailable.Also applies to: 13-15
apps/desktop/src/routes/app/ext-host.tsx (2)
10-16: LGTM! Route configuration is straightforward.The route definition and search parameter validation are clear and appropriate for passing extension metadata to the iframe host.
77-79: Preserving and restoring__hypr_panel_exportsis good practice.The code correctly saves any previous
__hypr_panel_exportsvalue before script injection and restores it afterward. This prevents collisions if multiple extensions are loaded sequentially.Also applies to: 96-98
apps/desktop/src/components/main/body/extensions/index.tsx (2)
111-114: Based on my thorough verification, I can now provide the rewritten review comment:Store is correctly typed as MergeableStore—no runtime risk from this cast.
The
Storetype exported fromapps/desktop/src/store/tinybase/main.ts(line 73) is defined asMergeableStore<Schemas>. The cast to unparameterizedMergeableStoreis necessary due to a TypeScript generic variance limitation:createIframeSynchronizeriniframe-sync.ts(line 42) accepts the unparameterized type from TinyBase's import. While the double cast bypasses type checking, it doesn't indicate an actual type mismatch—the store is aMergeableStoreat runtime. The synchronizer will function correctly.Likely an incorrect or invalid review comment.
148-152: Based on my verification, I've confirmed thatconvertFileSrcis indeed idempotent according to Tauri's official documentation—it's a pure converter function that returns the same URL given the same file path input. Additionally, the code flow shows thatentry_pathalways originates from the plugin backend's metadata (via@hypr/plugin-extensionsPanelInfo) as a file system path, andscriptUrlis generated fresh each time without being persisted back to storage. There is no evidence of pre-converted URLs being stored or reused.No issue exists. The concern about double-conversion is not applicable to this codebase.
The code is safe because:
convertFileSrcis idempotent by designentry_pathis always a file system path from plugin metadata- The converted URL is only passed as a parameter, never persisted back to storage
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/routes/app/ext-host.tsx (1)
61-106: Add content-type validation before script execution.The script is fetched and executed without verifying the response content-type. As noted in a prior review, adding a content-type check provides an additional safeguard against executing unexpected payloads.
const response = await fetch(scriptUrl); if (!response.ok) { throw new Error(`Failed to fetch extension script: ${response.status}`); } + const contentType = response.headers.get("content-type"); + if (!contentType?.includes("javascript") && !contentType?.includes("text/plain")) { + throw new Error(`Invalid content-type for extension script: ${contentType}`); + } + const scriptContent = await response.text();
🧹 Nitpick comments (2)
apps/desktop/src/store/tinybase/schema-internal.ts (1)
85-88: Consider schema extensibility for future extensions.The
extension_statetable schema is currently hardcoded withcounterandlast_updatedfields specific to the hello-world demo. If other extensions need different state shapes, they would require schema modifications.Consider whether a more flexible approach (e.g., a generic
dataJSON string field per extension) would better support diverse extension state needs, or if each extension is expected to have its own dedicated table entry in the schema.apps/desktop/src/routes/app/ext-host.tsx (1)
61-106: MoveloadExtensionScriptinside theuseEffector wrap withuseCallback.
loadExtensionScriptis defined as a function inside the component body but is only called from within theuseEffect. Since it referencesscriptUrland state setters, this creates a stale closure risk if the effect re-runs. Either move the function inside theuseEffectcallback or wrap it withuseCallbackand add it to the dependency array.useEffect(() => { isMountedRef.current = true; initExtensionGlobals(); const store = createMergeableStore(); storeRef.current = store; const synchronizer = createParentSynchronizer(store); synchronizerRef.current = synchronizer; + const loadExtensionScript = async () => { + if (!scriptUrl) { + setError("No script URL provided"); + return; + } + + try { + const response = await fetch(scriptUrl); + if (!response.ok) { + throw new Error(`Failed to fetch extension script: ${response.status}`); + } + + const scriptContent = await response.text(); + + const previousExports = ( + window as Window & { __hypr_panel_exports?: unknown } + ).__hypr_panel_exports; + + const script = document.createElement("script"); + script.textContent = scriptContent; + document.head.appendChild(script); + document.head.removeChild(script); + + const exports = ( + window as Window & { + __hypr_panel_exports?: { + default?: ComponentType<ExtensionViewProps>; + }; + } + ).__hypr_panel_exports; + + if (exports?.default) { + setComponent(() => exports.default!); + ( + window as Window & { __hypr_panel_exports?: unknown } + ).__hypr_panel_exports = previousExports; + } else { + setError("Extension did not export a default component"); + } + } catch (err) { + setError( + `Failed to load extension: ${err instanceof Error ? err.message : String(err)}`, + ); + } + }; + synchronizer .startSync() .then(() => { if (isMountedRef.current) { loadExtensionScript(); } }) // ... rest of the effect - }, []); + }, [scriptUrl]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/main.tsx(1 hunks)apps/desktop/src/routes/app/devtool.tsx(3 hunks)apps/desktop/src/routes/app/ext-host.tsx(1 hunks)apps/desktop/src/routes/app/route.tsx(2 hunks)apps/desktop/src/store/tinybase/main.ts(2 hunks)apps/desktop/src/store/tinybase/schema-internal.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/desktop/src/store/tinybase/main.tsapps/desktop/src/store/tinybase/schema-internal.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/store/tinybase/main.tsapps/desktop/src/routes/app/route.tsxapps/desktop/src/store/tinybase/schema-internal.tsapps/desktop/src/routes/app/devtool.tsxapps/desktop/src/routes/app/ext-host.tsxapps/desktop/src/main.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/app/ext-host.tsx (6)
apps/desktop/src/routes/app/devtool.tsx (1)
Route(18-20)apps/desktop/src/routes/app/route.tsx (1)
Route(8-13)apps/desktop/src/routes/__root.tsx (1)
Route(21-25)extensions/hello-world/ui.tsx (1)
ExtensionViewProps(14-17)apps/desktop/src/store/tinybase/iframe-sync.ts (1)
createParentSynchronizer(113-165)apps/desktop/src/extension-globals.ts (1)
initExtensionGlobals(21-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: fmt
🔇 Additional comments (7)
apps/desktop/src/routes/app/devtool.tsx (1)
126-157: LGTM!The
ExtensionStateMonitorcomponent correctly usesuseRowto read extension state from the persisted store. The type assertion is appropriate given TinyBase's dynamic row shape. The fallback handling forcounter(defaulting to 0) and conditional rendering oflastUpdatedare well implemented.apps/desktop/src/routes/app/route.tsx (1)
15-26: LGTM!The conditional rendering correctly bypasses
ListenerProviderandSideEffectsfor the ext-host route, which is appropriate since iframes should not run main-window side effects. UsingstartsWithfor the pathname check is suitable here.apps/desktop/src/main.tsx (3)
69-71: LGTM!Standard iframe context detection pattern. The
window.self !== window.topcheck reliably identifies when running inside an iframe.
78-82: LGTM!The logic correctly ensures iframes don't persist the store (parent handles persistence) and don't claim to be the main window.
97-99: Based on my verification, I can now provide the final assessment:No issues found - plugin initialization is safe in iframe context, but for different reasons than stated.
The review comment incorrectly assumes
initWindowsPlugin()relies on the Tauri polyfill. However, examining the actual implementation inplugins/windows/js/index.ts, theinit()function only sets up DOM event listeners for drag-and-drop functionality—it does NOT call any Tauri APIs. Similarly,initExtensionGlobals()only assigns React and utility libraries to window globals.The polyfill in
index.htmlis actually needed for other parts of the codebase (like thegetCurrentWebviewWindowLabel()call on line 82), which is properly guarded to only execute outside iframe context. Both unconditional initialization calls on lines 98-99 are genuinely safe in iframe context regardless of the polyfill.apps/desktop/src/routes/app/ext-host.tsx (1)
29-59: LGTM on mount guard implementation.The
isMountedRefpattern correctly prevents state updates after unmount, and error handling forstartSync()is properly implemented. This addresses the concerns from previous reviews.apps/desktop/src/store/tinybase/main.ts (1)
159-199: Persist-guarded creation oflocalPersister2and[persist]deps look correctThe early
if (!persist) return undefined;plus the[persist]dependency array alignlocalPersister2’s lifecycle with thepersistflag and avoid unnecessary FS work when persistence is disabled. This is consistent with how the mainlocalPersisteris handled and should behave well even ifpersistchanges at runtime, especially given the existinguseEffectguard on!persist || !localPersister || !localPersister2.If you expect
persistto toggle dynamically in practice (not just per-context), it’s worth manually testing that transition to confirm persisters are created/destroyed and window listeners registered/unregistered as intended.
- Add explicit type checking for extensionId and scriptUrl in validateSearch - Validate scriptUrl is a well-formed URL, reject javascript: protocol - Return undefined for invalid inputs instead of silent casts - Add content-type validation before script injection - Add early return for missing extensionId in component - Add security documentation to extensions.mdx Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/routes/app/ext-host.tsx (1)
61-91: Sync lifecycle properly guarded—addresses previous review.The useEffect correctly implements the mount guard and error handling requested in the previous review. The isMountedRef prevents post-unmount state updates, and the .catch() handler surfaces sync failures gracefully.
apps/web/content/docs/developers/8.extensions.mdx (1)
219-236: Security documentation addresses previous review feedback.The new Security Model section documents the trust boundaries and validation mechanisms requested in the previous review. The documentation accurately describes:
- How convertFileSrc restricts file access
- URL and content-type validation steps
- Export contract validation
- Iframe sandbox restrictions
Verify that the iframe implementation actually uses
sandbox="allow-scripts"as documented:#!/bin/bash # Description: Verify iframe sandbox attribute matches documentation # Expected: Find iframe with sandbox="allow-scripts" attribute rg -n '<iframe' apps/desktop/src --type tsx --type ts -A 5 | rg -i 'sandbox'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/app/ext-host.tsx(1 hunks)apps/web/content/docs/developers/8.extensions.mdx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/routes/app/ext-host.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/app/ext-host.tsx (3)
extensions/hello-world/ui.tsx (1)
ExtensionViewProps(14-17)apps/desktop/src/store/tinybase/iframe-sync.ts (1)
createParentSynchronizer(113-165)apps/desktop/src/extension-globals.ts (1)
initExtensionGlobals(21-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: fmt
- GitHub Check: ci (macos, macos-14)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
🔇 Additional comments (1)
apps/desktop/src/routes/app/ext-host.tsx (1)
27-48: Validation improvements address previous review feedback.The validateSearch now properly validates types and URL format, rejecting javascript: URLs. While the previous review suggested throwing errors for invalid params, returning partial results is a valid alternative approach—the component safely handles missing fields with appropriate error states (lines 94-97, 147-156).
… unmount - Wrap setError and setComponent calls in loadExtensionScript with isMountedRef checks - Only restore previousExports when component is still mounted - Prevents React state update warnings on unmounted components Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/routes/app/ext-host.tsx (1)
15-45: Clarify script URL trust model and optionally tighten allowed protocols
isValidUrlcurrently accepts any syntactically valid URL exceptjavascript:, soscriptUrlcould point at arbitrary HTTP/data/blob/file URLs if someone manually navigates to this route. Given this host is intended to run extension bundles from trusted locations (e.g.,convertFileSrcoutput), you may want to (a) document that assumption here and (b) optionally narrow which protocols you accept.One way (leaving the exact allow‑list for you to tune) is:
-function isValidUrl(url: string): boolean { - try { - const parsed = new URL(url); - if (parsed.protocol === "javascript:") { - return false; - } - return true; - } catch { - return false; - } -} +// Extension scripts are expected to be served from trusted locations +// (for example, Tauri convertFileSrc output for installed extensions). +// This helper intentionally rejects obviously unsafe schemes and can +// be tightened further if needed. +function isValidUrl(url: string): boolean { + try { + const parsed = new URL(url); + if (parsed.protocol === "javascript:") { + return false; + } + // Optionally narrow to an explicit allow‑list that matches your setup, + // e.g. asset:/http(s) only: + // const allowed = new Set(["asset:", "https:", "http:"]); + // if (!allowed.has(parsed.protocol)) return false; + return true; + } catch { + return false; + } +}This keeps current behavior but makes the trust model explicit and leaves a clear place to harden further if desired. This overlaps with the earlier security‑assumptions comment.
🧹 Nitpick comments (2)
apps/desktop/src/routes/app/ext-host.tsx (2)
93-101: Avoid loading scripts whenextensionIdis missing to keep invalid routes side‑effect‑freeIf the route is hit with a valid
scriptUrlbut missing/invalidextensionId,ExtHostComponentwill still create the store, start sync, and callloadExtensionScript; only the render path short‑circuits on!extensionId. For malformed URLs or manual navigation, it’s slightly safer and cheaper to skip the loader entirely whenextensionIdis falsy.You can guard this in
loadExtensionScriptwithout touching hook dependencies:- const loadExtensionScript = async () => { - if (!scriptUrl) { + const loadExtensionScript = async () => { + if (!extensionId) { + // Render-level guard will already show a clearer message. + return; + } + + if (!scriptUrl) { if (isMountedRef.current) { setError("No script URL provided"); } return; }This keeps behavior for valid routes the same while avoiding unnecessary sync and network work when the extension ID itself is invalid.
Also applies to: 155-164
115-145: Always restore__hypr_panel_exportsso globals don’t leak across loads or after unmountRight now you restore
__hypr_panel_exportsonly in the success path and only whenisMountedRef.currentis true. If the script sets__hypr_panel_exportsbut:
- Fails the “default export” contract, or
- Resolves after unmount,
then the global may remain overwritten until the next host load.
You can decouple global cleanup from React mount state and run it unconditionally after inspecting
exports:- const previousExports = ( - window as Window & { __hypr_panel_exports?: unknown } - ).__hypr_panel_exports; + const windowWithExports = window as Window & { + __hypr_panel_exports?: { + default?: ComponentType<ExtensionViewProps>; + }; + }; + const previousExports = windowWithExports.__hypr_panel_exports; @@ - const script = document.createElement("script"); + const script = document.createElement("script"); script.textContent = scriptContent; document.head.appendChild(script); document.head.removeChild(script); - const exports = ( - window as Window & { - __hypr_panel_exports?: { - default?: ComponentType<ExtensionViewProps>; - }; - } - ).__hypr_panel_exports; - - if (exports?.default) { - if (isMountedRef.current) { - setComponent(() => exports.default!); - ( - window as Window & { __hypr_panel_exports?: unknown } - ).__hypr_panel_exports = previousExports; - } - } else { - if (isMountedRef.current) { - setError("Extension did not export a default component"); - } - } + const exports = windowWithExports.__hypr_panel_exports; + + if (exports?.default) { + if (isMountedRef.current) { + setComponent(() => exports.default); + } + } else if (isMountedRef.current) { + setError("Extension did not export a default component"); + } + + // Always restore previous exports so global state doesn't leak between loads. + windowWithExports.__hypr_panel_exports = previousExports;This keeps the React behavior identical while ensuring the global helper surface is always returned to its prior value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/app/ext-host.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/routes/app/ext-host.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (macos, macos-14)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: fmt
🔇 Additional comments (2)
apps/desktop/src/routes/app/ext-host.tsx (2)
61-91: startSync lifecycle and unmount behavior now look robustThe
useEffectblock now correctly:
- Tracks mount state with
isMountedRef.- Wraps
startSync()in a.then/.catch, guardingloadExtensionScript()andsetErrorwithisMountedRef.current.- Always calls
synchronizer.destroy()in the cleanup.This addresses the earlier unhandled‑rejection and post‑unmount setState concerns for the TinyBase sync bootstrap. No changes needed here.
166-191: Error/loading states and TinyBase provider wiring look correctThe render logic cleanly separates:
- Missing/invalid
extensionId→ dedicated error message.- Generic loader failures →
errorbranch.- Initial “no component or store yet” → loading message.
- Happy path →
TinyBaseProviderwith a non‑null store and the extension component receivingextensionId.This keeps the UX straightforward and ensures the TinyBase store is always present when you render the extension. No changes needed.
1 participant
feat(extensions): implement iframe-based isolation with TinyBase sync
Summary
This PR implements VS Code-style iframe isolation for extension UIs, replacing the previous direct script injection approach. Extensions now run in sandboxed iframes with TinyBase state synchronized via postMessage.
Key changes:
/app/ext-hostroute - Hosts extension iframes, initializes in-memory TinyBase store, and loads extension scriptsiframe-sync.ts) - Custom synchronizer usingcreateCustomSynchronizerfor parent↔iframe communicationTabContentExtension- Now renders extensions in iframes instead of directly in the main windowtinybase/ui-reactto extensions via the build system and extension globalsArchitecture: The parent (main webview) owns the canonical TinyBase store with persistence. Each extension iframe has an in-memory MergeableStore that syncs with the parent via postMessage. The iframe never calls Tauri directly - the parent handles all Tauri operations including
convertFileSrcfor script URLs.Updates since last revision
Input validation, content-type check & unmount guards (latest):
validateSearch- verifiesextensionIdandscriptUrlare non-empty stringsnew URL()to reject malformed URLs andjavascript:protocolundefinedfor invalid inputs instead of silent castsextensionIdjavascriptisMountedRefguards to allsetErrorandsetComponentcalls inloadExtensionScriptto prevent state updates after unmountapps/web/content/docs/developers/8.extensions.mdxSecurity hardening (previous):
targetOrigindefault from"*"towindow.location.originfor postMessage securityisAllowedOrigin()checkcontentWindownull check with error logging instead of silent no-op.catch()handlers forstartSync()rejections with proper error loggingisMountedRefinext-host.tsxto prevent state updates after unmount__root.tsxto use exact match (=== "/app/ext-host") or subpath (startsWith("/app/ext-host/")) to avoid matching unintended routesallow-same-originfrom iframe sandbox attribute (now justsandbox="allow-scripts")Previous updates:
MergeableStorefromtinybaseinstead of schema-typed versionsconvertFileSrcand passesscriptUrlto iframe__TAURI_INTERNALS__polyfill inindex.html: Provides stub implementations for Tauri APIs in iframe contextauth.tsxandmain.tsxto skip Tauri-dependent codemain-app-layout.tsx: Loaded vialazy()to preventauth.tsxfrom being imported in iframe contextReview & Testing Checklist for Human
ONBOARDING=0 pnpm -F desktop tauri dev, click "Unknown" profile section, click "Hello World". Verify extension loads with "Store Status: Connected: Yes" in green. The content-type check may reject scripts if the dev server doesn't set proper headers containing "javascript".isMountedRefguards).Test recording (recorded before latest security changes - recommend re-testing):
Notes
use-config.ts- these are expected and come from the polyfill rejecting Tauri API calls in iframe contextextension_statetable used by hello-world may not exist in the TinyBase schema - verify this doesn't cause errorsMergeableStoreallow-same-originfrom sandbox is more restrictive - if extensions need same-origin access for any reason, this may need adjustmentRequested by: yujonglee (@yujonglee)
Link to Devin run: https://app.devin.ai/sessions/1f3bfa6345fe4092ab3dcd1a59391e00