|
| 1 | +# AnrokX Open PR Review — 2026-03-06 |
| 2 | + |
| 3 | +## Open PRs |
| 4 | + |
| 5 | +| PR | Title | Size | Branch | |
| 6 | +|----|-------|------|--------| |
| 7 | +| **#796** | fix: clean windows onboarding changes against main | +6382/-2785, 12 files | `fix/windows-onboarding-clean-main` | |
| 8 | +| **#791** | fix(windows): prevent updater plugin startup panic | +5/-0, 1 file | `fix/windows-updater-plugin-config-startup` | |
| 9 | +| **#790** | test: fix windows CI recovery cleanup tests | +10/-10, 2 files | `fix/windows-build-verification` | |
| 10 | +| **#788** | feat: add serverOnlyFileWatching user setting | +115/-1, 9 files | `feature/server-only-file-watching` | |
| 11 | + |
| 12 | +## Recently Merged (last 48hrs) |
| 13 | + |
| 14 | +| PR | Title | Merged | Size | |
| 15 | +|----|-------|--------|------| |
| 16 | +| #794 | feat: polish windows onboarding UX and setup detection | Mar 4 | +3245/-476, 9 files | |
| 17 | +| #793 | feat(onboarding): guide dependency setup step-by-step | Mar 3 | +303/-62, 4 files | |
| 18 | + |
| 19 | +--- |
| 20 | + |
| 21 | +## PR #796 — Windows Onboarding Wizard (main review) |
| 22 | + |
| 23 | +Full first-run dependency wizard for Windows users. Checks for Git, Node, npm, GitHub CLI, Claude Code, and Codex CLI, then guides install via winget/PowerShell. |
| 24 | + |
| 25 | +### Medium Issues |
| 26 | + |
| 27 | +1. **Dead code: `guidance` variable** (client/app.js) — ~60 lines computing a variable that is never rendered in the template. Remove or wire it up. |
| 28 | + |
| 29 | +2. **Memory leak in `setupActionRuns`** (server/setupActionService.js) — `setupActionRuns` and `latestRunByActionId` Maps grow unboundedly with no cleanup/TTL. Add pruning. |
| 30 | + |
| 31 | +3. **Empty updater `pubkey`** (src-tauri/tauri.conf.json) — `pubkey: ""` prevents the startup panic but could silently accept unsigned updates. Set a real key or remove updater config entirely. |
| 32 | + |
| 33 | +4. **Duplicated Windows tool path logic** — Both `diagnosticsService.js` and `setupActionService.js` independently resolve Windows tool paths (~50 lines each). Extract a shared module to prevent divergence. |
| 34 | + |
| 35 | +5. **`autoCreate: true` behavioral change** (server/workspaceManager.js) — Changed from `false` to `true`. Existing users will see worktrees auto-created on new workspaces. Should be documented in PR description. |
| 36 | + |
| 37 | +6. **`stdout`/`stderr` nulled for ALL platforms** (src-tauri/src/main.rs) — Backend crash output is lost on all platforms, not just Windows. Consider routing to a log file. |
| 38 | + |
| 39 | +### Low Issues |
| 40 | + |
| 41 | +7. **Unescaped `statusText`/`runLabel`/`nextLabel`** in HTML template — Currently safe (hardcoded values) but violates defense-in-depth. Wrap in `escapeHtml()`. |
| 42 | + |
| 43 | +8. **`configureGitIdentity` platform-locked to win32** — `git config --global` works everywhere. The guard is unnecessary. |
| 44 | + |
| 45 | +9. **1,400-line method** (`setupDependencySetupWizard`) — Should be extracted to its own file (e.g. `client/dependency-wizard.js`). |
| 46 | + |
| 47 | +10. **Inline `require('child_process')`** inside route handler in server/index.js — Minor style inconsistency, already required elsewhere. |
| 48 | + |
| 49 | +### What's Good |
| 50 | + |
| 51 | +- Security is solid: URL validation on `open-url` endpoint, `escapeHtml()` used consistently, command injection surface is closed (hardcoded action IDs only) |
| 52 | +- Workspace startup race condition fixed properly with `workspaceSystemReady` promise gate |
| 53 | +- Toast notification system rewrite is a clean improvement over the old inline-style approach |
| 54 | +- Scrollbar theming well-implemented with `@supports` fallback for `color-mix()` |
| 55 | +- ARIA attributes on the onboarding modal |
| 56 | +- CSS specificity fix in notifications.js scopes generic class names properly |
| 57 | + |
| 58 | +### Architecture Note |
| 59 | + |
| 60 | +`setupActionService.js` does NOT follow the singleton class pattern used by every other service (SessionManager, WorkspaceManager, etc.). Uses module-level Maps with plain exported functions. Works fine but breaks consistency. |
| 61 | + |
| 62 | +--- |
| 63 | + |
| 64 | +## PR #791 — Updater Plugin Fix |
| 65 | + |
| 66 | +**5 lines.** Adds `plugins.updater.pubkey: ""` to `tauri.conf.json` to prevent Tauri crash on Windows packaged builds. Correct fix for the immediate panic. Same empty pubkey concern as noted in #796 review. |
| 67 | + |
| 68 | +**Note:** This commit is cherry-picked into #796, so if #796 merges first, #791 can be closed as redundant. |
| 69 | + |
| 70 | +--- |
| 71 | + |
| 72 | +## PR #790 — Test Fixes |
| 73 | + |
| 74 | +**+10/-10, 2 files.** Updates stale test expectations: |
| 75 | +- `clearAgent` → `markAgentInactive` (matching current behavior) |
| 76 | +- Seeds in-memory state directly instead of mocking `loadWorkspaceState` |
| 77 | + |
| 78 | +Clean, correct, minimal. Should merge. |
| 79 | + |
| 80 | +--- |
| 81 | + |
| 82 | +## PR #788 — Server-Only File Watching |
| 83 | + |
| 84 | +**+115/-1, 9 files.** Adds `serverOnlyFileWatching` toggle so client-only file changes don't restart nodemon. |
| 85 | + |
| 86 | +- New `scripts/dev-server.js` launcher reads user-settings.json at startup |
| 87 | +- Settings UI toggle under new "Developer" section |
| 88 | +- Follows existing patterns well |
| 89 | +- Includes `ai-memory/` files that should probably be excluded from the PR |
| 90 | + |
| 91 | +Should merge. |
| 92 | + |
| 93 | +--- |
| 94 | + |
| 95 | +## Merge Recommendations |
| 96 | + |
| 97 | +| Priority | PR | Action | |
| 98 | +|----------|----|--------| |
| 99 | +| 1 | **#790** | Merge as-is | |
| 100 | +| 2 | **#788** | Merge (optionally remove ai-memory files first) | |
| 101 | +| 3 | **#791** | Merge, file follow-up for real pubkey | |
| 102 | +| 4 | **#796** | Address medium issues (dead code, memory leak, escape hardening), then merge | |
0 commit comments