Conversation
Contributor
WalkthroughAdds subscription schemas and translations, API helpers to open portal/cancel, resolves subscriptions into cosmetics, store UI and CosmeticButton previews, SubscriptionPanel component, AccountModal integration, main flow update, and a test fixture change. ChangesSubscription Management Feature
Sequence DiagramsequenceDiagram
participant AccountModal
participant Api
participant SubscriptionPortal
participant UserMeCache
AccountModal->>Api: fetchCosmetics()
AccountModal->>Api: getUserMe()
AccountModal->>AccountModal: render SubscriptionPanel(sub, cosmetic)
AccountModal->>Api: openSubscriptionPortal(returnUrl)
Api->>SubscriptionPortal: POST /subscriptions/@me/portal
SubscriptionPortal-->>Api: url
Api-->>AccountModal: url | false
AccountModal->>UserMeCache: invalidateUserMe()
AccountModal->>Api: cancelSubscription()
Api->>SubscriptionPortal: POST /subscriptions/@me/cancel
SubscriptionPortal-->>Api: response.ok
Api-->>AccountModal: true | false
AccountModal->>Api: getUserMe() (refresh)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@src/client/Api.ts`:
- Around line 172-221: Both cancelSubscription() and openSubscriptionPortal()
need to handle 401/unauthorized like other authenticated calls: after receiving
the fetch response, if response.status === 401 call logOut() (the existing auth
cleanup function) before returning false, and leave other non-OK handling and
console.error logging as-is; also ensure the catch block still returns false on
network errors. Update the checks inside cancelSubscription and
openSubscriptionPortal to explicitly detect 401 and invoke logOut() to clear
stale auth state.
In `@src/client/components/CosmeticButton.ts`:
- Line 111: The hardcoded "/day" label in the CosmeticButton component must be
localized: replace the literal "/day" text in the JSX (seen in CosmeticButton)
with a call to translateText() using a new key (e.g., "pricing.per_day") and add
the corresponding entry to resources/lang/en.json; ensure both occurrences (the
span at line ~111 and the other at ~118) use translateText("pricing.per_day") so
all user-visible text follows the translateText() pattern.
In `@src/client/components/SubscriptionPanel.ts`:
- Line 109: Replace the hard-coded "/day" text in the SubscriptionPanel
component with a call to translateText() and add a corresponding key in
resources/lang/en.json; specifically, in SubscriptionPanel (the <span
class="text-[10px] ...">"/day"</span> occurrences at lines noted), change the
literal to something like translateText('subscription.perDay') and create an
"subscription.perDay": "/day" entry in en.json so all user-visible text uses
translateText().
In `@src/client/Cosmetics.ts`:
- Around line 349-356: The loop that builds subscription entries always sets
relationship to "purchasable"; change the rel computation to mark subscriptions
as "owned" when the user already has that subscription by checking for the key
`subscription:${subKey}` in the user's ownership set (use the module's existing
ownership collection, e.g., owned/ownedCosmetics/userOwned or ownedKeys).
Concretely, replace the rel assignment (currently `const rel = sub.product ?
"purchasable" : "blocked";`) with logic that first forms the key `const key =
\`subscription:${subKey}\`` then if the ownership set contains that key set rel
= "owned", else if sub.product set rel = "purchasable" else "blocked", and use
that rel in the result.push.
- Around line 69-72: The currency-purchase branch uses a type assertion to treat
c as Pattern|Flag|Pack and passes it to purchaseWithCurrency, but there's no
runtime guard to prevent a Subscription (which lacks priceHard/priceSoft) from
being passed; add a runtime type check before the currency branch: when method
is "hard" or "soft", verify c has priceHard/priceSoft (e.g., check "priceHard"
in c or typeof (c as any).priceHard !== "undefined") and if the check fails,
throw or return an error instead of proceeding; replace the blind cast to priced
with this guarded path and only call purchaseWithCurrency(priced, method) after
the guard passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c2ecd4b-2d4f-4a5e-9e2c-c32c301043e4
📒 Files selected for processing (10)
resources/lang/en.jsonsrc/client/AccountModal.tssrc/client/Api.tssrc/client/Cosmetics.tssrc/client/Store.tssrc/client/components/CosmeticButton.tssrc/client/components/SubscriptionPanel.tssrc/core/ApiSchemas.tssrc/core/CosmeticSchemas.tstests/ResolveCosmetics.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/components/SubscriptionPanel.ts (1)
109-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove
/dayto translations.The literal
/daytext should usetranslateText()with a corresponding key inen.jsonfor proper localization.As per coding guidelines: "
src/client/**/*.{ts,tsx}: All user-visible text must go throughtranslateText()function with corresponding entries inresources/lang/en.json."Also applies to: 116-116
🤖 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 `@src/client/components/SubscriptionPanel.ts` at line 109, Replace the literal "/day" string in SubscriptionPanel.ts with a call to translateText (e.g., translateText('subscription.per_day')) in both places mentioned (the span at the current snippet and the other occurrence), and add the corresponding "subscription.per_day" entry to resources/lang/en.json; ensure you import or use the existing translateText function used across the project and update both occurrences so all user-visible text goes through translation.
🧹 Nitpick comments (1)
src/client/components/SubscriptionPanel.ts (1)
97-101: 💤 Low valueConsider whether
cosmetic.descriptionneeds translation.The description is rendered directly without
translateText()ortranslateCosmetic(). If descriptions are stored pre-translated in the cosmetics schema, this is fine. Otherwise, they should go through the translation system.Verify whether cosmetic descriptions are already localized in the schema or need runtime translation.
🤖 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 `@src/client/components/SubscriptionPanel.ts` around lines 97 - 101, The cosmetic.description is rendered raw in SubscriptionPanel (cosmetic.description) — confirm whether the cosmetics schema stores localized text; if not, pass the description through the translation helper (e.g., translateText() or translateCosmetic()) before rendering; update the ternary branch that outputs ${cosmetic.description} to call the appropriate translator so runtime localization is applied only when needed, and ensure any translator import/usage matches existing translation utilities in the component.
🤖 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.
Duplicate comments:
In `@src/client/components/SubscriptionPanel.ts`:
- Line 109: Replace the literal "/day" string in SubscriptionPanel.ts with a
call to translateText (e.g., translateText('subscription.per_day')) in both
places mentioned (the span at the current snippet and the other occurrence), and
add the corresponding "subscription.per_day" entry to resources/lang/en.json;
ensure you import or use the existing translateText function used across the
project and update both occurrences so all user-visible text goes through
translation.
---
Nitpick comments:
In `@src/client/components/SubscriptionPanel.ts`:
- Around line 97-101: The cosmetic.description is rendered raw in
SubscriptionPanel (cosmetic.description) — confirm whether the cosmetics schema
stores localized text; if not, pass the description through the translation
helper (e.g., translateText() or translateCosmetic()) before rendering; update
the ternary branch that outputs ${cosmetic.description} to call the appropriate
translator so runtime localization is applied only when needed, and ensure any
translator import/usage matches existing translation utilities in the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35ecb351-8f08-4bae-aa1a-9ce98b75a79a
📒 Files selected for processing (11)
resources/lang/en.jsonsrc/client/AccountModal.tssrc/client/Api.tssrc/client/Cosmetics.tssrc/client/Main.tssrc/client/Store.tssrc/client/components/CosmeticButton.tssrc/client/components/SubscriptionPanel.tssrc/core/ApiSchemas.tssrc/core/CosmeticSchemas.tstests/ResolveCosmetics.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/ApiSchemas.ts
- src/client/Api.ts
- src/core/CosmeticSchemas.ts
- resources/lang/en.json
- tests/ResolveCosmetics.test.ts
- src/client/Cosmetics.ts
- src/client/components/CosmeticButton.ts
- src/client/AccountModal.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/AccountModal.ts (1)
40-40: ⚡ Quick winConsider using
@state() decorator for consistency.Other reactive properties in this class (
isLoadingUser) use the@state()decorator. While manualrequestUpdate()calls work, using@state()would make updates automatic and keep the pattern consistent.♻️ Proposed change
+ `@state`() private cosmetics: Cosmetics | null = null; - private cosmetics: Cosmetics | null = null;Then remove the manual
requestUpdate()call on line 434 since it's redundant:void fetchCosmetics().then((cosmetics) => { this.cosmetics = cosmetics; - this.requestUpdate(); });🤖 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 `@src/client/AccountModal.ts` at line 40, Change the manual reactive field declaration "private cosmetics: Cosmetics | null = null;" in the AccountModal class to use the `@state`() decorator (consistent with other reactive properties like email and isLoadingUser) so updates are automatic; then remove the redundant manual requestUpdate() call that updates cosmetics (the requestUpdate() call referenced in the review) since `@state`() will trigger re-rendering for the cosmetics property.
🤖 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.
Inline comments:
In `@src/client/AccountModal.ts`:
- Around line 432-435: The promise from fetchCosmetics() is currently unhandled;
wrap the call so rejections are caught and handled: call fetchCosmetics() and in
the .then set this.cosmetics and requestUpdate(), and add a .catch handler that
logs the error (or calls this.error reporting), sets this.cosmetics to an
appropriate fallback (e.g., empty array or error state), and calls
this.requestUpdate() so the UI renders a non-blocking subscription panel;
reference the existing fetchCosmetics invocation and the this.cosmetics /
this.requestUpdate usage when making the change.
- Around line 228-245: handleCancelSubscription shows a success alert but
doesn’t handle errors from the subsequent getUserMe() call, so the UI may not
reflect the cancelled subscription; update handleCancelSubscription to wrap the
getUserMe() call in try/catch, call invalidateUserMe() before/after fetching,
set this.userMeResponse only on successful fetch, and on failure log the error
and ensure the UI is updated (this.requestUpdate()) so the subscription panel
reflects the cancelled state even if user fetch fails; reference
functions/methods: handleCancelSubscription, cancelSubscription, getUserMe,
invalidateUserMe, this.userMeResponse, this.requestUpdate.
---
Nitpick comments:
In `@src/client/AccountModal.ts`:
- Line 40: Change the manual reactive field declaration "private cosmetics:
Cosmetics | null = null;" in the AccountModal class to use the `@state`()
decorator (consistent with other reactive properties like email and
isLoadingUser) so updates are automatic; then remove the redundant manual
requestUpdate() call that updates cosmetics (the requestUpdate() call referenced
in the review) since `@state`() will trigger re-rendering for the cosmetics
property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bf7a4ef-b616-4f49-b523-38b87d2a4ffe
📒 Files selected for processing (11)
resources/lang/en.jsonsrc/client/AccountModal.tssrc/client/Api.tssrc/client/Cosmetics.tssrc/client/Main.tssrc/client/Store.tssrc/client/components/CosmeticButton.tssrc/client/components/SubscriptionPanel.tssrc/core/ApiSchemas.tssrc/core/CosmeticSchemas.tstests/ResolveCosmetics.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/ResolveCosmetics.test.ts
- src/core/CosmeticSchemas.ts
- src/client/Api.ts
- src/client/components/CosmeticButton.ts
- src/client/Cosmetics.ts
- src/client/Main.ts
- src/client/Store.ts
- src/core/ApiSchemas.ts
- src/client/components/SubscriptionPanel.ts
Comment on lines
+228
to
+245
| private handleCancelSubscription = async (): Promise<void> => { | ||
| const confirmed = window.confirm( | ||
| translateText("account_modal.cancel_subscription_confirm"), | ||
| ); | ||
| if (!confirmed) return; | ||
| const ok = await cancelSubscription(); | ||
| if (!ok) { | ||
| alert(translateText("account_modal.cancel_subscription_failed")); | ||
| return; | ||
| } | ||
| alert(translateText("account_modal.cancel_subscription_success")); | ||
| invalidateUserMe(); | ||
| const userMe = await getUserMe(); | ||
| if (userMe) { | ||
| this.userMeResponse = userMe; | ||
| } | ||
| this.requestUpdate(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Add error handling for getUserMe() after successful cancellation.
If cancelSubscription() succeeds but the subsequent getUserMe() call fails (line 240), the UI won't update to reflect the cancellation. The user will see the success alert but the subscription panel will continue to display.
🛡️ Proposed fix
alert(translateText("account_modal.cancel_subscription_success"));
invalidateUserMe();
- const userMe = await getUserMe();
- if (userMe) {
- this.userMeResponse = userMe;
- }
- this.requestUpdate();
+ try {
+ const userMe = await getUserMe();
+ if (userMe) {
+ this.userMeResponse = userMe;
+ }
+ this.requestUpdate();
+ } catch (err) {
+ console.warn("Failed to refresh user after cancel:", err);
+ this.requestUpdate();
+ }🤖 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 `@src/client/AccountModal.ts` around lines 228 - 245, handleCancelSubscription
shows a success alert but doesn’t handle errors from the subsequent getUserMe()
call, so the UI may not reflect the cancelled subscription; update
handleCancelSubscription to wrap the getUserMe() call in try/catch, call
invalidateUserMe() before/after fetching, set this.userMeResponse only on
successful fetch, and on failure log the error and ensure the UI is updated
(this.requestUpdate()) so the subscription panel reflects the cancelled state
even if user fetch fails; reference functions/methods: handleCancelSubscription,
cancelSubscription, getUserMe, invalidateUserMe, this.userMeResponse,
this.requestUpdate.
Comment on lines
+432
to
+435
| void fetchCosmetics().then((cosmetics) => { | ||
| this.cosmetics = cosmetics; | ||
| this.requestUpdate(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add error handling for fetchCosmetics().
If fetchCosmetics() rejects, the promise is unhandled and this.cosmetics will remain null indefinitely. Users with subscriptions would see an incomplete subscription panel.
🛡️ Proposed fix
- void fetchCosmetics().then((cosmetics) => {
- this.cosmetics = cosmetics;
- this.requestUpdate();
- });
+ void fetchCosmetics()
+ .then((cosmetics) => {
+ this.cosmetics = cosmetics;
+ this.requestUpdate();
+ })
+ .catch((err) => {
+ console.warn("Failed to fetch cosmetics in AccountModal:", err);
+ this.requestUpdate();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void fetchCosmetics().then((cosmetics) => { | |
| this.cosmetics = cosmetics; | |
| this.requestUpdate(); | |
| }); | |
| void fetchCosmetics() | |
| .then((cosmetics) => { | |
| this.cosmetics = cosmetics; | |
| this.requestUpdate(); | |
| }) | |
| .catch((err) => { | |
| console.warn("Failed to fetch cosmetics in AccountModal:", err); | |
| this.requestUpdate(); | |
| }); |
🤖 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 `@src/client/AccountModal.ts` around lines 432 - 435, The promise from
fetchCosmetics() is currently unhandled; wrap the call so rejections are caught
and handled: call fetchCosmetics() and in the .then set this.cosmetics and
requestUpdate(), and add a .catch handler that logs the error (or calls
this.error reporting), sets this.cosmetics to an appropriate fallback (e.g.,
empty array or error state), and calls this.requestUpdate() so the UI renders a
non-blocking subscription panel; reference the existing fetchCosmetics
invocation and the this.cosmetics / this.requestUpdate usage when making the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<cosmetic-button>with description, daily Pu/Caps amounts, and a Stripe checkout button driven by the existingcreateCheckoutSessionflow.<subscription-panel>Lit component (status badge, period-end / cancel-at-period-end, daily currency breakdown).POST /subscriptions/@me/portal).cancelAtPeriodEnd === true) callsPOST /subscriptions/@me/cancelafter aconfirm()prompt, then invalidates the userMe cache and refetches.subscription:*flare alerts "Already subscribed" before opening checkout (upgrade/downgrade flows are out of scope for now).CosmeticsSchema.subscriptions: Record<string, SubscriptionSchema>(optional) insrc/core/CosmeticSchemas.ts.UserMeResponse.player.subscription: { tier, status, currentPeriodEnd, cancelAtPeriodEnd } | nullinsrc/core/ApiSchemas.ts.store.*andaccount_modal.sub_*keys inresources/lang/en.json(English only — Crowdin handles the rest).Discord
evanpelle