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 |
WalkthroughThe changes update the configuration management and state handling across the codebase. A new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Env as Environment (INLINED_APP_CONFIG)
participant Server as Remote Config Server
App->>Env: Check for bundled configuration
alt Bundled config exists
Env-->>App: Return bundled configuration
App->>App: Load configuration locally
else Bundled config missing
App->>Server: Fetch config.json
Server-->>App: Return configuration data or error
App->>App: Process configuration or handle error
end
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ 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)
| @@ -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 |
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
Refactor