feat: add runtime safe margin controls#464
Open
Albermonte wants to merge 2 commits intoCap-go:mainfrom
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds runtime APIs to toggle safe-area top and bottom margins for in-app webviews across TypeScript, web (no-op), Android, and iOS implementations; platform code updates options/constraints and triggers inset/layout reapplication for the targeted webview (optional id or active one). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as App/Client
participant PluginA as Android Plugin
participant UIThread as UI Thread
participant Dialog as WebViewDialog
participant WebV as WebView
Client->>PluginA: setEnabledSafeTopMargin({enabled, id?})
PluginA->>PluginA: parse options, find dialog
PluginA->>UIThread: run on UI thread
UIThread->>Dialog: setEnabledSafeTopMargin(enabled)
Dialog->>Dialog: update options if changed
Dialog->>WebV: ViewCompat.requestApplyInsets(WebV)
WebV->>WebV: recompute insets/layout
Dialog-->>UIThread: complete
UIThread-->>PluginA: resolve call
PluginA-->>Client: Promise<void>
sequenceDiagram
participant Client as App/Client
participant PluginI as iOS Plugin
participant MainQ as Main Queue
participant VC as WKWebViewController
participant Layout as Layout Engine
Client->>PluginI: setEnabledSafeBottomMargin({enabled, id?})
PluginI->>MainQ: dispatch async
MainQ->>VC: locate target controller
MainQ->>VC: updateSafeBottomMargin(enabled)
VC->>VC: deactivate old bottom constraint
VC->>VC: create & activate new bottom constraint (safeArea or view)
VC->>Layout: setNeedsLayout / layoutIfNeeded
Layout->>VC: recompute webView frame
VC-->>MainQ: complete
MainQ-->>PluginI: resolve call
PluginI-->>Client: Promise<void>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java`:
- Around line 1281-1285: The code in setEnabledSafeTopMargin currently defaults
missing "enabled" to true, hiding client bugs; change it to validate presence of
the "enabled" key and reject the call immediately when absent (use
call.reject("missing 'enabled' boolean") and return) instead of defaulting;
update the same pattern where call.getBoolean("enabled", true) is used (also the
occurrence near the resolveTargetId usage) so both the
call.getData().has("enabled") check leads to a rejection path and subsequent
code uses call.getBoolean("enabled") only after presence is confirmed.
In `@ios/Sources/InAppBrowserPlugin/WKWebViewController.swift`:
- Around line 2035-2070: The methods updateSafeTopMargin(_:) and
updateSafeBottomMargin(_:) should still flip the enabled flags but must avoid
changing constraints when the webView is not a subview of self.view to prevent
"no common ancestor" Auto Layout crashes; after guarding and setting
self.enabledSafeTopMargin/self.enabledSafeBottomMargin and obtaining webView,
add a check that webView.superview === self.view and only then run the
constraint deactivation/activation and layoutIfNeeded logic; leave early
(return) if the webView has been moved to another hierarchy.
In `@README.md`:
- Around line 716-734: Update the runtime docs for setEnabledSafeTopMargin and
setEnabledSafeBottomMargin to explicitly state that these APIs are no-ops on the
web platform; locate the entries for setEnabledSafeTopMargin (around the "Sets
the enabled safe top margin..." description) and setEnabledSafeBottomMargin (the
corresponding bottom margin section) and append a short note like "On web, this
API is a no-op" or similar to each description so readers know the behavior
differs by platform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d14994-67ca-45dd-9a2e-eccc041a2326
📒 Files selected for processing (7)
README.mdandroid/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.javaandroid/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.javaios/Sources/InAppBrowserPlugin/InAppBrowserPlugin.swiftios/Sources/InAppBrowserPlugin/WKWebViewController.swiftsrc/definitions.tssrc/web.ts
android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java
Outdated
Show resolved
Hide resolved
Require explicit boolean input for Android runtime safe-margin toggles instead of silently defaulting, preventing hidden client-side bugs. Guard iOS runtime safe-margin constraint updates when webView is detached from self.view to avoid no-common-ancestor Auto Layout crashes. Document that web implementations of runtime safe-margin APIs are no-ops in generated API docs.
|
Member
|
Build seems to fail can you have a look? |
Contributor
Author
Weird, it was working when creating the PR, could be the changes from CodeRabbit? Edit: Seeing this in the CI, can you try to re-run it? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds runtime control for safe margins by introducing setEnabledSafeTopMargin and setEnabledSafeBottomMargin.
It enables dynamic safe-area toggling without reopening the browser.
Tested on iOS and Android, though I don't see many changes on Android as I do on iOS.
Summary by CodeRabbit