Skip to content

Comments

feat: Add Copy Strategy button to Insights signal cards#494

Merged
aldin4u merged 1 commit intostagingfrom
copy-strategy
Jan 8, 2026
Merged

feat: Add Copy Strategy button to Insights signal cards#494
aldin4u merged 1 commit intostagingfrom
copy-strategy

Conversation

@aldin4u
Copy link
Collaborator

@aldin4u aldin4u commented Jan 8, 2026

Description

  • Added Copy Strategy button to signal cards (positioned left of timestamp, white styling)
  • Button copies trading strategy to clipboard in JSON format
  • Reverted development-only changes (mock data and subscription bypass

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Summary by CodeRabbit

  • New Features

    • Added copy-to-clipboard functionality for trading signals with two-second visual feedback confirmation.
    • Signal copy button now appears in the header adjacent to time/status information.
  • Style

    • Minor formatting and spacing adjustments across components for improved layout consistency.
  • Chores

    • Enhanced internal logging for API responses and signal data initialization.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

📝 Walkthrough

Walkthrough

This pull request enhances the trading signals interface by adding clipboard copy functionality to the SignalCard component, improving logging in the signal fetching hook, and applying minor formatting adjustments across the insights module.

Changes

Cohort / File(s) Summary
Copy Strategy Feature
src/apps/insights/components/SignalCard/SignalCard.tsx
Introduces copyStrategy function to serialize signal data (orderSide, ticker, exchange, entryPrice, stopLoss, tp1/tp2/tp3) to clipboard JSON format. Adds copied state for 2-second feedback display, Copy Strategy button with icon toggle, and imports Copy/Check icons from lucide-react. Minor structural adjustments to header layout.
Initial Load Handling & Logging
src/apps/insights/hooks/useTradingSignals.ts
Adds isInitialLoad to fetchSignals dependency array for proper memoization. Sets isInitialLoad to false post-fetch and introduces verbose logging for API responses and signal structure inspection. No changes to hook public interface.
Formatting & Style
src/apps/insights/index.tsx
Whitespace and blank line adjustments around FeedEventCard and surrounding blocks. Formatting-only changes with no behavioral impact.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit hops through signals bright,
Copying strategies left and right,
With clipboard magic and feedback glow—
Check marks dance, the strategies flow! ✨📋

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete. While it lists the main feature change, it lacks critical details: testing information is entirely blank, screenshots are missing, and the types of changes boxes are unchecked despite this being a new feature. Complete the 'How Has This Been Tested?' section with testing environment and test cases; check the 'New feature' checkbox under 'Types of changes'; optionally add screenshots demonstrating the Copy Strategy button functionality.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a Copy Strategy button to Insights signal cards, which aligns with the primary feature introduced across the modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@aldin4u aldin4u requested review from RanaBug and vignesha22 January 8, 2026 08:17
@cloudflare-workers-and-pages
Copy link

Deploying pillarx-debug with  Cloudflare Pages  Cloudflare Pages

Latest commit: f73c58c
Status: ✅  Deploy successful!
Preview URL: https://276b7883.pillarx-debug.pages.dev
Branch Preview URL: https://copy-strategy.pillarx-debug.pages.dev

View logs

@cloudflare-workers-and-pages
Copy link

Deploying x with  Cloudflare Pages  Cloudflare Pages

Latest commit: f73c58c
Status: ✅  Deploy successful!
Preview URL: https://2db4ed60.x-e62.pages.dev
Branch Preview URL: https://copy-strategy.x-e62.pages.dev

View logs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @src/apps/insights/components/SignalCard/SignalCard.tsx:
- Around line 42-61: The copyStrategy function currently hardcodes the exchange
as "BINANCE"; update it to use dynamic data by reading the exchange from the
signal object (e.g., signal.exchange or signal.exchange_name) and fall back to a
module-level default constant (e.g., EXCHANGE_DEFAULT) or a config value if the
signal exchange is missing; ensure the identifier copyStrategy is changed to
reference that dynamic value when building the strategy object and keep the same
fallback behavior to avoid breaking copies for older signals.
🧹 Nitpick comments (1)
src/apps/insights/hooks/useTradingSignals.ts (1)

30-70: Review the dependency array inclusion of isInitialLoad.

Adding isInitialLoad to the useCallback dependency array (line 70) will cause fetchSignals to be recreated when isInitialLoad transitions from true to false. However, isInitialLoad is only written inside the callback (lines 60-62) and not used for any conditional logic within it.

This creates an unnecessary dependency that triggers callback recreation. Consider whether this behavior is intentional:

  • If you only need to track initial load completion, isInitialLoad can be safely removed from the dependency array
  • The state update inside the callback will work correctly without this dependency

The verbose logging additions (lines 32-54) are helpful for debugging and provide good visibility into the signal fetching process.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 345fcd9 and f73c58c.

📒 Files selected for processing (3)
  • src/apps/insights/components/SignalCard/SignalCard.tsx
  • src/apps/insights/hooks/useTradingSignals.ts
  • src/apps/insights/index.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-03T10:01:15.801Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 471
File: src/apps/pillarx-app/components/AlgoInsightsTile/AlgoInsightsTile.tsx:36-48
Timestamp: 2025-12-03T10:01:15.801Z
Learning: In the AlgoInsightsTile component (src/apps/pillarx-app/components/AlgoInsightsTile/AlgoInsightsTile.tsx), the data is always hardcoded and guaranteed to be present with complete history arrays for all timeframes, so edge case handling for empty or sparse history data is not required.

Applied to files:

  • src/apps/insights/index.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.

Applied to files:

  • src/apps/insights/components/SignalCard/SignalCard.tsx
🧬 Code graph analysis (2)
src/apps/insights/index.tsx (1)
src/apps/insights/components/FeedEventCard/FeedEventCard.tsx (1)
  • FeedEventCard (16-100)
src/apps/insights/hooks/useTradingSignals.ts (2)
src/apps/insights/api/insightsApi.ts (1)
  • getTradingSignals (46-65)
src/apps/insights/types/index.ts (1)
  • TradingSignal (5-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: unit-tests
  • GitHub Check: lint
  • GitHub Check: build
  • GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (2)
src/apps/insights/index.tsx (1)

127-127: LGTM! Formatting improvements with safe error handling.

The changes consist primarily of spacing improvements for readability. The empty catch blocks on lines 309 and 328 appropriately suppress errors during subscription refresh operations, preventing UI disruption when background checks fail.

Also applies to: 165-165, 191-191, 197-197, 201-201, 206-206, 262-262, 270-270, 309-309, 328-328, 646-651

src/apps/insights/components/SignalCard/SignalCard.tsx (1)

136-159: LGTM! Well-implemented copy functionality.

The Copy Strategy button provides good UX with:

  • Visual feedback via state toggle (Check icon + "Copied!" text)
  • Appropriate 2-second timeout for feedback reset
  • Error handling with console logging
  • Conditional rendering only for open signals

Copy link
Collaborator

@IAmKio IAmKio left a comment

Choose a reason for hiding this comment

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

LGTM

@aldin4u aldin4u merged commit e822a37 into staging Jan 8, 2026
7 checks passed
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