Show a notification for Daemon timeouts on macOS for pre-release users #5796
Conversation
|
@codex review |
|
Thank you for the pull request, DanTup! I've reviewed the changes and they look good. This PR introduces a new mechanism to log and notify pre-release macOS users about Flutter daemon timeouts. This is a valuable addition for diagnosing the issue described in #5793. Here's a summary of the key changes:
These changes are well-implemented and will significantly help in understanding and resolving the daemon timeout issues on macOS. The refactoring of the prompt severity is also a good general improvement for the codebase. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds notifications for Flutter daemon timeouts on macOS for pre-release users to help gather diagnostic information about issue #5793. The timeout mechanism was originally added to track down a Windows-only issue but appears to be recording many timeouts on macOS.
Key changes:
- Enhanced timeout logging with request method names and daemon uptime information
- Added user-facing warning notifications for daemon timeouts (macOS pre-release/dev only)
- Refactored
promptToReloadExtensionparameter from booleanuseErrorto enum-likeseverityfor more granular notification types
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/shared/constants.ts | Added FlutterDaemonTimeout enum value for tracking timeout-related extension restarts |
| src/extension/utils/misc.ts | Updated promptToReloadExtension call to use new severity parameter instead of useError |
| src/extension/utils.ts | Refactored promptToReloadExtension to accept severity with ERROR/WARNING/INFO levels instead of boolean useError |
| src/extension/flutter/flutter_daemon.ts | Added timeout notifications for macOS pre-release users with enhanced logging including request method and daemon uptime |
| src/extension/analytics.ts | Enhanced analytics logging to include the specific request method that timed out |
| src/extension/analysis/analyzer.ts | Updated calls to promptToReloadExtension to use new severity parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is to help gather more information about the issue in #5793, that the timeout added for trying to track down a Windows-only issue seems to be record a lot of timeouts on macOS.
f238920 to
7a7a47a
Compare
There was a problem hiding this comment.
💡 Codex Review
Line 44 in f238920
The commit now logs the timed-out daemon request name by passing it through Analytics.logErrorFlutterDaemonTimeout() into the data field, but the Error_FlutterDaemonTimeout entry in telemetry.json still has an empty schema. Telemetry fields must be declared with their classification/purpose before they can be sent, so this event will now emit undocumented data and will fail the telemetry-compliance checks. Please add a data entry (similar to the other events above it) describing the request name.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This is to help gather more information about the issue in #5793, that the timeout added for trying to track down a Windows-only issue seems to be record a lot of timeouts on macOS.