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 webapp crash when no pipeline #333

Merged
merged 2 commits into from
Dec 8, 2022
Merged

Fix webapp crash when no pipeline #333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
214e301
Fix webapp crash when no pipeline
JosephMarinier Dec 8, 2022
dfdc5d6
Show _No pipeline_ if there is no pipeline
JosephMarinier Dec 8, 2022
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 @@ -20,5 +20,6 @@ Released changes are shown in the

### Fixed
- Showing long utterances fully on hover in similar and perturbed utterances tables.
- Webapp crash when no pipeline.

### Security
2 changes: 1 addition & 1 deletion webapp/src/components/Metrics/PerformanceAnalysisTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ const PerformanceAnalysisTable: React.FC<Props> = ({
<PipelineSelect
selectedPipeline={comparedPipeline}
onChange={setComparedPipeline}
pipelines={config.pipelines!}
pipelines={config.pipelines}
disabledPipelines={[pipeline.pipelineIndex]}
/>
}
Expand Down
4 changes: 2 additions & 2 deletions webapp/src/components/PageHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,11 @@ const PageHeader = () => {
<span className={classes.label}>Project:</span>
<span>{datasetInfo.projectName}</span>
</Typography>
{config && (
{config?.pipelines && (
<PipelineSelect
selectedPipeline={pipeline.pipelineIndex}
onChange={setPipeline}
pipelines={config.pipelines!}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This inappropriate ! was letting a null in, which was crashing PipelineSelect.

pipelines={config.pipelines}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks good to me. Just want to confirm that this is the desired behaviour:
No pipelines -> don't render the selector
rather than
No pipelines -> render empty selector.

Good question @christyler3030. 👍 I wanted to do the quickest fix possible so JavaScript stops crashing and it was midnight so I didn't think too much. Let's take the time now, it's not so much of a rush.

I like your alternative suggestion. In fact, that's how it used to work before the bug was introduced here, so I'll revert to that. It has two (other) benefits:

  1. The place for the select menu is taken while the config loads, as opposed to popping and moving the project name once it loads.
  2. This implementation supports pipelines being [] the same as when it is null.
Suggested change
pipelines={config.pipelines}
pipelines={config?.pipelines ?? []}

On that topic, note that the back end currently crashes with an unclear error message when you configure "pipeline": []. I created an issue to look into this: #335

/>
)}
</>
Expand Down