packages/sheets/test/formula/function-catalog.test.ts (1)

419-431: Add representative Operator catalog assertions.

Line 427 verifies that some Operator category exists, but it does not lock down the new catalog entries or the CONCAT/UNIQUE category move. Consider adding explicit assertions such as findFunction('ADD')!.category, findFunction('CONCAT')!.category, and findFunction('UNIQUE')!.category resolving to Operator. As per coding guidelines, **/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheets/test/formula/function-catalog.test.ts` around lines 419 -
431, Add explicit assertions to lock down the Operator category entries: call
findFunction('ADD'), findFunction('CONCAT') and findFunction('UNIQUE') and
assert their .category resolves to 'Operator' (i.e.,
expect(findFunction('ADD')!.category).toEqual('Operator'), etc.). Add these
assertions in the same test near the listFunctionCategories() check so the test
validates the catalog entries and the CONCAT/UNIQUE category assignment using
the existing findFunction helper.

packages/sheets/src/formula/functions-operator.ts (4)

31-52: compareScalars return type declares an unreachable bool variant.

The declared return type is number | { t: 'bool'; v: boolean } | ErrNode, but no path returns a bool node — only number or ErrNode. That then forces the awkward typeof cmp === 'object' && 't' in cmp check plus as ErrNode cast at the call sites (Line 213, and again at Lines 269–270). Tightening the return type to number | ErrNode removes the dead variant and lets the callers use a simple typeof cmp !== 'number' narrow without casts.

♻️ Proposed change

-function compareScalars(
-  left: EvalNode,
-  right: EvalNode,
-): number | { t: 'bool'; v: boolean } | ErrNode {
+function compareScalars(
+  left: EvalNode,
+  right: EvalNode,
+): number | ErrNode {

and at call sites:

-    if (typeof cmp === 'object' && 't' in cmp) return cmp as ErrNode;
+    if (typeof cmp !== 'number') return cmp;

-  if (typeof cmpLower === 'object') return cmpLower as ErrNode;
-  if (typeof cmpUpper === 'object') return cmpUpper as ErrNode;
+  if (typeof cmpLower !== 'number') return cmpLower;
+  if (typeof cmpUpper !== 'number') return cmpUpper;

Also applies to: 213-213

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheets/src/formula/functions-operator.ts` around lines 31 - 52, The
compareScalars function's declared return type includes an unused "{ t: 'bool';
v: boolean }" variant; change its signature to return number | ErrNode (remove
the bool node variant) and keep the implementation returning only numbers or
ErrNode; then update call sites that handle its result (the comparisons that
currently test for an object or cast to ErrNode) to simply use typeof cmp !==
'number' to detect errors and narrow types without casts (look for usages of
compareScalars and the existing object/'t' in cmp checks and replace them with a
numeric-type check).

---

9-23: DRY: resolveScalar duplicates resolveValue from formula.ts.

The JSDoc explicitly says this mirrors formula.ts's resolveValue. Having two copies of the empty/TRUE/FALSE/Number coercion will drift over time (e.g., one is updated to handle a new literal and the other isn't), which would silently change comparison semantics between =A1<B1 and =LT(A1,B1). Prefer exporting the existing helper from formula.ts and importing it here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheets/src/formula/functions-operator.ts` around lines 9 - 23, The
resolveScalar function duplicates coercion logic already implemented in
resolveValue (formula.ts); remove resolveScalar, export or make resolveValue
available from formula.ts, import resolveValue into this module, and replace all
uses of resolveScalar with resolveValue (ensuring EvalNode and Grid types are
imported or compatible). Verify signatures match (node: EvalNode, grid?: Grid)
and update any call sites in functions-operator.ts to pass the same args so
comparison semantics remain consistent.

---

200-223: Exhaustive switch lacks a return outside the cases.

comparisonFunc's arrow returns inside each case, but there is no trailing return/throw. Under noImplicitReturns/strict settings this can fail to type-check; even if it compiles today, a future addition to the op union would silently fall through to undefined. Add an exhaustive fallback.

🛡️ Proposed change

     switch (op) {
       case 'eq': return { t: 'bool', v: cmp === 0 };
       case 'ne': return { t: 'bool', v: cmp !== 0 };
       case 'lt': return { t: 'bool', v: cmp < 0 };
       case 'lte': return { t: 'bool', v: cmp <= 0 };
       case 'gt': return { t: 'bool', v: cmp > 0 };
       case 'gte': return { t: 'bool', v: cmp >= 0 };
     }
+    const _exhaustive: never = op;
+    return _exhaustive;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheets/src/formula/functions-operator.ts` around lines 200 - 223,
The function comparisonFunc currently switches on op
('eq'|'ne'|'lt'|'lte'|'gt'|'gte') and returns inside each case but has no
fallback return; add an exhaustive fallback after the switch in the returned
arrow function to avoid undefined returns — either throw a descriptive Error
mentioning the unexpected op or return an ErrNode/EvalNode indicating an
internal error. Update comparisonFunc (the inner returned function that calls
twoArgs, resolveScalar and compareScalars) so any unknown op value is handled
deterministically (use the same ErrNode shape used elsewhere) to satisfy
noImplicitReturns and prevent silent undefined.

---

315-333: Use a proper type signature instead of any[] for the exported operator registry.

operatorEntries uses [string, (...args: any[]) => EvalNode][], which loses type safety. Every operator function in this file already declares the correct signature with ctx: FunctionContext, visit: (tree: ParseTree) => EvalNode, grid?: Grid. Define a shared type alias and apply it to operatorEntries so misregistration is caught at compile time:

+type OperatorFn = (
+  ctx: FunctionContext,
+  visit: (tree: ParseTree) => EvalNode,
+  grid?: Grid,
+) => EvalNode;
+
-export const operatorEntries: [string, (...args: any[]) => EvalNode][] = [
+export const operatorEntries: [string, OperatorFn][] = [

All required imports (FunctionContext, ParseTree, Grid) are already present at the top of this file.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheets/src/formula/functions-operator.ts` around lines 315 - 333,
Create a proper function type alias (e.g., OperatorFn) matching the real
signature used by all operator implementations: (ctx: FunctionContext, visit:
(tree: ParseTree) => EvalNode, grid?: Grid) => EvalNode, then change the
operator registry declaration for operatorEntries from [string, (...args: any[])
=> EvalNode][] to [string, OperatorFn][] so misregistered entries (e.g.,
addFunc, minusFunc, concatFunc, uniqueFunc, etc.) are type-checked at compile
time; keep the alias and the updated operatorEntries export in this file so
existing imports (FunctionContext, ParseTree, Grid) are reused.

packages/sheets/test/formula/formula.test.ts (1)

3700-3830: LGTM.

Good, focused coverage for the new operator-backed functions. One optional enhancement to consider: none of the ISBETWEEN cases exercise cell-reference inputs or error-propagation (e.g., ISBETWEEN(A1, 1, 10) with A1 unset, or with a #DIV/0! value/bound). Adding a case or two there would lock in the resolveScalar/error-propagation paths in isbetweenFunc.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheets/test/formula/formula.test.ts` around lines 3700 - 3830, Add
tests to exercise ISBETWEEN with cell-reference inputs and error propagation:
add cases using evaluate('=ISBETWEEN(A1,1,10)', grid) where A1 is unset (should
behave like blank/zero per resolveScalar) and where A1 or bounds contain an
error token (e.g., '#DIV/0!') to verify isbetweenFunc forwards errors; target
the test block named "ISBETWEEN" and use the evaluate helper and isbetweenFunc
semantics to assert correct TRUE/FALSE or error string results so
resolveScalar/error-propagation paths are covered.

Fix all unresolved CodeRabbit comments on this PR:

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