fix(extension-chrome): address all findings from second PR review sweep#20
Merged
Conversation
…ks.get The bare catch silently swallowed every kind of failure including extension-context invalidation, storage I/O errors, and permission issues — exactly the conditions where the propagation path (added by PR #16) needs to surface failure rather than continue with a stale read. Inconsistent with the sibling delete branch which already case-narrows by 'not found' and rethrows other errors. Mirror the delete branch's pattern: silently skip when the message matches 'Can't find bookmark' / 'not found' (expected when the node was deleted locally between mapping and apply); otherwise log and rethrow so runPollRemoteOnce's outer catch records gitmarks:lastError and withholds the etag persist (next poll retries). TDD: added 2 tests — silent-skip-on-not-found + rethrow-on-other. Watched rethrow test fail RED (bare catch swallowed everything). Implementation passes; full suite 85/85 (was 83).
PR #16 added applyRemoteEdit which propagates remote title/URL edits via chrome.bookmarks.update. For URL changes, Chrome's onChanged payload carries the new URL and URL-suppression catches the echo. For TITLE-only changes, Chrome's changeInfo.url is undefined and URL-suppression doesn't see the echo — so the listener pushes a no-op patch back to GitHub, exactly the wasted round-trip class of bug PR #15 was made to fix. Add a node-id-keyed suppression registry alongside the URL-keyed one. applyRemoteEdit suppresses both URLs (catching the URL-change echo and any racing user edit on the old URL) AND the node id (catching the title-only echo). The listener's surviving filter checks both registries for update/remove events. TDD: started with a failing test in listeners.test.ts asserting that an onChanged for a node previously suppressed via suppressNode does NOT trigger client.update. Watched it fail RED with 'suppressNode is not a function'. Added the new API + filter integration. Test passes. Added 5 unit tests for the new suppression API. Full suite 91/91 (was 85).
PollDeps.etag was 'string | null', which admits the empty string ''.
runPollRemoteOnce branches on 'deps.etag ? readIfChanged : read', so an
empty-string etag would silently take the wrong branch — calling
readIfChanged with an invalid header value. The current call site in
background.ts only writes non-empty etags, but the type didn't enforce
that.
Brand Etag via unique symbol; add toEtag(s) that returns null for
empty strings; update PollDeps.etag to 'Etag | null'; switch the
runtime check to '!== null' (since empty string is now impossible
to construct). background.ts pipes the stored value through toEtag.
TDD: added toEtag unit test (empty → null, non-empty → non-null).
Watched it fail RED (toEtag didn't exist). Implementation lifts;
typecheck catches the now-illegal string-literal etags in existing
tests; updated those to toEtag('...'). Full suite 92/92.
…ageWrites
ReconcileDeps.setStorage and PollDeps.setStorage previously accepted
'Record<string, unknown>' — any key, any value, no compile-time guard
against typos.
Extract a shared StorageOps interface with:
- StorageWrites: discriminated union keyed on the three known storage
keys (RECONCILED_AT, LAST_ETAG, LAST_ERROR) with typed payloads
- StorageKey: literal union of the same keys
- LastErrorRecord: shape for gitmarks:lastError that the popup reads
ReconcileDeps and PollDeps now both 'extends StorageOps'. Constants
are re-exported so other modules (listeners.ts) can reuse them
instead of redeclaring the same string literals. popup.ts gains a
proper type for the lastError it displays — previously cast inline
to '{ message: string; source: string; kind?: string }'.
Reviewer's type-design report rated the storage seam 3/5 encapsulation
and 2/5 enforcement; this lifts both to ~5/5: a typo would now be
a compile error, and the LastErrorRecord shape is locked across
writers (listeners.ts flush + background-core.ts poll/reconcile)
and the reader (popup.ts).
Existing tests unaffected; full suite 92/92 still green.
… coverage Test-analyzer findings from the second review sweep: F. The 'propagates a remote URL change' test asserted suppression of the NEW URL but not the OLD URL. The production code suppresses both (the old in case a racing user edit on the old path is in flight) but a future refactor dropping suppress(current.url) would silently pass. Add the second assertion. G. listeners.test.ts had a regression test for onChanged-on-unmapped- node (issue #8 path) but no symmetric test for onRemoved. The same bug class would land silently for removes. Add the analogous test. Full suite 93/93 (was 92).
…onale Reviewer flagged this as a non-obvious WHY: lastReconciledAt is only bumped on success, so a failed reconcile retries on the next cold start rather than waiting a full hour. A future maintainer 'helpfully' moving the stamp into the catch or before the try would silently turn transient GitHub 5xx failures into hour-long sync blackouts. (apply-remote.ts's analogous 'suppress both URLs + node id' comment was already rewritten WHY-first during commit A of this sweep.)
CI hardening: add 'permissions: contents: read' at workflow level so the default GITHUB_TOKEN cannot write to the repo even if a future step or compromised dependency tries to. The workflow only runs tests and builds; it doesn't need any write scope. DRY: BOOKMARKS_PATH was declared as a module-private constant in background-core.ts and reconcile.ts, while bookmarks-file.ts already exported the same value. Both consumers now import it from bookmarks-file.ts (the canonical owner of bookmarks.json-related operations).
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
1 participant
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
Addresses every finding from the second comprehensive 5-agent PR review of the native-tree integration work. 7 focused TDD commits covering 3 real bugs, 1 type refactor, 2 test gaps, 1 comment WHY, and 2 minor cleanups.
Commits (in priority order)
changeInfo.url === undefined, URL-suppression couldn't catch them. Added node-id-keyed suppression registry alongside the URL one. Same bug class as onChanged for an unmapped node still triggers a no-op GitHub round-trip #8 via a different path.etag: string | nulladmitted""which would silently take the wrong branch indeps.etag ? readIfChanged : read. Brand +toEtag(s)helper.Record<string, unknown>with a discriminated union keyed on the three storage keys. SharedStorageOpsinterface. Popup now imports the properLastErrorRecordtype instead of casting inline.runMaybeReconcile: stamp lives in success path so failures retry on next cold start. A future maintainer moving it would cause hour-long silent blackouts on transient 5xx.permissions: contents: readon the CI workflow.BOOKMARKS_PATHwas redeclared in 3 files; now imported frombookmarks-file.ts(canonical owner).TDD discipline
Every behavior change started with a failing test:
Refactors (D, E, minor) had the existing suite as the safety net — full 93/93 stayed green throughout.
Test plan
pnpm test93/93 (was 83 before this sweep — +10 from the 6 new B/A/C/FG tests + existing strengthened)pnpm typecheckcleanpnpm buildcleanOut of scope (filed separately)
Closes #18