Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a crash that occurs when refreshing the feed with certain Wikipedia language combinations (e.g., Kannada Wikipedia with Hindi UI language). The crash was related to null pointer exceptions in the shared transition system when content became null during refresh.
Changes:
- Replaced generic fallback keys (
"title","desc","imgsrc") with context-specific prefixed keys - Added null checks before applying sharedBounds modifiers
- Removed nested SharedTransitionLayout from AsyncInfobox and passed parent scope down
- Fixed NPE for null views count in trending articles
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ArticleFeed.kt | Updated shared transition keys with context-specific prefixes (tfa-, trending-, news-, otd-, potd-) and added null checks; fixed NPE for views |
| PageContent.kt | Updated shared transition keys with "article-" prefix for titles and descriptions |
| AsyncInfobox.kt | Removed nested SharedTransitionLayout, now receives parent SharedTransitionScope as parameter; removed shared transitions for internal title animation |
| ImageCard.kt | Updated image shared key to use "article-image-" prefix with null check |
| PageImage.kt | Updated shared keys for both article images and gallery images with prefixes and null checks |
| ImageWithCaption.kt | Updated caption image and description keys with "caption-" prefixes |
| FullScreenImageTopBar.kt | Updated key for caption description to match ImageWithCaption |
| ParsedBodyText.kt | Added sharedScope parameter to pass down to AsyncInfobox |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/org/nsh07/wikireader/ui/homeScreen/PageContent.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/nsh07/wikireader/ui/homeScreen/PageContent.kt
Outdated
Show resolved
Hide resolved
| photo.source?.let { src -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "article-image-$src" |
There was a problem hiding this comment.
The shared transition key "article-image-$src" in PageImage.kt (used by ImageCard.kt which is called from PageContent.kt) doesn't match the keys used in ArticleFeed.kt. When navigating from the feed to an article, the shared element transition won't work because ArticleFeed uses keys like "tfa-image-$src" for the Today's Featured Article, "trending-$page-$i-image-$src" for trending articles, and "potd-image-$src" for the Picture of the Day. For shared element transitions to work across navigation, the keys must match exactly on both the source and destination screens.
| feedContent.tfa.description?.let { description -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "tfa-desc-$description" |
There was a problem hiding this comment.
Using the full description text as part of the shared transition key (e.g., "tfa-desc-$description") could be problematic if the description is very long. Long keys may cause performance issues or exceed practical limits. Consider using a hash or a stable unique identifier instead of the full text content for descriptions and titles.
| feedContent.tfa.titles?.normalized?.let { title -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "tfa-title-$title" |
There was a problem hiding this comment.
The PR description states that feed titles should use "article-title-{canonical_title_or_pageId}" to match with article titles, but the code uses "tfa-title-$title" where $title is the normalized title. This creates multiple problems: (1) the prefix doesn't match PageContent.kt which uses "article-title-", (2) the value uses normalized title instead of canonical title as described, and (3) when loadPage is called with the canonical title, it won't match the key that used normalized title.
| feedContent.mostReadArticles[i].description?.let { description -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "trending-$page-$i-desc-$description" |
There was a problem hiding this comment.
Including page and index variables in the shared transition key ("trending-$page-$i-desc-$description") makes it impossible for the destination screen to use a matching key. The article page has no knowledge of which pager page or index the article came from. This will prevent shared element transitions from working.
| photo.source?.let { src -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "article-image-$src" |
There was a problem hiding this comment.
The shared transition key "article-image-$src" in ImageCard.kt (used by PageContent.kt) doesn't match the keys used in ArticleFeed.kt. When navigating from the feed to an article, the shared element transition won't work because ArticleFeed uses keys like "tfa-image-$src" for the Today's Featured Article, "trending-$page-$i-image-$src" for trending articles, and "potd-image-$src" for the Picture of the Day. For shared element transitions to work across navigation, the keys must match exactly on both the source and destination screens.
| feedContent.tfa.originalImage?.source?.let { src -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "tfa-image-$src" |
There was a problem hiding this comment.
There is a significant discrepancy between the PR description and the actual code changes. The PR description states that feed images should use the key pattern "article-image-{source_uri}" to match with the article header image, but the code uses "tfa-image-$src". This means the shared element transition will not work when navigating from the Today's Featured Article to the article page, as the keys don't match. The implementation doesn't follow the stated key mapping strategy in the PR description.
| feedContent.mostReadArticles[i].titles?.normalized?.let { title -> | ||
| Modifier.sharedBounds( | ||
| sharedContentState = rememberSharedContentState( | ||
| "trending-$page-$i-title-$title" |
There was a problem hiding this comment.
Including page and index variables in the shared transition key ("trending-$page-$i-title-$title") makes it impossible for the destination screen to use a matching key. The article page (PageContent.kt) has no knowledge of which pager page or index the article came from. This will prevent shared element transitions from working. Keys should only include data that is known on both the source and destination screens, such as the article title or a unique article identifier.
app/src/main/java/org/nsh07/wikireader/ui/homeScreen/ArticleFeed.kt
Outdated
Show resolved
Hide resolved
|
Can you apply the fixes suggested by copilot? Otherwise it looks good to me. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
Why? Can you not fix the issues in this PR itself? |
…nt.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…nt.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ed.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ed.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ed.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Hey, I tried using “Implement all suggestions” earlier and it ended up commenting instead. I’ve now committed the suggestions one by one. Could you take a look? |
Problem
Refreshing the feed crashes the app when Wikipedia language is set to Kannada (kn.wikipedia.org) and the App/Interface language is Hindi. This happens after applying the fix for #315 and reproduces on some other language combinations as well. The crash occurs during pull-to-refresh on the Home/Feed screen while shared transitions are active.
Stack Trace Sample
java.lang.NullPointerException at androidx.compose.animation.SharedTransitionStateMachine.configureActiveMatch(...) at ... during lookahead layout phase ...Solution
Implemented a two-part fix:
Make shared transition keys unique + remove collision fallbacks
"title","desc","imgsrc") with stable, prefixed, unique keys."{context}-{element}-{unique_id}".Guard sharedBounds against null + stabilize transition scope
sharedBoundsonly when the key is non-null and stable. If null, returnModifierwith no shared transition.SharedTransitionLayoutinsideAsyncInfobox.ktand instead pass the parent SharedTransitionScope down to child composables.Changes
Files Modified
ArticleFeed.kt
rememberSharedContentState(...)calls to use prefixed, unique keys."title","desc","imgsrc".sharedBounds(...)usage with a null-check.df.format(views)→df.format(views ?: 0)PageContent.kt
AsyncInfobox.kt
SharedTransitionLayout.ImageCard.kt
PageImage.kt
ImageWithCaption.kt
FullScreenImageTopBar.kt
Key Mapping Strategy
To ensure correct shared-element pairing across screens:
Feed article image → Article header image → Fullscreen image
article-image-{source_uri}Feed article title → Article title
article-title-{canonical_title_or_pageId}Feed article description → Article description
article-desc-{canonical_title_or_pageId}Gallery image → Fullscreen gallery image
gallery-image-{uri}Testing
2025.10.00) causes further instability.