Repository navigation
fix(feed): keep inspired-by credit off the video player - #9173
Conversation
This comment has been minimized.
This comment has been minimized.
6b7d07c to
1f32e18
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Overlap heads-up: this PR intersects #9113, #9112, and draft #8540 across ARB files and generated localization output. Please coordinate merge order. Whichever lands later should rebase onto fresh origin/main, regenerate localization output rather than resolving generated files by hand, and verify that inspired-by display copy stays removed without regressing auth or Lists copy. |
|
The player row is gone, the published event keeps its attribution line, and every creator the removed row used to name stays visible in About. Three commits went on top of Changes I pushed
Checks
Over to you@NotThatKindOfDrLiz please take another look at the three commits and decide on merge. A tip-to-tip run of the async-safety baseline guard against current Pending review requests, still on the old head: @divinevideo/reviewers, @rabble, @realmeylisdev. |
This comment has been minimized.
This comment has been minimized.
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
The player row is gone, the published event keeps its attribution line, and every creator the removed row named stays visible in About. The final state of the branch is 4c417f7..ce84dfc; this approval covers ce84dfc.
Changes on top of the author's branch
3a5d757keeps clip-source credits in About. The removed feed row was the only surface for a creator named by an additionalclip-sourcetag, so a multi-clip reuse would have lost that credit entirely.4323f4dkeeps an npub credit beside an inspiring video. The primary attribution getter is first-wins, so a video that also carries anireference dropped that creator; About now decodes and adds it.ce84dfcleaves legacy reply credits in About. Replies withinspired-byp-tags render as they did before this change, and only the new npub and clip-source sources stay off replies, matching how a reply shows its parent context.
Checks
dart format --output=none --set-exit-if-changed lib test integration_test tools: 0 changed.flutter analyze lib test integration_test tools: no issues.- Affected widget and ARB tests: 259 passed, including four new regression tests for the credited-creator cases.
flutter testinmobile/packages/models: 976 passed. - CI on
ce84dfc: all required and relevant checks pass, including Mobile CI, Generated Files, Tests (shards 0-3/4), Integration Tests (services), Analyze, Format, Goldens, Models build, semantic-pull-request, VGV Tag Gate, and license/cla. The only non-passing entry is the daily service-test result publish, which runs only on the scheduled workflow.
Over to you
@NotThatKindOfDrLiz the branch merges cleanly with main and is ready for your merge decision. Your merge-order note about #9113, #9112, and #8540 still applies. Pending review requests on this head: @divinevideo/reviewers, @rabble, @realmeylisdev.
rabble
left a comment
rabble
left a comment
There was a problem hiding this comment.
Requesting changes for the unresolved items below.
Reviewed commit ce84dfc86634f3bb9bcf4b60cacfab50f772cb5b.
REST-loaded videos lose their only visible inspired-by credit. The display getter strips attribution unconditionally, but VideoStats.toVideoEvent copies the REST description without populating inspiredByNpub/inspiredByPubkeys/inspiredByVideo. About reads those fields, so it has no replacement credit to show.
An executable Flutter probe used a valid synthetic npub in a REST description. The raw content retained the attribution, displayContent became only the caption, all inspired-by fields were empty, and clip-source credits were zero. The credited-creator assertion failed. Before this PR the caption retained the credit. Parse attribution on REST conversion/model access or condition suppression on equivalent About credit, then add a conversion regression test.
Existing focused model tests passed: 86. Exact-head CI is green, including Models and app checks. Widget suites were not rerun locally. Prior populated-model fixes for extra clip-source creators, an independent npub beside an inspiring-video credit, and legacy reply credits are present. Localization overlap still requires merge coordination. Current merge state is clean.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Found two behavior regressions worth a look before merge, verified against ce84dfc8:
1. Collaborator/reply UI can silently disappear for captionless videos
mobile/packages/models/lib/src/video_event.dart:1133 — displayContent now strips the trailing Inspired by nostr:npub1... line via stripInspiredByAttribution(). For a video whose entire content was that attribution line (a captionless publish), displayContent now returns '' where it previously returned the raw string.
In mobile/lib/widgets/video_feed_item/video_feed_item.dart, CollaboratorAvatarRow (line 605) and VideoReplyParentLink (line 610) are both nested inside if (hasTextContent) ...[ (line 540) — pre-existing structure this PR doesn't touch (its diff to this file only removes the old InspiredByAttributionRow from that same block). hasTextContent derives from descriptionText = video.displayContent.trim().
So a captionless video with a collaborator tag or a reply parent, whose only content was the now-stripped attribution line, loses its collaborator row / reply-parent link in the feed overlay — gated behind an unrelated caption-emptiness check as a side effect.
2. Malformed inspiredByNpub now fails silently
mobile/lib/widgets/video_feed_item/metadata/metadata_user_chips.dart:271 — npubToHexOrNull(video.inspiredByNpub) swallows a decode failure with no logging (traced through normalizeToHex → normalizePublicIdentifier, which does catch (e) { return null; }). The deleted inspired_by_attribution_row.dart logged Log.warning('Failed to decode inspiredByNpub ...') on the same failure path. A corrupted npub in production is now undiagnosable from logs.
Two smaller, non-blocking notes:
metadata_user_chips.dart:252(_creditedPubkeys) re-implements theisVideoReplygate thatVideoEvent.inspiredByCreatorPubkey/hasInspiredByalready centralize — worth folding into a model-layer getter so the policy lives in one place, but not a live bug today.VideoEvent.hasInspiredBy(video_event.dart:1065) has no remaining production call sites afterInspiredByAttributionRow's removal — only two doc-comment mentions. Worth confirming it's meant to stay as public API vs. cleaning up alongside the rest of this PR's teardown.
The publish-side writer hardcoded 'Inspired by nostr:' separately from the regex the display side strips with, so a wording change on either side would have published lines the app could no longer read back. Both now derive from inspiredByAttributionPrefix, and a round-trip test pins the writer's output to the parser.
The parser accepted any npub1... token and the display side stripped the line regardless, so a bad-checksum or foreign-client attribution vanished from the caption while crediting nobody in About. Parsing and stripping now share one decodability check: an undecodable line is neither parsed nor hidden, matching what the caption showed before this branch. The fixtures that used a synthetic, non-decoding npub now use real encodings, which also retires two assertions that could not fail: the semantics identifier nothing emits any more, and a textContaining check that only held because the fixture npub could not render as a mention.
A caption typed with a trailing newline publishes as 'text\n\n\nInspired by ...', and stripping the line left 'text\n'. The overlay trims that away but the About sheet counted it as a description, so the two surfaces disagreed about whether the video had one.
ce84dfc to
6fd7668
Compare
This comment has been minimized.
This comment has been minimized.
VideoStats.toVideoEvent copied the REST description into content without parsing the trailing attribution line, so once displayContent stripped it, a REST-loaded video had no inspired-by credit anywhere: the caption hid it and About had no field to read. Parse it on conversion exactly as fromNostrEvent does.
6fd7668 to
ab9e500
Compare
This comment has been minimized.
This comment has been minimized.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
@NotThatKindOfDrLiz I pushed eight commits on top of ce84dfc and rebased the branch onto fresh origin/main (no conflicts; flutter gen-l10n produced no diff after the rebase). Please look the new head over before merging. @rabble your blocker is fixed in the REST commit below; @dcadenas your approval covered ce84dfc, so the head has moved under it.
Review items
Fixed:
- rabble: REST-loaded videos lose their only visible credit.
afe4bbac1parses the trailing attribution line inVideoStats.toVideoEventexactly asfromNostrEventdoes, so About has the credit thatdisplayContentnow strips from the caption. Conversion test added. - realmeylisdev: malformed npub fails silently.
19e5c8134makes parsing and stripping share one decodability check. A line whose npub does not decode is neither parsed nor hidden, so the caption shows what it showed before this branch. TheNip19decode error is still logged by the SDK; nothing inmodelscan name the event, and the raw line staying visible is the user-facing signal. - realmeylisdev:
_creditedPubkeysre-implements the reply gate;hasInspiredByhas no readers.fb056cbcbmoves the rule ontoVideoEvent.creditedInspiredByPubkeys(inspiring video, content reference, p-tags, clip credits; deduped; replies keep only legacy p-tags). The widget renders the list.hasInspiredByandinspiredByCreatorPubkeyare removed with their tests rewritten against the list. Note the About chip order is now fixed to that sequence; it was[primary, npub, clip credits, p-tags]before.
Escalated (product call, left as-is):
- A reply that carries the attribution line loses its credit everywhere. The picker is not gated on replies and the publisher appends the line for replies, but
displayContentstrips it, About skips the npub on a reply, and replies never get p-tags. Pre-branch the caption rendered the line as a tappable mention. Either replies should credit the npub too (one line increditedInspiredByPubkeys, and theadds no new credits to a replytest flips) or the picker should be hidden for replies. Your call which. - realmeylisdev: captionless videos lose the collaborator row and reply-parent link. Both sit inside the
if (hasTextContent)block invideo_feed_item.dart, and a caption that was only the attribution line now strips to empty. The structure predates this branch, and hoisting those two rows out of the caption block would also start showing them on videos that never had a caption, so I did not change it. If you want that, it is a small move.
Other changes (not raised in a review)
d2211aae9builds the publisher's line and the parser's regex from oneinspiredByAttributionPrefix, with a round-trip test, so the two cannot drift apart. Also refreshes a test comment that still described the deleted edit-form regex.f08afa1e9trims the caption after stripping, so a caption typed with a trailing newline does not leave'text\n'that the About sheet counts as a description while the overlay trims it away.fc12b42fasavesdisplayContentinstead of raw content for a saved sound's source context, so the Library card and its search index no longer carry the wire-format line.ff948206aresolves theInspired bylabel from l10n in the reply test and adds a positive control, replacing a hardcoded-EnglishfindsNothingthat could not fail.ab9e50093computesdisplayContentonce perVideoEvent(late final) instead of running the regex and zalgo stripper on every read; the grid reads it three times per tile.- The fixtures that used the non-decoding
npub1syntheticcreator...now use real encodings, which retired two assertions that could not fail: a semantics identifier nothing emits any more, and atextContaining('Inspired by nostr:')that only held because that npub could not render as a mention.
Validation
Local, on the rebased head:
dart format --set-exit-if-changedover every changed Dart file: clean.flutter analyze lib test integration_testinmobile/, andflutter analyzeinsidepackages/modelsunder its own options: no issues.flutter testinpackages/models: 984 passed.packages/categories_repository: 18 passed.- Every app suite that references inspired-by,
displayContent, saved sounds,toVideoEventor the About section (37 files,DIVINE_STRICT_CHANNELS=true): 948 passed. - CI-only ratchets run by hand: placeholder tests, unpinned unchanged assertions, ungrouped tests, shared-setup stubs, l10n delegates, legacy design imports, Nostr id truncation, pubkey log encoding, empty catch, untested services, orphaned ARB keys, raw TextStyle, async safety: all pass. Two of those failed before the rebase only because the branch still carried files #9113 deleted on
main; the rebase resolved them.
CI on ab9e50093: Mobile CI (Analyze, Format, Generated Files, Goldens, Tests shards 0-3, VGV Tag Gate), Models CI, Mergeability, semantic-pull-request and the PR preview build all pass. Integration Tests (services): pass.
The first push, 6fd76687b, failed Models CI on a prefer_const_literals_to_create_immutables info in the new video_stats_test.dart case; that fix is folded into the REST commit.
Pre-existing gap, not introduced here: REST-loaded videos still carry no inspired-by p-tag or clip-source credits, since VideoStats never parsed those markers. Only the content line was affected by this branch.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Approving ab9e50093. Every review finding that had a clear in-repo fix is pushed and green; see my handback review above for the per-commit breakdown and validation. Two items stay open as product calls and do not need to land first: whether a reply should credit the npub it publishes, and whether the collaborator row and reply-parent link should show on captionless videos. Both are one small change once decided. @NotThatKindOfDrLiz the merge decision is yours; @rabble your blocker is fixed in afe4bbac1 if you want to lift the changes-requested.
|
Overlap warning: this PR and #9168 both modify |
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
ab9e500 is the rebased head with eight commits on top of ce84dfc. The REST credit loss rabble flagged is fixed, and the credit rules still cover every creator the removed player row named.
afe4bbaparses the trailing attribution line during the REST conversion, so a REST-loaded video keeps in About the credit the caption no longer shows. A conversion test covers it.19e5c81gives parsing and stripping one decodability check. A line whose npub does not decode is neither credited nor hidden, and there is no throw path.fb056bcmoves the credit set onto the model and drops the getters that no longer had callers.
Open product calls
Two display decisions remain, and neither has to land before merge:
- A reply that carries the attribution line has no in-app credit surface. The caption no longer renders the line, and About skips the content npub on replies. Extending credit to replies is one change; hiding the picker for replies is the other.
- A captionless video whose content was only the attribution line now shows neither the collaborator row nor the reply-parent link.
Checks
dart format --output=none --set-exit-if-changed lib test integration_test tools: 0 changed.flutter analyze lib test integration_test tools: no issues.mobile/packages/models: 984 tests passed.- App suite (
--exclude-tags integration,golden, strict channels): 20,612 passed, 32 skipped. The one local failure was a stale ignored package directory left by an earlier removal on main; the affected file passed 21 of 21 after it was deleted. - CI on
ab9e500: 21 checks pass, including Mobile CI, Generated Files, the test shards, Integration Tests, Analyze, Format, Goldens, and Models. The only entry not run is the daily scheduled service-test publish.
Merge order
#9168 merged first, and both branches touch the video editor provider. The merged result is conflict free and the provider suite passes on it (144 tests). The merge queue reruns CI on the merged commit.
Remaining gate
The credit loss from rabble's earlier review is fixed in afe4bba. rabble's review request is still pending on this head and is the remaining gate before merge.
@NotThatKindOfDrLiz the two display calls and the merge decision are yours.
There was a problem hiding this comment.
Reviewed commit ab9e50093706bfaec1de3baa6a5a05f71080cd71. The prior REST-conversion blocker is fixed: VideoStats.toVideoEvent now populates the content-based credit that About consumes.
Two previously raised, actionable display decisions remain in this head:
-
Reply videos can lose their only visible inspired-by credit. Display stripping also applies to replies, while the About credit getter excludes their content reference. The relevant source anchors are
mobile/packages/models/lib/src/video_event.dart:1126andmobile/packages/models/lib/src/video_event.dart:1063. The publisher still appends that reference for replies and omits inspired-by p-tags for them. The author/product owner should explicitly choose whether to preserve that credit in About or retain the caption line when no equivalent credit is rendered. Hiding the picker only addresses future creation, not existing replies. -
An attribution-only caption can hide collaborator and reply-parent controls. Once stripping leaves no title or description, the caption gate also suppresses the collaborator row and reply-parent link at
mobile/lib/widgets/video_feed_item/video_feed_item.dart:605-616; the enclosing condition ismobile/lib/widgets/video_feed_item/video_feed_item.dart:540. Those controls are unrelated to caption text. Preserve their prior visibility or explicitly accept this user-visible change.
These are already documented in the other current-head handbacks; this review confirms they remain, rather than treating the resolved REST finding as a blanket resolution. No additional findings found.
Validation: exact-commit diff, model conversion/getters, publisher tag/content paths, overlay conditions, and focused regression-test inspection. Flutter/widget suites were not rerun in this review.
Review performed by Codex (GPT-6); posted through rabble.
rabble
left a comment
rabble
left a comment
There was a problem hiding this comment.
Reviewed commit ab9e50093706bfaec1de3baa6a5a05f71080cd71.
The earlier REST-conversion blocker is fixed: VideoStats.toVideoEvent now parses the credit. Two previously discussed behavior regressions remain and should be resolved, or explicitly accepted by the product owner, before merge:
-
[P2] Keep reply credit discoverable when suppressing its caption line.
mobile/packages/models/lib/src/video_event.dart:1126-1128strips every decodable attribution line, whilemobile/packages/models/lib/src/video_event.dart:1073-1079omits the content-npub credit for replies. Publishing can still append it on a reply (mobile/lib/services/video_event_publisher.dart:1760-1764). A reply with a valid attribution line and no legacy inspired-by p-tag therefore loses its only visible credit. An exact-head Flutter probe confirmsisVideoReply == true, a populatedinspiredByNpub, empty display content, and an empty About credit list. Preserve this existing credit in About or leave its caption line visible when there is no replacement surface; add the reply regression test. Disabling future selection alone would not preserve already-published replies. -
[P2] Decouple collaborator and reply controls from caption presence. The stripping at
mobile/packages/models/lib/src/video_event.dart:1126-1128makes an attribution-only, titleless video's display content empty.mobile/lib/widgets/video_feed_item/video_feed_item.dart:540gates the entire block containing the collaborator row and reply-parent link (mobile/lib/widgets/video_feed_item/video_feed_item.dart:605-618). Those existing controls disappear even though this PR is intended to remove inspiration text from the player. Move those controls outside the text-only condition and cover attribution-only videos in a widget test, unless product explicitly accepts the changed behavior.
Prior feedback: REST conversion, invalid-npub preservation, centralized credit collection and removal of unused getters are addressed. The two display decisions above remain unresolved; previous agent approvals do not establish a product decision.
Validation: 126 focused model tests passed (attribution line, display, REST conversion and reply metadata); targeted analysis of the three changed model sources passed. The additional reply-credit preservation probe fails as described. Inspected the overlay condition and publishing call site at this exact head. Existing GitHub checks are green. Widget suites and full localization generation were not rerun locally.
Review performed by Codex; posted through rabble.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mobile PR PreviewPreview refreshed for Last refresh:
|
|
Implemented the author decisions and addressed the re-review findings on the new head Fixed:
Declined: none. Verification on the exact head is green: analyze, format, generated files, goldens, all four test shards, service integration tests, models build, mergeability, and semantic PR. The changed UI also passes the localization hardcoded-string scan. @rabble please re-review the resolved blocker. @realmeylisdev and @dcadenas, the head moved after your reviews; please take another look. @divinevideo/reviewers review requested per the normal ownership flow. |
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
b047d6f90 is the new head: three commits on top of the last approved head ab9e500, and both items from the re-review are addressed here.
c3bb051ckeeps a reply's content credit visible in About while the reply-parent and clip-source relationships stay suppressed, and moves the collaborator row and reply-parent link out of the caption block so an attribution-only video still shows both.a84dba068andb047d6f90are test-only, draining the provider disposal timer the new reply-credit test left pending.
The publisher diff against cb51c728 is a comment edit, so the published event is unchanged. Coverage was added for the reply credit, with an off-reply positive control, and for the attribution-only overlay. Reverting either fix turns the matching tests red.
Recorded, no action
Clip-source provenance tags are still published on replies, where About shows no clip-source row. That behavior predates this branch (the base emitted them the same way), the About reply test pins it as intended, and the publisher change here is a comment edit, so I declined it as pre-existing and out of scope. Not a blocker, and worth knowing if the credit rules are revisited.
Checks
dart format --output=none --set-exit-if-changed lib test integration_test tools: 0 changed.flutter analyze lib test integration_test tools: no issues.mobile/packages/models: 984 tests passed, and the focused app suites for the changed surfaces pass.- App suite (
--exclude-tags integration,golden, strict channels): 20,613 passed, 32 skipped. One local failure inmobile/test/blocs/video_recorder/video_recorder_bloc_test.dartis a load flake on a path this head does not touch: the file passes 121/121 alone and the single test 5 of 5 at this head, and the CI shards are green. - CI on
b047d6f90: 21 checks pass, including Mobile CI, Generated Files, the four test shards, Integration Tests (services), Analyze, Format, Goldens, and Models CI. The daily scheduled service-test publish is the only entry that did not run.
Remaining gate
The branch is mergeable with main, and #9168 is already merged, so no rebase is needed. rabble's changes-requested review is the remaining gate, and it sits on ab9e500, so only rabble or you can lift it. Pending requests on this head: @rabble, @realmeylisdev, @divinevideo/reviewers.
@NotThatKindOfDrLiz both re-review items are resolved on this head, and the merge decision is yours.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Approving b047d6f90. Re-reviewed the three commits on top of ab9e50093 against that exact head.
Both items I escalated yesterday are now resolved by the author's decision rather than left open:
c3bb051c6credits a reply's content reference in About (creditedInspiredByPubkeysno longer gates the npub onisVideoReply; the inspiring-video and clip-source credits stay suppressed on replies), and moves the collaborator row and reply-parent link out of thehasTextContentblock so an attribution-only video keeps both. The reply test flips to assert the credit renders and keeps its off-reply positive control; the new overlay test pins the attribution-only case. The publisher change is a comment edit, so the wire format is unchanged.a84dba068andb047d6f90are test-only and drain the provider disposal timer the reply test left pending.
No new findings.
Validation
On b047d6f90 locally: dart format --set-exit-if-changed over the six changed Dart files is clean; flutter analyze is clean on the changed app files and inside packages/models under its own options; flutter test in packages/models passes 984; the overlay and About widget suites plus the publisher suites pass 494. Placeholder, unpinned-assertion, ungrouped-test, shared-setup-stub, l10n-delegate, legacy-import, raw-TextStyle, Nostr-id and pubkey-log ratchets pass.
CI on this head: 21 checks pass, including Mobile CI with all four shards, Generated Files, Goldens, Integration Tests (services), Models CI, mergeability and semantic-pull-request; the scheduled daily publish is the only entry that did not run.
One thing to know before merging: the head is 23 commits behind main (merge base cb51c7285), and two of those, #9120 and #9110, shrank rows in mobile/scripts/baseline/future_delayed_test_counts.txt that this branch never touched. Run against today's main, check_future_delayed_ceiling therefore reports this branch's untouched baseline as growth. A squash merge keeps main's baseline, so the post-merge tree is unaffected, but a CI re-run on this head would go red on that job until a rebase. I checked the combined tree rather than assuming: a throwaway rebase of this head onto today's main (c7562ded1) replays all 17 commits without conflict, check_future_delayed_ceiling passes there, flutter analyze lib test integration_test tools is clean, packages/models passes 986, and the editor-provider, feed-item, saved-sound, attribution and publisher suites pass 673. So the drift is cosmetic to a squash merge; a rebase before merging would only be needed if you want a fresh green run on the branch itself.
@NotThatKindOfDrLiz the merge decision is yours. @rabble your changes-requested review sits on ab9e50093; both items it named are resolved on this head, and only you or Liz can lift it.
Dismissing as stale. Both items were raised against ab9e500 and are resolved on the current head b047d6f (commit c3bb051), verified directly against the diff, not just the review bodies:
- Reply credit: mobile/packages/models/lib/src/video_event.dart:1074 — the inspiredByNpub credit in creditedInspiredByPubkeys is no longer gated on isVideoReply. Only the inspiring-video credit (line 1073) and clip-source credits (lines 1076-1078) remain reply-excluded.
- Collaborator row / reply-parent link: mobile/lib/widgets/video_feed_item/video_feed_item.dart:610-618 — both now sit in their own independent conditions (video.hasCollaborators, video.isVideoReply), outside the hasTextContent caption block that used to gate them.
dcadenas and realmeylisdev have both approved b047d6f, and CI is green on that head. We approve the PR.
Videos with inspired-by attribution currently repeat that credit over the player: once in the published caption text and again in a dedicated row. The About panel already presents the attribution as creator chips, so the duplicate player treatment adds clutter and can also repeat inside About.
This strips the trailing NIP-27 attribution line only at the shared display boundary, removes the dedicated player row, and reuses the same matcher when parsing published events and seeding the editor. The published event content remains unchanged, so other Nostr clients and older Divine builds retain attribution compatibility. The About panel continues to show its creator chips.
The now-unused widget, tests, localized messages, generated localization APIs, and raw-TextStyle ratchet entry are removed. This deliberately does not change either publishing path or the Explore debug route.
Verification:
Closes #9166