Skip to content

fix(token-detail): rest the sheet on the top of the chart - #5982

Merged
johnnyluo merged 1 commit into
mainfrom
aminsato/5959_token_sheet_chart_peek
Sep 27, 2026
Merged

johnnyluo merged 1 commit into
mainfrom
aminsato/5959_token_sheet_chart_peek

Conversation

@aminsato

@aminsato aminsato commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #5959

Summary

  • The token detail sheet rested on the balance and the action row only. The sheet's 48dp cut-edge fade covered the 40dp gap under the actions, so nothing showed below them and the sheet did not look scrollable.
  • When the token has a chart, the resting height now reaches past the chart card's price header, so the fade falls across the top of the chart line.
  • The extra height is added at composition, not in the onSizeChanged callback. The chart arrives after the block above it has been measured, and without a size change that callback never fires again.
  • A coin with no chart keeps the old resting height. ExpandingBottomSheet is unchanged, so expand-on-scroll and pull-down-to-dismiss behave as before.

Note: the issue says iOS rests this sheet at half the screen. On iOS main, CoinDetailScreen uses .presentationDetents([.large]), so this follows the issue's own fix (peek the top of the chart) rather than a half-screen fraction.

Before / After

Before After (chart) After (no chart)
before after no chart

Test plan

  • ./gradlew assembleDebug
  • Emulator: a charted token opens with all five action labels visible, and the chart header plus the top of the line below them
  • Emulator: a token with no chart opens at the previous height, with no empty gap
  • Large font scale on a device

Summary by CodeRabbit

  • Improvements
    • The token details sheet now rests at a height that reveals part of the chart when one is available, making it easier to preview the chart without fully expanding the sheet.
    • When no chart is available, the sheet rests at the actions section.

The resting height stopped at the action row, and the cut-edge fade covered the 40dp gap
under it, so nothing showed below the actions and the sheet did not read as scrollable.
When a chart is present, the resting height now reaches past the card's price header so
the fade falls across the top of the chart line. The peek is added at composition rather
than in the size callback because the chart arrives after the block above it is measured.
Coins without a chart keep the previous resting height.
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: vultisig/vultisig-android/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: dfbe5351-017f-45c8-a727-16781e2060c4

📥 Commits

Reviewing files that changed from the base of the PR and between dd59d28 and bce35fa.

📒 Files selected for processing (1)
  • app/src/main/java/com/vultisig/wallet/ui/screens/TokenDetailScreen.kt

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.


📝 Walkthrough

Walkthrough

The token detail sheet now rests at the measured actions block height plus a chart preview when a chart exists. Without a chart, the preview adds no height. The chart gap and preview height use named constants.

Changes

Token sheet chart preview

Layer / File(s) Summary
Actions measurement and resting height
app/src/main/java/com/vultisig/wallet/ui/screens/TokenDetailScreen.kt
The content reports the measured actions bottom. The sheet adds the 72 dp chart preview when a chart exists. The chart spacer uses the 40 dp ChartGap constant.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to bce35

No actionable merge-blocking risk was established; normal validation should still include large-font-scale coverage.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adjusting the token detail sheet to rest at the top of the chart.
Linked Issues check ✅ Passed The changes meet the coding requirements in [#5959]. TokenDetailScreen measures the action block and adds ChartGap + ChartPeek only when uiModel.chart exists. This exposes the chart preview whil…
Out of Scope Changes check ✅ Passed The reviewed change is limited to the Android token detail screen's resting-height measurement and chart spacing. It does not change ExpandingBottomSheet, expanded content order, or iOS code. The co…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

@johnnyluo johnnyluo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@johnnyluo
johnnyluo added this pull request to the merge queue Sep 27, 2026
Merged via the queue into main with commit 4a590e3 Sep 27, 2026
4 checks passed
@johnnyluo
johnnyluo deleted the aminsato/5959_token_sheet_chart_peek branch September 27, 2026 11:02
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.

[Fix] Token sheet should peek the chart so it reads as scrollable [Token detail]

2 participants