Comments Read/Create E2E - Changes to DB/Discovery/SDK/Desktop UI by DejayJD · Pull Request #9319 · AudiusProject/apps

DejayJD · 2024-07-30T22:04:20Z

Description

Big PR co-written by myself & Isaac. Sets up an entire e2e slice of functionality for comment reads & writes.

No rush merging this PR so please take time to review patterns we've laid out & anywhere that puts existing stuff at risk.

The goal for this PR was to have:

Working comment posts e2e - UI->SDK->EntityManager->DB
Working comment reads e2e - UI->SDK->API->DB
Desktop web UI Scaffolding for future backend work to easily build on top of and to set a pattern for the mobile work
We ended up squeezing in some working reply logic as well 💪

All the Changes

Desktop Client

Added feature flags for comments
TrackPage refactors.
- Refactored to support new layouts for the comments UI & moved away from css modules as much as possible (only remaining spot is where we pass stuff into Lineup)
Basic UI component structure
Some temporary context state management for testing purposes
Still some more to do here but didn't want this branch to keep growing infinitely

DB Changes

Added migration for comments & comment threads

Discovery Changes

New comments backend APIs & queries -
- Get comment by ID
- Get comments by track
Entity manager changes to support comments as a new entity type. Also, working Create action changes

SDK Changes

Generated SDK APIs for reading comments
Added SDK method for comment writes

TODO: Add changeset for SDK changes

How Has This Been Tested?

Aside from the basics of comments working, the main focus of testing was making sure these changes didn't impact any existing stuff.

On the UI, tested:

Flag off track page, looks as expected
Flag off track page of a remix-parent track, looks as expected
Flag off track page of a remix-child track, looks as expected

Backend changes should be standalone

changeset-bot · 2024-07-30T22:04:24Z

🦋 Changeset detected

Latest commit: d1a6152

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@audius/sdk	Minor
@audius/sp-actions	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

…jd/comments-e2e-scaffolding # Conflicts: # packages/common/src/services/remote-config/feature-flags.ts # packages/discovery-provider/src/tasks/entity_manager/entity_manager.py # packages/libs/src/sdk/api/generated/default/.openapi-generator/FILES # packages/libs/src/sdk/api/generated/default/models/index.ts

gitguardian · 2024-07-31T18:52:14Z

⚠️ GitGuardian has uncovered 12 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
11648676	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/discovery-one.env	View secret
11648676	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/discovery-one.docker.env	View secret
11648676	Triggered	Generic High Entropy Secret	`dcf45bd`	core/infra/dev_config/discovery-one.docker.env	View secret
11648678	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/content-three.env	View secret
11648678	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/content-three.docker.env	View secret
11648678	Triggered	Generic High Entropy Secret	`dcf45bd`	core/infra/dev_config/content-three.docker.env	View secret
11648679	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/content-two.env	View secret
11648679	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/content-two.docker.env	View secret
11648679	Triggered	Generic High Entropy Secret	`dcf45bd`	core/infra/dev_config/content-two.docker.env	View secret
11648680	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/content-one.docker.env	View secret
11648680	Triggered	Generic High Entropy Secret	`12bdddd`	core/infra/dev_config/content-one.env	View secret
11648680	Triggered	Generic High Entropy Secret	`dcf45bd`	core/infra/dev_config/content-one.docker.env	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

audius-infra · 2024-07-31T19:10:19Z

Preview this change https://demo.audius.co/jd/comments-e2e-scaffolding

audius-infra · 2024-07-31T23:34:33Z

Preview this change https://demo.audius.co/jd/comments-e2e-scaffolding

audius-infra · 2024-08-01T15:37:38Z

Preview this change https://demo.audius.co/jd/comments-e2e-scaffolding

Kyle-Shanks · 2024-08-05T20:41:27Z

+ "message": fields.String(required=True),
+ "timestamp_s": fields.Integer(required=False),
+ "react_count": fields.Integer(required=True),
+ "is_pinned": fields.Boolean(required=True),


seems a little interesting to have a reply be pinned?

Fair point. This will always exist in the db since its just a nested comment; but maybe it makes sense to omit it in our model for replies 🤔
Not sure if that makes our types a little messier though

audius-infra · 2024-08-05T21:40:43Z

Preview this change https://demo.audius.co/jd/comments-e2e-scaffolding

sliptype

Awesome work!

sliptype · 2024-08-05T15:47:27Z

+export const CommentBlock = ({
+ comment,
+ parentCommentId, // Parent comment ID can be passed in order to reply to a reply, otherwise it's assumed you're replying to this comment
+ parentCommentIndex
+}: CommentBlockProps) => {


Let's prefer destructuring in the component and just having a props arg

Is there a specific reason for that? I'm not against, just curious cause I generally prefer this method

I personally prefer destructing in the args too but there are some good reasons to destructure in the component!
Sometimes you need access to certain props and the entire props object in a component. For example, to pass ...props to a child
Components that only optionally accept props can't destructure in the arguments
It's easier to refactor to a forwardRef component, and a little bit easier to parse the first few lines of the component

Fair, but I'm not quite sure what you mean by 2? Usually I prefer avoiding props until 1/3 demands it but dont really care that much
Do you mean using like a union type or something?
like
typeProps={}|{thing: number}

Actually now that I think about it #2 doesn't really apply to components, only functions. I think we just wanted to go for consistency because of the possibility of #1
Let's talk about it during the frontend sync!

…jd/comments-e2e-scaffolding # Conflicts: # packages/common/src/services/remote-config/feature-flags.ts

audius-infra · 2024-08-06T22:20:39Z

Preview this change https://demo.audius.co/jd/comments-e2e-scaffolding

[5fe7681] Comments Read/Create E2E - Changes to DB/Discovery/SDK/Desktop UI (#9319) JD Francis [1ccfcd4] [PAY-3331] Purchase/sale endpoint filtering by content id (#9376) Reed [2ad3bd4] cleanup audio analysis code (#9379) Michelle Brier [45a92a7] [QA-875] Use size for es query to fix feed pagination bug (#9380) Raymond Jacobson

* main: (82 commits) Improve useClickOutside (#9377) [PAY-2267] Update stage payment router and program addresses (#9387) [QA-1446] Fix popup alignment (#9389) Audius Protocol v0.6.164 Add priority fees to purchases (#9392) [PAY-3314] Track select dropdown for purchase/remix targeting (#9386) Comments Read/Create E2E - Changes to DB/Discovery/SDK/Desktop UI (#9319) [C-4676] Fix es search returning deactivated content (#9382) [PAY-3331] Purchase/sale endpoint filtering by content id (#9376) cleanup audio analysis code (#9379) [PAY-3330] Fix spacing on RepostsFavoritesStats (#9375) Fix short trending bug (#9378) [QA-1114] Include menu items even if buttons are present (#9381) [QA-875] Use size for es query to fix feed pagination bug (#9380) Audius Protocol v0.6.163 As/proto 1888 core plays (#9348) Remove audius-cmd purchase-content (#9374) [QA-1486] Fix sign up select artists screen (#9371) [PAY-3313] Targeted DMs CTA and modal (#9364) [C-4921] Fix search v2 navigating back from profile (#9370) ...

rickyrombo

just took a peek at the schema and had some nitpicks, but overall looks great!!

rickyrombo · 2024-08-09T00:52:59Z

+ track_timestamp_ms BIGINT,
+ created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
+ updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
+ is_delete BOOLEAN DEFAULT FALSE,


nit: is_deleted?

rickyrombo · 2024-08-09T00:53:21Z

+ comment_id INTEGER PRIMARY KEY,
+ text TEXT NOT NULL,
+ user_id INTEGER NOT NULL,
+ entity_id INTEGER NOT NULL,


I think this should be an index? Seems like we'd query by it often

rickyrombo · 2024-08-09T01:00:02Z

+ user_id INTEGER NOT NULL,
+ entity_id INTEGER NOT NULL,
+ entity_type TEXT NOT NULL,
+ track_timestamp_ms BIGINT,


I don't think this needs to be a big int...
32 bit integers can be up to 4,294,967,295 unsigned or 2,147,483,647 signed
2,147,483,647 / 1000 ms/s / 60 s/min / 60 min/hr =~ 596 hrs, I don't think we have any plans to support that length!

[7121301] Set is_custom_musical_key (#9418) Sebastian Klingler [ee99fd4] add is_custom_musical_key (#9416) Michelle Brier [f30a637] [PAY-3332] Remixers users endpoint (#9401) Reed [f2eabb1] [QA-1491] Fix balance polling bug (#9402) Raymond Jacobson [c5e7348] Fix lint (#9412) Raymond Jacobson [5598656] [QA-1356] Improve notification UI (#9390) Raymond Jacobson [dcb74eb] Fix useClickOutside (#9409) Andrew Mendelsohn [975b630] Add better error messages for Claimable Tokens Program (#9369) Marcus Pasell [17904fb] [PAY-3335] Add priority fees to SDK Solana usages (#9399) Marcus Pasell [c34cd83] Record Track Downloads via Entity Manager (#9103) Marcus Pasell [e59dfc8] [QA-1357] Remove excessive polling and amplitude events in unlock (#9391) Raymond Jacobson [26b2fc9] [QA-1496] fix unsafe read of audio analysis field (#9406) Randy Schott [abc1b37] [C-4934] Search bar fixes (#9403) Sebastian Klingler [d525a1b] [C-4930, C-4931] Add IconText, ArtistPick, and Identifier components to harmony and harmony native (#9400) Kyle Shanks [53bf3e2] [PAY-3321][PAY-3333] Add network cut feature flag and update staking bridge and payment router addresses (#9393) Saliou Diallo [4d0fbc2] [PAY-3294] Hide save/repost for locked albums on mobile (#9333) Reed [7e3a75c] [C-4936] FilterButton fixes (#9397) Sebastian Klingler [517251d] [PAY-3286] Upload/edit confirmation drawer & modal rework (#9326) Raymond Jacobson [dce9903] [C-4920] Only show bpm decimals if necessary (#9398) Sebastian Klingler [d6dbab0] [C-4937] Adding some pagination buttons (#9394) JD Francis [f183f7e] [PAY-3336] fix isPlaying logic for details screens (#9395) Randy Schott [5ed523f] Add comment actions: edit, delete, react (#9372) Isaac Solo [88e24b5] [C 4922] Update mobile filter button (#9385) Dylan Jeffers [901d811] [C-4923] Persist verified filter & general cleanup (#9384) Sebastian Klingler [bab49d6] Add Pin icon and Timestamp component to harmony and harmony-native (#9383) Kyle Shanks [53a1a32] [QA-1485] Redir to signup/in on direct /feed link (#9373) Raymond Jacobson [c618eb9] Improve useClickOutside (#9377) Sebastian Klingler [a97deb8] [PAY-2267] Update stage payment router and program addresses (#9387) Raymond Jacobson [def034f] [QA-1446] Fix popup alignment (#9389) Raymond Jacobson [c86bc70] Add priority fees to purchases (#9392) Marcus Pasell [66f1a0e] [PAY-3314] Track select dropdown for purchase/remix targeting (#9386) Andrew Mendelsohn [5fe7681] Comments Read/Create E2E - Changes to DB/Discovery/SDK/Desktop UI (#9319) JD Francis [1ccfcd4] [PAY-3331] Purchase/sale endpoint filtering by content id (#9376) Reed [fc59a10] [PAY-3330] Fix spacing on RepostsFavoritesStats (#9375) Raymond Jacobson [7712005] Fix short trending bug (#9378) Raymond Jacobson [196fdba] [QA-1114] Include menu items even if buttons are present (#9381) Raymond Jacobson [8e6b1e8] [QA-1486] Fix sign up select artists screen (#9371) Sebastian Klingler [34d4417] [PAY-3313] Targeted DMs CTA and modal (#9364) Andrew Mendelsohn [3a19003] [C-4921] Fix search v2 navigating back from profile (#9370) Sebastian Klingler [d266394] [C-4918] Improve harmony web FilterButton (#9366) Dylan Jeffers [4ef81a7] Bump mobile versions to 110 (#9367) Dylan Jeffers

DejayJDand others added 9 commits July 17, 2024 13:26

wip
64e39a4

lots of WIP - UI scaffolding, little bit of SDK & backend APIs
155af7c

Add hooks for all the things
a935d7b

Fix parent comment id and make timestamp work
a037426

basic working e2e
126a22f

working e2e replies
64a6c2c

working e2e replies
f26a15c

filter comment replies from parent comments
b7b46c6

Fix css with flag off
0b1a37a

pull-request-sizeBot added the size/XXL label Jul 30, 2024

DejayJD changed the title ~~Comments Read/Create E2E - DB + Discovery + SDK + Desktop UI~~Comments Read/Create E2E - Changes to DB/Discovery/SDK/Desktop UIJul 30, 2024

DejayJD added 6 commits July 30, 2024 18:00

UI cleanup & types cleanup
bd6f275

type cleanup
16304ff

UI tweaks, work with no moreBy tracks
9afaebb

Fixing types
74cd0c3

last bit of type changes
fbd6442

DejayJD marked this pull request as ready for review July 31, 2024 19:32

DejayJD requested review from Kyle-Shanks, dylanjeffers, isaacsolo and sliptype July 31, 2024 22:34

Edit UI working
c53d24a

clean up discovery
541327f

isaacsolo approved these changes Aug 5, 2024
View reviewed changes

Kyle-Shanks reviewed Aug 5, 2024
View reviewed changes

Comment threadpackages/discovery-provider/src/queries/get_comments.py Outdated

Kyle-Shanks reviewed Aug 5, 2024
View reviewed changes

Comment threadpackages/discovery-provider/src/queries/get_comments.py

clean track args
7df0044

sliptype approved these changes Aug 6, 2024
View reviewed changes

DejayJD added 4 commits August 6, 2024 16:17

Merge branch 'main' of github.com:AudiusProject/audius-protocol into …
dcf45bd
…jd/comments-e2e-scaffolding # Conflicts: # packages/common/src/services/remote-config/feature-flags.ts

PR feedback
b8d44ef

Connect timestamps on UI, add user links and fix userId not being sent
7568a44

add changeset
d1a6152

DejayJD enabled auto-merge (squash) August 6, 2024 22:21

DejayJD merged commit 5fe7681 into mainAug 6, 2024

DejayJD deleted the jd/comments-e2e-scaffolding branch August 6, 2024 22:39

rickyrombo reviewed Aug 9, 2024
View reviewed changes

Uh oh!

Conversation

DejayJD commented Jul 30, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

All the Changes

How Has This Been Tested?

Uh oh!

changeset-botBot commented Jul 30, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

gitguardianBot commented Jul 31, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ GitGuardian has uncovered 12 secrets following the scan of your pull request.

Uh oh!

audius-infra commented Jul 31, 2024

Uh oh!

audius-infra commented Jul 31, 2024

Uh oh!

audius-infra commented Aug 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

audius-infra commented Aug 5, 2024

Uh oh!

sliptype left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DejayJDAug 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

audius-infra commented Aug 6, 2024

Uh oh!

rickyrombo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DejayJD commented Jul 30, 2024•
edited
Loading

changeset-botBot commented Jul 30, 2024•
edited
Loading

gitguardianBot commented Jul 31, 2024•
edited
Loading

DejayJDAug 6, 2024•
edited
Loading