feat(config-json): Only either bundle or load from remote #291
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis pull request refines configuration handling and state management across the codebase. A new module now defines configuration types and loading logic, allowing the application to use either an inline (bundled) configuration or fetch remotely. The changes include renaming state flags from Changes
Suggested labels
Poem
✨ Finishing Touches
🪧 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 (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/appConfig.ts (1)
52-59: Improve error handling for remote configuration loadingWhile the code sets a loading screen status on fetch failure, it would be better to provide more robust error handling with potential fallbacks.
- void window.fetch('config.json').then(async res => res.json()).then(c => c, (error) => { - setLoadingScreenStatus('Failed to load app config.json', true) - }).then((config: AppConfig | {}) => { - loadAppConfig(config) - }) + void window.fetch('config.json') + .then(async res => { + if (!res.ok) { + throw new Error(`Failed to fetch config.json: ${res.status} ${res.statusText}`) + } + return res.json() + }) + .then( + (config: AppConfig) => { + loadAppConfig(config) + }, + (error) => { + console.warn('Failed to load optional app config.json', error) + setLoadingScreenStatus('Failed to load app config.json', true) + // Load with empty config as fallback + loadAppConfig({}) + } + )🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config.json(1 hunks)rsbuild.config.ts(2 hunks)src/appConfig.ts(1 hunks)src/browserfs.ts(1 hunks)src/customChannels.ts(1 hunks)src/globalState.ts(2 hunks)src/index.ts(1 hunks)src/optionsStorage.ts(3 hunks)src/panorama.ts(1 hunks)src/react/MainMenuRenderApp.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/appConfig.ts
[error] 56-56: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (18)
config.json (1)
3-3: Good addition of configuration source propertyThis new property
"configSource": "bundled"clearly indicates that the configuration is meant to be bundled with the application rather than loaded from a remote source. This aligns with the PR title to ensure only either bundle or load from remote.src/panorama.ts (1)
51-51: State management improvementGood change from
m.appLoadedtom.fsReady. This makes the condition more specific and accurate, as it's checking for file system readiness rather than general app loading. This aligns with similar changes across the codebase.src/browserfs.ts (2)
44-44: State management consistency changeChanged from
miscUiState.appLoaded = truetomiscUiState.fsReady = true, which is part of the systematic update to state management across the application. This makes the state variable name more accurately reflect what it's representing.
50-50: State management consistency changeChanged from
miscUiState.appLoaded = truetomiscUiState.fsReady = true, maintaining consistency with the error case above and the rest of the codebase.src/customChannels.ts (1)
88-88: Enhanced type definitions for protocol fieldsThe changes to these three fields (
id,categoryTitle, anditems) enhance the protocol definition by specifying a count type of 'i16' (16-bit integer) for the string length. This makes the protocol definition more precise and helps ensure correct data handling.These changes align with the existing type definition pattern used elsewhere in the file (see line 22 and 38).
Also applies to: 92-92, 96-96
rsbuild.config.ts (2)
75-75: Improved configuration loading logic with conditional inlining.The change conditionally inlines the app configuration based on
configSource, ensuring bundled configurations are included while remote configurations remain separate. This is cleaner than always inlining all configurations.
112-114: Consistent with configuration source strategy.This change ensures the
config.jsonfile is only written to the output directory when using the "remote" configuration source. This prevents duplicating configuration data and maintains a single source of truth based on the configured strategy.src/globalState.ts (2)
8-8: Imported AppConfig from new dedicated file.You've moved the AppConfig type from this file to a dedicated
appConfig.tsfile, which improves code organization and separates concerns.
128-128: Renamed state property from appLoaded to fsReady.Changed state property name from
appLoadedtofsReadyto better reflect its purpose - tracking the readiness of the file system rather than the general app loading state. This naming is more precise and aligns with its actual usage.src/react/MainMenuRenderApp.tsx (2)
78-78: Updated state destructuring to use fsReady.Consistently updated to use the renamed state property
fsReadyinstead ofappLoadedin the destructuring assignment frommiscUiState.
80-80: Updated conditional logic to use fsReady.All usages of
appLoadedhave been correctly replaced withfsReadyin the component's conditional logic, maintaining consistent behavior with the renamed state property.Also applies to: 83-83
src/index.ts (1)
916-919: Enhanced initialization condition.The condition for triggering subsequent initialization has been improved to check for both
fsReadyandappConfigbeing truthy. This ensures the application only proceeds when both the file system is ready and the configuration is loaded, providing a more robust initialization sequence.src/appConfig.ts (3)
5-23: Well-structured AppConfig type definitionThe AppConfig type is well-defined with clear documentation comments for some fields. The commented-out properties suggest future extensions or deprecated options.
25-45: Robust configuration loading with appropriate fallbacksThe loadAppConfig function handles both new and existing configurations properly. It correctly merges new config with existing one and handles forced settings by either adding them to disabled settings or removing them as appropriate.
47-51: Type-safe handling of bundled configurationThe implementation checks for bundled config presence and loads it correctly.
src/optionsStorage.ts (3)
8-8: Correct import path for AppConfig typeThe import has been updated to use the new location of the AppConfig type from './appConfig' instead of './globalState', which aligns with the reorganization of the configuration management.
190-190: Enhanced WatchValue type signature with cleanup capabilityThe WatchValue type now returns a function to unsubscribe, which is a good practice for managing resources and preventing memory leaks.
200-213:❓ Verification inconclusive
Improved subscription management with cleanup
The implementation now properly collects unsubscribe functions and provides a way to clean them up, which is an excellent enhancement for resource management.
However, ensure that all existing uses of
watchValueare updated to properly use the returned unsubscribe function where needed to prevent memory leaks.
🏁 Script executed:
#!/bin/bash # Find all usages of watchValue in the codebase rg "watchValue\(" --type tsLength of output: 1182
Subscription cleanup improved in src/optionsStorage.ts – well done on aggregating and returning unsubscribe functions for proper resource management.
The new implementation (lines 200–213 in
src/optionsStorage.ts) correctly collects unsubscribe functions and provides a cleanup routine. However, multiple calls towatchValueexist throughout the codebase (e.g., insrc/watchOptions.ts,src/panorama.ts,src/reactfiles, etc.) that don’t appear to capture or use the returned unsubscribe functions. Please review these usages to ensure they either store and invoke the unsubscribe when appropriate or that their lifecycle management inherently prevents memory leaks.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
have an idea to cache config.json in localStorage and display prompt to use cached config if available instead of just an error screen |
| if (isBundledConfigUsed) { | ||
| loadAppConfig(process.env.INLINED_APP_CONFIG as AppConfig ?? {}) | ||
| } else { | ||
| void window.fetch('config.json').then(async res => res.json()).then(c => c, (error) => { |
There was a problem hiding this comment.
This should probably use await not void to ensure the config was loaded before the client continues to load.
There was a problem hiding this comment.
Hm, I was avoiding any usage of top level awaits because I think it can break the app in some unexpected way and don't really see any benefit of using it
There was a problem hiding this comment.
Well I don't really see what is stopping the client from using the bundled config.json values until the remote one is loaded and imo await is the proper way to force async queries to block the app until it's loaded? If it really breaks something then that would be a bug elsewhere (and if it fails to load the file then it should of course break as the app doesn't work without the config)
rsbuild.config.ts
Outdated
| @@ -72,7 +72,7 @@ const appConfig = defineConfig({ | |||
| 'process.env.RELEASE_LINK': JSON.stringify(releaseLink), | |||
| 'process.env.RELEASE_CHANGELOG': JSON.stringify(releaseChangelog), | |||
| 'process.env.DISABLE_SERVICE_WORKER': JSON.stringify(disableServiceWorker), | |||
| 'process.env.INLINED_APP_CONFIG': JSON.stringify(configJson), | |||
| 'process.env.INLINED_APP_CONFIG': JSON.stringify(configJson.configSource === 'bundled' ? configJson : null), | |||
There was a problem hiding this comment.
Defining this value in the same config which one wants to switch the behaviour of seems like a roundabout way. (As you would need to manually modify the config before building)
I would expect it to be toggleable at built time without modifying the checked out source e.g. via a build param, feature flag, custom goal, or environment variable.
There was a problem hiding this comment.
Yes, you are absolutely right, it affects only bundling process and should work like in the same way as disabling service worker works
There was a problem hiding this comment.
Though I put it here to make it noticeable. Imagine you don't know about the build flag, anyway, I'm not sure what behaviour I should use in repo by default (remote I guess?)
There was a problem hiding this comment.
Well remote would make more sense to keep the behaviour compatible with the previous one.
There was a problem hiding this comment.
The previous behavior didn't make app menu delay to load before config.json is set, that's why I'm so unsure
That feels unnecessary as the browser would already cache it. Also whether or not it should be cached should be defined by the server imo. The user should not have any direct say about which server-enforced config options should be uset. |
| disabledSettings.value.add(key) | ||
| // since the setting is forced, we need to set it to that value | ||
| if (appConfig.defaultSettings?.[key]) { | ||
| options[key] = appConfig.defaultSettings[key] |
There was a problem hiding this comment.
Will break setting from qs params , eed to fix
|
Another idea that I had which kinda gets back the previous behaviour: Have it check for a config.js in a separate (non-git-included) directory on build and merge it into the bundled config.json similarly to how the Another approach could be to always bundle the config.json but still fetch minecraft-web-client/src/index.ts Lines 975 to 980 in ceb4cb0 |
I didn't get what you meant, it all already worked before like you described, but merging the config was happening later (since the config json would be available instantly). The whole purpose of this pr so we don't have to think about race conditions like that anymore. I will introduce the build ENV param INLINE_CONFIG_JSON (that would be false by default), thus delay app loading before config.json is fetched |
Which part of it?
I don't think so? The current state on
Sounds like a good solution |
|
i already received like 5 complains because of this breakage needs to be merged asap |
even worse, because the default was an empty string before, now using ||= assignment logic, the wrong proxy server will be saved in cache of your users that loaded the page before, even after fixing this issue. Yes definitely using an empty string to indicate using of proxy from the same origin was not the best UX decision, but I didn't find a better one |
I already answered this part a few times. The config values are applied but not at time when server connect UI was displayed and autoConnect option along with default proxy option was checked minecraft-web-client/src/index.ts Line 907 in 10f1706 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/dockerPrepare.mjs (3)
21-27: Handle null repository values more gracefully.The current implementation always includes the repository property in the release.json file, even when it's null. This could cause issues if other parts of the application expect valid repository information.
Consider conditionally adding the repository field:
// write release tag and repository info const commitShort = execSync('git rev-parse --short HEAD').toString().trim() const repository = getGitRepository() -fs.writeFileSync('./assets/release.json', JSON.stringify({ - latestTag: `${commitShort} (docker)`, - repository -}), 'utf8') +const releaseInfo = { + latestTag: `${commitShort} (docker)` +} +if (repository) { + releaseInfo.repository = repository +} +fs.writeFileSync('./assets/release.json', JSON.stringify(releaseInfo), 'utf8')
7-19: Add validation for the extracted repository name format.The extracted repository name should follow the format
owner/repo, but there's no validation to ensure this pattern is correctly extracted.Add a validation step to ensure the extracted repository follows the expected pattern:
const getGitRepository = () => { try { const gitConfig = fs.readFileSync('.git/config', 'utf8') const originUrlMatch = gitConfig.match(/\[remote "origin"\][\s\S]*?url = .*?github\.com[:/](.*?)(\.git)?\n/m) if (originUrlMatch) { - return originUrlMatch[1] + const repo = originUrlMatch[1] + // Validate repo format (owner/repo) + if (/^[^/]+\/[^/]+$/.test(repo)) { + return repo + } + console.warn('Extracted repository name does not match expected format (owner/repo):', repo) } } catch (err) { console.warn('Failed to read git repository from config:', err) } return null }
7-19:❓ Verification inconclusive
Add support for non-GitHub repository hosts.
The current implementation only works with GitHub repositories. If your project might be hosted on other platforms like GitLab or Bitbucket in the future, you might want to add support for those as well.
Let's check if the project uses other git hosts elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Search for references to different git hosting services echo "Searching for references to different git hosting services..." rg -i "github\.com|gitlab\.com|bitbucket\.org" --type js --type ts --type jsonLength of output: 1294
Expand Repository Host Support Consideration
The repository extraction logic in
scripts/dockerPrepare.mjs(lines 7–19) currently relies on a regex that targets GitHub URLs. Our search confirms that all existing references in the codebase point exclusively to GitHub. If there’s any possibility of hosting on platforms like GitLab or Bitbucket in the future, please consider one of the following:
- Generalize the Regex: Update the logic to also match repository URLs for GitLab, Bitbucket, etc.
- Document the Limitation: If the project is intended to be GitHub-only, add a comment explaining this design choice for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(2 hunks)rsbuild.config.ts(4 hunks)scripts/dockerPrepare.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rsbuild.config.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
Dockerfile (2)
12-12: Add CONFIG_JSON_SOURCE Build Argument
The introduction of the build argumentCONFIG_JSON_SOURCE(defaulting toREMOTE) aligns well with the PR objective of conditionally loading configuration from either bundled or remote sources. Ensure that this default value is consistent with your intended deployment behavior.
26-27: Propagate Configuration Variables in the Build Command
The updatedRUNcommand now passes bothDISABLE_SERVICE_WORKERandCONFIG_JSON_SOURCEenvironment variables to the build process. This change enables the build system to dynamically control service worker disabling and configuration sourcing, ensuring that the build reflects the updated logic inrsbuild.config.ts. Please verify that downstream scripts (e.g.,pnpm run build) correctly reference these environment variables for proper configuration handling.
| // Get repository from git config | ||
| const getGitRepository = () => { | ||
| try { | ||
| const gitConfig = fs.readFileSync('.git/config', 'utf8') | ||
| const originUrlMatch = gitConfig.match(/\[remote "origin"\][\s\S]*?url = .*?github\.com[:/](.*?)(\.git)?\n/m) | ||
| if (originUrlMatch) { | ||
| return originUrlMatch[1] | ||
| } | ||
| } catch (err) { | ||
| console.warn('Failed to read git repository from config:', err) | ||
| } | ||
| return null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use native git commands instead of parsing config file.
The function attempts to extract the repository information by parsing the git config file directly with regex, which is fragile and may break with different git configurations or formats.
I recommend using git commands directly through execSync, which would be more robust:
// Get repository from git config
const getGitRepository = () => {
try {
- const gitConfig = fs.readFileSync('.git/config', 'utf8')
- const originUrlMatch = gitConfig.match(/\[remote "origin"\][\s\S]*?url = .*?github\.com[:/](.*?)(\.git)?\n/m)
- if (originUrlMatch) {
- return originUrlMatch[1]
- }
+ const remoteUrl = execSync('git config --get remote.origin.url').toString().trim()
+ // Extract repo from GitHub URL format (handles both HTTPS and SSH formats)
+ const githubRepoMatch = remoteUrl.match(/github\.com[:/](.*?)(\.git)?$/)
+ if (githubRepoMatch) {
+ return githubRepoMatch[1]
+ }
+ console.warn('Remote URL does not appear to be a GitHub repository:', remoteUrl)
} catch (err) {
console.warn('Failed to read git repository from config:', err)
}
return null
}📝 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.
| // Get repository from git config | |
| const getGitRepository = () => { | |
| try { | |
| const gitConfig = fs.readFileSync('.git/config', 'utf8') | |
| const originUrlMatch = gitConfig.match(/\[remote "origin"\][\s\S]*?url = .*?github\.com[:/](.*?)(\.git)?\n/m) | |
| if (originUrlMatch) { | |
| return originUrlMatch[1] | |
| } | |
| } catch (err) { | |
| console.warn('Failed to read git repository from config:', err) | |
| } | |
| return null | |
| } | |
| // Get repository from git config | |
| const getGitRepository = () => { | |
| try { | |
| const remoteUrl = execSync('git config --get remote.origin.url').toString().trim() | |
| // Extract repo from GitHub URL format (handles both HTTPS and SSH formats) | |
| const githubRepoMatch = remoteUrl.match(/github\.com[:/](.*?)(\.git)?$/) | |
| if (githubRepoMatch) { | |
| return githubRepoMatch[1] | |
| } | |
| console.warn('Remote URL does not appear to be a GitHub repository:', remoteUrl) | |
| } catch (err) { | |
| console.warn('Failed to read git repository from config:', err) | |
| } | |
| return null | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/appConfig.ts (3)
5-23: Consider defining a more explicit structure forpromoteServers.
Inline types such as{ ip, description, version? }may reduce clarity and type safety. Declaring a dedicated interface or specifying the field types (e.g.ip: string) can improve readability and correctness.
25-30: Perform a deep merge if nested objects need merging.
UsingObject.assignonly merges top-level properties. IfappConfigcontains nested objects, updates in deeper levels could be lost or incorrectly overridden. Consider implementing or utilizing a deep-merge approach if that scenario is expected.
32-44: Double-check logic for forced settings versus disabled settings.
When forcing a setting (i.e.,valueistrue), you disable user modifications and also assign a default value. If the intent is to prevent changes and always override with defaults, this is fine. Otherwise, consider clarifying the distinction between forcing and disabling a setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/appConfig.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (3)
src/appConfig.ts (3)
49-51: Validate environment config shape.
Castingprocess.env.INLINED_APP_CONFIGtoAppConfigmay be risky if it doesn't match the expected shape. Consider parsing or validating the environment config object to avoid runtime inconsistencies.
52-52: Useawaitto block application load until config is fetched.
This mirrors an existing suggestion to replacevoidwithawait, ensuring the remote config is loaded before continuing.
52-58: Handle the possibility ofconfigbeing null or undefined.
When fetchingconfig.jsonfails, the code sets a loading screen status but still proceeds to callloadAppConfig(config). Ensureconfigis defined and valid, or provide a fallback for partial or missing config objects.
|
@ Phoenix616 thanks |
PR Type
Enhancement, Bug fix
Description
Introduced conditional handling for bundled or remote
config.json.Added a new
AppConfigtype and refactored its usage across modules.Replaced
appLoadedstate withfsReadyfor better clarity.Improved
watchValueutility to support unsubscribing.Changes walkthrough 📝
10 files
Conditional handling for bundled or remote `config.json`Added `AppConfig` type and loading logicReplaced `appLoaded` with `fsReady` stateUpdated packet structure for custom channelsRefactored `AppConfig` type and state managementAdjusted app initialization to use `fsReady` and `AppConfig`Enhanced `watchValue` utility with unsubscribe supportUpdated panorama logic to use `fsReady`Updated main menu rendering logic with `fsReady`Added `configSource` field to configurationSummary by CodeRabbit
New Features
Chores