perf(packages/core): replace system_profiler with pure JS font scanning#86
Merged
zrosenbauer merged 9 commits intomainfrom Mar 18, 2026
Merged
perf(packages/core): replace system_profiler with pure JS font scanning#86zrosenbauer merged 9 commits intomainfrom
zrosenbauer merged 9 commits intomainfrom
Conversation
…ory scanning The icons middleware was taking ~9.5 seconds to start on macOS because `system_profiler SPFontsDataType` enumerates every installed font with full metadata. Replace with a pure JS implementation that scans well-known font directories using `readdir`/`lstat`, bringing startup down to ~140ms (~97% improvement). - Scan platform-specific font dirs (macOS, Linux, Windows) - Use `lstat` instead of `stat` to prevent symlink loop hangs - Skip symlinks entirely during recursive traversal - Add CodSpeed benchmarking with vitest bench for CI regression tracking - Add dedicated GitHub Actions workflow for benchmarks Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6a84825 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds benchmark infrastructure (GitHub Actions workflow .github/workflows/codspeed.yml, vitest.bench.config.ts, tests/bench/icons.bench.ts, and package.json script/devDependency for CodSpeed) and updates CI workflows to Node.js 24. Refactors system font enumeration: replaces shell/command-based discovery with recursive fs-based directory traversal (list-system-fonts.ts) and updates detectNerdFonts accordingly. Tests updated to mock fs traversal (list-system-fonts.test.ts). Example runner error handling now throws on non-zero exit results (tests/helpers.ts). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/middleware/icons/list-system-fonts.test.ts (1)
37-37:⚠️ Potential issue | 🟠 MajorAdd
configurable: trueto everyprocess.platformoverride.These
Object.defineProperty()calls create non-configurable descriptors, causing the restore inafterEach()to throw when subsequent tests attempt to overrideprocess.platform. All 5 instances (lines 37, 43, 141, 163, 174) must includeconfigurable: truein the descriptor.🐛 Proposed fix
- Object.defineProperty(process, 'platform', { value: originalPlatform }) + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true })Apply the same fix to lines 43, 141, 163, and 174.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/icons/list-system-fonts.test.ts` at line 37, Every override of process.platform in list-system-fonts.test.ts is defined as a non-configurable property which makes restoring it in afterEach() throw; update each Object.defineProperty call that sets process.platform (e.g., the calls currently written as Object.defineProperty(process, 'platform', { value: ... })) to include configurable: true in the descriptor so the property can be redefined/restored; apply this change to all five occurrences referenced in the review (the overrides around the platform-specific test cases and the restore in afterEach()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/middleware/icons/list-system-fonts.ts`:
- Around line 95-99: The current fallback for localAppData erroneously uses
process.env['APPDATA'] which points to a different profile tree; change the
localAppData assignment in list-system-fonts.ts so it only uses
process.env['LOCALAPPDATA'] and if unset falls back to join(home, 'AppData',
'Local') (remove the APPDATA fallback), and ensure the returned paths still use
this corrected localAppData variable when building the per-user Fonts path.
- Around line 177-183: safeReaddir and safeLstat are currently swallowing all fs
errors and returning empty/nullable values, causing listSystemFonts to appear
successful on unexpected failures; change both functions to only suppress
expected "missing path" errors (ENOENT, ENOTDIR) and otherwise surface the error
through the Result-style tuple used in this codebase (do not throw). Update the
functions' signatures to return a Result-like tuple (e.g., [value, error]) and
update callers — notably listSystemFonts and detectNerdFonts — to check the
returned error and propagate it instead of treating an empty array/null as
success; ensure tests and any downstream handling follow the tuple convention.
In `@tests/bench/icons.bench.ts`:
- Around line 8-13: The benchmark runner currently ignores the child process
exit status so failures are treated as successful runs; update
createExampleRunner (the helper used by these benches) to capture and return
spawnSync's exit status (or throw on non-zero status) along with combined
output, and then adjust the bench usage (the run call in
tests/bench/icons.bench.ts) to assert/throw when the returned status is
non-zero; ensure you reference the spawnSync result fields (status, error) in
createExampleRunner and propagate/raise errors so the bench fails when the CLI
crashes or build is missing.
In `@vitest.bench.config.ts`:
- Around line 3-9: The current config mutates a local array (const plugins = [];
plugins.push(...)) inside defineConfig; replace this with an immutable
expression that constructs the plugins array without push, e.g. build and return
plugins via a conditional expression that uses await
import('@codspeed/vitest-plugin') only when process.env['CI'] is truthy, invoke
the imported default (codspeedPlugin) to include the plugin, and otherwise
return an empty array—keeping the async defineConfig function and referencing
defineConfig, process.env['CI'], and the imported codspeedPlugin by name so no
mutable plugins array or push() calls remain.
---
Outside diff comments:
In `@packages/core/src/middleware/icons/list-system-fonts.test.ts`:
- Line 37: Every override of process.platform in list-system-fonts.test.ts is
defined as a non-configurable property which makes restoring it in afterEach()
throw; update each Object.defineProperty call that sets process.platform (e.g.,
the calls currently written as Object.defineProperty(process, 'platform', {
value: ... })) to include configurable: true in the descriptor so the property
can be redefined/restored; apply this change to all five occurrences referenced
in the review (the overrides around the platform-specific test cases and the
restore in afterEach()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 107daf64-76b1-443f-b512-909a16fdad36
⛔ Files ignored due to path filters (2)
.changeset/perf-icons-startup.mdis excluded by!.changeset/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (7)
.github/workflows/codspeed.ymlpackage.jsonpackages/core/src/middleware/icons/detect.tspackages/core/src/middleware/icons/list-system-fonts.test.tspackages/core/src/middleware/icons/list-system-fonts.tstests/bench/icons.bench.tsvitest.bench.config.ts
Fix Node.js 20 deprecation warnings by upgrading to v5 which supports Node.js 24. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codspeed.yml:
- Around line 36-46: The workflow runs "Run Benchmarks" after "Build"; move the
"Run Benchmarks" step (uses: CodSpeedHQ/action@v4 with run: pnpm test:bench) to
occur before the "Build" step (the step named "Build" that runs pnpm build) or
remove the "Build" step entirely if benchmarks don't require a build; ensure
"Rebuild bin links" (pnpm install --frozen-lockfile) remains in the correct
place relative to the benchmark invocation so pnpm test:bench has required
dependencies.
- Line 19: The workflow uses a non-GitHub-hosted runner label "runs-on:
codspeed-macro" which requires organization-specific self-hosted runners; either
configure self-hosted runners with the codspeed-macro label or change the job's
runner to a GitHub-hosted label (for example replace the value of runs-on in the
workflow with "ubuntu-latest") so the job will actually execute; locate the
"runs-on: codspeed-macro" entry in the codspeed.yml workflow and update it
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7dbde23-4080-45c5-b44a-8ca4eff9182c
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/codspeed.yml
- Add `configurable: true` to all Object.defineProperty process.platform overrides in tests to prevent restore failures - Remove incorrect APPDATA fallback for Windows LOCALAPPDATA (APPDATA points to Roaming, not Local) - Extract CodSpeed plugin loading into named function to avoid ternary and await expression member access - Check spawnSync exit status in createExampleRunner so benchmarks fail on CLI crashes Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
zrosenbauer
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
All actionable items fixed: configurable:true on Object.defineProperty, APPDATA fallback removed, vitest.bench.config immutable, createExampleRunner exit status check. Remaining comments (codspeed-macro runner, build order, safeReaddir error swallowing) are intentional design decisions.
The bench tests require examples/icons/dist/index.mjs which is not built by `pnpm build` (only builds packages). Add explicit step to build the icons example before running benchmarks. Co-Authored-By: Claude <noreply@anthropic.com>
Bare `turbo` isn't on PATH in the CodSpeed macro runner; use `pnpm turbo` to resolve it from node_modules. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/middleware/icons/list-system-fonts.ts (1)
176-182:⚠️ Potential issue | 🟠 MajorDo not swallow all filesystem errors in
safeReaddir/safeLstat.Current behavior treats every fs failure as “missing path,” which can silently mask real scan failures and return false negatives as successful results.
🐛 Proposed fix (only suppress missing-path errors)
+function isMissingPathError(error: unknown): boolean { + if (!(error instanceof Error)) { + return false + } + const code = (error as NodeJS.ErrnoException).code + return code === 'ENOENT' || code === 'ENOTDIR' +} + async function safeReaddir(dir: string): Promise<readonly string[]> { try { return await readdir(dir) - } catch { - return [] + } catch (error) { + if (isMissingPathError(error)) { + return [] + } + return Promise.reject(error) } } async function safeLstat( filePath: string ): Promise<ReturnType<typeof lstat> extends Promise<infer T> ? T | null : never> { try { return await lstat(filePath) - } catch { - return null + } catch (error) { + if (isMissingPathError(error)) { + return null + } + return Promise.reject(error) } }#!/bin/bash set -euo pipefail echo "Current fs helper error handling:" rg -n "async function safe(Readdir|Lstat)|catch" packages/core/src/middleware/icons/list-system-fonts.ts -A6 -B3 echo echo "Current test coverage of fs error codes:" rg -n "ENOENT|ENOTDIR|EACCES|EPERM" packages/core/src/middleware/icons/list-system-fonts.test.ts -A2 -B2Expected verification result: only missing-path errors should be suppressed; unexpected fs errors should propagate to
err(...)throughlistSystemFonts.
As per coding guidelines, contributing/standards/typescript/errors.md requires explicit Result-style error propagation in multi-step flows instead of collapsing failures into success-like values.Also applies to: 195-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/icons/list-system-fonts.ts` around lines 176 - 182, The helpers safeReaddir and safeLstat currently swallow all fs errors; change their catch clauses to only suppress missing-path errors by checking the error.code (e.g., ENOENT and ENOTDIR) and return the existing "missing" sentinel (e.g., [] from safeReaddir) in those cases, but rethrow any other errors so they propagate up to listSystemFonts and its err(...) handling; locate safeReaddir and safeLstat in the diff and replace the bare catch with: catch (e) { if ((e as NodeJS.ErrnoException)?.code === 'ENOENT' || (e as NodeJS.ErrnoException)?.code === 'ENOTDIR') return /* original missing value */; throw e }.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers.ts`:
- Around line 30-36: The helper currently throws on failure paths (when
result.error or nonzero result.status); change the runner to return a
Result-style tuple instead of throwing: when result.error exists return
[result.error, null] (or equivalent Error-first tuple), and when result.status
!== 0 return [new Error(`CLI exited with status ${String(result.status)}:
${result.stderr}`), null]; on success return [null, result] (or [null, <success
value>]). Update the function signature/return type (e.g., in the runner
function that references the local variable result) to reflect the Result tuple
and remove the throw statements so callers receive the tuple rather than
exceptions.
---
Duplicate comments:
In `@packages/core/src/middleware/icons/list-system-fonts.ts`:
- Around line 176-182: The helpers safeReaddir and safeLstat currently swallow
all fs errors; change their catch clauses to only suppress missing-path errors
by checking the error.code (e.g., ENOENT and ENOTDIR) and return the existing
"missing" sentinel (e.g., [] from safeReaddir) in those cases, but rethrow any
other errors so they propagate up to listSystemFonts and its err(...) handling;
locate safeReaddir and safeLstat in the diff and replace the bare catch with:
catch (e) { if ((e as NodeJS.ErrnoException)?.code === 'ENOENT' || (e as
NodeJS.ErrnoException)?.code === 'ENOTDIR') return /* original missing value */;
throw e }.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84de7672-c297-4239-b864-984ba04c488d
📒 Files selected for processing (5)
.github/workflows/codspeed.ymlpackages/core/src/middleware/icons/list-system-fonts.test.tspackages/core/src/middleware/icons/list-system-fonts.tstests/helpers.tsvitest.bench.config.ts
- Bump all workflows to actions/checkout@v5 and actions/setup-node@v5 - Set node-version: 24 across all workflows - Add FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env to eliminate deprecation warnings from third-party actions still bundled with Node 20 - Add engines.node >= 24 to root package.json Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/codspeed.yml (1)
39-52:⚠️ Potential issue | 🟠 MajorRun benchmark tests before the full build step.
The workflow currently executes benchmarks after
pnpm build. Reorder so benchmark tests run first (or drop the full build if not required), while keeping only the minimum prerequisite build needed for the benchmark target.Proposed step order adjustment
- - name: Build - run: pnpm build - - name: Rebuild bin links run: pnpm install --frozen-lockfile - name: Build benchmark examples run: pnpm turbo run build --filter=@examples/icons - name: Run Benchmarks uses: CodSpeedHQ/action@v4 with: mode: walltime run: pnpm test:bench + + - name: Build + run: pnpm buildAs per coding guidelines,
.github/workflows/**/*.ymlmust "Ensure tests run before build".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codspeed.yml around lines 39 - 52, Reorder the CI steps so the "Run Benchmarks" step executes before the full "Build" step: move the "Run Benchmarks" (uses: CodSpeedHQ/action@v4 with run: pnpm test:bench) above the "Build" step and ensure its minimal prerequisites remain directly before it—keep "Rebuild bin links" (run: pnpm install --frozen-lockfile) and the "Build benchmark examples" (run: pnpm turbo run build --filter=@examples/icons) immediately before the "Run Benchmarks" step (or drop the full "Build" step entirely if not required for benchmarks) so benchmarks run prior to the larger build.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/codspeed.yml:
- Around line 39-52: Reorder the CI steps so the "Run Benchmarks" step executes
before the full "Build" step: move the "Run Benchmarks" (uses:
CodSpeedHQ/action@v4 with run: pnpm test:bench) above the "Build" step and
ensure its minimal prerequisites remain directly before it—keep "Rebuild bin
links" (run: pnpm install --frozen-lockfile) and the "Build benchmark examples"
(run: pnpm turbo run build --filter=@examples/icons) immediately before the "Run
Benchmarks" step (or drop the full "Build" step entirely if not required for
benchmarks) so benchmarks run prior to the larger build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32b94722-077f-480d-a179-a25fd681e3b5
📒 Files selected for processing (5)
.github/workflows/changeset-check.yml.github/workflows/ci.yml.github/workflows/codspeed.yml.github/workflows/release.ymlpackage.json
zrosenbauer
commented
Mar 18, 2026
Member
Author
zrosenbauer
left a comment
zrosenbauer
left a comment
There was a problem hiding this comment.
❌ Request Changes
Please address comments in review.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/middleware/icons/list-system-fonts.ts (1)
174-180:⚠️ Potential issue | 🟠 MajorPropagate unexpected filesystem failures instead of silently returning empty results.
Line 177 and Line 198 currently collapse all
readdir/lstaterrors into[]/null. That can make font detection report a false negative when scanning actually failed (e.g., EACCES, EIO). Only suppress missing-path cases (likeENOENT/ENOTDIR) and let other failures propagate tolistSystemFonts()as an error Result.Proposed fix
+const isMissingPathError = (error: unknown): boolean => + match(error) + .when( + (value): value is NodeJS.ErrnoException => + value instanceof Error && 'code' in value, + ({ code }) => code === 'ENOENT' || code === 'ENOTDIR' + ) + .otherwise(() => false) + async function safeReaddir(dir: string): Promise<readonly string[]> { try { return await readdir(dir) - } catch { - return [] + } catch (error) { + if (isMissingPathError(error)) { + return [] + } + return Promise.reject(error) } } @@ async function safeLstat( filePath: string ): Promise<ReturnType<typeof lstat> extends Promise<infer T> ? T | null : never> { try { return await lstat(filePath) - } catch { - return null + } catch (error) { + if (isMissingPathError(error)) { + return null + } + return Promise.reject(error) } }As per coding guidelines,
contributing/standards/typescript/errors.mdrequires explicit Result-style error propagation for multi-step flows instead of masking unexpected failures.Also applies to: 193-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/icons/list-system-fonts.ts` around lines 174 - 180, The helper functions (safeReaddir and the analogous safeLstat) currently swallow all FS errors and return []/null; change them to only suppress missing-path errors (errno/code ENOENT and ENOTDIR) and for any other error rethrow (or return a propagated error Result) so listSystemFonts() receives failures instead of false-negative empty results; update safeReaddir to catch the error, if (err.code === 'ENOENT' || err.code === 'ENOTDIR') return [] else throw err (and apply the same pattern to safeLstat), ensuring listSystemFonts's multi-step flow can surface unexpected filesystem errors as Results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/middleware/icons/list-system-fonts.ts`:
- Around line 51-62: The exported function listSystemFonts must not use
try/catch; remove the internal try/catch and instead wrap the promise chain
using the async Result wrapper pattern: call
Promise.all(dirs.map(scanDirectory)) and convert that promise into a Result via
the project's async-to-Result helper (the "async wrapper" from the standards),
then on success flatten the nested arrays to produce fonts and return ok(fonts),
and on failure return err(error); keep references to listSystemFonts, dirs,
scanDirectory, ok and err so reviewers can locate the changes.
- Around line 132-148: Replace the multi-branch if chain in processEntry with a
ts-pattern match on the awaited info value: after acquiring info via await
safeLstat(fullPath), use match(info) from 'ts-pattern' and add cases for null
(return []), a case where info.isSymbolicLink() (return []), info.isDirectory()
(return scanDirectory(fullPath)), and a case for info.isFile() &&
isFontFile(fullPath) (return [fullPath]), with an otherwise() returning [];
ensure you import match from 'ts-pattern' and preserve the existing async/await
behavior and returns from processEntry, referencing the function processEntry
and helpers safeLstat, scanDirectory, and isFontFile.
---
Duplicate comments:
In `@packages/core/src/middleware/icons/list-system-fonts.ts`:
- Around line 174-180: The helper functions (safeReaddir and the analogous
safeLstat) currently swallow all FS errors and return []/null; change them to
only suppress missing-path errors (errno/code ENOENT and ENOTDIR) and for any
other error rethrow (or return a propagated error Result) so listSystemFonts()
receives failures instead of false-negative empty results; update safeReaddir to
catch the error, if (err.code === 'ENOENT' || err.code === 'ENOTDIR') return []
else throw err (and apply the same pattern to safeLstat), ensuring
listSystemFonts's multi-step flow can surface unexpected filesystem errors as
Results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdb38b0c-7f4b-490d-9667-ced9c565008a
📒 Files selected for processing (1)
packages/core/src/middleware/icons/list-system-fonts.ts
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
1 participant
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
system_profiler SPFontsDataTypeenumerates every installed font with full metadatareaddir/lstat) that checks well-known font directories per OS, bringing startup to ~140ms (~97% improvement)Changes
list-system-fonts.ts— replaced shell command with recursive font directory scanning~/Library/Fonts,/Library/Fonts,/System/Library/Fonts,/System/Library/Fonts/Supplemental,/Network/Library/Fonts/usr/share/fonts,/usr/local/share/fonts,~/.fonts,~/.local/share/fonts%WINDIR%\Fonts,%LOCALAPPDATA%\Microsoft\Windows\Fontslstat(notstat) to prevent infinite loops from cyclic symlinksnode:fs/promisesPerformance
system_profiler SPFontsDataTypereaddir+lstatTest plan
pnpm checkpasses (typecheck + lint + format)pnpm testpasses (735 unit + 62 integration)pnpm test:benchruns successfullytime ./examples/icons/dist/index.mjs show 0completes in <300ms