-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix undefined node error on save - part 2 #2820
Conversation
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## maintenance #2820 +/- ##
===============================================
+ Coverage 93.29% 93.44% +0.14%
===============================================
Files 103 103
Lines 10665 10673 +8
Branches 2319 2318 -1
===============================================
+ Hits 9950 9973 +23
+ Misses 714 699 -15
Partials 1 1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be ok to rebase this draft against main
?
or were we planning on yet another patch release? 😋
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Quality Gate passedIssues Measures |
I believe we plan another patch release, so I've added this PR to the 2.15.3 milestone 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! 😋
And I'm not getting any cannot read property of undefined (reading ...)
@@ -375,9 +379,13 @@ export async function compareFileContent( | |||
label?: string, | |||
profile?: imperative.IProfileLoaded | |||
): Promise<void> { | |||
node = node ?? TreeProviders.ds.openFiles?.[doc.uri.fsPath] ?? TreeProviders.uss.openFiles?.[doc.uri.fsPath]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we should also search for the node in the uploadContent
method before we check for == null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time uploadContent
is called, we should have already handled the case where node is null or undefined in saveFile
for data sets or saveUSSFile
for USS files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks @t1m0thyj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey all, just adding the changes requested to ask about the Theia test on this branch that are broken? Looks like only the first test passed and the rest fail.
@JillieBeanSim Good catch, in order to fix the Theia tests we'll need to cherry-pick some commits from #2838. Should I create a separate PR that contains just those commits? I don't want to be blocking other PRs going into maintenance branch with failing Theia tests 😋 |
broken Theia tests are not specific to the branch, breakages are already introduced to maintenance branch |
Hey @t1m0thyj no worries, having the fix in other PR for maintenance branch is good and can link the open issue I created to track #2855 😄 |
Proposed changes
openFiles
map prematurely when diff editor is closedRelease Notes
Milestone: 2.15.3
Changelog: Fixed issue where saving changes to favorited data set or USS file could fail when it is opened outside of favorites
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the revieweryarn workspace vscode-extension-for-zowe vscode:prepublish
has been executedFurther comments