refactor: reuse ChangelogModal in HeaderOsVersion component#1607
refactor: reuse ChangelogModal in HeaderOsVersion component#1607
ChangelogModal in HeaderOsVersion component#1607Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdates version metadata, relaxes a GraphQL field’s nullability, and refactors the OS changelog modal to support both prop-driven and store-driven modes. Header component now computes a release object and uses the new ChangelogModal API. No new exported API beyond the modal’s optional release prop and close event. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Header as HeaderOsVersion.ce.vue
participant Modal as UpdateOs/ChangelogModal (prop mode)
participant Docs as Docs Iframe
User->>Header: Click "Release notes"
Header->>Modal: Open with props { open: true, release }
Modal->>Modal: Compute isPropMode / currentRelease
Modal->>Docs: Load changelogPretty (iframe src)
Modal-->>User: Displays changelog for currentRelease
User->>Modal: Close
Modal-->>Header: emit("close")
Header->>Header: set showOsReleaseNotesModal = false
sequenceDiagram
autonumber
actor User
participant UI as Update Flow (Store mode)
participant Store as Update Store
participant Modal as UpdateOs/ChangelogModal (store mode)
participant Docs as Docs Iframe
User->>UI: Start OS update
UI->>Store: setReleaseForUpdate(release), setChangelogModalVisible(true)
Store-->>Modal: releaseForUpdate, changelogModalVisible
Modal->>Modal: Compute isPropMode=false, currentRelease from store
Modal->>Docs: Load changelogPretty (iframe src), send theme
User->>Modal: Continue / Extend License
Modal->>Store: Actions using currentRelease.sha256
User->>Modal: Close
Modal->>Store: setChangelogModalVisible(false), setReleaseForUpdate(null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @pujitm's task —— View job Code Review: Critical Issues Analysis
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/components/UpdateOs/ChangelogModal.vue (2)
114-124: postMessage target origin should be restrictedUsing
'*'broadcasts to any origin. Since docs origin is known at runtime, compute it and target that origin for safer messaging.Apply:
+const docsOrigin = computed(() => { + try { + return docsChangelogUrl.value ? new URL(docsChangelogUrl.value).origin : null; + } catch { + return null; + } +}); + const sendThemeToIframe = () => { if (iframeRef.value && iframeRef.value.contentWindow) { try { const message = { type: 'theme-update', theme: darkMode.value ? 'dark' : 'light' }; - iframeRef.value.contentWindow.postMessage(message, '*'); + iframeRef.value.contentWindow.postMessage(message, docsOrigin.value ?? 'null'); } catch (error) { console.error('Failed to send theme to iframe:', error); } } };Note: The
'null'target only delivers tonullorigins (sandboxed iframes). Prefer a concrete origin when available.
186-186: Removeallow-top-navigationfrom sandbox (security)
allow-top-navigationlets the embedded docs navigate the top-level page. This is unnecessary and increases risk if content is compromised. Prefer removing it; if you need user-initiated popups, useallow-popupsinstead.Apply:
- sandbox="allow-scripts allow-same-origin allow-top-navigation" + sandbox="allow-scripts allow-same-origin"If a later requirement needs external links from within the iframe to open, consider
allow-popupsrather than top navigation.
🧹 Nitpick comments (9)
web/components/UpdateOs/ChangelogModal.vue (6)
26-36: Prop-driven mode and release shape — consider centralizing the Release typeThe prop shape looks good. To reduce duplication and ensure consistency with store types, consider exporting a shared
Releaseinterface from a types module and reusing it here.
69-75: Close behavior in store mode — verify it also hides the modal
handleClosesetssetReleaseForUpdate(null)in store mode. IfchangelogModalVisibleis not derived fromreleaseForUpdate, the modal could remain open. Verify store semantics and, if needed, also flip the open flag.Option if needed:
} else { - setReleaseForUpdate(null); + setReleaseForUpdate(null); + updateOsStore.setModalOpen?.(false); }
125-136: Comment contradicts behaviorThe comment says “Attach event listener right away instead of waiting for mount,” but the code attaches in
onMounted. Update the comment to avoid confusion.Apply:
-// Attach event listener right away instead of waiting for mount +// Attach event listener on mount
156-160: Reset iframe when reopening modal (nice to have)If users navigate within the docs, reopening the modal could retain the last navigated page. If you want a fresh state per open, watch
modalVisibleand reset when it becomes true.Apply:
watch(darkMode, () => { // The iframe will only pick up the message if it has sent theme-ready sendThemeToIframe(); }); + +watch(modalVisible, (isOpen) => { + if (isOpen) { + revertToInitialChangelog(); + } +});
216-223: Localize button aria-labels
aria-label="Back to Changelog"should be localized like other UI strings.Apply:
- aria-label="Back to Changelog" + :aria-label="t('Back to Changelog')"
225-233: External link button — prefer parent-provided label or i18nYou translate
viewDocsLabelviathere. To decouple this component from i18n, consider makingviewDocsLabela pre-localized string from the parent. If keeping as a key, align the prop name toviewDocsLabelKeyfor clarity and localizearia-labeltoo.Apply minimal i18n for aria label:
- aria-label="View on Docs" + :aria-label="t('View on Docs')"web/components/HeaderOsVersion.ce.vue (3)
39-54: Release object mapping for current version is clearThe computed
currentVersionReleaseis well-formed. Minor: considerdate: nullinstead ofundefinedto match optional fields typically serialized in JSON, but not required.
163-169: Guard menu item when OS version is unknownWhen
displayOsVersionis null, clicking “View OS Release Notes” opens an empty modal (release is null; modal won’t render, but the flag flips). Consider disabling the item in that case.Apply:
- <DropdownMenuItem @click="showOsReleaseNotesModal = true"> + <DropdownMenuItem :disabled="!displayOsVersion" @click="displayOsVersion && (showOsReleaseNotesModal = true)">
204-210: Pass a pre-localized docs label or a stable i18n keyYou pass
view-docs-label="Open in new tab", whichChangelogModaltreats as an i18n key. If “Open in new tab” is not defined in locales, it will display the raw text (acceptable), but consider passing an explicit key (e.g.,view-docs-label="Open in new tab" // key present in locales) or pre-localizing it here and letting the modal accept a plain string to reduce coupling.Alternative (pre-localize here):
- <ChangelogModal + <ChangelogModal :open="showOsReleaseNotesModal" :release="currentVersionRelease" - view-docs-label="Open in new tab" + :view-docs-label="t('Open in new tab')" :t="t" @close="showOsReleaseNotesModal = false" />This change would require the modal to treat
viewDocsLabelas “final text,” not a key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
api/dev/configs/api.json(1 hunks)api/generated-schema.graphql(1 hunks)web/components/HeaderOsVersion.ce.vue(3 hunks)web/components/UpdateOs/ChangelogModal.vue(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/components/UpdateOs/ChangelogModal.vueweb/components/HeaderOsVersion.ce.vue
**/components/**/*.vue
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
Some Vue files may require explicit imports like ref or computed due to Nuxt auto-imports not applying in tests
Files:
web/components/UpdateOs/ChangelogModal.vueweb/components/HeaderOsVersion.ce.vue
🧠 Learnings (1)
📚 Learning: 2025-02-06T17:24:58.784Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/components/UserProfile/DropdownLaunchpad.vue:38-42
Timestamp: 2025-02-06T17:24:58.784Z
Learning: In the DropdownLaunchpad.vue component, the current implementation of conditional icon rendering using `h(BrandLoading, { variant: 'white' })` is a temporary fix to maintain build stability, with a proper refactoring being handled in a separate branch.
Applied to files:
web/components/UpdateOs/ChangelogModal.vueweb/components/HeaderOsVersion.ce.vue
🔇 Additional comments (8)
api/dev/configs/api.json (1)
2-2: Version 4.15.1 bump—consistency confirmedAll references to the new API version have been updated and no stale mentions of 4.14.x remain (aside from historical changelog entries). You’re good to merge.
• api/dev/configs/api.json → version “4.15.1”
• package.json in root, web/, unraid-ui/, plugin/, and api/ → “4.15.1”
• api/CHANGELOG.md → entry for [4.15.1] dated 2025-08-20
• No lingering hard-coded “4.14.*” outside of prior changelog headers
• Tests referencing other version patterns (e.g. parsePackageArg) are unaffectedapi/generated-schema.graphql (1)
1548-1548: Nullable change for InfoVersions.packages — no unsafe usages foundA grep across all .ts/.tsx/.vue files for direct
versions.packages.*access without null checks came up empty, and no .graphql/.gql operations still declarepackages: PackageVersions!. This indicates existing clients and queries won’t break on this loosened contract.Next steps:
- Confirm your resolvers explicitly return
null(notundefined) whenpackagesis absent.- (Optional) Add a brief docstring to
InfoVersions.packagesexplaining under what conditions it may be null.web/components/UpdateOs/ChangelogModal.vue (4)
44-47: Event model is clearEmitting
closein prop mode is a good API. No issues.
55-63: Dual-mode computed flags (prop vs store) are cleanThe
isPropMode,currentRelease, andmodalVisiblesplit is straightforward and readable.
193-199: RawChangelogRenderer props are consistentPassing both raw and pretty changelog variants keyed off availability is sound.
236-254: Store-mode actions are gated correctlyNice separation: renew action vs continue action only when
sha256exists. Looks good.web/components/HeaderOsVersion.ce.vue (2)
9-9: Use of getReleaseNotesUrl is appropriateGood move centralizing URL construction. Ensure
getReleaseNotesUrlaligns withallowedDocsUrlRegexused by the modal.
16-16: Switch to shared ChangelogModalReplacing the bespoke ReleaseNotesModal with the reusable
ChangelogModalaligns the UI. Good reuse.
| const docsChangelogUrl = computed(() => { | ||
| return releaseForUpdate.value?.changelogPretty ?? null; | ||
| return currentRelease.value?.changelogPretty ?? null; | ||
| }); | ||
|
|
||
| const showRawChangelog = computed<boolean>(() => { | ||
| return !docsChangelogUrl.value && !!releaseForUpdate.value?.changelog; | ||
| return !docsChangelogUrl.value && !!currentRelease.value?.changelog; | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard iframe source against untrusted URLs
docsChangelogUrl comes from currentRelease.changelogPretty. Add a defensive check before using it (you already have allowedDocsUrlRegex for postMessage). This prevents accidentally embedding an unexpected origin.
Apply:
watch(docsChangelogUrl, (newUrl) => {
- currentIframeUrl.value = newUrl;
+ currentIframeUrl.value = newUrl && allowedDocsUrlRegex.test(newUrl) ? newUrl : null;
hasNavigated.value = false;
- if (newUrl) {
- actualIframeSrc.value = newUrl;
+ if (currentIframeUrl.value) {
+ actualIframeSrc.value = currentIframeUrl.value;
} else {
actualIframeSrc.value = null;
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const docsChangelogUrl = computed(() => { | |
| return releaseForUpdate.value?.changelogPretty ?? null; | |
| return currentRelease.value?.changelogPretty ?? null; | |
| }); | |
| const showRawChangelog = computed<boolean>(() => { | |
| return !docsChangelogUrl.value && !!releaseForUpdate.value?.changelog; | |
| return !docsChangelogUrl.value && !!currentRelease.value?.changelog; | |
| }); | |
| watch(docsChangelogUrl, (newUrl) => { | |
| currentIframeUrl.value = newUrl && allowedDocsUrlRegex.test(newUrl) ? newUrl : null; | |
| hasNavigated.value = false; | |
| if (currentIframeUrl.value) { | |
| actualIframeSrc.value = currentIframeUrl.value; | |
| } else { | |
| actualIframeSrc.value = null; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In web/components/UpdateOs/ChangelogModal.vue around lines 83 to 90,
docsChangelogUrl is set directly from currentRelease.changelogPretty which can
be untrusted; validate and sanitize it before returning: parse the value as a
URL (guarding with try/catch) and only return it if it matches the existing
allowedDocsUrlRegex (and optional scheme check for https/http); otherwise return
null so the iframe src is never set to an unsafe origin.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
left a comment
elibosley
left a comment
There was a problem hiding this comment.
One minor thing but no reason to fix this.
|
|
||
| const showRawChangelog = computed<boolean>(() => { | ||
| return !docsChangelogUrl.value && !!releaseForUpdate.value?.changelog; | ||
| return !docsChangelogUrl.value && !!currentRelease.value?.changelog; |
There was a problem hiding this comment.
Should we use Boolean() instead of !!?
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/components/UpdateOs/ChangelogModal.vue (1)
129-136: Fix: iframe doesn’t load initially in prop mode
actualIframeSrcis only set inside thewatchcallback, which doesn’t run on first render when the prop is already present. Make the watcherimmediateand remove the manualonMountedassignment.onMounted(() => { // Set initial values only window.addEventListener('message', handleDocsPostMessages); - currentIframeUrl.value = docsChangelogUrl.value; });-watch(docsChangelogUrl, (newUrl) => { +watch(docsChangelogUrl, (newUrl) => { currentIframeUrl.value = newUrl; hasNavigated.value = false; if (newUrl) { actualIframeSrc.value = newUrl; } else { actualIframeSrc.value = null; } -}); +}, { immediate: true });Also applies to: 149-158
♻️ Duplicate comments (1)
web/components/UpdateOs/ChangelogModal.vue (1)
87-90: SanitizedocsChangelogUrlbefore use (XFO/bounce risk via arbitrary iframes)
currentRelease.changelogPrettyis used to drive iframe src without validation. Validate againstallowedDocsUrlRegexhere so the iframe and all consumers only see a trusted origin.-const docsChangelogUrl = computed(() => { - return currentRelease.value?.changelogPretty ?? null; -}); +const docsChangelogUrl = computed(() => { + const url = currentRelease.value?.changelogPretty ?? null; + return typeof url === 'string' && allowedDocsUrlRegex.test(url) ? url : null; +});
🧹 Nitpick comments (3)
web/components/UpdateOs/ChangelogModal.vue (3)
49-55: Avoid potential NPE ontheme.value.nameIf
theme.valueis not yet initialized, this will throw. Use optional chaining.-const isDarkMode = computed(() => { - if (theme.value.name === 'azure') { +const isDarkMode = computed(() => { + if (theme.value?.name === 'azure') { return true; } return darkMode.value; });
184-193: Tighten iframe sandbox policyPrefer
allow-top-navigation-by-user-activationoverallow-top-navigationto prevent unsolicited top-level navigations from the embedded page.<iframe v-if="actualIframeSrc" ref="iframeRef" :src="actualIframeSrc" class="w-full h-full border-0 rounded-md" - sandbox="allow-scripts allow-same-origin allow-top-navigation" + sandbox="allow-scripts allow-same-origin allow-top-navigation-by-user-activation" title="Unraid Changelog" />
129-131: Nit: comment doesn’t match behaviorThe comment says “Attach event listener right away instead of waiting for mount” but the code does it on mount. Update the comment for clarity.
-// Attach event listener right away instead of waiting for mount +// Attach event listener on mount
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
web/components/UpdateOs/ChangelogModal.vue(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/components/UpdateOs/ChangelogModal.vue
**/components/**/*.vue
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
Some Vue files may require explicit imports like ref or computed due to Nuxt auto-imports not applying in tests
Files:
web/components/UpdateOs/ChangelogModal.vue
🧠 Learnings (1)
📚 Learning: 2025-02-06T17:24:58.784Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/components/UserProfile/DropdownLaunchpad.vue:38-42
Timestamp: 2025-02-06T17:24:58.784Z
Learning: In the DropdownLaunchpad.vue component, the current implementation of conditional icon rendering using `h(BrandLoading, { variant: 'white' })` is a temporary fix to maintain build stability, with a proper refactoring being handled in a separate branch.
Applied to files:
web/components/UpdateOs/ChangelogModal.vue
🔇 Additional comments (5)
web/components/UpdateOs/ChangelogModal.vue (5)
26-34: Prop-driven mode API looks solidThe added
releaseprop and its shape are clear and keep this component reusable outside the store-driven flow.
42-44: Emittingclosefrom prop mode is the right contractThis cleanly separates ownership of
openbetween prop vs. store modes. No concerns.
59-67: Nice, clean mode switching
isPropMode,currentRelease, andmodalVisiblekeep the data flow well-isolated between view and update flows. 👍
69-70: Correct scoping of renewal CTAHiding license actions in prop mode avoids leaking update-flow controls into read-only contexts.
72-80: Close behavior aligns with ownershipEmitting in prop mode and clearing store in update mode is the right separation of concerns.
| const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' }; | ||
| iframeRef.value.contentWindow.postMessage(message, '*'); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Send postMessage to a constrained targetOrigin, not '*'
Limit the outbound postMessage to the iframe’s origin to reduce the blast radius if a page tries to listen globally.
- const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' };
- iframeRef.value.contentWindow.postMessage(message, '*');
+ const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' };
+ let targetOrigin = '*';
+ try {
+ const candidate = currentIframeUrl.value ?? docsChangelogUrl.value ?? null;
+ if (candidate) {
+ const origin = new URL(candidate).origin;
+ if (allowedDocsOriginRegex.test(origin) || origin === 'http://localhost:3000') {
+ targetOrigin = origin;
+ }
+ }
+ } catch {}
+ iframeRef.value.contentWindow.postMessage(message, targetOrigin);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/components/UpdateOs/ChangelogModal.vue around lines 121 to 123, the
postMessage call uses '*' as targetOrigin which is unsafe; change it to send to
the iframe's actual origin: derive the iframe URL (e.g. iframeRef.value.src or
iframeRef.value.contentWindow.location.origin if same-origin) or parse the known
trusted origin for this iframe, validate that iframeRef.value and contentWindow
exist, then call postMessage(message, trustedOrigin) instead of '*' and handle
the case where origin cannot be determined by skipping the send or logging an
error.
| v-if="currentIframeUrl || currentRelease?.changelogPretty" | ||
| variant="underline" | ||
| :external="true" | ||
| :href="currentIframeUrl || releaseForUpdate?.changelogPretty" | ||
| :icon="ArrowTopRightOnSquareIcon" | ||
| :href="currentIframeUrl || currentRelease?.changelogPretty" | ||
| :icon-right="ArrowTopRightOnSquareIcon" | ||
| aria-label="View on Docs" |
There was a problem hiding this comment.
Use sanitized URL for external link
This should rely on the validated docsChangelogUrl rather than the raw changelogPretty prop.
<BrandButton
- v-if="currentIframeUrl || currentRelease?.changelogPretty"
+ v-if="currentIframeUrl || docsChangelogUrl"
variant="underline"
:external="true"
- :href="currentIframeUrl || currentRelease?.changelogPretty"
+ :href="currentIframeUrl || docsChangelogUrl"
:icon-right="ArrowTopRightOnSquareIcon"
aria-label="View on Docs"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| v-if="currentIframeUrl || currentRelease?.changelogPretty" | |
| variant="underline" | |
| :external="true" | |
| :href="currentIframeUrl || releaseForUpdate?.changelogPretty" | |
| :icon="ArrowTopRightOnSquareIcon" | |
| :href="currentIframeUrl || currentRelease?.changelogPretty" | |
| :icon-right="ArrowTopRightOnSquareIcon" | |
| aria-label="View on Docs" | |
| <BrandButton | |
| v-if="currentIframeUrl || docsChangelogUrl" | |
| variant="underline" | |
| :external="true" | |
| :href="currentIframeUrl || docsChangelogUrl" | |
| :icon-right="ArrowTopRightOnSquareIcon" | |
| aria-label="View on Docs" | |
| /> |
🤖 Prompt for AI Agents
In web/components/UpdateOs/ChangelogModal.vue around lines 230 to 235, the
external link currently uses the raw currentRelease?.changelogPretty which is
unvalidated; change the href (and any conditional that chooses the link) to use
the validated docsChangelogUrl instead (i.e., currentIframeUrl ||
docsChangelogUrl) so the external link only ever points to the sanitized,
validated URL.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/components/UpdateOs/ChangelogModal.vue (2)
184-194: Gate the iframe container on the sanitized, actual source to prevent a permanent spinner when the URL is invalid.Using docsChangelogUrl for the container v-if suppresses the raw changelog fallback even when the URL is rejected. Drive the container off actualIframeSrc instead.
- <div v-if="docsChangelogUrl" class="w-[calc(100%+3rem)] h-[475px] -mx-6 -my-6"> + <div v-if="actualIframeSrc" class="w-[calc(100%+3rem)] h-[475px] -mx-6 -my-6"> <iframe - v-if="actualIframeSrc" + v-if="actualIframeSrc" ref="iframeRef" :src="actualIframeSrc" class="w-full h-full border-0 rounded-md" - sandbox="allow-scripts allow-same-origin allow-top-navigation" + sandbox="allow-scripts allow-same-origin allow-top-navigation-by-user-activation" title="Unraid Changelog" /> </div>
190-190: Remove unrestricted top-level navigation from the iframe sandbox.allow-top-navigation lets embedded content navigate the parent window. If you must support user-initiated navigations, prefer allow-top-navigation-by-user-activation (as in the previous comment’s diff). Otherwise, drop the flag entirely.
♻️ Duplicate comments (1)
web/components/UpdateOs/ChangelogModal.vue (1)
87-90: Block untrusted docs URLs and avoid injecting unsafe iframes/links (also fixes initial-load spinner).docsChangelogUrl is used to drive both the iframe src and the external link without sanitization at source. Combined with sandbox="allow-top-navigation", a malicious URL could navigate the top window. Also, because the watch on docsChangelogUrl is not immediate, actualIframeSrc stays null on first render in prop mode, leaving the modal stuck in the loading state.
Harden by sanitizing the URL upfront, making the watch immediate, and ensuring all consumers (iframe, external link, revertToInitialChangelog) use the sanitized value only.
Apply:
@@ -const docsChangelogUrl = computed(() => { - return currentRelease.value?.changelogPretty ?? null; -}); +const docsChangelogUrl = computed(() => { + const raw = currentRelease.value?.changelogPretty; + if (!raw) return null; + try { + const url = new URL(raw); + if (!/^https?:$/.test(url.protocol)) return null; + return allowedDocsUrlRegex.test(url.toString()) ? url.toString() : null; + } catch { + return null; + } +}); @@ -const revertToInitialChangelog = () => { - if (iframeRef.value && docsChangelogUrl.value) { - iframeRef.value.src = docsChangelogUrl.value; - hasNavigated.value = false; - currentIframeUrl.value = docsChangelogUrl.value; - } -}; +const revertToInitialChangelog = () => { + if (!iframeRef.value || !docsChangelogUrl.value) return; + iframeRef.value.src = docsChangelogUrl.value; + hasNavigated.value = false; + currentIframeUrl.value = docsChangelogUrl.value; + actualIframeSrc.value = docsChangelogUrl.value; +}; @@ -watch(docsChangelogUrl, (newUrl) => { - currentIframeUrl.value = newUrl; - hasNavigated.value = false; - - if (newUrl) { - actualIframeSrc.value = newUrl; - } else { - actualIframeSrc.value = null; - } -}); +watch( + docsChangelogUrl, + (newUrl) => { + currentIframeUrl.value = newUrl; + hasNavigated.value = false; + actualIframeSrc.value = newUrl; + }, + { immediate: true } +); @@ - <BrandButton - v-if="currentIframeUrl || currentRelease?.changelogPretty" + <BrandButton + v-if="currentIframeUrl" variant="underline" :external="true" - :href="currentIframeUrl || currentRelease?.changelogPretty" + :href="currentIframeUrl" :icon-right="ArrowTopRightOnSquareIcon" aria-label="View on Docs" />Also applies to: 141-147, 149-158, 230-235
🧹 Nitpick comments (4)
web/components/UpdateOs/ChangelogModal.vue (4)
224-226: Localize aria-labels.Hard-coded English strings bypass i18n.
- aria-label="Back to Changelog" + :aria-label="t('Back to Changelog')" @@ - aria-label="View on Docs" + :aria-label="t('View on Docs')"Also applies to: 235-236
211-211: Minor: Prefer t(...) over props.t(...) for consistency.You pass t to Modal and elsewhere; use t here too.
- <p>{{ props.t('Loading changelog…') }}</p> + <p>{{ t('Loading changelog…') }}</p>
131-135: Optional: with the watch made immediate, currentIframeUrl assignment here is redundant.Not harmful, but you can drop it to avoid double-writes.
101-115: Dev origin allowance: consider covering https localhost or using an env-gated check.Currently only http://localhost:3000 is whitelisted. You might want to allow https and/or 5173 for Vite, gated by NODE_ENV.
I can ship a small helper like isAllowedDocsOrigin(origin: string): boolean that adds dev allowances behind process.dev.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
web/components/UpdateOs/ChangelogModal.vue(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/components/UpdateOs/ChangelogModal.vue
**/components/**/*.vue
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
Some Vue files may require explicit imports like ref or computed due to Nuxt auto-imports not applying in tests
Files:
web/components/UpdateOs/ChangelogModal.vue
🧠 Learnings (1)
📚 Learning: 2025-02-06T17:24:58.784Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/components/UserProfile/DropdownLaunchpad.vue:38-42
Timestamp: 2025-02-06T17:24:58.784Z
Learning: In the DropdownLaunchpad.vue component, the current implementation of conditional icon rendering using `h(BrandLoading, { variant: 'white' })` is a temporary fix to maintain build stability, with a proper refactoring being handled in a separate branch.
Applied to files:
web/components/UpdateOs/ChangelogModal.vue
🔇 Additional comments (7)
web/components/UpdateOs/ChangelogModal.vue (7)
59-67: Nice abstraction: prop vs. store mode separation reads clean and is future-proof.The isPropMode/currentRelease/modalVisible triad is clear and keeps the component reusable for both Header and Update flows.
72-80: Close behavior is correct but consider also hiding the modal via store if that’s the trigger.If changelogModalVisible can be true independently of releaseForUpdate, also toggle the visibility flag to avoid stale UI.
Would you like a patch that sets changelogModalVisible = false here, or is releaseForUpdate = null sufficient in all flows?
197-203: Prop usage is consistent, but avoid passing unused props to RawChangelogRenderer.If RawChangelogRenderer doesn’t read changelog-pretty, drop it to reduce surface area. Otherwise, ensure it is only used for rendering a “view on docs” link and not to create an iframe internally.
I can scan RawChangelogRenderer.vue and trim/add props as needed—say the word and I’ll follow up with a patch.
169-180: Modal bindings look solid.
- Title interpolates the version safely.
- open is correctly sourced from modalVisible.
- close event funnels through handleClose, which respects prop vs. store mode.
160-164: Theme sync watch is fine.Watching isDarkMode only (instead of both theme and darkMode) centralizes logic. sendThemeToIframe is already guarded.
170-170: Guard modal render on complete release objects in prop mode.You already check currentRelease?.version, which is good. If you later rely on date/name in the header, consider minimal required fields validation.
Do we want to fail closed (not render) if both changelogPretty and changelog are missing?
1-262: Sanity check passed: HeaderOsVersion integrationVerified that in web/components/HeaderOsVersion.ce.vue you’re using ChangelogModal exactly as intended in “prop mode”:
- Passing both :open="showOsReleaseNotesModal" and :release="currentVersionRelease"
- Listening to @close="showOsReleaseNotesModal = false"
This matches the new API defined in ChangelogModal.vue, so there are no regressions here.
| const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' }; | ||
| iframeRef.value.contentWindow.postMessage(message, '*'); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain postMessage target origin to the iframe’s origin (avoid broadcasting to '*').
You already validate incoming messages by origin. Do the same for outgoing messages to prevent leaking theme info to unrelated frames.
- const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' };
- iframeRef.value.contentWindow.postMessage(message, '*');
+ const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' };
+ const targetOrigin =
+ actualIframeSrc.value ? new URL(actualIframeSrc.value).origin : '*';
+ iframeRef.value.contentWindow.postMessage(message, targetOrigin);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' }; | |
| iframeRef.value.contentWindow.postMessage(message, '*'); | |
| } catch (error) { | |
| const message = { type: 'theme-update', theme: isDarkMode.value ? 'dark' : 'light' }; | |
| const targetOrigin = | |
| actualIframeSrc.value ? new URL(actualIframeSrc.value).origin : '*'; | |
| iframeRef.value.contentWindow.postMessage(message, targetOrigin); | |
| } catch (error) { |
🤖 Prompt for AI Agents
In web/components/UpdateOs/ChangelogModal.vue around lines 121–123, the outgoing
postMessage currently uses '*' which broadcasts the theme to any listening
frame; derive the iframe's origin (e.g. new URL(iframeRef.value.src).origin)
after confirming iframeRef.value and iframeRef.value.contentWindow exist, and
pass that origin as the second argument to postMessage instead of '*'; if the
iframe src is unavailable or invalid, bail out or log and do not send the
message.
2 participants
Summary by CodeRabbit