Skip to content
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.

Sign up for GitHub

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(ui): add error messaging for cells in dashdboard view #17754

Merged
merged 3 commits into from
Apr 15, 2020
Merged

fix(ui): add error messaging for cells in dashdboard view #17754

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f34e2e2
fix(ui): add error messaging for cells in dashdboard view
TCL735 Apr 15, 2020
b653c2f
chore: update CHANGELOG
TCL735 Apr 15, 2020
f581a3b
fix(ui): remove spacing above error message in Cells
TCL735 Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
1. [17694](https://github.com/influxdata/influxdb/pull/17694): Fixed typos in the Flux functions list
1. [17701](https://github.com/influxdata/influxdb/pull/17701): Allow mouse cursor inside Script Editor for Safari
1. [17609](https://github.com/influxdata/influxdb/pull/17609): Fixed an issue where Variables could not use other Variables
1. [17754](https://github.com/influxdata/influxdb/pull/17754): Adds error messaging for Cells in Dashboard View

### UI Improvements

Expand Down
8 changes: 6 additions & 2 deletions ui/src/shared/components/EmptyGraphError.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
color: $c-dreamsicle;
border: $cf-border solid $g2-kevlar;

pre {
pre {
font-size: 14px;
padding: $cf-marg-b;
user-select: text !important;
cursor: text;
}
}

.empty-graph-error--icon {
.empty-graph-error--icon {
display: inline-block;
margin-right: $cf-marg-b;
}
Expand All @@ -24,3 +24,7 @@
margin-top: $cf-marg-a;
margin-right: $cf-marg-a;
}

.empty-graph-error--scroll {
margin-top: $cf-marg-c;
}
16 changes: 4 additions & 12 deletions ui/src/shared/components/EmptyGraphError.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
Button,
ComponentSize,
ComponentColor,
Icon,
IconFont,
DapperScrollbars,
} from '@influxdata/clockface'
Expand Down Expand Up @@ -40,19 +41,10 @@ const EmptyGraphError: FunctionComponent<Props> = ({message, testID}) => {
className="empty-graph-error--copy"
/>
</CopyToClipboard>
<DapperScrollbars
className="empty-graph-error--scroll"
autoHide={false}
thumbStartColor="#FF8564"
thumbStopColor="#DC4E58"
>
<DapperScrollbars className="empty-graph-error--scroll" autoHide={true}>
<pre>
<span
className={`icon ${
IconFont.AlertTriangle
} empty-graph-error--icon`}
/>
<code className="cell--error-message">{message}</code>
<Icon glyph={IconFont.AlertTriangle} />
<code className="cell--error-message"> {message}</code>
</pre>
</DapperScrollbars>
</div>
Expand Down
2 changes: 1 addition & 1 deletion ui/src/shared/components/RefreshingView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class RefreshingView extends PureComponent<Props, State> {
}) => {
return (
<EmptyQueryView
errorFormat={ErrorFormat.Tooltip}
errorFormat={ErrorFormat.Scroll}
errorMessage={errorMessage}
hasResults={checkResultsLength(giraffeResult)}
loading={loading}
Expand Down
4 changes: 4 additions & 0 deletions ui/src/shared/components/TimeSeries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ class TimeSeries extends Component<Props & WithRouterProps, State> {

try {
const startTime = Date.now()
let errorMessage: string = ''

// Cancel any existing queries
this.pendingResults.forEach(({cancel}) => cancel())
Expand Down Expand Up @@ -212,10 +213,12 @@ class TimeSeries extends Component<Props & WithRouterProps, State> {

for (const result of results) {
if (result.type === 'UNKNOWN_ERROR') {
errorMessage = result.message
throw new Error(result.message)
}

if (result.type === 'RATE_LIMIT_ERROR') {
errorMessage = result.message
notify(rateLimitReached(result.retryAfter))

throw new Error(result.message)
Expand All @@ -234,6 +237,7 @@ class TimeSeries extends Component<Props & WithRouterProps, State> {

this.setState({
giraffeResult,
errorMessage,
files,
duration,
loading: RemoteDataState.Done,
Expand Down
2 changes: 1 addition & 1 deletion ui/src/shared/utils/checkQueryResult.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

See https://github.com/influxdata/flux/blob/master/docs/SPEC.md#errors.
*/
export const checkQueryResult = (file: string): void => {
export const checkQueryResult = (file: string = ''): void => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do?

Copy link
Contributor Author

@TCL735 TCL735 Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkQueryResult is called here

influxdb/ui/src/shared/components/TimeSeries.tsx

Line 231 in b653c2f

checkQueryResult(result.csv)

At runtime, result.csv may be undefined (not a string) when there is an error. The code immediately above the call to checkQueryResult throws a runtime error which will hide the fact that checkQueryResult will throw its own error when result.csv is not a string.

By adding a default value to the parameter we prevent checkQueryResult from throwing its own error (which would be visible in the console should the throws above it are ever removed).

Copy link
Contributor Author

@TCL735 TCL735 Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, the error messaging will become checkQueryResult's thrown error rather than the actual error. And we want the error message to be useful to the user (rather than the dev). The user won't care or know what to do with "Cannot read property 'slice' of undefined"

Screen Shot 2020-04-15 at 11 43 58 AM

// Don't check the whole file, since it could be huge and the error table
// will be within the first few lines (if it exists)
const fileHead = file.slice(0, findNthIndex(file, '\n', 6))
Expand Down