feat(crud): Bulk update modal COMPASS-7325 #5007
Conversation
lerouxb
added
the
feature flagged
lerouxb
changed the title
|
|
||
| let preview; | ||
| try { | ||
| // TODO(COMPASS-7369): we should automatically retry if the get "Write |
| // conflict during plan execution and yielding is disabled." | ||
| preview = await this.dataService.previewUpdate(ns, filter, update, { | ||
| sample: 3, | ||
| // TODO(COMPASS-7368): aborting the in-flight operation is still buggy, |
| return; | ||
| } | ||
|
|
||
| // TODO(COMPASS-7327): in-progress, success, error in toast |
| @@ -2113,7 +2128,7 @@ class DataServiceImpl extends WithLogContext implements DataService { | |||
| result = await raceWithAbort(start(session), abortSignal); | |||
| } catch (err) { | |||
| if (isCancelError(err)) { | |||
| void abort(); | |||
| await abort(); | |||
There was a problem hiding this comment.
No idea why we didn't do this before.
There was a problem hiding this comment.
because we didn't want to throw if abort failed. now that you changed it, should we catch about error and then continue to throw err, which is relevant in our case?
There was a problem hiding this comment.
We already wrote abort in a way such that it won’t throw, and if it does, that’s a genuine programming error then that we should raise, right?
| await session.abortTransaction(); | ||
| await session.endSession(); | ||
| } catch (err: any) { | ||
| if (isTransactionAbortError(err)) { |
There was a problem hiding this comment.
We might be able to avoid this if we check if the transaction has been aborted or the session otherwise closed after every async call above, but I don't think we can really avoid that. The session can be aborted/killed at any point if we pass in an abortSignal.
| }); | ||
|
|
||
| const codeLightContainerStyles = css({ | ||
| backgroundColor: palette.gray.light3, |
There was a problem hiding this comment.
Not sure how this correlates with the styles below: this is exactly the same color we use in the editor? Is it only the dark mode that's different? Why?
There was a problem hiding this comment.
To match the design. Otherwise it is white. We do the exact same thing in the aggregation stage editor:
There's a ticket to refactor that so we don't keep duplicating it along with the banner area at the bottom.
There was a problem hiding this comment.
Not sure I follow, default multiline background is the same gray color:
Sounds good if we're planning to do a follow-up clean up for this 👍
| }: BulkUpdateDialogProps) { | ||
| const darkMode = useDarkMode(); | ||
|
|
||
| const [text, setText] = useState(updateText); |
There was a problem hiding this comment.
Doesn't look like we reset this when modal is re-opened, is this expected?
There was a problem hiding this comment.
Maybe worth leaving a comment that it's on purpose, we have a bunch of modal forms where we reset forms on modal open, I'm worried we can loose this behavior by accident
This reverts commit 2e38636.
Not very many tests yet, but adding the toast will properly test in-progress, done and error states. And we'll add e2e tests in a later ticket.
There's also probably some duplication between this and the delete modal that we can clean up separately as well.