-
Notifications
You must be signed in to change notification settings - Fork 448
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.
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
feat(sanity): display release errors #8482
Changes from all commits
e417e6f
c6c419f
56671b4
b35bc24
05971f6
14ccade
05cf15c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
import {describe, expect, it} from 'vitest' | ||
|
||
import {NO_EMISSION} from '../../../../../test/matchers/toMatchEmissions' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 |
||
import { | ||
activeUndecidedErrorRelease, | ||
activeUndecidedRelease, | ||
archivedScheduledRelease, | ||
publishedASAPRelease, | ||
scheduledRelease, | ||
} from '../../__fixtures__/release.fixture' | ||
import {releaseStoreErrorCount} from '../createReleaseStore' | ||
|
||
describe('releaseStoreErrorCount', () => { | ||
it('emits the count of releases in an error state when the count changes', async () => { | ||
await expect(releaseStoreErrorCount).toMatchEmissions([ | ||
[ | ||
{ | ||
releases: new Map([['a', activeUndecidedRelease]]), | ||
state: 'loaded', | ||
}, | ||
0, | ||
], | ||
[ | ||
{ | ||
releases: new Map([ | ||
['a', activeUndecidedErrorRelease], | ||
['b', activeUndecidedErrorRelease], | ||
]), | ||
state: 'loaded', | ||
}, | ||
2, | ||
], | ||
[ | ||
{ | ||
releases: new Map([ | ||
['a', activeUndecidedErrorRelease], | ||
['b', activeUndecidedErrorRelease], | ||
['c', archivedScheduledRelease], | ||
['d', publishedASAPRelease], | ||
]), | ||
state: 'loaded', | ||
}, | ||
NO_EMISSION, | ||
], | ||
[ | ||
{ | ||
releases: new Map([ | ||
['a', activeUndecidedErrorRelease], | ||
['b', activeUndecidedErrorRelease], | ||
['c', publishedASAPRelease], | ||
['d', activeUndecidedErrorRelease], | ||
['e', activeUndecidedErrorRelease], | ||
['f', scheduledRelease], | ||
]), | ||
state: 'loaded', | ||
}, | ||
4, | ||
], | ||
[ | ||
{ | ||
releases: new Map(), | ||
state: 'loaded', | ||
}, | ||
0, | ||
], | ||
[ | ||
{ | ||
releases: new Map(), | ||
state: 'loaded', | ||
}, | ||
NO_EMISSION, | ||
], | ||
]) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,17 +4,21 @@ import { | |
catchError, | ||
concat, | ||
concatWith, | ||
count, | ||
filter, | ||
from, | ||
merge, | ||
type Observable, | ||
of, | ||
type OperatorFunction, | ||
pipe, | ||
scan, | ||
shareReplay, | ||
Subject, | ||
switchMap, | ||
tap, | ||
} from 'rxjs' | ||
import {map, startWith} from 'rxjs/operators' | ||
import {distinctUntilChanged, map, startWith} from 'rxjs/operators' | ||
|
||
import {type DocumentPreviewStore} from '../../preview' | ||
import {listenQuery} from '../../store/_legacy' | ||
|
@@ -45,6 +49,12 @@ const QUERY_PROJECTION = `{ | |
"title": "", | ||
"releaseType": "${DEFAULT_RELEASE_TYPE}", | ||
}), | ||
// Content Lake initially encoded non-error states as {error: {message: ""}}. This projection | ||
// ensures the error field only appears if the document has a non-empty error message. | ||
...select( | ||
juice49 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
length(error.message) > 0 => { error }, | ||
{} | ||
), | ||
}` | ||
|
||
// Newest releases first | ||
|
@@ -135,11 +145,30 @@ export function createReleaseStore(context: { | |
shareReplay(1), | ||
) | ||
|
||
const errorCount$ = state$.pipe(releaseStoreErrorCount(), shareReplay(1)) | ||
|
||
const getMetadataStateForSlugs$ = createReleaseMetadataAggregator(client) | ||
|
||
return { | ||
state$, | ||
errorCount$, | ||
getMetadataStateForSlugs$, | ||
dispatch, | ||
} | ||
} | ||
|
||
/** | ||
* @internal | ||
*/ | ||
export function releaseStoreErrorCount(): OperatorFunction<ReleasesReducerState, number> { | ||
return pipe( | ||
switchMap(({releases}) => | ||
from(releases.values()).pipe( | ||
filter((release) => release.state === 'active'), | ||
filter((release) => typeof release.error !== 'undefined'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. COULD: roll this as a single filter, then since the check on state There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree it would make sense to abstract this to some guards: function isActiveRelease(maybeActiveRelease): maybeActiveRelease is ReleaseDocument & { state: 'active' } function isErrorRelease(maybeErrorRelease): maybeErrorRelease is ReleaseDocument & { error: { message: string } } These could then be composed to abstract the logic around whether an error should be displayed. |
||
count(), | ||
), | ||
), | ||
distinctUntilChanged(), | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: more a musing and not blocking for this work, but I wonder how we can get these sorts of error messages to work with i18n. Perhaps in the future it best we avoid just passing through the message and focus on error types so we can localise them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in principle, however
error.message
reflects a technical error from the server and is not intended to be presented to the user in the UI (we do output it, for debugging purposes). When we know about other types of error that will be encapsulated here, we'll need to use some heuristic to show a relevant (localised) message in the UI.