fix[ux] :: remove stale WebKit workaround and restore home back behavior#612
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR standardizes embedded JavaScript to explicitly return ChangesBack-Navigation & Script Return Standardization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
github-actions
Bot
added
test
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d0dca903a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| homeLaunchedSiteFamily: activeTab.homeLaunchedSiteFamily, | ||
| ); | ||
| final canGoBack = await activeTab.webViewController?.canGoBack() ?? false; | ||
| if (shouldReturnHome) { |
There was a problem hiding this comment.
When the first page opened from home later accumulates same-site WebView history (for example, opening GitLab from home and then following a link to another GitLab page), homeLaunchedSiteFamily still matches the current URL, so this branch runs before the canGoBack branch. That nulls the controller and shows the internal home page instead of calling goBack(), preventing users from stepping back within the site they are browsing. Consider only taking the home shortcut when the WebView cannot go back or when the current entry is known to be the initial page loaded from home.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a fair point. I think that broader Back-navigation policy may be better handled separately so this PR can stay focused on removing the stale WebKit workaround and restoring the original path without expanding scope.
| if (!siteKey.contains('.')) return siteKey; | ||
| final parts = siteKey.split('.'); | ||
| if (parts.length < 2) return siteKey; | ||
| return '${parts[parts.length - 2]}.${parts[parts.length - 1]}'; |
There was a problem hiding this comment.
For hosts under multi-label public suffixes, this takes only the last two labels, so unrelated domains such as amazon.co.uk and bbc.co.uk both become co.uk. If a user opens one .co.uk site from home and then navigates to another, shouldReturnHomeOnBack treats the second site as the original launched site family and sends Back to home instead of preserving normal WebView history. Use the full host or a public-suffix-aware registrable-domain parser here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Also fair. A correct fix here likely needs a broader host-matching approach, so I think it is safer to track that separately rather than fold a larger domain-matching change into this cleanup PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ux/browser_page.dart`:
- Around line 195-201: _siteFamilyKeyForUrl currently assumes the last two
labels are the site-family which collapses distinct registrable domains (e.g.
foo.github.io and bar.github.io or a.co.uk and b.co.uk); update
_siteFamilyKeyForUrl to compute a public-suffix/registrable-domain-aware key
instead of blindly returning the last two labels by using a public suffix list
library (e.g., a Dart publicsuffix package) to derive the registrable domain
from the output of _siteKeyForUrl and return that registrable domain; if you
cannot add the PSL dependency immediately, change the function to fall back to
returning the full host (i.e., the siteKey) rather than the last-two-label
heuristic so shouldReturnHomeOnBack() preserves real WebView history until a
proper PSL-based implementation is added.
- Around line 5883-5885: The per-tab local/session storage clear can throw and
currently lives inside a try that aborts the whole loop; modify the
reset/clear-caches loop so each call to
tab.webViewController?.runJavaScript('localStorage.clear();
sessionStorage.clear(); true;') is wrapped in its own try/catch (or await with
.catchError) to swallow storage-origin errors and allow the loop to continue
across all tabs, and ensure _navigationCacheIndex.clear() is always executed
after the loop (not skipped by a thrown error). Reference tab.webViewController
and _navigationCacheIndex.clear() when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c8fea43-5e14-4c0e-a2e6-d9885608334d
📒 Files selected for processing (5)
assets/legacy_layout_fix.jslib/features/login_detection.dartlib/features/webauthn_script.dartlib/ux/browser_page.darttest/ux/browser_page_logic_test.dart
| String? _siteFamilyKeyForUrl(String? rawUrl) { | ||
| final siteKey = _siteKeyForUrl(rawUrl); | ||
| if (siteKey == null || siteKey.isEmpty) return null; | ||
| if (!siteKey.contains('.')) return siteKey; | ||
| final parts = siteKey.split('.'); | ||
| if (parts.length < 2) return siteKey; | ||
| return '${parts[parts.length - 2]}.${parts[parts.length - 1]}'; |
There was a problem hiding this comment.
Make the site-family key public-suffix aware.
This helper currently treats the last two labels as the "family", so unrelated sites like foo.github.io / bar.github.io or a.co.uk / b.co.uk collapse to the same key. That makes shouldReturnHomeOnBack() send users straight home and skip real WebView history for valid cross-site navigations, which is the opposite of the intended fallback behavior. Please use a registrable-domain/public-suffix-aware key here, or fall back to full-host matching until that exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ux/browser_page.dart` around lines 195 - 201, _siteFamilyKeyForUrl
currently assumes the last two labels are the site-family which collapses
distinct registrable domains (e.g. foo.github.io and bar.github.io or a.co.uk
and b.co.uk); update _siteFamilyKeyForUrl to compute a
public-suffix/registrable-domain-aware key instead of blindly returning the last
two labels by using a public suffix list library (e.g., a Dart publicsuffix
package) to derive the registrable domain from the output of _siteKeyForUrl and
return that registrable domain; if you cannot add the PSL dependency
immediately, change the function to fall back to returning the full host (i.e.,
the siteKey) rather than the last-two-label heuristic so
shouldReturnHomeOnBack() preserves real WebView history until a proper PSL-based
implementation is added.
There was a problem hiding this comment.
Fair point. This overlaps with the earlier host-matching concern, and I think a correct fix likely needs a broader approach, so it seems safer to track separately rather than expand the scope of this cleanup PR.
There was a problem hiding this comment.
@bniladridas Sounds good—agree it’s safer to track this separately and keep #612 focused.
If you’d like, I can open a follow-up GitHub issue with the draft below and assign it to you. Prefer that?
-
Title
Make site-family key public-suffix (eTLD+1) aware for home-back heuristic -
Background
In lib/ux/browser_page.dart, _siteFamilyKeyForUrl currently uses a last-two-label heuristic. This collapses unrelated hosts under the same public suffix (e.g., foo.github.io vs bar.github.io, a.co.uk vs b.co.uk), which can cause shouldReturnHomeOnBack(...) to skip real WebView history and jump home. -
Impact
Cross-site navigations under multi-label public suffixes may be falsely treated as the “same site family,” leading to premature home returns. -
Proposal
- Replace _siteFamilyKeyForUrl with a registrable-domain (PSL/eTLD+1) aware computation.
- If PSL data/library isn’t available, fall back to full-host matching (not last-two-labels) to avoid false positives.
- Keep behavior unchanged when homeUrl != defaultHomepageUrl.
-
Tests (test/ux/browser_page_logic_test.dart)
- foo.github.io vs bar.github.io → different families → shouldReturnHomeOnBack = false
- a.co.uk vs b.co.uk → different families → false
- sub.example.com vs api.example.com → same registrable domain → true
- defaultHomepageUrl gating remains respected.
-
Acceptance criteria
- PSL-aware key derivation with safe fallback.
- Updated unit tests reflecting the above.
- Brief inline doc/TODO near _siteFamilyKeyForUrl explaining the PSL dependency/fallback.
Reply “yes” and I’ll open the issue and link back to this PR and comment; I can also drop a TODO next to _siteFamilyKeyForUrl once the issue number exists. If you’d rather handle it yourself, no problem—I’ll stand by.
✏️ Learnings added
Learnt from: bniladridas
Repo: bniladridas/browser PR: 612
File: lib/ux/browser_page.dart:195-201
Timestamp: 2026-05-06T22:36:19.455Z
Learning: Repository: browser. In lib/ux/browser_page.dart, _siteFamilyKeyForUrl should be replaced with a public-suffix (eTLD+1) aware implementation. Current last-two-label heuristic can collapse unrelated hosts (e.g., *.github.io, *.co.uk). Team chose to defer and track as a separate follow-up issue from PR `#612`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| await tab.webViewController?.runJavaScript( | ||
| 'localStorage.clear(); sessionStorage.clear(); true;', | ||
| ); |
There was a problem hiding this comment.
Keep cache clearing best-effort across all tabs.
localStorage / sessionStorage access can throw on opaque, sandboxed, or file: origins. Because this runs inside the outer try, one failing tab aborts the rest of the loop and also skips _navigationCacheIndex.clear(), so "Clear caches" can silently leave data behind. Catch these storage errors per tab/per storage access instead of failing the whole reset.
Suggested hardening
- await tab.webViewController?.runJavaScript(
- 'localStorage.clear(); sessionStorage.clear(); true;',
- );
+ await tab.webViewController?.runJavaScript(r'''
+ (() => {
+ try { localStorage.clear(); } catch (_) {}
+ try { sessionStorage.clear(); } catch (_) {}
+ return true;
+ })();
+ ''');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ux/browser_page.dart` around lines 5883 - 5885, The per-tab local/session
storage clear can throw and currently lives inside a try that aborts the whole
loop; modify the reset/clear-caches loop so each call to
tab.webViewController?.runJavaScript('localStorage.clear();
sessionStorage.clear(); true;') is wrapped in its own try/catch (or await with
.catchError) to swallow storage-origin errors and allow the loop to continue
across all tabs, and ensure _navigationCacheIndex.clear() is always executed
after the loop (not skipped by a thrown error). Reference tab.webViewController
and _navigationCacheIndex.clear() when making these changes.
There was a problem hiding this comment.
Also fair. This looks like a useful hardening change, but it is separate from the stale WebKit workaround cleanup in this PR, so I think it is better tracked on its own rather than folded into the current scope.
There was a problem hiding this comment.
@bniladridas Sounds good to keep this hardening separate. I can open a follow-up GitHub issue and link back here. Want me to file it and assign it to you?
Proposed issue
- Title: Harden _clearAllCaches to swallow per-tab storage-origin errors and always clear navigation cache
- Why: localStorage/sessionStorage access (and runJavaScript itself) can fail on opaque/sandboxed/file: origins or disposed controllers. Today a single failure can abort the loop and skip _navigationCacheIndex.clear(), leaving data behind.
- Scope of fix:
- Wrap per-tab cache/storage clears in try/catch so one tab can’t abort the loop.
- Use an IIFE that guards each storage clear.
- Ensure navigation-cache clearing runs after the loop regardless of per-tab failures.
- Sketch:
for (final tab in tabs) {
try {
await tab.webViewController?.clearCache();
} catch (_) {}
try {
await tab.webViewController?.runJavaScript(r'''
(() => {
try { localStorage.clear(); } catch (_) {}
try { sessionStorage.clear(); } catch (_) {}
return true;
})();
''');
} catch (_) {} // swallow per-tab errors
}
// then always:
_navigationCacheIndex.clear();
// ...persisted index removals, etc.If you’d like, I’ll open the issue now with this content and link to PR #612 and this comment thread.
(づ｡◕‿‿◕｡)づ 🐇
|
Thanks for the review comments. The navigation-policy and host-matching concerns raised in the inline threads look valid, but they also broaden the scope beyond the stale WebKit workaround cleanup this PR is intended to handle. I have replied inline and I think the safer path is to track those items separately so we can review them as focused follow-up changes rather than mix them into this cleanup. The cache-clearing hardening comment also looks useful, but it is likewise separate from the main purpose of this PR. |
2 participants
Summary
Impact
Related Items
Notes for reviewers
Review Process
Summary by CodeRabbit
Bug Fixes
Tests