You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Ignore/Log when --force-browser-download set for Safari
🔧 Implementation Notes
The 2 options are to error quickly or ignore. I went with the behavior that matches what happens when Edge is already downloaded and force download is enabled. Would be entirely valid to fail since user is asking for something that is not possible, but I think there is user convenience in being able to always set it in a matrix job on github, etc.
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix
Description
Ignore force browser download flag for Safari
Log debug message when Safari download requested
Use local discovery instead of attempting download
Diagram Walkthrough
flowchart LR
A["Force download requested"] --> B{"Is Safari?"}
B -->|Yes| C["Log debug message"]
C --> D["Disable download flag"]
D --> E["Use local discovery"]
B -->|No| F["Proceed with download"]
Loading
File Walkthrough
Relevant files
Bug fix
lib.rs
Add Safari force download handling with logging
rust/src/lib.rs
Added check for Safari browser when force download is enabled
Logs debug message explaining Safari downloads are unsupported
Sets download flag to false for Safari to use local discovery instead
Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data.
Status: Unstructured debug log: A new debug log line is added as a plain string and it is not verifiable from the diff whether the logging backend enforces structured logging and redaction guarantees.
Referred Code
self.get_logger().debug("Force browser download requested for Safari, but downloads are not supported; using local discovery",);
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 7
__
Why: This suggestion correctly identifies that using contains is not robust. Proposing eq_ignore_ascii_case correctly addresses both potential case-sensitivity issues and false positives from substring matches, improving the code's correctness and reliability.
Medium
General
Add browser name to log
Add the browser name to the debug log message to provide more context for troubleshooting.
self.get_logger().debug(
- "Force browser download requested for Safari, but downloads are not supported; using local discovery",+ &format!(+ "Force browser download requested for {}, but downloads are not supported; using local discovery",+ self.get_browser_name(),+ ),
);
Apply / Chat
Suggestion importance[1-10]: 4
__
Why: This is a good suggestion for improving log clarity, which can aid in future debugging. The change is minor but enhances maintainability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
💥 What does this PR do?
🔧 Implementation Notes
The 2 options are to error quickly or ignore. I went with the behavior that matches what happens when Edge is already downloaded and force download is enabled. Would be entirely valid to fail since user is asking for something that is not possible, but I think there is user convenience in being able to always set it in a matrix job on github, etc.
🔄 Types of changes
PR Type
Bug fix
Description
Ignore force browser download flag for Safari
Log debug message when Safari download requested
Use local discovery instead of attempting download
Diagram Walkthrough
flowchart LR A["Force download requested"] --> B{"Is Safari?"} B -->|Yes| C["Log debug message"] C --> D["Disable download flag"] D --> E["Use local discovery"] B -->|No| F["Proceed with download"]File Walkthrough
lib.rs
Add Safari force download handling with loggingrust/src/lib.rs