Skip to content

General Improvements: Harden storage, anchoring, imports, and a11y; gate deploys on tests - #5

Merged
akashgoswami merged 2 commits into
mainfrom
fix/codex-review-findings
Oct 4, 2026
Merged

akashgoswami merged 2 commits into
mainfrom
fix/codex-review-findings

Conversation

@akashgoswami

Copy link
Copy Markdown
Contributor

Correctness & data safety:

  • Persist deletions immediately (before the undo toast expires) so a reload in the undo window can no longer resurrect a deleted comment; Undo restores idempotently and only into the page it belongs to.
  • Surface storage failures: store.set reports success, an explicit banner/toast warns when comments cannot be persisted, and unsaved comments stay in memory and remain exportable.
  • Single schema gate for storage/import: reject unknown geometry kinds and comment types, id-less records, malformed anchors/replies, and oversized batches; dedupe within-batch and existing-project IDs.
  • SPA isolation: a non-explicit page key tracks location.pathname and recomputes only on refresh(), so routes keep separate comments.

Anchoring:

  • Refuse to guess: highlight quotes that are lost or ambiguous (even accounting for prefix/suffix context) surface an explicit unanchored pill instead of highlighting an arbitrary copy.
  • Shapes/pins whose anchor element is gone show a distinct "anchor missing" badge instead of silently redrawing over .
  • Coalesced ResizeObserver/scroll relayout re-renders only when an anchor actually moved.

Accessibility:

  • Markers are focusable role=button with aria-labels and open on Enter/Space; add :focus-visible outlines.

Tooling:

  • Add Firefox/WebKit Playwright matrix workflow and gate Pages deploy on the chromium suite; switch demo server to serve on port 4200.
  • Ignore .env and opencode.json.

Correctness & data safety:
- Persist deletions immediately (before the undo toast expires) so a
  reload in the undo window can no longer resurrect a deleted comment;
  Undo restores idempotently and only into the page it belongs to.
- Surface storage failures: store.set reports success, an explicit
  banner/toast warns when comments cannot be persisted, and unsaved
  comments stay in memory and remain exportable.
- Single schema gate for storage/import: reject unknown geometry kinds
  and comment types, id-less records, malformed anchors/replies, and
  oversized batches; dedupe within-batch and existing-project IDs.
- SPA isolation: a non-explicit page key tracks location.pathname and
  recomputes only on refresh(), so routes keep separate comments.

Anchoring:
- Refuse to guess: highlight quotes that are lost or ambiguous (even
  accounting for prefix/suffix context) surface an explicit unanchored
  pill instead of highlighting an arbitrary copy.
- Shapes/pins whose anchor element is gone show a distinct "anchor
  missing" badge instead of silently redrawing over <body>.
- Coalesced ResizeObserver/scroll relayout re-renders only when an
  anchor actually moved.

Accessibility:
- Markers are focusable role=button with aria-labels and open on
  Enter/Space; add :focus-visible outlines.

Tooling:
- Add Firefox/WebKit Playwright matrix workflow and gate Pages deploy on
  the chromium suite; switch demo server to `serve` on port 4200.
- Ignore .env and opencode.json.
… caps

Addresses the codex advisor's review of the hardening commit plus a follow-up
review of the fixes themselves.

Storage:
- Keep an in-memory `unsaved` store and overlay it in pageComments() so work
  that failed to persist (storage denied/quota) survives edits, refresh(),
  storage sync and imports instead of being silently discarded.
- patchComment() falls back to the unsaved copy and re-stages it; removeComment()
  clears it; clear() drops the current page's unsaved records.
- Import dedup builds the existing-ID set from the whole project (all pages),
  not just the current route, so cross-page edits/deletes can't collide.

Validation & import:
- Enforce comment/reply/thread caps at input time (5000/5000/500) and keep the
  import/storage gate in lock-step so native exports always round-trip.
- Validate stored records on load (skip, don't crash on, malformed entries);
  enforce type<->geometry kind consistency and finite rect/circle dimensions.
- isValidAnchor() guards null/missing anchors and uses a generous exact cap so
  legitimate highlights are never hidden.
- Failed import writes keep the imported records in memory and surface the banner.

SPA & anchoring:
- Split load() into syncPageKey() + loadData() so storage events/import/clear no
  longer switch PAGE identity mid-route; only boot/refresh() re-sync the route.
- countOccurrences() counts overlapping matches individually (ana in banana = 2).
- anchorsMoved() treats present->missing as a move; observe anchor elements and
  reconcile observers to avoid leaking detached subtrees.

A11y & polish:
- Highlight marks and the unanchored pill are keyboard-operable (role=button,
  tabindex=0) and open their comment with Enter/Space, moving focus into the card.
- Undo toast is guarded against double-activation; submitReply preserves the
  draft and explains when the thread cap refuses a reply.

Tests: add `_annotatePatchForTest` / `_annotateCountOccurrencesForTest` hooks and
11 regression tests covering the above.
@akashgoswami
akashgoswami merged commit 5880b2a into main Oct 4, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant