Skip to content

Simplify IterableEmbeddedManager.handleEmbeddedClick API#1022

Closed
franco-zalamena-iterable wants to merge 1 commit intomasterfrom
fix/969-simplify-handle-embedded-click
Closed

Simplify IterableEmbeddedManager.handleEmbeddedClick API#1022
franco-zalamena-iterable wants to merge 1 commit intomasterfrom
fix/969-simplify-handle-embedded-click

Conversation

@franco-zalamena-iterable
Copy link
Copy Markdown
Contributor

@franco-zalamena-iterable franco-zalamena-iterable commented Apr 7, 2026

Summary

  • Add new simplified handleEmbeddedClick(clickedUrl: String) method that only requires the URL parameter
  • Deprecate old handleEmbeddedClick(message, buttonIdentifier, clickedUrl) method with @Deprecated annotation and ReplaceWith hint
  • Add KDoc documentation to both methods
  • Old method delegates to new one internally

Test plan

  • Verify new method works correctly with action://, itbl://, and standard URLs
  • Verify old deprecated method still works and delegates to new method
  • Check deprecation warning appears in IDE when using old method

🤖 Generated with Claude Code

Add a new handleEmbeddedClick(clickedUrl: String) method that only takes
the URL parameter, since message and buttonIdentifier were unused.
Deprecate the old three-parameter method with @deprecated annotation
pointing to the new simplified version. Add KDoc documentation to both.

Fixes #969

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@franco-zalamena-iterable
Copy link
Copy Markdown
Contributor Author

PR Analysis

Problem: IterableEmbeddedManager.handleEmbeddedClick(message, buttonIdentifier, clickedUrl) requires message and buttonIdentifier parameters that are never used internally, making the API unnecessarily cumbersome. No documentation exists to clarify this.

Ideal fix plan:

  • Add a new overload handleEmbeddedClick(clickedUrl: String) containing the actual logic
  • Deprecate the old 3-parameter method and have it delegate to the new one
  • Add KDoc to both methods
  • Update internal callers (e.g., IterableEmbeddedView) to use the new method so they don't trigger deprecation warnings
  • Add unit tests for the new method (at minimum: action:// URL, itbl:// URL, standard URL, empty string)

What the PR did:

  • Added handleEmbeddedClick(clickedUrl: String) with the core logic
  • Deprecated the old method with @Deprecated + ReplaceWith annotation
  • Old method delegates to new one via clickedUrl ?: ""
  • Added KDoc to both methods

Assessment:

  • Root cause identified: yes -- the unused parameters are correctly identified
  • Fix correctness: partially correct -- the new overload and deprecation are done well, but there is an omission (see below)
  • Missed:
    • IterableEmbeddedView.kt (in iterableapi-ui) calls the old method in both setDefaultAction and setButton. These internal SDK callers should be updated to use the new simplified method. Shipping a deprecated method that your own SDK still calls is not ideal -- consumers will see the deprecated API in the SDK's own view code, and IDE warnings will fire inside the SDK itself.
    • No unit tests were added. The handleEmbeddedClick method has branching logic (action:// vs itbl:// vs standard URL vs empty string) that should be covered by tests. The PR's own test plan lists manual verification, but automated tests are appropriate here.
  • Wrong assessment: none -- the analysis and approach are sound
  • Tests: needed but missing -- branching URL-handling logic with three distinct code paths warrants unit test coverage

@franco-zalamena-iterable franco-zalamena-iterable deleted the fix/969-simplify-handle-embedded-click branch April 8, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant