chore: address PR #414/#415 review feedback#417
Conversation
- ci.yml: pin actions/checkout & actions/setup-node to commit SHAs and add a least-privilege `permissions: contents: read` block (CodeRabbit / SEAL). - workerFactory.test.ts: use Object.defineProperty for readonly VS Code mock fields instead of `as any` (the project's documented convention). - native_worker_ddl_batch.ts: use the real `bundle.establishConnection(...)` API (the benchmark called a non-existent `loadDatabase` masked by `as any`). - globals.css: fix the self-referential `@theme` font variables (via `--font-*-stack`) and move `:root`/`.dark` + base styles out of `@layer utilities` into the correct layers (Gemini). Verified: extension build OK; `tsc --noEmit` 0 errors; `npm test` 311 pass; website `next build` compiles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 23 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the native worker DDL batch benchmark to use a unique local SQLite database file and ensures cleanup in a finally block. It also refactors how VS Code mocks are reset in unit tests using Object.defineProperty, and reorganizes global CSS variables and utilities in the website styling. Feedback on the changes highlights a potential race condition when deleting the database file before the worker process fully releases its lock, and warns about test pollution due to modifying shared VS Code mock singletons without restoring them in afterEach.
| } finally { | ||
| bundle.workerMethods[Symbol.dispose](); | ||
| await fs.rm(databasePath, { force: true }); | ||
| } |
There was a problem hiding this comment.
There is a potential race condition when deleting the database file immediately after calling bundle.workerMethods[Symbol.dispose]().
The Symbol.dispose call triggers worker.stop(), which asynchronously kills the child process (this.process.kill()). Since process termination and file descriptor release by the OS are asynchronous, fs.rm might execute while the file is still locked by the terminating process (especially on Windows). This can cause the benchmark to fail with EBUSY or EPERM errors.
To make this robust, we can implement a simple retry loop with a small delay to ensure the file is successfully deleted once the lock is released.
} finally {
bundle.workerMethods[Symbol.dispose]();
// Allow the worker process some time to release the file lock (especially on Windows)
for (let i = 0; i < 5; i++) {
try {
await fs.rm(databasePath, { force: true });
break;
} catch (err) {
if (i === 4) throw err;
await new Promise(resolve => setTimeout(resolve, 100));
}
}
}| Object.defineProperty(mockVscode.workspace, 'fs', { | ||
| value: { | ||
| readFile: async () => new Uint8Array(), | ||
| stat: async () => ({ size: 0 }) | ||
| }, | ||
| writable: true, | ||
| configurable: true | ||
| }); | ||
| Object.defineProperty(mockVscode.Uri, 'joinPath', { | ||
| value: () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' }), | ||
| writable: true, | ||
| configurable: true | ||
| }); |
There was a problem hiding this comment.
Modifying shared mock objects like mockVscode.workspace and mockVscode.Uri in beforeEach without restoring them in afterEach can lead to test pollution, causing other tests in the suite to fail or behave unpredictably.
Since mockVscode is imported from a shared mock module (./mocks/vscode), it is a singleton cached by Node's module system. Any mutations to it will persist across test files.
To prevent this, we should capture the original properties before modifying them and restore them in afterEach. Since afterEach is not in the diff, you can implement this by storing the original values and restoring them. For example:
let originalFs: any;
let originalJoinPath: any;
beforeEach(() => {
connectionFailed = false;
workerTerminated = false;
originalFs = mockVscode.workspace.fs;
originalJoinPath = mockVscode.Uri.joinPath;
// Reset VSCode mock behaviors
Object.defineProperty(mockVscode.workspace, 'fs', {
value: {
readFile: async () => new Uint8Array(),
stat: async () => ({ size: 0 })
},
writable: true,
configurable: true
});
Object.defineProperty(mockVscode.Uri, 'joinPath', {
value: () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' }),
writable: true,
configurable: true
});
});
afterEach(() => {
// Restore original mock behaviors
if (originalFs) {
Object.defineProperty(mockVscode.workspace, 'fs', {
value: originalFs,
writable: true,
configurable: true
});
}
if (originalJoinPath) {
Object.defineProperty(mockVscode.Uri, 'joinPath', {
value: originalJoinPath,
writable: true,
configurable: true
});
}
// Restore the original require implementation to avoid leaking to other tests
Module.prototype.require = originalRequire;
});
1 participant
Applies the valid findings from the CodeRabbit/Gemini reviews on #414 and #415 (implemented via Codex, verified locally).
#414 (CI / tests):
ci.yml: pinactions/checkout&actions/setup-nodeto commit SHAs + add least-privilegepermissions: contents: read(supply-chain hardening)workerFactory.test.ts:Object.definePropertyfor readonly VS Code mock fields instead ofas any(project convention)native_worker_ddl_batch.ts: use the realbundle.establishConnection(...)API instead of a non-existentloadDatabasehidden byas any#415 (Tailwind v4):
globals.css: fix self-referential@themefont vars (via--font-*-stack); move:root/.dark+ base styles out of@layer utilitiesinto the correct layersVerified: extension build OK ·
tsc --noEmit0 errors ·npm test311 pass · websitenext buildcompiles. (Skipped the cosmeticbg-(--ui-bg)→bg-backgroundsuggestion — works as-is, large churn.)🤖 Generated with Claude Code