feature: Warn user if they leave the /editor page with unsaved changes #109 #139
Conversation
|
@Satvik1769 is attempting to deploy a commit to the tscircuit Team on Vercel. A member of the Team first needs to authorize it. |
|
@Satvik1769 failing format check, you can fix with |
src/components/CodeEditor.tsx
Outdated
| @@ -221,7 +227,7 @@ export const CodeEditor = ({ | |||
| hoverTooltip((view, pos, side) => { | |||
| const { from, to, text } = view.state.doc.lineAt(pos) | |||
| const line = text.slice(from, to) | |||
| const match = line.match(/@tsci\/[\w\-.]+/) | |||
| const match = line.match(/@tsci\/[\w.]+/) | |||
There was a problem hiding this comment.
you reverted some fixes here. Make sure to only commit changes that are relevent for your fix
src/components/CodeEditor.tsx
Outdated
| window.removeEventListener("beforeunload", handleBeforeUnload) | ||
| window.removeEventListener("popstate", handlePopState) | ||
| } | ||
| }, [hasUnsavedChanges]) |
There was a problem hiding this comment.
this component (the CodeEditor) is already really big, can you put this code in the parent component? Also can you make a dedicated hook e.g. useWarnUserForUnsavedChanges(hasUnsavedChanges)
There was a problem hiding this comment.
i created a hook for the functionality now we just have to call it. it is in hooks folder with name useWarnUserForUnsavedChanges. I also reverted this code to previous state i.e.
const match = line.match(/@tsci/[\w-.]+/)
I hope there is no more conflicts and hope it helps.
|
Ok will fix the issues |
src/components/CodeEditor.tsx
Outdated
| @@ -323,6 +328,10 @@ export const CodeEditor = ({ | |||
| } | |||
| }, [!isStreaming, currentFile, code !== ""]) | |||
|
|
|||
| useWarnUserForUnsavedChanges({ | |||
| hasUnsavedChanges: hasUnsavedChanges ?? false, | |||
| }) | |||
There was a problem hiding this comment.
why does this have to be in the CodeEditor, can you move it to CodeAndPreview? This file is already very large and I don't see why this functionality has to be in here
There was a problem hiding this comment.
Yes it can also be moved in CodeAndPreview and it will work fine.
| ) | ||
| if (!userConfirmed) { | ||
| // Push the state again to stay on the same page | ||
| window.history.pushState(null, "", window.location.href) |
There was a problem hiding this comment.
do we have to push state to stay on the same page? Can we just not do anything and block the action?
There was a problem hiding this comment.
state is necessary here this solution works by storing the current url in the state and if user clicks on cancel instead of sending it to previous page it send it to the current page
There was a problem hiding this comment.
What would happen if you removed the pushState?
There was a problem hiding this comment.
if i understand your code correctly, preventDefault stops the page from changing, so I'm not sure why you need to pushState
There was a problem hiding this comment.
It basically stores the current url (editor page) and only allows when cancelled. Is there a problem with storing the data in the state?
There was a problem hiding this comment.
I believe to add this functionality we will need to have states involved. However, if you have better method in mind I can make it .
There was a problem hiding this comment.
let's remove the pushStates, if we only work for reloading that's OK for now.
There was a problem hiding this comment.
as you wish but i will just comment out that part just in case you want to update
There was a problem hiding this comment.
let's just remove it, i'd rather not have ghost code, people can come back and look at this PR.
Also make sure to revert the changes to files where things were accidentally changed. I'll leave some comments that indicate where that happened. There should be no changes to CodeEditor (IIUC), you can run git checkout origin/main -- path/to/file to revert a file
|
|
||
| // Push the initial state only once, when component mounts | ||
| if (hasUnsavedChanges) { | ||
| window.history.pushState(null, "", window.location.href) |
There was a problem hiding this comment.
why do we need to push state here?
There was a problem hiding this comment.
we push the states two times:
- At the starting of mounting if there is some unsaved changes
- if person goes black but cancels then we repush the state to prevent moving back
| @@ -128,9 +128,13 @@ export const CodeEditor = ({ | |||
| .replace(registryPrefixes[2], "") | |||
| const packageName = fullPackageName.split("/")[0].replace(/\./, "/") | |||
| const pathInPackage = fullPackageName.split("/").slice(1).join("/") | |||
| const jsdelivrPath = `${packageName}${pathInPackage ? `/${pathInPackage}` : ""}` | |||
| const jsdelivrPath = `${packageName}${ | |||
There was a problem hiding this comment.
revert unnecessary change here
| `${apiUrl}/snippets/download?jsdelivr_resolve=${input.includes("/resolve/")}&jsdelivr_path=${encodeURIComponent(jsdelivrPath)}`, | ||
| `${apiUrl}/snippets/download?jsdelivr_resolve=${input.includes( | ||
| "/resolve/", | ||
| )}&jsdelivr_path=${encodeURIComponent(jsdelivrPath)}`, |
There was a problem hiding this comment.
revert unnecessary change here
| @@ -263,7 +267,7 @@ export const CodeEditor = ({ | |||
| for (let pos = from; pos < to; ) { | |||
| const line = view.state.doc.lineAt(pos) | |||
| const lineText = line.text | |||
| const matches = lineText.matchAll(/@tsci\/[\w\-.]+/g) | |||
| const matches = lineText.matchAll(/@tsci\/[\w.]+/g) | |||
There was a problem hiding this comment.
this would introduce a bug, revert
There was a problem hiding this comment.
I am sorry for all this trouble i am thinking to shut this pr and create a cleaner pr that is easier to manage for you and ofcourse myself
There was a problem hiding this comment.
and no problem- my messages are short because i often type them from my phone, not because i'm angry at you 😁
This pr solves issue #109
Using beforeUnload and popStatus event I solved the issue and tested it in various ways manually.
I believe conflicts will not occur now
Please merge this pr as soon as possible.
Regards,
Satvik