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.

Sign up for GitHub

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

Add warning if user is creating scratch with the same name as an exis… #1426

Merged
merged 4 commits into from
Feb 2, 2025
Merged

Add warning if user is creating scratch with the same name as an exis… #1426

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10bbb60
Add warning if user is creating scratch with the same name as an exis…
mkst Feb 1, 2025
9f509ac
Add avatar
mkst Feb 1, 2025
089ff66
belt and braces
mkst Feb 1, 2025
4f9c513
debounce label
mkst Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions frontend/src/app/(navfooter)/new/NewScratchForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ import { scratchUrl } from "@/lib/api/urls";
import basicSetup from "@/lib/codemirror/basic-setup";
import { cpp } from "@/lib/codemirror/cpp";
import getTranslation from "@/lib/i18n/translate";
import { get } from "@/lib/api/request";

import styles from "./new.module.scss";
import type { TerseScratch } from "@/lib/api/types";
import { SingleLineScratchItem } from "@/components/ScratchList";

function getLabels(asm: string): string[] {
const lines = asm.split("\n");
Expand Down Expand Up @@ -69,6 +72,8 @@ export default function NewScratchForm({
const [libraries, setLibraries] = useState<Library[]>([]);
const [presetId, setPresetId] = useState<number | undefined>();

const [duplicates, setDuplicates] = useState([]);

const [ready, setReady] = useState(false);

const [valueVersion, incrementValueVersion] = useReducer((x) => x + 1, 0);
Expand Down Expand Up @@ -257,6 +262,34 @@ export default function NewScratchForm({
}
};

useEffect(() => {
if (!label) {
// reset potential duplicates if no diff label
setDuplicates([]);
return;
}

const filterCandidates = (scratches: TerseScratch[]) => {
return scratches.filter((scratch: TerseScratch) => {
// search endpoint is greedy, so only match whole-name
if (scratch.name !== label) {
return false;
}
// filter on preset if we have it
if (typeof presetId !== "undefined") {
return scratch.preset === presetId;
}
// otherwise filter on platform
return scratch.platform === platform;
});
};

get(`/scratch?search=${label}`)
.then((x) => x.results)
.then(filterCandidates)
.then(setDuplicates);
}, [label, platform, presetId]);
Copy link
Member

Choose a reason for hiding this comment

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

Since label is a text input, every character the user types into it will trigger a network request, which we don't want. I'd probably wrap use useDebounce, like this code that will only make the api call 1000ms after the user finishes typing:

decomp.me/frontend/src/components/Scratch/panels/DecompilePanel.tsx

Lines 24 to 39 in ae05290

const [debouncedContext] = useDebounce(scratch.context, 1000, {
leading: false,
trailing: true,
});
const [valueVersion, setValueVersion] = useState(0);
const url = scratchUrl(scratch);
useEffect(() => {
api.post(`${url}/decompile`, {
context: debouncedContext,
compiler: scratch.compiler,
}).then(({ decompilation }: { decompilation: string }) => {
setDecompiledCode(decompilation);
setValueVersion((v) => v + 1);
});
}, [scratch.compiler, debouncedContext, url]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whilst label is editable by humans, it's most likely auto-populated based on the target asm being pasted in.. but yeh I can wrap it in a debounce to avoid spamming the API if someone starts typing there 👍


return (
<div>
<div>
Expand Down Expand Up @@ -323,6 +356,23 @@ export default function NewScratchForm({
spellCheck={false}
/>
</div>
{duplicates.length > 0 && (
<div className={styles.duplicatesContainer}>
<p>
The following scratches have been found that share this
name:
</p>
<div className={styles.duplicatesList}>
{duplicates.map((scratch) => (
<SingleLineScratchItem
key={scratchUrl(scratch)}
scratch={scratch}
showOwner={true}
/>
))}
</div>
</div>
)}
<div className={styles.editorContainer}>
<p className={styles.label}>
Target assembly <small>(required)</small>
Expand Down
11 changes: 11 additions & 0 deletions frontend/src/app/(navfooter)/new/new.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,14 @@
}
}
}

.duplicatesContainer {
Copy link
Member

Choose a reason for hiding this comment

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

Probably use tailwind instead given #1394

font-size: 0.9rem;
margin-top: 1em;

padding: 2px 10px;
}

.duplicatesList {
padding-left: 1em;
}
32 changes: 3 additions & 29 deletions frontend/src/components/Nav/Search.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useEffect, useRef, useState } from "react";

import Image from "next/image";
import { useRouter } from "next/navigation";

import { SearchIcon } from "@primer/octicons-react";
Expand All @@ -9,14 +8,13 @@ import { useCombobox } from "downshift";
import { useLayer } from "react-laag";

import * as api from "@/lib/api";
import { scratchUrl, userAvatarUrl } from "@/lib/api/urls";
import { scratchUrl } from "@/lib/api/urls";

import LoadingSpinner from "../loading.svg";
import PlatformLink from "../PlatformLink";
import AnonymousFrogAvatar from "../user/AnonymousFrog";
import verticalMenuStyles from "../VerticalMenu.module.scss"; // eslint-disable-line css-modules/no-unused-class

import { getMatchPercentString } from "../ScratchList";
import { getMatchPercentString, ScratchOwnerAvatar } from "../ScratchList";

import styles from "./Search.module.scss";

Expand Down Expand Up @@ -170,31 +168,7 @@ function MountedSearch({ className }: { className?: string }) {
<span>
{getMatchPercentString(scratch)}
</span>
{scratch.owner &&
(!api.isAnonUser(scratch.owner) ? (
userAvatarUrl(scratch.owner) && (
<Image
src={userAvatarUrl(
scratch.owner,
)}
alt={scratch.owner.username}
width={16}
height={16}
className={
styles.scratchOwnerAvatar
}
/>
)
) : (
<AnonymousFrogAvatar
user={scratch.owner}
width={16}
height={16}
className={
styles.scratchOwnerAvatar
}
/>
))}
<ScratchOwnerAvatar scratch={scratch} />
</a>
</li>
);
Expand Down
33 changes: 31 additions & 2 deletions frontend/src/components/ScratchList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@

import { type ReactNode, useState } from "react";

import Image from "next/image";
import Link from "next/link";

import classNames from "classnames";

import TimeAgo from "@/components/TimeAgo";
import * as api from "@/lib/api";
import { presetUrl, scratchUrl } from "@/lib/api/urls";
import { presetUrl, scratchUrl, userAvatarUrl } from "@/lib/api/urls";

import getTranslation from "@/lib/i18n/translate";

import AnonymousFrogAvatar from "./user/AnonymousFrog";
import AsyncButton from "./AsyncButton";
import Button from "./Button";
import LoadingSpinner from "./loading.svg";
Expand Down Expand Up @@ -281,9 +284,34 @@ export function ScratchItemPresetList({
);
}

export function ScratchOwnerAvatar({ scratch }: { scratch: api.TerseScratch }) {
return (
scratch.owner &&
(!api.isAnonUser(scratch.owner) ? (
userAvatarUrl(scratch.owner) && (
Comment on lines +288 to +291
Copy link
Member

Choose a reason for hiding this comment

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

This could be more clearly written without ternaries: if (scratch.owner && ...) { return <Image /> } else { return <Frog /> }. Great idea for a component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just lifted this code verbatim from the Search code, but yeh that makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't appease the linter so gonna leave this as-is.

<Image
src={userAvatarUrl(scratch.owner)}
alt={scratch.owner.username}
width={16}
height={16}
className={styles.scratchOwnerAvatar}
/>
)
) : (
<AnonymousFrogAvatar
user={scratch.owner}
width={16}
height={16}
className={styles.scratchOwnerAvatar}
/>
))
);
}

export function SingleLineScratchItem({
scratch,
}: { scratch: api.TerseScratch }) {
showOwner = false,
}: { scratch: api.TerseScratch; showOwner?: boolean }) {
const matchPercentString = getMatchPercentString(scratch);

return (
Expand All @@ -296,6 +324,7 @@ export function SingleLineScratchItem({
{scratch.name}
</Link>
<div className={styles.metadata}>{matchPercentString}</div>
{showOwner && <ScratchOwnerAvatar scratch={scratch} />}
</li>
);
}