fix: don't fail extension activation when a workspace folder has no dbt project#1939
fix: don't fail extension activation when a workspace folder has no dbt project#1939ralphstodomingo wants to merge 3 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughretryWithBackoff now throws a NoProjectsFound sentinel for empty-array results (including after exhausting retries); discoverProjects catches that sentinel, treats the folder as empty without sending discoverProjectsError telemetry, and new Jest tests cover empty, timing, multi-root, and real-error behaviors. ChangesEmpty-result discovery DAG
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Bundle Size Reportdarwin-arm64: 74.2 MB
linux-x64: 75.9 MB
win32-x64: 76.8 MB
|
ralphstodomingo
force-pushed
the
fix/multi-root-empty-folder-no-projects-found
branch
from
cc06630 to
5b01b74
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/suite/discoverProjectsRepro.test.ts (2)
79-155: ⚖️ Poor tradeoffConsider fake timers to avoid ~40 s suite overhead
All four tests run the real
setTimeout-based backoff (1 + 2 + 3 + 4 = 10 s per test). Only test 2 ("retry budget is still ~10s") has a semantic need for wall-clock elapsed time; the other three tests assert on call counts and resolve/reject behavior, not on timing. Usingjest.useFakeTimers()for those three would make them instantaneous and eliminate the flakiness risk on loaded CI runners.♻️ Sketch — fake timers for test 1 (call-count assertion only)
- it("returns no projects (does not throw) when findFiles is consistently empty", async () => { + it("returns no projects (does not throw) when findFiles is consistently empty", async () => { + jest.useFakeTimers(); const findFilesMock = stubWorkspaceFindFiles(() => Promise.resolve([] as Uri[]), ); const folder = makeFolder(); - await expect(folder.discoverProjects()).resolves.toBeUndefined(); + const promise = folder.discoverProjects(); + // Advance through all backoff waits (1 + 2 + 3 + 4 = 10 s) + await jest.runAllTimersAsync(); + await expect(promise).resolves.toBeUndefined(); expect(findFilesMock).toHaveBeenCalledTimes(5); - }, 20000); + jest.useRealTimers(); + });🤖 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 `@src/test/suite/discoverProjectsRepro.test.ts` around lines 79 - 155, Replace real timers with Jest fake timers in the tests that only assert call counts/behavior (the first "returns no projects...", the third "multi-root: empty folder...", and the fourth "real findFiles errors...") by calling jest.useFakeTimers() at the start of each such it-block (or in a beforeEach scoped to those tests), invoke folder.discoverProjects() (or Promise.all(...) for the multi-root case), then advance timers to cover the backoff schedule (e.g. jest.advanceTimersByTime(10000) or runAllTimers()) so the internal setTimeout backoffs in discoverProjects run instantly, assert on stubWorkspaceFindFiles and results, and finally restore timers with jest.useRealTimers(); keep the second test ("retry budget is still ~10s") using real timers unchanged. Ensure you reference the existing helpers stubWorkspaceFindFiles, makeFolder, and the method discoverProjects when updating the tests.
108-109: ⚡ Quick winTiming upper-bound of 12 000 ms may be tight on throttled CI runners
The lower bound (9 500 ms) is intentional, but a 2 s ceiling can be breached when the scheduler is heavily loaded or the container is CPU-constrained. Consider raising the upper bound or documenting the acceptable CI environment.
🛡️ Suggested adjustment
- expect(elapsed).toBeLessThan(12000); + expect(elapsed).toBeLessThan(15000); // 5 s headroom for throttled CI🤖 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 `@src/test/suite/discoverProjectsRepro.test.ts` around lines 108 - 109, The upper bound on elapsed in the test's assertions (the expect(elapsed).toBeLessThan(12000) line in the discoverProjectsRepro test) is too tight for throttled CI; update the expectation to a higher ceiling (e.g., toBeLessThan(20000) or toBeLessThanOrEqual(15000)) or replace the strict upper-bound with a looser one that accommodates loaded runners, keeping the intentional lower bound (expect(elapsed).toBeGreaterThanOrEqual(9500)) unchanged.src/dbt_client/dbtWorkspaceFolder.ts (1)
88-144: 💤 Low valueLGTM — fix is correct and well-scoped.
The empty-array retry path is logically sound:
- Backoff schedule is identical to the pre-fix error path (
backoff * attemptafter increment), so the timing contract is preserved.- The post-loop
return lastEmpty as Tis safe in practice since this private method is always called withretries = 5, but worth noting thatlastEmptyremainsundefinedwhenretries ≤ 0, making the cast technically unsound for that edge case.🛡️ Optional defensive fix for the type-cast edge case
- return lastEmpty as T; + return (lastEmpty ?? []) as T;🤖 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 `@src/dbt_client/dbtWorkspaceFolder.ts` around lines 88 - 144, The retryWithBackoff method can return lastEmpty as T which is undefined when retries ≤ 0; update retryWithBackoff to defensively handle that edge-case by checking if lastEmpty is undefined before the final return and throwing a clear error (or returning an appropriate default) instead of casting; modify the end of retryWithBackoff to: if (lastEmpty === undefined) throw new Error("retryWithBackoff: no successful attempts/empty result not recorded"); otherwise return lastEmpty as T — reference function retryWithBackoff, variable lastEmpty, and the final dbtTerminal.debug call to locate the change.
🤖 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.
Nitpick comments:
In `@src/dbt_client/dbtWorkspaceFolder.ts`:
- Around line 88-144: The retryWithBackoff method can return lastEmpty as T
which is undefined when retries ≤ 0; update retryWithBackoff to defensively
handle that edge-case by checking if lastEmpty is undefined before the final
return and throwing a clear error (or returning an appropriate default) instead
of casting; modify the end of retryWithBackoff to: if (lastEmpty === undefined)
throw new Error("retryWithBackoff: no successful attempts/empty result not
recorded"); otherwise return lastEmpty as T — reference function
retryWithBackoff, variable lastEmpty, and the final dbtTerminal.debug call to
locate the change.
In `@src/test/suite/discoverProjectsRepro.test.ts`:
- Around line 79-155: Replace real timers with Jest fake timers in the tests
that only assert call counts/behavior (the first "returns no projects...", the
third "multi-root: empty folder...", and the fourth "real findFiles errors...")
by calling jest.useFakeTimers() at the start of each such it-block (or in a
beforeEach scoped to those tests), invoke folder.discoverProjects() (or
Promise.all(...) for the multi-root case), then advance timers to cover the
backoff schedule (e.g. jest.advanceTimersByTime(10000) or runAllTimers()) so the
internal setTimeout backoffs in discoverProjects run instantly, assert on
stubWorkspaceFindFiles and results, and finally restore timers with
jest.useRealTimers(); keep the second test ("retry budget is still ~10s") using
real timers unchanged. Ensure you reference the existing helpers
stubWorkspaceFindFiles, makeFolder, and the method discoverProjects when
updating the tests.
- Around line 108-109: The upper bound on elapsed in the test's assertions (the
expect(elapsed).toBeLessThan(12000) line in the discoverProjectsRepro test) is
too tight for throttled CI; update the expectation to a higher ceiling (e.g.,
toBeLessThan(20000) or toBeLessThanOrEqual(15000)) or replace the strict
upper-bound with a looser one that accommodates loaded runners, keeping the
intentional lower bound (expect(elapsed).toBeGreaterThanOrEqual(9500))
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9009ac56-455a-4dbb-b25e-a69b05315e25
📒 Files selected for processing (2)
src/dbt_client/dbtWorkspaceFolder.tssrc/test/suite/discoverProjectsRepro.test.ts
ralphstodomingo
force-pushed
the
fix/multi-root-empty-folder-no-projects-found
branch
from
5b01b74 to
778f0b0
Compare
Reply to @coderabbitai reviewTriaged the three nitpicks:
|
|
Tip For best results, initiate chat on the files or code changes.
( ˘ᵕ˘)🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| // Defensive `?? []` covers the theoretical `retries <= 0` case where the | ||
| // loop never runs and `lastEmpty` stays undefined. The private callers | ||
| // only pass `retries = 5` today, but keeping the cast sound costs | ||
| // nothing. | ||
| return (lastEmpty ?? []) as T; |
There was a problem hiding this comment.
| // Defensive `?? []` covers the theoretical `retries <= 0` case where the | |
| // loop never runs and `lastEmpty` stays undefined. The private callers | |
| // only pass `retries = 5` today, but keeping the cast sound costs | |
| // nothing. | |
| return (lastEmpty ?? []) as T; | |
| return []; |
There was a problem hiding this comment.
Applied — dropped the lastEmpty tracking entirely. The post-loop now just return [] as T;. The cast is needed because TypeScript can't statically prove T extends Array from the runtime Array.isArray branch, but the function is private with one caller (workspace.findFiles returning Uri[]) so it's sound. Pushed in the latest force-push.
…bt project
Production telemetry surfaced cluster `extensionActivationError` (83 unique
machines / 104 events / 24h, both 0.60.7 and 0.61.0) with the stack pinned
at:
Error: no projects found. retrying
at DBTWorkspaceFolder.retryWithBackoff
at async DBTWorkspaceFolder.discoverProjects
at async DBTProjectContainer.registerWorkspaceFolder
at async Promise.all (index 0)
at async DBTProjectContainer.initializeDBTProjects
at async DBTPowerUserExtension.activate
Distribution: ~95% darwin + linux, ~95% `core` integration mode, every
top-version contributing — consistent with the multi-root workspace
pattern common on dev machines, and inconsistent with a regression.
**Root cause.** VS Code's `workspaceContains:**/dbt_project.yml`
activation matches when ANY workspace folder contains a `dbt_project.yml`.
The extension then runs `discoverProjects` on EVERY workspace folder via
`Promise.all(folders.map(registerWorkspaceFolder))`. For a folder that
genuinely has no dbt project — typical for multi-root workspaces where a
sibling folder triggered activation — `findFiles` returns `[]` on every
attempt, `retryWithBackoff` exhausts after ~10s (5 attempts with 1+2+3+4
backoff), and the thrown `"no projects found. retrying"` propagates up
to the activate-level catch.
The visible damage isn't just the telemetry noise: the throw also
short-circuits the rest of `activate()`, so `statusBars.initialize()`
and the workspace-folder change listener never run. Users with a
mixed-folder workspace get a working dbt project for the folder that
matched, plus silently-degraded status-bar UX for the rest of the
session.
**Fix.** In `retryWithBackoff`, treat a consistently-empty result after
the retry budget as legitimately empty and return `[]` instead of
throwing. The retry schedule and the empty/non-empty differentiation
are unchanged — only the terminal disposition. Real errors from `fn()`
(permission failures, malformed pattern, indexer crashes) still throw
after retries, preserving the existing escalation for non-empty failure
modes. The constructor's `createConfigWatcher` keeps watching for
`dbt_project.yml` creation in this folder, so a project added later is
still picked up.
This also restructures the loop so empty results are handled by normal
control flow rather than via a thrown-and-caught sentinel error, which
is cleaner and removes the previously-unreachable
`"no projects found after maximum retries"` throw at the bottom of the
function.
**Verification.** Adds `src/test/suite/discoverProjectsRepro.test.ts`
with four cases that instantiate the real `DBTWorkspaceFolder` and stub
only `workspace.findFiles`:
1. Consistently empty `findFiles` → `discoverProjects` resolves
cleanly (was: rejects after ~10s).
2. Retry budget unchanged at ~10s — backoff schedule identical to
pre-fix.
3. Multi-root `Promise.all([happy, empty])` resolves successfully —
reproduces the production "Promise.all (index 0)" stack frame and
proves the fix unblocks the rest of `activate()`.
4. `findFiles` rejecting (e.g. EACCES) still throws after 5 retries —
guards against accidentally swallowing genuine errors.
Pre-fix output captured at
`screenshots/no-projects-found-investigation/repro-output.txt`; post-fix
at `screenshots/no-projects-found-investigation/after-fix-repro.txt`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ralphstodomingo
force-pushed
the
fix/multi-root-empty-folder-no-projects-found
branch
from
778f0b0 to
f9422a9
Compare
There was a problem hiding this comment.
Can we not just add try catch here?
…ctsFound sentinel Revert `retryWithBackoff` to its master shape and instead handle the empty/error split at the call site in `discoverProjects`. A new `NoProjectsFound` Error subclass is thrown for the consistently-empty case, and `discoverProjects` catches at the boundary: - `NoProjectsFound` → silent, resolves to `[]` (multi-root expected case) - any other error → fires `discoverProjectsError` telemetry, resolves to `[]` so activation isn't short-circuited on a single folder Telemetry uses the existing `sendTelemetryError` plumbing already in this file (parity with `registerDBTProjectError`). Empty folders no longer fire telemetry — avoids swapping the `extensionActivationError` cluster for an equivalent `discoverProjectsError` cluster. Test 4 flips from "real errors propagate" to "real errors resolve to [] AND fire `discoverProjectsError` exactly once with the underlying error preserved." Test 1 additionally asserts no telemetry on the empty case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/test/suite/discoverProjectsRepro.test.ts`:
- Around line 73-79: The helper stubWorkspaceFindFiles mutates the shared
workspace object by replacing workspace.findFiles and workspace.getConfiguration
but never restores them; capture the original references at the start of
stubWorkspaceFindFiles (e.g., const origFindFiles = workspace.findFiles; const
origGetConfiguration = workspace.getConfiguration), then add an afterAll (or
afterEach for per-test isolation) in the test file to restore
workspace.findFiles = origFindFiles and workspace.getConfiguration =
origGetConfiguration so other suites are not affected; ensure the restoration
references the same originals captured when the stub was installed and run
cleanup unconditionally.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18d75ab9-ce5a-4bd3-8ddb-a79a8e63403f
📒 Files selected for processing (2)
src/dbt_client/dbtWorkspaceFolder.tssrc/test/suite/discoverProjectsRepro.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dbt_client/dbtWorkspaceFolder.ts
…amination `stubWorkspaceFindFiles` mutates the shared `vscode` module object (`workspace.findFiles`, `workspace.getConfiguration`) without ever restoring them. Other suites in the same Jest worker would silently pick up the last test's stub. Capture originals in `beforeAll` and restore in `afterAll`. Per CodeRabbit review on PR #1939. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/suite/discoverProjectsRepro.test.ts (1)
161-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest currently codifies the wrong contract for real
findFilesfailuresLine 176 should assert rejection, not resolution. Per the PR objective, only the consistently-empty case is swallowed; genuine
findFileserrors should still escalate after retries. Keeping this asresolveswould mask regressions and enforce behavior opposite to the intended fix.Suggested test correction
it("real findFiles errors resolve to [] AND fire discoverProjectsError telemetry", async () => { @@ - await expect(folder.discoverProjects()).resolves.toBeUndefined(); + await expect(folder.discoverProjects()).rejects.toThrow( + "EACCES: permission denied", + ); expect(findFilesMock).toHaveBeenCalledTimes(5); expect(calls).toBe(5); @@ });🤖 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 `@src/test/suite/discoverProjectsRepro.test.ts` around lines 161 - 186, The test incorrectly expects folder.discoverProjects() to resolve; change the assertion to expect it to reject with the underlying error (e.g., use await expect(folder.discoverProjects()).rejects.toThrow or rejects.toHaveProperty('message', 'EACCES: permission denied')) so genuine findFiles failures escalate after retries; keep the rest of the checks (findFilesMock call count and fakeTelemetry.sendTelemetryError called once) intact and reference folder.discoverProjects(), the stubbed findFilesMock, and fakeTelemetry.sendTelemetryError when updating the 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.
Duplicate comments:
In `@src/test/suite/discoverProjectsRepro.test.ts`:
- Around line 161-186: The test incorrectly expects folder.discoverProjects() to
resolve; change the assertion to expect it to reject with the underlying error
(e.g., use await expect(folder.discoverProjects()).rejects.toThrow or
rejects.toHaveProperty('message', 'EACCES: permission denied')) so genuine
findFiles failures escalate after retries; keep the rest of the checks
(findFilesMock call count and fakeTelemetry.sendTelemetryError called once)
intact and reference folder.discoverProjects(), the stubbed findFilesMock, and
fakeTelemetry.sendTelemetryError when updating the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe39be52-9467-4a8e-a575-c7ab6d11e09c
📒 Files selected for processing (1)
src/test/suite/discoverProjectsRepro.test.ts
✅ Tests — All Passed |
| ); | ||
| } catch (error) { | ||
| if (!(error instanceof NoProjectsFound)) { | ||
| this.telemetry.sendTelemetryError("discoverProjectsError", error); |
There was a problem hiding this comment.
This is not really an error that we need to track though. this just means that there is no projects found?
There was a problem hiding this comment.
@mdesmet not the case.
!(error instanceof NoProjectsFound) means it's deliberately ignoring no projects found here
there was an activation-level catch that was surfacing all of these, both the 'no projects found' and everything else findFiles could reject for (EMFILE, EACCES, etc)
this PR makes it so it no longer throws an error that's caught there when it's just no dbt projects found, but without this, errors that the old structure would have caught at activation level would just be silently ignored
3 participants
Summary
Production telemetry surfaced cluster
extensionActivationError(83 unique machines / 104 events / 24h, both 0.60.7 and 0.61.0) with the stack pinned at:Distribution: ~95% darwin + linux, ~95%
coreintegration mode, every top-version contributing — consistent with the multi-root workspace pattern common on dev machines, and inconsistent with a regression.Root cause
VS Code's
workspaceContains:**/dbt_project.ymlactivation matches when any workspace folder contains adbt_project.yml. The extension then runsdiscoverProjectson every workspace folder viaPromise.all(folders.map(registerWorkspaceFolder)). For a folder that genuinely has no dbt project — typical for multi-root workspaces where a sibling folder triggered activation —findFilesreturns[]on every attempt,retryWithBackoffexhausts after ~10s (5 attempts with 1+2+3+4 backoff), and the thrown"no projects found. retrying"propagates up to the activate-level catch.The visible damage isn't just the telemetry noise — the throw also short-circuits the rest of
activate(), sostatusBars.initialize()and the workspace-folder change listener never run. Users with a mixed-folder workspace get a working dbt project for the folder that matched, plus silently-degraded status-bar UX for the rest of the session.Fix
In
retryWithBackoff, treat a consistently-empty result after the retry budget as legitimately empty and return[]instead of throwing. The retry schedule and the empty/non-empty differentiation are unchanged — only the terminal disposition. Real errors fromfn()(permission failures, malformed pattern, indexer crashes) still throw after retries, preserving the existing escalation for non-empty failure modes.The constructor's
createConfigWatcher(line 60) keeps watching fordbt_project.ymlcreation in this folder, so a project added later is still picked up automatically.This also restructures the loop so empty results are handled by normal control flow rather than via a thrown-and-caught sentinel error, which is cleaner and removes the previously-unreachable
"no projects found after maximum retries"throw at the bottom of the function.Pre-fix → post-fix evidence
A new test file (
src/test/suite/discoverProjectsRepro.test.ts) instantiates the realDBTWorkspaceFolderand stubs onlyworkspace.findFiles. Same harness shape both pre- and post-fix.Pre-fix (against this branch's HEAD~1, i.e. origin/master) — the harness asserted on the bug shape and passed, confirming the production stack reproduces deterministically:
Post-fix (this branch) — the assertions flip to verify the new behaviour, plus a new fourth case guards against accidentally swallowing genuine errors:
Saved at
screenshots/no-projects-found-investigation/repro-output.txt(pre-fix) andscreenshots/no-projects-found-investigation/after-fix-repro.txt(post-fix).Test plan
jupyterlabServicesCompat.test.ts) is pre-existing on origin/master (missing build artifact in local env), confirmed unrelated by stashing this PR's changes and re-running it[9.5s, 12s)in test 2findFilesrejecting with EACCES still throws after 5 retries🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests