refactor(newsletter): migrate the web client onto the SDK newsletter module - #1681
Conversation
…sletter module The web transport in features/newsletter (newsletter-api.ts, author-send-api.ts and the inline sender-standing fetch) now delegates to the SDK request functions, keeping only what is web-specific: fresh-token sourcing via ensureValidToken and the email-token confirm/unsubscribe flows, whose pages exist only on the web origin. Types re-export the SDK shapes under the existing web names and SendRefusedError IS the SDK's refusal error, so instanceof keeps working everywhere. SDK side: the two error classes move to a dependency-free errors.ts so the spec setup can hand out the real classes without pulling the config chain; dist is rebuilt in this commit because web typecheck now needs the new API. Specs stop re-pinning the wire format the SDK's own api.spec.ts already pins and instead pin delegation and rendering, mocking the SDK request functions through the global @ecency/sdk mock. Closes #1680
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
Code Review by Qodo
1.
|
PR Summary by QodoRefactor web newsletter client to delegate to @ecency/sdk module
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed8d07e9c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body: JSON.stringify({ ...input, ...(code ? { code } : {}) }) | ||
| }); | ||
| return parse<SubscribeResult>(res); | ||
| return subscribeDigestRequest(input, code ?? undefined); |
There was a problem hiding this comment.
On any configured web deployment whose hostname is not ecency.com or *.ecency.com (including local development), this delegation changes the request from same-origin /api/newsletter/... to the SDK host. apps/web/src/core/sdk-init.ts lines 29–34 sets that host to https://ecency.com for such browsers, while NewsletterRuntimeProvider enables the feature based on the current deployment's own newsletter credentials. Consequently subscriptions are sent cross-origin to Ecency—typically failing CORS, and in any case bypassing the deployment's configured relay. The SDK call needs a same-origin host override for web newsletter routes; the same issue applies to the migrated list/leave/unsubscribe and sender APIs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 66eaa2b. The SDK config gains a newsletterHost override (undefined falls back to privateApiHost, which is right for mobile; an empty string means same-origin) and the web client pins it to "" in the browser via sdk-init, so newsletter requests stay on the current origin for every deployment: production, localhost dev and custom hostnames alike. SSR never calls the relay, so the override is browser-only. A new SDK spec case pins both the fallback and the same-origin override.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (16)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe newsletter web clients now use ChangesNewsletter SDK migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change centralizes newsletter transport in the shared SDK while retaining web-specific token and confirmation flows; the reported test, typecheck, and lint checks are green, so no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The reviewable changes satisfy the linked issue requirements for SDK delegation, type and error re-exports, local token and email-token handling, sender standing migration, and updated specs. SDK distribution rebuild status cannot be verified because all relevant dist files are excluded by the !/dist/ path filter. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
…ployment Adds a newsletterHost override to the SDK config (undefined = fall back to privateApiHost, right for mobile; '' = same-origin). The web client pins it to '' in the browser, so a configured deployment whose hostname is not *.ecency.com (local dev, a custom domain) keeps talking to its OWN relay instead of following privateApiHost cross-origin to ecency.com. SSR never calls the relay, so the override is browser-only. Dist rebuilt.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
On the Low "Newsletter specs assert SDK calls" rule note: intentional, not an oversight. The SDK request functions are the web layer's OUTBOUND CONTRACT after this migration, exactly as the previous fetch-body assertions were before it; the specs assert what leaves the web layer (input shape plus a freshly ensured token), while user-visible behavior is still asserted alongside in every test. Wire-format pinning moved to the SDK's own api.spec.ts, which is the refactor-stability win the rule aims at. |
1 participant
Closes #1680. Follow-up to #1677: web and mobile now share one newsletter transport implementation.
Web
newsletter-api.tsdelegates subscribe/list/leave/unsubscribe-all to the SDK request functions. What stays is web-specific: fresh-token sourcing viaensureValidTokenand the email-token confirm/unsubscribe flows, whose pages exist only on the web origin.author-send-api.tsdelegates preview/send/candidates/issues;SendRefusedErroris a re-export of the SDK'sNewsletterSendRefusedError, soinstanceofkeeps working everywhere.useSenderStandingusesgetNewsletterSenderRequestinstead of its inline fetch;SenderStandingaliases the SDK shape.types.tsre-exports the SDK types under the existing web names.*.ecency.combrowser the SDK host is empty, so requests stay same-origin relative exactly as before.SDK
errors.ts(public surface unchanged via the module barrel) so the spec setup can hand out the real classes without pulling the config chain.Specs
api.spec.tsalready pins. They now mock the SDK request functions (exposed as stubs by the global@ecency/sdkmock, error classes real) and pin what web owns: delegation with a fresh token, cache behavior and rendering.author-send-api.specalso pins that the re-exported error class IS the SDK class.Verification: SDK 854 tests, web full suite 361 files / 3583 tests, workspace typecheck and lint all green.
Summary by CodeRabbit