Skip to content

All Feature's using Feature Flag Hook #324

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 4 commits into
base: dev
Choose a base branch
from

Conversation

HYDRO2070
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Mar 16, 2025

@HYDRO2070 is attempting to deploy a commit to the thieflord06's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Mar 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clearsky-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 7:25pm

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

the majority of these flags are associated with specific queries the app makes, but as coded here, a disabled flag does not prevent the app from making the query. this defeats the whole point of having the flags in the first place.

@@ -28,6 +29,8 @@ export function AccountHeader({ className, onInfoClick }) {
const handleHistoryQuery = useHandleHistory(resolved.data?.shortDID);
const handleHistory = handleHistoryQuery.data?.handle_history;

const userPlacement = useFeatureFlag('user-placement')

const placementquery = usePlacement(resolved.data?.shortDID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the placement flag needs to conditionally disable this query rather than just hiding the ui it displays in

@@ -41,6 +42,8 @@ export function BlockPanelGeneric({
const blocklistPages = data?.pages || [];
const blocklist = blocklistPages.flatMap((page) => page.blocklist);
const count = totalData?.count;
const pageName = (className == 'blocked-by-panel') ? 'blocked-by-count' : 'blocking-count';
const pageCount = useFeatureFlag(pageName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

likewise, this needs to disable the queries, but those are made elsewhere and passed by props, so this hook needs to be called elsewhere too

@@ -30,6 +31,7 @@ export function BlockedByLists() {
const listPages = data?.pages || [];
const allLists = listPages.flatMap((page) => page.blocklist);
const filteredLists = !search ? allLists : matchSearch(allLists, search, () => setTick(tick + 1));
const listsBlockedByCount = useFeatureFlag('lists-blocked-by-count')
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here as well. should be controlling whether a query is made

@@ -30,7 +31,7 @@ export function BlockingLists() {
const listPages = data?.pages || [];
const allLists = listPages.flatMap((page) => page.blocklist);
const filteredLists = !search ? allLists : matchSearch(allLists, search, () => setTick(tick + 1));

const listsBlockingCount = useFeatureFlag('lists-blocking-count')
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here

@@ -96,6 +97,7 @@ export default function LabeledPanel() {
const { data: labelers, isLoading: isLoadingLabelers } = useLabelers();

const { data: labels, isLoading } = useLabeled(did, labelers);
const labelsCount = useFeatureFlag('labels-count')
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator

Choose a reason for hiding this comment

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

this this is one last api call that wasn't addressed in the last round of changes

@@ -29,6 +30,7 @@ export function Lists() {
const [tick, setTick] = useState(0);
const search = (searchParams.get('q') || '').trim();
const [showSearch, setShowSearch] = useState(!!search);
const listsOnListCounts = useFeatureFlag('lists-on-list-counts')
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@@ -30,6 +31,7 @@ export default function Packed() {
const search = (searchParams.get('q') || '').trim();
const [tick, setTick] = useState(0);
const [showSearch, setShowSearch] = useState(!!search);
const starterPacksInCount = useFeatureFlag('starter-packs-in-count');
Copy link
Collaborator

Choose a reason for hiding this comment

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

and again here

@@ -30,6 +31,7 @@ export function Packs({ created = false }) {
const [tick, setTick] = useState(0);
const search = (searchParams.get('q') || '').trim();
const [showSearch, setShowSearch] = useState(!!search);
const starterPacksMadeCount = useFeatureFlag('starter-packs-made-count');
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here

@HYDRO2070
Copy link
Contributor Author

HYDRO2070 commented Apr 14, 2025

@noahm Done with the changes here. Please review it. (Name of the Variable are a bit long. I did it for Understanding Purpose.)
@thieflord06

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

This looks much better now. Very close to ready. There's one api call that got overlooked from last time, and two new places where you're now conditionally calling a hook. they should use the same pattern as the others where you can pass an enabled or skip parameter.

const userPlacement = useFeatureFlag('user-placement')

// call only if userPlacement is true
const placementquery = (userPlacement) ? usePlacement(resolved.data?.shortDID) : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you cannot conditionally call a hook. this breaks the rules of react hooks.

const ishandleHistory = useFeatureFlag('handle-history')

// calls only if ishandleHistory is true
const handleHistoryQuery = (ishandleHistory) ? useHandleHistory(account?.shortDID) : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is also breaking the rules of hooks

@@ -96,6 +97,7 @@ export default function LabeledPanel() {
const { data: labelers, isLoading: isLoadingLabelers } = useLabelers();

const { data: labels, isLoading } = useLabeled(did, labelers);
const labelsCount = useFeatureFlag('labels-count')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this this is one last api call that wasn't addressed in the last round of changes

@HYDRO2070
Copy link
Contributor Author

@noahm @thieflord06
Done.

Regarding the label count: the API is fetching all the labels, not just calling it for the count. If we were to stop that call, the labels wouldn't be fetched at all—which we don’t want.

Signed-off-by: Shashank pandey <[email protected]>
Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

You're totally right about the labels count, my apologies. (Seems like a completely pointless flag for us to have in that case) This looks good to go now!

@thieflord06
Copy link
Member

From testing it looks like the counts aren't behaving correctly when not enabled.

lists-blocking-count
blocked-by-count
blocking-count
lists-on-count
lists-blocked-by-count
lists-on-list-counts
starter-packs-in-count
labels-count

Labels-count is the only one I've been able to confirm is working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants