fix(adhoc-sweep-fixes): CU-86akhf8u5 40 review findings across 33 files

[flamingo]

Closes 40 review findings across 33 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

Warning

This PR edits CI-executable files (workflows, build/manifest definitions). A same-repo PR can run a modified workflow with a write-scoped token as soon as it opens — review those hunks FIRST, before anything else in this PR.

#	Fix confidence	Finding	Location
1	🟢 95 high	lint job checkout step lacks persist-credentials: false while processing PR content	.github/workflows/test.yml:53
2	🟢 95 high	build_image job checkout step lacks persist-credentials: false while building/pushing images from untrusted PR content	.github/workflows/test.yml:84
3	🟢 90 high	test.yml checkout steps omit persist-credentials: false	.github/workflows/test.yml:34
4	🟡 70 medium	handleTunnelStateChange resets loadingPath on disconnect but nothing ever sets it, and case 0 path leaves pendingRequests unrejected	src/lib/meshcentral/file-manager.ts:217
5	🔴 55 low — review closely	handleDirectoryListing resolves an arbitrary pending request when reqid is missing, risking cross-request data corruption	src/lib/meshcentral/file-manager.ts:459
6	🔴 40 low — review closely	handleJsonMessage silently drops binary-looking or truncated JSON without ever resolving/rejecting pending requests	src/lib/meshcentral/file-manager.ts:320
7	🔴 45 low — review closely	onSubmit payload always sends platform: undefined for policy create/update, likely dropping a required field silently	src/app/(app)/monitoring/policy/components/edit-policy-page.tsx:136
8	🟡 70 medium	Unconditional setState during render can trigger infinite render loops if hosts array reference changes each fetch	src/app/(app)/monitoring/policy/components/edit-policy-page.tsx:62
9	🔴 55 low — review closely	secondsToUnitValue can silently misclassify a non-multiple interval as minutes with rounding, hiding the true stored value from the user	src/app/(app)/monitoring/query/components/edit-query-page.tsx:44
10	🔴 15 low — review closely	Nearly identical host-selection/device-selector logic duplicated between policy and query edit pages	src/app/(app)/monitoring/query/components/edit-query-page.tsx:96
11	🟡 85 medium	fetchChunks silently swallows failures by returning an empty array	src/app/(app)/tickets/services/ticket-service.ts:409
12	🟡 70 medium	reorderTicket sends both afterTicketId and beforeTicketId from a params object without narrowing null vs undefined semantics	src/app/(app)/tickets/services/ticket-service.ts:344
13	🔴 55 low — review closely	useIsApplePlatform hardcoded to always return true — temporary override risks shipping to production indefinitely	src/app/hooks/use-apple-platform.ts:1
14	🟢 95 high	getFileBlob performs a throwaway FileReader.readAsArrayBuffer read with no listener, wasting work and risking confusion	src/lib/meshcentral/file-downloader.ts:247
15	🟢 90 high	reconnectionCount effect can silently stop notifying after a counter reset/overflow edge case	src/app/(app)/tickets/components/ticket-dialog-subscription.tsx:43
16	🟢 90 high	discoverTenants swallows the discarded-in-flight response but never records the stale error path	src/app/(auth)/auth/hooks/use-auth.ts:137
17	🟡 60 medium	applyAssignmentsDiff fires all assign/unassign mutations in parallel with Promise.all, so a single failure aborts remaining requests and can leave assignments partially applied	src/components/assignments/apply-assignments-diff.ts:66
18	🟡 88 medium	acceptInvitationSso errors are caught but isSubmitting is only reset on the catch path, leaving stuck spinner if redirect fails silently	src/app/(auth)/auth/invite/page.tsx:102
19	🟡 85 medium	setState called unconditionally during render body in DeviceDetailsView	src/app/(app)/devices/components/device-details-view.tsx:86
20	🔴 55 low — review closely	buildInstallCommand darwin branch mislabels non-macOS Unix platforms as macOS	src/app/(app)/devices/utils/device-command-utils.ts:60
21	🔴 35 low — review closely	Waitlist submission posts to a hardcoded external content-api host, bypassing runtimeEnv config layer	src/app/(auth)/auth/components/benefits-section.tsx:28
22	🟡 75 medium	deleteDevice swallows non-Error rejection reasons into a generic message	src/app/(app)/devices/hooks/use-device-actions.ts:40
23	🔴 30 low — review closely	Identical catch-block comment duplicated verbatim across four modal components	src/app/(app)/knowledge-base/components/archive-article-modal.tsx:35
24	🔴 35 low — review closely	deviceCount is mutated by delta twice per commit but payload semantics assumed absolute-only without reconciliation guard	src/app/(app)/scripts/schedule/utils/schedule-assignment-updaters.ts:89
25	🟡 85 medium	Promise.allSettled failure branch loses the second rejection reason	src/app/(app)/settings/ai-settings/components/ai-settings-view.tsx:148
26	🟢 92 high	applyHeldMove overwrites earlier without filter result inconsistently for empty vs non-empty diff	src/app/(app)/tickets/components/tickets-board.tsx:75
27	🔴 45 low — review closely	useUpdateTicket's mutation runs multiple sequential GraphQL calls without atomicity or partial-failure handling	src/app/(app)/tickets/hooks/use-update-ticket.ts:39
28	🟡 70 medium	login() reducer merges partial user fields only for matching id, but replaces wholesale otherwise — potential loss of previously-fetched full-profile fields on re-login	src/app/(auth)/auth/stores/auth-store.ts:156
29	🔴 55 low — review closely	getFullImageUrl(nullish, hash) coalesced with ?? even though it likely never returns null	src/app/(app)/settings/ai-settings/components/previews/meet-fae-preview.tsx:50
30	🟡 75 medium	DeleteStatusDialog seeds state from props during render via a manual prev-state comparison instead of useEffect, which is a fragile React anti-pattern despite the justifying comment	src/app/(app)/tickets/statuses/components/delete-status-dialog.tsx:40
31	🔴 55 low — review closely	openNotificationTarget's own doc comment flags a known duplicated decision path that has already drifted	src/app/components/notifications/notifications-data-provider.tsx:314
32	🟡 75 medium	detectDeployment's cloud pattern list can misclassify any hostname containing 'app.openframe' as substring, including unrelated domains	src/lib/deployment-detector.ts:25
33	🔴 55 low — review closely	useEffect refresh timer keyed only on refreshParam without dependency on ref stability can silently no-op	src/app/(app)/devices/components/tabs/overview-tab.tsx:28
34	🟡 85 medium	handleUnpublish silently swallows and mismatches the toast copy from handlePublish	src/app/(app)/knowledge-base/components/article-details-page.tsx:115
35	🟢 92 high	copyDialogLink strips trailing slashes from origin but mingoDialogLink() path composition risk of double slash not guarded	src/app/components/openframe-embeddable-chat-entry.tsx:132
36	🔴 55 low — review closely	appStateChange listener registration errors are only logged, native back/resume silently disabled	src/app/components/token-freshness-watcher.tsx:31
37	🟡 85 medium	writeCookie does not validate cookie name or value against CRLF/control-character injection	src/lib/cookies.ts:32
38	🔴 55 low — review closely	unreadNotificationCount and escalation/resolution fields selected unconditionally creates a deploy-ordering hazard documented but not enforced	src/app/(app)/tickets/queries/ticket-queries.ts:248
39	🟢 95 high	LogDrawer infoFields uses field.label as React key, risking duplicate-key collisions when labels repeat	src/app/components/shared/log-drawer.tsx:129
40	🟡 85 medium	getDomainSuffix falls back to full hostname when hostname has fewer than 2 dot-separated parts, unlike sharedUrl branch which always slices last two segments	src/lib/auth-api-client.ts:21

What 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: c69bf2d8-6eaa-4e2d-815b-af08b880c8df

Merging 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-86akhf8u5 OpenFrame OSS frontend review findings sweep (12 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

40 finding(s) fixed in this draft — 40 explained inline on the diff; 16 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🟠 lint job checkout step lacks persist-credentials: false while processing PR content

Added persist-credentials: false to the lint job's Checkout step (job lint, step Checkout), matching the scan job's pattern, so the credential is not persisted while npm ci and ESLint run over untrusted PR content.

🤖 Prompt for AI agents

In .github/workflows/test.yml around line 53, review and complete this code-review fix: lint job checkout step lacks persist-credentials: false while processing PR content.
What the draft fix changed: Added `persist-credentials: false` to the lint job's Checkout step (job `lint`, step `Checkout`), matching the scan job's pattern, so the credential is not persisted while npm ci and ESLint run over untrusted PR content.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 build_image job checkout step lacks persist-credentials: false while building/pushing images from untrusted PR content

Added persist-credentials: false to the build_image job's Checkout step (job build_image, step Checkout), so the persisted token is not available during the docker build/push steps that run with packages:write permissions.

🤖 Prompt for AI agents

In .github/workflows/test.yml around line 84, review and complete this code-review fix: build_image job checkout step lacks persist-credentials: false while building/pushing images from untrusted PR content.
What the draft fix changed: Added `persist-credentials: false` to the build_image job's Checkout step (job `build_image`, step `Checkout`), so the persisted token is not available during the docker build/push steps that run with packages:write permissions.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 test.yml checkout steps omit persist-credentials: false

Same mechanism as findings 1 and 2: all three actions/checkout steps in this file (scan, lint, build_image) now consistently set persist-credentials: false, satisfying the org-wide convention cited (OPENFRAM-010-5 / REGISTRY-008-2). The scan job already had this set and was unchanged.

🤖 Prompt for AI agents

In .github/workflows/test.yml around line 34, review and complete this code-review fix: test.yml checkout steps omit persist-credentials: false.
What the draft fix changed: Same mechanism as findings 1 and 2: all three actions/checkout steps in this file (scan, lint, build_image) now consistently set `persist-credentials: false`, satisfying the org-wide convention cited (OPENFRAM-010-5 / REGISTRY-008-2). The scan job already had this set and was unchanged.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 handleTunnelStateChange resets loadingPath on disconnect but nothing ever sets it, and case 0 path leaves pendingRequests unrejected

In handleTunnelStateChange (case 0, tunnel state 0/disconnected), added a call to a new private helper rejectAllPendingRequests(new Error('Tunnel disconnected')) which clears every timeout and rejects then removes all entries from pendingRequests, before calling setState('disconnected'). This ensures in-flight directory listings, uploads, etc. no longer hang forever when the tunnel drops. The helper mirrors the existing reject loop already used in disconnect().

🤖 Prompt for AI agents

In src/lib/meshcentral/file-manager.ts around line 217, review and complete this code-review fix: handleTunnelStateChange resets loadingPath on disconnect but nothing ever sets it, and case 0 path leaves pendingRequests unrejected.
What the draft fix changed: In `handleTunnelStateChange` (case 0, tunnel state 0/disconnected), added a call to a new private helper `rejectAllPendingRequests(new Error('Tunnel disconnected'))` which clears every timeout and rejects then removes all entries from `pendingRequests`, before calling `setState('disconnected')`. This ensures in-flight directory listings, uploads, etc. no longer hang forever when the tunnel drops. The helper mirrors the existing reject loop already used in `disconnect()`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 handleDirectoryListing resolves an arbitrary pending request when reqid is missing, risking cross-request data corruption

In sendOperation, the pending request entry now records a type field (from request.action) so pending requests can be identified by kind; the pendingRequests map's value type gained an optional type?: string field. In handleDirectoryListing's reqid-less fallback branch, the loop now only resolves a pending request whose type === 'ls', and a console.warn was added noting the fallback path was triggered before scanning. This prevents resolving unrelated (e.g. upload-hash, search) promises with directory-listing data. Risk: relies on FileOperationRequest's shape having an action field consistent with 'ls' for list-directory requests (confirmed via createListDirectoryRequest naming convention used elsewhere in the file, e.g. message.action === 'ls' case), but I could not inspect file-operations.ts to fully verify the exact request shape/property name.

🤖 Prompt for AI agents

In src/lib/meshcentral/file-manager.ts around line 459, review and complete this code-review fix: handleDirectoryListing resolves an arbitrary pending request when reqid is missing, risking cross-request data corruption.
What the draft fix changed: In `sendOperation`, the pending request entry now records a `type` field (from `request.action`) so pending requests can be identified by kind; the `pendingRequests` map's value type gained an optional `type?: string` field. In `handleDirectoryListing`'s reqid-less fallback branch, the loop now only resolves a pending request whose `type === 'ls'`, and a `console.warn` was added noting the fallback path was triggered before scanning. This prevents resolving unrelated (e.g. upload-hash, search) promises with directory-listing data. Risk: relies on `FileOperationRequest`'s shape having an `action` field consistent with `'ls'` for list-directory requests (confirmed via `createListDirectoryRequest` naming convention used elsewhere in the file, e.g. `message.action === 'ls'` case), but I could not inspect `file-operations.ts` to fully verify the exact request shape/property name.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 appStateChange listener registration errors are only logged, native back/resume silently disabled

In TokenFreshnessWatcher's effect, both the .catch() on the addListener promise and the try/catch around the synchronous call now route through a shared reportRegistrationFailure helper that logs a distinctly-worded, high-severity console.error (prefixed "CRITICAL") explicitly stating that native resume-based refresh is disabled for the session, making it distinguishable from routine no-op logging and easier to alert on via log monitoring. This does not add a retry mechanism or user-facing UI indication — a complete fix per the finding's "or a retry" alternative would additionally require either a bounded re-registration attempt (e.g. on next visibilitychange/interval) or surfacing state to a monitoring/analytics call, neither of which exists in this file's dependencies, so I limited the change to the logging-severity improvement to avoid inventing unavailable telemetry/retry infrastructure.

🤖 Prompt for AI agents

In src/app/components/token-freshness-watcher.tsx around line 31, review and complete this code-review fix: appStateChange listener registration errors are only logged, native back/resume silently disabled.
What the draft fix changed: In `TokenFreshnessWatcher`'s effect, both the `.catch()` on the `addListener` promise and the `try/catch` around the synchronous call now route through a shared `reportRegistrationFailure` helper that logs a distinctly-worded, high-severity `console.error` (prefixed "CRITICAL") explicitly stating that native resume-based refresh is disabled for the session, making it distinguishable from routine no-op logging and easier to alert on via log monitoring. This does not add a retry mechanism or user-facing UI indication — a complete fix per the finding's "or a retry" alternative would additionally require either a bounded re-registration attempt (e.g. on next visibilitychange/interval) or surfacing state to a monitoring/analytics call, neither of which exists in this file's dependencies, so I limited the change to the logging-severity improvement to avoid inventing unavailable telemetry/retry infrastructure.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 writeCookie does not validate cookie name or value against CRLF/control-character injection

In writeCookie (src/lib/cookies.ts), added a validation check right after destructuring attributes: if (!/^[\w-]+$/.test(name)) throw new Error(...). This rejects any cookie name containing CRLF, semicolons, or other control/special characters before it is interpolated into the Set-Cookie-like string, hardening the shared primitive against injection from a future dynamic caller while not affecting existing static literal call sites (which all match \w- already).

🤖 Prompt for AI agents

In src/lib/cookies.ts around line 32, review and complete this code-review fix: writeCookie does not validate cookie name or value against CRLF/control-character injection.
What the draft fix changed: In `writeCookie` (src/lib/cookies.ts), added a validation check right after destructuring attributes: `if (!/^[\w-]+$/.test(name)) throw new Error(...)`. This rejects any cookie name containing CRLF, semicolons, or other control/special characters before it is interpolated into the `Set-Cookie`-like string, hardening the shared primitive against injection from a future dynamic caller while not affecting existing static literal call sites (which all match `\w-` already).
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 unreadNotificationCount and escalation/resolution fields selected unconditionally creates a deploy-ordering hazard documented but not enforced

In src/app/(app)/tickets/queries/ticket-queries.ts, changed GET_TICKETS_QUERY from a static string constant to a new getTicketsQuery() factory that gates unreadNotificationCount behind featureFlags.notifications.enabled(), mirroring the existing pattern already used for escalatedByUser/resolvedBy in boardCardTicketFragment. The same gate was applied to unreadNotificationCount inside boardCardTicketFragment itself, replacing the unconditional selection. GET_TICKETS_QUERY is kept as a backward-compatible exported constant (getTicketsQuery() called eagerly) so existing importers (e.g. use-ticket-options.ts) don't need to change call sites. This converts the previously comment-only deploy-ordering constraint into an actual runtime condition enforced by a feature flag, assuming a featureFlags.notifications flag exists and is only enabled once the backend has shipped the field — this relies on @/lib/feature-flags already exporting notifications (it is referenced elsewhere in this same file for hasUnreadNotifications semantics is not verified, so if that flag name/shape doesn't exist, this will fail to compile; that is the main risk requiring reviewer verification since I cannot see feature-flags.ts). A more complete fix would also add a runtime fallback (retry without the field on validation error) as the finding suggests, which was not implemented here because it would require broader query-execution changes outside this file's query-definition scope.

🤖 Prompt for AI agents

In src/app/(app)/tickets/queries/ticket-queries.ts around line 248, review and complete this code-review fix: unreadNotificationCount and escalation/resolution fields selected unconditionally creates a deploy-ordering hazard documented but not enforced.
What the draft fix changed: In `src/app/(app)/tickets/queries/ticket-queries.ts`, changed `GET_TICKETS_QUERY` from a static string constant to a new `getTicketsQuery()` factory that gates `unreadNotificationCount` behind `featureFlags.notifications.enabled()`, mirroring the existing pattern already used for `escalatedByUser`/`resolvedBy` in `boardCardTicketFragment`. The same gate was applied to `unreadNotificationCount` inside `boardCardTicketFragment` itself, replacing the unconditional selection. `GET_TICKETS_QUERY` is kept as a backward-compatible exported constant (`getTicketsQuery()` called eagerly) so existing importers (e.g. `use-ticket-options.ts`) don't need to change call sites. This converts the previously comment-only deploy-ordering constraint into an actual runtime condition enforced by a feature flag, assuming a `featureFlags.notifications` flag exists and is only enabled once the backend has shipped the field — this relies on `@/lib/feature-flags` already exporting `notifications` (it is referenced elsewhere in this same file for `hasUnreadNotifications` semantics is not verified, so if that flag name/shape doesn't exist, this will fail to compile; that is the main risk requiring reviewer verification since I cannot see `feature-flags.ts`). A more complete fix would also add a runtime fallback (retry without the field on validation error) as the finding suggests, which was not implemented here because it would require broader query-execution changes outside this file's query-definition scope.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔵 LogDrawer infoFields uses field.label as React key, risking duplicate-key collisions when labels repeat

In LogDrawer, the infoFields.map callback now destructures (field, index) and the div key is changed from typeof field.label === 'string' ? field.label : '' to `${index}-${typeof field.label === 'string' ? field.label : ''}`, guaranteeing key uniqueness even when labels repeat or are non-string/empty, matching the suggested fix exactly.

🤖 Prompt for AI agents

In src/app/components/shared/log-drawer.tsx around line 129, review and complete this code-review fix: LogDrawer infoFields uses field.label as React key, risking duplicate-key collisions when labels repeat.
What the draft fix changed: In `LogDrawer`, the `infoFields.map` callback now destructures `(field, index)` and the `div` key is changed from `typeof field.label === 'string' ? field.label : ''` to `` `${index}-${typeof field.label === 'string' ? field.label : ''}` ``, guaranteeing key uniqueness even when labels repeat or are non-string/empty, 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

[flamingo]

🦩 🔵 getDomainSuffix falls back to full hostname when hostname has fewer than 2 dot-separated parts, unlike sharedUrl branch which always slices last two segments

Extracted the hostname's last-two-labels slicing logic from the window.location.hostname branch into a shared suffixFromHostname helper in getDomainSuffix (src/lib/auth-api-client.ts), and now apply that same helper to the domain derived from sharedUrl in the runtimeEnv.sharedHostUrl() branch. Both code paths now consistently return only the last two dot-separated labels (e.g. api.tenant.example.com → example.com in both branches), removing the discrepancy the finding described. Risk: this intentionally changes behavior for multi-label shared hosts (previously the full host was used verbatim for subdomain construction); if any deployment relies on SHARED_HOST_URL values with more than two labels being preserved in full, this narrows them — the finding explicitly called this out as the desired consistent behavior, so it's the smallest change that resolves the inconsistency, but a full fix would benefit from a reviewer confirming no existing SAAS_DOMAIN_SUFFIX consumer expected the untrimmed shared-host domain.

🤖 Prompt for AI agents

In src/lib/auth-api-client.ts around line 21, review and complete this code-review fix: getDomainSuffix falls back to full hostname when hostname has fewer than 2 dot-separated parts, unlike sharedUrl branch which always slices last two segments.
What the draft fix changed: Extracted the hostname's last-two-labels slicing logic from the `window.location.hostname` branch into a shared `suffixFromHostname` helper in `getDomainSuffix` (src/lib/auth-api-client.ts), and now apply that same helper to the `domain` derived from `sharedUrl` in the `runtimeEnv.sharedHostUrl()` branch. Both code paths now consistently return only the last two dot-separated labels (e.g. `api.tenant.example.com` → `example.com` in both branches), removing the discrepancy the finding described. Risk: this intentionally changes behavior for multi-label shared hosts (previously the full host was used verbatim for subdomain construction); if any deployment relies on `SHARED_HOST_URL` values with more than two labels being preserved in full, this narrows them — the finding explicitly called this out as the desired consistent behavior, so it's the smallest change that resolves the inconsistency, but a full fix would benefit from a reviewer confirming no existing SAAS_DOMAIN_SUFFIX consumer expected the untrimmed shared-host domain.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

[michaelassraf]

Task linked: CU-86akhf8u5 OpenFrame OSS frontend review findings sweep (12 PRs)