chat: fix URL approval settings merge #286218
Merged
+18
−3
Conversation
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
When approving requests and responses for a domain separately, the second approval would overwrite the first instead of merging them. This happened because _approvePattern was replacing the entire settings object rather than merging the new approval flags with existing ones. Now when approving a pattern, we preserve any existing approvals and merge the new approval flags using OR logic, so both approveRequest and approveResponse can be true simultaneously. - Modified _approvePattern to check for and merge existing settings - Uses OR logic to combine new flags with existing approvals - Preserves both request and response approvals when set separately Fixes #286107 (Commit message generated by Copilot)
rzhao271
approved these changes
Jan 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the chat URL fetching approval system where approving requests and responses for the same domain separately would overwrite previous approvals instead of merging them. The fix implements proper merge logic that preserves existing approvals when adding new ones.
Key Changes:
- Added merge logic to combine existing URL approval settings with new approvals using OR operations
- Properly handles both boolean and object-based approval settings when merging
- Ensures that once a request or response is approved for a pattern, it remains approved
Comment on lines
+239
to
260
| // Merge with existing settings for this pattern | ||
| const existingSettings = approvedUrls[pattern]; | ||
| let existingRequest = false; | ||
| let existingResponse = false; | ||
| if (typeof existingSettings === 'boolean') { | ||
| existingRequest = existingSettings; | ||
| existingResponse = existingSettings; | ||
| } else if (existingSettings) { | ||
| existingRequest = existingSettings.approveRequest ?? false; | ||
| existingResponse = existingSettings.approveResponse ?? false; | ||
| } | ||
|
|
||
| const mergedRequest = approveRequest || existingRequest; | ||
| const mergedResponse = approveResponse || existingResponse; | ||
|
|
||
| // Create the approval settings | ||
| let value: boolean | IUrlApprovalSettings; | ||
| if (approveRequest === approveResponse) { | ||
| value = approveRequest; | ||
| if (mergedRequest === mergedResponse) { | ||
| value = mergedRequest; | ||
| } else { | ||
| value = { approveRequest, approveResponse }; | ||
| value = { approveRequest: mergedRequest, approveResponse: mergedResponse }; | ||
| } |
There was a problem hiding this comment.
The merge logic for URL approval settings should have test coverage. This bug fix addresses a critical issue where sequential approvals would overwrite each other instead of merging. Consider adding tests that verify:
- Approving requests then responses results in both being approved
- Approving responses then requests results in both being approved
- The boolean optimization works (when both are true, stores as
trueinstead of{approveRequest: true, approveResponse: true}) - Merging works correctly when starting from a boolean value
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When approving requests and responses for a domain separately, the second approval would overwrite the first instead of merging them.
Fixes #286107