Repository navigation
Add reordering, key badges, and syncer - #36
Conversation
Add shortcut reordering (Move left/Move right) with an immutable moveItemInList helper and UI controls + disabled/bounds behavior. Add numeric key badges (1–9, 0) and speed-dial keyboard activation (1–9, 0; Shift opens new tab). Implement createSyncer in storage.js to debounce writes and guard against self-echoed chrome.storage.onChanged events. Update storage exports, wire syncer into newtab.js, adjust rendering to emit badges and move controls, add CSS for badges and dropdown-disabled states, include custom-shortcuts.json, and add unit/integration tests covering debounce, syncer, keyboard nav, and reordering.
Review:Blocker Issue 1 — storage.js:185-220 Required Fix: Store a stable per-install writer ID plus a monotonic revision (or equivalent provenance metadata) with every synchronized state. Suppress only the exact local write identified by that origin and revision. Apply remote states from other origins even if their business payload is byte-identical. Add a two-device regression test for a remote reversion to a prior local state. Issue 2 — storage.js:194-219 Required Fix: Route local writes and remote reconciliation through a single serialized async state machine. Use revisions and an invalidation check so a remote update can prevent an older local operation from committing. Add a test that holds the local sync write pending, delivers a remote update, then resolves the stale write and verifies the remote state remains authoritative. Issue 3 — storage.js:194-206 Required Fix: Make the syncer own the write promise and expose a flush()/completion path for lifecycle boundaries. Record an echo marker only after the sync-layer write succeeds, and propagate Chrome storage failures. Add failure-path coverage for rejected/failed local and sync writes, including an incoming state after a failed local write. |
|
@qodo review this change |
I reviewed the change and don’t see any actionable correctness issues. The implementation correctly:
|
…ed write queue, and error handling - Add stable writerId + monotonic revision for provenance (Issue 1) - Serialize writes through promise chain; remote updates invalidate pending local writes (Issue 2) - Track acknowledged revisions only after successful sync write; propagate errors (Issue 3) - Add regression tests: two-device reversion, flush(), bounded history - Update newtab.js to use async createSyncer and pass meta to onRemote
…initialization in UI
…ge, DOM interaction, and initialization
Review:0 Security vulnerabilities, 5 Potential bugs, 3 Code quality issues **Resolved in commit [fe9899f]**Prevent disabled move buttons from triggering moveShortcut by checking btn.disabled (and/or boundary guards) in handleClick. DescriptionDisabled move buttons can still trigger reordering via event delegation
@@
- } else if (action === 'move-shortcut-left' || action === 'move-shortcut-right') {
+ } else if (action === 'move-shortcut-left' || action === 'move-shortcut-right') {
e.preventDefault();
e.stopPropagation();
closeAllTileMenus();
moveShortcut(groupId, shortcutId, action === 'move-shortcut-left' ? 'left' : 'right');
} else if (action === 'edit-shortcut') {
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/newtab.js Fix saveAll() to use chrome.runtime.lastError instead of a callback error argument for chrome.storage.*.set. DescriptionIncorrect Chrome storage error handling: callbacks don’t receive an error argument
@@
- new Promise(resolve => chrome.storage.local.set(payload, resolve)),
- new Promise(resolve => chrome.storage.sync.set(payload, resolve)),
+ new Promise((resolve, reject) => chrome.storage.local.set(localPayload, err => err ? reject(err) : resolve())),
+ new Promise((resolve, reject) => chrome.storage.sync.set(syncPayload, err => err ? reject(err) : resolve())),
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/storage.js Fix debounce() so earlier returned promises are resolved/rejected when superseded by later calls before the timer fires. Descriptiondebounce() promise bookkeeping can leave earlier callers unresolved when multiple calls happen before timer fires
@@
const debounced = (...args) => {
@@
- // Create a new promise for this debounced call
- pendingPromise = new Promise((resolve, reject) => {
- pendingResolve = resolve;
- pendingReject = reject;
- });
+ // Create a new promise for this debounced call
+ pendingPromise = new Promise((resolve, reject) => {
+ pendingResolve = resolve;
+ pendingReject = reject;
+ });
@@
timer = setTimeout(async () => {
@@
}, ms);
return pendingPromise;
};
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/storage.js Make boot() idempotent (booted flag) to prevent double initialization and duplicate event listeners. Descriptionboot() can run twice in edge cases, duplicating event listeners and causing double-handling
+if (typeof document !== 'undefined' && document.readyState && document.readyState !== 'loading') {
+ boot();
+} else if (typeof document !== 'undefined' && document.addEventListener) {
+ document.addEventListener('DOMContentLoaded', boot);
+}
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/newtab.js Adjust acknowledgedRevisions pruning to evict oldest insertion (not numeric min) to preserve intended echo-suppression window semantics. DescriptionAcknowledged revision pruning removes the minimum value repeatedly, not the oldest, and can mis-prune when revisions are sparse
+ if (acknowledgedRevisions.size > historyLimit) {
+ const minRev = Math.min(...acknowledgedRevisions);
+ acknowledgedRevisions.delete(minRev);
+ }
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/storage.js Add trailing newline at EOF for storage.js and tests/newtab.test.js. DescriptionMissing trailing newline at EOF breaks some tooling and diffs
-export { init, saveAll, onChange, newId, validate, debounce, createSyncer, saveBackground, readBackground, clearBackground, saveBackgroundSize, readBackgroundSize };
+export { init, saveAll, onChange, newId, validate, debounce, createSyncer, saveBackground, readBackground, clearBackground, saveBackgroundSize, readBackgroundSize };
\ No newline at end of file
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/storage.js Move moveItemInList to a small utility module and import it in both newtab.js and tests, avoiding exporting internals from newtab.js. DescriptionExporting
+export { moveItemInList };
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/newtab.js Cache tile elements/hrefs during render and use the cache in handleKeyNavigation instead of querySelectorAll on each keydown. DescriptionKeyboard navigation queries the DOM on every digit keypress
+ const tiles = document.querySelectorAll('.tile[href]');
+ const tile = tiles[idx];
LocationFile: /Users/jkubik/Code/new-tab-shortcuts/newtab.js |
1 participant
Add shortcut reordering (Move left/Move right) with an immutable moveItemInList helper and UI controls + disabled/bounds behavior. Add numeric key badges (1–9, 0) and speed-dial keyboard activation (1–9, 0; Shift opens new tab). Implement createSyncer in storage.js to debounce writes and guard against self-echoed chrome.storage.onChanged events. Update storage exports, wire syncer into newtab.js, adjust rendering to emit badges and move controls, add CSS for badges and dropdown-disabled states, include custom-shortcuts.json, and add unit/integration tests covering debounce, syncer, keyboard nav, and reordering.
Closes: #34, #5