Add runtime errors resource and tool to clear errors. #94
Conversation
…otReload tool to clear the errors before a hot reload
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
…ors to clear them as well
| @@ -31,7 +32,7 @@ base mixin DartToolingDaemonSupport on LoggingSupport, ToolsSupport { | |||
| /// [VmService] objects are automatically removed from the Map when the | |||
| /// [VmService] shuts down. | |||
| @visibleForTesting | |||
| final activeVmServices = <String, VmService>{}; | |||
| final activeVmServices = <String, Future<VmService>>{}; | |||
There was a problem hiding this comment.
the dart doc needs to be updated since the key for this map changed (debug session id vs vm service uri).
Generally, why the change from vm service uri as the key to debug session id?
There was a problem hiding this comment.
When we get the event for a debug session starting or ending, a VM service URI is not included, only the debug session ID
| await activeVmServices | ||
| .remove((e.data['debugSession'] as DebugSession).id) | ||
| ?.then((service) => service.dispose()); |
There was a problem hiding this comment.
we are already doing this above when the vm service shuts down, do we need to do it here too? will calling dispose twice be a problem? Also, we'd want to avoid calling this here if if doesn't allow us to remove the resource in the onDone handler above
There was a problem hiding this comment.
It is safe to call dispose multiple times.
This also doesn't affect our ability to dispose of the resource (but will trigger it by triggering the onDone future).
Really, this is just defensive, in case somehow the VM service remained active but the debug session was completed. We will shut it down if either DTD tells us to, or the VM service connection closes.
There was a problem hiding this comment.
I did remove the (should be) unnecessary dispose call form the onDone callback though, that only gets completed when the VM service is disposed anyways.
| }) => DebugSession.fromJson({ | ||
| 'debuggerType': debuggerType, | ||
| 'id': id, | ||
| 'name': name, | ||
| 'projectRootPath': projectRootPath, | ||
| 'vmServiceUri': vmServiceUri, | ||
| if (vmServiceUri != null) 'vmServiceUri': vmServiceUri, |
There was a problem hiding this comment.
what type of debug sessions do not have a vm service uri?
There was a problem hiding this comment.
When we get the started notification, they don't have a VM service URI yet. Then we get a follow-up changed notification that does contain the VM service URI.
| ); | ||
| expect( | ||
| (nextResult.content.first as TextContent).text, | ||
| contains('No runtime errors found'), |
There was a problem hiding this comment.
I'm surprised there are no errors even though the clearRuntimeErrors arg was not passed, but maybe I'm not following the logic here.
There was a problem hiding this comment.
The logic is a bit hard to follow - we are passing clearRuntimeErrors in the retry loop above (which is waiting for the error to show up, this test was flaking without that since there is no explicit 1 second delay any more built into this tool).
This then asks for the errors again - to check that the errors were in fact cleared.
There was a problem hiding this comment.
Added a comment above this tool call
This ended up being a fair bit more complicated than I wanted, as it also refactors things and moves us towards a direction where we can support multiple connected apps (which was a natural extension of creating the resource URI, to include the debug session ID in it). Functionally, this: - Starts collecting errors as soon as a debug session is connected, and creates a runtime errors resource for each session. - Allows us to fire change notifications for when new errors come in. - Also added a `clear_runtime_errors` tool as well as an argument to the `hotReload` tool to clear the errors prior to the hot reload. - Drops the support for `since` in favor of just clearing the errors. Eventually we might want to support pagination of the resource instead (it is already modeled as many parts), I filed a feature request on the MCP repo for this, but it likely won't land for some time if at all.
This ended up being a fair bit more complicated than I wanted, as it also refactors things and moves us towards a direction where we can support multiple connected apps (which was a natural extension of creating the resource URI, to include the debug session ID in it).
Functionally, this:
clear_runtime_errorstool as well as an argument to thehotReloadtool to clear the errors prior to the hot reload.sincein favor of just clearing the errors.Eventually we might want to support pagination of the resource instead (it is already modeled as many parts), I filed a feature request on the MCP repo for this, but it likely won't land for some time if at all.