Update repo tracking system - respect repo#125
Update repo tracking system - respect repo#125Moustachauve merged 45 commits intoMoustachauve:devfrom
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.
app/src/main/java/ca/cgagnier/wlednativeandroid/repository/DevicesDatabase.kt
Outdated
Show resolved
Hide resolved
| 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
app/src/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/deviceEdit/DeviceEditViewModel.kt
Outdated
Show resolved
Hide resolved
…/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.
app/src/main/java/ca/cgagnier/wlednativeandroid/repository/DevicesDatabase.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ca/cgagnier/wlednativeandroid/model/Version.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/deviceEdit/DeviceEditViewModel.kt
Show resolved
Hide resolved
app/src/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/deviceEdit/DeviceEditViewModel.kt
Show resolved
Hide resolved
...c/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/list/DeviceWebsocketListViewModel.kt
Show resolved
Hide resolved
...c/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/list/DeviceWebsocketListViewModel.kt
Outdated
Show resolved
Hide resolved
...c/main/java/ca/cgagnier/wlednativeandroid/ui/homeScreen/list/DeviceWebsocketListViewModel.kt
Show resolved
Hide resolved
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
Think that is all correctly moved over to the new branch now |
|
The code doesn't pass lint checks. I tried to run it on my emulator and I get this error: |
- Add defaultValue = "'wled/WLED'" to Asset.repository field - Matches the DEFAULT 'wled/WLED' specified in migration SQL - Resolves "Migration didn't properly handle: Asset" error - Ensures consistency between entity annotations and actual database schema Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Add backticks around all table names in SQL statements - Add backticks around all column names in SQL statements - Add backticks around index names in CREATE INDEX statements - Update FOREIGN KEY to include "ON UPDATE NO ACTION" clause - Ensures migration SQL matches Room's generated schema exactly - Resolves schema validation errors for indices and tables Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
…tem' into copilot/update-repo-tracking-system
… by repository, and update repository method to handle deletions and insertions efficiently
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces multi-repository support for WLED device updates, allowing the application to fetch and manage releases from different WLED forks or brands. This involved updating the database schema for Version and Asset models to include a repository field, along with corresponding database migrations (9->10 and 10->11) to handle existing data. The API and service layers were refactored to pass repository information to GitHub API calls and database queries. A new WebsocketClientManager was added to centralize WebSocket client management, enabling the MainViewModel and DeviceEditViewModel to dynamically identify and refresh release information for all connected devices based on their reported repositories, rather than a single hardcoded one. The DeviceUpdateService was also updated to use a more flexible asset matching logic. Additionally, the app's version code and name were updated.
| githubOwner = "wled", | ||
| githubRepo = "WLED" |
There was a problem hiding this comment.
The githubOwner for OFFICIAL_WLED has been changed from "Aircoookie" to "wled". Please confirm this is an intentional change to reflect an updated official repository owner or primary source for WLED releases. This impacts where the application fetches official updates from.
There was a problem hiding this comment.
Yes this was renamed about a year ago
- Fix UpdateSourceDefinition list formatting - split items properly - Remove typo 't' from splitRepository documentation comment - Ensures code passes ktlint/spotless checks Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
…lines - Remove all trailing whitespace from DbMigration9To10.kt and ReleaseService.kt - Split FOREIGN KEY constraint in DbMigration9To10.kt to fit 120 char limit - Split long @query annotations in VersionDao.kt to multiline format - All lines now under 120 character limit - All trailing whitespace removed - Code now passes ktlint/spotless formatting checks Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the app's update mechanism to support multiple firmware repositories, moving away from a hardcoded repository. The changes are extensive, touching the database schema, data access objects, services, and view models. Key improvements include a more robust database update strategy that avoids deleting all cached data, and cleaner, more modular update-checking logic. I've found a couple of areas for improvement regarding the database migration strategy and code safety, which are detailed in the comments.
app/src/main/java/ca/cgagnier/wlednativeandroid/service/update/DeviceUpdateService.kt
Outdated
Show resolved
Hide resolved
| createNewTables(db) | ||
| migrateVersionData(db) | ||
| migrateAssetData(db) | ||
| dropOldTables(db) |
There was a problem hiding this comment.
The MIGRATION_10_11 seems redundant because this migration already drops the _old tables, which means the migration from version 10 to 11 will have no effect. To make the two-step migration meaningful and allow for inspection or easier rollback, consider removing the call to dropOldTables(db) from this migration. The MIGRATION_10_11 will then serve as the cleanup step as intended.
There was a problem hiding this comment.
Looks like this is a regression than has snuck in and should be fixed before merge @Moustachauve
…/DeviceUpdateService.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>
Issue 1 - Race Condition Fix: - Added repository field to Device entity with default "wled/WLED" - WebsocketClient now stores repository when device info is received - DeviceFirstContactService stores repository when creating/updating devices - MainViewModel.downloadUpdateMetadata() now queries devices from database instead of websockets - Eliminates race condition where websockets haven't connected yet on app launch - Ensures all device repositories are refreshed during 24h background checks Issue 2 - Remove Redundant Migration: - Deleted DbMigration10To11.kt (was only dropping already-dropped tables) - Deleted schema 11.json (identical to 10.json) - Consolidated all changes into single 9→10 migration - Updated MIGRATION_9_10 to also add repository column to Device2 table - Database version remains at 10 as requested Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
… field and modify update logic to handle null info
|
/gemini review - does 39bfdda address your concerns regarding the migration? |
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of refactoring the application to support multiple firmware repositories, moving away from a hardcoded value. The changes are comprehensive, touching the data models, database schema and migration, data access objects, and services correctly. The logic for determining the repository and refreshing version information on a per-repository basis is well-implemented and improves both flexibility and efficiency. The introduction of WebsocketClientManager is a good solution for sharing state between ViewModels. Overall, this is a solid architectural improvement.
|
|
||
| /** | ||
| * Splits a repository string (e.g., "owner/name") into owner and name parts for API calls. | ||
| * Returns a pair of (owner, name). Defaults to ("wled", "WLED") if format is invalid.t |
There was a problem hiding this comment.
There is a small typo at the end of this comment line.
| * Returns a pair of (owner, name). Defaults to ("wled", "WLED") if format is invalid.t | |
| * Returns a pair of (owner, name). Defaults to ("wled", "WLED") if format is invalid. |
Moustachauve
added
enhancement
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