Skip to content

Conversation

@sarahhjchung
Copy link
Contributor

Changes made:

  • This PR isolates the changes made to the environment variables from 2nd WIP android branch #653
  • The new environment variable will be added to Netlify and necessary docs (README.md or CONTRIBUTING.md) will be updated to reflect the changes

@sarahhjchung sarahhjchung marked this pull request as draft April 15, 2025 18:11
@netlify
Copy link

netlify bot commented Apr 15, 2025

Deploy Preview for fxms-skylight ready!

Name Link
🔨 Latest commit a460216
🔍 Latest deploy log https://app.netlify.com/sites/fxms-skylight/deploys/67fec22f366ed10009fc53e5
😎 Deploy Preview https://deploy-preview-662--fxms-skylight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (🟢 up 5 from production)
Accessibility: 88 (🔴 down 1 from production)
Best Practices: 92 (🟢 up 9 from production)
SEO: 100 (🟢 up 10 from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor Author

@sarahhjchung sarahhjchung left a comment

Choose a reason for hiding this comment

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

I've removed TODO.md and MOBILE-EPICS.md and cleaned up some comments. Overall the changes made to the environment variables look good and I've only made one change to the code!

app/fetchData.ts Outdated
const isLookerEnabled = process.env.IS_LOOKER_ENABLED === "true";

export async function fetchData(platform: Platform) {
const recipeCollection = new NimbusRecipeCollection(true, platform); //XXX YYY
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting the first parameter as false instead so that we are showing live experiments and rollouts as it seems we are leaving the completed page as is for now.

totalRolloutExperiments,
};
}
export async function getMsgExpRecipeCollection(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding missing JSDocs for functions in this file to make them more readable

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

app/types.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this empty file

@sarahhjchung sarahhjchung requested a review from AllegroFox April 15, 2025 19:47
@sarahhjchung
Copy link
Contributor Author

sarahhjchung commented Apr 15, 2025

@AllegroFox I've added the environment variable to Netlify here. Do you think it makes sense that I only need to add it to the production context? It seems like the deploy previews and local branches are working fine using the .env and .env.local files I think

@sarahhjchung sarahhjchung marked this pull request as ready for review April 15, 2025 19:52
@sarahhjchung sarahhjchung changed the title [WIP] Environment variable changes in Android PR Update experimenter api environment variable (as part of Android PR) Apr 15, 2025
@AllegroFox
Copy link
Collaborator

@AllegroFox I've added the environment variable to Netlify here. Do you think it makes sense that I only need to add it to the production context? It seems like the deploy previews and local branches are working fine using the .env and .env.local files I think

That makes sense to me from my understanding of Netlify!

@sarahhjchung
Copy link
Contributor Author

Actually I realized that I also have to add it to the deploy preview context as well as future builds were failing 😅

Copy link
Collaborator

@AllegroFox AllegroFox left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentional that this file and copilot-instructions.md have the same contents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the commit 374cd7a when these docs were added, they look to be intentionally the same. So I'll leave them as is and confirm with Dan!

totalRolloutExperiments,
};
}
export async function getMsgExpRecipeCollection(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@sarahhjchung sarahhjchung merged commit ea97e49 into main Apr 16, 2025
6 checks passed
@sarahhjchung sarahhjchung deleted the env-var branch April 16, 2025 13:46
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.

4 participants