Fix error string from #N/A! to #N/A#101
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis pull request standardizes the spreadsheet "not available" error token from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Verification: verify:selfResult: ✅ PASS in 108.0s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/sheets/src/formula/functions-date.ts (2)
450-489:⚠️ Potential issue | 🟠 Major
NETWORKDAYScurrently ignores the advertisedholidaysargument.Arity allows three arguments here, but
exprs[2]is never consumed, soNETWORKDAYS(start, end, holidays)silently returns a weekend-only count. Either implement holiday exclusion here or reject the third argument for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/formula/functions-date.ts` around lines 450 - 489, networkdaysFunc currently ignores exprs[2] (holidays); either reject the third arg or implement holiday exclusion—prefer implementing: when exprs.length === 3 call visit(exprs[2]) and convert the result into a set of Date-only keys (use parseDate for single values and handle arrays/ranges by iterating elements) then when iterating days in networkdaysFunc skip any date whose Date-only key exists in that holidays set; ensure you still use parseDate for start/end and return the same error node if any holiday entries fail to parse.
496-545:⚠️ Potential issue | 🟠 MajorPropagate optional-argument errors instead of defaulting.
NETWORKDAYS.INTL,WORKDAY.INTL, andDAYS360fall back to default weekend/method values when the optional argument evaluates to an error. Formulas likeWORKDAY.INTL(A1, 1, NA())should surface#N/A, not return a computed result.Also applies to: 850-903, 1011-1048
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/formula/functions-date.ts` around lines 496 - 545, In networkdaysintlFunc, optional-argument evaluation errors are being swallowed and the function falls back to defaults; ensure any error node from evaluating the optional weekend or holidays arguments is propagated instead of defaulting: if NumberArgs.map(visit(exprs[2]), grid) yields a node with t === 'err', return that node immediately (do not call getWeekendDays), and likewise if visit(exprs[3]) yields a node with t === 'err' return it (do not try to treat it as a ref); apply the same pattern to the analogous handling in WORKDAY.INTL and DAYS360 (the other ranges you noted) so optional-argument errors bubble up.packages/sheets/src/formula/functions-engineering.ts (1)
653-673:⚠️ Potential issue | 🟠 Major
COMPLEX(..., "j")still does not round-trip through theIM*functions.This branch can emit values like
1+2j, but the shared complex parser only recognizesi, soIMREAL(COMPLEX(1,2,"j"))and the otherIM*helpers still fail with#VALUE!. Normalizejbefore returning here, or teachparseComplexto accept both suffixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/formula/functions-engineering.ts` around lines 653 - 673, The COMPLEX handling in complexFunc can emit a "j" suffix which downstream functions (e.g., parseComplex used by IMREAL/IMAG/IMABS) don't recognize; update complexFunc so after obtaining suffix from toStr(visit(exprs[2]), grid) it normalizes 'j' to 'i' (e.g., if sNode.v === 'j' set suffix = 'i') before returning, while still validating only 'i' or 'j'; reference complexFunc, toStr, NumberArgs.map (and parseComplex for context) to locate where to normalize.packages/sheets/src/formula/functions-text.ts (1)
1136-1216:⚠️ Potential issue | 🟠 MajorAdd a comment explaining why direct regex compilation from formula input is unsafe.
The
REGEXMATCH,REGEXEXTRACT, andREGEXREPLACEfunctions accept user-supplied regex patterns that are compiled directly vianew RegExp(pattern.v). A pathological pattern—for example,(a+)+b—can trigger catastrophic backtracking and hang the evaluator thread indefinitely, with no recovery. The existing try-catch only guards against syntax errors (e.g., unclosed brackets), not ReDoS attacks. Consider restricting regex features or implementing complexity validation before compilation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/formula/functions-text.ts` around lines 1136 - 1216, The three functions regexmatchFunc, regexextractFunc, and regexreplaceFunc compile user-supplied patterns directly with new RegExp(pattern.v), which leaves the evaluator vulnerable to catastrophic backtracking / ReDoS; update these functions to validate or sandbox patterns before compilation by (a) enforcing a max pattern length, rejecting patterns containing known-dangerous constructs (e.g. nested quantifiers like (.+)+, (a+)+, excessive backreferences or lookaround combos), or (b) using a safe regex strategy such as a pre-compiled/safe-regex library or running the RegExp execution inside a time-limited worker/timeout and returning an error on timeout; ensure the validation/sandbox is applied in regexmatchFunc, regexextractFunc and regexreplaceFunc prior to calling new RegExp and that failures return a consistent error (e.g. '#VALUE!') to avoid hanging the evaluator.
🧹 Nitpick comments (1)
packages/sheets/src/formula/formula.ts (1)
496-499: Consider promoting error tokens to shared constants.This PR had to update the same literal in multiple files, and one missed runtime comparison would leave checks like
IFNA/ISNAout of sync with returned values. A single exported source of truth here would make future token changes mechanical.♻️ Suggested shape
+export const ERROR_VALUES = { + VALUE: '#VALUE!', + REF: '#REF!', + NA: '#N/A', + ERROR: '#ERROR!', + DIV0: '#DIV/0!', +} as const; + export type ErrNode = { t: 'err'; - v: '#VALUE!' | '#REF!' | '#N/A' | '#ERROR!' | '#DIV/0!'; + v: (typeof ERROR_VALUES)[keyof typeof ERROR_VALUES]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/formula/formula.ts` around lines 496 - 499, Replace the inline error literal union in ErrNode with a single exported set of error token constants and use those constants everywhere; specifically, add exported constants (e.g., ERR_VALUE, ERR_REF, ERR_NA, ERR_ERROR, ERR_DIV0) and change ErrNode.v to reference those constants (either via a union of typeof CONSTANTs or a shared ErrorTokens type), then update all places that construct or compare error nodes (functions that return ErrNode and checks like IFNA/ISNA) to use the exported constants instead of raw string literals so all runtime comparisons and returns come from the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/formula.md`:
- Around line 102-103: The ErrNode documentation's union of error strings is
missing "#DIV/0!" which differs from the engine's error type; update the ErrNode
declaration (`ErrNode { t: 'err', v: ... }`) to include "#DIV/0!" in the union
of documented values (and update the other occurrence noted around the second
block at lines ~183-184) so the docs match the engine's error set and remove
potential confusion for contributors.
---
Outside diff comments:
In `@packages/sheets/src/formula/functions-date.ts`:
- Around line 450-489: networkdaysFunc currently ignores exprs[2] (holidays);
either reject the third arg or implement holiday exclusion—prefer implementing:
when exprs.length === 3 call visit(exprs[2]) and convert the result into a set
of Date-only keys (use parseDate for single values and handle arrays/ranges by
iterating elements) then when iterating days in networkdaysFunc skip any date
whose Date-only key exists in that holidays set; ensure you still use parseDate
for start/end and return the same error node if any holiday entries fail to
parse.
- Around line 496-545: In networkdaysintlFunc, optional-argument evaluation
errors are being swallowed and the function falls back to defaults; ensure any
error node from evaluating the optional weekend or holidays arguments is
propagated instead of defaulting: if NumberArgs.map(visit(exprs[2]), grid)
yields a node with t === 'err', return that node immediately (do not call
getWeekendDays), and likewise if visit(exprs[3]) yields a node with t === 'err'
return it (do not try to treat it as a ref); apply the same pattern to the
analogous handling in WORKDAY.INTL and DAYS360 (the other ranges you noted) so
optional-argument errors bubble up.
In `@packages/sheets/src/formula/functions-engineering.ts`:
- Around line 653-673: The COMPLEX handling in complexFunc can emit a "j" suffix
which downstream functions (e.g., parseComplex used by IMREAL/IMAG/IMABS) don't
recognize; update complexFunc so after obtaining suffix from
toStr(visit(exprs[2]), grid) it normalizes 'j' to 'i' (e.g., if sNode.v === 'j'
set suffix = 'i') before returning, while still validating only 'i' or 'j';
reference complexFunc, toStr, NumberArgs.map (and parseComplex for context) to
locate where to normalize.
In `@packages/sheets/src/formula/functions-text.ts`:
- Around line 1136-1216: The three functions regexmatchFunc, regexextractFunc,
and regexreplaceFunc compile user-supplied patterns directly with new
RegExp(pattern.v), which leaves the evaluator vulnerable to catastrophic
backtracking / ReDoS; update these functions to validate or sandbox patterns
before compilation by (a) enforcing a max pattern length, rejecting patterns
containing known-dangerous constructs (e.g. nested quantifiers like (.+)+,
(a+)+, excessive backreferences or lookaround combos), or (b) using a safe regex
strategy such as a pre-compiled/safe-regex library or running the RegExp
execution inside a time-limited worker/timeout and returning an error on
timeout; ensure the validation/sandbox is applied in regexmatchFunc,
regexextractFunc and regexreplaceFunc prior to calling new RegExp and that
failures return a consistent error (e.g. '#VALUE!') to avoid hanging the
evaluator.
---
Nitpick comments:
In `@packages/sheets/src/formula/formula.ts`:
- Around line 496-499: Replace the inline error literal union in ErrNode with a
single exported set of error token constants and use those constants everywhere;
specifically, add exported constants (e.g., ERR_VALUE, ERR_REF, ERR_NA,
ERR_ERROR, ERR_DIV0) and change ErrNode.v to reference those constants (either
via a union of typeof CONSTANTs or a shared ErrorTokens type), then update all
places that construct or compare error nodes (functions that return ErrNode and
checks like IFNA/ISNA) to use the exported constants instead of raw string
literals so all runtime comparisons and returns come from the same source of
truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74a7f565-3323-49b5-be12-a51e5d758594
📒 Files selected for processing (16)
docs/design/formula.mddocs/tasks/archive/2026/02/20260227-harness-phase18-sheet-visual-baselines-todo.mdpackages/frontend/src/app/harness/visual/sheet-scenarios.tsxpackages/sheets/src/formula/formula.tspackages/sheets/src/formula/function-catalog.tspackages/sheets/src/formula/functions-database.tspackages/sheets/src/formula/functions-date.tspackages/sheets/src/formula/functions-engineering.tspackages/sheets/src/formula/functions-financial.tspackages/sheets/src/formula/functions-info.tspackages/sheets/src/formula/functions-logical.tspackages/sheets/src/formula/functions-lookup.tspackages/sheets/src/formula/functions-math.tspackages/sheets/src/formula/functions-statistical.tspackages/sheets/src/formula/functions-text.tspackages/sheets/test/formula/formula.test.ts
Collaborator
Visual baseline update neededThis PR changes the
How to update (pick one): # Option A — run locally with Docker (recommended, matches CI environment)
pnpm frontend test:visual:browser:docker:update
# Option B — run locally without Docker (needs a Chromium-compatible browser)
pnpm frontend test:visual:browser:updateThis will re-capture all visual baselines. Commit the updated After that, you can verify everything passes with: pnpm verify:browser:docker |
2 tasks
This was referenced May 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wafflebase return error
#N/A!when function return no applicable result (missing args), but google sheet/excel returns#N/A. It changes to return#N/A.Why
Compatible error string with google sheet.
Linked Issues
Fixes #
Verification
CI automatically posts a verification summary comment on this PR with
per-lane results for both
verify:selfandverify:integration.Skip reason (if applicable):
Risk Assessment
Notes for Reviewers
Summary by CodeRabbit
Bug Fixes
Documentation