Fleet UI: APRF Software title details page Library/Inventory layout - #47682
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/38504-auto-update-pin-rollback-fma #47682 +/- ##
==========================================================================
Coverage ? 67.26%
==========================================================================
Files ? 3639
Lines ? 230010
Branches ? 11923
==========================================================================
Hits ? 154715
Misses ? 61418
Partials ? 13877
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4977199 to
086c449
Compare
RachelElysia
changed the title
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Software title details UI (software/titles/:id) to match the new “Summary → Library → Inventory” layout, while adding supporting UI pieces (header pills slot, Policies modal, and a stubbed Versions action) and moving the versions table into the new Inventory section.
Changes:
- Reorganizes
SoftwareTitleDetailsPageinto Summary, Library, and Inventory sections, and extracts the versions table into a dedicatedTitleVersionsTable/component. - Adds header pills support to
SoftwareDetailsSummaryand uses it inSoftwareSummaryCardfor installer-kind / self-service / auto-install pills (including Policies modal routing logic). - Adds a stubbed “Versions” action (Premium + FMA + permissions gated) and updates shared policy helpers/badges + Tag tooltip/focus behavior.
Reviewed changes
Copilot reviewed 21 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/TitleVersionsTableConfig.tsx | Fixes relative import after moving the versions table into its own folder. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/TitleVersionsTable.tsx | New Inventory “Versions installed across all hosts” table component. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/TitleVersionsTable.tests.tsx | Adds unit coverage for versions table render + empty state. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/index.ts | Re-exports the new table component. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/_styles.scss | Adds spacing tweaks for the table empty state container. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsx | Implements the new page layout and section-level rendering/gating. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx | Moves pills into SoftwareDetailsSummary via headerPills, adds Policies modal behavior, and wires stub Versions action gating. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tests.tsx | Adds tests for Versions option gating/ordering and header pill behaviors + Policies modal open/direct-nav. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/SoftwareInstallerCard.tsx | Removes duplicated tag/policies UI now handled via summary pills/modal. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tsx | Adds a table help-text row for policy meaning (used by Policies modal). |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tests.tsx | Updates expectations after removing the dynamic-policy tooltip. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/PoliciesModal.tsx | New read-only Policies modal, reusing InstallerPoliciesTable. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/PoliciesModal.tests.tsx | Tests modal empty state and Done/exit behavior + prop forwarding. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/index.ts | Re-exports PoliciesModal. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/_styles.scss | Styles for Policies modal empty-state text. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditIconModal/EditIconModal.tsx | Updates import path for the moved last-updated helper. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/_styles.scss | Adds section layout styling and normalizes h2 spacing. |
| frontend/pages/SoftwarePage/helpers.tsx | Refactors mergePolicies to avoid mutating Set instances. |
| frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsx | Adds Versions action handling + headerPills slot and updates summary layout markup. |
| frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tests.tsx | Adds tests for Versions option ordering/tooltip + headerPills rendering. |
| frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/_styles.scss | Updates summary layout to grid and positions pills/actions/title consistently. |
| frontend/components/Tag/Tag.tsx | Adds optional icon, trailing icon, and optional tooltip wrapper. |
| frontend/components/Tag/Tag.tests.tsx | Adds unit tests for icon/trailingIcon/button/tooltip behavior. |
| frontend/components/Tag/_styles.scss | Fixes focus-outline positioning and uses :focus-visible. |
| frontend/components/SoftwareInstallPolicyBadges/SoftwareInstallPolicyBadges.tsx | Removes dynamic-policy tooltip wrapper to move context into modal help text. |
Comments suppressed due to low confidence (1)
frontend/components/Tag/Tag.tsx:49
- When
onClickis provided, this component renders a<button>without an explicittype. In HTML, the default button type issubmit, which can trigger unintended form submissions if a clickable Tag is ever used inside a<form>.
const tag = onClick ? (
// use a button element so that the tag can be focused and clicked
// with the keyboard
<button className={classNames} onClick={onClick}>
{content}
</button>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR introduces a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/pages/SoftwarePage/helpers.tsx (1)
359-379: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAlign object construction for consistency with the interface.
The automatic-install branch (lines 364–367) spreads the entire
installPolicyobject, potentially carrying extra fields beyond theISoftwareInstallPolicyUIinterface (id,name,type). The patch-only branch (lines 375–379) explicitly constructs only those three fields. For consistency and clarity, both branches should construct objects that strictly match the interface.♻️ Recommended refactor
(automaticInstallPolicies ?? []).forEach((installPolicy) => { byId.set(installPolicy.id, { - ...installPolicy, + id: installPolicy.id, + name: installPolicy.name, type: new Set(["dynamic"]), }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/SoftwarePage/helpers.tsx` around lines 359 - 379, The automatic-install branch in the forEach loop for automaticInstallPolicies spreads the entire installPolicy object which may include fields beyond the ISoftwareInstallPolicyUI interface, while the patch-policy branch (lines 375-379) explicitly constructs only the three required fields (id, name, type). Refactor the forEach callback to explicitly construct the object with only the id, name, and type properties instead of using the spread operator on installPolicy, ensuring both branches follow the same consistent pattern of matching the interface strictly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/pages/SoftwarePage/helpers.tsx`:
- Around line 359-379: The automatic-install branch in the forEach loop for
automaticInstallPolicies spreads the entire installPolicy object which may
include fields beyond the ISoftwareInstallPolicyUI interface, while the
patch-policy branch (lines 375-379) explicitly constructs only the three
required fields (id, name, type). Refactor the forEach callback to explicitly
construct the object with only the id, name, and type properties instead of
using the spread operator on installPolicy, ensuring both branches follow the
same consistent pattern of matching the interface strictly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6df94c1a-fea4-46bc-8e5b-9dbb0f61a994
📥 Commits
Reviewing files that changed from the base of the PR and between bad2a48 and 8f46494292b07e46a526d140bdbf0f409b4a6aba.
📒 Files selected for processing (24)
frontend/components/Tag/Tag.tests.tsxfrontend/components/Tag/Tag.tsxfrontend/components/Tag/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditIconModal/EditIconModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/PoliciesModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/PoliciesModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/SoftwareInstallerCard.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/TitleVersionsTable.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/TitleVersionsTable.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/TitleVersionsTableConfig.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/TitleVersionsTable/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/_styles.scssfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tests.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/_styles.scssfrontend/pages/SoftwarePage/helpers.tsx
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
49f3469 to
8f4a858
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx (1)
188-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEarly return drops PoliciesModal for multi-policy Auto install pills.
When
installerResultis falsy, the component returns before modal rendering. In that branch, the Auto install chip still callssetShowPoliciesModal(true)for multi-policy titles, but nothing is mounted to display it.Suggested fix
if (!installerResult) { return ( - <Card borderRadiusSize="xxlarge" className={baseClass}> - <SoftwareDetailsSummary - displayName={softwareDisplayName} - type={formatSoftwareType(softwareTitle)} - versions={softwareTitle.versions?.length ?? 0} - hostCount={softwareTitle.hosts_count} - countsUpdatedAt={softwareTitle.counts_updated_at} - queryParams={{ software_title_id: softwareId, fleet_id: teamId }} - name={softwareTitle.name} - source={softwareTitle.source} - iconUrl={softwareTitle.icon_url} - iconUploadedAt={iconUploadedAt} - headerPills={headerPills} - /> - </Card> + <> + <Card borderRadiusSize="xxlarge" className={baseClass}> + <SoftwareDetailsSummary + displayName={softwareDisplayName} + type={formatSoftwareType(softwareTitle)} + versions={softwareTitle.versions?.length ?? 0} + hostCount={softwareTitle.hosts_count} + countsUpdatedAt={softwareTitle.counts_updated_at} + queryParams={{ software_title_id: softwareId, fleet_id: teamId }} + name={softwareTitle.name} + source={softwareTitle.source} + iconUrl={softwareTitle.icon_url} + iconUploadedAt={iconUploadedAt} + headerPills={headerPills} + /> + </Card> + {showPoliciesModal && ( + <PoliciesModal + policies={mergedPolicies} + teamId={teamId} + onExit={() => setShowPoliciesModal(false)} + /> + )} + </> ); }Also applies to: 345-351
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx` around lines 188 - 205, The early return when installerResult is falsy renders the SoftwareDetailsSummary component which displays the Auto install chip that calls setShowPoliciesModal for multi-policy titles, but the PoliciesModal component is not rendered in this branch, so the modal never appears when triggered. Move the PoliciesModal component rendering to be accessible in both the early return branch and the main rendering path, or place it at a higher level in the component so it's always mounted regardless of the installerResult value. Apply the same fix to the other early return pattern mentioned in the comment.frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tests.tsx (1)
35-57:⚠️ Potential issue | 🟠 MajorAwait async hover interactions and use async queries for tooltip assertions.
The nested
waitForblocks with non-awaiteduser.hover()calls create a flaky test pattern. In userEvent v14+,hover()returns a Promise and must be awaited. Usescreen.findByText()for async assertions instead of wrapping synchronous queries inwaitFor.Suggested fix
- await waitFor(() => { - waitFor(() => { - user.hover(screen.getByTestId("refresh-icon")); - }); - - expect( - screen.getByText( - "Software will be automatically installed when hosts fail this policy." - ) - ).toBeInTheDocument(); - }); + await user.hover(screen.getByTestId("refresh-icon")); + expect( + await screen.findByText( + "Software will be automatically installed when hosts fail this policy." + ) + ).toBeInTheDocument(); - await waitFor(() => { - waitFor(() => { - user.hover(screen.getByText(/patch/i)); - }); - - expect( - screen.getByText( - "Hosts will fail this policy if they're running an older version." - ) - ).toBeInTheDocument(); - }); + await user.hover(screen.getByText(/patch/i)); + expect( + await screen.findByText( + "Hosts will fail this policy if they're running an older version." + ) + ).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tests.tsx` around lines 35 - 57, Remove the nested waitFor blocks and properly handle the async hover interactions in the InstallerPoliciesTable.tests.tsx file. For each tooltip test section, await the user.hover() call since it returns a Promise in userEvent v14+, and replace the synchronous screen.getByText() queries with screen.findByText() to handle the async tooltip rendering. Structure each test so that the awaited hover interaction and the findByText assertion work together within a single waitFor or as separate awaited operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tests.tsx`:
- Around line 35-57: Remove the nested waitFor blocks and properly handle the
async hover interactions in the InstallerPoliciesTable.tests.tsx file. For each
tooltip test section, await the user.hover() call since it returns a Promise in
userEvent v14+, and replace the synchronous screen.getByText() queries with
screen.findByText() to handle the async tooltip rendering. Structure each test
so that the awaited hover interaction and the findByText assertion work together
within a single waitFor or as separate awaited operations.
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx`:
- Around line 188-205: The early return when installerResult is falsy renders
the SoftwareDetailsSummary component which displays the Auto install chip that
calls setShowPoliciesModal for multi-policy titles, but the PoliciesModal
component is not rendered in this branch, so the modal never appears when
triggered. Move the PoliciesModal component rendering to be accessible in both
the early return branch and the main rendering path, or place it at a higher
level in the component so it's always mounted regardless of the installerResult
value. Apply the same fix to the other early return pattern mentioned in the
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f64fe69-c802-4618-a5bd-d9e229f7843c
📥 Commits
Reviewing files that changed from the base of the PR and between 2441456212aebc6aea9124fa96a61914cddfecef and 49f34696dcd5f81c81bd25d3052e427426454060.
📒 Files selected for processing (14)
frontend/components/Chip/Chip.stories.tsxfrontend/components/Chip/Chip.tests.tsxfrontend/components/Chip/Chip.tsxfrontend/components/Chip/_styles.scssfrontend/components/Chip/index.tsfrontend/components/Tag/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/InstallerPoliciesTable/InstallerPoliciesTable.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/SoftwareInstallerCard.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/_styles.scss
💤 Files with no reviewable changes (1)
- frontend/components/Tag/index.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/components/Chip/index.ts
- frontend/components/Chip/Chip.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsx
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tests.tsx
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareInstallerCard/SoftwareInstallerCard.tsx
- frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/_styles.scss
- frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsx
The `?? []` fallback was minting a fresh array literal each render whenever both optional-chained sources were nullish, busting the mergedPolicies cache and cascading into the headerPills useMemo. Depend on the optional-chained values directly; mergePolicies already accepts null | undefined.
- Renamed the Tag component to Chip to free up the overloaded "tag" term for other UI uses. Updated BEM (`chip`, `chip__clickable-chip`) and the two callers (SoftwareInstallerCard, SoftwareSummaryCard). - Added Chip.stories.tsx with playground + icon / clickable / tooltip variants. - Restored the auto-install refresh-icon hover-tooltip assertion in InstallerPoliciesTable.tests.tsx now that the tooltip is back on the icon. - Removed comments that won't age: cross-test breadcrumbs, "prior gating" history references, and a few that just restated the next line.
- `showVersionsTable` was gating the whole Inventory section (header, description, table) — renamed to `showInventorySection` to match what it actually controls. - Removed the obvious JSDoc on `Chip`'s `tooltip` prop.
- Edit appearance / Edit configuration / Edit software (vpp_apps) now disable directly on gitOpsModeEnabled instead of keying off the tooltip variable, which only populated when repoURL was truthy. Empty repoURL in gitops mode previously left them clickable. - Dropped the dead `&__installer-policies-table` rule from SoftwareInstallerCard/_styles.scss; the wrapping div was removed earlier and nothing else composes the class.
Storybook file didn't need the React import under the new JSX transform.
- PoliciesModal rendered only in the installer branch even though the show-modal state was hoisted above the early return. Lifted the modal into a local that both branches render, so a future source of linked policies in the no-installer path won't silently fail to open it. - InstallerPoliciesTable test: replaced nested waitFor + non-awaited user.hover with `await user.hover` + `await findByText`. Dropped the now-unused waitFor import.
…ipt/Android status copy - Hide Library section when no installer is present (e.g. All Teams view) - Mirror InstallerStatusTableConfig: script-only "ran" label + tooltips, Android Play Store pending/failed tooltips - Drop dead renderSoftwareInstallerCard and its unused context destructures - Hoist pickLabels helper; reuse aggregateInstallStatusCounts - Move FMA/script flags inside the pkg branch
060d65a to
f3011da
Compare
- LibraryItemAccordion: cover info-tooltip wording for package, tarball, and Android variants; drop reference to deleted SoftwareInstallerCard - SoftwareTitleDetailsPage: render ViewYamlModal directly so the ?gitops_yaml=true redirect and the gitops-mode edit success path keep a mount point - SoftwareDetailsSummary: replace broken source === "vpp_apps" GitOps gate with an isAppleVpp prop threaded from SoftwareSummaryCard - Chip: explicit type="button" - Remove dead SoftwareInstallerCard.tsx and its InstallerStatusTable/ subtree (no remaining consumers); keep InstallerDetailsWidget/ and InstallerPoliciesTable/
- LibraryItemAccordion: make "All hosts" badge clickable (mirrors the label-count badge — fires onLabelCountClick) when canEditSoftware - SoftwareTitleDetailsPage: wire onTrashClick → DeleteSoftwareModal and onDownloadClick → token-based download for library rows (handlers were missing on the accordion when SoftwareInstallerCard was dropped) - LibraryItemAccordion: drop "policy automation" from the info tooltip for iOS/iPadOS apps — automatic_install isn't supported there - Comment audit: drop stale temporal anchors and a couple of TODOs that no longer describe reality
|
@claude review |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tsx:306-320— The status-row info-outline tooltip for script-only packages (sh_packages/ps1_packages) reads "Latest status from policy automation, setup experience, or manual install." — but script-only packages cannot be selected in the Setup Experience UI (getSetupExperienceLinuxPackageCopyinInstallSoftwareTableConfig.tsxonly switches onrpm_packages/deb_packages/tgz_packages). Add anisScriptPackagebranch togetStatusCountTooltip()atLibraryItemAccordion.tsx:306(matching the three sibling getters added in this PR) returning null or script-appropriate copy, and gate the surroundingTooltipWrapperon a non-null tooltip.Extended reasoning...
What the bug is
getStatusCountTooltip()atLibraryItemAccordion.tsx:306-320only branches onisAndroidAppandisTarballPackage. Script-only packages (sh_packages/ps1_packages) fall through to the default return:return ( <> Latest status from policy automation, <br /> setup experience, or manual install. </> );
The three sibling tooltip getters added in the same diff —
getInstalledIconTooltip,getPendingIconTooltip,getFailedIconTooltip— all have anisScriptPackagebranch (e.g. "The script successfully ran on these hosts"). The omission here is clearly an oversight, not intentional. Pre-PR, the deletedInstallerStatusTable.renderTableHelpTextexplicitly returnednullforisScriptPackage(if (isScriptPackage) { return null; }), so script packages had no help text at all. The new component reintroduces stale copy.Step-by-step proof
- Admin views a
.shor.ps1software title at/software/titles/:id.SoftwareTitleDetailsPage.tsx'srenderLibrarySectionrendersLibraryItemAccordionwithisScriptPackage={true}(frominstallerResult.cardInfo.isScriptPackage),isTarballPackage={false}(only true whentitle.source === "tgz_packages"), and noandroidPlayStoreId. - Inside
LibraryItemAccordion,isAndroidApp = !!androidPlayStoreId = false,isTarballPackage = false,isScriptPackage = true. IngetStatusCountTooltip()both branches are skipped and the default copy is returned. - The component renders the info-outline icon at
LibraryItemAccordion.tsx:566-575unconditionally next to the installed count; it does not gate on installer type. - User expands the row and hovers the info-outline icon. The tooltip displays "Latest status from policy automation, setup experience, or manual install."
- But: setup-experience install of script-only software is not surfaced anywhere in the UI. I verified
frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareTable/InstallSoftwareTableConfig.tsx:19-30—getSetupExperienceLinuxPackageCopyswitches only onrpm_packages/deb_packages/tgz_packages;sh_packages/ps1_packagesreturnnulland the rows can't be selected.
Re: the refutation
One verifier argued setup experience does support script packages because
frontend/interfaces/setup.tsdefines asoftware_script_runstep type. That conflates two different things:software_script_runis the setup-experience step type used to designate that a chosen software (sh/ps1 source) is run as a script during enrollment, but the UI for choosing which Linux packages to expose in setup experience (InstallSoftwareTableConfig) restricts the picker torpm/deb/tgz. Script-only packages can't be added to setup experience via the admin UI, so the tooltip copy ("setup experience") is not surfaced for those titles.Even granting the verifier's framing, the consistency argument stands: the same component's three sibling getters explicitly branch on
isScriptPackage(swapping "ran" / "running" / "script" copy throughout), so the author's evident intent was that script packages get scripted-language-appropriate copy here too. The status-row label even switches viainstalledLabel = isScriptPackage ? "ran" : "installed"on the same render path — leavinggetStatusCountTooltipun-branched is the odd one out.Impact
Copy issue only. The tooltip is informational and does not gate or change any user action. Script-only packages still install and report status correctly. Filing as nit.
Test gap
The new
script-only packagetest atLibraryItemAccordion.tests.tsx:535-563exercises only the per-status icon tooltips (installed/pending/failed) and never hovers the info-outline icon forstatusCountsTooltip. The tarball test explicitly asserts setup experience is absent from its tooltip; the script test should do the same.Fix
Add an
isScriptPackagebranch at the top ofgetStatusCountTooltip():const getStatusCountTooltip = () => { if (isScriptPackage) { return null; // or script-appropriate copy } if (isAndroidApp) { /* ... */ } if (isTarballPackage) { /* ... */ } return ( /* default */ ); };
Then gate the surrounding
TooltipWrapper(around the info-outline icon) onstatusCountsTooltip !== null, matching the pattern already added atLibraryItemAccordion.tsx:275-286for the per-status icons. One added branch and a small wrap conditional — symmetric with the existinginstalledIconTooltipnull-handling. - Admin views a
| fleet_id: currentTeamId ?? APP_CONTEXT_NO_TEAM_ID, | ||
| }); | ||
|
|
||
| const installerMeta = installerResult?.meta; | ||
| const isFma = installerMeta?.isFleetMaintainedApp ?? false; | ||
| const isLatestFmaVersion = installerMeta?.isLatestFmaVersion ?? false; | ||
| const isScriptPackage = installerResult?.cardInfo.isScriptPackage ?? false; | ||
|
|
||
| interface ILabeledSource { | ||
| labels_include_any: ILabelSoftwareTitle[] | null; | ||
| labels_include_all: ILabelSoftwareTitle[] | null; | ||
| labels_exclude_any: ILabelSoftwareTitle[] | null; | ||
| } | ||
| interface IPickedLabels { | ||
| labels: ILabelSoftwareTitle[] | null; | ||
| kind: LibraryItemLabelKind; | ||
| } | ||
| const pickLabels = (source: ILabeledSource): IPickedLabels => { | ||
| if (source.labels_include_all?.length) { | ||
| return { labels: source.labels_include_all, kind: "includeAll" }; | ||
| // TODO #47623 multi-row rendering — render one accordion per version, | ||
| // and lift installer-meta lookup to per-row (only the latest is "active"). | ||
| const libraryAccordionList = () => { | ||
| const pkg = title.software_package; | ||
| const appStore = title.app_store_app; | ||
|
|
||
| if (appStore) { | ||
| const { labels, kind } = pickLabels(appStore); | ||
| const isAndroidPlayStoreApp = appStore.platform === "android"; |
There was a problem hiding this comment.
🔴 On All Teams view (currentTeamId === APP_CONTEXT_ALL_TEAMS_ID === -1), the new renderLibrarySection no longer guards against that state (the deleted renderSoftwareInstallerCard did), and the downstream currentTeamId ?? APP_CONTEXT_NO_TEAM_ID fallbacks don't catch -1 (the ?? only catches null/undefined). Result: the Library section renders for an All-Teams title that has a No-Team installer, statusPath links emit fleet_id=-1, and clicking a label-count badge opens <EditSoftwareModal teamId={-1}>, producing invalid backend API calls. Fix: either restore the All-Teams guard at the section level (if (currentTeamId === APP_CONTEXT_ALL_TEAMS_ID) return null;) or switch the fallback to teamIdForApi (which is undefined in All Teams view — the canonical API-shape sentinel used by renderInventorySection just below).
Extended reasoning...
Bug
The PR replaces renderSoftwareInstallerCard with renderLibrarySection (SoftwareTitleDetailsPage.tsx:191-263), but drops the explicit currentTeamId !== APP_CONTEXT_ALL_TEAMS_ID guard that the deleted card had. The new section only gates on !isPremiumTier || !isAvailableForInstall. Since useTeamIdParam is called with includeAllTeams: true (line 86), currentTeamId === -1 (APP_CONTEXT_ALL_TEAMS_ID) is a legitimate state for this route.
Downstream, two fallbacks at lines 204 and 350 use currentTeamId ?? APP_CONTEXT_NO_TEAM_ID — and the nullish-coalescing operator only catches null / undefined. -1 falls through unchanged.
How it manifests
- Admin on Premium navigates to
/software/titles/:idwithout afleet_idquery param (URL manipulation or following a link that drops fleet context).currentTeamId === -1. - The backend's
SoftwareTitleByID(server/service/software_titles.go:188-219) populatessoftware_packageeven whenteamIDis nil — the datastore (server/datastore/mysql/software_installers.go:1041-1047) coerces nil totmID=0and queries the No-Team installer. SoisAvailableForInstallis true on All Teams view for any title with a No-Team installer. renderLibrarySectiondoesn't bail, so the section renders.statusPathbuilds links withfleet_id: currentTeamId ?? APP_CONTEXT_NO_TEAM_ID→fleet_id=-1. PeruseTeamIdParam.ts:341, negativefleet_idis treated as a redirect-to-default signal, breaking the manage-hosts target.renderLibraryEditModal(line 350) mounts<EditSoftwareModal teamId={currentTeamId ?? APP_CONTEXT_NO_TEAM_ID}>→teamId=-1.canWriteSoftware(user, -1)returns true for a global admin (permissions.ts:198-208short-circuits onisGlobalAdmin), so the label-count badge is clickable.- The modal then drives
softwareAPI.editSoftwarePackage({teamId: -1})on submission — invalid backend team id.
Step-by-step proof
- Premium tenant has Google Chrome uploaded as a No-Team installer (
global_or_team_id = 0). - Global admin opens
/software/titles/123directly (nofleet_idquery param).useTeamIdParamreturns{ currentTeamId: -1, teamIdForApi: undefined }. softwareAPI.getSoftwareTitle({ teamId: undefined })→ backend treats nil as 'no filter' and returns the title shaped withsoftware_packagepopulated from the No-Team installer.isAvailableForInstall = true.renderLibrarySectiongate (!isPremiumTier || !isAvailableForInstall) passes.- Library accordion renders. Its status links read
/hosts/manage?software_title_id=123&software_status=installed&fleet_id=-1. - Admin clicks the label-count badge.
openEditModalsetsshowLibraryEditModal=true.renderLibraryEditModalmounts<EditSoftwareModal teamId={-1}>. Modal opens.
Why existing code doesn't catch it
The sibling renderInventorySection (and the rest of the page) uses teamIdForApi, which is undefined in All Teams — the canonical API-shape sentinel. Only the Library section diverges by reaching for currentTeamId and dropping the All-Teams guard. TypeScript doesn't catch this because currentTeamId is typed number | undefined and -1 is a valid number; the contract that -1 is a sentinel lives only in shared constants, not in the type system.
Fix
Either of the following closes the regression. Option A is the cleanest because the modal flow needs a real team id to function, so allowing the section to render at all on All Teams produces broken downstream state regardless of what the fallback emits:
// Option A — restore the All-Teams guard at the section level
if (!isPremiumTier || !isAvailableForInstall) return null;
if (currentTeamId === APP_CONTEXT_ALL_TEAMS_ID) return null;// Option B — switch both fallbacks to teamIdForApi (undefined in All Teams)
fleet_id: teamIdForApi,
// and
teamId: teamIdForApi ?? APP_CONTEXT_NO_TEAM_ID,Note: Option B fixes the link, but the modal still needs a non-undefined team id to drive a real API call — so the section probably shouldn't render in this state at all. Prefer A.
- Add onTrashClick to the app-store branch — the trash icon was visible for VPP / Play Store titles but clicking it was a silent no-op - Forward isIosOrIpadosApp on the package branch — iOS in-house .ipa apps flow through software_package and were getting the default tooltip copy that advertises policy automation (not supported on iOS/iPadOS)
09e2912
into
feat/38504-auto-update-pin-rollback-fma
| const getStatusCountTooltip = () => { | ||
| if (isAndroidApp) { | ||
| return <>Latest status from the Google Play Store</>; | ||
| } | ||
|
|
||
| const pendingIconTooltip = ( | ||
| <> | ||
| Fleet is installing/uninstalling or will | ||
| <br /> | ||
| do so when the host comes online. | ||
| </> | ||
| ); | ||
| if (isTarballPackage) { | ||
| return <>Latest status from policy automation or manual install.</>; | ||
| } | ||
|
|
||
| const failedIconTooltip = ( | ||
| <> | ||
| These hosts failed to install/uninstall | ||
| <br /> | ||
| software. Click on a host to view error(s). | ||
| </> | ||
| ); | ||
| // `automatic_install` is not supported for iOS or iPadOS, so drop the | ||
| // policy-automation leg. | ||
| if (isIosOrIpadosApp) { | ||
| return <>Latest status from setup experience or manual install.</>; | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| Latest status from policy automation, | ||
| <br /> | ||
| setup experience, or manual install. | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🟡 Two cosmetic nits in getStatusCountTooltip (lines 320–342): (1) the function branches on isAndroidApp / isTarballPackage / isIosOrIpadosApp but not isScriptPackage, so script-only packages (sh_packages / ps1_packages) fall through to the default copy that advertises "setup experience" — the sibling helpers (getInstalledIconTooltip, getPendingIconTooltip, getFailedIconTooltip) all branch on isScriptPackage and the deleted InstallerStatusTable.renderTableHelpText explicitly returned null for scripts, so this is the lone holdout. (2) The Android Play Store branch at line 322 returns Latest status from the Google Play Store with no trailing period, while the tarball / iOS / default branches all end with install. — a one-character punctuation inconsistency visible on every Android Play Store accordion.
Extended reasoning...
What the bug is
getStatusCountTooltip (LibraryItemAccordion.tsx:320–342) renders the info-outline tooltip trailing the "installed" status count. It branches:
if (isAndroidApp) return <>Latest status from the Google Play Store</>; // ← no period
if (isTarballPackage) return <>Latest status from policy automation or manual install.</>;
if (isIosOrIpadosApp) return <>Latest status from setup experience or manual install.</>;
return <>Latest status from policy automation,<br/>setup experience, or manual install.</>; // ← scripts hit thisTwo cosmetic issues:
-
No
isScriptPackagebranch. For sourcessh_packages/ps1_packages,SoftwareTitleDetailsPagepassesisScriptPackage=trueto the accordion (wired frominstallerResult.cardInfo.isScriptPackage), but the function never inspects that flag, so scripts fall through to the default tooltip mentioning "setup experience". -
Android branch missing a trailing period. Line 322 returns
Latest status from the Google Play Storewith no period, while the three sibling branches all end withinstall..
Step-by-step proof
Bug 1 — scripts fall through to the wrong copy:
- Premium admin uploads
install.shas a script-only package on a team. - Backend returns
{source: 'sh_packages', software_package: {...}, app_store_app: null}. SoftwareTitleDetailsPage.renderLibrarySectiontakes the package branch (lines 256–280) and passesisScriptPackage={isScriptPackage}toLibraryItemAccordion.- Inside the accordion,
isAndroidApp = !!androidPlayStoreId = false,isTarballPackage = false(source issh_packages, nottgz_packages),isIosOrIpadosApp = false. getStatusCountTooltip()falls through to the default —Latest status from policy automation, setup experience, or manual install.- User expands the accordion and hovers the info-outline icon. Tooltip displays "setup experience" — a software-install affordance the accordion's per-status copy intentionally avoids for scripts (the installed label says "ran", the installed icon tooltip says "The script successfully ran on these hosts", etc.).
Bug 2 — Android missing period:
- Premium admin views any Android Play Store title.
renderLibrarySectiontakes the app-store branch, passesandroidPlayStoreId={appStore.app_store_id}.isAndroidApp = true→getStatusCountTooltipreturns<>Latest status from the Google Play Store</>.- Hovering the info-outline icon renders the tooltip without a trailing period, while every other accordion variant (tarball, iOS/iPadOS, default) ends with
install..
Addressing the refutations
Bug 1 refutation: "scripts CAN be triggered via setup_experience / policy automation / manual install, so the default copy is factually correct."
The refuter is right that the underlying sources may technically apply — software_script_run is a SETUP_STEP_TYPE, and patch/install policies do drive script runs. But the relevant signal is the codebase's existing UX choice, not what's theoretically possible:
- The deleted
InstallerStatusTable.renderTableHelpTextexplicitly returnednullforisScriptPackagepre-PR — scripts intentionally showed no such help text. - The sibling helpers in the same function (
getInstalledIconTooltip,getPendingIconTooltip,getFailedIconTooltip) all branch onisScriptPackageand emit "ran" / "running the script" / "failed to run the script" wording. The info-outline copy is the only holdout, so the per-installer-mode consistency the rest of the file maintains is broken here. Severity remains a nit because the wording isn't strictly wrong — just inconsistent with the rest of the function.
Bug 2 refutation: "single missing period is below the nit bar."
Filing as nit, not normal — agreed it's not blocking. But it sits inline with three sibling branches that all properly punctuate, in user-visible tooltip copy that ships on every Android Play Store accordion, and the fix is a literal one character. The combination of (a) sitting in the diff next to three correctly-punctuated siblings, (b) user-visible production copy, and (c) trivial fix puts it slightly above pure noise — worth flagging at this polish moment.
How to fix
Add an isScriptPackage branch (e.g. return null to match the pre-PR behavior, or emit "Latest status from policy automation or manual install." if scripts don't surface through setup experience), and add a trailing period to the Android branch:
if (isAndroidApp) {
return <>Latest status from the Google Play Store.</>; // add period
}
if (isScriptPackage) { // new branch
return null; // or whichever wording matches the design intent
}
#48293) **Related issue:** Resolves #38504 **Constituent PRs (merged into this feature branch):** - #47682 — Fleet UI: APRF Software title details page Library/Inventory layout - #47808 — Extend update software installer API to support FMA version pinning - #47944 — Fleet UI: APRF library item accordion component - #48081 — Versions modal, multi-row Library, pinned state - #48098 — Add `pinned_version` to `edited_software` activity - #48123 — Auto-update FMA cron - #48144 — Download a newly-published FMA version when pinned to it # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Fleet-maintained app version pinning (Latest, exact, and major) via a new Versions modal. * Introduced premium auto-updates for maintained apps with pin-aware promotion and rollback-safe caching. * Added expandable library version rows and a Policies modal. * **Bug Fixes** * Improved pin handling, cache/manifest hydration, and safer update behavior on per-app failures and deduplication. * **UI/UX** * Refreshed the Software title details experience with new accordion/list patterns, redesigned details widget/tooltips, and updated installer presentation. * **Documentation** * Expanded Storybook component/page coverage and adjusted Storybook canvas padding. * **Tests** * Added/updated unit and integration tests for pinning, auto-update flows, and new modal/UI behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Issue
Closes #47621
Description
Implements the new
SoftwareTitleDetailsPagelayout — top summary, then a Library section and an Inventory section.Layout: Library and Inventory sections — split the page into a top summary card (icon + name + pills + counts) and two labeled sections. Library wraps the installer card and is gated to Premium (
!isPremiumTierhides it on Fleet Free, per the APRF: Software title details page layout (actions dropdown, Policies modal, Library section shell) #47621 CoS); Inventory wraps the versions table. Sections gate their own rendering (Library only when an installer exists; Inventory only when hosts have the title installed and the source reports versions). PulledTitleVersionsTable/up to be a sibling ofSoftwareInstallerCard/since it no longer belongs to the summary card.SoftwareInstallerCardis a temporary host for the Library section — it will be replaced byLibraryAccordionListfrom Fleet UI: APRF library item accordion component #47944 once that lands and this branch rebases on top of it.Header pills moved into
SoftwareDetailsSummaryvia a newheaderPillsprop, so the Fleet-maintained / Self-service / Auto install tags sit next to the title regardless of which branch (installer / no-installer) the summary renders.Actions dropdown: Versions action stub — new
Versionsitem betweenPatchandSchedule auto updates, gated bycanManageVersions(Premium FMA only) and disabled with a GitOps tooltip when GitOps mode is on. Click handler is a no-op until APRF: Wire up version pinning (Versions modal, multi-row Library, pinned-state display) #47623 wires the Versions modal.Policies modal (read-only) — opened from the "Auto install" pill in the summary area, lists policies linked to the title (auto-install + patch policies merged via
mergePolicies). Layout and description are placeholders flagged with a TODO for Marko's design pass — the description in particular needs to call out patch policies that may not auto-update.Single-policy direct nav — when only one policy is linked to the title, the "Auto install" pill skips the modal and routes straight to the policy. Multi-policy case still opens the modal.
Tag component: optional
tooltipprop — wraps the rendered tag inTooltipWrapperwhen provided. Used by Self-service (Android / iOS / default copy viagetSelfServiceTooltip) and Auto install (singular/plural copy based on linked-policy count).Tag focus-ring fix — added
position: relativeto.tag__clickable-tagsobutton-focus-outline's::afterdoesn't escape the tag and stretch across whatever positioned ancestor it finds (was producing a 1px line spanning the page after the modal opened from a Tag click).Screenrecording
Updated screen recording 6/23/26 which includes the accordion rebase
Screen.Recording.2026-06-23.at.12.36.54.PM.mov
Tarballs example:
Testing
Summary by CodeRabbit
Release Notes