better mirrors page #426
better mirrors page #426
Conversation
Add better mirrors page
WalkthroughThe pull request rewrites the mirrors page from an iframe-based status display to a fully interactive Svelte component, introduces a new TypeScript configuration for mirrors, updates the handoff page's cancel button logic, and clarifies text in the tracker dialog. No changes to exported APIs or control flow were made outside the mirrors page. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MirrorsPage
participant BackupUtil
participant Toast
participant Browser
User->>MirrorsPage: View list of mirrors
MirrorsPage->>MirrorsPage: Highlight current mirror, show badges/notes
User->>MirrorsPage: Click "Go" on a mirror
MirrorsPage->>Browser: Navigate to mirror URL
User->>MirrorsPage: Click "Migrate" on a mirror
MirrorsPage->>BackupUtil: createBackup()
alt Success
BackupUtil-->>MirrorsPage: Backup data
MirrorsPage->>Browser: Redirect to mirror handoff URL with backup data
else Error
BackupUtil-->>MirrorsPage: Error
MirrorsPage->>Toast: Show error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related issues
Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying edutools-testing with
|
| Latest commit: |
462c07d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4b650b6a.edutools-testing.pages.dev |
| Branch Preview URL: | https://main.edutools-testing.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/routes/mirrors/mirrors.config.ts (1)
7-15: Consider adding notes to explain quality ratings.The mirror configurations look good, but adding notes would help users understand why certain mirrors are highly recommended or not recommended (e.g., performance, reliability, geographical location).
src/routes/mirrors/+page.svelte (1)
22-39: Consider reducing URL parsing duplication.The URL is parsed twice - once for selection check and once for display. Consider parsing once and reusing the result.
-{#each mirrors as mirror} - {@const selected = (() => { - try { - return page.url.hostname == new URL(mirror.url).hostname; - } catch (e) { - return false; - } - })()} +{#each mirrors as mirror} + {@const parsedUrl = (() => { + try { + return new URL(mirror.url); + } catch (e) { + return null; + } + })()} + {@const selected = parsedUrl && page.url.hostname === parsedUrl.hostname} <Card.Root class={clsx(selected && 'bg-neutral-200 dark:bg-neutral-700')}> <Card.Header> <Card.Title> - {(() => { - try { - return new URL(mirror.url).hostname; - } catch (e) { - return 'Invalid URL'; - } - })()} + {parsedUrl ? parsedUrl.hostname : 'Invalid URL'} </Card.Title>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/tracker-dialog.svelte(1 hunks)src/routes/handoff/+page.svelte(1 hunks)src/routes/mirrors/+page.svelte(1 hunks)src/routes/mirrors/mirrors.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.*
⚙️ CodeRabbit Configuration File
**/*.*: Do not correct spelling errors or grammar mistakes.
Files:
src/lib/components/tracker-dialog.sveltesrc/routes/handoff/+page.sveltesrc/routes/mirrors/mirrors.config.tssrc/routes/mirrors/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
src/routes/mirrors/mirrors.config.ts (1)
1-5: Well-structured type definition with good type safety.The Mirror type is clean and uses string literal unions effectively for the quality field.
src/routes/handoff/+page.svelte (1)
83-85: Verify navigation behavior when users access this page directly.The removal of the same-origin check simplifies the code, but could potentially navigate users away from the application if they access this page directly (e.g., via bookmark or shared link).
Consider whether a fallback to the home page is still needed for edge cases:
onclick={() => { - history.back(); + if (window.history.length > 1) { + history.back(); + } else { + window.location.href = '/'; + } }}src/lib/components/tracker-dialog.svelte (1)
14-15: Clear and transparent messaging improvements.The updated text provides better clarity about Posthog usage and maintains user trust.
src/routes/mirrors/+page.svelte (2)
1-16: Clean and well-organized imports.All imports are properly used in the component.
41-56: Excellent visual hierarchy with quality badges.The badge implementation provides clear visual distinction between quality levels with appropriate colors and icons.
| <Button | ||
| onclick={() => { | ||
| try { | ||
| const backup = createBackup(); | ||
| const url = new URL('/handoff', mirror.url); | ||
| url.searchParams.set('data', backup); | ||
| location.href = url.toString(); | ||
| } catch (error) { | ||
| console.error('Migration failed:', error); | ||
| toast.error('Migration failed'); | ||
| } | ||
| }} | ||
| > | ||
| Migrate | ||
| </Button> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify backup data size for URL parameter limitations.
Passing backup data via URL query parameters may hit browser URL length limits (typically 2048-8192 characters). Large backups could fail silently or be truncated.
Consider validating the backup size or using an alternative method for large data transfers:
Alternative approach for large backups:
onclick={() => {
try {
const backup = createBackup();
+ // Check backup size before URL encoding
+ const encodedBackup = encodeURIComponent(backup);
+ if (encodedBackup.length > 2000) {
+ toast.error('Backup data too large for migration. Please export and import manually.');
+ return;
+ }
const url = new URL('/handoff', mirror.url);
url.searchParams.set('data', backup);
location.href = url.toString();
} catch (error) {
console.error('Migration failed:', error);
toast.error('Migration failed');
}
}}🏁 Script executed:
#!/bin/bash
# Check if createBackup function has any size validation or if there are any tests that verify large backup handling
rg -A 10 "createBackup" --type ts --type js --type svelte
# Look for any existing URL length validation
rg -i "url.*length|length.*limit|max.*length" --type ts --type js --type svelteLength of output: 201
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Searching for createBackup definitions and references in .ts, .js, and .svelte files"
rg -n "createBackup" -g "*.ts" -g "*.js" -g "*.svelte" || echo "No matches for createBackup"
echo
echo "2. Searching for any length, size, or limit checks in .ts, .js, and .svelte files"
rg -n -i "length|size|limit" -g "*.ts" -g "*.js" -g "*.svelte" || echo "No length/size/limit validations found"Length of output: 263771
Implement Backup Size Handling Before URL Navigation
A review of createBackup() usages in src/routes/.../+page.svelte and src/lib/createBackup.ts shows no existing data‐size validation or URL‐length safeguards. Large backups risk exceeding browser URL limits, causing silent failures.
Please add logic before location.href to:
- Encode and measure the backup payload (e.g. via
encodeURIComponent(...)) - If its length exceeds a safe threshold (e.g. 2000 characters), show an error and skip navigation
- Otherwise proceed as before
Example diff for src/routes/mirrors/+page.svelte:
onclick={() => {
try {
- const backup = createBackup();
+ const backup = createBackup();
+ const encoded = encodeURIComponent(backup);
+ if (encoded.length > 2000) {
+ toast.error('Backup too large for migration. Please export/import manually.');
+ return;
+ }
const url = new URL('/handoff', mirror.url);
url.searchParams.set('data', backup);
location.href = url.toString();
} catch (error) {
console.error('Migration failed:', error);
toast.error('Migration failed');
}
}}Apply similar checks in all code paths that construct a migration URL.
📝 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.
| <Button | |
| onclick={() => { | |
| try { | |
| const backup = createBackup(); | |
| const url = new URL('/handoff', mirror.url); | |
| url.searchParams.set('data', backup); | |
| location.href = url.toString(); | |
| } catch (error) { | |
| console.error('Migration failed:', error); | |
| toast.error('Migration failed'); | |
| } | |
| }} | |
| > | |
| Migrate | |
| </Button> | |
| <Button | |
| onclick={() => { | |
| try { | |
| const backup = createBackup(); | |
| const encoded = encodeURIComponent(backup); | |
| if (encoded.length > 2000) { | |
| toast.error('Backup too large for migration. Please export/import manually.'); | |
| return; | |
| } | |
| const url = new URL('/handoff', mirror.url); | |
| url.searchParams.set('data', backup); | |
| location.href = url.toString(); | |
| } catch (error) { | |
| console.error('Migration failed:', error); | |
| toast.error('Migration failed'); | |
| } | |
| }} | |
| > | |
| Migrate | |
| </Button> |
🤖 Prompt for AI Agents
In src/routes/mirrors/+page.svelte around lines 61 to 75, the backup data
created by createBackup() is directly appended to the URL without checking its
size, risking exceeding browser URL length limits. To fix this, encode the
backup string using encodeURIComponent, measure its length, and if it exceeds a
safe threshold like 2000 characters, display an error toast and do not proceed
with navigation. Only set location.href if the encoded backup size is within the
limit. Apply this size check and error handling in all similar migration URL
constructions in this file.
nathblade16
left a comment
nathblade16
left a comment
There was a problem hiding this comment.
seems to work and code looks fine, the only improvement I could think of related to this pr is make the mirror list seperate a json file compared to a typescript
Summary by CodeRabbit
New Features
Improvements
Bug Fixes