Skip to content

Comments

Migrate Navbar from StarkNet to Stellar Wallet#267

Open
chiscookeke11 wants to merge 10 commits intoStreamFi-x:devfrom
chiscookeke11:issue-236
Open

Migrate Navbar from StarkNet to Stellar Wallet#267
chiscookeke11 wants to merge 10 commits intoStreamFi-x:devfrom
chiscookeke11:issue-236

Conversation

@chiscookeke11
Copy link
Contributor

@chiscookeke11 chiscookeke11 commented Feb 22, 2026

Description

This PR updates components/explore/Navbar.tsx to replace StarkNet wallet integration with the Stellar wallet context (stellar-wallet-kit).

_Closes #236

Changes proposed

What were you told to do?

Migrate the Navbar from using StarkNet (useAccount, useDisconnect) to the Stellar wallet context. Update address display, connect/disconnect logic, session storage, and profile fetch accordingly.

What did you do?

  1. Removed StarkNet hooks

    • useAccount and useDisconnect imports removed.
  2. Added Stellar wallet hook

    • Imported useWallet from stellar-wallet-kit.

    • Updated hook usage:

      const { account, isConnected, disconnect, connect } = useWallet();
      const publicKey = account?.publicKey;
  3. Address display updated to Stellar G-address

    • Replaced all address.slice(...) with publicKey.slice(...).
    • Example: GABCDE...WXYZ
  4. Connect/Disconnect behavior updated

    • Connect button opens Stellar wallet modal using connect().
    • Disconnect button calls disconnect() from Stellar wallet context.
  5. Profile fetch updated

    • /api/users/wallet/${address}/api/users/wallet/${publicKey}
    • Session storage now saves publicKey as walletAddress.
  6. Truncation and display logic

    • getDisplayName() and renderDisplayName() updated to handle Stellar public key.
    • Maximum length for display name applied.
  7. Other updates

    • Removed all StarkNet-specific logic.
    • Preserved UI layout, modals, and profile dropdown behavior.

Check List (Check all the applicable boxes)

🚨Please review the contribution guideline for this repository.

  • My code follows the code style of this project.
  • This PR does not contain plagiarized content.
  • The title and description of the PR is clear and explains the approach.
  • I am making a pull request against the main branch (left side).
  • My commit messages styles matches our requested structure.
  • My code additions will fail neither code linting checks nor unit test.
  • I am only making changes to files I was requested to.

Screenshots/Videos

https://www.loom.com/share/e52926c979bb42918d77f77d0c1dd2cc

@vercel
Copy link

vercel bot commented Feb 22, 2026

@chiscookeke11 is attempting to deploy a commit to the david's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@davedumto davedumto left a comment

Choose a reason for hiding this comment

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

Review — Changes Requested

This PR has a critical bug and massive scope creep that must be addressed.


1. CRITICAL Bug - Wrong Property Access

Location: components/explore/Navbar.tsx line 45

const publicKey = account?.address  // ❌ WRONG

Stellar Wallet Kit uses publicKey, not address. This should be:

const publicKey = account?.publicKey  // ✅ CORRECT

This bug breaks the entire wallet display feature.


2. CRITICAL - Massive Scope Creep

Issue #236 asks to update only components/explore/Navbar.tsx. This PR changes 17 files with 12,759 additions and 5,427 deletions:

Out of scope changes:

  • package.json - Dependency updates unrelated to Navbar
  • app/globals.css - Changed primary color (line 56)
  • components/explore/Sidebar.tsx - Complete refactor
  • components/explore/home/LiveStreams.tsx - Refactor
  • components/explore/home/TrendingStreams.tsx - Refactor
  • components/footer.tsx - Updates
  • data/landing-page/community.ts - Data changes
  • Image file changes
  • Type definition updates

This violates the single responsibility principle and makes review extremely difficult.

Please:

  1. Revert all changes unrelated to Navbar wallet migration
  2. Create separate PRs for other refactors if needed
  3. Keep this PR focused on ONLY what issue #236 requests

3. DRY Violations in Navbar

Duplicate display name logic:

  • getDisplayName() (lines 48-73)
  • renderDisplayName() (lines 76-112)

Both have nearly identical logic. Consolidate into a single function.

Repeated sessionStorage parsing:
SessionStorage parsing with try/catch appears 3 times (lines 56-65, 93-103, 119-130). Extract to helper function:

function getSessionData<T>(key: string): T | null {
  try {
    const data = sessionStorage.getItem(key);
    return data ? JSON.parse(data) : null;
  } catch {
    return null;
  }
}

4. API Route Rename Risk

Renamed /api/users/wallet/[wallet]/route.ts to /api/users/wallet/[publicKey]/route.ts. This could break existing code that calls the old endpoint. Please:

  1. Search codebase for references to the old path
  2. Update all callers
  3. Document the breaking change

5. No Tests

Zero test coverage for the wallet migration. Add tests for:

  • Navbar with wallet connected/disconnected states
  • Address display formatting
  • Profile fetch with new endpoint

6. PR Checklist Incomplete

The PR description shows several unchecked boxes:

  • ❌ "I am making a pull request against the main branch"
  • ❌ "My commit messages styles matches our requested structure"
  • ❌ "My code additions will fail neither code linting checks nor unit test"
  • ❌ "I am only making changes to files I was requested to"

The last one is particularly concerning given the scope creep.


Summary

Before approval:

  1. Fix the critical bug (account?.publicKey not account?.address)
  2. Remove all out-of-scope changes (keep only Navbar wallet migration)
  3. Fix DRY violations
  4. Verify API route rename doesn't break existing code
  5. Add tests

This PR needs significant rework to match the issue scope.

@chiscookeke11
Copy link
Contributor Author

Alright
Please take a look @davedumto

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