#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 10 commits into
Conversation
…pp is not installed to home screen
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds Safari-specific warning detection and dismissal persistence, renders a dismissible banner on annotation pages, and shows a temporary “Annotation saved…” hint after bookmark, highlight, or note creation. ChangesSafari annotation warning and hint flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. |
chrisvire
left a comment
chrisvire
left a comment
There was a problem hiding this comment.
I had a conversation with ChatGPT about this.
The main concern is the web app running in a iOS browser tab (any browser, not just Safari since all browsers on iOS are required to use Apple's WebKit engine).
Please make these changes:
- Check for iOS instead of just Safari.
- Have different messages for iOS and macOS
- Dismissing the message should not be permanent. We should re-display it periodically (the x hides it for 7 days).
- The first time they create an annotation, we should display a popup message that goes away (saying see the page for more details). [Note: for an example, see AudioBar's play mode button.]
- Please make the warning dialog have some vertical padding so it isn't right next to the note:
Here is a suggestion for iOS:
title: Important for iPhone and iPad users
body: Safari may remove locally stored website data, including , if this website is only used in the browser. To reduce the risk of data loss, install this app to your Home Screen.
Learn more:
Apple: Add this app to your Home Screen
WebKit: Website storage and privacy policies - 7-Day Cap on All Script-Writable Storage
For macOS, change "to your Home Screen" to be "to your Dock"
|
Understood will be working on this tonight. Thank you! |
…WarningContext type, first-annotation popup
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/notes/edit/[noteid]/+page.svelte (1)
52-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOnly mark the annotation hint as shown after it can actually be displayed —
createNote()consumes the one-timesafari_annotation_hint_shownflag without any UI on this page, so a first note can hide the hint forever before the toolbar gets a chance to render it. CallmarkAnnotationHintShown()from the component that shows the hint, or add the same transient hint here.🤖 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/routes/notes/edit/`[noteid]/+page.svelte around lines 52 - 66, The createNote() flow in +page.svelte is marking the annotation hint as shown too early, before this page can actually display it. Move the markAnnotationHintShown() call into the component that renders the hint (or make createNote() show the same transient hint on this page) so the one-time safari_annotation_hint_shown flag is only consumed when the hint is actually visible.
🤖 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.
Outside diff comments:
In `@src/routes/notes/edit/`[noteid]/+page.svelte:
- Around line 52-66: The createNote() flow in +page.svelte is marking the
annotation hint as shown too early, before this page can actually display it.
Move the markAnnotationHintShown() call into the component that renders the hint
(or make createNote() show the same transient hint on this page) so the one-time
safari_annotation_hint_shown flag is only consumed when the hint is actually
visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 329214b5-8b5f-4a06-a7c9-2596e6d9ae4c
📒 Files selected for processing (7)
src/lib/components/SafariAnnotationWarning.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/scripts/safariUtils.tssrc/routes/bookmarks/+page.sveltesrc/routes/highlights/+page.sveltesrc/routes/notes/+page.sveltesrc/routes/notes/edit/[noteid]/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/highlights/+page.svelte
- src/routes/bookmarks/+page.svelte
- src/routes/notes/+page.svelte
|
okay i think i am done. The remaining failures are unrelated to the annotation warning feature i think. |
3 participants
Summary
Test Plan
closes #968
Summary by CodeRabbit