Re: [PR] fix(chart): set tab name as chart name [superset]

via GitHub Wed, 04 Jun 2025 14:21:47 -0700


korbit-ai[bot] commented on code in PR #33694:
URL: https://github.com/apache/superset/pull/33694#discussion_r2127462776



##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -269,6 +269,16 @@ function ExploreViewContainer(props) {
 
   const theme = useTheme();
 
+  const originalDocumentTitle = document.title;
+  useEffect(() => {
+    if (props.sliceName) {
+      document.title = props.sliceName;
+    }
+    return () => {
+      document.title = originalDocumentTitle;
+    };
+  }, [props.sliceName])

Review Comment:
   ### Undocumented Title Update Effect <sub>![category 
Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The code modifies document.title without documenting the intention and 
implications of this side effect.
   
   
   ###### Why this matters
   Future maintainers won't understand why the title is being changed or the 
cleanup behavior's importance without proper context.
   
   ###### Suggested change ∙ *Feature Preview*
     // Updates the document title to reflect the current slice name when 
viewing
     // a specific chart, restoring the original title when unmounting
     useEffect(() => {
       if (props.sliceName) {
         document.title = props.sliceName;
       }
       return () => {
         document.title = originalDocumentTitle;
       };
     }, [props.sliceName])
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6b752e5-2a43-48b3-870b-bd6e785cc6c5/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6b752e5-2a43-48b3-870b-bd6e785cc6c5?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6b752e5-2a43-48b3-870b-bd6e785cc6c5?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6b752e5-2a43-48b3-870b-bd6e785cc6c5?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6b752e5-2a43-48b3-870b-bd6e785cc6c5)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:a2671137-dc8f-4d84-8276-3da0cb55b29f -->
   
   
   [](a2671137-dc8f-4d84-8276-3da0cb55b29f)



##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -269,6 +269,16 @@ function ExploreViewContainer(props) {
 
   const theme = useTheme();
 
+  const originalDocumentTitle = document.title;
+  useEffect(() => {
+    if (props.sliceName) {
+      document.title = props.sliceName;
+    }
+    return () => {
+      document.title = originalDocumentTitle;
+    };
+  }, [props.sliceName])

Review Comment:
   ### Stale Document Title Reference <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The originalDocumentTitle variable is captured outside the useEffect hook, 
meaning it will only store the initial document title when the component first 
renders. If another component changes the document title in between, this value 
will be stale.
   
   
   ###### Why this matters
   This could lead to incorrect document title restoration when the component 
unmounts, potentially overwriting document titles set by other components in 
the application.
   
   ###### Suggested change ∙ *Feature Preview*
   Move the originalDocumentTitle variable inside the useEffect hook to capture 
the current title when the effect runs:
   ```jsx
   useEffect(() => {
       const originalDocumentTitle = document.title;
       if (props.sliceName) {
         document.title = props.sliceName;
       }
       return () => {
         document.title = originalDocumentTitle;
       };
     }, [props.sliceName])
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b78e28e6-1415-4c8a-9bea-649faca942e3/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b78e28e6-1415-4c8a-9bea-649faca942e3?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b78e28e6-1415-4c8a-9bea-649faca942e3?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b78e28e6-1415-4c8a-9bea-649faca942e3?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b78e28e6-1415-4c8a-9bea-649faca942e3)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:f94cc963-b740-413e-8e67-f6a25b00742f -->
   
   
   [](f94cc963-b740-413e-8e67-f6a25b00742f)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] fix(chart): set tab name as chart name [superset]

Reply via email to