ZO: Added patchnotes for home page, added home page package#2721
Conversation
WalkthroughThis PR introduces new environment variables in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant PageHome
participant PatchNotesCard
participant GitHubAPI
User->>App: Open application
App->>PageHome: Render new PageHome (from @genshin-optimizer/zzz/page-home)
PageHome->>PatchNotesCard: Render PatchNotesCard
PatchNotesCard->>GitHubAPI: Fetch patch notes using __VERSION__
GitHubAPI-->>PatchNotesCard: Return patch data
PatchNotesCard-->>PageHome: Display patch notes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[zzz-frontend] [Wed Feb 12 10:31:40 UTC 2025] - Deployed 452c5bf to https://genshin-optimizer-prs.github.io/pr/2721/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Feb 12 10:32:05 UTC 2025] - Deployed 452c5bf to https://genshin-optimizer-prs.github.io/pr/2721/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Feb 12 10:33:01 UTC 2025] - Deployed 452c5bf to https://genshin-optimizer-prs.github.io/pr/2721/sr-frontend (Takes 3-5 minutes after this completes to be available) [Wed Feb 12 20:27:08 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: 2
🧹 Nitpick comments (1)
libs/zzz/page-home/src/index.tsx (1)
73-77: Loading state needs some caffeine... I mean, stylingJust showing 'Loading...' text? Let's make it more polished with a proper skeleton loader.
- {isLoaded ? ( - <ReactMarkdown children={text} remarkPlugins={[remarkGfm]} /> - ) : ( - 'Loading...' - )} + {isLoaded ? ( + <ReactMarkdown remarkPlugins={[remarkGfm]}> + {text} + </ReactMarkdown> + ) : ( + <Box sx={{ p: 2 }}> + <Skeleton variant="text" sx={{ mb: 1 }} /> + <Skeleton variant="text" sx={{ mb: 1 }} /> + <Skeleton variant="text" /> + </Box> + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/zzz-frontend/.env(1 hunks)apps/zzz-frontend/src/app/App.tsx(1 hunks)apps/zzz-frontend/src/app/PageHome.tsx(0 hunks)apps/zzz-frontend/vite.config.mts(2 hunks)libs/zzz/localization/assets/locales/en/page_home.json(1 hunks)libs/zzz/page-home/.babelrc(1 hunks)libs/zzz/page-home/.eslintrc.json(1 hunks)libs/zzz/page-home/README.md(1 hunks)libs/zzz/page-home/project.json(1 hunks)libs/zzz/page-home/src/index.tsx(1 hunks)libs/zzz/page-home/tsconfig.eslint.json(1 hunks)libs/zzz/page-home/tsconfig.json(1 hunks)libs/zzz/page-home/tsconfig.lib.json(1 hunks)tsconfig.base.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/zzz-frontend/src/app/PageHome.tsx
✅ Files skipped from review due to trivial changes (7)
- libs/zzz/page-home/README.md
- libs/zzz/page-home/.babelrc
- libs/zzz/page-home/tsconfig.lib.json
- libs/zzz/page-home/tsconfig.eslint.json
- libs/zzz/page-home/project.json
- libs/zzz/page-home/.eslintrc.json
- libs/zzz/page-home/tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
libs/zzz/page-home/src/index.tsx
[error] 74-74: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: call-build / build
🔇 Additional comments (4)
apps/zzz-frontend/src/app/App.tsx (1)
4-4: LGTM! yawnsThe import path change looks good. Moving PageHome to a separate package improves modularity. Just like how I separate my coffee breaks into 5-minute intervals to stay awake...
apps/zzz-frontend/vite.config.mts (1)
9-9: Looks good, but let's verify the version assertion... sips coffeeThe version configuration looks solid. Just make sure the assertion syntax is supported in your Node.js version. I've been burned by this at 3 AM before...
Also applies to: 68-68
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check Node.js version for JSON import assertions support node --versionLength of output: 24
Node.js version supports JSON import assertions – configuration is solid.
Hey there, just double-checked with Node v22.9.0, and we're all set. JSON assertion syntax works fine here, so no worries about this configuration.
libs/zzz/localization/assets/locales/en/page_home.json (1)
1-11: LGTM! yawnsThe JSON structure looks clean... just like my desk after my 4th coffee. Nice hierarchical organization with the
quickLinksCard. The tooltips are helpful for those late-night browsing sessions.tsconfig.base.json (1)
203-204: Looks good... sips coffeeThe path mappings are as organized as my browser tabs at 3 AM. Nice consistent formatting with the rest of the paths. The new
page-homemapping follows our project structure perfectly.
| function PatchNotesCard() { | ||
| const { t } = useTranslation('page_home') | ||
| const [{ isLoaded, text }, setState] = useState({ isLoaded: false, text: '' }) | ||
| useEffect(() => { | ||
| const regex = /^(\d+)\.(\d+)\.(\d+)$/ | ||
| const minorVersion = __VERSION__.replace(regex, `$1.$2.${0}`) | ||
| fetch(process.env['NX_URL_GITHUB_API_ZZZ_RELEASES'] + minorVersion) | ||
| .then((res) => res.arrayBuffer()) | ||
| .then((buffer) => { | ||
| const decoder = new TextDecoder('utf-8') | ||
| const data = decoder.decode(buffer) | ||
| const release = JSON.parse(data) | ||
| setState({ isLoaded: true, text: release.body }) | ||
| }) | ||
| .catch((err) => console.log('Error: ' + err.message)) | ||
| }, []) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling needs some love... rubs eyes
The fetch error handling just logs to console. We should show a user-friendly error message. Trust me, I've spent too many nights debugging silent failures.
- .catch((err) => console.log('Error: ' + err.message))
+ .catch((err) => {
+ console.error('Failed to fetch patch notes:', err);
+ setState({ isLoaded: true, text: 'Failed to load patch notes. Please try again later.' });
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function PatchNotesCard() { | |
| const { t } = useTranslation('page_home') | |
| const [{ isLoaded, text }, setState] = useState({ isLoaded: false, text: '' }) | |
| useEffect(() => { | |
| const regex = /^(\d+)\.(\d+)\.(\d+)$/ | |
| const minorVersion = __VERSION__.replace(regex, `$1.$2.${0}`) | |
| fetch(process.env['NX_URL_GITHUB_API_ZZZ_RELEASES'] + minorVersion) | |
| .then((res) => res.arrayBuffer()) | |
| .then((buffer) => { | |
| const decoder = new TextDecoder('utf-8') | |
| const data = decoder.decode(buffer) | |
| const release = JSON.parse(data) | |
| setState({ isLoaded: true, text: release.body }) | |
| }) | |
| .catch((err) => console.log('Error: ' + err.message)) | |
| }, []) | |
| function PatchNotesCard() { | |
| const { t } = useTranslation('page_home') | |
| const [{ isLoaded, text }, setState] = useState({ isLoaded: false, text: '' }) | |
| useEffect(() => { | |
| const regex = /^(\d+)\.(\d+)\.(\d+)$/ | |
| const minorVersion = __VERSION__.replace(regex, `$1.$2.${0}`) | |
| fetch(process.env['NX_URL_GITHUB_API_ZZZ_RELEASES'] + minorVersion) | |
| .then((res) => res.arrayBuffer()) | |
| .then((buffer) => { | |
| const decoder = new TextDecoder('utf-8') | |
| const data = decoder.decode(buffer) | |
| const release = JSON.parse(data) | |
| setState({ isLoaded: true, text: release.body }) | |
| }) | |
| .catch((err) => { | |
| console.error('Failed to fetch patch notes:', err); | |
| setState({ isLoaded: true, text: 'Failed to load patch notes. Please try again later.' }); | |
| }) | |
| }, []) | |
| } |
| NX_URL_GITHUB_API_ZZZ_RELEASES=https://api.github.com/repos///releases/tags/ | ||
| NX_URL_GITHUB_GO_CURRENT_VERSION= |
There was a problem hiding this comment.
Ugh, incomplete GitHub API URL... downs another espresso
The GitHub API URL is missing the repository information in the path. This will cause the fetch to fail faster than my attempts to maintain a normal sleep schedule.
The URL should be in this format: https://api.github.com/repos/{owner}/{repo}/releases/tags/
Also, why is NX_URL_GITHUB_GO_CURRENT_VERSION empty? Are we planning to use it later? If not, we should remove it to avoid confusion.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash
# Verify if the API endpoint is accessible
curl -I "${NX_URL_GITHUB_API_ZZZ_RELEASES}0.1.0"Length of output: 10694
Heads up: Incomplete GitHub API URL & empty version variable found!
It looks like the environment variable NX_URL_GITHUB_API_ZZZ_RELEASES is missing the necessary repository information ({owner}/{repo}), which ends up causing the URL to be malformed—hence the failed connection when attempting to query "0.1.0". Additionally, NX_URL_GITHUB_GO_CURRENT_VERSION remains empty, so unless it’s planned for future use, it’s probably best to remove it to avoid confusion.
- File:
apps/zzz-frontend/.env(Lines 1-2) - Issue 1:
NX_URL_GITHUB_API_ZZZ_RELEASESshould be in the formathttps://api.github.com/repos/{owner}/{repo}/releases/tags/ - Issue 2:
NX_URL_GITHUB_GO_CURRENT_VERSIONis empty and might be unnecessary.
| // https://vitejs.dev/guide/troubleshooting.html#this-package-is-esm-only | ||
| import { nxViteTsPaths } from '@nx/vite/plugins/nx-tsconfig-paths.plugin' | ||
| import { viteStaticCopy } from 'vite-plugin-static-copy' | ||
| import pkg from '../../package.json' assert { type: 'json' } |
There was a problem hiding this comment.
Do you wanna use zzz-frontend package json version instead?
There was a problem hiding this comment.
I would agree, except we have no package.json there, and we have not been tracking version for ZO.
There was a problem hiding this comment.
can also add it in project.json if its allowed, or create package.json if allowed. either way, we have options available for separate version per-site
3 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