fix: load embedded dolt projects without starting server#78
Open
victorlitvinenko wants to merge 1 commit into
Open
fix: load embedded dolt projects without starting server#78victorlitvinenko wants to merge 1 commit into
victorlitvinenko wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR adds embedded Dolt project detection to automatically select between CLI and SQL backends. A new ChangesEmbedded Dolt Backend Detection and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (4)
src/backend/__tests__/BeadsCommandRunner.test.ts (1)
1-1: ⚡ Quick winRename this new test file to kebab-case.
BeadsCommandRunner.test.tsshould be renamed to kebab-case to satisfy repository naming conventions.
As per coding guidelines "Use kebab-case for source code, docs, and configs (e.g.,my-module.ts,api-reference.md)."🤖 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/backend/__tests__/BeadsCommandRunner.test.ts` at line 1, Rename the test file from PascalCase to kebab-case to match repo conventions: change src/backend/__tests__/BeadsCommandRunner.test.ts to src/backend/__tests__/beads-command-runner.test.ts and update any references/imports accordingly (e.g., the import of createListCommandArgs from "../BeadsCommandRunner" if relative paths or test globs rely on filename); ensure test runner and CI picks up the renamed file and run tests to verify nothing else needs path updates.src/backend/__tests__/BeadsBackendFactory.test.ts (2)
29-35: ⚡ Quick winAdd an assertion for
onDetectionErrorinvocation.You verify fallback behavior, but not whether the optional detection-error callback is actually called. Adding this assertion protects error observability behavior from regressions.
Patch suggestion
it("falls back to the Dolt SQL backend when mode detection fails", async () => { + const onDetectionError = jest.fn(); const kind = await selectBackendKind(async () => { throw new Error("bd dolt show failed"); - }); + }, onDetectionError); expect(kind).toBe("dolt-sql"); + expect(onDetectionError).toHaveBeenCalledTimes(1); });🤖 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/backend/__tests__/BeadsBackendFactory.test.ts` around lines 29 - 35, The test for selectBackendKind should also assert that the optional onDetectionError callback is invoked when mode detection throws; update the test in BeadsBackendFactory.test.ts to pass a jest.fn() (or sinon spy) as the onDetectionError argument to selectBackendKind and add an expectation that this spy was called once and received the thrown Error (or at least has been called), referencing selectBackendKind and onDetectionError to locate where to attach the spy and the assertion.
1-1: ⚡ Quick winRename this new test file to kebab-case.
BeadsBackendFactory.test.tsshould use kebab-case to match repository naming rules.
As per coding guidelines "Use kebab-case for source code, docs, and configs (e.g.,my-module.ts,api-reference.md)."🤖 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/backend/__tests__/BeadsBackendFactory.test.ts` at line 1, Rename the test file from BeadsBackendFactory.test.ts to kebab-case (e.g., beads-backend-factory.test.ts) to follow repository naming rules; keep the test contents (including the import of selectBackendKind) unchanged and update any references or test runner patterns if they explicitly list the old filename so the test suite still discovers it.src/backend/BeadsBackendFactory.ts (1)
1-1: ⚡ Quick winRename this new file to kebab-case.
BeadsBackendFactory.tsdoes not follow the repository naming convention for.tsfiles. Please rename it (and imports) to kebab-case before merge.
As per coding guidelines "Use kebab-case for source code, docs, and configs (e.g.,my-module.ts,api-reference.md)."🤖 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/backend/BeadsBackendFactory.ts` at line 1, The file name BeadsBackendFactory.ts violates the kebab-case naming guideline; rename the file to beads-backend-factory.ts and update all corresponding imports/usages (e.g., import/require statements that reference "BeadsBackendFactory" or paths to that module) to the new kebab-case path; also rename any default export or class identifier if your style requires matching file name (e.g., class or export named BeadsBackendFactory can remain, but ensure type-only imports or tooling that expects file-name casing are updated); run the project build/tests to verify no import errors remain.
🤖 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 `@CHANGELOG.md`:
- Line 13: Update the changelog entry that reads "Embedded Dolt projects now
load Dashboard and Issues through the CLI backend without calling `bd dolt
start`, including closed issues for the `All` filter." to include the required
bead reference token (e.g., append or prepend a token like `vsbeads-xxx`) so the
entry conforms to the guideline requiring bead references; ensure the bead token
is placed adjacent to the entry text and follows the existing changelog
formatting convention.
In `@src/backend/BeadsBackendFactory.ts`:
- Around line 34-40: The factory currently returns BeadsDoltBackend for the
non-embedded branch which bypasses the CLI; update the logic in
BeadsBackendFactory so that when kind === "cli" or in the non-embedded branch it
instantiates and returns BeadsCommandRunner(params) instead of
BeadsDoltBackend(params), and update the corresponding log message from "Using
Dolt SQL backend" to reflect the CLI backend; refer to the BeadsBackendFactory
function and the BeadsCommandRunner / BeadsDoltBackend symbols to locate and
change the returned class and log string.
- Around line 65-72: The execFileAsync call that runs params.bdPath with
["dolt","show","--json"] can hang because it has no timeout; add a timeout
property (e.g., 30_000 ms) to the options object passed to execFileAsync (the
same object that contains cwd, env, maxBuffer) and update the surrounding await
to catch timeout errors (check for error.code === 'ETIMEDOUT' or similar) so you
can log/throw a clear error and avoid blocking activation; reference the
existing execFileAsync invocation and params.bdPath/params.cwd/params.beadsDir
symbols when making the change.
---
Nitpick comments:
In `@src/backend/__tests__/BeadsBackendFactory.test.ts`:
- Around line 29-35: The test for selectBackendKind should also assert that the
optional onDetectionError callback is invoked when mode detection throws; update
the test in BeadsBackendFactory.test.ts to pass a jest.fn() (or sinon spy) as
the onDetectionError argument to selectBackendKind and add an expectation that
this spy was called once and received the thrown Error (or at least has been
called), referencing selectBackendKind and onDetectionError to locate where to
attach the spy and the assertion.
- Line 1: Rename the test file from BeadsBackendFactory.test.ts to kebab-case
(e.g., beads-backend-factory.test.ts) to follow repository naming rules; keep
the test contents (including the import of selectBackendKind) unchanged and
update any references or test runner patterns if they explicitly list the old
filename so the test suite still discovers it.
In `@src/backend/__tests__/BeadsCommandRunner.test.ts`:
- Line 1: Rename the test file from PascalCase to kebab-case to match repo
conventions: change src/backend/__tests__/BeadsCommandRunner.test.ts to
src/backend/__tests__/beads-command-runner.test.ts and update any
references/imports accordingly (e.g., the import of createListCommandArgs from
"../BeadsCommandRunner" if relative paths or test globs rely on filename);
ensure test runner and CI picks up the renamed file and run tests to verify
nothing else needs path updates.
In `@src/backend/BeadsBackendFactory.ts`:
- Line 1: The file name BeadsBackendFactory.ts violates the kebab-case naming
guideline; rename the file to beads-backend-factory.ts and update all
corresponding imports/usages (e.g., import/require statements that reference
"BeadsBackendFactory" or paths to that module) to the new kebab-case path; also
rename any default export or class identifier if your style requires matching
file name (e.g., class or export named BeadsBackendFactory can remain, but
ensure type-only imports or tooling that expects file-name casing are updated);
run the project build/tests to verify no import errors remain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88986c8c-2398-4b68-87d6-c3aa4731d913
📒 Files selected for processing (6)
CHANGELOG.mdsrc/backend/BeadsBackendFactory.tssrc/backend/BeadsCommandRunner.tssrc/backend/BeadsProjectManager.tssrc/backend/__tests__/BeadsBackendFactory.test.tssrc/backend/__tests__/BeadsCommandRunner.test.ts
| ### Fixed | ||
|
|
||
| - `beads.userId` and `beads.pathToBd` now expand `${env:VAR}` placeholders (#60) | ||
| - Embedded Dolt projects now load Dashboard and Issues through the CLI backend without calling `bd dolt start`, including closed issues for the `All` filter. |
There was a problem hiding this comment.
Add a bead reference to this changelog entry.
This new notable fix entry is missing the required bead reference token (for example, vsbeads-xxx).
Suggested edit
-- Embedded Dolt projects now load Dashboard and Issues through the CLI backend without calling `bd dolt start`, including closed issues for the `All` filter.
+- Embedded Dolt projects now load Dashboard and Issues through the CLI backend without calling `bd dolt start`, including closed issues for the `All` filter (`vsbeads-xxx`).As per coding guidelines, "Keep entries terse with bead references (e.g., vsbeads-xxx)."
📝 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.
Suggested change
| - Embedded Dolt projects now load Dashboard and Issues through the CLI backend without calling `bd dolt start`, including closed issues for the `All` filter. | |
| - Embedded Dolt projects now load Dashboard and Issues through the CLI backend without calling `bd dolt start`, including closed issues for the `All` filter (`vsbeads-xxx`). |
🤖 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 `@CHANGELOG.md` at line 13, Update the changelog entry that reads "Embedded
Dolt projects now load Dashboard and Issues through the CLI backend without
calling `bd dolt start`, including closed issues for the `All` filter." to
include the required bead reference token (e.g., append or prepend a token like
`vsbeads-xxx`) so the entry conforms to the guideline requiring bead references;
ensure the bead token is placed adjacent to the entry text and follows the
existing changelog formatting convention.
Comment on lines
+34
to
+40
| if (kind === "cli") { | ||
| params.log.info("Using bd CLI backend for embedded Dolt project"); | ||
| return new BeadsCommandRunner(params); | ||
| } | ||
|
|
||
| params.log.info("Using Dolt SQL backend"); | ||
| return new BeadsDoltBackend(params); |
There was a problem hiding this comment.
Factory still routes to a non-CLI backend.
The non-embedded branch instantiates BeadsDoltBackend, which routes Beads operations through Dolt SQL rather than CLI. That conflicts with the backend rule for src/backend/**/*.ts.
As per coding guidelines "All Beads operations must go through CLI (bd list --json, bd show <id> --json, etc.). Never access .beads files directly."
🤖 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/backend/BeadsBackendFactory.ts` around lines 34 - 40, The factory
currently returns BeadsDoltBackend for the non-embedded branch which bypasses
the CLI; update the logic in BeadsBackendFactory so that when kind === "cli" or
in the non-embedded branch it instantiates and returns
BeadsCommandRunner(params) instead of BeadsDoltBackend(params), and update the
corresponding log message from "Using Dolt SQL backend" to reflect the CLI
backend; refer to the BeadsBackendFactory function and the BeadsCommandRunner /
BeadsDoltBackend symbols to locate and change the returned class and log string.
Comment on lines
+65
to
+72
| const { stdout } = await execFileAsync(params.bdPath, ["dolt", "show", "--json"], { | ||
| cwd: params.cwd, | ||
| env: { | ||
| ...process.env, | ||
| BEADS_DIR: params.beadsDir, | ||
| }, | ||
| maxBuffer: 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
Add a timeout to backend-mode detection command execution.
This activation-path bd dolt show --json call has no timeout, so a hung process can block project activation indefinitely.
Patch suggestion
const { stdout } = await execFileAsync(params.bdPath, ["dolt", "show", "--json"], {
cwd: params.cwd,
env: {
...process.env,
BEADS_DIR: params.beadsDir,
},
maxBuffer: 1024 * 1024,
+ timeout: 5000,
+ killSignal: "SIGTERM",
});🤖 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/backend/BeadsBackendFactory.ts` around lines 65 - 72, The execFileAsync
call that runs params.bdPath with ["dolt","show","--json"] can hang because it
has no timeout; add a timeout property (e.g., 30_000 ms) to the options object
passed to execFileAsync (the same object that contains cwd, env, maxBuffer) and
update the surrounding await to catch timeout errors (check for error.code ===
'ETIMEDOUT' or similar) so you can log/throw a clear error and avoid blocking
activation; reference the existing execFileAsync invocation and
params.bdPath/params.cwd/params.beadsDir symbols when making the change.
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
1 participant
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.
Fix embedded Dolt projects in Dashboard and Issues
Summary
Fixes Dashboard and Issues loading for Beads projects that use embedded Dolt mode.
Recent
bdversions can run with an embedded in-process Dolt engine. In that mode there is no Dolt SQL server, sobd dolt startfails with:The extension previously always created the Dolt SQL backend for active projects. That path can try to start a Dolt server while loading Dashboard or Issues, which breaks embedded-mode workspaces.
Changes
bd dolt show --json.bd list --all --limit 0 --json, so the IssuesAllfilter can show closed issues too.CHANGELOG.md.Testing
bun run testbun run compile:quietbun run lintManual checks:
bd dolt startembedded-mode error.Not ClosedtoAllshows closed issues that were hidden by the CLI default filter.Summary by CodeRabbit
Bug Fixes
bd dolt startAllfilter now includes closed issuesDocumentation