Skip to content

Conversation

elibosley
Copy link
Member

@elibosley elibosley commented Sep 22, 2025

Before Submitting This PR, Please Ensure You Have Completed The Following:

  1. Are internal links to wiki documents using relative file links?
  2. Are all new documentation files lowercase, with dash separated names (ex. unraid-os.mdx)?
  3. Are all assets (images, etc), located in an assets/ subfolder next to the .md/mdx files?
  4. Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  5. Is the build succeeding?

Summary by CodeRabbit

  • New Features
    • Full iframe embedding support with URL-driven configuration for embed, theme, entry, and sidebar; host messaging for readiness, theme, and navigation; in-iframe back navigation and persisted iframe state.
  • Style
    • Auto-hide sidebar/TOC when sidebar disabled, expand content to full width, and add a fixed back-button UI.
  • Documentation
    • Comprehensive embedding guide with parameter usage, session keys, example URL builder, host integration flow, and demo test page.
  • Tests
    • New tests covering embed messaging, message handling, and legacy compatibility.

- Introduced a new documentation file for embedding Unraid Docs in iframe-driven experiences, detailing required and optional query parameters, session storage keys, and example URL builders.
- Implemented a `normalizePath` utility function to ensure consistent handling of paths in iframe navigation.
Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

📝 Walkthrough

Walkthrough

Adds iframe embedding support with a structured postMessage Host Messaging API and URL/session-driven configuration for theme, sidebar, and entry navigation; introduces SSR-safe iframe utilities, a public useIframe hook, updated layout components (navigation, theme sync, sidebar visibility), CSS tweaks, a rewritten iframe test harness, and embedding docs.

Changes

Cohort / File(s) Summary of Changes
Embedding Docs Guide
docs/embedding.md
New guide documenting iframe embedding, query params (embed, theme, entry, sidebar), session-storage keys, a URL builder, host messaging contract (postMessage) with message types and legacy compatibility, and an example host integration.
Iframe Test Harness
iframe-test.html
Reworked test page: unified URL-parameter-driven iframe configuration (buildIframeUrl, applyIframeConfiguration), Show sidebar control, removal of per-iframe readiness/timeouts, broadcast-on-apply theme postMessage, and real-time reconfiguration.
Iframe Utilities
src/utils/iframeConstants.ts
New SSR-safe constants and helpers: query/storage keys, SupportedTheme type, parseBooleanFlag, normalizeTheme/normalizePath, and resilient read/write/clear session helpers.
Embed Messaging API
src/utils/embedMessaging.ts, src/utils/__tests__/embedMessaging.test.ts
New embed messaging module: typed message shapes (READY, THEME_CHANGE, NAVIGATION, SET_THEME), postEmbedMessage, subscribeToEmbedMessages, legacy compatibility helpers, factories for messages, and tests verifying posting/subscription and legacy mapping.
Iframe Detection Hook
src/hooks/useIframe.ts
Exported useIframe hook: SSR-guarded detection via query param and window.self !== window.top, persists iframe flag to session storage.
Layout: Iframe-aware Behavior
src/theme/Layout/IframeNavigation.tsx, src/theme/Layout/ThemeSync.tsx, src/theme/Layout/index.tsx
- IframeNavigation: compute/persist entryPath (query/storage/fallback), render back Link when appropriate, send legacy + structured navigation messages; return type -> ReactElement | null.
- ThemeSync: initialize/apply theme from query or session, persist theme locally, subscribe to embed SET_THEME messages and emit READY/THEME_CHANGE via postEmbedMessage and legacy messages; return type -> ReactElement | null.
- Layout index: track sidebar visibility from query/storage, set data attributes data-iframe and data-iframe-sidebar.
CSS for Iframe Modes
src/css/custom.css
Conditional hiding/layout when iframe sidebar is not visible (full-width main content and adjusted padding), fixed-position iframe back-button styles, and integration with existing iframe element hiding rules.
Release Notes Formatting
docs/unraid-os/release-notes/7.2.0.md
Formatting escapes applied to underscores; no behavioral changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Host
  participant Iframe as Docs Iframe
  participant Utils as iframeConstants/embedMessaging
  participant Layout as ThemeSync / IframeNavigation

  Host->>Iframe: Load URL with ?embed=1&theme=...&entry=...&sidebar=...
  Iframe->>Utils: Read query params and session storage (SSR-guarded)
  Iframe->>Layout: Init ThemeSync & LayoutWrapper
  Layout->>Utils: Determine sidebar visibility and entryPath, persist to session
  Layout->>Layout: Apply theme from query/session (setColorMode)
  Layout->>Utils: postEmbedMessage(READY or THEME_CHANGE) and sendLegacyThemeMessage(...)
  Note right of Host: Host may subscribe to iframe messages and send SET_THEME
  Host->>Iframe: postMessage(SET_THEME)
  Iframe->>Layout: subscribeToEmbedMessages → update theme & persist
Loading
sequenceDiagram
  autonumber
  participant User
  participant Iframe as Docs Iframe
  participant Router as Client Router
  participant Layout as IframeNavigation

  User->>Iframe: Click internal doc link
  Iframe->>Router: Navigate to new path
  Iframe->>Layout: Compare currentPath vs entryPath
  alt at entryPath
    Layout-->>User: No back button
  else deeper path
    Layout-->>User: Show back button
    User->>Layout: Click back
    Layout->>Router: Navigate to entryPath
    Layout->>Host: Optionally post navigation message
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

A rabbit hops through frames and themes,
I stitch the queries into seamless streams.
Sidebar hides, the back button gleams,
Messages travel—old and new schemes.
Docs snug in iframes, dreams of bright beams. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description only contains the repository's submitter checklist template with items left unchecked and does not provide a prose summary of the changes, rationale, testing or CI/build status, so it is incomplete for reviewers. Please update the PR description to include a short summary of what changed and why, list the major files or modules affected, provide testing or verification steps and the CI/build status, and mark or explain the checklist items so reviewers have the necessary context to evaluate the change.
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "feat: use query params for embedded views" is concise and accurately captures the primary intent of the changeset (shifting embedded/iframe behavior to be driven by query parameters), making it clear and relevant for reviewers scanning history.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/query-param-iframe-nav

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

github-actions bot commented Sep 22, 2025

PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-09-22 16:32 UTC

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (13)
src/utils/iframeConstants.ts (2)

11-21: Broaden accepted “true” values for robustness.

Accept common truthy tokens like "on" and "y". Keeps parsing tolerant to CMS/config toggles.

Apply:

-const BOOLEAN_TRUE_VALUES = new Set(["1", "true", "yes"]);
+const BOOLEAN_TRUE_VALUES = new Set(["1", "true", "yes", "on", "y"]);

46-51: Avoid hard-coding origin in path normalization.

Prefer the runtime origin when available; also the || "/" fallback is unreachable with a non-null template string.

-  try {
-    const parsed = new URL(trimmed, "https://docs.unraid.net");
-    return `${parsed.pathname}${parsed.search}${parsed.hash}` || "/";
+  try {
+    const base =
+      typeof window !== "undefined" && window.location?.origin
+        ? window.location.origin
+        : "https://docs.unraid.net";
+    const parsed = new URL(trimmed, base);
+    return `${parsed.pathname}${parsed.search}${parsed.hash}`;
   } catch (error) {
     return trimmed.startsWith("/") ? trimmed : `/${trimmed}`;
   }
docs/embedding.md (2)

3-3: Document legacy postMessage for hosts still listening.

You still emit a navigation postMessage for compatibility; mention it briefly so hosts know it remains available and uses * intentionally.

 Use the following guidance when loading the Unraid documentation inside an iframe-driven experience. The options described here help control UI chrome, theme, and navigation behavior without relying on `postMessage` exchanges.
+Note: For legacy hosts, the docs still emit a lightweight `postMessage` on navigation (targetOrigin `*`) with `{type: "unraid-docs-navigation", pathname, search, hash, url}`. No sensitive data is included.

30-49: Normalize entry on the host side (optional).

You normalize on the docs side already; optionally show a host-side normalization to reduce surprises when passing full URLs.

 function buildDocsUrl(path, { theme, entry, sidebar } = {}) {
   const url = new URL(path, "https://docs.unraid.net");
   url.searchParams.set("embed", "1");
 
   if (theme === "light" || theme === "dark") {
     url.searchParams.set("theme", theme);
   }
 
-  if (entry) {
-    url.searchParams.set("entry", entry);
+  if (entry) {
+    // Normalize to a docs-internal path to match how the iframe resolves it
+    const e = new URL(entry, url);
+    const normalizedEntry = `${e.pathname}${e.search}${e.hash}`;
+    url.searchParams.set("entry", normalizedEntry);
   }
src/hooks/useIframe.ts (2)

26-32: Assume “framed” on cross-origin access errors.

If a browser throws while probing, treating the context as framed is the safer default.

-    const iframeFromWindow = (() => {
-      try {
-        return window.self !== window.top;
-      } catch (error) {
-        return false;
-      }
-    })();
+    const iframeFromWindow = (() => {
+      try {
+        // parent check + frameElement covers most cases
+        return window.parent !== window || !!window.frameElement;
+      } catch (error) {
+        // If probing throws (unexpected), err on the side of being framed
+        return true;
+      }
+    })();

17-25: Consider reacting to query param changes.

If hosts toggle embed during a session, the hook won’t re-evaluate. Optional: add a popstate listener or accept a dependency (e.g., location.search) to recompute.

If you want to validate this need, try toggling ?embed=0/1 in the local harness and observe the data attributes.

src/css/custom.css (1)

245-285: Respect prefers-reduced-motion for the back button.

Avoid hover lifts/animated transitions when users prefer reduced motion.

 div[data-iframe="true"] .iframe-back-button {
   position: fixed;
@@
   transition: transform 0.2s ease, box-shadow 0.2s ease;
 }
 
+@media (prefers-reduced-motion: reduce) {
+  div[data-iframe="true"] .iframe-back-button {
+    transition: none;
+  }
+  div[data-iframe="true"] .iframe-back-button:hover {
+    transform: none;
+  }
+}
src/theme/Layout/IframeNavigation.tsx (1)

24-40: Gate postMessage to real iframes; keep * target for legacy.

Emit only when actually framed to avoid no-op self-messages on standalone pages with embed=1. Keeping * aligns with prior guidance since payload is non-sensitive.

-  useEffect(() => {
-    if (!isInIframeState) {
-      return;
-    }
-
-    window.parent.postMessage(
+  useEffect(() => {
+    if (!isInIframeState) return;
+    let actuallyFramed = false;
+    try {
+      actuallyFramed = window.parent !== window || !!window.frameElement;
+    } catch {
+      actuallyFramed = true;
+    }
+    if (!actuallyFramed) return;
+
+    window.parent.postMessage(
       {
         type: "unraid-docs-navigation",
         pathname: location.pathname,
         search: location.search,
         hash: location.hash,
         url: window.location.href,
       },
       "*",
     );
   }, [location, isInIframeState]);

Please confirm legacy hosts still expect this payload shape; if not, we can extend it with a version field.

src/theme/Layout/ThemeSync.tsx (2)

2-2: Avoid extra renders and initial double writes by using a ref for lastPersistedTheme.

State here causes an extra rerender and can lead to two writes on first mount inside an iframe (both effects race before state updates settle). A ref eliminates both issues.

Apply:

-import React, { useEffect, useState } from "react";
+import React, { useEffect, useRef } from "react";
@@
-  const [lastPersistedTheme, setLastPersistedTheme] = useState<string | null>(null);
+  const lastPersistedThemeRef = useRef<string | null>(null);
@@
-    if (themeToApply && themeToApply !== colorMode) {
+    if (themeToApply && themeToApply !== colorMode) {
       setColorMode(themeToApply);
     }
@@
-    if (themeToApply) {
-      if (themeToApply !== lastPersistedTheme) {
-        writeSessionValue(THEME_STORAGE_KEY, themeToApply);
-        setLastPersistedTheme(themeToApply);
-      }
+    if (themeToApply) {
+      if (themeToApply !== lastPersistedThemeRef.current) {
+        writeSessionValue(THEME_STORAGE_KEY, themeToApply);
+        lastPersistedThemeRef.current = themeToApply;
+      }
       return;
     }
@@
-    if (colorMode !== lastPersistedTheme) {
-      writeSessionValue(THEME_STORAGE_KEY, colorMode);
-      setLastPersistedTheme(colorMode);
+    if (colorMode !== lastPersistedThemeRef.current) {
+      writeSessionValue(THEME_STORAGE_KEY, colorMode);
+      lastPersistedThemeRef.current = colorMode;
     }
-  }, [colorMode, isInIframeState, lastPersistedTheme, setColorMode]);
+  }, [colorMode, isInIframeState, setColorMode]);
@@
-    if (colorMode === lastPersistedTheme) {
+    if (colorMode === lastPersistedThemeRef.current) {
       return;
     }
@@
-    writeSessionValue(THEME_STORAGE_KEY, colorMode);
-    setLastPersistedTheme(colorMode);
-  }, [colorMode, isInIframeState, lastPersistedTheme]);
+    writeSessionValue(THEME_STORAGE_KEY, colorMode);
+    lastPersistedThemeRef.current = colorMode;
+  }, [colorMode, isInIframeState]);

Also applies to: 18-18, 32-34, 36-47, 50-62


27-34: Minor: potential first-paint theme flash in iframe.

Theme applies after mount; users may see a flash of the default theme. Consider a small pre-hydration inline script (in Layout/head) that reads ?theme= and sets data-theme to reduce FOUC.

iframe-test.html (3)

136-137: Status text still implies postMessage.

Update copy to match param-driven flow.

Apply:

-                <span id="theme-status" class="theme-status status-waiting">Waiting for theme sync...</span>
+                <span id="theme-status" class="theme-status status-waiting">Ready — applies theme via URL params</span>

202-219: Make buildIframeUrl robust to missing options; explicitly set sidebar off.

Default options avoids accidental undefined access; explicitly send sidebar=0 for clarity.

Apply:

-            function buildIframeUrl(baseUrl, path, options) {
+            function buildIframeUrl(baseUrl, path, options = {}) {
               const url = new URL(path, baseUrl);
               url.searchParams.set('embed', '1');
@@
-                if (options.sidebar) {
-                    url.searchParams.set('sidebar', '1');
-                }
+                if (options.sidebar) {
+                    url.searchParams.set('sidebar', '1');
+                } else {
+                    url.searchParams.set('sidebar', '0');
+                }

151-157: Optional: sandbox the iframes during local testing.

If it doesn’t break functionality, add a permissive sandbox to reduce risk while testing.

Apply:

-                    <iframe id="test-iframe" src="http://localhost:3000/" title="Documentation iframe (localhost)"></iframe>
+                    <iframe id="test-iframe" src="http://localhost:3000/" title="Documentation iframe (localhost)" sandbox="allow-same-origin allow-scripts allow-popups"></iframe>
@@
-                    <iframe id="cloudflare-iframe" src="https://docs.unraid.net/" title="Documentation iframe (Cloudflare)"></iframe>
+                    <iframe id="cloudflare-iframe" src="https://docs.unraid.net/" title="Documentation iframe (Cloudflare)" sandbox="allow-same-origin allow-scripts allow-popups"></iframe>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ecaf92e and 807a033.

📒 Files selected for processing (8)
  • docs/embedding.md (1 hunks)
  • iframe-test.html (2 hunks)
  • src/css/custom.css (2 hunks)
  • src/hooks/useIframe.ts (2 hunks)
  • src/theme/Layout/IframeNavigation.tsx (1 hunks)
  • src/theme/Layout/ThemeSync.tsx (1 hunks)
  • src/theme/Layout/index.tsx (1 hunks)
  • src/utils/iframeConstants.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-21T16:22:52.935Z
Learnt from: elibosley
PR: unraid/docs#278
File: iframe-test.html:127-133
Timestamp: 2025-05-21T16:22:52.935Z
Learning: In the Unraid docs iframe implementation, using '*' as targetOrigin for postMessage is necessary when communicating theme preferences between the parent window and iframe, as the documentation needs to function across multiple Unraid server instances with different origins. This is acceptable since the theme information being transmitted is not sensitive.

Applied to files:

  • docs/embedding.md
🧬 Code graph analysis (4)
src/hooks/useIframe.ts (1)
src/utils/iframeConstants.ts (5)
  • IFRAME_QUERY_PARAM (1-1)
  • parseBooleanFlag (15-21)
  • writeSessionValue (66-76)
  • IFRAME_STORAGE_KEY (6-6)
  • clearSessionValue (78-88)
src/theme/Layout/index.tsx (2)
src/hooks/useIframe.ts (1)
  • useIframe (14-45)
src/utils/iframeConstants.ts (6)
  • clearSessionValue (78-88)
  • SIDEBAR_STORAGE_KEY (9-9)
  • SIDEBAR_QUERY_PARAM (4-4)
  • parseBooleanFlag (15-21)
  • writeSessionValue (66-76)
  • readSessionValue (54-64)
src/theme/Layout/ThemeSync.tsx (2)
src/hooks/useIframe.ts (1)
  • useIframe (14-45)
src/utils/iframeConstants.ts (5)
  • normalizeTheme (23-34)
  • THEME_QUERY_PARAM (2-2)
  • readSessionValue (54-64)
  • THEME_STORAGE_KEY (7-7)
  • writeSessionValue (66-76)
src/theme/Layout/IframeNavigation.tsx (2)
src/hooks/useIframe.ts (1)
  • useIframe (14-45)
src/utils/iframeConstants.ts (5)
  • ENTRY_QUERY_PARAM (3-3)
  • normalizePath (36-52)
  • writeSessionValue (66-76)
  • ENTRY_STORAGE_KEY (8-8)
  • readSessionValue (54-64)
⏰ 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). (1)
  • GitHub Check: deploy-preview
🔇 Additional comments (10)
src/utils/iframeConstants.ts (2)

1-10: Good centralization of query/storage keys (SSR-safe).

Constants and the separation of query vs. session keys look consistent and future-proof.


54-88: Harden sessionStorage access against Safari/private-mode quirks.

Accessing window.sessionStorage can throw; probe it inside try/catch and use a local reference. Location: src/utils/iframeConstants.ts (readSessionValue, writeSessionValue, clearSessionValue).

 export function readSessionValue(key: string): string | null {
-  if (typeof window === "undefined" || !window.sessionStorage) {
-    return null;
-  }
-
-  try {
-    return window.sessionStorage.getItem(key);
+  if (typeof window === "undefined") return null;
+  try {
+    const ss = window.sessionStorage;
+    return ss ? ss.getItem(key) : null;
   } catch (error) {
     return null;
   }
 }
 
 export function writeSessionValue(key: string, value: string): void {
-  if (typeof window === "undefined" || !window.sessionStorage) {
-    return;
-  }
-
-  try {
-    window.sessionStorage.setItem(key, value);
+  if (typeof window === "undefined") return;
+  try {
+    const ss = window.sessionStorage;
+    if (ss) ss.setItem(key, value);
   } catch (error) {
     // Failing to write session state should not break the docs experience.
   }
 }
 
 export function clearSessionValue(key: string): void {
-  if (typeof window === "undefined" || !window.sessionStorage) {
-    return;
-  }
-
-  try {
-    window.sessionStorage.removeItem(key);
+  if (typeof window === "undefined") return;
+  try {
+    const ss = window.sessionStorage;
+    if (ss) ss.removeItem(key);
   } catch (error) {
     // Removing session state is a best-effort operation.
   }
 }

Verification note: the provided repo scan failed with "unrecognized file type: tsx" — re-run the key-search locally to confirm no hard-coded storage keys remain.

docs/embedding.md (1)

1-14: Clear, actionable embedding guide.

Required/optional params and session keys are called out succinctly. Good defaults and examples.

src/css/custom.css (1)

230-244: Scoped sidebar/TOC hiding is correct.

Selectors target only iframe mode and only when sidebar isn’t explicitly visible—good guardrails.

src/theme/Layout/index.tsx (2)

21-50: State sync logic for sidebar visibility looks sound.

Query-param precedence over session, reset when not framed, and location-aware updates are correct.


52-58: Data attributes are well-structured for CSS gating.

Explicit ‘visible/hidden’ values aid debugging and CSS specificity. Nice.

src/theme/Layout/IframeNavigation.tsx (2)

42-73: Entry-path resolution is resilient and idempotent.

Query > storage > current route fallback with normalization is correct for SPA navigation.


91-104: Back button UX is accessible and minimal.

Internal Link, ARIA label, and SVG sizing are good. No changes needed.

src/theme/Layout/ThemeSync.tsx (1)

16-16: Return type change to ReactElement | null is fine.

Matches usage (no render output). No concerns.

iframe-test.html (1)

221-241: Nice consolidation; apply config in one place.

URL composition/readability is good and mirrors the new iframe contract.

- Added a new messaging API for embedding Unraid Docs in iframes, allowing dynamic theme updates and navigation events.
- Introduced structured message types for better communication between the iframe and parent window.
- Updated documentation to reflect the new messaging capabilities and legacy compatibility.
- Enhanced existing components to utilize the new messaging system for theme synchronization and navigation handling.
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (9)
docs/embedding.md (3)

31-49: Example URL builder: normalize absolute URLs to force docs origin

If callers pass a full absolute URL (possibly cross-origin), the current builder will preserve that origin. Normalize to the docs origin to avoid surprises.

Apply this diff:

-function buildDocsUrl(path, { theme, entry, sidebar } = {}) {
-  const url = new URL(path, "https://docs.unraid.net");
+function buildDocsUrl(path, { theme, entry, sidebar } = {}) {
+  const DOCS_ORIGIN = "https://docs.unraid.net";
+  // Normalize any absolute URL to a path on docs origin.
+  const input = (() => {
+    try {
+      const parsed = new URL(path, DOCS_ORIGIN);
+      return `${parsed.pathname}${parsed.search}${parsed.hash}`;
+    } catch {
+      return typeof path === "string" && path ? path : "/";
+    }
+  })();
+  const url = new URL(input, DOCS_ORIGIN);
   url.searchParams.set("embed", "1");

78-101: Host listener: optionally verify the sender frame in addition to data.source

To reduce noise/spoofing, also check event.source matches your iframe(s) when you can reference them.

Example (illustrative):

const frames = new Set([docsIframe.contentWindow, cfIframe.contentWindow]);

window.addEventListener('message', (event) => {
  const { data, source } = event;
  if (!data || data.source !== 'unraid-docs') return;
  if (source && !frames.has(source)) return; // optional hardening

  // handle...
});

105-108: Legacy events: add a deprecation timeline

You note legacy events “will be removed in a future release.” Please specify a target version/date to aid host migration planning.

iframe-test.html (2)

279-304: Gate on message sender to avoid handling stray window messages

Filter by event.source so only the two known iframes are accepted.

Apply this diff:

 window.addEventListener('message', function(event) {
-                const data = event.data;
+                const data = event.data;
+                const sourceWin = event.source;
                 if (!data || typeof data !== 'object') {
                     return;
                 }
 
+                // Only accept messages from our test iframes
+                if (sourceWin !== iframe.contentWindow && sourceWin !== cloudflareIframe.contentWindow) {
+                    return;
+                }
+
                 if (data.source === EMBED_MESSAGE_SOURCE) {
                   if (data.type === EMBED_READY) {
                     updateThemeStatus(`Iframe reported ready with ${data.theme} theme`);
                     return;
                   }

306-311: Avoid double theme-apply (broadcast + reload) to reduce flicker

Right now you post set-theme and then reload both iframes, which can race and flicker. If the goal is to demo message-based theming, you can skip the reload on theme-only changes.

Apply this diff:

 applyThemeButton.addEventListener('click', function() {
   currentTheme = themeMode.value;
   broadcastThemeUpdate(currentTheme);
-  applyIframeConfiguration();
 });
src/theme/Layout/IframeNavigation.tsx (2)

31-47: Double‑emitting navigation (new contract + undocumented legacy)

You send both the structured unraid-docs:navigation and an extra "unraid-docs-navigation" legacy event that isn’t documented in docs/embedding.md. Consider removing or documenting it to avoid ambiguity.

Apply this diff to rely solely on the structured message:

     postEmbedMessage(createNavigationMessage(payload));
-
-    window.parent.postMessage(
-      {
-        type: "unraid-docs-navigation",
-        ...payload,
-      },
-      "*",
-    );

82-88: Avoid window access for currentPath; derive from router location

This is a bit more SSR‑friendly and consistent with useLocation.

Apply this diff:

-  const currentPath = useMemo(() => {
-    if (typeof window === "undefined") {
-      return null;
-    }
-
-    return `${window.location.pathname}${window.location.search}${window.location.hash}`;
-  }, [location]);
+  const currentPath = useMemo(
+    () => `${location.pathname}${location.search}${location.hash}`,
+    [location],
+  );
src/utils/embedMessaging.ts (2)

105-139: Harden inbound messages: ensure they come from the parent window

Add a sender check so the iframe ignores messages not from its parent.

Apply this diff:

 export function subscribeToEmbedMessages(handler: (message: EmbedInboundMessage) => void): () => void {
   if (!isWindowAvailable()) {
     return () => undefined;
   }

   const listener = (event: MessageEvent): void => {
+    // Only accept messages from our parent window when in an iframe.
+    try {
+      if (isInIframe() && event.source && event.source !== window.parent) {
+        return;
+      }
+    } catch {
+      // If we cannot verify, fall through to data-based validation below.
+    }
+
     if (isEmbedSetThemeMessage(event.data)) {
       handler({
         ...event.data,
         theme: normalizeTheme(event.data.theme)!,
       });
       return;
     }

97-103: Optional: provide origin‑aware send helpers for tighter hosts

Keep existing APIs but add variants that accept a targetOrigin for deployments that know the parent origin.

Apply this addition:

 export function postEmbedMessage(message: EmbedOutboundMessage): void {
   if (!isInIframe()) {
     return;
   }

   window.parent.postMessage(message, "*");
 }
+
+export function postEmbedMessageToOrigin(message: EmbedOutboundMessage, targetOrigin: string): void {
+  if (!isInIframe()) {
+    return;
+  }
+  window.parent.postMessage(message, targetOrigin);
+}
@@
 export function sendLegacyThemeMessage(type: LegacyThemeMessageType, theme: SupportedTheme): void {
   if (!isInIframe()) {
     return;
   }

   window.parent.postMessage({ type, theme }, "*");
 }
+
+export function sendLegacyThemeMessageToOrigin(
+  type: LegacyThemeMessageType,
+  theme: SupportedTheme,
+  targetOrigin: string,
+): void {
+  if (!isInIframe()) {
+    return;
+  }
+  window.parent.postMessage({ type, theme }, targetOrigin);
+}

Also applies to: 141-147

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 807a033 and 258823a.

📒 Files selected for processing (7)
  • docs/embedding.md (1 hunks)
  • docs/unraid-os/release-notes/7.2.0.md (5 hunks)
  • iframe-test.html (2 hunks)
  • src/theme/Layout/IframeNavigation.tsx (1 hunks)
  • src/theme/Layout/ThemeSync.tsx (1 hunks)
  • src/utils/__tests__/embedMessaging.test.ts (1 hunks)
  • src/utils/embedMessaging.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • docs/unraid-os/release-notes/7.2.0.md
  • src/utils/tests/embedMessaging.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-21T16:22:52.935Z
Learnt from: elibosley
PR: unraid/docs#278
File: iframe-test.html:127-133
Timestamp: 2025-05-21T16:22:52.935Z
Learning: In the Unraid docs iframe implementation, using '*' as targetOrigin for postMessage is necessary when communicating theme preferences between the parent window and iframe, as the documentation needs to function across multiple Unraid server instances with different origins. This is acceptable since the theme information being transmitted is not sensitive.

Applied to files:

  • docs/embedding.md
  • iframe-test.html
  • src/utils/embedMessaging.ts
🧬 Code graph analysis (3)
src/theme/Layout/IframeNavigation.tsx (3)
src/hooks/useIframe.ts (1)
  • useIframe (14-45)
src/utils/embedMessaging.ts (2)
  • postEmbedMessage (97-103)
  • createNavigationMessage (165-171)
src/utils/iframeConstants.ts (5)
  • ENTRY_QUERY_PARAM (3-3)
  • normalizePath (36-52)
  • writeSessionValue (66-76)
  • ENTRY_STORAGE_KEY (8-8)
  • readSessionValue (54-64)
src/utils/embedMessaging.ts (1)
src/utils/iframeConstants.ts (2)
  • SupportedTheme (13-13)
  • normalizeTheme (23-34)
src/theme/Layout/ThemeSync.tsx (3)
src/hooks/useIframe.ts (1)
  • useIframe (14-45)
src/utils/iframeConstants.ts (6)
  • SupportedTheme (13-13)
  • normalizeTheme (23-34)
  • THEME_QUERY_PARAM (2-2)
  • readSessionValue (54-64)
  • THEME_STORAGE_KEY (7-7)
  • writeSessionValue (66-76)
src/utils/embedMessaging.ts (6)
  • subscribeToEmbedMessages (105-139)
  • EMBED_MESSAGE_TYPES (5-10)
  • postEmbedMessage (97-103)
  • createReadyMessage (149-155)
  • sendLegacyThemeMessage (141-147)
  • createThemeChangeMessage (157-163)
⏰ 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). (1)
  • GitHub Check: deploy-preview
🔇 Additional comments (3)
iframe-test.html (1)

254-276: Note on targetOrigin '*' is acceptable here

Using '*' is intentional for this harness given cross‑origin docs instances and non‑sensitive payloads (theme). No change requested.

If you want to demo a locked‑down variant as well, I can add a toggle to use a custom targetOrigin and origin checks.

src/theme/Layout/ThemeSync.tsx (1)

101-124: Structured + legacy theme signals look good; resolves prior readiness concerns

You emit unraid-docs:ready and unraid-docs:theme-change and also legacy theme-ready/theme-changed. This addresses the earlier concern about dropped readiness/theme updates.

Please confirm no remaining consumers depend on any other legacy types beyond these two.

src/utils/embedMessaging.ts (1)

1-171: Messaging contract, types, and legacy shims are solid

Clear, typed contract; guards for SSR; normalization is correct. No changes requested.

@elibosley elibosley merged commit 233902a into main Sep 22, 2025
3 checks passed
@elibosley elibosley deleted the feat/query-param-iframe-nav branch September 22, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant