upcoming-release-notes/test-budget-sync-disabled.md (1)

6-6: ⚡ Quick win

Consider removing internal implementation terms from the release note.

The message can stay user-facing by dropping internals like cloudFileId and mutation mechanics.

Proposed wording

-Test budgets (created via `Create test file`) no longer attempt to sync when a sync server is configured, matching the existing behavior of demo budgets. Previously any mutation in a test budget would trigger an "invalid fileId" sync error toast because test budgets are local-only and never get a `cloudFileId`.
+Test budgets created via `Create test file` no longer try to sync when a sync server is configured, matching demo budget behavior and preventing the previous “invalid fileId” sync error toast.

Based on learnings: For files in upcoming-release-notes/*.md, keep release notes concise: ideally one sentence and non-technical content unless the category is Maintenance.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@upcoming-release-notes/test-budget-sync-disabled.md` at line 6, The release
note in upcoming-release-notes/test-budget-sync-disabled.md exposes internal
implementation terms ("cloudFileId" and mutation/sync mechanics); update the
single-sentence note to be concise and user-facing by removing internal terms
and technical details (e.g., say: "Test budgets created via 'Create test file'
no longer attempt to sync when a sync server is configured, matching demo
budgets."), keeping it non-technical and fitting the Maintenance guideline for
that folder.

packages/loot-core/src/types/models/gocardless.ts (1)

79-80: ⚡ Quick win

Consider using Pick<GoCardlessInstitution, 'name' | 'logo'> instead of an inline structural type.

The inline { name: string; logo?: string } duplicates structure already described by GoCardlessInstitution. Using Pick ties the hydrated subset to the canonical type and prevents silent drift if GoCardlessInstitution evolves (e.g., logo becomes a richer object).

Note also the inconsistency: GoCardlessInstitution.logo is required (string), but the hydrated version makes it optional. If hydration can genuinely omit the logo, GoCardlessInstitution.logo should probably also be made optional for consistency; if it cannot, then logo here should be required too.

♻️ Proposed refactor

-  /** Institution object from the sync-server after hydration. */
-  institution: { name: string; logo?: string };
+  /** Institution object from the sync-server after hydration. */
+  institution: Pick<GoCardlessInstitution, 'name' | 'logo'>;

If logo can be absent after hydration, also update GoCardlessInstitution:

-  logo: string;
+  logo?: string;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/loot-core/src/types/models/gocardless.ts` around lines 79 - 80,
Replace the inline hydrated institution type with a Pick of the canonical type
to avoid duplication: change the property type for institution to
Pick<GoCardlessInstitution, 'name' | 'logo'> and then reconcile the optionality
of logo between GoCardlessInstitution and the hydrated version—if hydration can
omit logo, make GoCardlessInstitution.logo optional; otherwise make the Pick
contain a required logo—so update either GoCardlessInstitution or the
institution field accordingly to keep them consistent.

packages/desktop-client/src/components/banksync/useBuiltInBankSyncProviders.ts (1)

49-54: 💤 Low value

connector type duplicates the exported PluggyAiConnector.

The inline anonymous type is structurally identical to PluggyAiConnector from @actual-app/core/types/models/pluggyai. Reusing it avoids silent drift if fields are added to the canonical type later.

♻️ Proposed refactor

+import type { PluggyAiConnector } from '@actual-app/core/types/models/pluggyai';

 type PluggyAiAccount = {
   // ...
-  /** Optional: sync-server merges Item.connector onto each account when the Item fetch succeeds. */
-  connector?: {
-    name?: string;
-    imageUrl?: string;
-    institutionUrl?: string;
-  };
+  /** Optional: sync-server merges Item.connector onto each account when the Item fetch succeeds. */
+  connector?: PluggyAiConnector;
 };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/desktop-client/src/components/banksync/useBuiltInBankSyncProviders.ts`
around lines 49 - 54, The inline anonymous type for "connector" duplicates the
exported PluggyAiConnector; replace the inline type with the canonical
PluggyAiConnector type from `@actual-app/core/types/models/pluggyai` by importing
PluggyAiConnector and changing the connector?: { ... } declaration to
connector?: PluggyAiConnector (ensure the import is added/used in
useBuiltInBankSyncProviders.ts and update any optional/nullable handling to
match the imported type).

packages/desktop-client/src/components/accounts/icon-picker/IconPickerModal.tsx (2)

539-560: ⚡ Quick win

useCallback in new file violates the React Compiler auto-memoization guideline.

EmojiTab is new code; handleEmoji should be a plain function and let the compiler handle memoization.

♻️ Proposed fix

-  const handleEmoji = useCallback(
-    (value: string) => {
-      setEmoji(value);
-      try {
-        if (!value.trim()) {
-          setPreview(null);
-          return;
-        }
-        const dataUrl = emojiToDataUrl(value);
-        setPreview(dataUrl);
-        setError(null);
-      } catch (err) {
-        const message =
-          err instanceof IconNormalizationError
-            ? err.message
-            : t('Failed to render emoji');
-        setError(message);
-        setPreview(null);
-      }
-    },
-    [setError, setPreview, t],
-  );
+  const handleEmoji = (value: string) => {
+    setEmoji(value);
+    try {
+      if (!value.trim()) {
+        setPreview(null);
+        return;
+      }
+      const dataUrl = emojiToDataUrl(value);
+      setPreview(dataUrl);
+      setError(null);
+    } catch (err) {
+      const message =
+        err instanceof IconNormalizationError
+          ? err.message
+          : t('Failed to render emoji');
+      setError(message);
+      setPreview(null);
+    }
+  };

As per coding guidelines: "Use React Compiler auto-memoization in desktop-client; omit manual useCallback, useMemo, and React.memo when adding or refactoring code."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/desktop-client/src/components/accounts/icon-picker/IconPickerModal.tsx`
around lines 539 - 560, Remove the manual useCallback wrapper around handleEmoji
in IconPickerModal (the handler defined in EmojiTab); make handleEmoji a plain
function that calls setEmoji, uses emojiToDataUrl, and catches
IconNormalizationError to call setError and setPreview as currently implemented,
and keep the same dependencies (setEmoji, setPreview, setError, t) referenced
directly so the React Compiler can auto-memoize it.

---

614-636: ⚡ Quick win

FullEmojiPicker: dynamic imports triggered on every render, not in useEffect.

Each render while Picker or data is null registers a new .then() handler. When the module resolves, all registered handlers fire, calling setPicker/setData redundantly. In React StrictMode (development double-invoke) this triggers the import twice on mount.

♻️ Proposed fix

 function FullEmojiPicker({ onPick }: { onPick: (emoji: string) => void }) {
   const [Picker, setPicker] = useState<ComponentType<{
     data: unknown;
     onEmojiSelect: (e: { native?: string }) => void;
     autoFocus?: boolean;
     perLine?: number;
   }> | null>(null);
   const [data, setData] = useState<unknown>(null);

-  if (!Picker || !data) {
-    void Promise.all([
-      import('@emoji-mart/react'),
-      import('@emoji-mart/data'),
-    ]).then(([picker, d]) => {
-      setPicker(() => picker.default);
-      setData(d.default);
-    });
-    return (
-      <Text style={{ color: theme.pageTextSubdued, fontSize: 12 }}>
-        <Trans>Loading emoji picker…</Trans>
-      </Text>
-    );
-  }
+  useEffect(() => {
+    void Promise.all([
+      import('@emoji-mart/react'),
+      import('@emoji-mart/data'),
+    ]).then(([picker, d]) => {
+      setPicker(() => picker.default);
+      setData(d.default);
+    });
+  }, []);

+  if (!Picker || !data) {
+    return (
+      <Text style={{ color: theme.pageTextSubdued, fontSize: 12 }}>
+        <Trans>Loading emoji picker…</Trans>
+      </Text>
+    );
+  }

   return (
     <Picker ...

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/desktop-client/src/components/accounts/icon-picker/IconPickerModal.tsx`
around lines 614 - 636, FullEmojiPicker currently triggers dynamic imports on
every render because the Promise.all call is placed outside a lifecycle hook,
causing multiple .then handlers and redundant setPicker/setData calls (and
double import in StrictMode); move the dynamic import logic into a useEffect
inside FullEmojiPicker that runs only when Picker or data are null (e.g.,
useEffect with dependency [Picker, data] or a single-run effect that sets state
via setPicker and setData), and ensure you guard against setting state after
unmount by tracking a mounted flag or cancelling if needed.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/sync-server/src/app-favicon.ts (1)

54-66: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

⚠️ SSRF guard is still bypassable via domain names and HTTP redirects (unresolved from prior review)

Two attack vectors remain open:

1. Domain-name bypass (line 65): ipaddr.isValid returns true only when the string is a valid IPv4 or IPv6 address, so any hostname string (e.g. attacker.com resolving to 127.0.0.1) falls through to return true and is fetched without restriction.

2. Redirect bypass (lines 74–75): fetchWithTimeout uses bare fetch() with no redirect: 'manual' option, so a public domain can redirect to a private IP and the response body is consumed without revalidating the final res.url.

Possible mitigations (pick at least one):

• Resolve the hostname to its IP(s) via dns.promises.lookup and run each resolved IP through isHostnameAllowed before calling fetch.
• Set redirect: 'manual' and re-validate the Location header (and resolved IP of the redirect target) before following.
• After fetch resolves, parse res.url and re-run isHostnameAllowed on the final URL's hostname (catches redirects to literal-IP addresses, but not DNS-based rebinding).

Also applies to: 68-79, 88-96

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/sync-server/src/app-favicon.ts` around lines 54 - 66, The SSRF check
in isHostnameAllowed is insufficient because it only blocks literal IPs and
allows domain names and redirects; update the fetch flow (function
fetchWithTimeout and any call sites that call isHostnameAllowed) to (a) resolve
the request hostname via dns.promises.lookup (or lookupAll) and run each
returned IP through isHostnameAllowed before making the request, and (b) set
fetch option redirect: 'manual' so you can validate any Location header (resolve
that location's hostname via DNS and re-run isHostnameAllowed) before following
redirects; additionally, after any final fetch that returns a response, parse
res.url and re-run isHostnameAllowed on that final hostname as a last check to
catch literal-IP redirects.

packages/desktop-client/src/components/accounts/icon-picker/IconPickerModal.tsx (1)

614-636: ⚡ Quick win

FullEmojiPicker: dynamic imports triggered in render body — move to useEffect

Calling Promise.all([import(...), import(...)]) directly inside the render function (outside any useEffect) is a side effect in render. While module-import caching prevents duplicate network requests, there are two practical concerns:

1. React Strict Mode double-invokes renders, scheduling multiple .then callbacks.
2. If either import fails, Picker and data stay null forever — the component is permanently stuck in "Loading…" with no error surfaced to the user.

♻️ Proposed refactor

Add useEffect to the React import (alongside the earlier useRef/useState change):

-import { useRef, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';

Then in FullEmojiPicker:

 function FullEmojiPicker({ onPick }: { onPick: (emoji: string) => void }) {
   const [Picker, setPicker] = useState<...>(null);
   const [data, setData] = useState<unknown>(null);
+  const [loadError, setLoadError] = useState(false);

-  if (!Picker || !data) {
-    void Promise.all([
-      import('@emoji-mart/react'),
-      import('@emoji-mart/data'),
-    ]).then(([picker, d]) => {
-      setPicker(() => picker.default);
-      setData(d.default);
-    });
-    return (
-      <Text style={{ color: theme.pageTextSubdued, fontSize: 12 }}>
-        <Trans>Loading emoji picker…</Trans>
-      </Text>
-    );
-  }
+  useEffect(() => {
+    void Promise.all([
+      import('@emoji-mart/react'),
+      import('@emoji-mart/data'),
+    ])
+      .then(([picker, d]) => {
+        setPicker(() => picker.default);
+        setData(d.default);
+      })
+      .catch(() => setLoadError(true));
+  }, []);
+
+  if (loadError) {
+    return (
+      <Text style={{ color: theme.errorText, fontSize: 12 }}>
+        <Trans>Failed to load emoji picker</Trans>
+      </Text>
+    );
+  }
+  if (!Picker || !data) {
+    return (
+      <Text style={{ color: theme.pageTextSubdued, fontSize: 12 }}>
+        <Trans>Loading emoji picker…</Trans>
+      </Text>
+    );
+  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/desktop-client/src/components/accounts/icon-picker/IconPickerModal.tsx`
around lines 614 - 636, FullEmojiPicker currently triggers dynamic imports in
the render body (setting Picker and data), which should be moved into a
useEffect to avoid render-side effects and duplicate/uncaught promises; update
FullEmojiPicker to run Promise.all([import('@emoji-mart/react'),
import('@emoji-mart/data')]) inside a useEffect with an empty dependency array,
call setPicker(() => picker.default) and setData(d.default) from the effect, and
add error handling (e.g., set an error state or fallback flag) so the component
can render an error message instead of staying stuck on "Loading…"; keep the
existing Picker and data state variables and ensure the effect is
canceled/guarded against updates after unmount if necessary.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes