Only allow one open model alerts view#3461
Conversation
shati-patel
force-pushed
the
shati-patel/one-alerts-view
branch
from
d7c8afe to
1241fbf
Compare
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Generally looking good, just a question around where the information will be stored
| export interface ModelEvaluationRun { | ||
| isPreparing: boolean; | ||
| variantAnalysisId: number | undefined; | ||
| isModelAlertsViewOpen?: boolean; |
There was a problem hiding this comment.
Shall we make this a non-optional boolean to avoid having to deal with undefined?
I'm in two minds about whether the ModelEvaluationRun domain entity is the right place to hold information around whether a view is open or not. It feels a bit wrong because it's not a property of the evaluation run, just some information around the user's modeling setup. So it could be information we have in the InternalDbModelingState instead. Any thoughts?
There was a problem hiding this comment.
I think I agree that ModelEvaluationRun feels wrong 🤔 I hadn't thought of InternalDbModelingState, but will try adding it there instead!
|
Pushed a new commit to add |
| selectedMethod: Method | undefined; | ||
| selectedUsage: Usage | undefined; | ||
| modelEvaluationRun: ModelEvaluationRun | undefined; | ||
| isModelAlertsViewOpen: boolean | undefined; |
There was a problem hiding this comment.
Shall we drop the possibility of this being undefined?
| return this.getState(dbItem).isModelAlertsViewOpen ?? false; | ||
| } | ||
|
|
||
| public updateIsModelAlertsViewOpen(dbItem: DatabaseItem, isOpen: boolean) { |
There was a problem hiding this comment.
Can we follow the pattern we use elsewhere for update methods?
2 participants
There should only ever be one model alerts view open, so this PR adds some logic to check that we don't already have one! 👀
The second commit is a related tweak that makes sure we can't open the model alerts view until the variant analysis has kicked off. (This is similar to the MRVA results view, which only opens once the actual workflow run has been triggered, since there's nothing to show otherwise.)
Checklist
N/A—feature-flagged for internal use/testing 🏴
ready-for-doc-reviewlabel there.