Update repo tracking system - respect repo#125
Update repo tracking system - respect repo#125netmindz wants to merge 12 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>
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