forked from hidroh/materialistic
-
-
Notifications
You must be signed in to change notification settings - Fork 1
Expand file tree
/
Copy pathcycle2_feedback.txt
More file actions
2983 lines (2983 loc) · 320 KB
/
cycle2_feedback.txt
File metadata and controls
2983 lines (2983 loc) · 320 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
788
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
827
828
829
830
831
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855
856
857
858
859
860
861
862
863
864
865
866
867
868
869
870
871
872
873
874
875
876
877
878
879
880
881
882
883
884
885
886
887
888
889
890
891
892
893
894
895
896
897
898
899
900
901
902
903
904
905
906
907
908
909
910
911
912
913
914
915
916
917
918
919
920
921
922
923
924
925
926
927
928
929
930
931
932
933
934
935
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
955
956
957
958
959
960
961
962
963
964
965
966
967
968
969
970
971
972
973
974
975
976
977
978
979
980
981
982
983
984
985
986
987
988
989
990
991
992
993
994
995
996
997
998
999
1000
Found new feedback: {'reviews': 11, 'inline': 17, 'general': 19}
[
{
"id": 3675907152,
"node_id": "PRR_kwDOQBDOvc7bGeRQ",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "Hey - I've found 2 issues, and left some high level feedback:\n\n- In LazyLoadFragment, the new eagerLoad() implementation only checks isEagerLoad() and no longer guards on an unloaded state, so load() can now be invoked multiple times; consider tracking and checking an isLoaded flag (from LazyLoadViewModel) before calling load(), and revisiting isNewInstance() now that it always returns true.\n- The new getFragment/unbindViewPager logic in BaseListActivity and ItemActivity relies on hardcoded ViewPager2 fragment tags (\"f0\", \"f1\"); this is brittle if itemIds or page count change\u2014consider deriving tags from adapter itemIds or resolving current fragments via FragmentManager and container id rather than fixed tag strings.\n- In ItemActivity.onCreate, mNavButton is assigned twice consecutively from findViewById(R.id.navigation_button); the second assignment is redundant and can be removed.\n\n<details>\n<summary>Prompt for AI Agents</summary>\n\n~~~markdown\nPlease address the comments from this code review:\n\n## Overall Comments\n- In LazyLoadFragment, the new eagerLoad() implementation only checks isEagerLoad() and no longer guards on an unloaded state, so load() can now be invoked multiple times; consider tracking and checking an isLoaded flag (from LazyLoadViewModel) before calling load(), and revisiting isNewInstance() now that it always returns true.\n- The new getFragment/unbindViewPager logic in BaseListActivity and ItemActivity relies on hardcoded ViewPager2 fragment tags (\"f0\", \"f1\"); this is brittle if itemIds or page count change\u2014consider deriving tags from adapter itemIds or resolving current fragments via FragmentManager and container id rather than fixed tag strings.\n- In ItemActivity.onCreate, mNavButton is assigned twice consecutively from findViewById(R.id.navigation_button); the second assignment is redundant and can be removed.\n\n## Individual Comments\n\n### Comment 1\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:135-136` </location>\n<code_context>\n final void eagerLoad() {\n- if (mEagerLoad && !mLoaded) {\n- mLoaded = true;\n+ if (mViewModel.isEagerLoad()) {\n+ mViewModel.setLoaded(true);\n load();\n }\n</code_context>\n\n<issue_to_address>\n**issue (bug_risk):** Guard `load()` with the `loaded` flag to avoid repeated loading.\n\nBefore the refactor, `eagerLoad()` guarded `load()` with `!mLoaded`, so it ran at most once per instance even if `loadNow()` or lifecycle events fired multiple times. Now `eagerLoad()` only checks `isEagerLoad()` and always calls `load()`, so multiple invocations (e.g., repeated `loadNow()` or visibility changes) can trigger repeated loads. Please reintroduce the `!isLoaded()` check:\n\n```java\nfinal void eagerLoad() {\n if (mViewModel.isEagerLoad() && !mViewModel.isLoaded()) {\n mViewModel.setLoaded(true);\n load();\n }\n}\n```\n\nand ensure `STATE_LOADED` is restored from the ViewModel if that flag must be persisted.\n</issue_to_address>\n\n### Comment 2\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:39-48` </location>\n<code_context>\n */\n- @SuppressWarnings(\"deprecation\") // Using deprecated onActivityCreated; migration to onViewCreated requires\n- // refactoring\n- @Override\n- public void onActivityCreated(Bundle savedInstanceState) {\n- super.onActivityCreated(savedInstanceState);\n</code_context>\n\n<issue_to_address>\n**suggestion:** Using `setUserVisibleHint` is deprecated and won\u2019t be triggered by ViewPager2.\n\nWith ViewPager2 + FragmentStateAdapter, `setUserVisibleHint` is never invoked because visibility is controlled via `setMaxLifecycle`, so this method is effectively dead code and can mislead future maintenance around lazy loading.\n\nGiven that `loadNow()` is already triggered from the ViewPager2 `OnPageChangeCallback`, you can remove this override. If you still need lifecycle-driven loading, tie it to fragment lifecycle callbacks (e.g., `onResume` with the adapter\u2019s current item) instead of `setUserVisibleHint`.\n</issue_to_address>\n~~~\n\n</details>\n\n***\n\n<details>\n<summary>Sourcery is free for open source - if you like our reviews please consider sharing them \u2728</summary>\n\n- [X](https://twitter.com/intent/tweet?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai)\n- [Mastodon](https://mastodon.social/share?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai)\n- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https://sourcery.ai)\n- [Facebook](https://www.facebook.com/sharer/sharer.php?u=https://sourcery.ai)\n\n</details>\n\n<sub>\nHelp me be more useful! Please click \ud83d\udc4d or \ud83d\udc4e on each comment and I'll use the feedback to improve your reviews.\n</sub>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675907152",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675907152"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:02:48Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"id": 3675907757,
"node_id": "PRR_kwDOQBDOvc7bGeat",
"user": {
"login": "ellipsis-dev[bot]",
"id": 65095814,
"node_id": "MDM6Qm90NjUwOTU4MTQ=",
"avatar_url": "https://avatars.githubusercontent.com/in/64358?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D",
"html_url": "https://github.com/apps/ellipsis-dev",
"followers_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "> [!CAUTION]\n> **Changes requested :x:**\n> <details>\n> <summary>Reviewed everything up to 4f73c22bf8e4d1a3ce2692db6e9365847ef18557 in <ellipsistime>2 minutes and 3 seconds</ellipsistime>. Click for details.</summary> \n> \n> - Reviewed `826` lines of code in `13` files\n> - Skipped `0` files when reviewing.\n> - Skipped posting `8` draft comments. View those below.\n> - Modify your [settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review) and [rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules) to customize what types of comments Ellipsis leaves. And don't forget to react with :+1: or :-1: to teach Ellipsis.\n> \n> <details>\n> <summary>1. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:101</code></summary>\n> \n> - **Draft comment:** \n> Usage of setUserVisibleHint is deprecated; consider using a modern visibility observer or the FragmentTransaction#setMaxLifecycle API.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is about code that was added in this PR (the setUserVisibleHint method is new). The comment is technically correct - setUserVisibleHint is deprecated. However, I need to consider the rules: the PR is migrating from setRetainInstance to ViewModel, which is already a significant refactoring. The author may have intentionally kept setUserVisibleHint for now as part of a gradual migration. The comment suggests alternatives but doesn't provide specific actionable code. It's more of a \"you should consider\" type comment rather than pointing out a clear bug. Given the context that this is already a migration PR, the author likely knows about this deprecation and may be addressing it incrementally. The author is already doing a significant refactoring to migrate to ViewModel. They may be aware of the deprecation and intentionally keeping this for now. The comment doesn't show that this causes a concrete problem, just that it's deprecated. Without knowing if there's a specific issue this causes, this might be premature optimization feedback. While the author may be aware of the deprecation, adding deprecated APIs in new code is generally a code smell. However, the comment is somewhat speculative (\"consider using\") rather than pointing to a definite issue. The rules state not to make speculative comments and to only comment if there's clearly a code change required. This comment points out a deprecated API but is more of a suggestion than identifying a clear bug or required change. Given the rules against speculative comments and the fact that this is already a refactoring PR, this comment should be deleted as it's not pointing to a definite issue that must be fixed.\n> \n> \n> </details>\n> \n> <details>\n> <summary>2. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java:578</code></summary>\n> \n> - **Draft comment:** \n> Unbind logic removes fragments using hardcoded tags 'f0' and 'f1'; ensure these tags remain valid if the page count ever changes.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` The comment is asking the author to ensure that the tags remain valid if the page count changes. This is a form of asking the author to double-check their work, which violates the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.\n> \n> \n> </details>\n> \n> <details>\n> <summary>3. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/widget/ItemPagerAdapter.java:43</code></summary>\n> \n> - **Draft comment:** \n> Migration to FragmentStateAdapter is successful; note that caching of fragments is no longer manual. If state retention is needed, consider managing state via ViewModels.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `20%` <= threshold `50%` This comment is purely informative, explaining a change in behavior related to `FragmentStateAdapter`. It doesn't provide a specific suggestion or ask for confirmation about an intentional change. It seems to be informing the author about a feature rather than addressing a potential issue or improvement in the code.\n> \n> \n> </details>\n> \n> <details>\n> <summary>4. <code>app/src/main/res/layout-w820dp-land/activity_list.xml:83</code></summary>\n> \n> - **Draft comment:** \n> Replacement of the custom ViewPager with ViewPager2 and proper TabLayoutMediator usage looks good.\n> - **Reason this comment was not posted:**\n> Confidence changes required: `0%` <= threshold `50%` None\n> \n> \n> </details>\n> \n> <details>\n> <summary>5. <code>app/src/main/res/layout/activity_item.xml:57</code></summary>\n> \n> - **Draft comment:** \n> The layout now uses ViewPager2 in place of the deprecated custom ViewPager; ensure all IDs (e.g. 'view_pager') are consistently referenced in code.\n> - **Reason this comment was not posted:**\n> Confidence changes required: `0%` <= threshold `50%` None\n> \n> \n> </details>\n> \n> <details>\n> <summary>6. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java:56</code></summary>\n> \n> - **Draft comment:** \n> Typo: Consider changing \"restored in to the view\" to \"restored into the view\".\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about a grammatical typo in documentation. While technically correct, I need to consider if this is \"obvious or unimportant\" according to the rules. The typo is minor - \"in to\" vs \"into\" - and doesn't affect code functionality. However, looking more closely at the diff, this JavaDoc comment appears to be copied from Android's standard documentation for onViewCreated. The text \"restored in to the view\" is actually the original Android documentation text. If this is copied from the Android SDK documentation, then the author likely copied it verbatim and changing it might not be appropriate. But I can't be 100% sure without checking the Android source. The comment is very minor and could be considered \"obvious or unimportant\" - it's just a small grammatical issue in a comment that doesn't affect functionality. This could be standard Android documentation that was copied verbatim, in which case changing it might not be appropriate. Also, even if it's a legitimate typo, it's extremely minor and could fall under \"obvious or unimportant\" comments that should not be made. While it might be standard documentation, I cannot verify that without additional context. However, the rule about \"obvious or unimportant\" comments is clear - this is a very minor grammatical issue in documentation that has no impact on code functionality. Such minor typo corrections typically fall into the category of unimportant comments. This comment should be deleted. It's pointing out a very minor grammatical typo in documentation that has no functional impact. This falls under the rule against \"obvious or unimportant\" comments. Additionally, the JavaDoc may be copied from Android's standard documentation, making the change potentially inappropriate.\n> \n> \n> </details>\n> \n> <details>\n> <summary>7. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:67</code></summary>\n> \n> - **Draft comment:** \n> Typographical suggestion: On line 67, consider replacing \"in to\" with \"into\" for better readability.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a comment about a typo in documentation/comments. Looking at the rules, I need to check: 1) Is this about a code change? Yes, the javadoc comment was added in this diff. 2) Does it require a code change? It's suggesting a typo fix, which is very minor. 3) Is it obvious or unimportant? This is a very minor typo fix in a comment. The rules say \"Do NOT make comments that are obvious or unimportant.\" A typo in documentation is quite minor and could be considered unimportant. 4) The rules emphasize not making comments unless there is \"clearly a code change required\" - a typo in a comment is not really a code change that affects functionality. This seems like the kind of nitpicky comment that should be filtered out. While fixing typos improves code quality, this is an extremely minor issue in a comment that doesn't affect functionality. The rules specifically say not to make obvious or unimportant comments, and a two-word typo in documentation could fall into that category. You're right that this is very minor. The rules emphasize not making comments unless there's clearly a code change required, and fixing \"in to\" vs \"into\" in a javadoc comment is not a critical code change. This is the type of nitpicky, unimportant comment that should be filtered out. This comment should be deleted. While it correctly identifies a typo, it's an unimportant and obvious issue in documentation that doesn't warrant a review comment according to the rules.\n> \n> \n> </details>\n> \n> <details>\n> <summary>8. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ListFragment.java:185</code></summary>\n> \n> - **Draft comment:** \n> Typo in the documentation comment: consider using \"into\" instead of \"in to\" in the phrase \"restored in to the view.\"\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a documentation typo fix suggestion. The rules state \"Do NOT make comments that are obvious or unimportant.\" A typo in documentation could be considered minor/unimportant. However, the rules also say \"Comments that suggest code quality refactors are good! But only if they are actionable and clear.\" This is actionable and clear - it's a simple typo fix. The question is whether fixing a documentation typo counts as important enough. The phrase \"in to\" vs \"into\" is indeed incorrect grammar - \"into\" is the correct preposition here. However, this is a very minor issue in a comment that was just added. The rules emphasize not commenting unless there's \"clearly a code change required\" and to avoid \"obvious or unimportant\" comments. A documentation typo, while technically incorrect, is quite minor and doesn't affect functionality. This seems to fall into the \"unimportant\" category. While the typo is real and the fix is correct, documentation typos are generally considered minor issues. The comment doesn't affect code functionality, and the meaning is still clear despite the grammatical error. This might be too pedantic for a code review. Even though it's minor, documentation quality matters for maintainability. However, the rules explicitly state not to make \"obvious or unimportant\" comments, and a small grammatical error in documentation that doesn't affect understanding likely falls into this category. The bar for keeping comments should be high. This comment points out a legitimate typo, but it's a very minor documentation issue that doesn't affect code functionality or clarity. Given the rules emphasize avoiding unimportant comments, this should be deleted.\n> \n> \n> </details>\n> \n> \n> Workflow ID: <workflowid>`wflow_aWmqFbXzZ6ycdrMk`</workflowid>\n> </details>\n> \n> <sup>You can customize </sup>[<img alt=\"Ellipsis\" src=\"https://img.shields.io/badge/Ellipsis-blue?color=175173\">](https://www.ellipsis.dev?ref=sheepdestroyer%2Fmaterialisheep&utm_source=github&utm_medium=referral)<sup> by changing your [verbosity settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review), reacting with :+1: or :-1:, [replying to comments](https://docs.ellipsis.dev/features/code-review#teaching-ellipsis), or adding [code review rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules).</sup>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675907757",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675907757"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:03:40Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"id": 3675907778,
"node_id": "PRR_kwDOQBDOvc7bGebC",
"user": {
"login": "gemini-code-assist[bot]",
"id": 176961590,
"node_id": "BOT_kgDOCow4Ng",
"avatar_url": "https://avatars.githubusercontent.com/in/956858?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D",
"html_url": "https://github.com/apps/gemini-code-assist",
"followers_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "## Code Review\n\nThis pull request is a significant and well-executed refactoring to modernize the app's architecture. The migration from the deprecated `ViewPager` and `FragmentStatePagerAdapter` to `ViewPager2` and `FragmentStateAdapter` is a great improvement. I also appreciate the modernization of Fragment APIs, such as replacing `onActivityCreated` with `onViewCreated`, using `MenuProvider` for menus, and adopting `ViewModel` for state retention instead of `setRetainInstance`. The code is cleaner and follows current Android best practices. I've left a few minor suggestions to further improve code quality and maintainability, but overall this is excellent work.",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675907778",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675907778"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:03:42Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"id": 3675908915,
"node_id": "PRR_kwDOQBDOvc7bGesz",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "Hey - I've found 4 issues, and left some high level feedback:\n\n- The new `getFragment(int position)` helpers in `BaseListActivity` and `ItemActivity` rely on the internal `FragmentStateAdapter` tag format (`\"f\" + position`) and `unbindViewPager()` manually removes `f0`/`f1`; this is brittle\u2014consider holding references in the adapter or using a `FragmentTransactionCallback`/`FragmentStateAdapter` API rather than hard-coding tags and positions.\n- In `LazyLoadFragment`, `setUserVisibleHint` is reintroduced even though it is deprecated and not invoked by `ViewPager2`; if you need visibility-based loading with `ViewPager2`, consider using `ViewPager2.OnPageChangeCallback` (as you already do) or `FragmentTransactionCallback` instead of `setUserVisibleHint`.\n- The `LazyLoadFragment` migration changes `isNewInstance()` to always return `true` and no longer uses the `STATE_LOADED` flag from saved state, which effectively forces reloads on every view recreation; if this is unintentional, you may want to restore `loaded` state into `LazyLoadViewModel` or adjust `isNewInstance()` semantics to avoid redundant loads.\n\n<details>\n<summary>Prompt for AI Agents</summary>\n\n~~~markdown\nPlease address the comments from this code review:\n\n## Overall Comments\n- The new `getFragment(int position)` helpers in `BaseListActivity` and `ItemActivity` rely on the internal `FragmentStateAdapter` tag format (`\"f\" + position`) and `unbindViewPager()` manually removes `f0`/`f1`; this is brittle\u2014consider holding references in the adapter or using a `FragmentTransactionCallback`/`FragmentStateAdapter` API rather than hard-coding tags and positions.\n- In `LazyLoadFragment`, `setUserVisibleHint` is reintroduced even though it is deprecated and not invoked by `ViewPager2`; if you need visibility-based loading with `ViewPager2`, consider using `ViewPager2.OnPageChangeCallback` (as you already do) or `FragmentTransactionCallback` instead of `setUserVisibleHint`.\n- The `LazyLoadFragment` migration changes `isNewInstance()` to always return `true` and no longer uses the `STATE_LOADED` flag from saved state, which effectively forces reloads on every view recreation; if this is unintentional, you may want to restore `loaded` state into `LazyLoadViewModel` or adjust `isNewInstance()` semantics to avoid redundant loads.\n\n## Individual Comments\n\n### Comment 1\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:134-135` </location>\n<code_context>\n+ return true;\n }\n\n final void eagerLoad() {\n- if (mEagerLoad && !mLoaded) {\n- mLoaded = true;\n+ if (mViewModel.isEagerLoad()) {\n+ mViewModel.setLoaded(true);\n load();\n</code_context>\n\n<issue_to_address>\n**issue (bug_risk):** Guard eagerLoad() with a loaded flag to avoid calling load() multiple times.\n\nThe previous `if (mEagerLoad && !mLoaded)` ensured `load()` ran only once per fragment. Now we only check `mViewModel.isEagerLoad()`, always set `setLoaded(true)`, and never read the loaded state, so repeated `eagerLoad()` calls (e.g. via `loadNow()` or visibility changes) will re-invoke `load()`. Unless `load()` is guaranteed idempotent, this is a behavioral regression. Please reintroduce a loaded guard using the ViewModel state, e.g. `if (mViewModel.isEagerLoad() && !mViewModel.isLoaded()) { mViewModel.setLoaded(true); load(); }`.\n</issue_to_address>\n\n### Comment 2\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:39-48` </location>\n<code_context>\n */\n- @SuppressWarnings(\"deprecation\") // Using deprecated onActivityCreated; migration to onViewCreated requires\n- // refactoring\n- @Override\n- public void onActivityCreated(Bundle savedInstanceState) {\n- super.onActivityCreated(savedInstanceState);\n</code_context>\n\n<issue_to_address>\n**issue (bug_risk):** Using setUserVisibleHint() with ViewPager2 is deprecated and likely ineffective.\n\nThis means the lazy-load check (`if (isVisibleToUser && !mViewModel.isLoaded())`) may never run as intended. Instead, trigger lazy loading from lifecycle callbacks (`onStart`/`onResume`) or from ViewPager2\u2019s page change callbacks (similar to how `loadNow()` is used in Activities), and drive any per-page visibility handling from those callbacks rather than `setUserVisibleHint()`.\n</issue_to_address>\n\n### Comment 3\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java:531-540` </location>\n<code_context>\n+ mViewPager.setAdapter(mAdapter);\n</code_context>\n\n<issue_to_address>\n**issue (bug_risk):** TabLayoutMediator and callbacks are never detached, which can leak and cause duplicate callbacks on rebind.\n\n`bindViewPager()` now creates a `TabLayoutMediator`, a `ViewPager2.OnPageChangeCallback`, and a `TabLayout.OnTabSelectedListener`, but `unbindViewPager()` only removes fragments. If `bindViewPager()` is invoked multiple times, you\u2019ll accumulate mediators and callbacks, leading to leaks and duplicate events. Store these as fields and clean them up in `unbindViewPager()` (e.g. `mediator.detach()`, `mViewPager.unregisterOnPageChangeCallback(...)`, `mTabLayout.removeOnTabSelectedListener(...)`).\n</issue_to_address>\n\n### Comment 4\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java:186-187` </location>\n<code_context>\n mSystemUiHelper = new AppUtils.SystemUiHelper(getWindow());\n mReplyButton = findViewById(R.id.reply_button);\n mNavButton = findViewById(R.id.navigation_button);\n+ mNavButton = findViewById(R.id.navigation_button);\n mNavButton.setNavigable(direction ->\n</code_context>\n\n<issue_to_address>\n**nitpick:** mNavButton is assigned twice in a row; the first assignment appears redundant.\n\nIn `onCreate`, `mNavButton` is assigned twice with `findViewById(R.id.navigation_button)` in consecutive lines; the second is redundant and should be removed, or the ID corrected if a different view was intended.\n</issue_to_address>\n~~~\n\n</details>\n\n***\n\n<details>\n<summary>Sourcery is free for open source - if you like our reviews please consider sharing them \u2728</summary>\n\n- [X](https://twitter.com/intent/tweet?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai)\n- [Mastodon](https://mastodon.social/share?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai)\n- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https://sourcery.ai)\n- [Facebook](https://www.facebook.com/sharer/sharer.php?u=https://sourcery.ai)\n\n</details>\n\n<sub>\nHelp me be more useful! Please click \ud83d\udc4d or \ud83d\udc4e on each comment and I'll use the feedback to improve your reviews.\n</sub>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675908915",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675908915"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:05:25Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"id": 3675909373,
"node_id": "PRR_kwDOQBDOvc7bGez9",
"user": {
"login": "gemini-code-assist[bot]",
"id": 176961590,
"node_id": "BOT_kgDOCow4Ng",
"avatar_url": "https://avatars.githubusercontent.com/in/956858?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D",
"html_url": "https://github.com/apps/gemini-code-assist",
"followers_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "## Code Review\n\nThis pull request is a great step forward in modernizing the app's architecture by migrating from the deprecated `ViewPager` and `FragmentStatePagerAdapter` to `ViewPager2` and `FragmentStateAdapter`. The changes also correctly replace other deprecated APIs like `onActivityCreated`, `setRetainInstance`, and fragment menu methods with their modern counterparts (`onViewCreated`, `ViewModel`, and `MenuProvider`).\n\nThe implementation is solid overall, but I've identified a few issues:\n- A duplicated line of code in `ItemActivity`.\n- A potential bug in `LazyLoadFragment` that could cause data to be loaded multiple times.\n- Some redundant code in `LazyLoadFragment` due to API changes in `ViewPager2`.\n\nI've left specific comments with suggestions to address these points. Once these are fixed, this will be a very strong contribution to the codebase's health.",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675909373",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675909373"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:06:04Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"id": 3675909676,
"node_id": "PRR_kwDOQBDOvc7bGe4s",
"user": {
"login": "ellipsis-dev[bot]",
"id": 65095814,
"node_id": "MDM6Qm90NjUwOTU4MTQ=",
"avatar_url": "https://avatars.githubusercontent.com/in/64358?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D",
"html_url": "https://github.com/apps/ellipsis-dev",
"followers_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "> [!IMPORTANT]\n> **Looks good to me! :+1:**\n> <details>\n> <summary>Reviewed everything up to 4f73c22bf8e4d1a3ce2692db6e9365847ef18557 in <ellipsistime>2 minutes and 13 seconds</ellipsistime>. Click for details.</summary> \n> \n> - Reviewed `826` lines of code in `13` files\n> - Skipped `0` files when reviewing.\n> - Skipped posting `12` draft comments. View those below.\n> - Modify your [settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review) and [rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules) to customize what types of comments Ellipsis leaves. And don't forget to react with :+1: or :-1: to teach Ellipsis.\n> \n> <details>\n> <summary>1. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java:58</code></summary>\n> \n> - **Draft comment:** \n> Good migration to onViewCreated with MenuProvider integration. Verify that mMenuTintDelegate callbacks (~lines 61\u201364) faithfully reproduce the previous deprecated lifecycle behavior.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` The comment is asking the PR author to verify the behavior of `mMenuTintDelegate` callbacks, which violates the rule against asking the author to confirm or verify behavior. It does not provide a specific suggestion or point out a specific issue.\n> \n> \n> </details>\n> \n> <details>\n> <summary>2. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java:468</code></summary>\n> \n> - **Draft comment:** \n> Switching to ViewPager2 and using setUserInputEnabled is appropriate. Note the getFragment method uses hard-coded tags (e.g., \"f0\", \"f1\"); ensure this remains correct given the expected page count.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` The comment is asking the author to ensure that the hard-coded tags remain correct given the expected page count. This is a request for confirmation of intention, which violates the rules. The comment does not provide a specific suggestion or point out a clear issue.\n> \n> \n> </details>\n> \n> <details>\n> <summary>3. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:101</code></summary>\n> \n> - **Draft comment:** \n> setUserVisibleHint is still used for lazy loading, but it is deprecated. Consider migrating to the FragmentTransaction#setMaxLifecycle approach in future revisions.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about code that was added in this PR (the setUserVisibleHint method). However, the comment is phrased as \"Consider migrating... in future revisions\" which makes it sound like a suggestion for future work rather than something that needs to be fixed now. According to the rules, comments should be about clear code changes required, not speculative or informational. The phrase \"in future revisions\" suggests this is not urgent or required for this PR. Additionally, if setUserVisibleHint is deprecated, that would likely be caught by the build system with a deprecation warning, which falls under \"anything that would be obviously caught by the build.\" The author might be intentionally using setUserVisibleHint because they're maintaining compatibility with older Android versions or because a full migration to the newer approach would require more extensive refactoring. The comment could be useful as a reminder for future work, even if it's not actionable now. While the comment might be useful as a future reminder, the rules explicitly state not to make comments unless there is clearly a code change required. The phrase \"in future revisions\" indicates this is not a required change for this PR. Additionally, deprecation warnings would be caught by the build system, so this falls under things that would be \"obviously caught by the build.\" This comment should be deleted because it's phrased as a suggestion for future work (\"in future revisions\") rather than a required change for this PR. It violates the rule about only commenting when there's clearly a code change required, and deprecation warnings would be caught by the build system anyway.\n> \n> \n> </details>\n> \n> <details>\n> <summary>4. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java:189</code></summary>\n> \n> - **Draft comment:** \n> The migration to ViewPager2 is well done. Ensure that using getFragment(0) (which relies on fragment tags) correctly retrieves the intended fragment in the new adapter setup.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` The comment is asking the author to ensure that a specific method call behaves as intended. This falls under the rule of not asking the author to ensure behavior is intended. The comment does not provide a specific suggestion or ask for a test to be written, which would make it more actionable.\n> \n> \n> </details>\n> \n> <details>\n> <summary>5. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.java:535</code></summary>\n> \n> - **Draft comment:** \n> Fullscreen toggle logic (removing/reattaching scroll view content) is complex; ensure this approach remains robust during orientation or configuration changes.\n> - **Reason this comment was not posted:**\n> Comment was not on a location in the diff, so it can't be submitted as a review comment.\n> \n> \n> </details>\n> \n> <details>\n> <summary>6. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/widget/ItemPagerAdapter.java:68</code></summary>\n> \n> - **Draft comment:** \n> Migration to FragmentStateAdapter via createFragment looks correct. Confirm that passing EXTRA_EAGER_LOAD in the bundle properly initializes lazy loading in both ItemFragment and WebFragment.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` This comment is asking the PR author to confirm the behavior of passing `EXTRA_EAGER_LOAD` in the bundle. It is not making a specific code suggestion or pointing out a specific issue. It violates the rule against asking the author to confirm their intention or ensure behavior.\n> \n> \n> </details>\n> \n> <details>\n> <summary>7. <code>app/src/main/res/layout-w820dp-land/activity_list.xml:83</code></summary>\n> \n> - **Draft comment:** \n> Layout now uses ViewPager2; ensure that the id '@id/content' consistently matches its usage in code.\n> - **Reason this comment was not posted:**\n> Comment was on unchanged code.\n> \n> \n> </details>\n> \n> <details>\n> <summary>8. <code>app/src/main/res/layout/activity_item.xml:56</code></summary>\n> \n> - **Draft comment:** \n> Updated activity layout to ViewPager2 is appropriate; verify that toolbar and header elements function correctly with the new ViewPager2 integration.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` This comment is asking the PR author to verify the functionality of toolbar and header elements with the new `ViewPager2` integration. It falls under the category of asking the author to ensure behavior is intended or tested, which is against the rules.\n> \n> \n> </details>\n> \n> <details>\n> <summary>9. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.java:146</code></summary>\n> \n> - **Draft comment:** \n> LocalBroadcastManager is used for fullscreen toggling; note that it is deprecated. Consider alternatives (e.g., LiveData or Flow) for future architecture improvements.\n> - **Reason this comment was not posted:**\n> Comment was not on a location in the diff, so it can't be submitted as a review comment.\n> \n> \n> </details>\n> \n> <details>\n> <summary>10. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java:56</code></summary>\n> \n> - **Draft comment:** \n> Typo: In the javadoc comment, consider changing \"in to the view\" to \"into the view\" for clarity.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment identifies a real typo in the javadoc. However, I need to check the rules carefully. The rules state \"Do NOT comment unless there is clearly a code change required\" and \"Do NOT make comments that are obvious or unimportant.\" A typo in a javadoc comment is relatively minor and could be considered unimportant. However, looking at the diff more carefully, I notice that this javadoc text appears to be copied from Android's standard documentation (it's describing onViewCreated). The text \"in to\" might actually be from the original Android documentation that was copied. But wait - this is still just a documentation typo, not a functional code issue. The rules emphasize not making obvious or unimportant comments, and a minor typo in documentation could fall into that category. This is a legitimate typo that should be fixed for code quality. Documentation quality matters, and fixing typos improves readability. The comment is actionable and clear, which the rules say is good for code quality refactors. While documentation quality does matter, the rules explicitly state not to make comments that are \"obvious or unimportant.\" A single-word typo in a javadoc comment is quite minor and could be considered unimportant compared to functional code issues. The rules seem to prioritize avoiding noise over catching every small issue. This comment identifies a real but very minor typo in documentation. Given the rules emphasize avoiding unimportant comments and the low impact of this issue, I should delete this comment. It's the kind of minor documentation issue that doesn't warrant blocking or cluttering a PR review.\n> \n> \n> </details>\n> \n> <details>\n> <summary>11. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:67</code></summary>\n> \n> - **Draft comment:** \n> Typographical error: consider replacing \"in to\" with \"into\" in the comment (\"...restored in to the view\").\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a comment about a typo in documentation. While it's technically correct that \"into\" is more proper than \"in to\" in this context, I need to consider the review rules. The rules state: \"Do NOT comment unless there is clearly a code change required\" and \"Do NOT make comments that are obvious or unimportant.\" A typo in a comment/documentation is not a code change - it doesn't affect functionality. This is a very minor documentation issue that doesn't impact the code's behavior. The rules emphasize only keeping comments that require clear code changes, and this is just a stylistic/grammatical suggestion for documentation. However, code quality includes documentation quality, and fixing typos in documentation could be considered a valid improvement. The comment is actionable and clear, which the rules say is good for code quality refactors. While documentation quality matters, the rules explicitly state not to comment on obvious or unimportant things. A minor typo in a javadoc comment is both obvious (once pointed out) and relatively unimportant compared to functional code issues. This doesn't meet the bar of \"clearly a code change required.\" This comment should be deleted. It's pointing out a minor typo in documentation that doesn't affect code functionality, which falls under \"obvious or unimportant\" comments that should not be made according to the review rules.\n> \n> \n> </details>\n> \n> <details>\n> <summary>12. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ListFragment.java:185</code></summary>\n> \n> - **Draft comment:** \n> Typographical error: In the comment, consider changing \"in to\" to \"into\" to read \"has been restored into the view.\"\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about a typo in a JavaDoc comment that was added in this diff. Looking at the rules: 1) The rule says \"Do NOT comment unless there is clearly a code change required\" - this is a documentation typo, not a code change. 2) The rule says \"Do NOT make comments that are obvious or unimportant\" - fixing a typo in documentation could be considered minor/unimportant. 3) However, the rules also say \"Comments that suggest code quality refactors are good! But only if they are actionable and clear\" - this is actionable and clear. But is fixing a typo really a \"code quality refactor\"? It's more of a documentation quality issue. The most important consideration is: does this require a code change? It's just a typo in a comment. While it would be nice to fix, it's not critical and falls under \"obvious or unimportant\" changes. The automated tool is being overly pedantic about a minor typo in documentation. While the typo is real and the suggestion is correct, documentation typos are generally considered minor issues. The rules explicitly state not to make \"obvious or unimportant\" comments, and a single-word typo in a JavaDoc comment could fall into this category. Additionally, this doesn't affect code functionality at all. Although the typo is minor, it's in newly added documentation and fixing it would improve code quality. However, the rules prioritize avoiding unimportant comments, and a documentation typo that doesn't affect functionality is likely not important enough to warrant keeping the comment. The bar for keeping comments is high - \"STRONG EVIDENCE that the comment is correct\" - and while correct, this is too minor. This comment should be deleted. While the typo is real, it's a minor documentation issue that falls under \"obvious or unimportant\" comments according to the rules. The automated tool is being overly pedantic about a trivial typo that doesn't affect code functionality.\n> \n> \n> </details>\n> \n> \n> Workflow ID: <workflowid>`wflow_9ayYUp98gHG48qpi`</workflowid>\n> </details>\n> \n> <sup>You can customize </sup>[<img alt=\"Ellipsis\" src=\"https://img.shields.io/badge/Ellipsis-blue?color=175173\">](https://www.ellipsis.dev?ref=sheepdestroyer%2Fmaterialisheep&utm_source=github&utm_medium=referral)<sup> by changing your [verbosity settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review), reacting with :+1: or :-1:, [replying to comments](https://docs.ellipsis.dev/features/code-review#teaching-ellipsis), or adding [code review rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules).</sup>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675909676",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675909676"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:06:34Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"id": 3675912398,
"node_id": "PRR_kwDOQBDOvc7bGfjO",
"user": {
"login": "ellipsis-dev[bot]",
"id": 65095814,
"node_id": "MDM6Qm90NjUwOTU4MTQ=",
"avatar_url": "https://avatars.githubusercontent.com/in/64358?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D",
"html_url": "https://github.com/apps/ellipsis-dev",
"followers_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "> [!IMPORTANT]\n> **Looks good to me! :+1:**\n> <details>\n> <summary>Reviewed a54e2533e73737e69ab715749c49cab76f498b9e in <ellipsistime>1 minute and 14 seconds</ellipsistime>. Click for details.</summary> \n> \n> - Reviewed `98` lines of code in `3` files\n> - Skipped `0` files when reviewing.\n> - Skipped posting `3` draft comments. View those below.\n> - Modify your [settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review) and [rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules) to customize what types of comments Ellipsis leaves. And don't forget to react with :+1: or :-1: to teach Ellipsis.\n> \n> <details>\n> <summary>1. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java:529</code></summary>\n> \n> - **Draft comment:** \n> Good refactor: storing the TabLayoutMediator, OnPageChangeCallback, and TabSelectedListener in member variables improves lifecycle management. Ensure unbindViewPager is always called to detach these, as shown in lines 585\u2013598.\n> - **Reason this comment was not posted:**\n> Confidence changes required: `0%` <= threshold `50%` None\n> \n> \n> </details>\n> \n> <details>\n> <summary>2. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java:185</code></summary>\n> \n> - **Draft comment:** \n> Removed duplicate view lookup for mNavButton improves code clarity.\n> - **Reason this comment was not posted:**\n> Confidence changes required: `0%` <= threshold `50%` None\n> \n> \n> </details>\n> \n> <details>\n> <summary>3. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:134</code></summary>\n> \n> - **Draft comment:** \n> Enhanced eagerLoad() by adding a check for !isLoaded() prevents multiple load() invocations. This is a solid improvement.\n> - **Reason this comment was not posted:**\n> Confidence changes required: `0%` <= threshold `50%` None\n> \n> \n> </details>\n> \n> \n> Workflow ID: <workflowid>`wflow_WkyHdFC1hCJ85VjG`</workflowid>\n> </details>\n> \n> <sup>You can customize </sup>[<img alt=\"Ellipsis\" src=\"https://img.shields.io/badge/Ellipsis-blue?color=175173\">](https://www.ellipsis.dev?ref=sheepdestroyer%2Fmaterialisheep&utm_source=github&utm_medium=referral)<sup> by changing your [verbosity settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review), reacting with :+1: or :-1:, [replying to comments](https://docs.ellipsis.dev/features/code-review#teaching-ellipsis), or adding [code review rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules).</sup>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675912398",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675912398"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:10:26Z",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"_type": "review_summary"
},
{
"id": 3675912508,
"node_id": "PRR_kwDOQBDOvc7bGfk8",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "Hey - I've found 2 issues, and left some high level feedback:\n\n- In both `BaseListActivity` and `ItemActivity`, `getFragment(int)` and `unbindViewPager()` rely on hard-coded tags like `\"f0\"`/`\"f1\"` and fixed page counts; consider deriving tags from the adapter (e.g. using `getItemId()`/`getItemCount()`) to avoid brittleness if the number of pages or IDs change.\n- `LazyLoadFragment` reintroduces the deprecated `setUserVisibleHint`, which is not used by `ViewPager2`/`FragmentStateAdapter`; if you rely on visibility for eager loading, consider wiring that through the `ViewPager2` page change callbacks instead of `setUserVisibleHint`.\n- The new lazy-load state handling splits logic between `LazyLoadFragment` and `LazyLoadViewModel` (e.g. `STATE_LOADED` is written to the bundle but no longer read, `isNewInstance()` always returns true); it may be worth simplifying and centralizing this logic so that initialization and reload behavior is clearly defined in one place.\n\n<details>\n<summary>Prompt for AI Agents</summary>\n\n~~~markdown\nPlease address the comments from this code review:\n\n## Overall Comments\n- In both `BaseListActivity` and `ItemActivity`, `getFragment(int)` and `unbindViewPager()` rely on hard-coded tags like `\"f0\"`/`\"f1\"` and fixed page counts; consider deriving tags from the adapter (e.g. using `getItemId()`/`getItemCount()`) to avoid brittleness if the number of pages or IDs change.\n- `LazyLoadFragment` reintroduces the deprecated `setUserVisibleHint`, which is not used by `ViewPager2`/`FragmentStateAdapter`; if you rely on visibility for eager loading, consider wiring that through the `ViewPager2` page change callbacks instead of `setUserVisibleHint`.\n- The new lazy-load state handling splits logic between `LazyLoadFragment` and `LazyLoadViewModel` (e.g. `STATE_LOADED` is written to the bundle but no longer read, `isNewInstance()` always returns true); it may be worth simplifying and centralizing this logic so that initialization and reload behavior is clearly defined in one place.\n\n## Individual Comments\n\n### Comment 1\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:39-48` </location>\n<code_context>\n */\n- @SuppressWarnings(\"deprecation\") // Using deprecated onActivityCreated; migration to onViewCreated requires\n- // refactoring\n- @Override\n- public void onActivityCreated(Bundle savedInstanceState) {\n- super.onActivityCreated(savedInstanceState);\n</code_context>\n\n<issue_to_address>\n**issue:** Using setUserVisibleHint for lazy loading is deprecated and ineffective with ViewPager2.\n\nViewPager2 uses `setMaxLifecycle` rather than `setUserVisibleHint`, so any eager-load logic there is effectively dead code and reintroduces a deprecated API. Because `loadNow()` is already invoked from page change callbacks with ViewModel state, you can likely remove this override and rely on that flow instead.\n</issue_to_address>\n\n### Comment 2\n<location> `app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java:600-608` </location>\n<code_context>\n }\n- transaction.commit();\n+\n+ // Remove fragments by Tag (\"f\" + id). IDs are 0 and 1.\n+ FragmentTransaction transaction = getSupportFragmentManager().beginTransaction();\n+ Fragment f0 = getSupportFragmentManager().findFragmentByTag(\"f0\");\n+ if (f0 != null)\n+ transaction.remove(f0);\n+ Fragment f1 = getSupportFragmentManager().findFragmentByTag(\"f1\");\n+ if (f1 != null)\n+ transaction.remove(f1);\n+ transaction.commitAllowingStateLoss();\n+ }\n+\n</code_context>\n\n<issue_to_address>\n**suggestion (bug_risk):** Hard-coding fragment tags/indices and using commitAllowingStateLoss makes the unbind logic brittle.\n\nThis assumes the adapter always has exactly two positions with default `itemId`s mapping to tags `\"f0\"` and `\"f1\"`, making the unbind logic tightly coupled to the current `ItemPagerAdapter` and likely to break if page count or IDs change. It also relies on `commitAllowingStateLoss`, which risks silently dropping these removals. Instead, derive removals from `mViewPager.getAdapter().getItemCount()` (or let the adapter manage its fragments) and prefer `commit()` where feasible.\n\nSuggested implementation:\n\n```java\n if (mTabSelectedListener != null) {\n mTabLayout.removeOnTabSelectedListener(mTabSelectedListener);\n mTabSelectedListener = null;\n }\n\n RecyclerView.Adapter<?> adapter = mViewPager.getAdapter();\n if (adapter instanceof FragmentStateAdapter) {\n FragmentManager fragmentManager = getSupportFragmentManager();\n FragmentTransaction transaction = fragmentManager.beginTransaction();\n\n int itemCount = adapter.getItemCount();\n for (int position = 0; position < itemCount; position++) {\n long itemId = ((FragmentStateAdapter) adapter).getItemId(position);\n String tag = \"f\" + itemId;\n Fragment fragment = fragmentManager.findFragmentByTag(tag);\n if (fragment != null) {\n transaction.remove(fragment);\n }\n }\n\n transaction.commit();\n }\n\n/**\n\n```\n\nTo compile successfully, ensure the following imports exist at the top of `BaseListActivity.java` (or equivalent are already present):\n\n```java\nimport androidx.recyclerview.widget.RecyclerView;\nimport androidx.viewpager2.adapter.FragmentStateAdapter;\nimport androidx.fragment.app.Fragment;\nimport androidx.fragment.app.FragmentManager;\nimport androidx.fragment.app.FragmentTransaction;\n```\n\nIf your `ItemPagerAdapter` uses a custom tag format or overrides how fragments are tagged, adjust the `tag` construction (`\"f\" + itemId`) to match that scheme.\n</issue_to_address>\n~~~\n\n</details>\n\n***\n\n<details>\n<summary>Sourcery is free for open source - if you like our reviews please consider sharing them \u2728</summary>\n\n- [X](https://twitter.com/intent/tweet?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai)\n- [Mastodon](https://mastodon.social/share?text=I%20just%20got%20an%20instant%20code%20review%20from%20%40SourceryAI%2C%20and%20it%20was%20brilliant%21%20It%27s%20free%20for%20open%20source%20and%20has%20a%20free%20trial%20for%20private%20code.%20Check%20it%20out%20https%3A//sourcery.ai)\n- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https://sourcery.ai)\n- [Facebook](https://www.facebook.com/sharer/sharer.php?u=https://sourcery.ai)\n\n</details>\n\n<sub>\nHelp me be more useful! Please click \ud83d\udc4d or \ud83d\udc4e on each comment and I'll use the feedback to improve your reviews.\n</sub>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675912508",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675912508"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:10:35Z",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"_type": "review_summary"
},
{
"id": 3675913232,
"node_id": "PRR_kwDOQBDOvc7bGfwQ",
"user": {
"login": "gemini-code-assist[bot]",
"id": 176961590,
"node_id": "BOT_kgDOCow4Ng",
"avatar_url": "https://avatars.githubusercontent.com/in/956858?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D",
"html_url": "https://github.com/apps/gemini-code-assist",
"followers_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "## Code Review\n\nThis pull request is a significant and well-executed refactoring to modernize the app's architecture by migrating from the deprecated `ViewPager` to `ViewPager2` and updating Fragment lifecycle methods. The introduction of `ViewModel` for state management instead of `setRetainInstance` is a great improvement. The code is cleaner and follows modern Android development practices. I've identified a potential memory leak in `ItemActivity` where `TabLayoutMediator` is not being detached, some dead code in `LazyLoadFragment`, and a minor style issue in `BaseListActivity`. Addressing these points will further improve the code's robustness and maintainability.",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675913232",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675913232"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:11:33Z",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"_type": "review_summary"
},
{
"id": 3675913666,
"node_id": "PRR_kwDOQBDOvc7bGf3C",
"user": {
"login": "ellipsis-dev[bot]",
"id": 65095814,
"node_id": "MDM6Qm90NjUwOTU4MTQ=",
"avatar_url": "https://avatars.githubusercontent.com/in/64358?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D",
"html_url": "https://github.com/apps/ellipsis-dev",
"followers_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "> [!IMPORTANT]\n> **Looks good to me! :+1:**\n> <details>\n> <summary>Reviewed everything up to a54e2533e73737e69ab715749c49cab76f498b9e in <ellipsistime>2 minutes and 44 seconds</ellipsistime>. Click for details.</summary> \n> \n> - Reviewed `2790` lines of code in `14` files\n> - Skipped `0` files when reviewing.\n> - Skipped posting `12` draft comments. View those below.\n> - Modify your [settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review) and [rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules) to customize what types of comments Ellipsis leaves. And don't forget to react with :+1: or :-1: to teach Ellipsis.\n> \n> <details>\n> <summary>1. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java:63</code></summary>\n> \n> - **Draft comment:** \n> Ensure the MenuProvider added in onViewCreated is removed in onDestroyView to avoid potential memory leaks.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking for manual cleanup of the MenuProvider. However, the code uses `addMenuProvider(menuProvider, lifecycleOwner, state)` with `getViewLifecycleOwner()` as the lifecycle owner. This is the recommended pattern in Android - when you provide a LifecycleOwner to addMenuProvider, the framework automatically removes the MenuProvider when that lifecycle is destroyed. The view lifecycle owner is destroyed in onDestroyView, so the MenuProvider should be automatically cleaned up. This comment appears to be incorrect because it's asking for manual cleanup that isn't necessary and would be redundant. The comment also violates the rule about not asking the author to \"ensure\" something. I might be wrong about the automatic cleanup behavior. Perhaps there's a specific bug or edge case in the MenuProvider API where manual removal is still needed even with a LifecycleOwner. I should consider that the automated tool might have knowledge of such issues that I'm not aware of. While there could be edge cases, the standard Android documentation and best practices indicate that using addMenuProvider with a LifecycleOwner parameter handles cleanup automatically. The comment doesn't provide any specific evidence of a problem, and asking to \"ensure\" something is done falls under the category of comments we should remove. Without strong evidence that manual cleanup is needed here, this appears to be an incorrect suggestion. This comment should be deleted. The MenuProvider is properly scoped to the view lifecycle owner, which means Android will automatically remove it when the view is destroyed. Manual removal in onDestroyView would be redundant. Additionally, the comment starts with \"Ensure that...\" which is explicitly mentioned as a pattern to avoid in the rules.\n> \n> \n> </details>\n> \n> <details>\n> <summary>2. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java:611</code></summary>\n> \n> - **Draft comment:** \n> Relying on fragment tags ('f'+position) for getFragment may be brittle; verify that this tagging scheme remains consistent with ViewPager2.\n> - **Reason this comment was not posted:**\n> Comment looked like it was already resolved.\n> \n> \n> </details>\n> \n> <details>\n> <summary>3. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java:405</code></summary>\n> \n> - **Draft comment:** \n> The migration to ViewPager2 looks good; ensure getFragment() reliably fetches the correct fragment and that FAB toggling synchronizes with page changes.\n> - **Reason this comment was not posted:**\n> Comment was not on a location in the diff, so it can't be submitted as a review comment.\n> \n> \n> </details>\n> \n> <details>\n> <summary>4. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:101</code></summary>\n> \n> - **Draft comment:** \n> setUserVisibleHint is deprecated; consider using FragmentTransaction#setMaxLifecycle or Lifecycle observers for visibility handling.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The `setUserVisibleHint` override is indeed being added in this PR (it's in the diff as new code). The comment is technically correct that this method is deprecated. However, I need to consider: 1) This is a migration PR that's moving from `setRetainInstance` to ViewModel, so the author is already doing modernization work. 2) The comment suggests an alternative approach but doesn't provide concrete implementation details. 3) The author may have intentionally kept this deprecated method for compatibility reasons or because they're doing incremental migration. 4) The comment is somewhat speculative - it's suggesting a refactor but not pointing out a definite bug. 5) According to the rules, comments suggesting code quality refactors are good if they are actionable and clear, but this one is somewhat vague about how to implement the alternative. The author is clearly doing modernization work (migrating to ViewModel), so they may be aware of the deprecation but chose to keep this method for now. The comment doesn't explain why this is a problem or provide concrete steps to fix it. It's possible the deprecated method is still needed for backward compatibility or the author is doing incremental migration. While the author is doing modernization, the comment does point out a legitimate issue with newly added code using a deprecated API. However, the comment lacks specificity and actionable guidance - it just mentions alternatives without explaining how to implement them in this specific context. This makes it more of an informational comment than an actionable one, which violates the rules. The comment should be deleted. While it's technically correct that `setUserVisibleHint` is deprecated, the comment is not sufficiently actionable - it merely suggests alternatives without providing concrete implementation guidance. This falls into the category of informational comments that don't clearly indicate a required code change, which violates the rules.\n> \n> \n> </details>\n> \n> <details>\n> <summary>5. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ListFragment.java:80</code></summary>\n> \n> - **Draft comment:** \n> The switch to onViewCreated and using getViewLifecycleOwner for LiveData observers is well implemented; confirm that adapter state restoration operates as expected.\n> - **Reason this comment was not posted:**\n> Comment was not on a location in the diff, so it can't be submitted as a review comment.\n> \n> \n> </details>\n> \n> <details>\n> <summary>6. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.java:525</code></summary>\n> \n> - **Draft comment:** \n> Review the setFullscreen implementation for potential UI reflow issues when moving scroll view content between containers.\n> - **Reason this comment was not posted:**\n> Comment was not on a location in the diff, so it can't be submitted as a review comment.\n> \n> \n> </details>\n> \n> <details>\n> <summary>7. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/widget/ItemPagerAdapter.java:62</code></summary>\n> \n> - **Draft comment:** \n> The conversion to FragmentStateAdapter is clean; double-check if the 'retain instance' flag is still necessary given modern fragment APIs.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` The comment is asking the PR author to double-check something, which violates the rule against asking the author to confirm or double-check things. It doesn't provide a specific suggestion or point out a clear issue.\n> \n> \n> </details>\n> \n> <details>\n> <summary>8. <code>app/src/main/res/layout-w820dp-land/activity_list.xml:83</code></summary>\n> \n> - **Draft comment:** \n> The layout update to use ViewPager2 appears straightforward; ensure that IDs and references are updated consistently in the code.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` This comment is asking the PR author to ensure consistency in updating IDs and references, which falls under the category of asking the author to double-check things. It doesn't provide a specific suggestion or point out a specific issue.\n> \n> \n> </details>\n> \n> <details>\n> <summary>9. <code>app/src/main/res/layout/activity_item.xml:57</code></summary>\n> \n> - **Draft comment:** \n> Verify that the activity layout\u2019s switch to ViewPager2 preserves proper toolbar and AppBar behavior across orientations.\n> - **Reason this comment was not posted:**\n> Comment did not seem useful. Confidence is useful = `0%` <= threshold `50%` This comment is asking the PR author to verify behavior across orientations, which falls under the rule of not asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.\n> \n> \n> </details>\n> \n> <details>\n> <summary>10. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java:56</code></summary>\n> \n> - **Draft comment:** \n> Typographical suggestion: Consider changing \"restored in to the view\" to \"restored into the view\" for improved clarity.\n> - **Reason this comment was not posted:**\n> Comment was on unchanged code.\n> \n> \n> </details>\n> \n> <details>\n> <summary>11. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java:67</code></summary>\n> \n> - **Draft comment:** \n> Typographical error: Consider changing \"in to\" to \"into\" in the comment for clarity.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a comment about a typo in documentation. Looking at the rules: \"Do NOT comment unless there is clearly a code change required\" - this is just a documentation typo, not a code change. The rules also say \"Do NOT make comments that are obvious or unimportant\" - fixing a typo is relatively minor. However, the comment is technically correct - \"in to\" should be \"into\" in this context. But the rules emphasize not making comments unless there's a clear code change required, and typos in comments are generally low priority. The most important rule states I should only keep comments with STRONG EVIDENCE that they are correct and important. While the typo is real, it's a minor documentation issue, not a functional code problem. The typo is real and the suggestion is technically correct. Documentation quality matters, and fixing typos improves code quality. This could be considered an actionable code quality improvement, even if it's just in a comment. While the typo is real, the rules explicitly state not to make comments that are \"obvious or unimportant\" and to only comment when there's \"clearly a code change required.\" A minor typo in a javadoc comment doesn't meet the threshold of importance for keeping the comment, especially given the instruction to err on the side of deleting comments unless there's STRONG EVIDENCE they should be kept. This comment should be deleted. While it correctly identifies a typo, it's a minor documentation issue that doesn't meet the threshold for \"clearly a code change required\" and falls under \"obvious or unimportant\" comments that should be avoided.\n> \n> \n> </details>\n> \n> <details>\n> <summary>12. <code>app/src/main/java/io/github/sheepdestroyer/materialisheep/ListFragment.java:185</code></summary>\n> \n> - **Draft comment:** \n> Typo: Consider changing \"in to\" to \"into\" in the Javadoc comment.\n> - **Reason this comment was not posted:**\n> Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a minor typo fix in documentation. While technically correct (both \"in to\" and \"into\" are grammatically acceptable in some contexts, but \"into\" is more standard here), this falls under \"obvious or unimportant\" comments. The comment is about a very minor documentation issue that doesn't affect code functionality. According to the rules, I should \"Do NOT make comments that are obvious or unimportant.\" A typo like \"in to\" vs \"into\" in a Javadoc comment is quite minor and unimportant. However, I need to consider that this Javadoc appears to be copied from Android framework documentation, so the typo might actually be in the original source. This could be a legitimate documentation improvement, even if minor. Documentation quality matters, and fixing typos improves readability. The comment is actionable and clear. Also, looking at the Javadoc, it appears to be copied from Android's standard documentation for onViewCreated, so the typo might have been introduced during copying or might exist in the original. While documentation quality does matter, this is an extremely minor typo that doesn't affect understanding or functionality. The rules explicitly state \"Do NOT make comments that are obvious or unimportant.\" A single-word spacing issue in a Javadoc comment falls squarely into the \"unimportant\" category. The comment doesn't meet the threshold of being clearly necessary. This comment should be deleted. It's about a very minor typo in documentation that doesn't affect code functionality or understanding. It falls under the category of \"obvious or unimportant\" comments that should not be made according to the rules.\n> \n> \n> </details>\n> \n> \n> Workflow ID: <workflowid>`wflow_4Box9Gdhbs6o0fKc`</workflowid>\n> </details>\n> \n> <sup>You can customize </sup>[<img alt=\"Ellipsis\" src=\"https://img.shields.io/badge/Ellipsis-blue?color=175173\">](https://www.ellipsis.dev?ref=sheepdestroyer%2Fmaterialisheep&utm_source=github&utm_medium=referral)<sup> by changing your [verbosity settings](https://app.ellipsis.dev/sheepdestroyer/settings/code-review), reacting with :+1: or :-1:, [replying to comments](https://docs.ellipsis.dev/features/code-review#teaching-ellipsis), or adding [code review rules](https://app.ellipsis.dev/sheepdestroyer/code-review/rules).</sup>",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675913666",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675913666"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:12:05Z",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"_type": "review_summary"
},
{
"id": 3675913893,
"node_id": "PRR_kwDOQBDOvc7bGf6l",
"user": {
"login": "coderabbitai[bot]",
"id": 136622811,
"node_id": "BOT_kgDOCCSy2w",
"avatar_url": "https://avatars.githubusercontent.com/in/347564?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/coderabbitai%5Bbot%5D",
"html_url": "https://github.com/apps/coderabbitai",
"followers_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/coderabbitai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "**Actionable comments posted: 1**\n\n> [!CAUTION]\n> Some comments are outside the diff and can\u2019t be posted inline due to platform limitations.\n> \n> \n> \n> <details>\n> <summary>\u26a0\ufe0f Outside diff range comments (1)</summary><blockquote>\n> \n> <details>\n> <summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java (1)</summary><blockquote>\n> \n> `46-57`: **Remove orphaned Javadoc comment.**\n> \n> Lines 46-52 contain the old Javadoc for the now-removed `onActivityCreated` method. This documentation block is orphaned and should be deleted to avoid confusion.\n> \n> \n> \n> <details>\n> <summary>Proposed fix</summary>\n> \n> ```diff\n> }\n> \n> - /**\n> - * Called when the fragment's activity has been created and this\n> - * fragment's view hierarchy instantiated.\n> - *\n> - * `@param` savedInstanceState If the fragment is being re-created from\n> - * a previous saved state, this is the state.\n> - */\n> /**\n> * Called immediately after\n> * {`@link` `#onCreateView`(LayoutInflater, ViewGroup, Bundle)}\n> * has returned, but before any saved state has been restored in to the view.\n> */\n> ```\n> </details>\n> \n> </blockquote></details>\n> \n> </blockquote></details>\n\n<details>\n<summary>\ud83e\udd16 Fix all issues with AI agents</summary>\n\n```\nIn\n`@app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java`:\n- Around line 101-108: The override of setUserVisibleHint(...) in\nLazyLoadFragment is deprecated for ViewPager2 and can cause an NPE if mViewModel\nis null; remove this method and instead trigger eager loading from a\nlifecycle-aware place: override onResume() in LazyLoadFragment and inside\nonResume() check that getUserVisibleHint() is not used but that the fragment\nisResumed/visible, then if mViewModel != null and !mViewModel.isLoaded() call\nmViewModel.setEagerLoad(true) and eagerLoad(); alternatively observe the\nfragment lifecycle (LifecycleObserver) to perform the same checks so loading\nonly happens when the fragment reaches RESUMED, ensuring you null-check\nmViewModel before calling isLoaded()/setEagerLoad().\n```\n\n</details>\n\n<details>\n<summary>\ud83e\uddf9 Nitpick comments (7)</summary><blockquote>\n\n<details>\n<summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseFragment.java (1)</summary><blockquote>\n\n`59-84`: **Good migration to MenuProvider API with lifecycle-aware registration.**\n\nThe implementation correctly uses `getViewLifecycleOwner()` with `Lifecycle.State.RESUMED` to scope the menu provider to the fragment's view lifecycle. The `isAttached()` guards are appropriately retained.\n\nTwo minor style suggestions:\n\n1. **Inconsistent null-safety**: Line 62 uses `getActivity()` while line 63 uses `requireActivity()`. Consider using `requireActivity()` consistently.\n\n2. **Fully qualified annotations**: The inline fully qualified names (`@androidx.annotation.NonNull`, `androidx.core.view.MenuProvider`, etc.) work but reduce readability. Consider adding imports.\n\n\n\n\n<details>\n<summary>Optional cleanup</summary>\n\nAdd imports at the top of the file:\n```java\nimport android.view.MenuItem;\nimport android.view.View;\nimport androidx.annotation.NonNull;\nimport androidx.annotation.Nullable;\nimport androidx.core.view.MenuProvider;\nimport androidx.lifecycle.Lifecycle;\n```\n\nThen simplify the method signature and body:\n```diff\n `@Override`\n- public void onViewCreated(`@androidx.annotation.NonNull` android.view.View view,\n- `@androidx.annotation.Nullable` Bundle savedInstanceState) {\n+ public void onViewCreated(`@NonNull` View view, `@Nullable` Bundle savedInstanceState) {\n super.onViewCreated(view, savedInstanceState);\n- mMenuTintDelegate.onActivityCreated(getActivity());\n- requireActivity().addMenuProvider(new androidx.core.view.MenuProvider() {\n+ mMenuTintDelegate.onActivityCreated(requireActivity());\n+ requireActivity().addMenuProvider(new MenuProvider() {\n `@Override`\n- public void onCreateMenu(`@androidx.annotation.NonNull` Menu menu,\n- `@androidx.annotation.NonNull` MenuInflater menuInflater) {\n+ public void onCreateMenu(`@NonNull` Menu menu, `@NonNull` MenuInflater menuInflater) {\n // ...\n }\n\n `@Override`\n- public boolean onMenuItemSelected(`@androidx.annotation.NonNull` android.view.MenuItem menuItem) {\n+ public boolean onMenuItemSelected(`@NonNull` MenuItem menuItem) {\n return BaseFragment.this.onOptionsItemSelected(menuItem);\n }\n\n `@Override`\n- public void onPrepareMenu(`@androidx.annotation.NonNull` Menu menu) {\n+ public void onPrepareMenu(`@NonNull` Menu menu) {\n // ...\n }\n- }, getViewLifecycleOwner(), androidx.lifecycle.Lifecycle.State.RESUMED);\n+ }, getViewLifecycleOwner(), Lifecycle.State.RESUMED);\n }\n```\n</details>\n\n</blockquote></details>\n<details>\n<summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadViewModel.java (1)</summary><blockquote>\n\n`5-34`: **Move `mInitialized` field declaration with the other fields.**\n\nThe `mInitialized` field is declared at line 33, after its getter/setter methods. For consistency and readability, group all field declarations together at the top of the class.\n\n\n\n<details>\n<summary>Suggested reordering</summary>\n\n```diff\n public class LazyLoadViewModel extends ViewModel {\n private boolean mEagerLoad;\n private boolean mLoaded;\n+ private boolean mInitialized;\n\n public boolean isEagerLoad() {\n return mEagerLoad;\n }\n@@ ... @@\n public void setInitialized(boolean initialized) {\n mInitialized = initialized;\n }\n-\n- private boolean mInitialized;\n }\n```\n</details>\n\n</blockquote></details>\n<details>\n<summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java (2)</summary><blockquote>\n\n`57-68`: **Remove stale Javadoc comment.**\n\nThere are two Javadoc blocks here: the first (lines 57-62) describes the old `onActivityCreated` method, and the second (lines 64-68) describes `onViewCreated`. Remove the outdated comment block.\n\n\n\n<details>\n<summary>Remove stale comment</summary>\n\n```diff\n- /**\n- * Called when the fragment's activity has been created and this\n- * fragment's view hierarchy instantiated.\n- *\n- * `@param` savedInstanceState If the fragment is being re-created from\n- * a previous saved state, this is the state.\n- */\n /**\n * Called immediately after\n * {`@link` `#onCreateView`(LayoutInflater, ViewGroup, Bundle)}\n * has returned, but before any saved state has been restored in to the view.\n */\n```\n</details>\n\n---\n\n`130-132`: **Consider removing this method since it always returns `true`.**\n\n`isNewInstance()` no longer provides meaningful logic. Consider inlining the check or removing the method entirely to reduce dead code.\n\n</blockquote></details>\n<details>\n<summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java (1)</summary><blockquote>\n\n`146-153`: **Guard navigation against a null fragment.** \nViewPager2 creates fragments lazily, so `getFragment(0)` can be null; a simple instance check avoids potential NPEs. \n \n\n<details>\n<summary>\ud83d\udee0\ufe0f Proposed guard</summary>\n\n```diff\n- mNavButton.setNavigable(direction ->\n- // if callback is fired navigable should not be null\n- ((Navigable) getFragment(0)).onNavigate(direction));\n+ mNavButton.setNavigable(direction -> {\n+ Fragment fragment = getFragment(0);\n+ if (fragment instanceof Navigable) {\n+ ((Navigable) fragment).onNavigate(direction);\n+ }\n+ });\n```\n</details>\n\n\nAlso applies to: 589-590\n\n</blockquote></details>\n<details>\n<summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/widget/ItemPagerAdapter.java (1)</summary><blockquote>\n\n`41-43`: **Remove stale deprecation suppression/TODO.** \nThe adapter is already on `FragmentStateAdapter`, so the class-level suppression/comment looks outdated. \n \n\n<details>\n<summary>\ud83e\uddf9 Suggested cleanup</summary>\n\n```diff\n-@SuppressWarnings(\"deprecation\") // TODO: Uses deprecated FragmentStatePagerAdapter; migrate to\n- // ViewPager2/FragmentStateAdapter\n public class ItemPagerAdapter extends androidx.viewpager2.adapter.FragmentStateAdapter {\n```\n</details>\n\n</blockquote></details>\n<details>\n<summary>app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java (1)</summary><blockquote>\n\n`186-189`: **Drop the duplicate `mNavButton` assignment.** \nThere are two identical assignments back-to-back; keeping one is sufficient. \n\n<details>\n<summary>\ud83e\uddf9 Suggested cleanup</summary>\n\n```diff\n- mNavButton = findViewById(R.id.navigation_button);\n```\n</details>\n\n</blockquote></details>\n\n</blockquote></details>\n\n<!-- This is an auto-generated comment by CodeRabbit for review status -->",
"state": "COMMENTED",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675913893",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"author_association": "NONE",
"_links": {
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#pullrequestreview-3675913893"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"submitted_at": "2026-01-19T00:12:23Z",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"_type": "review_summary"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884107",
"pull_request_review_id": 3675907152,
"id": 2702884107,
"node_id": "PRRC_kwDOQBDOvc6hGr0L",
"diff_hunk": "@@ -123,15 +127,13 @@ public void loadNow() {\n *\n * @return True if the fragment is a new instance, false otherwise.\n */\n- @SuppressWarnings(\"deprecation\") // Using deprecated getRetainInstance; migration to ViewModel requires\n- // significant refactoring\n protected boolean isNewInstance() {\n- return !getRetainInstance() || mNewInstance;\n+ return true;\n }\n \n final void eagerLoad() {\n- if (mEagerLoad && !mLoaded) {\n- mLoaded = true;\n+ if (mViewModel.isEagerLoad()) {\n+ mViewModel.setLoaded(true);",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "**issue (bug_risk):** Guard `load()` with the `loaded` flag to avoid repeated loading.\n\nBefore the refactor, `eagerLoad()` guarded `load()` with `!mLoaded`, so it ran at most once per instance even if `loadNow()` or lifecycle events fired multiple times. Now `eagerLoad()` only checks `isEagerLoad()` and always calls `load()`, so multiple invocations (e.g., repeated `loadNow()` or visibility changes) can trigger repeated loads. Please reintroduce the `!isLoaded()` check:\n\n```java\nfinal void eagerLoad() {\n if (mViewModel.isEagerLoad() && !mViewModel.isLoaded()) {\n mViewModel.setLoaded(true);\n load();\n }\n}\n```\n\nand ensure `STATE_LOADED` is restored from the ViewModel if that flag must be persisted.\n\n\u2705 Addressed in a54e2533e73737e69ab715749c49cab76f498b9e: `eagerLoad()` now checks both `mViewModel.isEagerLoad()` and `!mViewModel.isLoaded()` before setting `loaded` and calling `load()`, restoring the single-load guard via the ViewModel state.",
"created_at": "2026-01-19T00:02:48Z",
"updated_at": "2026-01-19T00:09:33Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884107",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884107"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884107"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884107/reactions",
"total_count": 2,
"+1": 1,
"-1": 1,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": null,
"original_start_line": 135,
"start_side": "RIGHT",
"line": null,
"original_line": 136,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 124,
"position": 1,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884108",
"pull_request_review_id": 3675907152,
"id": 2702884108,
"node_id": "PRRC_kwDOQBDOvc6hGr0M",
"diff_hunk": "@@ -28,39 +29,28 @@ public abstract class LazyLoadFragment extends BaseFragment {\n public static final String EXTRA_RETAIN_INSTANCE = WebFragment.class.getName() + \".EXTRA_RETAIN_INSTANCE\";\n private static final String STATE_EAGER_LOAD = \"state:eagerLoad\";\n private static final String STATE_LOADED = \"state:loaded\";\n- private boolean mEagerLoad, mLoaded, mActivityCreated;\n- private boolean mNewInstance;\n-\n- /**\n- * Called when a fragment is first attached to its context.\n- *\n- * @param context The context.\n- */\n- @Override\n- public void onAttach(Context context) {\n- super.onAttach(context);\n- mNewInstance = false;\n- }\n+ private boolean mActivityCreated;\n+ private LazyLoadViewModel mViewModel;\n \n /**\n * Called to do initial creation of a fragment.\n *\n * @param savedInstanceState If the fragment is being re-created from\n * a previous saved state, this is the state.\n */\n- @SuppressWarnings(\"deprecation\") // Using deprecated setRetainInstance; migration to ViewModel requires\n- // significant refactoring\n @Override\n public void onCreate(Bundle savedInstanceState) {\n super.onCreate(savedInstanceState);\n- setRetainInstance(getArguments().getBoolean(EXTRA_RETAIN_INSTANCE, false));\n- mNewInstance = true;\n- if (savedInstanceState != null) {\n- mEagerLoad = savedInstanceState.getBoolean(STATE_EAGER_LOAD);\n- mLoaded = savedInstanceState.getBoolean(STATE_LOADED);\n- } else {\n- mEagerLoad = getArguments() != null && getArguments().getBoolean(EXTRA_EAGER_LOAD) ||\n- !Preferences.shouldLazyLoad(getActivity());\n+ mViewModel = new ViewModelProvider(this).get(LazyLoadViewModel.class);\n+ if (!mViewModel.isInitialized()) {\n+ mViewModel.setInitialized(true);\n+ if (savedInstanceState != null) {\n+ mViewModel.setEagerLoad(savedInstanceState.getBoolean(STATE_EAGER_LOAD));",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "**suggestion:** Using `setUserVisibleHint` is deprecated and won\u2019t be triggered by ViewPager2.\n\nWith ViewPager2 + FragmentStateAdapter, `setUserVisibleHint` is never invoked because visibility is controlled via `setMaxLifecycle`, so this method is effectively dead code and can mislead future maintenance around lazy loading.\n\nGiven that `loadNow()` is already triggered from the ViewPager2 `OnPageChangeCallback`, you can remove this override. If you still need lifecycle-driven loading, tie it to fragment lifecycle callbacks (e.g., `onResume` with the adapter\u2019s current item) instead of `setUserVisibleHint`.",
"created_at": "2026-01-19T00:02:49Z",
"updated_at": "2026-01-19T00:02:49Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884108",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884108"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884108"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884108/reactions",
"total_count": 2,
"+1": 1,
"-1": 1,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": 39,
"original_start_line": 39,
"start_side": "RIGHT",
"line": 48,
"original_line": 48,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 51,
"position": 51,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884717",
"pull_request_review_id": 3675907757,
"id": 2702884717,
"node_id": "PRRC_kwDOQBDOvc6hGr9t",
"diff_hunk": "@@ -184,9 +185,10 @@ protected void onCreate(Bundle savedInstanceState) {\n mSystemUiHelper = new AppUtils.SystemUiHelper(getWindow());\n mReplyButton = findViewById(R.id.reply_button);\n mNavButton = findViewById(R.id.navigation_button);\n+ mNavButton = findViewById(R.id.navigation_button);",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "ellipsis-dev[bot]",
"id": 65095814,
"node_id": "MDM6Qm90NjUwOTU4MTQ=",
"avatar_url": "https://avatars.githubusercontent.com/in/64358?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D",
"html_url": "https://github.com/apps/ellipsis-dev",
"followers_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/ellipsis-dev%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "Duplicate assignment of `mNavButton` appears; consider removing one redundant `findViewById` call.\n```suggestion\n\n```\n",
"created_at": "2026-01-19T00:03:40Z",
"updated_at": "2026-01-19T00:03:40Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884717",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884717"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884717"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884717/reactions",
"total_count": 0,
"+1": 0,
"-1": 0,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": null,
"original_start_line": null,
"start_side": null,
"line": null,
"original_line": 188,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 34,
"position": 1,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884734",
"pull_request_review_id": 3675907778,
"id": 2702884734,
"node_id": "PRRC_kwDOQBDOvc6hGr9-",
"diff_hunk": "@@ -522,32 +521,73 @@ private void openMultiPaneItem() {\n \n private void bindViewPager() {\n if (mAdapter != null) {\n- mAdapter.unbind(mTabLayout);\n+ // No custom unbind needed for Adapter anymore\n }\n- mAdapter = new ItemPagerAdapter(this, getSupportFragmentManager(), new ItemPagerAdapter.Builder()\n+ mAdapter = new ItemPagerAdapter(this, new ItemPagerAdapter.Builder()\n .setItem(mSelectedItem)\n .setCacheMode(getItemCacheMode())\n .setShowArticle(true)\n .setDefaultViewMode(mStoryViewMode));\n- mAdapter.bind(mViewPager, mTabLayout, mNavButton, mReplyButton);\n+ mViewPager.setAdapter(mAdapter);\n+ new TabLayoutMediator(mTabLayout, mViewPager,\n+ (tab, position) -> tab.setText(mAdapter.getPageTitle(position))).attach();\n+ mViewPager.registerOnPageChangeCallback(new ViewPager2.OnPageChangeCallback() {\n+ @Override\n+ public void onPageSelected(int position) {\n+ super.onPageSelected(position);\n+ AppUtils.toggleFab(mNavButton, position == 0 && Preferences.navigationEnabled(BaseListActivity.this));\n+ AppUtils.toggleFab(mReplyButton, true);\n+ AppUtils.toggleFabAction(mReplyButton, mSelectedItem, position == 0);",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "gemini-code-assist[bot]",
"id": 176961590,
"node_id": "BOT_kgDOCow4Ng",
"avatar_url": "https://avatars.githubusercontent.com/in/956858?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D",
"html_url": "https://github.com/apps/gemini-code-assist",
"followers_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "\n\nThis logic for toggling the FABs is duplicated below when setting the initial state (lines 568-570). To improve maintainability and reduce code duplication, consider extracting this into a private helper method. A good example is the `toggleFabs` method implemented in `ItemActivity`.",
"created_at": "2026-01-19T00:03:42Z",
"updated_at": "2026-01-19T00:03:43Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884734",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884734"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884734"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884734/reactions",
"total_count": 0,
"+1": 0,
"-1": 0,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": 543,
"original_start_line": 538,
"start_side": "RIGHT",
"line": 545,
"original_line": 540,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 78,
"position": 90,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884739",
"pull_request_review_id": 3675907778,
"id": 2702884739,
"node_id": "PRRC_kwDOQBDOvc6hGr-D",
"diff_hunk": "@@ -522,32 +521,73 @@ private void openMultiPaneItem() {\n \n private void bindViewPager() {\n if (mAdapter != null) {\n- mAdapter.unbind(mTabLayout);\n+ // No custom unbind needed for Adapter anymore\n }\n- mAdapter = new ItemPagerAdapter(this, getSupportFragmentManager(), new ItemPagerAdapter.Builder()\n+ mAdapter = new ItemPagerAdapter(this, new ItemPagerAdapter.Builder()\n .setItem(mSelectedItem)\n .setCacheMode(getItemCacheMode())\n .setShowArticle(true)\n .setDefaultViewMode(mStoryViewMode));\n- mAdapter.bind(mViewPager, mTabLayout, mNavButton, mReplyButton);\n+ mViewPager.setAdapter(mAdapter);\n+ new TabLayoutMediator(mTabLayout, mViewPager,\n+ (tab, position) -> tab.setText(mAdapter.getPageTitle(position))).attach();\n+ mViewPager.registerOnPageChangeCallback(new ViewPager2.OnPageChangeCallback() {\n+ @Override\n+ public void onPageSelected(int position) {\n+ super.onPageSelected(position);\n+ AppUtils.toggleFab(mNavButton, position == 0 && Preferences.navigationEnabled(BaseListActivity.this));\n+ AppUtils.toggleFab(mReplyButton, true);\n+ AppUtils.toggleFabAction(mReplyButton, mSelectedItem, position == 0);\n+\n+ Fragment fragment = getFragment(position);\n+ if (fragment instanceof LazyLoadFragment) {\n+ ((LazyLoadFragment) fragment).loadNow();\n+ }\n+ }\n+ });\n+ mTabLayout.addOnTabSelectedListener(new TabLayout.OnTabSelectedListener() {\n+ @Override\n+ public void onTabSelected(TabLayout.Tab tab) {\n+ }\n+\n+ @Override\n+ public void onTabUnselected(TabLayout.Tab tab) {\n+ }\n+\n+ @Override\n+ public void onTabReselected(TabLayout.Tab tab) {\n+ Fragment fragment = getFragment(mViewPager.getCurrentItem());\n+ if (fragment instanceof Scrollable) {\n+ ((Scrollable) fragment).scrollToTop();\n+ }\n+ }\n+ });\n+\n+ // Initial FAB state\n+ int current = mViewPager.getCurrentItem();\n+ AppUtils.toggleFab(mNavButton, current == 0 && Preferences.navigationEnabled(this));\n+ AppUtils.toggleFab(mReplyButton, true);\n+ AppUtils.toggleFabAction(mReplyButton, mSelectedItem, current == 0);\n if (mFullscreen) {\n setFullscreen();\n }\n }\n \n @SuppressLint(\"RestrictedApi\")\n private void unbindViewPager() {\n- // fragment manager always restores view pager fragments,\n- // even when view pager no longer exists (e.g. after rotation),\n- // so we have to explicitly remove those with view pager ID\n+ // Remove fragments by Tag (\"f\" + id). IDs are 0 and 1.\n FragmentTransaction transaction = getSupportFragmentManager().beginTransaction();\n- // noinspection Convert2streamapi\n- for (Fragment fragment : getSupportFragmentManager().getFragments()) {\n- if (fragment != null && fragment.getId() == R.id.content) {\n- transaction.remove(fragment);\n- }\n- }\n- transaction.commit();\n+ Fragment f0 = getSupportFragmentManager().findFragmentByTag(\"f0\");\n+ if (f0 != null)\n+ transaction.remove(f0);\n+ Fragment f1 = getSupportFragmentManager().findFragmentByTag(\"f1\");\n+ if (f1 != null)\n+ transaction.remove(f1);\n+ transaction.commitAllowingStateLoss();",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "gemini-code-assist[bot]",
"id": 176961590,
"node_id": "BOT_kgDOCow4Ng",
"avatar_url": "https://avatars.githubusercontent.com/in/956858?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D",
"html_url": "https://github.com/apps/gemini-code-assist",
"followers_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "\n\nUsing `commitAllowingStateLoss()` can prevent `IllegalStateException` crashes when a transaction is committed after `onSaveInstanceState()`. However, this comes at the risk of fragment state loss if the activity is restored. While this may be an acceptable trade-off here for cleaning up fragments during a configuration change, it's generally safer to use `commit()` and ensure the transaction is performed before the activity state is saved.",
"created_at": "2026-01-19T00:03:43Z",
"updated_at": "2026-01-19T00:03:43Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884739",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884739"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884739"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884739/reactions",
"total_count": 0,
"+1": 0,
"-1": 0,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": null,
"original_start_line": null,
"start_side": null,
"line": 608,
"original_line": 586,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 134,
"position": 163,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884741",
"pull_request_review_id": 3675907778,
"id": 2702884741,
"node_id": "PRRC_kwDOQBDOvc6hGr-F",
"diff_hunk": "@@ -184,9 +185,10 @@ protected void onCreate(Bundle savedInstanceState) {\n mSystemUiHelper = new AppUtils.SystemUiHelper(getWindow());\n mReplyButton = findViewById(R.id.reply_button);\n mNavButton = findViewById(R.id.navigation_button);\n+ mNavButton = findViewById(R.id.navigation_button);",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/ItemActivity.java",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "gemini-code-assist[bot]",
"id": 176961590,
"node_id": "BOT_kgDOCow4Ng",
"avatar_url": "https://avatars.githubusercontent.com/in/956858?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D",
"html_url": "https://github.com/apps/gemini-code-assist",
"followers_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/gemini-code-assist%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "\n\nThis line is a duplicate of the one above it and can be removed.",
"created_at": "2026-01-19T00:03:43Z",
"updated_at": "2026-01-19T00:03:43Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884741",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884741"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702884741"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702884741/reactions",
"total_count": 0,
"+1": 0,
"-1": 0,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": null,
"original_start_line": null,
"start_side": null,
"line": null,
"original_line": 188,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 34,
"position": 1,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702886142",
"pull_request_review_id": 3675908915,
"id": 2702886142,
"node_id": "PRRC_kwDOQBDOvc6hGsT-",
"diff_hunk": "@@ -123,15 +127,13 @@ public void loadNow() {\n *\n * @return True if the fragment is a new instance, false otherwise.\n */\n- @SuppressWarnings(\"deprecation\") // Using deprecated getRetainInstance; migration to ViewModel requires\n- // significant refactoring\n protected boolean isNewInstance() {\n- return !getRetainInstance() || mNewInstance;\n+ return true;\n }\n \n final void eagerLoad() {\n- if (mEagerLoad && !mLoaded) {\n- mLoaded = true;\n+ if (mViewModel.isEagerLoad()) {",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/LazyLoadFragment.java",
"commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "**issue (bug_risk):** Guard eagerLoad() with a loaded flag to avoid calling load() multiple times.\n\nThe previous `if (mEagerLoad && !mLoaded)` ensured `load()` ran only once per fragment. Now we only check `mViewModel.isEagerLoad()`, always set `setLoaded(true)`, and never read the loaded state, so repeated `eagerLoad()` calls (e.g. via `loadNow()` or visibility changes) will re-invoke `load()`. Unless `load()` is guaranteed idempotent, this is a behavioral regression. Please reintroduce a loaded guard using the ViewModel state, e.g. `if (mViewModel.isEagerLoad() && !mViewModel.isLoaded()) { mViewModel.setLoaded(true); load(); }`.\n\n\u2705 Addressed in a54e2533e73737e69ab715749c49cab76f498b9e: The guard on `eagerLoad()` has been updated to `if (mViewModel.isEagerLoad() && !mViewModel.isLoaded())`, so `load()` is only invoked once per fragment instance.",
"created_at": "2026-01-19T00:05:25Z",
"updated_at": "2026-01-19T00:09:35Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702886142",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702886142"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702886142"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702886142/reactions",
"total_count": 2,
"+1": 1,
"-1": 1,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0
},
"start_line": null,
"original_start_line": 134,
"start_side": "RIGHT",
"line": null,
"original_line": 135,
"side": "RIGHT",
"author_association": "NONE",
"original_position": 123,
"position": 1,
"subject_type": "line",
"_type": "inline_comment"
},
{
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702886143",
"pull_request_review_id": 3675908915,
"id": 2702886143,
"node_id": "PRRC_kwDOQBDOvc6hGsT_",
"diff_hunk": "@@ -522,32 +521,73 @@ private void openMultiPaneItem() {\n \n private void bindViewPager() {\n if (mAdapter != null) {\n- mAdapter.unbind(mTabLayout);\n+ // No custom unbind needed for Adapter anymore\n }\n- mAdapter = new ItemPagerAdapter(this, getSupportFragmentManager(), new ItemPagerAdapter.Builder()\n+ mAdapter = new ItemPagerAdapter(this, new ItemPagerAdapter.Builder()\n .setItem(mSelectedItem)\n .setCacheMode(getItemCacheMode())\n .setShowArticle(true)\n .setDefaultViewMode(mStoryViewMode));\n- mAdapter.bind(mViewPager, mTabLayout, mNavButton, mReplyButton);\n+ mViewPager.setAdapter(mAdapter);\n+ new TabLayoutMediator(mTabLayout, mViewPager,\n+ (tab, position) -> tab.setText(mAdapter.getPageTitle(position))).attach();\n+ mViewPager.registerOnPageChangeCallback(new ViewPager2.OnPageChangeCallback() {\n+ @Override\n+ public void onPageSelected(int position) {\n+ super.onPageSelected(position);\n+ AppUtils.toggleFab(mNavButton, position == 0 && Preferences.navigationEnabled(BaseListActivity.this));\n+ AppUtils.toggleFab(mReplyButton, true);\n+ AppUtils.toggleFabAction(mReplyButton, mSelectedItem, position == 0);",
"path": "app/src/main/java/io/github/sheepdestroyer/materialisheep/BaseListActivity.java",
"commit_id": "a54e2533e73737e69ab715749c49cab76f498b9e",
"original_commit_id": "4f73c22bf8e4d1a3ce2692db6e9365847ef18557",
"user": {
"login": "sourcery-ai[bot]",
"id": 58596630,
"node_id": "MDM6Qm90NTg1OTY2MzA=",
"avatar_url": "https://avatars.githubusercontent.com/in/48477?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/sourcery-ai%5Bbot%5D",
"html_url": "https://github.com/apps/sourcery-ai",
"followers_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/followers",
"following_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/following{/other_user}",
"gists_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/gists{/gist_id}",
"starred_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/subscriptions",
"organizations_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/orgs",
"repos_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/repos",
"events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/events{/privacy}",
"received_events_url": "https://api.github.com/users/sourcery-ai%5Bbot%5D/received_events",
"type": "Bot",
"user_view_type": "public",
"site_admin": false
},
"body": "**issue (bug_risk):** TabLayoutMediator and callbacks are never detached, which can leak and cause duplicate callbacks on rebind.\n\n`bindViewPager()` now creates a `TabLayoutMediator`, a `ViewPager2.OnPageChangeCallback`, and a `TabLayout.OnTabSelectedListener`, but `unbindViewPager()` only removes fragments. If `bindViewPager()` is invoked multiple times, you\u2019ll accumulate mediators and callbacks, leading to leaks and duplicate events. Store these as fields and clean them up in `unbindViewPager()` (e.g. `mediator.detach()`, `mViewPager.unregisterOnPageChangeCallback(...)`, `mTabLayout.removeOnTabSelectedListener(...)`).\n\n\u2705 Addressed in a54e2533e73737e69ab715749c49cab76f498b9e: `TabLayoutMediator`, `OnPageChangeCallback`, and `OnTabSelectedListener` are now stored as fields and detached/unregistered in `unbindViewPager()` to prevent leaks and duplicate callbacks.",
"created_at": "2026-01-19T00:05:25Z",
"updated_at": "2026-01-19T00:09:36Z",
"html_url": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702886143",
"pull_request_url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56",
"_links": {
"self": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702886143"
},
"html": {
"href": "https://github.com/sheepdestroyer/materialisheep/pull/56#discussion_r2702886143"
},
"pull_request": {
"href": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/56"
}
},
"reactions": {
"url": "https://api.github.com/repos/sheepdestroyer/materialisheep/pulls/comments/2702886143/reactions",
"total_count": 2,
"+1": 1,
"-1": 1,
"laugh": 0,
"hooray": 0,
"confused": 0,
"heart": 0,
"rocket": 0,
"eyes": 0