Deduplicate breakpoint promotion logic#725
Conversation
|
@ayush99336 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
Pull request overview
This PR factors breakpoint “promotion” (whether to set a runtime function breakpoint) into a shared helper and wires that decision through both the heuristic resolver and the backend-resolved path.
Changes:
- Introduce
shouldPromoteToFunctionBreakpoint()as the single decision point for function-breakpoint promotion. - Use the helper in
resolveSourceBreakpoints()and propagate asetBreakpointflag from backend resolution results. - Add a small decision-table style test in the VS Code smoke suite to validate the helper logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| extensions/vscode/src/dap/sourceBreakpoints.ts | Adds shared promotion helper and replaces duplicated setBreakpoint booleans with helper calls. |
| extensions/vscode/src/cli/debuggerProcess.ts | Computes and returns setBreakpoint for backend breakpoint-resolution results. |
| extensions/vscode/src/dap/adapter.ts | Consumes setBreakpoint from backend resolution instead of recomputing it locally. |
| extensions/vscode/src/test/suites.ts | Adds smoke-test assertions for shouldPromoteToFunctionBreakpoint() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| functionName: bp.function, | ||
| reasonCode: bp.reason_code, | ||
| message: bp.message, | ||
| setBreakpoint: shouldPromoteToFunctionBreakpoint(bp.verified, bp.function, bp.reason_code), | ||
| })); |
There was a problem hiding this comment.
resolveSourceBreakpoints() is declared to return array items without a setBreakpoint property, but the implementation now returns object literals that include setBreakpoint. With strict: true, this will fail TypeScript excess-property checking on the map() return. Update the method’s explicit return type to include setBreakpoint (likely setBreakpoint: boolean or setBreakpoint?: boolean) so it matches the implementation and downstream consumers.
| resolved = serverResolved.map((bp) => ({ | ||
| requestedLine: bp.requestedLine, | ||
| line: bp.line, | ||
| verified: bp.verified, | ||
| functionName: bp.functionName, | ||
| reasonCode: bp.reasonCode, | ||
| message: bp.message, | ||
| setBreakpoint: bp.verified && Boolean(bp.functionName) | ||
| setBreakpoint: bp.setBreakpoint | ||
| })); |
There was a problem hiding this comment.
setBreakpoint can now be true even when the resolved breakpoint’s line differs from the requested line (e.g., HEURISTIC_REANCHORED). However, later in this method managedBreakpoints / syncSourceBreakpoints derive functionName and setBreakpoint by doing resolved.find(...resolvedBreakpoint.line === bp.line). When resolved.line !== requestedLine, that lookup returns undefined, so the promoted function breakpoint will never be synced. Use requestedLine (or otherwise propagate the resolved entry) when associating resolved entries back to the user-requested breakpoints so re-anchored promotions actually take effect.
Closes #650