feat: Safari support #125
feat: Safari support #125
Conversation
devop: fix Encryption Excempt
…nto feat/safari-support
fix: Removed unnecessary link to `.entitlemens` (duplicate)
devop: keyring lock issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/extension/src/ui/action/views/asset-detail-view/components/asset-detail-action.vue (1)
47-47: Global Safari detection flag implemented.The code now assigns the global
__IS_SAFARI__flag to a local variable. This implementation correctly supports the conditional rendering of UI elements based on browser detection.For consistency with the rest of the codebase, consider using camelCase for variable naming (
isSafariinstead ofis_safari). Also, ensure that__IS_SAFARI__is properly defined in your build configuration.-const is_safari = __IS_SAFARI__; +const isSafari = __IS_SAFARI__;And update the conditionals accordingly:
-v-if="!is_safari" +v-if="!isSafari"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
packages/extension/src/ui/action/views/asset-detail-view/components/asset-detail-action.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build all
- GitHub Check: safari build
- GitHub Check: test
- GitHub Check: test all
🔇 Additional comments (2)
packages/extension/src/ui/action/views/asset-detail-view/components/asset-detail-action.vue (2)
5-5: Conditional rendering for Buy/Sell button in Safari.The code now hides the Buy/Sell button when running in Safari browsers. This aligns with the PR objective to add Safari support, presumably because this functionality is not supported or behaves differently in Safari.
Can you confirm why the Buy/Sell functionality needs to be disabled specifically for Safari? Adding a code comment would help future developers understand this limitation.
25-25: Conditional rendering for Swap link in Safari.Similar to the Buy/Sell button, the Swap functionality is now hidden when running in Safari browsers.
Is there a specific Safari limitation that prevents the Swap functionality from working properly? Consider documenting this constraint for maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/extension/src/ui/action/App.vue (2)
246-250: Safari keepalive interval works but could guard against unhandled promise rejectionsThe Safari-specific keepalive via
setInterval(isKeyRingLocked, 5000)is a pragmatic way to keep the action window alive. However, becauseisKeyRingLockedis async, any thrown error or rejected promise will be unhandled here.Low risk, but you might want to explicitly “fire and forget” with error handling, and optionally keep an interval id to clear on unmount:
-import { computed, onMounted, ref } from 'vue'; +import { computed, onMounted, onBeforeUnmount, ref } from 'vue'; let keepaliveIntervalId: number | undefined; onMounted(async () => { // ... if (isInitialized) { const _isLocked = await isKeyRingLocked(); if (__IS_SAFARI__) { - setInterval(() => { - isKeyRingLocked(); // keepalive safari action window - }, 1000 * 5); + keepaliveIntervalId = window.setInterval(() => { + void isKeyRingLocked().catch(console.error); // keepalive safari action window + }, 1000 * 5); } // ... } }); +onBeforeUnmount(() => { + if (keepaliveIntervalId !== undefined) { + clearInterval(keepaliveIntervalId); + } +});Not strictly required for correctness in a short-lived popup, but it tightens things up.
486-493: External Google Fonts import: consider bundling font locallyImporting Roboto from Google Fonts will work, but it adds an external network dependency and potential privacy/performance considerations in an extension context.
If store policies or your own privacy posture are strict, consider bundling the font with the extension and referencing it locally instead of via
fonts.googleapis.com. Otherwise, this change is fine as-is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension/package.json(1 hunks)packages/extension/src/ui/action/App.vue(7 hunks)
⏰ 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). (4)
- GitHub Check: build all
- GitHub Check: test all
- GitHub Check: safari build
- GitHub Check: test
🔇 Additional comments (4)
packages/extension/package.json (1)
19-19: Verify if Safari build should include system info generation step.The new
build:safariscript follows the established pattern for browser-specific builds and is consistent withbuild:chromeandbuild:opera. However, thebuild:firefoxscript (Line 17) includes an additional step&& node configs/get-system-info.jsthatbuild:safaridoes not.Verify whether this system info generation step should also be applied to Safari builds, or if it's Firefox-specific and intentionally omitted for Safari.
Additionally, ensure that the build infrastructure (particularly
vite.config.ts) properly recognizes and handlesBROWSER='safari'as a valid environment value, and thatmanifest.safari.tsis properly integrated into the build pipeline.packages/extension/src/ui/action/App.vue (3)
12-18: Main app template refactor looks clean and preserves behaviorThe restructuring of the main app template (loading block,
app-menu,accounts-header, routed view, and modals) is readable and keeps the existing behavior intact. Prop wiring and event handlers (setNetwork,toggleDepositWindow, lock action, updates/settings/addNetwork modals) still line up with the script logic.No issues here from a functional standpoint.
Also applies to: 21-27, 30-32, 36-37, 39-40, 42-45
190-191: Multi-line buy URL template remains correctThe reflowed default
buyLinktemplate string still produces the same URL: the newline is inside the${ ... }expression and doesn’t introduce whitespace into the actual URL. Interpolation ofnetwork,crypto, andplatformremains intact.Looks good.
530-533: Gradient/background tweak is cosmetic and safeThe updated
radial-gradientplus@primarybackground on.app__loadingis syntactically valid and only affects visuals. No behavioral impact on layout or logic.Looks good.
| <div :class="['app', 'restricted-container', 'expanded']" v-if="foundRestrictedAddress || geoRestricted"> | ||
| <swap-looking-animation /> | ||
| <div v-if="popupClosed" class="app__loading__text"> | ||
| <h2>Close this window</h2> | ||
| <p>Continue to onboard on the background page</p> | ||
| </div> |
There was a problem hiding this comment.
popupClosed banner is effectively unreachable with current state gating
popupClosed is only set to true in the openOnboard().then(...) branch when the keyring is not initialized and geoRestricted is false. In that scenario, foundRestrictedAddress is still false and geoRestricted is false, so the restricted container (v-if="foundRestrictedAddress || geoRestricted") never renders, and the “Close this window / Continue to onboard…” block is never shown.
If the goal is to show this Safari fallback message when window.close() fails after onboarding starts, it probably needs to live in the main app branch (or some global overlay) rather than inside the restricted container, or the condition for the outer v-if needs to consider popupClosed as well.
I’d recommend revisiting the placement/conditions so the new banner actually appears in the intended Safari onboarding scenario.
Also applies to: 124-124, 281-283
🤖 Prompt for AI Agents
In packages/extension/src/ui/action/App.vue around lines 3 to 8 (also check
lines ~124 and 281-283), the "popupClosed" banner is unreachable because the
outer v-if only checks foundRestrictedAddress || geoRestricted, so when
popupClosed is set the restricted container may not render; update the rendering
logic to include popupClosed in the outer condition or move the banner into the
main app branch/global overlay so it can render independently (e.g., change the
outer v-if to (foundRestrictedAddress || geoRestricted || popupClosed) or
relocate the banner component to a top-level area that always mounts and
conditionally shows when popupClosed is true), and ensure any related
styles/structure still apply.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension/package.json(1 hunks)packages/extension/src/ui/action/App.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/package.json
⏰ 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). (4)
- GitHub Check: build all
- GitHub Check: test
- GitHub Check: safari build
- GitHub Check: test all
| if (__IS_SAFARI__) { | ||
| setInterval(() => { | ||
| isKeyRingLocked(); // keepalive safari action window | ||
| }, 1000 * 5); | ||
| } |
There was a problem hiding this comment.
Memory leak: Safari keepalive interval is never cleaned up.
The setInterval runs indefinitely without cleanup. Once the interval starts, it continues making background calls every 5 seconds even after the component unmounts, wasting resources and potentially impacting extension performance.
Store the interval ID and clear it in the onUnmounted lifecycle hook.
🔎 Proposed fix
Add an import for onUnmounted at the top (line 118):
-import { computed, onMounted, ref } from 'vue';
+import { computed, onMounted, onUnmounted, ref } from 'vue';Then update the Safari keepalive logic to store and clean up the interval:
+let safariKeepaliveInterval: ReturnType<typeof setInterval> | null = null;
+
onMounted(async () => {
geoRestricted.value = await isGeoRestricted();
const isInitialized = await kr.isInitialized();
isWalletInitialized.value = isInitialized;
if (geoRestricted.value) return;
if (isInitialized) {
const _isLocked = await isKeyRingLocked();
if (__IS_SAFARI__) {
- setInterval(() => {
+ safariKeepaliveInterval = setInterval(() => {
isKeyRingLocked(); // keepalive safari action window
}, 1000 * 5);
}
// ... rest of the code
}
});
+
+onUnmounted(() => {
+ if (safariKeepaliveInterval) {
+ clearInterval(safariKeepaliveInterval);
+ }
+});📝 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.
| if (__IS_SAFARI__) { | |
| setInterval(() => { | |
| isKeyRingLocked(); // keepalive safari action window | |
| }, 1000 * 5); | |
| } | |
| import { computed, onMounted, onUnmounted, ref } from 'vue'; | |
| // ... other code ... | |
| let safariKeepaliveInterval: ReturnType<typeof setInterval> | null = null; | |
| onMounted(async () => { | |
| geoRestricted.value = await isGeoRestricted(); | |
| const isInitialized = await kr.isInitialized(); | |
| isWalletInitialized.value = isInitialized; | |
| if (geoRestricted.value) return; | |
| if (isInitialized) { | |
| const _isLocked = await isKeyRingLocked(); | |
| if (__IS_SAFARI__) { | |
| safariKeepaliveInterval = setInterval(() => { | |
| isKeyRingLocked(); // keepalive safari action window | |
| }, 1000 * 5); | |
| } | |
| // ... rest of the code | |
| } | |
| }); | |
| onUnmounted(() => { | |
| if (safariKeepaliveInterval) { | |
| clearInterval(safariKeepaliveInterval); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/extension/src/ui/action/App.vue around lines 287 to 291, the Safari
keepalive setInterval is started but never cleared, causing a memory leak;
update the component to import and use Vue's onUnmounted, store the interval ID
returned by setInterval in a variable, and call clearInterval(intervalId) inside
an onUnmounted hook so the timer is cleaned up when the component unmounts.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.