add separate toggles for auto-skip categories #1253
Conversation
WalkthroughThe recent changes introduce a more granular control over auto-skipping content within a media platform, allowing users to individually toggle auto-skipping for different categories of segments such as sponsors, intros, etc. This is achieved by adding a new configuration option in the UI and updating the database schema and server logic to handle multiple auto-skip categories instead of a single auto-skip flag. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
docs/api.yamlis excluded by:!**/*.yaml
Files selected for processing (9)
- client/src/components/RoomSettingsForm.vue (5 hunks)
- client/src/locales/en.ts (1 hunks)
- common/models/rest-api.ts (2 hunks)
- common/models/types.ts (3 hunks)
- server/api/room.ts (1 hunks)
- server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js (1 hunks)
- server/models/room.ts (4 hunks)
- server/room.ts (9 hunks)
- server/storage/room.ts (3 hunks)
Additional comments: 23
server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js (1)
- 6-17: The migration script correctly adds a new
autoSkipSegmentCategoriescolumn with a JSONB type and a default value. However, it's important to ensure that the default values align with the application's expected behavior and that thedefaultValueobject keys match the segment categories used by the application.Verification successful
It seems that the default values for the
autoSkipSegmentCategoriesare consistent across the application. The keys and usage of these categories are found in theRoomSettingsForm.vuecomponent, API models, database migrations, and server logic. The default values match the expected behavior and are aligned with the segment categories used elsewhere in the application.Given this information, it can be concluded that the default values align with the application's expected behavior and that the keys are consistent with the segment categories used elsewhere in the application.
Conclusion
The default values for the
autoSkipSegmentCategoriescolumn are consistent with the application's expected behavior and the segment categories used elsewhere in the application.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the default values match the application's expected behavior # and that the keys are consistent with the segment categories used elsewhere in the application. # This script should be run in the repository root. grep -r "autoSkipSegmentCategories" .Length of output: 2988
common/models/rest-api.ts (1)
- 56-56: The addition of
autoSkipSegmentCategoriesto theOttApiResponseGetRoominterface is correct and aligns with the PR objectives to allow users to configure auto-skipping for individual segment categories.common/models/types.ts (2)
- 57-59: The
SegmentCategoriestype is correctly defined using a mapped type over theCategorytype from the "sponsorblock-api" module. This ensures that the keys ofSegmentCategorieswill match the categories defined by SponsorBlock.- 71-71: The
autoSkipSegmentCategoriesproperty is correctly added to theRoomSettingsinterface, which is consistent with the PR's goal to provide granular control over auto-skipping of video segment categories.server/models/room.ts (2)
- 21-21: The
autoSkipSegmentCategoriesproperty is correctly added to theRoomAttributesinterface. This change is necessary for the ORM to recognize and interact with the new database column.- 107-118: The
autoSkipSegmentCategoriesfield is correctly added to thecreateModelfunction with the appropriate type and default value. This ensures that new rooms will have theautoSkipSegmentCategoriesproperty initialized correctly.server/storage/room.ts (3)
- 108-108: The
autoSkipSegmentCategoriesproperty is correctly added to theRoomOptionsinterface, which is used when creating or updating room instances in the database.- 135-135: The
autoSkipSegmentCategoriesproperty is correctly included in theroomToDbfunction, which is responsible for converting room state into a format that can be stored in the database.- 171-171: The
autoSkipSegmentCategoriesproperty is correctly included in theroomToDbPartialfunction, ensuring that partial updates to the room state include changes to the auto-skip segment categories.client/src/components/RoomSettingsForm.vue (4)
- 74-117: The new section for auto-skip segment categories in the
RoomSettingsForm.vuecomponent is correctly implemented. It allows users to configure various segment types for automatic skipping, and theautoSkipSegmentCategoriesobject is initialized with default values. Ensure that the labels used here match the keys in theautoSkipSegmentCategoriesobject and that they are correctly localized.Verification successful
The labels used in the
RoomSettingsForm.vuecomponent match the keys in theautoSkipSegmentCategoriesobject and are correctly localized in theen.tsfile. The verification process is successful, and the implementation aligns with the expected behavior.Conclusion
* 223-231: The initialization of `autoSkipSegmentCategories` with default values in the component's data is correct and aligns with the PR's objectives. * 255-255: The `autoSkipSegmentCategories` property is correctly picked from the API response and assigned to the component's state, ensuring that the room settings are loaded correctly. * 276-276: The `autoSkipSegmentCategories` property is correctly included in the `getRoomSettingsSubmit` function, which prepares the room settings for submission to the server.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the labels used in the RoomSettingsForm.vue component match the keys in the autoSkipSegmentCategories object # and that they are correctly localized in the en.ts file. grep -r "auto-skip-text-" client/src/locales/en.tsLength of output: 341
client/src/locales/en.ts (1)
- 247-254: The addition of specific auto-skip text options related to SponsorBlock data in the
en.tslocalization file is correct. This ensures that the new settings are properly described in the user interface.server/api/room.ts (1)
- 216-216: The addition of the "autoSkipSegmentCategories" property to the
getRoomrequest handler is consistent with the PR's objective to allow users to configure the automatic skipping of various segment types individually.server/room.ts (8)
- 48-48: Import of
SegmentCategoriesis correct and aligns with the PR objectives.- 231-239: The
autoSkipSegmentCategoriesproperty is initialized with default values, which is consistent with the PR objectives to allow users to configure auto-skipping for individual segment categories.- 172-172: The inclusion of
autoSkipSegmentCategoriesinsyncablePropsis correct, ensuring that the property is synchronized with clients.- 197-197: The inclusion of
autoSkipSegmentCategoriesinstorablePropsis correct, ensuring that the property is stored in Redis.- 415-422: Getter and setter methods for
autoSkipSegmentCategorieshave been implemented correctly, including marking the property as dirty when it is set, which is necessary for synchronization.- 821-821: The usage of
autoSkipSegmentCategoriesin theupdatemethod correctly checks the category of the segment against the user's settings to determine if it should be skipped.- 890-890: The
autoSkipSegmentCategoriesproperty is correctly included in theRoomStatePersistabletype, ensuring it is part of the room's persistent state.- 1528-1528: The
autoSkipSegmentCategoriesproperty is correctly checked in theapplySettingsmethod, ensuring that permissions are verified before applying changes to this setting.
There was a problem hiding this comment.
Thanks for the high quality PR! Love to see it. I'll give this a bit more scrutiny when the CI passes and these change requests are resolved.
The only major blocker I see is that there's no validation on the SegmentCategories before it is applied to the Room and saved in redis or database. A malicious client could DoS because they could add a bunch of items to the object and eat up a bunch of memory, and/or disk space. Preferably, this should be done in the handler for RoomRequest.ApplySettingsRequest.
Also, I'd like to have unit tests for storing and retrieving this new column in the database.
dyc3
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1253 +/- ##
================================================
- Coverage 62.4673% 62.2464% -0.2210%
================================================
Files 118 118
Lines 8795 8823 +28
Branches 1174 1169 -5
================================================
- Hits 5494 5492 -2
- Misses 3301 3331 +30 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
docs/api.yamlis excluded by:!**/*.yamltests/e2e/fixtures/get-room-foo.jsonis excluded by:!**/*.jsontests/e2e/fixtures/patch-room-foo.jsonis excluded by:!**/*.json
Files selected for processing (11)
- client/src/components/RoomSettingsForm.vue (8 hunks)
- client/src/locales/en.ts (1 hunks)
- common/constants.ts (1 hunks)
- common/models/rest-api.ts (2 hunks)
- common/models/types.ts (2 hunks)
- server/api/room.ts (1 hunks)
- server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js (1 hunks)
- server/models/room.ts (4 hunks)
- server/room.ts (15 hunks)
- server/storage/room.ts (3 hunks)
- server/tests/unit/api/room.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- client/src/components/RoomSettingsForm.vue
- client/src/locales/en.ts
- common/models/rest-api.ts
- common/models/types.ts
- server/api/room.ts
- server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js
- server/models/room.ts
- server/storage/room.ts
Additional comments: 2
common/constants.ts (1)
- 17-25: The addition of
ALL_SKIP_CATEGORIESis aligned with the PR's objective to support individual toggles for auto-skipping different video segment categories. This constant provides a centralized list of all available categories, which is a good practice for maintainability and consistency across the application.server/tests/unit/api/room.spec.ts (1)
- 374-446: The test suite for validating
autoSkipSegmentCategorieseffectively covers various scenarios, including duplicate categories, invalid categories, and empty arrays. This ensures that the application correctly filters and saves only unique and valid auto-skip segment categories. The use ofit.eachfor parameterized tests enhances test readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- client/src/components/RoomSettingsForm.vue (6 hunks)
- server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js (1 hunks)
- server/room.ts (15 hunks)
- server/tests/unit/api/room.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- client/src/components/RoomSettingsForm.vue
- server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js
- server/room.ts
- server/tests/unit/api/room.spec.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- server/tests/unit/api/room.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/tests/unit/api/room.spec.ts
|
When I run Is there something I should change in the config? Output.... /home/teofil/projects/opentogethertube/common/ts-out/tests/unit/voteskip.spec.js ✖ 110 problems (110 errors, 0 warnings) error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/workspaces for documentation about this command. |
|
Ah yeah I've fixed that on |
server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js (1 hunks)
- server/room.ts (15 hunks)
Additional comments: 7
server/migrations/20240121172024-add-auto-skip-fields-to-rooms.js (1)
- 20-24: Ensure that setting "autoSkipSegmentCategories" to an empty array ('[]') for rooms where "autoSkipSegments" is false aligns with the application's expected behavior. This logic assumes all rooms with autoSkipSegments disabled should have no categories selected for auto-skipping, which might not be intuitive for end-users or consistent with future requirements.
server/room.ts (6)
- 228-228: The initialization of
_autoSkipSegmentCategorieswithArray.from(ALL_SKIP_CATEGORIES)ensures all categories are enabled by default. Confirm this default behavior aligns with user expectations and the application's requirements.- 394-400: Getter and setter for
autoSkipSegmentCategoriescorrectly manage the internal state and mark the property as dirty when changed. This is consistent with the class's design pattern for property management.- 543-543: The condition to set
wantSponsorBlockto true based on the presence of auto-skip categories and a current source is logical. However, ensure that this logic is invoked at appropriate times to avoid unnecessary SponsorBlock requests, especially in scenarios where the current video does not support SponsorBlock data.- 1566-1578: The logic to filter
autoSkipSegmentCategoriesagainstALL_SKIP_CATEGORIESensures that only valid categories are saved. This is a good practice for data integrity. However, ensure that the application's UI and backend validation align to prevent invalid categories from being submitted in the first place.- 1601-1606: The condition to fetch SponsorBlock segments upon enabling auto-skip categories during a live session is appropriate. This proactive approach enhances the user experience by applying the new settings immediately.
- 1648-1648: Setting
wantSponsorBlockto true inplayNowwhen auto-skip categories are enabled ensures that the newly played video fetches relevant SponsorBlock segments. This maintains consistency with the auto-skip feature's behavior across different user actions.
Yup, everything works fine when I run |
|
I've resolved the import issues with |
Resolves #1209