Conversation
|
E2E Tests 🚀 |
|
Full suite running at https://github.com/posit-dev/positron/actions/runs/21079125210 In previous runs I noticed some failures due to, it seems, Ark not being properly pulled from the linked PR. Logs showed that it was downloaded from the branch by positron-r, but the test behaviour indicated an older Ark, so not sure what's going on. The following do pass checks locally:
|
948bff1 to
b9ff940
Compare
| @@ -373,27 +373,25 @@ export type CommBackendMessage = | |||
| * Disposing the `DapComm` automatically disposes of the nested `Comm`. | |||
There was a problem hiding this comment.
This is the welcome view but not the streamlined one. I suggested in #10769 (review) to look up the language of the active console when there is no opened editor, to avoid this.
|
|
||
| this._comm = comm; | ||
| this._port = serverPort; | ||
| async connect() { |
There was a problem hiding this comment.
Should all the methods be static?
There was a problem hiding this comment.
nope the methods are stateful and use this
| // Check if the debugger wants breakpoints sent on all saves | ||
| const languageId = model.getLanguageId(); | ||
| const shouldSendAnyway = this.debugService.getAdapterManager() | ||
| .shouldSendBreakpointsOnAllSaves(languageId); | ||
|
|
||
| if (!somethingChanged && !shouldSendAnyway) { | ||
| // --- End Positron --- |
There was a problem hiding this comment.
Something smells funny to me about the fact that this method is onModelDecorationsChanged() and nowhere in the file is "saving" referenced except for this added code. Is shouldSendBreakpointsOnAllSaves() really the right name? Does it have something more general to do with "model decorations" instead? I don't know enough to say whether this is definitely right or wrong.
There was a problem hiding this comment.
It is the right name and right place. I've updated the comment, is this clearer?
// Some debuggers need breakpoints re-sent on every file save, even if line
// numbers haven't changed. If so, we proceed to call `updateBreakpoints()`
// which queues this URI to send breakpoints on the next save.7c8a166 to
01d7c39
Compare
b9ff940 to
50a1f3c
Compare
Because we invalidate breakpoints on every keystrokes
50a1f3c to
6885cab
Compare
|
Full suite with released Ark running at https://github.com/posit-dev/positron/actions/runs/21260187796 |


Branched from #10815
Addresses #1766
Addresses #11402
Backend PR: posit-dev/ark#1003
This PR implements the frontend side of breakpoint support for R in Positron. The backend now injects
browser()calls at parse time, and verifies breakpoints during evaluation. This PR adds the infrastructure needed to:Maintain a permanent DAP session so breakpoints can be communicated to Ark
Notify Ark backend of active breakpoints in a timely manner
sendBreakpointsOnAllSavesonly triggers on save)Permanent DAP session
Previously, the DAP session was only connected when a browser REPL was active. The R extension would send a
start_debugnotification to request Positron to connect with a DAP client. This is no longer the case: the DAP is now expected to always be connected so that the backend can receive notifications about breakpoint state at all time. This allows R to inject breakpoints in executed or sourced code, without requiring the user to enable a debug session first.The
start_debugandstop_debugmessages sent via the Jupyter comm are now hints for showing/hiding the debug toolbar rather than session lifecycle events. When R enters the debugger, Ark sendsstart_debug; when it exits, it sendsstop_debug. The DAP connection remains active throughout.DapComm refactoring
DapCommnow uses a static factory pattern and manages reconnections automatically:DapComm.create()which sets up the comm and configuration.connect()anddisconnect()control the debug session lifecycle.start_debugmessage now callssetSuppressDebugToolbar(false)instead of starting a new session.stop_debugmessage suppresses the toolbar again. It is debounced by 100ms (via newDebouncedutility) to prevent toolbar flickering when stepping through code.Multi-session
In case of multiple console sessions, only the DAP of the foreground session is connected. If all DAP sessions remained connected, then the verification status of breakpoints could be very surprising. A breakpoint verified in a background session but not the foreground session with which the user is working would appear as verified in the UI, even though there is no way the foreground session can break there.
To manage this, the R extension now manages LSP and DAP together through unified
activateServices()anddeactivateServices()methods (previouslyactivateLsp/deactivateLsp). Both services are activated/deactivated in parallel through a shared queue, ensuring proper ordering during session switches.Foreground vs background debug sessions
With a permanent DAP connection, we need to distinguish between "foreground" debug sessions (user is actively debugging) and "background" sessions (DAP connected for breakpoint management only). This distinction affects several behaviors.
suppressDebugToolbaras an indicator of background debug sessionsThe main indication that there is an active foreground debug session is that the debug toolbar is shown, i.e. the
suppressDebugToolbarproperty of the session is not set.In VS Code, it is only possible to start a session in background mode. I made Positron-only changes to allow extensions to flip the debug toolbar on and off. When R enters in a
browser()state, a notification is sent to "unsuppress" the debug toolbar, i.e. bring the debug session to the foreground.New extension API:
Allows extensions to toggle toolbar suppression on a running debug session. The R extension uses this to show the toolbar when entering the debugger and hide it when exiting.
Context key and helpers
CONTEXT_DEBUG_TOOLBAR_SUPPRESSEDcontext key:truewhen all active debug sessions havesuppressDebugToolbarset, which means the debug toolbar is not shown.isForegroundDebugSession()helper: returnstruewhen a debug session is active AND the toolbar is not suppressed.getForegroundDebugState()helper: returns the debug state string only for foreground sessions, otherwise'inactive'.Key distinction:
debug.toolBarLocation: "docked"do NOT affect the context key.suppressDebugToolbarDOES affect the context key.Console history selection
The console maintains separate history navigators for debug vs normal sessions. Previously determined by
CONTEXT_DEBUG_STATE, which didn't account for background sessions with suppressed debug toolbars.Now uses new
isForegroundDebugSession()helper for selecting the history navigator inconsoleInput.tsx, and newgetForegroundDebugState()for tagging history entries inlanguageInputHistory.tsandsessionInputHistory.ts. This ensures that only foreground debug sessions cause commands to be logged in the debug-only history.Debug pane focus
When a debug session starts, VS Code normally focuses the debug pane. Background sessions (with
suppressDebugToolbar: true) should not cause this focus change. ThedebugUxvalue computation now accounts for session suppression.Welcome view visibility
The debug welcome view should appear when there's no "foreground" debug session. Previously it would hide whenever any debug session was active, including background ones. We now check whether all sessions are suppressed and show the welcome view in that case.
Timely breakpoint notifications
First of all, I regret having implementing this here, as this adds significant churn to this PR. This should have been implemented in another PR. Sorry for the extra cognitive load that implies for the review. Given the timeline, I would prefer to keep this here so I don't have to reconsider how verification works across frontend and backend.
The goal here is to avoid stepping through a stale view of an editor. To prevent this, VS Code has an internal mechanism of breakpoint invalidation. Basically if a breakpoint is on a line that was moved up or down, the breakpoint gets invalidated and a breakpoint updated is sent to the backend.
This invalidation is incomplete because if you change code below a breakpoint, it will not get invalidated even though the file and source have changed and might lead to stale stepping. Since The Ark DAP lives alongside an LSP, we can do better: Take document-change notifications as an indication that the file is now stale and breakpoints are invalid. This approach works well but required changes to ensure that we receive breakpoint notifications event when VS Code think there is no need.
Furthermore, it's confusing for users having to save files so that breakpoint verification may work. Since we now have code execution gestures as a way of injecting/verifying breakpoints, I found that the UX was poor because of the need to save.
Two new mechanisms solve these:
The
sendBreakpointsOnAllSavesdebugger capability (described below) ensures Ark receives breakpoints on every file save, even when VS Code thinks breakpoints haven't been invalidated.When executing code via Cmd+Enter in a dirty document, breakpoints haven't been sent since the last save. We now send them right before execution:
VS Code is not equipped by default to verify breakpoints in dirty documents though, so to make it respond to our verification notifications some changes were needed.
Two new debugger contribution properties allow extensions to opt into these behaviors.
sendBreakpointsOnAllSavesVS Code normally only sends
SetBreakpointsDAP events on file save when breakpoint decorations have changed position. This optimization prevents unnecessary DAP traffic but doesn't work for Ark.When
sendBreakpointsOnAllSaves: true, thesomethingChangedcheck inbreakpointEditorContribution.tsis bypassed, ensuring breakpoints are sent on every save.This required:
shouldSendBreakpointsOnAllSaves(languageId)method toIAdapterManager, which checks if any debugger interested in the language has the capability enabledbreakpointEditorContribution.tsto call this method and bypass the early return when the capability is setverifyBreakpointsInDirtyDocumentsVS Code marks breakpoints as unverified when a file has unsaved changes, assuming unsaved changes invalidate breakpoint locations. However, Ark handles source modifications internally and can verify breakpoints even in dirty documents because:
SetBreakpointswithsourceModified: trueon every saveSetBreakpointswhen code is executed in dirty documentsWhen
verifyBreakpointsInDirtyDocuments: true:Breakpoint.verifiedgetter trusts the adapter's verification status even when the file is dirtyTo implement
verifyBreakpointsInDirtyDocuments, theBreakpointclass needs to check whether the debugger supports this capability. This required changing VS Code files with the following fenced adjustments:IDebugServicetoBreakpoint,DebugModel, andDebugStorageconstructorsshouldVerifyBreakpointsInDirtyDocuments(uri)method toIAdapterManager, which checks if any debugger interested in the file's language has the capability enabledverifiedandmessagegetters inBreakpointto call this methodR extension changes
The
positron-rextension's debugger contribution now includes:{ "type": "ark", "label": "R Debugger", "languages": ["r"], "supportsUiLaunch": false, "sendBreakpointsOnAllSaves": true, "verifyBreakpointsInDirtyDocuments": true }And enables breakpoints for R files:
The debug welcome view message is also updated from "limited debugging support" to "first-class debugging support".
Release Notes
New Features
pak::pak("r-lib/pkgload")and callload_all()to activate breakpoints.Bug Fixes
QA notes
See also notes in linked PR
posit-dev/ark#1003
The main additional things to watch out for have to do with the background debug session that is now permanently connected to the frontend (should not affect existing UX related to console history or debug pane in any way), and with the way we validate (verify), invalidate, and revalidate breakpoints.
stop_debug).@:all