[B3-2868] Fix apps crashing, revert some recent refactoring#419
[B3-2868] Fix apps crashing, revert some recent refactoring#419
Conversation
Summary of ChangesHello @GioLogist, 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 addresses critical application crashes by rolling back recent refactoring efforts. The core change involves centralizing the 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. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
GioLogist
force-pushed
the
B3-2868-fix-crash-revert-some-refactoring
branch
from
1d7fb10 to
ff8d288
Compare
![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 a significant refactoring of the React context and provider logic within the B3 SDK, consolidating B3ConfigProvider and AuthenticationProvider into a new B3Provider and B3Context. This change simplifies the provider structure and standardizes context access through a new useB3 hook. While this is a positive change, the review has identified a few regressions that could impact application stability and user experience. Key issues include a missing account property in the native provider's context, the removal of a crucial authentication check in the useB3Account hook, and the loss of cross-tab state synchronization for user data. Addressing these issues is important to ensure the refactoring is robust and bug-free.
| defaultPermissions={defaultPermissions} | ||
| <B3Context.Provider | ||
| value={{ | ||
| //account: effectiveAccount, |
There was a problem hiding this comment.
The account property in the B3Context.Provider is commented out. This is inconsistent with the web implementation of B3Provider and will likely cause crashes or incorrect behavior in native builds for any component that consumes this context and expects account to be present. Please uncomment this line to provide the effectiveAccount to the context.
| //account: effectiveAccount, | |
| account: effectiveAccount, |
| import { useActiveAccount } from "thirdweb/react"; | ||
| import { useAuthStore } from "../../stores/useAuthStore"; | ||
|
|
||
| // Wrapper around useActiveAccount | ||
| export const useB3Account = () => { | ||
| const isAuthenticated = useAuthStore(state => state.isAuthenticated); | ||
| const activeAccount = useActiveAccount(); | ||
| const effectiveAccount = isAuthenticated ? activeAccount : undefined; | ||
|
|
||
| return effectiveAccount; | ||
| const account = useActiveAccount(); | ||
| return account; | ||
| }; |
There was a problem hiding this comment.
The isAuthenticated check from useAuthStore has been removed. This is a regression that could lead to incorrect behavior, as useB3Account will now return an account even if the user is not authenticated with B3. This could cause UI components to incorrectly assume a user is logged in.
Please restore the authentication check to ensure this hook only returns an account when the user is authenticated.
| import { useActiveAccount } from "thirdweb/react"; | |
| import { useAuthStore } from "../../stores/useAuthStore"; | |
| // Wrapper around useActiveAccount | |
| export const useB3Account = () => { | |
| const isAuthenticated = useAuthStore(state => state.isAuthenticated); | |
| const activeAccount = useActiveAccount(); | |
| const effectiveAccount = isAuthenticated ? activeAccount : undefined; | |
| return effectiveAccount; | |
| const account = useActiveAccount(); | |
| return account; | |
| }; | |
| import { useActiveAccount } from "thirdweb/react"; | |
| import { useAuthStore } from "../../stores/useAuthStore"; | |
| // Wrapper around useActiveAccount | |
| export const useB3Account = () => { | |
| const isAuthenticated = useAuthStore(state => state.isAuthenticated); | |
| const activeAccount = useActiveAccount(); | |
| return isAuthenticated ? activeAccount : undefined; | |
| }; |
| export function useUserQuery() { | ||
| const user = useUserStore(state => state.user); | ||
| const setUserStore = useUserStore(state => state.setUser); | ||
| const clearUserStore = useUserStore(state => state.clearUser); | ||
| const setUser = useUserStore(state => state.setUser); | ||
| const clearUser = useUserStore(state => state.clearUser); | ||
|
|
||
| // Listen for storage events from other tabs/windows | ||
| useEffect(() => { | ||
| const handleStorageChange = (e: StorageEvent) => { | ||
| if (e.key === "b3-user") { | ||
| // Sync with changes from other tabs/windows | ||
| const stored = e.newValue; | ||
| if (stored) { | ||
| try { | ||
| const parsed = JSON.parse(stored); | ||
| // Zustand persist format: { state: { user: ... }, version: ... } | ||
| const userData = parsed?.state?.user ?? parsed?.user ?? null; | ||
| useUserStore.setState({ user: userData }); | ||
| } catch (error) { | ||
| console.warn("Failed to parse user from storage event:", error); | ||
| } | ||
| } else { | ||
| useUserStore.setState({ user: null }); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("storage", handleStorageChange); | ||
| return () => { | ||
| window.removeEventListener("storage", handleStorageChange); | ||
| }; | ||
| }, []); | ||
|
|
||
| // Helper function to set user (maintains backward compatibility) | ||
| const setUser = (newUser?: Users) => { | ||
| setUserStore(newUser); | ||
| }; | ||
|
|
||
| // Helper function to invalidate and refetch user | ||
| const refetchUser = async () => { | ||
| // Re-read from localStorage and update store | ||
| // Zustand persist stores data as { state: { user: ... }, version: ... } | ||
| const stored = localStorage.getItem("b3-user"); | ||
| if (stored) { | ||
| try { | ||
| const parsed = JSON.parse(stored); | ||
| // Zustand persist format: { state: { user: ... }, version: ... } | ||
| const userData = parsed?.state?.user ?? parsed?.user ?? null; | ||
| useUserStore.setState({ user: userData }); | ||
| return userData ?? undefined; | ||
| } catch (error) { | ||
| console.warn("Failed to refetch user from localStorage:", error); | ||
| // Fallback to current store state | ||
| return useUserStore.getState().user ?? undefined; | ||
| } | ||
| if (user) { | ||
| debug("User loaded from store", user); | ||
| } | ||
| useUserStore.setState({ user: null }); | ||
| return undefined; | ||
| }; | ||
|
|
||
| // Helper function to clear user | ||
| const clearUser = () => { | ||
| clearUserStore(); | ||
| }; | ||
| }, [user]); | ||
|
|
||
| return { | ||
| user: user ?? undefined, | ||
| user, | ||
| setUser, | ||
| refetchUser, | ||
| clearUser, | ||
| queryKey: USER_QUERY_KEY, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The logic for synchronizing the user state across browser tabs via the storage event has been removed. This is a regression that will cause inconsistent state between tabs when a user logs in or out in one tab. Please reintroduce this functionality to ensure a consistent user experience.
export function useUserQuery() {
const user = useUserStore(state => state.user);
const setUser = useUserStore(state => state.setUser);
const clearUser = useUserStore(state => state.clearUser);
useEffect(() => {
const handleStorageChange = (e: StorageEvent) => {
if (e.key === "b3-user") {
const stored = e.newValue;
if (stored) {
try {
const parsed = JSON.parse(stored);
const userData = parsed?.state?.user ?? null;
useUserStore.setState({ user: userData });
} catch (error) {
console.warn("Failed to parse user from storage event:", error);
}
} else {
useUserStore.setState({ user: null });
}
}
};
window.addEventListener("storage", handleStorageChange);
return () => {
window.removeEventListener("storage", handleStorageChange);
};
}, []);
useEffect(() => {
if (user) {
debug("User loaded from store", user);
}
}, [user]);
return {
user,
setUser,
clearUser,
};
}| // WOJ: why if isAuthenticated is false, we don't use activeAccount, which should be undefined? | ||
| // skip isAuthenticated check ? |
There was a problem hiding this comment.
This comment appears to be a leftover from a code review discussion. It should be removed to keep the codebase clean and avoid confusion for future developers.
GioLogist
force-pushed
the
B3-2868-fix-crash-revert-some-refactoring
branch
from
ff8d288 to
4d1637a
Compare
lubojanski
left a comment
lubojanski
left a comment
There was a problem hiding this comment.
DO NOT THE PR.
Pls, I pushed a fix ;_;
2 participants
B3-2868
Description
In short, it seems the changes we are making that are moving things from initial B3 Provider to separate contexts or states are becoming counter intuitive. First we extracted to x-state instead of just leaving in useB3 here. Now, we moved from x-state to B3ConfigProvider here. In the end, I think the efforts are proving counter-intuitive vs just leaving in the main provider
While we dig deeper, this PR unwinds much of those changes. In order to make "reverting THIS revert" easy. I've isolated all unwinding into a single commit here https://github.com/b3-fun/b3-mono/pull/1816/commits/f76d15eb107c726fdc74aacc77ff16ce0fad3e56
Test Plan
Screenshots
For BE, include snippets, response payloads and/or curl commands to test endpoints
[FE] Before
[FE] After
[BE] Snippets/Response/Curl
automerge=false