Skip to content

PR review sweep 2: address all 5-reviewer findings against d41623c..3a145f6 #18

@paperhurts

Description

@paperhurts

Umbrella issue tracking findings from the second comprehensive multi-agent PR review (covering PRs #12, #14, #15, #16, #17 merged on top of the native-tree integration).

Important (real bugs / regressions)

  • A. Title-only remote echo not suppressed. When apply-remote.ts:applyRemoteEdit propagates a title-only change, chrome.bookmarks.update(nodeId,{title}) fires onChanged with changeInfo.url === undefined. listeners.ts surviving filter sees url == null and lets the event through. client.update() is invoked with empty-patch no-op. Same bug-class as onChanged for an unmapped node still triggers a no-op GitHub round-trip #8 via a different path.
  • B. applyRemoteEdit bare catch on chrome.bookmarks.get. apply-remote.ts:75-81 swallows ALL errors (extension-context invalidation, storage errors, permission issues). Inconsistent with the sibling delete branch which case-narrows by 'not found' and rethrows other errors.
  • C. etag: string | null admits ''. background-core.ts:PollDeps.etag — empty string would take the wrong branch in deps.etag ? readIfChanged : read. Brand Etag or guard at the call site.

Type / code quality

  • D. Record<string, unknown> storage typing too loose. background-core.ts setStorage accepts any key — a typo would land silently. Extract shared StorageOps + typed StorageWrites union + LastErrorRecord type the popup/options can share.
  • E. Missing-WHY comments. background-core.ts:23 (time-since-success vs time-since-attempt) — a future maintainer could helpfully move the stamp into the catch and silently turn failed reconciles into hour-long blackouts. apply-remote.ts:90-91 — explains 'both old and new URL' (mechanics) but not loop-back-prevention (goal).

Test gaps

Suggestions

  • CI workflow missing explicit permissions: contents: read — minor hardening
  • BOOKMARKS_PATH duplicated in background-core.ts, reconcile.ts, bookmarks-file.ts (the last one already exports it) — minor DRY

Out of scope (will file separately)

  • Pre-existing create+update-in-same-batch race within the 500ms debounce window. Not regressed by recent PRs.

Fix plan: one branch fix/review-sweep-2, one PR, multiple focused commits each addressing one finding TDD-style.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions