Refactor backend toward Dolt-backed reads and project controls#69
Merged
Conversation
Comprehensive research into upstream beads breaking changes: - Daemon/RPC removal (v0.50.0), change notification gap - Dolt migration issues and stability concerns - Homebrew tap to core migration - Notable upstream issues affecting extension development - Dolt deep dive (architecture, locking, change detection) - Draft upstream issue for change detection guidance Related: #65 Upstream: gastownhall/beads#1810 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace daemon-coupled project management with a CLI-backed backend interface, add version gating, and implement file-based refresh/polling to restore dynamic updates across Beads projects. Related: vsbeads-65
Owner
Author
|
related to #65 |
Owner
Author
|
related to #63 |
Use project-root fallback selection and sequence guards to prevent stale list responses, and force CLI commands through project-specific BEADS_DIR to keep data isolated across workspaces. Related: vsbeads-65
Stop tracking local .beads backup snapshots and ignore future backup artifacts so feature PRs stay focused on source changes only. Related: vsbeads-65
Ignore project-local tool mirrors and configure workspace beads settings to use the locally built bd binary for deterministic testing across projects. Related: vsbeads-65
Make project switches update immediately while reserving blank loading states for switch/initial loads, keep refreshes in-place, and remove the Beads create-issue command/button from the editor UI. Related: vsbeads-65
Tighten project-switch loading behavior, clean up daemon terminology/commands, improve Dolt connection error messaging, and stop tracking .beads runtime artifacts in git. Related: vsbeads-65
Reduce worktree and Dolt refresh churn while moving shared agent config into reusable tool directories.
Stop relying on repeated bd list/show reads by moving the hot read path to direct Dolt SQL, removing file-watch refresh loops, and introducing a starter Projects panel for project and server management. Related: vsbeads-65
jdillon
changed the title
Unify the compact dashboard with the issues panel spacing, hard-refresh behavior, and project-switch affordances while keeping Dolt actions accessible without hidden stale UI states. Related: vsbeads-65
Limit the dashboard project-directory hover and click affordance to the path text itself so the surrounding row is no longer an oversized interactive target. Related: vsbeads-65
Replace the old refresh heuristics with Dolt change-token polling, remove misleading storage mode plumbing, and set a real default polling interval for near-real-time updates. Related: vsbeads-65
Align release-facing docs with the Dolt-backed architecture and remove leftover watcher/lint debris that no longer applies after the backend pivot. Related: vsbeads-65
Capture all user-facing changes since v0.12.0: Dolt-backed backend, daemon removal (breaking: requires beads v0.50.0+), project switching improvements, and UX refinements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes the daemon/RPC architecture and replaces it with a BeadsBackend abstraction backed by CLI and Dolt SQL. It adds two backend implementations (CLI and Dolt SQL), polling-based change detection, UI/type changes to surface Dolt controls, and extensive documentation updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant VSCode as VS Code<br/>Extension
participant PM as ProjectManager
participant CLI as bd CLI
participant Dolt as Dolt<br/>Server
participant SQL as mysql2<br/>Connection Pool
User->>VSCode: open project / refresh
VSCode->>PM: resolve projects
PM->>CLI: bd where (resolve beadsDir)
CLI-->>PM: beadsDir path
VSCode->>PM: instantiate BeadsDoltBackend
VSCode->>CLI: bd version / bd dolt status
CLI->>Dolt: check/start server as needed
Dolt-->>CLI: server status / port info
VSCode->>SQL: create connection pool (mysql2)
SQL-->>VSCode: pool ready
VSCode->>SQL: SELECT ... FROM issues
SQL-->>VSCode: issue rows
VSCode->>User: render issues
loop polling every beads.refreshInterval
PM->>CLI: bd or SQL check (change token)
CLI->>Dolt: query commit/hash or run bd list
Dolt-->>CLI: token/state
CLI-->>PM: token
alt token changed
PM->>VSCode: _onDataChanged -> refresh views
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/extension.ts (1)
293-308:⚠️ Potential issue | 🟠 MajorClear stale active-project state when discovery returns none.
This handler only updates the project list. When the last project disappears,
activeProjectandbackendstay pointed at the removed project, so the subsequent view refreshes still operate on stale state until a separate manual refresh clears it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension.ts` around lines 293 - 308, When discovery finds no projects, clear the stale active project and shutdown/reset any backend tied to it: after projects = projectManager.getProjects() and the projects.length === 0 branch, call projectManager.setActiveProject(null or undefined) or a dedicated projectManager.clearActiveProject() to clear activeProject, then stop/reset the backend (e.g. call backend?.dispose() or backend?.shutdown() and set backend = undefined) and finally call updateStatusBar() so UI reflects the cleared state; reference projectManager.getActiveProject, projectManager.setActiveProject (or clearActiveProject), backend, and updateStatusBar to locate where to add these steps.package.json (1)
115-145:⚠️ Potential issue | 🟡 MinorExpose
beads.projectsin the settings schema.
BeadsProjectManager.getConfiguredProjectPaths()(line 209) and extension activation (line 35 ofsrc/extension.ts) both readbeads.projects, but this setting is not declared incontributes.configuration.propertiesin package.json. Users can only configure projects by manually editingsettings.json; the setting is invisible to the VS Code settings UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 115 - 145, The package.json configuration schema is missing the beads.projects property which BeadsProjectManager.getConfiguredProjectPaths() and the extension activation (reading beads.projects in src/extension.ts) expect; add a new property "beads.projects" to contributes.configuration.properties with type "array", items of type "string", a sensible default (e.g., []), and a clear description so the setting appears in VS Code UI and users can configure project paths without editing settings.json manually.
🧹 Nitpick comments (5)
src/webview/App.tsx (1)
109-115: Inconsistent indentation may indicate structural issue.The
ifstatement at lines 110-112 has unusual indentation (extra leading spaces) that doesn't match the surrounding code. While the logic appears intentional (early return for beadsPanel loading state before the switch), the indentation inconsistency could cause confusion during future maintenance.♻️ Suggested fix for consistent indentation
// Render the appropriate view const renderView = () => { - if (state.viewType === "beadsPanel" && state.loading && state.beads.length === 0) { - return <Loading />; - } + if (state.viewType === "beadsPanel" && state.loading && state.beads.length === 0) { + return <Loading />; + } - switch (state.viewType) { + switch (state.viewType) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webview/App.tsx` around lines 109 - 115, Fix the inconsistent indentation inside the renderView function: align the early-return if block (checking state.viewType === "beadsPanel" && state.loading && state.beads.length === 0) with the surrounding code so it matches the switch block indentation and existing style; leave the logic intact (return <Loading /> when that condition is true) and ensure the if and switch both use the same indentation level within renderView to avoid confusion when scanning state.viewType handling.src/providers/BeadDetailsViewProvider.ts (1)
93-96: Consider if trace level is appropriate for comment fetch failures.Downgrading comment fetch errors from
warntotracemay make it harder to diagnose issues in production. While the error is handled gracefully (returning empty array), completely suppressing visibility at trace level could hide persistent problems.Consider using
debuglevel instead, which provides more visibility while still not alarming users:💡 Suggested adjustment
client.listComments(this.currentBeadId).catch((err) => { - this.log.trace(`Failed to fetch comments: ${err}`); + this.log.debug(`Failed to fetch comments: ${err}`); return []; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/BeadDetailsViewProvider.ts` around lines 93 - 96, The current catch for client.listComments(this.currentBeadId) logs failures at trace level via this.log.trace which can hide recurring issues; change the log level to debug (this.log.debug) and include contextual info (bead id and error) in the message so failures remain visible for troubleshooting while still non-alarming, i.e., update the catch handler on client.listComments(...) to call this.log.debug with a descriptive message containing this.currentBeadId and the err and keep returning [].src/webview/views/IssuesView.tsx (1)
866-872: Loading indicator overlays existing content.The loading indicator is rendered alongside the table content rather than replacing it. This means users will see both a loading spinner and potentially stale data simultaneously. If this is intentional (to show stale data while refreshing), consider adding a visual dimming effect to the table to indicate the data may be stale. If not intentional, the table should be hidden while loading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webview/views/IssuesView.tsx` around lines 866 - 872, The loading spinner currently renders alongside the table inside the beads-table-wrapper, causing both spinner and stale rows to be visible; update IssuesView so that when viewMode === "table" and loading is true the table content is not rendered (or alternatively apply a dimming overlay to the table) — i.e., move the conditional so loading === true renders only the .issues-loading-state (or add a semi-transparent overlay CSS class to beads-table-wrapper when loading) and ensure the component's loading boolean controls either hiding of the table rows or toggling the dimmed-overlay state.sandbox/upstream-sync-v0.51.md (1)
69-77: Add language specifiers to fenced code blocks for lint compliance.The static analysis tool flags several fenced code blocks without language specifiers (lines 69, 191, 259). For directory structure or plain-text diagrams, use
textorplaintext.Example fix for line 69
-``` +```text internal/storage/ storage.go -- shared Transaction interface🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sandbox/upstream-sync-v0.51.md` around lines 69 - 77, Add a language specifier to the fenced code blocks that contain plain-directory or plaintext diagrams (e.g., the block starting with "internal/storage/ -- shared Transaction interface" and the other blocks at the flagged locations), replacing ``` with ```text (or ```plaintext) so the linter recognizes them as plain text and the static analysis warnings on lines 69, 191, and 259 are resolved.src/providers/DashboardViewProvider.ts (1)
90-105: Minor: RedundantsetLoading(false)calls.
setLoading(false)is called at line 91 in the success path and again at line 103 in thefinallyblock whenthisRequest === this.loadSequence. This is harmless but redundant. The same pattern exists inBeadsPanelViewProvider.Consider extracting this loading/sequencing pattern to the base class to avoid duplication across providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/DashboardViewProvider.ts` around lines 90 - 105, Redundant setLoading(false) is called in DashboardViewProvider (and duplicated in BeadsPanelViewProvider): unify the loading/sequencing logic into the shared base provider so callers don't repeat it. Create a protected helper on the base class (e.g., finalizeLoad or ensureLoadingComplete) that accepts thisRequest and loadingStartedAt, performs the showLoading wait via waitForMinimumLoading, checks thisRequest === this.loadSequence, calls setError/handleBackendError when needed, and always sets setLoading(false) once; then remove the extra setLoading(false) from the success path in DashboardViewProvider and similar spots in BeadsPanelViewProvider and call the new base helper from catch/finally to centralize behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sandbox/dolt-deep-dive.md`:
- Around line 455-464: The sections referencing the removed daemon model
(mentions of "bd --json", DOLT_HASHOF('HEAD') polling, and tying auto-commit to
a daemon flag) must be updated to reflect this PR's hybrid backend: explain that
control-plane operations use "bd", reads use direct Dolt SQL, and change
detection should use explicit Dolt change-token polling (replace references to
DOLT_HASHOF('HEAD') polling and dolt_status/dolt_diff as the primary integration
path), or clearly mark the existing text as historical/legacy; update all
affected paragraphs (including the blocks that mention DOLT_HASHOF('HEAD'),
dolt_diff, dolt_status, and the auto-commit/daemon behavior) so readers follow
the hybrid integration path instead of the removed daemon model.
In `@sandbox/theia-beads-standalone.md`:
- Line 23: The sandbox doc still references the removed BeadsDaemonClient.ts and
daemon callbacks; update all occurrences (e.g., mentions of BeadsDaemonClient.ts
and onStartDaemon) to reference the new BeadsBackend abstraction and the
concrete backends (CLI and Dolt), replacing daemon-specific wording with the
current architecture, or explicitly add a short note stating that daemon support
was removed and the document is retained for historical/Theia reference; ensure
references to daemon-related callbacks are removed or mapped to the new
BeadsBackend API and CLI/Dolt backends.
In `@sandbox/upstream-notable-issues.md`:
- Line 28: Update the heading text that reads ### `#376` — "I want to love Beads
but the AI generated docs make it impossible" (OPEN) to use the compound
adjective form by changing "AI generated docs" to "AI-generated docs" so the
heading becomes ### `#376` — "I want to love Beads but the AI-generated docs make
it impossible" (OPEN).
In `@src/backend/BeadsBackend.ts`:
- Around line 80-96: Add a cleanup hook to the BeadsBackend interface by
declaring a dispose(): Promise<void> (or close(): Promise<void>) method so
implementations like BeadsDoltBackend can release resources (e.g., its MySQL
pool); update all classes implementing BeadsBackend to implement that method and
modify BeadsProjectManager.activateProject() to call await oldBackend.dispose()
before replacing the instance to avoid leaking connections.
In `@src/backend/BeadsCLIBackend.ts`:
- Around line 188-207: The execBd method currently logs the full command argv
and raw stdout/stderr which can leak user payloads; before calling
this.log.debug/trace, sanitize the args and outputs: detect when args include
payload-bearing subcommands (e.g., "create", "update", "show", "comments add")
or flags like --title, --description, --note, --ref, --body and produce a
sanitizedArgs/commandLabel that only includes the subcommand and non-sensitive
flags (or replaces flag values with "<redacted>"), and similarly redact
stdout/stderr for those subcommands (or only log a short status message); update
usages around commandLabel, stdout, and stderr in execBd to use the sanitized
values while preserving timing and trace granularity.
- Around line 145-147: The code may push two "--type" flags because both
args.type and args.issue_type are handled independently; update the argument
normalization so "--type" is emitted only once: compute a single effectiveType =
args.type ?? args.issue_type, and if both args.type and args.issue_type are
provided but differ, throw or return an error to reject conflicting inputs; then
push "--type" with effectiveType to cmdArgs (referencing args.type,
args.issue_type, cmdArgs and the code in BeadsCLIBackend.ts where argv is
built).
- Around line 193-200: The execFileAsync call in BeadsCLIBackend (invoking
this.bdPath with args and maxBuffer) lacks a timeout so a hanging bd/Dolt child
can block the extension; add a timeout option (e.g., CHILD_PROCESS_TIMEOUT_MS
constant) to the options passed to execFileAsync (or use an AbortController if
execFileAsync supports it) so the child is killed and the Promise rejects after
the timeout, and update the surrounding call site to catch and handle the
timeout error with a clear, descriptive error message so the extension doesn't
remain stuck.
In `@src/backend/BeadsDoltBackend.ts`:
- Around line 438-449: The waitForServerReady method currently returns the last
doltStatus output even if the server never became ready; change it to throw an
error when the loop times out instead of returning a bogus status so callers
like startDoltServer() fail fast. Capture the last status string (from
this.cli.doltStatus()) and include it in a thrown Error with a clear message
(e.g., "Dolt server did not become ready: <status>") so the caller can surface
startup failure.
- Around line 381-406: Move the resetPool() call so it happens before you assign
this.poolPromise (call await this.resetPool() prior to setting this.poolPromise
= (async () => { ... })()) and make the finally block defensive: only clear
this.poolPromise if it still equals the local promise you awaited (e.g. capture
const localPromise = this.poolPromise before awaiting and in finally do if
(this.poolPromise === localPromise) this.poolPromise = null). This ensures
this.poolPromise remains stable during initialization and prevents races/leaks
around getConnectionInfo(), resetPool(), and the assignment to this.pool.
- Around line 452-466: The code currently uses execBdText (which merges stdout
and stderr) and JSON.parse on that combined string in getDoltShowInfo, which
fails if bd emits warnings to stderr; add a new helper execBdJson<T>(args:
string[]): Promise<T> that calls execFileAsync similar to execBdText but parses
only stdout with JSON.parse, logs or records stderr separately (do not include
stderr in the parse), then update getDoltShowInfo to call
execBdJson<DoltShowInfo>(["dolt","show","--json"]) and remove JSON.parse usage
on merged output; follow the pattern used by BeadsCLIBackend.runJson() for
error/stderr handling.
In `@src/backend/BeadsProjectManager.ts`:
- Around line 144-168: getBackendStatus currently returns "running" based solely
on this.backend.checkCompatibility(), which misses live health (so
zombie/not_initialized states never surface); update getBackendStatus to, after
compatibility passes, perform a live probe (e.g. call an existing backend
health/liveness method such as this.backend.ping() / this.backend.getStatus() or
implement a new this.backend.probeLive()) and branch on its result: return
"running" only if the live probe indicates healthy, return "not_initialized" or
"zombie" when the probe indicates uninitialized/stopped/unhealthy, and include
relevant details (e.g. activeProject.beadsDir, detectedVersion) in the details
object; keep the compatibility failure path as-is.
In `@src/extension.ts`:
- Around line 149-156: The Start/Stop Dolt handlers call
projectManager.refresh() but that does not trigger onActiveProjectChanged, so
updateStatusBar() is not invoked and the status badge/tooltip can remain stale;
after the calls to projectManager.refresh(), dashboardProvider.refresh(),
beadsPanelProvider.refresh(), detailsProvider.refresh() in the startDoltServer()
success path (and the analogous stop handler), explicitly call the function that
updates the UI badge—updateStatusBar()—and/or manually fire the same
onActiveProjectChanged notification used elsewhere so the status bar is
refreshed; update both the start handler (around client.startDoltServer()) and
the stop handler (the similar block later) to ensure the status bar is updated
immediately after refresh.
- Around line 34-43: Replace raw sensitive values in the activation debug logs:
do not log the full config.get("pathToBd") value, config.get("userId"), or the
full workspaceFolders paths. Instead log presence or counts (e.g., whether
pathToBd is the default or overridden, and configuredProjects.length and
workspaceFolders.length) and mask or indicate that userId is present/empty (no
raw email/username). Update the debug statements that reference
config.get("pathToBd"), config.get("userId"), and workspaceFolders so they emit
only non-sensitive indicators (counts, booleans, or masked labels) while leaving
other non-sensitive config logs unchanged.
- Around line 131-136: The manual refresh currently calls
dashboardProvider.hardRefresh(), beadsPanelProvider.hardRefresh(), and
detailsProvider.hardRefresh() before awaiting projectManager.refresh(), causing
views to load against stale project/backend state; change the order so you await
projectManager.refresh() first (await projectManager.refresh()), then call the
hardRefresh() methods on dashboardProvider, beadsPanelProvider, and
detailsProvider to ensure loadData("manualRefresh") runs with up-to-date
activeProject/backend state.
In `@src/webview/common/Dropdown.tsx`:
- Around line 105-118: The left position is computed from the raw measured menu
width but you later force minWidth to rect.width, causing misalignment for
"bottom-end"; update compute order in the Dropdown: after measuring
menuEl.offsetWidth and determining minWidth = rect.width, compute an
effectiveWidth = Math.max(menuWidth, minWidth) (or only apply this for
menuPlacement === "bottom-end"), then calculate left using that effectiveWidth
(with the same clamping logic), and finally call setMenuPosition({ top, left,
minWidth: rect.width }); reference menuEl, rect, menuPlacement, and
setMenuPosition to locate the change.
- Around line 130-155: When the portal-mounted menu opens, transfer focus into
the menu and when it closes restore focus to the trigger: in Dropdown component
add a useEffect that watches isOpen and when true finds the first focusable
element inside menuRef.current (e.g. querySelector for 'button, [href], input,
select, textarea, [tabindex]:not([tabindex="-1"])') and calls .focus(); when
isOpen becomes false, call triggerRef.current?.focus() to restore focus. Ensure
the close function also restores focus when invoked programmatically, and guard
against missing refs; use menuRef and triggerRef names from the component and
rely on children (DropdownItem) being focusable or having tabindex set.
In `@src/webview/common/ProjectDropdown.tsx`:
- Around line 53-56: The component uses a composite string
`${project.id}:${project.rootPath}` as the rendered key but still checks
`project.id === activeProject?.id` to decide active highlighting, causing
duplicate active rows when ids collide; update the active comparison to use the
same identity as the key (e.g., compare `${project.id}:${project.rootPath}` to
the same composite derived from `activeProject`) or equivalently check both
`project.id` and `project.rootPath` match `activeProject` so the highlighting
logic in the ProjectDropdown (where `project.id`, `project.rootPath`, and
`activeProject` are referenced and `handleSelect` is used for selection) uses a
consistent identity for selection and rendering.
In `@src/webview/views/DashboardView.tsx`:
- Around line 194-216: The dashboard work lists lost their drill-down because
BeadListItem is now passive; restore navigation by wiring a click handler from
DashboardView into each list item (for openBeads, inProgressBeads, blockedBeads
and the identical block at 226-236): either pass an onClick/onSelect prop to
BeadListItem or wrap each mapped item in a clickable element that calls the
dashboard navigation function (e.g., navigateToBead or openBeadDetail) with
bead.id (or the bead object). Ensure the UI still shows pointer/click styling
and that keyboard accessibility is preserved (role/button or onKeyDown) when
re-adding the handler.
---
Outside diff comments:
In `@package.json`:
- Around line 115-145: The package.json configuration schema is missing the
beads.projects property which BeadsProjectManager.getConfiguredProjectPaths()
and the extension activation (reading beads.projects in src/extension.ts)
expect; add a new property "beads.projects" to
contributes.configuration.properties with type "array", items of type "string",
a sensible default (e.g., []), and a clear description so the setting appears in
VS Code UI and users can configure project paths without editing settings.json
manually.
In `@src/extension.ts`:
- Around line 293-308: When discovery finds no projects, clear the stale active
project and shutdown/reset any backend tied to it: after projects =
projectManager.getProjects() and the projects.length === 0 branch, call
projectManager.setActiveProject(null or undefined) or a dedicated
projectManager.clearActiveProject() to clear activeProject, then stop/reset the
backend (e.g. call backend?.dispose() or backend?.shutdown() and set backend =
undefined) and finally call updateStatusBar() so UI reflects the cleared state;
reference projectManager.getActiveProject, projectManager.setActiveProject (or
clearActiveProject), backend, and updateStatusBar to locate where to add these
steps.
---
Nitpick comments:
In `@sandbox/upstream-sync-v0.51.md`:
- Around line 69-77: Add a language specifier to the fenced code blocks that
contain plain-directory or plaintext diagrams (e.g., the block starting with
"internal/storage/ -- shared Transaction interface" and the other blocks at the
flagged locations), replacing ``` with ```text (or ```plaintext) so the linter
recognizes them as plain text and the static analysis warnings on lines 69, 191,
and 259 are resolved.
In `@src/providers/BeadDetailsViewProvider.ts`:
- Around line 93-96: The current catch for
client.listComments(this.currentBeadId) logs failures at trace level via
this.log.trace which can hide recurring issues; change the log level to debug
(this.log.debug) and include contextual info (bead id and error) in the message
so failures remain visible for troubleshooting while still non-alarming, i.e.,
update the catch handler on client.listComments(...) to call this.log.debug with
a descriptive message containing this.currentBeadId and the err and keep
returning [].
In `@src/providers/DashboardViewProvider.ts`:
- Around line 90-105: Redundant setLoading(false) is called in
DashboardViewProvider (and duplicated in BeadsPanelViewProvider): unify the
loading/sequencing logic into the shared base provider so callers don't repeat
it. Create a protected helper on the base class (e.g., finalizeLoad or
ensureLoadingComplete) that accepts thisRequest and loadingStartedAt, performs
the showLoading wait via waitForMinimumLoading, checks thisRequest ===
this.loadSequence, calls setError/handleBackendError when needed, and always
sets setLoading(false) once; then remove the extra setLoading(false) from the
success path in DashboardViewProvider and similar spots in
BeadsPanelViewProvider and call the new base helper from catch/finally to
centralize behavior.
In `@src/webview/App.tsx`:
- Around line 109-115: Fix the inconsistent indentation inside the renderView
function: align the early-return if block (checking state.viewType ===
"beadsPanel" && state.loading && state.beads.length === 0) with the surrounding
code so it matches the switch block indentation and existing style; leave the
logic intact (return <Loading /> when that condition is true) and ensure the if
and switch both use the same indentation level within renderView to avoid
confusion when scanning state.viewType handling.
In `@src/webview/views/IssuesView.tsx`:
- Around line 866-872: The loading spinner currently renders alongside the table
inside the beads-table-wrapper, causing both spinner and stale rows to be
visible; update IssuesView so that when viewMode === "table" and loading is true
the table content is not rendered (or alternatively apply a dimming overlay to
the table) — i.e., move the conditional so loading === true renders only the
.issues-loading-state (or add a semi-transparent overlay CSS class to
beads-table-wrapper when loading) and ensure the component's loading boolean
controls either hiding of the table rows or toggling the dimmed-overlay state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8e80f76-5cfe-4476-98a0-46af258a6eeb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.gitignore.vscode/settings.jsonCHANGELOG.mdREADME.mddocs/development.mddocs/reference/beads-caveats.mddocs/reference/beads-daemon-api.mddocs/reference/beads-protected-branch.mdpackage.jsonsandbox/dolt-deep-dive.mdsandbox/theia-beads-standalone.mdsandbox/upstream-daemon-removal.mdsandbox/upstream-dolt-issues.mdsandbox/upstream-homebrew-migration.mdsandbox/upstream-issue-draft-change-notification.mdsandbox/upstream-notable-issues.mdsandbox/upstream-sync-v0.51.mdsrc/backend/BeadsBackend.tssrc/backend/BeadsCLIBackend.tssrc/backend/BeadsDaemonClient.tssrc/backend/BeadsDoltBackend.tssrc/backend/BeadsProjectManager.tssrc/backend/types.tssrc/extension.tssrc/providers/BaseViewProvider.tssrc/providers/BeadDetailsViewProvider.tssrc/providers/BeadsPanelViewProvider.tssrc/providers/DashboardViewProvider.tssrc/webview/App.tsxsrc/webview/common/Dropdown.tsxsrc/webview/common/ErrorMessage.tsxsrc/webview/common/ProjectDropdown.tsxsrc/webview/common/ProjectSelector.tsxsrc/webview/styles.csssrc/webview/types.tssrc/webview/views/DashboardView.tsxsrc/webview/views/DetailsView.tsxsrc/webview/views/IssuesView.tsx
💤 Files with no reviewable changes (4)
- src/webview/views/DetailsView.tsx
- docs/reference/beads-daemon-api.md
- src/webview/common/ProjectSelector.tsx
- src/backend/BeadsDaemonClient.ts
Comment on lines
+105
to
+118
| const menuWidth = menuEl.offsetWidth || rect.width; | ||
| const menuHeight = menuEl.offsetHeight || 0; | ||
| const viewportWidth = window.innerWidth; | ||
| const viewportHeight = window.innerHeight; | ||
|
|
||
| let left = menuPlacement === "bottom-end" ? rect.right - menuWidth : rect.left; | ||
| left = Math.max(8, Math.min(left, viewportWidth - menuWidth - 8)); | ||
|
|
||
| let top = rect.bottom + 2; | ||
| if (top + menuHeight > viewportHeight - 8) { | ||
| top = Math.max(8, rect.top - menuHeight - 2); | ||
| } | ||
|
|
||
| setMenuPosition({ top, left, minWidth: rect.width }); |
There was a problem hiding this comment.
Compute bottom-end alignment from the final menu width.
When menuPlacement is "bottom-end", Line 105 can measure a width smaller than the trigger, but Line 118 then forces minWidth up to rect.width. That means left is computed from the smaller width, so the menu opens shifted too far right until the next scroll/resize event.
💡 Proposed fix
- const menuWidth = menuEl.offsetWidth || rect.width;
+ const menuWidth = Math.max(menuEl.offsetWidth, rect.width);📝 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
| const menuWidth = menuEl.offsetWidth || rect.width; | |
| const menuHeight = menuEl.offsetHeight || 0; | |
| const viewportWidth = window.innerWidth; | |
| const viewportHeight = window.innerHeight; | |
| let left = menuPlacement === "bottom-end" ? rect.right - menuWidth : rect.left; | |
| left = Math.max(8, Math.min(left, viewportWidth - menuWidth - 8)); | |
| let top = rect.bottom + 2; | |
| if (top + menuHeight > viewportHeight - 8) { | |
| top = Math.max(8, rect.top - menuHeight - 2); | |
| } | |
| setMenuPosition({ top, left, minWidth: rect.width }); | |
| const menuWidth = Math.max(menuEl.offsetWidth, rect.width); | |
| const menuHeight = menuEl.offsetHeight || 0; | |
| const viewportWidth = window.innerWidth; | |
| const viewportHeight = window.innerHeight; | |
| let left = menuPlacement === "bottom-end" ? rect.right - menuWidth : rect.left; | |
| left = Math.max(8, Math.min(left, viewportWidth - menuWidth - 8)); | |
| let top = rect.bottom + 2; | |
| if (top + menuHeight > viewportHeight - 8) { | |
| top = Math.max(8, rect.top - menuHeight - 2); | |
| } | |
| setMenuPosition({ top, left, minWidth: rect.width }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/webview/common/Dropdown.tsx` around lines 105 - 118, The left position is
computed from the raw measured menu width but you later force minWidth to
rect.width, causing misalignment for "bottom-end"; update compute order in the
Dropdown: after measuring menuEl.offsetWidth and determining minWidth =
rect.width, compute an effectiveWidth = Math.max(menuWidth, minWidth) (or only
apply this for menuPlacement === "bottom-end"), then calculate left using that
effectiveWidth (with the same clamping logic), and finally call
setMenuPosition({ top, left, minWidth: rect.width }); reference menuEl, rect,
menuPlacement, and setMenuPosition to locate the change.
Comment on lines
+130
to
155
| const menu = isOpen ? createPortal( | ||
| <div | ||
| ref={menuRef} | ||
| className={`dropdown-menu ${menuClassName}`} | ||
| style={{ position: "fixed", top: menuPosition.top, left: menuPosition.left, minWidth: menuPosition.minWidth }} | ||
| > | ||
| {children} | ||
| </div>, | ||
| document.body | ||
| ) : null; | ||
|
|
||
| return ( | ||
| <DropdownContext.Provider value={{ close }}> | ||
| <div className={`dropdown ${className}`} ref={dropdownRef}> | ||
| <button | ||
| ref={triggerRef} | ||
| className={`dropdown-trigger ${triggerClassName}`} | ||
| onClick={() => setIsOpen(!isOpen)} | ||
| title={title} | ||
| > | ||
| {trigger} | ||
| {showChevron && <ChevronIcon open={isOpen} />} | ||
| </button> | ||
|
|
||
| {isOpen && ( | ||
| <div className={`dropdown-menu ${menuClassName}`}> | ||
| {children} | ||
| </div> | ||
| )} | ||
| </div> | ||
| {menu} | ||
| </DropdownContext.Provider> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/webview/common && wc -l Dropdown.tsxRepository: jdillon/vscode-beads
Length of output: 80
🏁 Script executed:
cat -n src/webview/common/Dropdown.tsxRepository: jdillon/vscode-beads
Length of output: 6922
Add focus management for keyboard accessibility when menu is portaled.
Once the menu lives under document.body, tab order no longer flows naturally from the trigger into the DropdownItems. The dropdown should transfer focus to the first menu item when opening and restore focus to the trigger when closing. Without this, keyboard users tabbing after opening the dropdown will bypass the menu entirely.
Currently, there is no focus transfer logic. Test by focusing the trigger, pressing Enter/Space, then Tab—focus should land on the first menu item, not the next control in the page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/webview/common/Dropdown.tsx` around lines 130 - 155, When the
portal-mounted menu opens, transfer focus into the menu and when it closes
restore focus to the trigger: in Dropdown component add a useEffect that watches
isOpen and when true finds the first focusable element inside menuRef.current
(e.g. querySelector for 'button, [href], input, select, textarea,
[tabindex]:not([tabindex="-1"])') and calls .focus(); when isOpen becomes false,
call triggerRef.current?.focus() to restore focus. Ensure the close function
also restores focus when invoked programmatically, and guard against missing
refs; use menuRef and triggerRef names from the component and rely on children
(DropdownItem) being focusable or having tabindex set.
Comment on lines
+53
to
+56
| key={`${project.id}:${project.rootPath}`} | ||
| className="project-dropdown-item" | ||
| active={project.id === activeProject?.id} | ||
| onClick={() => handleSelect(project.id)} | ||
| onClick={() => handleSelect(project)} |
There was a problem hiding this comment.
Use the same project identity for highlighting as for selection.
Line 53 switched the rendered identity to ${project.id}:${project.rootPath}, but Line 55 still compares only project.id. If two entries share an id, both rows will render as active even though only one BeadsProject was selected.
💡 Proposed fix
- active={project.id === activeProject?.id}
+ active={
+ project.id === activeProject?.id &&
+ project.rootPath === activeProject?.rootPath
+ }📝 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
| key={`${project.id}:${project.rootPath}`} | |
| className="project-dropdown-item" | |
| active={project.id === activeProject?.id} | |
| onClick={() => handleSelect(project.id)} | |
| onClick={() => handleSelect(project)} | |
| key={`${project.id}:${project.rootPath}`} | |
| className="project-dropdown-item" | |
| active={ | |
| project.id === activeProject?.id && | |
| project.rootPath === activeProject?.rootPath | |
| } | |
| onClick={() => handleSelect(project)} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/webview/common/ProjectDropdown.tsx` around lines 53 - 56, The component
uses a composite string `${project.id}:${project.rootPath}` as the rendered key
but still checks `project.id === activeProject?.id` to decide active
highlighting, causing duplicate active rows when ids collide; update the active
comparison to use the same identity as the key (e.g., compare
`${project.id}:${project.rootPath}` to the same composite derived from
`activeProject`) or equivalently check both `project.id` and `project.rootPath`
match `activeProject` so the highlighting logic in the ProjectDropdown (where
`project.id`, `project.rootPath`, and `activeProject` are referenced and
`handleSelect` is used for selection) uses a consistent identity for selection
and rendering.
Address follow-up backend lifecycle issues from review, rename the CLI control-plane helper to BeadsCommandRunner, and restore dashboard drill-down/status refresh behavior. Related: vsbeads-65
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/backend/BeadsDoltBackend.ts (2)
522-524: Unused methodsameConnectionInfo.This method is defined but never called. Consider removing it or marking it for future use with a TODO comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/BeadsDoltBackend.ts` around lines 522 - 524, The method sameConnectionInfo(a: DoltConnectionInfo, b: DoltConnectionInfo) is unused; either remove this private method from BeadsDoltBackend or keep it with a clear TODO comment explaining intended future use to avoid dead code. Locate the sameConnectionInfo method declaration in the BeadsDoltBackend class, then either delete the entire method to clean up unused code or prepend a TODO (and a brief justification) above the method to document why it remains and when it will be used.
467-477: Add timeout toexecBdTextto matchexecBdJsonbehavior.Unlike
execBdJson(which inherits timeout viaexecBdTextnot being used for it), this method lacks a timeout. Ifbd dolt statushangs, the extension could block indefinitely.🔧 Suggested fix
private async execBdText(args: string[]): Promise<string> { const { stdout, stderr } = await execFileAsync(this.bdPath, args, { cwd: this.cwd, env: { ...process.env, BEADS_DIR: this.beadsDir, }, maxBuffer: 10 * 1024 * 1024, + timeout: 30_000, }); return [stdout.trim(), stderr.trim()].filter(Boolean).join("\n").trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/BeadsDoltBackend.ts` around lines 467 - 477, The execBdText helper currently calls execFileAsync(this.bdPath, args, { cwd: this.cwd, env: { ...process.env, BEADS_DIR: this.beadsDir }, maxBuffer: 10 * 1024 * 1024 }) without a timeout, so add the same timeout option used by execBdJson to this execFileAsync call (i.e., include timeout: <the timeout value/property execBdJson uses>), keeping cwd, env and maxBuffer intact; update execBdText to pass timeout to execFileAsync (referencing execBdText, execBdJson, execFileAsync, this.bdPath, this.cwd, this.beadsDir).src/backend/BeadsProjectManager.ts (2)
183-189: Fix indentation inconsistency.The
returnstatement at line 183 has extra leading spaces compared to the surrounding code blocks.🔧 Suggested fix
- return { - state: "running", - message: compatibility.message, - details: { - beadsDir: this.activeProject.beadsDir, - }, - }; + return { + state: "running", + message: compatibility.message, + details: { + beadsDir: this.activeProject.beadsDir, + }, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/BeadsProjectManager.ts` around lines 183 - 189, The returned object block in BeadsProjectManager.ts is over-indented; locate the method containing the shown return (the one returning { state: "running", message: compatibility.message, details: { beadsDir: this.activeProject.beadsDir } }) and adjust the leading spaces so the return line and its object literal align with the surrounding code block indentation style used in that method/class (BeadsProjectManager), removing the extra leading spaces before "return".
275-279: Add timeout to discovery probe to prevent extension hang.The
execFileAsynccall forbd wherelacks a timeout. If the CLI hangs during project discovery, the extension initialization could block indefinitely.🔧 Suggested fix
const { stdout } = await execFileAsync(bdPath, ["where"], { cwd: rootPath, env, maxBuffer: 1024 * 1024, + timeout: 10_000, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/BeadsProjectManager.ts` around lines 275 - 279, The execFileAsync invocation in BeadsProjectManager that runs bd where should include a timeout to avoid blocking extension init; update the call (the execFileAsync(bdPath, ["where"], {...}) in the method) to pass a reasonable timeout option (e.g., timeout in milliseconds) in the options object (or use an AbortSignal wrapper) so the command is killed after the timeout and handle the resulting error path accordingly (ensure the existing stdout handling and error logging in the same method account for timeout failures).src/extension.ts (1)
209-216: Path construction uses redundantUri.filewrapping.Line 209 wraps a
Uri.joinPathresult inUri.file, then immediately accesses.fsPath. This is redundant and could cause path encoding issues on some platforms.🔧 Suggested fix
- const logUri = vscode.Uri.file(vscode.Uri.joinPath(vscode.Uri.file(project.beadsDir), "dolt-server.log").fsPath); + const logUri = vscode.Uri.joinPath(vscode.Uri.file(project.beadsDir), "dolt-server.log");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension.ts` around lines 209 - 216, The code constructs logUri with redundant Uri.file wrapping around the result of vscode.Uri.joinPath; change the assignment so logUri is the Uri returned by vscode.Uri.joinPath(vscode.Uri.file(project.beadsDir), "dolt-server.log") (i.e., remove the extra vscode.Uri.file(... .fsPath) step) so openTextDocument(logUri) and error reporting via log.errorNotify continue to work without path encoding/platform issues.src/backend/BeadsCommandRunner.ts (1)
444-463: Recovery logs raw error messages that may contain sensitive data.Line 445 logs the raw failure message, and line 456 logs the full recovery output. These could contain user data from failed operations. Consider sanitizing or truncating these messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/BeadsCommandRunner.ts` around lines 444 - 463, The tryRecoverDolt method logs full raw CLI messages (rawMessage) and recovery output which may contain sensitive data; update tryRecoverDolt to sanitize/truncate before logging: create or use a small sanitizer helper (e.g., sanitizeLogEntry) and call it on rawMessage and the output returned from runText(["dolt","start"]) and runText(["dolt","status"]); replace direct log.info/log.debug/log.warn calls to include only the sanitized/trimmed string (for example strip newlines, limit to ~200 chars and append "…[TRUNCATED]" when longer) and avoid printing full error stack or payload in logs from tryRecoverDolt and the error catch paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/BeadsCommandRunner.ts`:
- Around line 152-155: The code currently pushes two --estimate flags when both
args.estimated_minutes and args.estimate are set; update BeadsCommandRunner so
you detect this conflict (similar to the existing type/issue_type validation)
and either throw an Error when both are provided or reconcile them (e.g., prefer
one field or verify they are equal and use a single value). Specifically, check
both args.estimated_minutes and args.estimate before building cmdArgs and only
push one --estimate (using String(...) on the chosen value) or raise a clear
error referencing the conflicting fields to prevent sending duplicate flags.
---
Nitpick comments:
In `@src/backend/BeadsCommandRunner.ts`:
- Around line 444-463: The tryRecoverDolt method logs full raw CLI messages
(rawMessage) and recovery output which may contain sensitive data; update
tryRecoverDolt to sanitize/truncate before logging: create or use a small
sanitizer helper (e.g., sanitizeLogEntry) and call it on rawMessage and the
output returned from runText(["dolt","start"]) and runText(["dolt","status"]);
replace direct log.info/log.debug/log.warn calls to include only the
sanitized/trimmed string (for example strip newlines, limit to ~200 chars and
append "…[TRUNCATED]" when longer) and avoid printing full error stack or
payload in logs from tryRecoverDolt and the error catch paths.
In `@src/backend/BeadsDoltBackend.ts`:
- Around line 522-524: The method sameConnectionInfo(a: DoltConnectionInfo, b:
DoltConnectionInfo) is unused; either remove this private method from
BeadsDoltBackend or keep it with a clear TODO comment explaining intended future
use to avoid dead code. Locate the sameConnectionInfo method declaration in the
BeadsDoltBackend class, then either delete the entire method to clean up unused
code or prepend a TODO (and a brief justification) above the method to document
why it remains and when it will be used.
- Around line 467-477: The execBdText helper currently calls
execFileAsync(this.bdPath, args, { cwd: this.cwd, env: { ...process.env,
BEADS_DIR: this.beadsDir }, maxBuffer: 10 * 1024 * 1024 }) without a timeout, so
add the same timeout option used by execBdJson to this execFileAsync call (i.e.,
include timeout: <the timeout value/property execBdJson uses>), keeping cwd, env
and maxBuffer intact; update execBdText to pass timeout to execFileAsync
(referencing execBdText, execBdJson, execFileAsync, this.bdPath, this.cwd,
this.beadsDir).
In `@src/backend/BeadsProjectManager.ts`:
- Around line 183-189: The returned object block in BeadsProjectManager.ts is
over-indented; locate the method containing the shown return (the one returning
{ state: "running", message: compatibility.message, details: { beadsDir:
this.activeProject.beadsDir } }) and adjust the leading spaces so the return
line and its object literal align with the surrounding code block indentation
style used in that method/class (BeadsProjectManager), removing the extra
leading spaces before "return".
- Around line 275-279: The execFileAsync invocation in BeadsProjectManager that
runs bd where should include a timeout to avoid blocking extension init; update
the call (the execFileAsync(bdPath, ["where"], {...}) in the method) to pass a
reasonable timeout option (e.g., timeout in milliseconds) in the options object
(or use an AbortSignal wrapper) so the command is killed after the timeout and
handle the resulting error path accordingly (ensure the existing stdout handling
and error logging in the same method account for timeout failures).
In `@src/extension.ts`:
- Around line 209-216: The code constructs logUri with redundant Uri.file
wrapping around the result of vscode.Uri.joinPath; change the assignment so
logUri is the Uri returned by
vscode.Uri.joinPath(vscode.Uri.file(project.beadsDir), "dolt-server.log") (i.e.,
remove the extra vscode.Uri.file(... .fsPath) step) so openTextDocument(logUri)
and error reporting via log.errorNotify continue to work without path
encoding/platform issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e20c8c8e-4e07-4ee0-b255-a86235161cd7
📒 Files selected for processing (7)
src/backend/BeadsBackend.tssrc/backend/BeadsCommandRunner.tssrc/backend/BeadsDoltBackend.tssrc/backend/BeadsProjectManager.tssrc/extension.tssrc/webview/App.tsxsrc/webview/views/DashboardView.tsx
✅ Files skipped from review due to trivial changes (1)
- src/backend/BeadsBackend.ts
Detect conflicting estimated_minutes and estimate fields (matching the existing type/issue_type validation pattern) and emit a single --estimate flag or throw a clear error. Related: vsbeads-65 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
bdfor discovery, version checks, and Dolt lifecycle control while moving hot read paths off repeated CLIlist/show/commentscallsBeadsDoltBackendthat reads issues, labels, dependencies, and comments directly from Dolt SQLdolt_hashof_db()BeadsCommandRunnerto reflect its current roleDirection Change
This PR started as a CLI-abstraction refactor, but real-world testing showed repeated CLI reads and implicit Dolt auto-start behavior were too slow and unstable for the extension UX.
The branch is now moving toward:
bdfor control-plane operationsNotes
tracewhile actual change detection stays atdebugRelated: vsbeads-65
Summary by CodeRabbit
New Features
Changed
Bug Fixes
Documentation
Chores