Fixing issue where JSON and XML with a % character breaks opening in a new tab#21585
Fixing issue where JSON and XML with a % character breaks opening in a new tab#21585
% character breaks opening in a new tab#21585Conversation
…n a new editor tab
Benjin
requested review from
aasimkhan30,
allancascante,
caohai,
kburtram,
laurenastrid1,
lewis-sanchez,
manujoseph85 and
ssreerama
as code owners
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where JSON and XML content containing a % character would throw an error when the user clicked to open it in a new tab. The % causes decodeURIComponent to throw a URIError when it encounters a bare % that isn't a valid percent-encoded sequence.
Changes:
- Added a new
decodeQueryResultLinkFragmenthelper inutils.tsthat wrapsdecodeURIComponentin a try/catch, falling back to the raw fragment on error. - Added inline try/catch in
mainController.tsfor thequery-result-linktext document content provider (duplicating the helper logic instead of using it). - Added unit tests for the new helper function.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
extensions/mssql/src/utils/utils.ts |
New decodeQueryResultLinkFragment helper with try/catch fallback |
extensions/mssql/src/controllers/mainController.ts |
Inline try/catch added to provideTextDocumentContent (duplicates helper) |
extensions/mssql/test/unit/utils.test.ts |
Unit tests for the new helper function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Attempt to decode the URI fragment | ||
| try { | ||
| return decodeURIComponent(uri.fragment); | ||
| } catch { | ||
| // If decoding fails, return the raw fragment as a fallback. Some characters (like '%') can cause decodeURIComponent to throw an error. | ||
| return uri.fragment; | ||
| } |
There was a problem hiding this comment.
The new decodeQueryResultLinkFragment helper in utils.ts was created for exactly this purpose, but it's not being used here. Instead, the try/catch logic is duplicated inline. This should call decodeQueryResultLinkFragment(uri.fragment) instead of reimplementing the same logic.
| encoded = encodeURIComponent(original); | ||
| result = utilUtils.decodeQueryResultLinkFragment(encoded); |
There was a problem hiding this comment.
This test case doesn't actually exercise the fallback path. encodeURIComponent('{"test":"%"}') produces %7B%22test%22%3A%22%25%22%7D, where % is encoded as %25. Decoding %25 succeeds and returns %, so decodeURIComponent never throws here.
To test the fallback, you need to pass a string that contains a bare % that isn't a valid percent-encoded sequence (e.g., '{"test":"%"}' directly without encoding it first). That's the scenario the bug describes—a raw % in the URI fragment.
| encoded = encodeURIComponent(original); | |
| result = utilUtils.decodeQueryResultLinkFragment(encoded); | |
| // Pass the raw fragment so that decodeURIComponent throws and the fallback is exercised | |
| result = utilUtils.decodeQueryResultLinkFragment(original); |
There was a problem hiding this comment.
This is untrue; the fallback path is hit when debugging.
PR Changes
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (47.36%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #21585 +/- ##
==========================================
- Coverage 72.82% 72.82% -0.01%
==========================================
Files 331 331
Lines 98825 98842 +17
Branches 5511 5512 +1
==========================================
+ Hits 71973 71982 +9
- Misses 26852 26860 +8
🚀 New features to boost your workflow:
|
Pull Request Template – vscode-mssql
Description
Addresses #20826
XML and JSON containing a
%throws when being decoded with the built-in function. Work-around is to return the raw string if this occurs.Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines