#968 Warn Safari users that annotations may be deleted when app is not installed to home screen#999
#968 Warn Safari users that annotations may be deleted when app is not installed to home screen#999cdelgado10 wants to merge 4 commits into
Conversation
…pp is not installed to home screen
|
Warning Review limit reached
More reviews will be available in 48 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA new ChangesSafari Annotation Warning Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant AnnotationPage as Annotation Page (bookmarks/highlights/notes)
participant SafariAnnotationWarning
participant safariUtils
participant localStorage
User->>AnnotationPage: navigate to page
AnnotationPage->>SafariAnnotationWarning: mount component
SafariAnnotationWarning->>safariUtils: shouldShowSafariWarning()
safariUtils->>localStorage: read dismissal timestamp
localStorage-->>safariUtils: timestamp or null
safariUtils-->>SafariAnnotationWarning: true (Safari, not standalone, not dismissed)
SafariAnnotationWarning-->>AnnotationPage: render amber warning banner
User->>SafariAnnotationWarning: click dismiss
SafariAnnotationWarning->>safariUtils: dismissSafariWarning()
safariUtils->>localStorage: write current timestamp
SafariAnnotationWarning-->>AnnotationPage: set visible = false, hide banner
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/components/SafariAnnotationWarning.svelte (1)
28-32: ⚡ Quick winLocalize banner text instead of hardcoding English copy.
These lines bypass the existing translation pattern (
$t) used across annotation pages, so localized users will still see English here. Please move this copy to translation keys.🤖 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 `@src/lib/components/SafariAnnotationWarning.svelte` around lines 28 - 32, The SafariAnnotationWarning.svelte component contains hardcoded English text in the two paragraph elements instead of using the translation function pattern (`$t`) that is used throughout the rest of the annotation pages. Replace the hardcoded strings "Annotations may be deleted by Safari" and the inactivity warning message with corresponding translation keys using the `$t` function. Move these text strings to the appropriate translation files and reference them via `$t('key_name')` in both `<p>` tags to ensure localized users see translated content instead of English.
🤖 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 `@src/lib/scripts/safariUtils.ts`:
- Around line 15-16: The debug safari check is currently parsing
window.location.hash instead of the actual query string, which prevents the
debug_safari=true parameter from being recognized in normal URL query
parameters. Replace window.location.hash with window.location.search on line 15
to properly check the URL query parameters, allowing the debug override to work
correctly with query strings like ?debug_safari=true instead of requiring it in
the hash portion of the URL.
- Around line 18-24: The code needs to handle storage access exceptions and
invalid timestamp values gracefully. In the function that checks if the warning
should be shown (containing the localStorage.getItem call on line 18 and
parseInt on line 20), wrap the localStorage.getItem call in a try-catch block
and return true (show warning) if an exception occurs. Additionally, validate
the result of parseInt by checking if the value is NaN, and return true (show
warning) in that case to fail open instead of treating malformed timestamps as
dismissed. In the dismissSafariWarning function (line 23-24), wrap the
localStorage.setItem call in a try-catch block to prevent crashes when storage
is unavailable, and handle the exception gracefully without breaking the warning
system.
---
Nitpick comments:
In `@src/lib/components/SafariAnnotationWarning.svelte`:
- Around line 28-32: The SafariAnnotationWarning.svelte component contains
hardcoded English text in the two paragraph elements instead of using the
translation function pattern (`$t`) that is used throughout the rest of the
annotation pages. Replace the hardcoded strings "Annotations may be deleted by
Safari" and the inactivity warning message with corresponding translation keys
using the `$t` function. Move these text strings to the appropriate translation
files and reference them via `$t('key_name')` in both `<p>` tags to ensure
localized users see translated content instead of English.
🪄 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: 48762cb6-793a-4d86-9952-01b1568cf546
📒 Files selected for processing (5)
src/lib/components/SafariAnnotationWarning.sveltesrc/lib/scripts/safariUtils.tssrc/routes/bookmarks/+page.sveltesrc/routes/highlights/+page.sveltesrc/routes/notes/+page.svelte
| const hashQuery = window.location.hash.split('?')[1] ?? ''; | ||
| if (new URLSearchParams(hashQuery).get('debug_safari') === 'true') return true; |
There was a problem hiding this comment.
Debug override parsing checks the hash instead of URL query.
Line 15 parses window.location.hash even though the comment says ?debug_safari=true on the URL. That makes the override fail for normal query params and can mislead QA.
💡 Proposed fix
- const hashQuery = window.location.hash.split('?')[1] ?? '';
- if (new URLSearchParams(hashQuery).get('debug_safari') === 'true') return true;
+ const searchQuery = window.location.search;
+ const hashQuery = window.location.hash.split('?')[1] ?? '';
+ const params = new URLSearchParams(searchQuery || hashQuery);
+ if (params.get('debug_safari') === 'true') return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hashQuery = window.location.hash.split('?')[1] ?? ''; | |
| if (new URLSearchParams(hashQuery).get('debug_safari') === 'true') return true; | |
| const searchQuery = window.location.search; | |
| const hashQuery = window.location.hash.split('?')[1] ?? ''; | |
| const params = new URLSearchParams(searchQuery || hashQuery); | |
| if (params.get('debug_safari') === 'true') 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 `@src/lib/scripts/safariUtils.ts` around lines 15 - 16, The debug safari check
is currently parsing window.location.hash instead of the actual query string,
which prevents the debug_safari=true parameter from being recognized in normal
URL query parameters. Replace window.location.hash with window.location.search
on line 15 to properly check the URL query parameters, allowing the debug
override to work correctly with query strings like ?debug_safari=true instead of
requiring it in the hash portion of the URL.
|
This should not be limited to Safari. Other browsers use the Safari browser component on iOS. |
3 participants
Summary
Test Plan
closes #968
Summary by CodeRabbit