feat: User preferences storage & admin: sorted billing plans by populality #3266
Conversation
WalkthroughAdds per-user JSON storage to UserPreferences with GET/PUT endpoints, DTO, service logic, DB migration, and tests. Webapp adds a hook to consume these endpoints and updates plan selectors: GenericPlanSelector gains a loading prop, sorts by user-preferred counts, and increments preferences on selection; wrapper selectors now derive loading internally. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Webapp Hook / UI
participant API as UserPreferencesController
participant SVC as UserPreferencesService
participant REPO as UserPreferencesRepository
participant DB as DB (user_preferences.storage_json)
rect rgb(240,248,255)
note left of UI: Read field
UI->>API: GET /v2/user-preferences/storage/{fieldName}
API->>SVC: findOrCreate(user)
SVC->>REPO: fetch prefs
REPO-->>SVC: UserPreferences(storageJson)
SVC-->>API: value = storageJson[fieldName] or null
API-->>UI: 200 UserStorageResponse{ data }
end
rect rgb(245,255,250)
note left of UI: Write field
UI->>API: PUT /v2/user-preferences/storage/{fieldName} { data }
API->>SVC: setStorageJsonField(fieldName, data, user)
SVC->>REPO: fetch or create prefs
SVC->>DB: update storage_json (set or remove key)
DB-->>SVC: persisted
SVC-->>API: done
API-->>UI: 200
end
sequenceDiagram
autonumber
participant User as Admin UI
participant GPS as GenericPlanSelector
participant Pref as useUserPreferenceStorage
participant API as /v2/user-preferences/storage/{fieldName}
participant Sort as useSortPlans
User->>GPS: Open selector
GPS->>Pref: GET preferred counts
Pref->>API: GET storage/billingAdminPreferredPlans
API-->>Pref: { data: { planId: count, ... } }
Pref-->>GPS: loadable with data
GPS->>Sort: sort(plans, counts)
Sort-->>GPS: sorted plans
User->>GPS: Select plan X
GPS->>Pref: update(increment count for X)
Pref->>API: PUT storage/billingAdminPreferredPlans { data: updatedCounts }
API-->>Pref: 200
GPS-->>User: onPlanChange(selected plan)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/genericFields/GenericPlanSelector.tsx (1)
webapp/src/hooks/useUserPreferenceStorage.ts (1)
🪛 Biome (2.1.2)webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/genericFields/GenericPlanSelector.tsx[error] 51-51: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. Hooks should not be called after an early return. For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. (lint/correctness/useHookAtTopLevel) [error] 52-52: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. Hooks should not be called after an early return. For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. (lint/correctness/useHookAtTopLevel) [error] 86-86: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render. This is the call path until the hook. Hooks should not be called after an early return. For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. (lint/correctness/useHookAtTopLevel) [error] 88-88: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render. This is the call path until the hook. Hooks should not be called after an early return. For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. (lint/correctness/useHookAtTopLevel) [error] 103-103: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render. This is the call path until the hook. Hooks should not be called after an early return. For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. (lint/correctness/useHookAtTopLevel) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Comment |
![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 (7)
webapp/src/service/apiSchema.generated.ts (1)
5975-5978: Consider makingUserStorageResponse.datarequired.200-success implies the field exists; optional
datablurs “missing” vs “explicit null”. Preferdata: unknownand use 404 for missing.Apply at the OpenAPI source so generation yields:
- UserStorageResponse: { - /** @description The data stored for the field */ - data?: unknown; - }; + UserStorageResponse: { + /** @description The data stored for the field */ + data: unknown; + };backend/data/src/main/kotlin/io/tolgee/model/UserPreferences.kt (1)
41-47: Make column mapping explicit and prefer null default.Avoid relying on naming strategies; map explicitly and default to null to prevent writing
{}unintentionally.- @Type(JsonBinaryType::class) - @Column(columnDefinition = "jsonb") - var storageJson: Map<String, Any>? = mutableMapOf() + @Type(JsonBinaryType::class) + @Column(name = "storage_json", columnDefinition = "jsonb") + var storageJson: Map<String, Any>? = nullbackend/data/src/main/kotlin/io/tolgee/dtos/request/UserStorageResponse.kt (1)
1-10: Move DTO to a response package for clarity.This is a response DTO but lives under dtos.request. Consider relocating.
-package io.tolgee.dtos.request +package io.tolgee.dtos.responseFollow-up: update imports (controller, any generated schema references).
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/UserPreferencesController.kt (2)
65-71: Response semantics for missing field.Currently returns
{ data: null }when the key is absent. If desired, consider 404 or 204 to signal absence more explicitly; otherwise OK as-is.
73-84: Consider 204 No Content on update.PUT currently returns 200 with empty body. Returning 204 would better reflect no response content.
webapp/src/hooks/useUserPreferenceStorage.ts (1)
4-27: Generalize typing and auto-invalidate/refetch after update.Current cast fixes the type to Record<number, number>, which breaks reuse. Also, mutation doesn’t invalidate/refetch, so UI may stay stale.
-export function useUserPreferenceStorage(fieldName: string) { +export function useUserPreferenceStorage<T = unknown>(fieldName: string) { const loadable = useApiQuery({ url: '/v2/user-preferences/storage/{fieldName}', method: 'get', path: { fieldName }, - }) as UseQueryResult<{ data: Record<number, number> }>; + }) as UseQueryResult<{ data: T }>; - const mutation = useApiMutation({ + const mutation = useApiMutation({ url: '/v2/user-preferences/storage/{fieldName}', method: 'put', + invalidatePrefix: '/v2/user-preferences/storage', + options: { + onSuccess: () => loadable.refetch(), + }, }); return { loadable, - update: (value: Record<string, any>) => { + update: (value: T) => { mutation.mutate({ path: { fieldName }, content: { 'application/json': value, }, }); }, }; }Note: For your current use case, set
T = Record<string, number>.backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserPreferencesControllerTest.kt (1)
47-150: Good test coverage, but consider adding edge case tests.The test suite provides solid coverage for the storage JSON functionality, including basic operations, field isolation, overwrites, and different data types. However, consider adding tests for the following edge cases to improve robustness:
- Null value handling: Test storing and retrieving explicit null values vs. non-existent fields.
- Field name validation: Test special characters, very long field names, empty field names, or field names with path-like patterns (e.g.,
field/subfield).- Large payload handling: Test storing large values to ensure the JSON storage can handle them without performance degradation.
- Concurrent updates: Test race conditions when multiple requests update the same field simultaneously.
- Invalid data types: Test edge cases like extremely large numbers, deeply nested objects, or circular references.
Would you like me to generate sample tests for these edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/UserPreferencesController.kt(3 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserPreferencesControllerTest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/UserStorageResponse.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/UserPreferences.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/security/UserPreferencesService.kt(1 hunks)backend/data/src/main/resources/db/changelog/schema.xml(1 hunks)webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/cloud/fields/CloudPlanSelector.tsx(2 hunks)webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/genericFields/GenericPlanSelector.tsx(5 hunks)webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/selfHostedEe/fields/SelfHostedEePlanSelector.tsx(2 hunks)webapp/src/hooks/useUserPreferenceStorage.ts(1 hunks)webapp/src/service/apiSchema.generated.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/genericFields/GenericPlanSelector.tsx (1)
webapp/src/hooks/useUserPreferenceStorage.ts (1)
useUserPreferenceStorage(4-27)
webapp/src/hooks/useUserPreferenceStorage.ts (1)
webapp/src/service/http/useQueryApi.ts (2)
useApiQuery(119-141)useApiMutation(224-265)
webapp/src/service/apiSchema.generated.ts (1)
webapp/src/service/billingApiSchema.generated.ts (2)
operations(1466-4390)components(218-1464)
🪛 Biome (2.1.2)
webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/genericFields/GenericPlanSelector.tsx
[error] 51-51: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 52-52: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 86-86: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 88-88: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 101-101: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (4)
webapp/src/service/apiSchema.generated.ts (2)
1022-1025: New storage field endpoints wired correctly.Route and op bindings look consistent with existing patterns.
20267-20305: GET response typing looks good.Returns
UserStorageResponsewith standard error shapes.webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/cloud/fields/CloudPlanSelector.tsx (1)
8-16: LGTM — derive loading internally.Props surface is cleaner; passing plans and computed loading to GenericPlanSelector looks correct.
Also applies to: 25-30
webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/selfHostedEe/fields/SelfHostedEePlanSelector.tsx (1)
11-25: LGTM! Clean refactor.The component now derives and manages its own loading state internally from the query hook, making it more self-contained and removing the need to propagate loading externally. This aligns with standard react-query patterns and improves the component's API surface.
backend/data/src/main/kotlin/io/tolgee/service/security/UserPreferencesService.kt
Show resolved
Hide resolved
| if (loading) { | ||
| return <BoxLoading />; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix conditional hook usage (violates Rules of Hooks).
Hooks are called after early returns (loading/no plans) and indirectly via useSortPlans. Move hooks to top level.
export const GenericPlanSelector = <T extends GenericPlanType>({
onChange,
value,
selectProps,
onPlanChange,
plans,
loading,
}: GenericPlanSelector<T>) => {
- if (loading) {
- return <BoxLoading />;
- }
+ const { incrementPlanWithId } = usePreferredPlans();
+ const sortedPlans = useSortPlans(plans || []);
+
+ if (loading) {
+ return <BoxLoading />;
+ }
if (!plans) {
return (
<Box>
<T keyName="administration-assign-plan-no-plans-to-assign" />
</Box>
);
}
- const { incrementPlanWithId } = usePreferredPlans();
- const sortedPlans = useSortPlans(plans);Also applies to: 51-53, 85-95, 100-118
| incrementPlanWithId: async (planId: number) => { | ||
| const refetched = await loadable.refetch(); | ||
| const current = refetched.data?.data[planId] ?? 0; | ||
| const newValue = { | ||
| ...refetched.data, | ||
| [planId]: current + 1, | ||
| }; | ||
|
|
||
| update(newValue); | ||
| }, |
There was a problem hiding this comment.
Bug: newValue spreads the whole response, not the stored map.
You should spread the inner data map; otherwise you persist { data: ..., [planId]: ... }.
return {
preferredPlansLoadable: loadable,
incrementPlanWithId: async (planId: number) => {
const refetched = await loadable.refetch();
- const current = refetched.data?.data[planId] ?? 0;
- const newValue = {
- ...refetched.data,
- [planId]: current + 1,
- };
+ const currentMap = (refetched.data?.data ?? {}) as Record<string, number>;
+ const newValue = {
+ ...currentMap,
+ [planId]: (currentMap[planId] ?? 0) + 1,
+ };
update(newValue);
},
};📝 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.
| incrementPlanWithId: async (planId: number) => { | |
| const refetched = await loadable.refetch(); | |
| const current = refetched.data?.data[planId] ?? 0; | |
| const newValue = { | |
| ...refetched.data, | |
| [planId]: current + 1, | |
| }; | |
| update(newValue); | |
| }, | |
| return { | |
| preferredPlansLoadable: loadable, | |
| incrementPlanWithId: async (planId: number) => { | |
| const refetched = await loadable.refetch(); | |
| const currentMap = (refetched.data?.data ?? {}) as Record<string, number>; | |
| const newValue = { | |
| ...currentMap, | |
| [planId]: (currentMap[planId] ?? 0) + 1, | |
| }; | |
| update(newValue); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In
webapp/src/ee/billing/administration/subscriptionPlans/components/planForm/genericFields/GenericPlanSelector.tsx
around lines 107-116, the code spreads refetched.data (the whole response) into
newValue which produces an object like { data: ..., [planId]: ... }; instead,
spread the inner data map so the stored map is updated. Change newValue to
spread refetched.data.data (with safe null checks) and set [planId]: current + 1
so the resulting object is the updated map (e.g. { ...refetched.data.data,
[planId]: current + 1 }) before calling update.
| setStorageField: { | ||
| parameters: { | ||
| path: { | ||
| fieldName: string; | ||
| }; | ||
| }; | ||
| responses: { | ||
| /** OK */ | ||
| 200: unknown; | ||
| /** Bad Request */ | ||
| 400: { | ||
| content: { | ||
| "application/json": string; | ||
| }; | ||
| }; | ||
| /** Unauthorized */ | ||
| 401: { | ||
| content: { | ||
| "application/json": string; | ||
| }; | ||
| }; | ||
| /** Forbidden */ | ||
| 403: { | ||
| content: { | ||
| "application/json": string; | ||
| }; | ||
| }; | ||
| /** Not Found */ | ||
| 404: { | ||
| content: { | ||
| "application/json": string; | ||
| }; | ||
| }; | ||
| }; | ||
| requestBody: { | ||
| content: { | ||
| "application/json": { [key: string]: unknown }; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
PUT request body is too restrictive; should accept any JSON, not only objects.
{ [key: string]: unknown } blocks primitives/arrays. Backend flow suggests setting a field to any JSON value. Use a permissive schema (e.g., JsonNode) or reuse the response DTO wrapper for symmetry.
Update the OpenAPI annotations and regenerate so this segment becomes one of:
Option A (most flexible):
- "application/json": { [key: string]: unknown };
+ "application/json": components["schemas"]["JsonNode"];Option B (symmetry with GET wrapper):
- "application/json": { [key: string]: unknown };
+ "application/json": components["schemas"]["UserStorageResponse"];Can you confirm the backend controller method parameter type? If it’s currently Map<String, Object>, switching to JsonNode (Jackson) would align with Option A and permit primitives/arrays. If it already expects the wrapper, Option B is correct.
🏁 Script executed:
#!/bin/bash
set -e
# Search for controller method named setStorageField
rg -n 'setStorageField' -g '*.java' -g '*.kt' || true
# Inspect PutMapping annotations for storage endpoints
rg -n '@PutMapping' -g '*.java' -g '*.kt' -C5 || true
# Find Map<String, Object> parameter usage
rg -nP 'Map<\s*String,\s*Object>' -g '*.java' -g '*.kt' || trueLength of output: 138304
🏁 Script executed:
#!/bin/bash
set -e
# Search for UserStorageResponse schema/type definition
rg -n 'UserStorageResponse' -- 'webapp/src/service/apiSchema.generated.ts'
# Search for JsonNode in schema components
rg -n 'JsonNode' -- 'webapp/src/service/apiSchema.generated.ts'Length of output: 259
Use JsonNode for the PUT request body schema
Change the OpenAPI annotation so the requestBody becomes:
- "application/json": { [key: string]: unknown };
+ "application/json": components["schemas"]["JsonNode"];This allows primitives, arrays and objects. Regenerate apiSchema.generated.ts.
📝 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.
| setStorageField: { | |
| parameters: { | |
| path: { | |
| fieldName: string; | |
| }; | |
| }; | |
| responses: { | |
| /** OK */ | |
| 200: unknown; | |
| /** Bad Request */ | |
| 400: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| /** Unauthorized */ | |
| 401: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| /** Forbidden */ | |
| 403: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| /** Not Found */ | |
| 404: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| }; | |
| requestBody: { | |
| content: { | |
| "application/json": { [key: string]: unknown }; | |
| }; | |
| }; | |
| }; | |
| setStorageField: { | |
| parameters: { | |
| path: { | |
| fieldName: string; | |
| }; | |
| }; | |
| responses: { | |
| /** OK */ | |
| 200: unknown; | |
| /** Bad Request */ | |
| 400: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| /** Unauthorized */ | |
| 401: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| /** Forbidden */ | |
| 403: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| /** Not Found */ | |
| 404: { | |
| content: { | |
| "application/json": string; | |
| }; | |
| }; | |
| }; | |
| requestBody: { | |
| content: { | |
| "application/json": components["schemas"]["JsonNode"]; | |
| }; | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In webapp/src/service/apiSchema.generated.ts around lines 20306 to 20345, the
PUT operation setStorageField currently types the requestBody content as { [key:
string]: unknown } which only models objects; update the OpenAPI annotation so
the requestBody content for "application/json" uses JsonNode (to allow
primitives, arrays, and objects), then regenerate apiSchema.generated.ts so the
generated TypeScript reflects JsonNode for that request body.
Marketa hated that she had to scroll trough many billing plans to pick the one she uses all the time. It was time consuming for her.
I added the storage field which can be operated from frontend. We can store any data required for proper displaying of data on the frontend.
useUserPreferenceStorageso it's easy to get and update the data in the storage.Summary by CodeRabbit
New Features
Improvements
Tests