Conversation
- Updated Testimonials component to display 6 random testimonials instead of 5. - Added icons for Trustpilot and Google links in the testimonials section. - Improved styling for the testimonials section. feat: update index route with border radius styling feat: add plugins resource page for tracking plugin updates - Introduced a new Plugins resource page to manage and track Spigot plugin updates. - Implemented functionality to fetch and display plugin data from Spigot API. - Added UI elements for user interaction and plugin version management. fix: extend data utility types for plugins - Updated type definitions in dataUtils to include 'plugins' for better type safety.
…fo and download options
…ment - Introduced `loadingItemsContext` to manage loading states across components. - Enhanced user experience by providing loading indicators for actions that require time.
- Elements no longer have a default style again, got kinda annoying after a bit - Removed unnecessary class modifiers like 'my-0!' and 'mt-0!' for cleaner code. - Added alt attributes to Background components for better accessibility. - Enhanced layout styles in various sections for improved visual hierarchy.
- can add rename delete servers - can add delete plugins - saves to cookies
Contributor
📝 WalkthroughWalkthroughConsolidates analyze flows and redirects, adds a Plugin Updates feature with Spigot/Modrinth/Misc add dialogs and PluginCard, introduces a reusable SelectList and deepTrack for nested store reactivity, replaces static sitemap.txt with a dynamic sitemap route, updates robots.txt, and applies widespread typography/theme CSS and minor UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant AddSpigot as AddSpigotDialog
participant Spiget as Spiget API
participant Context as resolvedPluginContext
participant SelectList as SelectList Component
participant PluginCard as PluginCard
Browser->>AddSpigot: Paste Spigot resource URL
AddSpigot->>Spiget: Fetch metadata & versions
Spiget-->>AddSpigot: Return plugin data & versions
AddSpigot->>Context: Populate resolvedPlugin.plugins / resolvedPlugin.plugin
AddSpigot->>SelectList: Render versions SelectList
Browser->>SelectList: Choose version
SelectList->>Context: Set resolvedPlugin.plugin.version
Context->>PluginCard: Render PluginCard with plugin
PluginCard->>Spiget: Lazy fetch missing plugin details
Spiget-->>PluginCard: Return details & latestVersion
Browser->>PluginCard: Click "Download latest"
PluginCard->>PluginCard: Check/enforce rate limit
PluginCard-->>Browser: Open download link / start download
PluginCard->>Context: Update stored plugin.version to latestVersion
sequenceDiagram
participant Browser as Client
participant AnalyzeForm as Analyze Component
participant Redirect as Redirect.onGet
participant AnalyzeRoute as /resources/analyze/[id]
participant ProfilerAPI as Profiler API
Browser->>AnalyzeForm: Submit Spark/Paper link
AnalyzeForm->>Browser: Validate/parse -> produce id or error
Browser->>AnalyzeRoute: Navigate to /resources/analyze/{id}
AnalyzeRoute->>Redirect: onGet invoked (server)
Redirect-->>Browser: 301 redirect to canonical /resources/analyze/{id}
Browser->>AnalyzeRoute: Load route component
AnalyzeRoute->>ProfilerAPI: Fetch analysis (spark or timings)
ProfilerAPI-->>AnalyzeRoute: Return analysis results
AnalyzeRoute->>ProfilerAPI: Call collector endpoint (async)
AnalyzeRoute-->>Browser: Render results grid with actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
birdflop-com | e983161 | Mar 31 2026, 02:42 AM |
Deploying web with
|
| Latest commit: |
e983161
|
| Status: | 🚫 Build failed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “Plugin Updater” resource and performs a broad UI/route refactor across the site, including a new dynamic sitemap route and consolidation of the Spark/Timings analyzer into a single “Analyze” flow.
Changes:
- Add a new
/resources/pluginsplugin updater UI (localStorage-backed) plus supporting plugin components. - Replace the static
public/sitemap.txtwith a generatedsitemap.xmlroute and updaterobots.txtto reference it. - Consolidate
/resources/sparkprofile+/resources/papertimingsinto/resources/analyzewith redirects, and apply consistent heading/secondary-text styling across many routes.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/themeUtil.ts | Adds --lum-depth values per theme. |
| src/util/misc.ts | Adds deepTrack helper for store tracking. |
| src/util/dataUtils.ts | Guards JSON parsing + removes stray cookie debug log. |
| src/routes/sitemap.xml/index.tsx | New sitemap endpoint that enumerates routes. |
| src/routes/sitemap.xml/create-sitemap.ts | Sitemap XML string builder. |
| src/routes/settings/index.tsx | Heading/secondary text styling updates. |
| src/routes/resources/sparkprofile/index.tsx | Redirect legacy route to new analyze route. |
| src/routes/resources/sparkprofile/[id]/index.tsx | Redirect legacy dynamic route to new analyze route. |
| src/routes/resources/rgb/presets/index.tsx | Heading/text styling + “no results” layout change. |
| src/routes/resources/rgb/presets/[id]/index.tsx | Rename loading signal + heading/text styling tweaks. |
| src/routes/resources/rgb/index.tsx | Header slot heading/text styling tweaks. |
| src/routes/resources/plugins/index.tsx | New Plugin Updater page implementation. |
| src/routes/resources/papertimings/index.tsx | Redirect legacy route to new analyze route. |
| src/routes/resources/papertimings/[id]/index.tsx | Redirect legacy dynamic route to new analyze route. |
| src/routes/resources/index.tsx | Resources landing page redesign + links updated (Analyze/Plugins). |
| src/routes/resources/flags/index.tsx | Uses deepTrack and updates heading/text styling. |
| src/routes/resources/banner/index.tsx | Heading/text styling tweaks. |
| src/routes/resources/animtexture/index.tsx | Uses deepTrack and removes unused loading state. |
| src/routes/resources/animtab/index.tsx | Uses deepTrack and updates heading/text styling. |
| src/routes/resources/animpreview/index.tsx | Heading/text styling tweaks. |
| src/routes/resources/analyze/index.tsx | New consolidated Analyze landing page. |
| src/routes/resources/analyze/[id]/index.tsx | New unified analyzer results route (spark vs timings by id length). |
| src/routes/profile/[id]/index.tsx | Heading styling update. |
| src/routes/plans/index.tsx | Styling/layout tweaks for plans page sections. |
| src/routes/node-stats/index.tsx | Card styling changes + adds Hoverable behavior. |
| src/routes/layout.tsx | Adds alt props to background images + notification heading styling. |
| src/routes/layout-profile.tsx | Profile layout heading/text styling updates. |
| src/routes/layout-markdown.tsx | Adds .markdown wrapper class for markdown layouts. |
| src/routes/index.tsx | Home page styling/layout adjustments (headings/cards/sections). |
| src/routes/docs/layout.tsx | Applies .markdown styling wrapper to docs layout. |
| src/routes/docs/[...404]/index.tsx | Heading styling tweak + removes outdated comment. |
| src/routes/admin/index@profile.tsx | Heading/text styling tweaks. |
| src/routes/acornmc/vote/index.tsx | Heading styling tweak. |
| src/routes/[...404]/index.tsx | Removes outdated comment. |
| src/global.css | Attempts to scope formatting styles to .markdown + HR styling + removes global heading margins. |
| src/components/plugins/PluginCard.tsx | New plugin display card + spiget fetching + actions. |
| src/components/plugins/AddSpigotDialog.tsx | New dialog flow for adding spigot plugins + version selection. |
| src/components/plugins/AddMiscDialog.tsx | New dialog flow for adding manual “misc” plugins. |
| src/components/home/Testimonials.tsx | Changes layout + adds Trustpilot/Google icons/links. |
| src/components/docs/SideBar.tsx | Minor heading styling tweak. |
| src/components/docs/Contributors.tsx | Button styling tweak for contributor links. |
| src/components/analyze/Redirect.tsx | Shared redirect handler for legacy analyze routes. |
| src/components/analyze/PaperTimings.tsx | Removes old dedicated PaperTimings page component. |
| src/components/analyze/Analyze.tsx | Updates Analyze component to handle both spark + timings links. |
| src/components/Rgbirdflop/UsersPublicPresets.tsx | Heading/layout tweaks. |
| src/components/Rgbirdflop/RGBirdflop.tsx | Switches to deepTrack + updates experimental label key + docs link. |
| src/components/Rgbirdflop/Presets.tsx | Replaces custom list UI with new SelectList. |
| src/components/Rgbirdflop/PresetPreview.tsx | Renames loading signal to isLoading. |
| src/components/Rgbirdflop/MyPrivatePresets.tsx | Modal layout refactor and publish UX tweaks. |
| src/components/Rgbirdflop/Input.tsx | Minor label layout tweak. |
| src/components/Elements/SelectList.tsx | Adds new SelectList component. |
| src/components/Elements/Nav.tsx | Updates nav link from Spark Profile to Analyze. |
| src/components/Elements/Footer.tsx | Adds top margin to footer. |
| public/sitemap.txt | Removes static sitemap list. |
| public/robots.txt | Adds sitemap reference and adjusts disallow rules. |
Comments suppressed due to low confidence (2)
src/components/analyze/Analyze.tsx:45
- When a valid Spark/Timings link is entered,
error.valueis never cleared, so a previous error message can remain visible even thoughredirect.valueis set. Clearerror.valuein the branches where a validcodeis extracted (and consider validating the timings link contains?id=before extracting).
src/components/analyze/Analyze.tsx:65 - The help text prints the same analyze URL twice. This looks like an accidental duplication; remove one line or replace it with the second (distinct) example URL you intended to show.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+109
to
+112
| useTask$(({ track }) => { | ||
| deepTrack(track, pluginsStore); | ||
| console.log(Date.now()); | ||
|
|
There was a problem hiding this comment.
console.log(Date.now()) inside a tracked useTask$ will spam the console and can significantly slow down the page because this task reruns on any tracked store change. Please remove this log (or guard it behind a dev-only flag).
| const select = selectRef.value; | ||
| if (select) { | ||
| select.value = value.toString(); | ||
| select.dispatchEvent(new Event('change')); |
There was a problem hiding this comment.
select.dispatchEvent(new Event('change')) creates a non-bubbling event by default. Since Qwik uses event delegation for onChange$, this may not trigger the handler in production. Dispatch the event with { bubbles: true } (or call the onChange$ handler directly) to ensure the selection change is observed.
Suggested change
| select.dispatchEvent(new Event('change')); | |
| select.dispatchEvent(new Event('change', { bubbles: true })); |
Comment on lines
+3
to
+7
| export const deepTrack = (track: any, obj: any) => { | ||
| track(obj); | ||
| for (const o of Object.values(obj)) { | ||
| if (unwrapStore(o) !== o) deepTrack(track, o); | ||
| } |
There was a problem hiding this comment.
deepTrack calls track(obj) but Qwik's track expects a function (e.g. track(() => store.foo)), so this will either throw at runtime or fail to register dependencies. Also, iterating Object.values(obj) without guarding for null/non-objects (and without cycle detection) can crash on null values or recurse infinitely on cyclic references.
Refactor deepTrack to track(() => obj[key]) per key (and recurse only into non-null objects), ideally with a WeakSet to prevent cycles.
Comment on lines
57
to
60
| useTask$(({ track }) => { | ||
| if (isBrowser) setCookies('animtab', { version: rgbStore.version, ...animtabStore }); | ||
| (Object.keys(animtabStore) as Array<keyof typeof animtabStore>).forEach((key) => { | ||
| track(() => animtabStore[key]); | ||
| }); | ||
| deepTrack(track, rgbStore); | ||
| }); |
There was a problem hiding this comment.
This task writes the animtab cookie using animtabStore, but it only tracks rgbStore changes. As a result, changes to animtabStore (speed/type/etc.) may not trigger the task and the cookie can become stale.
Track animtabStore here (or track both stores) so cookie persistence updates when animtab settings change.
| import AddMiscDialog from '~/components/plugins/AddMiscDialog'; | ||
| import { deepTrack } from '~/util/misc'; | ||
|
|
||
| const debug = true; |
There was a problem hiding this comment.
debug is hard-coded to true, which exposes test-only UI/actions (e.g., “Mark all out of date”) in production builds. Gate this behind an environment flag (e.g. import.meta.env.DEV) or remove it before merging.
Comment on lines
+9
to
+17
| return ` | ||
| <urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.sitemaps.org/schemas/sitemap/0.9 http://www.sitemaps.org/schemas/sitemap/0.9/sitemap.xsd"> | ||
| ${entries.map( | ||
| (entry) => ` | ||
| <url> | ||
| <loc>${baseUrl}${entry.loc.startsWith('/') ? '' : '/'}${entry.loc}</loc> | ||
| <priority>${entry.priority}</priority> | ||
| </url>`, | ||
| )} |
There was a problem hiding this comment.
entries.map(...) is interpolated directly into the template string, which will stringify as a comma-separated list. That will insert commas into the XML output and can make the sitemap invalid. Join the mapped strings (e.g. entries.map(...).join('')) before interpolating.
Comment on lines
+6
to
10
|
|
||
| .markdown { | ||
| @import "@luminescent/ui/formatting"; | ||
| } | ||
|
|
There was a problem hiding this comment.
CSS @import rules must be top-level; nesting @import "@luminescent/ui/formatting" inside .markdown { ... } is invalid CSS and is likely to break the build or be ignored by the browser.
If the goal is to scope formatting styles, import them at the top level and scope the selectors (or switch to a CSS module / a @layer-based approach) rather than nesting the import.
Suggested change
| .markdown { | |
| @import "@luminescent/ui/formatting"; | |
| } | |
| @import "@luminescent/ui/formatting"; |
Comment on lines
+45
to
+46
| <p class="lum-text-secondary"> | ||
| {field.value} |
There was a problem hiding this comment.
class="lum-text-secondary" is inconsistent with the rest of the codebase (which uses text-lum-text-secondary) and likely won’t apply any styling unless there’s a dedicated CSS class for it. Consider changing this to text-lum-text-secondary for consistency and to ensure the text is actually styled as secondary.
| id="publish-preset" | ||
| disabled={isSubmitting.value} | ||
| > | ||
| <Save size={20} /> Publish {isSubmitting.value ? 'Validating...' : 'Publish'} |
There was a problem hiding this comment.
Button label renders as Publish Publish when not submitting because it includes the word "Publish" twice (Publish {… ? 'Validating...' : 'Publish'}). Consider changing the dynamic portion to something like 'Validating…' : '' or use 'Publish' : 'Validating…' without the extra prefix.
Suggested change
| <Save size={20} /> Publish {isSubmitting.value ? 'Validating...' : 'Publish'} | |
| <Save size={20} /> Publish{isSubmitting.value ? ' Validating...' : ''} |
Comment on lines
+75
to
+76
| console.log('Searching for plugin:', value); | ||
|
|
There was a problem hiding this comment.
console.log('Searching for plugin:', value); will end up in production bundles and can leak user input into logs. Please remove it or guard it behind a dev-only flag.
Suggested change
| console.log('Searching for plugin:', value); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/util/dataUtils.ts (1)
29-33:⚠️ Potential issue | 🟠 MajorPrevent empty JSON fields from being coerced to
0At Line 29, falsy
format/colors/shadowcolorsnow skipJSON.parse, then fall through to Line 33 where''becomes0. This breaks downstream assumptions (src/routes/api/v2/rgb/index.tsxLine 97 and Lines 103-108 expect object/array shapes).💡 Proposed fix
- if ((key == 'format' || key == 'colors' || key == 'shadowcolors') && params[key]) { - params[key] = JSON.parse(params[key]); - } - else if (params[key] === 'true' || params[key] === 'false') params[key] = params[key] === 'true'; - else if (!isNaN(Number(params[key]))) params[key] = Number(params[key]); + const isJsonField = key == 'format' || key == 'colors' || key == 'shadowcolors'; + if (isJsonField) { + if (params[key] === '' || params[key] == null) { + params[key] = undefined; + } else { + params[key] = JSON.parse(params[key]); + } + } + else if (params[key] === 'true' || params[key] === 'false') params[key] = params[key] === 'true'; + else if (!isNaN(Number(params[key]))) params[key] = Number(params[key]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/dataUtils.ts` around lines 29 - 33, The code currently JSON.parses format/colors/shadowcolors only when truthy, but then falls through to numeric coercion so empty strings ('') convert to 0; update the branching so that for the specific keys 'format', 'colors', and 'shadowcolors' you only call JSON.parse when params[key] is a non-empty string (e.g., params[key] !== ''), and in the numeric conversion branch (the check using Number(params[key]) / isNaN) guard against empty strings as well (skip numeric coercion when params[key] === '') so '' is left as '' (or null/undefined if you prefer) to preserve expected object/array shapes read later from params.src/routes/layout-profile.tsx (1)
55-66:⚠️ Potential issue | 🟡 MinorKeep the
<h1>text-only; move the admin action link outside it.At Line 61, putting a navigation action inside the main heading is semantically noisy for assistive tech.
♿ Suggested structure tweak
-<h1 class="font-extrabold text-3xl mb-2 flex items-center gap-4"> +<div class="flex items-center gap-4 mb-2"> + <h1 class="font-extrabold text-3xl flex items-center gap-4 m-0"> {session.value.user.image && <img src={session.value.user.image} width={36} height={36} class="rounded-full! w-9 h-9" /> } {t('nav.profile.hey@@Hey')}, {session.value.user?.name || 'User'}! - {isAdmin && - <Link href="/admin" class="lum-btn"> - <AppWindow /> - Admin Panel - </Link> - } -</h1> + </h1> + {isAdmin && + <Link href="/admin" class="lum-btn"> + <AppWindow /> + Admin Panel + </Link> + } +</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/layout-profile.tsx` around lines 55 - 66, The <h1> currently contains interactive content (the admin Link and AppWindow icon) which is semantically incorrect for assistive tech; move the conditional Link (the element rendered when isAdmin) out of the <h1> and place it immediately after the heading so the <h1> remains text-only. Keep the existing checks using session.value.user.image and session.value.user?.name inside the <h1>, and retain the conditional rendering on isAdmin for the Link and AppWindow component, only changing their placement to follow the <h1>.src/components/Rgbirdflop/PresetPreview.tsx (1)
142-173:⚠️ Potential issue | 🟠 MajorAlways clear
isLoadingon failure.A rejected
savePreset,unsavePreset, orsetUserDataskips the last assignment here, leaving the button disabled and the optimistic local mutation half-applied. Wrap the async body intry/finally, and only commit or roll back the local preset changes once the request result is known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Rgbirdflop/PresetPreview.tsx` around lines 142 - 173, The click handler currently sets isLoading.value and mutates privatePresets.value/savedPresets.value optimistically but never clears isLoading or rolls back on rejected savePreset/unsavePreset/setUserData; wrap the async body of the onClick$ handler in try/finally to ensure isLoading.value is cleared, and restructure to perform optimistic changes only after capturing originals (e.g., const prevPrivate = [...privatePresets.value], const prevSaved = [...savedPresets.value]) and then await the network call (savePreset/unsavePreset/setUserData) — on failure restore privatePresets.value = prevPrivate and savedPresets.value = prevSaved before rethrowing or handling the error; also only update Preset.saves after a successful result and persist to localStorage after success.src/components/Rgbirdflop/Presets.tsx (1)
131-148:⚠️ Potential issue | 🟡 MinorRestore the empty state for saved presets.
SelectListstill renders its card even when it has novaluesand no slotted buttons. With both preset lists empty, this change shows a blank panel instead of telling the user why it's empty. Add the previous fallback copy or skip rendering the list until it has content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Rgbirdflop/Presets.tsx` around lines 131 - 148, The SelectList component currently renders an empty card when both privatePresets.value and savedPresets.value are empty; update the JSX around SelectList so it either returns the previous fallback copy (e.g., a small message explaining there are no saved presets) or conditionally omits rendering the SelectList entirely when privatePresets.value.concat(savedPresets.value) is empty; locate the SelectList usage and the mapping over privatePresets.value.concat(savedPresets.value.map(...)) and add a guard or render a fallback node (using the same styling/context) before mapping, ensuring functions/identifiers like SelectList, privatePresets, savedPresets, renderPreview, and loadPresetJSON remain unchanged.src/routes/resources/animtexture/index.tsx (1)
60-116:⚠️ Potential issue | 🟠 MajorCancel the RAF loop before starting a new one when the task reruns.
deepTrack(track, animtextureStore)triggers this task on most store changes, and each rerun starts a freshrequestAnimationFrame(animate)chain without cancelling the previous one. Multiple uncancelled loops will paint the same canvas concurrently, causing jank and potential visual corruption. Store the animation frame ID and cancel it before starting a new animation sequence, or use a cleanup return handler to cancel pending frames when the task reruns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/animtexture/index.tsx` around lines 60 - 116, The task spawns a new RAF loop on every rerun (useTask$ / deepTrack) and never cancels the previous one; add a scoped rafId (e.g., let rafId: number | null) in the useTask$ body, call cancelAnimationFrame(rafId) before starting a new requestAnimationFrame(animate), and return a cleanup function from the useTask$ callback that calls cancelAnimationFrame(rafId) to ensure any pending RAF from the previous run is cancelled; do this around the existing animate/requestAnimationFrame usage referencing animate, animtextureStore, animCanvasRef, and textureCanvasRef.src/routes/resources/rgb/presets/[id]/index.tsx (1)
195-214:⚠️ Potential issue | 🟠 MajorHandle save/unsave failures before committing the optimistic state.
If
savePresetorunsavePresetrejects,isLoadingnever resets and the local signals have already been mutated, so the page shows the wrong saved state until reload.🛠️ Suggested fix
}} disabled={isLoading.value} onClick$={async () => { isLoading.value = true; - - if (existingPreset) { - privatePresets.value = privatePresets.value.filter((p) => p !== existingPreset); - if (presetInfo.id) { - savedPresets.value = savedPresets.value.filter((p) => p.id !== presetInfo.id); - await unsavePreset(presetInfo.id); - } - } - else { - privatePresets.value = [...privatePresets.value, presetInfo.preset]; - if (presetInfo.id) { - savedPresets.value = [...savedPresets.value, presetInfo]; - await savePreset(presetInfo.id); - } - } - - if (isBrowser) localStorage.setItem('privatePresets', JSON.stringify(privatePresets.value)); - isLoading.value = false; + try { + if (presetInfo.id) { + if (existingPreset) await unsavePreset(presetInfo.id); + else await savePreset(presetInfo.id); + } + + privatePresets.value = existingPreset + ? privatePresets.value.filter( + (preset) => JSON.stringify(preset) !== JSON.stringify(presetInfo.preset), + ) + : [...privatePresets.value, presetInfo.preset]; + + if (presetInfo.id) { + savedPresets.value = existingPreset + ? savedPresets.value.filter((p) => p.id !== presetInfo.id) + : [...savedPresets.value, presetInfo]; + } + + if (isBrowser) { + localStorage.setItem('privatePresets', JSON.stringify(privatePresets.value)); + } + } catch (err) { + notifications.push( + new Notification() + .setTitle('Failed to update preset') + .setDescription(`Error: ${err}`) + .setBgColor('lum-bg-red/50') + .setPersist(true), + ); + } finally { + isLoading.value = false; + } }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/rgb/presets/`[id]/index.tsx around lines 195 - 214, The current click handler mutates signals (privatePresets, savedPresets, localStorage) before awaiting savePreset/unsavePreset and never resets isLoading on rejection; update the flow in the onClick$ handler so that you first set isLoading.value = true, then call and await savePreset(presetInfo.id) or unsavePreset(presetInfo.id) inside try/catch, only mutate privatePresets.value and savedPresets.value (and write localStorage) after the awaited call succeeds, and in a finally block set isLoading.value = false; ensure you reference existingPreset, presetInfo, savePreset, unsavePreset, privatePresets, savedPresets, localStorage and isLoading exactly as in the diff so failures do not leave optimistic state or a stuck loader.
🟡 Minor comments (12)
src/routes/sitemap.xml/index.tsx-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorRemove debug
console.logstatement.This appears to be leftover debug code that will log every route path on each sitemap request, polluting server logs.
🧹 Proposed fix
// Check if the route has a priority override Object.keys(priorities).forEach(priorityRoute => { - console.log(loc); if (loc.startsWith(priorityRoute)) { priority = priorities[priorityRoute as keyof typeof priorities]; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/sitemap.xml/index.tsx` at line 33, Remove the debug logging call console.log(loc) from the sitemap generation path (the stray console.log call in src/routes/sitemap.xml/index.tsx) so route paths are not emitted to server logs; either delete that line or replace it with an appropriate logger at debug level if you need to keep the information, ensuring the sitemap code no longer prints loc to stdout on each request.src/components/home/Testimonials.tsx-146-149 (1)
146-149:⚠️ Potential issue | 🟡 MinorRemove duplicate
id="testimonials"to keep valid DOM anchors.Line 146 and Line 147 both use the same
id, which can cause ambiguous anchor targeting and accessibility issues.Suggested fix
- <Anchor id="testimonials"> - <h3 id="testimonials" class="font-extrabold text-5xl my-6 mr-2"> + <Anchor id="testimonials"> + <h3 class="font-extrabold text-5xl my-6 mr-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/home/Testimonials.tsx` around lines 146 - 149, The Anchor wrapper and the inner h3 both set id="testimonials", producing duplicate IDs; remove the id from one of them (preferably the inner <h3> or adjust the <Anchor> to only provide the id) so only a single element uses id="testimonials". Update the JSX in the Testimonials component (look for the Anchor component and the h3 element) to leave the anchor target on one element and keep the other id-free to preserve valid DOM anchors and accessibility.src/components/Rgbirdflop/MyPrivatePresets.tsx-255-255 (1)
255-255:⚠️ Potential issue | 🟡 MinorButton text is duplicated.
The button will display "Publish Publish" when not submitting, and "Publish Validating..." when submitting. The static "Publish" text should be removed since the ternary already handles both states.
🐛 Proposed fix
- <Save size={20} /> Publish {isSubmitting.value ? 'Validating...' : 'Publish'} + <Save size={20} /> {isSubmitting.value ? 'Validating...' : 'Publish'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Rgbirdflop/MyPrivatePresets.tsx` at line 255, The Save button currently renders duplicate text because it concatenates a static "Publish" with the ternary using isSubmitting.value; in the JSX for the Save element in MyPrivatePresets (where you have <Save size={20} /> Publish {isSubmitting.value ? 'Validating...' : 'Publish'}), remove the static "Publish" token and render only the ternary (keeping the <Save size={20} /> icon and spacing) so the label becomes {isSubmitting.value ? 'Validating...' : 'Publish'}.src/routes/index.tsx-114-116 (1)
114-116:⚠️ Potential issue | 🟡 MinorAvoid duplicate anchor IDs in the same section.
Line 114 + Line 115 both define
plans, and Line 171 + Line 172 both definefeatures. Duplicate IDs can cause inconsistent#hashnavigation and accessibility issues.Suggested fix
- <Anchor id="plans"> - <h3 id="plans" class="font-extrabold text-5xl my-6 mr-2"> + <Anchor id="plans"> + <h3 class="font-extrabold text-5xl my-6 mr-2"> Plans </h3> </Anchor> ... - <Anchor id="features"> - <h3 id="features" class="font-extrabold text-5xl my-6 mr-2"> + <Anchor id="features"> + <h3 class="font-extrabold text-5xl my-6 mr-2"> Features </h3> </Anchor>Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.tsx` around lines 114 - 116, The page currently sets the same id on both the Anchor wrapper and its inner h3 (e.g., id="plans" and id="features"), causing duplicate IDs; update the JSX in src/routes/index.tsx so each id is unique by removing the duplicate id from either the Anchor or the h3 (prefer removing the id on the inner h3 and keep it on the Anchor, or vice versa) for the Anchor/h3 pairs that reference "plans" and "features" (look for the Anchor component and the h3 elements with id="plans" and id="features") so hash navigation and accessibility use a single unique element id.src/components/analyze/Redirect.tsx-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorEncode
params.idbefore building the redirect URL.Line 5 currently injects
params.iddirectly into the path. UseencodeURIComponentso special characters don’t break the redirect target.Suggested fix
export const onGet: RequestHandler = ({ redirect, params }) => { - if (params.id) redirect(301, `/resources/analyze/${params.id}`); + if (params.id) redirect(301, `/resources/analyze/${encodeURIComponent(params.id)}`); else redirect(301, '/resources/analyze'); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/analyze/Redirect.tsx` at line 5, The redirect uses params.id directly which can break URLs with special characters; update the redirect call in Redirect.tsx to encode params.id before building the path (use encodeURIComponent on params.id) so the redirect(301, ...) target becomes safe; locate the conditional that checks params.id and replace the unencoded template segment with the encoded value while keeping the same redirect function and behavior.src/routes/layout.tsx-214-219 (1)
214-219:⚠️ Potential issue | 🟡 MinorKeep these backgrounds decorative.
These fixed blurred backgrounds sit behind the page content, so
"Background"adds noise for assistive tech instead of useful context. If these components render<img>, usealt=""; otherwise hide them from the accessibility tree witharia-hidden="true".Also applies to: 221-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/layout.tsx` around lines 214 - 219, The Background components are decorative and currently expose unnecessary content to assistive tech; update the Background instances (e.g., Background id="bg" and the other Background at 221-227) to be hidden from the accessibility tree: if Background renders an <img> provide alt="" to make it decorative, otherwise pass aria-hidden="true" (or ensure the component outputs aria-hidden when used as decorative) so screen readers ignore these fixed blurred backgrounds.src/routes/resources/plugins/index.tsx-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorDebug flag left enabled.
The
debugconstant is hardcoded totrue, which will show the "Mark all out of date" debug button in production. Consider using an environment variable or removing this before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` at line 12, The hardcoded debug flag (const debug = true) should be removed or controlled via an environment variable; update the declaration of debug to read from an env var (e.g., check process.env.NODE_ENV or a dedicated REACT_APP_DEBUG / VITE_DEBUG flag) so the debug button isn't shown in production, and ensure the rest of the module that references debug (the "Mark all out of date" button logic) uses this env-driven boolean; also add a safe default of false if the env var is not set.src/components/analyze/Analyze.tsx-62-65 (1)
62-65:⚠️ Potential issue | 🟡 MinorDuplicate URL lines.
Lines 63 and 64 are identical. Based on the context mentioning "timings or profile id," this appears to be an unintentional duplication. Consider removing one line or differentiating them if the intent was to show different examples.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/analyze/Analyze.tsx` around lines 62 - 65, In the Analyze component remove the duplicated example URL span (the two identical <span class="text-lum-text-secondary">https://birdflop.com/resources/analyze/[id]</span>) — either delete one of them or replace the second with a distinct example (e.g., show a timings vs profile example such as /resources/analyze/timings/[id] or /resources/analyze/profile/[id]) so the JSX no longer renders the same URL twice; update the JSX in Analyze.tsx where those span elements are defined.src/components/Elements/SelectList.tsx-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorIncorrect ref type.
The
selectRefis typed asHTMLInputElementbut is assigned to a<select>element. This will cause type errors when accessing select-specific properties.🔧 Proposed fix
- const selectRef = useSignal<HTMLInputElement>(); + const selectRef = useSignal<HTMLSelectElement>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Elements/SelectList.tsx` at line 22, The ref for the select element is incorrectly typed as HTMLInputElement; update the useSignal generic to HTMLSelectElement (change selectRef = useSignal<HTMLInputElement>() to useSignal<HTMLSelectElement>()) and adjust any usages of selectRef.current to use select-specific properties (e.g., selectedIndex, value) so TypeScript reflects the correct DOM element type in the SelectList component.src/routes/resources/plugins/index.tsx-109-112 (1)
109-112:⚠️ Potential issue | 🟡 MinorRemove debug console.log.
Line 111 contains a debug
console.log(Date.now())statement that logs on every store change. This should be removed before merging.🔧 Proposed fix
useTask$(({ track }) => { deepTrack(track, pluginsStore); - console.log(Date.now()); if (!isBrowser) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 109 - 112, Remove the debug console.log by deleting the console.log(Date.now()) call inside the useTask$ callback where deepTrack(track, pluginsStore) is invoked; the cleanup should leave the useTask$ callback only performing deepTrack(track, pluginsStore) (no console logging) so pluginsStore changes no longer print timestamps.src/components/analyze/Analyze.tsx-36-38 (1)
36-38:⚠️ Potential issue | 🟡 MinorSpark URL parsing doesn't strip query parameters or hash fragments.
The Spark URL extraction uses a simple
replace()which will include any trailing query strings or hash fragments in the code. For example,https://spark.lucko.me/abc123#sectionwould produce codeabc123#section, causing the analysis to fail.Consider aligning with the timings parsing approach:
🔧 Proposed fix
- else if (link.startsWith('https://spark.lucko.me')) { - code = link.replace('https://spark.lucko.me/', ''); - } + else if (link.startsWith('https://spark.lucko.me')) { + code = link.replace('https://spark.lucko.me/', '').split('?')[0].split('#')[0]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/analyze/Analyze.tsx` around lines 36 - 38, The current branch that handles Spark links (the else if checking link.startsWith('https://spark.lucko.me')) uses replace to extract code but doesn't remove query strings or hash fragments; update the extraction so it first gets the pathname segment (or split the replaced string on / then take the last segment) and then strip any query/hash by splitting on '?' or '#' (or use the URL constructor to read pathname) before assigning to code; reference the variables/conditions in this block (link, code, the startsWith('https://spark.lucko.me') branch) and ensure the final code contains only the identifier without leading slashes, queries, or fragments.src/routes/resources/plugins/index.tsx-249-258 (1)
249-258:⚠️ Potential issue | 🟡 MinorImport lacks validation and error feedback.
The import handler silently fails if JSON parsing throws, and doesn't validate that the parsed data matches the expected
PluginWithData[]structure. Malformed imports could corrupt the plugins store.🔧 Proposed fix
onInput$={(e, el) => { if (!pluginsStore.openServer) return; try { const importedPlugins = JSON.parse(el.value) as PluginWithData[]; + if (!Array.isArray(importedPlugins)) { + throw new Error('Invalid format: expected an array'); + } pluginsStore.servers[pluginsStore.openServer].push(...importedPlugins); + el.value = ''; + const notification = new Notification() + .setTitle('Plugins imported') + .setDescription(`Successfully imported ${importedPlugins.length} plugin(s).`) + .setBgColor('lum-bg-green/50'); + notifications.push(notification); } catch (err) { - console.error('Failed to parse imported plugins:', err); + const notification = new Notification() + .setTitle('Import failed') + .setDescription(`Failed to parse imported plugins: ${err}`) + .setBgColor('lum-bg-red/50'); + notifications.push(notification); } }}/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 249 - 258, The import input's onInput$ handler for the element with id "import" currently swallows JSON parse errors and doesn't validate the parsed value before mutating pluginsStore.servers[pluginsStore.openServer]; update the handler to (1) validate pluginsStore.openServer exists, (2) try/catch JSON.parse and show user-facing error feedback (e.g., set a local error state or dispatch a toast) instead of only console.error, (3) validate the parsed object is an array and each item conforms to PluginWithData shape (check required fields/types) before pushing, and (4) only push the validated items into pluginsStore.servers[pluginsStore.openServer] using the existing symbols pluginsStore and PluginWithData to locate and guard the mutation.
🧹 Nitpick comments (4)
src/routes/sitemap.xml/index.tsx (1)
32-37: Priority matching relies on object key iteration order.The current logic assigns the priority of the last matching key in
priorities. While object key order is generally preserved for string keys in modern JavaScript, this behavior is implicit and could lead to unexpected results if keys are reordered.Consider sorting priority keys by specificity (longest first) or using a more explicit matching strategy.
♻️ Alternative: Match most specific route first
- // Check if the route has a priority override - Object.keys(priorities).forEach(priorityRoute => { - if (loc.startsWith(priorityRoute)) { - priority = priorities[priorityRoute as keyof typeof priorities]; - } - }); + // Check if the route has a priority override (most specific match wins) + const matchingRoute = Object.keys(priorities) + .filter(priorityRoute => loc.startsWith(priorityRoute)) + .sort((a, b) => b.length - a.length)[0]; + + if (matchingRoute) { + priority = priorities[matchingRoute as keyof typeof priorities]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/sitemap.xml/index.tsx` around lines 32 - 37, The priority selection currently iterates Object.keys(priorities) and assigns the priority of the last matching key (variables: priorities, priorityRoute, loc), which depends on object key order; change this to match the most specific route first by sorting the priority keys by specificity (for example by descending string length) or by an explicit specificity comparator, then iterate the sorted keys and break on the first loc.startsWith(match) to set priority; ensure you use priorities[matchingKey as keyof typeof priorities] and stop after the first match so the most specific route wins.src/global.css (1)
145-147: Consider making<hr>border rendering explicit.Adding
border-0 border-tavoids browser-default variance.🎯 Optional tweak
hr { - `@apply` my-6 border-lum-border/10; + `@apply` my-6 border-0 border-t border-lum-border/10; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/global.css` around lines 145 - 147, Update the hr rule to explicitly reset browser defaults and enforce a top border by adding the utility classes "border-0" and "border-t" to the existing selector (hr { `@apply` my-6 border-lum-border/10; }) so the style uses a consistent top border across browsers; keep the existing spacing and color class but include the explicit border reset and top-border utilities.src/routes/resources/rgb/presets/index.tsx (1)
371-378: Keep the title and CTA stacked on small screens.Removing the mobile
flex-collayout forces the heading text and “Publish your own preset” button onto one row. On narrow viewports that is likely to overflow or squeeze the CTA.♻️ Suggested tweak
- <h1 class="flex gap-3 text-2xl font-extrabold items-center my-2"> + <h1 class="flex flex-col gap-3 text-2xl font-extrabold items-start sm:flex-row sm:items-center my-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/rgb/presets/index.tsx` around lines 371 - 378, The heading and CTA were forced into a single row; restore mobile stacking by making the h1 use a column layout on small screens and switch to row on larger screens (e.g., add a responsive utility like "flex-col md:flex-row" to the h1 with the current "flex" class), keeping existing gap/centering classes; ensure the inner span and the <a> (lum-btn) still align by leaving the span's "flex-1" and other classes intact so the CTA flows below the title on narrow viewports and returns to a row on medium+ screens.src/components/plugins/PluginCard.tsx (1)
65-67: RedundantisBrowsercheck.
useVisibleTask$only executes on the client, making theisBrowsercheck on line 66 unnecessary.🔧 Proposed fix
useVisibleTask$(async () => { - if (!isBrowser) return; // dont request plugin data on the server if (plugin.data || !plugin.id) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/PluginCard.tsx` around lines 65 - 67, Remove the redundant client check: inside the useVisibleTask$ callback delete the isBrowser guard (the conditional using isBrowser) because useVisibleTask$ only runs on the client; keep the existing early returns for plugin.data and plugin.id (references: useVisibleTask$, isBrowser, plugin.data, plugin.id) so the task still skips when plugin data exists or id is missing and ensure no other logic depends on isBrowser elsewhere in this function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/home/Testimonials.tsx`:
- Around line 139-141: The current code mutates the module-scoped testimonials
array by calling testimonials.sort(...) during render and uses a biased random
comparator; fix by making a shallow copy and applying an in-place Fisher‑Yates
shuffle to that copy before slicing: create e.g. const arr = [...testimonials];
implement a FisherYatesShuffle(arr) that swaps elements using Math.random(),
call FisherYatesShuffle(arr) and then const sixTestimonials = arr.slice(0, 6);
ensure you remove the .sort(...) usage so the original testimonials array
remains untouched (refer to the sixTestimonials variable and the testimonials
array in Testimonials.tsx).
In `@src/components/plugins/AddMiscDialog.tsx`:
- Around line 27-33: Compute a non-empty derived name from the input instead of
using value.split('/').pop() with the nullish coalescing that preserves a
previous name; in the onInput$ handler for resolvedPlugin.plugin derive the name
by splitting value on '/', filtering out empty segments, taking the last
segment, and if that result is empty or value is empty fallback to a safe
default (e.g. 'misc-plugin'); assign that derived non-empty name to
resolvedPlugin.plugin.name and avoid using ?? to preserve a cleared name.
In `@src/components/plugins/AddSpigotDialog.tsx`:
- Around line 135-146: Wrap the Spiget API call inside the onChange$ handler in
a try-catch and guard against non-OK responses: when handling the selectedPlugin
in the onChange$ function, perform the
fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?...`) and await
response.json() inside a try block, check response.ok before parsing, and only
assign selectedPlugin.data.versions and update resolvedPlugin.plugin when the
request succeeds; in the catch block (or on non-ok) log or surface the error
(e.g., via console.error or an existing logger) and avoid mutating
resolvedPlugin so the UI remains consistent.
- Around line 70-112: The onChange$ handler's network call needs robust error
handling: wrap the fetch to https://api.spiget.org/v2/search/resources/... and
the subsequent response.json() in a try-catch inside the onChange$ callback,
verify response.ok before parsing, validate that the parsed JSON is an array
before mapping into resolvedPlugin.plugins, and on any failure push a
descriptive Notification (e.g., "Search failed" with error message or
"Unexpected response format") instead of assuming success; update the code paths
around onChange$, fetch, response.json(), and resolvedPlugin.plugins to
implement these checks and error notifications.
- Around line 37-68: Wrap the Spiget API calls and JSON parsing (the fetch for
`https://api.spiget.org/v2/resources/${pluginId}` and the fetch for
`/versions?size=100...`) in a try-catch, check each response's
`res.ok`/`versionsRes.ok` and throw or handle non-OK responses before calling
`res.json()`/`versionsRes.json()`, and on error set an appropriate failure state
on `resolvedPlugin` (or return early) instead of proceeding to construct
`newPlugin` and assigning `resolvedPlugin.plugin = newPlugin`; ensure errors
from JSON parsing or network failures are caught and a clear error message is
propagated via the same `resolvedPlugin` mechanism you use for success.
In `@src/global.css`:
- Around line 7-9: The `@import` rule is incorrectly placed inside the .markdown
selector; move the `@import` "@luminescent/ui/formatting" statement out of the
.markdown block to the top of the stylesheet (before any selectors) and remove
it from inside .markdown so the stylesheet no longer violates CSS import
placement rules.
In `@src/routes/resources/analyze/index.tsx`:
- Around line 8-14: Update the Analyze component's instructional block (the <p
class="my-12"> currently listing Spark steps) to also include concise Paper
Timings collection instructions so the UI matches the component description; add
a short numbered subsection after the Spark steps explaining how to enable Paper
Timings on Paper servers (include a link to the Paper Timings docs/site), the
command to run to generate a timings report (e.g., the timings command used on
Paper), and how to copy & paste the resulting report link into the analyzer
input; ensure labels mention both "Spark Profile" and "Paper Timings" and keep
the same styling/structure as the existing Spark instructions.
In `@src/routes/resources/animtab/index.tsx`:
- Around line 57-60: The useTask$ currently only subscribes to rgbStore via
deepTrack(track, rgbStore), so changes in animtabStore
(speed/type/length/output) aren't tracked; update the task to also
deepTrack(track, animtabStore) (or otherwise include animtabStore in the
deepTrack call) so the cookie writer reacts to animtabStore changes, leaving the
setCookies('animtab', { version: rgbStore.version, ...animtabStore }) call
as-is.
In `@src/routes/resources/plugins/index.tsx`:
- Around line 298-309: The loop prematurely exits because the click handler uses
return inside the for...of over plugins; replace that return with continue so
non-updatable plugins are skipped but the loop continues processing remaining
items. Update the block around the for (const plugin of plugins) loop, keeping
the existing checks (updateAvailable and plugin.data?.file?.url), keeping the
spigot branch that calls downloadSpigotPlugin(plugin, spigotRateLimit) and the
window.open fallback, and ensure plugin.version = plugin.data?.latestVersion
still runs for updated plugins.
- Around line 206-220: The rename handler for the button uses
pluginsStore.openServer and pluginsStore.servers but deletes the old entry
before copying it, so the plugin data is lost; to fix, read the old server
object into a temporary variable (e.g., const oldServer =
pluginsStore.servers[pluginsStore.openServer]) then assign
pluginsStore.servers[newName] = oldServer and only after that delete
pluginsStore.servers[pluginsStore.openServer] and update pluginsStore.openServer
= newName; update the onClick$ handler accordingly to perform the
copy-then-delete sequence and handle name-collision checks as already
implemented.
In `@src/routes/sitemap.xml/create-sitemap.ts`:
- Around line 11-17: The template string interpolates entries.map(...) directly,
which coerces the array to a comma-separated string and inserts commas between
<url> elements; update the sitemap generation to join the mapped fragments into
a single string (e.g., use entries.map(...).join('')) so the output contains
contiguous <url> blocks without commas—look for the entries.map(...)
interpolation that builds <loc> and <priority> and replace it with a joined
string.
In `@src/util/misc.ts`:
- Around line 3-7: The deepTrack function can recurse infinitely on cyclic
object graphs; modify deepTrack to use a WeakSet (e.g., visited) to track seen
objects and skip recursing into any object already in visited, and also guard
against non-object/null values before recursing; implement this by either adding
an internal helper that accepts the visited WeakSet or by making deepTrack
accept an optional visited parameter, and use unwrapStore(o) as before but only
recurse when o is an object, non-null, and not present in visited.
---
Outside diff comments:
In `@src/components/Rgbirdflop/PresetPreview.tsx`:
- Around line 142-173: The click handler currently sets isLoading.value and
mutates privatePresets.value/savedPresets.value optimistically but never clears
isLoading or rolls back on rejected savePreset/unsavePreset/setUserData; wrap
the async body of the onClick$ handler in try/finally to ensure isLoading.value
is cleared, and restructure to perform optimistic changes only after capturing
originals (e.g., const prevPrivate = [...privatePresets.value], const prevSaved
= [...savedPresets.value]) and then await the network call
(savePreset/unsavePreset/setUserData) — on failure restore privatePresets.value
= prevPrivate and savedPresets.value = prevSaved before rethrowing or handling
the error; also only update Preset.saves after a successful result and persist
to localStorage after success.
In `@src/components/Rgbirdflop/Presets.tsx`:
- Around line 131-148: The SelectList component currently renders an empty card
when both privatePresets.value and savedPresets.value are empty; update the JSX
around SelectList so it either returns the previous fallback copy (e.g., a small
message explaining there are no saved presets) or conditionally omits rendering
the SelectList entirely when privatePresets.value.concat(savedPresets.value) is
empty; locate the SelectList usage and the mapping over
privatePresets.value.concat(savedPresets.value.map(...)) and add a guard or
render a fallback node (using the same styling/context) before mapping, ensuring
functions/identifiers like SelectList, privatePresets, savedPresets,
renderPreview, and loadPresetJSON remain unchanged.
In `@src/routes/layout-profile.tsx`:
- Around line 55-66: The <h1> currently contains interactive content (the admin
Link and AppWindow icon) which is semantically incorrect for assistive tech;
move the conditional Link (the element rendered when isAdmin) out of the <h1>
and place it immediately after the heading so the <h1> remains text-only. Keep
the existing checks using session.value.user.image and session.value.user?.name
inside the <h1>, and retain the conditional rendering on isAdmin for the Link
and AppWindow component, only changing their placement to follow the <h1>.
In `@src/routes/resources/animtexture/index.tsx`:
- Around line 60-116: The task spawns a new RAF loop on every rerun (useTask$ /
deepTrack) and never cancels the previous one; add a scoped rafId (e.g., let
rafId: number | null) in the useTask$ body, call cancelAnimationFrame(rafId)
before starting a new requestAnimationFrame(animate), and return a cleanup
function from the useTask$ callback that calls cancelAnimationFrame(rafId) to
ensure any pending RAF from the previous run is cancelled; do this around the
existing animate/requestAnimationFrame usage referencing animate,
animtextureStore, animCanvasRef, and textureCanvasRef.
In `@src/routes/resources/rgb/presets/`[id]/index.tsx:
- Around line 195-214: The current click handler mutates signals
(privatePresets, savedPresets, localStorage) before awaiting
savePreset/unsavePreset and never resets isLoading on rejection; update the flow
in the onClick$ handler so that you first set isLoading.value = true, then call
and await savePreset(presetInfo.id) or unsavePreset(presetInfo.id) inside
try/catch, only mutate privatePresets.value and savedPresets.value (and write
localStorage) after the awaited call succeeds, and in a finally block set
isLoading.value = false; ensure you reference existingPreset, presetInfo,
savePreset, unsavePreset, privatePresets, savedPresets, localStorage and
isLoading exactly as in the diff so failures do not leave optimistic state or a
stuck loader.
In `@src/util/dataUtils.ts`:
- Around line 29-33: The code currently JSON.parses format/colors/shadowcolors
only when truthy, but then falls through to numeric coercion so empty strings
('') convert to 0; update the branching so that for the specific keys 'format',
'colors', and 'shadowcolors' you only call JSON.parse when params[key] is a
non-empty string (e.g., params[key] !== ''), and in the numeric conversion
branch (the check using Number(params[key]) / isNaN) guard against empty strings
as well (skip numeric coercion when params[key] === '') so '' is left as '' (or
null/undefined if you prefer) to preserve expected object/array shapes read
later from params.
---
Minor comments:
In `@src/components/analyze/Analyze.tsx`:
- Around line 62-65: In the Analyze component remove the duplicated example URL
span (the two identical <span
class="text-lum-text-secondary">https://birdflop.com/resources/analyze/[id]</span>)
— either delete one of them or replace the second with a distinct example (e.g.,
show a timings vs profile example such as /resources/analyze/timings/[id] or
/resources/analyze/profile/[id]) so the JSX no longer renders the same URL
twice; update the JSX in Analyze.tsx where those span elements are defined.
- Around line 36-38: The current branch that handles Spark links (the else if
checking link.startsWith('https://spark.lucko.me')) uses replace to extract code
but doesn't remove query strings or hash fragments; update the extraction so it
first gets the pathname segment (or split the replaced string on / then take the
last segment) and then strip any query/hash by splitting on '?' or '#' (or use
the URL constructor to read pathname) before assigning to code; reference the
variables/conditions in this block (link, code, the
startsWith('https://spark.lucko.me') branch) and ensure the final code contains
only the identifier without leading slashes, queries, or fragments.
In `@src/components/analyze/Redirect.tsx`:
- Line 5: The redirect uses params.id directly which can break URLs with special
characters; update the redirect call in Redirect.tsx to encode params.id before
building the path (use encodeURIComponent on params.id) so the redirect(301,
...) target becomes safe; locate the conditional that checks params.id and
replace the unencoded template segment with the encoded value while keeping the
same redirect function and behavior.
In `@src/components/Elements/SelectList.tsx`:
- Line 22: The ref for the select element is incorrectly typed as
HTMLInputElement; update the useSignal generic to HTMLSelectElement (change
selectRef = useSignal<HTMLInputElement>() to useSignal<HTMLSelectElement>()) and
adjust any usages of selectRef.current to use select-specific properties (e.g.,
selectedIndex, value) so TypeScript reflects the correct DOM element type in the
SelectList component.
In `@src/components/home/Testimonials.tsx`:
- Around line 146-149: The Anchor wrapper and the inner h3 both set
id="testimonials", producing duplicate IDs; remove the id from one of them
(preferably the inner <h3> or adjust the <Anchor> to only provide the id) so
only a single element uses id="testimonials". Update the JSX in the Testimonials
component (look for the Anchor component and the h3 element) to leave the anchor
target on one element and keep the other id-free to preserve valid DOM anchors
and accessibility.
In `@src/components/Rgbirdflop/MyPrivatePresets.tsx`:
- Line 255: The Save button currently renders duplicate text because it
concatenates a static "Publish" with the ternary using isSubmitting.value; in
the JSX for the Save element in MyPrivatePresets (where you have <Save size={20}
/> Publish {isSubmitting.value ? 'Validating...' : 'Publish'}), remove the
static "Publish" token and render only the ternary (keeping the <Save size={20}
/> icon and spacing) so the label becomes {isSubmitting.value ? 'Validating...'
: 'Publish'}.
In `@src/routes/index.tsx`:
- Around line 114-116: The page currently sets the same id on both the Anchor
wrapper and its inner h3 (e.g., id="plans" and id="features"), causing duplicate
IDs; update the JSX in src/routes/index.tsx so each id is unique by removing the
duplicate id from either the Anchor or the h3 (prefer removing the id on the
inner h3 and keep it on the Anchor, or vice versa) for the Anchor/h3 pairs that
reference "plans" and "features" (look for the Anchor component and the h3
elements with id="plans" and id="features") so hash navigation and accessibility
use a single unique element id.
In `@src/routes/layout.tsx`:
- Around line 214-219: The Background components are decorative and currently
expose unnecessary content to assistive tech; update the Background instances
(e.g., Background id="bg" and the other Background at 221-227) to be hidden from
the accessibility tree: if Background renders an <img> provide alt="" to make it
decorative, otherwise pass aria-hidden="true" (or ensure the component outputs
aria-hidden when used as decorative) so screen readers ignore these fixed
blurred backgrounds.
In `@src/routes/resources/plugins/index.tsx`:
- Line 12: The hardcoded debug flag (const debug = true) should be removed or
controlled via an environment variable; update the declaration of debug to read
from an env var (e.g., check process.env.NODE_ENV or a dedicated REACT_APP_DEBUG
/ VITE_DEBUG flag) so the debug button isn't shown in production, and ensure the
rest of the module that references debug (the "Mark all out of date" button
logic) uses this env-driven boolean; also add a safe default of false if the env
var is not set.
- Around line 109-112: Remove the debug console.log by deleting the
console.log(Date.now()) call inside the useTask$ callback where deepTrack(track,
pluginsStore) is invoked; the cleanup should leave the useTask$ callback only
performing deepTrack(track, pluginsStore) (no console logging) so pluginsStore
changes no longer print timestamps.
- Around line 249-258: The import input's onInput$ handler for the element with
id "import" currently swallows JSON parse errors and doesn't validate the parsed
value before mutating pluginsStore.servers[pluginsStore.openServer]; update the
handler to (1) validate pluginsStore.openServer exists, (2) try/catch JSON.parse
and show user-facing error feedback (e.g., set a local error state or dispatch a
toast) instead of only console.error, (3) validate the parsed object is an array
and each item conforms to PluginWithData shape (check required fields/types)
before pushing, and (4) only push the validated items into
pluginsStore.servers[pluginsStore.openServer] using the existing symbols
pluginsStore and PluginWithData to locate and guard the mutation.
In `@src/routes/sitemap.xml/index.tsx`:
- Line 33: Remove the debug logging call console.log(loc) from the sitemap
generation path (the stray console.log call in src/routes/sitemap.xml/index.tsx)
so route paths are not emitted to server logs; either delete that line or
replace it with an appropriate logger at debug level if you need to keep the
information, ensuring the sitemap code no longer prints loc to stdout on each
request.
---
Nitpick comments:
In `@src/components/plugins/PluginCard.tsx`:
- Around line 65-67: Remove the redundant client check: inside the
useVisibleTask$ callback delete the isBrowser guard (the conditional using
isBrowser) because useVisibleTask$ only runs on the client; keep the existing
early returns for plugin.data and plugin.id (references: useVisibleTask$,
isBrowser, plugin.data, plugin.id) so the task still skips when plugin data
exists or id is missing and ensure no other logic depends on isBrowser elsewhere
in this function.
In `@src/global.css`:
- Around line 145-147: Update the hr rule to explicitly reset browser defaults
and enforce a top border by adding the utility classes "border-0" and "border-t"
to the existing selector (hr { `@apply` my-6 border-lum-border/10; }) so the style
uses a consistent top border across browsers; keep the existing spacing and
color class but include the explicit border reset and top-border utilities.
In `@src/routes/resources/rgb/presets/index.tsx`:
- Around line 371-378: The heading and CTA were forced into a single row;
restore mobile stacking by making the h1 use a column layout on small screens
and switch to row on larger screens (e.g., add a responsive utility like
"flex-col md:flex-row" to the h1 with the current "flex" class), keeping
existing gap/centering classes; ensure the inner span and the <a> (lum-btn)
still align by leaving the span's "flex-1" and other classes intact so the CTA
flows below the title on narrow viewports and returns to a row on medium+
screens.
In `@src/routes/sitemap.xml/index.tsx`:
- Around line 32-37: The priority selection currently iterates
Object.keys(priorities) and assigns the priority of the last matching key
(variables: priorities, priorityRoute, loc), which depends on object key order;
change this to match the most specific route first by sorting the priority keys
by specificity (for example by descending string length) or by an explicit
specificity comparator, then iterate the sorted keys and break on the first
loc.startsWith(match) to set priority; ensure you use priorities[matchingKey as
keyof typeof priorities] and stop after the first match so the most specific
route wins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a60811fa-08a3-438b-8a3c-82685f6907fd
📒 Files selected for processing (55)
public/robots.txtpublic/sitemap.txtsrc/components/Elements/Footer.tsxsrc/components/Elements/Nav.tsxsrc/components/Elements/SelectList.tsxsrc/components/Rgbirdflop/Input.tsxsrc/components/Rgbirdflop/MyPrivatePresets.tsxsrc/components/Rgbirdflop/PresetPreview.tsxsrc/components/Rgbirdflop/Presets.tsxsrc/components/Rgbirdflop/RGBirdflop.tsxsrc/components/Rgbirdflop/UsersPublicPresets.tsxsrc/components/analyze/Analyze.tsxsrc/components/analyze/PaperTimings.tsxsrc/components/analyze/Redirect.tsxsrc/components/docs/Contributors.tsxsrc/components/docs/SideBar.tsxsrc/components/home/Testimonials.tsxsrc/components/plugins/AddMiscDialog.tsxsrc/components/plugins/AddSpigotDialog.tsxsrc/components/plugins/PluginCard.tsxsrc/global.csssrc/routes/[...404]/index.tsxsrc/routes/acornmc/vote/index.tsxsrc/routes/admin/index@profile.tsxsrc/routes/docs/[...404]/index.tsxsrc/routes/docs/layout.tsxsrc/routes/index.tsxsrc/routes/layout-markdown.tsxsrc/routes/layout-profile.tsxsrc/routes/layout.tsxsrc/routes/node-stats/index.tsxsrc/routes/plans/index.tsxsrc/routes/profile/[id]/index.tsxsrc/routes/resources/analyze/[id]/index.tsxsrc/routes/resources/analyze/index.tsxsrc/routes/resources/animpreview/index.tsxsrc/routes/resources/animtab/index.tsxsrc/routes/resources/animtexture/index.tsxsrc/routes/resources/banner/index.tsxsrc/routes/resources/flags/index.tsxsrc/routes/resources/index.tsxsrc/routes/resources/papertimings/[id]/index.tsxsrc/routes/resources/papertimings/index.tsxsrc/routes/resources/plugins/index.tsxsrc/routes/resources/rgb/index.tsxsrc/routes/resources/rgb/presets/[id]/index.tsxsrc/routes/resources/rgb/presets/index.tsxsrc/routes/resources/sparkprofile/[id]/index.tsxsrc/routes/resources/sparkprofile/index.tsxsrc/routes/settings/index.tsxsrc/routes/sitemap.xml/create-sitemap.tssrc/routes/sitemap.xml/index.tsxsrc/util/dataUtils.tssrc/util/misc.tssrc/util/themeUtil.ts
💤 Files with no reviewable changes (3)
- public/sitemap.txt
- src/routes/[...404]/index.tsx
- src/components/analyze/PaperTimings.tsx
Comment on lines
+139
to
141
| // pick random 6 testimonials to show | ||
| const sixTestimonials = testimonials.sort(() => 0.5 - Math.random()).slice(0, 6); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify in-place mutation and scope in this file
rg -nP '^\s*const\s+testimonials\s*=\s*\[' src/components/home/Testimonials.tsx
rg -nP '\btestimonials\.sort\s*\(' src/components/home/Testimonials.tsxRepository: birdflop/web
Length of output: 168
Avoid mutating testimonials during render.
Line 140 mutates the module-scoped testimonials array via in-place .sort(). In SSR, this creates shared-state nondeterminism across renders/requests. Additionally, the random comparator 0.5 - Math.random() does not produce a uniform shuffle.
Implement Fisher-Yates shuffle on a copy instead:
Suggested fix
- // pick random 6 testimonials to show
- const sixTestimonials = testimonials.sort(() => 0.5 - Math.random()).slice(0, 6);
+ // pick random 6 testimonials to show (without mutating module state)
+ const shuffled = [...testimonials];
+ for (let i = shuffled.length - 1; i > 0; i--) {
+ const j = Math.floor(Math.random() * (i + 1));
+ [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]];
+ }
+ const sixTestimonials = shuffled.slice(0, 6);📝 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.
Suggested change
| // pick random 6 testimonials to show | |
| const sixTestimonials = testimonials.sort(() => 0.5 - Math.random()).slice(0, 6); | |
| // pick random 6 testimonials to show (without mutating module state) | |
| const shuffled = [...testimonials]; | |
| for (let i = shuffled.length - 1; i > 0; i--) { | |
| const j = Math.floor(Math.random() * (i + 1)); | |
| [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]; | |
| } | |
| const sixTestimonials = shuffled.slice(0, 6); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/home/Testimonials.tsx` around lines 139 - 141, The current
code mutates the module-scoped testimonials array by calling
testimonials.sort(...) during render and uses a biased random comparator; fix by
making a shallow copy and applying an in-place Fisher‑Yates shuffle to that copy
before slicing: create e.g. const arr = [...testimonials]; implement a
FisherYatesShuffle(arr) that swaps elements using Math.random(), call
FisherYatesShuffle(arr) and then const sixTestimonials = arr.slice(0, 6); ensure
you remove the .sort(...) usage so the original testimonials array remains
untouched (refer to the sixTestimonials variable and the testimonials array in
Testimonials.tsx).
Comment on lines
+27
to
+33
| onInput$={(e, el) => { | ||
| const value = el.value; | ||
| resolvedPlugin.plugin = { | ||
| ...resolvedPlugin.plugin, | ||
| type: 'misc', | ||
| name: resolvedPlugin.plugin?.name ?? value.split('/').pop(), | ||
| url: value, |
Contributor
There was a problem hiding this comment.
Don't let misc plugins resolve to a blank name.
This fallback stores '' for inputs like https://example.com/plugin/, and ?? keeps a previously cleared name instead of re-deriving it. The add flow later persists resolvedPlugin.plugin.name directly, so blank names can make it into the saved plugin list and collide as keys.
🛠️ Suggested fix
<input type="text" class="lum-input" placeholder="https://example.com/plugin" id="plugin-link"
onInput$={(e, el) => {
const value = el.value;
+ const currentName = resolvedPlugin.plugin?.name?.trim();
+ let derivedName = '';
+ try {
+ derivedName = new URL(value).pathname.split('/').filter(Boolean).pop() ?? '';
+ } catch {
+ derivedName = value.split('/').filter(Boolean).pop() ?? '';
+ }
resolvedPlugin.plugin = {
...resolvedPlugin.plugin,
type: 'misc',
- name: resolvedPlugin.plugin?.name ?? value.split('/').pop(),
+ name: currentName || derivedName,
url: value,
};
}}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddMiscDialog.tsx` around lines 27 - 33, Compute a
non-empty derived name from the input instead of using value.split('/').pop()
with the nullish coalescing that preserves a previous name; in the onInput$
handler for resolvedPlugin.plugin derive the name by splitting value on '/',
filtering out empty segments, taking the last segment, and if that result is
empty or value is empty fallback to a safe default (e.g. 'misc-plugin'); assign
that derived non-empty name to resolvedPlugin.plugin.name and avoid using ?? to
preserve a cleared name.
Comment on lines
+37
to
+68
| const res = await fetch(`https://api.spiget.org/v2/resources/${pluginId}`); | ||
| const data = await res.json() as any; | ||
|
|
||
| const versionsRes = await fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?size=100&sort=-releaseDate`); | ||
| const versionsData = await versionsRes.json() as any; | ||
|
|
||
| const newPlugin: PluginWithData = { | ||
| id: data.id, | ||
| name: data.name, | ||
| type: 'spigot', | ||
| data: { | ||
| external: data.external, | ||
| name: data.name, | ||
| tag: data.tag, | ||
| iconUrl: data.icon?.url, | ||
| releaseDate: data.releaseDate, | ||
| updateDate: data.updateDate, | ||
| file: data.file ? { | ||
| type: data.file.type, | ||
| size: data.file.size, | ||
| sizeUnit: data.file.sizeUnit, | ||
| url: data.file.url, | ||
| externalUrl: data.file.externalUrl, | ||
| } : undefined, | ||
| testedVersions: data.testedVersions?.length | ||
| ? data.testedVersions : undefined, | ||
| sourceCodeLink: data.sourceCodeLink, | ||
| versions: versionsData.map((version: any) => version), | ||
| }, | ||
| }; | ||
|
|
||
| resolvedPlugin.plugin = newPlugin; |
Contributor
There was a problem hiding this comment.
Missing error handling for API calls.
The fetch calls to Spiget API lack try-catch blocks. Network failures or API errors will result in unhandled promise rejections, potentially leaving the UI in an inconsistent state.
🔧 Proposed fix
+ try {
const res = await fetch(`https://api.spiget.org/v2/resources/${pluginId}`);
+ if (!res.ok) throw new Error(`HTTP ${res.status}`);
const data = await res.json() as any;
const versionsRes = await fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?size=100&sort=-releaseDate`);
+ if (!versionsRes.ok) throw new Error(`HTTP ${versionsRes.status}`);
const versionsData = await versionsRes.json() as any;
// ... rest of the code
+ } catch (err) {
+ const notification = new Notification()
+ .setTitle('Error fetching plugin')
+ .setDescription(`Failed to fetch plugin data: ${err}`)
+ .setBgColor('lum-bg-red/50');
+ notifications.push(notification);
+ }📝 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.
Suggested change
| const res = await fetch(`https://api.spiget.org/v2/resources/${pluginId}`); | |
| const data = await res.json() as any; | |
| const versionsRes = await fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?size=100&sort=-releaseDate`); | |
| const versionsData = await versionsRes.json() as any; | |
| const newPlugin: PluginWithData = { | |
| id: data.id, | |
| name: data.name, | |
| type: 'spigot', | |
| data: { | |
| external: data.external, | |
| name: data.name, | |
| tag: data.tag, | |
| iconUrl: data.icon?.url, | |
| releaseDate: data.releaseDate, | |
| updateDate: data.updateDate, | |
| file: data.file ? { | |
| type: data.file.type, | |
| size: data.file.size, | |
| sizeUnit: data.file.sizeUnit, | |
| url: data.file.url, | |
| externalUrl: data.file.externalUrl, | |
| } : undefined, | |
| testedVersions: data.testedVersions?.length | |
| ? data.testedVersions : undefined, | |
| sourceCodeLink: data.sourceCodeLink, | |
| versions: versionsData.map((version: any) => version), | |
| }, | |
| }; | |
| resolvedPlugin.plugin = newPlugin; | |
| try { | |
| const res = await fetch(`https://api.spiget.org/v2/resources/${pluginId}`); | |
| if (!res.ok) throw new Error(`HTTP ${res.status}`); | |
| const data = await res.json() as any; | |
| const versionsRes = await fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?size=100&sort=-releaseDate`); | |
| if (!versionsRes.ok) throw new Error(`HTTP ${versionsRes.status}`); | |
| const versionsData = await versionsRes.json() as any; | |
| const newPlugin: PluginWithData = { | |
| id: data.id, | |
| name: data.name, | |
| type: 'spigot', | |
| data: { | |
| external: data.external, | |
| name: data.name, | |
| tag: data.tag, | |
| iconUrl: data.icon?.url, | |
| releaseDate: data.releaseDate, | |
| updateDate: data.updateDate, | |
| file: data.file ? { | |
| type: data.file.type, | |
| size: data.file.size, | |
| sizeUnit: data.file.sizeUnit, | |
| url: data.file.url, | |
| externalUrl: data.file.externalUrl, | |
| } : undefined, | |
| testedVersions: data.testedVersions?.length | |
| ? data.testedVersions : undefined, | |
| sourceCodeLink: data.sourceCodeLink, | |
| versions: versionsData.map((version: any) => version), | |
| }, | |
| }; | |
| resolvedPlugin.plugin = newPlugin; | |
| } catch (err) { | |
| const notification = new Notification() | |
| .setTitle('Error fetching plugin') | |
| .setDescription(`Failed to fetch plugin data: ${err}`) | |
| .setBgColor('lum-bg-red/50'); | |
| notifications.push(notification); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddSpigotDialog.tsx` around lines 37 - 68, Wrap the
Spiget API calls and JSON parsing (the fetch for
`https://api.spiget.org/v2/resources/${pluginId}` and the fetch for
`/versions?size=100...`) in a try-catch, check each response's
`res.ok`/`versionsRes.ok` and throw or handle non-OK responses before calling
`res.json()`/`versionsRes.json()`, and on error set an appropriate failure state
on `resolvedPlugin` (or return early) instead of proceeding to construct
`newPlugin` and assigning `resolvedPlugin.plugin = newPlugin`; ensure errors
from JSON parsing or network failures are caught and a clear error message is
propagated via the same `resolvedPlugin` mechanism you use for success.
Comment on lines
+70
to
+112
| onChange$={async (e, el) => { | ||
| const value = el.value; | ||
| const spigotMatch = value.match(/spigotmc\.org\/resources\/(.+)\.(\d+)/); | ||
| if (spigotMatch) return; | ||
|
|
||
| console.log('Searching for plugin:', value); | ||
|
|
||
| const searchRes = await fetch(`https://api.spiget.org/v2/search/resources/${encodeURIComponent(value)}?size=5`); | ||
| const searchData: any[] = await searchRes.json(); | ||
|
|
||
| if (searchData.length === 0) { | ||
| const notification = new Notification() | ||
| .setTitle('No results found') | ||
| .setDescription(`No plugins found matching "${value}". Please try searching by plugin name or pasting the plugin link.`) | ||
| .setBgColor('lum-bg-yellow/50'); | ||
| notifications.push(notification); | ||
| return; | ||
| } | ||
|
|
||
| resolvedPlugin.plugins = searchData.map((result: any) => ({ | ||
| id: result.id, | ||
| name: result.name, | ||
| type: 'spigot', | ||
| data: { | ||
| external: result.external, | ||
| name: result.name, | ||
| tag: result.tag, | ||
| iconUrl: result.icon?.url, | ||
| releaseDate: result.releaseDate, | ||
| updateDate: result.updateDate, | ||
| file: result.file ? { | ||
| type: result.file.type, | ||
| size: result.file.size, | ||
| sizeUnit: result.file.sizeUnit, | ||
| url: result.file.url, | ||
| externalUrl: result.file.externalUrl, | ||
| } : undefined, | ||
| testedVersions: result.testedVersions?.length | ||
| ? result.testedVersions : undefined, | ||
| sourceCodeLink: result.sourceCodeLink, | ||
| }, | ||
| })); | ||
| }} |
Contributor
There was a problem hiding this comment.
Search handler also lacks error handling.
The search fetch call has the same issue - no try-catch to handle network failures. Additionally, the response is cast directly to any[] without checking if the response was successful or if the JSON structure is as expected.
🔧 Proposed fix
onChange$={async (e, el) => {
const value = el.value;
const spigotMatch = value.match(/spigotmc\.org\/resources\/(.+)\.(\d+)/);
if (spigotMatch) return;
console.log('Searching for plugin:', value);
+ try {
const searchRes = await fetch(`https://api.spiget.org/v2/search/resources/${encodeURIComponent(value)}?size=5`);
+ if (!searchRes.ok) throw new Error(`HTTP ${searchRes.status}`);
const searchData: any[] = await searchRes.json();
+ if (!Array.isArray(searchData)) throw new Error('Unexpected response format');
// ... rest of the code
+ } catch (err) {
+ const notification = new Notification()
+ .setTitle('Search failed')
+ .setDescription(`Failed to search plugins: ${err}`)
+ .setBgColor('lum-bg-red/50');
+ notifications.push(notification);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddSpigotDialog.tsx` around lines 70 - 112, The
onChange$ handler's network call needs robust error handling: wrap the fetch to
https://api.spiget.org/v2/search/resources/... and the subsequent
response.json() in a try-catch inside the onChange$ callback, verify response.ok
before parsing, validate that the parsed JSON is an array before mapping into
resolvedPlugin.plugins, and on any failure push a descriptive Notification
(e.g., "Search failed" with error message or "Unexpected response format")
instead of assuming success; update the code paths around onChange$, fetch,
response.json(), and resolvedPlugin.plugins to implement these checks and error
notifications.
Comment on lines
+135
to
+146
| } onChange$={async (e, el) => { | ||
| const pluginId = Number(el.value); | ||
| const selectedPlugin = resolvedPlugin.plugins?.find((plugin) => plugin.id === pluginId); | ||
| if (!selectedPlugin || !selectedPlugin.data) return; | ||
|
|
||
| const versionsRes = await fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?size=100&sort=-releaseDate`); | ||
| const versionsData = await versionsRes.json() as any; | ||
| selectedPlugin.data.versions = versionsData.map((version: any) => version); | ||
|
|
||
| resolvedPlugin.plugin = selectedPlugin; | ||
| resolvedPlugin.plugins = undefined; | ||
| }}/> |
Contributor
There was a problem hiding this comment.
Version fetch also needs error handling.
The fetch call on line 140-141 follows the same pattern without error handling. Consider wrapping all Spiget API calls in try-catch blocks consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddSpigotDialog.tsx` around lines 135 - 146, Wrap the
Spiget API call inside the onChange$ handler in a try-catch and guard against
non-OK responses: when handling the selectedPlugin in the onChange$ function,
perform the
fetch(`https://api.spiget.org/v2/resources/${pluginId}/versions?...`) and await
response.json() inside a try block, check response.ok before parsing, and only
assign selectedPlugin.data.versions and update resolvedPlugin.plugin when the
request succeeds; in the catch block (or on non-ok) log or surface the error
(e.g., via console.error or an existing logger) and avoid mutating
resolvedPlugin so the UI remains consistent.
Comment on lines
57
to
60
| useTask$(({ track }) => { | ||
| if (isBrowser) setCookies('animtab', { version: rgbStore.version, ...animtabStore }); | ||
| (Object.keys(animtabStore) as Array<keyof typeof animtabStore>).forEach((key) => { | ||
| track(() => animtabStore[key]); | ||
| }); | ||
| deepTrack(track, rgbStore); | ||
| }); |
Contributor
There was a problem hiding this comment.
Track animtabStore in the cookie writer.
This task serializes animtabStore into the animtab cookie, but it only subscribes to rgbStore. Changes like speed, type, length, or output format won't persist until some RGB field also changes.
💡 Minimal fix
useTask$(({ track }) => {
+ deepTrack(track, animtabStore);
if (isBrowser) setCookies('animtab', { version: rgbStore.version, ...animtabStore });
deepTrack(track, rgbStore);
});📝 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.
Suggested change
| useTask$(({ track }) => { | |
| if (isBrowser) setCookies('animtab', { version: rgbStore.version, ...animtabStore }); | |
| (Object.keys(animtabStore) as Array<keyof typeof animtabStore>).forEach((key) => { | |
| track(() => animtabStore[key]); | |
| }); | |
| deepTrack(track, rgbStore); | |
| }); | |
| useTask$(({ track }) => { | |
| deepTrack(track, animtabStore); | |
| if (isBrowser) setCookies('animtab', { version: rgbStore.version, ...animtabStore }); | |
| deepTrack(track, rgbStore); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/animtab/index.tsx` around lines 57 - 60, The useTask$
currently only subscribes to rgbStore via deepTrack(track, rgbStore), so changes
in animtabStore (speed/type/length/output) aren't tracked; update the task to
also deepTrack(track, animtabStore) (or otherwise include animtabStore in the
deepTrack call) so the cookie writer reacts to animtabStore changes, leaving the
setCookies('animtab', { version: rgbStore.version, ...animtabStore }) call
as-is.
Comment on lines
+206
to
+220
| <button class="lum-btn lum-btn-p-1 lum-bg-transparent rounded-lum-1" onClick$={() => { | ||
| if (!pluginsStore.openServer) return; | ||
| const newName = prompt('Enter new server name', `${pluginsStore.openServer}`); | ||
| if (newName && newName !== pluginsStore.openServer) { | ||
| if (pluginsStore.servers[newName]) { | ||
| alert('A server with that name already exists.'); | ||
| return; | ||
| } | ||
| delete pluginsStore.servers[pluginsStore.openServer]; | ||
| pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer]; | ||
| pluginsStore.openServer = newName; | ||
| } | ||
| }} title="Rename server"> | ||
| <Pencil size={16} /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Server rename logic loses all plugin data.
Line 214 deletes the old server entry before line 215 tries to copy it to the new name. This results in undefined being assigned to the new server, losing all plugins.
🐛 Proposed fix
if (newName && newName !== pluginsStore.openServer) {
if (pluginsStore.servers[newName]) {
alert('A server with that name already exists.');
return;
}
- delete pluginsStore.servers[pluginsStore.openServer];
- pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer];
+ pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer];
+ delete pluginsStore.servers[pluginsStore.openServer];
pluginsStore.openServer = newName;
}📝 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.
Suggested change
| <button class="lum-btn lum-btn-p-1 lum-bg-transparent rounded-lum-1" onClick$={() => { | |
| if (!pluginsStore.openServer) return; | |
| const newName = prompt('Enter new server name', `${pluginsStore.openServer}`); | |
| if (newName && newName !== pluginsStore.openServer) { | |
| if (pluginsStore.servers[newName]) { | |
| alert('A server with that name already exists.'); | |
| return; | |
| } | |
| delete pluginsStore.servers[pluginsStore.openServer]; | |
| pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer]; | |
| pluginsStore.openServer = newName; | |
| } | |
| }} title="Rename server"> | |
| <Pencil size={16} /> | |
| </button> | |
| <button class="lum-btn lum-btn-p-1 lum-bg-transparent rounded-lum-1" onClick$={() => { | |
| if (!pluginsStore.openServer) return; | |
| const newName = prompt('Enter new server name', `${pluginsStore.openServer}`); | |
| if (newName && newName !== pluginsStore.openServer) { | |
| if (pluginsStore.servers[newName]) { | |
| alert('A server with that name already exists.'); | |
| return; | |
| } | |
| pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer]; | |
| delete pluginsStore.servers[pluginsStore.openServer]; | |
| pluginsStore.openServer = newName; | |
| } | |
| }} title="Rename server"> | |
| <Pencil size={16} /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 206 - 220, The rename
handler for the button uses pluginsStore.openServer and pluginsStore.servers but
deletes the old entry before copying it, so the plugin data is lost; to fix,
read the old server object into a temporary variable (e.g., const oldServer =
pluginsStore.servers[pluginsStore.openServer]) then assign
pluginsStore.servers[newName] = oldServer and only after that delete
pluginsStore.servers[pluginsStore.openServer] and update pluginsStore.openServer
= newName; update the onClick$ handler accordingly to perform the
copy-then-delete sequence and handle name-collision checks as already
implemented.
Comment on lines
+298
to
+309
| for (const plugin of plugins) { | ||
| const updateAvailable = plugin.data?.latestVersion?.releaseDate !== undefined | ||
| && plugin.version?.releaseDate !== undefined | ||
| && plugin.data.latestVersion.releaseDate > plugin.version.releaseDate; | ||
|
|
||
| if (!updateAvailable || !plugin.data?.file?.url) return; | ||
|
|
||
| if (plugin.type === 'spigot') await downloadSpigotPlugin(plugin, spigotRateLimit); | ||
| else window.open(plugin.data.file.url, '_blank'); | ||
|
|
||
| plugin.version = plugin.data?.latestVersion; | ||
| } |
Contributor
There was a problem hiding this comment.
return exits entire loop prematurely.
Line 303 uses return instead of continue, which exits the for...of loop and the entire click handler on the first plugin that doesn't have an update available. This means only plugins before the first up-to-date plugin will be processed.
🐛 Proposed fix
for (const plugin of plugins) {
const updateAvailable = plugin.data?.latestVersion?.releaseDate !== undefined
&& plugin.version?.releaseDate !== undefined
&& plugin.data.latestVersion.releaseDate > plugin.version.releaseDate;
- if (!updateAvailable || !plugin.data?.file?.url) return;
+ if (!updateAvailable || !plugin.data?.file?.url) continue;
if (plugin.type === 'spigot') await downloadSpigotPlugin(plugin, spigotRateLimit);
else window.open(plugin.data.file.url, '_blank');
plugin.version = plugin.data?.latestVersion;
}📝 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.
Suggested change
| for (const plugin of plugins) { | |
| const updateAvailable = plugin.data?.latestVersion?.releaseDate !== undefined | |
| && plugin.version?.releaseDate !== undefined | |
| && plugin.data.latestVersion.releaseDate > plugin.version.releaseDate; | |
| if (!updateAvailable || !plugin.data?.file?.url) return; | |
| if (plugin.type === 'spigot') await downloadSpigotPlugin(plugin, spigotRateLimit); | |
| else window.open(plugin.data.file.url, '_blank'); | |
| plugin.version = plugin.data?.latestVersion; | |
| } | |
| for (const plugin of plugins) { | |
| const updateAvailable = plugin.data?.latestVersion?.releaseDate !== undefined | |
| && plugin.version?.releaseDate !== undefined | |
| && plugin.data.latestVersion.releaseDate > plugin.version.releaseDate; | |
| if (!updateAvailable || !plugin.data?.file?.url) continue; | |
| if (plugin.type === 'spigot') await downloadSpigotPlugin(plugin, spigotRateLimit); | |
| else window.open(plugin.data.file.url, '_blank'); | |
| plugin.version = plugin.data?.latestVersion; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 298 - 309, The loop
prematurely exits because the click handler uses return inside the for...of over
plugins; replace that return with continue so non-updatable plugins are skipped
but the loop continues processing remaining items. Update the block around the
for (const plugin of plugins) loop, keeping the existing checks (updateAvailable
and plugin.data?.file?.url), keeping the spigot branch that calls
downloadSpigotPlugin(plugin, spigotRateLimit) and the window.open fallback, and
ensure plugin.version = plugin.data?.latestVersion still runs for updated
plugins.
Comment on lines
+11
to
+17
| ${entries.map( | ||
| (entry) => ` | ||
| <url> | ||
| <loc>${baseUrl}${entry.loc.startsWith('/') ? '' : '/'}${entry.loc}</loc> | ||
| <priority>${entry.priority}</priority> | ||
| </url>`, | ||
| )} |
Contributor
There was a problem hiding this comment.
Array.map() produces comma-separated output in template literals.
When entries.map(...) is interpolated directly into a template string, JavaScript coerces the array to a string using commas as delimiters. This results in invalid XML with commas between <url> elements.
🐛 Proposed fix to join array elements without commas
${entries.map(
(entry) => `
<url>
<loc>${baseUrl}${entry.loc.startsWith('/') ? '' : '/'}${entry.loc}</loc>
<priority>${entry.priority}</priority>
</url>`,
- )}
+ ).join('')}
</urlset>`.trim();📝 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.
Suggested change
| ${entries.map( | |
| (entry) => ` | |
| <url> | |
| <loc>${baseUrl}${entry.loc.startsWith('/') ? '' : '/'}${entry.loc}</loc> | |
| <priority>${entry.priority}</priority> | |
| </url>`, | |
| )} | |
| ${entries.map( | |
| (entry) => ` | |
| <url> | |
| <loc>${baseUrl}${entry.loc.startsWith('/') ? '' : '/'}${entry.loc}</loc> | |
| <priority>${entry.priority}</priority> | |
| </url>`, | |
| ).join('')} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/sitemap.xml/create-sitemap.ts` around lines 11 - 17, The template
string interpolates entries.map(...) directly, which coerces the array to a
comma-separated string and inserts commas between <url> elements; update the
sitemap generation to join the mapped fragments into a single string (e.g., use
entries.map(...).join('')) so the output contains contiguous <url> blocks
without commas—look for the entries.map(...) interpolation that builds <loc> and
<priority> and replace it with a joined string.
Comment on lines
+3
to
+7
| export const deepTrack = (track: any, obj: any) => { | ||
| track(obj); | ||
| for (const o of Object.values(obj)) { | ||
| if (unwrapStore(o) !== o) deepTrack(track, o); | ||
| } |
Contributor
There was a problem hiding this comment.
Add cycle protection to recursive traversal.
Current recursion can blow the stack on cyclic references in tracked store graphs.
🔧 Proposed fix
export const deepTrack = (track: any, obj: any) => {
- track(obj);
- for (const o of Object.values(obj)) {
- if (unwrapStore(o) !== o) deepTrack(track, o);
- }
+ const walk = (value: any, visited: WeakSet<object>) => {
+ if (!value || typeof value !== 'object') return;
+ if (visited.has(value)) return;
+ visited.add(value);
+
+ track(value);
+ for (const child of Object.values(value)) {
+ if (child && typeof child === 'object' && unwrapStore(child) !== child) {
+ walk(child, visited);
+ }
+ }
+ };
+
+ walk(obj, new WeakSet<object>());
};📝 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.
Suggested change
| export const deepTrack = (track: any, obj: any) => { | |
| track(obj); | |
| for (const o of Object.values(obj)) { | |
| if (unwrapStore(o) !== o) deepTrack(track, o); | |
| } | |
| export const deepTrack = (track: any, obj: any) => { | |
| const walk = (value: any, visited: WeakSet<object>) => { | |
| if (!value || typeof value !== 'object') return; | |
| if (visited.has(value)) return; | |
| visited.add(value); | |
| track(value); | |
| for (const child of Object.values(value)) { | |
| if (child && typeof child === 'object' && unwrapStore(child) !== child) { | |
| walk(child, visited); | |
| } | |
| } | |
| }; | |
| walk(obj, new WeakSet<object>()); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/misc.ts` around lines 3 - 7, The deepTrack function can recurse
infinitely on cyclic object graphs; modify deepTrack to use a WeakSet (e.g.,
visited) to track seen objects and skip recursing into any object already in
visited, and also guard against non-object/null values before recursing;
implement this by either adding an internal helper that accepts the visited
WeakSet or by making deepTrack accept an optional visited parameter, and use
unwrapStore(o) as before but only recurse when o is an object, non-null, and not
present in visited.
… titles and headings
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/routes/resources/plugins/index.tsx (2)
303-303:⚠️ Potential issue | 🔴 Critical
returnexits loop prematurely.Using
returnhere exits the entire click handler on the first plugin that doesn't need an update. Change tocontinueto skip that plugin and process remaining ones.🐛 Proposed fix
- if (!updateAvailable || !plugin.data?.file?.url) return; + if (!updateAvailable || !plugin.data?.file?.url) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` at line 303, The click handler loop currently uses "if (!updateAvailable || !plugin.data?.file?.url) return;" which exits the entire handler on the first non-updatable plugin; replace that return with "continue" so the loop skips that plugin and continues processing the remaining plugins (locate the loop in the click handler where the conditional references updateAvailable and plugin.data?.file?.url and change the control flow from return to continue).
214-215:⚠️ Potential issue | 🔴 CriticalServer rename logic loses all plugin data.
The delete on line 214 executes before the copy on line 215, causing
pluginsStore.servers[pluginsStore.openServer]to beundefinedwhen assigned to the new name. Swap the order: copy first, then delete.🐛 Proposed fix
if (pluginsStore.servers[newName]) { alert('A server with that name already exists.'); return; } - delete pluginsStore.servers[pluginsStore.openServer]; - pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer]; + pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer]; + delete pluginsStore.servers[pluginsStore.openServer]; pluginsStore.openServer = newName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 214 - 215, The rename currently deletes pluginsStore.servers[pluginsStore.openServer] before copying it, so the assignment to pluginsStore.servers[newName] gets undefined; in the rename flow (use symbols pluginsStore.servers, pluginsStore.openServer, and newName) first assign pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer] and only after that delete pluginsStore.servers[pluginsStore.openServer] to preserve the plugin data.
🧹 Nitpick comments (2)
src/routes/resources/plugins/index.tsx (2)
340-342: Use unique key and explicit filter.
plugin.namemay not be unique across sources. Also, returningundefinedfrom.map()is less idiomatic than filtering first.♻️ Suggested improvement
- {pluginsStore.servers[pluginsStore.openServer].map((plugin) => { + {pluginsStore.servers[pluginsStore.openServer] + .filter((plugin) => { const updateAvailable = plugin.data?.latestVersion?.releaseDate !== undefined && plugin.version?.releaseDate !== undefined && plugin.data.latestVersion.releaseDate > plugin.version.releaseDate; - - if ((pluginsStore.showOnlyOutdated && !updateAvailable)) return; - - return <PluginCard key={plugin.name} + return !(pluginsStore.showOnlyOutdated && !updateAvailable); + }) + .map((plugin) => { + const updateAvailable = ...; + return <PluginCard key={`${plugin.type}-${plugin.id}`} plugin={plugin}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 340 - 342, The map is returning undefined and using a non-unique key; first filter the plugins array for the outdated condition (use pluginsStore.showOnlyOutdated and updateAvailable) before mapping, and when rendering PluginCard use a stable unique key (prefer plugin.id if available, otherwise compose one like `${plugin.source}-${plugin.name}`) instead of plugin.name; update the code around PluginCard, pluginsStore.showOnlyOutdated, and updateAvailable to perform filter(...).map(...) and set the key accordingly.
249-258: Import lacks structure validation.The JSON is parsed and spread directly without verifying it's an array or that entries have expected properties. Invalid shapes could corrupt
pluginsStore.servers.🛡️ Proposed validation
onInput$={(e, el) => { if (!pluginsStore.openServer) return; try { const importedPlugins = JSON.parse(el.value) as PluginWithData[]; + if (!Array.isArray(importedPlugins)) { + throw new Error('Imported data must be an array'); + } pluginsStore.servers[pluginsStore.openServer].push(...importedPlugins); + el.value = ''; } catch (err) { console.error('Failed to parse imported plugins:', err); + // Consider showing user notification here } }}/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 249 - 258, The onInput$ handler currently parses JSON and directly pushes to pluginsStore.servers[pluginsStore.openServer]; instead validate the parsed value (from JSON.parse in the input handler tied to PluginWithData) is an array via Array.isArray, then verify each item has the required shape (e.g., required fields on PluginWithData such as id/name/url or whichever properties your PluginWithData type requires) before mutating pluginsStore; if validation fails, log a clear error and do not push or mutate pluginsStore.servers. Ensure you reference the same symbols: the onInput$ handler, PluginWithData type, pluginsStore, pluginsStore.openServer, and pluginsStore.servers when implementing these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/resources/plugins/index.tsx`:
- Line 111: Remove the debug console logging that prints Date.now() on every
store change: delete the console.log(Date.now()) call (or guard it behind a
non-production check) so it no longer spams the console in production; if you
need retained debugging, replace it with a conditional debug logger (e.g., only
run when process.env.NODE_ENV !== 'production' or use an existing logger.debug)
instead of the plain console.log(Date.now()).
- Line 12: The debug flag (const debug) is left enabled which exposes the "Mark
all out of date" debug UI in production; replace the hard-coded debug boolean
with an environment-gated check or remove the debug UI entirely: either remove
the const debug and the debug-only button rendering for "Mark all out of date",
or change the debug definition to read an environment variable (e.g., NODE_ENV
or a specific REACT_APP/VITE_DEBUG flag) and update the conditional rendering
around the "Mark all out of date" button so it only renders when the env-driven
debug flag is true (refer to the debug constant and the "Mark all out of date"
button render block).
- Around line 39-40: Guard against undefined plugin file URLs before calling
window.open: check that plugin.data?.file?.url is a non-empty string and only
then call window.open(`https://www.spigotmc.org/${plugin.data.file.url}`,
'_blank'); if it's missing, handle gracefully (e.g., show a user-facing
message/notification or disable the download action). Update the click handler
or the function that triggers the download (the code that currently calls
window.open) to perform this validation and early-return when the URL is absent.
---
Duplicate comments:
In `@src/routes/resources/plugins/index.tsx`:
- Line 303: The click handler loop currently uses "if (!updateAvailable ||
!plugin.data?.file?.url) return;" which exits the entire handler on the first
non-updatable plugin; replace that return with "continue" so the loop skips that
plugin and continues processing the remaining plugins (locate the loop in the
click handler where the conditional references updateAvailable and
plugin.data?.file?.url and change the control flow from return to continue).
- Around line 214-215: The rename currently deletes
pluginsStore.servers[pluginsStore.openServer] before copying it, so the
assignment to pluginsStore.servers[newName] gets undefined; in the rename flow
(use symbols pluginsStore.servers, pluginsStore.openServer, and newName) first
assign pluginsStore.servers[newName] =
pluginsStore.servers[pluginsStore.openServer] and only after that delete
pluginsStore.servers[pluginsStore.openServer] to preserve the plugin data.
---
Nitpick comments:
In `@src/routes/resources/plugins/index.tsx`:
- Around line 340-342: The map is returning undefined and using a non-unique
key; first filter the plugins array for the outdated condition (use
pluginsStore.showOnlyOutdated and updateAvailable) before mapping, and when
rendering PluginCard use a stable unique key (prefer plugin.id if available,
otherwise compose one like `${plugin.source}-${plugin.name}`) instead of
plugin.name; update the code around PluginCard, pluginsStore.showOnlyOutdated,
and updateAvailable to perform filter(...).map(...) and set the key accordingly.
- Around line 249-258: The onInput$ handler currently parses JSON and directly
pushes to pluginsStore.servers[pluginsStore.openServer]; instead validate the
parsed value (from JSON.parse in the input handler tied to PluginWithData) is an
array via Array.isArray, then verify each item has the required shape (e.g.,
required fields on PluginWithData such as id/name/url or whichever properties
your PluginWithData type requires) before mutating pluginsStore; if validation
fails, log a clear error and do not push or mutate pluginsStore.servers. Ensure
you reference the same symbols: the onInput$ handler, PluginWithData type,
pluginsStore, pluginsStore.openServer, and pluginsStore.servers when
implementing these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b6f676c-1b72-4252-8d2e-a338e72fb592
📒 Files selected for processing (2)
src/routes/resources/index.tsxsrc/routes/resources/plugins/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/resources/index.tsx
| import AddMiscDialog from '~/components/plugins/AddMiscDialog'; | ||
| import { deepTrack } from '~/util/misc'; | ||
|
|
||
| const debug = true; |
Contributor
There was a problem hiding this comment.
Debug flag left enabled.
debug = true will expose the debug "Mark all out of date" button (lines 263-279) in production. Either remove this flag and the associated UI, or gate it behind an environment variable.
🐛 Proposed fix
-const debug = true;
+const debug = import.meta.env.DEV;📝 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.
Suggested change
| const debug = true; | |
| const debug = import.meta.env.DEV; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` at line 12, The debug flag (const
debug) is left enabled which exposes the "Mark all out of date" debug UI in
production; replace the hard-coded debug boolean with an environment-gated check
or remove the debug UI entirely: either remove the const debug and the
debug-only button rendering for "Mark all out of date", or change the debug
definition to read an environment variable (e.g., NODE_ENV or a specific
REACT_APP/VITE_DEBUG flag) and update the conditional rendering around the "Mark
all out of date" button so it only renders when the env-driven debug flag is
true (refer to the debug constant and the "Mark all out of date" button render
block).
Comment on lines
+39
to
+40
| // open the plugin file url in a new tab to trigger the download | ||
| window.open(`https://www.spigotmc.org/${plugin.data?.file?.url}`, '_blank'); |
Contributor
There was a problem hiding this comment.
Missing guard for undefined file URL.
If plugin.data?.file?.url is undefined, this opens https://www.spigotmc.org/undefined. Add a guard or ensure callers validate this before invoking.
🛡️ Proposed fix
+ if (!plugin.data?.file?.url) return;
+
// open the plugin file url in a new tab to trigger the download
window.open(`https://www.spigotmc.org/${plugin.data?.file?.url}`, '_blank');📝 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.
Suggested change
| // open the plugin file url in a new tab to trigger the download | |
| window.open(`https://www.spigotmc.org/${plugin.data?.file?.url}`, '_blank'); | |
| if (!plugin.data?.file?.url) return; | |
| // open the plugin file url in a new tab to trigger the download | |
| window.open(`https://www.spigotmc.org/${plugin.data?.file?.url}`, '_blank'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 39 - 40, Guard against
undefined plugin file URLs before calling window.open: check that
plugin.data?.file?.url is a non-empty string and only then call
window.open(`https://www.spigotmc.org/${plugin.data.file.url}`, '_blank'); if
it's missing, handle gracefully (e.g., show a user-facing message/notification
or disable the download action). Update the click handler or the function that
triggers the download (the code that currently calls window.open) to perform
this validation and early-return when the URL is absent.
|
|
||
| useTask$(({ track }) => { | ||
| deepTrack(track, pluginsStore); | ||
| console.log(Date.now()); |
Contributor
There was a problem hiding this comment.
Remove debug logging.
This console.log fires on every store change and will spam the console in production.
🧹 Proposed fix
useTask$(({ track }) => {
deepTrack(track, pluginsStore);
- console.log(Date.now());
if (!isBrowser) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` at line 111, Remove the debug console
logging that prints Date.now() on every store change: delete the
console.log(Date.now()) call (or guard it behind a non-production check) so it
no longer spams the console in production; if you need retained debugging,
replace it with a conditional debug logger (e.g., only run when
process.env.NODE_ENV !== 'production' or use an existing logger.debug) instead
of the plain console.log(Date.now()).
- Implemented AddModrinthDialog component for adding Modrinth plugins. - Modified PluginCard to accommodate Modrinth plugins and their respective actions. - Enhanced the plugin source selection to include Modrinth with appropriate descriptions. - Adjusted the resolvedPlugin context to default to Modrinth type.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
src/routes/resources/plugins/index.tsx (2)
299-312:⚠️ Potential issue | 🔴 CriticalUse
continueinstead ofreturninside the bulk loop.Line 304 exits the whole click handler on the first plugin that doesn't need an update. Later plugins are skipped, and
isLoadingnever reaches the cleanup at Line 312.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 299 - 312, The loop over plugins incorrectly uses return inside the for (const plugin of plugins) loop which aborts the entire click handler on the first plugin that doesn't need an update; change that return to continue so the loop skips to the next plugin but still allows subsequent plugins to be processed and the cleanup line (isLoading.value = isLoading.value.filter(...)) to run; update the block around the updateAvailable check in the function that contains plugins, downloadSpigotPlugin, and isLoading so plugin.version = plugin.data?.latestVersion only runs for plugins that were actually updated and control flow uses continue instead of return.
207-217:⚠️ Potential issue | 🔴 CriticalCopy the server entry before deleting it.
Lines 215-216 delete the old key before reading it, so renaming a server drops the entire plugin list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 207 - 217, When renaming a server in the onClick handler, the code deletes pluginsStore.servers[pluginsStore.openServer] before reading it which causes loss of the plugin list; fix by first copying the existing entry into a temporary variable (e.g., const oldEntry = pluginsStore.servers[pluginsStore.openServer]), then assign that oldEntry to pluginsStore.servers[newName], and only then delete the old key and update pluginsStore.openServer to newName (references: pluginsStore.servers and pluginsStore.openServer in the button onClick$ handler).src/components/plugins/AddSpigotDialog.tsx (1)
37-41:⚠️ Potential issue | 🟠 MajorGuard every Spiget response before parsing and mutating state.
All three Spiget calls still assume success. A non-OK response or bad JSON will either reject the handler or leave
resolvedPluginin a half-resolved state because the code proceeds straight into.json()and assignment.Also applies to: 76-77, 139-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/AddSpigotDialog.tsx` around lines 37 - 41, The Spiget fetches in AddSpigotDialog (the calls using pluginId that populate versionsData, data and ultimately resolvedPlugin) must validate HTTP responses and JSON parsing before mutating state; for each fetch (the initial resource fetch, the versions fetch, and the other call around lines 139-141) check response.ok and parse JSON inside try/catch (or use a small helper like fetchJsonWithCheck), and if any fetch or parse fails return/throw and avoid setting resolvedPlugin or partial fields. Ensure you only call setResolvedPlugin (or mutate resolvedPlugin) after all three responses are validated and parsed successfully, and provide an error path (set error state or bail out) when a non-OK response or invalid JSON is encountered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/plugins/AddModrinthDialog.tsx`:
- Around line 37-42: The code in AddModrinthDialog performs fetches (project
lookup and version fetch using pluginId) and calls res.json() without checking
response.ok or catching JSON parsing errors, which can throw or leave
resolvedPlugin in a partial state; wrap each network call (the project fetch,
search fetch, and version fetch used in functions around pluginId and
resolvedPlugin) in try/catch, verify response.ok before calling .json() (and
surface or handle non-OK statuses like 404/429), only assign to resolvedPlugin
after both responses are successfully parsed and validated, and ensure any JSON
parsing errors are caught and handled (e.g., set an error state or return early)
so the dialog never renders partial/invalid data.
- Around line 19-23: The resolved plugin state (resolvedPlugin.plugin /
resolvedPlugin.plugins) is not being cleared when the user changes the input or
switches lookup modes; update the handlers to reset this stale state: in the
onInput$ handler (inside AddModrinthDialog.tsx) clear resolvedPlugin (e.g., set
to null or an empty object) whenever the input value changes or doesn't match
the Modrinth URL pattern, and likewise clear resolvedPlugin in any lookup-mode
toggle handlers (the functions that switch between pasted-URL, free-text search,
or results view) and after empty search results so the Add action cannot submit
a stale plugin; reference the resolvedPlugin state variable and the onInput$ and
lookup-mode toggle handlers when making the changes.
- Around line 176-180: The onChange$ handler converts el.value to Number and
compares against version.id, which fails for non-numeric IDs; update the
selection logic in the onChange$ callback (the code that computes
selectedVersion from resolvedPlugin.plugin.data?.versions) to compare IDs as
strings (e.g., compare String(version.id) === el.value or compare
version.id.toString() to el.value) instead of using Number(el.value), so
selectedVersion is found and resolvedPlugin.plugin.version gets set correctly.
- Around line 44-45: In AddModrinthDialog, the newPlugin object is using the
mutable slug for persistent storage; change the stored identifier from data.slug
to data.id (the immutable base62 project ID) wherever newPlugin.id is set (e.g.,
in the newPlugin construction and the other assignment mentioned) and retain
data.slug only when building URLs (keep a separate slug variable or use
data.slug inline for URL construction). Update references that expect the
persistent ID to use the id field (data.id) so persistence uses the immutable
project ID while URL creation still uses the slug.
In `@src/components/plugins/AddSpigotDialog.tsx`:
- Around line 19-23: When the user changes the query or switches input modes in
AddSpigotDialog (the onInput$ handler and the other handlers around the
search/paste logic), any previous resolvedPlugin.plugin or
resolvedPlugin.plugins must be cleared so stale data cannot be submitted; update
the onInput$ handler (and the other handlers referenced around 67-111) to detect
query changes/no-match and call the state setter to reset resolvedPlugin to an
empty state (e.g., plugin: null and plugins: []), and also ensure mode switches
explicitly clear resolvedPlugin before starting a new lookup.
In `@src/components/plugins/PluginCard.tsx`:
- Around line 65-100: The useVisibleTask$ in PluginCard.tsx only hydrates
plugin.data for plugin.type === 'spigot', so persisted Modrinth entries (which
arrive with only id/name/version) never get plugin.data and stay loading; update
the task to also handle plugin.type === 'modrinth' by fetching the Modrinth
project and latest version endpoints, mapping the response into the same
PluginData shape and setting plugin.data.latestVersion, and ensure you set
plugin.data for both types before exiting the try block (references:
useVisibleTask$, plugin.type, plugin.data, PluginData, latestVersion).
In `@src/routes/resources/plugins/index.tsx`:
- Around line 440-462: Before appending the constructed PluginType in the
onClick$ handler, check pluginsStore.servers[pluginsStore.openServer] for an
existing entry with the same id (from resolvedPlugin.plugin.id) and skip
updating the store if found; i.e., in the onClick$ block (where PluginType is
built from resolvedPlugin) perform a duplicate check by id against the current
array and only set pluginsStore.servers = { ... , [pluginsStore.openServer]:
[...existing, plugin] } when no matching id exists.
- Around line 250-259: The import handler attached to the input (id "import")
runs on every keystroke via onInput$ and pushes parsed plugins into
pluginsStore.servers[pluginsStore.openServer], causing duplicates; change the
event to a single-action trigger (e.g., onBlur$ or a dedicated "Import" button
click) and in the handler parse the JSON once, then replace or merge safely
(e.g., assign pluginsStore.servers[pluginsStore.openServer] = importedPlugins or
deduplicate by ID) instead of always doing push(...importedPlugins); keep the
early return when !pluginsStore.openServer and retain the try/catch for
JSON.parse errors.
- Around line 15-47: downloadSpigotPlugin currently awaits a long timeout before
calling window.open, which breaks user activation and causes subsequent
downloads in a bulk loop to be popup-blocked; change the flow so window.open is
called synchronously from the user-initiated context (inside
downloadSpigotPlugin or the bulk-download handler) and defer navigation or
rate-limit waiting without blocking the open. Specifically, to fix: in
downloadSpigotPlugin (and the bulk-download handler that calls it) remove any
await that occurs before calling window.open and instead either (a) open a
synchronous placeholder window (about:blank) immediately and then set its
location after the rate-limit delay, or (b) create and click hidden <a download>
anchors synchronously for each file, or (c) implement a user-driven queue that
opens each window only while user activation is present; ensure the rate-limit
wait happens after opening the window so activation is preserved.
---
Duplicate comments:
In `@src/components/plugins/AddSpigotDialog.tsx`:
- Around line 37-41: The Spiget fetches in AddSpigotDialog (the calls using
pluginId that populate versionsData, data and ultimately resolvedPlugin) must
validate HTTP responses and JSON parsing before mutating state; for each fetch
(the initial resource fetch, the versions fetch, and the other call around lines
139-141) check response.ok and parse JSON inside try/catch (or use a small
helper like fetchJsonWithCheck), and if any fetch or parse fails return/throw
and avoid setting resolvedPlugin or partial fields. Ensure you only call
setResolvedPlugin (or mutate resolvedPlugin) after all three responses are
validated and parsed successfully, and provide an error path (set error state or
bail out) when a non-OK response or invalid JSON is encountered.
In `@src/routes/resources/plugins/index.tsx`:
- Around line 299-312: The loop over plugins incorrectly uses return inside the
for (const plugin of plugins) loop which aborts the entire click handler on the
first plugin that doesn't need an update; change that return to continue so the
loop skips to the next plugin but still allows subsequent plugins to be
processed and the cleanup line (isLoading.value = isLoading.value.filter(...))
to run; update the block around the updateAvailable check in the function that
contains plugins, downloadSpigotPlugin, and isLoading so plugin.version =
plugin.data?.latestVersion only runs for plugins that were actually updated and
control flow uses continue instead of return.
- Around line 207-217: When renaming a server in the onClick handler, the code
deletes pluginsStore.servers[pluginsStore.openServer] before reading it which
causes loss of the plugin list; fix by first copying the existing entry into a
temporary variable (e.g., const oldEntry =
pluginsStore.servers[pluginsStore.openServer]), then assign that oldEntry to
pluginsStore.servers[newName], and only then delete the old key and update
pluginsStore.openServer to newName (references: pluginsStore.servers and
pluginsStore.openServer in the button onClick$ handler).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebf7e1eb-1595-473c-8e5d-423156fab513
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.jsonsrc/components/plugins/AddModrinthDialog.tsxsrc/components/plugins/AddSpigotDialog.tsxsrc/components/plugins/PluginCard.tsxsrc/routes/resources/plugins/index.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
Comment on lines
+19
to
+23
| onInput$={async (e, el) => { | ||
| const value = el.value; | ||
| const modrinthMatch = value.match(/modrinth\.com\/plugin\/(.+)/); | ||
|
|
||
| if (!modrinthMatch) return; |
Contributor
There was a problem hiding this comment.
Reset stale resolved state when the user changes lookup mode.
The old resolvedPlugin.plugin / resolvedPlugin.plugins survives when the input flips between pasted URL, free-text search, and empty-result paths. As a result, the add action in src/routes/resources/plugins/index.tsx, Lines 433-468, can still submit a plugin that no longer matches the current input.
Also applies to: 72-73, 74-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddModrinthDialog.tsx` around lines 19 - 23, The
resolved plugin state (resolvedPlugin.plugin / resolvedPlugin.plugins) is not
being cleared when the user changes the input or switches lookup modes; update
the handlers to reset this stale state: in the onInput$ handler (inside
AddModrinthDialog.tsx) clear resolvedPlugin (e.g., set to null or an empty
object) whenever the input value changes or doesn't match the Modrinth URL
pattern, and likewise clear resolvedPlugin in any lookup-mode toggle handlers
(the functions that switch between pasted-URL, free-text search, or results
view) and after empty search results so the Add action cannot submit a stale
plugin; reference the resolvedPlugin state variable and the onInput$ and
lookup-mode toggle handlers when making the changes.
Comment on lines
+37
to
+42
| const res = await fetch(`https://api.modrinth.com/v2/project/${pluginId}`); | ||
| const data = await res.json() as any; | ||
| console.log(data); | ||
|
|
||
| const versionsRes = await fetch(`https://api.modrinth.com/v2/project/${pluginId}/version`); | ||
| const versionsData = await versionsRes.json() as any; |
Contributor
There was a problem hiding this comment.
Handle failed Modrinth responses before calling .json().
The direct lookup, search, and version fetch all parse blindly and then mutate resolvedPlugin. A 404/429 or invalid JSON here will either throw out of the event handler or populate the dialog with partial data.
Also applies to: 81-84, 144-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddModrinthDialog.tsx` around lines 37 - 42, The code
in AddModrinthDialog performs fetches (project lookup and version fetch using
pluginId) and calls res.json() without checking response.ok or catching JSON
parsing errors, which can throw or leave resolvedPlugin in a partial state; wrap
each network call (the project fetch, search fetch, and version fetch used in
functions around pluginId and resolvedPlugin) in try/catch, verify response.ok
before calling .json() (and surface or handle non-OK statuses like 404/429),
only assign to resolvedPlugin after both responses are successfully parsed and
validated, and ensure any JSON parsing errors are caught and handled (e.g., set
an error state or return early) so the dialog never renders partial/invalid
data.
Comment on lines
+44
to
+45
| const newPlugin: PluginWithData = { | ||
| id: data.slug, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/plugins/AddModrinthDialog.tsx | head -120Repository: birdflop/web
Length of output: 5628
🌐 Web query:
Modrinth API project response schema fields id slug project_id
💡 Result:
In the Modrinth API (Labrinth v2), the project response schema from endpoints like GET /project/{id|slug} includes the following key fields related to identification: - id: required, string, The ID of the project, encoded as a base62 string (example: "AABBCCDD"). This is the unique identifier for the project. - slug: string, The slug of a project, used for vanity URLs. Regex: ^[\w!@$.+,"-']{3,64}$ (example: "my_project"). Note: There is no top-level field named "project_id" in the project response object itself. "project_id" appears in related schemas, such as the version object (e.g., in GET /project/{id|slug}/version), where it is a required string referencing the ID of the parent project (example: "AABBCCDD"). In search responses or dependencies, "project_id" may refer to a project's ID in hit objects or dependency arrays. The path parameter for project endpoints accepts either "id" or "slug". Full schema includes many other fields like title, description, categories, downloads, team, versions, etc., as documented in official sources.
Citations:
- 1: https://docs.modrinth.com/api/operations/getproject/
- 2: https://docs.modrinth.com/api/
- 3: https://docs.modrinth.com/api/operations/createproject/
- 4: https://docs.modrinth.com/api
- 5: https://docs.modrinth.com/api/operations/getprojectversions/
- 6: https://docs.modrinth.com/api/operations/getversionfromidornumber/
🌐 Web query:
Modrinth API search response hits schema fields id slug
💡 Result:
The Modrinth API search endpoint (/v2/search or /search) returns a JSON object with a top-level "hits" array containing search result objects. Each hit object includes the fields "id" (the unique base62-encoded project ID) and "slug" (the URL-friendly project slug, regex ^[\w!@$.+,"-']{3,64}$). Other common fields in hits include: title, description, categories (array), project_type (e.g., "mod"), downloads (integer), icon_url, color, follows (integer), date_created, etc. The response also includes top-level fields like offset and total_hits.
Citations:
- 1: https://docs.modrinth.com/api/operations/searchprojects/
- 2: https://mintlify.com/modrinth/code/api/search
Use the immutable project ID instead of the slug for persistent storage.
Modrinth documents that slugs can change and recommends using the unique ID for stable references. Both the direct project endpoint and search results expose an id field (base62-encoded immutable identifier) which should be used for persisting plugins across sessions. Replace data.slug with data.id at lines 45 and 96, and keep the slug separately only for constructing URLs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddModrinthDialog.tsx` around lines 44 - 45, In
AddModrinthDialog, the newPlugin object is using the mutable slug for persistent
storage; change the stored identifier from data.slug to data.id (the immutable
base62 project ID) wherever newPlugin.id is set (e.g., in the newPlugin
construction and the other assignment mentioned) and retain data.slug only when
building URLs (keep a separate slug variable or use data.slug inline for URL
construction). Update references that expect the persistent ID to use the id
field (data.id) so persistence uses the immutable project ID while URL creation
still uses the slug.
Comment on lines
+176
to
+180
| } onChange$={(e, el) => { | ||
| const versionId = Number(el.value); | ||
| if (!resolvedPlugin.plugin) return; | ||
| const selectedVersion = resolvedPlugin.plugin.data?.versions | ||
| ?.find((version) => version.id == versionId); |
Contributor
There was a problem hiding this comment.
Compare selected version IDs as strings here.
src/components/Elements/SelectList.tsx, Lines 48-70, writes option values through toString(), so el.value is always a string. Converting it to number here means any non-numeric version ID never matches resolvedPlugin.plugin.data.versions, leaving resolvedPlugin.plugin.version unset and the parent Add button disabled.
Suggested fix
- const versionId = Number(el.value);
+ const versionId = el.value;
if (!resolvedPlugin.plugin) return;
const selectedVersion = resolvedPlugin.plugin.data?.versions
- ?.find((version) => version.id == versionId);
+ ?.find((version) => String(version.id) === versionId);📝 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.
Suggested change
| } onChange$={(e, el) => { | |
| const versionId = Number(el.value); | |
| if (!resolvedPlugin.plugin) return; | |
| const selectedVersion = resolvedPlugin.plugin.data?.versions | |
| ?.find((version) => version.id == versionId); | |
| } onChange$={(e, el) => { | |
| const versionId = el.value; | |
| if (!resolvedPlugin.plugin) return; | |
| const selectedVersion = resolvedPlugin.plugin.data?.versions | |
| ?.find((version) => String(version.id) === versionId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddModrinthDialog.tsx` around lines 176 - 180, The
onChange$ handler converts el.value to Number and compares against version.id,
which fails for non-numeric IDs; update the selection logic in the onChange$
callback (the code that computes selectedVersion from
resolvedPlugin.plugin.data?.versions) to compare IDs as strings (e.g., compare
String(version.id) === el.value or compare version.id.toString() to el.value)
instead of using Number(el.value), so selectedVersion is found and
resolvedPlugin.plugin.version gets set correctly.
Comment on lines
+19
to
+23
| onInput$={async (e, el) => { | ||
| const value = el.value; | ||
| const spigotMatch = value.match(/spigotmc\.org\/resources\/(.+)\.(\d+)/); | ||
|
|
||
| if (!spigotMatch) return; |
Contributor
There was a problem hiding this comment.
Clear stale lookup state when the query changes.
A previous resolvedPlugin.plugin / resolvedPlugin.plugins survives when the user switches between pasted-URL mode, search mode, or a no-results query. The parent add action in src/routes/resources/plugins/index.tsx, Lines 433-468, only reads resolvedPlugin.plugin, so stale data here can still be submitted.
Also applies to: 67-68, 69-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/AddSpigotDialog.tsx` around lines 19 - 23, When the
user changes the query or switches input modes in AddSpigotDialog (the onInput$
handler and the other handlers around the search/paste logic), any previous
resolvedPlugin.plugin or resolvedPlugin.plugins must be cleared so stale data
cannot be submitted; update the onInput$ handler (and the other handlers
referenced around 67-111) to detect query changes/no-match and call the state
setter to reset resolvedPlugin to an empty state (e.g., plugin: null and
plugins: []), and also ensure mode switches explicitly clear resolvedPlugin
before starting a new lookup.
Comment on lines
+65
to
+100
| useVisibleTask$(async () => { | ||
| if (!isBrowser) return; // dont request plugin data on the server | ||
| if (plugin.data || !plugin.id) return; | ||
| try { | ||
| if (plugin.type === 'spigot') { | ||
| const res = await fetch(`https://api.spiget.org/v2/resources/${plugin.id}`); | ||
| const data = await res.json() as any; | ||
|
|
||
| const pluginData: PluginData = { | ||
| external: data.external, | ||
| name: data.name, | ||
| tag: data.tag, | ||
| iconUrl: data.icon?.url, | ||
| releaseDate: data.releaseDate, | ||
| updateDate: data.updateDate, | ||
| file: data.file ? { | ||
| type: data.file.type, | ||
| size: data.file.size, | ||
| sizeUnit: data.file.sizeUnit, | ||
| url: data.file.url, | ||
| externalUrl: data.file.externalUrl, | ||
| } : undefined, | ||
| testedVersions: data.testedVersions, | ||
| sourceCodeLink: data.sourceCodeLink, | ||
| }; | ||
| // fetch latest version | ||
| const latestVerResponse = await fetch(`https://api.spiget.org/v2/resources/${plugin.id}/versions/latest`); | ||
| const latestVersion = await latestVerResponse.json() as any; | ||
| pluginData.latestVersion = { | ||
| id: latestVersion.id, | ||
| name: latestVersion.name, | ||
| releaseDate: latestVersion.releaseDate, | ||
| }; | ||
|
|
||
| plugin.data = pluginData; | ||
| } |
Contributor
There was a problem hiding this comment.
Persisted Modrinth plugins never hydrate here.
This task only repopulates plugin.data for plugin.type === 'spigot'. In src/routes/resources/plugins/index.tsx, Lines 117-133 and 443-462 strip plugin.data before storing/appending entries, so Modrinth cards arrive here with only id/name/version, stay in the loading state, and never regain latestVersion for update checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/PluginCard.tsx` around lines 65 - 100, The
useVisibleTask$ in PluginCard.tsx only hydrates plugin.data for plugin.type ===
'spigot', so persisted Modrinth entries (which arrive with only id/name/version)
never get plugin.data and stay loading; update the task to also handle
plugin.type === 'modrinth' by fetching the Modrinth project and latest version
endpoints, mapping the response into the same PluginData shape and setting
plugin.data.latestVersion, and ensure you set plugin.data for both types before
exiting the try block (references: useVisibleTask$, plugin.type, plugin.data,
PluginData, latestVersion).
Comment on lines
+15
to
+47
| export const downloadSpigotPlugin = $(async ( | ||
| plugin: PluginWithData, | ||
| spigotRateLimit?: { downloadCount: number, resetTime: number }, | ||
| notifications?: Notification[], | ||
| ) => { | ||
| // if the plugin has an external url, open that instead of spigot to avoid rate limits | ||
| if (plugin.data?.file?.externalUrl) { | ||
| window.open(plugin.data.file.externalUrl, '_blank'); | ||
| return; | ||
| } | ||
|
|
||
| // spigot rate limits downloads to 10 per minute | ||
| if (spigotRateLimit && spigotRateLimit.downloadCount >= 10 && Date.now() < spigotRateLimit.resetTime) { | ||
| if (notifications) { | ||
| const notification = new Notification() | ||
| .setTitle('Spigot Download Rate Limit Reached') | ||
| .setDescription(`Spigot limits downloads to 10 per minute. Waiting ${Math.ceil((spigotRateLimit.resetTime - Date.now()) / 1000)} seconds to continue downloading.`) | ||
| .setBgColor('lum-bg-yellow/50') | ||
| .setPersist(true); | ||
| notifications.push(notification); | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, spigotRateLimit.resetTime - Date.now())); | ||
| spigotRateLimit.downloadCount = 0; | ||
| } | ||
|
|
||
| // open the plugin file url in a new tab to trigger the download | ||
| window.open(`https://www.spigotmc.org/${plugin.data?.file?.url}`, '_blank'); | ||
|
|
||
| if (!spigotRateLimit) return; | ||
| spigotRateLimit.downloadCount++; | ||
| // set the reset time to 1 minute from now | ||
| if (spigotRateLimit.resetTime < Date.now()) | ||
| spigotRateLimit.resetTime = Date.now() + 60 * 1000; |
Contributor
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
wc -l src/routes/resources/plugins/index.tsxRepository: birdflop/web
Repository: birdflop/web
Exit code: 0
stdout:
480 src/routes/resources/plugins/index.tsx
Script executed:
cat -n src/routes/resources/plugins/index.tsx | sed -n '1,50p'Repository: birdflop/web
Repository: birdflop/web
Exit code: 0
stdout:
1 import { $, component$, createContextId, isBrowser, useComputed$, useContext, useContextProvider, useSignal, useStore, useTask$, useVisibleTask$ } from '@builder.io/qwik';
2 import { inlineTranslate } from 'qwik-speak';
3 import { Notification, NotificationContext } from '~/util/Notification';
4 import { Blocks, Check, Copy, Download, Pencil, Plus, Trash, X } from 'lucide-icons-qwik';
5 import { defaultDescription, generateHead } from '~/root';
6 import { SelectMenu, Toggle } from '@luminescent/ui-qwik';
7 import PluginCard, { PluginSource, PluginType, PluginWithData } from '~/components/plugins/PluginCard';
8 import AddSpigotDialog from '~/components/plugins/AddSpigotDialog';
9 import AddModrinthDialog from '~/components/plugins/AddModrinthDialog';
10 import AddMiscDialog from '~/components/plugins/AddMiscDialog';
11 import { deepTrack } from '~/util/misc';
12
13 const debug = true;
14
15 export const downloadSpigotPlugin = $(async (
16 plugin: PluginWithData,
17 spigotRateLimit?: { downloadCount: number, resetTime: number },
18 notifications?: Notification[],
19 ) => {
20 // if the plugin has an external url, open that instead of spigot to avoid rate limits
21 if (plugin.data?.file?.externalUrl) {
22 window.open(plugin.data.file.externalUrl, '_blank');
23 return;
24 }
25
26 // spigot rate limits downloads to 10 per minute
27 if (spigotRateLimit && spigotRateLimit.downloadCount >= 10 && Date.now() < spigotRateLimit.resetTime) {
28 if (notifications) {
29 const notification = new Notification()
30 .setTitle('Spigot Download Rate Limit Reached')
31 .setDescription(`Spigot limits downloads to 10 per minute. Waiting ${Math.ceil((spigotRateLimit.resetTime - Date.now()) / 1000)} seconds to continue downloading.`)
32 .setBgColor('lum-bg-yellow/50')
33 .setPersist(true);
34 notifications.push(notification);
35 }
36 await new Promise((resolve) => setTimeout(resolve, spigotRateLimit.resetTime - Date.now()));
37 spigotRateLimit.downloadCount = 0;
38 }
39
40 // open the plugin file url in a new tab to trigger the download
41 window.open(`https://www.spigotmc.org/${plugin.data?.file?.url}`, '_blank');
42
43 if (!spigotRateLimit) return;
44 spigotRateLimit.downloadCount++;
45 // set the reset time to 1 minute from now
46 if (spigotRateLimit.resetTime < Date.now())
47 spigotRateLimit.resetTime = Date.now() + 60 * 1000;
48 });
49
50 type ResolvedPluginType = {
Script executed:
cat -n src/routes/resources/plugins/index.tsx | sed -n '280,320p'Repository: birdflop/web
Repository: birdflop/web
Exit code: 0
stdout:
280 )}
281 <button class="lum-btn lum-btn-p-1 lum-bg-transparent rounded-lum-1" onClick$={() => {
282 const plugins = pluginsStore.servers[pluginsStore.openServer!];
283 plugins.forEach((plugin) => {
284 if (plugin.data?.latestVersion) plugin.version = plugin.data.latestVersion;
285 plugin.updateDate = Date.now();
286 });
287 }}>
288 <Check size={16} />
289 Mark all updated
290 </button>
291 </>}
292
293 {!!outdatedPlugins.value &&
294 <button class="lum-btn lum-btn-p-1 lum-bg-transparent rounded-lum-1 group" onClick$={async () => {
295
296 isLoading.value = [...isLoading.value, 'downloadAll'];
297
298 const plugins = pluginsStore.servers[pluginsStore.openServer!];
299 for (const plugin of plugins) {
300 const updateAvailable = plugin.data?.latestVersion?.releaseDate !== undefined
301 && plugin.version?.releaseDate !== undefined
302 && plugin.data.latestVersion.releaseDate > plugin.version.releaseDate;
303
304 if (!updateAvailable || !plugin.data?.file?.url) return;
305
306 if (plugin.type === 'spigot') await downloadSpigotPlugin(plugin, spigotRateLimit);
307 else window.open(plugin.data.file.url, '_blank');
308
309 plugin.version = plugin.data?.latestVersion;
310 }
311
312 isLoading.value = isLoading.value.filter((item) => item !== 'downloadAll');
313 }} disabled={isLoading.value.includes('downloadAll')}>
314 <Download size={16} />
315 Download all out of date ({outdatedPlugins.value})
316 {outdatedPlugins.value > 10 &&
317 <span class="lum-card p-2 absolute bottom-full left-0 w-full text-xs mb-2 opacity-0 group-hover:opacity-100 transition-opacity pointer-events-none">
318 Spigot limits downloads to 10 per minute, so some of these may not open immediately.
319 </span>
320 }
window.open user activation requirement browser behavior await setTimeout
In modern browsers (Chrome, Firefox, Safari, Edge as of 2026), window.open requires transient user activation to succeed without being blocked by popup blockers. Transient activation is granted on direct user interactions like click, mousedown, pointerdown (mouse), keydown (non-special keys), touchend. Transient activation expires after a short browser-specific timeout (~1 second in Chrome per Chrome 72+ User Activation v2; "short while"/"few seconds" unspecified exactly in MDN/HTML spec; up to 5s observed in some contexts) or is consumed by calling certain APIs like window.open. setTimeout preserves transient activation if called from a user activation handler and executes before timeout (~100ms chained setTimeouts work in Chrome demos). However, longer delays or multiple levels risk expiration. await (async/await, Promises) can preserve it in modern Chrome (User Activation v2 makes async consistent), but Safari/Firefox are stricter: async/await often loses direct user gesture link, causing blocks. Workaround: Open blank window synchronously in click handler (window.open('about:blank', '_blank')), then async set newWindow.location.href = url. Check navigator.userActivation.isActive before calling. window.open consumes transient activation (only one per activation in Chrome; varies slightly by browser). Sources confirm strict enforcement post-2019 standards alignment.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Security/User_activation
- 2: https://developer.mozilla.org/en-US/docs/Web/API/UserActivation
- 3: https://webkit.org/blog/13862/the-user-activation-api/
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Window/open
- 5: https://developer.chrome.com/blog/user-activation
- 6: https://developer.mozilla.org/en-US/docs/Glossary/Transient_activation
- 7: https://webkit.org/blog/13862/the-user-activation-api
- 8: https://macwright.com/2022/07/11/activation.html
- 9: https://www.ryanthomson.net/articles/you-shouldnt-call-window-open-asynchronously/
- 10: https://stackoverflow.com/questions/74128322/using-window-open-in-an-async-function-in-firefox-and-safari
Script executed:
rg -A5 -B5 "navigator.userActivation|isActive|about:blank" src/routes/resources/plugins/index.tsxRepository: birdflop/web
Repository: birdflop/web
Exit code: 1
Script executed:
grep -n "TODO\|FIXME\|BUG\|NOTE.*activation\|popup" src/routes/resources/plugins/index.tsxRepository: birdflop/web
Repository: birdflop/web
Exit code: 1
Bulk-download strategy loses user activation and will popup-block after the first download.
downloadSpigotPlugin() awaits a setTimeout() at line 36 that can delay up to 60 seconds before calling window.open(). The bulk-download handler at line 294-307 awaits this function repeatedly in a loop. Browsers only grant transient user activation for 1–5 seconds after user input, and each window.open() call consumes it. Modern Firefox and Safari also break the activation chain with await. This means the first download may succeed, but subsequent downloads in the loop will be silently popup-blocked and fail.
Consider switching to explicit links, a user-driven queue with synchronous windows, or the about:blank + location.href pattern to preserve activation across multiple opens. See MDN User Activation and Window.open behavior.
Also applies to: 294-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 15 - 47,
downloadSpigotPlugin currently awaits a long timeout before calling window.open,
which breaks user activation and causes subsequent downloads in a bulk loop to
be popup-blocked; change the flow so window.open is called synchronously from
the user-initiated context (inside downloadSpigotPlugin or the bulk-download
handler) and defer navigation or rate-limit waiting without blocking the open.
Specifically, to fix: in downloadSpigotPlugin (and the bulk-download handler
that calls it) remove any await that occurs before calling window.open and
instead either (a) open a synchronous placeholder window (about:blank)
immediately and then set its location after the rate-limit delay, or (b) create
and click hidden <a download> anchors synchronously for each file, or (c)
implement a user-driven queue that opens each window only while user activation
is present; ensure the rate-limit wait happens after opening the window so
activation is preserved.
Comment on lines
+250
to
+259
| <input class="lum-input lum-input-p-1 rounded-lum-1 lum-bg-transparent" id="import" name="import" placeholder={`${t('plugins.import@@Import')} - ${t('plugins.pasteHere@@Paste here')}`} | ||
| onInput$={(e, el) => { | ||
| if (!pluginsStore.openServer) return; | ||
| try { | ||
| const importedPlugins = JSON.parse(el.value) as PluginWithData[]; | ||
| pluginsStore.servers[pluginsStore.openServer].push(...importedPlugins); | ||
| } catch (err) { | ||
| console.error('Failed to parse imported plugins:', err); | ||
| } | ||
| }}/> |
Contributor
There was a problem hiding this comment.
Importing on input will re-append the same JSON.
This fires on every keystroke/paste. Once the text becomes valid JSON, any further edit or re-paste appends the whole array again, so importing is not idempotent and quickly creates duplicates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 250 - 259, The import
handler attached to the input (id "import") runs on every keystroke via onInput$
and pushes parsed plugins into pluginsStore.servers[pluginsStore.openServer],
causing duplicates; change the event to a single-action trigger (e.g., onBlur$
or a dedicated "Import" button click) and in the handler parse the JSON once,
then replace or merge safely (e.g., assign
pluginsStore.servers[pluginsStore.openServer] = importedPlugins or deduplicate
by ID) instead of always doing push(...importedPlugins); keep the early return
when !pluginsStore.openServer and retain the try/catch for JSON.parse errors.
Comment on lines
+440
to
+462
| onClick$={() => { | ||
| if (!pluginsStore.openServer || !resolvedPlugin.plugin) return; | ||
|
|
||
| const plugin: PluginType = { | ||
| type: resolvedPlugin.type, | ||
| id: resolvedPlugin.plugin.id, | ||
| name: resolvedPlugin.plugin.name, | ||
| url: resolvedPlugin.plugin.url, | ||
| iconUrl: resolvedPlugin.plugin.data?.iconUrl, | ||
| version: resolvedPlugin.plugin.version ? { | ||
| id: resolvedPlugin.plugin.version.id, | ||
| name: resolvedPlugin.plugin.version.name, | ||
| releaseDate: resolvedPlugin.plugin.version.releaseDate, | ||
| } : undefined, | ||
| }; | ||
|
|
||
| pluginsStore.servers = { | ||
| ...pluginsStore.servers, | ||
| [pluginsStore.openServer]: [ | ||
| ...pluginsStore.servers[pluginsStore.openServer], | ||
| plugin, | ||
| ], | ||
| }; |
Contributor
There was a problem hiding this comment.
Add a final duplicate check before appending to the server.
This block blindly spreads plugin into the active server list. The direct-URL dialogs warn on duplicates, but search/selection flows can still reach this path with an existing plugin, so the store can accumulate duplicate entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 440 - 462, Before
appending the constructed PluginType in the onClick$ handler, check
pluginsStore.servers[pluginsStore.openServer] for an existing entry with the
same id (from resolvedPlugin.plugin.id) and skip updating the store if found;
i.e., in the onClick$ block (where PluginType is built from resolvedPlugin)
perform a duplicate check by id against the current array and only set
pluginsStore.servers = { ... , [pluginsStore.openServer]: [...existing, plugin]
} when no matching id exists.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
src/components/plugins/AddModrinthDialog.tsx (4)
177-181:⚠️ Potential issue | 🟠 MajorCompare selected version IDs as strings.
SelectListemits string values; coercing toNumberprevents matching non-numeric IDs.🔧 Proposed fix
-const versionId = Number(el.value); +const versionId = el.value; ... - ?.find((version) => version.id == versionId); + ?.find((version) => String(version.id) === versionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/AddModrinthDialog.tsx` around lines 177 - 181, The onChange$ handler currently coerces el.value to Number which breaks matching when SelectList emits string IDs; update the handler in AddModrinthDialog (the onChange$ using resolvedPlugin and selectedVersion) to compare strings instead of numbers—e.g., use el.value directly and match with String(version.id) === el.value (or otherwise convert both sides to the same string form) so non-numeric IDs will match correctly.
19-24:⚠️ Potential issue | 🟠 MajorReset resolved state whenever lookup mode/input changes.
The current flow keeps previous
resolvedPlugin.plugin/resolvedPlugin.pluginsalive, so a stale selection can still be submitted after input mode changes or empty results.🔧 Proposed fix
onInput$={async (e, el) => { const value = el.value; + resolvedPlugin.plugin = undefined; + resolvedPlugin.plugins = undefined; const modrinthMatch = value.match(/modrinth\.com\/plugin\/(.+)/); if (!modrinthMatch) return; ... - resolvedPlugin.plugin = newPlugin; + resolvedPlugin.plugin = newPlugin; + resolvedPlugin.plugins = undefined; }} onChange$={async (e, el) => { const value = el.value; const modrinthMatch = value.match(/modrinth\.com\/plugin\/(.+)/); if (modrinthMatch) return; + resolvedPlugin.plugin = undefined; + resolvedPlugin.plugins = undefined; ... if (searchData.hits.length === 0) { + resolvedPlugin.plugin = undefined; + resolvedPlugin.plugins = undefined; ... return; }Also applies to: 72-73, 74-77, 86-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/AddModrinthDialog.tsx` around lines 19 - 24, Previous search results remain in resolvedPlugin.plugin / resolvedPlugin.plugins when the input or lookup mode changes, allowing a stale selection to be submitted; update the handlers that respond to input changes (onInput$) and lookup mode toggles to clear the resolved results and selection by resetting resolvedPlugin to its empty initial state (e.g., set resolvedPlugin.plugin = null and resolvedPlugin.plugins = [] or call the component's resetResolvedPlugin helper) whenever the input is empty, the modrinth URL no longer matches, or the lookup mode switches, and also clear resolvedPlugin when a lookup returns no results so stale selections cannot be submitted.
37-43:⚠️ Potential issue | 🟠 MajorGuard fetch failures before parsing JSON.
Direct lookup, search, and version fetch currently parse blindly. Non-2xx or invalid JSON will throw and can leave inconsistent state.
🛡️ Proposed fix
-const res = await fetch(`https://api.modrinth.com/v2/project/${pluginId}`); -const data = await res.json() as any; +const res = await fetch(`https://api.modrinth.com/v2/project/${pluginId}`); +if (!res.ok) return; +const data = await res.json() as any; ... -const searchRes = await fetch(`https://api.modrinth.com/v2/search?query=${encodeURIComponent(value)}`); -const searchData = await searchRes.json(); +const searchRes = await fetch(`https://api.modrinth.com/v2/search?query=${encodeURIComponent(value)}`); +if (!searchRes.ok) return; +const searchData = await searchRes.json(); ... -const versionsRes = await fetch(`https://api.modrinth.com/v2/project/${pluginId}/version?...`); -const versionsData = await versionsRes.json() as any; +const versionsRes = await fetch(`https://api.modrinth.com/v2/project/${pluginId}/version?...`); +if (!versionsRes.ok) return; +const versionsData = await versionsRes.json() as any;Also applies to: 81-84, 145-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/AddModrinthDialog.tsx` around lines 37 - 43, The fetches in AddModrinthDialog (the calls that assign res/data and versionsRes/versionsData) parse JSON without checking response.ok and will throw on non-2xx or invalid JSON; update the code around those fetches to check response.ok (or response.status) before calling res.json(), handle non-OK responses by logging or setting an error state and returning early, and wrap the res.json() calls in try/catch to handle invalid JSON; apply the same guards to the other fetch sites referenced (lines near 81-84 and 145-147) so all network responses are validated before parsing and the component state remains consistent (refer to variables res, data, versionsRes, versionsData and the AddModrinthDialog component).
45-45:⚠️ Potential issue | 🟠 MajorPersist immutable Modrinth project IDs, not slugs.
slugcan change; usedata.idforplugin.idand keep slug only for URL rendering.Modrinth API v2: In project and search responses, is `id` the immutable project identifier while `slug` is mutable? Which field should be used for persistent storage keys?🔧 Proposed fix
- id: data.slug, + id: data.id, ... - id: data.slug, + id: data.id,Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/AddModrinthDialog.tsx` at line 45, The code is persisting Modrinth's mutable slug as the plugin identifier; change assignments where you set plugin.id to data.slug to use data.id instead (e.g., in AddModrinthDialog where you build the plugin object from the Modrinth response), and keep data.slug only for URL or display fields (e.g., plugin.slug or plugin.displayUrl). Locate all occurrences in AddModrinthDialog that assign id: data.slug (and the duplicate occurrence) and replace them with id: data.id while preserving slug for presentation.src/routes/resources/plugins/index.tsx (6)
221-230:⚠️ Potential issue | 🔴 CriticalRename flow deletes the source server before copying it.
This drops the server data because the old entry is removed before assignment.
🐛 Proposed fix
- delete pluginsStore.servers[pluginsStore.openServer!]; - pluginsStore.servers[newName] = pluginsStore.servers[pluginsStore.openServer!]; + const oldServer = pluginsStore.servers[pluginsStore.openServer!]; + pluginsStore.servers[newName] = oldServer; + delete pluginsStore.servers[pluginsStore.openServer!]; pluginsStore.openServer = newName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 221 - 230, The rename flow currently deletes the source server entry before assigning it to the new key, causing data loss; in the onClick$ handler for the rename button capture the source value into a temporary variable (e.g., const oldKey = pluginsStore.openServer and const serverData = pluginsStore.servers[oldKey]) then assign pluginsStore.servers[newName] = serverData and only afterwards delete pluginsStore.servers[oldKey], finally set pluginsStore.openServer = newName to complete the rename without losing data.
454-471:⚠️ Potential issue | 🟠 MajorAdd a final duplicate guard before pushing into server plugins.
Search/select paths can still reach this block with an already-added plugin.
🔧 Proposed fix
const plugin: PluginType = { ... }; -pluginsStore.servers[pluginsStore.openServer!].plugins.push(plugin); +const current = pluginsStore.servers[pluginsStore.openServer!].plugins; +const exists = current.some((p) => String(p.id) === String(plugin.id)); +if (exists) return; +current.push(plugin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 454 - 471, Before pushing the constructed plugin into pluginsStore.servers[pluginsStore.openServer!].plugins, add a duplicate guard that checks the target server's plugins array for an existing entry with the same id (and type if relevant) — e.g., find/ some on pluginsStore.servers[pluginsStore.openServer!].plugins comparing plugin.id (and plugin.type) and return early if found; this ensures the block in the onClick$ handler that builds the plugin (using resolvedPlugin, plugin, pluginsStore, openServer) does not insert duplicates.
318-318:⚠️ Potential issue | 🔴 CriticalUse
continueinstead ofreturninside the bulk-download loop.Line 318 exits the entire handler on the first non-updatable plugin.
🐛 Proposed fix
-if (!updateAvailable || !plugin.data?.file?.url) return; +if (!updateAvailable || !plugin.data?.file?.url) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` at line 318, The handler currently uses "return" inside the bulk-download loop when a plugin is not updatable, which aborts processing of all remaining plugins; replace that "return" with "continue" so the loop skips the non-updatable plugin and proceeds to the next one (change the check that uses updateAvailable and plugin.data?.file?.url inside the loop to use continue). Locate the loop that iterates plugins (where the condition "if (!updateAvailable || !plugin.data?.file?.url) return;" appears) and update that statement to continue the loop instead of returning from the handler.
121-124:⚠️ Potential issue | 🟡 MinorRemove hot-path debug logging from persistence task.
This logs on every tracked store mutation and spams production consoles.
🔧 Proposed fix
useTask$(({ track }) => { deepTrack(track, pluginsStore); - console.log(Date.now());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 121 - 124, The persistence task inside useTask$ is logging Date.now() on every tracked mutation (via deepTrack(track, pluginsStore)), which spams consoles; remove the console.log(Date.now()) call from the persistence task in src/routes/resources/plugins/index.tsx (the useTask$ block referencing deepTrack and pluginsStore), or wrap it behind an environment/dev-only guard so it runs only in development builds.
14-14:⚠️ Potential issue | 🟠 MajorDo not ship debug mode permanently enabled.
This exposes debug-only UI in production.
🔧 Proposed fix
-const debug = true; +const debug = import.meta.env.DEV;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` at line 14, Replace the hard-coded debug flag so debug UI cannot be shipped enabled: remove or stop exporting the const debug = true and instead derive debug from an environment/build flag (e.g. check process.env.NODE_ENV !== 'production' or a dedicated env var like REACT_APP_DEBUG === 'true' or import.meta.env.MODE) where the default for production is false; update any consumers of the debug symbol in this module (references to debug) to use the new env-driven boolean so production builds disable debug-only UI by default.
265-273:⚠️ Potential issue | 🟠 MajorImporting on
onInput$repeatedly appends duplicates.As soon as the JSON is parseable, every subsequent keystroke/paste can append again.
🔧 Proposed fix
-<input ... onInput$={(e, el) => { +<input ... onChange$={(e, el) => { try { const importedPlugins = JSON.parse(el.value) as PluginWithData[]; - pluginsStore.servers[pluginsStore.openServer!].plugins.push(...importedPlugins); + const existing = pluginsStore.servers[pluginsStore.openServer!].plugins; + const merged = new Map(existing.map((p) => [String(p.id), p])); + for (const plugin of importedPlugins) merged.set(String(plugin.id), plugin); + pluginsStore.servers[pluginsStore.openServer!].plugins = [...merged.values()]; } catch (err) { console.error('Failed to parse imported plugins:', err); } }}/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/resources/plugins/index.tsx` around lines 265 - 273, The current onInput$ handler on the input with id "import" repeatedly parses and pushes parsed PluginWithData into pluginsStore.servers[pluginsStore.openServer!].plugins on every keystroke causing duplicates; change this to a single-fire handler (e.g., onBlur$ or onPaste$) or move logic to a submit button handler so parsing only happens once, and when adding use a dedupe/replace strategy (compare unique plugin id or name) before pushing into pluginsStore.servers[pluginsStore.openServer!].plugins to avoid duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/resources/plugins/index.tsx`:
- Around line 70-73: serverDefaults is a single mutable object (type ServerType)
being reused for every new server, causing the plugins array to be shared across
instances; when you create a new server (where the code currently assigns
serverDefaults to a new entry around the block that stores servers), create a
fresh object instead of reusing serverDefaults — e.g., shallow-clone
serverDefaults with an object spread and also clone the plugins array (or
explicitly set plugins: []) so each server gets its own plugins array; update
the creation site that references serverDefaults to use this cloned object.
---
Duplicate comments:
In `@src/components/plugins/AddModrinthDialog.tsx`:
- Around line 177-181: The onChange$ handler currently coerces el.value to
Number which breaks matching when SelectList emits string IDs; update the
handler in AddModrinthDialog (the onChange$ using resolvedPlugin and
selectedVersion) to compare strings instead of numbers—e.g., use el.value
directly and match with String(version.id) === el.value (or otherwise convert
both sides to the same string form) so non-numeric IDs will match correctly.
- Around line 19-24: Previous search results remain in resolvedPlugin.plugin /
resolvedPlugin.plugins when the input or lookup mode changes, allowing a stale
selection to be submitted; update the handlers that respond to input changes
(onInput$) and lookup mode toggles to clear the resolved results and selection
by resetting resolvedPlugin to its empty initial state (e.g., set
resolvedPlugin.plugin = null and resolvedPlugin.plugins = [] or call the
component's resetResolvedPlugin helper) whenever the input is empty, the
modrinth URL no longer matches, or the lookup mode switches, and also clear
resolvedPlugin when a lookup returns no results so stale selections cannot be
submitted.
- Around line 37-43: The fetches in AddModrinthDialog (the calls that assign
res/data and versionsRes/versionsData) parse JSON without checking response.ok
and will throw on non-2xx or invalid JSON; update the code around those fetches
to check response.ok (or response.status) before calling res.json(), handle
non-OK responses by logging or setting an error state and returning early, and
wrap the res.json() calls in try/catch to handle invalid JSON; apply the same
guards to the other fetch sites referenced (lines near 81-84 and 145-147) so all
network responses are validated before parsing and the component state remains
consistent (refer to variables res, data, versionsRes, versionsData and the
AddModrinthDialog component).
- Line 45: The code is persisting Modrinth's mutable slug as the plugin
identifier; change assignments where you set plugin.id to data.slug to use
data.id instead (e.g., in AddModrinthDialog where you build the plugin object
from the Modrinth response), and keep data.slug only for URL or display fields
(e.g., plugin.slug or plugin.displayUrl). Locate all occurrences in
AddModrinthDialog that assign id: data.slug (and the duplicate occurrence) and
replace them with id: data.id while preserving slug for presentation.
In `@src/routes/resources/plugins/index.tsx`:
- Around line 221-230: The rename flow currently deletes the source server entry
before assigning it to the new key, causing data loss; in the onClick$ handler
for the rename button capture the source value into a temporary variable (e.g.,
const oldKey = pluginsStore.openServer and const serverData =
pluginsStore.servers[oldKey]) then assign pluginsStore.servers[newName] =
serverData and only afterwards delete pluginsStore.servers[oldKey], finally set
pluginsStore.openServer = newName to complete the rename without losing data.
- Around line 454-471: Before pushing the constructed plugin into
pluginsStore.servers[pluginsStore.openServer!].plugins, add a duplicate guard
that checks the target server's plugins array for an existing entry with the
same id (and type if relevant) — e.g., find/ some on
pluginsStore.servers[pluginsStore.openServer!].plugins comparing plugin.id (and
plugin.type) and return early if found; this ensures the block in the onClick$
handler that builds the plugin (using resolvedPlugin, plugin, pluginsStore,
openServer) does not insert duplicates.
- Line 318: The handler currently uses "return" inside the bulk-download loop
when a plugin is not updatable, which aborts processing of all remaining
plugins; replace that "return" with "continue" so the loop skips the
non-updatable plugin and proceeds to the next one (change the check that uses
updateAvailable and plugin.data?.file?.url inside the loop to use continue).
Locate the loop that iterates plugins (where the condition "if (!updateAvailable
|| !plugin.data?.file?.url) return;" appears) and update that statement to
continue the loop instead of returning from the handler.
- Around line 121-124: The persistence task inside useTask$ is logging
Date.now() on every tracked mutation (via deepTrack(track, pluginsStore)), which
spams consoles; remove the console.log(Date.now()) call from the persistence
task in src/routes/resources/plugins/index.tsx (the useTask$ block referencing
deepTrack and pluginsStore), or wrap it behind an environment/dev-only guard so
it runs only in development builds.
- Line 14: Replace the hard-coded debug flag so debug UI cannot be shipped
enabled: remove or stop exporting the const debug = true and instead derive
debug from an environment/build flag (e.g. check process.env.NODE_ENV !==
'production' or a dedicated env var like REACT_APP_DEBUG === 'true' or
import.meta.env.MODE) where the default for production is false; update any
consumers of the debug symbol in this module (references to debug) to use the
new env-driven boolean so production builds disable debug-only UI by default.
- Around line 265-273: The current onInput$ handler on the input with id
"import" repeatedly parses and pushes parsed PluginWithData into
pluginsStore.servers[pluginsStore.openServer!].plugins on every keystroke
causing duplicates; change this to a single-fire handler (e.g., onBlur$ or
onPaste$) or move logic to a submit button handler so parsing only happens once,
and when adding use a dedupe/replace strategy (compare unique plugin id or name)
before pushing into pluginsStore.servers[pluginsStore.openServer!].plugins to
avoid duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc7c683a-d17c-4ef3-910e-de913cc74ef7
📒 Files selected for processing (3)
src/components/plugins/AddModrinthDialog.tsxsrc/routes/resources/flags/index.tsxsrc/routes/resources/plugins/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/resources/flags/index.tsx
Comment on lines
+70
to
+73
| const serverDefaults: ServerType = { | ||
| software: 'paper', | ||
| plugins: [], | ||
| }; |
Contributor
There was a problem hiding this comment.
Avoid reusing one mutable serverDefaults object for all servers.
Line 201 stores the same object reference for each new server, so plugins can leak across servers.
🔧 Proposed fix
-const serverDefaults: ServerType = {
- software: 'paper',
- plugins: [],
-};
+const createServerDefaults = (): ServerType => ({
+ software: 'paper',
+ plugins: [],
+});
...
-pluginsStore.servers[serverName] = serverDefaults;
+pluginsStore.servers[serverName] = createServerDefaults();Also applies to: 201-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/resources/plugins/index.tsx` around lines 70 - 73, serverDefaults
is a single mutable object (type ServerType) being reused for every new server,
causing the plugins array to be shared across instances; when you create a new
server (where the code currently assigns serverDefaults to a new entry around
the block that stores servers), create a fresh object instead of reusing
serverDefaults — e.g., shallow-clone serverDefaults with an object spread and
also clone the plugins array (or explicitly set plugins: []) so each server gets
its own plugins array; update the creation site that references serverDefaults
to use this cloned object.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes