Akmal / feat: language fixes

[akmal-deriv]

Description

(Brief description of what this PR does)

Related Issue: Fixes #(issue)

Type of Change

• 🐛 Bug fix
• ✨ New feature
• 💥 Breaking change
• 🔧 Refactoring
• 📚 Documentation
• 🧪 Tests

Testing

• Unit tests pass
• Manual testing completed
• No console errors
• No test needed

Screenshots

(Add screenshots if UI changes)

Before:

After:

[cloudflare-workers-and-pages]

Deploying derivatives-trader with    Cloudflare Pages

Latest commit:	678bf6c
Status:	✅  Deploy successful!
Preview URL:	https://f83d3af4.derivatives-trader.pages.dev
Branch Preview URL:	https://b-language-issues-4.derivatives-trader.pages.dev

View logs

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

[claude]

Claude finished @akmal-deriv's task —— View job

---

Code Review Complete

Reviewing PR #402: "Akmal / feat: language fixed"

---

Overview

This PR addresses language-related issues across 4 files with 3 key changes focused on error message handling and conditional logic cleanup.

Changes Summary

1. purchase-fieldset.tsx (Line 170)

Change: Conditional message display in Popover

- message={info.message}
+ message={info.has_error ? info.message : undefined}

✅ GOOD: Prevents displaying messages when there's no error, improving UX.

2. contract-info.tsx (Lines 145-147)

Change: Restricted info Popover to vanilla contracts only

- {!is_multiplier && !is_accumulator && !isMobile && (
+ {is_vanilla && (

⚠️ MEDIUM PRIORITY: This change significantly reduces when the info popover is shown.

Concerns:

• Previously shown for: non-multiplier, non-accumulator, desktop contracts
• Now shown for: only vanilla contracts
• Missing coverage: Turbos, high/low, rise/fall, and other contract types no longer show the info popover
• The TODO comment suggests this needs further review

Question: Is this intentional? Should other contract types also show the info popover on desktop?

3. proposal.ts (Lines 128-130)

Change: Removed proposal.longcode from message fallback

- message: proposal.longcode || mapErrorMessage(response?.error || {}),
+ message: mapErrorMessage(response?.error || {}),

⚠️ HIGH PRIORITY: This is a breaking change in message behavior.

Concerns:

• proposal.longcode previously provided human-readable contract descriptions (e.g., "Win payout if EUR/USD is strictly higher than entry spot at 5 minutes after contract start time")
• Now only error messages are shown, even for valid proposals
• This means users may not see important contract details in the UI where proposal_info.message is displayed
• The TODO comment acknowledges this needs a decision for accumulator/multiplier, but the change affects all contract types

Impact Analysis: Need to verify where proposal_info.message is used:

• If used only for error display: ✅ OK
• If used for general info display: ❌ Breaking change

4. trade-store.ts (Line 1950)

Change: Removed redundant is_accumulator check

- if (!this.is_dtrader_v2 && !this.is_accumulator) {
+ if (!this.is_dtrader_v2) {

✅ GOOD: Cleaner logic. The is_accumulator check was likely redundant if the v2 check already handles accumulator scenarios.

Verification needed: Confirm that dtrader_v2 mode already excludes accumulators, or that accumulators should indeed go through processNewValuesAsync in languageChangeListener().

---

Priority Issues

🔴 High Priority

1. proposal.ts message change - Verify this doesn't break contract info displays
   • Check all usages of proposal_info.message in the codebase
   • Ensure longcode information is still available elsewhere when needed
   • Consider language translation implications (longcode may contain translatable content)

🟡 Medium Priority

2. contract-info.tsx popover restriction - May reduce information availability
   • Verify if turbos, high/low, and other contract types need the info popover
   • The TODO comment suggests this needs review - should be addressed before merging

🟢 Low Priority

3. Missing PR description - The PR template wasn't filled out
   • Add description of what issue this fixes
   • Specify testing done
   • Add context about the language-related issues being addressed

---

Code Quality

Strengths ✅

• Clean, focused changes
• Good use of TODO comments to mark areas needing future work
• Defensive programming with conditional checks

Concerns ⚠️

• Two changes have TODO comments suggesting they're temporary solutions
• The PR title "language fixed" doesn't clearly describe what language issues were addressed
• No test coverage mentioned or visible in the changes

---

Testing Recommendations

Please verify:

1. All contract types display correctly (not just vanilla):
   • Multipliers
   • Accumulators
   • Turbos
   • Rise/Fall
   • High/Low
   • Touch/No Touch
2. Error messages display correctly when proposals fail
3. Language switching works properly (given the "language fixed" title):
   • Switch between languages and verify contract info displays correctly
   • Check that error messages are properly translated
4. Desktop vs Mobile - info popovers show appropriately
5. Edge cases:
   • What happens when there's a valid proposal with no error?
   • Is contract description information still visible to users?

---

Security & Performance

• ✅ No security concerns identified
• ✅ No performance concerns (changes are display logic only)

---

Recommendations

1. Address the TODOs - Either resolve them in this PR or create follow-up tickets
2. Provide more context - What specific language issues were these changes fixing?
3. Consider rollback strategy - If longcode removal causes issues, have a plan to revert
4. Add tests - Unit tests for the conditional logic would prevent regressions

---

Final Assessment

Overall: The changes are well-intentioned but have potential for unintended side effects. The removal of proposal.longcode is particularly concerning as it may hide useful information from users.

Recommendation: ⚠️ Request Changes - Need clarification on the impact of removing longcode and restricting the info popover before approving.

Suggested next steps:

1. Verify proposal_info.message usage across the codebase
2. Confirm the contract-info popover restriction is intentional
3. Add more descriptive commit messages/PR description
4. Manual testing of all contract types with language switching

---

Tasks

• Gather PR changes and context
• Review purchase-fieldset.tsx
• Review contract-info.tsx
• Review proposal.ts
• Review trade-store.ts
• Provide comprehensive review summary

[claude]

Claude finished @akmal-deriv's task —— View job

---

Code Review Complete

Reviewing PR #402: "Akmal / feat: language fixed"

---

Overview

This PR addresses language-related issues with 3 key changes across 5 files. The changes focus on message handling, conditional popover rendering, and cleanup of redundant logic. While the changes are well-intentioned, there are critical concerns that need to be addressed before merging.

---

Changes Analysis

1. proposal.ts (Lines 128-130) - Message Handling

Change: Removed proposal.longcode from message fallback

- message: proposal.longcode || mapErrorMessage(response?.error || {}),
+ message: mapErrorMessage(response?.error || {}),

🔴 HIGH PRIORITY - Critical Breaking Change

Concerns:

• proposal.longcode previously provided human-readable contract descriptions like:
  • "Win payout if EUR/USD is strictly higher than entry spot at 5 minutes after contract start time"
  • "You will receive a payout at expiry if the spot price never touches or breaches the barrier throughout the contract duration"
• Now only error messages populate proposal_info.message, even for successful proposals
• When response.error is empty/undefined, mapErrorMessage({}) returns an empty string or default message
• This means users lose important contract information that was previously displayed

Impact Assessment:

• If proposal_info.message is used only for error display: ✅ Acceptable
• If proposal_info.message is used for general contract info: ❌ Breaking change - users lose contract details

Where is this used?
Looking at the codebase:

1. purchase-fieldset.tsx:156 - Used in error popover: message={info.message}
2. purchase-fieldset.tsx:170 - Used in multiplier popover: message={info.has_error ? info.message : undefined} (now conditional)
3. contract-info.tsx:82,86,154 - Used in setHintMessage() function

The problem: In contract-info.tsx, the setHintMessage() function returns message for non-vanilla contracts:

const setHintMessage = () => {
    if (is_vanilla) return vanilla_payout_message;
    return message; // This now returns empty string for successful proposals!
};

Recommendation:

• ⚠️ Request Changes - This breaks contract info display for successful proposals
• Need to either:
  1. Restore proposal.longcode OR
  2. Update setHintMessage() to handle empty messages OR
  3. Provide alternative source for contract descriptions

---

2. contract-info.tsx (Lines 145-147) - Popover Restriction

Change: Restricted info popover to vanilla contracts only

- {!is_multiplier && !is_accumulator && !isMobile && (
+ {is_vanilla && (

🟡 MEDIUM PRIORITY - Reduced Information Availability

Concerns:

• Before: Info popover shown for all non-multiplier, non-accumulator, desktop contracts
• After: Info popover shown only for vanilla contracts
• Lost coverage: Turbos, Rise/Fall, Higher/Lower, Touch/No Touch, Digits, and other contract types

Questions:

1. Is this intentional or an oversight?
2. Do other contract types need access to the info popover?
3. The TODO comment suggests this needs further review - should it be resolved before merging?

Additional concern: Combined with change #1 (removing longcode), users of non-vanilla contracts now have:

• ❌ No info popover
• ❌ No longcode descriptions
• Result: Significant loss of contract information

Recommendation:

• Clarify the intent: Should other contract types have info popovers?
• If yes, consider: {(is_vanilla || is_turbos || is_high_low) && !isMobile && (
• Address the TODO before merging

---

3. purchase-fieldset.tsx (Line 170) - Conditional Message Display

Change: Only show popover message when there's an error

- message={info.message}
+ message={info.has_error ? info.message : undefined}

✅ GOOD CHANGE - Defensive Programming

Benefits:

• Prevents empty or irrelevant messages from showing in popovers
• Improves UX by only showing meaningful information
• Works well with change build(deps-dev): bump webpack-dev-server from 5.2.0 to 5.2.1 #1 if message is meant to be error-only

Note: This change is consistent with the removal of longcode from message, suggesting the intent is to make message error-only. However, this conflicts with usage in contract-info.tsx (see concern #1).

---

4. trade-store.ts (Line 1950) - Redundant Check Removal

Change: Removed is_accumulator condition

- if (!this.is_dtrader_v2 && !this.is_accumulator) {
+ if (!this.is_dtrader_v2) {

✅ GOOD CHANGE - Code Cleanup

Reasoning: If is_dtrader_v2 already handles accumulator scenarios, the additional check is redundant.

Verification needed:

• Confirm that v2 mode includes accumulators OR
• Confirm that accumulators should go through processNewValuesAsync in languageChangeListener()

Likely safe based on the commit message and context.

---

5. contract-info.spec.tsx - Test Removal

Change: Removed test for rendering component with children

⚠️ CONCERNING - Reduced Test Coverage

The removed test was:

it('should render component with children', () => {
    render(<ContractInfo {...default_mock_props} />);
    expect(screen.getByText(test_message)).toBeInTheDocument();
});

Why this matters:

• This test was verifying that proposal_info.message gets displayed
• After change build(deps-dev): bump webpack-dev-server from 5.2.0 to 5.2.1 #1, message no longer contains longcode for successful proposals
• The test failed because message is now empty for non-error proposals
• Instead of fixing the logic, the test was removed

This is a red flag: When tests fail after a change, we should:

1. Fix the implementation OR
2. Update the test to reflect new intended behavior

Removing the test hides the problem rather than solving it.

Recommendation:

• ❌ Don't just remove failing tests
• Either restore the test with updated expectations OR
• Add a new test that validates the new behavior (error-only messages)

---

Cross-Cutting Concerns

Correctness & Regressions

• ❌ Major regression: Users lose contract description information for successful proposals
• ❌ Inconsistent usage: message is treated as error-only in some places but general-purpose in others
• ⚠️ Test removal: Hiding test failures instead of fixing root cause

Code Quality & Readability

• ✅ Good: Clear conditional logic in purchase-fieldset.tsx
• ⚠️ TODO comments: Two TODO comments suggest incomplete work
• ⚠️ Inconsistent intent: Changes suggest message should be error-only, but usage in contract-info.tsx contradicts this

Performance

• ✅ No performance concerns - these are display-logic changes only

Security

• ✅ No security concerns identified
• Changes don't introduce XSS, auth, or secrets issues

Tests

• ❌ Critical issue: Test was removed to hide failure
• ❌ No new tests: No tests added for the new conditional behavior
• ⚠️ Coverage gap: Need tests for error vs non-error message handling

---

Testing Recommendations

Must test before merging:

1. All contract types (not just vanilla):

   • Multipliers - verify error messages show correctly
   • Accumulators - verify growth rate displays
   • Turbos - verify barrier information displays
   • Rise/Fall - verify contract details visible
   • Higher/Lower - verify strike information displays
   • Touch/No Touch - verify barrier information displays
   • Digits - verify digit prediction info displays
   • Vanilla - verify payout per point info shows

2. Error scenarios:

   • Invalid barrier - error message displays in popover
   • Insufficient balance - error message displays
   • Market closed - appropriate message shows
   • Invalid duration - error message displays

3. Language switching:

   • Switch languages during active proposal
   • Verify contract info updates correctly
   • Verify error messages are translated

4. Desktop vs Mobile:

   • Desktop: Info popovers show appropriately
   • Mobile: Contract info displays in compact view
   • Error messages display correctly on both

5. Edge cases:

   • Valid proposal with no error - what displays in info popover?
   • Proposal loading state - no stale messages shown
   • Rapid contract type switching - messages update correctly

---

Architecture Concerns

Semantic Confusion

The message field in proposal_info is now semantically confused:

• Some code treats it as error-only (purchase-fieldset.tsx line 170)
• Some code treats it as general-purpose (contract-info.tsx line 86)

Better approach:

// Instead of:
message: mapErrorMessage(response?.error || {})

// Consider:
error_message: mapErrorMessage(response?.error || {}),
description: proposal.longcode || '',

// Then use appropriately:
{info.has_error ? info.error_message : info.description}

This makes the intent explicit and prevents confusion.

---

Priority Summary

🔴 High Priority - Must Fix Before Merge

1. Restore contract descriptions - Users are losing important information
   • Either restore proposal.longcode OR provide alternative source
   • Update setHintMessage() to handle empty messages appropriately
2. Fix test coverage - Don't hide test failures
   • Restore test with correct expectations OR add new tests for error-only behavior

🟡 Medium Priority - Should Address

3. Clarify popover restriction - Is vanilla-only intentional?
   • Address the TODO comment in contract-info.tsx
   • Document why other contract types don't get info popovers
4. Semantic clarity - Separate error messages from descriptions
   • Consider renaming or splitting the message field

🟢 Low Priority - Nice to Have

5. Complete PR description - Template not filled out
   • Add description of what language issues were fixed
   • Specify testing completed
   • Link to related issue

---

Security & Performance ✅

• Security: No concerns identified
• Performance: No concerns identified (display logic only)

---

Final Recommendation

⚠️ REQUEST CHANGES

Reasoning:

1. Breaking change: Removal of proposal.longcode likely breaks contract info display
2. Hidden test failure: Test removed instead of fixing root cause
3. Incomplete work: Two TODO comments suggest decisions still pending
4. Unclear intent: What specific language issues does this fix?

Before approving, need:

1. ✅ Confirm proposal_info.message usage won't break contract info display
2. ✅ Restore test coverage with appropriate expectations
3. ✅ Clarify intent of popover restriction (vanilla-only)
4. ✅ Manual testing of all contract types with language switching
5. ✅ Address or remove TODO comments

If these concerns are addressed, the changes represent good defensive programming and error handling improvements.

---

Positive Notes ✨

Despite the concerns, there are good aspects:

• ✅ Defensive programming with conditional checks
• ✅ Code cleanup (redundant condition removal)
• ✅ Clear intent to improve error message handling
• ✅ Good use of TODO comments to mark incomplete decisions

With the issues addressed, this will be a solid improvement to error handling!

---

Suggested Fix (Quick Patch)

If you want to keep the error-only approach for message, update contract-info.tsx:

const setHintMessage = () => {
    if (is_vanilla) return vanilla_payout_message;
    // Don't return message for non-vanilla, as it's now error-only
    return turbos_payout_message; // or appropriate fallback
};

Or in proposal.ts, restore the longcode but rename for clarity:

error_message: mapErrorMessage(response?.error || {}),
description: proposal.longcode || '',

---