refactor: codebase improvements across docs, google, and gql packages#82
refactor: codebase improvements across docs, google, and gql packages#82
Conversation
- Refactor SearchModal (915→167 lines): extract apiData to searchData.ts, keyboard logic to useSearchKeyboard hook, add useCallback memoization - Deduplicate Google Android flavors: move OpenIapViewModel to main source set, extract CommonHelpers.kt with shared suspend functions - Add GQL schema convention linter: validates iOS/Android suffixes, union/future markers before code generation - Add prettier format check to CI/CD for docs package - Align signal pattern to object-based format per docs conventions - Mark unused template-plugin.ts as @deprecated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing code quality, maintainability, and developer experience across multiple packages. It streamlines the documentation search functionality by modularizing its components, reduces code duplication in the Android implementation by centralizing shared logic, and introduces a robust schema linter to enforce GraphQL naming conventions and improve the reliability of code generation. These changes collectively contribute to a more organized, efficient, and error-resistant codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements across the codebase. The refactoring in the docs package, particularly extracting the search data and keyboard logic from SearchModal, greatly improves maintainability and readability. The deduplication of code in the google package by moving shared helpers and the ViewModel to the main source set is also a very positive change. The addition of a schema linter in the gql package is a proactive step towards ensuring schema consistency.
I have found one critical issue related to the refactoring in the google package, where a field required for alternative billing seems to have been dropped. Please see the detailed comment.
packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt
Show resolved
Hide resolved
The developerBillingOption field was dropped during the extraction of AndroidPurchaseArgs from Helpers.kt to CommonHelpers.kt, causing compilePlayDebugKotlin to fail with unresolved reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (38)
📝 WalkthroughWalkthroughThis pull request consolidates documentation search infrastructure, refactors modal keyboard handling into a reusable hook, reorganizes Android purchase helper implementations, removes a ViewModel wrapper, adds formatting checks to CI, and introduces a GraphQL schema linter. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
- Inline keyboard logic back into SearchModal (remove useSearchKeyboard hook) - Revert signals.ts to simple boolean pattern (no object wrapper) - Revert App.tsx signal usage accordingly - Delete empty SharedHelpers.kt (Horizon flavor, comments-only file) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Run prettier --write on 36 files to pass CI format check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/gql/codegen/core/schema-linter.ts (2)
88-93: Consider a more precise union marker pattern.The current check
trimmed.toLowerCase().includes('=> union')could match unintended comments like# This is not => union related. A regex anchored closer to expected marker format would reduce false positives.♻️ Suggested refinement
// Track union marker - if (trimmed.startsWith('#') && trimmed.toLowerCase().includes('=> union')) { + if (/^#\s*=>\s*union\b/i.test(trimmed)) { pendingUnionMarker = true; pendingUnionLine = lineNum; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gql/codegen/core/schema-linter.ts` around lines 88 - 93, Replace the loose includes check for union markers with a stricter anchored regex: instead of using trimmed.toLowerCase().includes('=> union'), test trimmed against a pattern that matches a comment-start followed by optional space and the literal "=> union" (e.g. /^#\s*=>\s*union\b/i) so that only intended markers set pendingUnionMarker and pendingUnionLine (the block using trimmed.startsWith('#'), pendingUnionMarker, pendingUnionLine).
36-41: Usepath.basename()for cross-platform path handling.
filePath.split('/').pop()assumes Unix-style paths. While Node.js often normalizes paths, usingpath.basename()is more robust and idiomatic.♻️ Suggested fix
+import { basename } from 'node:path'; import type { ParsedSchema } from './parser.js';for (const [filePath, sdl] of parsedSchema.sdlContents.entries()) { - const fileName = filePath.split('/').pop() ?? filePath; + const fileName = basename(filePath); const lines = sdl.split(/\r?\n/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gql/codegen/core/schema-linter.ts` around lines 36 - 41, The code currently derives fileName via filePath.split('/').pop(), which is platform-unsafe; update the loop that iterates parsedSchema.sdlContents to use path.basename(filePath) to compute fileName (replace the filePath.split('/').pop() usage) and ensure the Node.js path module is imported (add import path from 'path' or const path = require('path') depending on module style) so isIOSFile/isAndroidFile logic continues to work correctly across platforms.packages/google/openiap/src/horizon/java/dev/hyo/openiap/helpers/SharedHelpers.kt (1)
3-8: Consider deleting this empty placeholder file.After the move it no longer contributes symbols, but the filename still implies shared helper logic lives here. Keeping the breadcrumb in
Helpers.ktor package docs will age better than an always-empty source file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/google/openiap/src/horizon/java/dev/hyo/openiap/helpers/SharedHelpers.kt` around lines 3 - 8, Delete the empty placeholder file SharedHelpers.kt because all helpers were moved to main/helpers/CommonHelpers.kt and Horizon-specific utilities live in Helpers.kt; remove SharedHelpers.kt to avoid misleading filename and leave any explanatory note in Helpers.kt or package-level documentation instead.packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt (1)
57-70: Please pin this mapper down with regression tests.This is now the shared normalization path for both flavors, and the follow-up
developerBillingOptionfix in this PR shows how easy it is to miss one field during future schema changes. A small purchase/subscription matrix test around theandroidfallback and copied fields would pay for itself quickly.Also applies to: 76-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt` around lines 57 - 70, Add regression tests that lock down the AndroidPurchaseArgs normalization/mapper logic by exercising the shared normalization path for both "google" and deprecated "android" flavors: write unit tests that cover a matrix of product types (one-time purchase vs subscription), subscription offers vs subscriptionProductReplacementParams, presence/absence of developerBillingOption, useAlternativeBilling and obfuscatedAccountId/obfuscatedProfileId/offerToken/purchaseToken/replacementMode fields, and verify fields are correctly copied/fallbacked from google to AndroidPurchaseArgs; target the mapper/normalizer code that produces AndroidPurchaseArgs (use the class name AndroidPurchaseArgs and any mapper function used in the same module) so future schema changes will fail tests if a field is missed.packages/docs/src/hooks/useSearchKeyboard.ts (1)
13-19: Document the exported hook contract.This hook now owns global listeners and navigation side effects, so a short JSDoc block at the export site would make that behavior much easier to discover.
📝 Suggested doc block
+/** + * Registers keyboard shortcuts for the docs search modal and returns the + * shared result-selection handler. + */ export function useSearchKeyboard({As per coding guidelines "Add JSDoc comments for public functions and exported APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/hooks/useSearchKeyboard.ts` around lines 13 - 19, Add a concise JSDoc block above the exported function useSearchKeyboard (and referencing UseSearchKeyboardProps) that documents the hook's public contract: list the parameters, describe that it registers global keyboard listeners and causes navigation/selection side effects (including which callbacks it invokes: setSelectedIndex and onClose), and note lifecycle behavior such as cleanup on unmount and when isOpen controls listener registration; keep it short and actionable so callers know it creates global listeners and performs navigation side effects that must be considered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/components/SearchModal.tsx`:
- Around line 12-24: The highlightMatch function is constructing a RegExp from
raw user input (received via handleSearchChange/searchQuery) which breaks on
regex metacharacters; fix it by escaping the query before building the RegExp
(add an escapeRegExp helper that replaces /[.*+?^${}()|[\]\\]/g with an escaped
version) and use the escaped string in new RegExp(`(${escapedQuery})`, 'gi')
inside highlightMatch so user input is treated as a literal string.
In `@packages/docs/src/lib/searchData.ts`:
- Around line 196-202: The dataset contains duplicate ApiItem.id values
("subscription-status-ios") which breaks React keys; find the duplicate entries
in searchData.ts (the ApiItem objects with title 'subscriptionStatusIOS' /
category 'iOS APIs') and make their id values unique (e.g., append a
distinguishing suffix like "-storekit2" or "-v2") and update any references that
consume api.id (e.g., the key used in SearchModal.tsx) so keys match the changed
ids; ensure every ApiItem.id across the dataset is globally unique to avoid
duplicate-key reconciliation issues.
In `@packages/docs/src/lib/signals.ts`:
- Around line 4-13: searchModalSignal was changed to hold an object { isOpen:
boolean } but callers still treat searchModalSignal.value as a boolean; update
the consumer in App.tsx to read and set the nested isOpen property instead of
the whole object: use searchModalSignal.value.isOpen when initializing state
(e.g., const [isSearchOpen, setIsSearchOpen] =
useState(searchModalSignal.value.isOpen)) and when updating state
(setIsSearchOpen(searchModalSignal.value.isOpen)); ensure any other usages that
pass searchModalSignal.value to the SearchModal prop or setState likewise access
.isOpen, and keep the existing openSearchModal and closeSearchModal helpers
(searchModalSignal, openSearchModal, closeSearchModal) unchanged unless you want
to revert the signal shape.
In
`@packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt`:
- Around line 82-95: The code is copying the externally supplied `type` into
AndroidPurchaseArgs, creating two sources of truth; change the constructor calls
that set `type = type` (the AndroidPurchaseArgs instantiation at lines ~82-95
and the similar block at ~106-119) to derive the type from the branch context
instead of the external variable — e.g., set `type = ProductQueryType.INAPP` in
the purchase branch and `type = ProductQueryType.SUBS` in the subscription
branch, or alternatively add a `require(type == expectedType)` before returning
to assert they match; update only the AndroidPurchaseArgs constructions (and any
similar helper calls) to use the branch-determined value or validate equality.
---
Nitpick comments:
In `@packages/docs/src/hooks/useSearchKeyboard.ts`:
- Around line 13-19: Add a concise JSDoc block above the exported function
useSearchKeyboard (and referencing UseSearchKeyboardProps) that documents the
hook's public contract: list the parameters, describe that it registers global
keyboard listeners and causes navigation/selection side effects (including which
callbacks it invokes: setSelectedIndex and onClose), and note lifecycle behavior
such as cleanup on unmount and when isOpen controls listener registration; keep
it short and actionable so callers know it creates global listeners and performs
navigation side effects that must be considered.
In
`@packages/google/openiap/src/horizon/java/dev/hyo/openiap/helpers/SharedHelpers.kt`:
- Around line 3-8: Delete the empty placeholder file SharedHelpers.kt because
all helpers were moved to main/helpers/CommonHelpers.kt and Horizon-specific
utilities live in Helpers.kt; remove SharedHelpers.kt to avoid misleading
filename and leave any explanatory note in Helpers.kt or package-level
documentation instead.
In
`@packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt`:
- Around line 57-70: Add regression tests that lock down the AndroidPurchaseArgs
normalization/mapper logic by exercising the shared normalization path for both
"google" and deprecated "android" flavors: write unit tests that cover a matrix
of product types (one-time purchase vs subscription), subscription offers vs
subscriptionProductReplacementParams, presence/absence of
developerBillingOption, useAlternativeBilling and
obfuscatedAccountId/obfuscatedProfileId/offerToken/purchaseToken/replacementMode
fields, and verify fields are correctly copied/fallbacked from google to
AndroidPurchaseArgs; target the mapper/normalizer code that produces
AndroidPurchaseArgs (use the class name AndroidPurchaseArgs and any mapper
function used in the same module) so future schema changes will fail tests if a
field is missed.
In `@packages/gql/codegen/core/schema-linter.ts`:
- Around line 88-93: Replace the loose includes check for union markers with a
stricter anchored regex: instead of using trimmed.toLowerCase().includes('=>
union'), test trimmed against a pattern that matches a comment-start followed by
optional space and the literal "=> union" (e.g. /^#\s*=>\s*union\b/i) so that
only intended markers set pendingUnionMarker and pendingUnionLine (the block
using trimmed.startsWith('#'), pendingUnionMarker, pendingUnionLine).
- Around line 36-41: The code currently derives fileName via
filePath.split('/').pop(), which is platform-unsafe; update the loop that
iterates parsedSchema.sdlContents to use path.basename(filePath) to compute
fileName (replace the filePath.split('/').pop() usage) and ensure the Node.js
path module is imported (add import path from 'path' or const path =
require('path') depending on module style) so isIOSFile/isAndroidFile logic
continues to work correctly across platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4badd76f-25ab-4c98-867f-f66c7ce1cd82
📒 Files selected for processing (14)
.github/workflows/ci.ymlpackages/docs/src/App.tsxpackages/docs/src/components/SearchModal.tsxpackages/docs/src/hooks/useSearchKeyboard.tspackages/docs/src/lib/searchData.tspackages/docs/src/lib/signals.tspackages/google/openiap/src/horizon/java/dev/hyo/openiap/helpers/SharedHelpers.ktpackages/google/openiap/src/main/java/dev/hyo/openiap/OpenIapViewModel.ktpackages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.ktpackages/google/openiap/src/play/java/dev/hyo/openiap/OpenIapViewModel.ktpackages/google/openiap/src/play/java/dev/hyo/openiap/helpers/Helpers.ktpackages/gql/codegen/core/schema-linter.tspackages/gql/codegen/index.tspackages/gql/codegen/plugins/template-plugin.ts
💤 Files with no reviewable changes (1)
- packages/google/openiap/src/play/java/dev/hyo/openiap/OpenIapViewModel.kt
| AndroidPurchaseArgs( | ||
| skus = params.skus, | ||
| isOfferPersonalized = params.isOfferPersonalized, | ||
| obfuscatedAccountId = params.obfuscatedAccountId, | ||
| obfuscatedProfileId = params.obfuscatedProfileId, | ||
| offerToken = params.offerToken, | ||
| purchaseToken = null, | ||
| replacementMode = null, | ||
| subscriptionOffers = null, | ||
| subscriptionProductReplacementParams = null, | ||
| developerBillingOption = params.developerBillingOption, | ||
| type = type, | ||
| useAlternativeBilling = useAlternativeBilling | ||
| ) |
There was a problem hiding this comment.
Don’t keep two sources of truth for the product type.
Both branches already know whether they are normalizing a purchase or a subscription, but they still copy the externally supplied type unchanged. If those ever drift, this helper can build one branch’s args with the other branch’s ProductQueryType, which can send the purchase flow down the wrong billing path. Please derive type from the branch or require(...) that it matches before returning.
Also applies to: 106-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt`
around lines 82 - 95, The code is copying the externally supplied `type` into
AndroidPurchaseArgs, creating two sources of truth; change the constructor calls
that set `type = type` (the AndroidPurchaseArgs instantiation at lines ~82-95
and the similar block at ~106-119) to derive the type from the branch context
instead of the external variable — e.g., set `type = ProductQueryType.INAPP` in
the purchase branch and `type = ProductQueryType.SUBS` in the subscription
branch, or alternatively add a `require(type == expectedType)` before returning
to assert they match; update only the AndroidPurchaseArgs constructions (and any
similar helper calls) to use the branch-determined value or validate equality.
There was a problem hiding this comment.
This is the same pattern as the original code before the refactoring — type is an external field on RequestPurchaseProps that can be InApp, Subs, or All. The branch (Purchase vs Subscription) determines the request shape, not the product query type. For example, ProductQueryType.All is valid for both branches. Hardcoding the type per branch would break the existing API contract.
No change needed here.
There was a problem hiding this comment.
@hyochan, understood — thanks for the clarification. type on RequestPurchaseProps represents the product query type (InApp, Subs, All) and is orthogonal to the request shape branch (Purchase vs Subscription). Since ProductQueryType.All is a legitimate value for either branch, passing it through directly is the correct pattern. My concern was unfounded. No change needed.
( •ᴥ• ) ʕっ•ᴥ•ʔっ
✏️ Learnings added
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 82
File: packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt:82-95
Timestamp: 2026-03-20T17:02:34.907Z
Learning: In `packages/google/openiap/src/main/java/dev/hyo/openiap/helpers/CommonHelpers.kt` (Kotlin, Android), the `type` field on `RequestPurchaseProps` (`ProductQueryType.InApp`, `ProductQueryType.Subs`, or `ProductQueryType.All`) is independent of the `when` branch (`Request.Purchase` vs `Request.Subscription`). The branch determines the request payload shape, not the product query type. `ProductQueryType.All` is valid for both branches, so `type` must be passed through from the external field rather than hardcoded per branch. Hardcoding it would break the existing API contract.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hyochan
Repo: hyodotdev/openiap PR: 67
File: packages/apple/Sources/Helpers/StoreKitTypesBridge.swift:449-466
Timestamp: 2026-01-18T15:32:05.480Z
Learning: Treat introductoryOfferEligibility as a server-signed JWS string across all platforms. Currently defined as Boolean in packages/apple/Sources/Helpers/StoreKitTypesBridge.swift and in the GraphQL schema, but StoreKit API Product.PurchaseOption.introductoryOfferEligibility(compactJWS:) requires a JWS string. This is a breaking change that requires updates across all platform bindings (TypeScript, Swift, Kotlin, Dart, GDScript) and the GraphQL schema. Plan a coordinated migration: change the type from Boolean to String (JWS), update all schema and client bindings, add validation/migration tests, and document the upgrade path for downstream consumers.
CI checks src/**/*.{ts,tsx,css} but previous commit only formatted
ts/tsx files, missing the CSS glob.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add escapeRegExp helper to prevent crash when typing regex metacharacters (e.g., (, [, +) in the search modal - Fix duplicate 'subscription-status-ios' id in searchData.ts (renamed to 'subscription-status-ios-type' for the Types entry) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Changes
Documentation (packages/docs)
searchData.tswith API data previously inline in SearchModaluseCallbackmemoization to SearchModal handlersAndroid (packages/google)
OpenIapViewModelto main source set (shared across flavors)CommonHelpers.ktwith shared suspend functionsHelpers.ktandSharedHelpers.ktGQL (packages/gql)
schema-linter.tsthat validates iOS/Android suffix conventions, union/future markerstemplate-plugin.tsas@deprecatedCI/CD
Test plan
bun run typecheckpasses (docs)./gradlew :openiap:compilePlayDebugKotlinpasses./gradlew :openiap:compileHorizonDebugKotlinpassesbun run generatepasses (gql)🤖 Generated with Claude Code