SRO/ZO: Add footer to SR and ZZZ#2833
Conversation
WalkthroughThe pull request integrates a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as App Component
participant F as Footer Component
participant S as Suspense Wrapper
participant C as FooterContent
U->>A: Load App
A->>F: Render Footer
F->>S: Initiate Suspense loading
S->>C: Request FooterContent
alt Content loading
S-->>A: Display Skeleton fallback
else Content ready
S-->>F: Render FooterContent with version info
end
F-->>A: Footer rendered at bottom
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[zzz-frontend] [Sun Mar 9 18:51:56 UTC 2025] - Deployed f54dfba to https://genshin-optimizer-prs.github.io/pr/2833/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sun Mar 9 18:52:28 UTC 2025] - Deployed f54dfba to https://genshin-optimizer-prs.github.io/pr/2833/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Mar 9 18:53:09 UTC 2025] - Deployed f54dfba to https://genshin-optimizer-prs.github.io/pr/2833/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Mar 9 19:04:59 UTC 2025] - Deployed fc78d3e to https://genshin-optimizer-prs.github.io/pr/2833/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sun Mar 9 19:05:55 UTC 2025] - Deployed fc78d3e to https://genshin-optimizer-prs.github.io/pr/2833/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sun Mar 9 19:18:29 UTC 2025] - Deployed 7edf596 to https://genshin-optimizer-prs.github.io/pr/2833/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Mar 9 19:18:40 UTC 2025] - Deployed 7edf596 to https://genshin-optimizer-prs.github.io/pr/2833/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sun Mar 9 19:23:30 UTC 2025] - Deployed 7edf596 to https://genshin-optimizer-prs.github.io/pr/2833/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sun Mar 9 19:23:50 UTC 2025] - Deployed 7edf596 to https://genshin-optimizer-prs.github.io/pr/2833/sr-frontend (Takes 3-5 minutes after this completes to be available) [Sun Mar 9 20:12:05 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/sr-frontend/src/app/Header.tsx (2)
192-197: Dynamic commit hash implementation looks good but could be improved.Yawns The code correctly extracts the commit hash from the environment variable, but this same logic is repeated across all files. Also, no null checking before the chain of operations.
Consider extracting this to a shared utility function to avoid repeating yourself. Something like:
+ // In a shared utility file + export function getCommitHash(): string { + return process.env['NX_URL_GITHUB_GO_CURRENT_VERSION'] + ?.replace(/.*commit\//, '') + ?.slice(0, 7) || 'unknown'; + } // Then in this file - (Dev Mode -{' '} - {process.env['NX_URL_GITHUB_GO_CURRENT_VERSION'] - ?.replace(/.*commit\//, '') - .slice(0, 7)} - ) + (Dev Mode - {getCommitHash()})I mean, I could refactor this myself, but gestures vaguely too many PRs to review already...
340-346: Duplicated logic in mobile view.Same commit hash extraction code as in desktop view. Sighs deeply Duplicated code is making my eye twitch.
Extract this logic to a shared utility function to avoid duplication between desktop and mobile views.
apps/frontend/src/app/Header.tsx (2)
217-223: Same commit hash extraction pattern repeated.Rubs tired eyes I'm seeing triple now. This exact same code pattern appears in three different files.
This screams for a shared utility function that all three frontends could import. DRY principle applies here - especially for error handling and consistency.
+ // In a common utility file + export function getDisplayCommitHash(): string { + const fullHash = process.env['NX_URL_GITHUB_GO_CURRENT_VERSION']; + if (!fullHash) return 'unknown'; + return fullHash.replace(/.*commit\//, '').slice(0, 7); + } // Then in all header files: - (Dev Mode -{' '} - {process.env['NX_URL_GITHUB_GO_CURRENT_VERSION'] - ?.replace(/.*commit\//, '') - .slice(0, 7)} - ) + (Dev Mode - {getDisplayCommitHash()})
396-402: Yet more duplicated hash extraction code.Another copy of the same pattern... yawns audibly My coffee isn't strong enough for this.
Same recommendation as before - extract to a shared utility to reduce duplication and centralize any future changes.
apps/zzz-frontend/src/app/Header.tsx (2)
183-189: Triple déjà vu with commit hash extraction.Blinks slowly Same code pattern for the third time. This is almost hypnotic at this point.
This strengthens the case for a utility function across all frontends. Also consider adding error handling for when the environment variable is undefined:
function getCommitHash(): string { const envVar = process.env['NX_URL_GITHUB_GO_CURRENT_VERSION']; if (!envVar) return 'unknown'; try { return envVar.replace(/.*commit\//, '').slice(0, 7); } catch (e) { console.error('Failed to parse commit hash:', e); return 'error'; } }Not that anyone reads console logs anymore... sighs
335-341: The final instance of duplicated hash extraction.Last one, thank goodness. stretches Been staring at the same code for what feels like hours now.
Same recommendation - extract to a utility. All instances should be consistent in how they handle edge cases.
The implementation works as intended though, so this is just a refactoring suggestion to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/app/Header.tsx(2 hunks)apps/sr-frontend/src/app/Header.tsx(2 hunks)apps/zzz-frontend/src/app/Header.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
This reverts commit 2083ab7.
nguyentvan7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/zzz-frontend/src/app/Footer.tsx (1)
1-46: Duplicate Footer implementation across applications.This code is identical to the SR frontend Footer... stares blankly at screen I've been reviewing code for so long I'm seeing double. Definitely a candidate for shared component extraction.
Since this Footer component is identical in both applications, consider moving it to a shared location:
-// In both sr-frontend and zzz-frontend Footer.tsx files -import { AppBar, Box, Skeleton, Typography } from '@mui/material' -import { Suspense } from 'react' - -declare const __VERSION__: string -export default function Footer() { - return ( - <Suspense fallback={<Skeleton variant="rectangular" height={64} />}> - <FooterContent /> - </Suspense> - ) -} -function FooterContent() { - return ( - <AppBar - component="footer" - position="static" - sx={{ bgcolor: 'neutral800.main' }} - elevation={0} - > - <Box - display="flex" - justifyContent="space-between" - sx={{ px: 2, py: 1 }} - gap={2} - > - <Typography - variant="caption" - sx={{ color: 'neutral400.main', textAlign: 'right' }} - > - Version: - <a - href={ - process.env.NX_URL_GITHUB_GO_CURRENT_VERSION || - `${process.env.NX_URL_GITHUB_GO}/releases` - } - target="_blank" - rel="noreferrer" - style={{ color: 'inherit' }} - > - {__VERSION__} - </a> - </Typography> - </Box> - </AppBar> - ) -}And create a shared component in something like:
// In a shared package, e.g. @genshin-optimizer/common/ui import { AppBar, Box, Skeleton, Typography } from '@mui/material' import { Suspense } from 'react' declare const __VERSION__: string export default function Footer() { return ( <Suspense fallback={<Skeleton variant="rectangular" height={64} />}> <FooterContent /> </Suspense> ) } function FooterContent() { return ( <AppBar component="footer" position="static" sx={{ bgcolor: 'neutral800.main' }} elevation={0} > <Box display="flex" justifyContent="space-between" sx={{ px: 2, py: 1 }} gap={2} > <Typography variant="caption" sx={{ color: 'neutral400.main', textAlign: 'right' }} > Version: <a href={ process.env.NX_URL_GITHUB_GO_CURRENT_VERSION || `${process.env.NX_URL_GITHUB_GO}/releases` } target="_blank" rel="noreferrer" style={{ color: 'inherit' }} > {__VERSION__} </a> </Typography> </Box> </AppBar> ) }Then import it in both apps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/sr-frontend/src/app/App.tsx(2 hunks)apps/sr-frontend/src/app/Footer.tsx(1 hunks)apps/zzz-frontend/src/app/App.tsx(2 hunks)apps/zzz-frontend/src/app/Footer.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (6)
apps/sr-frontend/src/app/App.tsx (2)
17-17: New import for Footer component looks good.Importing the Footer component... yawn... just another component to maintain. But at least it's clean and clear where it's coming from.
87-89: Footer implementation with proper bottom positioning.The Box with flexGrow={1} is a solid approach to push the footer to the bottom. Been using this trick since... checks watch... too many sleepless nights ago. The comment is helpful for any poor soul who has to maintain this code at 3 AM.
apps/zzz-frontend/src/app/App.tsx (2)
19-19: Footer import looks good.Another import, another day... At least it's consistent with the sr-frontend implementation. sips coffee
82-84: Footer positioning implementation is correct.Same implementation as in sr-frontend. The flexGrow trick works well here too. rubs eyes It's nice to see consistency across codebases, makes my late-night debugging sessions slightly less painful.
apps/sr-frontend/src/app/Footer.tsx (2)
1-3: Appropriate imports for Footer component.All the necessary imports for Material UI components and React's Suspense. Nothing fancy, just the basics... like my diet of coffee and energy drinks.
5-11: Footer component with Suspense is well structured.The component with Suspense fallback pattern is good... I've coded this pattern so many times I could do it in my sleep (which I probably have). The Skeleton as fallback is a nice touch for UX.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/zzz-frontend/src/app/Footer.tsx (2)
36-40:⚠️ Potential issueCommit hash extraction needs safer implementation.
Eyes half-closed This commit hash extraction is pretty brittle. If
NX_URL_GITHUB_GO_CURRENT_VERSIONis undefined or doesn't contain "commit/", this will break in weird ways. We should add some defensive coding here.- {process.env.NX_URL_GITHUB_GO_CURRENT_VERSION?.replace( - /.*commit\//, - '' - ).substring(0, 7)} + {process.env.NX_URL_GITHUB_GO_CURRENT_VERSION + ? process.env.NX_URL_GITHUB_GO_CURRENT_VERSION.includes('commit/') + ? process.env.NX_URL_GITHUB_GO_CURRENT_VERSION.replace(/.*commit\//, '').substring(0, 7) + : 'latest' + : 'latest'}
25-41:⚠️ Potential issueMissing implementation for dev mode commit hash display.
Massages temples The PR mentioned showing commit hash in dev mode, but I'm not seeing any conditional logic based on the environment. We should add something to specifically handle dev mode vs production.
<Typography variant="caption" sx={{ color: 'neutral400.main' }}> Build: <a href={ process.env.NX_URL_GITHUB_GO_CURRENT_VERSION || `${process.env.NX_URL_GITHUB_GO}/releases` } target="_blank" rel="noreferrer" style={{ color: 'inherit' }} > + {process.env.NODE_ENV === 'development' + ? 'dev-' + (process.env.NX_URL_GITHUB_GO_CURRENT_VERSION?.replace(/.*commit\//, '').substring(0, 7) || 'local') + : process.env.NX_URL_GITHUB_GO_CURRENT_VERSION?.replace(/.*commit\//, '').substring(0, 7) || 'latest'} - {process.env.NX_URL_GITHUB_GO_CURRENT_VERSION?.replace( - /.*commit\//, - '' - ).substring(0, 7)} </a> </Typography>
🧹 Nitpick comments (3)
apps/zzz-frontend/src/app/Footer.tsx (3)
1-2: Imports look good, but check if Suspense is really needed.Staring at the screen with bloodshot eyes These imports look fine, but I'm not seeing any lazy-loaded components or async operations that would justify using Suspense here. It's adding complexity for potentially no reason, unless there's something I'm missing in my sleep-deprived state.
-import { AppBar, Box, Skeleton, Typography } from '@mui/material' -import { Suspense } from 'react' +import { AppBar, Box, Typography } from '@mui/material'
4-10: Simplify component if Suspense isn't needed.Chugs coffee If we're not lazy loading anything, we can remove the Suspense wrapper and just render the FooterContent directly. One less thing to debug at 3am.
-export default function Footer() { - return ( - <Suspense fallback={<Skeleton variant="rectangular" height={64} />}> - <FooterContent /> - </Suspense> - ) -} +export default function Footer() { + return <FooterContent /> +}
19-24: Using space-between with only one child element.Rubs eyes tiredly You're using
justifyContent="space-between"but there's only one child element in the Box. Space-between won't do anything unless you have at least two elements to create space between. Either add another element or change the justification.<Box display="flex" - justifyContent="space-between" + justifyContent="flex-start" sx={{ px: 2, py: 1 }} gap={2} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/sr-frontend/src/app/Footer.tsx(1 hunks)apps/zzz-frontend/src/app/Footer.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/sr-frontend/src/app/Footer.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (1)
apps/zzz-frontend/src/app/Footer.tsx (1)
28-31:Details
❓ Verification inconclusive
URL handling looks fragile.
Sighs deeply The URL handling here depends on environment variables being set correctly. Make sure these are properly configured in your build pipeline or you'll get mysterious bugs appearing at midnight that'll keep you up debugging.
🏁 Script executed:
#!/bin/bash # Check if the environment variables are defined in any config files echo "Checking for NX_URL_GITHUB_GO_CURRENT_VERSION and NX_URL_GITHUB_GO in environment configs..." rg -A 2 "NX_URL_GITHUB_GO_CURRENT_VERSION|NX_URL_GITHUB_GO" --type=js --type=json --type=txt --type=yaml --type=envLength of output: 340
Heads-Up on URL Environment Variables – Please Verify Pipeline Settings
- The
hrefinapps/zzz-frontend/src/app/Footer.tsx(Lines 28-31) usesprocess.env.NX_URL_GITHUB_GO_CURRENT_VERSIONand falls back to a releases URL based onprocess.env.NX_URL_GITHUB_GO.- Our initial search didn’t confirm these variables in local config files (likely because they’re expected to be injected via your CI/CD pipeline).
- Ensure your build pipeline properly supplies these environment variables to avoid mysterious runtime issues.
2 participants
Describe your changes
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit