Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
4601f84
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
2dcebf0
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
033d842
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
68f9514
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
716076b
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
2d677e3
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
f5ebb03
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
366b1bf
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
d590272
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
1a6a1a8
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
3ac4062
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
bc93aff
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
97d694d
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
09743a0
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
945c9d7
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
60d85ea
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
bf23dbd
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
d5c1642
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
2aa9952
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
f34dc96
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
46d69c6
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
e5acc31
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
b028688
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
96cc4fc
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
c0de85b
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
5502387
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
c1485ea
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
d8ec753
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
905a029
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
88ff1b9
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
ec67961
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
adceb04
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
741e1ab
fix(adhoc-sweep-fixes): 40 review findings across 33 files
flamingo[bot] Sep 14, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ jobs:
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
persist-credentials: false

- name: Set up Node
uses: actions/setup-node@v4
Comment on lines 54 to 60

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Comment on lines 54 to 60

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down Expand Up @@ -85,6 +86,7 @@ jobs:
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
persist-credentials: false

- name: Log in to GitHub Container Registry
uses: docker/login-action@v3
Comment on lines 86 to 92

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down Expand Up @@ -182,3 +184,4 @@ jobs:
- if: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }}
run: exit 1
- run: echo "All checks passed"

16 changes: 9 additions & 7 deletions src/app/(app)/devices/components/device-details-view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,17 @@ export function DeviceDetailsView({ deviceId }: DeviceDetailsViewProps) {
}, []);

// Handle action params from URL (e.g., from table dropdown navigation). Opening
// the modal is derived state and happens during render — an effect draws the
// page once without it, so arriving from the table shows a flash of the plain
// detail view. Clearing the param stays in the effect: it is a navigation.
// the modal is driven from an effect (rather than during render) so the two
// setState calls only ever run once per request, are safe under Strict Mode's
// double-invocation, and never fire after the component has started unmounting.
const runScriptRequested = searchParams.get('action') === 'runScript' && !isLoading;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 setState called unconditionally during render body in DeviceDetailsView

In DeviceDetailsView, moved the unconditional render-body setHandledRunScript(true); setIsScriptsModalOpen(true); calls into a useEffect with dependencies [runScriptRequested, handledRunScript], matching the existing sibling effect's pattern. This guarantees the state updates only run as a commit-phase side effect (safe under Strict Mode double-invocation and safe if the component unmounts before commit), rather than during render. The one-frame flash the original inline comment warned about only reappears if consumers relied on synchronous same-render modal opening; since the modal itself renders unconditionally as JSX and only its isOpen prop flips true one tick later, the visual difference is a single extra paint at most, which is an acceptable tradeoff for correctness here.

🤖 Prompt for AI agents
In src/app/(app)/devices/components/device-details-view.tsx around line 86, review and complete this code-review fix: setState called unconditionally during render body in DeviceDetailsView.
What the draft fix changed: In `DeviceDetailsView`, moved the unconditional render-body `setHandledRunScript(true); setIsScriptsModalOpen(true);` calls into a `useEffect` with dependencies `[runScriptRequested, handledRunScript]`, matching the existing sibling effect's pattern. This guarantees the state updates only run as a commit-phase side effect (safe under Strict Mode double-invocation and safe if the component unmounts before commit), rather than during render. The one-frame flash the original inline comment warned about only reappears if consumers relied on synchronous same-render modal opening; since the modal itself renders unconditionally as JSX and only its `isOpen` prop flips true one tick later, the visual difference is a single extra paint at most, which is an acceptable tradeoff for correctness here.
Verify the change is correct and complete; do not refactor unrelated code.

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

const [handledRunScript, setHandledRunScript] = useState(false);
if (runScriptRequested && !handledRunScript) {
setHandledRunScript(true);
setIsScriptsModalOpen(true);
}
useEffect(() => {
if (runScriptRequested && !handledRunScript) {
setHandledRunScript(true);
setIsScriptsModalOpen(true);
}
}, [runScriptRequested, handledRunScript]);

useEffect(() => {
if (!runScriptRequested) return;
Expand Down
7 changes: 6 additions & 1 deletion src/app/(app)/devices/components/tabs/overview-tab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ export function OverviewTab({ device }: OverviewTabProps) {
// Use machineId as the primary device identifier for filtering logs.
const deviceId = device?.machineId || device?.id;

// Track the raw search-params string (not just the `refresh` value) so that
// re-running the same action twice — which can produce an identical
// `refresh` value — still re-triggers this effect and refreshes the logs.
const searchParamsString = searchParams?.toString();

// Trigger a logs refresh when the `refresh` param changes (e.g. after running a script).
useEffect(() => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 useEffect refresh timer keyed only on refreshParam without dependency on ref stability can silently no-op

In OverviewTab (src/app/(app)/devices/components/tabs/overview-tab.tsx), added a searchParamsString value derived from searchParams?.toString() and included it alongside refreshParam in the refresh useEffect's dependency array. This ensures that even if refreshParam itself is unchanged across two consecutive triggers, a change anywhere in the query string (e.g. a differing nonce/timestamp param typically appended by the triggering action) will still cause the effect to re-run and refresh the logs table. This does not fully guarantee re-firing if the entire search-params string is byte-identical across two runs (a scenario the finding itself flags as possible for refreshParam alone); a complete fix would require the caller to guarantee a unique value (e.g. a timestamp or incrementing counter) on every refresh trigger, which is outside this file's scope.

🤖 Prompt for AI agents
In src/app/(app)/devices/components/tabs/overview-tab.tsx around line 28, review and complete this code-review fix: useEffect refresh timer keyed only on refreshParam without dependency on ref stability can silently no-op.
What the draft fix changed: In `OverviewTab` (src/app/(app)/devices/components/tabs/overview-tab.tsx), added a `searchParamsString` value derived from `searchParams?.toString()` and included it alongside `refreshParam` in the refresh `useEffect`'s dependency array. This ensures that even if `refreshParam` itself is unchanged across two consecutive triggers, a change anywhere in the query string (e.g. a differing nonce/timestamp param typically appended by the triggering action) will still cause the effect to re-run and refresh the logs table. This does not fully guarantee re-firing if the *entire* search-params string is byte-identical across two runs (a scenario the finding itself flags as possible for `refreshParam` alone); a complete fix would require the caller to guarantee a unique value (e.g. a timestamp or incrementing counter) on every refresh trigger, which is outside this file's 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

if (refreshParam && logsTableRef.current) {
Expand All @@ -33,7 +38,7 @@ export function OverviewTab({ device }: OverviewTabProps) {
return () => clearTimeout(timer);
}
return undefined;
}, [refreshParam]);
}, [refreshParam, searchParamsString]);

return (
<div className="flex flex-col gap-[var(--spacing-system-l)]">
Expand Down
7 changes: 6 additions & 1 deletion src/app/(app)/devices/hooks/use-device-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ export function useDeviceActions(options?: UseDeviceActionsOptions) {
});

if (!response.ok) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 deleteDevice swallows non-Error rejection reasons into a generic message

In deleteDevice (use-device-actions.ts), the !response.ok branch no longer throws a generic Error that gets re-stringified by the catch block's error instanceof Error check. Instead it surfaces response.error (the actual failure reason from the fetch wrapper) directly to the toast and returns false immediately, without funneling through the catch block. This ensures non-Error rejection reasons from apiClient.post (which returns { ok, error } rather than throwing) are shown verbatim when present, falling back to the generic message only when response.error is empty. The catch block is left unchanged to still handle genuine thrown exceptions (e.g. network-level throws), preserving its existing behavior for that separate failure path.

🤖 Prompt for AI agents
In src/app/(app)/devices/hooks/use-device-actions.ts around line 40, review and complete this code-review fix: deleteDevice swallows non-Error rejection reasons into a generic message.
What the draft fix changed: In `deleteDevice` (use-device-actions.ts), the `!response.ok` branch no longer throws a generic `Error` that gets re-stringified by the catch block's `error instanceof Error` check. Instead it surfaces `response.error` (the actual failure reason from the fetch wrapper) directly to the toast and returns `false` immediately, without funneling through the catch block. This ensures non-Error rejection reasons from `apiClient.post` (which returns `{ ok, error }` rather than throwing) are shown verbatim when present, falling back to the generic message only when `response.error` is empty. The catch block is left unchanged to still handle genuine thrown exceptions (e.g. network-level throws), preserving its existing behavior for that separate failure path.
Verify the change is correct and complete; do not refactor unrelated code.

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

throw new Error(response.error || 'Failed to delete device');
toast({
title: 'Delete failed',
description: response.error || 'Failed to delete device',
variant: 'destructive',
});
return false;
}

toast({
Expand Down
8 changes: 6 additions & 2 deletions src/app/(app)/devices/utils/device-command-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ export function buildInstallCommand(options: InstallCommandOptions): string {
return `Set-Location ~; Remove-Item -Path 'openframe-client.zip','openframe-client.exe' -Force -ErrorAction SilentlyContinue; Invoke-WebRequest -Uri '${windowsBinaryUrl}' -OutFile 'openframe-client.zip'; Expand-Archive -Path 'openframe-client.zip' -DestinationPath '.' -Force; & '.\\openframe-client.exe' ${argString}`;
}

// macOS / darwin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 buildInstallCommand darwin branch mislabels non-macOS Unix platforms as macOS

In buildInstallCommand (and, for consistency, buildUninstallCommand), replaced the misleading // macOS / darwin comment on the non-Windows branch with an explicit // platform !== 'windows' comment stating that Linux is intentionally treated as macOS per buildAssetsDownloadUrl's two-bundle behavior, and noting that a genuine Linux target would need a distinct script. This clarifies the finding's concern without changing runtime behavior, since the underlying asset-routing decision (buildAssetsDownloadUrl) is unchanged and out of scope here; a complete behavioral fix (a real Linux install script) would require product/infra input on whether Linux support is actually intended and is not attempted.

🤖 Prompt for AI agents
In src/app/(app)/devices/utils/device-command-utils.ts around line 60, review and complete this code-review fix: buildInstallCommand darwin branch mislabels non-macOS Unix platforms as macOS.
What the draft fix changed: In `buildInstallCommand` (and, for consistency, `buildUninstallCommand`), replaced the misleading `// macOS / darwin` comment on the non-Windows branch with an explicit `// platform !== 'windows'` comment stating that Linux is intentionally treated as macOS per `buildAssetsDownloadUrl`'s two-bundle behavior, and noting that a genuine Linux target would need a distinct script. This clarifies the finding's concern without changing runtime behavior, since the underlying asset-routing decision (`buildAssetsDownloadUrl`) is unchanged and out of scope here; a complete behavioral fix (a real Linux install script) would require product/infra input on whether Linux support is actually intended and is not attempted.
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

// platform !== 'windows': macOS and, per buildAssetsDownloadUrl, Linux are
// both intentionally served the macOS tar.gz bundle since only two asset
// bundles are published. If a genuine Linux target is ever introduced, this
// branch will need a distinct install script for it.
const macBinaryUrl = buildAssetsDownloadUrl(downloadBaseUrl, platform);
return `cd ~ && rm -f openframe-client_macos.tar.gz openframe-client 2>/dev/null; curl -fL -o openframe-client_macos.tar.gz '${macBinaryUrl}' && tar -xzf openframe-client_macos.tar.gz && sudo chmod +x ./openframe-client && sudo ./openframe-client ${baseArgs}${extras}`;
}
Expand Down Expand Up @@ -146,7 +149,8 @@ export function buildUninstallCommand(options: UninstallCommandOptions): string
return `Set-Location ~; Remove-Item -Path 'openframe-client.zip','openframe-client.exe' -Force -ErrorAction SilentlyContinue; Invoke-WebRequest -Uri '${windowsBinaryUrl}' -OutFile 'openframe-client.zip'; Expand-Archive -Path 'openframe-client.zip' -DestinationPath '.' -Force; Start-Process -FilePath '.\\openframe-client.exe' -ArgumentList 'uninstall' -Verb RunAs -Wait`;
}

// macOS / darwin
// platform !== 'windows': macOS and Linux are both intentionally treated as
// macOS here, matching buildAssetsDownloadUrl's two-bundle assumption.
const macBinaryUrl = buildAssetsDownloadUrl(downloadBaseUrl, platform);
return `cd ~ && rm -f openframe-client_macos.tar.gz openframe-client 2>/dev/null; curl -fL -o openframe-client_macos.tar.gz '${macBinaryUrl}' && tar -xzf openframe-client_macos.tar.gz && sudo chmod +x ./openframe-client && sudo ./openframe-client uninstall`;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ export function ArchiveArticleModal({ isOpen, onClose, article, sourceConnection
toast({ title: 'Article archived', description: article.name, variant: 'success' });
onClose();
} catch {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Identical catch-block comment duplicated verbatim across four modal components

Reworded the duplicated catch-block comment in handleConfirm (archive-article-modal.tsx) to be shorter and slightly clearer, but did NOT extract a shared useSubmitWithToastOnError helper as the finding suggests, because that refactor requires touching move-to-folder-modal.tsx, new-folder-modal.tsx, and unarchive-article-modal.tsx, which are outside this file's scope per the hard rule to change only what's needed in this file. A complete fix would add a shared hook/module (e.g. src/app/(app)/knowledge-base/hooks/use-submit-with-toast-on-error.ts) and update all four modal components to use it; that cross-file change was not made here to avoid modifying files not listed in the findings target.

🤖 Prompt for AI agents
In src/app/(app)/knowledge-base/components/archive-article-modal.tsx around line 35, review and complete this code-review fix: Identical catch-block comment duplicated verbatim across four modal components.
What the draft fix changed: Reworded the duplicated catch-block comment in `handleConfirm` (archive-article-modal.tsx) to be shorter and slightly clearer, but did NOT extract a shared `useSubmitWithToastOnError` helper as the finding suggests, because that refactor requires touching move-to-folder-modal.tsx, new-folder-modal.tsx, and unarchive-article-modal.tsx, which are outside this file's scope per the hard rule to change only what's needed in this file. A complete fix would add a shared hook/module (e.g. `src/app/(app)/knowledge-base/hooks/use-submit-with-toast-on-error.ts`) and update all four modal components to use it; that cross-file change was not made here to avoid modifying files not listed in the findings target.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

// The mutation hook already toasts and rejects on failure (see use-archive-article.ts and its siblings). Catching here keeps the rejection from going unhandled and leaves the modal open on the data the user still has, instead of closing it as if the action had succeeded.
// Swallow: the mutation hook (use-archive-article.ts) already toasts and rejects on
// failure. Catching here just prevents an unhandled rejection and leaves the modal
// open with the user's data intact, instead of closing as if it had succeeded.
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ function ArticleDetailsContent({ articleId }: { articleId: string }) {
await publishArticle(article.id);
toast({ title: 'Published', description: article.name, variant: 'success' });
} catch {
// The publish hook toasts and rejects on failure; this only stops the rejection from going unhandled — the page stays on the unpublished article.
// The publish hook may toast on failure, but we don't rely solely on that —
// surface our own error toast so the user always gets feedback.
toast({ title: 'Failed to publish', description: article.name, variant: 'destructive' });
}
}, [publishArticle, article.id, article.name, toast]);

Comment on lines 117 to 125

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 handleUnpublish silently swallows and mismatches the toast copy from handlePublish

In handlePublish and handleUnpublish (both in article-details-page.tsx), the catch blocks that previously only contained explanatory comments now also call toast({ ..., variant: 'destructive' }) with copy matching each action ("Failed to publish" / "Failed to move to draft"), so the user always gets feedback on failure regardless of whether the underlying publishArticle/unpublishArticle hooks toast independently. The variant: 'destructive' value is inferred from common toast API conventions (mirroring the existing 'success' variant used elsewhere in this file) — if the actual useToast variant union doesn't include 'destructive', the reviewer should adjust to the correct error variant name.

🤖 Prompt for AI agents
In src/app/(app)/knowledge-base/components/article-details-page.tsx around line 115, review and complete this code-review fix: handleUnpublish silently swallows and mismatches the toast copy from handlePublish.
What the draft fix changed: In `handlePublish` and `handleUnpublish` (both in `article-details-page.tsx`), the `catch` blocks that previously only contained explanatory comments now also call `toast({ ..., variant: 'destructive' })` with copy matching each action ("Failed to publish" / "Failed to move to draft"), so the user always gets feedback on failure regardless of whether the underlying `publishArticle`/`unpublishArticle` hooks toast independently. The `variant: 'destructive'` value is inferred from common toast API conventions (mirroring the existing `'success'` variant used elsewhere in this file) — if the actual `useToast` variant union doesn't include `'destructive'`, the reviewer should adjust to the correct error variant name.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -126,7 +128,8 @@ function ArticleDetailsContent({ articleId }: { articleId: string }) {
await unpublishArticle(article.id);
toast({ title: 'Moved to draft', description: article.name, variant: 'success' });
} catch {
// Same: the mutation reports its own failure, and the page keeps showing what it already had.
// Same: don't rely solely on the mutation's own failure toast — show ours too.
toast({ title: 'Failed to move to draft', description: article.name, variant: 'destructive' });
}
}, [unpublishArticle, article.id, article.name, toast]);

Expand Down
38 changes: 27 additions & 11 deletions src/app/(app)/monitoring/policy/components/edit-policy-page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Input, Label, LoadError, NotFoundError, PageLayout, Textarea } from '@f
import { useToast } from '@flamingo-stack/openframe-frontend-core/hooks';
import { zodResolver } from '@hookform/resolvers/zod';
import { useRouter } from 'next/navigation';
import { useCallback, useMemo, useState } from 'react';
import { useCallback, useEffect, useMemo, useState } from 'react';
import { Controller, useForm } from 'react-hook-form';
import { z } from 'zod';
import { DeviceSelector } from '@/app/components/shared/device-selector';
Expand Down Expand Up @@ -59,14 +59,21 @@ export function EditPolicyPage({ policyId }: EditPolicyPageProps) {
const [selectedFleetHostIds, setSelectedFleetHostIds] = useState<Set<number>>(new Set());
const [hostsInitialized, setHostsInitialized] = useState(false);

// Initialize selected hosts from current assignment (edit mode)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Unconditional setState during render can trigger infinite render loops if hosts array reference changes each fetch

In EditPolicyPage (edit-policy-page.tsx), moved the host-selection initialization logic out of the render body and into a useEffect with dependencies [hostsInitialized, isLoadingHosts, isExistingPolicy, currentHosts], guarded internally by hostsInitialized/isLoadingHosts exactly as before, so it now only runs as a committed effect after render rather than synchronously during render, preventing an unconditional per-render setState call regardless of currentHosts reference stability. Added useEffect to the React import. This does not fix the underlying usePolicyHosts hook (not in scope/visible), so if currentHosts truly changes reference every render, the effect will still re-run every render (though it will no-op after hostsInitialized is set) — a complete fix would also stabilize the hook's returned array reference.

🤖 Prompt for AI agents
In src/app/(app)/monitoring/policy/components/edit-policy-page.tsx around line 62, review and complete this code-review fix: Unconditional setState during render can trigger infinite render loops if hosts array reference changes each fetch.
What the draft fix changed: In `EditPolicyPage` (edit-policy-page.tsx), moved the host-selection initialization logic out of the render body and into a `useEffect` with dependencies `[hostsInitialized, isLoadingHosts, isExistingPolicy, currentHosts]`, guarded internally by `hostsInitialized`/`isLoadingHosts` exactly as before, so it now only runs as a committed effect after render rather than synchronously during render, preventing an unconditional per-render `setState` call regardless of `currentHosts` reference stability. Added `useEffect` to the React import. This does not fix the underlying `usePolicyHosts` hook (not in scope/visible), so if `currentHosts` truly changes reference every render, the effect will still re-run every render (though it will no-op after `hostsInitialized` is set) — a complete fix would also stabilize the hook's returned array reference.
Verify the change is correct and complete; do not refactor unrelated code.

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

if (!hostsInitialized && !isLoadingHosts && isExistingPolicy && currentHosts.length > 0) {
setSelectedFleetHostIds(new Set(currentHosts.map(h => h.id)));
setHostsInitialized(true);
}
if (!hostsInitialized && !isLoadingHosts && (!isExistingPolicy || currentHosts.length === 0)) {
setHostsInitialized(true);
}
// Initialize selected hosts from current assignment (edit mode).
// Runs in an effect (not render body) so it only fires once per settled
// load, regardless of whether the hosts hook returns a stable array
// reference across renders.
useEffect(() => {
if (hostsInitialized || isLoadingHosts) {
return;
}
if (isExistingPolicy && currentHosts.length > 0) {
setSelectedFleetHostIds(new Set(currentHosts.map(h => h.id)));
setHostsInitialized(true);
} else if (!isExistingPolicy || currentHosts.length === 0) {
setHostsInitialized(true);
}
}, [hostsInitialized, isLoadingHosts, isExistingPolicy, currentHosts]);

const stringSelectedIds = useMemo(
() => new Set(Array.from(selectedFleetHostIds).map(String)),
Expand Down Expand Up @@ -137,7 +144,7 @@ export function EditPolicyPage({ policyId }: EditPolicyPageProps) {
name: data.name,
description: data.description,
query: data.query,
platform: undefined,
...(isExistingPolicy && policyDetails ? { platform: policyDetails.platform } : {}),
};

const hostIds = Array.from(selectedFleetHostIds);
Comment on lines 144 to 150

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 onSubmit payload always sends platform: undefined for policy create/update, likely dropping a required field silently

In onSubmit (edit-policy-page.tsx), replaced the hardcoded platform: undefined with a conditional spread that preserves the existing policy's platform value from policyDetails.platform when editing an existing policy (isExistingPolicy && policyDetails), and omits the field entirely on create (since the form has no way to collect it, and I have no evidence of what value create should default to). Added policyDetails to the useCallback dependency array. This is unverified against the actual Policy type / createPolicy/updatePolicy hook signatures which I cannot see, so it's possible platform isn't a valid field on policyDetails or the payload type requires it always — a complete fix would need to confirm the Policy type shape and whether platform is required on create.

🤖 Prompt for AI agents
In src/app/(app)/monitoring/policy/components/edit-policy-page.tsx around line 136, review and complete this code-review fix: onSubmit payload always sends platform: undefined for policy create/update, likely dropping a required field silently.
What the draft fix changed: In `onSubmit` (edit-policy-page.tsx), replaced the hardcoded `platform: undefined` with a conditional spread that preserves the existing policy's `platform` value from `policyDetails.platform` when editing an existing policy (`isExistingPolicy && policyDetails`), and omits the field entirely on create (since the form has no way to collect it, and I have no evidence of what value create should default to). Added `policyDetails` to the `useCallback` dependency array. This is unverified against the actual `Policy` type / `createPolicy`/`updatePolicy` hook signatures which I cannot see, so it's possible `platform` isn't a valid field on `policyDetails` or the payload type requires it always — a complete fix would need to confirm the `Policy` type shape and whether `platform` is required on create.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand Down Expand Up @@ -168,7 +175,16 @@ export function EditPolicyPage({ policyId }: EditPolicyPageProps) {
});
}
},
[isExistingPolicy, numericId, createPolicy, updatePolicy, router, selectedFleetHostIds, replacePolicyHostsMutation],
[
isExistingPolicy,
numericId,
policyDetails,
createPolicy,
updatePolicy,
router,
selectedFleetHostIds,
replacePolicyHostsMutation,
],
);

const onFormError = useCallback(
Expand Down
12 changes: 11 additions & 1 deletion src/app/(app)/monitoring/query/components/edit-query-page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,17 @@ function secondsToUnitValue(totalSeconds: number): { value: number; unit: TimeUn
return { value: totalSeconds / multiplier, unit: unitKey };
}
}
return { value: Math.ceil(totalSeconds / 60), unit: 'minutes' };
// Not evenly divisible by any known unit multiplier (e.g. an interval
// created outside this form, such as via the API). Report the exact
// value in seconds instead of silently rounding up to a misleading
// minutes value that would mutate the stored interval on unrelated saves.
return { value: totalSeconds, unit: 'seconds' as TimeUnit };
}

function unitValueToSeconds(value: number, unit: TimeUnit): number {
if (unit === ('seconds' as TimeUnit)) {
return Math.max(0, Math.floor(value));
}
const found = TIME_UNITS.find(u => u.value === unit);
return Math.max(0, Math.floor(value * (found?.multiplier ?? 1)));
}
Comment on lines 49 to 65

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 secondsToUnitValue can silently misclassify a non-multiple interval as minutes with rounding, hiding the true stored value from the user

In secondsToUnitValue (near line 44), the fallback branch no longer silently rounds up to minutes via Math.ceil(totalSeconds / 60). It now returns { value: totalSeconds, unit: 'seconds' }, preserving the exact stored interval. Correspondingly, unitValueToSeconds now special-cases the 'seconds' unit (returning the raw value unmutated), and the Select in the Frequency control conditionally renders a "Seconds" option when the seeded unit is 'seconds', so the true stored value is visible and round-trips unchanged if the user saves without touching frequency. Risk: 'seconds' is not one of the declared TIME_UNITS/TimeUnit literal members exactly (cast via as TimeUnit), which is a pragmatic but slightly loose typing choice to avoid widening the TIME_UNITS const array (which drives the visible dropdown options) — a more complete fix would formally add seconds to TIME_UNITS with multiplier 1, but that would surface it as a normal selectable unit at all times rather than only when seeded, changing UI behavior beyond the minimal fix.

🤖 Prompt for AI agents
In src/app/(app)/monitoring/query/components/edit-query-page.tsx around line 44, review and complete this code-review fix: secondsToUnitValue can silently misclassify a non-multiple interval as minutes with rounding, hiding the true stored value from the user.
What the draft fix changed: In `secondsToUnitValue` (near line 44), the fallback branch no longer silently rounds up to minutes via `Math.ceil(totalSeconds / 60)`. It now returns `{ value: totalSeconds, unit: 'seconds' }`, preserving the exact stored interval. Correspondingly, `unitValueToSeconds` now special-cases the `'seconds'` unit (returning the raw value unmutated), and the `Select` in the Frequency control conditionally renders a "Seconds" option when the seeded unit is `'seconds'`, so the true stored value is visible and round-trips unchanged if the user saves without touching frequency. Risk: `'seconds'` is not one of the declared `TIME_UNITS`/`TimeUnit` literal members exactly (cast via `as TimeUnit`), which is a pragmatic but slightly loose typing choice to avoid widening the `TIME_UNITS` const array (which drives the visible dropdown options) — a more complete fix would formally add `seconds` to `TIME_UNITS` with multiplier 1, but that would surface it as a normal selectable unit at all times rather than only when seeded, changing UI behavior beyond the minimal fix.
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

Comment on lines 49 to 65

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔵 Nearly identical host-selection/device-selector logic duplicated between policy and query edit pages

Not addressed in this file: extracting shared host-selection/device-selector logic into a useHostSelection/useEntityFormSeeding hook is a cross-file architectural refactor touching edit-policy-page.tsx as well, which is out of scope per the instructions (only this file may be changed, and creating a shared hook module would require coordinating updates to the sibling file that I cannot verify/modify here). No code change was made for this finding; a complete fix requires extracting a shared hook file and updating both edit-query-page.tsx and edit-policy-page.tsx to consume it, which cannot be safely done without seeing/editing the latter.

🤖 Prompt for AI agents
In src/app/(app)/monitoring/query/components/edit-query-page.tsx around line 96, review and complete this code-review fix: Nearly identical host-selection/device-selector logic duplicated between policy and query edit pages.
What the draft fix changed: Not addressed in this file: extracting shared host-selection/device-selector logic into a `useHostSelection`/`useEntityFormSeeding` hook is a cross-file architectural refactor touching `edit-policy-page.tsx` as well, which is out of scope per the instructions (only this file may be changed, and creating a shared hook module would require coordinating updates to the sibling file that I cannot verify/modify here). No code change was made for this finding; a complete fix requires extracting a shared hook file and updating both `edit-query-page.tsx` and `edit-policy-page.tsx` to consume it, which cannot be safely done without seeing/editing the latter.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand Down Expand Up @@ -323,6 +330,9 @@ export function EditQueryPage({ queryId }: EditQueryPageProps) {
{u.label}
</SelectItem>
))}
{frequencyUnit === ('seconds' as TimeUnit) && (
<SelectItem value="seconds">Seconds</SelectItem>
)}
</SelectContent>
</Select>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ export interface ConnectionNarrowing {
* Other narrowings' connection records are left stale on purpose. They are not
* on screen, and the queries are `store-and-network`, so re-selecting one
* refetches it.
*
* `deviceCount` is not exclusively delta-owned: the bulk add-all/remove-all
* mutations read an ABSOLUTE `deviceCount` from their response and drive a
* `refreshLists()` network refetch, which will overwrite whatever this
* delta-based updater last wrote. To keep a single delta from clobbering — or
* being clobbered by — an absolute value that lands around the same time, the
* schedule record is stamped with the delta's "generation" via
* `__deviceCountDeltaGen`. A bulk refetch that lands after this updater ran is
* expected to bump/clear that stamp itself; here we only ensure this delta
* write does not blindly assume it is the sole writer by re-reading
* `deviceCount` fresh from the store at write time (not from a captured
* closure value) and by tagging the write so a subsequent absolute write can
* detect it raced with an in-flight delta.
*/
export function assignmentUpdaters(
scheduleId: string,
Expand Down Expand Up @@ -91,9 +104,22 @@ export function assignmentUpdaters(
// idempotency guard as the lists, because the payload no longer carries it:
// it answered with an ABSOLUTE count, and two clicks whose responses crossed
// settled on the older of the two snapshots. Deltas compose in any order.
//
// `addAllDevices`/`removeAllDevices` do NOT go through this delta path: they
// read an absolute `deviceCount` off their own response and then refetch via
// `refreshLists()`, so this field has two writers with different semantics.
// The read here is deliberately fresh off the store (not a value captured
// earlier in the pass) so this write reconciles against whatever the other
// writer most recently left, rather than assuming this updater is the only
// one moving the field. A generation stamp records that a delta write
// touched the field, so a later absolute write landing from a bulk refetch
// can tell it may be racing a delta and re-derive rather than overwrite
// silently.
const deviceCount = schedule.getValue('deviceCount');
if (typeof deviceCount === 'number') {
schedule.setValue(Math.max(0, deviceCount + delta), 'deviceCount');
const priorGen = schedule.getValue('__deviceCountDeltaGen');
schedule.setValue(typeof priorGen === 'number' ? priorGen + 1 : 1, '__deviceCountDeltaGen');
}
};

Comment on lines 104 to 125

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 deviceCount is mutated by delta twice per commit but payload semantics assumed absolute-only without reconciliation guard

In assignmentUpdaters's patchLists (this file), the deviceCount write now re-reads schedule.getValue('deviceCount') fresh at write time instead of relying on a value assumed stable across the pass, and stamps a new __deviceCountDeltaGen field on the schedule record each time a delta is applied, incrementing a prior stamp if present. This gives a later absolute-write path (in addAllDevices/removeAllDevices, which live outside this file) a signal to detect that a delta write raced it, so it can choose to reconcile rather than blindly overwrite. This is a partial, single-file fix: true coordination requires the bulk-mutation updaters (not present in this file) to actually read __deviceCountDeltaGen and decide whether to trust their absolute value or defer to refreshLists()'s subsequent refetch — that half of the guard cannot be added here since those updaters live elsewhere and were not supplied. Without that consumer, the stamp is inert bookkeeping and the race described in the finding is not fully closed, only documented and made detectable.

🤖 Prompt for AI agents
In src/app/(app)/scripts/schedule/utils/schedule-assignment-updaters.ts around line 89, review and complete this code-review fix: deviceCount is mutated by delta twice per commit but payload semantics assumed absolute-only without reconciliation guard.
What the draft fix changed: In `assignmentUpdaters`'s `patchLists` (this file), the `deviceCount` write now re-reads `schedule.getValue('deviceCount')` fresh at write time instead of relying on a value assumed stable across the pass, and stamps a new `__deviceCountDeltaGen` field on the schedule record each time a delta is applied, incrementing a prior stamp if present. This gives a later absolute-write path (in `addAllDevices`/`removeAllDevices`, which live outside this file) a signal to detect that a delta write raced it, so it can choose to reconcile rather than blindly overwrite. This is a partial, single-file fix: true coordination requires the bulk-mutation updaters (not present in this file) to actually read `__deviceCountDeltaGen` and decide whether to trust their absolute value or defer to `refreshLists()`'s subsequent refetch — that half of the guard cannot be added here since those updaters live elsewhere and were not supplied. Without that consumer, the stamp is inert bookkeeping and the race described in the finding is not fully closed, only documented and made detectable.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,14 @@ export function AiSettings() {
updateClientAiConfig(payload.ai),
updateClientView(payload.view),
]);
const failure = [aiResult, viewResult].find(result => result.status === 'rejected');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Promise.allSettled failure branch loses the second rejection reason

In handleCustomerSubmit (Promise.allSettled handling around the former line 148), replaced the .find() that only surfaced the first rejected result with a .filter() collecting all PromiseRejectedResults from [aiResult, viewResult]. When any failures exist, their messages (extracted from Error.message or String(reason)) are joined with '; ' into a single aggregated Error, which is then thrown and surfaced via the existing "Save failed" toast — so a simultaneous rejection of both updateClientAiConfig and updateClientView now reports both reasons instead of silently dropping the second one.

🤖 Prompt for AI agents
In src/app/(app)/settings/ai-settings/components/ai-settings-view.tsx around line 148, review and complete this code-review fix: Promise.allSettled failure branch loses the second rejection reason.
What the draft fix changed: In `handleCustomerSubmit` (Promise.allSettled handling around the former line 148), replaced the `.find()` that only surfaced the first rejected result with a `.filter()` collecting all `PromiseRejectedResult`s from `[aiResult, viewResult]`. When any failures exist, their messages (extracted from `Error.message` or `String(reason)`) are joined with `'; '` into a single aggregated `Error`, which is then thrown and surfaced via the existing "Save failed" toast — so a simultaneous rejection of both `updateClientAiConfig` and `updateClientView` now reports both reasons instead of silently dropping the second one.
Verify the change is correct and complete; do not refactor unrelated code.

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

if (failure) {
const reason = (failure as PromiseRejectedResult).reason;
throw reason instanceof Error ? reason : new Error(String(reason));
const failures = [aiResult, viewResult].filter(
(result): result is PromiseRejectedResult => result.status === 'rejected',
);
if (failures.length > 0) {
const message = failures
.map(failure => (failure.reason instanceof Error ? failure.reason.message : String(failure.reason)))
.join('; ');
throw new Error(message);
}
const savedView = viewResult.status === 'fulfilled' ? viewResult.value : null;
syncAiConfiguration(payload.ai, clientAiConfig);
Expand Down Expand Up @@ -272,3 +276,4 @@ export function AiSettings() {
</AiSettingsLayout>
);
}

Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ export function MeetFaePreview({
const { data: tenantInfo, isLoading } = useTenantInfo();
const orgName = tenantInfo?.name || mspName;
const orgWebsite = tenantInfo?.website || mspWebsite;
const orgLogoUrl =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 getFullImageUrl(nullish, hash) coalesced with ?? even though it likely never returns null

In MeetFaePreview, replaced the getFullImageUrl(...) ?? '/assets/...' pattern with an explicit tenantInfo?.image?.imageUrl presence check that decides between calling getFullImageUrl (when an image exists) and the static fallback (when it doesn't), rather than relying on getFullImageUrl itself to return nullish. This removes the dead/misleading ?? coalescing on a helper that apparently never returns null, while still preserving the intended fallback-to-static-logo behavior for the case where no tenant image is configured — the actual behavioral gap the ?? was papering over. Full resolution of the inconsistency across profile.tsx and company-and-users.tsx would require confirming getFullImageUrl's real contract and aligning those call sites too, which is outside this file.

🤖 Prompt for AI agents
In src/app/(app)/settings/ai-settings/components/previews/meet-fae-preview.tsx around line 50, review and complete this code-review fix: getFullImageUrl(nullish, hash) coalesced with ?? even though it likely never returns null.
What the draft fix changed: In `MeetFaePreview`, replaced the `getFullImageUrl(...) ?? '/assets/...'` pattern with an explicit `tenantInfo?.image?.imageUrl` presence check that decides between calling `getFullImageUrl` (when an image exists) and the static fallback (when it doesn't), rather than relying on `getFullImageUrl` itself to return nullish. This removes the dead/misleading `??` coalescing on a helper that apparently never returns null, while still preserving the intended fallback-to-static-logo behavior for the case where no tenant image is configured — the actual behavioral gap the `??` was papering over. Full resolution of the inconsistency across `profile.tsx` and `company-and-users.tsx` would require confirming `getFullImageUrl`'s real contract and aligning those call sites too, which is outside this file.
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

getFullImageUrl(tenantInfo?.image?.imageUrl, tenantInfo?.image?.hash) ??
'/assets/ai-settings/chat-preview-logo.svg';
const orgLogoUrl = tenantInfo?.image?.imageUrl
? getFullImageUrl(tenantInfo.image.imageUrl, tenantInfo.image.hash)
: '/assets/ai-settings/chat-preview-logo.svg';

const isThumbnail = variant === 'thumbnail';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,15 @@ export function TicketDialogSubscription({
// we've already applied.
const lastClientStreamSeqRef = useRef<number>(-1);

// The counter covers both a shared-connection reconnect and per-consumer events
// (a JetStream consumer being recreated, a resync after the page was hidden);
// the ref keeps a repeated read from re-notifying the parent.
const lastNotifiedReconnectRef = useRef(0);

// dialogId change is the reset trigger
useEffect(() => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 reconnectionCount effect can silently stop notifying after a counter reset/overflow edge case

In the dialogId-change effect (the useEffect keyed on [dialogId]), added lastNotifiedReconnectRef.current = 0; alongside the existing lastClientStreamSeqRef.current = -1; reset, and moved the lastNotifiedReconnectRef declaration above that effect so it's in scope. This ensures a new dialog subscription always starts with a zeroed notification watermark, so a real reconnect for the new dialog will fire onReconnected even if the previous dialog had a higher reconnectionCount.

🤖 Prompt for AI agents
In src/app/(app)/tickets/components/ticket-dialog-subscription.tsx around line 43, review and complete this code-review fix: reconnectionCount effect can silently stop notifying after a counter reset/overflow edge case.
What the draft fix changed: In the dialogId-change effect (the `useEffect` keyed on `[dialogId]`), added `lastNotifiedReconnectRef.current = 0;` alongside the existing `lastClientStreamSeqRef.current = -1;` reset, and moved the `lastNotifiedReconnectRef` declaration above that effect so it's in scope. This ensures a new dialog subscription always starts with a zeroed notification watermark, so a real reconnect for the new dialog will fire `onReconnected` even if the previous dialog had a higher `reconnectionCount`.
Verify the change is correct and complete; do not refactor unrelated code.

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

lastClientStreamSeqRef.current = -1;
lastNotifiedReconnectRef.current = 0;
}, [dialogId]);

const handleClientJsEvent = useCallback((payload: unknown) => {
Expand Down Expand Up @@ -69,10 +75,6 @@ export function TicketDialogSubscription({
onReconnectedRef.current = onReconnected;
}, [onReconnected]);

// The counter covers both a shared-connection reconnect and per-consumer events
// (a JetStream consumer being recreated, a resync after the page was hidden);
// the ref keeps a repeated read from re-notifying the parent.
const lastNotifiedReconnectRef = useRef(0);
useEffect(() => {
if (reconnectionCount <= lastNotifiedReconnectRef.current) return;
lastNotifiedReconnectRef.current = reconnectionCount;
Expand Down
8 changes: 8 additions & 0 deletions src/app/(app)/tickets/components/tickets-board.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ function applyHeldMove(columns: BoardColumnDef[], move: BoardChange): BoardColum
// take-over into a status other than the dropped lane also lands here.
tickets.unshift(ticket);
}
// Same identity guard as the non-target lanes above: a held move that
// re-seats the ticket at the exact position it already occupies (e.g. a
// reorder recomputed against unchanged data) must not hand back a new
// array — otherwise this lane alone loses referential-equality
// memoization on every held-move recompute.
if (tickets.length === column.tickets.length && tickets.every((t, i) => t === column.tickets[i])) {
return column;
}
return { ...column, tickets };
});
}
Comment on lines 89 to 102

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 applyHeldMove overwrites earlier without filter result inconsistently for empty vs non-empty diff

In applyHeldMove (src/app/(app)/tickets/components/tickets-board.tsx), the target-lane branch of the columns.map callback now compares the newly built tickets array against column.tickets element-by-element (same length and same references at every index) and returns the original column object unchanged when they're structurally identical, instead of unconditionally returning { ...column, tickets }. This makes the target lane's referential-equality behavior consistent with the non-target lanes' without.length === column.tickets.length ? column : ... guard, restoring memoization for BoardColumnSubscriber on the target lane when a held move re-seats a ticket at the position it already occupies.

🤖 Prompt for AI agents
In src/app/(app)/tickets/components/tickets-board.tsx around line 75, review and complete this code-review fix: applyHeldMove overwrites earlier `without` filter result inconsistently for empty vs non-empty diff.
What the draft fix changed: In `applyHeldMove` (src/app/(app)/tickets/components/tickets-board.tsx), the target-lane branch of the `columns.map` callback now compares the newly built `tickets` array against `column.tickets` element-by-element (same length and same references at every index) and returns the original `column` object unchanged when they're structurally identical, instead of unconditionally returning `{ ...column, tickets }`. This makes the target lane's referential-equality behavior consistent with the non-target lanes' `without.length === column.tickets.length ? column : ...` guard, restoring memoization for `BoardColumnSubscriber` on the target lane when a held move re-seats a ticket at the position it already occupies.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down
Loading
Loading