Skip to content

Add extra scroll to the candlesticks#7

Open
rturtu wants to merge 21 commits intomainfrom
pro-2155-apply-patch-to-lib
Open

Add extra scroll to the candlesticks#7
rturtu wants to merge 21 commits intomainfrom
pro-2155-apply-patch-to-lib

Conversation

@rturtu
Copy link

@rturtu rturtu commented Jan 20, 2026

Pull Request

Summary

Extra scroll to the right.

Mobile PR with testing development builds: https://github.com/elliottech/perps-fe-mobile/pull/1118

Linear ticket: PRO-2155

Author Checklist

  • PR tested locally
  • PR tested on preview

Open with Devin

@rturtu rturtu dismissed sinantalhakosar’s stale review February 10, 2026 14:39

The merge-base changed after approval.

@linear
Copy link

linear bot commented Feb 10, 2026

@rturtu rturtu dismissed sinantalhakosar’s stale review February 12, 2026 12:25

The merge-base changed after approval.

@rturtu rturtu marked this pull request as draft March 15, 2026 19:42
@github-actions
Copy link

github-actions bot commented Mar 15, 2026

✅ PR Checklist Validation Passed

Status: All 2 checklist items completed
Linear ticket: Valid ✅

🎉 This PR meets all requirements and is ready for review!


This validation ensures all PR requirements are met before merging.

@github-actions
Copy link

github-actions bot commented Mar 15, 2026

🚀 Prerelease Published

Version: 0.7.0-rturtu.1137896.pro-2155-apply-patch-to-lib
Tag: pro-2155-apply-patch-to-lib

Update package.json:

"@elliottech/react-native-kline-view": "0.7.0-rturtu.1137896.pro-2155-apply-patch-to-lib"

Previous prereleases for this PR have been cleaned up. This prerelease will be automatically cleaned up when the PR is merged.

devin-ai-integration[bot]

This comment was marked as resolved.

@rturtu rturtu changed the title Pro 2155 apply patch to lib Add extra scroll to the candlesticks Mar 16, 2026
@rturtu rturtu force-pushed the pro-2155-apply-patch-to-lib branch from 40f37e7 to 9888e3a Compare March 19, 2026 08:09
@devin-ai-integration
Copy link

iOS Swift Review

I reviewed the iOS/Swift changes in depth. The minVisibleCandles config addition and the scroll clamping in scrollViewDidScroll / reloadContentOffset look correct on their own. However, there are two critical issues where the auto-scroll-to-end checks weren't updated to match the new content size model, effectively making them dead code.


Bug 1 (Critical): wasAtEnd in addCandlesticksAtTheEnd is always false

HTKLineContainerView.swift:355

let wasAtEnd = klineView.contentOffset.x >= (klineView.contentSize.width - klineView.frame.width - 10)

With the new reloadContentSize adding + bounds.size.width, the threshold becomes itemWidth * count + paddingRight - 10. But scrollViewDidScroll and reloadContentOffset clamp contentOffset.x to at most itemWidth * (count - minVisibleCandles). For wasAtEnd to be true you'd need 10 >= 5 * itemWidth + paddingRight, which is false for any reasonable values.

Impact: When new candles arrive, the chart will never auto-scroll to keep showing the latest data, even if the user was already at the end. (This was flagged by the earlier automated review comment #9 and marked "Resolved", but the iOS code on the branch hasn't actually been updated.)

Suggested fix:

let allCandlesWidth = configManager.itemWidth * CGFloat(configManager.modelArray.count)
let maxAllowedOffset = max(0, allCandlesWidth - configManager.minVisibleCandles * configManager.itemWidth)
let wasAtEnd = klineView.contentOffset.x >= maxAllowedOffset - 10

Bug 2 (Critical): isEnd in reloadConfigManager is always false

HTKLineView.swift:138-145

let lastCandlestickOffset = contentSize.width - configManager.paddingRight - configManager.itemWidth / 2

After reloadContentSize(), contentSize.width = itemWidth * count + paddingRight + bounds.width, so lastCandlestickOffset = itemWidth * count + bounds.width - itemWidth/2. This is off by an entire bounds.width from the actual last candle position (itemWidth * count - itemWidth/2), because contentSize.width now includes the extra + bounds.size.width but this formula wasn't adjusted.

For the lower bound of isEnd to be satisfied at max scroll, you'd need 1.5 * itemWidth <= 1 — false for any candle wider than ~0.7pt.

Impact: On config reload with new data, the chart won't auto-scroll to end even when the user was already looking at the latest candles.

Suggested fix — use raw candle width instead of contentSize.width:

let lastCandlestickOffset = configManager.itemWidth * CGFloat(configManager.modelArray.count) - configManager.itemWidth / 2

Issue 3 (Medium): Inconsistent scroll-to-end target formulas

Three places compute "scroll to end" differently:

  • reloadConfigManager (line 152): contentSize.width - 2 * bounds.width
  • smoothScrollToEnd (line 207): contentSize.width - 2 * bounds.width
  • addCandlesticksAtTheEnd (line 378): contentSize.width - bounds.width

All get clamped by reloadContentOffset so runtime behavior converges, but the inconsistency makes the code harder to reason about. Consider extracting a single computed property.


Issue 4 (Minor): Debug print() statements

HTKLineContainerView.swift has many print() calls that will spam the console in production. Consider removing or guarding behind #if DEBUG.


Bottom line: Bugs 1 and 2 mean the auto-scroll-to-end feature is broken on iOS — the checks are dead code. Both need to be fixed before merging.

@rturtu rturtu marked this pull request as ready for review March 19, 2026 11:37
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 3 new potential issues.

View 9 additional findings in Devin Review.

Open in Devin Review

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.

4 participants