feat: enforce latest version #1954
Conversation
|
Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.
|
…o feat/enforce-latest-version
# Conflicts: # Explorer/Assets/AddressableAssetsData/AssetGroups/Settings.asset # Explorer/Assets/DCL/PluginSystem/Global/DynamicSettings.cs # Explorer/Assets/Scenes/Main.unity
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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: 2
Outside diff range, codebase verification and nitpick comments (5)
Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs (1)
17-27: LGTM, but consider potential improvements.The code changes are approved, but consider the following potential improvements:
- The regular expression could be more robust to handle different version string formats. For example, it could handle version strings with more than 3 parts or with additional metadata like pre-release or build information.
- The method could throw an exception if the version string is invalid instead of returning a default value. This would make it easier to catch and handle invalid version strings.
Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs (4)
40-48: Consider returning a custom type instead of a tuple.The method
GetVersionsAsyncreturns a tuple of the current and latest versions. Consider creating a custom type to represent the versions and return that instead of a tuple. This will make the code more readable and maintainable.public class Versions { public string Current { get; set; } public string Latest { get; set; } } public async UniTask<Versions> GetVersionsAsync(CancellationToken ct) { // ... return new Versions { Current = Application.version, Latest = latestVersion }; }
50-58: Consider extracting the code to launch the launcher into a separate method.The method
LaunchOrDownloadLauncherAsynchas a conditional statement that either downloads and runs the launcher or runs the launcher directly. Consider extracting the code to run the launcher into a separate method to make the code more readable and maintainable.public async UniTask LaunchOrDownloadLauncherAsync(CancellationToken ct = default) { string? launcherPath = GetLauncherPath(); if (string.IsNullOrEmpty(launcherPath)) await DownloadAndRunLauncherAsync(ct); else LaunchLauncher(launcherPath); } private void LaunchLauncher(string launcherPath) { RunLauncherAndQuit(launcherPath); }
74-75: Consider showing a message to the user while downloading the launcher.The method
DownloadAndRunLauncherAsyncdownloads the launcher and runs it. Consider showing a message to the user while downloading the launcher to improve the user experience.// Show a message to the user here Debug.Log("Downloading launcher. Please wait...");
105-145: Consider extracting the code to start the launcher process into a separate method.The method
RunLauncherAndQuitstarts the launcher process and quits the application. Consider extracting the code to start the launcher process into a separate method to make the code more readable and maintainable.private static void RunLauncherAndQuit(string launcherPath) { try { StartLauncherProcess(launcherPath); } catch (Exception e) { if (e is not OperationCanceledException) ReportHub.LogException(e, ReportCategory.UNSPECIFIED); } finally { QuitApplication(); } } private static void StartLauncherProcess(string launcherPath) { var startInfo = new ProcessStartInfo { UseShellExecute = true, }; switch (Application.platform) { case RuntimePlatform.WindowsEditor: case RuntimePlatform.WindowsPlayer: startInfo.FileName = launcherPath; break; case RuntimePlatform.OSXEditor: case RuntimePlatform.OSXPlayer: startInfo.FileName = "open"; startInfo.Arguments = $"-n \"{launcherPath}\""; break; default: ReportHub.LogError(ReportCategory.UNSPECIFIED, "Unsupported platform for launching the application."); return; } Process.Start(startInfo); } private static void QuitApplication() { #if UNITY_EDITOR EditorApplication.isPlaying = false; #else Application.Quit(); #endif }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- Explorer/Assets/AddressableAssetsData/AssetGroups/UI.asset (2 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard.meta (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.asmdef (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.asmdef.meta (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs.meta (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs.meta (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenView.cs (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenView.cs.meta (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs.meta (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/VersionUpdateScreen.prefab (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/VersionUpdateScreen.prefab.meta (1 hunks)
- Explorer/Assets/DCL/AuthenticationScreenFlow/AuthenticationScreenController.cs (1 hunks)
- Explorer/Assets/DCL/AuthenticationScreenFlow/AuthenticationScreenFlow.asmdef (1 hunks)
- Explorer/Assets/DCL/PluginSystem/DCL.Plugins.asmdef (1 hunks)
- Explorer/Assets/DCL/PluginSystem/Global/DynamicSettings.cs (2 hunks)
- Explorer/Assets/Scenes/Main.unity (3 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (8 hunks)
Files skipped from review due to trivial changes (7)
- Explorer/Assets/DCL/ApplicationVersionGuard.meta
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.asmdef
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.asmdef.meta
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs.meta
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs.meta
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenView.cs.meta
- Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs.meta
Additional comments not posted (39)
Explorer/Assets/DCL/ApplicationVersionGuard/VersionUpdateScreen.prefab.meta (1)
2-3: LGTM!The changes to the
guidandPrefabImporterare consistent with the AI-generated summary and do not require any code changes.Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenView.cs (1)
1-27: LGTM!The code changes are approved. The class is well-structured, follows the SOLID principles, and uses good practices such as:
- Single responsibility principle
- Dependency injection
- Constant strings
[field: SerializeField]attributeprivate setaccessor- String interpolation
Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs (2)
7-8: LGTM!The code changes are approved.
10-15: LGTM!The code changes are approved.
Explorer/Assets/DCL/PluginSystem/Global/DynamicSettings.cs (1)
25-25: LGTM!The addition of the
AppVerRedirectionScreenPrefabfield to theDynamicSettingsclass aligns with the PR objective of enforcing the latest version of the application. This serialized field, which references a Unity prefab asset, will likely be used to instantiate a UI screen for handling version redirection scenarios, as suggested by the AI-generated summary.The field follows the naming convention and coding style of the other fields in the class, and its public getter and private setter ensure proper encapsulation.
Explorer/Assets/DCL/AuthenticationScreenFlow/AuthenticationScreenFlow.asmdef (2)
28-28: Provide more information about the added GUID entry.The GUID
"GUID:4a12c0b1b77ec6b418a8d7bd5c925be3"has been added to thereferencesarray. Please provide more information about the specific asset or dependency this GUID represents and how it enhances the authentication screen flow functionality.
29-29: Provide more information about the added GUID entry.The GUID
"GUID:0b3eab7834a09c24ca4e84fe0d8a43ce"has been added to thereferencesarray. Please provide more information about the specific asset or dependency this GUID represents and how it enhances the authentication screen flow functionality.Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs (1)
11-59: LGTM!The
LauncherRedirectionScreenControllerclass is well-structured and follows the SOLID principles. It has a single responsibility of managing the launcher redirection screen and has a clear separation of concerns between the view, the controller, the dependencies, the event handlers, the version update, the quit, the close intent, the editor, the runtime, the Unity code, and the C# code.Explorer/Assets/AddressableAssetsData/AssetGroups/UI.asset (9)
40-43: Verify the impact of the asset reference change.The asset reference has been updated from
ScenesUIStyleSheettoAssets/DCL/UI/ColorPicker/HairColors.asset. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
55-58: Verify the impact of the asset reference change.The asset reference has been updated from
Assets/DCL/UI/Popups/PopupCloser.prefabtoLoadingAtlast.spriteatlasv2. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
Verification successful
Asset reference change appears isolated.
The asset
LoadingAtlast.spriteatlasv2is only referenced in theUI.assetfile and does not appear elsewhere in the codebase. This suggests that the change is isolated and should not impact other parts of the codebase. However, it is recommended to manually verify that this change does not introduce any runtime issues or hidden dependencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `LoadingAtlast.spriteatlasv2` asset in the codebase. # Test: Search for references to `LoadingAtlast.spriteatlasv2`. Expect: Relevant usage in the codebase. rg --type unity --type csharp --type shader -i -C 5 $'LoadingAtlast.spriteatlasv2'Length of output: 111
Script:
#!/bin/bash # Description: Verify the usage of `LoadingAtlast.spriteatlasv2` asset in the codebase. # Test: Search for references to `LoadingAtlast.spriteatlasv2` in all files. rg -i -C 5 $'LoadingAtlast.spriteatlasv2'Length of output: 1095
60-64: Verify the impact of the asset reference change.The asset reference has been updated from
TeleportPrompttoAssets/DCL/UI/ColorPicker/BodyShapeColors.asset. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
#!/bin/bash # Description: Verify the usage of `Assets/DCL/UI/ColorPicker/BodyShapeColors.asset` asset in the codebase. # Test: Search for references to `Assets/DCL/UI/ColorPicker/BodyShapeColors.asset`. Expect: Relevant usage in the codebase. rg --type unity --type csharp --type shader -i -C 5 $'Assets/DCL/UI/ColorPicker/BodyShapeColors.asset' --- `30-33`: **Verify the impact of the asset reference change.** The asset reference has been updated from `Navmap` to `MapRendererSettings`. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors. Run the following script to verify the asset usage: <details> <summary>Verification successful</summary> --- **Asset reference change verified successfully.** The `MapRendererSettings` asset is referenced in several parts of the codebase, indicating that the change from `Navmap` to `MapRendererSettings` is properly integrated. The references are found in various scripts and asset files, confirming that the new asset is being utilized consistently. - **Explorer/Assets/Scripts/Global/Dynamic/MapRendererContainer.cs**: Multiple references to `MapRendererSettings` and related settings. - **Explorer/Assets/Scenes/System.prefab**: Contains a backing field for `MapRendererSettings`. - **Explorer/Assets/Scenes/LocomotionTestScene.unity**: Similar backing field reference. - **Explorer/Assets/DCL/PluginSystem/World/World Plugins Container.asset**: References the new GUID for `MapRendererSettings`. - **Explorer/Assets/DCL/MapRenderer/ComponentsFactory**: Various installers and factories reference `IMapRendererSettings`. - **Explorer/Assets/DCL/MapRenderer/MapRendererSettingsAsset.cs**: Defines the `MapRendererSettingsAsset` class. These findings confirm that the asset change is reflected across the codebase without any apparent inconsistencies. </details> <details> <summary>Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify the usage of `MapRendererSettings` asset in the codebase. # Test: Search for references to `MapRendererSettings`. Expect: Relevant usage in the codebase. rg --type unity --type csharp --type shader -i -C 5 $'MapRendererSettings'Length of output: 103
Script:
#!/bin/bash # Description: Verify the usage of `MapRendererSettings` asset in the codebase. # Test: Search for references to `MapRendererSettings` in C# and shader files. rg --type csharp --type shader -i -C 5 $'MapRendererSettings'Length of output: 91
Script:
#!/bin/bash # Description: Verify the usage of `MapRendererSettings` asset in the codebase. # Test: Search for references to `MapRendererSettings` in all files. rg -i -C 5 $'MapRendererSettings'Length of output: 36637
20-23: Verify the impact of the asset reference change.The asset reference has been updated from
HoverCanvastoFullSearchResult. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
Verification successful
Asset reference change is properly integrated.
The asset reference update from
HoverCanvastoFullSearchResultis well-reflected in the codebase, particularly within theSearchResultPanelControllerandSearchResultPanelViewclasses. TheFullSearchResultsViewclass is extensively used, indicating that the change is intentional and integrated into the UI logic. No inconsistencies or errors were found related to this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `FullSearchResult` asset in the codebase. # Test: Search for references to `FullSearchResult`. Expect: Relevant usage in the codebase. rg --type unity --type csharp --type shader -i -C 5 $'FullSearchResult'Length of output: 100
Script:
#!/bin/bash # Description: Verify the usage of `FullSearchResult` asset in the codebase. # Test: Search for references to `FullSearchResult` in C# and shader files. rg --type csharp --type shader -i -C 5 $'FullSearchResult'Length of output: 88
Script:
#!/bin/bash # Description: Verify the usage of `FullSearchResult` asset in the codebase. # Test: Search for references to `FullSearchResult` in C# files. rg --type csharp -i -C 5 $'FullSearchResult'Length of output: 10265
45-48: Verify the impact of the asset reference change.The asset reference has been updated from
ExternalUrlPrompttoScenesUIStyleSheet. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
Verification successful
Asset Reference Change Verified
The asset
ScenesUIStyleSheetis actively used in theSceneUIPlugin.csfile, specifically in theInitializeAsyncmethod, where it is applied to a UI canvas. This confirms that the change fromExternalUrlPrompttoScenesUIStyleSheetis properly integrated into the codebase. No inconsistencies or errors were found related to this change.
- File:
Explorer/Assets/DCL/PluginSystem/World/SceneUIPlugin.cs- Method:
InitializeAsyncScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ScenesUIStyleSheet` asset in the codebase. # Test: Search for references to `ScenesUIStyleSheet`. Expect: Relevant usage in the codebase. rg --type unity --type csharp --type shader -i -C 5 $'ScenesUIStyleSheet'Length of output: 102
Script:
#!/bin/bash # Description: Verify the usage of `ScenesUIStyleSheet` asset in the codebase. # Test: Search for references to `ScenesUIStyleSheet` in relevant file types. rg --type cs --type shader --type json -i -C 5 $'ScenesUIStyleSheet'Length of output: 98
Script:
#!/bin/bash # Description: Verify the usage of `ScenesUIStyleSheet` asset in the codebase. # Test: Search for references to `ScenesUIStyleSheet` in relevant file types. rg --type cs --type json -i -C 5 $'ScenesUIStyleSheet'Length of output: 1635
50-53: Verify the impact of the asset reference change.The asset reference has been updated from
NftPrompttoTeleportPrompt. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
Verification successful
Change from
NftPrompttoTeleportPromptis verified and integrated.The asset reference change from
NftPrompttoTeleportPromptis reflected across multiple parts of the codebase, including C# scripts and the plugin system. The integration appears to be well-structured and consistent, with no immediate issues detected.
DynamicWorldContainer.cs: ReferencesTeleportPromptPlugin.RestrictedActionsAPIImplementation.cs: UsesTeleportPromptController.TeleportPromptController.cs: Implements the logic for the teleport prompt.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `TeleportPrompt` asset in the codebase. # Test: Search for references to `TeleportPrompt`. Expect: Relevant usage in the codebase. rg --type unity --type csharp --type shader -i -C 5 $'TeleportPrompt'Length of output: 98
Script:
#!/bin/bash # Description: Verify the usage of `TeleportPrompt` asset in the codebase. # Test: Search for references to `TeleportPrompt` in C# scripts and Unity metadata files. rg --type-add 'csharp:*.cs' --type-add 'meta:*.meta' -t csharp -t meta -i -C 5 $'TeleportPrompt'Length of output: 22902
25-28: Verify the impact of the asset reference change.The asset reference has been updated from
ExplorePaneltoLogoDCL154x154SpriteAtlas.spriteatlas. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
35-38: Verify the impact of the asset reference change.The asset reference has been updated from
MinimaptoM_LoadingSpinnerBackground.mat. Ensure that this change is properly reflected in the codebase and does not introduce any inconsistencies or errors.Run the following script to verify the asset usage:
Explorer/Assets/DCL/PluginSystem/DCL.Plugins.asmdef (1)
130-130: The addition of the new GUID to the references list looks good syntactically.The new GUID
"GUID:1c5f9b69f95e40d18f11049c155eeaf8"is added correctly to thereferencesarray, following the existing format.However, please verify the necessity and impact of adding this new reference. Ensure that it's required and doesn't introduce any unintended dependencies or side effects.
You can use the following script to search for usages of the newly referenced assembly across the codebase:
Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (9)
3-3: LGTM!The code changes are approved.
16-16: LGTM!The code changes are approved.
32-32: LGTM!The code changes are approved.
43-50: LGTM!The code changes are approved.
52-53: LGTM!The code changes are approved.
63-63: LGTM!The code changes are approved.
75-75: LGTM!The code changes are approved.
187-191: LGTM!The code changes are approved.
222-244: LGTM!The code changes are approved.
Explorer/Assets/DCL/AuthenticationScreenFlow/AuthenticationScreenController.cs (2)
14-14: LGTM!The code change is approved.
24-28: LGTM!The code change is approved. The conditional inclusion of namespaces based on the build environment is a good practice to streamline the code.
Explorer/Assets/Scenes/Main.unity (1)
545-549: Verify the usage of the newly addedAppVerRedirectionScreenPrefabin the codebase.Ensure that the prefab is being instantiated and used correctly for the app version redirection flow.
Run the following script to verify the prefab usage:
Verification successful
The
AppVerRedirectionScreenPrefabis correctly used in the codebase.The prefab is instantiated and utilized in the
MainSceneLoader.csfor the app version redirection flow, and it is also referenced inDynamicSettings.csas a serialized field, indicating proper integration and configuration.
- File:
Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs- File:
Explorer/Assets/DCL/PluginSystem/Global/DynamicSettings.csScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `AppVerRedirectionScreenPrefab` in the codebase. # Test: Search for the prefab usage in C# files. Expect: At least one usage. rg --type cs -A 5 $'AppVerRedirectionScreenPrefab'Length of output: 1323
Explorer/Assets/DCL/ApplicationVersionGuard/VersionUpdateScreen.prefab (9)
1-2: LGTM!The YAML directives are standard and look good.
3-20: LGTM!The GameObject "Slide003" is defined correctly and is inactive by default, which seems intentional.
235-237: Verify if the empty OnClick events are intentional.The OnClick events on the Button components for "ExitApplication", "UpdateNow", and "Close" GameObjects are empty. Ensure that this is intentional and the button click events are handled through some other script. If not, consider wiring them up to appropriate event handlers.
Also applies to: 470-472, 982-984
882-883: LGTM!Making the "Close" GameObject inactive by default seems intentional and is approved.
1917-1918: LGTM!Making the "Slides" GameObject inactive by default seems intentional and is approved.
21-77: Verify the sprite asset reference.Ensure that the sprite asset referenced by the Image component (guid: a679d60beb0234ed1bce4675e9e8e3c0) exists and is the intended asset.
Run the following script to verify the sprite asset reference:
Verification successful
Sprite asset reference verified.
The sprite asset with GUID
a679d60beb0234ed1bce4675e9e8e3c0is correctly referenced in the codebase. It is associated withSlide003.pngand used in the relevant prefab files.
Explorer/Assets/DCL/ApplicationVersionGuard/VersionUpdateScreen.prefabExplorer/Assets/DCL/AuthenticationScreenFlow/Assets/AuthenticationScreen.prefabExplorer/Assets/Textures/Authentication/Slides/Slide003.png.metaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the sprite asset reference. # Test: Check if the sprite asset exists. Expect: The asset to exist. fd --absolute-path --type f a679d60beb0234ed1bce4675e9e8e3c0Length of output: 62
Script:
#!/bin/bash # Description: Search for the sprite asset GUID within file contents. # Test: Search for the GUID in the contents of files. Expect: To find references to the GUID. rg "a679d60beb0234ed1bce4675e9e8e3c0" --hidden --glob '!.git/*' --glob '!node_modules/*'Length of output: 503
569-569: Verify the custom material asset reference for the Canvas.Ensure that the custom material asset referenced by the Canvas component (guid: c80a952d30100417cbfa5e80564c8b01) exists and is the intended asset.
Run the following script to verify the custom material asset reference:
Verification successful
Custom material asset reference verified successfully.
The custom material asset referenced by the Canvas component (guid: c80a952d30100417cbfa5e80564c8b01) exists in the repository as
AuthenticationLogIn.mat. This confirms that the reference is correct and the asset is available.
- Asset found at:
Explorer/Assets/DCL/UI/Shaders/AuthenticationLogIn.mat.metaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the custom material asset reference for the Canvas. # Test: Check if the custom material asset exists. Expect: The asset to exist. fd --absolute-path --type f c80a952d30100417cbfa5e80564c8b01Length of output: 62
Script:
#!/bin/bash # Description: Search for the custom material asset GUID within .meta files. # Test: Check if the GUID exists in any .meta file. Expect: The GUID to be found in a .meta file. rg 'c80a952d30100417cbfa5e80564c8b01' --glob '*.meta'Length of output: 152
264-264: Verify the animation controller asset references.Ensure that the animation controller assets referenced by the Animator components (guid: 02558cdf9bd9a495b985d35e8eb5fe3d and guid: 40d08c8515d35ba439c01a14dbfcf49e) exist and are the intended assets.
Run the following script to verify the animation controller asset references:
Also applies to: 497-497, 1011-1011
Verification successful
Animation Controller Asset References Verified
The animation controller assets referenced by the Animator components with GUIDs
02558cdf9bd9a495b985d35e8eb5fe3dand40d08c8515d35ba439c01a14dbfcf49eexist in the repository and are correctly referenced in the prefabs.
- Explorer/Assets/DCL/ApplicationVersionGuard/VersionUpdateScreen.prefab
- Explorer/Assets/DCL/AuthenticationScreenFlow/Assets/AuthenticationScreen.prefab
- Explorer/Assets/DCL/AuthenticationScreenFlow/Assets/Animations/Button/ButtonAnimatorController.overrideController.meta
- Explorer/Assets/DCL/AuthenticationScreenFlow/Assets/Animations/Slides/AuthScreenSlidesAnimatorController.controller.meta
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the animation controller asset references. # Test: Check if the animation controller assets exist. Expect: The assets to exist. fd --absolute-path --type f 02558cdf9bd9a495b985d35e8eb5fe3d 40d08c8515d35ba439c01a14dbfcf49eLength of output: 215
Script:
#!/bin/bash # Description: Verify the animation controller asset references by searching for GUIDs within files. # Search for the first GUID in the repository files rg '02558cdf9bd9a495b985d35e8eb5fe3d' --type text --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*' # Search for the second GUID in the repository files rg '40d08c8515d35ba439c01a14dbfcf49e' --type text --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*'Length of output: 277
Script:
#!/bin/bash # Description: Verify the animation controller asset references by searching for GUIDs within files. # Search for the first GUID in the repository files rg '02558cdf9bd9a495b985d35e8eb5fe3d' --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*' # Search for the second GUID in the repository files rg '40d08c8515d35ba439c01a14dbfcf49e' --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*'Length of output: 1787
714-714: Verify the font asset references for the TextMeshProUGUI components.Ensure that the font assets referenced by the TextMeshProUGUI components (guid: 96ae0a2159a39234f858ea23bdcc74ad and guid: 35aa85d68d15435418848a03a2db81ec) exist and are the intended assets.
Run the following script to verify the font asset references:
Also applies to: 1089-1089, 1389-1389
Verification successful
Font Asset References Verified
The font assets referenced by the TextMeshProUGUI components with GUIDs
96ae0a2159a39234f858ea23bdcc74adand35aa85d68d15435418848a03a2db81ecexist and are used across multiple files in the repository. These references are valid and intended.
- The first GUID
96ae0a2159a39234f858ea23bdcc74adis found in various.prefab,.unity, and.assetfiles.- The second GUID
35aa85d68d15435418848a03a2db81ecis similarly found in multiple files.Both GUIDs are also present in
.metafiles, confirming their proper tracking and usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the font asset references for the TextMeshProUGUI components. # Test: Check if the font assets exist. Expect: The assets to exist. fd --absolute-path --type f 96ae0a2159a39234f858ea23bdcc74ad 35aa85d68d15435418848a03a2db81ecLength of output: 215
Script:
#!/bin/bash # Description: Verify the font asset references for the TextMeshProUGUI components by searching for GUIDs in the files. # Search for the first GUID in the repository rg "96ae0a2159a39234f858ea23bdcc74ad" --type yaml --type prefab --type meta # Search for the second GUID in the repository rg "35aa85d68d15435418848a03a2db81ec" --type yaml --type prefab --type metaLength of output: 213
Script:
#!/bin/bash # Description: Verify the font asset references for the TextMeshProUGUI components by searching for GUIDs in the files. # Search for the first GUID in the repository rg "96ae0a2159a39234f858ea23bdcc74ad" --type yaml --type meta # Search for the second GUID in the repository rg "35aa85d68d15435418848a03a2db81ec" --type yaml --type metaLength of output: 181
Script:
#!/bin/bash # Description: Verify the font asset references for the TextMeshProUGUI components by searching for GUIDs in the files. # Search for the first GUID in the repository rg "96ae0a2159a39234f858ea23bdcc74ad" # Search for the second GUID in the repository rg "35aa85d68d15435418848a03a2db81ec"Length of output: 84066
Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs
This one is most critical to fix before the merge (because it could be a blocker for debug mode)
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs
Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs
Outdated
Show resolved
Hide resolved
popuz
force-pushed
the
feat/enforce-latest-version
branch
from
f4688f8 to
bc126a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (1)
Line range hint
212-305: Great work on implementing the version check feature!The code looks good and follows a clear logic flow. The use of
ApplicationVersionGuardandLauncherRedirectionScreenControllerabstracts the version check and redirection logic, making the code more maintainable.A few points to consider:
- The feature is well-guarded by the
EnableVersionUpdateGuardsetting and theversionControldebug flag, allowing flexibility in enabling/disabling the feature.- The splash screen is correctly hidden before displaying the redirection screen to provide a smooth user experience.
- Stopping the bootstrapping process is a good way to prevent users from accessing the application with an outdated version.
To further improve the architecture, consider the following suggestions:
- Extract the version check logic into a separate service class to keep the
MainSceneLoaderclass focused on scene loading responsibilities.- Define an interface for the version check service to allow for easier testing and extensibility.
- Consider using a dependency injection container to manage the dependencies between classes and improve testability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs (1 hunks)
- Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs (1 hunks)
- Explorer/Assets/DCL/Browser/DecentralandUrls/IDecentralandUrlsSource.cs (1 hunks)
- Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportCategory.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- Explorer/Assets/DCL/ApplicationVersionGuard/ApplicationVersionGuard.cs
Additional context used
Path-based instructions (5)
Explorer/Assets/DCL/Browser/DecentralandUrls/IDecentralandUrlsSource.cs (2)
Pattern
**/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.
Pattern
**/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs (2)
Pattern
**/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.
Pattern
**/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs (2)
Pattern
**/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.
Pattern
**/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportCategory.cs (2)
Pattern
**/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.
Pattern
**/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (2)
Pattern
**/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.
Pattern
**/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.
Additional comments not posted (10)
Explorer/Assets/DCL/Browser/DecentralandUrls/IDecentralandUrlsSource.cs (1)
5-7: LGTM!The addition of the constant string fields to the
IDecentralandUrlsSourceinterface is a good practice for standardizing access to important URLs related to the Decentraland project. The constant names are clear and descriptive, and the URLs follow a consistent pattern. The constants are declared with the appropriate access modifier, type, and location within the interface.These constants will likely be used by implementing classes to retrieve the latest versions of the Explorer and Launcher, thereby improving the functionality related to version management and updates.
Explorer/Assets/DCL/ApplicationVersionGuard/SemanticVersioningExtensions.cs (3)
7-8: LGTM!The
IsOlderThanextension method is well-structured and follows the single responsibility principle. It effectively utilizes theToSemanticVersionmethod for parsing and delegates the comparison logic to the privateIsOlderThanmethod.
10-15: LGTM!The private
IsOlderThanmethod correctly implements the comparison logic for semantic versions represented as tuples. It follows the semantic versioning rules by comparing the major, minor, and patch components in a sequential manner. The use of early returns enhances readability and avoids unnecessary comparisons.
17-27: LGTM!The
ToSemanticVersionmethod effectively parses a version string into a tuple format using a well-crafted regular expression. It handles various scenarios, such as optional "v" prefix and missing minor or patch numbers. The method gracefully handles invalid inputs by returning a default version of (0, 0, 0). The use of int.Parse with default values ensures that missing minor and patch numbers are treated as 0.Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs (1)
11-55: LGTM!The
LauncherRedirectionScreenControllerclass follows the MVC pattern and effectively manages the UI and user interactions related to application version updates. The class is well-structured, with clear separation of concerns and proper event handling.Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportCategory.cs (1)
286-286: LGTM!The addition of the
VERSION_CONTROLconstant to theReportCategoryclass is a valid enhancement. It expands the set of predefined report categories, allowing for better categorization and filtering of version control-related reports. The constant follows the existing naming convention and is declared aspublic, making it accessible from other parts of the codebase. The use of thenameofoperator ensures that the constant value matches the constant name, reducing the risk of typos.Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (4)
62-62: LGTM!The new field
enableVersionUpdateGuardis correctly added to theDebugSettingsclass with the appropriate type and serialization attribute.
83-83: LGTM!The
enableVersionUpdateGuardfield is correctly initialized totruein theReleasemethod, enabling the version update guard feature by default in the release build.
99-99: LGTM!The
EnableVersionUpdateGuardproperty is correctly added to theDebugSettingsclass, providing a convenient way to access theenableVersionUpdateGuardsetting based on the build type.
40-40: Verify the impact of adding a new property to the interface.Adding a new property to the
IDebugSettingsinterface is a breaking change. Ensure that all implementations of this interface have been updated to include theEnableVersionUpdateGuardproperty.Run the following script to find all implementations of the
IDebugSettingsinterface:Verification successful
To gather the necessary information for a comprehensive verification, we need to run additional scripts. Let's start by checking the implementation of the
DebugSettingsclass and searching for usage of the new property.
To complete our verification, we need to check the full implementation of the DebugSettings class to ensure the new property is correctly implemented. Let's run one more script to gather this information.
Verification complete: New property correctly implemented and used
The new property
EnableVersionUpdateGuardhas been correctly implemented in theDebugSettingsclass, which is the sole implementation of theIDebugSettingsinterface. The property is also being used in theMainSceneLoader.csfile. Here's a breakdown of the changes:
- The property is declared in the
IDebugSettingsinterface.- In the
DebugSettingsclass:
- A private field
enableVersionUpdateGuardis declared with the[SerializeField]attribute.- The field is initialized in the
Release()method.- Although not visible in the provided snippet, we know from previous results that the public property is implemented following the pattern of other properties in the class.
- The property is used in the
MainSceneLoader.csfile:bool runVersionControl = debugSettings.EnableVersionUpdateGuard;These findings confirm that the addition of the new property has been properly handled throughout the codebase, and no further actions are required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of the `IDebugSettings` interface. # Test: Search for the interface name. Expect: Only occurrences of the interface and its implementations. rg --type cs $'IDebugSettings'Length of output: 762
Script:
#!/bin/bash # Description: Check the implementation of DebugSettings class and search for usage of EnableVersionUpdateGuard # Check the implementation of DebugSettings class echo "DebugSettings class implementation:" rg --type cs -A 10 "class DebugSettings : IDebugSettings" Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs echo "\nUsage of EnableVersionUpdateGuard:" rg --type cs "EnableVersionUpdateGuard" # Search for potential implementations of IDebugSettings echo "\nPotential implementations of IDebugSettings:" rg --type cs "class.*:.*IDebugSettings"Length of output: 1357
Script:
#!/bin/bash # Description: View the full implementation of the DebugSettings class echo "Full implementation of DebugSettings class:" rg --type cs -A 50 "class DebugSettings : IDebugSettings" Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.csLength of output: 2945
Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/ApplicationVersionGuard/LauncherRedirectionScreenController.cs
Show resolved
Hide resolved
…o feat/enforce-latest-version
What does this PR change?
Close #1900
Adds check of the current explorer version on startup and if not, it shows a UI popup where user can select or to Update the version or to close the Explorer without the update. The option with Update will start Launcher (if installed) or will start downloading the Launcher from download page and close Explorer.
Feature is forced on the release and disabled for the debug mode.
How to test the changes?
--simulateVersion 0.1.0 --debug --versionControl trueparameterTest on Win, Mac and Mac arm.
Test "Update Now" with Launcher installed and with Launcher not installed machines.
Test "Update Now" with different Launcher installation folders (if supported).
Test with just parameters
--simulateVersion 0.1.0 --debug- it shouldn't popup.Our Code Review Standards
https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores