fix: support fs.watch on non-recursive platforms and make better-sqlite3 optional#21
fix: support fs.watch on non-recursive platforms and make better-sqlite3 optional#21httpsVishu wants to merge 1 commit intovtemian:mainfrom
Conversation
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/providers/shared/watch.ts">
<violation number="1" location="src/providers/shared/watch.ts:36">
P2: Platform allowlist incorrectly disables recursive fs.watch on Linux runtimes that support it, causing potential missed nested file change events.</violation>
</file>
<file name="src/providers/opencode/provider.ts">
<violation number="1" location="src/providers/opencode/provider.ts:30">
P2: Using top-level CommonJS `require("better-sqlite3")` in an ESM-distributed package can fail in ESM runtime paths and silently disable the OpenCode provider.</violation>
</file>
<file name="tests/opencode-fixtures.ts">
<violation number="1" location="tests/opencode-fixtures.ts:6">
P2: Catching all errors when requiring `better-sqlite3` can hide real runtime/dependency failures by silently skipping sqlite tests.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| onEvent(); | ||
| }); | ||
| const isRecursiveSupported = process.platform === "darwin" || process.platform === "win32"; |
There was a problem hiding this comment.
P2: Platform allowlist incorrectly disables recursive fs.watch on Linux runtimes that support it, causing potential missed nested file change events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/shared/watch.ts, line 36:
<comment>Platform allowlist incorrectly disables recursive fs.watch on Linux runtimes that support it, causing potential missed nested file change events.</comment>
<file context>
@@ -33,12 +33,18 @@ export function createProviderWatch(
- }
- onEvent();
- });
+ const isRecursiveSupported = process.platform === "darwin" || process.platform === "win32";
+
+ watcher = fsWatch(
</file context>
| let betterSqlite3: any; | ||
|
|
||
| try { | ||
| betterSqlite3 = require("better-sqlite3"); |
There was a problem hiding this comment.
P2: Using top-level CommonJS require("better-sqlite3") in an ESM-distributed package can fail in ESM runtime paths and silently disable the OpenCode provider.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/opencode/provider.ts, line 30:
<comment>Using top-level CommonJS `require("better-sqlite3")` in an ESM-distributed package can fail in ESM runtime paths and silently disable the OpenCode provider.</comment>
<file context>
@@ -25,17 +24,25 @@ import { createOpenCodeDatabase, type OpenCodeDatabase, type SessionStats } from
+let betterSqlite3: any;
+
+try {
+ betterSqlite3 = require("better-sqlite3");
+} catch {
+ // Optional dependency not available
</file context>
|
|
||
| try { | ||
| Database = require("better-sqlite3"); | ||
| } catch { |
There was a problem hiding this comment.
P2: Catching all errors when requiring better-sqlite3 can hide real runtime/dependency failures by silently skipping sqlite tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/opencode-fixtures.ts, line 6:
<comment>Catching all errors when requiring `better-sqlite3` can hide real runtime/dependency failures by silently skipping sqlite tests.</comment>
<file context>
@@ -1,5 +1,13 @@
+
+try {
+ Database = require("better-sqlite3");
+} catch {
+ // optional dependency not available
+}
</file context>
vtemian
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The problems you're solving (Linux fs.watch compat, optional native deps) are real and valuable. However, the implementation needs some changes to align with the project's coding standards:
-
No
anytypes — the project strictly forbidsany(see CLAUDE.md). All theanyusages (betterSqlite3: any,_testDb?: any,db: any, etc.) need to be replaced with proper typing usingunknown+ type guards or conditional dynamic imports. -
require()in ESM —package.jsonhas"type": "module", so barerequire()won't work correctly. Useawait import("better-sqlite3")with a lazy init pattern instead. -
Silent
catch {}— the empty catch hides real errors (native build failures, ABI mismatch). Please narrow toMODULE_NOT_FOUNDand re-throw other errors. -
Missing warning for non-recursive watch — on Linux, only top-level directory changes are detected without recursive watch. The code should emit a warning when
recursive: trueis unavailable so users know about the limitation. -
Test describe string —
opencode-provider.test.tschanges the describe label to"..."(literal dots), losing the actual suite name.
Would love to see a v2 addressing these. The direction is right, just needs implementation cleanup.
What
Fixes cross-platform issues with
fs.watchand improves handling of optional dependencies.Why
While setting up and running the project on a Linux/WSL environment,
fs.watchwith recursive support caused runtime errors, andbetter-sqlite3being treated as required also broke tests when not installed. This PR ensures smoother setup and execution across platforms.Results
fs.watchoptionsChecklist
npm run checkpassesSummary by cubic
Fix cross-platform file watching by using non-recursive mode on Linux/WSL, and make
better-sqlite3optional to prevent setup failures when not installed. Tests now skip SQLite-only suites if the module is missing and skip recursive watch checks on unsupported platforms.fs.watchrecursion only ondarwinandwin32; avoids runtime errors on Linux/WSL.better-sqlite3via saferequire; provider/test types updated to accept an injected db.hasSqlite; skip recursive watch test where not supported.Written for commit 93e3ca3. Summary will update on new commits.