-
Notifications
You must be signed in to change notification settings - Fork 85
feat(theme-check-vscode): surface orphaned files on startup #1218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| 'theme-check-vscode': minor | ||
| --- | ||
|
|
||
| Surface orphaned (dead) files on startup with a notification | ||
|
|
||
| When a theme has orphaned files, the VS Code extension now shows a single dismissable notification on startup with **Review** (opens the existing dead-code picker) and **Don't show again** actions, instead of requiring the user to run the dead-code command manually. Gated by the new `themeCheck.checkOrphanedFilesOnBoot` setting (default `true`). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import type { BaseLanguageClient } from 'vscode-languageclient'; | ||
|
|
||
| const mocks = vi.hoisted(() => ({ | ||
| showInformationMessage: vi.fn(), | ||
| showQuickPick: vi.fn(), | ||
| showTextDocument: vi.fn(), | ||
| openTextDocument: vi.fn(() => Promise.resolve({})), | ||
| executeCommand: vi.fn(), | ||
| activeTextEditor: { | ||
| document: { uri: { toString: () => 'file:///theme/layout/theme.liquid' } }, | ||
| } as any, | ||
| })); | ||
|
|
||
| vi.mock('vscode', () => ({ | ||
| window: { | ||
| get activeTextEditor() { | ||
| return mocks.activeTextEditor; | ||
| }, | ||
| showInformationMessage: mocks.showInformationMessage, | ||
| showQuickPick: mocks.showQuickPick, | ||
| showTextDocument: mocks.showTextDocument, | ||
| }, | ||
| workspace: { openTextDocument: mocks.openTextDocument }, | ||
| commands: { executeCommand: mocks.executeCommand }, | ||
| Uri: { parse: (s: string) => ({ toString: () => s }) }, | ||
| Position: class { | ||
| constructor( | ||
| public line: number, | ||
| public character: number, | ||
| ) {} | ||
| }, | ||
| Range: class { | ||
| constructor( | ||
| public start: any, | ||
| public end: any, | ||
| ) {} | ||
| }, | ||
| })); | ||
|
|
||
| import { makeDeadCode } from './commands'; | ||
|
|
||
| function makeClient(rootUri: string, deadCode: string[]): BaseLanguageClient { | ||
| return { | ||
| sendRequest: vi | ||
| .fn() | ||
| .mockResolvedValueOnce(rootUri) // ThemeGraphRootRequest | ||
| .mockResolvedValueOnce(deadCode), // ThemeGraphDeadCodeRequest | ||
| } as unknown as BaseLanguageClient; | ||
| } | ||
|
|
||
| describe('makeDeadCode (characterization — behavior must survive refactor)', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mocks.activeTextEditor = { | ||
| document: { uri: { toString: () => 'file:///theme/layout/theme.liquid' } }, | ||
| }; | ||
| }); | ||
|
|
||
| it('tells the user when there is no dead code', async () => { | ||
| const client = makeClient('file:///theme', []); | ||
|
|
||
| await makeDeadCode(client)(); | ||
|
|
||
| expect(mocks.showInformationMessage).toHaveBeenCalledWith('No dead code found.'); | ||
| expect(mocks.showQuickPick).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('offers a quick pick of the orphaned files when dead code is found', async () => { | ||
| const client = makeClient('file:///theme', [ | ||
| 'file:///theme/snippets/unused-a.liquid', | ||
| 'file:///theme/snippets/unused-b.liquid', | ||
| ]); | ||
|
|
||
| await makeDeadCode(client)(); | ||
|
|
||
| expect(mocks.showInformationMessage).not.toHaveBeenCalled(); | ||
| expect(mocks.showQuickPick).toHaveBeenCalledTimes(1); | ||
| const [items, options] = mocks.showQuickPick.mock.calls[0]; | ||
| expect(items).toHaveLength(2); | ||
| expect(options).toMatchObject({ canPickMany: true }); | ||
| }); | ||
|
|
||
| it('does nothing when there is no active editor', async () => { | ||
| mocks.activeTextEditor = undefined; | ||
| const client = makeClient('file:///theme', ['file:///theme/snippets/unused.liquid']); | ||
|
|
||
| await makeDeadCode(client)(); | ||
|
|
||
| expect(client.sendRequest).not.toHaveBeenCalled(); | ||
| expect(mocks.showInformationMessage).not.toHaveBeenCalled(); | ||
| expect(mocks.showQuickPick).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import type { BaseLanguageClient } from 'vscode-languageclient'; | ||
|
|
||
| const mocks = vi.hoisted(() => ({ | ||
| showInformationMessage: vi.fn(), | ||
| showQuickPick: vi.fn(), | ||
| showTextDocument: vi.fn(), | ||
| openTextDocument: vi.fn(() => Promise.resolve({})), | ||
| findFiles: vi.fn(), | ||
| getConfig: vi.fn(), | ||
| updateConfig: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock('vscode', () => ({ | ||
| window: { | ||
| showInformationMessage: mocks.showInformationMessage, | ||
| showQuickPick: mocks.showQuickPick, | ||
| showTextDocument: mocks.showTextDocument, | ||
| }, | ||
| workspace: { | ||
| getConfiguration: () => ({ get: mocks.getConfig, update: mocks.updateConfig }), | ||
| findFiles: mocks.findFiles, | ||
| openTextDocument: mocks.openTextDocument, | ||
| }, | ||
| commands: { executeCommand: vi.fn() }, | ||
| ConfigurationTarget: { Global: 1 }, | ||
| Uri: { parse: (s: string) => ({ toString: () => s }) }, | ||
| Position: class { | ||
| constructor( | ||
| public line: number, | ||
| public character: number, | ||
| ) {} | ||
| }, | ||
| Range: class { | ||
| constructor( | ||
| public start: any, | ||
| public end: any, | ||
| ) {} | ||
| }, | ||
| })); | ||
|
|
||
| import { checkOrphanedFilesOnBoot } from './orphanedFilesOnBoot'; | ||
|
|
||
| function uriLike(s: string) { | ||
| return { toString: () => s } as any; | ||
| } | ||
|
|
||
| /** sendRequest resolves root first, then dead code (matches fetchDeadCode's Promise.all order). */ | ||
| function clientWith(rootUri: string, deadCode: string[]): BaseLanguageClient { | ||
| return { | ||
| sendRequest: vi.fn().mockResolvedValueOnce(rootUri).mockResolvedValueOnce(deadCode), | ||
| } as unknown as BaseLanguageClient; | ||
| } | ||
|
|
||
| describe('checkOrphanedFilesOnBoot', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mocks.getConfig.mockReturnValue(true); | ||
| }); | ||
|
|
||
| it('notifies the user when the theme has orphaned files', async () => { | ||
| mocks.findFiles.mockResolvedValue([uriLike('file:///theme/.theme-check.yml')]); | ||
| const client = clientWith('file:///theme', ['file:///theme/snippets/unused.liquid']); | ||
|
|
||
| await checkOrphanedFilesOnBoot(client); | ||
|
|
||
| expect(mocks.showInformationMessage).toHaveBeenCalledTimes(1); | ||
| expect(mocks.showInformationMessage.mock.calls[0][0]).toContain('1 orphaned file'); | ||
| }); | ||
|
|
||
| it('does not notify when there are no orphaned files', async () => { | ||
| mocks.findFiles.mockResolvedValue([uriLike('file:///theme/.theme-check.yml')]); | ||
| const client = clientWith('file:///theme', []); | ||
|
|
||
| await checkOrphanedFilesOnBoot(client); | ||
|
|
||
| expect(mocks.showInformationMessage).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does nothing when the setting is disabled', async () => { | ||
| mocks.getConfig.mockReturnValue(false); | ||
| const client = clientWith('file:///theme', ['file:///theme/snippets/unused.liquid']); | ||
|
|
||
| await checkOrphanedFilesOnBoot(client); | ||
|
|
||
| expect(mocks.findFiles).not.toHaveBeenCalled(); | ||
| expect(client.sendRequest).not.toHaveBeenCalled(); | ||
| expect(mocks.showInformationMessage).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('disables the setting when the user picks "Don\'t show again"', async () => { | ||
| mocks.findFiles.mockResolvedValue([uriLike('file:///theme/.theme-check.yml')]); | ||
| mocks.showInformationMessage.mockResolvedValue("Don't show again"); | ||
| const client = clientWith('file:///theme', ['file:///theme/snippets/unused.liquid']); | ||
|
|
||
| await checkOrphanedFilesOnBoot(client); | ||
|
|
||
| expect(mocks.updateConfig).toHaveBeenCalledWith( | ||
| 'themeCheck.checkOrphanedFilesOnBoot', | ||
| false, | ||
| 1, // ConfigurationTarget.Global | ||
| ); | ||
| }); | ||
|
|
||
| it('opens the dead-code picker when the user picks "Review"', async () => { | ||
| mocks.findFiles.mockResolvedValue([uriLike('file:///theme/.theme-check.yml')]); | ||
| mocks.showInformationMessage.mockResolvedValue('Review'); | ||
| mocks.showQuickPick.mockResolvedValue(undefined); | ||
| const client = clientWith('file:///theme', ['file:///theme/snippets/unused.liquid']); | ||
|
|
||
| await checkOrphanedFilesOnBoot(client); | ||
|
|
||
| expect(mocks.showQuickPick).toHaveBeenCalledTimes(1); | ||
| expect(mocks.updateConfig).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { window, workspace, ConfigurationTarget } from 'vscode'; | ||
| import { BaseLanguageClient } from 'vscode-languageclient'; | ||
| import { fetchDeadCode, showDeadCodePicker } from './commands'; | ||
|
|
||
| export const CHECK_ORPHANED_ON_BOOT_SETTING = 'themeCheck.checkOrphanedFilesOnBoot'; | ||
|
|
||
| const REVIEW = 'Review'; | ||
| const DONT_SHOW_AGAIN = "Don't show again"; | ||
|
|
||
| /** | ||
| * On extension startup, surface orphaned (dead) files with a single dismissable | ||
| * notification instead of requiring the user to run the dead-code command. Gated | ||
| * by the `themeCheck.checkOrphanedFilesOnBoot` setting. Aggregates across every | ||
| * theme root in the workspace; "Review" opens the picker for the first root that | ||
| * has orphaned files. | ||
| */ | ||
| export async function checkOrphanedFilesOnBoot(client: BaseLanguageClient): Promise<void> { | ||
| const enabled = workspace.getConfiguration().get(CHECK_ORPHANED_ON_BOOT_SETTING, true); | ||
| if (!enabled) return; | ||
|
|
||
| const configFiles = await workspace.findFiles('**/.theme-check.yml', '**/node_modules/**'); | ||
|
|
||
| let total = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This aggregates total across all config files, but line 42 opens the picker only for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 2d33a1f. The check now dedupes the resolved roots and prompts once per theme that has orphaned files, so each notification's count matches the files its own "Review" picker opens — no more cross-theme aggregation. Your two-theme workspace would now get one notification per theme. Added unit tests for the multi-theme and dedupe cases. |
||
| let firstHit: { rootUri: string; deadCode: string[] } | undefined; | ||
| for (const file of configFiles) { | ||
| const { rootUri, deadCode } = await fetchDeadCode(client, file.toString()); | ||
| if (deadCode.length > 0) { | ||
| total += deadCode.length; | ||
| if (!firstHit) firstHit = { rootUri, deadCode }; | ||
| } | ||
| } | ||
|
|
||
| if (total === 0 || !firstHit) return; | ||
|
|
||
| const choice = await window.showInformationMessage( | ||
| `Found ${total} orphaned file${total === 1 ? '' : 's'} in your theme.`, | ||
| REVIEW, | ||
| DONT_SHOW_AGAIN, | ||
| ); | ||
|
|
||
| if (choice === REVIEW) { | ||
| await showDeadCodePicker(firstHit.rootUri, firstHit.deadCode); | ||
| } else if (choice === DONT_SHOW_AGAIN) { | ||
| await workspace | ||
| .getConfiguration() | ||
| .update(CHECK_ORPHANED_ON_BOOT_SETTING, false, ConfigurationTarget.Global); | ||
| } | ||
| } | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Startup check misses supported theme roots. This only scans
**/.theme-check.yml, but the existing root detector also supportsshopify.extension.tomlroots and configless themes inferred fromassets+snippets(seefind-root.ts(line 7)). Those workspaces can activate the extension but will never get this new orphaned-file prompt.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — fixed in 2d33a1f. The startup scan now globs every root signal
findRootrecognises ({.theme-check.yml,shopify.extension.toml,snippets/*}) instead of just.theme-check.yml, and each match is resolved to its root via the server'sthemeGraph/rootUrirequest, so theme app extensions and configless themes get the prompt too.