Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: mas #2692

Draft
wants to merge 13 commits into
base: dev
Choose a base branch
from
Draft

feat: mas #2692

wants to merge 13 commits into from

Conversation

DIYgod
Copy link
Member

@DIYgod DIYgod commented Feb 5, 2025

Description

PR Type

  • Feature
  • Bugfix
  • Hotfix
  • Other (please describe):

Screenshots (if UI change)

Demo Video (if new feature)

Linked Issues

Additional context

Changelog

  • I have updated the changelog/next.md with my changes.

Copy link

vercel bot commented Feb 5, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
follow ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 2:25pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
follow-external-ssr ⬜️ Ignored (Inspect) Visit Preview Feb 6, 2025 2:25pm

@follow-reviewer-bot
Copy link

Suggested PR Title:

feat: add macOS App Store support to build process

Change Summary:
Enhanced macOS build process by adding support for Mac App Store (MAS) packaging. Modified GitHub Actions workflow to handle additional certificate and package uploads, updated Forge config to include MAS-specific configurations, and refined Turbo tasks for better management of build outputs.

Code Review:

Code Review - Change Requests

1. .github/workflows/build.yml

  • Lines 93–97:
    The addition of a new BUILD_CERTIFICATE_MAS_BASE64 secret and the command echo -n "$BUILD_CERTIFICATE_MAS_BASE64" assumes this secret has already been configured in the repository settings. Verify that this secret exists in the repository's GitHub Actions secrets configuration. If not, this will cause the workflow to fail, and the secret must be added before merging.

  • Line 121:
    The npm exec turbo run //#build:mas command assumes that the //build:mas target is always defined in the turbo.json configuration. This dependency must be clarified to avoid potential runtime errors if it is missing or misconfigured. Ensure the appropriate checks or setups are communicated in the PR description.

2. forge.config.cts

  • Lines 168–173:
    The MakerPKG configuration specifies "mas" as the target platform. This implies a dependency on Electron's Mac App Store (MAS) configuration and provisioning. Ensure that the proper MAS entitlement files and provisioning profiles are already in place and that this configuration has been tested against MAS-specific requirements. Without these prerequisites, the app bundling for MAS will likely fail or be rejected during submission.

3. No Proper Documentation Included

  • Across all files:
    The changes (e.g., new MAS support, MakerPKG integration) introduce new functionality but lack corresponding updates to any relevant documentation. Update the project's README or any documentation files to include instructions or notes about:
    • The added BUILD_CERTIFICATE_MAS_BASE64 secret.
    • The process to configure MAS-related provisioning and entitlements.
    • How to use the new //build:mas Turbo command.

Failure to document these changes may lead to confusion for other contributors or future maintainers.


Summary of Changes Required:

  1. Confirm and ensure the BUILD_CERTIFICATE_MAS_BASE64 secret exists in the repository secrets.
  2. Verify the availability and proper setup of MAS-specific configurations (provisioning profiles, entitlement files).
  3. Update project documentation to include details about the newly added functionality.

Please address these issues before merging.

@DIYgod DIYgod changed the title Feature/mas feature: mas Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant