feat: remove unused packages and fix tsconfig #19
Conversation
WalkthroughRemove Vitest browser-specific packages and type references; add Vitest globals configuration and global test setup changes; introduce a Makefile. Core Vitest dependency remains. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
1-16: Consider adding conventional Makefile targets.While not required, adding conventional phony targets (
all,clean,test) improves discoverability and aligns with common Makefile patterns. You could alias them to your namespaced targets.🔎 Example additions:
.PHONY: all all: ci .PHONY: clean clean: clean/test .PHONY: test test: npm run test
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)tsconfig.json(1 hunks)vite.config.ts(1 hunks)vitest-setup.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsconfig.json
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 10-10: Missing required phony target "all"
(minphony)
[warning] 10-10: Missing required phony target "clean"
(minphony)
[warning] 10-10: Missing required phony target "test"
(minphony)
🔇 Additional comments (3)
Makefile (1)
10-16: CI orchestration looks good, pending fixes to clean targets.The CI workflow appropriately sequences cleanup, installation, build checks, and tests. However, this depends on resolving the issues in the clean targets (sudo usage and package-lock.json removal).
vitest-setup.js (1)
1-1: LGTM! The switch to standard Jest DOM matchers is correct and aligns with removing Vitest browser-specific packages. @testing-library/jest-dom is properly listed in devDependencies.vite.config.ts (1)
8-8: LGTM! Configuration is correct.The addition of
globals: trueenables global test APIs without imports and is properly paired with"vitest/globals"in the tsconfig.json types array.
| @@ -0,0 +1,16 @@ | |||
| .PHONY: clean/test | |||
| clean/test: | |||
| sudo rm -rf coverage build .svelte-kit test-results results-e2e.xml | |||
There was a problem hiding this comment.
Remove sudo from the clean command.
Using sudo in build/test scripts is a security and operational concern:
- It masks underlying permission problems that should be fixed at the source
- Coverage and build directories should not require elevated privileges
- CI environments should not need sudo for cleanup operations
If the directories are owned by root, fix the permission issue where they're created rather than elevating privileges to delete them.
🔎 Apply this diff to remove sudo:
- sudo rm -rf coverage build .svelte-kit test-results results-e2e.xml
+ rm -rf coverage build .svelte-kit test-results results-e2e.xml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo rm -rf coverage build .svelte-kit test-results results-e2e.xml | |
| rm -rf coverage build .svelte-kit test-results results-e2e.xml |
🤖 Prompt for AI Agents
In Makefile around line 3, the clean command uses sudo which should be removed;
update the line to remove the leading "sudo" so the command runs without
elevated privileges (e.g., change "sudo rm -rf ..." to "rm -rf ..."), and if CI
or local runs fail afterwards, ensure the directories are created with correct
ownership or change the creator steps to avoid root-owned artifacts rather than
reintroducing sudo.
| rm -rf node_modules | ||
| rm package-lock.json |
There was a problem hiding this comment.
Removing package-lock.json breaks reproducible builds.
The clean/node target removes package-lock.json, but this causes issues:
- The
citarget runsnpm ci, which requirespackage-lock.jsonto exist - Removing the lock file defeats the purpose of
npm ci(reproducible, deterministic installs) - This can lead to version drift and inconsistent builds
Consider either keeping the lock file or using npm install instead of npm ci (though npm ci is preferred for CI).
🔎 Apply this diff to preserve package-lock.json:
.PHONY: clean/node
clean/node:
rm -rf node_modules
- rm package-lock.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf node_modules | |
| rm package-lock.json | |
| rm -rf node_modules |
🤖 Prompt for AI Agents
In Makefile around lines 7-8, the clean/node target deletes package-lock.json
which breaks reproducible CI installs; remove the line that deletes
package-lock.json (or make it conditional via a CLEAN_LOCK variable) so the
lockfile is preserved and npm ci can run deterministically; alternatively, if
you really need to remove the lock in local cleans, add a separate target
(clean-local) that removes package-lock.json while keeping the default
clean/node only remove node_modules.
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.