Skip to content

ref(distributions): Refactor fetching flags and their suspect scores #89962

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Apr 18, 2025

This is in preparation of the new suspect flags table. The new table will call useFlagDrawerData() and then show any flags that are over a specific threshold. But that table and all the ui stuff is a bunch of code, so i split this off separately.

Once the new table starts calling useFlagDrawerData then there'll be two spots that will share the same caches for useGroupFeatureFlags and useGroupSuspectFlagScores; but all the useMemo calls in here will double up the memory footprint; optimizing for sharing code over memory i guess.

@ryan953 ryan953 requested a review from a team as a code owner April 18, 2025 23:12
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 18, 2025
@ryan953 ryan953 requested a review from a team April 19, 2025 00:07
refetch: () => void;
}

export default function useFlagDrawerData({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export default function useFlagDrawerData({
export default function useGroupFlagDrawerData({

To match file

}, [filteredFlags, orderBy, sortBy]);

return {
allFlagCount: suspectFlags.length,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
allFlagCount: suspectFlags.length,
allGroupFlagCount: suspectFlags.length,

});

// Combine the flag and suspect data into SuspectGroupTag objects
const suspectFlags = useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const suspectFlags = useMemo(() => {
const allFlagsWithScores = useMemo(() => {

Or something that's not suspectFlags. When I read suspectFlags, I think tthe highlighted, most suspicious flags.

isError: isSuspectEnabled ? isFlagsError || isSuspectError : isFlagsError,
isPending: isSuspectEnabled ? isFlagsPending || isSuspectPending : isFlagsPending,
refetch: () => {
refetchFlags();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it useful to include the refetch for scores too?

);
});
}, [data, search, tagValues]);
// const enableSuspectFlags = organization.features.includes('feature-flag-suspect-flags');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove?
I think we can skip checking this FF for the debug UI. It's more for gating the endpoint, which should be enabled internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants