fix(adhoc-sweep-fixes): 40 review findings across 40 files - #123
fix(adhoc-sweep-fixes): 40 review findings across 40 files#123flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -91,11 +91,10 @@ const ActivityItem = ({ | |||
| ? addGravatarUrlToResource({ email: actor_email }) | |||
| : { gravatar_url: undefined }; | |||
There was a problem hiding this comment.
🦩 🟠 ActivityItem silently swallows Date parse errors without any recovery signal
In ActivityItem, replaced the dead try/catch around new Date(activity.created_at) with a direct construction followed by an isNaN(activityCreatedAt.getTime()) check that falls back to new Date() when the parsed date is invalid. This matches the finding's suggested fix and removes the false sense of safety from the unreachable catch block.
🤖 Prompt for AI agents
In frontend/components/ActivityItem/ActivityItem.tsx around line 92, review and complete this code-review fix: ActivityItem silently swallows Date parse errors without any recovery signal.
What the draft fix changed: In `ActivityItem`, replaced the dead try/catch around `new Date(activity.created_at)` with a direct construction followed by an `isNaN(activityCreatedAt.getTime())` check that falls back to `new Date()` when the parsed date is invalid. This matches the finding's suggested fix and removes the false sense of safety from the unreachable catch block.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| // If we want to actual do file uploading, wrap in a label that | ||
| // references the hidden file input. Otherwise just use a span. | ||
| if (!onButtonClick) { | ||
| buttonMarkup = <label htmlFor="upload-file">{buttonMarkup}</label>; | ||
| buttonMarkup = <label htmlFor={fileInputId}>{buttonMarkup}</label>; | ||
| } else { | ||
| buttonMarkup = <span>{buttonMarkup}</span>; | ||
| } |
There was a problem hiding this comment.
🦩 🟠 Hardcoded input id="upload-file" duplicated across FileUploader instances causes DOM id collisions
In FileUploader, added const fileInputId = useId(); and replaced the hardcoded literal "upload-file" with fileInputId on both the <label htmlFor={fileInputId}> (in renderUploadButton) and the hidden <input id={fileInputId}> (in renderFileUploader), ensuring each mounted instance gets a unique DOM id and label click delegation works correctly for multiple instances on the same page.
🤖 Prompt for AI agents
In frontend/components/FileUploader/FileUploader.tsx around line 172, review and complete this code-review fix: Hardcoded input id="upload-file" duplicated across FileUploader instances causes DOM id collisions.
What the draft fix changed: In `FileUploader`, added `const fileInputId = useId();` and replaced the hardcoded literal `"upload-file"` with `fileInputId` on both the `<label htmlFor={fileInputId}>` (in `renderUploadButton`) and the hidden `<input id={fileInputId}>` (in `renderFileUploader`), ensuring each mounted instance gets a unique DOM id and label click delegation works correctly for multiple instances on the same page.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| const { document } = global; | ||
| const { wrapperHeight } = this; | ||
|
|
||
| const lookForOuterMenu = setInterval(() => { | ||
| this.lookForOuterMenu = setInterval(() => { | ||
| if (!this.mounted) { | ||
| clearInterval(this.lookForOuterMenu); | ||
| return; | ||
| } | ||
|
|
||
| if (document.querySelectorAll(".Select-menu-outer")) { | ||
| clearInterval(lookForOuterMenu); | ||
| clearInterval(this.lookForOuterMenu); | ||
| const coreWrapper = document.querySelector(".core-wrapper"); | ||
|
|
||
| if (!coreWrapper) { | ||
| return; | ||
| } | ||
|
|
||
| const currentWrapperHeight = coreWrapper.scrollHeight; | ||
| if (wrapperHeight < currentWrapperHeight) { | ||
| coreWrapper.style.height = `${ |
There was a problem hiding this comment.
🦩 🟠 onInputOpen's setInterval polling for '.Select-menu-outer' never has an explicit unmount guard tied to component unmount
In onInputOpen, the interval id is now stored on this.lookForOuterMenu (instead of a local const) and componentWillUnmount now calls clearInterval(this.lookForOuterMenu) to guarantee the poll stops on unmount. Additionally, the interval callback now checks this.mounted first (clearing and returning early if unmounted) and guards coreWrapper being null before dereferencing .scrollHeight, matching the mounted-check pattern used elsewhere in the file (e.g. fetchTargets). This directly addresses the missing unmount guard and the potential throw from document.querySelector('.core-wrapper') returning null after unmount. The pre-existing querySelectorAll always-truthy logic bug was left unchanged since fixing it was outside the scope of this finding.
🤖 Prompt for AI agents
In frontend/components/forms/fields/SelectTargetsDropdown/SelectTargetsDropdown.jsx around line 79, review and complete this code-review fix: onInputOpen's setInterval polling for '.Select-menu-outer' never has an explicit unmount guard tied to component unmount.
What the draft fix changed: In `onInputOpen`, the interval id is now stored on `this.lookForOuterMenu` (instead of a local const) and `componentWillUnmount` now calls `clearInterval(this.lookForOuterMenu)` to guarantee the poll stops on unmount. Additionally, the interval callback now checks `this.mounted` first (clearing and returning early if unmounted) and guards `coreWrapper` being null before dereferencing `.scrollHeight`, matching the mounted-check pattern used elsewhere in the file (e.g. `fetchTargets`). This directly addresses the missing unmount guard and the potential throw from `document.querySelector('.core-wrapper')` returning null after unmount. The pre-existing `querySelectorAll` always-truthy logic bug was left unchanged since fixing it was outside the scope of this finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| return availableTeams | ||
| ?.filter((t) => t.name !== "All fleets") | ||
| .map((t) => ({ | ||
| value: t.name, |
There was a problem hiding this comment.
🦩 🟠 getOptions is exported but EditTeamsAbmModal component duplicates the same filter/map logic inline via useMemo instead of calling it
In EditTeamsAbmModal component, replaced the inline useMemo body that duplicated the filter/map logic with a direct call to the exported getOptions(availableTeams) function, keeping the same [availableTeams] dependency array. This removes the duplicated logic while preserving identical behavior.
🤖 Prompt for AI agents
In frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsx around line 100, review and complete this code-review fix: getOptions is exported but EditTeamsAbmModal component duplicates the same filter/map logic inline via useMemo instead of calling it.
What the draft fix changed: In `EditTeamsAbmModal` component, replaced the inline `useMemo` body that duplicated the filter/map logic with a direct call to the exported `getOptions(availableTeams)` function, keeping the same `[availableTeams]` dependency array. This removes the duplicated logic while preserving identical behavior.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| requestData.mfa_enabled = formData.mfa_enabled; | ||
| if (formData.global_role !== entityData.global_role) | ||
| requestData.global_role = formData.global_role; | ||
| if (formData.teams && formData.teams.length > 0) | ||
| requestData.teams = formData.teams; | ||
| if (formData.teams) requestData.teams = formData.teams; | ||
| if (formData.new_password) requestData.new_password = formData.new_password; | ||
|
|
||
| let successMessage = `Successfully edited ${formData.name}`; |
There was a problem hiding this comment.
🦩 🟠 EditUserPage teams diff logic can never clear a user's last team assignment
In handleHumanUserSubmit in EditUserPage.tsx, changed the condition if (formData.teams && formData.teams.length > 0) requestData.teams = formData.teams; to if (formData.teams) requestData.teams = formData.teams;. This allows an empty array (all teams removed) to be included in the request payload, so the backend receives the cleared team list instead of silently keeping the old assignments, while still omitting the field when formData.teams is undefined/null (e.g., not applicable for this user). Risk: if formData.teams defaults to an empty array rather than undefined for users/forms where teams aren't managed, this could now send an unintended empty teams update; verifying the UserForm's default/initial value for teams would give full confidence.
🤖 Prompt for AI agents
In frontend/pages/admin/ManageUsersPage/EditUserPage/EditUserPage.tsx around line 136, review and complete this code-review fix: EditUserPage teams diff logic can never clear a user's last team assignment.
What the draft fix changed: In `handleHumanUserSubmit` in EditUserPage.tsx, changed the condition `if (formData.teams && formData.teams.length > 0) requestData.teams = formData.teams;` to `if (formData.teams) requestData.teams = formData.teams;`. This allows an empty array (all teams removed) to be included in the request payload, so the backend receives the cleared team list instead of silently keeping the old assignments, while still omitting the field when `formData.teams` is `undefined`/`null` (e.g., not applicable for this user). Risk: if `formData.teams` defaults to an empty array rather than `undefined` for users/forms where teams aren't managed, this could now send an unintended empty teams update; verifying the UserForm's default/initial value for `teams` would give full confidence.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| @@ -81,7 +81,7 @@ module.exports = { | |||
| } else { | |||
| // If an error was logged for a entra tenant, log the error, and increment the numberOfLoggedErrors | |||
There was a problem hiding this comment.
🦩 🔵 Misspelled tenant name in warning log message
Fixed the typo 'Microsfot' to 'Microsoft' in the sails.log.warn call inside the fn function's error-reporting loop at the end of the script, where warnings are logged for failed Entra tenant heartbeat requests.
🤖 Prompt for AI agents
In website/scripts/send-entra-heartbeat-requests.js around line 82, review and complete this code-review fix: Misspelled tenant name in warning log message.
What the draft fix changed: Fixed the typo 'Microsfot' to 'Microsoft' in the sails.log.warn call inside the fn function's error-reporting loop at the end of the script, where warnings are logged for failed Entra tenant heartbeat requests.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -23,7 +23,7 @@ func TestFSClient(t *testing.T) { | |||
| t.Run("MacOfficeReleaseNotes", func(t *testing.T) { | |||
| t.Run("directory does not exists", func(t *testing.T) { | |||
| sut := NewFSClient("asdf") | |||
There was a problem hiding this comment.
🦩 🔵 MacOfficeReleaseNotes error check duplicates MSRCBulletins call instead of its own method in fs_test.go
In TestFSClient's "MacOfficeReleaseNotes" > "directory does not exists" subtest, changed sut.MSRCBulletins() to sut.MacOfficeReleaseNotes() so the test correctly exercises the MacOfficeReleaseNotes error path instead of duplicating the MSRCBulletins test.
🤖 Prompt for AI agents
In server/vulnerabilities/io/fs_test.go around line 25, review and complete this code-review fix: MacOfficeReleaseNotes error check duplicates MSRCBulletins call instead of its own method in fs_test.go.
What the draft fix changed: In TestFSClient's "MacOfficeReleaseNotes" > "directory does not exists" subtest, changed `sut.MSRCBulletins()` to `sut.MacOfficeReleaseNotes()` so the test correctly exercises the MacOfficeReleaseNotes error path instead of duplicating the MSRCBulletins test.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| fn func(t *testing.T, ds *Datastore) | ||
| }{ | ||
| {"QueueAndProcessJobs", testQueueAndProcessJobs}, | ||
| {"QueueAndProcessJobs", testQueueAndProcessFilteredJobs}, | ||
| {"QueueAndProcessFilteredJobs", testQueueAndProcessFilteredJobs}, | ||
| {"CleanupWorkerJobs", testCleanupWorkerJobs}, | ||
| } | ||
| for _, c := range cases { |
There was a problem hiding this comment.
🦩 🔵 Duplicate test case name in TestJobs table hides the second subtest under an identical -run selector
In TestJobs, renamed the second cases-slice entry's name field from "QueueAndProcessJobs" to "QueueAndProcessFilteredJobs" (which maps to testQueueAndProcessFilteredJobs) so each t.Run subtest has a unique, descriptive name and can be selected independently via go test -run.
🤖 Prompt for AI agents
In server/datastore/mysql/jobs_test.go around line 19, review and complete this code-review fix: Duplicate test case name in TestJobs table hides the second subtest under an identical -run selector.
What the draft fix changed: In TestJobs, renamed the second cases-slice entry's name field from "QueueAndProcessJobs" to "QueueAndProcessFilteredJobs" (which maps to testQueueAndProcessFilteredJobs) so each t.Run subtest has a unique, descriptive name and can be selected independently via `go test -run`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| MigrationClient.AddMigration(Up_20260518194422, Down_20260518194422) | ||
| } | ||
|
|
||
| // Up_20260514220719 adds the encoding_type column that discriminates between |
There was a problem hiding this comment.
🦩 🔵 Migration function doc comment references wrong migration ID (copy-paste artifact)
Changed the doc comment above Up_20260518194422 from "Up_20260514220719 adds the encoding_type column..." to "Up_20260518194422 adds the encoding_type column...", correcting the copy-paste artifact so the comment references the correct migration function/ID it documents.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20260518194422_AddEncodingTypeToHostSCDData.go around line 12, review and complete this code-review fix: Migration function doc comment references wrong migration ID (copy-paste artifact).
What the draft fix changed: Changed the doc comment above Up_20260518194422 from "Up_20260514220719 adds the encoding_type column..." to "Up_20260518194422 adds the encoding_type column...", correcting the copy-paste artifact so the comment references the correct migration function/ID it documents.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| id, err := store.EnrollmentFromHash(r.Context(), hasher(cert)) | ||
| if err != nil { | ||
| ctxlog.Logger(r.Context(), logger).Info( | ||
| "msg", "retreiving enrollment from hash", | ||
| "msg", "retrieving enrollment from hash", | ||
| "err", err, | ||
| ) | ||
| http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) |
There was a problem hiding this comment.
🦩 🔵 Misspelled log message field 'retreiving' in mdm_cert.go
Fixed the typo 'retreiving' to 'retrieving' in the log message string within the CertWithEnrollmentIDMiddleware function's error handling branch (around the store.EnrollmentFromHash call), matching exactly the finding's evidence and location.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In server/mdm/nanomdm/http/mdm/mdm_cert.go around line 214, review and complete this code-review fix: Misspelled log message field 'retreiving' in mdm_cert.go.
What the draft fix changed: Fixed the typo 'retreiving' to 'retrieving' in the log message string within the CertWithEnrollmentIDMiddleware function's error handling branch (around the `store.EnrollmentFromHash` call), matching exactly the finding's evidence and location.
_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
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/components/ActivityItem/ActivityItem.tsx:92frontend/components/FileUploader/FileUploader.tsx:172frontend/components/forms/fields/SelectTargetsDropdown/SelectTargetsDropdown.jsx:79frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsx:100frontend/pages/admin/ManageUsersPage/EditUserPage/EditUserPage.tsx:136frontend/pages/ManageControlsPage/OSUpdates/OSUpdates.tsx:104routerandteamIdForApi, risking stale table configfrontend/pages/SoftwarePage/components/tables/SoftwareVulnerabilitiesTable/SoftwareVulnerabilitiesTable.tsx:106frontend/test/handlers/device-handler.ts:100frontend/utilities/software_uninstall_scripts.ts:18orbit/pkg/cryptoinfo/info.go:20orbit/pkg/platform/platform_notwindows.go:74orbit/pkg/table/diskutil/apfs/apfs_darwin.go:78orbit/pkg/table/tcc_access/tcc_access.go:178server/cron/calendar_cron.go:316server/datastore/mysql/migrations/tables/20220524102918_CleanupOrphanedPolicyMemberships_test.go:18server/datastore/mysql/migrations/tables/20241210140021_AddErrorsToCronStatsTable.go:8server/datastore/mysql/queries.go:218val.(*mdm.BootstrapToken)will panic if execStores returns nil interface on errorserver/mdm/nanomdm/storage/allmulti/bstoken.go:14server/mdm/reconcile/reconcile_test.go:40server/mdm/scep/depot/bolt/depot.go:178server/service/testing_utils_test.go:254tools/github-manage/pkg/ghapi/milestone.go:166website/api/controllers/android-proxy/create-android-enterprise.js:152website/api/controllers/android-proxy/delete-one-android-enterprise.js:83website/api/controllers/customers/get-stripe-checkout-session-url.js:33frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/components/DeleteBootstrapPackageModal/DeleteBootstrapPackageModal.tsx:27it-and-security/lib/linux/scripts/uninstall-fleetd-linux.sh:2pkg/fleetdbase/fleetd_base.go:1server/datastore/mysql/migrations/tables/20240829170023_CreateVPPTokenTeamsJoinTable.go:13server/mdm/nanomdm/service/nanomdm/service.go:1frontend/pages/hosts/details/cards/Policies/HostPoliciesTable/PolicyFailingCount/PolicyFailingCount.tsx:46tools/github-manage/pkg/ghapi/user.go:20frontend/pages/admin/IntegrationsPage/cards/Integrations/components/IntegrationForm/IntegrationForm.tsx:21server/datastore/mysql/migrations/tables/20251028140000_CreateTableOSVersionVulnerabilities.go:68website/assets/js/pages/articles/basic-comparison.page.js:27website/scripts/send-entra-heartbeat-requests.js:82server/vulnerabilities/io/fs_test.go:25server/datastore/mysql/jobs_test.go:19server/datastore/mysql/migrations/tables/20260518194422_AddEncodingTypeToHostSCDData.go:12server/mdm/nanomdm/http/mdm/mdm_cert.go:214What 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:
f6d23861-693f-45a1-b5b3-570aa3bc9ea7Merging 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.