Skip to content

Surface parser errors between input and canvas#6

Merged
psaegert merged 2 commits into
masterfrom
copilot/fix-expression-error-display
Apr 21, 2026
Merged

Surface parser errors between input and canvas#6
psaegert merged 2 commits into
masterfrom
copilot/fix-expression-error-display

Conversation

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown

The reported expression (a * b) - c + z / x in fact parses and renders correctly on main (verified in Node and headless Chrome); the warning icon is not triggered. The real issue is that infixToPostfix silently return nulls from ~10 failure sites, so the UI can only show a generic ⚠️ with no indication of what's wrong.

Changes

  • infixToPostfix.js — replace every return null failure path in tokenize / infixToPostfix with throw new Error(...) carrying a specific message (invalid character, unknown identifier, missing/mismatched ), stray ,, empty expression, etc.). Added a pre-pass paren-depth check so unbalanced parens produce a targeted message instead of a generic "malformed" one. Removed the now-dead isValidExpression / isValidTokens helpers.
  • canvas.jsrenderExpression catches errors from infixToPostfix and constructTree and writes error.message into a new element; clearError / showError helpers keep warning-icon and error-text state in sync. clear-tree and empty-input paths clear both.
  • index.html — new <div id="error-message" role="alert" aria-live="polite" hidden> inserted inside .controls, directly below .input-wrap, so it sits between the input and the canvas.
  • app.css — red outlined pill styling for .error-message, with a dark-theme variant.

Example messages

Input Message
(a+b Missing closing ')'
a+b) Mismatched ')' with no matching '('
foo(a) Unknown identifier 'foo' (use '#foo' for a custom function)
a + $ Invalid character '$' in expression
#(a) Expected an identifier after '#'
(empty) cleared, no error shown

@psaegert psaegert marked this pull request as ready for review April 21, 2026 13:02
Copilot AI review requested due to automatic review settings April 21, 2026 13:02
@psaegert psaegert merged commit 92a6631 into master Apr 21, 2026
2 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the UX around invalid expressions by surfacing specific parser errors in the UI (instead of silently failing and only showing a generic warning icon), bridging the gap between parsing logic and canvas rendering.

Changes:

  • Replace null failure returns in tokenize / infixToPostfix with targeted throw new Error(...) messages.
  • Catch parser/tree-construction errors in the canvas renderer and display the error text alongside the warning icon.
  • Add and style an #error-message element in the controls UI with live-region semantics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
infixToPostfix.js Converts silent parse failures into explicit exceptions with specific messages; adds token validation.
canvas.js Catches parse/build errors and keeps warning + error-message UI state in sync.
index.html Adds the error message container near the expression input with ARIA attributes.
app.css Styles the new error message element (including dark theme adjustments).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread infixToPostfix.js
Comment on lines +207 to +213
if (!ALLOWED_CHARACTERS_REGEX.test(expression)) {
var badMatch = expression.match(/[^A-Za-z0-9_#,+\-*/()\s]/)
throw new Error("Invalid character '" + (badMatch ? badMatch[0] : '?') + "' in expression")
}
var tokens = tokenize(expression)
if (tokens.length === 0) {
throw new Error('Empty expression')

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALLOWED_CHARACTERS_REGEX allows all whitespace via \s, but tokenize() only skips literal spaces (current === ' '). As a result, inputs containing tabs/newlines will pass the regex check and then fail later with an “Invalid character” error. Consider either skipping all whitespace in tokenize() (e.g., \s) or tightening the allowed-character regex to match what the tokenizer actually accepts.

Copilot uses AI. Check for mistakes.
Comment thread infixToPostfix.js
Comment on lines 237 to 250
} else if ("," === token) {
var commaTop = op_stack.pop()
while (commaTop !== '(') {
if (typeof commaTop === 'undefined') {
return null
throw new Error("Unexpected ',' outside of a function call")
}
postfixList.push(commaTop)
commaTop = op_stack.pop()
}
op_stack.push('(')
if (customCallStack.length === 0) {
return null
throw new Error("Unexpected ',' outside of a function call")
}
customCallStack[customCallStack.length - 1].commaCount++

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding validateTokens(tokens) (which rejects any expression that can’t be fully parsed by parseAddSubtract(..., stopOnComma=false)), a comma outside a custom function call will already throw before the shunting-yard loop runs. That makes these Unexpected ',' outside of a function call branches effectively unreachable and can prevent the intended comma-specific messaging from surfacing. Either move comma validation into validateTokens with a targeted message, or remove the dead/unreachable checks here.

Copilot uses AI. Check for mistakes.
Comment thread infixToPostfix.js
if (tokens === null || tokens.length === 0) {
return false;
if (parsed.nextIndex !== tokens.length) {
throw new Error('Unexpected token ' + describeToken(tokens[parsed.nextIndex]) + ' at position ' + parsed.nextIndex)

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message ... at position <n> is reporting a token index (not a character index). That can be confusing for users trying to locate the issue in the original input string. Consider renaming this to token <n> / token index <n>, or (if feasible) tracking the character offset during tokenization so the reported position maps to the original expression.

Suggested change
throw new Error('Unexpected token ' + describeToken(tokens[parsed.nextIndex]) + ' at position ' + parsed.nextIndex)
throw new Error('Unexpected token ' + describeToken(tokens[parsed.nextIndex]) + ' at token index ' + parsed.nextIndex)

Copilot uses AI. Check for mistakes.
Comment thread index.html
</button>
</div>

<div class="error-message" id="error-message" role="alert" aria-live="polite" hidden></div>

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

role="alert" has an implicit aria-live="assertive" in ARIA; setting aria-live="polite" alongside it is conflicting. If you want polite announcements, prefer role="status" (or drop role="alert" and keep aria-live="polite").

Suggested change
<div class="error-message" id="error-message" role="alert" aria-live="polite" hidden></div>
<div class="error-message" id="error-message" role="alert" hidden></div>

Copilot uses AI. Check for mistakes.
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.

3 participants