ui: proper handle of unset app name#83008
Conversation
|
This change is |
xinhaoz
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @maryliag)
pkg/server/combined_statement_stats.go line 431 at r1 (raw file):
buffer.WriteString(" AND (") for i, app := range req.AppNames { if app == "(unset)" {
What do we think about providing the 'empty string app name' in the request itself? Not sure how likely it is we'll be changing it in the client side, I assume it won't be very often but at least we won't have to change the code here if it does.
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/combined_statement_stats.go line 431 at r1 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
What do we think about providing the 'empty string app name' in the request itself? Not sure how likely it is we'll be changing it in the client side, I assume it won't be very often but at least we won't have to change the code here if it does.
the issue is how are you gonna differentiate the cases where you didn't pass any filter vs you want the filter with app name unset, because if we pass empty on unset both cases would have appNames= on the request, and those should have different results
so this way if there is no filter, we don't add the where on the select, but if there is a value (unset) we know we want to filter for specific statement with no app name
|
My comment was a little unclear -- I meant providing what app name would constitute looking for |
maryliag
left a comment
There was a problem hiding this comment.
Changed my approach. Now we only add the search param to the request if any filter was selected, so if there is a search param for appNames and the value is empty string, we know that is the value to look for. Also created the constant to make it easier in case we want to change the value in the future.
Reviewable status:
xinhaoz
left a comment
There was a problem hiding this comment.
Nice, even better
Reviewed all commit messages.
Reviewable status:
|
Threequals (strict equality) please. This feels very nitpicky, but comparisons in JavaScript should always use |
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx line 186 at r2 (raw file):
Previously, nathanstilwell (Nathan Stilwell) wrote…
Threequals (strict equality) please. This feels very nitpicky, but comparisons in JavaScript should always use
===instead of==. Especially with1and empty string. For example,const a = 1; const b = ""; // ALL of these will return true a == 1; a == "1"; a == true; b == ""; b == 0; b == false; // on the other hand, a === 1; // true a === "1"; // false a === true; // false
Done.
Previously, if a filter for `unset` app name was selected on Statement page, the Stament Details page was not finding the correct values. Now if no filter is selected, no search params is passed to the Details page. If there is an empty value for the appNames on request, is looking for unset. This commit also creates a constant for the unset value and replaces all places to use the constant instead. Fixes cockroachdb#83002 Release note (bug fix): Statement Details now find the stats when the `unset` app filter is selected.
|
bors r+ |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from e16c7be to blathers/backport-release-22.1-83008: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Previously, if a filter for
unsetapp namewas selected on Statement page, the Statement Details
page was not finding the correct values.
Now if no filter is selected, no search params is
passed to the Details page. If there is an empty value
for the appNames on request, is looking for unset.
This commit also creates a constant for the unset value
and replaces all places to use the constant instead.
Fixes #83002
https://www.loom.com/share/2bcb83ef35714fc4b90be8d513017eab
Release note (bug fix): Statement Details now find the stats
when the
unsetapp filter is selected.