-
Notifications
You must be signed in to change notification settings - Fork 525
AI Tutor: add warning dialog when mid-accept/reject and trying to navigate between lab2 levels #72140
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
AI Tutor: add warning dialog when mid-accept/reject and trying to navigate between lab2 levels #72140
Changes from all commits
8c6e215
c3108f4
d8cfad5
87220ee
6eebd82
25bd9cc
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 |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import AiTutorVersionFileChip from '@cdo/apps/aiComponentLibrary/aiTutorVersionF | |
| import Lab2Registry from '@cdo/apps/lab2/Lab2Registry'; | ||
| import {isViewingAiTutorVersionFileUpdates} from '@cdo/apps/lab2/redux/lab2ReduxSelectors'; | ||
| import {ProjectFile} from '@cdo/apps/lab2/types'; | ||
| import {DialogType, useDialogControl} from '@cdo/apps/lab2/views/dialogs'; | ||
| import {getAuthenticityToken} from '@cdo/apps/util/AuthenticityTokenStore'; | ||
| import {useAppSelector, useAppDispatch} from '@cdo/apps/util/reduxHooks'; | ||
| import getRejectNotification from '@cdo/apps/weblab2/helpers/getRejectNotification'; | ||
|
|
@@ -40,6 +41,48 @@ const AiTutorVersionActions: React.FC<AiTutorVersionActionsProps> = ({ | |
| ); | ||
|
|
||
| const dispatch = useAppDispatch(); | ||
| const dialogControl = useDialogControl(); | ||
|
|
||
| const handleReject = useCallback(async () => { | ||
| await dispatch(rejectAiTutorVersion(files)); | ||
| }, [dispatch, files]); | ||
|
|
||
| // Presents a confirmation dialog if the user attempts to navigate to another lab2 level with unsaved AI Tutor changes. | ||
| // If the user confirms they want to navigate away, we log a "reject" event and reject the proposed changes. | ||
| useEffect(() => { | ||
|
Contributor
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. Maybe add a comment here and the 'Block in-app level navigation...'
Contributor
Author
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. Done, thanks! Added one to the useEffect below as well. |
||
| Lab2Registry.getInstance().setLevelNavigationConfirmation(async () => { | ||
| if (!dialogControl) { | ||
| return true; | ||
| } | ||
|
|
||
| const {type} = await dialogControl.showDialog({ | ||
| type: DialogType.GenericDialog, | ||
| title: 'Please review AI Tutor changes', | ||
| message: | ||
| "AI Tutor has made changes that you haven't accepted or rejected. If you exit this level, those changes will be lost. Are you sure you want to continue?", | ||
| icon: {iconName: 'triangle-exclamation', iconStyle: 'solid'}, | ||
| showCloseButton: false, | ||
| buttons: { | ||
| confirm: { | ||
| text: 'Stay on this level', | ||
| }, | ||
| cancel: { | ||
| text: 'Continue anyway', | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (type !== 'cancel') { | ||
| return false; | ||
| } | ||
|
|
||
| await handleReject(); | ||
| return true; | ||
| }); | ||
| return () => { | ||
| Lab2Registry.getInstance().setLevelNavigationConfirmation(undefined); | ||
| }; | ||
| }, [dialogControl, handleReject]); | ||
|
|
||
| // Warn the user if they attempt to reload the page before accepting or | ||
| // rejecting the proposed updates. | ||
|
|
@@ -55,6 +98,7 @@ const AiTutorVersionActions: React.FC<AiTutorVersionActionsProps> = ({ | |
| }; | ||
| }, []); | ||
|
|
||
| // Logs a "reject" event if the user navigates away from the page without accepting or rejecting AI Tutor's proposed changes. | ||
| useEffect(() => { | ||
| const possiblyRejectOnPageHide = async (event: PageTransitionEvent) => { | ||
| if (viewingAiTutorVersionFileUpdates) { | ||
|
|
@@ -95,10 +139,6 @@ const AiTutorVersionActions: React.FC<AiTutorVersionActionsProps> = ({ | |
| } | ||
| }, [dispatch, files, commitDescription, isSaving]); | ||
|
|
||
| const handleReject = useCallback(() => { | ||
| dispatch(rejectAiTutorVersion(files)); | ||
| }, [dispatch, files]); | ||
|
|
||
| // Scroll to the bottom of the page when switching to accept mode, | ||
| // so that the commit description input and save button are visible to the user. | ||
| useLayoutEffect(() => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ | |
| // levels without doing page reloads. | ||
|
|
||
| import {setCurrentLevelId} from '@cdo/apps/code-studio/progressRedux'; | ||
| import {levelById} from '@cdo/apps/code-studio/progressReduxSelectors'; | ||
|
|
||
| import Lab2Registry from '../lab2/Lab2Registry'; | ||
| import notifyLevelChange from '../lab2/utils/notifyLevelChange'; | ||
| import {getStore} from '../redux'; | ||
|
|
||
|
|
@@ -30,16 +32,37 @@ export function canChangeLevelInPage(currentLevel, newLevel) { | |
| export function setupNavigationHandler(initialLevelId) { | ||
| // Store the starting level ID in the browser history stack. | ||
| window.history.replaceState({levelId: initialLevelId}, ''); | ||
| window.addEventListener('popstate', function (event) { | ||
| window.addEventListener('popstate', async function (event) { | ||
| const levelId = event.state?.levelId; | ||
| if (!levelId) { | ||
| return; | ||
| } | ||
| const store = getStore(); | ||
| const progressStoreState = store.getState().progress; | ||
| const previousLevelId = progressStoreState.currentLevelId; | ||
| const levelNavigationConfirmation = | ||
| Lab2Registry.getInstance().getLevelNavigationConfirmation(); | ||
| if (levelNavigationConfirmation && !(await levelNavigationConfirmation())) { | ||
| // Restore URL history for the current level when navigation is canceled. | ||
| if (previousLevelId && progressStoreState.currentLessonId) { | ||
| const previousLevel = levelById( | ||
| progressStoreState, | ||
| progressStoreState.currentLessonId, | ||
| previousLevelId | ||
| ); | ||
| if (previousLevel?.path) { | ||
| window.history.pushState( | ||
| {levelId: previousLevelId}, | ||
| '', | ||
| previousLevel.path + window.location.search | ||
| ); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| // Notify the Lab2 system (that handles changing levels without reload) about the level change. | ||
| // The browser history API does not provide access to the state of the page we just came from, | ||
|
Contributor
Author
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. This is true, but we're able to reconstruct the "from" page from Redux (see new logic), so I removed this comment. |
||
| // so we don't know the previous level ID. | ||
| notifyLevelChange(null, levelId); | ||
| getStore().dispatch(setCurrentLevelId(levelId)); | ||
| notifyLevelChange(previousLevelId || null, levelId); | ||
| store.dispatch(setCurrentLevelId(levelId)); | ||
| }); | ||
| } | ||
|
|
||
|
|
||
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.
Nit: maybe as a follow-up down the line, we could extract the navigation guard effects in a hook.
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.
Good thought, I like it!