Skip to content

feat: add scene insight model for editor#617

Open
luochen211 wants to merge 2 commits into
OpenWebGAL:mainfrom
luochen211:editor-scene-insight-model
Open

feat: add scene insight model for editor#617
luochen211 wants to merge 2 commits into
OpenWebGAL:mainfrom
luochen211:editor-scene-insight-model

Conversation

@luochen211

Copy link
Copy Markdown

Summary\n- add a scene insight model for the WebGAL editor\n- extract labels, choices, scene references, assets, variables, command counts, and diagnostics from scene scripts\n- emit editor:update-scene-insight when scene text loads or changes so future sidebars/tooling can consume the outline\n\n## Review notes\n- Feature-focused editor code for WebGAL/GalGame script tooling\n- Diff size: 902 insertions\n- Production code: 902 insertions; no test-only line inflation\n\n## Testing\n- cd packages/origine2 && yarn build

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new scene analysis utility (sceneInsight.ts) that parses scene text to generate diagnostics, track assets, and monitor variables, and integrates it into the TextEditor component to emit scene insights. The review feedback focuses on enhancing the robustness of the parser, such as correctly handling string quotes when splitting comments and colons, avoiding false-positive unreachable code diagnostics when conditional arguments are present, validating variable names, safely parsing whitespace-only arguments, and localizing stateful regular expressions to prevent concurrency issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +680 to +700
function splitInlineComment(rawLine: string): { statement: string; inlineComment: string } {
let escaped = false;
for (let i = 0; i < rawLine.length; i++) {
const char = rawLine[i];
if (escaped) {
escaped = false;
continue;
}
if (char === '\\') {
escaped = true;
continue;
}
if (char === '/' && rawLine[i + 1] === '/') {
return {
statement: rawLine.slice(0, i),
inlineComment: rawLine.slice(i).trim(),
};
}
}
return { statement: rawLine, inlineComment: '' };
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The splitInlineComment function currently splits the line at any // sequence. However, if the line contains a URL (e.g., http:// or https://) or a string literal with //, it will be incorrectly split, truncating the statement and producing a broken AST/insight. To fix this, the function should track whether it is currently inside a string quote (" or ') and only split when outside of quotes.

function splitInlineComment(rawLine: string): { statement: string; inlineComment: string } {
  let escaped = false;
  let inQuote: string | null = null;
  for (let i = 0; i < rawLine.length; i++) {
    const char = rawLine[i];
    if (escaped) {
      escaped = false;
      continue;
    }
    if (char === '\\') {
      escaped = true;
      continue;
    }
    if ((char === '"' || char === "'") && !inQuote) {
      inQuote = char;
      continue;
    }
    if (char === inQuote) {
      inQuote = null;
      continue;
    }
    if (!inQuote && char === '/' && rawLine[i + 1] === '/') {
      return {
        statement: rawLine.slice(0, i),
        inlineComment: rawLine.slice(i).trim(),
      };
    }
  }
  return { statement: rawLine, inlineComment: '' };
}

Comment on lines +730 to +745
function findUnescaped(input: string, target: string): number {
let escaped = false;
for (let i = 0; i < input.length; i++) {
const char = input[i];
if (escaped) {
escaped = false;
continue;
}
if (char === '\\') {
escaped = true;
continue;
}
if (char === target) return i;
}
return -1;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The findUnescaped function searches for the command separator : but does not respect string quotes. If a line contains a colon inside a quoted string (e.g., say: "hello: world" with no other colons, or in similar contexts), it could be incorrectly split. Updating the function to ignore colons inside string quotes ensures robust parsing.

function findUnescaped(input: string, target: string): number {
  let escaped = false;
  let inQuote: string | null = null;
  for (let i = 0; i < input.length; i++) {
    const char = input[i];
    if (escaped) {
      escaped = false;
      continue;
    }
    if (char === '\\') {
      escaped = true;
      continue;
    }
    if ((char === '"' || char === "'") && !inQuote) {
      inQuote = char;
      continue;
    }
    if (char === inQuote) {
      inQuote = null;
      continue;
    }
    if (!inQuote && char === target) return i;
  }
  return -1;
}

Comment on lines +588 to +595
function validateFlow(context: SceneInsightBuildContext) {
context.lines.forEach((line, index) => {
if (!TERMINAL_COMMANDS.has(line.command)) return;
const next = findNextExecutableLine(context.lines, index + 1);
if (!next || next.command === 'label') return;
pushDiagnostic(context, 'info', 'unreachable-line', 'This line follows a terminal command before the next label.', next);
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The validateFlow function flags any executable line following a terminal command (like changeScene) as unreachable. However, if the terminal command has a conditional argument (like -when), execution can continue if the condition is false. Checking for the presence of a when condition prevents false positive unreachable-line diagnostics.

function validateFlow(context: SceneInsightBuildContext) {
  context.lines.forEach((line, index) => {
    if (!TERMINAL_COMMANDS.has(line.command)) return;
    const hasWhenCondition = line.args.some((arg) => arg.key === 'when');
    if (hasWhenCondition) return;
    const next = findNextExecutableLine(context.lines, index + 1);
    if (!next || next.command === 'label') return;
    pushDiagnostic(context, 'info', 'unreachable-line', 'This line follows a terminal command before the next label.', next);
  });
}

Comment on lines +431 to +444
function collectWriteVariable(context: SceneInsightBuildContext, line: ParsedSceneLine) {
const expression = line.content.trim();
const equalIndex = expression.indexOf('=');
if (equalIndex < 0) {
pushDiagnostic(context, 'warning', 'invalid-set-var', 'setVar expression does not contain "=".', line);
return;
}

const name = expression.slice(0, equalIndex).trim();
if (!name) {
pushDiagnostic(context, 'warning', 'empty-var-name', 'setVar expression has an empty variable name.', line);
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The collectWriteVariable function extracts the variable name but does not validate whether it is a valid identifier. Adding a regex check ensures that invalid variable names (e.g., starting with numbers or containing invalid characters) are flagged with a diagnostic.

function collectWriteVariable(context: SceneInsightBuildContext, line: ParsedSceneLine) {
  const expression = line.content.trim();
  const equalIndex = expression.indexOf('=');
  if (equalIndex < 0) {
    pushDiagnostic(context, 'warning', 'invalid-set-var', 'setVar expression does not contain "=".', line);
    return;
  }

  const name = expression.slice(0, equalIndex).trim();
  if (!name) {
    pushDiagnostic(context, 'warning', 'empty-var-name', 'setVar expression has an empty variable name.', line);
    return;
  }

  const isValidName = /^[A-Za-z_\u4e00-\u9fa5][\w\u4e00-\u9fa5]*$/.test(name);
  if (!isValidName) {
    pushDiagnostic(context, 'warning', 'invalid-var-name', `Variable name "${name}" is invalid.`, line);
    return;
  }

Comment on lines +673 to +678
function parseArgValue(value: string): string | boolean | number {
if (value === 'true') return true;
if (value === 'false') return false;
if (value !== '' && !Number.isNaN(Number(value))) return Number(value);
return stripWrappingQuotes(value);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In parseArgValue, checking value !== '' is insufficient because whitespace-only strings (like " ") will pass this check and then Number(" ") will evaluate to 0, causing them to be incorrectly parsed as the number 0. Using value.trim() !== '' prevents this bug.

Suggested change
function parseArgValue(value: string): string | boolean | number {
if (value === 'true') return true;
if (value === 'false') return false;
if (value !== '' && !Number.isNaN(Number(value))) return Number(value);
return stripWrappingQuotes(value);
}
function parseArgValue(value: string): string | boolean | number {
if (value === 'true') return true;
if (value === 'false') return false;
if (value.trim() !== '' && !Number.isNaN(Number(value))) return Number(value);
return stripWrappingQuotes(value);
}

Comment on lines +758 to +769
function extractVariableNames(expression: string): string[] {
const names = new Set<string>();
const cleanExpression = expression.replace(/(["'`])(?:\\.|(?!\1).)*\1/g, ' ');
let match: RegExpExecArray | null;
while ((match = VARIABLE_RE.exec(cleanExpression)) !== null) {
const token = match[0];
if (!RESERVED_WORDS.has(token.toLowerCase())) {
names.add(token);
}
}
return Array.from(names);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The global VARIABLE_RE regular expression is defined with the /g flag. In JavaScript/TypeScript, global regular expressions are stateful and maintain lastIndex between executions. If used concurrently or if a loop exits early, this can lead to subtle bugs. Defining the regex locally inside extractVariableNames avoids any shared state issues.

Suggested change
function extractVariableNames(expression: string): string[] {
const names = new Set<string>();
const cleanExpression = expression.replace(/(["'`])(?:\\.|(?!\1).)*\1/g, ' ');
let match: RegExpExecArray | null;
while ((match = VARIABLE_RE.exec(cleanExpression)) !== null) {
const token = match[0];
if (!RESERVED_WORDS.has(token.toLowerCase())) {
names.add(token);
}
}
return Array.from(names);
}
function extractVariableNames(expression: string): string[] {
const names = new Set<string>();
const cleanExpression = expression.replace(/(["'])(?:\\.|(?!\1).)*\1/g, ' ');
const variableRe = /[A-Za-z_\u4e00-\u9fa5][\w\u4e00-\u9fa5]*/g;
let match: RegExpExecArray | null;
while ((match = variableRe.exec(cleanExpression)) !== null) {
const token = match[0];
if (!RESERVED_WORDS.has(token.toLowerCase())) {
names.add(token);
}
}
return Array.from(names);
}

Comment on lines +220 to +221
export function parseSceneLines(sceneText: string): ParsedSceneLine[] {
return sceneText.split(/\r?\n/).map((rawLine, index) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To ensure robust defensive programming, parseSceneLines should verify that sceneText is a valid string before attempting to call .split() on it, preventing potential runtime crashes if an unexpected type is passed.

Suggested change
export function parseSceneLines(sceneText: string): ParsedSceneLine[] {
return sceneText.split(/\r?\n/).map((rawLine, index) => {
export function parseSceneLines(sceneText: string): ParsedSceneLine[] {
if (typeof sceneText !== 'string') {
return [];
}
return sceneText.split(/\r?\n/).map((rawLine, index) => {

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