Skip to content
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

feature: Warn user if they leave the /editor page with unsaved changes #109 #139

Closed
wants to merge 10 commits into from
2 changes: 2 additions & 0 deletions src/components/CodeAndPreview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useEffect, useMemo, useState } from "react"
import { useMutation, useQueryClient } from "react-query"
import EditorNav from "./EditorNav"
import { PreviewContent } from "./PreviewContent"
import useWarnUserForUnsavedChanges from "@/hooks/useWarnUserForUnsavedChanges"

interface Props {
snippet?: Snippet | null
Expand Down Expand Up @@ -118,6 +119,7 @@ export function CodeAndPreview({ snippet }: Props) {
}

const hasUnsavedChanges = snippet?.code !== code
useWarnUserForUnsavedChanges({ hasUnsavedChanges })

if (!snippet && (urlParams.snippet_id || urlParams.should_create_snippet)) {
return (
Expand Down
12 changes: 8 additions & 4 deletions src/components/CodeEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export const CodeEditor = ({
onDtsChange?: (dts: string) => void
initialCode: string
readOnly?: boolean
manualEditsJson?: string
isStreaming?: boolean
manualEditsFileContent: string
showImportAndFormatButtons?: boolean
Expand Down Expand Up @@ -86,7 +87,6 @@ export const CodeEditor = ({
Object.entries(files).forEach(([filename, content]) => {
fsMap.set(filename, content)
})

createDefaultMapFromCDN(
{ target: ts.ScriptTarget.ES2022 },
"5.6.3",
Expand Down Expand Up @@ -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}${
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unnecessary change here

pathInPackage ? `/${pathInPackage}` : ""
}`
return fetch(
`${apiUrl}/snippets/download?jsdelivr_resolve=${input.includes("/resolve/")}&jsdelivr_path=${encodeURIComponent(jsdelivrPath)}`,
`${apiUrl}/snippets/download?jsdelivr_resolve=${input.includes(
"/resolve/",
)}&jsdelivr_path=${encodeURIComponent(jsdelivrPath)}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unnecessary change here

)
}
return fetch(input, init)
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would introduce a bug, revert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's always ok!

Copy link
Contributor

@seveibar seveibar Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and no problem- my messages are short because i often type them from my phone, not because i'm angry at you 😁

for (const match of matches) {
if (match.index !== undefined) {
const start = line.from + match.index
Expand Down
42 changes: 42 additions & 0 deletions src/hooks/useWarnUserForUnsavedChanges.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { useEffect } from "react"
export default function useWarnUserForUnsavedChanges({
hasUnsavedChanges,
}: {
hasUnsavedChanges: boolean
}) {
useEffect(() => {
const handleBeforeUnload = (event: BeforeUnloadEvent) => {
if (hasUnsavedChanges) {
event.preventDefault()
event.returnValue = "" // Shows the confirmation dialog on reload or close if there are unsaved changes
}
}

const handlePopState = (event: PopStateEvent) => {
if (hasUnsavedChanges) {
const userConfirmed = window.confirm(
"Are you sure you want to go back? Unsaved changes may be lost.",
)
if (!userConfirmed) {
// Push the state again to stay on the same page
window.history.pushState(null, "", window.location.href)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have to push state to stay on the same page? Can we just not do anything and block the action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if you removed the pushState?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understand your code correctly, preventDefault stops the page from changing, so I'm not sure why you need to pushState

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It basically stores the current url (editor page) and only allows when cancelled. Is there a problem with storing the data in the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove the pushStates, if we only work for reloading that's OK for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you wish but i will just comment out that part just in case you want to update

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will do

}
}
}

// Attach event listeners
window.addEventListener("beforeunload", handleBeforeUnload)
window.addEventListener("popstate", handlePopState)

// Push the initial state only once, when component mounts
if (hasUnsavedChanges) {
window.history.pushState(null, "", window.location.href)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to push state here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we push the states two times:

  1. At the starting of mounting if there is some unsaved changes
  2. if person goes black but cancels then we repush the state to prevent moving back

}

// Cleanup event listeners on component unmount
return () => {
window.removeEventListener("beforeunload", handleBeforeUnload)
window.removeEventListener("popstate", handlePopState)
}
}, [hasUnsavedChanges])
}
Loading