-
Notifications
You must be signed in to change notification settings - Fork 1
Embed SDK Explorer in docs #332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add SDKExplorerIframe component with theme sync via postMessage - Add SDK-Explorer.mdx page with full-width iframe layout - Update redirect and nav URL to /SDK-Explorer - Fade-in iframe on load, prevent reload on theme change
📝 WalkthroughWalkthroughAdds a new SDK Explorer iframe component and integrates it as the docs root when no slug is provided; updates global styles, navigation/redirects, metadata handling, and many internal documentation link targets and site metadata ordering. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextPage as Next.js Page
participant SDKExplorer as SDKExplorerIframe
participant IFrame as Explorer IFrame
participant External as External SDK Service
Browser->>NextPage: Request docs root (no slug)
NextPage->>SDKExplorer: Render SDKExplorerIframe
SDKExplorer->>IFrame: Set src with initialTheme
IFrame->>External: Load explorer assets/content
External-->>IFrame: Return content
Note over SDKExplorer,IFrame: Runtime interactions
SDKExplorer->>IFrame: postMessage(SET_THEME)
IFrame-->>SDKExplorer: postMessage(NAVIGATE, path)
SDKExplorer->>Browser: Validate origin/path and navigate (relative only)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (8)
🔇 Additional comments (15)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @components/SDKExplorerIframe.tsx:
- Around line 38-46: The message handler in SDKExplorerIframe.tsx's useEffect
(handleMessage) doesn't validate event.origin, enabling open redirects; update
handleMessage to verify the message origin (or message.source matches the known
iframe contentWindow) against an allowlist of trusted origins before acting on
event.data.type === 'NAVIGATE', and only then perform window.location.href =
event.data.path; keep the origin check strict (exact match or configured list)
and ensure the removal of the listener remains unchanged.
- Around line 28-35: The postMessage call in the useEffect currently uses '*'
which broadcasts the theme to all frames; change the targetOrigin to the
explorer iframe's origin instead (compute it via new
URL(explorerUrlOrSrc).origin) and pass that as the second argument to
iframeRef.current.contentWindow.postMessage; ensure you reference the iframe
source or prop (e.g., explorerUrl or iframeRef.current.src) to compute the
origin, validate it exists before posting, and fall back to no-op or a safe
default if the origin cannot be determined.
🧹 Nitpick comments (2)
app/global.css (1)
160-166: Hardcoded mobile nav height may drift from actual nav.The mobile breakpoint uses a hardcoded
56pxfortop, while the desktop version usesvar(--fd-nav-height, 0px). If the fumadocs-ui nav height changes, this will need manual synchronization.Consider using the CSS variable consistently, or add a comment documenting why
56pxis appropriate for mobile if the variable isn't available in that context.app/(docs)/[[...slug]]/page.tsx (1)
34-46: URL case sensitivity may cause issues.The check
params.slug?.[0] === 'SDK-Explorer'is case-sensitive. If users navigate to/sdk-exploreror/Sdk-Explorer, they'll get the regular DocsPage rendering instead of the iframe.Consider using a case-insensitive comparison if the URL should be flexible:
- const isSDKExplorer = params.slug?.[0] === 'SDK-Explorer'; + const isSDKExplorer = params.slug?.[0]?.toLowerCase() === 'sdk-explorer';Alternatively, if the canonical URL is strictly
/SDK-Explorer, this is fine as-is and Next.js redirects can enforce the casing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app/(docs)/[[...slug]]/page.tsxapp/global.cssapp/layout.config.tsxcomponents/SDKExplorerIframe.tsxcontent/Get-Started/what-is-agentuity.mdxcontent/SDK-Explorer.mdxcontent/meta.jsonnext.config.mjs
🔇 Additional comments (11)
content/SDK-Explorer.mdx (1)
1-7: LGTM!Clean placeholder MDX file with appropriate frontmatter. The
full: trueflag and comment explaining the special handling inpage.tsxprovide good context for future maintainers.next.config.mjs (1)
10-20: LGTM!The redirect configuration is well-structured:
- Root redirect to
/SDK-Explorerusespermanent: false, allowing flexibility to change the landing page later.- The
/Introductionredirect correctly usespermanent: truesince it's a URL migration.app/layout.config.tsx (1)
9-11: LGTM!The navigation URL update to
/SDK-Exploreris consistent with the redirect configuration innext.config.mjs, ensuring the navbar logo click and root redirect both lead to the same destination.content/meta.json (1)
4-6: LGTM!Placing
SDK-Explorerfirst in the navigation order is consistent with the PR's goal of making it the primary entry point. This aligns with the redirect and nav URL changes.content/Get-Started/what-is-agentuity.mdx (3)
1-4: LGTM!The title change from "Introduction" to "What is Agentuity" is more descriptive and aligns with the redirect from
/Introductionto this page's new URL.
10-28: Verify the model namegpt-5-miniis intentional.The example uses
openai("gpt-5-mini")which doesn't appear to be a current OpenAI model name. If this is a placeholder for documentation purposes, consider adding a comment or using a real model name likegpt-4o-minito avoid confusion for readers following the example.
51-64: LGTM!The table formatting and JSX wrapper changes are clean cosmetic updates with no functional impact.
app/global.css (1)
145-152: LGTM!The fixed positioning approach with CSS variables for sidebar and nav dimensions is clean and integrates well with the fumadocs-ui layout system.
app/(docs)/[[...slug]]/page.tsx (1)
37-46: LGTM on the dynamic import pattern.Using dynamic import for
SDKExplorerIframeis appropriate here—it keeps the component out of the main bundle for regular doc pages.components/SDKExplorerIframe.tsx (2)
14-20: Initial theme detection pattern is solid.Capturing the initial theme in state prevents iframe URL changes on theme toggle, avoiding unnecessary reloads. The SSR fallback to
'dark'is reasonable.
48-51: LGTM on the hydration mismatch prevention.Returning a placeholder div until mounted avoids React hydration errors from client-only iframe rendering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @components/SDKExplorerIframe.tsx:
- Around line 37-50: The message handler in the useEffect (handleMessage)
validates event.origin but then assigns event.data.path directly to
window.location.href; validate the path before navigating by ensuring
event.data.path is either a same-origin URL or a safe relative path: reject
schemes like "javascript:" or any absolute URL whose origin !==
window.location.origin, or require path startsWith('/') and does not contain
protocol characters; if valid, use window.location.assign(path) (or construct a
new URL(path, window.location.href) and compare origins) before navigating.
Ensure checks reference the existing symbols: useEffect, handleMessage,
EXPLORER_URL, event.data.path, and window.location.href/window.location.assign.
🧹 Nitpick comments (1)
components/SDKExplorerIframe.tsx (1)
57-66: Good UX with fade-in transition and accessibility attributes.One optional consideration: the iframe lacks a
sandboxattribute, giving it full capabilities. Since this is a trusted first-party domain, this is likely intentional. If the explorer doesn't need features like form submission or popups, addingsandbox="allow-scripts allow-same-origin"could provide defense-in-depth.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/(docs)/[[...slug]]/page.tsxapp/global.csscomponents/SDKExplorerIframe.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/global.css
- app/(docs)/[[...slug]]/page.tsx
🔇 Additional comments (4)
components/SDKExplorerIframe.tsx (4)
1-6: LGTM!Good use of the
'use client'directive and clean constant extraction for the explorer URL.
8-20: LGTM!The initial theme capture via lazy
useStatewith SSR guard is a solid pattern to avoid hydration mismatches while preserving the initial theme for the iframe URL.
27-35: Previous feedback addressed: targetOrigin now restricted toEXPLORER_URL.Minor consideration:
postMessagemay fire before the iframe finishes loading, so the initial theme message could be missed. The iframe's?theme=query param handles the initial state, so this is acceptable. However, if the user changes theme rapidly before iframe load, those intermediate messages are lost (likely harmless since the next change will sync correctly).
52-55: LGTM!Good hydration guard with a styled placeholder that maintains layout consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/(docs)/[[...slug]]/page.tsx (1)
110-112: Include the root path ingenerateStaticParams()for static generation of the SDK Explorer.For optional catch-all routes (
[[...slug]]),generateParams()typically generates params only for content pages and does not include the root path. The SDK Explorer page (at/withslug: undefined) should be explicitly included in static params. Add:export async function generateStaticParams() { return [ ...source.generateParams(), { slug: [] }, ]; }This ensures the root path is statically generated rather than dynamically rendered on each request.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(docs)/[[...slug]]/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/(docs)/[[...slug]]/page.tsx (2)
components/SDKExplorerIframe.tsx (1)
SDKExplorerIframe(8-77)lib/source.ts (1)
source(5-8)
🔇 Additional comments (4)
app/(docs)/[[...slug]]/page.tsx (4)
14-14: LGTM on imports.The new
SDKExplorerIframeimport at line 14 correctly follows the@/components/...alias pattern used by surrounding imports (lines 11-19). TheCodeFromFilesimport at line 21 follows the relative path pattern already used by adjacent imports (lines 22-25).Consider standardizing all component imports to use the
@/alias for consistency across the file, but this is a pre-existing style issue and not introduced by this PR.Also applies to: 21-25
32-38: LGTM on SDK Explorer routing.The conditional correctly handles the root path case (
params.slug === undefined), rendering the SDK Explorer with:
- Full-width layout via
fullprop- Footer disabled via
footer={{ enabled: false }}This aligns with the PR objectives and efficiently short-circuits before the
source.getPage()call.
40-41: LGTM on page retrieval repositioning.Moving
source.getPage()after the undefined slug guard is correct. TypeScript's control flow analysis narrowsparams.slugtostring[]at this point, and thenotFound()fallback appropriately handles missing pages.
118-124: LGTM on generateMetadata update.The metadata guard mirrors the page rendering logic correctly, providing appropriate SEO metadata for the SDK Explorer route. The title format (
SDK Explorer — Agentuity Docs) is consistent with the existing pattern used for other pages.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
a2d4c48 to
8573082
Compare
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.