Update "Setting up your device" page for MacOS Setup Experience - #33770
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #33770 +/- ##
=======================================
Coverage 64.00% 64.00%
=======================================
Files 2068 2070 +2
Lines 207230 207319 +89
Branches 6861 6788 -73
=======================================
+ Hits 132642 132702 +60
- Misses 64157 64182 +25
- Partials 10431 10435 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This file is the only backend change, to allow the host setup status API to return scripts info. Currently the array will have at most one entry, but no reason not to future-proof it for a world of multiple setup scripts.
There was a problem hiding this comment.
FWIW given that nopkg is landing, it looks like the plan is to have "scripts that behave as software" so the scripts section would actually go away here.
@rachaelshaw does that sound right? When is that work expected to land?
In any event, if we keep this result in we need updates to the contributor API docs to reflect the revised shape of the response payload. Looks like the existing response is wrong anyway given that it it shows script so there's a 100% chance we need to change that endpoint. This can/probably should be its own PR so we're not needing to get two different sets of codeowners to simultaneously approve a thing.
There was a problem hiding this comment.
In any event, if we keep this result in we need updates to the contributor API docs to reflect the revised shape of the response payload. Looks like the existing response is wrong anyway given that it it shows
scriptso there's a 100% chance we need to change that endpoint. This can/probably should be its own PR so we're not needing to get two different sets of codeowners to simultaneously approve a thing.
This is a different API (which is not documented at all AFAICT). It tripped me up too. The one you're referring to is:
POST /api/fleet/orbit/setup_experience/status
but this one is
POST /api/fleet/device/{token}/setup_experience/status
There was a problem hiding this comment.
Mainly renaming and adding mock setup script data.
There was a problem hiding this comment.
New component to render the name of a setup script in the table. Mostly a copy of the software version, with a different image and text.
| .graphic { | ||
| width: $pad-xlarge; | ||
| scale: 60%; | ||
| } |
There was a problem hiding this comment.
Open to other ideas here. The width matches the one added for the software icon so that all the icons / graphics can be centered with each other. Since it's an SVG with a defined width in the markup, scale is the only way to resize with CSS afaik.
There was a problem hiding this comment.
New component to render the status of a setup script in the table. Mostly a copy of the Software version, with some different labels.
| // Sort the statuses so that scripts are always at the bottom. | ||
| statuses.sort((a, b) => { | ||
| if (a.type === b.type) { | ||
| return 0; | ||
| } | ||
| if (a.type === "script") { | ||
| return 1; | ||
| } | ||
| return -1; | ||
| }); |
There was a problem hiding this comment.
Make sure that scripts show up last.
| isAllPagesSelected={false} | ||
| disableTableHeader={false} | ||
| disablePagination | ||
| manualSortBy |
There was a problem hiding this comment.
Don't sort (since we did it above).
| if (type === "software") { | ||
| return <SetupSoftwareProcessCell name={name || "Unknown software"} />; | ||
| } | ||
| if (type === "script") { | ||
| return <SetupScriptProcessCell name={name || "Unknown script"} />; | ||
| } |
There was a problem hiding this comment.
Render the right type of process cell based on type.
| if (type === "software") { | ||
| return <SetupSoftwareStatusCell status={status || "pending"} />; | ||
| } | ||
| if (type === "script") { | ||
| return <SetupScriptStatusCell status={status || "pending"} />; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Render the right type of status cell based on type.
| .empty-table__container { | ||
| max-width: 100%; | ||
| } |
There was a problem hiding this comment.
To center the content
| const SetupScriptProcessCell = ({ name }: ISetupScriptProcessCell) => { | ||
| return ( | ||
| <span className={baseClass}> | ||
| <Graphic name="file-sh" className={`${baseClass}__icon`} /> |
There was a problem hiding this comment.
I grabbed this from the Setup Experience admin UI; we don't have an <Icon> for it.
iansltx
left a comment
There was a problem hiding this comment.
Couple of test naming nits, plus the whole "we need to document contributor API changes, but that can be its own PR" thing.
We do need to talk through setup experience scripts as their own thing as based on my understanding of setup experience sort-term roadmap the intent is to replace those with nopkg for arbitrary ordering (vs. always having scripts last), so while the API changes we're making now are rather lightweight we'll be tossing them rather soon.
Cleaning scripts up so we don't have three ways to run scripts (script entities, setup experience scripts, nopkg) feels like something we need to do in Fleet 5 at the latest (I don't think we need to do it here, and given that we need the script for macOS setup experience, fair enough to include it here in a way that requires fewer FE changes to consume). /cc @rachaelshaw
Once the test nits are resolved and there's a parallel PR for contributor docs updates, I'll 👍 this.
There was a problem hiding this comment.
FWIW given that nopkg is landing, it looks like the plan is to have "scripts that behave as software" so the scripts section would actually go away here.
@rachaelshaw does that sound right? When is that work expected to land?
In any event, if we keep this result in we need updates to the contributor API docs to reflect the revised shape of the response payload. Looks like the existing response is wrong anyway given that it it shows script so there's a 100% chance we need to change that endpoint. This can/probably should be its own PR so we're not needing to get two different sets of codeowners to simultaneously approve a thing.
fix test names Co-authored-by: Ian Littman <iansltx@gmail.com>
Agreed. I don't have all the context for nopkg, and I'm always +1 on thinking about using existing systems to accomplish related tasks. It does seem like the software system has some baggage that I'm not keen on putting on scripts as well, versus just making self-service scripts possible using the existing scripts framework. We'll have to have guidance on when to use a script vs. a nopkg, people will want to schedule / batch nopkg installs like they can with scripts, etc. But if this is all in progress already then 🤐 .
Test nits are in, and as mentioned above there's no docs to update for this particular API. If that's unintentional, I'll make a PR to add one. |
Looks like it just got pushed to Ready to estimate (#31719, /cc @cdcme).
Missing docs at all on the endpoint was a miss from earlier then. Please PR the add to the contributor API docs. Will approve this one in the interim so as not to block. |
iansltx
left a comment
There was a problem hiding this comment.
Approving given the endpoint touched will get documented in another PR with its own approver.
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #46973 > **Draft:** on-device QA (Mac/iPhone/iPad) is pending hardware, expected next week. Opening as draft for early review of the approach. ## Description FMA and custom-package app icons were misaligned on the macOS setup experience ("Setting up your device") screen — icons rendered at different sizes and their "Install …" labels didn't line up. **Root cause:** `SetupSoftwareProcessCell` forced `.software-icon__small { width: $pad-xlarge }` (32px, width only). That class lands on different elements in `SoftwareIcon`'s two render paths, so it hit them differently: - **Fleet-maintained / VPP apps (icon URL)** → the class is on the wrapper `<div>`; the 24px `<img>` inside stayed 24px, left-aligned. - **Custom packages (no URL)** → the class is on the fallback **SVG** itself, which got stretched to 32px wide. Result: different icon sizes/positions by app type → the misalignment. **Fix:** remove the width override so every app type renders `SoftwareIcon` at its consistent, vertically-centered 24px "small" size. ## Testing Verified in Storybook (added `SetupSoftwareProcessCell.stories.tsx`, `MixedAlignment` story) with real matched brand icons (Chrome, 1Password, VS Code, Zoom), a generic custom-package icon, and a URL/`<img>` icon stacked together: - **Before** (with the override): icons render at mixed 24/32px sizes; "Install …" labels don't align. <img width="1135" height="398" alt="before" src="https://github.com/user-attachments/assets/fe216ab4-0a7b-4211-b8aa-96e65e6ca3f5" /> - **After** (this change): all icons render at 24px, centered, labels aligned. <img width="1185" height="425" alt="after" src="https://github.com/user-attachments/assets/28257eb8-df6e-4168-a0bb-28dda4bb40ba" /> On-device QA to follow once hardware is available. ## Notes for reviewer - The `width: $pad-xlarge` (32px) was added in #33770, so 32px may have been the *intended* icon size. This change makes them a consistent **24px**. If a larger icon is desired, that's a follow-up done properly via a real `SoftwareIcon` size (not a width-only override) — flagging for PD input since this is `:product`-labeled. - Included a Storybook story for visual verification/regression; happy to drop it if that's not wanted here. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [ ] QA'd all new/changed functionality manually <!-- pending on-device QA --> - Added a Storybook story for visual verification (not an automated test). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved software process cell styling for more consistent icon sizing and alignment. * **Tests** * Added Storybook scenarios covering fleet-maintained apps, custom packages, uploaded icons, and mixed app layouts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #33173
Details
This PR updates the "Setting up your device" page which appears in Linux and Windows (and as of #30117, MacOS) setup experiences. Front-end updates:
setup_onlyquery param to the/devicepage which, if set, will always show the "setting up your device" page even if all setup is complete. Normally as soon as setup finishes, the front-end redirects to the regular My Device page. In the case of MacOS setup experience, we don't want this to happen as we expect to either 1) keep the setup experience up indefinitely if we're blocking device setup on software install failure, or 2) close the setup dialog on successful completion. This query param is also handy for testing.This PR also includes one small backend change to the
/device/{token}/setup_experience/statusAPI endpoint, to have it return ascriptsarray alongside the existingsoftwarearray. This endpoint is not documented publicly.Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
Updated existing DeviceUserPage tests that check the SettingUpYourDevice content, and added new tests for the new scripts content and the new query param.
QA'd all new/changed functionality manually
(note that as of now we'd only have at most one script, showing multiple here to demonstrate the different states)