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

Turn on Share Link in nightly builds #5153

Open
wants to merge 60 commits into
base: main
Choose a base branch
from

Conversation

pierremtb
Copy link
Collaborator

@pierremtb pierremtb commented Jan 27, 2025

Fixes #5136

Whole loop (share link, open in browser, open in desktop app) tested on installed apps on:

  • Windows
  • macOS
  • Linux (left out of this PR)

How to test: Grab a build from https://github.com/KittyCAD/modeling-app/actions/runs/13080529925 (just one commit before current HEAD), will point to this branch's vercel deployment, as it includes a quick fix there that app.dev.zoo.dev doesn't have.

Copy link

qa-wolf bot commented Jan 27, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Jan 27, 2025

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

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Jan 31, 2025 9:16pm

@pierremtb pierremtb changed the title Pierremtb/issue5136 turn on link sharing in released apps with electron builder WIP: turn on Share command for releases Jan 28, 2025
package.json Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
@pierremtb pierremtb changed the title Turn on Share Link on Nightly builds Turn on Share Link in nightly builds Jan 31, 2025
Comment on lines -12 to -19
osxSign: (process.env.BUILD_RELEASE === 'true' && {}) || undefined,
osxNotarize:
(process.env.BUILD_RELEASE === 'true' && {
appleId: process.env.APPLE_ID || '',
appleIdPassword: process.env.APPLE_PASSWORD || '',
teamId: process.env.APPLE_TEAM_ID || '',
}) ||
undefined,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking the change to remove this, as we're not using forge for codesign anymore

Comment on lines -22 to -28
protocols: [
{
name: 'Zoo Studio',
schemes: ['zoo-studio'],
},
],
extendInfo: 'Info.plist', // Information for file associations.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to electron-builder.yml, that we're using to package the app. This was added before the migration to builder.

@@ -68,8 +68,6 @@ export const KCL_DEFAULT_DEGREE = `360`
/** localStorage key for the playwright test-specific app settings file */
export const TEST_SETTINGS_FILE_KEY = 'playwright-test-settings'

export const DEFAULT_HOST = 'https://api.zoo.dev'
export const PROD_APP_URL = 'https://app.zoo.dev'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this one to an env variable controlled by vite on build, this way we can flip between dev and prod easier

@@ -68,8 +68,6 @@ export const KCL_DEFAULT_DEGREE = `360`
/** localStorage key for the playwright test-specific app settings file */
export const TEST_SETTINGS_FILE_KEY = 'playwright-test-settings'

export const DEFAULT_HOST = 'https://api.zoo.dev'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find any use for this so I 🔪d it

@@ -145,7 +143,7 @@ export const VIEW_NAMES_SEMANTIC = {
export const SIDEBAR_BUTTON_SUFFIX = '-pane-button'

/** Custom URL protocol our desktop registers */
export const ZOO_STUDIO_PROTOCOL = 'zoo-studio:'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one of the biggest blockers to working deep links.

{
name: 'share-file-link',
displayName: 'Share file',
hide: IS_NIGHTLY_OR_DEBUG ? undefined : 'desktop',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly only for now until we can make sure it all works in prod.

@@ -191,7 +192,7 @@ function ProjectMenuPopover({
id: 'share-link',
Element: 'button',
children: 'Share link to file',
disabled: !DEV,
disabled: IS_NIGHTLY_OR_DEBUG || !findCommand(shareCommandInfo),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly only for now until we can make sure it all works in prod.

@@ -33,7 +33,7 @@ export const OpenInDesktopAppHandler = (props: React.PropsWithChildren) => {
function onOpenInDesktopApp() {
const newSearchParams = new URLSearchParams(globalThis.location.search)
newSearchParams.delete(ASK_TO_OPEN_QUERY_PARAM)
const newURL = `${ZOO_STUDIO_PROTOCOL}${globalThis.location.pathname.replace(
const newURL = `${ZOO_STUDIO_PROTOCOL}://${globalThis.location.pathname.replace(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing from the web app in order to properly open the app. Might be related to the change in the ZOO_STUDIO_PROTOCOL var too.

@@ -103,11 +103,11 @@
"make:dev": "make dev",
"generate:machine-api": "npx openapi-typescript ./openapi/machine-api.json -o src/lib/machine-api.d.ts",
"tron:start": "electron-forge start",
"tron:package": "electron-forge package",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this in favor of a more widespead use of electron-builder for packaged apps. Trying to make it easier for people to run 'the prime packaging' path that we actually deploy.

Comment on lines +1 to +2
NODE_ENV=production
DEV=false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with .env.development

@pierremtb pierremtb marked this pull request as ready for review January 31, 2025 21:02
Comment on lines +41 to +54
// default vite values based on mode
process.env.NODE_ENV ??= viteEnv.MODE
process.env.DEV ??= viteEnv.DEV + ''
process.env.BASE_URL ??= viteEnv.VITE_KC_API_BASE_URL
process.env.VITE_KC_API_WS_MODELING_URL ??= viteEnv.VITE_KC_API_WS_MODELING_URL
process.env.VITE_KC_API_BASE_URL ??= viteEnv.VITE_KC_API_BASE_URL
process.env.VITE_KC_SITE_BASE_URL ??= viteEnv.VITE_KC_SITE_BASE_URL
process.env.VITE_KC_SITE_APP_URL ??= viteEnv.VITE_KC_SITE_APP_URL
process.env.VITE_KC_SKIP_AUTH ??= viteEnv.VITE_KC_SKIP_AUTH
process.env.VITE_KC_CONNECTION_TIMEOUT_MS ??=
viteEnv.VITE_KC_CONNECTION_TIMEOUT_MS

// Likely convenient to keep for debugging
console.log('process.env', process.env)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of default prod values for packaged apps (outside of the repo dev env), we let the vite mode assign the right ones. This way we can package dev and prod apps.

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.

Turn on link sharing in released apps with electron-builder
1 participant