Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Gen AI: separate "publish" and "save" buttons when filling out model card #57949
Gen AI: separate "publish" and "save" buttons when filling out model card #57949
Changes from 11 commits
3636a8c
5f12660
fba4bcf
648e3fa
397c2da
80d84ea
51988fe
e02d328
2ef7371
2a592a1
6f8779c
6416f46
81b265f
7f8b29e
a180faa
486a02c
c333591
c9e4c54
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These multiple dispatches I believe are not the right way to do this, so looking for input :)
I see we have a couple instances where we put some redux updates that we want to happen after some async operation in this structure:
code-dot-org/apps/src/aichat/redux/aichatRedux.ts
Lines 318 to 331 in 194f7f0
We also dispatch some redux actions inside of the thunk here:
code-dot-org/apps/src/aichat/redux/aichatRedux.ts
Lines 120 to 139 in 194f7f0
Is one of these preferable? Is there a difference? Skimming through docs here, it seems like the first approach might be more standard?
https://redux-toolkit.js.org/api/createAsyncThunk
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.
Seems like either approach is reasonable but defer to @sanchitmalhotra126 on a preference.
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.
Yeah, I mean nothing wrong with back to back dispatches as such, but if the goal here is that we only update the publish state after saving has completed, we should move this into the async thunk. In fact, if we need to save the publish state to the project, it might actually be better to do this in the opposite order - on clicking save, if the model card is not filled out, set hasPublished to false so that when you do save, that value is saved to the project.
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.
Check out the updates in the last two commits...there's at least one thing to figure out (I have an
any
in there 😬 ), but I tried to refactor to use async thunks. I don't see the approach I have here (a shared bit of async code that takes inthunkAPI
as an arg), which usually isn't good. LMK what you think!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.
When the user clicks on 'Publish' and then the 'User View' is displayed, I imagine that the user may want to immediately update Model Card fields and click on 'Edit Mode'. What do you think about saving state so that the 'Publish' tab and not 'Setup' remains open to encourage an iterative process? Also related to comment below, that 'User View' remains displayed?
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 think that makes sense! Currently, we don't manage the selected tab in redux, and the Tabs component I think would need to refactored to manage the selected tab as a prop. We also should probably switch over to using Denys's new Tabs component rather than my home baked one. Would it be ok to do that as a follow-up?
Not sure I'm following this part, happy to chat offline if easier!
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.
That would be great as a follow-up and +1 on using Denys's new Tabs component - thanks!
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.
Ah sorry. I think once we save
hasPublished
to sources, this will not be an issue when a user moves from level to level. I think I was referring to how when a model card has been published, the toggle remain visible.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.
Not sure if this is what others would expect, but it seems like once the model card is published the first time, that unless the model card is modified so that one of the fields is no longer filled out, it remains published but can be updated.
This implementation seems to imply that if a user publishes, and then updates the model card, the model card is then unpublished since message states 'Ready to publish'. Or if the user publishes then refreshes the browser, the model card is no longer published. Is that the intention? This is probably more of a product question @samantha-code .
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.
Yeah good question, it's definitely a bit confusing. I was chatting with Sam about this yesterday and we were struggling a bit to come up with the right solution. Not sure I have the exact right answer, but I think we felt like this was "close enough" and we'd interate.
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.
Do you think if we had different status messages based on whether you have a currently published model or not would be the right path? eg, "Ready to update published model" and "In order to update your published model, you must complete the model card"?
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 defer to @samantha-code but I think that different status messages would be helpful to the user.
I see Sanchit suggested making this a separate component #57949 (comment)
If this was a separate component, we could also add a warning for
RetrievalCustomization
when a user adds/deletes a retrieval but still has to update/save.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.
Would be good to move this to a selector in aichatRedux since it's a state computation that directly drives some UI
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.
Done (I think), LMK if this looks ok!
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 don't exactly know what the performance implications are, but thoughts on just making these components instead of functions that create components every time the parent component is rendered?
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.
hmm good q, I think I just had these returning HTML originally before refactoring :) I think I moved them out of the render function just for readability (and I just added some more logic to cover an edge case Alice identified here). Is there a difference between this and an inline ternary that returns a component on each render? Isn't the component created each time in that case as well, or is React smarter in that case and keeps them around or something?
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.
Yeah, I think the key difference is using functions to render vs values? I don't think it matters a whole lot since these are small, but because they're static components, I was thinking we could benefit from memoizing them. I think we can either just make them static values if they're outside the main component (
const publishOkNotification = <PublishStatus ... props />
), or useuseMemo
inside the component (const publishOkNotification = useMemo(renderPublishOkNotification))
)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.
Mentioned at #57949 (comment), but if a separate component, could use in other customization panels like 'Retrieval' (if retrieval context added/deleted but not updated yet).
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 think this is already ready as a reusable component, just would need to be renamed from
PublishStatus
.d'oh re: static value / no args, updated
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.
Awesome - thanks!