feat: enhance callback data handling by supporting URL hash for encrypted data#32
Conversation
📝 WalkthroughWalkthroughAdds a CallbackConfig.useHash option (default true) and updates URL generation, parsing, and watcher logic so encrypted callback data is placed in the URL hash by default or in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
3ed8c9c to
b4ecb6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/__tests__/useSharedCallback.test.ts (1)
7-7: Typeanyis acceptable here for test flexibility.The dynamic import pattern requires this, but consider using the actual type if available:
-let useCallback: any +let useCallback: typeof import('../index')['useCallback']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/__tests__/useSharedCallback.test.ts(15 hunks)src/index.ts(4 hunks)src/types.ts(1 hunks)
🔇 Additional comments (10)
README.md (1)
80-98: Documentation looks good and accurately reflects the implementation.The explanation of the security benefit (preventing sensitive data from being sent in referrers) is helpful context for users. The interface definition matches
src/types.ts.Minor note: Line 85 references
useCallback, but the earlier usage example (lines 21-48) showscreateCallbackStore. Consider clarifying the relationship or updating the example to showuseCallbackusage with the config.src/types.ts (1)
185-193: Type definition is clean and well-documented.The optional
useHashproperty with JSDoc comment clearly communicates the default behavior. The implementation insrc/index.tscorrectly usesconfig.useHash !== falseto treatundefinedastrue.src/index.ts (4)
103-104: Correct default behavior foruseHash.Using
!== falseensures that bothundefinedandtrueresult in hash-based URLs, which matches the documented default.
126-131: Hash vs query parameter switching looks correct.The implementation consistently uses
encodeURIfor both paths, maintaining parity between the two modes.
172-202: Parsing logic is thorough but has a minor edge case consideration.The fallback chain (
searchParamData || hashData) correctly prioritizes query parameter for backward compatibility while supporting hash-based data.Note: Lines 189-194 support parsing
#data=...format in the hash, butsend()andgenerateUrl()only produce raw#encryptedDataformat. This is fine for robustness, but if intentional for future use, consider adding a comment explaining when#data=...in hash would occur.
232-237:generateUrlcorrectly mirrors thesendlogic.Consistent behavior between
send()andgenerateUrl()for hash vs query parameter switching.src/__tests__/useSharedCallback.test.ts (4)
14-38: Dynamic re-import pattern is necessary and correctly implemented.Since
createSharedComposablecaches the composable instance, re-importing the module fresh per test is the right approach to allow different configurations. The chained.then()calls work correctly with vitest's async beforeEach support.
172-190: Good test coverage for query parameter mode.This test verifies the
useHash: falseconfiguration works correctly for thesend()function, ensuring backward compatibility is maintained.
296-314: Good test for hash-based watcher parsing.This complements the existing query-param-based
baseUrltest and ensures the watcher correctly extracts data from URL hash.
535-555: Comprehensive test forgenerateUrlwithuseHash: false.Properly verifies that the query parameter mode works in URL generation and that the data can be successfully parsed back.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32 +/- ##
===========================================
- Coverage 100.00% 98.06% -1.94%
===========================================
Files 2 2
Lines 134 155 +21
Branches 38 47 +9
===========================================
+ Hits 134 152 +18
- Misses 0 3 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/__tests__/useSharedCallback.test.ts (2)
14-49: Refactor redundant window mocking and consider performance impact.The
beforeEachhas two issues:
Redundant window stubbing (lines 17-27): The window is stubbed if undefined, but lines 36-49 immediately re-mock
window.openandwindow.locationwith more detailed implementations, making the initial stub unnecessary.Performance concern (line 31):
vi.resetModules()runs before every test, causing the module to be re-imported fresh each time. While this enables per-test configuration despitecreateSharedComposable, it may significantly slow down the test suite as it grows.Apply this diff to remove redundant window stubbing:
beforeEach(async () => { vi.clearAllMocks() - - // Ensure window exists (JS DOM or stubbed) before importing the module. - if (typeof window === 'undefined') { - vi.stubGlobal('window', { - location: { - href: 'http://test.com/Tools/Update', - toString: () => 'http://test.com/Tools/Update', - replace: vi.fn(), - }, - open: vi.fn(), - }) - } // Re-import useCallback fresh for each test so configuration // (including useHash) can vary per test despite createSharedComposable.Consider grouping tests by configuration to minimize
resetModules()calls. For example, use nesteddescribeblocks foruseHash: true(default) anduseHash: falsetests, resetting modules only when switching between configurations.
301-308: Remove commented-out code.The commented code blocks should be removed to improve readability and maintainability.
Apply this diff:
- // The global URL is already mocked in setup.ts - // const originalURL = global.URL - // global.URL = MockURL as any // Use the mocked URL class const result = callback.watcher({ baseUrl: url.toString() }) - // Restore the original URL constructor - // global.URL = originalURL - expect(result).toEqual(testData)
🧹 Nitpick comments (3)
src/__tests__/useSharedCallback.test.ts (3)
7-8: Consider explicit typing.While
anyis acceptable in test code, consider typinguseCallbackexplicitly asReturnType<typeof import('../index').useCallback>for better type safety and IDE support.
100-102: Extract repeated hash extraction logic.The hash extraction pattern
url.hash.startsWith('#data=') ? url.hash.slice('#data='.length) : ''is repeated throughout the test file. Consider extracting this into a helper function to follow DRY principles.Add a helper function at the top of the test suite:
function extractHashData(url: URL): string { return url.hash.startsWith('#data=') ? url.hash.slice('#data='.length) : '' }Then replace all instances with:
- const encryptedData = url.hash.startsWith('#data=') - ? url.hash.slice('#data='.length) - : '' + const encryptedData = extractHashData(url)Also applies to: 123-125, 152-154, 442-444, 462-464, 485-487, 548-550
642-642: Remove trailing blank line.Minor formatting: remove the extra blank line at the end of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)src/__tests__/useSharedCallback.test.ts(15 hunks)src/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/useSharedCallback.test.ts (5)
src/index.ts (2)
useCallback(248-248)ExternalSignOut(282-282)dist/index.js (2)
useCallback(106-106)useCallback(106-106)dist/index.d.ts (1)
ExternalSignOut(10-10)dist/types.d.ts (1)
ExternalSignOut(75-77)src/types.ts (1)
ExternalSignOut(131-133)
🔇 Additional comments (6)
src/__tests__/useSharedCallback.test.ts (4)
190-207: Good test coverage for backward compatibility.The new tests for
useHash: falsemode provide excellent coverage for the query parameter fallback, ensuring backward compatibility when the hash-based approach is disabled.Also applies to: 560-580
313-331: Good coverage for hash-based baseUrl.This test correctly validates that the watcher can parse data from the URL hash when a baseUrl is provided, aligning with the new hash-based data handling feature.
439-450: Correctly updated expectations for hash-based data.The test expectations have been properly updated to verify that
generateUrlplaces encrypted data in the URL hash by default, aligning with the new feature behavior.Also applies to: 512-514, 525-526, 538-539
636-640: SSR test correctly updated.The server-side rendering test for
generateUrlhas been properly updated to verify hash-based data handling, ensuring the feature works in SSR contexts.package.json (2)
33-34: Restored devDependencies are secure.@vueuse/core@^13.0.0 and rimraf@^6.0.0 have no direct known vulnerabilities. However, consider upgrading rimraf to ^6.1.2 to include transitive dependency fixes (e.g., glob-parent patching).
38-38: pnpm version 10.25.0 is legitimate and recently released.The version was officially released on December 8, 2025, with improvements to per-registry inline certificate support, a new
pnpm init --bareflag, and several patch fixes. Update this version safely. Note that pnpm 10.x has a breaking change: dependency lifecycle scripts are not executed during install by default (a security measure). If any dependencies require install/build scripts, they should be added topnpm.onlyBuiltDependenciesin package.json.
Summary by CodeRabbit
New Features
Documentation
Tests
Chore
✏️ Tip: You can customize this high-level summary in your review settings.