feat!: introduce client and server entrypoints for callback handling #37
Conversation
📝 WalkthroughWalkthroughReplaces the previous shared composable callback store with two factories: Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client App
participant ClientFactory as createCallback (client)
participant Core as core (encrypt/append)
participant Browser as Browser (window)
participant Receiver as Receiver / Server
participant ServerFactory as createServerCallback (server)
participant ServerCore as core (decrypt/parse)
App->>ClientFactory: call send(payload, url, redirectType, sendType, sender)
ClientFactory->>Core: stringifyPayload(payload) -> encryptData(...)
Core-->>ClientFactory: encryptedData
ClientFactory->>Core: appendEncryptedDataToUrl(url, encryptedData, useHash)
Core-->>ClientFactory: urlWithData
ClientFactory->>Browser: navigate/open urlWithData (or return URL)
Browser->>Receiver: request contains encrypted data (hash or query)
Receiver->>ServerFactory: (server) parse(encryptedData)
ServerFactory->>ServerCore: parseEncryptedPayload(encryptedData, encryptionKey)
ServerCore-->>ServerFactory: QueryPayloads (actions, sender, type)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
elibosley
force-pushed
the
feat/simplify-lib
branch
from
4f49e91 to
b16289e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 97.75% 99.08% +1.32%
==========================================
Files 1 3 +2
Lines 89 109 +20
Branches 28 30 +2
==========================================
+ Hits 87 108 +21
+ Misses 2 1 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added `createCallback` for client-side usage, allowing encrypted callback data to be sent via URL hash or query parameters. - Introduced `createServerCallback` for server-side usage, ensuring safe parsing and URL generation without browser dependencies. - Updated `package.json` to include new exports for client and server entrypoints. - Enhanced README with usage examples for both client and server implementations. - Refactored build scripts to support type generation and JavaScript bundling. - Updated dependencies and lock files to ensure compatibility with the new structure.
elibosley
force-pushed
the
feat/simplify-lib
branch
from
b16289e to
7e37ddd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
56-73: These “Store Interface” sections look stale vs the new factory API—either update or remove to avoid contradicting the examples.
🧹 Nitpick comments (7)
tsup.config.ts (1)
3-18: Be explicit about whethercrypto-jsshould be bundled vs externalized.
noExternal: ["crypto-js"]forces bundling and can increase output size / duplicate dependency copies across consumers; if the intent is “leavecrypto-jsas a dependency”, preferexternal: ["crypto-js"]instead (or drop both and rely on tsup defaults).export default defineConfig({ entry: { index: "src/index.ts", client: "src/client.ts", server: "src/server.ts", }, format: ["esm"], target: "es2020", platform: "neutral", splitting: false, sourcemap: false, clean: false, dts: false, treeshake: true, - noExternal: ["crypto-js"], + // Pick one strategy: + // external: ["crypto-js"], // keep dependency external + // noExternal: ["crypto-js"], // bundle dependency });package.json (1)
4-7: Repository URL: consider adding the.gitsuffix for maximum registry/tooling compatibility."repository": { "type": "git", - "url": "git+https://github.com/unraid/shared-callbacks" + "url": "git+https://github.com/unraid/shared-callbacks.git" },src/__tests__/server.test.ts (1)
1-33: Enforce a Node test environment here to actually validate “no window” assumptions.Consider adding a file directive (or a suite-level config) so this test can’t accidentally run under
jsdom:+// @vitest-environment node import { describe, it, expect } from 'vitest'src/index.ts (1)
1-1: Considerexport * from "./client.js"for NodeNext-style ESM consistency (if that’s the repo standard).-export * from "./client"; +export * from "./client.js";src/core.ts (1)
39-49: Consider removing unnecessary array spread.The spread operator
[...payload]creates a shallow copy before JSON serialization, which is unnecessary sinceJSON.stringifydoesn't mutate the input.Apply this diff to simplify:
export const stringifyPayload = ( payload: SendPayloads, sender: string, sendType?: string ): string => { return JSON.stringify({ - actions: [...payload], + actions: payload, sender, type: sendType, }); };src/client.ts (2)
62-63: Consider making path transformation configurable.The hardcoded replacement of
"/Tools/Update"with"/Tools"(lines 63, 73) couples the library to a specific URL structure. If this logic is specific to the current use case, consider making it configurable via theCallbackConfigto improve flexibility.For example:
interface CallbackConfig { encryptionKey: string; useHash?: boolean; pathTransform?: (path: string) => string; // Optional path transformer } // Usage: const defaultSender = sender ?? (config.pathTransform ? config.pathTransform(window.location.href) : window.location.href);Also applies to: 72-74
100-150: Consider extracting URL data extraction logic.The watcher function contains complex nested logic (lines 117-143) that could be extracted into a separate helper function for improved readability and testability.
However, the logic is correct, including the proper handling of URI decoding before parsing.
Optional refactor example:
const extractEncryptedDataFromUrl = (urlString: string): string => { try { const currentUrl = new URL(urlString); const searchParamData = currentUrl.searchParams.get("data") ?? ""; let hashData = ""; const rawHash = currentUrl.hash ?? ""; if (rawHash) { const hashWithoutHash = rawHash.startsWith("#") ? rawHash.slice(1) : rawHash; if (hashWithoutHash.startsWith("data=")) { hashData = hashWithoutHash.slice("data=".length); } } return decodeURI(searchParamData || hashData); } catch { return ""; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
dist/client.d.tsis excluded by!**/dist/**dist/client.jsis excluded by!**/dist/**dist/core.d.tsis excluded by!**/dist/**dist/index.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/server.d.tsis excluded by!**/dist/**dist/server.jsis excluded by!**/dist/**dist/types.d.tsis excluded by!**/dist/**dist/types.jsis excluded by!**/dist/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
README.md(4 hunks)package.json(2 hunks)src/__tests__/server.test.ts(1 hunks)src/__tests__/useSharedCallback.test.ts(2 hunks)src/client.ts(1 hunks)src/core.ts(1 hunks)src/index.ts(1 hunks)src/server.ts(1 hunks)tsup.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/__tests__/useSharedCallback.test.ts (1)
src/client.ts (1)
useCallback(186-186)
src/core.ts (2)
dist/index.js (5)
decryptedMessage(2039-2039)decryptedString(2040-2040)decryptedString(2064-2064)dataToParse(2063-2063)decryptedData(2066-2066)dist/types.d.ts (2)
SendPayloads(96-96)QueryPayloads(107-107)
src/__tests__/server.test.ts (6)
src/server.ts (2)
createServerCallback(53-83)ExternalSignOut(116-116)src/client.ts (1)
ExternalSignOut(220-220)dist/server.d.ts (1)
ExternalSignOut(14-14)dist/types.d.ts (1)
ExternalSignOut(75-77)dist/index.js (2)
generateUrl(2149-2158)parse(2114-2116)dist/server.js (2)
generateUrl(2087-2097)parse(2084-2086)
src/server.ts (2)
dist/types.d.ts (3)
CallbackConfig(113-121)QueryPayloads(107-107)SendPayloads(96-96)src/core.ts (3)
parseEncryptedPayload(67-84)createEncryptedPayload(54-62)appendEncryptedDataToUrl(89-103)
src/client.ts (2)
dist/types.d.ts (4)
CallbackConfig(113-121)SendPayloads(96-96)QueryPayloads(107-107)WatcherOptions(108-112)src/core.ts (3)
createEncryptedPayload(54-62)parseEncryptedPayload(67-84)appendEncryptedDataToUrl(89-103)
🔇 Additional comments (13)
src/__tests__/useSharedCallback.test.ts (1)
28-33: Nice coverage additions + clearer module reset intent.Also applies to: 51-63
src/server.ts (1)
47-83:createServerCallbackshape/behavior looks solid (no browser globals + consistent hashing behavior).package.json (1)
15-28: Configuration is correct; no action needed.The
tsconfig.jsonalready setsoutDir: "dist"anddeclaration: true, and includessrc/with proper exclusions for tests. The source filessrc/index.ts,src/client.ts, andsrc/server.tswill emit asdist/index.d.ts,dist/client.d.ts, anddist/server.d.tsrespectively, matching the export map exactly. The build scripts are properly ordered withprebuild: pnpm cleanbefore the main build.src/core.ts (5)
1-10: LGTM!The encryption function is a clean wrapper around crypto-js AES encryption with appropriate type signatures.
16-34: LGTM!Excellent error handling for decryption failures and invalid UTF-8 results. The dual checks (try-catch + empty string validation) provide robust protection against malformed input.
54-62: LGTM!Clean functional composition that properly separates stringification from encryption.
67-84: LGTM!The function properly handles optional URI decoding and provides appropriate error handling for JSON parse failures. The separation of decryption and parsing concerns is well-structured.
89-103: LGTM!The URL constructor is available in Node.js (v10+) and browsers, making this suitable for both client and server contexts. The symmetric use of
encodeURIhere anddecodeURIin parsing functions ensures correct round-trip encoding.src/client.ts (5)
48-49: LGTM!Clean factory pattern with appropriate default handling for the
useHashconfiguration option.
93-98: LGTM!Clean wrapper around the core parsing function with appropriate parameter forwarding.
152-172: LGTM!Excellent implementation that correctly uses the
appendEncryptedDataToUrlhelper function. The conditional window check (lines 158-162) allows this function to work in both client and server environments when asenderis provided.
182-186: LGTM!Good backwards compatibility strategy with clear documentation about the behavioral change from shared singleton to factory function.
189-229: LGTM!Comprehensive type re-exports provide excellent developer experience by making the client entry point self-contained for both runtime and type imports.
| ### Server / client entrypoints | ||
|
|
||
| To make SSR / Workers usage explicit and safe, the package also exposes split entrypoints: | ||
|
|
||
| - `@unraid/shared-callbacks/client` – exports `createCallback` (send, watcher, parse, generateUrl) and all types. This is intended for browser/client-only code. | ||
| - `@unraid/shared-callbacks/client` – exports `createCallback` (send, watcher, parse, generateUrl) and all types. This is intended for browser/client-only code. | ||
| - `@unraid/shared-callbacks/server` – exports `createServerCallback`, which exposes only `parse` and `generateUrl` and never touches browser globals. | ||
|
|
||
| Example server usage: | ||
|
|
||
| ```ts | ||
| import { | ||
| createServerCallback, | ||
| type CallbackConfig, | ||
| } from '@unraid/shared-callbacks/server'; | ||
|
|
||
| const config: CallbackConfig = { | ||
| encryptionKey: 'your-encryption-key', | ||
| }; | ||
|
|
||
| const { parse, generateUrl } = createServerCallback(config); | ||
| ``` |
There was a problem hiding this comment.
Fix duplicated client entry + mismatched code fences (markdown rendering).
- `@unraid/shared-callbacks/client` – exports `createCallback` (send, watcher, parse, generateUrl) and all types. This is intended for browser/client-only code.
-- `@unraid/shared-callbacks/client` – exports `createCallback` (send, watcher, parse, generateUrl) and all types. This is intended for browser/client-only code.
- `@unraid/shared-callbacks/server` – exports `createServerCallback`, which exposes only `parse` and `generateUrl` and never touches browser globals.
@@
const { parse, generateUrl } = createServerCallback(config);
-```
+```Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In README.md around lines 94 to 115, there's a duplicated client entry and a
markdown code-fence issue: remove the second duplicate "-
`@unraid/shared-callbacks/client`" and replace it with the correct server entry
(if needed ensure the list contains exactly one client and one server entry with
correct descriptions), and fix the code fence by ensuring the example block uses
matching triple backticks with "ts" after the opening fence (```ts) and a
closing ``` at the end so the snippet renders properly.
- Added tests to verify default sender behavior when not provided. - Implemented tests for handling hashes without the '#' prefix and without 'data=' prefix. - Included tests for scenarios with no hash and null/undefined hash values. - Ensured comprehensive coverage for the callback's URL parsing logic.
elibosley
changed the title
- Replaced manual URL construction with appendEncryptedDataToUrl for better clarity and maintainability. - Removed unnecessary conditional logic for setting URL hash and search parameters. - Updated README to reflect the correct client entrypoint for callback handling. - Ensured consistent usage of the new URL handling method across client and server implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/__tests__/useSharedCallback.test.ts (1)
339-415: Consider simplifying or extracting MockURL implementations.The manual
MockURLimplementations for testing edge cases (hash without#prefix, hash withoutdata=prefix) are comprehensive but complex. While they successfully test important branches, they manually parse URLs which could be fragile.Consider extracting these mocks to a shared test helper or exploring whether the native
URLconstructor can be used with appropriate test data setup instead of full mocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/useSharedCallback.test.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/useSharedCallback.test.ts (4)
src/client.ts (2)
useCallback(186-186)ExternalSignOut(220-220)dist/types.d.ts (1)
ExternalSignOut(75-77)src/types.ts (1)
ExternalSignOut(131-133)dist/client.js (1)
decryptedData(2066-2066)
🔇 Additional comments (4)
src/__tests__/useSharedCallback.test.ts (4)
29-63: LGTM! Proper transition to factory pattern.The updated comment, test rename, and new distinct-instance test correctly reflect the shift from a shared singleton to a per-call factory pattern. The identity check on line 62 appropriately verifies that each invocation returns a new instance.
191-213: LGTM! Properly tests default sender resolution.The test correctly verifies that when the sender parameter is omitted, it defaults to
window.location.hrefwith normalization applied (from/Tools/Updateto/Tools). This aligns with the client-side default behavior described in the PR objectives.
560-714: LGTM! Comprehensive coverage of generateUrl functionality.The tests thoroughly verify
generateUrlbehavior across multiple scenarios:
- Default sender resolution (client vs server)
- Various URL formats and edge cases
- Query parameter preservation
- Query vs hash mode
- Empty payload handling
The distinction between
generateUrl(no normalization) andsend(normalizes/Tools/Updateto/Tools) is correctly documented on line 669.
716-774: LGTM! Proper SSR compatibility verification.The tests correctly verify that:
send()throws an appropriate error on the server sideparse()andgenerateUrl()work in SSR contextsThis aligns with the PR objective of providing server-safe callback handling without browser dependencies.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server.ts (1)
86-125: Consider using wildcard type re-export for maintainability.The individual type re-exports are explicit and clear, but could be simplified with a wildcard re-export. If all types from
types.tsare intended to be public, consider:export type * from "./types.js";This reduces duplication and automatically keeps the server entry in sync with type additions. However, the current explicit approach provides tighter control over the public API surface, so this is an optional optimization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/client.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
README.md(4 hunks)src/client.ts(1 hunks)src/server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/server.ts (2)
dist/server.js (2)
parse(2084-2086)generateUrl(2087-2097)src/core.ts (3)
parseEncryptedPayload(67-84)createEncryptedPayload(54-62)appendEncryptedDataToUrl(89-103)
🔇 Additional comments (4)
README.md (2)
21-43: LGTM! Clear and accurate usage examples.The updated examples correctly demonstrate the new
createCallbackAPI with proper configuration and usage patterns for both sending and watching callbacks.
94-114: LGTM! Server/client entrypoints are well-documented.The split entrypoints are clearly explained with accurate descriptions and a helpful server usage example. The past issues regarding duplicate entries and code fence formatting appear to have been resolved.
src/server.ts (2)
1-45: LGTM! Import specifiers are now consistent.All imports use explicit
.jsextensions consistently. The past issue regarding inconsistent import specifiers has been resolved.
53-83: LGTM! Clean server-safe implementation.The factory correctly:
- Delegates parsing to
parseEncryptedPayloadwith proper config- Normalizes
senderto empty string (Line 67) to satisfy thecreateEncryptedPayloadsignature- Defaults
useHashtotruewhen not explicitly set tofalse(Line 75), matching documented behavior
createCallbackfor client-side usage, allowing encrypted callback data to be sent via URL hash or query parameters.createServerCallbackfor server-side usage, ensuring safe parsing and URL generation without browser dependencies.package.jsonto include new exports for client and server entrypoints.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.