ui: create ui pages for active queries and transactions #76753
Conversation
|
This change is |
|
TODO: add loom, and tests for utility functions + hooks |
There was a problem hiding this comment.
Reviewed 55 of 55 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsSection.tsx, line 49 at r1 (raw file):
onClearFilters: () => void; onColumnsSelect: (columns: string[]) => void; };
FYI(note) props splitting only needs to happen when connecting a component to redux
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsSection.tsx, line 64 at r1 (raw file):
const tableColumns: SelectOption[] = getColumnOptions(selectedColumns); const data = statements || [];
@xinhaoz data should be removed, statement should be used
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsSection.tsx, line 68 at r1 (raw file):
ascending: true, columnTitle: "startTime", });
Nice, custom react hook! @xinhaoz pointed out that having 2 sources of truth (URL + localStorage) has led to many problems before. The idea/behaviors need to be more flushed out.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 20 at r1 (raw file):
export const ACTIVE_STATEMENT_SEARCH_PARAM = "q"; export function filterActiveStatements(
@xinhaoz oops, the filter was created but it was not used. So, use it 😛
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 42 at r1 (raw file):
): ActiveStatement[] { const activeQueries: ActiveStatement[] = []; if (sessionsResponse.sessions != null) {
nit: invert the conditions here to return early
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 48 at r1 (raw file):
executionID: query.id, transactionID: byteArrayToUuid(query.txn_id), sessionID: byteArrayToUuid(session.id),
note: sessionID is a uint128 instead of UUID. However, it's 16 bytes, which is same as UUID type.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 49 at r1 (raw file):
transactionID: byteArrayToUuid(query.txn_id), sessionID: byteArrayToUuid(session.id), query: query.sql?.length > 0 ? query.sql : query.sql_no_constants,
nit: comment that this is for VIEWACTIVTYREDACTED
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 102 at r1 (raw file):
const executionID = byteArrayToUuid(activeTxn.id); // Find most recent statement. const mostRecentStmt = activeStmts.find(
nit: build a map?
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsTable/activeStatementsTable.tsx, line 116 at r1 (raw file):
return ( <SortedTable columns={columns} className="statements-table" {...rest} />
nit: maybe do that cx() thingy?
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx, line 121 at r1 (raw file):
isSelected: isColumnSelected(selectedColumns, col), })); }
nit: might make sense to refactor this a bit to work on both stmt and txn page.
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 20 at r1 (raw file):
(selectedColumns == null && c.showByDefault !== false) || // show column if list of visible was never defined and can be show by default. (selectedColumns !== null && selectedColumns.includes(c.name)) || // show column if user changed its visibility. c.alwaysShow === true // show column if alwaysShow option is set explicitly.
note to self: check if we need to do this explicit comparision for boolean in typescript
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 22 at r1 (raw file):
c.alwaysShow === true // show column if alwaysShow option is set explicitly. ); };
nice extraction. super small nit move the comment to the top cuz the long lines
pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx, line 308 at r1 (raw file):
...this.state.filters, [field]: event.target.value ||
heh?
Code quote:
event.target.value ||
event.target.checked ||pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 100 at r1 (raw file):
const { value, submitted } = this.state; // eslint-disable-next-line @typescript-eslint/no-unused-vars const { onSubmit, onClear, ...inputProps } = this.props;
nit: comment here to clarify that the props here are pulled out so they are no longer part of inputProps. This allows the typescript typecheck to pass.
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 109 at r1 (raw file):
className={className} placeholder="Search Statements" onChange={this.onChange}
nit: drop the this here since we have pulled the props out already?
pkg/ui/workspaces/cluster-ui/src/sqlActivityContentRoot/sqlActivityContentRoot.tsx, line 24 at r1 (raw file):
}; export const SQLActivityContentRoot = ({
nit: SQLActivityContentBase and a comment indicating that this is meant to be inherited by the Statement and Transaction Page ?
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 584 at r1 (raw file):
.utc() : timeScaleEnd;
nit: changes belonging to a different PR ?
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 40 at r1 (raw file):
export type ActiveStatementsViewDispatchProps = { onColumnsSelect: (columns: string[]) => void; refreshSessions: () => void;
note to self: this effecitvely replaces the old componentDidUpdate
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 74 at r1 (raw file):
clearInterval(interval); }; }, [refreshSessions]);
TIL: it's common practice in React to useEffect with the callback it uses
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 90 at r1 (raw file):
}); resetPagination(); };
nit: syncHistory ?
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 96 at r1 (raw file):
queryParams.delete("app"); } else { queryParams.set("app", selectedFilters.app as string);
(optional) nit: add a TODO here for other filter type ?
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 118 at r1 (raw file):
const clearFilters = () => onSubmitFilters(inactiveFiltersState); const apps = appsFromActiveStatements(statements);
FYI to self, this can be a selector. But a regular utility functions encourages reuse across CC/DB Console. (Also the data are coming from the same source)
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 125 at r1 (raw file):
.filter( stmt => search == null || search === "" || stmt.query.includes(search), );
@xinhaoz: this can probably be extracted
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx, line 651 at r1 (raw file):
</PageConfigItem> </PageConfig> <div className={cx("table-area")}>
note: this is to resolve the scrolling issue fixed in CSS
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageTypes.ts, line 3 at r1 (raw file):
export enum TransactionViewType { ACTIVE = "active", FINGERPRINTS = "fingerprints",
nit: s/fingerprint/historical/ ?
xinhaoz
force-pushed
the
live-query-ui
branch
from
b6d6243 to
f76867c
Compare
There was a problem hiding this comment.
Woooo!! Great stuff 😄. This is big!
I got as far as reviewing pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx (plus a random comment in search.tsx cos I happened to see it). But I thought I would publish the present comments so that you can take a look if you like; my brain is starting to go 😛.
The spacing for the added UI seems little weird; not sure if that's part of that TODO? Even if you don't have the video ready I'd suggest commenting with a few screenshots to get approval from Anne.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsSection.tsx, line 68 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Nice, custom react hook! @xinhaoz pointed out that having 2 sources of truth (URL + localStorage) has led to many problems before. The idea/behaviors need to be more flushed out.
Yay hooks! 🎉
td;lr
Hrrmm... @matthewtodd you might be interested in this as well. In summary, this is the same pattern that I flagged as concerning in the databases page PR, which is that it works by coincidence without using React state. That said, the databases page PR also sets the precedence that @xinhaoz you don't have to address this now, since with the exception of the corner case behavior (see the second section below), it works! It seems like @maryliag had good intuition when she suggested to show and tell on this 😄. I'm hoping to slowly lay some foundation and intuition for this across the next few show and tells. Having seen this twice, I will one day eventually write an RFC.
But if you're interested in this particular issue for now, here is my explanation.
Explanation
In a React app, React state should be used as the source of truth. This provides centralization and consistency that everything else can flow from: you update state, and React will take care of updating everything downstream that you've said is connected to it. Failure to put things in React state can cause corner case bugs where downstream things fail to update because you are working outside of the React system.
@Azhng mentioned above that there are 2 things: "URL + localStorage". To be precise, it's more like "URL + Redux (which happens to be backed by local storage)". I would strongly say that the source of truth should be Redux (i.e. a type of React state, the other one being local component state), and that the URL should not be used as a "source of truth".
It helps to distinguish three things here. There's the:
(1) "initial/default sort selection", aka the "value to initialize state with"
(2) the "current sort selection", aka "current sort state", and
(3) the "derivative of the current sort selection", aka "derivative of state"
Crucially, only (2) is actually state itself. (1) and (3) are not state. (1) is the value to initialize state with, and (3) reads from state. But they are not state.
I see the URL as
(1) the initial sort selection
(3) a derivative of the current sort selection
The Redux value under the localStorage key is
(2) the state
To illustrate things more, we can further distinguish that the Redux value under the localStorage key is not itself the browser local storage (https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage). The Redux key is backed by local storage, which is to say that the existing implementation of the LocalSetting class encapsulates and automatically keeps the two in sync for you. With this in mind, the browser local storage also should not be used as state. The Redux value is the source of truth, so you would read from Redux value under the localStorage key. In contrast, one shouldn't read the sort value via localStorage.getItem('whatever the key is');.
I.e., the browser local storage is also
(1) the initial filter selection
(3) a derivative of the current filter selection
but it is not (2) state.
Corner case behavior
I wrote that failure to put things in React state can cause corner case bugs due to working outside of the React system. This might not exactly be a bug, but it's probably undesired behavior and demonstrates what happens when you don't use React state.
The behavior in the fingerprint tables is probably what we want. To demonstrate:
- go to the statements fingerprints table, and click on one of the columns is set the sort, e.g. "Max Memory"
- go to the transaction fingerprints table, and click on "Contention" to sort
- go back to the statement fingerprints table. "Max Memory" should still be the sort
- and if you go back to the transaction fingerprints table, "Contention" should still be the short
If you do the same thing with the active statements/transactions tables, the sort will always be reset to the default sort. Note that this behavior, switching within the same page, is due to stay being put in React/Redux, and not due to the persistence of local storage. The local storage should provide that the short will persist across browser tabs.
How, concretely, to address this
For addressing this issue, I'd suggest looking at how sort is handled in the existing statements page in cluster-ui/src/statementsPage/statementsPage.tsx. The sort state (2) is stored in Redux, and the component gets passed the state and the setState as props in the connection.
The state is initialized (1) in the constructor from the query params. An alternative to the getStateFromHistory would be to use the useTableSortFromURL introduced here 🙂.
And the query params are derived (3) from changing state in componentDidUpdate, here.
Should the filter be shared with the fingerprints pages? If so, I would make sure to both read from and write to the same Redux state. If they should be independent, use different bits of redux state (I'm not great with redux vocabulary 😅).
Side note
It might seem like the appropriate solution here is unnecessary to do. Why add these layers of indirection, if it works? The same can be said for why use Redux instead of just vanilla React, since Redux further adds additional layers of indirection. It's an assessment of whether these layers of indirection are worth the benefits that these systems bring. And for us, we've decided that both are worth it. And so using React means actually using state 😉.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 29 at r2 (raw file):
); if (search && search !== "") {
nit: "" is falsy (https://developer.mozilla.org/en-US/docs/Glossary/Falsy), so this can be simplified to if (search)
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 38 at r2 (raw file):
} export function activeStatementsFromSessions(
nit: please verb-ify this and other function names that are not currently verbs! s/activeStatementsFromSessions/getActiveStatementsFromSessions
Code quote:
activeStatementsFromSessionspkg/ui/workspaces/cluster-ui/src/activeExecutions/execTableCommon.tsx, line 24 at r2 (raw file):
| "startTime" | "statementCount" | "status";
nit: The string union type here is good, and I think an enum type would be even better! Please make this change for all string union types and usages introduced in this PR.
Docs are here. An enum would an able that when the value is used, it's easy to tell what type it is and what the other possible values are. So like, in places where you would use the string "Executing", you would use ExecutionStatus.Executing. And one can type ExecutionStatus. and have your IDE complete the other possible values.
I made a similar comment for Matthew's databases page PR, and you can see that in the difference between r1 and r3. https://reviewable.io/reviews/cockroachlabs/managed-service/7708#-. I think in later revisions he made some changes needed for typing. Happy to chat or hop on a tuple to help with anything! And yeah, I think @matthewtodd would also helpful, since he made very similar revisions for his own PR.
I should probably also write in RFC for this... one day it will happen 😄
Code quote:
export type ExecutionsColumn =
| "applicationName"
| "elapsedTime"
| "execution"
| "executionID"
| "mostRecentStatement"
| "retries"
| "startTime"
| "statementCount"
| "status";pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsTable/activeStatementsTable.tsx, line 112 at r2 (raw file):
const { selectedColumns, ...rest } = props; const columns = makeActiveStatementsColumns().filter(col => isColumnSelected(selectedColumns, col),
nit: I think s/isColumnSelected/isSelectedColumn would be clearer?
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx, line 121 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: might make sense to refactor this a bit to work on both stmt and txn page.
Just chiming in, it's a pretty small function so I'm not opinionated either way. But if it is gonna be pulled out for sharing, I wonder if similar things are done in the databases pages and other pages with tables that could/should also be shared. So if Xin Hao isn't particularly inspired to do it now, maybe that's a thing to document and file for later.
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 20 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
note to self: check if we need to do this explicit comparision for boolean in typescript
Theoretically, as a counterpoint, 0 == false is true, but 0 === false is not.
In practical usage, I think whether it matters it depends on how leaky your existing type definitions are (e.g. lots of any littered everywhere), and how concerned you are about doing this operation for other edge cases that accidentally pass it (usually, it's fine?).
I think I'm often even more permissive, and just use truthy tests. For PRs if I see if(c.alwaysShow === true || c.alwaysShow != null || c.alwaysShow != undefined || c.alwaysShow != ""), I'll comment with a nit to switch to truthy/falsy (the example is a bit exaggerated, but I set the bar at two things). If I see just one condition, I'm like eh, doesn't matter enough to figure out whether the author is trying to guard against other values/be opinionated on which to use.
The above is a javascript thing. Alternatively, Typescript gives you
const myFunction = (foo: string | boolean) => {
if (foo) {
// doing things specific to strings/booleans will cause typescript to yell
}
if (foo === true) {
// typescript knows here that it's a boolean
}
if (typeof foo == "string") {
// typescript knows here that it's a string
}
}
So typescript will help you if you try to do something that can't be done with one of the truthy values that you might end up with. I think the balance of preferences has to do with type strictness across different languages; R takes it to the extreme of letting you do absolutely everything with anything lol.
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 22 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nice extraction. super small nit move the comment to the top cuz the long lines
+1 on nice extraction 😄
pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx, line 99 at r2 (raw file):
const searchParams = new URLSearchParams(queryString); return Object.keys(defaultFilters).reduce((filters, filter): Filters => {
note: I'm understanding that this is just a type annotation causing an indentation change.
pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx, line 273 at r2 (raw file):
}; insideClick = (event: React.MouseEvent<HTMLDivElement>): void => {
Thank you!
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 100 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: comment here to clarify that the props here are pulled out so they are no longer part of
inputProps. This allows the typescript typecheck to pass.
So... this onSubmit isn't actually being used 🙂. It genuinely isn't being used anywhere in the render function!
The onSubmit that comes from props is actually being used in Line 46 😉.
this.props.onSubmit is the value from props
this.onSubmit is the onSubmit function on the class itself, defined in Line 43.
So to say, I haven't gotten deep enough into this PR to understand why you're omiting onSubmit. But given so, it's in Line 46 where you'll want to test for it and handle the case where it isn't passed.
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 109 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: drop the
thishere since we have pulled the props out already?
Oooo. Archer, if you look closely, this line is not this.props.onChange.
See comment for the addition above.
|
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 100 at r1 (raw file): Previously, jocrl (Josephine) wrote…
Pulling
In the txns and stmts pages, the onSubmit functions passed to the search component are typed as |
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 100 at r1 (raw file):
Ooo, okay. I'm starting to understand this and Archer's request for a comment now.
Rather than a comment, what do you think of either of these two alternatives, to be more explicit via the code that the intention here is to omit keys?
Alternative 1: spread props (for immutable cloning), then use plain javascript delete https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete
Alternative 2: use lodash clone, then omit https://docs-lodash.com/v4/omit/
The onSubmit passed to search is only used in the class function that's used for the form... In the txns and stmts pages, the onSubmit functions passed to the search component are typed as any to avoid the TS errors
Okay, interesting. Thanks for cleaning up the typing!
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 109 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
Oooo. Archer, if you look closely, this line is not
this.props.onChange.See comment for the addition above.
In light of better understanding the above thread, I now don't understand Archer what you mean 😛. There's still currently no onChange variable?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/sqlActivityContentRoot/sqlActivityContentRoot.tsx, line 24 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit:
SQLActivityContentBaseand a comment indicating that this is meant to be inherited by theStatementandTransactionPage ?
A few comments about this component:
Use state, not query params 🙂
Currently, this component has pseudo-state in which the query params is used as the source of truth, and there is no React state. This is the same comment that I previously wrote a long comment for, and that we chatted about. This component should instead be made stateful (I would think local component state, since no other components are concerned with this). Initializing through query params can be done by initializing useState, and updating history when state changes can be done with a useEffect. If you like, you could pull these things out into a custom hook, something like useStateWithSyncedQueryParam.
Composition over inheritance
Per Archer's comment, React prefers composition over inheritance. So instead of thinking of this component as a base, I would think of it as a wrapper. Concretely, it wraps the stmt and transaction page content with radio button controls, so maybe SQLActivityRootControls or StatementsTransactionsRootControls, since it's only for those two pages.
useMemo note
It's not always cost effective to use useMemo, even though it feels tempting and free to do performance optimizations. See this article and this article for more. A common, genuine use case for useMemo is if the function is genuinely expensive.
If the intention here was to save the cost of searching array... I'd point out that this is an array on the order of 2 😄
If the intention here was pre-emptively for referential equality, I refer to this all caps from the second article by Dan the React Dan. It's loud, but makes the point 😅. One generally shouldn't think about re-renders. And if there genuinely are problems due to referential equality, I would justify that in a comment (since the function isn't obviously expensive).
I think the mentality is definitely different from the back end side, where you're really trying to squeeze performance.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/search/search.tsx, line 109 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
In light of better understanding the above thread, I now don't understand Archer what you mean 😛. There's still currently no
onChangevariable?
Oh right, I think I might have missed that this is not from the props 😅
|
pkg/ui/workspaces/cluster-ui/src/activeExecutions/execTableCommon.tsx, line 24 at r2 (raw file): Previously, jocrl (Josephine) wrote…
AFAIK most editors will be able to autocomplete string union types / will complain about incorrectly typed string unions as well and missing values. Is there another specific reason to use string enums over string unions? |
|
pkg/ui/workspaces/cluster-ui/src/activeExecutions/execTableCommon.tsx, line 24 at r2 (raw file): Previously, xinhaoz (Xin Hao Zhang) wrote…
My preference is that it makes PRs easier to read, when you don't have your editor 😛. But I'm down to shelve this till we write an RFC. |
xinhaoz
force-pushed
the
live-query-ui
branch
2 times, most recently
from
5dda1d0 to
571334c
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsSection.tsx, line 64 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
@xinhaoz
datashould be removed,statementshould be used
Done.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsSection.tsx, line 68 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
Yay hooks! 🎉
td;lr
Hrrmm... @matthewtodd you might be interested in this as well. In summary, this is the same pattern that I flagged as concerning in the databases page PR, which is that it works by coincidence without using React state. That said, the databases page PR also sets the precedence that @xinhaoz you don't have to address this now, since with the exception of the corner case behavior (see the second section below), it works! It seems like @maryliag had good intuition when she suggested to show and tell on this 😄. I'm hoping to slowly lay some foundation and intuition for this across the next few show and tells. Having seen this twice, I will one day eventually write an RFC.But if you're interested in this particular issue for now, here is my explanation.
Explanation
In a React app, React state should be used as the source of truth. This provides centralization and consistency that everything else can flow from: you update state, and React will take care of updating everything downstream that you've said is connected to it. Failure to put things in React state can cause corner case bugs where downstream things fail to update because you are working outside of the React system.@Azhng mentioned above that there are 2 things: "URL + localStorage". To be precise, it's more like "URL + Redux (which happens to be backed by local storage)". I would strongly say that the source of truth should be Redux (i.e. a type of React state, the other one being local component state), and that the URL should not be used as a "source of truth".
It helps to distinguish three things here. There's the:
(1) "initial/default sort selection", aka the "value to initialize state with"
(2) the "current sort selection", aka "current sort state", and
(3) the "derivative of the current sort selection", aka "derivative of state"Crucially, only (2) is actually state itself. (1) and (3) are not state. (1) is the value to initialize state with, and (3) reads from state. But they are not state.
I see the URL as
(1) the initial sort selection
(3) a derivative of the current sort selectionThe Redux value under the
localStoragekey is
(2) the stateTo illustrate things more, we can further distinguish that the Redux value under the
localStoragekey is not itself the browser local storage (https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage). The Redux key is backed by local storage, which is to say that the existing implementation of theLocalSettingclass encapsulates and automatically keeps the two in sync for you. With this in mind, the browser local storage also should not be used as state. The Redux value is the source of truth, so you would read from Redux value under thelocalStoragekey. In contrast, one shouldn't read the sort value vialocalStorage.getItem('whatever the key is');.I.e., the browser local storage is also
(1) the initial filter selection
(3) a derivative of the current filter selectionbut it is not (2) state.
Corner case behavior
I wrote that failure to put things in React state can cause corner case bugs due to working outside of the React system. This might not exactly be a bug, but it's probably undesired behavior and demonstrates what happens when you don't use React state.The behavior in the fingerprint tables is probably what we want. To demonstrate:
- go to the statements fingerprints table, and click on one of the columns is set the sort, e.g. "Max Memory"
- go to the transaction fingerprints table, and click on "Contention" to sort
- go back to the statement fingerprints table. "Max Memory" should still be the sort
- and if you go back to the transaction fingerprints table, "Contention" should still be the short
If you do the same thing with the active statements/transactions tables, the sort will always be reset to the default sort. Note that this behavior, switching within the same page, is due to stay being put in React/Redux, and not due to the persistence of local storage. The local storage should provide that the short will persist across browser tabs.
How, concretely, to address this
For addressing this issue, I'd suggest looking at how sort is handled in the existing statements page incluster-ui/src/statementsPage/statementsPage.tsx. The sort state (2) is stored in Redux, and the component gets passed the state and the setState as props in the connection.The state is initialized (1) in the constructor from the query params. An alternative to the
getStateFromHistorywould be to use theuseTableSortFromURLintroduced here 🙂.And the query params are derived (3) from changing state in
componentDidUpdate, here.Should the filter be shared with the fingerprints pages? If so, I would make sure to both read from and write to the same Redux state. If they should be independent, use different bits of redux state (I'm not great with redux vocabulary 😅).
Side note
It might seem like the appropriate solution here is unnecessary to do. Why add these layers of indirection, if it works? The same can be said for why use Redux instead of just vanilla React, since Redux further adds additional layers of indirection. It's an assessment of whether these layers of indirection are worth the benefits that these systems bring. And for us, we've decided that both are worth it. And so using React means actually using state 😉.
Changed filters and sort setting to read from redux, which are in turn synced with the URL.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 20 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
@xinhaoz oops, the filter was created but it was not used. So, use it 😛
Done.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 42 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: invert the conditions here to return early
Done.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 49 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: comment that this is for
VIEWACTIVTYREDACTED
Done.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 102 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: build a map?
Done.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 29 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
nit:
""is falsy (https://developer.mozilla.org/en-US/docs/Glossary/Falsy), so this can be simplified toif (search)
Oh nice, didn't know that! Shortened.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 38 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
nit: please verb-ify this and other function names that are not currently verbs!
s/activeStatementsFromSessions/getActiveStatementsFromSessions
Done.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsTable/activeStatementsTable.tsx, line 112 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
nit: I think
s/isColumnSelected/isSelectedColumnwould be clearer?
Done.
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 20 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
Theoretically, as a counterpoint,
0 == falseis true, but0 === falseis not.In practical usage, I think whether it matters it depends on how leaky your existing type definitions are (e.g. lots of
anylittered everywhere), and how concerned you are about doing this operation for other edge cases that accidentally pass it (usually, it's fine?).I think I'm often even more permissive, and just use truthy tests. For PRs if I see
if(c.alwaysShow === true || c.alwaysShow != null || c.alwaysShow != undefined || c.alwaysShow != ""), I'll comment with a nit to switch to truthy/falsy (the example is a bit exaggerated, but I set the bar at two things). If I see just one condition, I'm like eh, doesn't matter enough to figure out whether the author is trying to guard against other values/be opinionated on which to use.The above is a javascript thing. Alternatively, Typescript gives you
const myFunction = (foo: string | boolean) => { if (foo) { // doing things specific to strings/booleans will cause typescript to yell } if (foo === true) { // typescript knows here that it's a boolean } if (typeof foo == "string") { // typescript knows here that it's a string } }So typescript will help you if you try to do something that can't be done with one of the truthy values that you might end up with. I think the balance of preferences has to do with type strictness across different languages; R takes it to the extreme of letting you do absolutely everything with anything lol.
Note this function was just lifted out of existing code and left unchanged. Going to avoid making further changes (aside from moving comments) to it in this PR so as to not potentially introduce any bugs, if that's okay.
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 22 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
+1 on nice extraction 😄
Done.
pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx, line 308 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
heh?
According to TS, 'value' was never a property on this event object. Actually, taking another look at this, if that is true then we never hit the first case. I've updated it to just remove the event.value case.
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 584 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: changes belonging to a different PR ?
Ah I reused one of the classes here for a link, but it was no longer correct to call it app-name so I renamed it.
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 90 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit:
syncHistory?
Done.
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 125 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
@xinhaoz: this can probably be extracted
Done.
pkg/ui/workspaces/cluster-ui/src/sqlActivityContentRoot/sqlActivityContentRoot.tsx, line 24 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
A few comments about this component:
Use state, not query params 🙂
Currently, this component has pseudo-state in which the query params is used as the source of truth, and there is no React state. This is the same comment that I previously wrote a long comment for, and that we chatted about. This component should instead be made stateful (I would think local component state, since no other components are concerned with this). Initializing through query params can be done by initializing useState, and updating history when state changes can be done with a useEffect. If you like, you could pull these things out into a custom hook, something likeuseStateWithSyncedQueryParam.Composition over inheritance
Per Archer's comment, React prefers composition over inheritance. So instead of thinking of this component as a base, I would think of it as a wrapper. Concretely, it wraps the stmt and transaction page content with radio button controls, so maybeSQLActivityRootControlsorStatementsTransactionsRootControls, since it's only for those two pages.useMemo note
It's not always cost effective to useuseMemo, even though it feels tempting and free to do performance optimizations. See this article and this article for more. A common, genuine use case for useMemo is if the function is genuinely expensive.If the intention here was to save the cost of searching array... I'd point out that this is an array on the order of 2 😄
If the intention here was pre-emptively for referential equality, I refer to this all caps from the second article by Dan the React Dan. It's loud, but makes the point 😅. One generally shouldn't think about re-renders. And if there genuinely are problems due to referential equality, I would justify that in a comment (since the function isn't obviously expensive).
I think the mentality is definitely different from the back end side, where you're really trying to squeeze performance.
Got it, removed!
There was a problem hiding this comment.
Thank you for this PR, and the changes! 😍
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx, line 584 at r4 (raw file):
{timeLabel ? timeLabel : "Statement fingerprint runs longer than"}
nit: I think this can be simplified to {timeLabel || "Statement fingerprint runs longer than"}
Code quote:
{timeLabel
? timeLabel
: "Statement fingerprint runs longer than"}pkg/ui/workspaces/cluster-ui/src/queryFilter/utils.ts, line 26 at r4 (raw file):
} filters[key] = typeof param === "boolean" ? param === "true" : param;
nit: does this line work? URLSearchParams.get returns string | null
Code quote:
typeof param === "boolean" ? param === "true" : param;pkg/ui/workspaces/cluster-ui/src/statementDetails/activeStatementDetails.tsx, line 57 at r4 (raw file):
refreshSessions(); } }, [refreshSessions, statement]);
Nice edge case handling!
pkg/ui/workspaces/cluster-ui/src/statementDetails/activeStatementDetails.tsx, line 88 at r4 (raw file):
</Col> </Row> {statement != null && (
nit: {statement && ...component stuff} is pretty common/I think more common syntax for conditional rendering
Code quote:
statement != null &&pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 74 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
TIL: it's common practice in React to
useEffectwith the callback it uses
@xinhaoz, I like your useEffects across the PR, and this polling! 🎉 Thank you also for making this stateful 😄
note to Archer: useEffect is what replaces the old componentDidUpdate 🙂. Xin Hao's clean up by returning is great, and it replaces the old componentWillUnmount :)
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 185 at r4 (raw file):
loading={statements == null} page="active statements" error={fetchError}
nit: could you please de-verbify this object 😅
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPageRoot.tsx, line 18 at r4 (raw file):
export type StatementsPageRootProps = { activeQueriesView: React.ComponentType; fingerprintsView: React.ComponentType;
Hmm, I find it curious that these two components are specified in their props instead of being hardcoded into this component. Was this from trying to reuse the existing connected components across DB and CC Consoles?
I feel like it's a strange way to do things given that activeQueriesView and fingerprintsView are not generic/arbitrary children to render, but pretty much hardcoded to one specific component, just differently connected. I feel like a cleaner solution would be to connect this component, and hardcode the unconnected ActiveStatementsView and StatementsPage components in this one, passing along their props 🤔. That seems like a more similar solution to whats done with other pages and their sub-parts. WDYT?
Code quote:
activeQueriesView: React.ComponentType;
fingerprintsView: React.ComponentType;pkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx, line 54 at r4 (raw file):
useEffect(() => { if (transaction == null) { // Refresh sessions if the statement was not found initially.
nit: transaction
Code quote:
statementpkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx, line 60 at r4 (raw file):
const returnToActiveTransactions = () => { history.push("/sql-activity?tab=Transactions&view=active");
The equivalent code in the fingerprint details pages likely also needs to be modified to specify the tab. Currently, clicking the back for that leads to active transactions/statements.
There's a chance that after modifying SQLActivityRootControls to be stateful this will be solved if the user previously came from the table page. But modifying the push location will still be needed for if the user starts off loading their tab from the details page.
pkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx, line 93 at r4 (raw file):
</Col> </Row> {transaction != null && (
nit: transaction && ...
Code quote:
transaction != null &&pkg/ui/workspaces/cluster-ui/src/transactionsPage/activeTransactionsView.tsx, line 99 at r4 (raw file):
// Note that the desired behaviour is currently that the user is unable to // clear filters via the URL, and must do so with page controls. if (document.referrer !== "") return;
Just curious what this line is for? I noticed that it's not in this equivalent statements component.
Code quote:
if (document.referrer !== "") return;pkg/ui/workspaces/cluster-ui/src/transactionsPage/activeTransactionsView.tsx, line 159 at r4 (raw file):
history, true, );
Hmm, should this app handling stuff be in the useEffect with syncHistory instead? It looks like it's derived from the filters state.
I'm also curious why I don't see this app handling in the equivalent statements component?
Code quote:
syncHistory(
{
app: app.toString(),
},
history,
true,
);pkg/ui/workspaces/cluster-ui/src/transactionsPage/activeTransactionsView.tsx, line 198 at r4 (raw file):
loading={transactions == null} page="active transactions" error={fetchError}
nit: same here, please de-verbify
pkg/ui/workspaces/cluster-ui/src/transactionsPage/emptyTransactionsPlaceholder.tsx, line 37 at r4 (raw file):
case TransactionViewType.ACTIVE: return { title: "No active SQL statements",
nit: transactions?
Code quote:
statementspkg/ui/workspaces/db-console/src/views/statements/activeStatementDetails.tsx, line 1 at r4 (raw file):
// Copyright 2022 The Cockroach Authors.
nit: could we name the connected files s/activeStatementDetails/activeStatementDetailsConnected? The code base is currently a jumble, but I think that having Connected in the file name is generally useful for distinguishing from the component.
One day we can write an RFC 😅
pkg/ui/workspaces/db-console/src/views/statements/activeStatementsPage.tsx, line 80 at r4 (raw file):
onColumnsSelect: (columns: string[]) => { statementColumnsLocalSetting.set( columns.length === 0 ? " " : columns.join(","),
tiny nit/question: is there any particular for reason for " " instead of ""?
Code quote:
" "pkg/ui/workspaces/cluster-ui/src/sqlActivityContentRoot/sqlActivityContentRoot.tsx, line 24 at r1 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Got it, removed!
Hmm, regarding the query params comment, this component still doesn't have state. Is that in progress, or is the decision to shelve that for now?
There was a problem hiding this comment.
Reviewed 2 of 11 files at r2, 12 of 35 files at r3, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/queryFilter/utils.ts, line 26 at r4 (raw file):
Previously, jocrl (Josephine) wrote…
nit: does this line work?
URLSearchParams.getreturnsstring | null
Hmm, would it be a big deal if we just return an empty Filters instead of null in the case where URL doesn't contain any filters?
pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx, line 74 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
@xinhaoz, I like your
useEffects across the PR, and this polling! 🎉 Thank you also for making this stateful 😄note to Archer:
useEffectis what replaces the oldcomponentDidUpdate🙂. Xin Hao's clean up by returning is great, and it replaces the oldcomponentWillUnmount:)
🎉
There was a problem hiding this comment.
Reviewed 12 of 55 files at r1, 2 of 11 files at r2, 12 of 35 files at r3, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/execTableCommon.tsx, line 110 at r4 (raw file):
placement="bottom" style="tableTitle" content={<p>{/* TODO (xzhang)*/}</p>}
just adding a comment here so we can easily check if that was done before merging :) (same for the ones below)
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementsTable/activeStatementsTable.tsx, line 52 at r4 (raw file):
), sort: (item: ActiveStatement) => item.query, alwaysShow: true,
the alwaysShow is used when we don't want the column to ever be hidden, so if you set this to true you won't see it on the column selectors. We usually set this just for the first column and the action column (when that one exists), so you can remove the values for all those column (the default is false, so you can remove the line completely). Make sure all columns (except first) are being shown on the column selector and also that the right selection are being done correctly.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx, line 52 at r4 (raw file):
), sort: (item: ActiveTransaction) => item.executionID, alwaysShow: true,
same thing as above, only this column should have alwaysShow and remove from all others
pkg/ui/workspaces/cluster-ui/src/columnsSelector/utils.ts, line 20 at r1 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Note this function was just lifted out of existing code and left unchanged. Going to avoid making further changes (aside from moving comments) to it in this PR so as to not potentially introduce any bugs, if that's okay.
the alwaysShow value can be true, false or undefined, I think when it was undefined it was causing issues so I had to add the explicit check for == true. I can't remember exactly what was the issue.
xinhaoz
force-pushed
the
live-query-ui
branch
2 times, most recently
from
35478d4 to
c0ac564
Compare
There was a problem hiding this comment.
Nice! This is a lot of work. I took a quick read through to catch up and just noticed a couple of places where you could use map on an array. Not really to the main point, I know! 😅
Reviewed 21 of 55 files at r1, 4 of 11 files at r2, 29 of 35 files at r3, 7 of 7 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 49 at r5 (raw file):
} sessionsResponse.sessions.forEach(session => {
nit: you can use flatMap for things like this, if you like.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts, line 133 at r5 (raw file):
}); sessionsResponse.sessions.forEach(session => {
nit: maybe a map here?
xinhaoz
force-pushed
the
live-query-ui
branch
2 times, most recently
from
fc12cf1 to
adf5f5f
Compare
There was a problem hiding this comment.
Awesome work on this one. A few questions:
- We mentioned there was a lot of space between the bullets selection and the filters. Did you decreased that space? Would be nice to see an image/video with the latest version of the page
- We also talked about maybe changing the order to be historical first and active second, did we ever got a final decision on that? cc @kevin-v-ngo
- I would also recommend you do a rebase, I notice a few changes on some styles here that might be affected by some other recent PRs :)
Reviewed 1 of 11 files at r2, 2 of 35 files at r3, 1 of 7 files at r5, 4 of 11 files at r7, 1 of 10 files at r8, 1 of 1 files at r9, 2 of 6 files at r10, 16 of 28 files at r12, 2 of 3 files at r13, 21 of 22 files at r14, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsSection.tsx, line 85 at r14 (raw file):
renderNoResult={ <EmptyTransactionsPlaceholder isEmptySearchResults={search?.length > 0 && transactions.length > 0}
why do we need the check if there is a search? what is the difference on the message between no results because there is no active transactions vs no results because nothing matches the search?
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageRoot.tsx, line 45 at r14 (raw file):
executed SQL transactions and their latencies.{" "} <Link to={statementsSql}>Learn more</Link> {/* TODO add 'Learn More' link to documentation page*/}
nit: don't forget this todo
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageRoot.tsx, line 59 at r14 (raw file):
application or elapsed time, to understand and tune workload performance. {/* TODO add 'Learn More' link to documentation page*/}
nit: don't forget this todo
Yeah we decided to swap the order. Was that not done? |
|
@kevin-v-ngo @maryliag Edit: Loom updated! |
There was a problem hiding this comment.
Looks great; thank you! For our synchronous discussions as well 😄
Three minor nits on the new changes, and otherwise everything looks good to me!
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/execTableCommon.tsx, line 100 at r14 (raw file):
“Preparing”, the {execType} is being parsed and planned. If “Executing”, the {execType} is currently being executed.
nit: instead of using ', it might be clearer to read what the symbols are if they were escaped with other string markers?
as in, sub with
{`The status of the ${execType}'s execution. If
“Preparing”, the ${execType} is being parsed and planned.
If “Executing”;, the ${execType} is currently being
executed.`}
Code quote:
The status of the {execType}'s execution. If
“Preparing”, the {execType} is being parsed and planned.
If “Executing”, the {execType} is currently being
executed.pkg/ui/workspaces/cluster-ui/src/selectWithDescription/selectWithDescription.tsx, line 25 at r14 (raw file):
value: string; label: string; description: React.ReactNode;
nit: would this work with ReactChild instead of ReactNode? Per these guidelines https://github.com/cockroachlabs/managed-service/blob/master/docs/frontend-codereview-guide.md#prop-typing
pkg/ui/workspaces/db-console/src/views/transactions/activeTransactionsSelectors.tsx, line 30 at r14 (raw file):
state?: CachedDataReducerState<SessionsResponseMessage>, ): ActiveTransaction[] => { if (state?.data == null) return [];
nit: if you're doing ?. chaining, not having state will return undefined. Do you also want to include that undefined case in this block (i.e., a broader falsy check and not just for == null)?
|
Thanks! CC @Annebirzin as well. |
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsSection.tsx, line 85 at r14 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
why do we need the check if there is a search? what is the difference on the message between no results because there is no active transactions vs no results because nothing matches the search?
There is a different message for if it is empty due to a search filter. It was part of the component design prior to this change.
pkg/ui/workspaces/cluster-ui/src/activeExecutions/execTableCommon.tsx, line 100 at r14 (raw file):
Previously, jocrl (Josephine) wrote…
nit: instead of using
', it might be clearer to read what the symbols are if they were escaped with other string markers?as in, sub with
{`The status of the ${execType}'s execution. If “Preparing”, the ${execType} is being parsed and planned. If “Executing”;, the ${execType} is currently being executed.`}
Good idea! Done.
pkg/ui/workspaces/cluster-ui/src/selectWithDescription/selectWithDescription.tsx, line 25 at r14 (raw file):
Previously, jocrl (Josephine) wrote…
nit: would this work with
ReactChildinstead ofReactNode? Per these guidelines https://github.com/cockroachlabs/managed-service/blob/master/docs/frontend-codereview-guide.md#prop-typing
Just checked and ReactChild works here. Done!
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageRoot.tsx, line 45 at r14 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: don't forget this todo
Filed a new followup issue.
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageRoot.tsx, line 59 at r14 (raw file):
Previously, kevin-v-ngo wrote…
Yeah we decided to swap the order. Was that not done?
Filed a followup issue for the link.
pkg/ui/workspaces/db-console/src/views/transactions/activeTransactionsSelectors.tsx, line 30 at r14 (raw file):
Previously, jocrl (Josephine) wrote…
nit: if you're doing
?.chaining, not havingstatewill returnundefined. Do you also want to include thatundefinedcase in this block (i.e., a broader falsy check and not just for== null)?
AFAIK == null checks for nullish, i.e. null and undefined, so that case should work here.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
New UI looks great! Just a small nit from me. Also, would you mind adding a pic of how it looks when you expand the Description part of the bullets?
Reviewed 1 of 22 files at r14, 1 of 4 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPageRoot.tsx, line 45 at r16 (raw file):
with underscores (_).\nThis can help you quickly identify frequently executed SQL statements and their latencies.`} <Link to={statementsSql}>Learn more</Link>
For Learn More we have been using Anchor (that already has the class with the right blue and target _blank), so you can use that one here
<Anchor href={statementsSql}>Learn More</Anchor>
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageRoot.tsx, line 45 at r14 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Filed a new followup issue.
Same comment as above, replace by <Anchor href={statementsSql}>Learn More</Anchor>
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/selectWithDescription/selectWithDescription.tsx, line 25 at r14 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Just checked and
ReactChildworks here. Done!
Great! Could you also modify the one on line 49, then?
const getDescription = (): React.ReactNode => {
pkg/ui/workspaces/db-console/src/views/transactions/activeTransactionsSelectors.tsx, line 30 at r14 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
AFAIK
== nullchecks for nullish, i.e. null and undefined, so that case should work here.
TIL. Thanks!
|
pkg/ui/workspaces/db-console/src/views/statements/activeStatementDetails.tsx, line 1 at r4 (raw file): Previously, xinhaoz (Xin Hao Zhang) wrote…
Ah - is this just me with reviewable, or does this not look done? |
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/selectWithDescription/selectWithDescription.tsx, line 25 at r14 (raw file):
Previously, jocrl (Josephine) wrote…
Great! Could you also modify the one on line 49, then?
const getDescription = (): React.ReactNode => {
Done.
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPageRoot.tsx, line 45 at r16 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
For
Learn Morewe have been using Anchor (that already has the class with the right blue and target _blank), so you can use that one here
<Anchor href={statementsSql}>Learn More</Anchor>
Done.
pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageRoot.tsx, line 45 at r14 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Same comment as above, replace by
<Anchor href={statementsSql}>Learn More</Anchor>
Done.
pkg/ui/workspaces/db-console/src/views/statements/activeStatementDetails.tsx, line 1 at r4 (raw file):
Previously, jocrl (Josephine) wrote…
Ah - is this just me with reviewable, or does this not look done?
Oops my bad, I did this for the other PR but not here. Done.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Awesome job on this one!
Reviewed 6 of 6 files at r17, all commit messages.
Reviewable status:
This commit creates the pages for active transactins and queries in db-console. Each page calls the /sessions API at an interval of 10 seconds to refresh the data. Release note (ui change): Users can now see actively running queries and transactions in the SQL Activity tab. The transactions and statements tabs in SQL activity now have a menu to show either active or historial transactions and statements data.
|
Thanks for the review, all! |
|
Build succeeded: |
This commit creates the pages for active transactins and
queries in db-console. Each page calls the /sessions API
at an interval of 10 seconds to refresh the data.
Release note (ui change): Users can now see actively running
queries and transactions in the SQL Activity tab. The transactions
and statements tabs in SQL activity now have a menu to show either
active or historial transactions and statements data.
https://www.loom.com/share/964ee2624f4e41d0b6a88d5a280ce6a3