Implement Compose-based Error Panel, Error UI Model, and Tests for Comments

[SttApollo]

What is it?

• Feature (user facing)
• Codebase improvement (dev facing)

Description of the changes in your PR

• Created a new Compose-based ErrorPanel component.
• Ensured the new error panel is fully theme-adaptive using MaterialTheme.colorScheme and that all text is centered for visual parity with the legacy design.
• Introduced a new ErrorPanelSpec data class to flexibly describe error panel content and actions.
• Created ErrorUiModel interface to represent different failure states along with UnableToLoadCommentsUiModel specific to Comments section.
• Added a focused test suite: Unit tests for ErrorUiModel and Integration tests for the two main error states in the comment section: Resource.Error (initial load failure) and LoadState.Error (paging failure). Would add Compose testing with approval to add a new dependency.
• Manual Testing: Verified error handling by forcing errors in both CommentsViewModel (Resource.Error) and CommentsSource (LoadState.Error) to ensure proper UI rendering and error model mapping.

Before/After Screenshots/Screen Record

Before	After

Fixes the following issue(s)

• Fixes CommentSection: Add an error panel #11716

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

[SttApollo]

@Stypox Apologies for the fairly large diff—thought it would make sense to add a new Compose-based ErrorPanel and have it wired up end-to-end as a first step towards migration. I’m happy to split it up or tweak anything to fit the overall architecture—just let me know!

[Stypox]

Thank you! I think this PR is the right size, and also you wrote many tests which increase the "PR line count" but do not add reviewing complexity, so there is no need to split it. And thanks for also writing test! I left a few comments to try to make the code more general so it can be used more seamlessly in other places in the app in the future (to replace other error panels in various places).

[SttApollo]

@Stypox I have incorporated your requests - removed the other files and just added one test file. Also I didn't want to add an additional dependency for compose testing but please let me know if you'd like me to!

[absurdlylongusername]

@Stypox I have incorporated your requests - removed the other files and just added one test file. Also I didn't want to add an additional dependency for compose testing but please let me know if you'd like me to!

@SttApollo Tbh I have no qualms with you adding compose testing, because we are going to be adding UI tests very soon anyway. Currently we have no UI tests whatsoever, and I don't really see a downside to having tests for new functionality.

@Stypox, thoughts?

[Stypox]

Yeah it would be cool if you added some tests! Would you be ok with opening a new PR after this one is merged, to setup the UI tests infrastructure and add tests for what you added in this PR? Otherwise you can also add UI tests in this PR, let us know what you prefer :-)

[SttApollo]

Sure, I can open a new PR after this one is merged. For the UI test infra, I was thinking we could start with Compose instrumented testing—does that align with what you had in mind? Or should we also add Robolectric-based Compose tests and/or snapshot testing early on?

[Stypox]

Looks good to me, thank you! I pushed two commits for correctly handling Locale-based ContextCompat.getString() in @Previews (otherwise some previews wouldn't be shown for me), and reordered the buttons to Recaptcha/Report, Retry, OpenInBrowser.

solved

[Stypox]

@absurdlylongusername feel free to merge if you are also ok with the changes, and then add the PR title + number to the 0.28.1 draft release :-)

[TobiGr]

Not to the 0.28.1 draft, but the refactor one, please :)

[absurdlylongusername]

Hi @SttApollo, sorry for late response.

Thanks for your work thus far.

I can add Compose tests in a follow-up PR, or include them here if you prefer.

I'd prefer them to be in this PR, but ultimately up to you which you want: I just want them to be done ASAP. Ideally I don't want them being in a different PR to delay them being done, so I'm hoping you would have already started working on them, but if not then I guess it doesn't matter lol.

---

Just to confirm I understand the purpose of this PR: the purpose is to add an error panel specifically for the comments section as stated in #11716, right? The ErrorPanel.kt that was added is not intended to replace the existing error_panel.xml at a later date, is it? Because if so, it doesn't have the same structure (i.e. error_panel.xml has three text fields as you can see in the image, but ErrorPanel.kt only has one)

Error panel preview

I was thinking we could start with Compose instrumented testing—does that align with what you had in mind?

This is fine. Would you be testing just ErrorPanel and CommentSection, or would you also plan to add higher level integration tests that replicate what is done for manual testing?

---

Was also going to suggest improving the ErrorPanel previews so that they show all the buttons, or have multiple previews for each expected code path; but @Stypox has just done that.

I'd also suggest having a preview for light and dark mode like we have in many other places.

I.e. adding this to ErrorPanel preview

@Preview(
    name = "Light mode",
    showBackground = true,
    uiMode = Configuration.UI_MODE_NIGHT_NO
)
@Preview(
    name = "Dark mode",
    showBackground = true,
    uiMode = Configuration.UI_MODE_NIGHT_YES
)

[Stypox]

@absurdlylongusername this is meant to completely replace error_panel.xml. It only has one text field, yes, but that's to reflect the fact that now the error message is chosen by the ErrorInfo instead of by the UI. Sure, we could add "getMainMessage()", "getSubmessage()", etc. to ErrorInfo, but I don't think it's worth it (the second and third lines of text were only used for a couple of errors even before).

[SttApollo]

@absurdlylongusername

I'd prefer them to be in this PR, but ultimately up to you which you want: I just want them to be done ASAP. Ideally I don't want them being in a different PR to delay them being done, so I'm hoping you would have already started working on them, but if not then I guess it doesn't matter lol.

Yes np, I'll add them to this PR in the next day or two.

Just to confirm I understand the purpose of this PR: the purpose is to add an error panel specifically for the comments section as stated in #11716, right? The ErrorPanel.kt that was added is not intended to replace the existing error_panel.xml at a later date, is it? Because if so, it doesn't have the same structure (i.e. error_panel.xml has three text fields as you can see in the image, but ErrorPanel.kt only has one)

It is intendend to replace the xml in the future as Stypox mentioned.

I was thinking we could start with Compose instrumented testing—does that align with what you had in mind?
This is fine. Would you be testing just ErrorPanel and CommentSection, or would you also plan to add higher level integration tests that replicate what is done for manual testing?

This is my plan so far

1. Compose tests for ErrorPanel: render the panel with sample data representing each path network error, retryable error, ReCaptcha, and assert the right text and buttons are visible.
2. Comments screen instrumentation test: launch the comments UI on device/emulator, run through load, scroll, retry, and ReCaptcha so it mirrors the manual check.

Was also going to suggest improving the ErrorPanel previews so that they show all the buttons, or have multiple previews for each expected code path; but @Stypox has just done that.
I'd also suggest having a preview for light and dark mode like we have in many other places.

Yes, will add this too.

[absurdlylongusername]

@SttApollo @Stypox Okay cool, fantastic

[absurdlylongusername]

@absurdlylongusername this is meant to completely replace error_panel.xml. It only has one text field, yes, but that's to reflect the fact that now the error message is chosen by the ErrorInfo instead of by the UI. Sure, we could add "getMainMessage()", "getSubmessage()", etc. to ErrorInfo, but I don't think it's worth it (the second and third lines of text were only used for a couple of errors even before).

Is there an issue currently to track replacing errorpanel.xml in the rest of the code?

[Stypox]

Is there an issue currently to track replacing errorpanel.xml in the rest of the code?

No, because we want to replace every *.xml with Compose code, and we can't open 1000 issues for each XML file xD

But maybe this is a special case, where it would make sense to have an issue lying around that can be worked on. But what I think makes more sense is to just use the Compose error panel whenever converting a screen to Compose (I included this in #12674 for lists).

[absurdlylongusername]

@Stypox

No, because we want to replace every *.xml with Compose code, and we can't open 1000 issues for each XML file xD

Touché, lol.

just use the Compose error panel whenever converting a screen to Compose

Yeah I'm more talking about noting this down somewhere so that it is documented. Idk where though.

[SttApollo]

@absurdlylongusername I ended up adding an inline TestCommentSection helper in CommentSectionInstrumentedTest instead of pulling in additional mock libraries, lmk if this approach works!

[absurdlylongusername]

@SttApollo You seem to have reverted @Stypox's changes in aed4278.

Please restore them.

[theimpulson]

LTGM!