Rebuild from scratch #569
Rebuild from scratch #569
Conversation
… into react-rewrite
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a React-based frontend (routes, layout, many tool pages, UI component library), Convex backend integration with BetterAuth (auth, backups, schema), CI/build/tooling changes (pnpm → bun, Dockerfile), workspace scripts, and assorted config and ignore updates; also deletes copilot instructions and Prettier config. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as React App
participant Auth as BetterAuth/AuthClient
participant Convex as Convex Backend
participant UI as App UI (Layout/Components)
User->>Browser: Navigate to protected route
Browser->>Auth: Check session / sign-in state
Auth->>Convex: Request auth/session validation
Convex-->>Auth: Return user/session or null
Auth-->>Browser: Session result
Browser->>UI: Render layout (sidebar/header) with feature flags
UI->>Convex: Fetch page data (e.g., backups, admin users, games)
Convex-->>UI: Return data
UI-->>User: Render page and enable actions (create/delete/restore)
User->>UI: Trigger mutation (e.g., createBackup, banUser)
UI->>Convex: Invoke mutation
Convex-->>UI: Return mutation result
UI-->>User: Show toast/update UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying edutools-testing with
|
| Latest commit: |
ad14db1
|
| Status: | 🚫 Build failed. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| lastIndex = regex.lastIndex; | ||
|
|
||
| if (!flags.global) break; | ||
| } |
There was a problem hiding this comment.
Bug: Regex exec loop can freeze browser with empty matches
The highlightMatches function uses regex.exec() in a while loop with global regex patterns, but doesn't handle the case where the regex matches an empty string. When a pattern like //g, /a*/g, or /^/gm matches empty strings, regex.lastIndex won't advance, causing an infinite loop that freezes the browser. The loop needs to manually advance lastIndex when match[0] is empty.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (13)
app/routes/tools/number-base.tsx-8-35 (1)
8-35: Validate or document the prefix-stripping behavior.The function strips common numeric prefixes (0x, 0b, 0o) regardless of the
fromBaseparameter. This can produce incorrect results when the prefix doesn't match the base. For example, if the input is "0x10" butfromBaseis 2 (binary), the function strips "0x" and parses "10" as binary (= 2 decimal) instead of treating it as hexadecimal (= 16 decimal).Additionally, negative numbers with prefixes (e.g., "-0x10") are not handled—the negative sign appears after the prefix strip, which may cause parsing issues.
Consider one of these approaches:
Option 1: Validate prefix against fromBase
function convertBase(value: string, fromBase: number, toBase: number): string { try { // Remove any prefix if present let cleanValue = value.trim(); - if (cleanValue.startsWith("0x") || cleanValue.startsWith("0X")) { + const isNegative = cleanValue.startsWith("-"); + const absValue = isNegative ? cleanValue.slice(1) : cleanValue; + + if ((absValue.startsWith("0x") || absValue.startsWith("0X")) && fromBase === 16) { - cleanValue = cleanValue.slice(2); + cleanValue = absValue.slice(2); - } else if (cleanValue.startsWith("0b") || cleanValue.startsWith("0B")) { + } else if ((absValue.startsWith("0b") || absValue.startsWith("0B")) && fromBase === 2) { - cleanValue = cleanValue.slice(2); + cleanValue = absValue.slice(2); - } else if (cleanValue.startsWith("0o") || cleanValue.startsWith("0O")) { + } else if ((absValue.startsWith("0o") || absValue.startsWith("0O")) && fromBase === 8) { - cleanValue = cleanValue.slice(2); + cleanValue = absValue.slice(2); + } else { + cleanValue = absValue; } + + if (isNegative) cleanValue = "-" + cleanValue; // Convert to decimal first const decimal = parseInt(cleanValue, fromBase);Option 2: Auto-detect base from prefix
Remove
fromBasevalidation and always auto-detect the base from the prefix, then document this behavior clearly in the UI.app/routes/tools/json-formatter.tsx-64-81 (1)
64-81: Fix stale closure bug in auto-format logic.Line 69 checks
if (!input)using the old state value beforesetInput(text)completes. Since React state updates are asynchronous, this condition evaluates against the previous input, not the newly pasted text. Auto-format will incorrectly trigger or not trigger based on stale state.Apply this diff to check the pasted text instead:
const pasteInput = async () => { try { const text = await navigator.clipboard.readText(); setInput(text); // Auto-format on paste if empty - if (!input) { + if (!input.trim()) { try { const parsed = JSON.parse(text); setOutput(JSON.stringify(parsed, null, indentation)); setError(""); } catch { // Ignore parse error on paste, let user fix } } } catch (err) { toast.error("Failed to paste from clipboard"); } };Note: This still references the old
inputvalue. For truly correct behavior, either remove the condition entirely (always attempt auto-format on paste) or restructure the logic to avoid the stale closure.Committable suggestion skipped: line range outside the PR's diff.
components/ui/collapsible.tsx-21-32 (1)
21-32: FixclassNameoverride inCollapsibleContent
classNameis composed viacn(...)but then{...props}(which still containsclassName) is spread afterwards, so the caller’s rawclassNameoverrides the composed one and the default animation/overflow styles are lost.Destructure
classNameand omit it frompropsso the merged value is preserved:-function CollapsibleContent({ - ...props -}: React.ComponentProps<typeof CollapsiblePrimitive.CollapsibleContent>) { +function CollapsibleContent({ + className, + ...props +}: React.ComponentProps<typeof CollapsiblePrimitive.CollapsibleContent>) { return ( <CollapsiblePrimitive.CollapsibleContent data-slot="collapsible-content" - className={cn( - "overflow-hidden data-[state=open]:animate-collapsible-down data-[state=closed]:animate-collapsible-up", - props.className, - )} + className={cn( + "overflow-hidden data-[state=open]:animate-collapsible-down data-[state=closed]:animate-collapsible-up", + className, + )} {...props} /> ); }app/routes/tools/url-encoder.tsx-38-41 (1)
38-41: Guard Clipboard API usage and handle copy failures
navigator.clipboard.writeText()is an async operation that returns a Promise, but the code calls it without awaiting and always shows a success toast regardless of the result. If the write fails (due to permissions or unavailability), the error is silently ignored while users see a success message.Other tools in this directory (random-number, password-generator, base64-converter, etc.) wrap clipboard operations in try/catch blocks with await. Apply the same pattern here:
- const copyToClipboard = (text: string) => { - navigator.clipboard.writeText(text); - toast.success("Copied to clipboard"); - }; + const copyToClipboard = async (text: string) => { + try { + await navigator.clipboard.writeText(text); + toast.success("Copied to clipboard"); + } catch { + toast.error("Failed to copy"); + } + };Update the onClick handlers to handle the async function properly if needed.
components.json-6-12 (1)
6-12: Settailwind.configto reference the existing configuration file.The repository contains
tailwind.config.ts, but components.json has an emptyconfigfield. This must point to the tailwind config file for the shadcn CLI to work correctly when adding new components. Update line 7 to"config": "tailwind.config.ts".app/routes/tools/qr-code-generator.tsx-111-119 (1)
111-119: Downloaded PNG will not match selected size.The
QRCodeSVGrenders atsize > 280 ? 280 : size, butdownloadQrCodeuses the statesizefor the canvas dimensions while serializing the capped-size SVG. When size is 512, the SVG is 280px but the canvas is 512px, causing the QR code to appear in the top-left corner with empty space, or stretched depending on how the image loads.Consider rendering a hidden SVG at full size for download, or re-render to a fresh SVG element:
<QRCodeSVG id="qr-code-svg" value={text} - size={size > 280 ? 280 : size} // Visual cap for preview, real size for download + size={size} fgColor={fgColor} bgColor={bgColor} level="H" includeMargin={true} + style={{ maxWidth: 280, maxHeight: 280, width: '100%', height: 'auto' }} />Alternatively, use a separate hidden
QRCodeSVGat fullsizefor download purposes..env.example-1-5 (1)
1-5: .env.example must documentCONVEX_DEPLOYMENTandPUBLIC_CONVEX_URLper project guidelinesThe file currently omits these required variables. Add them to
.env.examplealong with placeholder values to match the documented requirements..github/workflows/build.yml-18-24 (1)
18-24: Pin Bun version for reproducible builds.Using
bun-version: latestcreates non-deterministic builds where different CI runs may use different Bun versions, leading to inconsistent results and unexpected failures.Apply this diff to pin to a specific Bun version:
- uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: 1.1.38Check the latest stable Bun version and update accordingly. You can find it at https://bun.sh/docs/installation
app/routes/tools/calculator.tsx-36-99 (1)
36-99: Extract duplicate operation logic into a helper function.The switch statement for arithmetic operations appears twice: in
performOperation(lines 45-60) andhandleEquals(lines 77-92). This duplication violates the DRY principle and creates maintenance burden.Apply this diff to extract the shared logic:
+ const calculate = (operator: string, left: number, right: number): number => { + switch (operator) { + case "+": + return left + right; + case "-": + return left - right; + case "*": + return left * right; + case "/": + return left / right; + default: + return right; + } + }; + const performOperation = (nextOperation: string) => { const inputValue = parseFloat(display); if (previousValue === null) { setPreviousValue(inputValue); } else if (operation) { - const currentValue = previousValue || 0; - let newValue = currentValue; - - switch (operation) { - case "+": - newValue = currentValue + inputValue; - break; - case "-": - newValue = currentValue - inputValue; - break; - case "*": - newValue = currentValue * inputValue; - break; - case "/": - newValue = currentValue / inputValue; - break; - default: - break; - } + const newValue = calculate(operation, previousValue, inputValue); setDisplay(String(newValue)); setPreviousValue(newValue); } setWaitingForOperand(true); setOperation(nextOperation); }; const handleEquals = () => { const inputValue = parseFloat(display); if (previousValue !== null && operation) { - const currentValue = previousValue; - let newValue = currentValue; - - switch (operation) { - case "+": - newValue = currentValue + inputValue; - break; - case "-": - newValue = currentValue - inputValue; - break; - case "*": - newValue = currentValue * inputValue; - break; - case "/": - newValue = currentValue / inputValue; - break; - default: - break; - } + const newValue = calculate(operation, previousValue, inputValue); setDisplay(String(newValue)); setPreviousValue(null); setOperation(null); setWaitingForOperand(true); } };components/theme-selector.tsx-8-20 (1)
8-20: Incomplete Popover implementation: component renders as always-visible.The code imports
Popover,PopoverContent, andPopoverTriggerand manages anopenstate, but the component renders a bare<Command>without wrapping it in a Popover. This means the theme selector is always visible rather than appearing as a dropdown.This likely needs to be structured like:
export function ThemeSelector({ className }: { className?: string }) { const [open, setOpen] = React.useState(false); const theme = useSettingsState((state) => state.theme); const setTheme = useSettingsState((state) => state.setTheme); + + const currentTheme = themes[theme.theme]; return ( + <Popover open={open} onOpenChange={setOpen}> + <PopoverTrigger asChild> + <Button + variant="outline" + role="combobox" + aria-expanded={open} + className={cn("justify-between", className)} + > + {currentTheme?.name ?? "Select theme..."} + <ChevronsUpDown className="ml-2 h-4 w-4 shrink-0 opacity-50" /> + </Button> + </PopoverTrigger> + <PopoverContent className="w-[200px] p-0"> <Command> <CommandInput placeholder="Search theme..." className="h-9" /> {/* ... rest of Command ... */} </Command> + </PopoverContent> + </Popover> ); }Also applies to: 29-34
app/routes/tools/base64-converter.tsx-26-35 (1)
26-35: atob() decode needs to match the encoding approach.If you update the encode function to handle Unicode properly, the decode function must also use
TextDecoderto correctly reconstruct the original text.const handleDecode = () => { try { setError(""); - const decoded = atob(input); + const binString = atob(input); + const bytes = Uint8Array.from(binString, (char) => char.charCodeAt(0)); + const decoded = new TextDecoder().decode(bytes); setOutput(decoded); } catch (err) { setError("Failed to decode. Make sure the input is valid Base64."); setOutput(""); } };app/routes/tools/base64-converter.tsx-13-24 (1)
13-24: btoa() fails on Unicode characters outside Latin1.The
btoa()function only supports characters in the Latin1 range (U+0000 to U+00FF). It will throw an error on common Unicode characters like emoji or non-Latin scripts (e.g., Chinese, Arabic, emoji). This significantly limits the tool's utility.Use
TextEncoderfor proper Unicode support:const handleEncode = () => { try { setError(""); - const encoded = btoa(input); + const encoder = new TextEncoder(); + const data = encoder.encode(input); + const base64 = btoa(String.fromCharCode(...data)); setOutput(encoded); } catch (err) { setError( - "Failed to encode. Make sure the input contains valid characters.", + "Failed to encode. Please try again.", ); setOutput(""); } };Better yet, use a proper base64 library that handles Unicode correctly, or the Web Crypto API pattern:
const handleEncode = () => { try { setError(""); - const encoded = btoa(input); + const bytes = new TextEncoder().encode(input); + const binString = String.fromCodePoint(...bytes); + const encoded = btoa(binString); setOutput(encoded); } catch (err) {Committable suggestion skipped: line range outside the PR's diff.
components/settings.tsx-632-643 (1)
632-643: Add error handling and post-deletion navigation for account deletion.The
deleteUsercall doesn't handle failures, and the loading overlay is closed even if deletion fails. Additionally, the user should be redirected after successful account deletion.<AlertDialogAction - onClick={async () => { + onClick={async () => { setLoadingOverlayOpen(true); setLoadingOverlayMessage("Deleting account..."); - await authClient.deleteUser(); - setLoadingOverlayOpen(false); + try { + await authClient.deleteUser(); + // Redirect to home or sign-in page after deletion + window.location.href = "/"; + } catch (error) { + setLoadingOverlayOpen(false); + toast.error("Failed to delete account. Please try again."); + } }} className="bg-destructive text-destructive-foreground hover:bg-destructive/90" >Note: You'll need to import
toastfromsonnerif not already imported.Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (18)
app/routes/tools/roman-numeral.tsx-246-252 (1)
246-252: Copy button should respect conversion direction.Line 248 uses
roman || number, which always copies the Roman numeral when both values exist. The button should copy the conversion result based ondirection: the Roman numeral when converting to Roman, or the Arabic number when converting from Roman.Apply this diff:
<Button variant="default" - onClick={() => copyToClipboard(roman || number)} + onClick={() => copyToClipboard(direction === "to-roman" ? roman : number)} className="h-10 px-6" >app/routes/tools/roman-numeral.tsx-48-82 (1)
48-82: Fix empty string handling infromRoman.Line 50 returns
0for empty input, but this causes a UX issue: when the user clears the Roman numeral input, the number input displays "0" instead of being empty. InhandleRomanChange(line 108),!isNaN(0)is true, sosetNumber("0")executes instead ofsetNumber("").Apply this diff to return
NaNfor empty input:function fromRoman(roman: string): number { const upperRoman = roman.toUpperCase().trim(); - if (!upperRoman) return 0; + if (!upperRoman) return NaN; let result = 0;app/routes/tools/random-number.tsx-15-25 (1)
15-25: Allow equal min/max so single‑value ranges are validRight now
min >= maxrejects ranges like5–5, even though the rest of the logic can generate a single value correctly. If you want to support “fixed” ranges (common for random generators), consider relaxing the condition:- if (min >= max) { - alert("Minimum must be less than maximum"); + if (min > max) { + alert("Minimum must be less than or equal to maximum");This keeps invalid ranges out while allowing a single possible outcome.
.github/ISSUE_TEMPLATE/feature_request.yml-4-4 (1)
4-4: Remove the unsupportedtypefield.The
typefield is not part of GitHub's issue template specification. GitHub's supported top-level fields arename,description,body(required) andtitle,labels,assignees,projects(optional). If you need to categorize this template as a "Feature" request, use thelabelsfield instead.components/ui/kbd.tsx-18-26 (1)
18-26: Fix type mismatch in KbdGroup props.The component accepts
React.ComponentProps<"div">but renders a<kbd>element, causing a type mismatch. This prevents proper type checking for kbd-specific attributes.Apply this diff:
-function KbdGroup({ className, ...props }: React.ComponentProps<"div">) { +function KbdGroup({ className, ...props }: React.ComponentProps<"kbd">) { return ( <kbd data-slot="kbd-group" className={cn("inline-flex items-center gap-1", className)} {...props} /> ); }app/routes/g/play.tsx-225-247 (1)
225-247: Handle Web Share cancellation without spurious error toastIn the
Sharebutton handler, thenavigator.sharerejection handler compares the error object directly to a string:if (error == "AbortError: Share canceled") return; toast.error("Failed to share");The
navigator.share()method rejects with aDOMExceptionobject, not a string. This equality check will always be false, so when users cancel the share, they still see "Failed to share" instead of silently dismissing the cancellation.Check the error's
nameproperty instead:-.catch((error) => { - if (error == "AbortError: Share canceled") return; - toast.error("Failed to share"); -}); +.catch((error) => { + if ((error as any)?.name === "AbortError") return; + toast.error("Failed to share"); +});This prevents the error toast when the user intentionally cancels.
components/ui/popover.tsx-18-35 (1)
18-35: Use valid Tailwind arbitrary value syntax for transform-originLine 31 contains
origin-(--radix-popover-content-transform-origin), which uses invalid Tailwind syntax. Tailwind's arbitrary value syntax requires square brackets, not parentheses, so this class is ignored and the transform-origin variable never applies.Change to:
- "...z-50 w-72 origin-(--radix-popover-content-transform-origin) rounded-md..." + "...z-50 w-72 origin-[var(--radix-popover-content-transform-origin)] rounded-md..."app/routes/tools/qr-code-generator.tsx-39-39 (1)
39-39:btoa()fails on non-ASCII characters.If the SVG contains non-ASCII characters (e.g., from text content or generated attributes),
btoa()will throw. Consider using a UTF-8 safe encoding approach.- img.src = "data:image/svg+xml;base64," + btoa(svgData); + img.src = "data:image/svg+xml;base64," + btoa(unescape(encodeURIComponent(svgData)));components/ui/sidebar.tsx-200-203 (1)
200-203: SecondDrawerTitleshould beDrawerDescription.The mobile sidebar drawer header contains two
DrawerTitlecomponents. Based on the content, the second one ("Displays the mobile sidebar.") appears to be a description, not a title.<DrawerHeader className="sr-only"> <DrawerTitle>Sidebar</DrawerTitle> - <DrawerTitle>Displays the mobile sidebar.</DrawerTitle> + <DrawerDescription>Displays the mobile sidebar.</DrawerDescription> </DrawerHeader>components/theme-mode-selector.tsx-1-1 (1)
1-1: Remove Next.js-specific "use client" directive.The
"use client"directive is specific to Next.js App Router and has no effect in React Router applications. It appears to have been copied from a Next.js template.Apply this diff:
-"use client"; - import * as React from "react";app/routes/tools/hash-generator.tsx-50-53 (1)
50-53: Missing error handling in clipboard copy.The
navigator.clipboard.writeText()call is not awaited and has no error handling. If the clipboard operation fails (due to permissions or browser limitations), the success toast will still appear.-const copyToClipboard = (text: string) => { - navigator.clipboard.writeText(text); - toast.success("Copied to clipboard"); +const copyToClipboard = async (text: string) => { + try { + await navigator.clipboard.writeText(text); + toast.success("Copied to clipboard"); + } catch (error) { + toast.error("Failed to copy to clipboard"); + } };app/routes/g/columns.tsx-1-1 (1)
1-1: Remove unnecessary "use client" directive.The
"use client"directive is specific to Next.js App Router and has no effect in React Router. This can be safely removed.Apply this diff:
-"use client"; - import type { ColumnDef } from "@tanstack/react-table";components/theme-selector.tsx-1-1 (1)
1-1: Remove unnecessary "use client" directive.The
"use client"directive is specific to Next.js App Router and is not used in React Router. This directive has no effect here and can be safely removed.Apply this diff:
-"use client"; - import * as React from "react";app/routes/admin/page.tsx-130-146 (1)
130-146: KeepselectedUserin sync after unbanning
handleUnbanUserrefreshes the users list vialoadUsers(), but it doesn’t updateselectedUser. If the admin unbans a user while the details dialog is open, the table will reflect the new status but the dialog will continue to show “Banned”.You can keep the dialog in sync by updating
selectedUserwhen the unban succeeds, for example:} else { toast.success("User unbanned successfully"); - loadUsers(); + loadUsers(); + setSelectedUser((current) => + current && current.id === userId + ? { ...current, banned: false, banReason: null, banExpires: null } + : current, + ); }Adjust fields as appropriate for your backend’s unban semantics.
app/routes/home.tsx-48-57 (1)
48-57: Fix fallbackhreffor unknown game IDsWhen
getGmaeById(id)returnsundefined,hrefbecomes/g/undefinedwhile the label shows “Unknown Game”. That yields a broken link.You can keep the link stable by falling back to the original
idwhen the lookup fails, e.g.:const getGameDetails = (id: string) => { const game = getGmaeById(id); return { - id, - href: `/g/${game?.id}`, - label: game?.name || "Unknown Game", + id, + href: `/g/${game?.id ?? id}`, + label: game?.name || "Unknown Game", }; };Alternatively, you could skip linking entirely when the game metadata is missing.
components/ui/command.tsx-44-59 (1)
44-59: MoveDialogHeaderinsideDialogContentfor proper accessibility.Placing
DialogHeaderoutside ofDialogContentmay cause accessibility issues since screen readers expect the dialog title to be within the dialog's content region. Thesr-onlyclass hides it visually but the DOM structure matters for assistive technologies.return ( <Dialog {...props}> - <DialogHeader className="sr-only"> - <DialogTitle>{title}</DialogTitle> - <DialogDescription>{description}</DialogDescription> - </DialogHeader> <DialogContent className={cn("overflow-hidden p-0", className)} showCloseButton={showCloseButton} > + <DialogHeader className="sr-only"> + <DialogTitle>{title}</DialogTitle> + <DialogDescription>{description}</DialogDescription> + </DialogHeader> <Command className="[&_[cmdk-group-heading]]:text-muted-foreground ..."> {children} </Command> </DialogContent> </Dialog> );app/root.tsx-159-170 (1)
159-170: WraplocalStorageaccess in try-catch to handle storage errors.
JSON.parsecan throw if the stored value is malformed. Additionally,localStorageaccess can throw in private browsing modes or when storage quota is exceeded.useEffect(() => { - if ( - JSON.parse(localStorage.getItem("preferences") || "{}") - .experimentalFeatures - ) { - setExperimentalFeatures(true); - localStorage.removeItem("preferences"); - setTimeout(() => { - toast.success("Migrated to new EduTools"); - }, 500); + try { + const prefs = JSON.parse(localStorage.getItem("preferences") || "{}"); + if (prefs.experimentalFeatures) { + setExperimentalFeatures(true); + localStorage.removeItem("preferences"); + setTimeout(() => { + toast.success("Migrated to new EduTools"); + }, 500); + } + } catch { + // Ignore malformed or inaccessible storage } }, []);app/routes/backups/page.tsx-295-312 (1)
295-312: Avoid async promise executor.The async promise executor is an antipattern because exceptions thrown inside it won't reject the promise—they'll cause an unhandled rejection. Refactor to use async/await directly with
toast.promise.<AlertDialogAction onClick={() => { toast.promise( - new Promise(async (resolve, reject) => { - const result = await deleteCloudBackup({ - id: backup._id, - }); - if (result?.success) { - resolve(true); - } else { - reject(result?.error); - } - }), + deleteCloudBackup({ id: backup._id }).then((result) => { + if (!result?.success) { + throw new Error(result?.error || "Failed to delete backup"); + } + return result; + }), { loading: "Deleting backup...", success: "Backup deleted", error: "Failed to delete backup", }, ); }} >
| // Simple markdown parser | ||
| function parseMarkdown(markdown: string): string { | ||
| let html = markdown; | ||
|
|
||
| // Headers | ||
| html = html.replace(/^### (.*$)/gim, "<h3>$1</h3>"); | ||
| html = html.replace(/^## (.*$)/gim, "<h2>$1</h2>"); | ||
| html = html.replace(/^# (.*$)/gim, "<h1>$1</h1>"); | ||
|
|
||
| // Bold | ||
| html = html.replace(/\*\*(.*?)\*\*/gim, "<strong>$1</strong>"); | ||
| html = html.replace(/__(.*?)__/gim, "<strong>$1</strong>"); | ||
|
|
||
| // Italic | ||
| html = html.replace(/\*(.*?)\*/gim, "<em>$1</em>"); | ||
| html = html.replace(/_(.*?)_/gim, "<em>$1</em>"); | ||
|
|
||
| // Code blocks | ||
| html = html.replace(/```([\s\S]*?)```/gim, "<pre><code>$1</code></pre>"); | ||
| html = html.replace(/`([^`]+)`/gim, "<code>$1</code>"); | ||
|
|
||
| // Links | ||
| html = html.replace( | ||
| /\[([^\]]+)\]\(([^)]+)\)/gim, | ||
| '<a href="$2" target="_blank" rel="noopener noreferrer">$1</a>', | ||
| ); | ||
|
|
||
| // Images | ||
| html = html.replace( | ||
| /!\[([^\]]*)\]\(([^)]+)\)/gim, | ||
| '<img src="$2" alt="$1" />', | ||
| ); | ||
|
|
||
| // Lists | ||
| html = html.replace(/^\* (.*$)/gim, "<li>$1</li>"); | ||
| html = html.replace(/^- (.*$)/gim, "<li>$1</li>"); | ||
| html = html.replace(/^\+ (.*$)/gim, "<li>$1</li>"); | ||
| html = html.replace(/(<li>.*<\/li>)/s, "<ul>$1</ul>"); | ||
|
|
||
| // Ordered lists | ||
| html = html.replace(/^\d+\. (.*$)/gim, "<li>$1</li>"); | ||
| html = html.replace(/(<li>.*<\/li>)/s, "<ol>$1</ol>"); | ||
|
|
||
| // Line breaks | ||
| html = html.replace(/\n\n/gim, "</p><p>"); | ||
| html = html.replace(/\n/gim, "<br />"); | ||
|
|
||
| // Wrap in paragraph if not already wrapped | ||
| if (!html.startsWith("<")) { | ||
| html = "<p>" + html + "</p>"; | ||
| } | ||
|
|
||
| return html; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and read the complete markdown-preview.tsx file
wc -l app/routes/tools/markdown-preview.tsxRepository: EducationalTools/src
Length of output: 105
🏁 Script executed:
# Read the full file with line numbers to see context around lines 7-60 and 181-185
cat -n app/routes/tools/markdown-preview.tsxRepository: EducationalTools/src
Length of output: 7272
🏁 Script executed:
# Search for dangerouslySetInnerHTML usage in the codebase
rg "dangerouslySetInnerHTML" app/routes/tools/markdown-preview.tsx -B 2 -A 2Repository: EducationalTools/src
Length of output: 319
🏁 Script executed:
# Check if DOMPurify or other sanitization libraries are imported in this file
rg "DOMPurify|sanitize|xss" app/routes/tools/markdown-preview.tsxRepository: EducationalTools/src
Length of output: 46
🏁 Script executed:
# Check package.json for available sanitization libraries
cat package.json | grep -A 5 -B 5 "dompurify\|sanitize"Repository: EducationalTools/src
Length of output: 46
Sanitize rendered HTML before using dangerouslySetInnerHTML
parseMarkdown returns unsanitized HTML derived directly from user textarea input. At line 184, this output is injected via dangerouslySetInnerHTML. Because the parser doesn't escape HTML characters, a user can enter <script>alert('xss')</script> or tags with inline event handlers like <img src=x onerror="alert('xss')">, and these will execute in the preview—a direct XSS vector.
Consider either:
- Using a markdown library with built-in sanitization, or
- Running the generated HTML through a sanitizer like DOMPurify before injection.
For example:
-import { toast } from "sonner";
+import { toast } from "sonner";
+import DOMPurify from "dompurify";
function parseMarkdown(markdown: string): string {
let html = markdown;
// ...existing regex transforms...
- return html;
+ return DOMPurify.sanitize(html);
}You may also want to replace the regex-based parser over time with a maintained markdown library to avoid edge-case rendering issues.
Committable suggestion skipped: line range outside the PR's diff.
| const generatePassword = () => { | ||
| const uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; | ||
| const lowercase = "abcdefghijklmnopqrstuvwxyz"; | ||
| const numbers = "0123456789"; | ||
| const symbols = "!@#$%^&*()_+-=[]{}|;:,.<>?"; | ||
|
|
||
| let characters = ""; | ||
| if (includeUppercase) characters += uppercase; | ||
| if (includeLowercase) characters += lowercase; | ||
| if (includeNumbers) characters += numbers; | ||
| if (includeSymbols) characters += symbols; | ||
|
|
||
| if (characters.length === 0) { | ||
| setPassword(""); | ||
| return; | ||
| } | ||
|
|
||
| let result = ""; | ||
| for (let i = 0; i < length; i++) { | ||
| result += characters.charAt( | ||
| Math.floor(Math.random() * characters.length), | ||
| ); | ||
| } | ||
|
|
||
| setPassword(result); | ||
| setCopied(false); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "password-generator.tsx" --type fRepository: EducationalTools/src
Length of output: 103
🏁 Script executed:
cat -n "app/routes/tools/password-generator.tsx"Repository: EducationalTools/src
Length of output: 9030
Replace Math.random with cryptographically secure randomness
The generatePassword function uses Math.random, which is not cryptographically secure. Since the UI explicitly markets "Generate secure random passwords instantly" (line 64), this is a security issue.
Use the Web Crypto API instead:
const generatePassword = () => {
const uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
const lowercase = "abcdefghijklmnopqrstuvwxyz";
const numbers = "0123456789";
const symbols = "!@#$%^&*()_+-=[]{}|;:,.<>?";
let characters = "";
if (includeUppercase) characters += uppercase;
if (includeLowercase) characters += lowercase;
if (includeNumbers) characters += numbers;
if (includeSymbols) characters += symbols;
if (characters.length === 0) {
setPassword("");
return;
}
+ const randomBytes = new Uint32Array(length);
+ window.crypto.getRandomValues(randomBytes);
+
let result = "";
for (let i = 0; i < length; i++) {
- result += characters.charAt(
- Math.floor(Math.random() * characters.length),
- );
+ result += characters.charAt(randomBytes[i] % characters.length);
}
setPassword(result);
setCopied(false);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const generatePassword = () => { | |
| const uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; | |
| const lowercase = "abcdefghijklmnopqrstuvwxyz"; | |
| const numbers = "0123456789"; | |
| const symbols = "!@#$%^&*()_+-=[]{}|;:,.<>?"; | |
| let characters = ""; | |
| if (includeUppercase) characters += uppercase; | |
| if (includeLowercase) characters += lowercase; | |
| if (includeNumbers) characters += numbers; | |
| if (includeSymbols) characters += symbols; | |
| if (characters.length === 0) { | |
| setPassword(""); | |
| return; | |
| } | |
| let result = ""; | |
| for (let i = 0; i < length; i++) { | |
| result += characters.charAt( | |
| Math.floor(Math.random() * characters.length), | |
| ); | |
| } | |
| setPassword(result); | |
| setCopied(false); | |
| }; | |
| const generatePassword = () => { | |
| const uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; | |
| const lowercase = "abcdefghijklmnopqrstuvwxyz"; | |
| const numbers = "0123456789"; | |
| const symbols = "!@#$%^&*()_+-=[]{}|;:,.<>?"; | |
| let characters = ""; | |
| if (includeUppercase) characters += uppercase; | |
| if (includeLowercase) characters += lowercase; | |
| if (includeNumbers) characters += numbers; | |
| if (includeSymbols) characters += symbols; | |
| if (characters.length === 0) { | |
| setPassword(""); | |
| return; | |
| } | |
| const randomBytes = new Uint32Array(length); | |
| window.crypto.getRandomValues(randomBytes); | |
| let result = ""; | |
| for (let i = 0; i < length; i++) { | |
| result += characters.charAt(randomBytes[i] % characters.length); | |
| } | |
| setPassword(result); | |
| setCopied(false); | |
| }; |
| const highlightMatches = (text: string, regex: RegExp): string => { | ||
| if (!pattern || !text) return text; | ||
|
|
||
| try { | ||
| const parts: string[] = []; | ||
| let lastIndex = 0; | ||
| let match; | ||
|
|
||
| while ((match = regex.exec(text)) !== null) { | ||
| parts.push(text.substring(lastIndex, match.index)); | ||
| parts.push( | ||
| `<mark class="bg-yellow-300 dark:bg-yellow-600 rounded-sm px-0.5">${match[0]}</mark>`, | ||
| ); |
There was a problem hiding this comment.
XSS vulnerability: user input is rendered as HTML without sanitization.
The testString content is passed to dangerouslySetInnerHTML without escaping HTML entities. If a user enters <img src=x onerror=alert(1)> in the test string and it doesn't match the pattern, it will execute as HTML.
Add an HTML escape helper and use it when building the highlighted string:
+ const escapeHtml = (text: string): string => {
+ return text
+ .replace(/&/g, "&")
+ .replace(/</g, "<")
+ .replace(/>/g, ">")
+ .replace(/"/g, """)
+ .replace(/'/g, "'");
+ };
+
const highlightMatches = (text: string, regex: RegExp): string => {
if (!pattern || !text) return text;
try {
const parts: string[] = [];
let lastIndex = 0;
let match;
while ((match = regex.exec(text)) !== null) {
- parts.push(text.substring(lastIndex, match.index));
+ parts.push(escapeHtml(text.substring(lastIndex, match.index)));
parts.push(
- `<mark class="bg-yellow-300 dark:bg-yellow-600 rounded-sm px-0.5">${match[0]}</mark>`,
+ `<mark class="bg-yellow-300 dark:bg-yellow-600 rounded-sm px-0.5">${escapeHtml(match[0])}</mark>`,
);Also escape the final remaining portion after the loop:
- parts.push(text.substring(lastIndex));
+ parts.push(escapeHtml(text.substring(lastIndex)));Also applies to: 249-252
🤖 Prompt for AI Agents
In app/routes/tools/regex-tester.tsx around lines 43 to 55 (and similarly at
lines 249-252), the function builds HTML by concatenating user-provided text
directly into strings used with dangerouslySetInnerHTML, creating an XSS risk;
add an HTML-escape helper that replaces &, <, >, ", and ' with entities and use
it on every piece of user text (both non-matching segments and the match
content) before inserting into the highlighted HTML markup, and also escape the
final remaining substring after the loop so no raw user input is ever injected
into the generated HTML.
| OctagonXIcon, | ||
| TriangleAlertIcon, | ||
| } from "lucide-react"; | ||
| import { useTheme } from "next-themes"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for ThemeProvider usage and other theme-related imports
echo "=== Checking for ThemeProvider setup ==="
rg -n "ThemeProvider" --type tsx --type ts -C 2
echo -e "\n=== Checking for next-themes imports ==="
rg -n "from ['\"]next-themes['\"]" --type tsx --type ts
echo -e "\n=== Checking package.json for theme packages ==="
if [ -f package.json ]; then
jq '.dependencies, .devDependencies' package.json 2>/dev/null | grep -i theme || echo "No theme packages found in package.json"
fi
echo -e "\n=== Checking for alternative theme solutions ==="
rg -n "useContext.*theme|createContext.*theme" --type tsx --type ts -iRepository: EducationalTools/src
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Check for ThemeProvider without type restrictions
echo "=== Checking for ThemeProvider setup ==="
rg -n "ThemeProvider" -C 2
echo -e "\n=== Checking for next-themes imports (all files) ==="
rg -n "next-themes"
echo -e "\n=== Looking for root layout/app files ==="
fd -e "tsx" -e "ts" -e "jsx" -e "js" | grep -E "(layout|app|root|main|index)" | head -20
echo -e "\n=== Checking sonner.tsx file ==="
if [ -f "components/ui/sonner.tsx" ]; then
echo "File exists, showing content:"
cat -n "components/ui/sonner.tsx"
fiRepository: EducationalTools/src
Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Check app/root.tsx for ThemeProvider setup
echo "=== Checking app/root.tsx ==="
cat -n "app/root.tsx"Repository: EducationalTools/src
Length of output: 12649
Remove next-themes from the Toaster component or add a ThemeProvider wrapper.
The Toaster component imports useTheme from next-themes (line 8), but the app has no ThemeProvider set up. The app uses a custom theme system via useSettingsState (Zustand store) that directly manipulates the document element. Without a ThemeProvider context, useTheme() will return undefined or cause an error. Either remove the next-themes dependency from this component and use the app's existing theme state, or add a ThemeProvider wrapper around the root layout in app/root.tsx.
🤖 Prompt for AI Agents
In components/ui/sonner.tsx around line 8, the Toaster imports useTheme from
next-themes but the app has no ThemeProvider, causing undefined/erroneous
behavior; either remove the next-themes import and consume the app's existing
theme via the useSettingsState Zustand store (read current theme and map to
sonner props) or, alternatively, add a ThemeProvider in app/root.tsx so useTheme
has context — choose one approach, remove unused next-themes import and update
the component to rely on the Zustand theme (or add ThemeProvider at root) and
update any references to useTheme accordingly.
| import { Loader2Icon } from "lucide-react"; | ||
|
|
||
| import { cn } from "@/lib/utils"; | ||
|
|
||
| function Spinner({ className, ...props }: React.ComponentProps<"svg">) { | ||
| return ( | ||
| <Loader2Icon | ||
| role="status" | ||
| aria-label="Loading" | ||
| className={cn("size-4 animate-spin", className)} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n components/ui/spinner.tsxRepository: EducationalTools/src
Length of output: 510
🏁 Script executed:
find . -name "tsconfig.json" -type f | head -5Repository: EducationalTools/src
Length of output: 102
🏁 Script executed:
cat tsconfig.jsonRepository: EducationalTools/src
Length of output: 720
🏁 Script executed:
# Check for TypeScript error by looking at package.json for type checking scripts
cat package.json | grep -A 5 '"scripts"'Repository: EducationalTools/src
Length of output: 255
🏁 Script executed:
# Look for lucide-react type information or usage in the codebase
rg "Loader2Icon" --type tsx --type ts -A 3 -B 1Repository: EducationalTools/src
Length of output: 91
🏁 Script executed:
rg "Loader2Icon" -A 3 -B 1 --type-add 'tsx:*.tsx' --type tsxRepository: EducationalTools/src
Length of output: 1004
🏁 Script executed:
rg "ComponentProps" --type-add 'tsx:*.tsx' --type tsx -B 1 -A 1Repository: EducationalTools/src
Length of output: 26089
🏁 Script executed:
head -20 components/ui/button.tsxRepository: EducationalTools/src
Length of output: 1366
🏁 Script executed:
rg "from \"lucide-react\"" -B 5 -A 10 --type-add 'tsx:*.tsx' --type tsx | head -60Repository: EducationalTools/src
Length of output: 3164
Add missing React import and type Spinner props from Loader2Icon.
The current code uses React.ComponentProps<"svg"> without importing React, which will fail TypeScript checks. Additionally, typing props as React.ComponentProps<"svg"> omits lucide-react-specific props like size that Loader2Icon supports, preventing consumers from using them in a type-safe way.
Import ComponentProps from React and type Spinner directly from Loader2Icon to fix both issues:
+import type { ComponentProps } from "react";
import { Loader2Icon } from "lucide-react";
import { cn } from "@/lib/utils";
-function Spinner({ className, ...props }: React.ComponentProps<"svg">) {
+type SpinnerProps = ComponentProps<typeof Loader2Icon>;
+
+function Spinner({ className, ...props }: SpinnerProps) {
return (
<Loader2Icon
role="status"
aria-label="Loading"
className={cn("size-4 animate-spin", className)}
{...props}
/>
);
}🤖 Prompt for AI Agents
components/ui/spinner.tsx lines 1-12: the file uses React types but doesn't
import them and the props are typed as React.ComponentProps<"svg"> which omits
lucide-react-specific props; import the ComponentProps type from React (import
type { ComponentProps } from "react") and use ComponentProps<typeof Loader2Icon>
as the Spinner props type (e.g., type SpinnerProps = ComponentProps<typeof
Loader2Icon> and function Spinner(props: SpinnerProps) { ... }) so the component
accepts all Loader2Icon-specific props and TypeScript resolves correctly.
| export default { | ||
| providers: [ | ||
| { | ||
| domain: process.env.CONVEX_SITE_URL, | ||
| applicationID: "convex", | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Step 1: Locate and examine .env.example
git ls-files | grep -E "\.env" | head -20Repository: EducationalTools/src
Length of output: 76
🏁 Script executed:
# Step 2: Search for CONVEX_SITE_URL usage in the codebase
rg "CONVEX_SITE_URL" --type-list | head -5
rg "CONVEX_SITE_URL" -A 2 -B 2Repository: EducationalTools/src
Length of output: 1911
🏁 Script executed:
# Step 3: Search for VITE_CONVEX_SITE_URL and related patterns
rg "VITE_CONVEX" -A 1 -B 1Repository: EducationalTools/src
Length of output: 802
🏁 Script executed:
# Step 4: Examine the full convex/auth.config.ts file
cat -n convex/auth.config.tsRepository: EducationalTools/src
Length of output: 246
🏁 Script executed:
# Step 5: Check the contents of .env.example
cat -n .env.exampleRepository: EducationalTools/src
Length of output: 329
🏁 Script executed:
# Step 6: Search for all variations of CONVEX environment variables in config files
rg "CONVEX" convex/auth.config.ts convex/auth.ts -B 2 -A 2Repository: EducationalTools/src
Length of output: 636
🏁 Script executed:
# Step 7: Check if there are any type definitions or config setup for these env vars
fd "env" --type f | grep -E "(config|type|d\.ts)" | head -10Repository: EducationalTools/src
Length of output: 46
Fix incorrect environment variable name and add missing documentation.
The code uses process.env.CONVEX_SITE_URL, but the correct variable is process.env.PUBLIC_CONVEX_SITE_URL (as used in convex/auth.ts). Additionally, this required variable is not documented in .env.example, which only contains client-side variables.
Two changes needed:
- Update
convex/auth.config.tsline 4 to usePUBLIC_CONVEX_SITE_URLinstead ofCONVEX_SITE_URL - Add
PUBLIC_CONVEX_SITE_URLto.env.examplealongside other server-side configuration
🤖 Prompt for AI Agents
In convex/auth.config.ts around lines 1 to 8, the provider domain is reading
process.env.CONVEX_SITE_URL but should reference
process.env.PUBLIC_CONVEX_SITE_URL; change that occurrence to
PUBLIC_CONVEX_SITE_URL. Also update the repository .env.example to include a new
entry PUBLIC_CONVEX_SITE_URL (with a brief placeholder/comment matching the
style of other entries) so the required server-side env var is documented for
developers.
| disabled: optionsOnly, | ||
| }, | ||
| baseURL: siteUrl, | ||
| trustedOrigins: ["*"], // security? fuck that |
There was a problem hiding this comment.
Critical security vulnerability: wildcard trusted origins.
Setting trustedOrigins: ["*"] allows any origin to make authenticated requests to your auth endpoints, exposing the application to Cross-Site Request Forgery (CSRF) attacks. Malicious sites can trick authenticated users into performing unintended actions.
Restrict to known, trusted origins:
- trustedOrigins: ["*"], // security? fuck that
+ trustedOrigins: [
+ process.env.PUBLIC_APP_URL!,
+ // Add other trusted origins explicitly
+ ],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trustedOrigins: ["*"], // security? fuck that | |
| trustedOrigins: [ | |
| process.env.PUBLIC_APP_URL!, | |
| // Add other trusted origins explicitly | |
| ], |
🤖 Prompt for AI Agents
In convex/auth.ts around line 33, trustedOrigins is set to ["*"], which allows
any origin and creates a CSRF/security vulnerability; replace the wildcard with
a whitelist of specific, trusted origins (e.g., your production and staging app
domains and localhost for dev), ensure values include proper schemes and ports,
and load them from environment variables or a secure config so you don’t commit
secrets; validate and document the accepted origins and fail closed if the
env/config is missing.
| // Disable state check, see line 33 | ||
| { | ||
| id: "disable-state-check", | ||
| hooks: { | ||
| before: [ | ||
| { | ||
| matcher() { | ||
| return true; | ||
| }, | ||
| handler: createAuthMiddleware(async (ctx) => { | ||
| return { | ||
| context: { | ||
| context: { | ||
| oauthConfig: { | ||
| skipStateCookieCheck: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
| }), | ||
| }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Critical: Disabling OAuth state check removes CSRF protection.
The OAuth state parameter prevents CSRF attacks during the OAuth flow. Disabling skipStateCookieCheck allows attackers to initiate OAuth flows and potentially link their accounts to victim sessions.
If there's a legitimate cross-domain requirement causing state cookie issues, consider using a different mitigation (e.g., PKCE, custom state validation) rather than disabling the check entirely. Please document why this is needed if it must remain.
🤖 Prompt for AI Agents
In convex/auth.ts around lines 40 to 63, the config block is disabling the OAuth
state check (skipStateCookieCheck: true) which removes CSRF protection; instead
of turning off state validation, restore the state cookie check and implement an
alternative safe approach: re-enable skipStateCookieCheck = false (or remove the
override), and if cross-domain constraints exist, implement PKCE or a custom
validated state flow that stores and verifies a server-side state token (or
document and add a secure exception route with explicit validation) and add a
comment explaining why any deviation is required and how it preserves CSRF
protection.
| onClick={() => { | ||
| setIndentation(2); | ||
| if (output && !error) formatJson(); | ||
| }} |
There was a problem hiding this comment.
Bug: JSON formatter indentation uses stale state value
The indentation toggle buttons call setIndentation() followed immediately by formatJson(). Since React state updates are asynchronous, formatJson() executes with the previous indentation value rather than the newly set one. This means clicking "4 Spaces" when currently at "2 Spaces" won't actually change the output formatting until the next format action.
Additional Locations (1)
| toast.error("Failed to unban user"); | ||
| console.error(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Bug: Unban user leaves dialog showing stale data
The handleUnbanUser function updates the users list via loadUsers() but doesn't update selectedUser state. Unlike handleBanUser which calls setSelectedUser({ ...selectedUser, banned: true, banReason }), the unban handler leaves selectedUser with stale banned: true status. This causes the user detail dialog to incorrectly show the user as still banned after unbanning, with the wrong action buttons displayed.
| for (let i = 0; i < length; i++) { | ||
| result += characters.charAt( | ||
| Math.floor(Math.random() * characters.length), | ||
| ); |
There was a problem hiding this comment.
Bug: Password generator uses insecure random number source
The password generator advertises "secure random passwords" but uses Math.random() which is not cryptographically secure and can be predicted. For password generation, crypto.getRandomValues() is required to provide adequate security. Using Math.random() makes generated passwords vulnerable to prediction attacks.
| .catch((error) => { | ||
| if (error == "AbortError: Share canceled") return; | ||
| toast.error("Failed to share"); | ||
| }); |
There was a problem hiding this comment.
Bug: Share cancel detection incorrectly compares object to string
The error handling for navigator.share() uses error == "AbortError: Share canceled" to detect user cancellation. However, when the share dialog is cancelled, the API throws a DOMException object, not a string. This comparison will always fail, causing users who cancel the share dialog to incorrectly see a "Failed to share" error toast. The correct check is error.name === "AbortError".
|
|
||
| // Ordered lists | ||
| html = html.replace(/^\d+\. (.*$)/gim, "<li>$1</li>"); | ||
| html = html.replace(/(<li>.*<\/li>)/s, "<ol>$1</ol>"); |
There was a problem hiding this comment.
Bug: Markdown list parser produces invalid nested HTML
The markdown parser's list handling wraps all <li> elements in <ul> first (line 44), then runs the same greedy pattern (<li>.*<\/li>)/s again to wrap in <ol> (line 48). The regex with the /s flag matches <li> elements that are already inside <ul> tags, producing invalid nested HTML like <ul><ol><li>...</li></ol></ul> even for simple unordered lists without any ordered list items.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/routes/auth/ott.tsx (1)
18-23: Consider validating token before authentication attempt.If the token is null or empty, the authentication API will fail. Validating early and showing an error message immediately would provide better UX than making an API call that will fail anyway.
Apply this diff to validate the token upfront:
async function handleAuth() { + if (!token || token.trim() === "") { + setMessage("Invalid or missing token"); + setLoading(false); + return; + } setMessage("Logging in"); try { const result = await authClient.signIn.ott({ - token: token || "", + token, });app/root.tsx (4)
62-114: Layout initialization: Convex client placement is good; harden theme handling against bad dataCreating the
ConvexReactClientat module scope (Lines 62–63) is the right fix to avoid re-instantiation on every render.One robustness gap is in the theme effect (Lines 92–114):
themes[theme.theme][theme.mode]will throw iftheme.themeis an unknown key (e.g., corrupted localStorage). You can safely guard against this while preserving current behavior.useEffect(() => { const root = window.document.documentElement; Object.keys(themes).forEach((key) => { root.classList.remove("theme-" + key); }); root.classList.remove("dark"); - // if theme doesn't support selected mode - if (!themes[theme.theme][theme.mode]) { + const currentTheme = themes[theme.theme]; + // if theme key is unknown or doesn't support selected mode + if (!currentTheme || !currentTheme[theme.mode]) { // use supported mode if (!(theme.mode == "dark")) { root.classList.add("dark"); } } else { if (theme.mode == "dark") { root.classList.add("dark"); } } root.classList.add("theme-" + theme.theme); }, [theme]);
159-170: Defensively handle malformedpreferencesJSON in migration effectIf
localStorage["preferences"]is ever non‑JSON (user edits, previous buggy writes),JSON.parsewill throw and break the layout mount. A small try/catch makes this migration safer.- useEffect(() => { - if ( - JSON.parse(localStorage.getItem("preferences") || "{}") - .experimentalFeatures - ) { - setExperimentalFeatures(true); - localStorage.removeItem("preferences"); - setTimeout(() => { - toast.success("Migrated to new EduTools"); - }, 500); - } - }, []); + useEffect(() => { + try { + const prefs = JSON.parse( + localStorage.getItem("preferences") || "{}", + ) as { experimentalFeatures?: boolean }; + + if (prefs.experimentalFeatures) { + setExperimentalFeatures(true); + localStorage.removeItem("preferences"); + setTimeout(() => { + toast.success("Migrated to new EduTools"); + }, 500); + } + } catch { + // Malformed legacy value; clear it so it can't break future loads + localStorage.removeItem("preferences"); + } + }, []);
201-221: Ensure loading overlay closes on social sign‑in failureThe sign‑in handler opens the loading overlay and sets the message (Lines 214–215) but delegates directly to
authClient.signIn.social(Line 217). If that call rejects (network/auth error) without navigating away, the overlay may stay stuck open.You can attach a
.catch/.finallyto close the overlay and surface a toast in the failure path.- setLoadingOverlayOpen(true); - setLoadingOverlayMessage("Signing in..."); - - return authClient.signIn.social({ - ...params, - callbackURL: redirectUrl.toString(), - }); + setLoadingOverlayOpen(true); + setLoadingOverlayMessage("Signing in..."); + + return authClient + .signIn.social({ + ...params, + callbackURL: redirectUrl.toString(), + }) + .catch((error) => { + setLoadingOverlayOpen(false); + toast.error("Sign-in failed"); + throw error; + });Please double‑check the return type and error behavior of
authClient.signIn.socialin your auth client’s docs before applying this.
239-244:rounded-md!class looks non‑standard; likely intendedrounded-md
SidebarInsetcurrently has arounded-md!class (Line 240), which won’t match typical Tailwind utilities and is unlikely to have any effect. Dropping the trailing!keeps the intended rounded corners without relying on a non‑standard class name.- <SidebarInset className="w-full rounded-md! overflow-hidden"> + <SidebarInset className="w-full rounded-md overflow-hidden">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/root.tsx(1 hunks)app/routes/auth/ott.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: Do not correct spelling errors or grammar mistakes.
Files:
app/routes/auth/ott.tsxapp/root.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: EducationalTools/src PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-23T23:52:45.385Z
Learning: Applies to static/_app/tools/** : Place legacy educational games/tools under static/_app/tools/
🧬 Code graph analysis (1)
app/root.tsx (4)
lib/state.ts (3)
useSettingsState(93-146)useExperimentalFeatures(11-23)useUiState(40-55)lib/constants.ts (1)
NICE_EASE(5-5)lib/utils.ts (1)
cn(4-6)components/loading-overlay.tsx (1)
LoadingOverlay(5-19)
🪛 Biome (2.1.2)
app/root.tsx
[error] 51-51: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
app/routes/auth/ott.tsx (1)
30-40: Redirect handling is secure; no additional validation needed for React Router's navigate function.The code correctly addresses the previous URL parsing issue. The try-catch pattern safely handles absolute URLs by extracting the pathname, and React Router's
navigate()function inherently prevents external redirects. Protocol-relative URLs (//evil.com) passed tonavigate()are treated as relative paths within the single-page app and won't cause external navigation—they'll either fail to match a route or be handled as app-relative paths.app/root.tsx (3)
55-60: Staticlinksmetadata looks goodThe external font stylesheet link setup is straightforward and correct for React Router route metadata; no changes needed.
265-271: App wrapper delegating to<Outlet />is correctThe default
Appexport simply rendering<Outlet />is idiomatic for React Router route modules; nothing to change here.
273-329: ErrorBoundary behavior and UX look solidThe ErrorBoundary correctly distinguishes route errors vs generic errors, shows more detail only in development, and wraps the stack trace in a dialog with an explicit user action and clipboard copy feedback. The error handling and toasts are appropriate.
| export function meta({}: Route.MetaArgs) { | ||
| return [{ title: "EduTools" }]; | ||
| } |
There was a problem hiding this comment.
Fix Biome lint error in meta parameter
Biome flags the empty object pattern in Line 51; keep the type but avoid the empty destructuring.
-export function meta({}: Route.MetaArgs) {
+export function meta(_: Route.MetaArgs) {
return [{ title: "EduTools" }];
}Based on static analysis hints, this should clear noEmptyPattern.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function meta({}: Route.MetaArgs) { | |
| return [{ title: "EduTools" }]; | |
| } | |
| export function meta(_: Route.MetaArgs) { | |
| return [{ title: "EduTools" }]; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 51-51: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🤖 Prompt for AI Agents
In app/root.tsx around lines 51 to 53, the function signature uses an empty
destructuring pattern ({}: Route.MetaArgs) which triggers Biome's noEmptyPattern
lint; change the parameter to a named parameter that preserves the
Route.MetaArgs type (for example use args: Route.MetaArgs or _args:
Route.MetaArgs) and update any internal references if needed (or leave unused)
so the empty pattern is removed while keeping the type annotation.
| useEffect(() => { | ||
| if (!auth.isLoading) handleAuth(); | ||
| }, [auth.isLoading]); |
There was a problem hiding this comment.
Fix useEffect issues: missing dependency, no guard, and missing cleanup.
This effect has several problems:
- Missing dependency: ESLint exhaustive-deps would flag that
handleAuthis called but not in the dependency array, which can lead to stale closures - No guard against multiple calls: If
auth.isLoadingchanges from false to true and back to false, authentication will be attempted multiple times - Missing cleanup: The async
handleAuthcan callsetStateafter the component unmounts, causing React warnings and potential errors
Apply this diff to fix all three issues:
+ const hasAttempted = useRef(false);
+
async function handleAuth() {
+ if (hasAttempted.current) return;
+ hasAttempted.current = true;
+
setMessage("Logging in");
try {
const result = await authClient.signIn.ott({
useEffect(() => {
- if (!auth.isLoading) handleAuth();
+ let isMounted = true;
+
+ async function attemptAuth() {
+ if (hasAttempted.current || auth.isLoading) return;
+ hasAttempted.current = true;
+
+ setMessage("Logging in");
+ try {
+ const result = await authClient.signIn.ott({
+ token: token || "",
+ });
+
+ if (!isMounted) return;
+
+ if (result.error) {
+ setMessage(result.error.message || "");
+ setLoading(false);
+ } else {
+ toast.success("Logged in");
+ if (redirect && redirect.trim() !== "") {
+ try {
+ const redirectUrl = new URL(redirect);
+ navigate(redirectUrl.pathname);
+ } catch {
+ navigate(redirect);
+ }
+ } else {
+ navigate("/");
+ }
+ }
+ } catch (error) {
+ if (!isMounted) return;
+ setMessage("Something went wrong");
+ setLoading(false);
+ }
+ }
+
+ attemptAuth();
+
+ return () => {
+ isMounted = false;
+ };
}, [auth.isLoading]);Note: You'll need to add import { useRef } from "react"; at the top.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/routes/auth/ott.tsx around lines 48 to 50, update the effect to import
and use a ref to guard/track invocation and a mounted flag for cleanup: add
import { useRef, useEffect } from "react" (if not already), include handleAuth
in the dependency array to satisfy exhaustive-deps, use a callRef (e.g.,
hasRunRef) to prevent multiple invocations when auth.isLoading toggles, and
inside the effect set a mounted/aborted flag (or AbortController) so the async
handleAuth checks it before calling setState; ensure you cancel/flip the mounted
flag in the cleanup to prevent state updates after unmount.
| "", | ||
| "", | ||
| "width=500px,height=500px", | ||
| ); |
There was a problem hiding this comment.
Bug: Invalid window.open features string with px units
The window.open call uses "width=500px,height=500px" as the features string, but the px suffix is invalid. The features parameter expects integer values without units (e.g., "width=500,height=500"). This will cause the window dimensions to be ignored in most browsers, resulting in a default-sized popup window instead of the intended 500x500 size.
Note
Removes bundled Cookie Clicker/BitLife static assets and related files from
static/_app/tools/to slim the repo.static/_app/tools/(e.g.,cookieclicker/*,bitlife/*).DungeonGen.js,index.html,loc/*.js).Written by Cursor Bugbot for commit ad14db1. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Chores / Infra
✏️ Tip: You can customize this high-level summary in your review settings.