Skip to content

Debugging to user/index.ts in ndk:user namespace / Updated some tests#362

Open
digitalbase wants to merge 3 commits intomasterfrom
feature/user-debugging
Open

Debugging to user/index.ts in ndk:user namespace / Updated some tests#362
digitalbase wants to merge 3 commits intomasterfrom
feature/user-debugging

Conversation

@digitalbase
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@pablof7z pablof7z left a comment

Choose a reason for hiding this comment

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

Code Review: Needs Changes

This PR adds useful debug logging to user/index.ts, but has several issues:

Issues

  1. biome.json change is unrelated — Disabling noNonNullAssertion project-wide should not be bundled with a debugging PR.

  2. Side-effect in debug log — In getZapInfo(), the debug statement calls await this.fetchProfile() BEFORE the Promise.all, which means the profile is fetched twice and the promiseWithTimeout wrapper is removed, changing timeout behavior:

    d("Fetching user Profile and mint event", {
        profile: await this.fetchProfile(), // <-- fetches profile as side effect
    });
    const [userProfile, mintListEvent] = await Promise.all([
        this.fetchProfile(), // <-- fetches again
  3. Removed promiseWithTimeout wrapper — The getZapInfo() method had an optional timeout mechanism that's been removed, which is a behavioral change.

  4. Import reformatting — Cosmetic changes (removing spaces in destructuring) add noise to the diff.

  5. Test changes — Switching from ndk.getUser() to ndk.fetchUser() changes test semantics (sync vs async). These might be intentional improvements but need their own PR context.

Suggestions

  • Remove the biome.json change
  • Fix the side-effect fetch in the debug log (log the pubkey only, not the profile)
  • Keep the promiseWithTimeout wrapper
  • Submit import reformatting separately or not at all

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.

2 participants