feat(financial-entities): harden list-input validation for emails and phrases by gilgardosh · Pull Request #3852 · Urigo/accounter-fullstack

gilgardosh · 2026-07-06T09:55:09Z

What & why

Hardens validation on the business list-inputs so meaningless/error-prone entries can no longer be added:

Email lists — reject inserting the same address twice (case-insensitive). Covers general contacts, billing emails, auto-match recognition emails, and internal email links.
Auto-match phrases — reject a phrase that duplicates or is a substring of another (e.g. GOOGL vs GOOGLE). Such an overlap silently shadows the more specific phrase during matching.

Both cases were called out in the issue; the sweep for "more similar cases" also covered the insert-business modal, the modify-client dialog, and internal email links. Flight-path segments (round trips legitimately repeat) and read-only filter inputs were deliberately left out.

Approach

Two layers:

Client is the enforcement/UX layer. Add-handlers block a conflicting entry and surface an inline message ("already in the list" / "'GOOGL' overlaps 'GOOGLE'"), and the relevant zod schemas gained uniqueness refinements.
Server normalizes suggestion-data list fields on every write (dedupe emails/links case-insensitively; drop duplicate/substring-covered phrases, keeping the more specific one). Billing emails are deduped in the clients resolver.

Server-side normalize was chosen over hard-reject on purpose: rejecting would break editing a legacy business whose stored phrases already overlap (the client re-sends the full list on every save). Normalizing self-heals that data and keeps automated business-merges from failing.

Files

New shared helpers + unit tests: packages/server/src/modules/financial-entities/helpers/list-input-validation.helper.ts, packages/client/src/helpers/list-input-validation.ts
Server: business-suggestion-data-schema.helper.ts (new normalizeSuggestionListData), businesses.helper.ts, businesses.resolver.ts, clients.resolvers.ts
Client: contact-info-section.tsx, configurations-section.tsx, insert-business.tsx, modify-client-dialog.tsx

Testing

Unit tests were added for both helpers (duplicate detection, substring/overlap detection, phrase cleaning, normalization). Note: the full yarn install/yarn test/yarn lint pipeline could not be exercised in the authoring environment (the unrelated xlsx dependency fetches from a host blocked by the egress policy), so the added vitest suites should be run in CI/locally before merge. The helper algorithms were validated with standalone scripts.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QHPnaJtxYAwhWZAUrz1hrD

Generated by Claude Code

… phrases Guard against meaningless/error-prone entries in the business list-inputs: - Emails (general contacts, billing emails, recognition emails, internal email links): reject duplicate addresses (case-insensitive). - Auto-match phrases: reject a phrase that duplicates or is a substring of another (e.g. "GOOGL" vs "GOOGLE"), which would silently shadow the more specific phrase during matching. Client add-handlers now block conflicting entries with an inline message, and the relevant zod schemas gained uniqueness refinements. The server normalizes suggestion-data list fields on every write (dedupe + drop substring-covered phrases), which self-heals legacy data and keeps automated merges from storing redundant entries; client billing emails are deduped in the clients resolver. Shared helpers added on both sides (list-input-validation) with unit tests. Closes #3848 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QHPnaJtxYAwhWZAUrz1hrD

gemini-code-assist

Code Review

This pull request introduces client-side and server-side validation and normalization helpers to prevent duplicate entries and substring-overlapping phrases in list inputs (such as emails, transaction phrases, and internal links). The feedback highlights critical logic errors in the phrase overlap validation and cleaning functions, where the logic for keeping broader vs. more specific phrases is reversed. Additionally, the reviewer recommends updating the corresponding unit tests and adding defensive checks to handle potentially undefined form values safely.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-06T09:58:30Z

+export function cleanPhrases(list: readonly string[]): string[] {
+  const deduped = dedupeList(list);
+  const normalized = deduped.map(normalizeListEntry);
+  return deduped.filter((_, i) => {
+    const current = normalized[i];
+    if (current === '') {
+      return true;
+    }
+    return !normalized.some(
+      (other, j) => j !== i && other !== current && other.includes(current),
+    );
+  });
+}


Keeping the more specific phrase (e.g., 'AMAZON UK') and dropping the broader phrase (e.g., 'AMAZON') is a critical bug. If you drop 'AMAZON', then any transaction with just 'AMAZON' (like 'AMAZON.COM' or 'AMAZON SEATTLE') will fail to match because they do not contain 'AMAZON UK'. To preserve maximum matching coverage while removing redundancy, you should keep the broader phrase and drop the more specific one (i.e., check current.includes(other) instead of other.includes(current)).

Suggested change

export function cleanPhrases(list: readonly string[]): string[] {

const deduped = dedupeList(list);

const normalized = deduped.map(normalizeListEntry);

return deduped.filter((_, i) => {

const current = normalized[i];

if (current === '') {

return true;

}

return !normalized.some(

(other, j) => j !== i && other !== current && other.includes(current),

);

});

}

export function cleanPhrases(list: readonly string[]): string[] {

const deduped = dedupeList(list);

const normalized = deduped.map(normalizeListEntry);

return deduped.filter((_, i) => {

const current = normalized[i];

if (current === '') {

return true;

}

return !normalized.some(

(other, j) => j !== i && other !== current && current.includes(other),

);

});

}

Thanks — I checked the actual matcher (packages/server/src/modules/app-providers/helpers/business-matcher.helper.ts) before deciding, and the coverage argument doesn't hold for this codebase.

Phrase matching there is whole-word and bidirectional (isWordMatch on space-padded tokens, checked as phrase ⊆ txn and txn ⊆ phrase) — not a substring .includes() against the transaction text. Under whole-word matching a shorter substring like GOOGL is not a whole word inside GOOGLE, so GOOGL does not match GOOGLE / GOOGLE PAYMENT transactions at all — only the (rare) literal token googl. So keeping the broader substring and dropping GOOGLE would lose the real matches — the opposite of the intended effect, and this is precisely the issue's own example (GOOGL vs GOOGLE).

Because matching is bidirectional, neither of two overlapping phrases strictly subsumes the other, so there is no coverage-preserving choice; keeping the more specific phrase is the safer default and matches the issue's intent. Keeping cleanPhrases as-is (drop the broader, keep the more specific).

Generated by Claude Code

gemini-code-assist · 2026-07-06T09:58:30Z

+export function phraseConflictError(list: readonly string[], value: string): string | null {
+  const candidate = normalizeEntry(value);
+  if (candidate === '') {
+    return null;
+  }
+  for (const item of list) {
+    const existing = normalizeEntry(item);
+    if (existing === '') {
+      continue;
+    }
+    if (existing === candidate) {
+      return `"${value.trim()}" is already in the list`;
+    }
+    if (existing.includes(candidate)) {
+      return `"${value.trim()}" is already covered by "${item}"`;
+    }
+    if (candidate.includes(existing)) {
+      return `"${value.trim()}" overlaps with "${item}" — remove the broader phrase first`;
+    }
+  }
+  return null;
+}


The substring overlap logic and error messages are reversed. If the existing phrase is 'AMAZON UK' and the user tries to add 'AMAZON', 'existing.includes(candidate)' is true. The current code tells the user that 'AMAZON' is already covered by 'AMAZON UK', which is false (a transaction with 'AMAZON' won't match 'AMAZON UK'). Conversely, if the existing phrase is 'AMAZON' and they try to add 'AMAZON UK', 'candidate.includes(existing)' is true, and they are told to remove the broader phrase first, which is also incorrect since 'AMAZON UK' is already covered by 'AMAZON'. Swapping the conditions and messages fixes this.

export function phraseConflictError(list: readonly string[], value: string): string | null { const candidate = normalizeEntry(value); if (candidate === '') { return null; } for (const item of list) { const existing = normalizeEntry(item); if (existing === '') { continue; } if (existing === candidate) { return `"${value.trim()}" is already in the list`; } if (existing.includes(candidate)) { return `"${value.trim()}" is broader than "${item}" — remove the more specific phrase first`; } if (candidate.includes(existing)) { return `"${value.trim()}" is already covered by "${item}"`; } } return null; }

Same reasoning as the cleanPhrases thread — these messages are intentionally paired with the server's keep-the-more-specific behavior. Swapping the wording would make the client explanation contradict what the server actually keeps (it would tell the user the longer phrase is "already covered" and to remove it, while the server keeps the longer one). Given the whole-word matcher, the shorter substring is the weaker phrase, so the current pairing is the correct one. Leaving as-is.

Generated by Claude Code

gemini-code-assist · 2026-07-06T09:58:30Z

+  it('flags a new phrase already covered by an existing broader one', () => {
+    expect(phraseConflictError(['GOOGL'], 'GOOGLE')).toContain('overlaps');
+  });
+
+  it('flags a new phrase that would shadow an existing more specific one', () => {
+    expect(phraseConflictError(['GOOGLE'], 'GOOGL')).toContain('already covered');
+  });


Update the test expectations to match the corrected phrase conflict validation logic.

Suggested change

it('flags a new phrase already covered by an existing broader one', () => {

expect(phraseConflictError(['GOOGL'], 'GOOGLE')).toContain('overlaps');

});

it('flags a new phrase that would shadow an existing more specific one', () => {

expect(phraseConflictError(['GOOGLE'], 'GOOGL')).toContain('already covered');

});

it('flags a new phrase already covered by an existing broader one', () => {

expect(phraseConflictError(['GOOGL'], 'GOOGLE')).toContain('already covered');

});

it('flags a new phrase that would shadow an existing more specific one', () => {

expect(phraseConflictError(['GOOGLE'], 'GOOGL')).toContain('broader than');

});

Tied to the phraseConflictError / cleanPhrases threads above: the behavior is intentionally keep-the-more-specific-phrase (the shorter substring is the weaker phrase under the whole-word matcher), so these expectations are correct as written. Leaving as-is.

Generated by Claude Code

gemini-code-assist · 2026-07-06T09:58:30Z

+describe('cleanPhrases', () => {
+  it('drops the broader phrase when one contains another', () => {
+    expect(cleanPhrases(['GOOGL', 'GOOGLE', 'AMAZON'])).toEqual(['GOOGLE', 'AMAZON']);
+  });
+
+  it('removes case-insensitive duplicates', () => {
+    expect(cleanPhrases(['GOOGLE', 'google', 'AMAZON'])).toEqual(['GOOGLE', 'AMAZON']);
+  });
+});


Update the test expectations to match the corrected cleanPhrases logic where the broader phrase is kept and the more specific one is dropped.

Suggested change

describe('cleanPhrases', () => {

it('drops the broader phrase when one contains another', () => {

expect(cleanPhrases(['GOOGL', 'GOOGLE', 'AMAZON'])).toEqual(['GOOGLE', 'AMAZON']);

});

it('removes case-insensitive duplicates', () => {

expect(cleanPhrases(['GOOGLE', 'google', 'AMAZON'])).toEqual(['GOOGLE', 'AMAZON']);

});

});

describe('cleanPhrases', () => {

it('drops the more specific phrase when one contains another', () => {

expect(cleanPhrases(['GOOGL', 'GOOGLE', 'AMAZON'])).toEqual(['GOOGL', 'AMAZON']);

});

it('removes case-insensitive duplicates', () => {

expect(cleanPhrases(['GOOGLE', 'google', 'AMAZON'])).toEqual(['GOOGLE', 'AMAZON']);

});

});

Tied to the cleanPhrases thread above: keeping the more-specific phrase and dropping the broader substring is intentional (see the matcher explanation there), so cleanPhrases(['GOOGL', 'GOOGLE', 'AMAZON']) → ['GOOGLE', 'AMAZON'] is the expected result. Leaving as-is.

Generated by Claude Code

gemini-code-assist · 2026-07-06T09:58:30Z

+  it('drops a phrase that is a substring of another, keeping the more specific one', () => {
+    expect(normalizeSuggestionListData({ phrases: ['GOOGL', 'GOOGLE'] })).toEqual({
+      phrases: ['GOOGLE'],
+    });
+  });


Update the test expectation to match the corrected cleanPhrases logic where the broader phrase is kept.

Suggested change

it('drops a phrase that is a substring of another, keeping the more specific one', () => {

expect(normalizeSuggestionListData({ phrases: ['GOOGL', 'GOOGLE'] })).toEqual({

phrases: ['GOOGLE'],

});

});

it('drops a phrase that is a substring of another, keeping the broader one', () => {

expect(normalizeSuggestionListData({ phrases: ['GOOGL', 'GOOGLE'] })).toEqual({

phrases: ['GOOGL'],

});

});

Same as the cleanPhrases thread: the more-specific phrase is kept, so ['GOOGL', 'GOOGLE'] → ['GOOGLE'] is intended. Leaving as-is.

Generated by Claude Code

Default form.getValues(...) to [] before duplicate/phrase-conflict checks so the list helpers never receive undefined. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QHPnaJtxYAwhWZAUrz1hrD

gilgardosh temporarily deployed to accounter-fullstack July 6, 2026 09:55 — with GitHub Actions Inactive

gilgardosh self-assigned this Jul 6, 2026

gemini-code-assist Bot reviewed Jul 6, 2026

View reviewed changes

gilgardosh temporarily deployed to accounter-fullstack July 7, 2026 05:36 — with GitHub Actions Inactive

Conversation

gilgardosh commented Jul 6, 2026

What & why

Approach

Files

Testing

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gilgardosh Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gilgardosh Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gilgardosh Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gilgardosh Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gilgardosh Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants