feat: Safari support #125
feat: Safari support #125
Conversation
devop: fix Encryption Excempt
…nto feat/safari-support
fix: Removed unnecessary link to `.entitlemens` (duplicate)
Release: v2.0.0
WalkthroughThis pull request introduces multiple changes to the GitHub Actions workflows and the macOS Enkrypt application. Key updates include the addition of new workflows for building and deploying the macOS application, modifications to existing workflows for building and testing browser extensions, and the introduction of new files for entitlements, configuration, and assets. Additionally, several files have been created or modified to enhance support for Safari, including the addition of a Safari-specific manifest and adjustments to the browser compatibility settings. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (32)
macos/Enkrypt/fastlane/README.md (2)
22-22: Fix capitalization of "macOS"The operating system name should be capitalized as "macOS" instead of "macos".
-Prepare the macos app for build +Prepare the macOS app for build🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: The operating system from Apple is written “macOS”.
Context: ...exec] fastlane preparePrepare the macos app for build ### releasesh [bund...(MAC_OS)
16-38: Improve heading structure and action documentationThe heading structure should be consistent throughout the document. Additionally, the action descriptions could be more detailed to better guide users.
Consider updating the structure and descriptions:
-### prepare +## Available Actions + +### Preparation + +#### prepare ```sh [bundle exec] fastlane prepare-Prepare the macos app for build
+Prepares the macOS app for building by setting up necessary configurations and dependencies.-### release
+### Deployment
+
+#### release[bundle exec] fastlane release-Push a new release build to the App Store
+Pushes a new release build to the App Store, handling code signing and submission process.-### buildandrelease
+#### buildandrelease[bundle exec] fastlane buildandrelease-Prepare build and release build to the App Store
+Combines both preparation and release steps into a single command for streamlined deployment.<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [grammar] ~22-~22: The operating system from Apple is written “macOS”. Context: ...exec] fastlane prepare ``` Prepare the macos app for build ### release ```sh [bund... (MAC_OS) </details> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 16-16: Expected: h2; Actual: h3 Heading levels should only increment by one level at a time (MD001, heading-increment) --- 16-16: Expected: setext; Actual: atx Heading style (MD003, heading-style) --- 24-24: Expected: setext; Actual: atx Heading style (MD003, heading-style) --- 32-32: Expected: setext; Actual: atx Heading style (MD003, heading-style) </details> </details> </blockquote></details> <details> <summary>macos/Enkrypt/Enkrypt/Base.lproj/Main.html (3)</summary><blockquote> `5-5`: **Consider expanding Content Security Policy** The current CSP might be too restrictive. Consider adding necessary directives for: - `img-src` for images - `style-src` for styles - `script-src` for scripts ```diff - <meta http-equiv="Content-Security-Policy" content="default-src 'self'"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; img-src 'self'; style-src 'self'; script-src 'self';">
9-10: Add integrity checks and preload hints for critical resourcesConsider adding:
- Subresource integrity checks for external resources
- Preload hints for critical CSS/JS
+ <link rel="preload" href="../Style.css" as="style"> + <link rel="preload" href="../Script.js" as="script"> <link rel="stylesheet" href="../Style.css"> <script src="../Script.js" defer></script>
20-21: Enhance image handling for dark mode and performanceConsider using modern HTML features for better dark mode support and performance:
- Use
srcsetfor responsive images- Add
loading="lazy"for images below the fold- Preserve aspect ratio using
aspect-ratio- <img class="image" src="../Light.png" width="540" alt="Enkrypt's extension" /> - <img class="image-dark" src="../Dark.png" width="540" alt="Enkrypt's extension" /> + <picture> + <source srcset="../Dark.png" media="(prefers-color-scheme: dark)"> + <source srcset="../Light.png" media="(prefers-color-scheme: light)"> + <img + class="image" + src="../Light.png" + width="540" + height="auto" + loading="lazy" + alt="Enkrypt's extension" + style="aspect-ratio: auto;" + /> + </picture>macos/Enkrypt/Gemfile (1)
1-6: Document Fastlane setup and deployment process.Since this is part of adding Safari support, consider adding the following to help other developers:
- Add a README in the
fastlanedirectory explaining the setup process- Document required certificates and provisioning profiles for Safari extension signing
- Include instructions for local development setup
- Add troubleshooting guidelines for common deployment issues
This will help maintain a smooth CI/CD process for the Safari extension.
macos/Enkrypt/fastlane/Appfile (1)
1-3: Enhance documentation with environment variable requirementsThe current documentation link is helpful but doesn't specify the required environment variables.
Add documentation for required environment variables:
# For more information about the Appfile, see: # https://docs.fastlane.tools/advanced/#appfile +# +# Required environment variables: +# - APPLE_ID: Your Apple ID for deployment +# - APP_ID: The bundle identifier of your app +# - ITC_TEAM_ID: Your App Store Connect Team ID +# - TEAM_ID: Your Apple Developer Team IDmacos/Enkrypt/fastlane/Matchfile (1)
5-11: Consider enabling additional security optionsThe commented examples are helpful, but there are important security configurations missing.
Consider adding these configurations:
-# app_identifier(["tools.fastlane.app", "tools.fastlane.app2"]) -# username("[email protected]") # Your Apple Developer Portal username - -# For all available options run `fastlane match --help` -# Remove the # in the beginning of the line to enable the other options - -# The docs are available on https://docs.fastlane.tools/actions/match +# Security configurations +readonly true # Prevent modification of certificates +git_branch "certificates" # Use a dedicated branch + +# Specify the type of certificates to sync +type "appstore" # For App Store distribution + +# For more information, see: +# https://docs.fastlane.tools/actions/matchpackages/extension/src/libs/utils/selective-wasm.ts (1)
4-6: Verify error handling for WASM initializationWhile the initialization logic is correct, consider adding error handling for the WASM initialization to gracefully handle potential failures.
if (__IS_SAFARI__) { - initBridge(createWasm); + try { + initBridge(createWasm); + } catch (error) { + console.error('Failed to initialize WASM bridge:', error); + // Consider showing a user-friendly error message or fallback + } }.github/workflows/test-all.yml (1)
7-7: Consider adding Safari-specific testsSince this PR adds Safari support, consider adding a dedicated job or steps for testing Safari-specific functionality on a macOS runner.
Example addition:
test-safari: name: test safari runs-on: macos-latest steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: "20.10.0" cache: "yarn" - run: yarn install - run: yarn build:safari - run: yarn test:safari # Add Safari-specific testsmacos/Enkrypt/Enkrypt/Resources/Script.js (1)
1-9: Add error handling and type validationThe
showfunction could be more robust:
- Add null checks for document.body
- Use safer class manipulation methods
function show(enabled) { + if (!document.body) return; if (typeof enabled === "boolean") { - document.body.classList.toggle(`state-on`, enabled); - document.body.classList.toggle(`state-off`, !enabled); + const classes = document.body.classList; + classes.remove(`state-on`, `state-off`); + classes.add(enabled ? `state-on` : `state-off`); } else { document.body.classList.remove(`state-on`); document.body.classList.remove(`state-off`); } }macos/Enkrypt/Enkrypt Extension/SafariWebExtensionHandler.swift (1)
11-11: Consider moving the message key to an enumFor better type safety and maintainability, consider moving the message key to an enum or struct.
-let SFExtensionMessageKey = "message" +enum ExtensionConstants { + static let messageKey = "message" + + enum MessageTypes { + static let response = "Response to" + // Add other message types here + } +}packages/extension/src/scripts/chrome/background.ts (1)
Line range hint
38-47: Add error handling for content script registrationWhile the content script registration is correctly implemented, it lacks error handling which could be important for debugging Safari-specific issues.
Consider adding error handling:
if (__IS_OPERA__ || __IS_SAFARI__) { - Browser.scripting.registerContentScripts([ + Browser.scripting.registerContentScripts([ { id: 'inject-script', js: ['scripts/inject.js'], persistAcrossSessions: false, matches: ['http://*/*', 'https://*/*'], runAt: 'document_start', world: 'MAIN', } as any, - ]); + ]).catch((error) => { + console.error('Failed to register content script:', error); + }); }macos/Enkrypt/Enkrypt/ViewController.swift (2)
12-12: Consider moving extension ID to configurationThe extension bundle identifier should be moved to a configuration file or environment variable for better maintainability and to support different environments (development, staging, production).
41-51: Improve user experience for preferences navigationThe current implementation immediately terminates the app after showing preferences, which might be jarring for users.
Consider providing visual feedback before closing:
func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) { if (message.body as! String != "open-preferences") { return; } SFSafariApplication.showPreferencesForExtension(withIdentifier: extensionBundleIdentifier) { error in + guard error == nil else { + DispatchQueue.main.async { + self.showError("Failed to open preferences: \(error?.localizedDescription ?? "")") + } + return + } DispatchQueue.main.async { - NSApplication.shared.terminate(nil) + // Give user time to see the preferences window + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + NSApplication.shared.terminate(nil) + } } } }packages/extension/src/manifest/manifest-safari.json (1)
52-57: Consider Safari version range implicationsThe strict version constraints (
16.6to18.*) might need periodic updates. Consider implementing a version check in the code to handle unsupported versions gracefully.packages/extension/src/ui/action/components/network-menu/index.vue (2)
24-24: Document DApps feature limitationThe code disables the DApps feature in Safari, but there's no documentation explaining why this limitation exists.
Add a code comment explaining the Safari limitation:
- v-if="DappList[network.name] && !is_safari" + v-if="DappList[network.name] && !is_safari // DApps feature is not supported in Safari due to extension API limitations"
Line range hint
24-30: Consider user experience for Safari usersInstead of silently hiding the DApps menu item, consider showing a disabled state with a tooltip explaining why it's not available in Safari.
Example implementation:
- <router-link - v-if="DappList[network.name] && !is_safari" + <div + class="dapps-menu-item" + :class="{ 'disabled': is_safari }" + :title="is_safari ? 'DApps are not supported in Safari' : ''" + > + <router-link + v-if="DappList[network.name] && !is_safari" + :to="{ + name: 'dapps', + params: { id: !!selected ? selected : null }, + }" + > + <DApps /><br />DApps + </router-link> + <template v-else> + <DApps class="disabled-icon" /><br />DApps + </template> + </div>Add these styles:
.dapps-menu-item { &.disabled { opacity: 0.5; cursor: not-allowed; .disabled-icon { pointer-events: none; } } }macos/Enkrypt/fastlane/Fastfile (1)
35-39: Consider validating manifest version formatThe version number is read directly from manifest.json without validation. Consider adding version format validation to prevent build failures.
+ require 'semantic' # Add at the top manifest = read_json( json_path: "../../packages/extension/dist/manifest.json" ) + version = manifest[:version] + raise "Invalid version format" unless SemanticVersion.valid?(version) increment_version_number_in_xcodeproj(version_number: version, xcodeproj: "Enkrypt.xcodeproj")packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vue (1)
Line range hint
7-27: Consider graceful UI adaptation for SafariThe current implementation might lead to uneven spacing when exchange features are hidden. Consider adjusting the layout to maintain visual consistency across browsers.
<template> <div class="network-activity__action"> - <div class="network-activity__action-wrap"> + <div class="network-activity__action-wrap" :class="{ 'safari-layout': !showExchange }"> ... </div> </template> <style lang="less"> .network-activity__action-wrap { &.safari-layout { justify-content: center; gap: 20px; } } </style>packages/extension/src/ui/onboard/hardware-wallet/add-hardware-wallet.vue (1)
Line range hint
58-69: Enhance error handling for hardware wallet detectionThe current implementation silently fails and returns an empty object. Consider providing more informative error handling.
hwWallet .getSupportedPaths({ wallet: HWwalletType.ledger, networkName: route.query.network as NetworkNames, }) .then(paths => { if (paths) isLedgerSupported.value = !(__IS_FIREFOX__ || __IS_SAFARI__); }) - .catch(() => ({})); + .catch((error) => { + console.error('Failed to detect Ledger support:', error); + isLedgerSupported.value = false; + });.github/workflows/build-all.yml (3)
7-7: Consider renaming job for clarityThe job name "build all" is misleading as it only builds Chrome and Firefox extensions, not Safari.
Consider:
- name: build all + name: build chrome and firefox
Line range hint
44-61: Security: Avoid exposing API keys in curl commandsThe VirusTotal API key is visible in the curl commands' output. Consider using the
-s/--silentflag to suppress curl's output.- curl --request GET --url https://www.virustotal.com/api/v3/files/upload_url --header "x-apikey: $VIRUS_TOTAL_API_KEY" + curl -s --request GET --url https://www.virustotal.com/api/v3/files/upload_url --header "x-apikey: $VIRUS_TOTAL_API_KEY" > /dev/null
Line range hint
44-61: Modernize shell script syntaxReplace legacy backtick syntax with
$()for command substitution and add proper quoting.- FILE_SHA256=`sha256sum ./packages/extension/dist/release.zip | awk '{print $1}'` + FILE_SHA256="$(sha256sum ./packages/extension/dist/release.zip | awk '{print $1}')" - UPLOAD_URL=`curl --request GET --url https://www.virustotal.com/api/v3/files/upload_url --header "x-apikey: $VIRUS_TOTAL_API_KEY" | jq --raw-output '.data'` + UPLOAD_URL="$(curl -s --request GET --url https://www.virustotal.com/api/v3/files/upload_url --header "x-apikey: $VIRUS_TOTAL_API_KEY" | jq --raw-output '.data')".github/workflows/macos-deploy.yml (3)
14-16: Modernize shell script syntaxReplace legacy backtick syntax with
$()for command substitution and add proper quoting.- docker build -t enkrypt-build-container . - echo "NODE_VERSION=$(docker run --rm -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "node --version")" >> $GITHUB_OUTPUT + docker build -t enkrypt-build-container . + echo "NODE_VERSION=$(docker run --rm -v "$(pwd)":/home:rw enkrypt-build-container /bin/bash -c "node --version")" >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint (1.7.4)
14-14: shellcheck reported issue in this script: SC2046:warning:2:41: Quote this to prevent word splitting
(shellcheck)
14-14: shellcheck reported issue in this script: SC2006:style:2:41: Use $(...) notation instead of legacy backticks
...(shellcheck)
14-14: shellcheck reported issue in this script: SC2086:info:2:115: Double quote to prevent globbing and word splitting
(shellcheck)
66-86: Add smoke tests before App Store submissionConsider adding basic smoke tests to verify the build before submitting to the App Store.
Add a test step before deployment:
- name: Run smoke tests run: | xcodebuild test -project "macos/Enkrypt/Enkrypt.xcodeproj" \ -scheme "Enkrypt" \ -destination "platform=macOS" \ -only-testing "EnkryptTests/SmokeTests"
87-92: Consider adding version tag to artifact nameThe current artifact naming only includes the path. Adding a version tag would help with tracking builds.
- name: safari-build-pkg + name: safari-build-pkg-${{ github.ref_name }}🧰 Tools
🪛 actionlint (1.7.4)
88-88: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
packages/extension/src/ui/action/views/accounts/index.vue (2)
165-165: Document browser compatibility constraintsThe addition of
isBrowserHWSupportedcorrectly handles browser compatibility, but consider documenting these limitations.Add a comment explaining why hardware wallet support is disabled in Firefox and Safari:
+// Hardware wallet support is disabled in Firefox and Safari due to limited WebUSB support const isBrowserHWSupported = !(__IS_FIREFOX__ || __IS_SAFARI__);
66-68: Consider adding user feedback for unsupported featuresWhen hardware wallet support is unavailable, users might benefit from understanding why the feature is hidden.
Consider adding a disabled button with a tooltip explaining browser compatibility:
- v-if=" - hwWallet.isNetworkSupported(network.name) && isBrowserHWSupported - " + v-if="hwWallet.isNetworkSupported(network.name)" + :class="{ 'disabled': !isBrowserHWSupported }" + :title="!isBrowserHWSupported ? 'Hardware wallet support is not available in this browser' : ''"macos/Enkrypt/Enkrypt/Base.lproj/Main.storyboard (3)
7-7: Consider updating the minimum tool version.The storyboard is set to support documents saved in Xcode 8 format. Consider updating to a more recent minimum version to ensure access to newer UI features and improvements.
57-61: Add standard keyboard shortcut for Help menu item.The Help menu item should use Command+? as the standard keyboard shortcut on macOS.
-<menuItem title="Enkrypt Help" keyEquivalent="?" id="FKE-Sm-Kum"> +<menuItem title="Enkrypt Help" keyEquivalent="?" id="FKE-Sm-Kum"> + <modifierMask key="keyEquivalentModifierMask" command="YES"/>
16-52: Add standard Preferences menu item.The Enkrypt menu should include a Preferences menu item (Command+,) as per macOS Human Interface Guidelines.
<menu key="submenu" title="Enkrypt" systemMenu="apple" id="uQy-DD-JDr"> <items> <menuItem title="About Enkrypt" id="5kV-Vb-QxS"> <!-- ... --> </menuItem> <menuItem isSeparatorItem="YES" id="VOq-y0-SEH"/> + <menuItem title="Preferences..." keyEquivalent="," id="BOF-NM-1cW"> + <connections> + <action selector="showPreferences:" target="Ady-hI-5gd" id="YLX-OP-Qku"/> + </connections> + </menuItem> + <menuItem isSeparatorItem="YES" id="wFC-TO-SCJ"/> <menuItem title="Hide Enkrypt" keyEquivalent="h" id="Olw-nP-bQN">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
macos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/icon_128x128.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/[email protected]is excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/icon_16x16.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/[email protected]is excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/icon_256x256.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/[email protected]is excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/icon_32x32.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/[email protected]is excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/icon_512x512.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/[email protected]is excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Resources/Dark.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Resources/Light.pngis excluded by!**/*.pngmacos/Enkrypt/Enkrypt/Resources/Logo.pngis excluded by!**/*.pngmacos/Enkrypt/Gemfile.lockis excluded by!**/*.lockpackages/extension/public/assets/img/safari-icons/icon16.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon19.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon192.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon32.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon38.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon48.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon64.pngis excluded by!**/*.pngpackages/extension/public/assets/img/safari-icons/icon72.pngis excluded by!**/*.png
📒 Files selected for processing (49)
.github/workflows/build-all.yml(1 hunks).github/workflows/macos-deploy.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/test-all.yml(1 hunks).gitignore(1 hunks)macos/Enkrypt/.gitignore(1 hunks)macos/Enkrypt/Enkrypt Extension/Enkrypt_Extension.entitlements(1 hunks)macos/Enkrypt/Enkrypt Extension/Info.plist(1 hunks)macos/Enkrypt/Enkrypt Extension/SafariWebExtensionHandler.swift(1 hunks)macos/Enkrypt/Enkrypt.xcodeproj/project.pbxproj(1 hunks)macos/Enkrypt/Enkrypt.xcodeproj/xcshareddata/xcschemes/Enkrypt.xcscheme(1 hunks)macos/Enkrypt/Enkrypt/AppDelegate.swift(1 hunks)macos/Enkrypt/Enkrypt/Assets.xcassets/AccentColor.colorset/Contents.json(1 hunks)macos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/Contents.json(1 hunks)macos/Enkrypt/Enkrypt/Assets.xcassets/Contents.json(1 hunks)macos/Enkrypt/Enkrypt/Assets.xcassets/LargeIcon.imageset/Contents.json(1 hunks)macos/Enkrypt/Enkrypt/Base.lproj/Main.html(1 hunks)macos/Enkrypt/Enkrypt/Base.lproj/Main.storyboard(1 hunks)macos/Enkrypt/Enkrypt/Enkrypt.entitlements(1 hunks)macos/Enkrypt/Enkrypt/Info.plist(1 hunks)macos/Enkrypt/Enkrypt/Resources/Script.js(1 hunks)macos/Enkrypt/Enkrypt/Resources/Style.css(1 hunks)macos/Enkrypt/Enkrypt/ViewController.swift(1 hunks)macos/Enkrypt/Gemfile(1 hunks)macos/Enkrypt/fastlane/Appfile(1 hunks)macos/Enkrypt/fastlane/Fastfile(1 hunks)macos/Enkrypt/fastlane/Matchfile(1 hunks)macos/Enkrypt/fastlane/Pluginfile(1 hunks)macos/Enkrypt/fastlane/README.md(1 hunks)macos/Enkrypt/fastlane/report.xml(1 hunks)packages/extension/.browserlistrc(0 hunks)packages/extension/configs/vite/transform-manifest.ts(3 hunks)packages/extension/package.json(1 hunks)packages/extension/src/libs/utils/selective-wasm.ts(1 hunks)packages/extension/src/manifest/manifest-safari.json(1 hunks)packages/extension/src/scripts/chrome/background.ts(1 hunks)packages/extension/src/ui/action/components/network-menu/index.vue(2 hunks)packages/extension/src/ui/action/main.ts(1 hunks)packages/extension/src/ui/action/views/accounts/components/add-account-form.vue(1 hunks)packages/extension/src/ui/action/views/accounts/components/delete-account-form.vue(1 hunks)packages/extension/src/ui/action/views/accounts/components/rename-account-form.vue(1 hunks)packages/extension/src/ui/action/views/accounts/index.vue(2 hunks)packages/extension/src/ui/action/views/deposit/index.vue(1 hunks)packages/extension/src/ui/action/views/import-account/index.vue(1 hunks)packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vue(3 hunks)packages/extension/src/ui/onboard/create-wallet/wallet-ready.vue(1 hunks)packages/extension/src/ui/onboard/hardware-wallet/add-hardware-wallet.vue(2 hunks)packages/extension/src/ui/onboard/main.ts(1 hunks)packages/extension/src/ui/provider-pages/main.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/extension/.browserlistrc
✅ Files skipped from review due to trivial changes (19)
- macos/Enkrypt/Enkrypt Extension/Enkrypt_Extension.entitlements
- macos/Enkrypt/Enkrypt/Assets.xcassets/AccentColor.colorset/Contents.json
- macos/Enkrypt/fastlane/Pluginfile
- packages/extension/src/ui/action/views/accounts/components/delete-account-form.vue
- packages/extension/src/ui/action/views/accounts/components/add-account-form.vue
- macos/Enkrypt/Enkrypt/Assets.xcassets/Contents.json
- packages/extension/src/ui/action/views/accounts/components/rename-account-form.vue
- packages/extension/src/ui/onboard/main.ts
- packages/extension/src/ui/provider-pages/main.ts
- packages/extension/src/ui/action/main.ts
- macos/Enkrypt/Enkrypt/Info.plist
- macos/Enkrypt/Enkrypt/Assets.xcassets/LargeIcon.imageset/Contents.json
- macos/Enkrypt/Enkrypt/Assets.xcassets/AppIcon.appiconset/Contents.json
- macos/Enkrypt/.gitignore
- packages/extension/src/ui/action/views/import-account/index.vue
- macos/Enkrypt/Enkrypt/Enkrypt.entitlements
- macos/Enkrypt/Enkrypt/Resources/Style.css
- macos/Enkrypt/Enkrypt.xcodeproj/xcshareddata/xcschemes/Enkrypt.xcscheme
- .gitignore
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/macos-deploy.yml
10-10: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: shellcheck reported issue in this script: SC2046:warning:2:41: Quote this to prevent word splitting
(shellcheck)
14-14: shellcheck reported issue in this script: SC2006:style:2:41: Use $(...) notation instead of legacy backticks ...
(shellcheck)
14-14: shellcheck reported issue in this script: SC2086:info:2:115: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:1:46: Double quote to prevent globbing and word splitting
(shellcheck)
23-23: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
30-30: shellcheck reported issue in this script: SC2046:warning:1:20: Quote this to prevent word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2006:style:1:20: Use $(...) notation instead of legacy backticks ...
(shellcheck)
33-33: shellcheck reported issue in this script: SC2046:warning:2:47: Quote this to prevent word splitting
(shellcheck)
33-33: shellcheck reported issue in this script: SC2006:style:2:47: Use $(...) notation instead of legacy backticks ...
(shellcheck)
37-37: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
54-54: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
57-57: the runner of "actions/download-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
88-88: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
macos/Enkrypt/fastlane/README.md
[grammar] ~22-~22: The operating system from Apple is written “macOS”.
Context: ...exec] fastlane prepare Prepare the macos app for build ### release sh [bund...
(MAC_OS)
🪛 Markdownlint (0.37.0)
macos/Enkrypt/fastlane/README.md
16-16: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
4-4: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
14-14: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
16-16: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
24-24: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
32-32: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
🔇 Additional comments (23)
macos/Enkrypt/fastlane/README.md (1)
42-46: LGTM: Auto-generation notice and documentation links
The footer section appropriately indicates that this is an auto-generated file and provides useful links to Fastlane documentation.
macos/Enkrypt/Enkrypt.xcodeproj/project.pbxproj (2)
397-397: Review deployment target configuration.
There's an inconsistency in the macOS deployment targets:
- Main app (Enkrypt): macOS 12.3
- Extension: macOS 10.14
This could lead to compatibility issues. Consider aligning the deployment targets or documenting the reason for this difference.
Also applies to: 478-478, 550-550, 591-591
1-641: Overall configuration looks good.
The Xcode project configuration includes:
- Proper code signing setup with manual signing and provisioning profiles
- Correct framework dependencies (SafariServices, WebKit)
- Appropriate build settings and version information
- Comprehensive resource management
packages/extension/src/ui/onboard/create-wallet/wallet-ready.vue (2)
40-40: LGTM! Good use of webextension-polyfill
Using the polyfill ensures consistent browser API behavior across different browsers, including Safari.
43-45: Verify tab management behavior in Safari
Since this PR adds Safari support, we should verify that the tab management behavior works consistently in Safari.
✅ Verification successful
Let me gather more information about the tab management implementation to ensure there are no browser-specific differences.
Based on the search results, I can now generate the final response:
Tab management implementation appears browser-agnostic and safe for Safari
The codebase uses the standard WebExtension Browser.tabs API consistently across all tab management operations, with no browser-specific conditions or workarounds. The implementation in wallet-ready.vue follows the same pattern used throughout the codebase, which is known to work across browsers:
- Uses standard
query()to find tabs - Uses standard
remove()for tab removal - No Safari-specific edge cases or special handling needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any Safari-specific tab management code or workarounds in the codebase
rg -g '!{*.json,*.md}' -i 'safari.*tab|tab.*safari'
# Look for any Safari-specific conditions or checks
ast-grep --pattern 'if ($BROWSER === "safari") { $$$ }'
Length of output: 822
Script:
#!/bin/bash
# Check for any browser-specific tab management implementations
rg -g '!{*.json,*.md}' 'Browser\.tabs\.(query|remove|create)' -A 3
# Look for any browser detection or conditional logic around tab management
ast-grep --pattern 'Browser.tabs.$_($$$)'
# Check for any browser detection utilities or constants
rg -g '!{*.json,*.md}' 'isSafari|BROWSER_TYPE|browserType' -B 2 -A 2
Length of output: 3898
macos/Enkrypt/Gemfile (2)
1-1: LGTM! Secure gem source configuration.
The source is correctly configured to use HTTPS for the official RubyGems repository.
5-6: Verify Pluginfile setup and contents.
The plugin loading logic is implemented correctly, but let's ensure the Pluginfile exists and contains the necessary plugins for Safari extension deployment.
✅ Verification successful
Pluginfile setup is correct with required plugins
The Pluginfile exists and contains the necessary plugins for managing versioning and JSON operations:
fastlane-plugin-json: For JSON manipulation tasksfastlane-plugin-versioning: For version management
The plugin configuration is properly set up and the file is correctly placed in the expected location at macos/Enkrypt/fastlane/Pluginfile.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Pluginfile existence and contents
# Expected: Find Pluginfile and list any Fastlane plugins configured for Safari deployment
# Check if Pluginfile exists
if [ -f "macos/Enkrypt/fastlane/Pluginfile" ]; then
echo "Found Pluginfile:"
cat "macos/Enkrypt/fastlane/Pluginfile"
else
echo "Warning: Pluginfile not found at macos/Enkrypt/fastlane/Pluginfile"
fi
# Search for any other Fastlane configuration files
echo -e "\nRelated Fastlane configuration files:"
fd -t f -p "macos/Enkrypt/fastlane/*" --exec ls -l {}
Length of output: 922
packages/extension/src/libs/utils/selective-wasm.ts (1)
4-4: Verify the IS_SAFARI flag definition
The code relies on the __IS_SAFARI__ compile-time constant. Let's verify its definition.
✅ Verification successful
__IS_SAFARI__ flag is properly defined and used consistently
The flag is correctly defined in the build configuration (packages/extension/vite.config.ts) as a compile-time constant based on the BROWSER environment variable. It's also properly declared in the TypeScript declaration file (packages/extension/env.d.ts) and used consistently across the codebase for browser-specific logic, including hardware wallet support, UI components, and WASM loading.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for __IS_SAFARI__ definition in build configuration
rg -l "__IS_SAFARI__" "vite.config*"
# Search for its usage across the codebase
rg "__IS_SAFARI__"
Length of output: 1065
.github/workflows/test-all.yml (1)
7-7: 🛠️ Refactor suggestion
Consider using LTS version of Node.js
The workflow uses Node.js v22.10.0 which is an alpha version. Consider using an LTS version for better stability.
- node-version: "22.10.0"
+ node-version: "20.10.0" # Latest LTS versionLikely invalid or redundant comment.
macos/Enkrypt/Enkrypt/AppDelegate.swift (2)
17-19: LGTM: Appropriate window management behavior
The implementation ensures the application terminates when the last window is closed, which is the expected behavior for most macOS applications.
13-15: Consider implementing necessary initialization logic
The applicationDidFinishLaunching method is currently empty. Consider adding necessary initialization logic such as:
- Setting up logging
- Configuring initial window state
- Initializing any required services
packages/extension/configs/vite/transform-manifest.ts (2)
21-29: LGTM: Content script handling for Safari
The conditional logic correctly prevents direct injection of inject.js for Safari and Opera browsers, which is appropriate since these browsers require using the scripting API for content script injection.
30-31: LGTM: Required permission for Safari content scripts
The addition of the 'scripting' permission for Safari is correct and necessary to support the dynamic content script registration implemented in the background script.
packages/extension/src/manifest/manifest-safari.json (2)
42-44: Verify CSP configuration for Safari
The CSP allows wasm-unsafe-eval which is necessary for WebAssembly but should be carefully reviewed. Ensure that the CSP is compatible with Safari's security model.
✅ Verification successful
CSP configuration for Safari is appropriate and necessary
The wasm-unsafe-eval directive in the CSP is justified and correctly configured:
- The extension uses
@polkadot/wasm-cryptofor cryptographic operations - There's a dedicated Safari-specific WASM initialization in
selective-wasm.ts - The same CSP configuration is consistently applied across all browser manifests (Chrome, Firefox, Safari)
The WASM usage is properly scoped and initialized only when needed (specifically for Safari through the __IS_SAFARI__ check), making this a secure implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WebAssembly usage to verify the necessity
rg -l "WebAssembly" --type ts
Length of output: 29
Script:
#!/bin/bash
# Search for any WASM-related imports, configurations, or usage patterns
rg -i "wasm|webassembly" --type ts --type js --type json -A 3
Length of output: 3042
41-41: Review broad host and permission scope
The extension requests broad host permissions (file://*/*, http://*/*, https://*/*) and extensive permissions including storage, unlimitedStorage, tabs, clipboardWrite, and scripting. While these might be necessary, they increase the security surface area.
Let's verify if all these permissions are actually used:
Also applies to: 45-51
packages/extension/src/ui/action/components/network-menu/index.vue (1)
45-45: Verify __IS_SAFARI__ constant definition
The code uses __IS_SAFARI__ but its definition is not visible in the current context.
✅ Verification successful
Let's check the definition and usage of __IS_SAFARI__ in the configuration files and related code to understand its purpose.
Let's check how the Safari detection is being used in the network menu component to understand the context of hiding DApps.
Based on the gathered information, I can now provide a final response:
__IS_SAFARI__ constant is properly defined and used correctly
The constant __IS_SAFARI__ is properly defined in the Vite configuration as a build-time constant based on the BROWSER environment variable. The Safari detection is used to hide the DApps menu item in Safari browsers, which is a deliberate feature considering:
- The constant is properly typed in
env.d.ts - Safari has specific browser settings defined in
manifest-safari.json - The implementation in the network menu component correctly uses this flag to conditionally render the DApps section
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for __IS_SAFARI__ definition
echo "Searching for __IS_SAFARI__ definition..."
rg -l "__IS_SAFARI__" --type ts --type js
# Search for any Safari-related configuration
echo "Searching for Safari-related configuration..."
rg -l "safari" --type ts --type js --type json
Length of output: 1120
Script:
#!/bin/bash
# Check vite config for __IS_SAFARI__ definition
echo "Checking vite.config.ts..."
cat packages/extension/vite.config.ts
# Check env.d.ts for type definition
echo -e "\nChecking env.d.ts..."
cat packages/extension/env.d.ts
# Check browser detection utility
echo -e "\nChecking browser detection utility..."
cat packages/extension/src/ui/action/utils/browser.ts
# Check Safari manifest
echo -e "\nChecking Safari manifest..."
cat packages/extension/src/manifest/manifest-safari.json
Length of output: 7041
Script:
#!/bin/bash
# Check network menu implementation
echo "Checking network menu implementation..."
cat packages/extension/src/ui/action/components/network-menu/index.vue
# Check if there are any Safari-specific limitations or issues mentioned in documentation
echo -e "\nChecking for Safari-related documentation..."
find . -type f -name "README*" -o -name "*.md" | while read -r file; do
echo "=== $file ==="
grep -i "safari" "$file" || true
done
Length of output: 5485
macos/Enkrypt/fastlane/Fastfile (1)
21-31: Verify git authorization handling
The git authorization token is being Base64 encoded but transmitted as an environment variable. While this is a common practice, ensure:
- The token has minimal required permissions
- The token is properly secured in CI/CD secrets
✅ Verification successful
The previous search was too narrow with just YAML files. Let's expand our search to all files and look for CI configurations where this environment variable might be defined.
Let's examine the CI workflow files to verify how this environment variable is handled in the CI/CD pipeline, particularly focusing on the macOS deployment workflow where this secret is likely to be used.
Git authorization is properly secured in CI/CD pipeline
The GIT_AUTHORIZATION token is properly handled:
- It's stored as a GitHub Actions secret (OSX_GIT_AUTHORIZATION)
- It's only exposed during the macOS deployment workflow
- It's used in a readonly context for certificate matching
- The secret is properly scoped to the specific deployment task
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the git authorization is used in other files
rg -l "GIT_AUTHORIZATION" --type yaml
Length of output: 37
Script:
#!/bin/bash
# Let's broaden our search to all files and include some context
rg -n "GIT_AUTHORIZATION" -C 2
# Also check for any GitHub Actions or CI configuration files that might define this variable
fd -e yml -e yaml . .github/
Length of output: 571
Script:
#!/bin/bash
# Let's check the GitHub workflow files for any references to this secret
cat .github/workflows/macos-deploy.yml
cat .github/workflows/build-all.yml
Length of output: 8200
packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vue (1)
7-12: Verify consistent feature flagging across the application
The exchange features are being disabled for Safari using __IS_SAFARI__. Ensure this flag is consistently applied across all exchange-related features to prevent partial functionality.
Also applies to: 25-27, 47-47
✅ Verification successful
Let me modify the search to include the correct file type and also look for the Safari flag usage. This will help us verify the consistency of feature flagging across exchange-related components.
Safari-specific feature flag is correctly implemented
The __IS_SAFARI__ flag is consistently used across the codebase, and its usage for exchange features is limited to the network activity component. Other instances of the Safari flag are related to hardware wallet support, which is a separate concern. The exchange/swap functionality itself is not disabled in Safari - only its visibility in the network activity view is affected, which appears to be an intentional UI decision.
Key findings:
- Exchange features are present in multiple components but intentionally hidden only in network activity view for Safari
- Other
__IS_SAFARI__usages are properly scoped to hardware wallet compatibility - No inconsistencies found in feature flagging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other exchange-related components that might need similar Safari checks
rg -l "Buy|Swap|Exchange" --type vue
Length of output: 66
Script:
#!/bin/bash
# Search for files containing Buy/Swap/Exchange keywords and __IS_SAFARI__ flag
rg -l "Buy|Swap|Exchange" --type-add 'vue:*.vue' --type vue
# Also search for __IS_SAFARI__ usage
rg "__IS_SAFARI__" --type-add 'vue:*.vue' --type vue
Length of output: 3072
packages/extension/package.json (1)
19-19: LGTM: Safari build script follows established patterns
The new build script maintains consistency with existing browser-specific build commands and includes all necessary build steps.
packages/extension/src/ui/action/views/deposit/index.vue (1)
122-122: Verify modal positioning across browsers
The change from position: fixed to position: absolute may fix Safari-specific issues, but ensure the deposit modal remains correctly positioned in all supported browsers. The parent container must have position: relative for proper absolute positioning.
Consider adding a comment explaining the Safari-specific positioning requirement for future maintenance.
✅ Verification successful
Position change is appropriate for the modal implementation
The change from position: fixed to position: absolute is correct and well-implemented because:
- The deposit modal has proper z-index layering (z-index: 105-107) for the modal, overlay, and content
- The modal has specific positioning (left: -340px, top: 0px) relative to its container
- The overlay properly covers the entire modal area with absolute positioning
- The modal wrap has proper relative positioning and z-index stacking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parent container positioning to ensure proper absolute positioning context
rg -B 5 -A 5 "class=\"deposit\"" --type vue
Length of output: 71
Script:
#!/bin/bash
# Search for parent container positioning using ripgrep without file type filter
rg -B 5 -A 5 'class="deposit"'
Length of output: 730
Script:
#!/bin/bash
# Search for style definitions related to deposit modal
rg -B 5 -A 5 'position:?\s*(fixed|absolute|relative).*deposit'
Length of output: 64
Script:
#!/bin/bash
# Find all files that might contain deposit modal related code
fd -e vue -e js -e ts -e scss deposit
Length of output: 98
Script:
#!/bin/bash
# Search for style definitions in the deposit modal component
rg -B 10 -A 10 '\.deposit\s*{' packages/extension/src/ui/action/views/deposit/index.vue
Length of output: 179
Script:
#!/bin/bash
# Search for overlay and wrap styles
rg -B 5 -A 5 '(deposit__overlay|deposit__wrap)' packages/extension/src/ui/action/views/deposit/index.vue
Length of output: 462
Script:
#!/bin/bash
# Search for style definitions with fixed quotes
rg -B 5 -A 10 "\.deposit" packages/extension/src/ui/action/views/deposit/index.vue
Length of output: 363
Script:
#!/bin/bash
# Look for parent components that might provide positioning context
cat packages/extension/src/ui/action/views/deposit/index.vue
Length of output: 6890
packages/extension/src/ui/action/views/accounts/index.vue (1)
165-165: Verify compile-time constants
Ensure that __IS_FIREFOX__ and __IS_SAFARI__ constants are properly defined in the build configuration.
✅ Verification successful
Let's verify the actual definitions of these constants.
Constants are properly defined in build configuration
The compile-time constants __IS_FIREFOX__ and __IS_SAFARI__ are correctly defined in the Vite configuration:
__IS_FIREFOX__is set based onBROWSER === 'firefox'__IS_SAFARI__is set based onBROWSER === 'safari'
Both constants are also properly typed in env.d.ts as boolean values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constant definitions in build configuration
rg -l "__IS_FIREFOX__|__IS_SAFARI__" --type js --type ts
Length of output: 607
Script:
#!/bin/bash
# Check the constant definitions in vite.config.ts
cat packages/extension/vite.config.ts
# Also check type definitions
cat packages/extension/env.d.ts
Length of output: 3477
macos/Enkrypt/Enkrypt/Base.lproj/Main.storyboard (2)
81-85: Review window configuration for better user experience.
Several window configurations might impact user experience:
- The window is non-resizable with a fixed size of 720x500, which might not be optimal for different screen sizes and resolutions
- Full screen mode is disabled (
fullScreenNone) - The window is not released when closed, which could impact memory usage
Consider:
- Making the window resizable with minimum/maximum size constraints
- Enabling full screen support
- Reviewing the
releasedWhenClosedsetting
109-112: Verify Safari extension integration capabilities.
Since this PR adds Safari support, ensure that the web view configuration allows proper integration with Safari extensions.
Run the following to check for Safari extension integration files:
✅ Verification successful
Safari extension integration is properly configured
The web view configuration in the storyboard, combined with the implementation in ViewController.swift, shows proper Safari extension integration:
- WKWebView is correctly configured with extension support
- SafariServices framework is imported and utilized
- SFSafariExtensionManager is used to manage extension state
- Info.plist confirms Safari web extension conversion (SFSafariWebExtensionConverterVersion)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Safari extension configuration files
fd -e appex -e plist -e entitlements . | grep -i safari
# Check for Safari extension related code
rg -l "SFSafariApplication|WKWebView.*extension" --type swift
Length of output: 164
Script:
#!/bin/bash
# Let's examine the ViewController.swift to understand the Safari integration
cat macos/Enkrypt/Enkrypt/ViewController.swift
# Also check for any Safari extension related configurations in Info.plist files
fd -e plist . | xargs grep -l "safari\|Safari"
Length of output: 1871
Script:
#!/bin/bash
# Let's check the Info.plist content for Safari configuration
cat macos/Enkrypt/Enkrypt/Info.plist
# Also check for any additional Safari extension related files
fd . -e xcodeproj -e entitlements | xargs grep -l "safari\|Safari"
Length of output: 594
| <testcase classname="fastlane.lanes" name="10: upload_to_app_store" time="16.011041"> | ||
|
|
||
| <failure message="/Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/actions/actions_helper.rb:67:in `execute_action' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:255:in `block in execute_action' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:229:in `chdir' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:229:in `execute_action' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:157:in `trigger_action_by_name' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/fast_file.rb:159:in `method_missing' Fastfile:53:in `block in parsing_binding' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/lane.rb:33:in `call' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:204:in `try_switch_to_lane' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:146:in `trigger_action_by_name' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/fast_file.rb:159:in `method_missing' Fastfile:67:in `block in parsing_binding' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/lane.rb:33:in `call' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:49:in `block in execute' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:45:in `chdir' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:45:in `execute' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/lane_manager.rb:47:in `cruise_lane' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/command_line_handler.rb:36:in `handle' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/commands_generator.rb:110:in `block (2 levels) in run' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/command.rb:187:in `call' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/command.rb:157:in `run' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/runner.rb:444:in `run_active_command' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane_core/lib/fastlane_core/ui/fastlane_runner.rb:124:in `run!' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/delegates.rb:18:in `run!' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/commands_generator.rb:354:in `run' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/commands_generator.rb:43:in `start' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/cli_tools_distributor.rb:123:in `take_off' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/bin/fastlane:23:in `<top (required)>' /usr/local/bin/fastlane:23:in `load' /usr/local/bin/fastlane:23:in `<main>' Error uploading ipa file: " /> | ||
|
|
There was a problem hiding this comment.
Investigate the App Store upload failure
The upload to App Store step failed but the error message is empty. This could indicate an issue with error handling in the Fastlane configuration or insufficient permissions.
Please check:
- App Store Connect API key permissions
- Application binary validation
- Fastlane logs for detailed error messages
|
|
||
| <testcase classname="fastlane.lanes" name="10: upload_to_app_store" time="16.011041"> | ||
|
|
||
| <failure message="/Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/actions/actions_helper.rb:67:in `execute_action' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:255:in `block in execute_action' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:229:in `chdir' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:229:in `execute_action' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:157:in `trigger_action_by_name' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/fast_file.rb:159:in `method_missing' Fastfile:53:in `block in parsing_binding' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/lane.rb:33:in `call' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:204:in `try_switch_to_lane' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:146:in `trigger_action_by_name' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/fast_file.rb:159:in `method_missing' Fastfile:67:in `block in parsing_binding' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/lane.rb:33:in `call' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:49:in `block in execute' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:45:in `chdir' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/runner.rb:45:in `execute' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/lane_manager.rb:47:in `cruise_lane' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/command_line_handler.rb:36:in `handle' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/commands_generator.rb:110:in `block (2 levels) in run' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/command.rb:187:in `call' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/command.rb:157:in `run' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/runner.rb:444:in `run_active_command' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane_core/lib/fastlane_core/ui/fastlane_runner.rb:124:in `run!' /Library/Ruby/Gems/2.6.0/gems/commander-4.6.0/lib/commander/delegates.rb:18:in `run!' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/commands_generator.rb:354:in `run' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/commands_generator.rb:43:in `start' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/fastlane/lib/fastlane/cli_tools_distributor.rb:123:in `take_off' /Library/Ruby/Gems/2.6.0/gems/fastlane-2.212.2/bin/fastlane:23:in `<top (required)>' /usr/local/bin/fastlane:23:in `load' /usr/local/bin/fastlane:23:in `<main>' Error uploading ipa file: " /> |
There was a problem hiding this comment.
Security: Update Ruby version
The stack trace indicates usage of Ruby 2.6.0 (/Library/Ruby/Gems/2.6.0/), which has reached end-of-life and may have security vulnerabilities. Consider upgrading to a supported Ruby version.
Recommendations:
- Use Ruby version manager (rbenv/rvm) to manage Ruby versions
- Update to Ruby 3.x
- Update the CI environment to use the new Ruby version
| 8493952928AC474E009F7287 /* favicon.ico */ = {isa = PBXFileReference; lastKnownFileType = image.ico; name = favicon.ico; path = ../../../packages/extension/dist/favicon.ico; sourceTree = "<group>"; }; | ||
| 8493952A28AC474E009F7287 /* index.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; name = index.html; path = ../../../packages/extension/dist/index.html; sourceTree = "<group>"; }; | ||
| 8493952B28AC474E009F7287 /* css */ = {isa = PBXFileReference; lastKnownFileType = folder; name = css; path = ../../../packages/extension/dist/css; sourceTree = "<group>"; }; | ||
| 8493952C28AC474F009F7287 /* onboard.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; name = onboard.html; path = ../../../packages/extension/dist/onboard.html; sourceTree = "<group>"; }; | ||
| 8493952D28AC474F009F7287 /* js */ = {isa = PBXFileReference; lastKnownFileType = folder; name = js; path = ../../../packages/extension/dist/js; sourceTree = "<group>"; }; | ||
| 8493952E28AC474F009F7287 /* img */ = {isa = PBXFileReference; lastKnownFileType = folder; name = img; path = ../../../packages/extension/dist/img; sourceTree = "<group>"; }; | ||
| 8493952F28AC474F009F7287 /* trezor-usb-permissions.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; name = "trezor-usb-permissions.html"; path = "../../../packages/extension/dist/trezor-usb-permissions.html"; sourceTree = "<group>"; }; | ||
| 8493953028AC474F009F7287 /* scripts */ = {isa = PBXFileReference; lastKnownFileType = folder; name = scripts; path = ../../../packages/extension/dist/scripts; sourceTree = "<group>"; }; | ||
| 8493953128AC474F009F7287 /* manifest.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; name = manifest.json; path = ../../../packages/extension/dist/manifest.json; sourceTree = "<group>"; }; | ||
| 8493953228AC474F009F7287 /* action.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; name = action.html; path = ../../../packages/extension/dist/action.html; sourceTree = "<group>"; }; | ||
| 8493953328AC474F009F7287 /* assets */ = {isa = PBXFileReference; lastKnownFileType = folder; name = assets; path = ../../../packages/extension/dist/assets; sourceTree = "<group>"; }; | ||
| 8493953428AC474F009F7287 /* vendor */ = {isa = PBXFileReference; lastKnownFileType = folder; name = vendor; path = ../../../packages/extension/dist/vendor; sourceTree = "<group>"; }; |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect resource paths need to be updated
The project references files from ../../../packages/extension/dist/, but the output shows that:
- The
distdirectory doesn't exist in the expected location - Most referenced files are actually located in different directories:
- HTML files are in
packages/extension/root andsrc/ui/subdirectories favicon.icoandtrezor-usb-permissions.htmlare inpackages/extension/public/- Assets, images, and vendor files are scattered across different source directories
- HTML files are in
The paths in the Xcode project file need to be updated to point to the correct source locations, or the build process needs to be modified to generate the expected dist directory structure before the iOS build.
🔗 Analysis chain
Verify relative resource paths.
The extension resources are referenced using relative paths from a different package:
../../../packages/extension/dist/
Ensure these paths will resolve correctly during the build process and consider using a more robust resource management strategy.
Run the following script to verify the resource paths:
This script will help us verify:
- If the dist directory exists in the expected location
- If the specific files referenced in the project exist anywhere in the repository
- If the referenced directories exist anywhere in the repository
This will give us a better understanding of the resource structure and help determine if there's a potential path resolution issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all referenced resources exist
# Check if the extension dist directory exists and contains required files
fd --base-directory ../../../packages/extension/dist \
-e html -e ico -e json \
-E node_modules \
--exec echo "Found resource: {}"
Length of output: 221
Script:
#!/bin/bash
# Let's first check if the packages/extension/dist directory exists
fd -t d "extension/dist$" -d 3
# Then let's check for the existence of the specific files mentioned in the project.pbxproj
fd -t f "(favicon\.ico|index\.html|onboard\.html|trezor-usb-permissions\.html|action\.html|manifest\.json)$"
# Also check for the directories mentioned
fd -t d "^(css|js|img|scripts|assets|vendor)$"
Length of output: 1101
| Browser.tabs | ||
| .query({ active: true }) | ||
| .then(info => Browser.tabs.remove(info[0].id!)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety
While using the Browser API is the right approach for cross-browser compatibility, the current implementation needs more robust error handling and type safety.
Consider this safer implementation:
-Browser.tabs
- .query({ active: true })
- .then(info => Browser.tabs.remove(info[0].id!));
+Browser.tabs
+ .query({ active: true, currentWindow: true })
+ .then(tabs => {
+ if (tabs.length === 0) {
+ console.warn('No active tab found');
+ return;
+ }
+ const tabId = tabs[0].id;
+ if (typeof tabId === 'undefined') {
+ console.warn('Tab ID is undefined');
+ return;
+ }
+ return Browser.tabs.remove(tabId);
+ })
+ .catch(error => {
+ console.error('Error closing tab:', error);
+ });Changes:
- Added
currentWindow: truefor more precise tab targeting - Added proper error handling
- Removed unsafe non-null assertion
- Added type checking for tab ID
📝 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.
| Browser.tabs | |
| .query({ active: true }) | |
| .then(info => Browser.tabs.remove(info[0].id!)); | |
| Browser.tabs | |
| .query({ active: true, currentWindow: true }) | |
| .then(tabs => { | |
| if (tabs.length === 0) { | |
| console.warn('No active tab found'); | |
| return; | |
| } | |
| const tabId = tabs[0].id; | |
| if (typeof tabId === 'undefined') { | |
| console.warn('Tab ID is undefined'); | |
| return; | |
| } | |
| return Browser.tabs.remove(tabId); | |
| }) | |
| .catch(error => { | |
| console.error('Error closing tab:', error); | |
| }); |
| <img class="image" src="../Light.png" width="540" alt="Enkrypt's extension" /> | ||
| <img class="image-dark" src="../Dark.png" width="540" alt="Enkrypt's extension" /> | ||
| </div> | ||
| <button class="open-preferences">Open Safari Extensions Preferences</button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance button accessibility
The button lacks proper accessibility attributes and keyboard interaction handling.
- <button class="open-preferences">Open Safari Extensions Preferences</button>
+ <button
+ class="open-preferences"
+ aria-label="Open Safari Extensions Preferences"
+ role="button"
+ tabindex="0"
+ onclick="openPreferences()"
+ onkeypress="event.key === 'Enter' && openPreferences()"
+ >Open Safari Extensions Preferences</button>Committable suggestion skipped: line range outside the PR's diff.
| @@ -6,6 +6,7 @@ on: | |||
| - "v*" | |||
| jobs: | |||
| release: | |||
| name: new release | |||
There was a problem hiding this comment.
Add Safari build step to release workflow
The workflow is missing steps to build and package the Safari extension. This is necessary for the Safari support feature.
Add Safari build step after the Firefox build:
- name: build
env:
VIRUS_TOTAL_API_KEY: ${{secrets.VIRUS_TOTAL_API_KEY}}
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}
run: |
mkdir release
docker run --rm --user "$(id -u):$(id -g)" -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "yarn build:all"
docker run --rm --user "$(id -u):$(id -g)" -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "cd packages/extension && yarn build:chrome && yarn zip"
mv packages/extension/dist/release.zip release/enkrypt-chrome-edge-opera-${{ steps.get_release_tag.outputs.VERSION }}.zip
docker run --rm --user "$(id -u):$(id -g)" -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "cd packages/extension && yarn build:firefox && yarn zip"
mv packages/extension/dist/release.zip release/enkrypt-firefox-${{ steps.get_release_tag.outputs.VERSION }}.xpi
+ docker run --rm --user "$(id -u):$(id -g)" -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "cd packages/extension && yarn build:safari && yarn zip"
+ mv packages/extension/dist/release.zip release/enkrypt-safari-${{ steps.get_release_tag.outputs.VERSION }}.zipCommittable suggestion skipped: line range outside the PR's diff.
| build_mac_app(scheme: "Enkrypt", | ||
| export_team_id: CredentialsManager::AppfileConfig.try_fetch_value(:team_id), | ||
| destination: "generic/platform=macOS", | ||
| clean: true, | ||
| export_method: "app-store", | ||
| output_directory: "build", | ||
| installer_cert_name: ENV["INSTALLER_CERT"], | ||
| export_options: { | ||
| provisioningProfiles: { | ||
| "com.myetherwallet.enkrypt.safari" => ENV["SAFARI_PROVISION"], | ||
| "com.myetherwallet.enkrypt.safari.Extension" => ENV["EXTENSION_PROVISION"] | ||
| } | ||
| }) | ||
| upload_to_app_store(skip_metadata: true, skip_screenshots: true, run_precheck_before_submit: false) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for build process
The build process should handle potential failures gracefully, especially for provisioning profile issues which are common in macOS builds.
+ begin
build_mac_app(scheme: "Enkrypt",
export_team_id: CredentialsManager::AppfileConfig.try_fetch_value(:team_id),
destination: "generic/platform=macOS",
clean: true,
export_method: "app-store",
output_directory: "build",
installer_cert_name: ENV["INSTALLER_CERT"],
export_options: {
provisioningProfiles: {
"com.myetherwallet.enkrypt.safari" => ENV["SAFARI_PROVISION"],
"com.myetherwallet.enkrypt.safari.Extension" => ENV["EXTENSION_PROVISION"]
}
})
+ rescue FastlaneCore::Interface::FastlaneError => e
+ UI.error "Failed to build app: #{e.message}"
+ UI.error "Checking provisioning profiles..."
+ sh("security find-identity -v -p codesigning")
+ raise
+ end📝 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.
| build_mac_app(scheme: "Enkrypt", | |
| export_team_id: CredentialsManager::AppfileConfig.try_fetch_value(:team_id), | |
| destination: "generic/platform=macOS", | |
| clean: true, | |
| export_method: "app-store", | |
| output_directory: "build", | |
| installer_cert_name: ENV["INSTALLER_CERT"], | |
| export_options: { | |
| provisioningProfiles: { | |
| "com.myetherwallet.enkrypt.safari" => ENV["SAFARI_PROVISION"], | |
| "com.myetherwallet.enkrypt.safari.Extension" => ENV["EXTENSION_PROVISION"] | |
| } | |
| }) | |
| upload_to_app_store(skip_metadata: true, skip_screenshots: true, run_precheck_before_submit: false) | |
| end | |
| begin | |
| build_mac_app(scheme: "Enkrypt", | |
| export_team_id: CredentialsManager::AppfileConfig.try_fetch_value(:team_id), | |
| destination: "generic/platform=macOS", | |
| clean: true, | |
| export_method: "app-store", | |
| output_directory: "build", | |
| installer_cert_name: ENV["INSTALLER_CERT"], | |
| export_options: { | |
| provisioningProfiles: { | |
| "com.myetherwallet.enkrypt.safari" => ENV["SAFARI_PROVISION"], | |
| "com.myetherwallet.enkrypt.safari.Extension" => ENV["EXTENSION_PROVISION"] | |
| } | |
| }) | |
| rescue FastlaneCore::Interface::FastlaneError => e | |
| UI.error "Failed to build app: #{e.message}" | |
| UI.error "Checking provisioning profiles..." | |
| sh("security find-identity -v -p codesigning") | |
| raise | |
| end | |
| upload_to_app_store(skip_metadata: true, skip_screenshots: true, run_precheck_before_submit: false) | |
| end |
| @@ -55,7 +55,7 @@ hwWallet | |||
| networkName: route.query.network as NetworkNames, | |||
| }) | |||
| .then(paths => { | |||
| if (paths) isLedgerSupported.value = !__IS_FIREFOX__; | |||
| if (paths) isLedgerSupported.value = !(__IS_FIREFOX__ || __IS_SAFARI__); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document browser compatibility restrictions
The code disables hardware wallet support for both Firefox and Safari. Consider:
- Adding user-facing messaging explaining the browser limitations
- Documenting the technical reasons for these restrictions
+ const getBrowserCompatibilityMessage = () => {
+ if (__IS_SAFARI__) return "Hardware wallets are not supported in Safari due to WebUSB limitations";
+ if (__IS_FIREFOX__) return "Hardware wallets are not supported in Firefox due to U2F restrictions";
+ return null;
+ };Also applies to: 67-67
| @@ -0,0 +1,92 @@ | |||
| name: MacOS Safari Deploy | |||
|
|
|||
| on: [push] | |||
There was a problem hiding this comment.
Restrict workflow trigger to relevant events
Running the deployment workflow on every push could lead to unnecessary builds and potential App Store submission rate limits.
Consider limiting to specific branches or tags:
-on: [push]
+on:
+ push:
+ branches:
+ - main
+ - release/*
+ tags:
+ - 'v*'📝 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.
| on: [push] | |
| on: | |
| push: | |
| branches: | |
| - main | |
| - release/* | |
| tags: | |
| - 'v*' |
| <wkWebView wantsLayer="YES" fixedFrame="YES" translatesAutoresizingMaskIntoConstraints="NO" id="eOr-cG-IQY"> | ||
| <rect key="frame" x="0.0" y="0.0" width="720" height="500"/> | ||
| <autoresizingMask key="autoresizingMask" widthSizable="YES" heightSizable="YES"/> | ||
| <wkWebViewConfiguration key="configuration"> | ||
| <audiovisualMediaTypes key="mediaTypesRequiringUserActionForPlayback" none="YES"/> | ||
| <wkPreferences key="preferences"/> | ||
| </wkWebViewConfiguration> | ||
| </wkWebView> |
There was a problem hiding this comment.
Switch to Auto Layout and enhance web view security configuration.
- The web view uses a fixed frame with autoresizing mask instead of Auto Layout constraints, which might cause layout issues.
- The web view configuration lacks important security settings that are crucial for a crypto wallet application.
Consider:
- Replace fixed frame layout with Auto Layout:
-<wkWebView wantsLayer="YES" fixedFrame="YES" translatesAutoresizingMaskIntoConstraints="NO" id="eOr-cG-IQY">
+<wkWebView wantsLayer="YES" translatesAutoresizingMaskIntoConstraints="NO" id="eOr-cG-IQY">Add constraints in the view controller:
<constraints>
<constraint firstItem="eOr-cG-IQY" firstAttribute="leading" secondItem="m2S-Jp-Qdl" secondAttribute="leading" id="abc-12-xyz"/>
<constraint firstItem="eOr-cG-IQY" firstAttribute="trailing" secondItem="m2S-Jp-Qdl" secondAttribute="trailing" id="def-34-uvw"/>
<constraint firstItem="eOr-cG-IQY" firstAttribute="top" secondItem="m2S-Jp-Qdl" secondAttribute="top" id="ghi-56-rst"/>
<constraint firstItem="eOr-cG-IQY" firstAttribute="bottom" secondItem="m2S-Jp-Qdl" secondAttribute="bottom" id="jkl-78-opq"/>
</constraints>- Enhance web view security configuration:
<wkWebViewConfiguration key="configuration">
<audiovisualMediaTypes key="mediaTypesRequiringUserActionForPlayback" none="YES"/>
- <wkPreferences key="preferences"/>
+ <wkPreferences key="preferences">
+ <bool key="allowFileAccessFromFileURLs" value="NO"/>
+ <bool key="allowUniversalAccessFromFileURLs" value="NO"/>
+ <bool key="javaScriptCanOpenWindowsAutomatically" value="NO"/>
+ </wkPreferences>
+ <dataDetectorTypes key="dataDetectorTypes" none="YES"/>
</wkWebViewConfiguration>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/extension/src/manifest/manifest.safari.ts (1)
28-30: Consider using proper type checking for Safari manifest.Instead of using type assertion (
as ManifestV3Export), consider:
- Creating a Safari-specific manifest type that extends ManifestV3Export
- Adding runtime validation for Safari-specific manifest requirements
+interface SafariManifestV3 extends ManifestV3Export { + // Add Safari-specific typing requirements +} + -} as ManifestV3Export; +}: SafariManifestV3 = {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension/configs/vite/transform-manifest.ts(3 hunks)packages/extension/src/manifest/manifest.safari.ts(1 hunks)packages/extension/vite.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/configs/vite/transform-manifest.ts
🔇 Additional comments (6)
packages/extension/src/manifest/manifest.safari.ts (3)
5-18: Verify Safari icon assets and optimize sizes.
The manifest references multiple icon sizes in the 'safari-icons' directory. Please ensure:
- All referenced icon files exist in the correct location
- The icon sizes are actually required by Safari (some might be Chrome-specific)
19-27:
Review Safari permission compatibility and security implications.
Several concerns regarding the permissions:
- The broad
file://*/*and*://*/*host permissions might be rejected by Safari or require special handling - Some permissions like
unlimitedStorageand clipboard access might need additional entitlements in Safari - Consider limiting host permissions to only necessary domains for better security
✅ Verification successful
Let me check the content of the entitlement files to verify the Safari permissions configuration.
Safari permissions configuration is properly handled with appropriate entitlements and restrictions
The review concerns can be addressed based on the following findings:
- The extension has proper Safari-specific entitlements files that enforce app sandbox security model
- The manifest is specifically transformed for Safari with:
- Browser-specific version constraints (16.6 to 18.*)
- Special handling of content scripts registration
- The broad host permissions are necessary for the extension's functionality and are properly constrained by the app sandbox entitlements
The current implementation follows Safari's security model by:
- Using app sandbox (
com.apple.security.app-sandbox) - Limiting file access to user-selected files only (
com.apple.security.files.user-selected.read-only) - Properly configuring network access (
com.apple.security.network.client)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Safari-specific permission documentation and examples
rg -i "safari.*permission" --type md
rg -i "safari.*entitlement" --type md
# Look for any existing entitlements files
fd "\.entitlements$"
Length of output: 207
Script:
#!/bin/bash
# Check contents of entitlement files
cat macos/Enkrypt/Enkrypt/Enkrypt.entitlements
cat "macos/Enkrypt/Enkrypt Extension/Enkrypt_Extension.entitlements"
# Look for any Safari-specific configuration or documentation
rg -i "safari" --type=ts -A 5 -B 5
Length of output: 11580
1-4: 🛠️ Refactor suggestion
Rename operaManifest to safariManifest for clarity.
The variable name operaManifest is misleading as this file is specifically for Safari browser support. Additionally, spreading the entire chromeManifest might include Chrome-specific configurations that could cause issues in Safari.
-const operaManifest = {
+const safariManifest = {packages/extension/vite.config.ts (3)
23-24: LGTM! Safari case properly handled
The Safari case is correctly implemented in the getManifest function, following the same pattern as other browsers.
Line range hint 44-48: Consider Safari-specific build time requirements
The __BUILD_TIME__ configuration only has special handling for Firefox. Should Safari have similar special handling?
✅ Verification successful
Let me check the actual usage of BUILD_TIME to understand if Safari needs special handling.
No Safari-specific build time handling needed
The __BUILD_TIME__ is used only for two purposes:
- Firefox builds get a special static value 'FF-build'
- All other browsers use a timestamp format
Since Safari follows the default timestamp behavior like Chrome and Opera, and there's no evidence of Safari requiring special build time handling in the codebase, no changes are needed for the __BUILD_TIME__ configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any Safari-specific build requirements in the codebase
rg -l "BUILD_TIME|build.*time" --type ts --type js
Length of output: 193
Script:
#!/bin/bash
# Check the actual usage and implementation of BUILD_TIME
rg "BUILD_TIME" -A 5 -B 5 --type ts --type js
Length of output: 2380
Line range hint 71-76: Verify Safari compatibility with crx plugin configuration
The crx plugin configuration only distinguishes between 'firefox' and 'chrome'. Please verify if Safari requires any special handling in the crx plugin configuration.
| @@ -6,6 +6,7 @@ import { crx } from '@crxjs/vite-plugin'; | |||
| import chromeManifest from './src/manifest/manifest.chrome'; | |||
| import firefoxManifest from './src/manifest/manifest.firefox'; | |||
| import operaManifest from './src/manifest/manifest.opera'; | |||
| import safariManifest from './src/manifest/manifest.safari'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix incorrect manifest configuration and missing icons
There are several issues with the Safari manifest implementation:
- The manifest is incorrectly named
operaManifestinstead ofsafariManifest - It references non-existent Safari-specific icons in
assets/img/safari-icons/directory - The icon paths should use the common icons from
assets/img/icons/as defined in the base manifest
Specific issues:
- Rename the manifest variable from
operaManifesttosafariManifest - Update icon paths to use the common icons:
assets/img/icons/instead ofassets/img/safari-icons/ - Remove unnecessary icon sizes (19, 38, 72) that aren't used in other manifests
🔗 Analysis chain
Verify Safari manifest file existence and configuration
The import looks correct, but we should verify the Safari manifest file exists and contains the necessary configurations.
The Safari manifest exists but I need to verify:
- The Safari icons referenced in the manifest
- The Chrome manifest it's inheriting from to ensure proper configuration
I noticed the Safari manifest is actually named operaManifest and is using Safari-specific icons that don't exist. I need to verify the regular icons directory and check the base manifest before making a final assessment about potential issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Safari manifest exists and validate its content
if [ -f "packages/extension/src/manifest/manifest.safari.ts" ]; then
echo "Checking Safari manifest..."
cat "packages/extension/src/manifest/manifest.safari.ts"
else
echo "Safari manifest not found at expected location"
fi
Length of output: 1131
Script:
#!/bin/bash
# Check if the Safari icons exist
echo "Checking Safari icons existence..."
fd "icon" "packages/extension/src/assets/img/safari-icons"
# Compare with Chrome manifest to ensure proper inheritance
echo -e "\nChecking Chrome manifest for comparison..."
cat "packages/extension/src/manifest/manifest.chrome.ts"
Length of output: 1470
Script:
#!/bin/bash
# Check if the regular icons directory exists and contains the required icons
echo "Checking regular icons directory..."
fd "icon" "packages/extension/src/assets/img/icons"
# Check base manifest for complete context
echo -e "\nChecking base manifest..."
cat "packages/extension/src/manifest/manifest.base.ts"
Length of output: 1253
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
.github/workflows/macos-deploy.yml (1)
72-85: Document secrets usageWhile the secrets are properly used, adding documentation would help with maintenance.
Add comments explaining the purpose of each secret:
env: # Apple Developer account credentials APPLE_ID: "${{ secrets.OSX_APPLE_ID }}" APP_ID: "${{ secrets.OSX_APP_ID }}" # App Store Connect team configuration FASTLANE_ITC_TEAM_ID: "${{ secrets.OSX_FASTLANE_ITC_TEAM_ID }}" ITC_TEAM_ID: "${{ secrets.OSX_FASTLANE_ITC_TEAM_ID }}" # Certificates and provisioning profiles INSTALLER_CERT: "${{ secrets.OSX_INSTALLER_CERT }}" SAFARI_PROVISION: "${{ secrets.OSX_SAFARI_PROVISION }}" EXTENSION_PROVISION: "${{ secrets.OSX_EXTENSION_PROVISION }}" # Match configuration for certificate management MATCH_GIT_URL: "${{ secrets.OSX_MATCH_GIT_URL }}" MATCH_PASSWORD: "${{ secrets.OSX_MATCH_PASSWORD }}" # API authentication for App Store Connect API_KEY: "${{ secrets.OSX_API_KEY }}" API_KEY_ID: "${{ secrets.OSX_API_KEY_ID }}" API_ISSUER_ID: "${{ secrets.OSX_API_ISSUER_ID }}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
macos/Enkrypt/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/macos-deploy.yml(1 hunks)macos/Enkrypt/Enkrypt Extension/service-worker-loader.js(1 hunks)macos/Enkrypt/Enkrypt.xcodeproj/project.pbxproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- macos/Enkrypt/Enkrypt Extension/service-worker-loader.js
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/macos-deploy.yml
14-14: shellcheck reported issue in this script: SC2046:warning:2:41: Quote this to prevent word splitting
(shellcheck)
14-14: shellcheck reported issue in this script: SC2006:style:2:41: Use $(...) notation instead of legacy backticks ...
(shellcheck)
14-14: shellcheck reported issue in this script: SC2086:info:2:115: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:1:46: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2046:warning:1:20: Quote this to prevent word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2006:style:1:20: Use $(...) notation instead of legacy backticks ...
(shellcheck)
33-33: shellcheck reported issue in this script: SC2046:warning:2:47: Quote this to prevent word splitting
(shellcheck)
33-33: shellcheck reported issue in this script: SC2006:style:2:47: Use $(...) notation instead of legacy backticks ...
(shellcheck)
88-88: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/macos-deploy.yml (1)
3-3: Restrict workflow trigger to relevant events
Running the deployment workflow on every push could lead to unnecessary builds and potential App Store submission rate limits.
Consider limiting to specific branches or tags:
-on: [push]
+on:
+ push:
+ branches:
+ - main
+ - release/*
+ tags:
+ - 'v*'macos/Enkrypt/Enkrypt.xcodeproj/project.pbxproj (1)
59-82: Incorrect resource paths need to be updated
The project references files from ../../../packages/extension/dist/, but the dist directory may not exist in the expected location, or the files may be located elsewhere in the repository. Please verify that the resource paths are correct to ensure the build process can locate all necessary resources.
Run the following script to verify the existence and paths of the referenced resources:
#!/bin/bash
# Description: Verify the existence and paths of the referenced resource files and directories.
# Navigate to the repository root
cd "$(git rev-parse --show-toplevel)"
# List of resource files to check
resources=(
"service-worker-loader.js"
"favicon.ico"
"index.html"
"onboard.html"
"trezor-usb-permissions.html"
"action.html"
"manifest.json"
)
# Search for resource files
for resource in "${resources[@]}"; do
echo "Searching for $resource..."
fd -t f "$resource" --exec echo "Found: {}"
done
# List of directories to check
directories=(
"scripts"
"assets"
"vendor"
)
# Search for resource directories
for dir in "${directories[@]}"; do
echo "Searching for directory $dir..."
fd -t d "$dir" --exec echo "Found directory: {}"
done| run: | | ||
| docker build -t enkrypt-build-container . | ||
| echo "NODE_VERSION=$(docker run --rm -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "node --version")" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Fix shell script safety issues
The shell commands have several safety issues that could cause failures in edge cases.
Apply these fixes:
- docker build -t enkrypt-build-container .
- echo "NODE_VERSION=$(docker run --rm -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "node --version")" >> $GITHUB_OUTPUT
+ docker build -t enkrypt-build-container .
+ echo "NODE_VERSION=$(docker run --rm -v "$(pwd)":/home:rw enkrypt-build-container /bin/bash -c "node --version")" >> $GITHUB_OUTPUT
- run: docker run --rm -v `pwd`:/home:rw --user "$(id -u):$(id -g)" enkrypt-build-container /bin/bash -c "yarn install --silent"
+ run: docker run --rm -v "$(pwd)":/home:rw --user "$(id -u):$(id -g)" enkrypt-build-container /bin/bash -c "yarn install --silent"
- docker run --rm --user "$(id -u):$(id -g)" -v `pwd`:/home:rw enkrypt-build-container /bin/bash -c "cd packages/extension && yarn build:all && yarn build:safari"
+ docker run --rm --user "$(id -u):$(id -g)" -v "$(pwd)":/home:rw enkrypt-build-container /bin/bash -c "cd packages/extension && yarn build:all && yarn build:safari"Also applies to: 30-30, 34-35
🧰 Tools
🪛 actionlint (1.7.4)
14-14: shellcheck reported issue in this script: SC2046:warning:2:41: Quote this to prevent word splitting
(shellcheck)
14-14: shellcheck reported issue in this script: SC2006:style:2:41: Use $(...) notation instead of legacy backticks ...
(shellcheck)
14-14: shellcheck reported issue in this script: SC2086:info:2:115: Double quote to prevent globbing and word splitting
(shellcheck)
| uses: actions/upload-artifact@v3 | ||
| with: | ||
| name: safari-build-pkg | ||
| path: macos/Enkrypt/build/Enkrypt.pkg | ||
| retention-days: 1 |
There was a problem hiding this comment.
Update outdated GitHub Action
The actions/upload-artifact action is using an outdated version.
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4📝 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.
| uses: actions/upload-artifact@v3 | |
| with: | |
| name: safari-build-pkg | |
| path: macos/Enkrypt/build/Enkrypt.pkg | |
| retention-days: 1 | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: safari-build-pkg | |
| path: macos/Enkrypt/build/Enkrypt.pkg | |
| retention-days: 1 |
🧰 Tools
🪛 actionlint (1.7.4)
88-88: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: Deploy via Fastlane | ||
| uses: maierj/[email protected] | ||
| with: | ||
| lane: "buildandrelease" | ||
| subdirectory: "macos/Enkrypt" | ||
| env: | ||
| APPLE_ID: "${{ secrets.OSX_APPLE_ID }}" | ||
| APP_ID: "${{ secrets.OSX_APP_ID }}" | ||
| FASTLANE_ITC_TEAM_ID: "${{ secrets.OSX_FASTLANE_ITC_TEAM_ID }}" | ||
| FASTLANE_APPLE_APPLICATION_SPECIFIC_PASSWORD: "${{ secrets.OSX_FASTLANE_APPLE_APPLICATION_SPECIFIC_PASSWORD }}" | ||
| ITC_TEAM_ID: "${{ secrets.OSX_FASTLANE_ITC_TEAM_ID }}" | ||
| INSTALLER_CERT: "${{ secrets.OSX_INSTALLER_CERT }}" | ||
| SAFARI_PROVISION: "${{ secrets.OSX_SAFARI_PROVISION }}" | ||
| EXTENSION_PROVISION: "${{ secrets.OSX_EXTENSION_PROVISION }}" | ||
| MATCH_GIT_URL: "${{ secrets.OSX_MATCH_GIT_URL }}" | ||
| MATCH_PASSWORD: "${{ secrets.OSX_MATCH_PASSWORD }}" | ||
| GIT_AUTHORIZATION: "${{ secrets.OSX_GIT_AUTHORIZATION }}" | ||
| API_KEY: "${{ secrets.OSX_API_KEY }}" | ||
| API_KEY_ID: "${{ secrets.OSX_API_KEY_ID }}" | ||
| API_ISSUER_ID: "${{ secrets.OSX_API_ISSUER_ID }}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for deployment steps
The deployment steps lack explicit error handling which could make debugging failures difficult.
Consider adding:
- Timeout settings for the deployment step
- Retry logic for transient failures
- Error output capture
Example:
- name: Deploy via Fastlane
uses: maierj/[email protected]
with:
lane: "buildandrelease"
subdirectory: "macos/Enkrypt"
+ timeout-minutes: 30
+ continue-on-error: false
env:
APPLE_ID: "${{ secrets.OSX_APPLE_ID }}"
# ... other secrets ...
+ - name: Upload deployment logs on failure
+ if: failure()
+ uses: actions/upload-artifact@v4
+ with:
+ name: deployment-logs
+ path: macos/Enkrypt/fastlane/logs
+ retention-days: 5Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
.gitignoreto exclude additional build outputs and caches.Documentation