refactor(skills): remove the unreachable Skills page and the file count it rendered - #14259
Conversation
`activeView === 'skills'` has had no way to be set since #4535 removed the sidebar toolbox menu on 2026-06-02. That commit deleted the only `onSelect={openSkillsPage}` and left the page, its store actions, and its view member behind. Every release tag in the current window postdates it, so no shipped build can reach this surface. Removes the page, SkillCard, its filter and label helpers, the open/closeSkillsPage actions and `previousViewBeforeSkills`, the 'skills' member of TopLevelView, and 26 localization keys used only here. Dropping 'skills' from TopLevelViewSchema is safe for a paired old client: UiUpdate wraps every field in `tolerateUnknownValues`, so a value the schema cannot express is dropped from the payload and the rest of the batch still lands. Persisted state is already covered — `sanitizeHydratedActiveView` falls back to 'terminal' for any value failing `isTopLevelView`. The skill freshness components in the same directory are untouched; they are reached from App.tsx, the status bar, and Settings.
`DiscoveredSkill.fileCount` existed to render "N files" on a skill card, and that card was the surface removed in the previous commit. Producing it cost a second full walk of every skill package on the native path, and one `find` subprocess per discovered skill inside the WSL scan script. Native, on the 32-scan / 8-workspace repro: 2,956 -> 2,804 filesystem calls (5%). The walk it removes is cheap because it was already depth- and node_modules-bounded; the real win is WSL, where a 14-root scan of 31 skills drops from 45 `find` invocations to 14 (-69%). Removing the field is a Rule 3 wire change per docs/reference/remote-wire-compatibility.md — an old client reading it now sees undefined. Safe here because the only reader was unreachable UI in every shipped version, and it is deleted in the previous commit. The cross-version harness covers the terminal stream only, so this reasoning is the artifact rather than a test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe standalone Skills page, navigation actions, route rendering, filters, labels, and localized strings were removed. The Mergeability Score: 🔵 Low · up to The PR is otherwise mergeable, but the added WSL protocol test can leave its temporary fixture behind after repeated or failed runs, creating a bounded risk of test interference; clean up the fixture as follow-up or with owner awareness. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Readiness review found no defects but three gaps worth closing. The WSL scan is a positional NUL-delimited protocol whose producer is a bash script and whose consumer is a parser in the same file. Every test hand-encoded the record, so the two sides can drift together with a bug — this PR's own 6-to-5 field change was verified only by editing fixtures to match. Adds a test that runs the real generated script and feeds its real stdout to the real parser, so the field count is checked by execution rather than by memory. Mutation-verified: re-adding a field to the script fails it with 'unknown source'. Also prunes four locale-override entries keyed on i18n keys this PR deleted (nothing validates that override keys exist, which is why lint stayed green), and pins the 'skills' -> 'terminal' hydration demotion by name so the removal reads as a migration rather than an accident.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/skills/skill-discovery-wsl-script-roundtrip.test.ts (1)
12-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the protocol comment.
The rationale is useful, but this block also describes test flow and implementation details. Keep only the protocol-drift rationale and the macOS timestamp caveat in one or two lines.
As per coding guidelines, comments in TypeScript must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.
Suggested comment reduction
-// Why: the WSL scan is a positional NUL-delimited protocol whose producer is a bash -// script and whose consumer is a parser in the same file. Every other test hand-encodes -// the record, so a field added to or removed from one side is only caught by whoever -// remembers to edit the fixture — the two can drift together with the bug. This runs -// the real generated script and feeds its real stdout to the real parser, so the field -// count is checked by execution rather than by memory. -// -// The script is Linux-shaped but portable enough to run here: `stat -c` is GNU-only and -// fails on macOS, which is why `updatedAt` is not asserted — an absent timestamp is -// already the parser's documented degradation, not this test's subject. +// Execute the generated script to catch producer/parser drift in the WSL field layout. +// macOS may omit GNU `stat` timestamps, so `updatedAt` is not asserted here.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a8068db-d031-4707-b4be-b6262335356b
📒 Files selected for processing (5)
config/scripts/locale-cross-locale-key-overrides.mjsconfig/scripts/locale-key-overrides.mjsconfig/scripts/locale-ko-key-overrides.jsonsrc/main/skills/skill-discovery-wsl-script-roundtrip.test.tssrc/renderer/src/store/slices/ui.test.ts
💤 Files with no reviewable changes (3)
- config/scripts/locale-cross-locale-key-overrides.mjs
- config/scripts/locale-key-overrides.mjs
- config/scripts/locale-ko-key-overrides.json
| const base = await mkdtemp(join(tmpdir(), 'orca-wsl-roundtrip-')) | ||
| const presentRoot = join(base, 'skills') | ||
| await mkdir(join(presentRoot, 'review'), { recursive: true }) | ||
| await writeFile( | ||
| join(presentRoot, 'review', 'SKILL.md'), | ||
| '---\nname: code-review\ndescription: Review code changes.\n---\n' | ||
| ) | ||
| await mkdir(join(presentRoot, 'plan'), { recursive: true }) | ||
| await writeFile( | ||
| join(presentRoot, 'plan', 'SKILL.md'), | ||
| '---\nname: planner\ndescription: Plan the work.\n---\n' | ||
| ) | ||
| const roots = [root('present', presentRoot), root('missing', join(base, 'absent'))] | ||
|
|
||
| const { stdout } = await run('bash', ['-c', buildWslSkillDiscoveryCommand(roots)], { | ||
| encoding: 'utf8', | ||
| maxBuffer: 64 * 1024 * 1024 | ||
| }) | ||
| const result = parseWslSkillDiscoveryOutput(stdout, roots, 42) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the temporary fixture after the test.
mkdtemp creates base, but the test never removes it. Repeated or failed runs leave files in the OS temporary directory. Wrap setup, execution, and assertions in try/finally, then remove base with rm(..., { recursive: true, force: true }).
Suggested cleanup
-import { mkdir, mkdtemp, writeFile } from 'node:fs/promises'
+import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'
...
const base = await mkdtemp(join(tmpdir(), 'orca-wsl-roundtrip-'))
+ try {
+ // Move the existing fixture setup, execution, and assertions here.
+ } finally {
+ await rm(base, { recursive: true, force: true })
+ }🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
Round-2 review noted the case started from the store default, which is already 'terminal' — so it could not distinguish a demoted value from hydration never running. Seeding 'tasks' first makes it fail when 'skills' is restored to TOP_LEVEL_VIEW_LOOKUP, which is the invariant it exists to pin. Co-authored-by: Orca <help@stably.ai>
1 participant
ELI5
There's a "Skills" page in the app that nobody can open — the button that used to open it was deleted ten weeks ago and the page was left behind. This removes the page, and the expensive number it was the only thing displaying.
What Changed
Two removals, one commit each, each independently valid.
1. The unreachable Skills page.
activeView === 'skills'has had no way to be set since #4535 removed the sidebar toolbox menu on 2026-06-02 — that commit deleted the onlyonSelect={openSkillsPage}and left everything behind it in place. There is no button, command-palette entry, menu item, keybinding, deep link, or e2e spec that reaches it.Removed:
SkillsPage,SkillCard,skills-filter,skill-display-labels, theopenSkillsPage/closeSkillsPageactions,previousViewBeforeSkills, the'skills'member ofTopLevelView(and its lookup, RPC schema, and right-sidebar entry), and 26 localization keys used only here.Untouched: every skill freshness component in the same directory (
SkillFreshnessNudge,SkillFreshnessUpdateDialog,SkillUpdateRow, the status pill, the run store). Those are live fromApp.tsx, the status bar, and Settings — I classified all 22 files incomponents/skills/before deleting six.2.
DiscoveredSkill.fileCount. It existed solely to render "N files" on the card removed above. Producing it cost a second full walk of every skill package on the native path, and onefindsubprocess per discovered skill inside the WSL scan script.Why
countPackageFileswas pure waste the moment its only consumer became unreachable — a recursive walk per package, on the Electron main process's libuv threadpool, for a string nothing rendered.Measured on the same 32-scan / 8-workspace repro used in #14204:
I want to flag that honestly: I previously estimated this at "roughly a third" and that was wrong. The native saving is small because the walk was already depth- and
node_modules-bounded by #14204, so the second walk was cheap. The real win is WSL, where the per-skillfindloop disappears: a 14-root scan of 31 skills drops from 45findinvocations to 14 (-69%), and the WSL protocol record goes from 6 fields to 5.Linked Issue
No tracked issue — found while tracing where
fileCountwas rendered during #14204 review.Fixes #
Visual Proof
N/A— nothing rendered changes, because nothing rendered this. The page had no route into it and the "N files" label was inside it.Testing
Two existing tests caught the change and were updated rather than deleted, which is the evidence it does what it says:
skill-discovery-concurrency.test.ts—READDIR_CALLS_PER_POPULATED_ROOT3 → 2, because the second package walk is gone.skill-discovery-wsl.test.ts/skill-discovery-wsl-plugins.test.ts— the WSL wire fixture is positional, so dropping thefile_countfield required removing it from eachSrecord.Also removed the
countPackageFilestest block, andfileCountfrom 16DiscoveredSkillfixtures.Reviewed against the full readiness checklist across two rounds (3 agents, then 1). Neither round found a defect. Round one raised three gaps, closed in
98c7fa6f4b3:'skills'→'terminal'hydration demotion is now pinned by name, so the removal reads as a deliberate migration.pnpm lint(incl. all four localization gates),pnpm typecheck, and the full suite pass. Verified commit 1 typechecks standalone. Two failures insrc/relay/agent-exec-handler.test.tsare pre-existing — confirmed failing on cleanmain— and onegithub-projecttest is flaky under full-suite parallelism (passes in isolation, references none of these surfaces).AI Disclosure
Claude Opus 4.5 via Claude Code.
Review
Backwards compatibility (the one real risk). Removing
fileCountis a Rule 3 change perdocs/reference/remote-wire-compatibility.md— "a field the host stops populating (an old client reading it now seesundefined)". It is unobservable by any shipped client, verified from history rather than assumed:b960275db2e, Remove toolbox menu from sidebar toolbar #4535);SkillCard.tsxdid not exist yet at that commit;pluralize(skill.fileCount, 'file')line was introduced on 2026-07-29 (8f36cd9bafd, fix(skills): read installed skills from the connected remote runtime #6887).So the label was added to a component that had already been unreachable for two months. No build has ever rendered it, which means no paired old client can observe the field going missing. Note also that
runtime-skills-client.ts:31casts the RPC result without zod validation, so there is no schema rejection path either — the field simply becomesundefinedon a surface nothing can reach. The cross-version harness covers the terminal stream only, so it would not catch this either way; this reasoning is the artifact.Dropping
'skills'fromTopLevelViewSchemais safe by existing design:UiUpdatewraps every field intolerateUnknownValues(.catch(() => undefined)), so an old client sendingactiveView: 'skills'has just that value dropped and the rest of the batch still lands — which is precisely what that helper was built for.Persisted state needs no migration:
sanitizeHydratedActiveViewalready falls back to'terminal'for anything failingisTopLevelView.Security / cross-platform / mobile — no new surface; WSL script emits one fewer field and no per-skill subprocess; mobile has no skills consumer.
Note for the reviewer
This deletes the only UI for browsing installed skills. The native-chat
@-picker still lists them inline and Settings keeps its per-feature install prompts, but there is no longer a "here is everything installed and where it came from" view. It has been unreachable for ten weeks, and restoring it is agit revert— but if the intent was to re-link it rather than drop it, this is the PR to say so on.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm testpass (pre-existing unrelated failures noted)Made with Orca 🐋