make it more profesh#342
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughReplaces the header's Next/Image logo with an internal inline SupersetLogo SVG and updates anchor classes. Adds a new TypewriterText component (with barrel export) and integrates it into the HeroSection to render the hero heading with a typing animation. No public API signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolved conflicts in Header.tsx keeping SVG logo while adopting new ctaButtons prop interface and theme-aware styling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/marketing/src/app/components/HeroSection/HeroSection.tsx (1)
77-86: Typing animation in anh1needs a11y guardrails (screen readers / reduced motion).
At minimum: ensure the animated text is not announced char-by-char (e.g., animated spanaria-hidden, plus ansr-onlyfull string). This likely belongs insideTypewriterText.
🧹 Nitpick comments (1)
apps/marketing/src/app/components/Header/Header.tsx (1)
9-25: Avoid multi-componentHeader.tsx; extractSupersetLogoto its own component folder/file.
This violates the “one component per file” structure under**/components/**and makes the Header harder to maintain as the SVG evolves. (Based on learnings / coding guidelines.)- function SupersetLogo() { - return (/* ... */); - } + // Move to: apps/marketing/src/app/components/Header/components/SupersetLogo/SupersetLogo.tsx + // and re-export from: apps/marketing/src/app/components/Header/components/SupersetLogo/index.ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/marketing/src/app/components/Header/Header.tsx(2 hunks)apps/marketing/src/app/components/HeroSection/HeroSection.tsx(2 hunks)apps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsx(1 hunks)apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.tsapps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsxapps/marketing/src/app/components/HeroSection/HeroSection.tsxapps/marketing/src/app/components/Header/Header.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.tsapps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsxapps/marketing/src/app/components/HeroSection/HeroSection.tsxapps/marketing/src/app/components/Header/Header.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.tsapps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsxapps/marketing/src/app/components/HeroSection/HeroSection.tsxapps/marketing/src/app/components/Header/Header.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-12T05:45:09.672Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.672Z
Learning: Applies to **/components/**/*.{ts,tsx} : Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Applied to files:
apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.ts
🧬 Code graph analysis (2)
apps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsx (1)
apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.ts (1)
TypewriterText(1-1)
apps/marketing/src/app/components/HeroSection/HeroSection.tsx (1)
apps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsx (1)
TypewriterText(15-64)
🔇 Additional comments (3)
apps/marketing/src/app/components/HeroSection/HeroSection.tsx (1)
10-10: Barrel import forTypewriterTextis a nice direction.
Keeps HeroSection from importing deep paths directly and aligns with the component folder structure. (Based on coding guidelines.)apps/marketing/src/app/components/HeroSection/components/TypewriterText/index.ts (1)
1-1: LGTM—simple, clean barrel export.apps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsx (1)
6-13: [Rewritten review comment]
[Classification tag]
Comment on lines
+11
to
+23
| <svg | ||
| viewBox="0 0 392 64" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| aria-label="Superset" | ||
| className="h-5 w-auto" | ||
| > | ||
| <title>Superset</title> | ||
| <path | ||
| d="M25.2727 -0.00017944H37.9091V12.6362H25.2727V-0.00017944ZM12.6364 -0.00017944H25.2727V12.6362H12.6364V-0.00017944ZM0 12.6362H12.6364V25.2725H0V12.6362ZM0 25.2725H12.6364V37.9089H0V25.2725ZM12.6364 25.2725H25.2727V37.9089H12.6364V25.2725ZM25.2727 25.2725H37.9091V37.9089H25.2727V25.2725ZM25.2727 37.9089H37.9091V50.5453H25.2727V37.9089ZM25.2727 50.5453H37.9091V63.1816H25.2727V50.5453ZM12.6364 50.5453H25.2727V63.1816H12.6364V50.5453ZM0 50.5453H12.6364V63.1816H0V50.5453ZM0 -0.00017944H12.6364V12.6362H0V-0.00017944ZM50.4961 -0.00017944H63.1325V12.6362H50.4961V-0.00017944ZM50.4961 12.6362H63.1325V25.2725H50.4961V12.6362ZM50.4961 25.2725H63.1325V37.9089H50.4961V25.2725ZM50.4961 37.9089H63.1325V50.5453H50.4961V37.9089ZM50.4961 50.5453H63.1325V63.1816H50.4961V50.5453ZM63.1325 50.5453H75.7688V63.1816H63.1325V50.5453ZM75.7688 50.5453H88.4052V63.1816H75.7688V50.5453ZM75.7688 37.9089H88.4052V50.5453H75.7688V37.9089ZM75.7688 25.2725H88.4052V37.9089H75.7688V25.2725ZM75.7688 12.6362H88.4052V25.2725H75.7688V12.6362ZM75.7688 -0.00017944H88.4052V12.6362H75.7688V-0.00017944ZM100.992 -0.00017944H113.629V12.6362H100.992V-0.00017944ZM100.992 12.6362H113.629V25.2725H100.992V12.6362ZM100.992 25.2725H113.629V37.9089H100.992V25.2725ZM100.992 37.9089H113.629V50.5453H100.992V37.9089ZM100.992 50.5453H113.629V63.1816H100.992V50.5453ZM113.629 -0.00017944H126.265V12.6362H113.629V-0.00017944ZM126.265 -0.00017944H138.901V12.6362H126.265V-0.00017944ZM126.265 12.6362H138.901V25.2725H126.265V12.6362ZM126.265 25.2725H138.901V37.9089H126.265V25.2725ZM113.629 25.2725H126.265V37.9089H113.629V25.2725ZM151.488 -0.00017944H164.125V12.6362H151.488V-0.00017944ZM151.488 12.6362H164.125V25.2725H151.488V12.6362ZM151.488 25.2725H164.125V37.9089H151.488V25.2725ZM151.488 37.9089H164.125V50.5453H151.488V37.9089ZM151.488 50.5453H164.125V63.1816H151.488V50.5453ZM164.125 -0.00017944H176.761V12.6362H164.125V-0.00017944ZM164.125 50.5453H176.761V63.1816H164.125V50.5453ZM164.125 25.2725H176.761V37.9089H164.125V25.2725ZM176.761 -0.00017944H189.397V12.6362H176.761V-0.00017944ZM176.761 50.5453H189.397V63.1816H176.761V50.5453ZM201.984 50.5453H214.621V63.1816H201.984V50.5453ZM201.984 37.9089H214.621V50.5453H201.984V37.9089ZM201.984 25.2725H214.621V37.9089H201.984V25.2725ZM201.984 12.6362H214.621V25.2725H201.984V12.6362ZM201.984 -0.00017944H214.621V12.6362H201.984V-0.00017944ZM214.621 -0.00017944H227.257V12.6362H214.621V-0.00017944ZM227.257 -0.00017944H239.893V12.6362H227.257V-0.00017944ZM227.257 12.6362H239.893V25.2725H227.257V12.6362ZM214.621 25.2725H227.257V37.9089H214.621V25.2725ZM227.257 37.9089H239.893V50.5453H227.257V37.9089ZM227.257 50.5453H239.893V63.1816H227.257V50.5453ZM277.753 -0.00017944H290.39V12.6362H277.753V-0.00017944ZM265.117 -0.00017944H277.753V12.6362H265.117V-0.00017944ZM252.48 12.6362H265.117V25.2725H252.48V12.6362ZM252.48 25.2725H265.117V37.9089H252.48V25.2725ZM265.117 25.2725H277.753V37.9089H265.117V25.2725ZM277.753 25.2725H290.39V37.9089H277.753V25.2725ZM277.753 37.9089H290.39V50.5453H277.753V37.9089ZM277.753 50.5453H290.39V63.1816H277.753V50.5453ZM265.117 50.5453H277.753V63.1816H265.117V50.5453ZM252.48 50.5453H265.117V63.1816H252.48V50.5453ZM252.48 -0.00017944H265.117V12.6362H252.48V-0.00017944ZM302.977 -0.00017944H315.613V12.6362H302.977V-0.00017944ZM302.977 12.6362H315.613V25.2725H302.977V12.6362ZM302.977 25.2725H315.613V37.9089H302.977V25.2725ZM302.977 37.9089H315.613V50.5453H302.977V37.9089ZM302.977 50.5453H315.613V63.1816H302.977V50.5453ZM315.613 -0.00017944H328.249V12.6362H315.613V-0.00017944ZM315.613 50.5453H328.249V63.1816H315.613V50.5453ZM315.613 25.2725H328.249V37.9089H315.613V25.2725ZM328.249 -0.00017944H340.886V12.6362H328.249V-0.00017944ZM328.249 50.5453H340.886V63.1816H328.249V50.5453ZM353.473 -0.00017944H366.109V12.6362H353.473V-0.00017944ZM366.109 -0.00017944H378.745V12.6362H366.109V-0.00017944ZM378.745 -0.00017944H391.382V12.6362H378.745V-0.00017944ZM366.109 12.6362H378.745V25.2725H366.109V12.6362ZM366.109 25.2725H378.745V37.9089H366.109V25.2725ZM366.109 37.9089H378.745V50.5453H366.109V37.9089ZM366.109 50.5453H378.745V63.1816H366.109V50.5453Z" | ||
| fill="currentColor" | ||
| /> | ||
| </svg> |
Contributor
There was a problem hiding this comment.
Ensure the home link has an explicit accessible name (not just the SVG).
Consider adding aria-label on the <motion.a> and making the SVG either decorative (aria-hidden) or strictly aria-labelledby-based to avoid duplicate naming paths.
- <motion.a
- href="/"
- className="flex items-center gap-2 text-neutral-100 hover:text-white transition-colors"
- >
- <SupersetLogo />
- </motion.a>
+ <motion.a
+ href="/"
+ aria-label="Superset home"
+ className="flex items-center gap-2 text-neutral-100 hover:text-white transition-colors"
+ >
+ <SupersetLogo />
+ </motion.a>Also applies to: 43-51
🤖 Prompt for AI Agents
In apps/marketing/src/app/components/Header/Header.tsx around lines 11-23 (and
similarly for 43-51), the anchor wrapping the logo must have an explicit
accessible name instead of relying on the SVG; add a clear aria-label (e.g.,
"Home" or "Go to homepage") to the <motion.a> element, and make the SVG purely
decorative by removing its aria-label/title or setting aria-hidden="true" (or
alternatively wire the SVG to the link via aria-labelledby if you need the SVG
to provide the name). Ensure you do not leave both the link and SVG with
separate accessible names to avoid duplicate announcements.
Comment on lines
+26
to
+45
| useEffect(() => { | ||
| const startTimeout = setTimeout(() => { | ||
| setIsTyping(true); | ||
| }, delay); | ||
|
|
||
| return () => clearTimeout(startTimeout); | ||
| }, [delay]); | ||
|
|
||
| useEffect(() => { | ||
| if (!isTyping) return; | ||
|
|
||
| if (displayedText.length < text.length) { | ||
| const timeout = setTimeout(() => { | ||
| setDisplayedText(text.slice(0, displayedText.length + 1)); | ||
| }, speed); | ||
|
|
||
| return () => clearTimeout(timeout); | ||
| } | ||
| }, [displayedText, isTyping, speed, text]); | ||
|
|
Contributor
There was a problem hiding this comment.
Reset typing state when text changes (and handle shorter text).
Right now, changing text mid-stream can produce jumps or leave stale characters when the new string is shorter.
useEffect(() => {
+ // Reset when the content changes.
+ setDisplayedText("");
+ setIsTyping(false);
const startTimeout = setTimeout(() => {
setIsTyping(true);
}, delay);
return () => clearTimeout(startTimeout);
-}, [delay]);
+}, [delay, text]);🤖 Prompt for AI Agents
In
apps/marketing/src/app/components/HeroSection/components/TypewriterText/TypewriterText.tsx
around lines 26 to 45, the component doesn't reset typing state when the
incoming `text` prop changes, causing jumps or leftover characters if the new
text is shorter; add an effect that watches `text` and on change clears any
pending timers, resets `displayedText` to either '' or the truncated new text
(if new text is shorter than current displayedText), and resets `isTyping` (or
restarts the initial delay) so the typewriter restarts cleanly; ensure timers
are cleared in the cleanup to avoid races.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
This was referenced Dec 14, 2025
Merged
Merged
Merged
Closed
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.