feat: Client side js mods. Modding! #255
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
Warning Rate limit exceeded@zardoy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis set of changes introduces comprehensive support for client and server mods within the application. It adds a new module for managing client mods and repositories, including installation, activation, update checks, and error handling. UI components and styles for displaying and interacting with mods are implemented, including a modal page for mod management and integration into the main options interface. Server-side plugin selection is added to the world creation flow, and the build process is updated to include a configuration HTML file. Additionally, hooks are provided for mods to interact with the graphics backend, and several configuration and storage options are extended to support mod-related features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (ModsPage)
participant ClientMods Module
participant IndexedDB
participant Repository
participant Mod Script
User->>UI (ModsPage): Open mods modal
UI (ModsPage)->>ClientMods Module: getAllModsDisplayList()
ClientMods Module->>IndexedDB: Fetch mods and repositories
IndexedDB-->>ClientMods Module: Return mods/repositories
ClientMods Module-->>UI (ModsPage): List of mods
User->>UI (ModsPage): Click "Install" on a mod
UI (ModsPage)->>ClientMods Module: installModByName(repoUrl, name)
ClientMods Module->>Repository: Fetch mod files
Repository-->>ClientMods Module: Mod data
ClientMods Module->>IndexedDB: Save mod
ClientMods Module->>Mod Script: Dynamically load and activate mod
Mod Script-->>ClientMods Module: Activation complete
ClientMods Module-->>UI (ModsPage): Update mod list
User->>UI (ModsPage): Enable/disable mod
UI (ModsPage)->>ClientMods Module: setEnabledModAction(name, enabled)
ClientMods Module->>IndexedDB: Update mod state
ClientMods Module-->>UI (ModsPage): Update mod list
sequenceDiagram
participant User
participant CreateWorld UI
participant ClientMods Module
participant FileSystem
User->>CreateWorld UI: Click "Use Mods"
CreateWorld UI->>ClientMods Module: getAvailableServerPlugins()
ClientMods Module-->>CreateWorld UI: List of server plugins
User->>CreateWorld UI: Select plugins and click "Create"
CreateWorld UI->>ClientMods Module: getServerPlugin(plugin)
ClientMods Module-->>CreateWorld UI: Plugin content
CreateWorld UI->>FileSystem: Write plugin file to world folder
FileSystem-->>CreateWorld UI: File written
Assessment against linked issues
Suggested labels
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/index.ts (1)
1057-1057: Add error handling for mod initialization.The
appStartup()function is called to initialize the client mods system, but errors aren't handled. Since this runs during application startup, uncaught exceptions could prevent the application from loading properly.-void appStartup() +void appStartup().catch(error => { + console.error("Failed to initialize client mods:", error) + showNotification("Mod System Error", "Failed to initialize the mod system. Some mods may not work properly.", true) +})assets/config.html (1)
23-23: Consider adding a label/placeholder for the input field.
A placeholder or label would improve clarity for what the user should input.src/optionsGuiScheme.tsx (1)
214-219: Consider using reactive state to track loaded mods.
Referencingwindow.loadedModsworks, but if you want the button label to update automatically when mods change, movingloadedModsinto a reactive store (similar tomodsUpdateStatus) could help.src/clientMods.ts (1)
53-56: Avoid using thedeleteoperator for performance reasons.
Per lint suggestions, assigningundefinedmay reduce the risk of de-optimizations. If removing these properties is essential, continue usingdelete, but be aware of potential performance implications.🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 54-54: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
assets/config.html(1 hunks)rsbuild.config.ts(1 hunks)src/clientMods.ts(1 hunks)src/index.ts(2 hunks)src/optionsGuiScheme.tsx(2 hunks)src/react/ModsPage.tsx(1 hunks)src/reactUi.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
rsbuild.config.ts (1)
scripts/build.js (1) (1)
fs(5-5)
src/optionsGuiScheme.tsx (2)
src/react/SelectOption.tsx (1) (1)
useSnapshot(89-176)src/clientMods.ts (1) (1)
modsUpdateStatus(162-162)
src/index.ts (1)
src/clientMods.ts (1) (1)
appStartup(152-160)
src/clientMods.ts (1)
src/optionsStorage.ts (1) (1)
options(186-191)
🪛 Biome (1.9.4)
src/clientMods.ts
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 54-54: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: build-and-deploy
src/clientMods.ts
[failure] 247-247:
Property 'modsUpdatePeriodCheck' does not exist on type '{ renderDistance: number; keepChunksDistance: number; multiplayerRenderDistance: number; closeConfirmation: boolean; autoFullScreen: boolean; mouseRawInput: boolean; autoExitFullscreen: boolean; ... 78 more ...; highlightBlockColor: "auto" | "blue" | "classic"; }'.
[failure] 245-245:
Property 'modsAutoUpdate' does not exist on type '{ renderDistance: number; keepChunksDistance: number; multiplayerRenderDistance: number; closeConfirmation: boolean; autoFullScreen: boolean; mouseRawInput: boolean; autoExitFullscreen: boolean; ... 78 more ...; highlightBlockColor: "auto" | "blue" | "classic"; }'.
[failure] 202-202:
Property 'modsAutoUpdate' does not exist on type '{ renderDistance: number; keepChunksDistance: number; multiplayerRenderDistance: number; closeConfirmation: boolean; autoFullScreen: boolean; mouseRawInput: boolean; autoExitFullscreen: boolean; ... 78 more ...; highlightBlockColor: "auto" | "blue" | "classic"; }'.
[failure] 6-6:
Cannot find module './react/storageProvider' or its corresponding type declarations.
🪛 GitHub Actions: CI
src/clientMods.ts
[error] 6-6: Cannot find module './react/storageProvider' or its corresponding type declarations.
🔇 Additional comments (3)
rsbuild.config.ts (1)
176-176: Enhance build process to include config.html file.The addition correctly copies the config.html file from assets to the distribution directory, making it available for the client-side modding system. This follows the same pattern as other asset files.
src/index.ts (1)
100-100: New import for client mod functionality.The import adds the client mod module to the application, which implements the modding system mentioned in the PR description.
src/optionsGuiScheme.tsx (1)
17-17: Import statement looks fine.
No concerns found with this new import ofmodsUpdateStatus.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
src/screens.css (2)
16-21: Establishing Stacking Context for the Backdrop Element
The addition ofz-index: 12;to the.backdropclass correctly establishes a new stacking context, ensuring that modal or overlay elements relying on this style will appear above base content. However, note that the.fullscreenclass also usesz-index: 12; if the intent is to layer these elements distinctly (for example, to have the fullscreen modal appear above the backdrop), you might consider assigning a higher z-index value to one of them to avoid potential overlay conflicts.
16-19: Viewport Height Unit Consideration
The use ofheight: 100dvh;ensures the backdrop covers the dynamic viewport height, which is beneficial for handling mobile browser UI changes. However, consider potential compatibility issues with browsers that do not fully support thedvhunit. Including a fallback such as100vhmight improve robustness across all target browsers.src/react/ModsPage.tsx (3)
122-129: Consider using a more stable identifier than array index.The useMemo for allModsArray is well-implemented, but selecting mods by index could lead to issues if the array order changes (e.g., when filtering or sorting).
- const [selectedModIndex, setSelectedModIndex] = useState<number | null>(null) + const [selectedModName, setSelectedModName] = useState<string | null>(null) - const selectedMod = selectedModIndex === null ? null : allModsArray[selectedModIndex] + const selectedMod = selectedModName === null ? null : allModsArray.find(mod => mod.name === selectedModName) // And then update the onClick handlers: - onClick={() => setSelectedModIndex(allModsArray.findIndex(m => m.name === mod.name))} + onClick={() => setSelectedModName(mod.name)}
130-140: Good reactive data fetching, but consider adding loading state.The useEffect for fetching mods data when the modal is active is well implemented, but there's no explicit loading state handling.
Consider adding a loading state and displaying a loading indicator:
const [modsData, setModsData] = useState<ModsData | null>(null) + const [isLoading, setIsLoading] = useState(false) useEffect(() => { if (isModalActive) { + setIsLoading(true) void getAllModsDisplayList().then(mods => { setModsData(mods) // Update selected mod index if needed if (selectedModIndex !== null && selectedModIndex < allModsArray.length) { setSelectedModIndex(selectedModIndex) } + setIsLoading(false) + }).catch(error => { + console.error('Failed to load mods:', error) + setIsLoading(false) }) } }, [isModalActive, counter])
179-304: Well-structured UI with responsive layout, but consider adding pagination.The Screen component is well utilized with appropriate styling. The UI is organized into clear sections with responsive layout handling. However, for repositories with many mods, consider adding pagination.
You could implement pagination for large mod lists to improve performance and UI clarity:
// Add pagination state + const [pageSize, setPageSize] = useState(10) + const [currentPage, setCurrentPage] = useState(1) // Modify filteredMods to include pagination const filteredMods = modsData ? { repos: modsData.repos.map(repo => ({ ...repo, - packages: repo.packages.filter(modFilter) + packages: repo.packages.filter(modFilter) })), modsWithoutRepos: modsData.modsWithoutRepos.filter(modFilter) } : null // Add pagination controls at the bottom of the modList div <div className={styles.modList}> {/* Existing code */} + {filteredModsCount > pageSize && ( + <div className={styles.pagination}> + <Button + onClick={() => setCurrentPage(p => Math.max(1, p - 1))} + disabled={currentPage === 1} + title="Previous Page" + /> + <span>Page {currentPage} of {Math.ceil(filteredModsCount / pageSize)}</span> + <Button + onClick={() => setCurrentPage(p => Math.min(Math.ceil(filteredModsCount / pageSize), p + 1))} + disabled={currentPage === Math.ceil(filteredModsCount / pageSize)} + title="Next Page" + /> + </div> + )} </div>src/clientMods.ts (8)
15-15: Fix spelling of “sensetiveKeys”.
You have a minor typographical error in the variable name: consider renaming “sensetiveKeys” to “sensitiveKeys” for clarity.- const sensetiveKeys = new Set(['authenticatedAccounts', 'serversList', 'username']) + const sensitiveKeys = new Set(['authenticatedAccounts', 'serversList', 'username'])
121-127: Avoid performance overhead from usingdelete.
Using thedeleteoperator repeatedly can degrade object shape optimizations in some JS engines. Setting properties toundefinedcan be more performant.const cleanupFetchedModData = (mod: ClientModDefinition | Record<string, any>) => { - delete mod['enabled'] - delete mod['repo'] - delete mod['autoUpdateOverride'] - delete mod['lastUpdated'] + mod.enabled = undefined + mod.repo = undefined + mod.autoUpdateOverride = undefined + mod.lastUpdated = undefined return mod }🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 123-123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
218-229: Remove the useless catch block.
The catch block rethrows the original error without additional logic, making it redundant. Simplify by removing the try/catch here.// eslint-disable-next-line no-useless-catch try { const module = await import(/* webpackIgnore: true */ url) module.default?.(structuredClone(mod)) window.loadedMods[mod.name] = module - } catch (e) { - // ... - throw e + } catch (error) { + console.error(`Error loading mod ${mod.name}:`, error) + throw error }🧰 Tools
🪛 Biome (1.9.4)
[error] 227-227: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
273-276: Remove redundant catch block rethrow.
This try/catch is simply rethrowing the error. Consider removing it or adding meaningful handling if needed.try { const response = await fetch(url) if (!response.ok) throw new Error(`Failed to fetch ${url}: ${response.status} ${response.statusText}`) return await response.text() -} catch (e) { - throw e }🧰 Tools
🪛 Biome (1.9.4)
[error] 275-275: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
306-307: Drop the useless catch clause.
Again, this catch only rethrows the same error. Remove it unless you plan to provide additional error handling.try { // ... } catch (e) { - // console.error(`Error installing mod ${mod.name}:`, e) - throw e }🧰 Tools
🪛 Biome (1.9.4)
[error] 307-307: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
436-436: Correct the UI message.
There is a typo in the'won\' be automatically removed'text. Fix it to “won’t” or “will not” for clarity.- const choice = await showOptionsModal('Remove repository? Installed mods won\' be automatically removed.', ['Yes']) + const choice = await showOptionsModal('Remove repository? Installed mods won\'t be automatically removed.', ['Yes'])
406-407: Offer help with the missing deactivation logic.
You have atodocomment about deactivating the mod. Let me know if you want assistance implementing a proper teardown for disabled mods.
1-462: Consider adding tests for this new module.
This file manages crucial functionality (storage, activation, updates, deletion). Automated tests would help ensure stability and maintainability.🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 123-123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 227-227: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 275-275: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 307-307: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/clientMods.ts(1 hunks)src/core/progressReporter.ts(2 hunks)src/index.ts(4 hunks)src/optionsGuiScheme.tsx(2 hunks)src/optionsStorage.ts(1 hunks)src/react/Input.tsx(1 hunks)src/react/ModsPage.tsx(1 hunks)src/react/Screen.tsx(1 hunks)src/react/SelectOption.tsx(2 hunks)src/react/appStorageProvider.ts(3 hunks)src/react/mods.module.css(1 hunks)src/react/mods.module.css.d.ts(1 hunks)src/reactUi.tsx(2 hunks)src/screens.css(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/react/mods.module.css.d.ts
- src/react/mods.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- src/optionsGuiScheme.tsx
- src/reactUi.tsx
🧰 Additional context used
🧬 Code Definitions (2)
src/core/progressReporter.ts (1)
src/react/NotificationProvider.tsx (1) (1)
showNotification(22-37)
src/clientMods.ts (4)
src/core/progressReporter.ts (1) (1)
ProgressReporter(6-19)src/optionsStorage.ts (1) (1)
options(189-194)src/react/appStorageProvider.ts (1) (1)
appStorage(88-88)src/react/SelectOption.tsx (2) (2)
showOptionsModal(24-49)showInputsModal(57-88)
🪛 Biome (1.9.4)
src/clientMods.ts
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 123-123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 227-227: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 275-275: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 307-307: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (16)
src/core/progressReporter.ts (2)
4-4: Added new icon import for notification enhancements.The import of
pixelartIconsfrom the PixelartIcon module is now used to display visual feedback in notifications, improving the user interface.
173-173: Enhanced notification with visual feedback.Adding the checkmark icon (
pixelartIcons.check) to the end notification improves the user experience by providing a clear visual indicator of successful completion.src/react/Input.tsx (1)
45-45: Input component now supports custom class names.The updated implementation correctly concatenates the base input styles with any additional class names passed as props, enhancing component flexibility and allowing for custom styling in the mod management UI.
src/react/SelectOption.tsx (2)
55-55: Added placeholder support to input options.The addition of an optional
placeholderproperty to theInputOptiontype extends the component's functionality for input fields, improving user guidance.
134-134: Implemented placeholder support in text inputs.The placeholder property is now correctly passed to the InputWithLabel component, providing visual hints to users when interacting with form fields in the modding interface.
src/index.ts (3)
100-100: Added client mods system import.The import of
appStartupfrom the clientMods module integrates the new modding system into the application.
750-750: Improved loading message clarity.Using
progress.setMessageinstead ofsetLoadingScreenStatusprovides more consistent user feedback during world loading and block placement.Also applies to: 782-782
1057-1057: Integrated client mods initialization.The addition of
appStartup()to the application startup sequence initializes the client-side modding system, ensuring mods are loaded when the application starts.This aligns with the PR objective of implementing a client-side modding system, enabling the mod management capabilities described in the PR summary.
src/optionsStorage.ts (1)
65-67: Well-structured configuration for mod system.The additions to defaultOptions correctly implement the necessary configuration settings for the client-side modding system. The default values are appropriate - modding is disabled by default, update checks are enabled, and the check period is set to 24 hours.
src/react/appStorageProvider.ts (3)
10-10: Good refactoring for localStorage access.Extracting localStorage from window makes the code more consistent and readable.
41-41: Type definition properly added for mod update tracking.The addition of
modsAutoUpdateLastCheckto the StorageData type interface is appropriate for tracking when mod updates were last checked.
81-81: Default value correctly set for new storage property.Setting the default value to undefined for the new storage property is appropriate as it indicates that no updates have been checked yet.
src/react/Screen.tsx (1)
9-9: Flexible enhancement to Screen component.The addition of the contentStyle prop improves the flexibility of the Screen component, allowing for more customized styling. The implementation correctly merges the new style with existing conditional styling.
Also applies to: 12-12, 17-17
src/react/ModsPage.tsx (2)
1-16: Good component structure and type definitions.The imports are well-organized and the ModsData type is properly defined using the return type of getAllModsDisplayList for type safety.
17-108: Well-implemented ModSidebar component with appropriate error handling.The ModSidebar component is well structured with appropriate conditional rendering and error handling. The handleAction function properly manages async operations and displays error messages.
src/clientMods.ts (1)
317-317: EnsuremodsAutoUpdate&modsUpdatePeriodCheckexist inAppOptions.
References tooptions.modsAutoUpdateandoptions.modsUpdatePeriodCheckmay cause type errors if these properties aren’t defined in theAppOptionsinterface.Also applies to: 367-367
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-qp65go3oj-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
src/react/CreateWorldProvider.tsx (1)
30-43: Implementation of plugin installation for new worldsThis code now extracts plugins from the creation state and processes each plugin by:
- Fetching the plugin content and version
- Creating a plugins directory in the world save path
- Writing the plugin content to a file with a versioned filename
The implementation correctly handles potential null returns from
getServerPluginand uses proper async/await patterns.Consider adding error handling for the file operations to inform the user if a plugin installation fails:
for (const plugin of plugins) { // eslint-disable-next-line no-await-in-loop const { content, version } = await getServerPlugin(plugin) ?? {} if (content) { // eslint-disable-next-line no-await-in-loop - await mkdirRecursive(path.join(savePath, 'plugins')) - // eslint-disable-next-line no-await-in-loop - await fs.promises.writeFile(path.join(savePath, 'plugins', `${plugin}-${version}.js`), content) + try { + // eslint-disable-next-line no-await-in-loop + await mkdirRecursive(path.join(savePath, 'plugins')) + // eslint-disable-next-line no-await-in-loop + await fs.promises.writeFile(path.join(savePath, 'plugins', `${plugin}-${version}.js`), content) + } catch (error) { + console.error(`Failed to install plugin ${plugin}:`, error) + // Could show a toast notification here if you have that functionality + } } }src/react/CreateWorld.tsx (1)
79-109: Added "Use Mods" functionality to world creationImplemented a new button and functionality that allows users to:
- View and select from available server plugins (mods)
- See the count of selected plugins
- Access the full mods page via an "Install More" button
- Apply selected plugins to the world being created
The implementation includes a disabled "Save Type: Java" button for future functionality.
Consider adding a loading state to the "Use Mods" button for better UX:
<Button onClick={async () => { + const [isLoading, setIsLoading] = useState(false) + setIsLoading(true) const availableServerPlugins = await getAvailableServerPlugins() + setIsLoading(false) const availableModNames = availableServerPlugins.map(mod => mod.name) // ...rest of the implementation }} ->Use Mods ({plugins.length}) +>{isLoading ? 'Loading...' : `Use Mods (${plugins.length})`} </Button>Also, it would be helpful to show a message when no mods are available:
const availableServerPlugins = await getAvailableServerPlugins() +if (availableServerPlugins.length === 0) { + showOptionsModal('No mods available', [ + { label: 'Install Mods', value: 'install', onClick: () => showModal({ reactType: 'mods' }) }, + { label: 'Cancel', value: 'cancel' } + ]) + return +} const availableModNames = availableServerPlugins.map(mod => mod.name)src/react/ModsPage.tsx (2)
30-31: Clarify thedata-enabledlogic.Currently,
data-enabledis assigned an empty string if a mod is installed and the value ofmod.activatedotherwise. Ensure this reflects the intended enabled state (e.g., installed vs. activated) and confirm that your stylesheet selectors handle the resulting attributes properly.
235-239: Centralize stats calculation logic.The code in
getStatsText()branches onshowOnlyEnabledandshowOnlyInstalled, each building a separate count. Consider extracting these conditions into a helper function or a single calculation pass, which could help improve readability and maintainability.src/react/mods.module.css (2)
17-27: Remove duplicated.statsRowdefinitions.
.statsRowis declared twice (lines 17–21 and 23–27) with identical properties. Consider merging them into a single declaration to eliminate duplication.
1-178: Consider extracting recurring color values into variables or a theme file.Multiple color codes (
#999,#bcbcbc,#ff6b6b, etc.) are scattered throughout. Centralizing them can simplify future theming or changes to the color palette.src/clientMods.ts (5)
127-130: Avoid usingdeletefor performance-sensitive code.The repeated use of
deletecan impact performance according to JS engine implementations. Consider setting properties toundefinedor refactoring these keys to be omitted from the object at creation time.- delete mod['enabled'] - delete mod['repo'] - delete mod['autoUpdateOverride'] - delete mod['lastUpdated'] + mod['enabled'] = undefined + mod['repo'] = undefined + mod['autoUpdateOverride'] = undefined + mod['lastUpdated'] = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 128-128: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 129-129: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 130-130: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
229-235: Remove or integrate this try/catch that rethrows the same error.This catch block simply rethrows the error, rendering it effectively redundant and possibly confusing to readers. Eliminate it to simplify the flow, or include additional error handling logic if needed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 233-233: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
279-282: Simplify the catch clause.Similar to the previous comment, this catch block rethrows the same error. Consider removing the catch entirely or adding more meaningful error handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 281-281: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
323-324: Eliminate the no-op catch block.This catch clause only rethrows the error; remove or expand it if you need actual fallback or logging here.
🧰 Tools
🪛 Biome (1.9.4)
[error] 324-324: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
404-407: Improve mod deactivation logic.Current uninstallation logic deletes the loaded mod from
window.loadedMods, but additional cleanup code (e.g., reversing injected styles or calling a proper deactivation method) remains partially commented. Consider finalizing mod teardown to avoid stale references or partial state in memory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.json(1 hunks)src/clientMods.ts(1 hunks)src/defaultLocalServerOptions.js(1 hunks)src/react/CreateWorld.tsx(4 hunks)src/react/CreateWorldProvider.tsx(3 hunks)src/react/ModsPage.tsx(1 hunks)src/react/SelectOption.tsx(3 hunks)src/react/appStorageProvider.ts(3 hunks)src/react/mods.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/react/SelectOption.tsx
- src/react/appStorageProvider.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/react/CreateWorldProvider.tsx (2)
src/react/CreateWorld.tsx (1) (1)
creatingWorldState(17-23)src/clientMods.ts (1) (1)
getServerPlugin(483-493)
src/clientMods.ts (4)
src/core/progressReporter.ts (1) (1)
ProgressReporter(6-19)src/optionsStorage.ts (1) (1)
options(189-194)src/react/appStorageProvider.ts (1) (1)
appStorage(90-90)src/react/SelectOption.tsx (2) (2)
showOptionsModal(24-49)showInputsModal(58-89)
🪛 Biome (1.9.4)
src/clientMods.ts
[error] 127-127: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 128-128: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 129-129: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 130-130: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 233-233: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 281-281: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 324-324: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (10)
src/defaultLocalServerOptions.js (1)
11-11: New plugin folder configuration addedA new configuration option
pluginsFolderhas been added with a default value oftrue. This aligns well with the modding system implementation and will enable plugin functionality by default.package.json (1)
78-78:❓ Verification inconclusive
Updated flying-squid dependency
The flying-squid dependency has been updated from version
^0.0.51to^0.0.57. This update likely includes fixes or features required for the new modding system.
🏁 Script executed:
#!/bin/bash # Check for changes between the two versions of flying-squid that might be relevant to plugin support # Clone the repo to inspect changes git clone https://github.com/zardoy/flying-squid.git temp-flying-squid cd temp-flying-squid # Check commits between v0.0.51 and v0.0.57 echo "Changes between v0.0.51 and v0.0.57:" git log --oneline v0.0.51..v0.0.57 | grep -i "plugin\|mod"Length of output: 276
Updated flying-squid dependency
The dependency in
package.jsonhas been updated from version^0.0.51to^0.0.57. A search in the upstream commit history (using keywords "plugin" and "mod") returned no explicit references to modding support changes between these versions. This could mean the improvements are internal or not documented via commit messages. It is recommended to review the release notes or upstream documentation to confirm that the new version indeed provides the necessary fixes or features for the modding system.src/react/CreateWorldProvider.tsx (2)
1-7: Import updates to support plugin functionalityThe addition of
fs,path, andgetServerPluginimports support the new plugin system implementation. This allows the component to handle file operations and retrieve plugin information from the modding system.
20-20: Improved version label for clarityChanged version label format to simply show "(default)" instead of an offline availability message, making it clearer to users which version is the default.
src/react/CreateWorld.tsx (5)
4-5: New imports to support mod functionalityAdded imports for mod-related functionality:
getAvailableServerPluginsto fetch available server pluginsshowModalto display the mods modal- UI components for selection options
These imports properly support the new modding features being added.
Also applies to: 11-11
17-23: Added plugins array to world creation stateThe
creatingWorldStateproxy has been extended with apluginsproperty initialized as an empty array. This properly enables tracking selected plugins during world creation.
28-28: Updated state snapshot to include pluginsModified the state destructuring to include the new
pluginsproperty, ensuring it's properly tracked in the component.
76-77: Improved Game Mode label formattingChanged "Gamemode" to "Game Mode" for better readability and consistency with standard terminology.
118-122: Enhanced create button visibilityThe "Create" button text is now bold, making it more prominent as the primary action. This is a good UI improvement.
src/react/ModsPage.tsx (1)
211-217: Confirm the need for multiple filter conditions.The filter leverages
showOnlyInstalledandshowOnlyEnabledin combination with the search text. Double-check that this logic matches your intended user experience (e.g., to ensure that installed and enabled filters don't conflict). This is a subtle area where logic mistakes can easily creep in.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
assets/config.html (3)
15-26: FunctionremoveAllData: Consider a DRY Approach
The function correctly removes several specific items from localStorage. For better maintainability and clarity, consider iterating over an array of keys instead of making multipleremoveItemcalls. For example:- localStorage.removeItem('serversList') - localStorage.removeItem('serversHistory') - localStorage.removeItem('authenticatedAccounts') - localStorage.removeItem('modsAutoUpdateLastCheck') - localStorage.removeItem('firstModsPageVisit') - localStorage.removeItem('proxiesData') - localStorage.removeItem('keybindings') - localStorage.removeItem('username') - localStorage.removeItem('customCommands') - localStorage.removeItem('options') + ['serversList', 'serversHistory', 'authenticatedAccounts', 'modsAutoUpdateLastCheck', 'firstModsPageVisit', 'proxiesData', 'keybindings', 'username', 'customCommands', 'options'] + .forEach(key => localStorage.removeItem(key));This refactor minimizes repetition and makes future changes easier.
29-36: User Interface Layout: Maintainability and Cleanup
The buttons are correctly grouped within a flex container to ensure spacing and alignment. As a future enhancement, consider moving the inline CSS (display: flex; gap: 10px;) to a dedicated CSS file or<style>block for better maintainability. Additionally, if the commented-out buttons (lines 33-35) are not planned for immediate use, they could be removed to reduce clutter.
37-37: Input Field: Clarification Needed
The file contains an empty<input />element without any label or placeholder. If this input is meant for future functionality, consider adding a placeholder text or appropriate labeling; otherwise, remove it to avoid confusing users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/config.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (3)
assets/config.html (3)
1-6: HTML Structure is Solid and Compliant
The document includes a proper<!DOCTYPE html>declaration, sets the language attribute, and defines essential meta tags along with the title. No issues found.
7-13: FunctionremoveSettings: Clear and Integrated
The function is implemented correctly by prompting the user with a confirmation dialog, resetting theoptionskey in localStorage, and reloading the page. This ensures that users apply the changes immediately upon reset.
38-39: Proper Closure of HTML Document
The closing tags for<body>and<html>are correctly in place, ensuring proper document structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/clientMods.ts (2)
132-135: Consider avoiding the delete operator for potential performance gains.Certain JavaScript engines deoptimize objects when properties are deleted. If performance is critical in your use case, assign
undefinedinstead.Here’s a possible replacement:
- delete mod['enabled'] - delete mod['repo'] - delete mod['autoUpdateOverride'] - delete mod['lastUpdated'] + mod.enabled = undefined + mod.repo = undefined + mod.autoUpdateOverride = undefined + mod.lastUpdated = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 132-132: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
239-240: Remove catch blocks that only rethrow the original error.These catch clauses provide no additional error handling, and can be removed to simplify the code.
Here’s a quick sample diff for removing the redundant catch:
- try { - ... - } catch (e) { - throw e - }Also applies to: 250-251, 298-299, 349-350
🧰 Tools
🪛 Biome (1.9.4)
[error] 239-239: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clientMods.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/clientMods.ts
[error] 132-132: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 239-239: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 250-250: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 298-298: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 349-349: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (3)
src/clientMods.ts (3)
13-79: Good approach to protect sensitive localStorage keys.Wrapping
localStoragein a proxy is an effective client-side measure to guard sensitive properties. This should mitigate accidental or malicious usage of these keys.
9-9:✅ Verification successful
Verify import correctness to avoid pipeline errors.
A previous review flagged a missing
storageProviderimport; this file references./react/appStorageProviderinstead, which may resolve the issue. However, please confirm that./react/appStorageProviderexists and is correctly spelled to avoid potential build failures.Use this script to confirm the file is present:
🏁 Script executed:
#!/bin/bash # Description: Verify that appStorageProvider is present in the `react` folder. fd appStorageProvider --extension ts --extension tsxLength of output: 86
Import Verification Confirmed
The file
src/react/appStorageProvider.tswas located, confirming that the import is valid and correctly spelled. No further changes are needed to address this comment.
359-359:❓ Verification inconclusive
Ensure that options.modsAutoUpdate is declared in your options interface.
Past pipeline failures indicated that
modsAutoUpdateandmodsUpdatePeriodCheckmight not be defined inAppOptions. Confirm they’re declared to avoid type errors.
🏁 Script executed:
#!/bin/bash # Searching if 'modsAutoUpdate' and 'modsUpdatePeriodCheck' are declared in the 'AppOptions' interface within 'optionsStorage.ts'. ast-grep --pattern $'interface AppOptions {\n $$$\n modsAutoUpdate?: $_\n modsUpdatePeriodCheck?: $_\n $$$\n}'Length of output: 110
Action Required: Verify declaration of modsAutoUpdate and modsUpdatePeriodCheck in AppOptions
- Our initial AST query did not return any matches for these properties in the AppOptions interface (likely defined in optionsStorage.ts).
- Please manually verify that both
modsAutoUpdateandmodsUpdatePeriodCheckare declared in AppOptions. This is critical since they're referenced insrc/clientMods.ts(line 359) and missing declarations could lead to type errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/clientMods.ts (2)
132-135: Avoiddeleteoperator for performance reasons.Repeated use of the
deleteoperator can cause performance issues in JavaScript engines. Instead, consider setting these properties toundefinedto improve performance and maintain object shape.- delete mod['enabled'] - delete mod['repo'] - delete mod['autoUpdateOverride'] - delete mod['lastUpdated'] + mod.enabled = undefined + mod.repo = undefined + mod.autoUpdateOverride = undefined + mod.lastUpdated = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 132-132: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
239-241: Remove or refactor useless catch clauses.These
try/catchblocks merely rethrow the original error and add no value. Consider removing them or adding meaningful error-handling logic if needed.Also applies to: 247-249, 296-303, 351-353
🧰 Tools
🪛 Biome (1.9.4)
[error] 239-239: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
renderer/viewer/three/graphicsBackend.ts (2)
51-52: Confirm safe usage ofcallModsMethod.The function iterates over
window.loadedModsto call arbitrary methods. Ensure your mods code is stable and won't break the rendering lifecycle if a method fails or is not defined. Consider adding error handling or event-based architecture for better decoupling.Also applies to: 72-72, 116-116
114-116: Track potential global usage pitfalls.Storing references on
globalThis(e.g.,threeJsBackendandresourcesManager) can lead to unintended namespace collisions in large applications. If feasible, encapsulate these references within your module or a dedicated context object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.json(1 hunks)renderer/viewer/three/graphicsBackend.ts(4 hunks)src/clientMods.ts(1 hunks)src/core/progressReporter.ts(2 hunks)src/index.ts(3 hunks)src/optionsGuiScheme.tsx(2 hunks)src/optionsStorage.ts(1 hunks)src/react/appStorageProvider.ts(3 hunks)src/reactUi.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/core/progressReporter.ts
- package.json
- src/optionsGuiScheme.tsx
- src/optionsStorage.ts
- src/index.ts
- src/react/appStorageProvider.ts
- src/reactUi.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/clientMods.ts
[error] 132-132: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 239-239: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 253-253: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 301-301: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 352-352: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (2)
src/clientMods.ts (2)
362-362: ** AddmodsAutoUpdateandmodsUpdatePeriodChecktoAppOptions.**References to
options.modsAutoUpdateandoptions.modsUpdatePeriodCheckappear here, but ensure they are defined in theAppOptionsinterface (inoptionsStorage.tsor a similar file). If already defined, ignore this comment.Also applies to: 410-412
13-79: Caution with global localStorage proxy.The
protectRuntime()function replaceswindow.localStoragewith a proxy. This has broad impact across the application. Verify that no dependencies rely on direct localStorage behavior and confirm this won't introduce unexpected side effects.
| const installOrUpdateMod = async (repo: Repository, mod: ClientModDefinition, activate = true, progress?: ProgressReporter) => { | ||
| // eslint-disable-next-line no-useless-catch | ||
| try { | ||
| const fetchData = async (urls: string[]) => { | ||
| const errored = [] as string[] | ||
| // eslint-disable-next-line no-unreachable-loop | ||
| for (const urlTemplate of urls) { | ||
| const modNameOnly = mod.name.split('.').pop() | ||
| const modFolder = repo.prefix === false ? modNameOnly : typeof repo.prefix === 'string' ? `${repo.prefix}/${modNameOnly}` : mod.name | ||
| const url = new URL(`${modFolder}/${urlTemplate}`, normalizeRepoUrl(repo.url).replace(/\/$/, '') + '/').href | ||
| // eslint-disable-next-line no-useless-catch | ||
| try { | ||
| const response = await fetch(url) | ||
| if (!response.ok) throw new Error(`Failed to fetch ${url}: ${response.status} ${response.statusText}`) | ||
| return await response.text() | ||
| } catch (e) { | ||
| // errored.push(String(e)) | ||
| throw e | ||
| } | ||
| } | ||
| console.warn(`[${mod.name}] Error installing component of ${urls[0]}: ${errored.join(', ')}`) | ||
| return undefined | ||
| } | ||
| if (mod.stylesGlobal) { | ||
| await progress?.executeWithMessage( | ||
| `Downloading ${mod.name} styles`, | ||
| async () => { | ||
| mod.stylesGlobal = await fetchData(['global.css']) as any | ||
| } | ||
| ) | ||
| } | ||
| if (mod.scriptMainUnstable) { | ||
| await progress?.executeWithMessage( | ||
| `Downloading ${mod.name} script`, | ||
| async () => { | ||
| mod.scriptMainUnstable = await fetchData(['mainUnstable.js']) as any | ||
| } | ||
| ) | ||
| } | ||
| if (mod.threeJsBackend) { | ||
| await progress?.executeWithMessage( | ||
| `Downloading ${mod.name} three.js backend`, | ||
| async () => { | ||
| mod.threeJsBackend = await fetchData(['three.js']) as any | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated fetch logic for improved maintainability.
The repeated blocks in installOrUpdateMod for downloading scripts and styles can be extracted into helper functions, reducing code duplication and improving readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 301-301: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
renderer/viewer/three/entity/EntityMesh.ts (1)
554-554: Exposing EntityMesh globally for mod compatibility.The addition of
window.EntityMesh = EntityMeshmakes the EntityMesh class globally accessible through the browser's window object. While this approach supports the modding system being introduced, be aware of the following considerations:
- It creates an implicit dependency that's not visible through imports
- It potentially pollutes the global namespace
- TypeScript type checking won't recognize this global declaration without appropriate
.d.tsfilesConsider adding a type declaration for better TypeScript support:
// Add to a global.d.ts file interface Window { EntityMesh: typeof import('./renderer/viewer/three/entity/EntityMesh').EntityMesh; }
|
so does it work? |
|
/deploy |
Yes all done |
|
Deployed to Vercel Preview: https://prismarine-4rimlvl69-zaro.vercel.app |
|
yo is there any way I could add an mod to it? |
Open pr to my repo (recommended) or use own repo |
what's that? |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/clientMods.ts (1)
298-305: Redundantcatch { throw e }blocks – remove for clearer stacks
installOrUpdateModcontains severaltry/catchclauses that immediately re‑throw (catch (e) { throw e }). They add no value and wrap the original stack with another layer of noise.Search & eliminate them to keep error traces intact.
Also applies to: 354-356
🧰 Tools
🪛 Biome (1.9.4)
[error] 304-304: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
🧹 Nitpick comments (8)
src/appViewerLoad.ts (2)
18-18: Promise fromloadBackendis silently discarded
loadBackendis nowasync, so any rejection will be swallowed when you cast it tovoid.
At minimum, add a.catch()handler (or explicitlyawaitin the few call‑sites that can afford to block) so that backend‑initialisation failures surface in the UI/console instead of becoming “lost” promises.- void appViewer.loadBackend(backend) + appViewer.loadBackend(backend).catch(err => { + console.error('[appViewerLoad] backend failed to load', err) + showNotification(`Backend failed to load: ${err.message}`, 'error') + })
30-32: Use an explicit delay (or a micro‑task) when deferring the call
setTimeout(() => { loadBackend() })defaults to0 ms, but browsers clamp the delay to ≥1 ms.
If the goal is just to yield to the event‑loop once,queueMicrotask(loadBackend)is cheaper; if you really want a macrotask, be explicit:- setTimeout(() => { - loadBackend() - }) + // Yield one tick so that critical UI is painted first + setTimeout(loadBackend, 1)src/appViewer.ts (1)
92-93:waitBackendLoadPromiseslifecycle needs safeguardsGreat idea to serialise backend loads, but consider:
- Promises are cleared immediately after an
await; if another piece of code appends while the await is in‑flight, you’ll miss it.- Nothing prevents external code from pushing non‑
Promise<void>values (runtime error later).A defensive approach is to copy & reset atomically and to validate items:
- await Promise.all(this.waitBackendLoadPromises) - this.waitBackendLoadPromises = [] + const pending = [...this.waitBackendLoadPromises] + this.waitBackendLoadPromises.length = 0 + await Promise.all(pending.map(p => Promise.resolve(p)))renderer/viewer/three/graphicsBackend.ts (1)
133-143: Don’t re‑throw insidecallModsMethod– it crashes the whole appThrowing after
showNotificationaborts rendering and prevents other mods from running:- showNotification(errorMessage, 'error') - throw new Error(errorMessage) + showNotification(errorMessage, 'error') + // Log & continue so that one bad mod does not break the rest + console.error(errorMessage)If you want to propagate failures, collect them and reject once all mods have been iterated, but avoid stopping the first time a mod misbehaves.
src/react/ModsPage.tsx (2)
28-34: Clickable<div>without keyboard semantics hurts accessibility
<div className={styles.modRow} onClick={...}>is focus‑inaccessible for keyboard users and screen‑reader users won’t know it’s interactive.
Wrap the row in a<button>/<li><button>or addrole="button"and keyboard handlers (onKeyDownforEnter/Space).
245-254:Esckey is captured but never closes the editor
handleKeyDownprevents default whene.key === 'Escape', yet it does not invokeonClose. Users press Esc expecting the dialog to exit.- if (e.key === 'Escape') { - e.preventDefault() - e.stopImmediatePropagation() - } + if (e.key === 'Escape') { + e.preventDefault() + e.stopImmediatePropagation() + onClose(undefined) // cancel editing + }This minor fix aligns behaviour with common UX conventions.
src/clientMods.ts (2)
17-18: Typo:sensetiveKeys→sensitiveKeysA tiny spelling fix improves code clarity and avoids future confusion.
- const sensetiveKeys = new Set(['authenticatedAccounts', 'serversList', 'username']) + const sensitiveKeys = new Set(['authenticatedAccounts', 'serversList', 'username'])(remember to update subsequent references).
74-78: Overridingwindow.localStoragemay break existing referencesReassigning the global
localStorageobject late in runtime means modules that captured the original reference (const ls = localStorage) earlier bypass the proxy and still access sensitive keys.Consider an earlier hook (e.g., before any other imports) or wrapping the dangerous methods (
getItem,setItem, …) instead of replacing the whole object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
renderer/viewer/three/graphicsBackend.ts(4 hunks)src/appConfig.ts(1 hunks)src/appViewer.ts(2 hunks)src/appViewerLoad.ts(2 hunks)src/clientMods.ts(1 hunks)src/index.ts(3 hunks)src/optionsGuiScheme.tsx(2 hunks)src/react/ModsPage.tsx(1 hunks)src/react/SelectOption.tsx(4 hunks)src/react/mods.module.css(1 hunks)src/react/mods.module.css.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/appConfig.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/index.ts
- src/optionsGuiScheme.tsx
- src/react/mods.module.css
- src/react/SelectOption.tsx
- src/react/mods.module.css.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/appViewerLoad.ts (1)
src/appViewer.ts (2)
appViewer(261-261)loadBackend(119-161)
🪛 Biome (1.9.4)
src/clientMods.ts
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 136-136: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 137-137: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 242-242: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 256-256: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 304-304: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 355-355: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 458-458: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 462-462: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
zardoy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/react/ModsPage.tsx (1)
30-33:⚠️ Potential issueFix the inverted
data-enabledattribute logicThe current implementation of
data-enabledis confusing and likely to cause issues with CSS styling and JavaScript logic that depends on this attribute. The attribute will be an empty string when the mod is installed, which is counterintuitive.- data-enabled={mod.installed ? '' : mod.activated} + data-enabled={mod.activated}This issue was noted in a previous review but wasn't addressed.
🧹 Nitpick comments (8)
src/react/ModsPage.tsx (8)
73-84: Consider adding field validation when saving mod fieldsThe
handleSaveFieldfunction correctly handles errors, but it doesn't perform any validation on the content before saving. This could potentially allow invalid or malformed data to be saved.const handleSaveField = async (newContents: string) => { if (!editingField) return try { + // Validate content based on field type/language + if (editingField.language === 'json' && newContents.trim()) { + try { + JSON.parse(newContents); + } catch (e) { + throw new Error(`Invalid JSON format: ${e.message}`); + } + } + mod[editingField.name] = newContents mod.wasModifiedLocally = true await saveClientModData(mod) setEditingField(null) showNotification('Success', 'Contents saved successfully') } catch (error) { showNotification('Error', 'Failed to save contents: ' + error.message, true) } }
208-224: Extract field selection logic for better readabilityThis complex object structure being created inline for
showInputsModalis hard to read and maintain. Consider extracting this logic to a helper function.+ const createFieldOptions = (fields) => { + return Object.fromEntries(fields.map(field => { + return [field.field, { + type: 'button' as const, + label: field.label, + onButtonClick() { + setEditingField({ + name: field.field, + content: field.getContent?.() || mod.installed![field.field] || '', + language: field.language + }) + } + }] + })) + } onClick={async (e) => { const fields = e.shiftKey ? getAllModsModifiableFields() : modifiableFields - const result = await showInputsModal('Edit Mod Field', Object.fromEntries(fields.map(field => { - return [field.field, { - type: 'button' as const, - label: field.label, - onButtonClick () { - setEditingField({ - name: field.field, - content: field.getContent?.() || mod.installed![field.field] || '', - language: field.language - }) - } - }] - })), { + const result = await showInputsModal('Edit Mod Field', createFieldOptions(fields), { showConfirm: false }) }
245-254: Keyboard handler could block other crucial keyboard eventsThe current implementation of the keyboard event handler uses
capture: trueand callsstopImmediatePropagation(), which will prevent all other keyboard handlers from receiving the Escape key event.useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { if (e.key === 'Escape') { e.preventDefault() - e.stopImmediatePropagation() + // Only stop propagation, but let other handlers with capture: true + // still process the event if needed + e.stopPropagation() + onClose(undefined) } } window.addEventListener('keydown', handleKeyDown, { capture: true }) return () => window.removeEventListener('keydown', handleKeyDown, { capture: true }) }, [])
299-314: First-time mod repository loading could be more robustThe current implementation directly fetches a hardcoded default repository on first visit. Consider making this repository configurable or providing fallback logic if the fetching fails.
- if (appStorage.firstModsPageVisit) { - appStorage.firstModsPageVisit = false - const defaultRepo = 'zardoy/mcraft-client-mods' - void fetchRepository(defaultRepo, defaultRepo) - } + if (appStorage.firstModsPageVisit) { + appStorage.firstModsPageVisit = false + try { + const defaultRepo = 'zardoy/mcraft-client-mods' + await fetchRepository(defaultRepo, defaultRepo) + } catch (error) { + console.error('Failed to fetch default repository:', error) + showNotification('Warning', 'Failed to fetch default repository. You can add repositories manually.', true) + } + }
325-331: Optimize the modFilter function with useCallbackThe
modFilterfunction is defined inside the render function and will be recreated on each render. Optimize it using theuseCallbackhook to prevent unnecessary re-creations.+ const modFilter = useCallback((mod: ModsData['repos'][0]['packages'][0]) => { + const matchesSearch = mod.name.toLowerCase().includes(search.toLowerCase()) || + mod.description?.toLowerCase().includes(search.toLowerCase()) + const matchesInstalledFilter = !showOnlyInstalled || mod.installed + const matchesEnabledFilter = !showOnlyEnabled || mod.activated + return matchesSearch && matchesInstalledFilter && matchesEnabledFilter + }, [search, showOnlyInstalled, showOnlyEnabled]) - const modFilter = (mod: ModsData['repos'][0]['packages'][0]) => { - const matchesSearch = mod.name.toLowerCase().includes(search.toLowerCase()) || - mod.description?.toLowerCase().includes(search.toLowerCase()) - const matchesInstalledFilter = !showOnlyInstalled || mod.installed - const matchesEnabledFilter = !showOnlyEnabled || mod.activated - return matchesSearch && matchesInstalledFilter && matchesEnabledFilter - }
358-469: Add ARIA attributes for better accessibilityThe UI lacks proper ARIA attributes for screen readers. Enhance accessibility by adding appropriate ARIA roles, labels, and relationships.
For example, you could add:
<div className={styles.modList}> + <div role="region" aria-label="Mods List"> {filteredMods ? ( <> {filteredMods.repos.map(repo => ( <div key={repo.url}> <div className={styles.repoHeader} onClick={() => toggleRepo(repo.url)} + role="button" + aria-expanded={expandedRepos[repo.url]} + aria-controls={`repo-content-${repo.url.replace(/[^a-zA-Z0-9]/g, '-')}`} > <span>{expandedRepos[repo.url] ? '▼' : '▶'}</span> <span>{repo.name || repo.url}</span> <span>({repo.packages.length})</span> </div> {expandedRepos[repo.url] && ( - <div className={styles.repoContent}> + <div + className={styles.repoContent} + id={`repo-content-${repo.url.replace(/[^a-zA-Z0-9]/g, '-')}`} + role="list" + >Similarly, add ARIA attributes to the sidebar, buttons, and other interactive elements throughout the component.
361-375: Enhance filter toggle button with visual indication of current stateThe filter button toggles between three states (all mods, installed mods, enabled mods), but there's no visual indication of the current state beyond the button title.
<Button style={{}} icon={pixelartIcons['sliders']} + className={`${showOnlyEnabled ? styles.enabledFilter : showOnlyInstalled ? styles.installedFilter : styles.allFilter}`} onClick={() => { if (showOnlyEnabled) { setShowOnlyEnabled(false) } else if (showOnlyInstalled) { setShowOnlyInstalled(false) setShowOnlyEnabled(true) } else { setShowOnlyInstalled(true) } }} title={showOnlyEnabled ? 'Show all mods' : showOnlyInstalled ? 'Show enabled mods' : 'Show installed mods'} />Then add the corresponding styles to the CSS module:
.enabledFilter { background-color: #4CAF50 !important; /* Green for enabled */ } .installedFilter { background-color: #2196F3 !important; /* Blue for installed */ } .allFilter { /* Default button style */ }
402-407: Add clear button to search input for better user experienceConsider adding a clear button to the search input to allow users to quickly clear their search query.
<Input className={styles.searchBar} value={search} onChange={e => setSearch(e.target.value)} placeholder="Search mods in added repositories..." autoFocus + suffix={search && ( + <Button + style={{ minWidth: 'auto', padding: '0 5px' }} + onClick={() => setSearch('')} + icon={pixelartIcons.close} + title="Clear search" + /> + )} />This assumes that the
Inputcomponent supports asuffixprop for adding elements after the input field. If it doesn't, you could wrap the input in a container and position the clear button absolutely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(5 hunks)src/react/CreateWorldProvider.tsx(4 hunks)src/react/ModsPage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/react/CreateWorldProvider.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (1)
src/react/ModsPage.tsx (1)
56-65: Well-implemented error handling for asynchronous actionsThe
handleActionutility properly manages error states and progress reporting for asynchronous operations, with clean error propagation to the notification system.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-h0w8ghpcp-zaro.vercel.app |
PR Type
Enhancement, Tests
fixes #314
Description
Introduced a client-side modding system with mod management.
Enhanced UI with mod-related features.
Added configuration options for mod support and updates.
Included a new HTML configuration page for client settings.
Changes walkthrough 📝
1 files
Added support for copying `config.html` to the build output.8 files
Implemented client-side modding system with database and activationlogic.Integrated mod startup logic into the application initialization.Added "Client Mods" button to the options menu.Added mod-related configuration options.Created a new mods management page.Added storage support for mod update checks.Integrated the mods page into the React UI structure.Added a new HTML configuration page for client settings.Summary by CodeRabbit
New Features
Improvements
Style
Bug Fixes
Chores
Bundle Size