(testing around) Add gfcli remove command to uninstall installed Google Fonts#3
(testing around) Add gfcli remove command to uninstall installed Google Fonts#3
gfcli remove command to uninstall installed Google Fonts#3Conversation
systemFont.uninstall removes TTF/WOFF2 files from the same per-user directories used by install. googleFont.uninstallAsync resolves variants via the GWFH file map and deletes matching basenames. Extend tests and types accordingly. Co-authored-by: Tin Sever <mail@tin-sever.de>
Introduces gfcli remove <family...> with optional --variants, matching install behavior for multi-family and partial success reporting. Co-authored-by: Tin Sever <mail@tin-sever.de>
📝 WalkthroughWalkthroughA new font removal feature is introduced across the CLI and library layers. The Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI (cli.js)
participant GF as GoogleFont<br/>(lib/google-font.js)
participant SF as SystemFont<br/>(lib/system-font.js)
participant FS as File System
participant Cache as fc-cache<br/>(Linux only)
User->>CLI: remove <family...>
CLI->>GF: loadFonts()
CLI->>GF: getFontByNameAsync(family)
GF-->>CLI: GoogleFont instance
CLI->>GF: uninstallAsync(variants)
GF->>GF: Fetch variant→URL map
GF->>GF: Normalize variant identifiers
loop For each variant
GF->>SF: uninstall(PascalCase-fileBase)
SF->>SF: _uninstallCandidatePaths(fileBase)
SF->>SF: Compute platform-specific paths
SF->>FS: Delete .ttf and .woff2 files
FS-->>SF: File removal result
alt Linux platform
SF->>Cache: fc-cache -f
Cache-->>SF: Cache refresh (best effort)
end
SF-->>GF: FontResult[]
end
GF-->>CLI: Aggregated FontResult[]
CLI->>User: Report results & exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
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: 1
🧹 Nitpick comments (2)
lib/google-font.js (1)
190-218: LGTM with optional cleanup.The implementation correctly mirrors
installAsyncand properly delegates tosystemFont.uninstall. The variant resolution via the GWFH file map ensures consistency with installed file basenames.One optional cleanup: the
catch (err) { throw err; }block (lines 211-213) is redundant—the error would propagate naturally without it. The same pattern exists ininstallAsync, so this is consistent but could be simplified in both places.♻️ Optional: Remove redundant catch block
if (url) { - try { - const paths = await systemFont.uninstall(fileName + '-' + norm); - for (const p of paths) { - resultList.push({ family: this.getFamily(), variant: norm, path: p }); - } - } catch (err) { - throw err; + const paths = await systemFont.uninstall(fileName + '-' + norm); + for (const p of paths) { + resultList.push({ family: this.getFamily(), variant: norm, path: p }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/google-font.js` around lines 190 - 218, Remove the redundant try/catch that simply rethrows the error in googleFont.prototype.uninstallAsync: inside the loop where you call systemFont.uninstall (and similarly in installAsync if you want consistency), delete the catch (err) { throw err; } block so the original error will propagate naturally; locate the try around systemFont.uninstall call in uninstallAsync and remove the catch only, leaving the await call and result handling intact.test/system-font.test.js (1)
138-156: Good test coverage for uninstall success and failure paths.The tests verify:
- Successful removal returns paths and calls
unlink- ENOENT on all files throws "Font file not found"
Minor note:
jest.spyOn(fsPromises, 'unlink')on lines 139 and 153 is redundant since it's already set up inbeforeEach(line 34). You can simply call.mockResolvedValue()or.mockRejectedValue()on the existing spy.♻️ Optional: Remove redundant spy setup
it('uninstall removes existing font files and returns their paths', async () => { - jest.spyOn(fsPromises, 'unlink').mockResolvedValue(undefined); + fsPromises.unlink.mockResolvedValue(undefined); const execSpy = jest.spyOn(require('child_process'), 'exec').mockImplementation((cmd, cb) => {it('uninstall throws when no matching files exist', async () => { - jest.spyOn(fsPromises, 'unlink').mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); + fsPromises.unlink.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system-font.test.js` around lines 138 - 156, The tests for uninstall currently call jest.spyOn(fsPromises, 'unlink') again even though a spy on fsPromises.unlink is already created in beforeEach; update the two test cases to remove the redundant jest.spyOn calls and instead call .mockResolvedValue(...) or .mockRejectedValue(...) directly on the existing fsPromises.unlink spy, leaving the exec/child_process spy logic and assertions for systemFont.uninstall intact so the behavior and expectations for uninstall remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/system-font.js`:
- Around line 260-266: The uninstall search currently only adds the per-user
fonts folder (%LOCALAPPDATA%\Microsoft\Windows\Fonts) for Windows, but install
copies to the system fonts folder (C:\Windows\Fonts) in install; update the
_uninstallCandidatePaths logic so the Windows branch also includes the system
fonts folder used by install (e.g., path.join(process.env.WINDIR ||
process.env.SystemROOT || 'C:\\Windows', 'Fonts')) so that
_uninstallCandidatePaths (and related uninstall routines) can find files
installed by install.
---
Nitpick comments:
In `@lib/google-font.js`:
- Around line 190-218: Remove the redundant try/catch that simply rethrows the
error in googleFont.prototype.uninstallAsync: inside the loop where you call
systemFont.uninstall (and similarly in installAsync if you want consistency),
delete the catch (err) { throw err; } block so the original error will propagate
naturally; locate the try around systemFont.uninstall call in uninstallAsync and
remove the catch only, leaving the await call and result handling intact.
In `@test/system-font.test.js`:
- Around line 138-156: The tests for uninstall currently call
jest.spyOn(fsPromises, 'unlink') again even though a spy on fsPromises.unlink is
already created in beforeEach; update the two test cases to remove the redundant
jest.spyOn calls and instead call .mockResolvedValue(...) or
.mockRejectedValue(...) directly on the existing fsPromises.unlink spy, leaving
the exec/child_process spy logic and assertions for systemFont.uninstall intact
so the behavior and expectations for uninstall remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5ce7250-4504-45e7-82c8-038ece9461ad
📒 Files selected for processing (6)
cli.jslib/google-font.jslib/system-font.jslib/types.d.tstest/google-font.test.jstest/system-font.test.js
| case 'win32': { | ||
| const localAppData = process.env.LOCALAPPDATA; | ||
| if (localAppData) { | ||
| dirs.push(path.join(localAppData, 'Microsoft', 'Windows', 'Fonts')); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the Windows install path behavior by checking the PowerShell namespace constant
# Search for how install handles Windows to understand the target directory
rg -n -A 10 "Namespace\(0x14\)" --type js
# Check if there are any comments or documentation about Windows font paths
rg -n -i "windows.*font" --type js --type mdRepository: t2sever/gfcli
Length of output: 1599
🌐 Web query:
What is the Shell.Application Namespace(0x14) fonts folder path in Windows 10 and 11?
💡 Result:
In Windows 10 and Windows 11, Shell.Application.Namespace(0x14) (aka ssfFONTS / CSIDL_FONTS) resolves to the system Fonts folder:
- Environment form:
%WINDIR%\Fonts(or%SystemRoot%\Fonts) (learn.microsoft.com) - Typical absolute path:
C:\Windows\Fonts(learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid?utm_source=openai
- 2: https://learn.microsoft.com/id-id/windows/win32/api/shldisp/ne-shldisp-shellspecialfolderconstants?utm_source=openai
Windows install/uninstall path mismatch prevents font removal.
The install method (lines 199-236) uses PowerShell with Shell.Application.Namespace(0x14) to copy fonts to the system fonts folder (C:\Windows\Fonts). However, _uninstallCandidatePaths searches for fonts in %LOCALAPPDATA%\Microsoft\Windows\Fonts (the per-user fonts folder).
Fonts installed via this tool will not be found during uninstall on Windows, causing "Font file not found" errors. Update the uninstall path to match the system folder where install places fonts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/system-font.js` around lines 260 - 266, The uninstall search currently
only adds the per-user fonts folder (%LOCALAPPDATA%\Microsoft\Windows\Fonts) for
Windows, but install copies to the system fonts folder (C:\Windows\Fonts) in
install; update the _uninstallCandidatePaths logic so the Windows branch also
includes the system fonts folder used by install (e.g.,
path.join(process.env.WINDIR || process.env.SystemROOT || 'C:\\Windows',
'Fonts')) so that _uninstallCandidatePaths (and related uninstall routines) can
find files installed by install.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| return resultList; | ||
| }; |
There was a problem hiding this comment.
Uninstall aborts on first missing variant, losing deletions
High Severity
uninstallAsync re-throws immediately when systemFont.uninstall fails for any variant (e.g., a variant that was never installed). When no --variants flag is given, the method attempts all variants from the API. If only a subset was installed, the first non-installed variant throws "Font file not found," aborting the loop. Files already deleted by earlier iterations are gone, but the partial resultList is never returned. The CLI counts this as a failure, leaving the user unaware that some fonts were actually removed. Unlike saveAtAsync, which collects errors and continues, this method uses a catch (err) { throw err; } pattern that makes partial uninstalls both destructive and unreported.
| if (localAppData) { | ||
| dirs.push(path.join(localAppData, 'Microsoft', 'Windows', 'Fonts')); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Windows uninstall path mismatches install destination
Medium Severity
On Windows, install uses Shell.Application.Namespace(0x14) which dynamically resolves to either C:\Windows\Fonts (admin) or %LOCALAPPDATA%\...\Fonts (non-admin on Win10 1809+). The new uninstall hardcodes only the %LOCALAPPDATA% per-user path. Fonts installed with admin privileges live in a different directory and will never be found by uninstall, yielding a misleading "Font file not found" error. Additionally, fs.unlink alone doesn't remove the Windows Registry entry (HKCU\...\Fonts), so even when the file is found, the font remains registered and appears broken to applications.
Additional Locations (1)
| } | ||
| } | ||
|
|
||
| return resultList; |
There was a problem hiding this comment.
Remove silently succeeds when no files are deleted
Medium Severity
In uninstallAsync, the if (url) guard silently skips any variant whose name doesn't match a key in the GWFH API file map. If all user-specified variants are skipped (e.g., typos or API changes since installation), the function returns an empty array without error. In cli.js, this increments successCount and produces zero output — the user gets no feedback and nothing was removed, yet the command appears to succeed.
Additional Locations (1)
|
Interesting. |
tinsever
changed the title
gfcli remove command to uninstall installed Google Fontsgfcli remove command to uninstall installed Google Fonts
2 participants
Summary
Adds a
removeCLI command that deletes font files previously installed withgfcli install, using the same per-user font directories and PascalCase basenames as installation.Implementation
systemFont.uninstall(fileBase)— Tries both.ttfand.woff2under the same paths used byinstall(Linux~/.local/share/fonts, macOS~/Library/Fonts, Windows%LOCALAPPDATA%\Microsoft\Windows\Fonts). Runsfc-cache -fon Linux when something was removed.googleFont.uninstallAsync(variants)— Resolves variants via the GWFH file map (same as install), then removes matching files.Usage
Tests
Extended
google-fontandsystem-fontunit tests;npm testandnpm run typecheckpass.Summary by CodeRabbit
Release Notes
New Features
removecommand for uninstalling Google Fonts from your system. Specify one or more font families to remove, with support for targeting specific variants. Works across Windows, macOS, and Linux platforms.Tests