feat: show error details on insert COMPASS-8864 #6755
Conversation
paula-stacho
force-pushed
the
COMPASS-8864
branch
from
8d6417a to
5b75d6b
Compare
|
Why I'm setting initialFocus: https://mongodb.slack.com/archives/G2L10JAV7/p1740755117364199. But specifically in the context of these changes, it cost me some time and 5 sanity points to figure out why I need to click the "back" button two times 😅 |
packages/compass-components/src/components/modals/error-details-modal.tsx
Show resolved
Hide resolved
| <ErrorDetailsModal | ||
| open={!!errorDetailsOpen} | ||
| onClose={closeErrorDetailsDialog} | ||
| details={errorDetailsOpen?.details} |
There was a problem hiding this comment.
This modal error flow looks real nice! Noticed one small thing:
When we close this modal, we're also unsetting these details, which causes the modal to change in height as it does its close animation (it's not very noticeable, so if this is hard to change no worries). Would it make sense to have an isOpen boolean in the errorDetailsOpen instead of setting it to null to manage it's state? That way the content stays the same as it closes.
There was a problem hiding this comment.
Thank you, this feels much better!
There was a problem hiding this comment.
The change in store alone didn't prevent the flickering, I resolved this by only updating the code content when the modal is open. I also played around with setTimeout for cleaning up the details, but 0 is not enough and I didn't want to risk a race condition with next modal opening. I don't see any flicker if the modal opens with a different content.
| const [stringDetails, setStringDetails] = useState<string>(''); | ||
|
|
||
| useEffect(() => { | ||
| if (open) { | ||
| setStringDetails(JSON.stringify(details, undefined, 2)); | ||
| } | ||
| }, [details, open]); |
There was a problem hiding this comment.
This really doesn't belong in the effect, this is a derived value that, if we're trying to optimise the rendering here a bit and avoid stringifying too often, should be a memo.
There was a problem hiding this comment.
It was a memo, I changed it now because we saw a flicker (see conversation above). we set open -> close, details -> no details at the same time, but it renders sequentially - the modal closes a bit later than the details disappear. I see it as a problem with rendering, my best solution was to keep the details when the modal is closing.
I could rewrite this with memo + previousDetails ref, but this made more sense to me. But as I'm explaining it I see we need a comment at least. Would you prefer the memo + ref? Or do you have a better idea? I do admit it feels a bit hackish, maybe I'm missing something weird that we do that causes the issue.
There was a problem hiding this comment.
Insert document modal has the same flicker. Bulk update does not.
There was a problem hiding this comment.
Would an alternative be to not un-set the error details when we close it? Then we can fully use the prop with useMemo. Not a blocker for me, might be nice to mention why we are doing it that way in a comment for future folks though.
There was a problem hiding this comment.
my best solution was to keep the details when the modal is closing.
This is already a store state, if you want to not reset it when the modal is closed, just don't do it in the store itself and reset it on open in the store logic instead of splitting this logic between store and component in a hard to control and explain way.
Would an alternative be to not un-set the error details when we close it?
Yes, we already do this in a couple of places
There was a problem hiding this comment.
Alright, I will change it in the store then. It seemed off to me as the store shouldn't be concerned with the rendering timing. But if it's a common pattern already, it'll at least lower the confusion
Description
Side changes:
The new dialog lives in the document tab. Even though the button says back, it is opened on top of the previous tab, not instead of. This is so that we can easily reuse it for other operations, whether they originate in a modal or directly from the documents list. Let me know if you see problems with this.
Added e2e test.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes