Skip to content

feat: .env UI editor#8520

Open
royendo wants to merge 15 commits intomainfrom
feat/env-ui-editor
Open

feat: .env UI editor#8520
royendo wants to merge 15 commits intomainfrom
feat/env-ui-editor

Conversation

@royendo
Copy link
Contributor

@royendo royendo commented Dec 16, 2025

Before Deploy: Screenshot 2025-12-17 at 13 25 34

After Deploy: Screenshot 2025-12-17 at 13 26 54

Code view Before Deploy:
Screenshot 2025-12-17 at 14 37 39

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
Copy link
Contributor Author

royendo commented Dec 16, 2025

where to consolidate utils?
web-common/src/features/environment-variables/utils.ts
and
web-admin/src/features/projects/environment-variables/utils.ts

have the same functions and lots of repeat since they are the same functionality

@royendo
Copy link
Contributor Author

royendo commented Dec 17, 2025

Before Deploy: Screenshot 2025-12-17 at 13 25 34

After Deploy: Screenshot 2025-12-17 at 13 26 54

Code view Before Deploy:
Screenshot 2025-12-17 at 14 37 39

looks like we didnt habe this already so claude code/cursor did some work and i rebuild proto...
since its new feature it shouldnt effect anything else
@royendo royendo requested a review from mindspank December 17, 2025 19:17
@royendo
Copy link
Contributor Author

royendo commented Dec 17, 2025

@begelundmuller , based on Mike's requests to add push-pull button in the UI here: https://rilldata.slack.com/archives/C01A9DYP013/p1765989576246559?thread_ts=1765901488.470909&cid=C01A9DYP013

required some new backend changes, so tagging you as a reviewer for this PR, as well.

If this is "too complicated" or adds brittleness to the code base, we can remove the backend changes and buttons for this PR and tackle it in a dedicated PR in the future.

Thoughts?

Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

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

The backend code here seems correct at a high level, although it could do with some cleaning up (there are some anti-patterns and no tests)

@royendo
Copy link
Contributor Author

royendo commented Jan 20, 2026

Code Review

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

Strengths:

  • Comprehensive implementation of PullEnv and PushEnv RPC handlers
  • Good use of maps.Equal for checking if variables are up to date
  • Proto definitions are clean with proper comments

Issues to Address:

  1. Error handling could be more specific in cli/pkg/local/server.go:717:

    return nil, fmt.Errorf("failed to write .env file: %w", err)

    Consider distinguishing between permission errors and other I/O errors.

  2. Default environment assumption - The PullEnv defaults to "dev" which may surprise users expecting "prod".

E2E Testing: ⚠️ Missing

No E2E tests for pull/push env functionality.

Recommendations:

  • Add E2E test for rill env pull CLI command
  • Test conflict resolution when local and cloud variables differ
  • Test authentication required scenarios

General Comments:

  • The merge strategy (local first, then cloud overwrites) is clear and documented in the request/response messages
  • Good use of EnsureGitignoreHasDotenv for security

@royendo
Copy link
Contributor Author

royendo commented Jan 22, 2026

Just spoke to Mike and got confirmed that we want this in UI

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