fix(utils): prefix watch callback strips prefix and filters external events#785
fix(utils): prefix watch callback strips prefix and filters external events#785LeSingh1 wants to merge 1 commit into
Conversation
…events When calling watch() on a prefixStorage, the callback received full storage keys (including the prefix) and was triggered for events on keys outside the namespace. Override watch on the prefixed storage so the callback only fires for matching keys and receives the key relative to the prefix, consistent with how getKeys and getItem work.
📝 WalkthroughWalkthroughThe PR adds watch event support to ChangesWatch support in prefixed storage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/storage.test.ts (1)
278-298: 💤 Low valueConsider adding explicit event type and count assertions.
The test correctly validates prefix-stripping and filtering, but could be more thorough:
- The assertion on line 295 checks for the presence of the expected event but doesn't verify it's the only event with that key.
- The event type ("update") is implicitly checked by
toContainEqual, but an explicit assertion would make the test clearer.- Testing "remove" events in addition to "update" would provide better coverage.
📋 Suggested enhancement
// key delivered to the callback should be relative (no "ns:" prefix) expect(events).toContainEqual({ event: "update", key: "foo" }); + expect(events).toHaveLength(1); + expect(events[0].event).toBe("update"); // events for keys outside the prefix must be filtered out expect(events.every((e) => !e.key.startsWith("other"))).toBe(true);Or add a separate test case for "remove" events:
await pStorage.removeItem("foo"); await new Promise((resolve) => setTimeout(resolve, 10)); expect(events).toContainEqual({ event: "remove", key: "foo" });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/storage.test.ts` around lines 278 - 298, The test for prefixStorage should assert explicit event types and counts: after calling pStorage.setItem("foo", "bar") and waiting, assert that events includes an {event: "update", key: "foo"} and that there is exactly one update event for key "foo" (e.g. filter events by key === "foo" and assert count), then call await pStorage.removeItem("foo"), wait again, and assert events includes an {event: "remove", key: "foo"}; use the existing symbols pStorage.watch, pStorage.setItem, pStorage.removeItem, storage.setItem and the events array to locate and change the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/storage.test.ts`:
- Around line 278-298: The test for prefixStorage should assert explicit event
types and counts: after calling pStorage.setItem("foo", "bar") and waiting,
assert that events includes an {event: "update", key: "foo"} and that there is
exactly one update event for key "foo" (e.g. filter events by key === "foo" and
assert count), then call await pStorage.removeItem("foo"), wait again, and
assert events includes an {event: "remove", key: "foo"}; use the existing
symbols pStorage.watch, pStorage.setItem, pStorage.removeItem, storage.setItem
and the events array to locate and change the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a543675-a4c6-4d42-b245-22efd60a06bd
📒 Files selected for processing (2)
src/utils.tstest/storage.test.ts
1 participant
When you call
watch()on aprefixStorageinstance, two things go wrong today:"ns:foo"instead of"foo"), which is inconsistent with every other method on the prefixed storage.The root cause is that
prefixStoragespreads the underlying storage object and then overrides individual methods, butwatchwas never overridden. Because of this,nsStorage.watchis juststorage.watch-- the full-store watcher with no filtering or key transformation.The fix adds a
watchoverride on the prefixed storage that wraps the caller's callback. The wrapper checks whether each incoming key starts with the base prefix, drops the event if it does not, and strips the prefix from the key before invoking the callback. This makeswatchconsistent with howgetKeys,getItem, and the other prefixed methods already behave.A regression test covers both the key-stripping and the event-filtering behaviour.
Summary by CodeRabbit