refactor(read-state): rename persisted isUnread fields to isRead#38
refactor(read-state): rename persisted isUnread fields to isRead#38johan456789 wants to merge 1 commit into
Conversation
Rename Room model fields from `isUnread` to `isRead` with proper boolean semantics while preserving database compatibility: - Article.isUnread -> Article.isRead (default false) - ArticleMeta.isUnread -> ArticleMeta.isRead (default false) - PendingReadStateOp.isUnread -> PendingReadStateOp.isRead Use @ColumnInfo(name = "isUnread") to map to the existing database column, avoiding the need for Room migrations. Update DAO parameters from storedUnread to isRead with inverted SQL logic (isUnread = NOT :isRead) to maintain correct behavior. Update all callers in services, repositories and tests. Closes #23 Made-with: Cursor
![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 refactors the article read state tracking by replacing the isUnread property with isRead across the domain models, database access objects, and services. While the goal is to simplify the logic, the current implementation introduces a critical logic inversion bug: by mapping the new isRead field to the existing isUnread database column via @ColumnInfo(name = "isUnread") without a data migration or value inversion, the application will incorrectly treat unread articles as read. Feedback also points out that several DAO method names are now inconsistent with their renamed parameters, which may cause confusion for future maintenance.
| @field:ColumnInfo(name = "isUnread") | ||
| var isRead: Boolean = false, |
There was a problem hiding this comment.
This change introduces a logic inversion bug. By renaming the field to isRead but mapping it to the existing isUnread database column via @ColumnInfo(name = "isUnread"), the boolean logic is flipped upon persistence.
In the database, isUnread = 1 (true) represents an unread article. When Room loads this value into the new isRead field, it will set isRead = true, meaning the application will treat unread articles as read.
To fix this while avoiding database migrations, you should keep the underlying field named isUnread (matching the database semantics) and use a computed property for isRead (reverting the changes in lines 57-61). If you truly want to rename the persisted field, you must provide a database migration to rename the column and invert all existing boolean values (isRead = NOT isUnread).
| @field:ColumnInfo(name = "isUnread") | ||
| var isRead: Boolean = false, |
There was a problem hiding this comment.
Similar to the Article model, renaming this field to isRead while mapping it to the isUnread column inverts the logic. Articles that are unread in the database (isUnread = 1) will be loaded as isRead = true. This will break the read/unread state tracking throughout the application.
| @ColumnInfo(name = "isUnread") | ||
| val isRead: Boolean, |
There was a problem hiding this comment.
Renaming this field to isRead while mapping it to the isUnread column inverts the logic. Pending operations stored in the database will be interpreted with the opposite meaning when loaded, leading to incorrect synchronization with remote services.
| text: String, | ||
| groupId: String, | ||
| isUnread: Boolean, | ||
| isRead: Boolean, |
There was a problem hiding this comment.
The parameter has been renamed to isRead, but the method name searchArticleByGroupIdWhenIsUnread still contains 'WhenIsUnread'. This creates a confusing API where passing isRead = true to a 'WhenIsUnread' method returns read articles. If the refactor proceeds (after fixing the logic inversion in the models), these methods should be renamed to something more neutral like searchArticleByGroupIdByReadState to maintain clarity.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f21779d12e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @field:ColumnInfo(name = "isUnread") | ||
| var isRead: Boolean = false, |
There was a problem hiding this comment.
Article.isRead is now bound directly to the legacy isUnread column, but Room does not invert booleans. That means rows stored as unread (isUnread = 1) are materialized as isRead = true, and inserts with isRead = true persist back as unread. Code paths that trust article.isRead (for example read/unread UI state and diff toggling) will therefore treat unread items as read and can apply the opposite action. Keep an explicit inversion layer when mapping this column.
Useful? React with 👍 / 👎.
| @field:ColumnInfo(name = "isUnread") | ||
| var isRead: Boolean = false, |
There was a problem hiding this comment.
ArticleMeta.isRead is now also mapped directly from isUnread, so metadata-based sync logic reads inverted states. In GoogleReaderRssService, local unread/read sets are built from queryMetadataAll via filterNot { it.isRead } and filter { it.isRead }; with this mapping those sets are swapped, so reconciliation can miss true local-vs-remote read mismatches and skip necessary updates. ArticleMeta.isRead must remain inverted from the stored unread flag (or SQL should project an inverted value).
Useful? React with 👍 / 👎.
1 participant
Summary
isUnreadtoisRead(Article,ArticleMeta,PendingReadStateOp)@ColumnInfo(name = "isUnread")to map to existing database column, avoiding migrationsTest plan
./gradlew assembleDebug)./gradlew testFdroidDebugUnitTest)Closes #23
Made with Cursor