Add: Trubbel’s Utilities 1.0.0 #259

Trubbel · 2025-01-07T02:50:27Z

Show custom timestamp for clips and videos
Video preview for thumbnails in the directory
Automatically reset player on error
Collapse drops in the inventory page
…etc

SirStendec · 2025-02-11T18:29:54Z

src/trubbel/utilities/collapsible-drops.js

+  // Load collapsed state from localStorage
+  function loadCollapsedState() {
+    try {
+      const stored = localStorage.getItem(STORAGE_KEY);


Please use FFZ's settings provider for storing values rather than localStorage.

this.settings.provider.get(key); this.settings.provider.set(key, value); this.settings.provider.has(key); this.settings.provider.delete(key);

SirStendec · 2025-02-11T18:35:02Z

src/trubbel/utilities/collapsible-drops.js

+  });
+
+  // Start observing
+  observer.observe(document.body, {


This observer does not need to run if you aren't on twitch.tv/drops/inventory.

You can use, and in fact I see you are using, the site.router:route event to detect navigation changes within the React app and use that to check the current URL and decide to enable or disable your logic, rather than just leave it running all the time. But you don't have any code that stops the observer when the user leaves that page.

SirStendec · 2025-02-11T18:36:38Z

src/trubbel/utilities/data/clip_info.gql

@@ -0,0 +1,5 @@
+query FFZ_GetClipDate($slug: ID!) {
+	clip(slug: $slug) {


You should always include the id column when fetching something like a clip model to that Apollo's caching layer stays happy.

SirStendec · 2025-02-11T18:36:54Z

src/trubbel/utilities/data/video_info.gql

@@ -0,0 +1,5 @@
+query FFZ_GetVideoDate($id: ID!) {
+	video(id: $id) {


You should always include the id column when fetching something like a video model to that Apollo's caching layer stays happy. Yes, even though the query includes the id.

SirStendec · 2025-02-11T18:39:58Z

src/trubbel/utilities/get-clip-video.js

+      });
+
+      try {
+        if (result?.data?.clip && result?.data?.clip.createdAt) {


You can just do if (result?.data?.clip?.createdAt)

SirStendec · 2025-02-11T18:42:17Z

src/trubbel/index.js

+			defaultDateTimeFormat: this.settings.get("addon.trubbel.format.datetime")
+		});
+
+		this.on("addon.trubbel.format.datetime-clip", () => this.setClipOrVideoTimestamp());


I think this is supposed to be settings:changed:addon.trubbel.format.datetime-clip and not just addon.trubbel.format.datetime-clip? Similar issue with the line below this.

SirStendec · 2025-02-11T18:45:32Z

src/trubbel/index.js

+		});
+
+		this.on("addon.trubbel.format.datetime-clip", () => this.setClipOrVideoTimestamp());
+		this.on("addon.trubbel.format.datetime-video", () => this.setClipOrVideoTimestamp());


A lot of these event listeners have unnecessary lambdas. You can just do this.on(some_event, this.handler) and it will work. If you don't supply a context as the third argument, the context (aka what this is when the handler is called) will be set to the this from this.on() by default.

SirStendec · 2025-02-11T18:49:17Z

src/trubbel/index.js

+								if (metadataElement) {
+									const hasError = errorCodes.some(code => metadataElement.textContent.includes(code));
+									if (hasError) {
+										this.site.children.player.resetPlayer(this.site.children.player.current);


My main issue with this code is that there isn't really a good safety check against it potentially breaking and causing a stampede. There should be a limit so that it can only try resetting the player so many times within a certain time window.

While reducing frustration for the user is good, we also want to ensure we can't accidentally create excessive load on Twitch's servers.

SirStendec · 2025-02-11T18:50:52Z

src/trubbel/index.js

+
+
+	onDisable() {
+		if (this.handleMouseEnterListener) {


There's a lot of event listeners and potentially long running code that isn't disabled in this onDisable method. It may as well not be here in its current state since a user will need to refresh the page to fully clean out this add-on's side effects.

SirStendec · 2025-02-11T18:53:40Z

src/trubbel/index.js

+	}
+
+	handleMouseLeave(event) {
+		handleMouseLeave(event);


I'm not sure what the point of this is, to be honest. If you really want handleMouseLeave to be accessible under your module you could just do this.handleMouseLeave = handleMouseLeave; in your constructor or even assign it to the prototype. This is just an extra layer of function when you're already using an arrow function elsewhere where actually binding this method?

Trubbel · 2025-02-12T21:49:32Z

Had made a few changes beforehand. Hopefully this is better.

SirStendec

Looking better! Just a few more things to bring up. I should've gone over the GQL better the first time, so sorry about that.

The GQL requests are the only thing I consider a real blocker here, though I'd suggest also addressing the change to settings (removing an unnecessary JSON serialization) since changing that in the future, once this is in user hands, would be more annoying.

SirStendec · 2025-02-14T17:55:46Z

src/trubbel/settings/chat-commands.js

+
+  removeCommandHandlers() {
+    for (const handler of this.commandHandlers) {
+      ffz.off("chat:get-tab-commands", handler);


Why are you using the window global ffz to register/unregister events? You're already working within a Module so you can just use this.on(...) and this.off(...)

SirStendec · 2025-02-14T18:04:32Z

src/trubbel/settings/drops-rewards.js

+
+  saveCollapsedState(state) {
+    try {
+      this.settings.provider.set(STORAGE_KEY_CONFIG, JSON.stringify(state));


To clarify, FFZ's setting providers can store JSON safe objects, you don't need to run stringify / parse yourself.

SirStendec · 2025-02-14T18:07:00Z

src/trubbel/settings/remove-things.js

+  updateCSS() {
+    // Remove/Hide Things - Stream - Remove the Gradient from Player
+    if (this.settings.get("addon.trubbel.remove-things.player-gradient")) {
+      this.css_tweaks.set("hide-player-gradient", ".video-player .top-bar, .video-player .player-controls { background: transparent !important; }");


Just curious, is there any reason you're using css_tweaks to store your css when you have a custom ManagedStyle instance you could be setting these in?

A little bit of being stupid, and a little bit of starting something and not finishing it. And going back and forth.
(Just like with the translation core)

SirStendec · 2025-02-14T18:10:57Z

src/trubbel/utils/graphql/followage.gql

@@ -0,0 +1,10 @@
+query FFZ_GetUserFollowAge($login: String!) {


This query is redundant. FFZ's twitch_data module has a getUserFollowed(id, login) method that returns followedAt.

SirStendec · 2025-02-14T18:15:14Z

src/trubbel/utils/graphql/accountage.gql

@@ -0,0 +1,6 @@
+query FFZ_GetUserAccountAge($login: String!) {


I forgot to mention it before, but please set the unique prefix of your gql query names to something other than FFZ, like Trubbel perhaps. This follows the pattern established by the Deck add-on and helps if we need to identify the source of a specific query.

SirStendec · 2025-02-14T18:18:02Z

src/trubbel/settings/clips-videos.js

+  }
+
+  onEnable() {
+    this._ = new NewTransCore({


I looked through the code again and I don't think you're ever actually using this for anything? Just assigning a format to it. You should probably drop creating your own translation core and instead access i18n._ in that one specific place where you want to iterate the available formats.

SirStendec · 2025-02-14T18:33:36Z

src/trubbel/settings/drops-rewards.js

+
+  async waitForElements() {
+    return new Promise((resolve) => {
+      const checkElements = () => {


If it doesn't end up finding what it wants, this code could end up running once per frame until the page is closed/reloaded. I would suggest adding a timeout.

Add: Trubbel’s Utilities 1.0.0

c60f45e

SirStendec requested changes Feb 11, 2025

View reviewed changes

Complete overhaul

0e70e2f

Trubbel requested a review from SirStendec February 14, 2025 13:05

SirStendec requested changes Feb 14, 2025

View reviewed changes

Fixing the requests

afaeb51

Trubbel requested a review from SirStendec February 14, 2025 21:57

SirStendec approved these changes Feb 14, 2025

View reviewed changes

SirStendec merged commit 5008a39 into FrankerFaceZ:master Feb 14, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: Trubbel’s Utilities 1.0.0 #259

Add: Trubbel’s Utilities 1.0.0 #259

Trubbel commented Jan 7, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

SirStendec Feb 11, 2025

Trubbel commented Feb 12, 2025

SirStendec left a comment

SirStendec Feb 14, 2025

SirStendec Feb 14, 2025

SirStendec Feb 14, 2025

Trubbel Feb 14, 2025

SirStendec Feb 14, 2025

SirStendec Feb 14, 2025

SirStendec Feb 14, 2025

SirStendec Feb 14, 2025

		@@ -0,0 +1,5 @@
		query FFZ_GetClipDate($slug: ID!) {
		clip(slug: $slug) {

		@@ -0,0 +1,5 @@
		query FFZ_GetVideoDate($id: ID!) {
		video(id: $id) {

		@@ -0,0 +1,10 @@
		query FFZ_GetUserFollowAge($login: String!) {

		@@ -0,0 +1,6 @@
		query FFZ_GetUserAccountAge($login: String!) {

Add: Trubbel’s Utilities 1.0.0 #259

Add: Trubbel’s Utilities 1.0.0 #259

Conversation

Trubbel commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trubbel commented Feb 12, 2025

SirStendec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment