Add download button, tab drag-drop, keyboard shortcuts#1
Add download button, tab drag-drop, keyboard shortcuts#1afar1 wants to merge 1 commit intoneedle-tools:mainfrom
Conversation
- Download markdown button to save page as .md file - Tab drag and drop reordering - Cmd+W close tab, Cmd+N/T new tab with URL focus - Fix extraction to capture all article sections - SPA URL tracking for proper address bar updates
afar1
force-pushed
the
feature/download-tabs-shortcuts
branch
from
5cf10eb to
3dc4543
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds several user-facing features to improve the browsing experience: a download button for saving markdown files, tab drag-and-drop reordering, additional keyboard shortcuts (Cmd+W to close tabs, Cmd+N for new tabs with URL bar auto-focus), fixes HTML extraction to capture all article sections on multi-article pages, and implements SPA URL tracking to keep the address bar synchronized with single-page application navigation.
Changes:
- Download markdown button (↓) with smart filename generation from URL or title
- Drag-and-drop tab reordering with visual feedback (opacity and border indicators)
- Keyboard shortcuts: Cmd+W closes active tab, Cmd+N/T creates new tab with URL bar focus
- Removed article tag fallback in HTML extraction to capture all sections instead of just the first article
- SPA URL tracking via history API hooks and polling to update address bar on client-side navigation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/toolbar-svelte/lib/rpc.ts | Added downloadMarkdown function with filename sanitization; removed article tag matching fallback to fix multi-article extraction |
| src/toolbar-svelte/App.svelte | Added drag-drop handlers and state; implemented Cmd+W/N/T shortcuts; added SPA URL tracking in preload script; added download button UI |
| src/bun/index.ts | Removed article tag matching for consistency; added URL update in refetchForViewMode for redirect handling; whitespace cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function handleTabDrop(e: DragEvent, targetTabId: number) { | ||
| e.preventDefault(); | ||
| if (draggedTabId !== null && draggedTabId !== targetTabId) { | ||
| // Reorder tabs locally | ||
| const tabs = [...appStore.tabs]; | ||
| const draggedIndex = tabs.findIndex(t => t.id === draggedTabId); | ||
| const targetIndex = tabs.findIndex(t => t.id === targetTabId); | ||
|
|
||
| if (draggedIndex !== -1 && targetIndex !== -1) { | ||
| const [draggedTab] = tabs.splice(draggedIndex, 1); | ||
| tabs.splice(targetIndex, 0, draggedTab); | ||
| appStore.setTabs(tabs); | ||
| } |
There was a problem hiding this comment.
Tab reordering only updates the local frontend state but does not persist the new order to the backend. When loadTabs is called (e.g., after creating/closing tabs or receiving tab updates from the backend), the tab order will revert to the backend order, losing the user's drag-drop reordering. Consider adding a reorderTabs RPC call to sync the new order with the backend tabs array, or track tab order separately from the tabs array.
| filename = `${content.title.replace(/[^a-z0-9]/gi, "-").toLowerCase()}.md`; | ||
| } | ||
| } catch {} | ||
|
|
There was a problem hiding this comment.
The filename generation could produce empty or invalid filenames in edge cases. For example, if pathname consists only of slashes (like "///") and hostname is empty/invalid, pathPart could be empty. Similarly, if title contains only special characters, the sanitized filename could be empty. Consider adding a fallback to ensure a valid filename is always generated, such as defaulting to "download.md" if the generated filename is empty after sanitization.
| // Ensure we always have a valid, non-empty filename | |
| filename = filename.trim(); | |
| if (!filename || filename === ".md") { | |
| filename = "download.md"; | |
| } |
| "history.pushState = function() { origPush.apply(this, arguments); checkUrl(); };", | ||
| "history.replaceState = function() { origReplace.apply(this, arguments); checkUrl(); };", | ||
| "window.addEventListener('popstate', checkUrl);", | ||
| "setInterval(checkUrl, 500);", |
There was a problem hiding this comment.
The SPA URL tracking uses a 500ms polling interval as a fallback to detect URL changes. While this ensures URL changes are caught even if history methods aren't used, it could be wasteful. Consider increasing the interval (e.g., 1000ms or 2000ms) to reduce unnecessary polling, since the primary detection mechanism (history.pushState/replaceState hooks and popstate event) should handle most SPA navigation cases.
| "setInterval(checkUrl, 500);", | |
| "setInterval(checkUrl, 2000);", |
| class="download-btn electrobun-webkit-app-region-no-drag" | ||
| disabled={!appStore.currentContent?.markdown} | ||
| onclick={() => downloadMarkdown()} | ||
| title="Download Markdown" |
There was a problem hiding this comment.
The download button should include an aria-label attribute for better accessibility. The down arrow symbol (↓) might not be clearly announced by screen readers. Consider adding aria-label="Download Markdown" to complement the title attribute.
| title="Download Markdown" | |
| title="Download Markdown" | |
| aria-label="Download Markdown" |
Summary
Test plan