fix(newsletter): profile subscriber row layout and first-class Email digest action - #3523
Conversation
…y card's height channel The profile summary CollapsibleCard measures its content once on first layout; the subscriber row appears only after the sender query resolves, so it grew the content past the measured height and was clipped under the tab bar. The row now has a fixed height and reports its visibility to ProfileSummaryView, which combines it with the VP/RC bars toggle into the ONE moreHeight value the card accepts (the channel is single-valued, so two direct writers would overwrite each other). The count text is single line and shrinks on narrow devices. Closes #3522
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
PR Summary by QodoPrevent profile subscriber row clipping in summary cards
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a71bea7d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { isShowPercentText, senderInfoVisible } = this.state; | ||
| if (handleUIChange) { | ||
| handleUIChange( | ||
| (isShowPercentText ? 30 : 0) + (senderInfoVisible ? NEWSLETTER_SENDER_INFO_HEIGHT : 0), |
There was a problem hiding this comment.
When the own profile is popped and reopened while the sender query remains in the in-memory cache, NewsletterSenderInfo renders its 28-point row during the card's initial layout, so CollapsibleCard already includes that row in contentHeight. The visibility effect then sets senderInfoVisible, and this expression adds the same 28 points again, leaving an extra blank row beneath the summary on repeat visits; only report the delta when the row appeared after the initial measurement, or make the card remeasure its content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c45ed63 by removing the arithmetic entirely: the row moved OUT of the CollapsibleCard and renders as a sibling below it in normal layout flow, so nothing is ever added to moreHeight and a cached first render cannot be counted twice. It follows isSummaryOpen, so collapsing the summary hides it too.
|
|
||
| const styles = EStyleSheet.create({ | ||
| row: { | ||
| height: NEWSLETTER_SENDER_INFO_HEIGHT, |
There was a problem hiding this comment.
With accessibility font scaling enabled, the subscriber count and Manage label can exceed this fixed 28-point height (and the existing IconButton is already 30 points tall). Because the collapsible card is also told to reserve exactly 28 points, large text is clipped or overlaps the following bars; use a measured/minimum height or otherwise include the scaled content's actual height.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c45ed63: the fixed height is gone. As a sibling outside the measured card the row takes its natural height, so scaled text and the 30pt icon simply make it taller instead of clipping.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
|
||
| const styles = EStyleSheet.create({ | ||
| row: { | ||
| height: NEWSLETTER_SENDER_INFO_HEIGHT, |
There was a problem hiding this comment.
1. Row shorter than icon 📎 Requirement gap ≡ Correctness
NEWSLETTER_SENDER_INFO_HEIGHT fixes the row at 28px, but its IconButton child is fixed at 30×30px. The row therefore does not fully accommodate its own content and can still clip or overlap vertically.
Agent Prompt
## Issue description
The fixed subscriber row is 28px high while its link `IconButton` is 30px high, so the row does not fully contain its content.
## Issue Context
Compliance rule 1 requires the asynchronously displayed row to be fully accommodated and not clipped. Increase the exported fixed height or reduce the child size, and keep the reported `moreHeight` synchronized through the shared constant.
## Fix Focus Areas
- src/components/newsletterSenderInfo/newsletterSenderInfo.tsx[20-20]
- src/components/newsletterSenderInfo/newsletterSenderInfo.tsx[72-78]
- src/components/newsletterSenderInfo/newsletterSenderInfo.tsx[87-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in c45ed63: no fixed row height any more; the row sits outside the measured card in normal flow and its natural height contains the 30pt icon.
| paddingTop: 6, | ||
| }, | ||
| countText: { | ||
| flexShrink: 1, |
There was a problem hiding this comment.
2. manage width remains unconstrained 📎 Requirement gap ≡ Correctness
The PR makes the count text shrink, but the Manage action still has no fixed-width container or width constraint. A long localized action label can therefore consume or exceed the available row width on small devices, contrary to the required fixed action area.
Agent Prompt
## Issue description
The subscriber count now shrinks, but the `Manage` action remains content-sized rather than occupying a fixed or otherwise bounded action area.
## Issue Context
Compliance rule 1 requires fixed icon and action areas so localized labels cannot push controls off-screen on small devices. Add an explicit bounded wrapper style for the action while preserving a usable single-line label.
## Fix Focus Areas
- src/components/newsletterSenderInfo/newsletterSenderInfo.tsx[79-81]
- src/components/newsletterSenderInfo/newsletterSenderInfo.tsx[94-103]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in c45ed63: the Manage label is single line with maxWidth 120, and the count text keeps flexShrink with one line, so a long localized action cannot push the row content off narrow screens.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe newsletter sender row now uses natural height and constrained text. ProfileView renders it outside the summary card for qualifying expanded own profiles. ProfileSummaryView no longer tracks sender visibility and reports percentage-bar height changes directly. ChangesNewsletter sender placement
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change fixes the clipped subscriber row and improves small-device layout behavior; with the reported checks passing, no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address clipping, natural row height, collapsed-summary visibility, and small-device text constraints. However, linked issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4 files. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Review round: the fixed 28pt reservation was smaller than the row's own 30pt icon, fought accessibility font scaling and double-counted when cached data rendered the row before the card's one-shot measurement. The row now lives as a SIBLING below the CollapsibleCard in normal layout flow: natural height, no moreHeight arithmetic at all, hidden together with the collapsed summary. The summary view goes back to the plain bars toggle and the Manage label is single line with a max width so a long translation cannot push the actions off narrow screens.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Mirrors the website's mobile layout, where Email digest is a first-class profile action instead of a menu entry: an envelope icon button sits next to Message (compact enough for narrow devices, accessibility-labeled) and the dropdown entry is removed so the action lives in one place. Closes #3524
feruzm
changed the title
1 participant
Closes #3522. Closes #3524. Two profile newsletter UX fixes from owner testing.
Subscriber row clipped by the summary card (#3522)
The own-profile subscriber row rendered inside the profile summary CollapsibleCard, which measures its content ONCE on first layout; the row appears only after the sender query resolves, so it was clipped under the tab bar. A first attempt reserved a fixed height through the card's
moreHeightchannel; review correctly killed it (28pt reservation smaller than the row's own 30pt icon, breaks under font scaling, double-counts when cached data renders before measurement). Final design: the row lives as a SIBLING below the card in normal layout flow with natural height, no height arithmetic at all, hidden together with the collapsed summary viaisSummaryOpen. Small-device hardening: single-line shrinking count text, single-line max-width Manage label.Email digest as a first-class profile action (#3524)
The website's mobile layout shows Email digest in the profile action row; the app buried it in the ••• dropdown. Now an envelope icon button sits next to Message (accessibility-labeled, disabled while the profile loads) opening the digest sheet, and the dropdown entry is removed so the action lives in one place.
Verification:
yarn typecheck0 errors (empty baseline),yarn lint0 errors, full jest suite 927 passed.