Repository navigation
Add richer autocompletion for variables - #3
Merged
Merged
Conversation
Reviewer's GuideIntroduces context-aware GLSL autocomplete by scanning declared variable types, offering vector swizzles, matrix indexing skeletons, and constructor snippets, while enabling autocomplete by default with persisted preferences and adding unit/end-to-end coverage; the deployed HTML/assets and analytics snippet are also updated. Sequence diagram for context-aware GLSL member completionsequenceDiagram
participant User
participant Editor as CodeMirror Editor
participant Source as glslCompletionSource
participant Vars as glsl-vars
participant Popup as Completion Popup
User->>Editor: Type `.` or `[` after identifier
Editor->>Editor: insertThenComplete(ch)
Editor->>Source: startCompletion()
Source->>Vars: scanVariables(document)
Vars-->>Source: variable name to type map
Source->>Vars: membersForType(type)
Vars-->>Source: swizzles or matrix index skeleton
Source-->>Popup: Return context-specific completions
Popup-->>User: Show members with context hints
Sequence diagram for persisted autocomplete preferencesequenceDiagram
participant User
participant Main as Application Boot
participant Storage as localStorage
participant Editor
Main->>Storage: getItem(acStoreKey)
Storage-->>Main: Stored preference or null
Main->>Editor: setAutocomplete(checked)
User->>Main: Toggle autocomplete checkbox
Main->>Editor: setAutocomplete(checked)
Main->>Storage: setItem(acStoreKey, 1 or 0)
Flow diagram for GLSL completion context routingflowchart LR
A[Completion request] --> B{Previous character}
B -->|.| C[resolveType]
C --> D[membersForType]
D --> E[Vector swizzles]
B -->|"["| F[resolveType]
F --> G[membersForType]
G --> H[Matrix index skeleton]
B -->|Word| I[GLSL_COMPLETIONS]
I --> J[constructorCompletions]
J --> K[Keywords functions types constructors]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/glsl-vars.ts" line_range="86-94" />
<code_context>
+ * `m[0][0]` indexing skeleton whose row index is left selected. Scalars,
+ * samplers, and arrays have no such accessors.
+ */
+export function membersForType(type: string): Completion[] {
+ if (vectorComponents(type) !== null) {
+ // The user already typed `.`, so each completion applies just the swizzle
+ // char at the cursor (from = after the dot), never a leading `.`.
+ return SWIZZLE.map(({ char, detail }) => ({
+ label: "." + char,
+ type: "keyword",
+ apply: char,
+ detail,
+ }));
+ }
+ if (matrixDimension(type) !== null) {
</code_context>
<issue_to_address>
**issue (bug_risk):** `membersForType` returns every swizzle component for every vector dimension, so `vec2` and `vec3` offer invalid members such as `.z`, `.w`, `.b`, `.a`, `.p`, and `.q`. Accepting those completions inserts GLSL that fails to compile.
**Triggers:** When the declared variable is a `vec2` or `vec3`.
**Suggested fix:** Limit the position, color, and texture-coordinate swizzles to the components supported by the vector dimension.
```suggestion
const components = vectorComponents(type);
if (components !== null) {
// The user already typed `.`, so each completion applies just the swizzle
// char at the cursor (from = after the dot), never a leading `.`.
return SWIZZLE.filter((_, index) => index % 4 < components).map(({ char, detail }) => ({
label: "." + char,
type: "keyword",
apply: char,
detail,
}));
```
</issue_to_address>
### Comment 2
<location path="src/glsl-completions.ts" line_range="43-45" />
<code_context>
+ const ch = pos > 0 ? line[pos - 1] : "";
+
+ // ── Bracket context: `m[` — matrix-index skeleton ─────────────────────
+ if (ch === "[" && pos >= 2 && /\w/.test(line[pos - 2])) {
+ const type = resolveType(ctx, pos - 1);
+ if (type) {
+ const options = membersForType(type);
+ // from covers the typed `[`, so the skeleton replaces it.
+ if (options.length > 0) return { from: ctx.pos - 1, options, validFor: /^\[?\d*$/ };
+ }
+ return null;
</code_context>
<issue_to_address>
**issue (bug_risk):** Bracket completion reuses the vector dot-member options while replacing the typed `[`. Accepting a vector option after `v[` inserts only `x`, `y`, or another swizzle character, producing malformed code such as `vx` instead of a numeric index.
**Triggers:** When a vector variable is followed by `[`.
**Suggested fix:** Provide numeric index completions for vectors or return no vector options in bracket context; do not reuse dot-member completions.
```suggestion
if (/^[ib]?vec\d$/.test(type)) return null;
const options = membersForType(type);
// from covers the typed `[`, so the skeleton replaces it.
if (options.length > 0) return { from: ctx.pos - 1, options, validFor: /^\[?\d*$/ };
```
</issue_to_address>
### Comment 3
<location path="src/glsl-vars.ts" line_range="116" />
<code_context>
+
+/** Constructor completions shown alongside the bare type keyword. */
+export function constructorCompletions(): Completion[] {
+ return CONSTRUCTOR_TYPES.map((type) =>
+ snippetCompletion(type + "($0)", { label: type, detail: "constructor", boost: -100 } as Completion)
+ );
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** The constructor snippets use `$0` instead of CodeMirror's `${0}` placeholder syntax, so accepting a constructor completion inserts the literal `$0` into the document rather than creating a cursor/selection placeholder.
**Triggers:** When a user accepts a constructor completion such as `vec3`.
**Suggested fix:** Use CodeMirror snippet placeholders such as `vec3(${0})` or `vec3(${1})` in the template.
```suggestion
snippetCompletion(type + "(${0})", { label: type, detail: "constructor", boost: -100 } as Completion)
```
</issue_to_address>
### Comment 4
<location path="src/glsl-vars.ts" line_range="64-74" />
<code_context>
+export function scanVariables(src: string): Map<string, string> {
+ const vars = new Map<string, string>();
+ // Optional qualifier span and precision, then <type> <name> [array] [= init];
+ const decl = /^(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*[^;]*?)?\s*;?$/;
+
+ for (const raw of src.split("\n")) {
+ const line = raw.split("//")[0].trim();
+ const m = line.match(decl);
+ if (!m) continue;
+ // Only a known type token counts as a declaration — this rejects
+ // statements like `return value;` from being misread as `type name`.
+ if (!KNOWN_TYPES.has(m[1])) continue;
+ vars.set(m[2], m[1]);
+ }
+ return vars;
</code_context>
<issue_to_address>
**issue (bug_risk):** The declaration regex only accepts one complete declaration per line, so legal declarations with multiple declarators such as `vec3 a, b;`, or declarations sharing a line with braces or other statements, are not recorded and receive no member completion.
**Triggers:** When shader code uses multiple declarators or places a declaration alongside another statement on the same line.
**Suggested fix:** Tokenize declarations independently of line boundaries, or at least support comma-separated declarators and leading/trailing statements.
```suggestion
const decl = /(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+([^;]+);/g;
const source = src.replace(/\/\/[^\n]*/g, "");
let m: RegExpExecArray | null;
while ((m = decl.exec(source))) {
// Only a known type token counts as a declaration — this rejects
// statements like `return value;` from being misread as `type name`.
if (!KNOWN_TYPES.has(m[1])) continue;
for (const declarator of m[2].split(",")) {
const name = declarator.trim().match(/^(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*.*)?$/);
if (name) vars.set(name[1], m[1]);
}
}
```
</issue_to_address>
### Comment 5
<location path="src/glsl-vars.ts" line_range="61-73" />
<code_context>
+ * (redeclaration updates the type); plain reassignment (`q = other;`) is not
+ * tracked — the type is left at the last declaration.
+ */
+export function scanVariables(src: string): Map<string, string> {
+ const vars = new Map<string, string>();
+ // Optional qualifier span and precision, then <type> <name> [array] [= init];
+ const decl = /^(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*[^;]*?)?\s*;?$/;
+
+ for (const raw of src.split("\n")) {
+ const line = raw.split("//")[0].trim();
+ const m = line.match(decl);
+ if (!m) continue;
+ // Only a known type token counts as a declaration — this rejects
+ // statements like `return value;` from being misread as `type name`.
+ if (!KNOWN_TYPES.has(m[1])) continue;
+ vars.set(m[2], m[1]);
+ }
+ return vars;
</code_context>
<issue_to_address>
**issue (bug_risk):** The global name-to-type map ignores lexical scope and resolves the last textual declaration everywhere. A later declaration of the same name in another block or function overwrites the type used for earlier code, so member completion suggests members for the wrong type.
**Triggers:** When the shader redeclares the same variable name in different scopes or functions.
**Suggested fix:** Track declarations by scope and resolve the declaration visible at the completion position.
</issue_to_address>
### Comment 6
<location path="src/main.ts" line_range="153-156" />
<code_context>
+// Single source of truth: checkbox checked ⇔ localStorage. Persist so the user's
+// preference survives reloads. Autocomplete defaults ON for new visitors.
+const acStoreKey = "glsl.autocomplete";
+const storedAc = localStorage.getItem(acStoreKey);
+if (storedAc !== null) {
+ acCheckbox.checked = storedAc === "1";
+}
+editor.setAutocomplete(acCheckbox.checked);
acCheckbox.addEventListener("change", () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** The new autocomplete preference accesses `localStorage` without the try/catch used by the existing persistence module. In private-browsing or storage-blocked contexts, `getItem` or `setItem` throws and the boot/change handler fails instead of preserving editor usability.
**Triggers:** When the browser denies localStorage access, such as some private-browsing or embedded contexts.
**Suggested fix:** Wrap preference reads and writes in best-effort try/catch helpers, matching `loadDoc` and `storeDoc`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 6 findings to address first, and the new Google Analytics script executes third-party code and sends visitor activity to an external service for every user by default. Reverting removes future collection but cannot retract data already transmitted, and the tracking decision takes effect immediately without an application failure signaling a mistaken choice.
Blocking findings: src/glsl-vars.ts:94, src/glsl-completions.ts:45, src/glsl-vars.ts:116, src/glsl-vars.ts:74, src/glsl-vars.ts:73, and 1 more
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+64
to
+74
| const decl = /^(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*[^;]*?)?\s*;?$/; | ||
|
|
||
| for (const raw of src.split("\n")) { | ||
| const line = raw.split("//")[0].trim(); | ||
| const m = line.match(decl); | ||
| if (!m) continue; | ||
| // Only a known type token counts as a declaration — this rejects | ||
| // statements like `return value;` from being misread as `type name`. | ||
| if (!KNOWN_TYPES.has(m[1])) continue; | ||
| vars.set(m[2], m[1]); | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): The declaration regex only accepts one complete declaration per line, so legal declarations with multiple declarators such as vec3 a, b;, or declarations sharing a line with braces or other statements, are not recorded and receive no member completion.
Triggers: When shader code uses multiple declarators or places a declaration alongside another statement on the same line.
Suggested fix: Tokenize declarations independently of line boundaries, or at least support comma-separated declarators and leading/trailing statements.
Suggested change
| const decl = /^(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*[^;]*?)?\s*;?$/; | |
| for (const raw of src.split("\n")) { | |
| const line = raw.split("//")[0].trim(); | |
| const m = line.match(decl); | |
| if (!m) continue; | |
| // Only a known type token counts as a declaration — this rejects | |
| // statements like `return value;` from being misread as `type name`. | |
| if (!KNOWN_TYPES.has(m[1])) continue; | |
| vars.set(m[2], m[1]); | |
| } | |
| const decl = /(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+([^;]+);/g; | |
| const source = src.replace(/\/\/[^\n]*/g, ""); | |
| let m: RegExpExecArray | null; | |
| while ((m = decl.exec(source))) { | |
| // Only a known type token counts as a declaration — this rejects | |
| // statements like `return value;` from being misread as `type name`. | |
| if (!KNOWN_TYPES.has(m[1])) continue; | |
| for (const declarator of m[2].split(",")) { | |
| const name = declarator.trim().match(/^(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*.*)?$/); | |
| if (name) vars.set(name[1], m[1]); | |
| } | |
| } |
Comment on lines
+61
to
+73
| export function scanVariables(src: string): Map<string, string> { | ||
| const vars = new Map<string, string>(); | ||
| // Optional qualifier span and precision, then <type> <name> [array] [= init]; | ||
| const decl = /^(?:(?:const|uniform|in|out|varying|attribute)\s+)?(?:(?:highp|mediump|lowp)\s+)?(\w+)\s+(\w+)(?:\s*\[[^\]]*\])?(?:\s*=\s*[^;]*?)?\s*;?$/; | ||
|
|
||
| for (const raw of src.split("\n")) { | ||
| const line = raw.split("//")[0].trim(); | ||
| const m = line.match(decl); | ||
| if (!m) continue; | ||
| // Only a known type token counts as a declaration — this rejects | ||
| // statements like `return value;` from being misread as `type name`. | ||
| if (!KNOWN_TYPES.has(m[1])) continue; | ||
| vars.set(m[2], m[1]); |
There was a problem hiding this comment.
issue (bug_risk): The global name-to-type map ignores lexical scope and resolves the last textual declaration everywhere. A later declaration of the same name in another block or function overwrites the type used for earlier code, so member completion suggests members for the wrong type.
Triggers: When the shader redeclares the same variable name in different scopes or functions.
Suggested fix: Track declarations by scope and resolve the declaration visible at the completion position.
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
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 by Sourcery
Provide richer GLSL autocomplete with type-aware members and constructors, while adding shader interaction analytics and persistent autocomplete settings.
New Features:
Enhancements:
Tests:
Chores: