Conversation
Captures decisions, storage schema, sync semantics, and code organization for adding tabs to the extension.
15 bite-sized tasks covering module split, migration, tab CRUD, sync semantics, orphan recovery, overflow, and quota fallback. Each task ends with manual verification steps in Firefox since the project has no automated test framework.
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Scratchpad Firefox extension from a single editor into a multi-tab editor with per-tab content, undo/cursor state, and cross-device sync, while migrating existing single-pad content into a default “Scratchpad” tab.
Changes:
- Introduces a tab model (
tabIndex+ per-tabtab_<id>keys) with CRUD, rename, undo-close, and overflow UI. - Refactors the monolithic scratchpad logic into modules (
storage,editor,migration,tabs,sync) underwindow.Scratchpad. - Adds sync logic for tab list + content, including orphan recovery and a “tab deleted on another device” banner.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
tabs.js |
Implements tab state, editor-per-tab creation, saving, rename/close/undo, shortcuts, overflow menu, and quota fallback. |
sync.js |
Adds storage.onChanged + polling sync logic for tabIndex and tab_<id> entries, plus orphan recovery and deletion banner flow. |
storage.js |
Wraps sync/local storage selection and provides a primary-area change listener + local helper area. |
scratchpad.js |
Rewires lifecycle to initialize modules, hydrate tabs from storage, run orphan recovery, and hook toolbar/refresh/intervals. |
scratchpad.html |
Adds tab bar + editor stack containers and loads new module scripts in order. |
scratchpad.css |
Styles tab bar, overflow menu, editor stack, undo toast, and banners. |
migration.js |
Migrates legacy scratchpadContent into tabbed storage and sweeps partial migrations. |
editor.js |
Centralizes HTML/URL sanitization and editor shortcut behavior. |
manifest.json |
Bumps extension version to 2.0 and updates description. |
README.md |
Updates documentation for multi-tab behavior, shortcuts, and storage layout. |
docs/...tabs-design.md |
Adds design spec for the multi-tab approach. |
docs/...tabs-implementation.md |
Adds detailed implementation plan and manual verification steps. |
Comments suppressed due to low confidence (1)
scratchpad.js:49
updateStatus('error')sets inlinebackgroundColor/color, but non-error statuses never clear those inline styles. After the first error, subsequent "Saving"/"Saved"/"Ready" states can remain red because inline styles override the.status/.status.saving/.status.savedCSS. ResetstatusDiv.style.backgroundColor/statusDiv.style.colorat the start ofupdateStatus, or use a dedicated.status.errorCSS class instead of inline styles.
function updateStatus(status, message = '') {
statusDiv.className = 'status';
switch (status) {
case 'saving':
statusDiv.textContent = 'Saving...';
statusDiv.classList.add('saving');
break;
case 'saved':
const storageType = Scratchpad.Storage.usingSync ? 'Synced' : 'Saved';
statusDiv.textContent = storageType;
statusDiv.classList.add('saved');
setTimeout(() => {
if (statusDiv.textContent === storageType) updateStatus('ready');
}, 2000);
break;
case 'loaded':
case 'ready':
statusDiv.textContent = Scratchpad.Storage.usingSync ? 'Ready (Synced)' : 'Ready (Local)';
break;
case 'error':
statusDiv.textContent = message ? `Error: ${message}` : 'Error';
statusDiv.style.backgroundColor = '#f8d7da';
statusDiv.style.color = '#721c24';
break;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+570
to
+573
| try { | ||
| await Scratchpad.Storage.set({ tabIndex: Tabs._buildIndex() }); | ||
| await Scratchpad.Storage.remove('tab_' + id); | ||
| } catch (e) { |
Comment on lines
+222
to
+245
| const writeKey = 'tab_' + id; | ||
| const useLocal = Tabs._localFallbackTabs.has(id); | ||
| const target = useLocal ? Scratchpad.Storage.Local : Scratchpad.Storage; | ||
|
|
||
| try { | ||
| await target.set({ [writeKey]: current }); | ||
| s.lastSavedContent = current; | ||
| s.contentChanged = false; | ||
| Tabs.onStatus('saved'); | ||
| Tabs.onUpdate(); | ||
| console.log(`[Tabs] ${id}: saved ${current.length} chars (${useLocal ? 'local fallback' : 'sync'})`); | ||
| } catch (e) { | ||
| if (Tabs._isQuotaError(e) && !useLocal) { | ||
| console.warn(`[Tabs] ${id}: quota exceeded; falling back to local storage`); | ||
| Tabs._localFallbackTabs.add(id); | ||
| Tabs._showQuotaBanner(); | ||
| // Retry once with local. | ||
| try { | ||
| await Scratchpad.Storage.Local.set({ [writeKey]: current }); | ||
| s.lastSavedContent = current; | ||
| s.contentChanged = false; | ||
| Tabs.onStatus('saved'); | ||
| Tabs.onUpdate(); | ||
| } catch (e2) { |
Comment on lines
+109
to
+114
| const result = await Scratchpad.Storage.get('tabIndex'); | ||
| const tabIndex = Array.isArray(result.tabIndex) ? result.tabIndex : []; | ||
| const contentKeys = tabIndex.map(t => 'tab_' + t.id); | ||
| const contents = contentKeys.length > 0 ? await Scratchpad.Storage.get(contentKeys) : {}; | ||
| const contentMap = {}; | ||
| for (const t of tabIndex) contentMap[t.id] = contents['tab_' + t.id] || ''; |
Comment on lines
+77
to
+89
| const tabEl = document.createElement('div'); | ||
| tabEl.className = 'tab'; | ||
| tabEl.dataset.tabId = meta.id; | ||
| const labelEl = document.createElement('span'); | ||
| labelEl.className = 'tab-label'; | ||
| labelEl.textContent = meta.name; | ||
| const closeBtn = document.createElement('button'); | ||
| closeBtn.className = 'tab-close'; | ||
| closeBtn.title = 'Close tab'; | ||
| closeBtn.textContent = '×'; | ||
| tabEl.appendChild(labelEl); | ||
| tabEl.appendChild(closeBtn); | ||
| Tabs.tabListEl.appendChild(tabEl); |
Comment on lines
+372
to
+396
| saveBtn.addEventListener('click', async () => { | ||
| banner.remove(); | ||
| // Drop the deleted-tab placeholder from local state BEFORE persisting, | ||
| // so the new tabIndex written to sync storage does not include the | ||
| // deleted id (which would otherwise resurrect on other devices). | ||
| Tabs._removeTabInternal(tabId); | ||
| const newId = Scratchpad.Migration.newTabId(); | ||
| Tabs._addTabInternal({ id: newId, name: tabName }, content); | ||
| try { | ||
| await Tabs._persistIndexAndContent(newId, content); | ||
| Tabs.switchToTab(newId); | ||
| Tabs.onUpdate(); | ||
| } catch (e) { | ||
| console.error('[Tabs] save-as-new-tab failed:', e); | ||
| Tabs._removeTabInternal(newId); | ||
| Tabs.onStatus('error', 'Save as new tab failed'); | ||
| } | ||
| Tabs._refreshOnlyTabClass(); | ||
| }); | ||
|
|
||
| dismissBtn.addEventListener('click', () => { | ||
| banner.remove(); | ||
| Tabs._removeTabInternal(tabId); | ||
| Tabs._refreshOnlyTabClass(); | ||
| }); |
| // Serialize per-tab saves: chain onto any in-flight save so writes for the | ||
| // same tab apply in call order. Without this, two concurrent Storage.set | ||
| // calls have no ordering guarantee, and a stale write could land last. | ||
| s.saveChain = (s.saveChain || Promise.resolve()).then(() => Tabs._saveTabNow(id)); |
Comment on lines
+94
to
+125
| // Capture state about the active tab before applying changes (we may need it for the banner). | ||
| const activeId = Tabs.activeTabId; | ||
| const activeState = activeId ? Tabs.tabs.get(activeId) : null; | ||
| const activeName = activeState?.name; | ||
| const activeContent = activeState ? Scratchpad.Editor.getContent(activeState.editor) : ''; | ||
|
|
||
| // Orphan recovery: if storage has tab_<id> keys not in the new index, append them. | ||
| const merged = await Sync._recoverOrphans(newIndex); | ||
|
|
||
| // Apply additions/removals/renames (the function leaves a removed-active tab in DOM for now). | ||
| const result = Tabs.applyRemoteIndex(merged, contentMap); | ||
|
|
||
| // For added tabs whose content wasn't in the change payload, fetch from storage. | ||
| const missingContent = result.added.filter(meta => !(meta.id in contentMap)); | ||
| if (missingContent.length) { | ||
| const keys = missingContent.map(m => 'tab_' + m.id); | ||
| Scratchpad.Storage.get(keys).then(res => { | ||
| for (const meta of missingContent) { | ||
| if (Scratchpad.Tabs.isLocalFallback(meta.id)) continue; | ||
| const v = res['tab_' + meta.id]; | ||
| if (typeof v === 'string') Scratchpad.Tabs.applyRemoteContent(meta.id, v); | ||
| } | ||
| }).catch(e => console.error('[Sync] backfill content failed:', e)); | ||
| } | ||
|
|
||
| // If the active tab was removed remotely, show the banner. | ||
| if (result.activeRemoved && activeId) { | ||
| // Switch to a neighboring tab first. | ||
| const neighbor = merged[0]?.id; | ||
| if (neighbor) Tabs.switchToTab(neighbor); | ||
| Tabs.showTabDeletedBanner(activeId, activeName, activeContent); | ||
| } |
| }, | ||
|
|
||
| async _restoreClosedTab(meta, content, removedIndex) { | ||
| const state = Tabs._addTabInternal(meta, content); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converts the scratchpad from a single contenteditable into a multi-tab editor. Each tab keeps its own content, undo history, and cursor position; tabs and content sync across devices via Firefox Sync the same way the single pad did. Existing single-pad content migrates seamlessly to a tab named "Scratchpad" on first launch.
The monolithic
scratchpad.jsis split into focused modules sharing a singlewindow.Scratchpadnamespace:flowchart LR HTML[scratchpad.html] --> S[storage.js] HTML --> E[editor.js] HTML --> M[migration.js] HTML --> T[tabs.js] HTML --> SY[sync.js] HTML --> SP[scratchpad.js<br/>lifecycle/wiring] SP --> S SP --> M SP --> T SP --> SY T --> S T --> E T --> M SY --> S SY --> TStorage layout in
browser.storage.sync:tabIndex— ordered[{id, name}]arraytab_<id>— one item per tab's HTML content (stays under the ~8KB per-item sync quota)activeTabIdlives inbrowser.storage.localso each device remembers its own focusHighlights:
…overflow dropdown×close with 5-second Undo toast; cannot close the last tabCtrl+Alt+T(new),Ctrl+Alt+W(close),Ctrl+Tab/Ctrl+Shift+Tab(cycle);Ctrl+B/I/U/Kformatting unchangedstorage.localwhen the sync quota is exceededscratchpadContentleft by a partial-failure migrationManifest version bumped to 2.0. Spec at
docs/superpowers/specs/2026-05-03-tabs-design.md, plan atdocs/superpowers/plans/2026-05-03-tabs-implementation.md.Complexity Notes
A few areas to look at carefully:
tabs.js_saveTab/_saveTabNow) — saves for the same tab serialize vias.saveChainso two concurrentStorage.setcalls cannot land out of order._removeTabInternalsets adisposedflag and cancels the pending timer to prevent a debounced save from resurrecting a just-closed tab.Sync._applyIndexordering — captures the active tab's content before mutation (becauseapplyRemoteIndexmay relocate it), runs orphan recovery, then applies adds/removes/renames. The "Save as new tab?" banner removes the deleted tab placeholder before persisting so the persistedtabIndexdoesn't re-insert it on other devices.Sync.pollOnceand_handleChangesskip per-tab content updates for tabs inTabs._localFallbackTabs. Without this, the next 10s poll would clobber the user's local-fallback content with stale sync data._installShortcutsearly-returns when the focus target is a.tab-rename-input, soCtrl+Alt+WandCtrl+Tabdon't hijack mid-rename.tabIndex. Orphan recovery converts that into eventual consistency for content (tab_<id>keys are never permanently lost), but two simultaneous renames or two simultaneous deletes will still resolve to a single winner.Test Steps
Load the build via
about:debugging#/runtime/this-firefox→ Load Temporary Add-on → selectmanifest.json, then click the toolbar icon to open the scratchpad. Verify:scratchpadContent, it now appears under a tab named "Scratchpad". Fresh installs show one empty "Tab 1". Browser console shows the relevant[Migration]log.+(orCtrl+Alt+T) to create. Double-click a label → edit → Enter / blur / Esc. Click×(orCtrl+Alt+W) → undo within 5s. With one tab open,×is hidden andCtrl+Alt+Wis a no-op.Ctrl+Alt+Wwhile the input is focused — the rename input keeps focus; the tab is NOT closed.Ctrl+Zundoes only within the active tab.Ctrl+B/I/U/Kformatting;Ctrl+TabandCtrl+Shift+Tabcycle (subject to Firefox interception in some configs).⋯button appears; clicking shows hidden tabs; selecting one activates it. Click outside the menu to close.await browser.storage.sync.set({ scratchpadBig: 'x'.repeat(120 * 1024) })), then type into a tab. The yellow "Sync storage full" banner should appear; subsequent typing in that tab continues to save (locally). Clean up withawait browser.storage.sync.remove('scratchpadBig').Checklist
README.mdrefreshed; spec + implementation plan committed underdocs/superpowers/