refactor(core): update BugReportExporter IF to be more robust

[Manabu-GT]

Summary by CodeRabbit

• Bug Fixes

  • Improved export error handling: failures show a snackbar and end the submission flow; initiated/successful exports mark reports submitted and clean up captures.

• Refactor

  • Export flow now reports granular outcomes (initiated, success, failure) for clearer status and feedback.

• New Features

  • Exporter now accepts a stable, read-only archive view of bug reports for safer, consistent access during sharing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[claude]

Claude finished @Manabu-GT's task in 2m 58s —— View job

---

Code Review Complete ✓

I've completed a thorough review of this refactoring. Overall, this is a well-designed improvement that introduces better type safety and semantic clarity to the bug report export system. The architecture prepares the codebase nicely for future extensibility.

---

Summary

This PR refactors the BugReportExporter interface to be more robust by:

• Introducing BugReportArchive abstraction to provide safe, read-only access to bug reports
• Replacing boolean return with semantic ExportResult types (Initiated, Success, Failure)
• Adding withContext(Dispatchers.Main) for proper dispatcher switching
• Excellent KDoc with practical examples for future implementers

---

What's Done Well ✨

1. Excellent documentation - The BugReportArchive KDoc includes practical code examples (Jira exporter) and explains important details like Content-Length requirements
2. Semantic result modeling - ExportResult clearly distinguishes between fire-and-forget exports (share sheet) vs confirmed delivery (HTTP uploads)
3. Idiomatic Kotlin - Proper use of data object for singleton results and sealed interfaces
4. Stream-based API - openInputStream() allows concurrent access without exposing file modification capabilities
5. Proper dispatcher switching - withContext(Dispatchers.Main) wraps only the UI operation

---

Issues Found

I've posted inline comments on specific issues. Here's the summary:

🔴 Major Issues (2)

1. Line 90: Using runCatching in suspend function

   • Violates project coding standards in docs/CODING.md
   • Breaks structured concurrency by catching CancellationException
   • Fix: Replace with runCatchingNonCancellation

2. Line 74: Type-unsafe downcast with check()

   • Interface accepts BugReportArchive but implementation requires specific subtype
   • Violates Liskov Substitution Principle
   • Will crash at runtime with different implementations
   • Fix options provided in inline comment

🟡 Minor Issues (2)

3. No tests added

   • Consider adding unit tests for BugReportArchiveImpl and ExportResult behavior
   • Tests should verify fileName, sizeBytes, openInputStream() work correctly

4. Error message clarity

   • The check() error message could be more developer-friendly
   • Consider using require() instead of check() since this validates caller input (LSP violation aside)

---

Recommendations

Priority	Issue	Action Required
🔴 Major	Structured concurrency	Replace runCatching with runCatchingNonCancellation
🔴 Major	Type safety	Add optional file property to interface OR document the limitation
🟡 Minor	Test coverage	Add unit tests for new abstractions
🟡 Minor	Error handling	Use require() instead of check() for parameter validation

---

Architecture Notes

The BugReportArchive abstraction is well-designed for future HTTP-based exporters (Jira, Slack, GitHub) that only need streams. The tension with IntentShareExporter needing file access is a known trade-off for Android's FileProvider API. See inline comment for resolution options.

The coroutine pattern is correct - documenting that export() is called on Dispatchers.IO and implementers should switch to Main for UI operations is the right approach.

---

Let me know if you'd like me to implement any of these fixes!

[coderabbitai]

Caution

Review failed

The pull request is closed.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a read-only BugReportArchive abstraction and implementation; replaces export(zipFile: File): Boolean with export(report: BugReportArchive): ExportResult; updates IntentShareExporter to accept the archive and start the share Intent on the main thread; adjusts BugReportActivity to handle ExportResult variants.

Changes

Cohort / File(s)	Summary
Archive Abstraction debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/model/BugReportArchive.kt	Add internal sealed interface BugReportArchive (fileName, sizeBytes, openInputStream()) and internal class BugReportArchiveImpl(file: File) delegating to File.
Export API & Result debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/BugReportExporter.kt	Add internal sealed interface ExportResult (Initiated, Success, Failure); change export signature to suspend fun export(report: BugReportArchive): ExportResult.
IntentShareExporter implementation debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/BugReportExporter.kt (same file)	Accepts BugReportArchive (enforces BugReportArchiveImpl at runtime), uses underlying file/FileProvider URI, derives subject from file name, calls startActivity on Dispatchers.Main via withContext, and returns ExportResult variants.
UI integration debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/ui/BugReportActivity.kt	Wrap ZIP in BugReportArchiveImpl for export call; branch on ExportResult (Initiated/Success -> mark submitted/delete/finish; Failure -> show snackbar/end flow).
Imports & docs .../bugreport/...	Add imports for BugReportArchive, BugReportArchiveImpl, coroutine Dispatchers/withContext; update in-code docs noting threading and archive usage.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Activity as BugReportActivity
participant Exporter as IntentShareExporter
participant Provider as FileProvider
participant System as AndroidSystem
Activity->>Exporter: export(BugReportArchiveImpl)
Exporter->>Exporter: obtain File from BugReportArchiveImpl
Exporter->>Provider: getUriForFile(file)
Exporter->>System: build ACTION_SEND Intent (uri, subject)
Exporter->>Exporter: withContext(Dispatchers.Main) → startActivity(intent)
alt startActivity succeeded
Exporter-->>Activity: ExportResult.Initiated
else exception
Exporter-->>Activity: ExportResult.Failure(exception)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• [feat] add bug report draft management UX #112: Modifies bug report export/invocation flows that overlap with Activity and exporter behavior changes.
• [feat][bugreport][2/N] add data collection logic and data export #98: Refactored the File/Boolean export API that this change replaces — directly related to exporter signature changes.
• bugfix(core): fix an issue where an overlay shows above bug reporting dialog/bottomsheet #147: Touches bug-reporting UI and overlay handling; may intersect with Activity-level changes here.

Poem

🐰 I tote the zip in gentle paws,
A wrapped-up archive, no messy claws.
Initiated hop, or Failure's thump—oh dear—
I nose the share, then munch a carrot beer! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title mentions updating BugReportExporter IF (interface) to be more robust, which aligns with the main changes: introducing a structured ExportResult sealed interface to replace Boolean returns, and wrapping File objects with BugReportArchive abstraction for safer, more granular export handling.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @Manabu-GT's task in 1m 22s —— View job

---

Code Review Complete ✓

I've reviewed the refactoring changes. The good news: the previous major issue about runCatching has been fixed! The code now correctly uses runCatchingNonCancellation at BugReportExporter.kt:91, which properly preserves structured concurrency.

Regarding the type-safety concern raised in the previous review, your response is correct - this is an internal, sealed interface, so the require() check is appropriate for defensive programming. The constraint is well-documented in the KDoc.

---

Summary

This PR successfully refactors the BugReportExporter interface with:

• BugReportArchive abstraction - provides safe, read-only access to bug reports via streams
• Semantic ExportResult types - replaces boolean with Initiated, Success, Failure states
• Proper coroutine handling - uses runCatchingNonCancellation and withContext(Dispatchers.Main)
• Excellent documentation - comprehensive KDoc with practical Jira exporter example

---

What's Working Well ✨

1. Fixed structured concurrency - BugReportExporter.kt:91 now uses runCatchingNonCancellation instead of runCatching, properly handling cancellation
2. Strong documentation - BugReportArchive.kt includes a complete working example showing how to implement a Jira exporter with proper Content-Length handling
3. Idiomatic Kotlin - proper use of data object for singleton results, sealed interfaces for type safety
4. Stream-based API - openInputStream() allows safe concurrent access without file modification risks
5. Defensive programming - The require() check at BugReportExporter.kt:75 is appropriate for internal code

---

Observations & Recommendations

🟡 Minor: Consider Optional Test Coverage

While not critical for internal refactoring, consider adding tests for:

• BugReportArchiveImpl.fileName, sizeBytes, openInputStream() behavior
• ExportResult type handling in different scenarios
• Concurrent stream access safety

This would provide regression safety for future changes.

🟡 Minor: Error Message Clarity

At BugReportExporter.kt:75, the require() message could be more helpful for future developers:

require(report is BugReportArchiveImpl) { 
  "IntentShareExporter requires file-based BugReportArchive (FileProvider needs direct File access)" 
}

This explains why the constraint exists, not just what is required.

📝 Documentation Note

The architecture correctly handles the tension between:

• Stream-based abstraction (good for HTTP exporters)
• File-based Android APIs (FileProvider requirement)

The current approach is pragmatic for internal code. If the interface becomes public API in the future, consider adding an optional file: File? property to the sealed interface as discussed in the previous review.

---

Verdict

Approved ✓ - This is a solid refactoring that improves type safety and semantic clarity. The structured concurrency issue has been fixed, and the defensive programming approach is appropriate for internal code.

The refactoring sets up the codebase well for future HTTP-based exporters (Jira, Slack, GitHub) while maintaining compatibility with Android's FileProvider requirements.

---

[claude]

Claude finished @Manabu-GT's task in 1m 35s —— View job

---

Code Review Complete ✓

I've completed a thorough review of this refactoring. The code is well-architected and follows project standards closely. The previous major issue regarding runCatching has been properly addressed.

---

Summary

This PR successfully refactors the BugReportExporter interface to be more robust by:

• Introducing BugReportArchive abstraction - provides safe, read-only access to bug reports via stream-based API
• Replacing boolean with semantic ExportResult types - Initiated, Success, Failure provide clear export outcomes
• Proper coroutine handling - correctly uses runCatchingNonCancellation and withContext(Dispatchers.Main)
• Excellent documentation - comprehensive KDoc with practical Jira exporter example

The refactoring prepares the codebase well for future HTTP-based exporters (Jira, Slack, GitHub) while maintaining compatibility with Android's FileProvider requirements.

---

What's Working Well ✨

1. Structured concurrency compliance (BugReportExporter.kt:93) - Correctly uses runCatchingNonCancellation instead of runCatching, properly handling cancellation per docs/CODING.md standards

2. Exceptional documentation (BugReportArchive.kt:14-48) - The complete Jira exporter example demonstrates:

   • How to use openInputStream() with OkHttp RequestBody
   • Why sizeBytes matters for Content-Length requirements
   • Proper resource cleanup with .use { }

3. Idiomatic Kotlin - Proper use of:

   • data object for singleton results (BugReportArchive.kt:28, 36)
   • Sealed interfaces for exhaustive type safety
   • Delegated properties (get() = file.name, BugReportArchive.kt:102)

4. Stream-based API design - openInputStream() allows safe concurrent access without file modification risks, good for multiple exporters

5. Semantic result modeling - ExportResult clearly distinguishes between:

   • Initiated - fire-and-forget (share sheet, can't verify completion)
   • Success - confirmed delivery (HTTP uploads with response verification)
   • Failure - explicit error with optional cause

6. Proper dispatcher switching (BugReportExporter.kt:95) - withContext(Dispatchers.Main) correctly wraps only the UI operation (startActivity), matching the documented threading contract

---

Observations & Recommendations

🟡 Minor: Type Safety Trade-off

Location: BugReportExporter.kt:75-77

The require() check enforces a runtime constraint that the sealed interface accepts BugReportArchive but this implementation specifically needs BugReportArchiveImpl:

require(report is BugReportArchiveImpl) {
  "IntentShareExporter requires file-based BugReportArchive (FileProvider needs direct File access)"
}

Analysis:

• This is appropriate for internal code where the sealed interface is internal and all implementations are controlled
• The author's response correctly notes this will only fail if someone incorrectly extends the sealed interface
• The error message clearly explains the FileProvider constraint

Suggestions:

1. Current approach is acceptable for internal code ✓
2. Consider require() instead of check() since this validates caller input (though both work here)
3. If this interface becomes public API in the future, consider adding an optional file: File? property to the sealed interface to make the constraint compile-time

🟡 Minor: Test Coverage

Status: No tests added for new abstractions

Recommendation: Consider adding unit tests for:

• BugReportArchiveImpl.fileName, sizeBytes, openInputStream() behavior
• ExportResult type handling in different scenarios
• Concurrent stream access safety

Why it's optional:

• These are simple delegating implementations
• The integration is tested via BugReportActivity flow
• Per docs/TESTING.md, we don't test framework behavior (File.inputStream(), File.length())

Why it could be valuable:

• Regression safety if implementation changes
• Documents expected behavior
• Validates the openInputStream() contract

Example test (if you choose to add):

@Test
fun `BugReportArchiveImpl provides correct metadata`() {
    val tempFile = File.createTempFile("test", ".zip")
    tempFile.writeText("test content")
    
    val archive = BugReportArchiveImpl(tempFile)
    
    assertThat(archive.fileName).isEqualTo(tempFile.name)
    assertThat(archive.sizeBytes).isEqualTo(12L)
    archive.openInputStream().use { stream ->
        assertThat(stream.readBytes().toString(Charsets.UTF_8))
            .isEqualTo("test content")
    }
}

---

Architecture Notes

The FileProvider Tension

The architecture correctly handles the tension between:

• Stream-based abstraction (good for HTTP exporters like Jira/Slack)
• File-based Android APIs (FileProvider requires direct File access)

The current approach is pragmatic:

• BugReportArchive provides the clean abstraction for future extensibility
• BugReportArchiveImpl exposes file property for internal platform needs
• Runtime check documents the limitation clearly

Threading Model

The documented threading contract is well-designed:

• export() is called on Dispatchers.IO (per KDoc at line 56)
• Implementers switch to Dispatchers.Main for UI operations (line 95)
• This keeps I/O operations on background threads while allowing Activity launches

---

Alignment with Project Standards

Standard	Compliance	Evidence
Coroutines (CODING.md)	✅ Pass	Uses runCatchingNonCancellation at line 93
Documentation	✅ Pass	Comprehensive KDoc with working examples
Kotlin idioms	✅ Pass	Sealed interfaces, data objects, delegated properties
String resources	✅ Pass	UI strings use resources (R.string.*)
Internal visibility	✅ Pass	New APIs marked internal appropriately

---

Verdict

Approved ✓

This is a solid, well-thought-out refactoring that:

• Improves type safety and semantic clarity
• Follows all project coding standards
• Provides excellent documentation for future implementers
• Sets up the codebase for extensibility (HTTP-based exporters)
• Maintains backward compatibility with existing Android FileProvider integration

The minor observations above are truly optional enhancements, not blockers. The code is production-ready as-is.

---