fix(adhoc-sweep-fixes): CU-86akj32d7 40 review findings across 40 files - #168
flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -67,10 +67,7 @@ const validateWindowLength = ( | |||
| .map(Number); | |||
| const startTotalMinutes = startHours * 60 + startMinutes; | |||
There was a problem hiding this comment.
🦩 🟠 validateWindowLength returns true (valid) when end time is before start time
In validateWindowLength (helpers.tsx), removed the endTotalMinutes < startTotalMinutes || short-circuit that made the function return true (valid) whenever the end time was earlier than the start time. The function now returns solely endTotalMinutes - startTotalMinutes >= 60, which correctly evaluates to false (invalid) both when the window is under 60 minutes and when end time precedes start time (since the subtraction becomes negative, failing the >= 60 check).
🤖 Prompt for AI agents
In frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditAutoUpdateConfigModal/helpers.tsx around line 68, review and complete this code-review fix: validateWindowLength returns true (valid) when end time is before start time.
What the draft fix changed: In validateWindowLength (helpers.tsx), removed the `endTotalMinutes < startTotalMinutes ||` short-circuit that made the function return `true` (valid) whenever the end time was earlier than the start time. The function now returns solely `endTotalMinutes - startTotalMinutes >= 60`, which correctly evaluates to false (invalid) both when the window is under 60 minutes and when end time precedes start time (since the subtraction becomes negative, failing the >= 60 check).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -67,6 +67,9 @@ func parsePMSetOutput(output []byte) map[string]interface{} { | |||
| curKey := "" | |||
| for scanner.Scan() { | |||
There was a problem hiding this comment.
🦩 🟠 parsePMSetOutput will panic on an empty output line
In parsePMSetOutput (orbit/pkg/table/pmset/pmset_darwin.go), added a if len(line) == 0 { continue } guard immediately after reading each scanned line and before indexing line[0], preventing an index-out-of-range panic on empty lines, matching the suggested fix exactly.
🤖 Prompt for AI agents
In orbit/pkg/table/pmset/pmset_darwin.go around line 68, review and complete this code-review fix: parsePMSetOutput will panic on an empty output line.
What the draft fix changed: In parsePMSetOutput (orbit/pkg/table/pmset/pmset_darwin.go), added a `if len(line) == 0 { continue }` guard immediately after reading each scanned line and before indexing `line[0]`, preventing an index-out-of-range panic on empty lines, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -30,5 +30,7 @@ CREATE TABLE IF NOT EXISTS query_hosts ( | |||
| } | |||
|
|
|||
| func Down_20260301000002(tx *sql.Tx) error { | |||
There was a problem hiding this comment.
🦩 🟠 query_hosts migration lacks a rollback body in Down_20260301000002, unlike sibling migration conventions
Changed Down_20260301000002 in server/datastore/mysql/migrations/openframe/20260301000002_AddQueryHostsJoinTable.go from a no-op return nil to execute DROP TABLE IF EXISTS query_hosts; and return the resulting error, matching the sibling migration convention of pairing CREATE TABLE with DROP TABLE IF EXISTS on rollback.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/openframe/20260301000002_AddQueryHostsJoinTable.go around line 32, review and complete this code-review fix: query_hosts migration lacks a rollback body in Down_20260301000002, unlike sibling migration conventions.
What the draft fix changed: Changed `Down_20260301000002` in `server/datastore/mysql/migrations/openframe/20260301000002_AddQueryHostsJoinTable.go` from a no-op `return nil` to execute `DROP TABLE IF EXISTS query_hosts;` and return the resulting error, matching the sibling migration convention of pairing `CREATE TABLE` with `DROP TABLE IF EXISTS` on rollback.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -79,8 +79,8 @@ func certAuthRetro(t *testing.T, ctx context.Context, store storage.CertAuthStor | |||
| } | |||
|
|
|||
| func expectErr(t *testing.T, have, want error) { | |||
There was a problem hiding this comment.
🦩 🟠 Hidden test.Helper() call placed after t.Errorf makes error location misleading
In expectErr (server/mdm/nanomdm/test/e2e/certauth.go), moved t.Helper() to be the first statement in the function, before the errors.Is check and t.Errorf call, so that all failure reporting from this helper correctly attributes to the caller regardless of future changes.
🤖 Prompt for AI agents
In server/mdm/nanomdm/test/e2e/certauth.go around line 81, review and complete this code-review fix: Hidden test.Helper() call placed after t.Errorf makes error location misleading.
What the draft fix changed: In expectErr (server/mdm/nanomdm/test/e2e/certauth.go), moved `t.Helper()` to be the first statement in the function, before the `errors.Is` check and `t.Errorf` call, so that all failure reporting from this helper correctly attributes to the caller regardless of future changes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| if terr != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("Failed to resolve fleet %q: %v", fleet, terr)), nil | ||
| } | ||
| if len(teamIDs) == 0 { | ||
| return mcp.NewToolResultError(fmt.Sprintf("Failed to resolve fleet %q: no matching team found", fleet)), nil | ||
| } | ||
| compliance, err = fleetClient.GetTeamPolicyCompliance(ctx, fmt.Sprintf("%d", teamIDs[0]), policyID) | ||
| } else { | ||
| compliance, err = fleetClient.GetPolicyCompliance(ctx, policyID) |
There was a problem hiding this comment.
🦩 🟠 registerGetPolicyCompliance ignores fleet-resolution edge case where teamIDs is empty
In registerGetPolicyCompliance (mcp_tools_policies.go), added a if len(teamIDs) == 0 { return mcp.NewToolResultError(...) } guard immediately after the resolveTeamNames call and before indexing teamIDs[0], mirroring the pattern used in registerCreateSavedQuery in mcp_tools_queries.go. This prevents an index-out-of-range panic when resolveTeamNames returns an empty slice with a nil error.
🤖 Prompt for AI agents
In tools/fleet-mcp/mcp_tools_policies.go around line 62, review and complete this code-review fix: registerGetPolicyCompliance ignores fleet-resolution edge case where teamIDs is empty.
What the draft fix changed: In registerGetPolicyCompliance (mcp_tools_policies.go), added a `if len(teamIDs) == 0 { return mcp.NewToolResultError(...) }` guard immediately after the resolveTeamNames call and before indexing teamIDs[0], mirroring the pattern used in registerCreateSavedQuery in mcp_tools_queries.go. This prevents an index-out-of-range panic when resolveTeamNames returns an empty slice with a nil error.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -73,7 +73,10 @@ const TransferHostModal = ({ | |||
| ); | |||
|
|
|||
| const onSubmitTransferHost = useCallback(() => { | |||
There was a problem hiding this comment.
🦩 🟠 TransferHostModal onSubmit typed as ITeam but can receive a synthetic no-team object without required ITeam fields
Removed the unsound as ITeam cast in onSubmitTransferHost. Widened the onSubmit prop type in ITransferHostModal to (team: ITeam | INoTeamOption) => void so the "Unassigned" synthetic option ({ id: "no-team" }) is passed through with its real, accurate type instead of being falsely claimed as a full ITeam. Added a guard (if (selectedTeam === undefined) return;) since onSubmit no longer accepts undefined, replacing the previous cast that silently allowed it. This pushes the type-safety hole to the caller boundary, where onSubmit consumers must now be updated to handle INoTeamOption explicitly (e.g. checking for "name" in team) — that downstream change is outside this file and not verified here, since callers of TransferHostModal are not visible in the given material.
🤖 Prompt for AI agents
In frontend/pages/hosts/components/TransferHostModal/TransferHostModal.tsx around line 75, review and complete this code-review fix: TransferHostModal onSubmit typed as ITeam but can receive a synthetic no-team object without required ITeam fields.
What the draft fix changed: Removed the unsound `as ITeam` cast in `onSubmitTransferHost`. Widened the `onSubmit` prop type in `ITransferHostModal` to `(team: ITeam | INoTeamOption) => void` so the "Unassigned" synthetic option (`{ id: "no-team" }`) is passed through with its real, accurate type instead of being falsely claimed as a full `ITeam`. Added a guard (`if (selectedTeam === undefined) return;`) since `onSubmit` no longer accepts `undefined`, replacing the previous cast that silently allowed it. This pushes the type-safety hole to the caller boundary, where `onSubmit` consumers must now be updated to handle `INoTeamOption` explicitly (e.g. checking for `"name" in team`) — that downstream change is outside this file and not verified here, since callers of `TransferHostModal` are not visible in the given material.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -93,7 +94,7 @@ const DiskEncryption = ({ | |||
| { | |||
There was a problem hiding this comment.
🦩 🟠 currentTeamId === 0 misused as sentinel for 'no team' throughout DiskEncryption and Passwords
Replaced all three hardcoded 0 sentinel checks for "no team" with the named constant API_NO_TEAM_ID, imported from interfaces/team (as done in Passwords.tsx): the enabled: currentTeamId !== 0 option in the useQuery call, the if (currentTeamId === 0) check in onUpdateDiskEncryption, and the if (currentTeamId === 0 && isLoadingTeam) check in the render body. This assumes API_NO_TEAM_ID is exported from interfaces/team (as referenced by the finding's description of Passwords.tsx); if it is instead exported from a different module (e.g. interfaces/team.ts under a different name or from pages/ManageControlsPage/... constants), the import path would need adjustment — I could not view Passwords.tsx or interfaces/team.ts directly to confirm the exact export location.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/OSSettings/cards/DiskEncryption/DiskEncryption.tsx around line 93, review and complete this code-review fix: currentTeamId === 0 misused as sentinel for 'no team' throughout DiskEncryption and Passwords.
What the draft fix changed: Replaced all three hardcoded `0` sentinel checks for "no team" with the named constant `API_NO_TEAM_ID`, imported from `interfaces/team` (as done in Passwords.tsx): the `enabled: currentTeamId !== 0` option in the `useQuery` call, the `if (currentTeamId === 0)` check in `onUpdateDiskEncryption`, and the `if (currentTeamId === 0 && isLoadingTeam)` check in the render body. This assumes `API_NO_TEAM_ID` is exported from `interfaces/team` (as referenced by the finding's description of Passwords.tsx); if it is instead exported from a different module (e.g. `interfaces/team.ts` under a different name or from `pages/ManageControlsPage/...` constants), the import path would need adjustment — I could not view Passwords.tsx or interfaces/team.ts directly to confirm the exact export location.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| renderFlash("error", "Couldn't update settings. Please try again."); | ||
| } | ||
|
|
||
| setIsUpdating(false); |
There was a problem hiding this comment.
🦩 🟠 onSubmit resets lockEndUserInfo state using stale closure value of canLockEndUserInfo
In onSubmit (UsersForm.tsx), moved the setFormData((prev) => ({ ...prev, lockEndUserInfo: canLockEndUserInfo })) call from after the try/catch block (where it ran unconditionally regardless of success/failure) to inside the try block immediately after the success flash message. This ensures the local lockEndUserInfo state is only synced to the submitted canLockEndUserInfo value when the update actually succeeds; on failure the state is left untouched. The setIsUpdating(false) call remains after the try/catch so the loading state clears in both success and failure cases. This does not fully eliminate the residual risk of clobbering concurrent in-flight edits made by the user while the request is pending, since the fix only gates on success/failure rather than diffing against the latest formData — a complete fix would need to only apply fields the request actually changed or compare against current state before overwriting.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx around line 123, review and complete this code-review fix: onSubmit resets lockEndUserInfo state using stale closure value of canLockEndUserInfo.
What the draft fix changed: In `onSubmit` (UsersForm.tsx), moved the `setFormData((prev) => ({ ...prev, lockEndUserInfo: canLockEndUserInfo }))` call from after the try/catch block (where it ran unconditionally regardless of success/failure) to inside the `try` block immediately after the success flash message. This ensures the local `lockEndUserInfo` state is only synced to the submitted `canLockEndUserInfo` value when the update actually succeeds; on failure the state is left untouched. The `setIsUpdating(false)` call remains after the try/catch so the loading state clears in both success and failure cases. This does not fully eliminate the residual risk of clobbering concurrent in-flight edits made by the user while the request is pending, since the fix only gates on success/failure rather than diffing against the latest `formData` — a complete fix would need to only apply fields the request actually changed or compare against current state before overwriting.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| SETUP_EXPERIENCE_NAV_ITEMS.find((item) => item.urlSection === section) ?? | ||
| DEFAULT_SETTINGS_SECTION; | ||
|
|
||
| if ( |
There was a problem hiding this comment.
🦩 🟠 SetupExperience calls router.replace on every render for non-install-software sections, causing redundant navigation
In SetupExperience component, moved the router.replace side effect out of the render body and into a useEffect hook (imported useEffect from React) that depends on currentFormSection.urlSection, urlPlatformParam, and queryString. This prevents router.replace from being invoked as a side effect of rendering, avoiding redundant navigation calls on every render/re-render. The premium-tier early return remains but is now placed after hook declarations to satisfy the Rules of Hooks (hooks must not be called conditionally), since currentFormSection computation and the effect must run before any early return.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/SetupExperience/SetupExperience.tsx around line 45, review and complete this code-review fix: SetupExperience calls router.replace on every render for non-install-software sections, causing redundant navigation.
What the draft fix changed: In `SetupExperience` component, moved the `router.replace` side effect out of the render body and into a `useEffect` hook (imported `useEffect` from React) that depends on `currentFormSection.urlSection`, `urlPlatformParam`, and `queryString`. This prevents `router.replace` from being invoked as a side effect of rendering, avoiding redundant navigation calls on every render/re-render. The premium-tier early return remains but is now placed after hook declarations to satisfy the Rules of Hooks (hooks must not be called conditionally), since `currentFormSection` computation and the effect must run before any early return.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
|
|
||
| // Include showVersions — the titles view can have installers even when | ||
| // the versions view is empty, so the toggle should stay interactive. | ||
| const isTrulyEmpty = |
There was a problem hiding this comment.
🦩 🟠 controlsDisabled logic can leave search/filter controls enabled while data is undefined, causing crash on empty responses
Changed the isTrulyEmpty calculation in SoftwareTable (SoftwareInventoryTable.tsx) so it no longer short-circuits to false unconditionally on showVersions === true. Now, when showVersions is true, isTrulyEmpty is !hasData && !hasQuery && !hasVulnFilters && !installableSoftwareExists — i.e. it also considers whether installable software exists elsewhere (a prop already passed to this component and used by EmptySoftwareTable) rather than blindly treating the versions view as never truly empty. When showVersions is false, the check remains !hasData && !hasQuery && !hasVulnFilters as before. This preserves the existing test expectations (titles view empty ⇒ disabled; versions view empty with installableSoftwareExists defaulting falsy in the "no data" test ⇒ now correctly evaluates based on that flag) while fixing the described bug that controls stayed enabled unconditionally whenever showVersions was true. Risk: this assumes the existing test 'Keeps controls enabled when versions toggle is applied but no data' passes installableSoftwareExists={true} (or otherwise expects controls enabled because installables exist) — if that test instead relies on the old unconditional bypass with installableSoftwareExists false/undefined, it would now fail and require updating; I could not view the test file to confirm this, so a full fix may need a corresponding test adjustment.
🤖 Prompt for AI agents
In frontend/pages/SoftwarePage/SoftwareInventory/SoftwareInventoryTable/SoftwareInventoryTable.tsx around line 181, review and complete this code-review fix: controlsDisabled logic can leave search/filter controls enabled while data is undefined, causing crash on empty responses.
What the draft fix changed: Changed the `isTrulyEmpty` calculation in `SoftwareTable` (SoftwareInventoryTable.tsx) so it no longer short-circuits to `false` unconditionally on `showVersions === true`. Now, when `showVersions` is true, `isTrulyEmpty` is `!hasData && !hasQuery && !hasVulnFilters && !installableSoftwareExists` — i.e. it also considers whether installable software exists elsewhere (a prop already passed to this component and used by `EmptySoftwareTable`) rather than blindly treating the versions view as never truly empty. When `showVersions` is false, the check remains `!hasData && !hasQuery && !hasVulnFilters` as before. This preserves the existing test expectations (titles view empty ⇒ disabled; versions view empty with `installableSoftwareExists` defaulting falsy in the "no data" test ⇒ now correctly evaluates based on that flag) while fixing the described bug that controls stayed enabled unconditionally whenever `showVersions` was true. Risk: this assumes the existing test 'Keeps controls enabled when versions toggle is applied but no data' passes `installableSoftwareExists={true}` (or otherwise expects controls enabled because installables exist) — if that test instead relies on the old unconditional bypass with `installableSoftwareExists` false/undefined, it would now fail and require updating; I could not view the test file to confirm this, so a full fix may need a corresponding test adjustment.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 40 review findings across 40 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditAutoUpdateConfigModal/helpers.tsx:68orbit/pkg/table/pmset/pmset_darwin.go:68server/datastore/mysql/migrations/openframe/20260301000002_AddQueryHostsJoinTable.go:32server/mdm/nanomdm/test/e2e/certauth.go:81tools/fleet-mcp/mcp_tools_policies.go:62ee/cis/macos-14/test/scripts/CIS_2.6.2.sh:3ee/maintained-apps/inputs/homebrew/scripts/cleanmymac-uninstall.sh:91frontend/hooks/useSoftwareInstallerMeta.ts:136frontend/pages/DashboardPage/cards/OperatingSystems/OSTable.tsx:18frontend/pages/labels/ManageLabelsPage/ManageLabelsPage.tsx:103frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareTable/InstallSoftwareTableConfig.tsx:107frontend/pages/policies/edit/components/PolicyErrorsTable/PolicyErrorsTableConfig.tsx:73orbit/pkg/keystore/keystore_darwin.go:112orbit/pkg/table/diskutil/apfs/apfs_darwin.go:158pkg/download/download.go:144server/activity/internal/mysql/new_activity.go:116server/aws_common/iam_auth.go:47server/datastore/mysql/cron_stats.go:71server/datastore/mysql/migrations/tables/20210601000008_TeamsEnrollSecrets.go:55server/datastore/mysql/migrations/tables/20210818151827_RemoveForeignKeysSchedQStats.go:40sha256shadows the importedcrypto/sha256packageserver/datastore/mysql/migrations/tables/20250701155654_AddEULAHashColumn_test.go:16server/mdm/acme/internal/service/challenge.go:129server/service/client_appconfig_test.go:63tools/mdm/assets/main.go:74website/assets/js/pages/docs/command-details.page.js:22cmd/osquery-perf/osquery_perf/stats.go:276frontend/pages/DashboardPage/cards/WelcomeHost/WelcomeHost.tsx:228tools/terraform/fleetdm_client/fleetdm_client_test.go:35platform_likewarning pushed but not part of returned row shape mismatch)ee/fleetd-chrome/src/tables/os_version.ts:91ee/maintained-apps/ingesters/winget/external_refs/main.go:18ee/maintained-apps/inputs/homebrew/scripts/docker_desktop_install.sh:6ee/vulnerability-dashboard/api/helpers/get-vulnerabilities.js:141frontend/components/ActivityDetails/InstallDetails/SoftwareScriptDetailsModal/SoftwareScriptDetailsModal.tsx:45frontend/interfaces/mdm.ts:336frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx:123frontend/pages/hosts/components/TransferHostModal/TransferHostModal.tsx:75frontend/pages/ManageControlsPage/OSSettings/cards/DiskEncryption/DiskEncryption.tsx:93frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx:123frontend/pages/ManageControlsPage/SetupExperience/SetupExperience.tsx:45frontend/pages/SoftwarePage/SoftwareInventory/SoftwareInventoryTable/SoftwareInventoryTable.tsx:181What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
1446a072-096e-4294-8082-c7cadffe76deMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akj32d7 FleetMDM bulk review findings sweep (12 PRs)