diff options
| author | David Czihak <git@dcz.at> | 2026-05-10 19:24:37 +0200 |
|---|---|---|
| committer | David Czihak <git@dcz.at> | 2026-05-10 19:24:37 +0200 |
| commit | 82901bb223b3592b847099d245495decfa0474c3 (patch) | |
| tree | ccfd2a64e3db1f160ee5344f6bcc0d12fdb8696a /ISSUES.md | |
| parent | b80b9c1f82585677a7c042557576c41b1670d259 (diff) | |
Fix: Code review fixes and 0.2.0 release prep
- localizeText: return fallback when localization key is missing instead
of a developer-facing "Localization missing for X" message
- resolveCleanPaths: remove dead `normalized === ""` branch (unreachable
after the preceding absolute-path guard)
- resolveBuildStepAction: validate step before the async resolveZigExecutable
call to avoid a pointless `which zig` subprocess on invalid input
- buildShellCommand: join cd + command with `&&` instead of `;` so a
failed cd aborts instead of running the command in the wrong directory
- resolveWatchAction: cap debounceMs at 60 000; add min/max to schema
- USER_OPTION_REGEX: tighten =.* to =.+ to reject empty option values
- Bump version to 0.2.0
- Finalize CHANGELOG for 0.2.0 — 2026-05-10
- NOTICES/README: update zig-debug → zig-mark asset references
- ISSUES.md: mark A1–A6 fixed, update all file paths to Zig.novaextension/
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Diffstat (limited to 'ISSUES.md')
| -rw-r--r-- | ISSUES.md | 108 |
1 files changed, 64 insertions, 44 deletions
@@ -18,19 +18,19 @@ A living catalog of identified issues in this extension. New findings should be ### FIXED — `launchInTerminal` could hang on synchronous start failure -- **Location**: [Scripts/main.js](Scripts/main.js), `launchInTerminal` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `launchInTerminal` - **Description**: `process.start()` was called outside any try/catch. A synchronous throw left the returned `Promise` unresolved, so the `runInTerminal` command would hang forever. - **Resolution**: Wrapped in try/catch; on throw, resolves with `{status: -1, stderr: String(error)}` so the existing caller's `result.status !== 0` branch surfaces the failure. ### FIXED — Fire-and-forget `pushConfiguration` swallowed rejections -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` and `ZigLanguageServer.observeConfig` (three call sites) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` and `ZigLanguageServer.observeConfig` (three call sites) - **Description**: All three sites used `void this.pushConfiguration()`. Any rejection became an unhandled promise rejection with no log trace. - **Resolution**: Replaced with `.catch((error) => console.error(...))`. ### FIXED — `runProcess` had no rejection path for stuck child processes -- **Location**: [Scripts/main.js](Scripts/main.js), `runProcess` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `runProcess` - **Description**: The promise resolved only on `onDidExit`. A child process that started but never exited (e.g. a misbehaving `build.zig`) would stall any caller indefinitely. `provideTasks` was the most exposed. - **Resolution**: Added optional `timeoutMs` option. On timeout, sends `SIGTERM` (guarded for missing `process.signal`) and resolves with `{status: -1, stderr: "...[timeout after Nms]"}`. Applied 60s timeout to the `zig build --list-steps` call in `stepCache.fetch`. @@ -42,13 +42,13 @@ A living catalog of identified issues in this extension. New findings should be #### FIXED — Dispose loops have no per-item try/catch -- **Location**: [Scripts/main.js](Scripts/main.js), `deactivate` and `ZigLanguageServer.dispose` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `deactivate` and `ZigLanguageServer.dispose` - **Description**: Both iterate disposables and call `.dispose()` without protection. One throwing disposable aborted the rest of the loop, leaking later registrations and listeners. - **Resolution**: Wrapped each `disposable.dispose()` call in a try/catch that logs the error and continues iterating. #### FIXED — ZLS crashed immediately on startup when zig was discovered automatically -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` (~L980) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` (~L980) - **Description**: `syncWorkspaceZlsConfiguration` was called immediately after `client.start()`. Writing `zls.zig_exe_path` to `nova.workspace.config` while the LanguageClient is newly registered triggers Nova to fire workspace config change events. @@ -64,7 +64,7 @@ A living catalog of identified issues in this extension. New findings should be #### FIXED — `findOnPath` always returned null when no explicit path was configured -- **Location**: [Scripts/main.js](Scripts/main.js), `findOnPath` (~L597) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `findOnPath` (~L597) - **Description**: Nova's `Process` constructor does not inherit the parent's environment automatically. The bare `new Process("/usr/bin/env", { args: ["which", name] })` call received a stripped environment with no `PATH`, so `which` could not locate any binary @@ -74,19 +74,19 @@ A living catalog of identified issues in this extension. New findings should be #### OPEN — `pushConfiguration` re-resolves Zig path on every observation -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.pushConfiguration` (~L1040) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.pushConfiguration` (~L1040) - **Description**: On rapid config-change storms, each observation calls `resolveSettings → resolveExecutable → findOnPath`, fanning out N parallel `which zig` subprocesses. The generation guard discards stale results but the work still runs. - **Suggested fix**: Debounce the call (small `setTimeout` consolidator) or cache the resolved zig path for the lifetime of a generation. #### OPEN — `start()` cannot distinguish handshake failure from runtime crash -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` (~L1000) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` (~L1000) - **Description**: The try/catch around `client.start()` only fires when `start()` itself throws synchronously. If the language client starts successfully but the LSP fails handshake, the user only sees the generic "stopped unexpectedly" warning (via `onDidStop`), never the more informative "Unable to start" message. - **Suggested fix**: Listen for an early failure on `onDidStop` within a brief window after start and emit the more informative warning. #### OPEN — `stop()` removes from subscriptions before `client.stop()` settles -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.stop` (~L1058) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.stop` (~L1058) - **Description**: If `client.stop()` is async in Nova's API, removal happens before the stop completes. Not observed to cause issues in practice; flagging for verification. - **Action**: Confirm whether Nova's `LanguageClient.stop()` is sync or returns a Promise; document the assumption either way. @@ -94,31 +94,31 @@ A living catalog of identified issues in this extension. New findings should be #### FIXED — `parseProjectName` matched commented-out `.name` lines -- **Location**: [Scripts/main.js](Scripts/main.js), `parseProjectName` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `parseProjectName` - **Description**: The regex did not strip line comments. A `build.zig.zon` with `// .name = "evil"` above the real name field would match the comment first. - **Resolution**: Strip `//`-to-EOL comments via `content.replace(/\/\/[^\n]*/g, "")` before matching. #### FIXED — Step discovery failures were invisible -- **Location**: [Scripts/main.js](Scripts/main.js), `stepCache.fetch` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `stepCache.fetch` - **Description**: When `--list-steps` failed (non-zero exit, timeout), the user saw a sparse task list with no signal that discovery was broken. - **Resolution**: Added a per-cwd `lastWarnedAt` map that emits a `console.warn` with the exit status and a 500-char stderr excerpt at most once per 5 minutes. `invalidate()` clears the throttle so manual cache reset re-enables warnings. #### OPEN — `resolveCleanAction` skips uninstall on cold cache -- **Location**: [Scripts/main.js](Scripts/main.js), `resolveCleanAction` (~L1202) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveCleanAction` (~L1202) - **Description**: The `zig build uninstall` step only runs when `stepCache.entries` already contains the cwd and includes "uninstall". A first-time clean (cold cache) silently skips uninstall and only removes the cache directories. -- **Suggested fix**: Either accept the asymmetry and document it in [MANUAL.md](MANUAL.md), or warm the cache before deciding. +- **Suggested fix**: Either accept the asymmetry and document it in [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md), or warm the cache before deciding. #### DOCUMENTED — `getConfigValue` treats empty string as missing -- **Location**: [Scripts/main.js](Scripts/main.js), `getConfigValue` (~L62) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `getConfigValue` (~L62) - **Description**: A user cannot explicitly *unset* a global value by setting an empty workspace override; they must use Nova's "remove from workspace" UI. Acceptable but worth a note for power users. -- **Action**: Document in [MANUAL.md](MANUAL.md) settings reference. +- **Action**: Document in [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) settings reference. #### OPEN — Step name regex rejects names starting with a digit -- **Location**: [Scripts/main.js](Scripts/main.js), `stepCache.fetch` filter (~L457) and `resolveBuildStepAction` (~L1290) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `stepCache.fetch` filter (~L457) and `resolveBuildStepAction` (~L1290) - **Description**: `/^[A-Za-z_][\w-]*$/` is stricter than Zig itself, which accepts step names beginning with a digit. - **Action**: Either relax to allow leading digits, or document as an extension-imposed constraint. @@ -126,7 +126,7 @@ A living catalog of identified issues in this extension. New findings should be #### FIXED — `resolveCleanAction` built its shell string by hand -- **Location**: [Scripts/main.js](Scripts/main.js), `resolveCleanAction` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveCleanAction` - **Description**: The uninstall+rm command was assembled with manual `quoteShellArgument` calls. Safe today but fragile: a future contributor adding a new arg could forget to quote. - **Resolution**: Compose the command from two `buildShellCommand` calls so all quoting routes through one helper. The intentional `;` (rather than `&&`) between the two is preserved and now commented — rm should run even if uninstall fails. @@ -134,27 +134,27 @@ A living catalog of identified issues in this extension. New findings should be #### OPEN — Class methods lack JSDoc -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer` (~L860) and `ZigTaskAssistant` (~L1080) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer` (~L860) and `ZigTaskAssistant` (~L1080) - **Description**: ~50% of utility helpers have JSDoc; 0% of class methods do. The prior review plan tracks this work; see [~/.claude/plans/i-want-to-perform-idiopotent-crystal.md](file:///Users/david/.claude/plans/i-want-to-perform-idiopotent-crystal.md) for the backlog. - **Action**: Resume Groups 1, 5, 5a, 6, 7 from the JSDoc plan. #### DOCUMENTED — Auto-discovered tasks cannot open the report automatically -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigTaskAssistant.provideTasks` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigTaskAssistant.provideTasks` - **Description**: Tasks returned from `provideTasks()` do not support the `openLogOnRun` setting. The property exists on task JSON files for user-created tasks but is not honoured on `Task` objects constructed in JS — setting `task.openLogOnRun` has no effect. Nova's extension API simply does not expose this for programmatic tasks. -- **Action**: Documented in [README.md](README.md) Known Issues. +- **Action**: Documented in [Zig.novaextension/README.md](Zig.novaextension/README.md) Known Issues. #### OPEN — Issue matcher pattern not documented -- **Location**: [extension.json](extension.json) issue matcher block, [README.md](README.md), [MANUAL.md](MANUAL.md) +- **Location**: [Zig.novaextension/extension.json](Zig.novaextension/extension.json) issue matcher block, [Zig.novaextension/README.md](Zig.novaextension/README.md), [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) - **Description**: Users authoring custom tasks have no reference for what fields the `zig.compiler` matcher captures. -- **Suggested fix**: Add a short "Issue matcher" section to [MANUAL.md](MANUAL.md) documenting the regex, captures, and how to use it from custom tasks. +- **Suggested fix**: Add a short "Issue matcher" section to [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) documenting the regex, captures, and how to use it from custom tasks. ### Structure #### OPEN — `Scripts/main.js` is a 1,398-line monolith -- **Location**: [Scripts/main.js](Scripts/main.js) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js) - **Description**: Section dividers help, but two ~300-line classes plus utilities in one file impede navigation and review. - **Suggested split**: - `main.js` — activate/deactivate/registerCommands @@ -174,39 +174,59 @@ A living catalog of identified issues in this extension. New findings should be #### OPEN — Dead `warnedMissing.delete("zig")` -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` (~L1010) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` (~L1010) - **Description**: `warnMissingTool` is only ever called with `"zls"`; the matching `delete("zig")` line never has anything to remove. - **Action**: Either delete the line, or wire up a missing-zig warning path through `warnMissingTool`. -#### OPEN — `buildShellCommand` uses `;` between cd and command +#### FIXED — `buildShellCommand` uses `;` between cd and command -- **Location**: [Scripts/main.js](Scripts/main.js), `buildShellCommand` (~L735) + + +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `buildShellCommand` - **Description**: If `cd` fails (e.g. cwd was deleted between resolution and execution), the command runs in the wrong directory silently. -- **Suggested fix**: Use `&&` so a failed `cd` aborts. +- **Resolution**: Changed `segments.join("; ")` to `segments.join(" && ")` so a failed `cd` aborts the command. ### Logic -#### OPEN — `USER_OPTION_REGEX` permits trailing `=` with empty value +#### FIXED — `localizeText` ignored its `fallback` parameter + +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `localizeText` (~L542) +- **Description**: When a localization key was missing, the function returned `"Localization missing for ${key}"` instead of the caller-supplied English fallback. All callers pass real English strings as fallbacks. +- **Resolution**: Changed the missing-key branch to `return fallback !== undefined ? String(fallback) : key`. + +#### FIXED — Dead `normalized === ""` branch in `resolveCleanPaths` + +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveCleanPaths` (~L307) +- **Description**: `nova.path.normalize(cwd)` can never return `""` when `cwd` starts with `"/"` (already guaranteed by the preceding guard), making the empty-string check dead code. +- **Resolution**: Removed `|| normalized === ""` from the condition. + +#### FIXED — Step validated after `await` in `resolveBuildStepAction` + +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveBuildStepAction` (~L1420) +- **Description**: `resolveZigExecutable()` (async, runs `which zig`) was called before validating the `step` argument, wasting a subprocess on invalid input. +- **Resolution**: Moved the step null/regex guard to the top of the function, before the `await`. + +#### FIXED — `USER_OPTION_REGEX` permitted trailing `=` with empty value -- **Location**: [Scripts/main.js](Scripts/main.js), `USER_OPTION_REGEX` (~L9) -- **Description**: `foo=` is accepted and produces `-Dfoo=`. Zig's behavior on empty values is option-dependent. -- **Action**: Tighten to require at least one character after `=`, or document the current behavior. +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `USER_OPTION_REGEX` (~L9) +- **Description**: `foo=` was accepted and produced `-Dfoo=`. Zig errors on empty option values. +- **Resolution**: Changed `=.*` to `=.+` so at least one character is required after `=`. #### OPEN — Build step name has no length cap -- **Location**: [Scripts/main.js](Scripts/main.js), `resolveBuildStepAction` (~L1290) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveBuildStepAction` (~L1290) - **Description**: A 1MB step name would pass the regex check. - **Suggested fix**: Add a length anchor like `{1,128}`. -#### OPEN — `debounceMs` has no upper bound +#### FIXED — `debounceMs` had no upper bound -- **Location**: [Scripts/main.js](Scripts/main.js), `resolveWatchAction` (~L1325) and [extension.json](extension.json) `debounceMs` schema -- **Description**: `Number.isFinite` and `>= 0` are checked, but `1e18` would be accepted. The manifest schema also lacks `min`/`max`. -- **Suggested fix**: Cap at 60_000 in code; add `"min": 0, "max": 60000` in `extension.json`. +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveWatchAction`; [Zig.novaextension/extension.json](Zig.novaextension/extension.json) `debounceMs` schema +- **Description**: `Number.isFinite` and `>= 0` were checked, but `1e18` would be accepted. +- **Resolution**: Added `&& n <= 60000` guard in code; added `"min": 0, "max": 60000` to the manifest schema. #### OPEN — `runProcess` joins stdout/stderr lines without separator -- **Location**: [Scripts/main.js](Scripts/main.js), `runProcess` (~L511) +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `runProcess` (~L511) - **Description**: `stdout.push(line)` then `stdout.join("")`. If Nova's `onStdout` strips trailing newlines, two lines `"foo"` and `"bar"` concatenate to `"foobar"`. - **Action**: Verify Nova's behavior; if newlines are stripped, join with `"\n"`. @@ -214,13 +234,13 @@ A living catalog of identified issues in this extension. New findings should be #### OPEN — `build-parser.sh` lacks explicit `xcrun` failure check -- **Location**: [Scripts/build-parser.sh](Scripts/build-parser.sh) `SDKROOT` line +- **Location**: [Zig.novaextension/Scripts/build-parser.sh](Zig.novaextension/Scripts/build-parser.sh) `SDKROOT` line - **Description**: An empty `SDKROOT` from a failed `xcrun` produces cryptic downstream errors. - **Suggested fix**: Check `xcrun --show-sdk-path` exit status explicitly and abort with a clear message. #### OPEN — Hardening tip for untrusted workspaces missing from MANUAL -- **Location**: [MANUAL.md](MANUAL.md) +- **Location**: [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) - **Description**: Login-shell PATH discovery means a workspace (or a shell profile that reads workspace-local `.envrc` / `direnv` / `.env` files) that injects `.` early in PATH could run a malicious local `zig`. The same surface area exists for `createAction` (tasks). Already @@ -231,26 +251,26 @@ A living catalog of identified issues in this extension. New findings should be #### OPEN — README troubleshooting omits common tool-not-found cases -- **Location**: [README.md](README.md) Troubleshooting +- **Location**: [Zig.novaextension/README.md](Zig.novaextension/README.md) Troubleshooting - **Description**: Mentions parser issues but not "ZLS not found" or "lldb-dap not found" recovery paths, which are the most common user-reported pain points. - **Suggested fix**: Add brief recovery steps for both. #### OPEN — CHANGELOG entries don't link to MANUAL sections -- **Location**: [CHANGELOG.md](CHANGELOG.md) -- **Description**: Polish: feature entries could deep-link to relevant [MANUAL.md](MANUAL.md) sections so users can discover usage. +- **Location**: [Zig.novaextension/CHANGELOG.md](Zig.novaextension/CHANGELOG.md) +- **Description**: Polish: feature entries could deep-link to relevant [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) sections so users can discover usage. - **Action**: Optional. ### Structure #### OPEN — Dual escape paths for shell strings -- **Location**: [Scripts/main.js](Scripts/main.js), `buildShellCommand` and `buildIssueNormalizedCommand` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `buildShellCommand` and `buildIssueNormalizedCommand` - **Description**: Both functions quote arguments but live separately. Future changes risk drift. - **Suggested fix**: Compose `buildIssueNormalizedCommand` on top of `buildShellCommand`. #### OPEN — `createAction` defined mid-class -- **Location**: [Scripts/main.js](Scripts/main.js), `ZigTaskAssistant.createAction` +- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigTaskAssistant.createAction` - **Description**: Central helper used by every `resolve*Action`; defined between two of its callers. Discoverability nit. - **Action**: Move to top of class above `resolveTaskAction`. |
