refactor(features): migrate FeaturesPage to RTK Query and eliminate legacy stores #6361
+1,490
−575
Conversation
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
Create reusable hook that wraps useUpdateProjectMutation with automatic toast notifications, supporting customizable messages and error callbacks.
Replace duplicated try/catch/toast patterns across 5 components with useUpdateProjectWithToast hook, eliminating ~70 lines of duplicated code.
Replace complex Omit/Partial approach with explicit Pick for better clarity and type safety. This makes it immediately clear which Project fields are updateable via the API.
Add RTK Query optimistic updates to updateProject mutation using onQueryStarted lifecycle hook. This immediately updates the cache when mutations are called and automatically rolls back on errors, providing instant UI feedback without manual rollback logic. Benefits: - Instant UI updates before server responds - Automatic error rollback via patchResult.undo() - Reduces component complexity by eliminating manual error handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove useEffect and manual rollback logic from 3 components that now benefit from mutation-level optimistic updates: - ProjectInformation: Remove useEffect syncing and onError rollback - AdditionalSettings: Remove useEffect syncing - FeatureNameValidation: Remove useEffect syncing and manual optimistic update Components now use local state only for form editing, with RTK Query handling all cache updates and automatic rollbacks on errors. Benefits: - Simpler components (~22 lines removed across 3 files) - No cursor jumps during typing when other settings update - No manual error handling needed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Create ChangeRequestsApprovalsSetting component following the same pattern as other settings (PreventFlagDefaultsSetting, CaseSensitivitySetting, FeatureNameValidation). Changes: - New ChangeRequestsApprovalsSetting.tsx component - AdditionalSettings simplified from 76 to 25 lines - All settings now follow consistent pattern (receive project prop) Benefits: - Better consistency across all setting components - Improved encapsulation (component owns its state) - Easier to test in isolation - Cleaner AdditionalSettings parent component 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove 9 unnecessary useCallback wrappers from project settings components. These callbacks had dependencies that change frequently (project properties, local state), causing recreation on every render and providing no memoization benefit. Changes across 7 files: - CaseSensitivitySetting: Remove useCallback from handleToggle - PreventFlagDefaultsSetting: Remove useCallback from handleToggle - FeatureNameValidation: Remove useCallback from handleToggle + handleSave - ProjectInformation: Remove useCallback from handleSubmit - ChangeRequestsApprovalsSetting: Remove useCallback from both functions - SDKSettingsTab: Remove useCallback from 2 toggle handlers - DeleteProject: Remove useCallback from handleDelete Benefits: - Simpler, more honest code (~36 lines removed) - Same performance (callbacks already recreated every render) - Removed unused useCallback imports 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Call AppActions.refreshOrganisation() after project updates to ensure the legacy OrganisationStore stays in sync with RTK Query cache. This fixes the navbar not updating when project name changes, which was causing the E2E test (project-test.ts) to fail.
Fixed critical bug where the case sensitivity toggle had inverted behavior.
The backend field 'only_allow_lower_case_feature_names' has a negative
semantic (true = force lowercase), but the UI shows "Case sensitive features"
with a positive semantic (ON = allow uppercase).
Therefore we must invert the checked state to match the UI's meaning:
- checked={!project.only_allow_lower_case_feature_names}
Now when users enable "Case sensitive features", it correctly sets
only_allow_lower_case_feature_names=false, allowing uppercase characters.
This matches the original implementation and fixes the critical issue
documented in PROJECT_ORG_FIXES.md #1.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
…etting - Add ?? null to handle undefined initial values from backend - Fix stale closure bug in auto-save by passing value explicitly to saveChangeRequests - Fix manual save by using arrow function closure for onSave prop - Ensure both toggle (auto-save) and manual save work correctly This fixes issues where: - Initial value could be undefined instead of null - Auto-save on toggle used stale state value - Manual save button passed event object instead of current state 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add optional data-test prop to component type definition - Pass data-test to underlying Setting component - Follow kebab-case naming convention (js-change-request-approvals) This enables E2E testing by providing stable selectors that won't break with styling changes, following the pattern used by other settings in the codebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add E2E tests covering four critical scenarios: 1. Auto-save on toggle ON - verifies toggle saves immediately 2. Manual save after value change - verifies save button works 3. Auto-save on toggle OFF - verifies disable persists 4. Manual save with new value - verifies full workflow Each test includes navigation away and back to ensure values persist to backend, catching stale closure bugs that would otherwise pass with in-memory state. Test uses kebab-case data-test selector following codebase conventions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…lability Changed from early return pattern to wrapped conditional block following the pattern from initialise-tests.ts. This allows future tests added after the Change Requests tests to run regardless of the segment_change_requests feature flag status. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add UpdateOrganisationBody type with optional fields - Add getOrganisation, updateOrganisation, deleteOrganisation request types - Support for organisation name, force_2fa, and project creation restrictions Part of OrganisationSettingsPage TypeScript migration
- Add getOrganisation query with cache tags - Add updateOrganisation mutation with optimistic updates - Add deleteOrganisation mutation with cache invalidation - Implement automatic rollback on error for optimistic updates Part of OrganisationSettingsPage TypeScript migration
- Add useUpdateOrganisationWithToast hook for consistent update handling - Add useDeleteOrganisationWithToast hook with success callbacks - Include toast notifications and error handling - Support legacy store sync via AppActions.refreshOrganisation() Part of OrganisationSettingsPage TypeScript migration
…ical-components-featurespagejs-2
Zaimwa9
requested changes
Dec 5, 2025
Contributor
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
It's honestly amazing to see and read. Great job and thanks for the effort.
I particularly love seeing the new hooks and having all the critical logic gathered and clearly isolated.
I have a couple of comments that are clearly minor in comparison of the large size of this PR. Let's have the tests passing and will quickly have a ✅
| import type { FilterState } from 'common/types/featureFilters' | ||
|
|
||
| /** Fetches filtered feature list with automatic API key to ID conversion. */ | ||
| export function useFeatureListWithFilters( |
| * usePageTracking(Constants.pages.FEATURES, [environmentId]) | ||
| * ``` | ||
| */ | ||
| export function usePageTracking( |
Contributor
There was a problem hiding this comment.
This one is not used. I do feel explicitely passing the required deps will be more self-explanatory in the future.
Wdyt about:
- merging the two of them
- pass the deps / context as a params object
- Add a boolean to save or not in the storage ?
Contributor
Author
There was a problem hiding this comment.
Sounds good, I will consolidate them
Comment on lines
7
to
14
| export function useProjectEnvironments(projectId: number): { | ||
| project: Project | undefined | ||
| environments: Environment[] | ||
| getEnvironmentIdFromKey: (apiKey: string) => number | undefined | ||
| getEnvironment: (apiKey: string) => Environment | undefined | ||
| isLoading: boolean | ||
| error: unknown | ||
| } { |
Contributor
There was a problem hiding this comment.
Do you mind extracting its type into an interface/type?
| ) | ||
|
|
||
| return { | ||
| environments, |
Contributor
There was a problem hiding this comment.
This is an amazing addition thanks
Comment on lines
8
to
16
| return ( | ||
| (filters.tags && filters.tags.length > 0) || | ||
| filters.showArchived || | ||
| (filters.search !== null && filters.search.length > 0) || | ||
| filters.is_enabled !== null || | ||
| (filters.value_search && filters.value_search.length > 0) || | ||
| (filters.owners && filters.owners.length > 0) || | ||
| (filters.group_owners && filters.group_owners.length > 0) | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested change
| return ( | |
| (filters.tags && filters.tags.length > 0) || | |
| filters.showArchived || | |
| (filters.search !== null && filters.search.length > 0) || | |
| filters.is_enabled !== null || | |
| (filters.value_search && filters.value_search.length > 0) || | |
| (filters.owners && filters.owners.length > 0) || | |
| (filters.group_owners && filters.group_owners.length > 0) | |
| ) | |
| return !!( | |
| filters.tags?.length || | |
| filters.search?.length || | |
| filters.value_search?.length || | |
| filters.owners?.length || | |
| filters.group_owners?.length || | |
| filters.showArchived || | |
| filters.is_enabled !== null | |
| ); |
could work ?
Comment on lines
89
to
107
| group_owners: | ||
| typeof params.group_owners === 'string' && params.group_owners | ||
| ? params.group_owners | ||
| .split(',') | ||
| .filter((v) => v) | ||
| .map((v: string) => parseInt(v)) | ||
| : [], | ||
| is_enabled: (() => { | ||
| if (params.is_enabled === 'true') return true | ||
| if (params.is_enabled === 'false') return false | ||
| return null | ||
| })(), | ||
| owners: | ||
| typeof params.owners === 'string' && params.owners | ||
| ? params.owners | ||
| .split(',') | ||
| .filter((v) => v) | ||
| .map((v: string) => parseInt(v)) | ||
| : [], |
Contributor
There was a problem hiding this comment.
What do you think about using const for each of them and just return the object at the end to have a bit less of inline operations ?
Contributor
Author
There was a problem hiding this comment.
Yep, sounds good, I will refactor this file
| style?: CSSProperties | ||
| } | ||
|
|
||
| export const FeatureRowSkeleton: FC<FeatureRowSkeletonProps> = ({ style }) => { |
Contributor
There was a problem hiding this comment.
Nice, just adding a screenshot for future readers
| type GetExtraMetricsFunc = ( | ||
| projectId: string, | ||
| environmentApiKey: string, | ||
| environmentId: string, |
Contributor
There was a problem hiding this comment.
Now I'm confused 😄 which one is AXerjeUR.. and which one is 4.
I tend to think this is the first one environmentApiKey right?
If yes, what do you think? Intuitively to avoid mixing them I would call it environmentKey and have environmentId with the number?
I'm honestly not opiniated on this but i'm curious to have your thoughts as I personally always mix them
Contributor
Author
There was a problem hiding this comment.
Great point about the naming confusion! You're absolutely right that this is confusing.
To clarify the current state:
environmentApiKey(string likeAXerjeUR..) - this is what RouteContext currently providesenvironmentId(number like4) - this is what the API actually needs
Your suggestion makes total sense: environmentKey and environmentId would be much clearer!
The reason I kept the current naming in this PR is to maintain consistency with the existing RouteContext while we complete this FeaturesPage migration. However, I think this would be an excellent candidate for a follow-up PR where we could:
- Add both values to RouteContext:
environmentKey(API key string) andenvironmentId(numeric ID) - Update all components to use the clearer naming
- Remove the conversion step that's currently needed in the hooks
This would make the code much more intuitive and eliminate the wrapper hook entirely. What do you think?
This commit fixes several issues with the FeaturesPage RTK Query migration: 1. **Cache Synchronization**: Added FeatureListStore event listeners to refetch RTK Query data when legacy Flux store saves or removes features, ensuring both caches stay synchronized during the migration period. 2. **Stale Cache on Navigation**: Added `refetchOnMountOrArgChange: true` to useFeatureListWithFilters hook to ensure fresh data is fetched when navigating between tabs, preventing stale empty results from showing. 3. **Empty State Flash**: Fixed rendering logic to show skeleton loaders during initial load and when fetching with no data, preventing the empty state from briefly flashing when features exist. 4. **Code Quality**: Removed duplicate skeleton loader code by consolidating conditions into a single check. These changes maintain backward compatibility with the legacy CreateFlag modal while ensuring proper data synchronization between Flux and RTK Query during the migration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Delete unused useFeatureListWithFilters hook - Make usePageTracking function internal (only export usePageTrackingWithContext) - Make ProjectEnvironmentsHook interface internal - Remove dead exports from useFeatureList (multivariate mutations, getFeatureList) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Convert TagStrategy from type alias to enum for type safety and consistency.
Before: type TagStrategy = 'INTERSECTION' | 'UNION'
After: enum TagStrategy { INTERSECTION = 'INTERSECTION', UNION = 'UNION' }
This enables using enum values in comparisons and eliminates type assertions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
…rParams Major refactoring to improve type safety and eliminate all type assertions: Changes: - Remove SortDirection and SortOrder enums (use string literals instead) - Add helper functions for type-safe URL param parsing: - parseStringParam: safely extract string from URL params - parsePageNumber: parse page with NaN check - parseSortOrder: parse sort order with type safety - parseIntArray: parse comma-separated integers - parseBooleanParam: parse boolean values - normalizeSortDirection: convert UI sort to API format - Use TagStrategy enum in comparisons instead of magic strings - Update buildApiFilterParams with comprehensive JSDoc and TODO comments - Extract EnvironmentIdResolver type for clarity Benefits: - Zero type assertions throughout the file - All URL params properly validated before use - Consistent helper function patterns - Better error handling for edge cases (arrays, undefined, NaN) - Improved code organization with clear sections 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add wrapper hook that bridges routing (API keys) and API (numeric IDs). This hook encapsulates the environment API key → numeric ID conversion logic, allowing FeaturesPage to use environment API keys directly without needing to know about ID conversion details. Changes: - Add useFeatureListWithApiKey hook with comprehensive JSDoc - Uses skipToken instead of type assertions for conditional queries - Delegates ID conversion to buildApiFilterParams via callback - Update FeaturesPage to use new wrapper hook TODO: This wrapper will be simplified once environmentNumericId is added to RouteContext. See FEATURES_MIGRATION_FOLLOWUP.md - PR #1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove noise and improve readability: - Remove section divider comments (Helper Functions, Exported Functions) - Simplify buildApiFilterParams JSDoc (from 22 lines to 6) - Simplify useFeatureListWithApiKey JSDoc (from 18 lines to 6) - Remove obvious inline comments that repeat what code already says Keeps concise one-line JSDoc for helper functions and public APIs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
talissoncosta
commented
Dec 5, 2025
| ) | ||
| }, [filters, page, environmentApiKey, projectId, getEnvironmentIdFromKey]) | ||
|
|
||
| return useGetFeatureListQuery(apiParams ?? skipToken, { |
Contributor
Author
There was a problem hiding this comment.
Why skipToken?
We use RTK Query's skipToken to conditionally skip queries when parameters aren't ready yet:
// frontend/common/hooks/useFeatureListWithApiKey.ts
const apiParams = useMemo(() => {
if (!environmentApiKey || !projectId || !getEnvironmentIdFromKey) {
return null // Parameters not ready
}
return buildApiFilterParams(filters, page, environmentApiKey, projectId, getEnvironmentIdFromKey)
}, [filters, page, environmentApiKey, projectId, getEnvironmentIdFromKey])
return useGetFeatureListQuery(apiParams ?? skipToken, {
refetchOnMountOrArgChange: true,
skip: isLoadingEnvironments,
})The Problem
apiParamscan benullwhen environment data isn't loaded yet- RTK Query hooks expect valid parameters or
skipToken - TypeScript would error if we pass
nulldirectly as query parameters
Why Not Just Use skip Option?
// ❌ This causes TypeScript errors
useGetFeatureListQuery(apiParams, {
skip: !apiParams || isLoadingEnvironments
})
// Error: Type 'null' is not assignable to parameterAlternative We Avoided
// ❌ Type assertion - exactly what we're trying to avoid!
useGetFeatureListQuery(apiParams as any, {
skip: !apiParams || isLoadingEnvironments
})Benefits of skipToken
✅ Type-safe - No type assertions needed
✅ Explicit intent - Clear that query should be skipped
✅ RTK Query pattern - Official solution from Redux Toolkit docs
✅ Clean code - One line: apiParams ?? skipToken
Reference
- Fix typo: "environementID" → "environmentID" in JSDoc comment - Add explicit radix parameter to parseInt() calls to ensure base-10 parsing and avoid potential octal interpretation issues 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Summary
This PR completes the migration of FeaturesPage from legacy Flux stores to RTK Query with comprehensive refactoring, improved TypeScript type safety, better error handling, and cleaner documentation.
Changes
1. Core RTK Query Migration
useFeatureListWithFiltershook for filtered feature fetching with automatic API key to ID conversionuseProjectEnvironmentshook to replace ProjectStore functionality2. Custom Hooks & Code Organization
useCallbackwrappers with empty dependenciesfeatureFilterParams.tsfor global reusefeatureFilters.ts3. UI/UX Improvements
isTogglingandisRemovingstates from mutation hooksisSavingbehavior4. Component Refactoring
useFeaturePageDisplayhookrenderFeaturesList()helper functionshowCreateButtonprop (was always true)5. Type Safety Improvements
projectIdtype tonumberacross all components6. Documentation Cleanup
useFeatureListWithFilters.ts: 81→34 linesfeatureFilterParams.ts: Simplified 4 JSDoc blocksuseFeatureFilters.ts: 26→3 linesuseRemoveFeatureWithToast.ts: 18→1 lineuseToggleFeatureWithToast.ts: 18→1 lineuseProjectEnvironments.ts: 25→1 linefeatureFilters.ts: Simplified type comments7. Performance Optimizations
8. Code Quality
Files Changed
Created:
common/hooks/useFeatureListWithFilters.tscommon/hooks/useProjectEnvironments.tscommon/types/featureFilters.tscommon/utils/featureFilterParams.tsweb/components/pages/features/hooks/useFeatureFilters.tsweb/components/pages/features/hooks/useToggleFeatureWithToast.tsweb/components/pages/features/hooks/useRemoveFeatureWithToast.tsweb/components/feature-summary/FeatureRowSkeleton.tsxweb/styles/components/_feature-row-skeleton.scssModified:
web/components/pages/features/FeaturesPage.tsx(major refactor)web/components/pages/features/components/FeaturesPageHeader.tsxweb/components/pages/features/components/FeaturesTableFilters.tsxRemoved:
web/components/pages/features/components/FeaturesList.tsxweb/components/pages/features/hooks/useFeaturePageDisplay.tsBenefits
Breaking Changes
None - all functionality preserved, this is a pure refactor.
How did you test this code?
Manual Testing:
Features Page Load
Filter Functionality
Feature Actions
Error Handling
Type Safety
Accessibility
Validation: