-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add approved version check for API Management button on iOS/Android #420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…roid - API Management button now follows same version check logic as pricing routes - On iOS/Android: only shows if app version is approved by server - On desktop/web: always shows (no restrictions) - Fetches products with version to check is_available status - Resolves #419 Co-authored-by: Anthony <AnthonyRonning@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds platform-aware visibility for the API Management menu item in AccountMenu: detects iOS, imports app version from package.json, queries mobile-specific products filtered by version, and sets showApiManagement based on fetched product availability; desktop remains always visible. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/AccountMenu.tsx`:
- Line 54: Replace the relative import of package.json in AccountMenu.tsx with
an alias-based module: create a small module (e.g., export const appVersion in
src/config/appVersion.ts) that exposes the version from a build-time env or
generated constant, then update AccountMenu.tsx to import the version via the
path alias (e.g., import { appVersion } from '@/config/appVersion'); ensure no
direct ../../package.json imports remain and the new module is used wherever
version is needed.
- Around line 159-173: The current showApiManagement IIFE uses isMobile() which
treats mobile web as mobile and hides API Management; change the platform check
to only treat native iOS/Android as restricted so web (including mobile web)
always shows the item. Update the top-level condition in the showApiManagement
logic (the IIFE using isMobile()) to check native platform flags (e.g., isIos()
|| isAndroid() or a dedicated isNativeMobile() helper) so that if the runtime is
not native iOS/Android you return true, and keep the existing
products/product.is_available checks for the native branch.
| import type { TeamStatus } from "@/types/team"; | ||
| import { TeamManagementDialog } from "@/components/team/TeamManagementDialog"; | ||
| import { ApiKeyManagementDialog } from "@/components/apikeys/ApiKeyManagementDialog"; | ||
| import packageJson from "../../package.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace the relative package.json import with an alias-based source.
The new relative import breaks the repo rule for TS/TSX imports and pulls from outside src. Consider exposing the version via a src module (or build-time env) and import it via @/….
♻️ Suggested update
-import packageJson from "../../package.json";
+import { appVersion } from "@/config/appVersion";You can back this with a small module like frontend/src/config/appVersion.ts that reads from a build-time env or a generated constant.
As per coding guidelines, "Use path aliases (@/* maps to ./src/*) for imports in TypeScript/React files".
🤖 Prompt for AI Agents
In `@frontend/src/components/AccountMenu.tsx` at line 54, Replace the relative
import of package.json in AccountMenu.tsx with an alias-based module: create a
small module (e.g., export const appVersion in src/config/appVersion.ts) that
exposes the version from a build-time env or generated constant, then update
AccountMenu.tsx to import the version via the path alias (e.g., import {
appVersion } from '@/config/appVersion'); ensure no direct ../../package.json
imports remain and the new module is used wherever version is needed.
Greptile OverviewGreptile SummaryThis PR changes the sidebar Implementation-wise, it adds a new React Query call to Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant AM as AccountMenu
participant P as platform.ts
participant RQ as React Query
participant BS as BillingService
participant API as Billing API
U->>AM: Open account dropdown
AM->>P: isIOS()
P-->>AM: boolean (throws if not initialized)
alt iOS platform
AM->>RQ: useQuery(products-version-check)
RQ->>BS: getProducts("v<package version>")
BS->>API: fetchProducts(version)
API-->>BS: BillingProduct[]
BS-->>RQ: BillingProduct[]
RQ-->>AM: products
AM->>AM: showApiManagement = products.some(is_available != false)
else non-iOS
AM->>AM: showApiManagement = true
end
AM-->>U: Render API Management item (or hide)
|
|
@claude is this comment valid or not? we should have our existing ways that we handle useQuery and should have been using it for pricing related api calls throughout the pricing page. I care more about our existing standards and correctness and whether or not our approach works, vs whatever this bot is saying. research this in our codebase. ignore with an explanation if it's invalid. or make the fix if it is valid: |
|
Claude finished @AnthonyRonning's task in 3m 42s —— View job Analysis Complete: Bot's Concern is Valid ✓After researching the codebase's React Query patterns, the bot's cache poisoning concern is valid and has been fixed. The ProblemAccountMenu.tsx and pricing.tsx shared the same query key
Why This Causes Cache PoisoningIf the AccountMenu's product fetch fails:
Codebase Pattern AnalysisI found that Marketing.tsx uses a different query key The FixChanged AccountMenu's query key from ✅ Committed and pushed: Changed query key in AccountMenu.tsx SummaryThe bot was correct about the cache poisoning risk. This is a real bug that would cause the pricing page to show incorrectly when navigating from AccountMenu after a failed product fetch. The fix ensures each query maintains its own cache entry, preventing cross-contamination. --- | Branch with fix |
…ing page The bot's concern was valid: AccountMenu and pricing page shared the same query key ["products", isIOSPlatform, isAndroidPlatform] but handled errors differently. AccountMenu caught errors and returned null as successful data, which could poison React Query's cache. When the user navigated to pricing, they'd see no paid plans and no error message. Changed AccountMenu's query key to ["products-version-check", ...] to prevent cache collision. This follows the pattern used by Marketing.tsx which uses a separate ["marketing-products"] key. Co-authored-by: Anthony <AnthonyRonning@users.noreply.github.com>
Deploying maple with
|
| Latest commit: |
9449a88
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://76d8fbd9.maple-ca8.pages.dev |
| Branch Preview URL: | https://claude-issue-419-20260209-21.maple-ca8.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 2 comments
|
Greptile brought up a good point: I told you incorrectly, please follow the same logic as the pricing page. We actually can allow the api management button always, don't need to check version. It's only iOS that has the approved version gate. Also, i see greptile is now complaining about something similar about Please made adjustments for the android gating, and reevaluate the useQuery thing, i thought we fixed the problems with it. |
|
Claude finished @AnthonyRonning's task in 3m 6s —— View job Fixes Complete ✅
SummaryFixed the Android gating issue identified by Greptile. The API Management button now follows the same logic as the pricing page. Changes MadeIssue #1: Android gating incorrect ✅ FIXED Modified
Before: if (!isMobile()) {
return true; // Always show on desktop/web
}
// On mobile, check if version is approved...After: if (!isIOSPlatform) {
return true; // Always show on desktop/web/Android
}
// On iOS, check if version is approved...Issue #2: useQuery error handling ✅ ADDRESSED Greptile's concern about error swallowing is technically valid, but the current behavior is correct for this use case:
This is the appropriate pattern: if we can't verify iOS version approval due to network/API issues, we default to hiding the feature rather than risking showing an unapproved feature. Testing✅ Prettier formatting passed | Branch |
Only iOS should check version approval - Android, desktop, and web should always show the API Management button. Co-authored-by: Anthony <AnthonyRonning@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 3 comments
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/src/components/AccountMenu.tsx
Line: 129:132
Comment:
**Uninitialized platform access**
`isIOS()` throws if `waitForPlatform()` hasn't run yet (`frontend/src/utils/platform.ts:206-211`). Calling it unconditionally during render means `AccountMenu` will hard-crash in any environment where platform init order is off (e.g., storybook/tests/partial renders). Consider using `getPlatformInfo()` in a component that is guaranteed to render only after initialization, or guard with a safe platform value derived from already-initialized state.
How can I resolve this? If you propose a fix, please make it concise. |
Summary
This PR implements the approved version check for the API Management button in the sidebar, following the same logic used for the pricing routes.
Changes
frontend/src/components/AccountMenu.tsxto add version checking logicis_availablestatusTesting
Resolves #419
Generated with Claude Code
Summary by CodeRabbit