EnxDev commented on code in PR #43376:
URL: https://github.com/apache/superset/pull/43376#discussion_r4082333660
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -51,7 +58,67 @@ const ViewQueryModalContainer = styled.div`
gap: ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const ViewQueryModal: FC<Props> = ({ latestQueryFormData, ownState }) => {
+const InspectorContainer = styled.div`
+ height: 100%;
+
+ .ant-tabs,
+ .ant-tabs-content,
+ .ant-tabs-tabpane {
+ height: 100%;
+ }
+
+ .ant-tabs-tabpane {
+ overflow: auto;
+ }
+`;
+
+const StatsGrid = styled.dl`
+ display: grid;
+ grid-template-columns: max-content 1fr;
+ gap: ${({ theme }) => theme.sizeUnit * 3}px
+ ${({ theme }) => theme.sizeUnit * 6}px;
+ margin: 0;
+
+ dt {
+ color: ${({ theme }) => theme.colorTextSecondary};
+ }
+
+ dd {
+ margin: 0;
+ }
+`;
+
+const getResponseStats = (queriesResponse: QueryData[] | null) => {
+ const responses = queriesResponse ?? [];
+ const serializedResponse = JSON.stringify(responses, null, 2);
Review Comment:
This pretty-prints the whole payload on every render, and line 105
stringifies it a second time for the byte count. That happens even when
`showResponse` is false, so a query-only user pays the cost for a tab they
can't see.
Could we wrap the stats in a `useMemo` keyed on `queriesResponse`, reuse one
compact stringify for the size, and only build the indented string when the
Response tab is shown?
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx:
##########
@@ -750,9 +788,6 @@ test('Dataset drill info API call is made for an
explore-only user', async () =>
(global as any).featureFlags = {
[FeatureFlag.DrillToDetail]: false,
};
- // "View as table" is offered to `canExplore || canViewTable`, so the fetch
that
- // feeds its column headers has to cover the same set -- an explore user with
- // neither `can_samples` nor `can_view_chart_as_table` opens the same modal.
Review Comment:
These comments explained why the drill_info fetch covers explore-only users,
and nothing in this PR touches that logic.
Mind restoring them, and the other rationale comments dropped across this
file? Without them the next person has to rediscover why the gate is shaped
that way.
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -119,6 +183,74 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData,
ownState }) => {
))}
</ViewQueryModalContainer>
);
+
+ if (queriesResponse === undefined) {
+ return queryContent;
+ }
+
+ const {
+ cachedQueries,
+ queryCount,
+ responseBytes,
+ returnedRows,
+ serializedResponse,
+ } = getResponseStats(queriesResponse);
+ const duration =
+ chartUpdateStartTime != null && chartUpdateEndTime != null
+ ? Math.max(0, chartUpdateEndTime - chartUpdateStartTime)
+ : null;
+ const items = [
+ {
+ key: 'query',
+ label: t('Query'),
+ children: queryContent,
+ },
+ ...(showResponse
+ ? [
+ {
+ key: 'response',
+ label: t('Response'),
+ children: queriesResponse?.length ? (
+ <CodeSyntaxHighlighter language="json" showLineNumbers>
Review Comment:
A table chart at a 10k or 50k row limit turns into hundreds of thousands of
lines here, and syntax highlighting that with line numbers will lock up the tab
when someone clicks Response.
Worth capping it: highlight up to some size, and past that fall back to a
plain `<pre>` or a truncated preview plus a way to copy the full JSON. A test
with a large payload would pin the fallback.
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -585,13 +589,21 @@ const SliceHeaderControls = (
label: (
<ModalTrigger
triggerNode={
- <div data-test="view-query-menu-item">{t('View query')}</div>
+ <div data-test="view-query-menu-item">{t('Query inspector')}</div>
}
- modalTitle={t('View query')}
+ modalTitle={t('Query inspector')}
modalBody={
<ViewQueryModal
latestQueryFormData={props.formData}
ownState={props.ownState}
+ queriesResponse={props.queriesResponse}
+ chartUpdateStartTime={props.chartUpdateStartTime}
+ chartUpdateEndTime={
+ props.chartStatus === 'stopped'
+ ? null
+ : props.chartUpdateEndTime
+ }
Review Comment:
`CHART_RENDERING_SUCCEEDED` overwrites `chartUpdateEndTime` with the render
finish time, so on a dashboard (where charts almost always end up `rendered`)
this Duration is fetch plus client render, not query time.
That's a fine number to show, but should the label say so, something like
"Load time" or a tooltip? Someone troubleshooting a slow chart will read
"Duration" as the query.
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -51,7 +58,67 @@ const ViewQueryModalContainer = styled.div`
gap: ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const ViewQueryModal: FC<Props> = ({ latestQueryFormData, ownState }) => {
+const InspectorContainer = styled.div`
+ height: 100%;
+
+ .ant-tabs,
+ .ant-tabs-content,
+ .ant-tabs-tabpane {
+ height: 100%;
+ }
+
+ .ant-tabs-tabpane {
+ overflow: auto;
+ }
+`;
Review Comment:
The core `Tabs` already handles this: `<Tabs fullHeight
allowOverflow={false} items={items} />` gives you full height and a scrolling
pane.
Could we use that and drop the `.ant-tabs*` overrides? Those class selectors
are what tends to break on the next antd bump.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
