fix(automations): don't flag valid-but-wrong-trigger {{ }} tokens as typos (#3653)

[Yeraze]

Follow-up fix to the token highlighter (#3727).

Bug

Highlighting scoped token validity to the current trigger only. A real token referenced under a different trigger — e.g. {{ trigger.from }} on a System/Schedule automation (from only exists for trigger.message) — was flagged red, identical to a genuine typo. Reported as "it flags all expansions as invalid" (tested with Hi {{trigger.from}} welcome to {{ trigger.asfd }} on a non-message trigger).

Fix — three tiers

• blue (ok) — valid for this trigger (or a known var.* / NOW).
• amber (foreign) — a real token that belongs to a different trigger; shown with a soft note "… not available for this trigger — will render blank." Not treated as a typo.
• red (bad) — unrecognized for every trigger → the "check for typos" warning.

classifyToken() drives the tiers; unknownTokens() lists only true typos, foreignTokens() the cross-trigger ones. So {{ trigger.from }} is never called a typo; only {{ trigger.asfd }} is.

Tests

tokenHints.test.ts covers classifyToken (ok/foreign/bad), and that unknownTokens flags only genuine typos while foreignTokens catches cross-trigger ones. Full suite 7524 passed, 0 failures; tsc + build clean.

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(automations): don't flag valid-but-wrong-trigger {{ }} tokens as typos

• Gather context and read changed files
• Review tokenHints.ts changes
• Review TokenTextField.tsx changes
• Review CSS changes
• Review test coverage
• Post final review

---

Summary

This PR correctly addresses the false-positive typo detection for cross-trigger tokens. The three-tier classification (ok/foreign/bad) is a clean model. Overall this is solid work — a few observations below.

---

tokenHints.ts

Module-level mutable cache (anyCache) — src/components/automations/tokenHints.ts:28-36

let anyCache: Set<string> | null = null;
export function anyTriggerTokenSet(): Set<string> {
  if (anyCache) return anyCache;
  ...
  anyCache = set;
  return set;
}

TRIGGER_TOKENS is a compile-time constant so this is safe in practice, but the mutable module-level variable is a code-smell: it can cause subtle issues in test environments where module state leaks between test runs (if the module is cached). Since TRIGGER_TOKENS is static, this can be simplified to a module-level const initialized once:

const ANY_TRIGGER_TOKEN_SET: Set<string> = (() => {
  const set = new Set<string>();
  for (const toks of Object.values(TRIGGER_TOKENS)) for (const [k] of toks) set.add(`trigger.${k}`);
  for (const [k] of UNIVERSAL_TOKENS) set.add(`trigger.${k}`);
  return set;
})();

This also removes the need to export anyTriggerTokenSet as a function — a minor API simplification.

classifyToken empty path shortcut — tokenHints.ts:40

if (path.length === 0 || valid.has(path)) return 'ok';

Returning 'ok' for an empty path is correct (empty {{ }} renders blank rather than being a typo), but the caller in tokenize already sets token: path.length > 0 — so an empty-path segment will never be rendered as a token mark. The early return here is harmless but slightly misleading; a comment or the early-return condition being path.length === 0 → 'ok' first is fine as-is.

---

TokenTextField.tsx

Four separate useMemo traversals — TokenTextField.tsx:21-24

const valid = useMemo(() => validTokenSet(triggerType, variableNames), [...]);
const segs = useMemo(() => tokenize(value, valid), [value, valid]);
const unknown = useMemo(() => unknownTokens(value, valid), [value, valid]);
const foreign = useMemo(() => foreignTokens(value, valid), [value, valid]);

tokenize, unknownTokens, and foreignTokens all call text.matchAll(TOKEN_RE) independently, so the text is scanned three times on each render. For typical automation text this is negligible, but it could be collapsed into a single pass: tokenize already produces all the segments with their statuses, so unknown and foreign can be derived from segs via .filter(s => s.token && s.status === 'bad') etc. This would also remove the two extra matchAll calls.

Doc comment is stale — TokenTextField.tsx:5

The JSDoc says "blue when recognized, red+wavy when not" but no longer mentions amber/foreign. Minor, but worth updating to match the three-tier model described in the PR.

---

CSS

.ae-token-foreign dotted underline — AutomationsPage.css:97-100

.ae-tokenfield-backdrop .ae-token-foreign {
  background: color-mix(in srgb, var(--ctp-yellow) 26%, transparent);
  color: var(--ctp-text); border-radius: 3px; text-decoration: underline dotted var(--ctp-yellow);
}

Looks correct and consistent with the ok (no underline) / foreign (dotted amber) / bad (wavy red) visual hierarchy.

---

Tests

anyCache not reset between tests — tokenHints.test.ts

The module-level anyCache is set on the first call and never cleared. If a test ever mutated TRIGGER_TOKENS (unlikely here, but possible in future), the cache would be stale. The existing tests are fine, but the exported anyTriggerTokenSet() function is not reset — this is another argument for the IIFE constant approach mentioned above.

Coverage is good — the three classifyToken scenarios (ok/foreign/bad), de-duplication, empty tokens, unknownTokens vs. foreignTokens split, and per-trigger matching are all tested.

Missing edge case: var.* tokens are never foreign — a {{ var.unknownVar }} is correctly bad (tested), but there's no test asserting that a var.* token is never classified as foreign even if it "looks valid". The current logic naturally handles this since classifyToken only checks path.startsWith('trigger.') for the foreign path, but an explicit test for var.nope → bad (not foreign) would lock that in.

---

Minor

• The anyCache export is not tested directly but is used transitively. If it's only needed internally, it doesn't need to be exported.
• tokenHints.ts:54-55: path = m[1].trim() is called in both tokenize and the two unknown/foreignTokens functions — no bug, just redundant trim calls when derived from segs.

---

Verdict

Approve with suggestions. The core fix is correct and well-tested. The main actionable items are:

1. (Nice to have) Replace the mutable anyCache with a module-level IIFE constant.
2. (Nice to have) Derive unknown/foreign lists from the already-computed segs to avoid triple text scanning.
3. (Minor) Update the JSDoc in TokenTextField.tsx to mention the three tiers.

None of these are blocking.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(automations): don't flag valid-but-wrong-trigger {{ }} tokens as typos

• Gather context and read changed files
• Review tokenHints.ts changes
• Review TokenTextField.tsx changes
• Review CSS changes
• Review test coverage
• Post final review

---

Summary

The PR correctly addresses the false-positive typo flagging. The three-tier classification (ok/foreign/bad) is the right model. This is well-implemented and well-tested.

---

tokenHints.ts

diagnoseTokens is a cleaner replacement for the old unknownTokens/foreignTokens split — the consolidated function with type-specific messages (:57-91) is better API design. Good call.

Mutable module-level cache — tokenHints.ts:28-36

let anyCache: Set<string> | null = null;
export function anyTriggerTokenSet(): Set<string> {
  if (anyCache) return anyCache;
  ...

Since TRIGGER_TOKENS is a compile-time constant, this can be simplified to avoid the mutable state:

const ANY_TRIGGER_TOKEN_SET: Set<string> = (() => {
  const set = new Set<string>();
  for (const toks of Object.values(TRIGGER_TOKENS)) for (const [k] of toks) set.add(`trigger.${k}`);
  for (const [k] of UNIVERSAL_TOKENS) set.add(`trigger.${k}`);
  return set;
})();

This is not a blocking issue — TRIGGER_TOKENS is static so the cache never goes stale — but the mutable variable is a code smell and can cause subtle test pollution if module state leaks.

diagnoseTokens calls text.matchAll(TOKEN_RE) independently from tokenize — this means the text is scanned twice per render. For typical automation text this is negligible, but diagnoseTokens could derive from tokenize's output if that ever matters.

classifyToken empty-path early return — tokenHints.ts:40

if (path.length === 0 || valid.has(path)) return 'ok';

Returning 'ok' for empty path is correct (avoids marking {{ }} as bad), and the tokenize caller sets token: path.length > 0 so empty segments won't show a mark anyway. Harmless but slightly opaque — the existing module-level comment covers it.

---

TokenTextField.tsx

Stale JSDoc — TokenTextField.tsx:5-7

The doc comment still says "blue when recognized, red+wavy when not". It should mention the three tiers now (blue/amber/red) to match reality. Minor, but the comment actively misleads anyone reading it.

diagnoseTokens replaces two separate useMemo calls — the refactored component is cleaner with three useMemos (valid, segs, diags) instead of four. Good simplification.

---

CSS

ae-token-foreign (dotted amber underline) visually distinct from ae-token-ok (no underline) and ae-token-bad (wavy red) — the hierarchy is clear and consistent. No issues.

ae-token-diag--warn using --ctp-yellow matches the foreign highlight color — visually coherent.

---

Tests

Coverage is solid for the happy paths: classifyToken (ok/foreign/bad), diagnoseTokens type-specific messages, deduplication, empty tokens, and tokenize status tagging.

One missing edge case — there's no test asserting var.nope → bad (not foreign). The logic is correct (classifyToken only checks path.startsWith('trigger.') for the foreign branch), but an explicit test would lock this in:

it('var.* tokens are never foreign, only bad', () => {
  const sys = validTokenSet('trigger.system', []);
  expect(classifyToken('var.nope', sys)).toBe('bad'); // not 'foreign'
});

anyTriggerTokenSet not directly tested — it's exercised transitively via classifyToken('trigger.from', sys), which is fine. But if the IIFE refactor above is applied, this becomes moot.

---

Verdict

Approve. The core fix is correct and well-tested. Two actionable items, neither blocking:

1. (Nice to have) Replace the mutable anyCache with a module-level IIFE const.
2. (Minor) Update the JSDoc in TokenTextField.tsx to describe the three tiers.

View job

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(automations): don't flag valid-but-wrong-trigger {{ }} tokens as typos

• Gather context
• Review tokenHints.ts
• Review TokenTextField.tsx
• Review CSS changes
• Review test coverage
• Post final review

---

Summary

This PR correctly fixes the false-positive typo detection bug. The three-tier classification (ok/foreign/bad) with a consolidated diagnoseTokens function is the right design. Previous reviewer comments have already been addressed well — the unknownTokens/foreignTokens split is gone and replaced with the cleaner diagnoseTokens API. Overall this is solid, ready-to-merge work. A few observations below.

---

tokenHints.ts

Mutable module-level cache — tokenHints.ts:28-36

let anyCache: Set<string> | null = null;
export function anyTriggerTokenSet(): Set<string> {
  if (anyCache) return anyCache;
  ...
  anyCache = set;
  return set;
}

Since TRIGGER_TOKENS is a compile-time constant, this can be a module-level const with an IIFE, eliminating mutable state and the risk of test pollution if the module is hot-reloaded:

const ANY_TRIGGER_TOKEN_SET: Set<string> = (() => {
  const set = new Set<string>();
  for (const toks of Object.values(TRIGGER_TOKENS)) for (const [k] of toks) set.add(`trigger.${k}`);
  for (const [k] of UNIVERSAL_TOKENS) set.add(`trigger.${k}`);
  return set;
})();

This is not blocking — TRIGGER_TOKENS won't change at runtime — but it's cleaner.

diagnoseTokens re-scans the text independently from tokenize — both call text.matchAll(TOKEN_RE). For typical automation payloads this is negligible, but diagnoseTokens could be derived from tokenize's output (filter segs where s.token && s.status !== 'ok') to avoid the second pass. Not blocking.

classifyToken empty-path early return — tokenHints.ts:40

if (path.length === 0 || valid.has(path)) return 'ok';

Returning 'ok' for an empty path is correct and harmless (the tokenize caller already sets token: path.length > 0 so empty segments don't render a mark). Behavior is fine.

---

TokenTextField.tsx

Stale JSDoc — TokenTextField.tsx:5-7

 * tokens colored — blue when recognized, red+wavy when not — while a transparent

This still describes only two tiers. Should mention the three tiers: blue (ok), amber/dotted (foreign), red+wavy (bad). The comment actively misleads maintainers.

diagnoseTokens replacing two useMemos is a net improvement — three useMemos (valid, segs, diags) vs. the previous four is cleaner. Good refactor.

No key stability concern — segs.map((s, i) => ... key={i}) — array indices as keys is fine here since segs is purely display-derived and never reordered independently.

---

CSS

ae-token-foreign (dotted amber underline) is visually distinct from ae-token-ok (no underline) and ae-token-bad (wavy red) — the hierarchy reads clearly. ae-token-diag--warn using --ctp-yellow is coherent with the amber highlight. No issues.

---

Tests — tokenHints.test.ts

Coverage is good — the three classifyToken paths (ok/foreign/bad), all four diagnoseTokens message variants (warn foreign, error bad-trigger, error bad-var, error unrecognized), deduplication, empty tokens, and per-trigger status tagging in tokenize are all exercised.

One missing edge case — there's no assertion that var.* tokens are never classified as foreign (only bad). The logic is correct (classifyToken only checks path.startsWith('trigger.') for the foreign branch), but an explicit test would lock this in:

it('var.* tokens with unknown name are bad, never foreign', () => {
  const sys = validTokenSet('trigger.system', []);
  expect(classifyToken('var.nope', sys)).toBe('bad'); // not 'foreign'
});

anyTriggerTokenSet not directly tested — it's exercised transitively, which is fine. If the IIFE refactor is done it becomes an internal detail.

---

Verdict

Approve with minor suggestions. The core fix is correct, well-tested, and properly handles the three-tier UX. Actionable items, in priority order:

1. (Minor) Update the JSDoc in TokenTextField.tsx to describe all three tiers.
2. (Nice to have) Replace the mutable anyCache with a module-level IIFE const.
3. (Nice to have) Add a test asserting var.nope → bad (never foreign).

None are blocking.