Skip to content

fix: CI/firebase fixes and expanded R8 dead-code-elimination tests#152

Merged
kirich1409 merged 4 commits intomainfrom
worktree-fix-ci-failures
Apr 3, 2026
Merged

fix: CI/firebase fixes and expanded R8 dead-code-elimination tests#152
kirich1409 merged 4 commits intomainfrom
worktree-fix-ci-failures

Conversation

@kirich1409
Copy link
Copy Markdown
Contributor

@kirich1409 kirich1409 commented Apr 3, 2026

Summary

CI / Firebase fixes

  • Remove pull_request trigger from docs build workflow — docs are only deployed on push to main
  • Remove stale ADR reference from best-practices.md and mkdocs.yml (file does not exist)
  • Fix FirebaseConfigValueProvider: remove redundant RuntimeException re-throw so all exceptions are consistently wrapped in FetchException

R8 elimination test coverage

Expanded R8EliminationTest from 2 cases to 5, covering the full flag-branch matrix:

Rule if-branch else-branch
return false eliminated survives
return true survives eliminated
no rule (baseline) survives survives

Also added an Int flag family: -assumevalues return 0 causes R8 to fold 0 > 0 to false and eliminate the guarded class; without the rule the class survives.

Key finding during implementation: R8 eliminates write-only fields and their owning class as an additional optimisation. The surviving branch's sideEffect field must be pinned with -keepclassmembers; the dead branch's class is intentionally left unprotected so R8 can remove it.

Test plan

  • CI passes on this PR
  • All 5 R8EliminationTest cases pass locally (./gradlew :featured-gradle-plugin:test)
  • Docs workflow no longer triggers on pull requests
  • Firebase fetch errors are properly wrapped in FetchException

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings April 3, 2026 08:53
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix Firebase exception wrapping and remove docs PR trigger

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Fix Firebase exception handling to consistently wrap all exceptions in FetchException
• Remove docs build trigger from pull_request workflow events
• Remove stale ADR 001 references from documentation and navigation
Diagram
flowchart LR
  A["Firebase Exception Handling"] -->|"Remove redundant RuntimeException re-throw"| B["Consistent FetchException Wrapping"]
  C["CI/CD Workflow"] -->|"Remove pull_request trigger"| D["Docs build on main/tags only"]
  E["Documentation Files"] -->|"Remove missing ADR 001 references"| F["Clean navigation and links"]
Loading

Grey Divider

File Changes

1. providers/firebase/src/main/kotlin/dev/androidbroadcast/featured/firebase/FirebaseConfigValueProvider.kt 🐞 Bug fix +0/-2

Wrap Firebase exceptions consistently in FetchException

• Remove redundant RuntimeException catch block that was re-throwing bare exceptions
• Ensure all exceptions from Firebase Remote Config operations are wrapped in FetchException
• Maintain consistent error handling contract for callers

providers/firebase/src/main/kotlin/dev/androidbroadcast/featured/firebase/FirebaseConfigValueProvider.kt


2. .github/workflows/docs.yml ⚙️ Configuration changes +0/-3

Remove docs build from pull request trigger

• Remove pull_request trigger from docs workflow configuration
• Docs now build and deploy only on push to main branch and version tags
• Prevent slow documentation builds from blocking PR merges

.github/workflows/docs.yml


3. docs/guides/best-practices.md 📝 Documentation +0/-1

Remove stale ADR 001 documentation reference

• Remove reference to non-existent ADR 001 documentation file
• Clean up dead link that was causing documentation build failures

docs/guides/best-practices.md


View more (1)
4. mkdocs.yml 📝 Documentation +0/-2

Remove ADR 001 from documentation navigation

• Remove ADR section from navigation menu
• Remove reference to missing ADR 001 file that was causing mkdocs strict mode failures

mkdocs.yml


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 3, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. Cancellation wrapped in FetchException🐞 Bug ≡ Correctness
Description
FirebaseConfigValueProvider.fetch() now catches all Exception from task.await() and wraps it in
FetchException, which will also catch coroutine CancellationException and prevent cancellation from
propagating. This can break structured concurrency by turning cancellations/timeouts into regular
failures and skipping expected cancellation cleanup paths.
Code

providers/firebase/src/main/kotlin/dev/androidbroadcast/featured/firebase/FirebaseConfigValueProvider.kt[R99-103]

   try {
       task.await()
-        } catch (e: RuntimeException) {
-            throw e
   } catch (e: Exception) {
       throw FetchException("Firebase Remote Config fetch failed", e)
   }
Evidence
The provider uses kotlinx.coroutines.tasks.await() inside a try/catch that catches Exception and
always throws FetchException, so any Exception subclasses thrown by await() (including cancellation)
are converted into FetchException. ConfigValues.fetch() is explicitly documented as unguarded and
intended to propagate exceptions to callers, making correct cancellation propagation particularly
important for callers using timeouts/cancellation.

providers/firebase/src/main/kotlin/dev/androidbroadcast/featured/firebase/FirebaseConfigValueProvider.kt[8-104]
core/src/commonMain/kotlin/dev/androidbroadcast/featured/ConfigValues.kt[27-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`FirebaseConfigValueProvider.fetch()` wraps all `Exception`s from `task.await()` into `FetchException`. This also intercepts coroutine cancellation (e.g., timeouts or parent-scope cancellation) and converts it into a normal failure, preventing cancellation from propagating correctly.
## Issue Context
This is a suspend function that callers may run inside structured concurrency contexts (e.g., `withTimeout`, `viewModelScope`). Cancellation should propagate unchanged.
## Fix Focus Areas
- providers/firebase/src/main/kotlin/dev/androidbroadcast/featured/firebase/FirebaseConfigValueProvider.kt[92-104]
### Suggested change
Add a dedicated `catch (e: CancellationException) { throw e }` before the generic `catch (e: Exception)` (import `kotlinx.coroutines.CancellationException`). Keep wrapping other exceptions in `FetchException`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Docs not validated on PR🐞 Bug ☼ Reliability
Description
After removing the docs workflow pull_request trigger, MkDocs is no longer built/validated for pull
requests, and the main CI workflow does not run mkdocs at all. Documentation changes can now merge
while breaking mkdocs build --strict, only failing after merge when the docs publish workflow runs
on main.
Code

.github/workflows/docs.yml[L10-12]

-  pull_request:
-    branches:
-      - main
Evidence
The docs workflow is now triggered only on push to main (and tags), with the PR trigger removed.
The primary CI workflow (which does run on pull_request) contains only tests/build/lint steps and
does not include any MkDocs/Dokka docs build step, so there is no PR-time docs validation remaining
in workflows.

.github/workflows/docs.yml[1-10]
.github/workflows/ci.yml[1-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Docs are no longer built on PRs, so `mkdocs build --strict` failures (broken nav, broken links, invalid markdown) will only be detected after merge when the `Publish Docs` workflow runs on `main`.
## Issue Context
You can keep publishing restricted to `push`, while still validating docs on `pull_request` via a separate check job.
## Fix Focus Areas
- .github/workflows/ci.yml[1-113]
- .github/workflows/docs.yml[1-74]
### Suggested change options
1) Add a lightweight `docs-check` job to `CI` that installs MkDocs and runs `mkdocs build --strict`.
2) Alternatively, re-add a `pull_request` trigger to `docs.yml` but ensure only the `build-docs` job runs on PRs (keep `publish-docs` gated to push, as it already is with `if: github.event_name == 'push'`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR streamlines documentation publishing in CI while cleaning up stale docs navigation and making Firebase Remote Config fetch errors consistently surface as FetchException within the Firebase provider module.

Changes:

  • Remove the docs workflow pull_request trigger so docs build/publish runs only on push (main/tags).
  • Remove stale ADR references from MkDocs navigation and best-practices guide.
  • Simplify FirebaseConfigValueProvider.fetch() exception handling by removing a redundant RuntimeException rethrow.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
providers/firebase/src/main/kotlin/dev/androidbroadcast/featured/firebase/FirebaseConfigValueProvider.kt Adjusts exception handling during Firebase Remote Config await() to wrap failures.
mkdocs.yml Removes a nav entry pointing to a non-existent ADR page.
docs/guides/best-practices.md Removes an in-text link to the removed/non-existent ADR.
.github/workflows/docs.yml Stops running the docs workflow on PRs; keeps it on push to main/tags.

Comment on lines 99 to 103
try {
task.await()
} catch (e: RuntimeException) {
throw e
} catch (e: Exception) {
throw FetchException("Firebase Remote Config fetch failed", e)
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch() now wraps all Exceptions in FetchException, which will also catch kotlinx.coroutines.CancellationException thrown by task.await(). That breaks structured concurrency by converting coroutine cancellation into a fetch failure. Add a dedicated catch (e: CancellationException) { throw e } (and import it) before the generic catch (e: Exception) so cancellation is propagated unchanged.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2fc7b63.

@kirich1409 kirich1409 changed the title fix(ci): remove docs build from pull_request trigger; fix firebase exception wrapping fix: CI/firebase fixes and expanded R8 dead-code-elimination tests Apr 3, 2026
kirich1409 and others added 4 commits April 3, 2026 12:54
Docs are now built and published only on push to main or release tags.
Running a full Dokka + MkDocs build on every PR is slow and its failure
should not block merging.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atrix and int flags

- Replace single-branch caller with a bifurcated caller (if + else) so one
  input JAR covers the full 2×2 boolean matrix:
    · flag=false → if-branch eliminated, else-branch present
    · flag=true  → else-branch eliminated, if-branch present
    · no assumevalues → both branches survive (baseline)
- Add Int flag test family: asserts that -assumevalues return 0 causes R8 to
  constant-fold `0 > 0` to false and eliminate the guarded class
- Extract sideEffectClassBytes(name) helper to deduplicate branch-target bytecode
- Add -keepclassmembers for the surviving branch's sideEffect field to prevent
  R8 from eliminating it via write-only field optimisation (dead class stays
  unprotected so R8 can eliminate it freely)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract runBooleanR8/runIntR8/runR8WithJar helpers to eliminate repeated
  three-line jar/rules/output setup from every test method
- Expand Opcodes.* wildcard to explicit imports (ktlint standard:no-wildcard-imports)
- Apply spotless formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Exception

Removing the RuntimeException catch in the previous commit inadvertently
caused CancellationException (a RuntimeException subclass) to be swallowed
and wrapped in FetchException, breaking structured concurrency.

Also restore pull_request trigger in docs.yml — publish-docs is already
gated with `if: github.event_name == 'push'` so docs are validated on PRs
but not published.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kirich1409 kirich1409 force-pushed the worktree-fix-ci-failures branch 2 times, most recently from 22eb01c to baeb076 Compare April 3, 2026 09:55
@kirich1409 kirich1409 merged commit 5301c4f into main Apr 3, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants