Update repo tracking system - respect repo#125
Update repo tracking system - respect repo#125netmindz wants to merge 21 commits intoMoustachauve:mainfrom
Conversation
- Added 'repo' field to Info model to capture repository from /json/info - Updated Version and Asset models to include repository field (as "owner/name" string) - Created database migrations (9->10->11) for repository support - Modified ReleaseService to fetch from multiple repositories - Updated DeviceUpdateManager to use repo field with fallback to "wled/WLED" - Changed default repository from "Aircoookie/WLED" to "wled/WLED" - Updated MainViewModel to collect repositories from connected devices - Modified queries and repository methods to filter by repository Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Add @Inject annotation to ReleaseService for dependency injection - Update GithubApi.downloadReleaseBinary to use repository from Asset - Add DeviceStateInfo import and clean up type annotation - Remove unused DEFAULT_REPO constants from GithubApi Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
The repo field was added in WLED 0.15.2, not 0.16 as previously documented in the code comment. Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Restored UpdateSourceType enum, UpdateSourceDefinition data class, and UpdateSourceRegistry object - Updated getRepositoryFromInfo() to use three-tier fallback strategy: 1. First: Use info.repo field if present (WLED 0.15.2+) 2. Second: Use UpdateSourceRegistry brand pattern matching 3. Third: Default to "wled/WLED" - Updated registry to use "wled/WLED" instead of "Aircoookie/WLED" for consistency - Maintains backward compatibility for devices without repo field - QuinLED devices will be correctly identified via brand matching Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Inject WebsocketClient map into DeviceEditViewModel - Update checkForUpdates() to collect repositories from all connected devices - Now matches MainViewModel's approach: default + unique list of repos - Ensures updates are fetched for all device types (QuinLED, MoonModules, etc.) Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Changed from collecting all device repositories to just the selected device - Lookup device by macAddress in websocketClients map - Only refresh that specific device's repository plus default fallback - More appropriate for device-specific edit context - MainViewModel still handles refreshing all repositories globally Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Summary of ChangesHello @netmindz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's update tracking system by introducing dynamic repository awareness. Previously, the app relied on a hardcoded GitHub repository for WLED updates. With these changes, the system can now identify and track updates from different WLED forks or custom repositories by leveraging a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, moving from a hardcoded repository to a dynamic system that respects the repo field from the device's info. This will correctly handle updates for forks of WLED. The database schema changes, API calls, and view models have been updated accordingly. I've found a critical issue with the database migration that will lead to data loss, a couple of high-severity bugs related to repository handling, and some medium-severity suggestions to improve maintainability and adhere to best practices like dependency injection. No comments were dropped or modified based on the provided rules.
| AutoMigration(from = 9, to = 10, spec = DbMigration9To10::class), | ||
| AutoMigration(from = 10, to = 11, spec = DbMigration10To11::class), |
There was a problem hiding this comment.
The current auto-migration strategy for versions 9 to 11 will result in data loss for existing users. The AutoMigration from 9 to 10 renames the Version and Asset tables, and Room then creates new, empty tables. The data from the old tables is never copied over before they are dropped in the migration from 10 to 11. This means all cached release information will be lost upon app update.
To fix this, you should use a manual Migration instead of AutoMigration for the 9 to 10 transition. In a manual migration, you can use SQL to create new tables, copy data from the old tables to the new ones (while populating the new repository column with a default value), and then drop the old tables.
| suspend fun refreshVersions(githubApi: GithubApi, repositories: Set<String>) = withContext(Dispatchers.IO) { | ||
| val allVersions = mutableListOf<Version>() | ||
| val allAssets = mutableListOf<Asset>() | ||
|
|
||
| for (repository in repositories) { | ||
| val (repoOwner, repoName) = splitRepository(repository) | ||
| Log.i(TAG, "Fetching releases from $repository") | ||
| githubApi.getAllReleases(repoOwner, repoName).onFailure { exception -> | ||
| Log.w(TAG, "Failed to refresh versions from $repository", exception) | ||
| }.onSuccess { releases -> | ||
| if (releases.isEmpty()) { | ||
| Log.w(TAG, "GitHub returned 0 releases for $repository.") | ||
| } else { | ||
| val versions = releases.map { createVersion(it, repository) } | ||
| val assets = releases.flatMap { createAssetsForVersion(it, repository) } | ||
| allVersions.addAll(versions) | ||
| allAssets.addAll(assets) | ||
| Log.i(TAG, "Added ${versions.size} versions and ${assets.size} assets from $repository") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Log.i(TAG, "Replacing DB with ${versions.size} versions and ${assets.size} assets") | ||
| versionWithAssetsRepository.replaceAll(versions, assets) | ||
| if (allVersions.isNotEmpty()) { | ||
| Log.i(TAG, "Replacing DB with ${allVersions.size} versions and ${allAssets.size} assets total") | ||
| versionWithAssetsRepository.replaceAll(allVersions, allAssets) | ||
| } | ||
| } |
There was a problem hiding this comment.
The refreshVersions function calls versionWithAssetsRepository.replaceAll, which deletes all entries from the version and asset tables before inserting the newly fetched ones. If fetching releases for one repository fails (e.g., due to a temporary network issue) while others succeed, the cached versions for the failing repository will be lost.
A more robust approach would be to delete and replace versions on a per-repository basis only for the repositories that were successfully fetched. This would involve:
- Adding a
deleteByRepository(repository: String)function toVersionDaoandAssetDao. - Modifying
VersionWithAssetsRepositoryto have a method that performs deletion for a specific set of repositories before inserting new data, all within a single transaction. - Updating this
refreshVersionsfunction to call the new repository method, passing the list of repositories that were successfully updated.
app/src/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/deviceEdit/DeviceEditViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ca/cgagnier/wlednativeandroid/service/update/ReleaseService.kt
Show resolved
Hide resolved
| @@ -109,7 +116,24 @@ class DeviceEditViewModel @Inject constructor( | |||
| repository.update(updatedDevice) | |||
| try { | |||
| val releaseService = ReleaseService(versionWithAssetsRepository) | |||
There was a problem hiding this comment.
ReleaseService is being instantiated directly here. Since ReleaseService is an injectable class (annotated with @Inject), it should be provided by Hilt via the ViewModel's constructor. This improves testability and follows dependency injection best practices.
…/ReleaseService.kt Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Will Tatam <will@netmindz.net>
…deviceEdit/DeviceEditViewModel.kt Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Will Tatam <will@netmindz.net>
- Made DEFAULT_REPO public in ReleaseService.kt - Import DEFAULT_REPO in MainViewModel and DeviceEditViewModel - Removes duplicate constant definitions - Ensures single source of truth for default repository value - Improves maintainability Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Implement onPostMigrate() to copy data from old tables to new ones - Copy all Version records from Version_old to Version with default repository "wled/WLED" - Copy all Asset records from Asset_old to Asset with default repository "wled/WLED" - Add comprehensive logging to track migration success - Prevents data loss that would have occurred without data copying - Follows same pattern as DbMigration7To8 - Migration 10→11 will then safely delete the old tables Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Inject ReleaseService via constructor instead of manual instantiation - Matches dependency injection pattern used in MainViewModel - Removes tight coupling and improves testability - Follows Hilt DI best practices - Addresses code review feedback from gemini-code-assist Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
|
Can you re-trigger the gemini-code-assist run @Moustachauve to see if everything it flagged is resolved now? |
|
Hello @netmindz, I cannot directly re-trigger a run. If you would like me to perform another code review on the current state of the pull request, please use the command |
…/ReleaseService.kt Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Will Tatam <will@netmindz.net>
…e. Since ReleaseService is an injectable class (annotated with @Inject), it should be provided by Hilt via the ViewModel's constructor. This improves testability and follows dependency injection best practices." Not sure if all this is needed?
…acy of the repo field
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request updates the application to dynamically track repositories for updates, moving away from hardcoded values. This involves significant changes to the database schema, data models, and update service logic to incorporate a repository field. New database migrations (MIGRATION_9_10 and MIGRATION_10_11) are introduced to handle the schema evolution, ensuring existing data is migrated to the new structure with a default repository. The GithubApi and ReleaseService are refactored to accept repository information, allowing them to fetch and manage releases from multiple sources. Additionally, a WebsocketClientManager is added to provide a centralized way to access active WebSocket clients, which is then used to determine the repositories associated with connected devices for update checks. The UI components (MainViewModel, DeviceEditViewModel, DeviceEdit) are updated to pass repository information when checking for and displaying update details. Overall, the changes improve the flexibility and maintainability of the update tracking system by supporting multiple WLED forks/repositories.
| import ca.cgagnier.wlednativeandroid.repository.migrations.MIGRATION_9_10 | ||
| import ca.cgagnier.wlednativeandroid.repository.migrations.MIGRATION_10_11 |
There was a problem hiding this comment.
The migration files are imported but not used in the autoMigrations array. While they are added to addMigrations later, it's good practice to keep imports clean if they are not directly referenced in the autoMigrations array.
| @ColumnInfo(index = true) | ||
| val repository: String, |
There was a problem hiding this comment.
Adding repository to the Asset data class with an index is a good move. This will improve query performance when filtering assets by repository, especially as the number of repositories and assets grows.
| @Entity( | ||
| primaryKeys = ["tagName", "repository"] | ||
| ) |
There was a problem hiding this comment.
Changing the primary key to include repository is essential for supporting multiple repositories. This ensures that tagName alone is not considered unique across different forks.
| @ColumnInfo(defaultValue = "'wled/WLED'") | ||
| val repository: String, |
There was a problem hiding this comment.
Adding repository with a default value to the Version data class is a good way to handle existing data during migration and ensure new entries have a sensible default. The ColumnInfo annotation is correctly used here.
| fun getPreviewVersion(): Version { | ||
| return Version( | ||
| tagName = "v1.0.0", | ||
| repository = "wled/WLED", |
There was a problem hiding this comment.
Adding the repository field to the getPreviewVersion function ensures that the preview version is correctly associated with a repository, maintaining consistency with the updated Version data class.
| // Look up the specific device's websocket client to get its repository | ||
| val client = websocketClientManager.getClients()[device.macAddress] | ||
| val info = client?.deviceState?.stateInfo?.value?.info | ||
| if (info != null) { | ||
| val repo = getRepositoryFromInfo(info) | ||
| repositories.add(repo) | ||
| Log.d(TAG, "Refreshing versions for device repository: $repo") | ||
| } else { | ||
| Log.d(TAG, "Device info not available, using default repository only") | ||
| } |
There was a problem hiding this comment.
This block correctly retrieves the info from the specific device's WebSocket client and uses getRepositoryFromInfo to add its repository to the set. This ensures that when checking for updates, the device's own repository is included.
| Log.i(TAG, "Refreshing versions from ${repositories.size} repositories: $repositories") | ||
| releaseService.refreshVersions(githubApi, repositories) |
There was a problem hiding this comment.
The releaseService.refreshVersions call now correctly passes the collected set of repositories. This ensures that the update service fetches releases from all relevant sources, including the device's specific repository.
| import ca.cgagnier.wlednativeandroid.service.update.DeviceUpdateManager | ||
| import ca.cgagnier.wlednativeandroid.service.websocket.DeviceWithState | ||
| import ca.cgagnier.wlednativeandroid.service.websocket.WebsocketClient | ||
| import ca.cgagnier.wlednativeandroid.service.websocket.WebsocketClientManager |
There was a problem hiding this comment.
Importing WebsocketClientManager is necessary to integrate the new manager for accessing active WebSocket clients.
| private val okHttpClient: OkHttpClient, | ||
| private val moshi: Moshi | ||
| private val moshi: Moshi, | ||
| private val websocketClientManager: WebsocketClientManager |
There was a problem hiding this comment.
Injecting WebsocketClientManager into DeviceWebsocketListViewModel is correct, as this ViewModel is responsible for managing and providing access to the active WebSocket clients.
| // Update the manager so other components can access the clients | ||
| websocketClientManager.updateClients(updatedClients) |
There was a problem hiding this comment.
Updating the websocketClientManager with the updatedClients map ensures that other components can access the most current list of active WebSocket clients. This centralizes client management.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Update the app to move away from hard coded lookup for updates from the old
AirCoookie/WLEDto use the newrepofield added with wled/WLED#4944 to ensure that the correct repo is referenced when looking for updates.I can't actually test at the moment as you can't to Android dev from an arm64 linux setup, but the code changed look reasonable
See netmindz#1 for more details