Repository navigation
fix: show community takes in post modals - #6520
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Drops the surface guard (isPostPage in the classic layout, !onClose in the focus card) so a post's community take also renders in post modals, with regression tests for the flag-on and flag-off states in both. Small, focused, and the tests assert the accessible region name rather than a test id, which is the right call.
No blocking code defect. Two things I want answered before this ships, and a few small notes.
Questions to resolve before merge
1. The running experiment needs a decision. shouldEvaluate has always been just !!communitySentimentData — the surface check was never part of it. So every modal viewer of a post with a take has already been enrolled without ever being exposed, which means the numbers collected so far are diluted by a cohort that saw control regardless of variation. This PR then changes exposure mid-flight, so the pre-deploy and post-deploy windows are not comparable.
Neither is caused by this diff, and widening exposure is arguably the fix. But please confirm with the experiment owner and reset the analysis start date (or restart the experiment) on deploy, otherwise the readout mixes three different exposure regimes.
2. Blast radius is wider than "post modals". PostContentRaw is shared by SquadPostContent, DigestPostContent, BriefPostContent, PollPostContent, CollectionPostContent and SocialTwitterPostContent; PostFocusCard is used by ArticlePostModal, CollectionPostModal and SharePostModal. With the guard gone, the take renders in every one of those wherever the post carries communitySentiment — including the share modal, where the focus card reads article.communitySentiment, i.e. the shared article's take.
If only articles ever carry sentiment, say so in the description and this is a non-issue. If collections or shares can, please confirm the section is wanted there.
Non-blocking
- Spacing in the classic modal.
CommunitySentimentis given a hardcodedclassName="mb-6"while its immediate neighbourPostCodeSnippetsusesisCompactModalSpacing ? 'mb-4' : 'mb-6'. Now that this renders in modals it should follow the same rule, or the modal gets one section spaced differently from the rest. - No visual verification. The modal is narrower than the page on desktop and near-fullscreen on mobile, and this component was laid out for the page. Screenshots (or a short clip) at desktop and mobile for both the classic modal and the focus-card modal would settle it — this is the kind of thing that comes back as a post-merge fix.
- Duplicated fixture. The ~45-line
postWithCommunitySentimentobject is copy-pasted into both spec files. Move it to__tests__/fixture/post.tsso the two surfaces cannot drift apart. - Coverage gaps. Nothing asserts that a post without a take skips enrollment, and nothing pins the post-page behaviour that the removed guard used to own. Both are cheap to add next to the tests already here.
- CI is still running on this head (lint, shared/webapp tests, build, both Vercel deploys). The local runs listed in the description are not a substitute.
Verification
- Consumers of
PostContentRawandPostFocusCardenumerated - Enrollment semantics traced through
useConditionalFeature(evaluation = exposure) - Flag default confirmed
false;isDevelopmentescape hatch unchanged - Tests read as behaviour assertions, using the accessible region name
- Not verified: modal layout at desktop and mobile widths, which post types can carry
communitySentiment, experiment-owner sign-off
Verdict
Comment — the code is fine. I'd approve once the experiment owner has signed off on the mid-flight exposure change and modal screenshots are attached. Happy to re-review as soon as those land.
Reviewed by AI.
(Posted as a comment review: the review credential is the PR author, so a formal review state is not available.)
| isPostPage && | ||
| !!communitySentimentData && | ||
| (communitySentimentEnabled || isDevelopment); | ||
| !!communitySentimentData && (communitySentimentEnabled || isDevelopment); |
There was a problem hiding this comment.
Worth being explicit about what this does to the running experiment: shouldEvaluate above is only !!communitySentimentData, and never included isPostPage. So modal viewers of a post with a take have been enrolled without ever being exposed — the data collected so far is diluted by a cohort that always saw control. Removing the surface guard here fixes that going forward, but it also changes exposure mid-flight, so the pre- and post-deploy windows are not comparable.
Please confirm with the experiment owner and reset the analysis start date (or restart the experiment) when this deploys, otherwise the readout blends three different exposure regimes.
Separately, the render site a few lines down passes a hardcoded className="mb-6", while the adjacent PostCodeSnippets uses isCompactModalSpacing ? 'mb-4' : 'mb-6'. Now that this renders inside modals it should follow the same spacing rule.
Reviewed by AI.
| // Only when the post actually has a take. `isDevelopment` lets the surface be | ||
| // previewed locally without flipping the committed (always-`false`) flag | ||
| // default. | ||
| const showCommunitySentiment = |
There was a problem hiding this comment.
The removed !onClose guard was the only thing keeping this out of surfaces other than the post page. PostFocusCard is used by ArticlePostModal, CollectionPostModal and SharePostModal, and the classic path's PostContentRaw is shared by the squad, digest, brief, poll, collection and social-Twitter post content components — so the take now renders in all of them whenever the post carries communitySentiment.
Note this reads article.communitySentiment, so inside SharePostModal it surfaces the shared article's take. That may well be what we want; it just is not what the PR description says.
Could you list which post types can actually carry sentiment? If it is articles only, this is a non-issue and worth stating. If collections or shares can, please confirm the section belongs in those modals.
Reviewed by AI.
| import { featureCommunitySentiment } from '../../lib/featureManagement'; | ||
| import { PostContentRaw } from './PostContent'; | ||
|
|
||
| const postWithCommunitySentiment: Post = { |
There was a problem hiding this comment.
This ~45-line fixture is copy-pasted verbatim into PostFocusCard.spec.tsx. Two copies of the same shape will drift the moment communitySentiment gains or renames a field, and then one surface's test silently stops representing reality.
Move it into __tests__/fixture/post.ts next to the existing post/sharePost fixtures and import it in both specs.
Reviewed by AI.
bc93b65 to
2d4993f
Compare
|
Addressed the review feedback in 2d4993f:
Re-ran:
|
rebelchris
left a comment
There was a problem hiding this comment.
Re-review of 2d4993f2a
All three inline items from the previous round are resolved:
- Compact modal spacing —
CommunitySentimentnow takesisCompactModalSpacing ? 'mb-4' : 'mb-6', matching its neighbour. - Fixture deduped —
postWithCommunitySentimentmoved to__tests__/fixture/post.tsand imported by both specs. One source of truth, so the two surfaces can no longer drift. - Experiment exposure documented — the description now states plainly that modal viewers were previously evaluated without seeing the surface, and that the analysis start date must be reset (or the experiment restarted) on deploy. That was the one thing I actually wanted written down; it now needs the experiment owner to act on it at deploy time, not a code change.
The scope section also answers the blast-radius question the right way: rendering is data-gated on the optional Post.communitySentiment payload, so no post without it can render the section or evaluate the flag, and share modals deliberately surface the underlying shared article's take.
Still open (both non-blocking)
- No modal screenshots. The preview is deployed and green. A quick look at the classic modal and the focus-card modal at desktop and mobile width — or just a note that you checked — would close this out; modal-width layout is a recurring source of post-merge fixes and it is the only thing here I cannot verify from the diff.
- Coverage gaps unchanged. Nothing asserts that a take-less post skips evaluation, and nothing pins the post-page behaviour the removed guard used to own. Cheap to add later; not worth holding this.
CI
typecheck_strict_changed, test_extension, build_extension and the webapp deploy are green. lint_shared, test_shared, test_webapp and build are still running.
Verdict
No remaining objections. I'd approve once CI finishes green and you confirm the modal was eyeballed at both widths.
Reviewed by AI.
|
Visual check done with a temporary local Storybook harness and Chrome screenshots:
CI is green on the current head; only |
Tweet posts use the dedicated SocialTwitterPostContent layout, which never rendered the community take. Gate and render it with the same inline conditional-enrollment pattern PostContent and PostFocusCard use on main, including the newer show-in-modal semantics from #6520/#6523. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebased onto main. The source-opening half of this branch shipped separately in #6514 (cover, title and read button all opening the article, with the reader gate, middle-click logging and the share-of-a-native-post predicate). What remains here is the surrounding card work, rebuilt on top of that rather than replacing it. **Read CTA** — a single "Read the full article" pill with a trailing open-link icon, above the TL;DR on mobile and after it from tablet up. It keeps the reader gate, `combinedClicks` and the read logging, and still swaps to the globe icon in the reader variant. Videos and X posts keep their own verb ("Watch video" / "Read on"). Rendered once and repositioned with `flex-col-reverse`, so it is one stop in the accessibility tree rather than two identical copies behind `hidden` wrappers. **Card layout / header** - `PostMetadata` (date · domain · read time) moved into the title column, directly under the title, so a short title pulls it up instead of it sitting below a taller cover. - Cover thumbnail enlarged (w-28 / tablet:w-48) and given the same lift-on-hover motion as the CTA. - Author avatar/name in the header now links to the profile. - Squad attribution covers both cases: "Shared via {squad}" for a post shared into a squad, "Posted in {squad}" for a freeform/welcome post written in one. - A share post's own commentary (`post.title`, when it differs from the article's) is surfaced above the shared article. - Boost CTA — which lived in `PostHeaderActions`, a component the redesign card doesn't render — restored into the header row. **Action bar** — one full-width floating glass bar pinned to the bottom from tablet up. Drops the top-pinned state, the pinned-only counts, the pinned "…" menu, the pinned close X and the copy-link overflow measurement. `BookmarkButton` is kept rather than a plain `CardAction`, so the bookmark-reminder dropdown survives. **SourceStrip** — hover underline applies to the source name itself rather than the whole row. Preserved from main while rebasing: the `withPostById` hydration wrapper and the community-sentiment surface in modals (#6520, #6523), `canReadArticle` testing the shared post rather than the wrapper, and `combinedClicks` / `withSelectionGuard` on the title and cover links. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Changes
Scope
Events
No new tracking events.
Experiment
No new experiment. This changes exposure for the existing community_sentiment experiment: modal viewers of posts with takes were previously eligible/evaluated without seeing the surface, and after this deploy they will see it when assigned treatment. The analysis start date should be reset to this deploy, or the experiment should be restarted, so results do not blend the old and new exposure regimes.
Manual Testing