insights: saving a new view of a scoped insight does not trigger recalculation #44679
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 471a445 and cdf3710 or learn more. Open explanation
|
33cb789 to
e0c5e7f
Compare
|
@leonore I pushed my 5 cent commit about optimistic mutation update fix. I haven't test it properly with local BE, I would like to have another 10-15 minutes sync with your just to make sure that everything is in place and ready for full-fledged review. |
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff cdf3710...471a445.
|
|
|
||
| setOriginalInsightFilters(filters) | ||
| history.push(`/insights/dashboard/${ALL_INSIGHTS_DASHBOARD.id}`) | ||
| history.push(`/insights/dashboards/${ALL_INSIGHTS_DASHBOARD.id}`) |
There was a problem hiding this comment.
yes, it looks like this might have been a bug to begin with? our single dashboard url looks like that. it's the only occurrence (apart from some doc entries)
There was a problem hiding this comment.
@coury-clark, this is my change in order to fix this problem that @leonore described above. This change is intentional and it fixes this bad routing problem
| filters, | ||
| title: values.insightName, | ||
| dashboard: null, | ||
| }).toPromise() |
There was a problem hiding this comment.
yes the promise is already returned in the new saveAsNewView hook: https://sourcegraph.com/github.com/sourcegraph/sourcegraph@leo/experiment-save-as-new-view/-/blob/client/web/src/enterprise/insights/core/hooks/use-save-insight-as-new-view.ts?L62-75 (@vovakulikov to confirm?)
There was a problem hiding this comment.
@leonore is right. Previously we used observables since we had to work with a generic GQL client interface (in which we had an Observable interface for this mutation) in this PR Leo implements a new method of doing mutation (more standard), which already returns a promise and doesn't do anything with observables.
final close https://github.com/sourcegraph/sourcegraph/issues/42116
Test plan
Manually tested for the frontend flow.
Integration test added