back button + popups #2707
back button + popups #2707
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces improvements to navigation and user feedback across various admin pages in the Danswer application. Key changes include:
- Added back button functionality and modified popup behavior in the EmbeddingFormPage component
- Implemented a new
usePopupFromQueryhook for handling popup messages based on URL query parameters - Updated URL handling in FormContext and EmbeddingContext to improve navigation and back button functionality
- Enhanced the indexing status page with a success popup for connector creation
- Improved the search settings page with more detailed information display and a success popup
Potential areas of concern:
- Console.log statements in EmbeddingContext and FormContext should be removed before merging
- The new PopupFromQuery component might benefit from additional error handling and cleanup of URL parameters
8 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings
| updatedSearchParams.set("step", formStep.toString()); | ||
| const newUrl = `${pathname}?${updatedSearchParams.toString()}`; | ||
| router.push(newUrl); | ||
|
|
||
| console.log("existingStep", existingStep); |
There was a problem hiding this comment.
style: Remove this console.log before merging to production
| if (!existingStep) { | ||
| router.replace(newUrl); | ||
| } else if (newUrl !== pathname) { | ||
| router.push(newUrl); | ||
| } |
There was a problem hiding this comment.
logic: This logic may cause unexpected behavior if the URL contains other query parameters
|
|
||
| if (!existingStep) { | ||
| router.replace(newUrl); | ||
| console.log("replacing", newUrl); |
There was a problem hiding this comment.
style: Remove this console.log statement before merging to production
|
|
||
| export const usePopupFromQuery = (messages: PopupMessages) => { | ||
| const router = useRouter(); | ||
| const { popup, setPopup } = usePopup(); |
There was a problem hiding this comment.
style: Consider destructuring setPopup only, as popup is not used in this hook
| const router = useRouter(); | ||
| const { popup, setPopup } = usePopup(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
logic: The effect has missing dependencies. Add router, messages, and setPopup to the dependency array
| for (const key in messages) { | ||
| if (messageValue === key) { | ||
| const popupMessage = messages[key]; | ||
| console.log("popupMessage", popupMessage); |
There was a problem hiding this comment.
style: Remove console.log statement before merging to production
| } | ||
| }, []); | ||
|
|
||
| return { popup }; |
There was a problem hiding this comment.
style: The popup value is not used within the hook, consider not returning it
Description
Fixes https://linear.app/danswer/issue/DAN-757/does-not-take-the-user-back-when-they-click-update-search
How Has This Been Tested?
[Describe the tests you ran to verify your changes]
Accepted Risk
[Any know risks or failure modes to point out to reviewers]
Related Issue(s)
[If applicable, link to the issue(s) this PR addresses]
Checklist: