Fix release pipeline node_modules bug, add test coverage, close cache leaks - #1
Merged
Merged
Conversation
…age, close cache leaks - Narrow the semantic-release git asset glob from `plugins/**/package.json` to `plugins/*/*/package.json` so it no longer recurses into node_modules; untrack the node_modules files it had already re-committed on the last release. - Add vitest + a real unit test suite (40 tests) for the plugin's optimizer/cache-identifier/config-merge logic, and wire root's `npm run test` to actually run it (CI's `--if-present` test step was previously a silent no-op with no test script anywhere). - Fix leaks in ImageCacheService: evict stale Cache API + localStorage entries instead of leaving them to accumulate forever, and revoke a blob URL before overwriting it in the registry. - Fix two pre-existing eslint errors (unused catch binding and any[] logger args) surfaced while getting lint green again. - Align the plugin's vite/vue-tsc/vite-plugin-dts/@vitejs/plugin-vue devDependencies with the workspace root instead of trailing several majors behind; fixes needed along the way: vite-plugin-dts v5 renamed rollupTypes to bundleTypes, needs @vue/language-core as a resolvable peer, and TS 6 rejects the unused baseUrl tsconfig option. - Rewrite the root README to describe this repo (home of @greener-games/image-optimiser) instead of a stale generic boilerplate template whose setup script no longer exists. Verified: plugin build, root build, full test suite, eslint, and stylelint all pass clean.
Vitest is now configured and passing for the plugin, so the old "tests only on request" rule is stale. Replace it with guidance to keep tests updated alongside the code they cover, and list the test command alongside the other common commands.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request adds Vitest support and broad test coverage, updates image cache eviction and Blob URL handling, makes Hygraph optimization synchronous, tightens logger types, and updates repository and release configuration. ChangesImage optimiser testing and runtime updates
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🏥 NPM Health Check ReportNPM Audit ResultsNPM Outdated Packages |
![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
🧹 Nitpick comments (3)
README.md (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new test command in the root README.
The README now describes the repository, but it does not expose the new root
npm run testworkflow. Add a short Testing section so users can discover the Vitest suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 1 - 8, Add a short Testing section to the root README describing the root npm run test command and indicating that it runs the Vitest suite.plugins/@greener-games/image-optimiser/tsconfig.json (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType-check the Vitest files separately.
This exclusion removes all
src/**/*.test.tsfiles from the package TypeScript check. Keep tests out of declaration output if required, but add a test-specifictsconfigor type-check command so test-only type errors cannot pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/`@greener-games/image-optimiser/tsconfig.json at line 15, Add a test-specific TypeScript configuration or type-check command for the Vitest files excluded by the package tsconfig.json, ensuring all src/**/*.test.ts files are type-checked separately while remaining excluded from declaration output.plugins/@greener-games/image-optimiser/src/services/imageCache.test.ts (1)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert stale sibling metadata removal.
This test verifies Cache API eviction only. It does not verify removal of
cache_meta_${largeUrl}. Add an assertion that this key isnullaftergetImageUrl(smallUrl).Proposed test update
expect(fakeCache.delete).toHaveBeenCalledWith(largeUrl); + expect(localStorage.getItem(`cache_meta_${largeUrl}`)).toBeNull(); expect(fetchMock).toHaveBeenCalledOnce();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/`@greener-games/image-optimiser/src/services/imageCache.test.ts at line 153, Add an assertion to the test around getImageUrl(smallUrl) confirming that the sibling metadata key cache_meta_${largeUrl} is null after eviction, while retaining the existing fakeCache.delete assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/`@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts:
- Around line 29-36: Update the HygraphOptimizer.canHandle SVG test fixture to
use a content type with parameters, such as an SVG media type followed by a
charset, and normalize the response content type to the media type before
comparing it with the SVG type. Preserve rejection of SVG responses while
allowing parameterized content-type values to be detected.
- Around line 8-11: Update HygraphOptimizer to parse URLs and allow HEAD
requests and transformations only for explicitly allow-listed hostnames,
preventing deceptive path or hostname substring matches; add tests in
plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts
lines 8-11 for deceptive hosts. Apply the same parsed-hostname allow-list
validation to HygraphCacheIdentifier and add corresponding deceptive-host tests
in
plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphCacheIdentifier.test.ts
lines 7-11.
In `@plugins/`@greener-games/image-optimiser/src/services/imageCache.ts:
- Around line 150-156: Update getImageUrl and blobGuiRegistry entries to retain
the cache source URL or expiry metadata, checking expiration before returning an
existing registry entry; revoke and remove expired entries, then continue
through cache lookup and fetchAndCache. When a smart-cache match creates a
registry entry, pass its sibling cache URL so isStale and evictCacheEntry use
the correct cache record. Add a regression test that advances the entry beyond
expiration and verifies refetching and URL.revokeObjectURL.
In
`@plugins/`@greener-games/image-optimiser/src/services/MediaOptimizerConfig.test.ts:
- Around line 35-42: Update the public input type for setupMediaOptimizerConfig
so imageSizeMap accepts Partial<Record<ImageSize, TransformOptions>>, allowing
partial per-size entries. Then remove the double cast from the partialMap
fixture in the related test while preserving the existing merge behavior.
---
Nitpick comments:
In `@plugins/`@greener-games/image-optimiser/src/services/imageCache.test.ts:
- Line 153: Add an assertion to the test around getImageUrl(smallUrl) confirming
that the sibling metadata key cache_meta_${largeUrl} is null after eviction,
while retaining the existing fakeCache.delete assertion.
In `@plugins/`@greener-games/image-optimiser/tsconfig.json:
- Line 15: Add a test-specific TypeScript configuration or type-check command
for the Vitest files excluded by the package tsconfig.json, ensuring all
src/**/*.test.ts files are type-checked separately while remaining excluded from
declaration output.
In `@README.md`:
- Around line 1-8: Add a short Testing section to the root README describing the
root npm run test command and indicating that it runs the Vitest suite.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e23b38ba-8ba0-47b6-a137-f59258308211
⛔ Files ignored due to path filters (19)
package-lock.jsonis excluded by!**/package-lock.jsonplugins/@greener-games/image-optimiser/node_modules/@esbuild/linux-x64/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/@vitejs/plugin-vue/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/@volar/language-core/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/@volar/source-map/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/@volar/typescript/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/@vue/language-core/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/alien-signals/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/brace-expansion/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/esbuild/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/minimatch/dist/commonjs/package.jsonis excluded by!**/dist/**,!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/minimatch/dist/esm/package.jsonis excluded by!**/dist/**,!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/minimatch/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/vite-plugin-dts/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/vite/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/vite/types/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/vue-tsc/node_modules/@vue/language-core/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/vue-tsc/node_modules/alien-signals/package.jsonis excluded by!**/node_modules/**plugins/@greener-games/image-optimiser/node_modules/vue-tsc/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (17)
.releaserc.jsonREADME.mdgemini.mdpackage.jsonplugins/@greener-games/image-optimiser/package.jsonplugins/@greener-games/image-optimiser/src/providers/default/DefaultFallback.test.tsplugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphCacheIdentifier.test.tsplugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.tsplugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.tsplugins/@greener-games/image-optimiser/src/services/Logger.tsplugins/@greener-games/image-optimiser/src/services/MediaOptimizerConfig.test.tsplugins/@greener-games/image-optimiser/src/services/imageCache.test.tsplugins/@greener-games/image-optimiser/src/services/imageCache.tsplugins/@greener-games/image-optimiser/src/services/imageOptimizer.test.tsplugins/@greener-games/image-optimiser/tsconfig.jsonplugins/@greener-games/image-optimiser/vite.config.tsplugins/@greener-games/image-optimiser/vitest.config.ts
Comment on lines
+8
to
+11
| it('rejects urls from unrelated hosts', async () => { | ||
| const optimizer = new HygraphOptimizer(); | ||
| expect(await optimizer.canHandle('https://example.com/a.jpg')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use hostname-based Hygraph provider selection.
Both implementations use substring matching. Unrelated URLs can enter Hygraph handling when their path or hostname contains a supported-host string.
plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts#L8-L11: Add deceptive-host tests. Gate HEAD requests and transformations on parsed, allow-listed hostnames.plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphCacheIdentifier.test.ts#L7-L11: Add deceptive-host tests. Gate cache identification on the same parsed, allow-listed hostnames.
📍 Affects 2 files
plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts#L8-L11(this comment)plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphCacheIdentifier.test.ts#L7-L11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/`@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts
around lines 8 - 11, Update HygraphOptimizer to parse URLs and allow HEAD
requests and transformations only for explicitly allow-listed hostnames,
preventing deceptive path or hostname substring matches; add tests in
plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts
lines 8-11 for deceptive hosts. Apply the same parsed-hostname allow-list
validation to HygraphCacheIdentifier and add corresponding deceptive-host tests
in
plugins/@greener-games/image-optimiser/src/providers/hygraph/HygraphCacheIdentifier.test.ts
lines 7-11.
Comment on lines
+29
to
+36
| it('rejects a hygraph url whose content-type is svg', async () => { | ||
| vi.spyOn(global, 'fetch').mockResolvedValue({ | ||
| headers: new Headers({ 'content-type': 'image/svg+xml' }), | ||
| } as Response); | ||
|
|
||
| const optimizer = new HygraphOptimizer(); | ||
| expect(await optimizer.canHandle('https://cdn.hygraph.com/xyz789')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the content type before SVG detection.
A valid response can return image/svg+xml; charset=utf-8. The strict equality check in HygraphOptimizer.canHandle treats that response as non-SVG and permits optimization.
Add this content type to the test. Compare the normalized media type before the semicolon.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/`@greener-games/image-optimiser/src/providers/hygraph/HygraphOptimizer.test.ts
around lines 29 - 36, Update the HygraphOptimizer.canHandle SVG test fixture to
use a content type with parameters, such as an SVG media type followed by a
charset, and normalize the response content type to the media type before
comparing it with the SVG type. Preserve rejection of SVG responses while
allowing parameterized content-type values to be detected.
Comment on lines
+150
to
156
| const existing = this.blobGuiRegistry.get(originalUrl); | ||
| if (existing) { | ||
| URL.revokeObjectURL(existing); | ||
| } | ||
| const blobUrl = URL.createObjectURL(blob); | ||
| this.blobGuiRegistry.set(originalUrl, blobUrl); | ||
| return blobUrl; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply expiry before returning a registered Blob URL.
getImageUrl returns an existing blobGuiRegistry entry before it calls fetchAndCache. Therefore, after the first successful request, the same URL cannot reach isStale, evictCacheEntry, or this replacement-time revocation path.
Store the cache source URL or expiry data with each registry entry. Before the registry early return, revoke and remove an expired entry, then continue to cache lookup. Pass the sibling cache URL when a smart-cache match creates the registry entry. Add a regression test that advances the cached entry past expiration and verifies both a refetch and URL.revokeObjectURL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/`@greener-games/image-optimiser/src/services/imageCache.ts around
lines 150 - 156, Update getImageUrl and blobGuiRegistry entries to retain the
cache source URL or expiry metadata, checking expiration before returning an
existing registry entry; revoke and remove expired entries, then continue
through cache lookup and fetchAndCache. When a smart-cache match creates a
registry entry, pass its sibling cache URL so isStale and evictCacheEntry use
the correct cache record. Add a regression test that advances the entry beyond
expiration and verifies refetching and URL.revokeObjectURL.
Comment on lines
+35
to
+42
| it('merges a partial imageSizeMap with the defaults instead of replacing it', () => { | ||
| // setupMediaOptimizerConfig merges per-key at runtime even though the | ||
| // public type declares a full map; cast to reflect that real usage. | ||
| const partialMap = { m: { width: 42 } } as Partial<Record<ImageSize, TransformOptions>> as Record< | ||
| ImageSize, | ||
| TransformOptions | ||
| >; | ||
| setupMediaOptimizerConfig({ imageSizeMap: partialMap }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose partial imageSizeMap input in the public type.
The double cast shows that setupMediaOptimizerConfig supports partial maps at runtime but rejects them in TypeScript. Partial only makes imageSizeMap optional. It does not make its entries optional.
Change the input type to use imageSizeMap?: Partial<Record<ImageSize, TransformOptions>>. Remove the unsafe cast from this test.
Proposed type direction
+type MediaOptimizerConfigInput =
+ Partial<Omit<MediaOptimizerConfigData, 'imageSizeMap' | 'optimizers' | 'cacheIdentifiers'>> & {
+ imageSizeMap?: Partial<Record<ImageSize, TransformOptions>>;
+ optimizers?: ICmsOptimizer[];
+ cacheIdentifiers?: ICmsCacheIdentifier[];
+ };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/`@greener-games/image-optimiser/src/services/MediaOptimizerConfig.test.ts
around lines 35 - 42, Update the public input type for setupMediaOptimizerConfig
so imageSizeMap accepts Partial<Record<ImageSize, TransformOptions>>, allowing
partial per-size entries. Then remove the double cast from the partialMap
fixture in the related test while preserving the existing merge behavior.
Move all *.test.ts files out of src/ into a tests/ folder that mirrors src/'s structure, so tests aren't interleaved with the source files. Update the relative imports, vitest.config.ts's include glob, and drop the now-unneeded src/**/*.test.ts exclude from tsconfig.json.
🏥 NPM Health Check ReportNPM Audit ResultsNPM Outdated Packages |
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
2 participants
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
Follow-up to a technical debt review of the repo. Fixes a real, reproducible bug in the release pipeline, adds real test coverage where there was none, and closes a couple of memory/storage leaks in the image cache.
.releaserc.json's@semantic-release/gitasset glob wasplugins/**/package.json, which recurses intonode_modules(globby ignores.gitignore). A prior commit (02ee976) untracked 19node_modules/**/package.jsonfiles, and the very next automated release commit (582ad35) silently re-added the same files. Narrowed the glob toplugins/*/*/package.json(matches only the plugin's own manifest) and untracked the re-added files.npm run test --if-presentran against a repo with notestscript and no test files anywhere. Added vitest with a 40-test suite covering the optimizer/cache-identifier/config-merge logic, and wired rootnpm run testto run it via workspaces.ImageCacheService: stale Cache API entries and theirlocalStoragemetadata were detected but never deleted, so both grew unbounded forever. Now evicted when found stale. Also revoke a blob URL before overwriting it in the in-memory registry.vite/vue-tsc/vite-plugin-dts/@vitejs/plugin-vuewere several majors behind the workspace root, risking building/testing against different tooling than CI's root install exercises. Aligned them, which required a few follow-on fixes:vite-plugin-dtsv5 renamedrollupTypes→bundleTypes, needs@vue/language-coreas a resolvable dependency, and TypeScript 6 rejects the plugin's unusedbaseUrltsconfig option.any[]logger args) fixed while getting lint green again.README.mdrewritten: it previously documented a generic scaffolding template (node .templateScripts/setup.cjs) that no longer exists in this repo. Now it actually describes this repo as the home of@greener-games/image-optimiserand points to the plugin's real docs.gemini.mdupdated: replaced the "tests only on request" stipulation with guidance to keep tests updated alongside the code they cover, now that there's a real, working test setup to maintain.Test plan
npm run build(root — builds plugin workspace + app) passesnpm run test(40 tests across 6 files) passesnpm run lint:eslintpassesnpm run lint:stylespassesnpm run type-checkpassesGenerated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests