Skip to content

feat(web): /calc expression evaluator -- pipeline-ab card 2 Arm A (BASELINE)#11

Open
eksno wants to merge 2 commits into
alphafrom
feat/calc-arm-a
Open

feat(web): /calc expression evaluator -- pipeline-ab card 2 Arm A (BASELINE)#11
eksno wants to merge 2 commits into
alphafrom
feat/calc-arm-a

Conversation

@eksno

@eksno eksno commented Jun 12, 2026

Copy link
Copy Markdown
Member

EXPERIMENT PR (card 2: harder, separating) -- DO NOT MERGE. pipeline-ab Arm A (BASELINE).

Baseline feature layout: Opus hands + full mid-process review loop + George QA + review-reconcile.

Correctness-deep card (expression parser/evaluator) chosen to separate Composer-hands-vs-Opus-hands. Both arms build from ONE shared plan. Stays OPEN for eyeball evaluation against its live Railway preview.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

@railway-app railway-app Bot temporarily deployed to testing / testing-pr-11 June 12, 2026 20:09 Destroyed
@railway-app

railway-app Bot commented Jun 12, 2026

Copy link
Copy Markdown

🚅 Deployed to the testing-pr-11 environment in testing

Service Status Web Updated (UTC)
testing ✅ Success (View Logs) Web Jun 12, 2026 at 8:31 pm

…d parser

Pure, independently-testable calc core under src/lib/calc (tokenize, parse,
evaluate, format, calculate). Precedence-climbing recursive descent:
right-associative ^, unary minus binds looser than ^, unary-negated exponents,
no implicit multiplication. 12-sig-digit float formatter (0.1+0.2 -> 0.3).
Exact error taxonomy (empty / unexpected char / unexpected token / incomplete /
mismatched parens / division by zero / non-finite).

/calc route: live result, inline error, capped (10) most-recent-first history
with per-entry copy-result button, auto-dismiss toast with full timer cleanup
on unmount, theme-token-only styling, prerender-safe. Nav wiring + global
Toaster mount.

Tooling: vitest + jsdom (only new deps), vitest.config.ts, test/test:watch
scripts, and the 7-file edge-case test floor (89 tests). check/build/format
green. docs/fixes/0002 records a latent eslint .svelte.ts parser-config gap.

Station-Trailer-Version: 1
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@eksno, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 1 minute and 32 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d5593b03-f548-4621-b512-3319118aa442

📥 Commits

Reviewing files that changed from the base of the PR and between c20b967 and a5a83ea.

⛔ Files ignored due to path filters (1)
  • web/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (25)
  • docs/fixes/0002-eslint-svelte-ts-runes-module-parser-gap.md
  • web/package.json
  • web/src/lib/calc/calculate.test.ts
  • web/src/lib/calc/calculate.ts
  • web/src/lib/calc/evaluate.test.ts
  • web/src/lib/calc/evaluate.ts
  • web/src/lib/calc/format.test.ts
  • web/src/lib/calc/format.ts
  • web/src/lib/calc/index.ts
  • web/src/lib/calc/parse.test.ts
  • web/src/lib/calc/parse.ts
  • web/src/lib/calc/tokenize.test.ts
  • web/src/lib/calc/tokenize.ts
  • web/src/lib/calc/types.ts
  • web/src/lib/clipboard.test.ts
  • web/src/lib/clipboard.ts
  • web/src/lib/components/site-header.svelte
  • web/src/lib/components/toaster.svelte
  • web/src/lib/toast-core.test.ts
  • web/src/lib/toast-core.ts
  • web/src/lib/toast.svelte.ts
  • web/src/routes/+layout.svelte
  • web/src/routes/calc/+page.svelte
  • web/src/routes/calc/+page.ts
  • web/vitest.config.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/calc-arm-a

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@eksno

eksno commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Final review (advisory -- not a merge gate)

calc Arm A (BASELINE) -- advisory red-team review

Worktree: /shared/repos/startino/.worktrees/calc-arm-a (web/)
Diff base: origin/alpha...HEAD (commit a5a83ea)
Status: ADVISORY -- does NOT block ship. tests/check/build pre-verified green.

Verdict: SHIP. The implementation is a faithful, high-fidelity realization of
calc-plan.md. Parser correctness is CLEAN across the pinned floor plus the
red-team edge cases I traced by hand. No act-worthy code fixes were found; the
only triage item is the docs/fixes/0002 mis-filing, judged keep-flagged (see
disposition). One real, reproducible defect exists OUTSIDE the feature's
acceptance gate: npm run lint (eslint half) fails on toast.svelte.ts -- this
is a pre-existing repo eslint-config gap, correctly diagnosed in 0002, with the
fix deferred because eslint.config.js is DO-NOT-TOUCH for this work.


Lens 1 -- Spec conformance (grammar / semantics / error-taxonomy / formatting)

PASS. Hand-traced every anti-ambiguity item in plan section 9 against the code:

  • ^ right-assoc (2^3^2 -> binary(^,2,binary(^,3,2))): parsePower recurses
    the RIGHT operand through parseUnary -> correct (parse.ts:74-84). Tested.
  • Unary minus looser than ^ (-2^2 -> unary(-, binary(^,2,2))): parseUnary
    consumes - then descends to parsePower which consumes the whole 2^2 as
    operand -> correct. AST shape test asserts it (parse.test.ts:13-24).
  • Exponent may be unary-negated (2^-2 = 0.25): RIGHT operand goes through
    parseUnary -> correct.
  • No implicit multiplication: 2(3)/2 3 -> trailing-token Unexpected token
    at top-level (parse.ts:136-138). Correct.
  • Number lexing (.5, 3., 1.5e3, 2E-2; reject bare . and 1e): sticky
    regex /(?:\d+\.?\d*|\.\d+)(?:[eE][+-]?\d+)?/y + the .-needs-trailing-digit
    guard (tokenize.ts:6,29). 1e matches just 1, leaves stray e ->
    Unexpected character "e". Correct and tested.
  • Division-by-zero guard (incl 0/0) in evaluate (evaluate.ts:25-27); non-finite
    final result gated in calculate (calculate.ts:17-20). Correct.
  • formatNumber: -0/0 -> '0'; round to 12 sig digits; exp branch for
    abs>=1e21 || abs<1e-6; trailing-zero normalization of the mantissa
    (format.ts). 0.1+0.2 -> '0.3', 1e21 -> '1e+21', 0.0000005 -> '5e-7'
    all correct.
  • Error taxonomy: every pinned string is produced at the right site. Empty
    pre-check, unexpected char (lex), unexpected token / incomplete / mismatched
    (parse), division by zero (eval), not-finite (calculate), defensive
    Invalid expression / Empty expression. All asserted by calculate.test.ts
    and the unit suites.

SPEC GAP (informational, NOT a code defect): plan line 353 contains an aside --
"a bare ) with no opener is Unexpected token". In this parser a LEADING bare
) reaches parsePrimary at a primary position and yields Incomplete expression (parse.ts:117-118), not Unexpected token. This is an internal
inconsistency in the SPEC: the pinned, tested () case (plan line 356, test
calculate.test.ts:156-158) REQUIRES rparen at primary position to yield
Incomplete expression. The implementer correctly satisfied the pinned test;
the aside is unreachable without positional context that the grammar does not
carry. Leading bare ) is not in the test floor, so behavior is undefined by
the floor and the chosen message is defensible. No action -- changing it would
break the pinned () case. Both arms will share this; it is a measurement
non-issue for the A/B.


Lens 2 -- Standards (Svelte 5 runes, cn(), prettier, ASCII, imports)

PASS.

  • Svelte 5 runes used correctly: $state, $derived, $effect (page +
    toaster + toast.svelte.ts). Module-scope runes live in .svelte.ts per
    convention. $props() in layout untouched-style.
  • $effect cleanup return in toaster.svelte:6-8 is the canonical unmount-leak
    pattern.
  • Intra-$lib imports use the .js extension and index.js barrels, matching
    the repo's existing convention.
  • ASCII house style honored: -- (not em-dash) in copy and comments; straight
    quotes; tabs; single quotes; semicolons. No non-ASCII introduced.
  • No cn() misuse -- conditional class arrays use Svelte's native
    class={[...]} (matches existing site-header.svelte style); inline strings
    elsewhere. Acceptable; plan said "use cn() IF combining conditional classes,
    otherwise inline" -- the array form is idiomatic here.
  • prettier --check passes (pre-verified). eslint FAILS (see Quality / 0002).

Lens 3 -- Quality (parser correctness, theme, leaks, SSR, dead code)

PARSER CORRECTNESS: CLEAN. Hand-traced beyond the floor:

  • (2+3)4 -> Unexpected token (no implicit mult). Correct.
  • - alone / 2+ -> Incomplete expression. Correct.
  • 2**3 -> RHS parseUnary->parsePower->parsePrimary sees * -> Unexpected
    token. Correct.
  • ((1+2)) nested + whitespace -> 3. Correct.
  • Right-assoc + unary-exponent interaction (2^-3 = 0.125) correct.
  • No off-by-one in the sticky-regex cursor advance (i += text.length).
  • Division-by-zero short-circuits BEFORE producing Infinity/NaN, giving the
    specific message; overflow (1e308*10) reaches the calculate-level not-finite
    gate. The two paths are correctly separated.

THEME: No hardcoded hex/rgb anywhere in new files. Toaster + page use only
token utilities (bg-card, text-card-foreground, border-border, text-destructive,
text-muted-foreground, text-foreground, bg-muted). Reads correctly in both
modes by construction.

LEAKS: Toast auto-dismiss timers tracked in a Map keyed by id; schedule()
clears a prior timer for the same id (idempotent re-schedule); clearAll() on
viewport unmount via $effect cleanup. The leak probe (toast-core.test.ts)
asserts clearAll prevents a pending callback from firing and pending() returns
to 0. Solid. The viewport is mounted ONCE globally, so clearAll fires only on
app teardown -- acceptable and matches plan intent.

SSR / PRERENDER: calculate('') is pure and SSR-safe; input starts empty ->
empty-state render. copyToClipboard SSR-guarded (typeof navigator + clipboard +
writeText checks). No window/document/navigator at module scope or in render --
only in event handlers. /calc/+page.ts restates prerender = true. Clean.

DEAD CODE: none material. evaluate.ts inner switch has no default and the
function relies on TS exhaustiveness of the op literal union to return on all
paths -- svelte-check is green, so the exhaustiveness holds; not dead code.

DOCS/FIXES/0002: see disposition. Diagnosis verified reproducible
(npx eslint src/lib/toast.svelte.ts -> Parsing error: Unexpected token interface, exit 1). Root cause (eslint-plugin-svelte **/*.svelte.ts glob
lacks the tseslint sub-parser) is correct. The fix-entry is arguably mis-filed
per "Log fixes only" (it records a DEFERRED gap, Commit: n/a) -- flagged, NOT
removed (removal would destroy a correct, reproducible, high-value diagnosis;
the fixes-format itself anticipates n/a for no-code-change entries).


Act-worthy fixes applied

NONE. No cheap-clearly-correct behavior-preserving code defect was found. The
code is spec-conformant and the gate is green; touching anything would be
churn, not improvement.

Left (judgment calls)

  1. docs/fixes/0002 mis-filing -- KEPT, flagged. Removal is destructive (loses a
    correct reproducible diagnosis), not "clearly-correct cleanup". The real
    follow-up is the one-line eslint.config.js widening, which is DO-NOT-TOUCH
    for this feature work.
  2. Leading bare ) yields Incomplete expression vs the spec aside's
    Unexpected token -- spec internal inconsistency; the implementer correctly
    satisfied the pinned () test instead. No action.
  3. npm run lint (eslint) currently fails on toast.svelte.ts -- pre-existing
    repo config gap, OUTSIDE this feature's acceptance gate (test/check/build/
    format). Surfaced in the PR body as a known follow-up.

@eksno

eksno commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

calc Arm A -- review disposition (PR-ready)

ADVISORY review. Does NOT block ship. tests/check/build verified green
(pre-existing) -- not re-run since no code fix was applied.

Outcome: SHIP. No code changes applied.

The Arm A (baseline) implementation faithfully realizes calc-plan.md. Parser
correctness is clean across the pinned test floor and the additional red-team
edge cases hand-traced during review. No cheap-clearly-correct code fix was
warranted, so the working tree is unchanged and no review commit was made.

Findings -> disposition

# Lens Finding Disposition
1 Quality/infra npm run lint (eslint half) fails on web/src/lib/toast.svelte.ts: Parsing error: Unexpected token interface. Reproduced (npx eslint exit 1). Pre-existing repo eslint-config gap; eslint.config.js **/*.svelte.ts glob lacks the tseslint sub-parser. LEFT. Outside this feature's acceptance gate (test/check/build/format). Fix is a one-line eslint.config.js widening -- DO-NOT-TOUCH here. Surface in PR body as known follow-up.
2 Quality/docs docs/fixes/0002 documents a DEFERRED, un-fixed gap (Commit: n/a); arguably mis-filed per CLAUDE.md "Log fixes only". LEFT (kept-flagged). Removal rejected: the entry is a correct, reproducible, high-value root-cause diagnosis; deleting it destroys debugging knowledge the fixes-log exists to preserve, and the fixes-format itself anticipates n/a for no-code-change entries. Not a "clearly-correct cleanup" -- it is a destructive judgment call. Flagged in PR instead.
3 Spec Leading bare ) yields Incomplete expression, contradicting the plan's informal aside (line 353) that a bare ) is Unexpected token. LEFT. SPEC internal inconsistency: the pinned, tested () case (line 356) REQUIRES rparen-at-primary -> Incomplete expression. Implementer correctly satisfied the pinned test. Not floor-tested; chosen message defensible. Shared by both arms -- A/B-neutral.

docs/fixes/0002 decision (explicitly requested)

KEPT, FLAGGED. Did NOT remove it. Rationale: the task offered removal as an
OPTIONAL cleanup ("if you judge it a cheap clearly-correct cleanup"). I judge it
is NOT -- it is destructive (the entry holds a verified, reproducible diagnosis
of a live lint failure with the exact one-line fix), and the fixes-format
explicitly accommodates Commit: n/a. The mis-filing is borderline, not
clearly-correct. Higher-value disposition: keep the diagnosis, surface the lint
gap + the one-line eslint.config.js follow-up in the PR body.

PR body -- suggested known-issues / follow-ups block

Known follow-up (out of scope for this feature; does not block):
npm run lint (eslint half) fails on src/lib/toast.svelte.ts with
Parsing error: Unexpected token interface. This is a pre-existing repo
eslint-config gap -- eslint-plugin-svelte's **/*.svelte.ts glob is not
wired to the typescript-eslint sub-parser, so the first module-scope-runes
file in the repo trips it. Fix is a one-line widening of
web/eslint.config.js (files: ['**/*.svelte', '**/*.svelte.ts', '**/*.svelte.js']), deferred because that file is DO-NOT-TOUCH for the
/calc work. Diagnosis recorded in docs/fixes/0002. The feature's own gate
(test/check/build/prettier) is green.

Gate after fix

n/a -- no fix applied; pre-existing green gate stands.

@eksno

eksno commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Known issues from QA (George -- baseline arm exploratory pass)

Triaged unblock-with-note (kept visible for the operator; not auto-fixed, so the two experiment arms stay comparable). Note: the parser/evaluator core is CORRECT on all edge cases -- these are UX/rendering issues a code review does not surface:

  1. Result rendered TWICE (worst) -- every valid expression shows a large bold = N live result AND a smaller = N badge directly below it; looks like a rendering glitch. NOTE: this is a SHARED-PLAN flaw -- the plan specified both a live-result element and a result badge -- so it is present in BOTH experiment arms; only this arm's QA surfaced it.
  2. Error rendered twice -- inline red text + a red badge below, for every error state.
  3. Long history rows -- a long expression eats the row width and pushes the Copy button / result off.
  4. Mobile @375px -- the "Compute" button label disappears (icon only); purpose unclear.
  5. Compute button has no keyboard focus ring -- every other control does.
  6. (Confirmed OK) history cap of 10 evicts correctly; 0^0=1 (JS semantics); 9^9^9 correctly errors as non-finite rather than showing Infinity.

These are the baseline arm's QA yield -- intentionally left visible as part of the experiment artifact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant