Skip to content

Conversation

@royendo
Copy link
Contributor

@royendo royendo commented Jan 14, 2026

follow up to #8636

https://www.loom.com/share/5041c12bf37740f190eac80b66fbc4e8

want to add fallback to OS vars and add warning in UI for such

Screenshot 2026-01-13 at 18 54 19 Screenshot 2026-01-13 at 19 07 24

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@royendo royendo changed the title feat: Env new behavior phase 2: OS Fallback and alert feat: part 2 Env new behavior: OS Fallback and alert Jan 14, 2026
@royendo royendo marked this pull request as draft January 16, 2026 16:58
@royendo
Copy link
Contributor Author

royendo commented Jan 20, 2026

Code Review

Professionalism & Code Quality: ⭐⭐⭐⭐ (Good)

Strengths:

  • Good addition of OsEnvVariables field to track which variables came from OS environment
  • Clean proto definition with proper documentation

Issues to Address:

  • The proto changes suggest there's UI work to display warnings, but I only see the proto changes in this diff. Ensure the frontend warning display is included or tracked separately.

E2E Testing: ⚠️ Missing

No E2E tests visible in this diff.

Recommendations:

  • Add E2E test verifying the warning is displayed when OS env variables are used
  • Test that the warning helps users understand they need to add variables to .env for deployment

General Comments:

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.

2 participants