search

lydavid / MusicSearch

An Android app for browsing songs, artists, and anything related to them
Apache License 2.0
18 stars 0 forks source link

feat: browse artists by area #918

Closed lydavid closed 1 month ago

sweep-ai[bot] commented 1 month ago

Sweep: PR Review

data/database/src/commonMain/kotlin/ly/david/musicsearch/data/database/DatabaseDaoModule.kt

The changes add ArtistsByEntityDao to the import statements and the dependency injection module, ensuring it can be used throughout the application.

data/database/src/commonMain/kotlin/ly/david/musicsearch/data/database/dao/ArtistsByEntityDao.kt

A new DAO class ArtistsByEntityDao has been added to manage database operations for the Artists_by_entity table, including insertions, deletions, and queries with support for coroutines and paging.

Sweep Found These Issues

  • The getNumberOfArtistsByEntity method uses a hardcoded query string "%%" which may not be necessary and could lead to unexpected results.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Fdatabase%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Fdatabase%2Fdao%2FArtistsByEntityDao.kt#L53 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-4108e79bfe6b1756db485e1fa86f38132e81642556642b65e889308c1f1858b3R53)
  • The insertAll method does not handle potential exceptions that could occur during the transaction, which might leave the database in an inconsistent state.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Fdatabase%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Fdatabase%2Fdao%2FArtistsByEntityDao.kt#L36-L43 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-4108e79bfe6b1756db485e1fa86f38132e81642556642b65e889308c1f1858b3R36-R43)
data/database/src/commonMain/sqldelight/ly.david.musicsearch.data.database/artists_by_entity.sq

Introduced a new table artists_by_entity and associated SQL operations for inserting, deleting, counting, and retrieving artists by entity.

Sweep Found These Issues

  • The deleteArtistsByEntity query incorrectly references the artist table instead of artists_by_entity for deletion, which could lead to unintended data loss.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Fdatabase%2Fsrc%2FcommonMain%2Fsqldelight%2Fly.david.musicsearch.data.database%2Fartists_by_entity.sq#L12-L18 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-538b6cd79e58d9b230b36b3337b5ba20ccd9266cba9f2c325adc7229e6de1881R12-R18)
data/database/src/commonMain/sqldelight/migrations/5.sqm

A new table artists_by_entity is created with entity_id and artist_id as non-null columns and a composite primary key.

data/musicbrainz/src/commonMain/kotlin/ly/david/musicsearch/data/musicbrainz/api/BrowseApi.kt

The changes generalize the method for browsing artists to support various entity types by introducing a new parameter for the entity type and updating the request URL construction accordingly.

Sweep Found These Issues

  • The change from collectionId to entityId and the addition of entity requires careful handling to ensure that the correct resourceUri is used for different entity types, which could introduce bugs if not properly managed.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Fmusicbrainz%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Fmusicbrainz%2Fapi%2FBrowseApi.kt#L45-L46 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-02609d7950ab2a9856856872e1caca60616361f662c68a846181cafe74961fa7R45-R46)
Potential Issues

Sweep isn't 100% sure if the following are issues or not but they may be worth taking a look at.

  • The dynamic construction of the request URL using entity.resourceUri could fail if resourceUri is not correctly defined for all possible MusicBrainzEntity values.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Fmusicbrainz%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Fmusicbrainz%2Fapi%2FBrowseApi.kt#L209-L210 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-02609d7950ab2a9856856872e1caca60616361f662c68a846181cafe74961fa7R209-R210)
data/repository/src/commonMain/kotlin/ly/david/musicsearch/data/repository/artist/ArtistsByEntityRepositoryImpl.kt

The changes introduce the ArtistsByEntityDao to handle database operations for artists by entity, expanding the functionality of several methods to support more entity types without raising errors.

Sweep Found These Issues

  • The deleteLinkedEntitiesByEntity method now deletes artists for all entity types without checking if the entity type is valid, which could lead to unintended deletions.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Frepository%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Frepository%2Fartist%2FArtistsByEntityRepositoryImpl.kt#L58-L60 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-fb8f07fd938c30b618b1d19f2dfecb60a142f00a17c55e9f939e2fba68781f51R58-R60)
  • The getLinkedEntitiesPagingSource method now retrieves artists for all entity types without checking if the entity type is valid, which could lead to incorrect data retrieval.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Frepository%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Frepository%2Fartist%2FArtistsByEntityRepositoryImpl.kt#L78-L83 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-fb8f07fd938c30b618b1d19f2dfecb60a142f00a17c55e9f939e2fba68781f51R78-R83)
  • The insertAllLinkingModels method now inserts artist-entity links for all entity types without checking if the entity type is valid, which could lead to incorrect data insertion.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/data%2Frepository%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fdata%2Frepository%2Fartist%2FArtistsByEntityRepositoryImpl.kt#L113-L119 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-fb8f07fd938c30b618b1d19f2dfecb60a142f00a17c55e9f939e2fba68781f51R113-R119)
docs/all_features.md

The change updates the documentation to reflect that users can now browse artists when viewing the details of an area.

shared/feature/details/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/details/DetailsFeatureModule.kt

Added a new dependency artistsByEntityPresenter to the AreaPresenter in the detailsFeatureModule.

shared/feature/details/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/details/area/AreaPresenter.kt

The changes introduce functionality to handle artist-related data within the AreaPresenter by adding new dependencies, initializing UI state, handling events, and updating the overall state management.

Potential Issues

Sweep isn't 100% sure if the following are issues or not but they may be worth taking a look at.

  • The new artistsByEntityPresenter dependency is not checked for nullability, which could lead to a NullPointerException if it is not properly initialized.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/shared%2Ffeature%2Fdetails%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fshared%2Ffeature%2Fdetails%2Farea%2FAreaPresenter.kt#L39 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-65070e3f423ef23d0a1136bc2db90ca1d8f3b4d2e574f5938a7d9b17736d581fR39)
shared/feature/details/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/details/area/AreaTab.kt

A new enum constant ARTISTS has been added to the AreaTab enum class.

shared/feature/details/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/details/area/AreaUi.kt

The changes introduce a new tab for displaying a list of artists associated with an area, using the ArtistsListScreen composable.

Potential Issues

Sweep isn't 100% sure if the following are issues or not but they may be worth taking a look at.

  • The lazyListState for the ArtistsListScreen is set to eventsLazyListState, which may cause issues as it should likely have its own distinct state.
    • https://github.com/lydavid/MusicSearch/blob/d401004d5fc0cc98af0145b06dbb84b393fb6326/shared%2Ffeature%2Fdetails%2Fsrc%2FcommonMain%2Fkotlin%2Fly%2Fdavid%2Fmusicsearch%2Fshared%2Ffeature%2Fdetails%2Farea%2FAreaUi.kt#L193 [View Diff](https://github.com/lydavid/MusicSearch/pull/918/files#diff-f2d509b9fa3bab8247e59e6f8e02160b1519b1a2aab0292524b87eb944131e52R193)
shared/feature/details/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/details/area/AreaUiState.kt

The changes include adding an import statement for ArtistsByEntityUiState and reordering the properties within the AreaUiState data class.

shared/feature/stats/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/stats/AreaStatsPresenter.kt

The changes introduce functionality to collect and include artist statistics in the StatsUiState by adding a new DAO dependency and state variables for remote and local artist counts.

shared/feature/stats/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/stats/Stats.kt

A new ArtistStats data class has been added and integrated into the Stats data class to include artist-related statistics.

shared/feature/stats/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/stats/StatsFeatureModule.kt

A new dependency artistsByEntityDao was added to the AreaStatsPresenter in the statsFeatureModule.

shared/feature/stats/src/commonMain/kotlin/ly/david/musicsearch/shared/feature/stats/StatsUi.kt

A new case for Tab.ARTISTS has been added to display artist statistics in the StatsUi function.

strings/src/commonMain/kotlin/ly/david/musicsearch/strings/AppStrings.kt

Two new properties, artists and cachedArtists, were added to the AppStrings data class to enhance string resource management and formatting capabilities.

strings/src/commonMain/kotlin/ly/david/musicsearch/strings/EnStrings.kt

The changes introduce new localized strings for "Artists" and a template for displaying cached artists information.

test-data/src/commonMain/kotlin/ly/david/data/test/api/FakeMusicBrainzApi.kt

The method browseArtistsByCollection was renamed to browseArtistsByEntity, with the parameter collectionId replaced by entityId and a new parameter entity added.

ui/common/src/commonMain/kotlin/ly/david/ui/common/topappbar/Tab.kt

The changes introduce a new ARTISTS value to the Tab enum and update the getTitle function to return the appropriate title for this new tab.