Add progress notifications for build tools#67
Add progress notifications for build tools#67digitarald wants to merge 1 commit intogetsentry:mainfrom
Conversation
|
@digitarald This looks great, I'll give this a test later and update this PR with any feedback I have. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces progress notifications for build tools, allowing asynchronous progress updates throughout the build and simulator run workflows.
- Adds an optional context parameter with notification support in build commands.
- Introduces a new type and helper (registerToolWithProgress) to register tools with progress notifications.
- Updates the iOS simulator build and run logic to emit progress updates at key steps.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/utils/build-utils.ts | Integrates context-based progress notifications into the xcodebuild command. |
| src/tools/common.ts | Adds a new type and helper function to support registration of tools with progress notifications. |
| src/tools/build_ios_simulator.ts | Updates simulator build and run logic to emit progress notifications and switches tool registration to use the new helper. |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| extra: any, // Using any to work around the type mismatch - the actual runtime has sendNotification |
There was a problem hiding this comment.
Consider refining the type for the 'extra' parameter in the wrappedHandler to improve type safety, instead of using 'any'.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| extra: any, // Using any to work around the type mismatch - the actual runtime has sendNotification | |
| extra: ExtraContext, // Replacing 'any' with a specific type to improve type safety |
| log('info', `Starting ${platformOptions.logPrefix} ${buildAction} for scheme ${params.scheme}`); | ||
|
|
||
| // Send initial setup progress | ||
| if (context?.sendNotification && context._meta?.progressToken) { |
There was a problem hiding this comment.
[nitpick] The repeated conditional check for progress notification existence across multiple sections suggests extracting a helper function to reduce duplication and improve readability.
| if (context?.sendNotification && context._meta?.progressToken) { | |
| if (canSendProgressNotification(context)) { |
|
Finally got around to testing and need to fix the access to |
|
@digitarald I'm going to close this for now, it's still an area I want to fix but it's still unclear the best way to handle this. |
2 participants
First stab at #63 for build tools