fix(env): stop shipping the development profile - #666
fix(env): stop shipping the development profile#666Alex Shabalin (alex-sparus) wants to merge 1 commit into
Conversation
🎨 Storybook previewUpdated for a105991
|
📋 PR Overview
🔬 Coverage
|
|
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
Hardcoded required: true (current) |
Consistent "FDA is required" framing after clicking grant | Advisory row may become blocking when the user acts on it | Low — one field in one match arm |
Mirror check_all_permissions logic |
Advisory row stays advisory after the grant action | No visible state change after clicking grant, which is correct but may feel unresponsive | Low |
Since the last review
- Still open: Callers outside the repository that pass old env names (
prod,dev,local) now get a hard build failure with no prior warning. (No PR comment, companion change, migration guide, or changelog entry covers external consumers of the old env name set. Only callers inside this repository are updated.) - Addressed in code: FDA inconclusive probe returned
Pending(implying not granted) when it could not determine the actual state. (check_full_disk_accessnow returns(PermissionStatus::Unknown, Some(explanation))when inconclusive, andcheck_all_permissionssetsperm.required = falsefor thefull-diskarm when status isUnknown.) - Still open: The
request_permissionaction for FDA returnsrequired: trueregardless of probe outcome, which could flip an advisory row to blocking when the user clicks "grant." (request_permissioninpermissions.rsstill constructsPermission { required: true, ... }with a hardcoded value even whencheck_full_disk_access()returnsUnknown.) - Still open: External repositories, partner CI, or published documentation may use
NIXMAC_ENV=prod,dev, orlocaland now get a hard build failure. (No PR comment, companion change, or changelog entry addresses external consumers of the old env name set.) - Still open: No plan exists to tell existing users that they will see the permissions banner for the first time on their next launch. (PR description shows a screenshot of the banner but checks "No docs update needed" and includes no in-app notice, release note, or changelog entry for existing users.)
Open questions
-
Does the UI merge the Permission object returned by
request_permissionback into the displayed permissions list? If so, a user whose FDA row is advisory (Unknown, non-blocking) sees it flip to blocking after clicking "grant," because the returned object hardcodesrequired: true. -
Do external repositories, partner CI pipelines, or published documentation set
NIXMAC_ENVtoprod,dev,local, or any value other thandevelopment,production, ore2e? Any such caller now gets a hard build failure with no prior warning. -
What is the plan to tell existing users that the permissions banner will appear for the first time on their next launch? Every prior production build shipped with the gate disabled, so this is a new experience for the entire user base.
Recommendation
Ship with changes
The core fix is correct and important: stopping the development profile from reaching production builds closes the bug that silently disabled the permissions gate in every prior release. Two issues need resolution before shipping. First, request_permission for FDA still hardcodes required: true in its response even when the probe is inconclusive, which can silently promote an advisory row to blocking the moment the user tries to act on it. Second, no user communication plan exists for a banner that every existing production user will see for the first time on their next launch.
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
request_permission("full-disk") always returns required: true, but this PR makes check_all_permissions dynamically set required: false when FDA status is Unknown (inconclusive probe).
apps/native/src-tauri/src/system/permissions.rs:553
1 finding(s) posted as inline comments.
2b74a48 to
0b2c779
Compare
This stack of pull requests is managed by jj-ryu. |
There was a problem hiding this comment.
apps/native/src-tauri/src/system/permissions.rs (line 550) [behavior_change]: request_permission("full-disk") still hardcodes required: true unconditionally, while check_all_permissions now applies if status == PermissionStatus::Unknown { perm.required = false } (lines 378–382). When a user clicks the "request" button in an inconclusive FDA state, the Settings → Permissions UI receives a Permission with required: true, which can re-trigger the amber permissions banner the PR intends to suppress. The fix is to mirror the same guard in the "full-disk" arm of request_permission: compute required: status != PermissionStatus::Unknown instead of hardcoding true.
0b2c779 to
86e719f
Compare
Prelint Review SummaryActive findings (1)Warning
1 active, 0 resolved |
86e719f to
03c9e56
Compare
03c9e56 to
7bdb7cc
Compare
7bdb7cc to
a823d8d
Compare
|
Prelint reached the review limit for this pull request (5 reviews of each kind). New pushes do not start a review.
|
Scott McMaster (scottmcmaster)
left a comment
Scott McMaster (scottmcmaster)
left a comment
There was a problem hiding this comment.
Mostly just suggestions about consolidating some distributed but similar/repetitive inline comments.
| )] | ||
| pub sentry_dsn: String, | ||
|
|
||
| /// No `build_embed`: `NIXMAC_ENV` selects which profile to embed, so |
There was a problem hiding this comment.
I think this is maybe one of those comments that got added as part of the PR but once the PR is merged isn't really important to future maintenance, in which case I suggest removing (most of) it.
There was a problem hiding this comment.
Thanks! Shortened to briefly explain why there's no build_embed
| } | ||
|
|
||
| /// JSON profile from `apps/native/env.{development,release,e2e}.json`, embedded at compile time. | ||
| /// |
There was a problem hiding this comment.
Same as previous comment about comment.
| None => { | ||
| return ( | ||
| PermissionStatus::Unknown, | ||
| Some("Could not determine Full Disk Access: this Mac reported no home directory, so the check could not run. If a rebuild fails with a permissions error, add nixmac under System Settings → Privacy & Security → Full Disk Access.".to_string()), |
There was a problem hiding this comment.
Since you have FULL_DISK_ACCESS_INCONCLUSIVE as a const above suggest this message string should also be a const for symmetry.
There was a problem hiding this comment.
Also factored out the common parts between those messages
| use std::process::Command; | ||
|
|
||
| /// Embed `apps/native/env.{development,release,e2e}.json` selected by `NIXMAC_ENV`. | ||
| /// |
There was a problem hiding this comment.
This is another comment that can probably be cut down to just what's needed later.
There was a problem hiding this comment.
I've adjusted the comments to focus on the future maintenance
| .unwrap_or_else(|error| panic!("cannot read {}: {error}", path.display())); | ||
| let value: serde_json::Value = serde_json::from_str(&json) | ||
| .unwrap_or_else(|error| panic!("cannot parse {}: {error}", path.display())); | ||
| // The selector picks the file; the file names the same environment in its own |
There was a problem hiding this comment.
After seeing related comments in several places I wonder if maybe they should just all be consolidated to a single location in README or another md file as design documentation.
There was a problem hiding this comment.
I'm more afraid of those docs becoming stale
a823d8d to
490dc5e
Compare
|
Review comments addressed and also added an extra CI check that the |
A Vite define is text substitution, so passing an object emitted an object literal where a string was expected; JSON.parse threw and the catch silently loaded env.development.json. Every release therefore ran its frontend with VITE_NIXMAC_SKIP_PERMISSIONS on, disabling the permissions gate. Confirmed against a CI production bundle. - one object-valued define, validated and coerced at build time, and no fallback: a bad profile fails the build, or throws at startup - NIXMAC_ENV only selects a profile and no longer overwrites the selected file's own value; unknown selectors fail the build instead of falling through to development - a prod profile refuses the skip-permissions and nix-installed bypasses whatever the profile asks for - Rust resolves the environment through one path instead of three - prerequisite steps are gated on onboarding being unfinished, so a missing permission after setup is a banner, not an internal error - an inconclusive Full Disk Access probe no longer counts against the required-permission gate
490dc5e to
a105991
Compare
2 participants
Summary
Every shipped release ran its frontend on
env.development.json, withVITE_NIXMAC_SKIP_PERMISSIONSon and the permissions gate disabled. The Rustside was unaffected, so backend logs stayed honest while the UI acted as if
permissions were fine — which is why this survived so long. Confirmed against a
CI production bundle.
Each problem below is paired with what replaces it.
The bundled profile
Problem.
__NIXMAC_PROFILE_JSON__was defined asJSON.stringify(merged),an object. A Vite
defineis raw text substitution, so the bundle got an objectliteral where a string literal was meant.
Solution. One object-valued define, validated and coerced at build time.
No JSON round trip, so there is no parse for anything to fail.
Problem.
JSON.parsethrew on that object and acatchsilently loaded thedevelopment profile. The failure had no symptom.
Solution. No fallback. A bad profile fails the build, or throws at startup.
Substituting a different profile is exactly how this shipped.
The selector
Problem.
NIXMAC_ENVwas both the file selector and an overridable profilekey, so
NIXMAC_ENV=prodoverwroteenv.release.json's own"production". Thetwo disagreed on every build.
Solution. The selector no longer writes the key. One vocabulary —
development,production,e2e— names both the accepted selector values andeach profile's own
NIXMAC_ENV.Problem. An unrecognised selector fell through to the development profile, so
a typo produced a build that looked like a release and carried development
settings.
Solution. Unknown selectors fail the build, in
nixmac-profile.tsandbuild.rsalike.Problem. Nothing checked that the selected file names the environment that
selected it. A profile mislabelled
developmentwould pass every existing check.Solution. Both selectors refuse a file whose own
NIXMAC_ENVis not thevalue that picked it.
The gates
Problem. A profile could switch off gates that exist for real users, and the
mislabelling above would have been enough to permit it.
Solution. A
productionprofile refuses the skip-permissions andnix-installed bypasses whatever the profile asks for.
Problem. With the gate live,
computeCurrentStepreturnspermissionsornix-setupfor a completed user whose prerequisite is missing — steps the widgethas no case for, which it reports as an internal error.
Solution. Both branches are gated on onboarding being unfinished. After
setup, a missing prerequisite is a repair banner, which is what the wizard's
completion latch already implies.
Problem.
check_full_disk_accessreturnedPendingwhen none of its probepaths existed, which establishes nothing. That row is required, so users who may
well have granted FDA would see a blocking banner.
Solution. An inconclusive probe returns
Unknown, drops the required flagfor that row only, and explains itself in the row's instructions.
The Rust side
Problem. The deployment environment resolved through three paths, and the
startup log resolved the version differently from telemetry, so the two could
disagree.
Solution. One path.
crate::envowns the precedence and both the log lineand telemetry read through it.
Behaviour changes to expect
developmentin everyrelease; it now reports
production.env.release.jsonalso declaresproductionrather than the oldprodselector token, so the Rust-side valueis unchanged and Sentry's existing environment series is continuous.
unknownin every release (the fallback profilehas no
NIXMAC_VERSION); now the real version. Sentry/PostHog series will stepat this release.
the version string, which finally changes.
Existing users may see a permissions banner for the first time.
NIXMAC_ENV=prodis no longer accepted; useproduction. All in-repocallers are updated.
productionwas already accepted before this change, soany external caller using it keeps working.
matching what telemetry already did. A release bundle launched from inside the
devenv shell logs
local-$(whoami)rather than the embedded version.Test Plan
bunx tsc --noEmitandcargo checkcleanbun run test:unit— 52 files, 351 tests;cargo test— 843 passedbun run gen:schemasproduces no diffenv.test.tscovers the bypass lockout per profile and asserts thebaked profile round-trips; new
utils.test.tscases cover both prerequisitebranches completed and not
temp-dir fixtures,
build.rsby doctoring a profile file (mislabelled,wrong type, and absent key each produce a distinct message)
never run in a shipped build
Docs