Merge pull request #128 from walkframe/fix/percentage

Fix percentage string handling and cache strip results in formula engine

Author: righ

e2e/formula.spec.ts

@@ -388,6 +388,14 @@ test('simple calculation', async ({ page }) => {
   await go(page, 'formula-simple--simple-calculation');
   const largeEditor = page.locator('.gs-formula-bar textarea');
 
+  // Verify A1 formula bar shows raw formula (formulaEnabled: false active)
+  await page.locator("[data-address='A1']").click();
+  expect(await largeEditor.inputValue()).toBe('=100 + 5');
+
+  // Verify that A-column cell containing an address reference is NOT converted to an internal ID
+  // A2 has value "=B1 - 60" with formulaEnabled:false — it must render the literal string, not e.g. "=#3- 60"
+  expect(await page.locator('[data-address="A2"] .gs-cell-rendered').textContent()).toBe('=B1 - 60');
+
   // Column A: formulaEnabled: false — formula is displayed as raw text, not evaluated
   // Column B: formula is evaluated normally
 
@@ -451,17 +459,13 @@ test('simple calculation', async ({ page }) => {
   expect(await page.locator('[data-address="B34"] .gs-cell-rendered').textContent()).toBe('123');
   expect(await page.locator('[data-address="B35"] .gs-cell-rendered').textContent()).toBe('0123');
 
-  // Verify that none of the B-column results are #VALUE!
-  for (let row = 1; row <= 35; row++) {
-    const text = await page.locator(`[data-address="B${row}"] .gs-cell-rendered`).textContent();
-    expect(text).not.toBe('#VALUE!');
-  }
-
-  // Verify A1 formula bar shows raw formula (formulaEnabled: false active)
-  await page.locator("[data-address='A1']").click();
-  expect(await largeEditor.inputValue()).toBe('=100 + 5');
-
-  // Verify that A-column cell containing an address reference is NOT converted to an internal ID
-  // A2 has value "=B1 - 60" with formulaEnabled:false — it must render the literal string, not e.g. "=#3- 60"
-  expect(await page.locator('[data-address="A2"] .gs-cell-rendered').textContent()).toBe('=B1 - 60');
+  // Percentage handling — scroll into view first (virtualized rows may not be rendered)
+  await page.locator('[data-address="A36"]').scrollIntoViewIfNeeded();
+  expect(await page.locator('[data-address="B37"] .gs-cell-rendered').textContent()).toBe('5.05');
+  expect(await page.locator('[data-address="B38"] .gs-cell-rendered').textContent()).toBe('10');
+  expect(await page.locator('[data-address="B39"] .gs-cell-rendered').textContent()).toBe('0.5');
+  expect(await page.locator('[data-address="B40"] .gs-cell-rendered').textContent()).toBe('1.5');
+  expect(await page.locator('[data-address="B41"] .gs-cell-rendered').textContent()).toBe('50%');
+  expect(await page.locator('[data-address="B42"] .gs-cell-rendered').textContent()).toBe('5.5');
+  expect(await page.locator('[data-address="B43"] .gs-cell-rendered').textContent()).toBe('23.43');
 });

gridsheet.png

@@ -326,6 +326,7 @@ export class Token {
   entity: any;
   precedence: number;
   closed: boolean;
+  raw?: string;
   private at?: Id;
 
   constructor(type: TokenType, entity: any, precedence = 0, at?: Id, closed = true) {
@@ -351,6 +352,9 @@ export class Token {
       if (typeof this.entity === 'boolean') {
         return this.entity ? 'TRUE' : 'FALSE';
       }
+      if (this.raw != null) {
+        return this.raw;
+      }
     }
     return this.entity as string;
   }
@@ -524,7 +528,7 @@ export class Lexer {
         switch (t.type) {
           case 'VALUE':
             if (typeof t.entity === 'number' || typeof t.entity === 'boolean') {
-              return t.entity;
+              return t.raw ?? t.entity;
             }
             return t.closed ? `"${t.entity}"` : `"${t.entity}`;
           case 'ID':
@@ -653,7 +657,9 @@ export class Lexer {
             break;
           }
           if (buf.match(/^[+-]?(\d*[.])?\d+$/)) {
-            this.tokens.push(new Token('VALUE', parseFloat(buf), 0, this.at));
+            const tok = new Token('VALUE', parseFloat(buf), 0, this.at);
+            tok.raw = buf;
+            this.tokens.push(tok);
           } else {
             const bool = BOOLS[buf.toLowerCase()];
             if (bool != null) {

packages/core/src/formula/evaluator.ts

@@ -19,6 +19,7 @@ import {
   asyncCacheMiss,
 } from './__async';
 import { Time } from '../../lib/time';
+import { isNumeric, isPercentage } from './__utils';
 
 export type FunctionCategory =
   | 'math'
@@ -87,7 +88,7 @@ const matchesType = (
       case 'any':
         return true;
       case 'number':
-        if (typeof value === 'number') {
+        if (isNumeric(value)) {
           return true;
         }
         break;
@@ -315,6 +316,9 @@ export class BaseFunction {
           value = stripMatrix(value, this.at);
         }
       }
+      if (isPercentage(value)) {
+        value = parseFloat(value.slice(0, -1)) / 100;
+      }
       if (!matchesType(value, def)) {
         throw new FormulaError(
           '#VALUE!',

packages/core/src/formula/functions/__base.ts

@@ -55,6 +55,26 @@ export type EnsureBooleanOptions = {
   ignore?: boolean;
 };
 
+export const isPercentage = (value: any): value is string => {
+  if (typeof value !== 'string' || !value.endsWith('%')) {
+    return false;
+  }
+  return !isNaN(parseFloat(value.slice(0, -1)));
+};
+
+export const isNumeric = (value: any): boolean => {
+  if (typeof value === 'number') {
+    return true;
+  }
+  if (isPercentage(value)) {
+    return true;
+  }
+  if (typeof value === 'string') {
+    return !isNaN(parseFloat(value)) && isFinite(Number(value));
+  }
+  return false;
+};
+
 export const ensureNumber = (value: any, options?: EnsureNumberOptions): number => {
   const { alternative, ignore } = options || {};
   if (Pending.is(value)) {
@@ -78,11 +98,8 @@ export const ensureNumber = (value: any, options?: EnsureNumberOptions): number
     return value.days;
   }
 
-  if (typeof value === 'string' && value.endsWith('%')) {
-    const num = parseFloat(value.slice(0, -1));
-    if (!isNaN(num)) {
-      return num / 100;
-    }
+  if (isPercentage(value)) {
+    return parseFloat(value.slice(0, -1)) / 100;
   }
 
   const num = parseFloat(value as string);

packages/core/src/formula/functions/__utils.ts

@@ -1,6 +1,6 @@
 import { FormulaError } from '../formula-error';
 import { BaseFunction, FunctionCategory, FunctionArgumentDefinition, isMatrix } from './__base';
-import { ensureNumber, eachMatrix } from './__utils';
+import { ensureNumber, isNumeric, eachMatrix } from './__utils';
 
 const description = `Returns the average of a series of numbers or cells.`;
 
@@ -26,8 +26,8 @@ export class AverageFunction extends BaseFunction {
         eachMatrix(
           val,
           (v) => {
-            if (typeof v === 'number') {
-              sum += v;
+            if (isNumeric(v)) {
+              sum += ensureNumber(v);
               count++;
             }
           },

packages/core/src/formula/functions/average.ts

@@ -1,5 +1,5 @@
 import { BaseFunction, FunctionCategory, FunctionArgumentDefinition, isMatrix } from './__base';
-import { ensureNumber, eachMatrix } from './__utils';
+import { ensureNumber, isNumeric, eachMatrix } from './__utils';
 
 const description = `Returns the count of a series of numbers or cells.`;
 
@@ -24,7 +24,7 @@ export class CountFunction extends BaseFunction {
         eachMatrix(
           val,
           (v) => {
-            if (typeof v === 'number') {
+            if (isNumeric(v)) {
               count++;
             }
           },

packages/core/src/formula/functions/count.ts

@@ -1,5 +1,5 @@
 import { BaseFunction, FunctionCategory, FunctionArgumentDefinition, isMatrix } from './__base';
-import { ensureNumber, eachMatrix } from './__utils';
+import { ensureNumber, isNumeric, eachMatrix } from './__utils';
 
 const description = `Returns the max in a series of numbers or cells.`;
 
@@ -25,9 +25,10 @@ export class MaxFunction extends BaseFunction {
         eachMatrix(
           val,
           (v) => {
-            if (typeof v === 'number') {
-              if (v > max) {
-                max = v;
+            if (isNumeric(v)) {
+              const num = ensureNumber(v);
+              if (num > max) {
+                max = num;
               }
               hasValues = true;
             }

packages/core/src/formula/functions/max.ts

@@ -1,5 +1,5 @@
 import { BaseFunction, FunctionCategory, FunctionArgumentDefinition, isMatrix } from './__base';
-import { ensureNumber, eachMatrix } from './__utils';
+import { ensureNumber, isNumeric, eachMatrix } from './__utils';
 
 const description = `Returns the min in a series of numbers or cells.`;
 
@@ -25,9 +25,10 @@ export class MinFunction extends BaseFunction {
         eachMatrix(
           val,
           (v) => {
-            if (typeof v === 'number') {
-              if (v < min) {
-                min = v;
+            if (isNumeric(v)) {
+              const num = ensureNumber(v);
+              if (num < min) {
+                min = num;
               }
               hasValues = true;
             }

packages/core/src/formula/functions/min.ts

@@ -67,21 +67,15 @@ export const solveFormula = ({ value, sheet, point, raise = true, resolution = '
     }
   }
 
-  if (resolution === 'RESOLVED' && solved instanceof Sheet) {
-    // Unwrap to scalar (top-left cell of the sheet's area).
-    // NOTE: We intentionally call solveSheet directly here instead of stripSheet,
-    // to avoid a three-way cycle: solveFormula → stripSheet → solveSheet → solveFormula.
-    // The mutual recursion between solveFormula and solveSheet is unavoidable by design,
-    // but routing through stripSheet would make the call graph harder to follow and reason about.
-    solved = solveSheet({ sheet: solved, raise, at })[0]?.[0];
-  }
-  // 'EVALUATED' resolution: keep Sheet objects intact (range formulas return Sheet, not scalar).
-
   if (Spilling.is(solved)) {
     solved = sheet.spill(point, solved.matrix);
   } else {
     sheet.finishSolvedCache(point, solved);
   }
+
+  if (resolution === 'RESOLVED' && solved instanceof Sheet) {
+    solved = stripSheet({ value: solved, raise, at });
+  }
   if (Pending.is(solved)) {
     sheet.finishSolvedCache(point, solved);
   }
@@ -140,8 +134,8 @@ export type StripSheetProps = {
 };
 
 export const stripSheet = ({ value, at, raise = true }: StripSheetProps): any => {
-  while (value instanceof Sheet) {
-    value = solveSheet({ sheet: value, raise, at })[0]?.[0];
+  if (value instanceof Sheet) {
+    return value.strip({ raise, at });
   }
   return value;
 };