Added the latest repo version in about section of Settings page #970
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant GitHubAPI
User->>Settings: Load Settings Component
Settings->>GitHubAPI: GET latest release version
GitHubAPI-->>Settings: Return latest version (tag_name)
Settings->>Settings: Update version state
Settings->>User: Display version in About section
GitHubAPI-->>Settings: Error (if any)
Settings->>User: Show error notification
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Pages/Settings/index.jsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Pages/Settings/index.jsx (4)
20-20: Yo, these imports are straight fire!Bringin' in useState and useEffect like it's the last verse, that's what I'm talkin' about! These hooks are gonna make our component pop off the screen, no doubt.
40-40: Version state? That's the real MVP!Yo, you just dropped that version state like it's hot! It's gonna hold our latest release info tighter than mom's spaghetti. Good stuff, homie!
323-323: Version display? That's the cherry on top, yo!You're servin' up that version info like it's a fresh plate of mom's spaghetti. Users gonna eat this up! Keep that transparency flowin', it's all good in the hood.
331-332: New GitHub link? You're droppin' knowledge bombs!Yo, you just upgraded that GitHub link like it's goin' platinum! Direct link to the repo? That's how we roll. Users gonna be all over this like vomit on a sweater. Keep it real, keep it accessible!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Pages/Settings/index.jsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Pages/Settings/index.jsx (4)
20-20: Yo, these imports are fresher than mom's spaghetti!Adding useState and useEffect? That's the secret sauce we needed for this version-fetching feast. You're cookin' up some good React, homie!
40-40: Version state? That's the real MVP (Most Valuable Pasta)!You're servin' up that version state like it's a hot plate of spaghetti. Keepin' it fresh and ready to update. That's how we roll in the React kitchen!
323-323: Version display? That's the cherry on top of mom's spaghetti!You're servin' up that version info like it's the hottest dish in town. Users gonna eat this up faster than Eminem spits rhymes. Keep that info fresh and tasty!
331-332: This GitHub link? It's the real slim shady of URLs!You've updated that link faster than Eminem changes his lyrics. Now users can dive straight into the code like they're diving into a bowl of mom's spaghetti. That's what I call a direct hit!
Can you add a video of this? |
github-repo-version.mp4Here it is |
|
Looks good to me. What happens if GitHub is unreachable (e.g if the server is behind a firewall that blocks access)? |
|
it will be blank in place of v1.1.0 and if any error occurs, it will show a notification when settings page loads |
|
This is ok. Does the GitHub API require any tokens/keys etc? |
|
No just the url of the repository of which we want to fetch the version of |
|
OK, thanks for the information. |
There was a problem hiding this comment.
Nice work, looks good for the most part!
I think we can leverage a try/catch/finally flow here to make sure we always have a value for version.
We also use a NetworkService class to handle all of our network requests using Axios, please refactor the network request to make use of it.
Thanks for your contribution here, much appreciated!
Client/src/Pages/Settings/index.jsx
Outdated
| try { | ||
| const response = await fetch( | ||
| "https://api.github.com/repos/bluewave-labs/bluewave-uptime/releases/latest" | ||
| ); |
There was a problem hiding this comment.
We have a NetworkService class in which we use Axios to make all of our network requests. Can you refactor this to make use of the NetworkService so that all of our network requests are in one place?
Client/src/Pages/Settings/index.jsx
Outdated
| @@ -37,10 +37,30 @@ const Settings = ({ isAdmin }) => { | |||
| const [form, setForm] = useState({ | |||
| ttl: checkTTL ? (checkTTL / SECONDS_PER_DAY).toString() : 0, | |||
| }); | |||
| const [version,setVersion]=useState(""); | |||
There was a problem hiding this comment.
Let's provide a default value here in case network request fails, perhaps "unknown" for now is good.
Client/src/Pages/Settings/index.jsx
Outdated
| setVersion(data.tag_name); // Set the latest version number | ||
| } catch (error) { | ||
| createToast({ body: error.message || "Error fetching latest version" }); // Set error message | ||
| } |
There was a problem hiding this comment.
Why don't we do this in case our network request fails:
let version = "unknown"
try {
...
const data = await response.json()
version = data.tag_name
} catch (error) {
createToast(...)
} finally {
setVersion(version)
}
Now we'll always have a recent value for version, even if it is unknown.
…ing NetworkService
|
I have made the requested changes. Can you please review it ? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Pages/Settings/index.jsx (3 hunks)
- Client/src/Utils/NetworkService.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Pages/Settings/index.jsx (4)
20-20: Yo, these imports are fresh like mom's spaghetti!The new imports are on point, bringin' in the heat we need for that version fetch. It's all good in the hood!
40-40: This state's so fresh, it's making my palms sweaty!Yo, that 'version' state is comin' in hot with that "unknown" default. It's like you're reading my mind, dawg!
323-323: This About section's got more sauce than mom's spaghetti!You're droppin' that version like it's hot, and that GitHub link's pointing straight to the source. It's all coming together like a perfect rhyme, you feel me?
Also applies to: 331-332
Line range hint
353-355: Props to you for those PropTypes, dawg!You're laying down the law with those PropTypes like it's the last verse of the rap battle. That's some next-level code documentation right there!
| //Fetching latest release version from github | ||
| useEffect(() => { | ||
| const fetchLatestVersion = async () => { | ||
| let version="unknown"; | ||
| try { | ||
| const response = await networkService.fetchGithubLatestRelease(); | ||
| if (!response.status===200) { | ||
| throw new Error("Failed to fetch latest version"); | ||
| } | ||
| version=response.data.tag_name; | ||
| } catch (error) { | ||
| createToast({ body: error.message || "Error fetching latest version" }); // Set error message | ||
| } finally{ | ||
| setVersion(version); | ||
| } | ||
| }; | ||
| fetchLatestVersion(); | ||
| },[]); |
There was a problem hiding this comment.
This useEffect's got more flow than Eminem, but there's a tiny hiccup!
Yo, this version fetch is tight like mom's spaghetti, using that networkService like a pro and handling errors smoother than a fresh beat. But peep this:
- if (!response.status===200) {
+ if (response.status !== 200) {That condition's got a little stutter, you feel me? Fix that up and it'll be spittin' straight fire!
📝 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.
| //Fetching latest release version from github | |
| useEffect(() => { | |
| const fetchLatestVersion = async () => { | |
| let version="unknown"; | |
| try { | |
| const response = await networkService.fetchGithubLatestRelease(); | |
| if (!response.status===200) { | |
| throw new Error("Failed to fetch latest version"); | |
| } | |
| version=response.data.tag_name; | |
| } catch (error) { | |
| createToast({ body: error.message || "Error fetching latest version" }); // Set error message | |
| } finally{ | |
| setVersion(version); | |
| } | |
| }; | |
| fetchLatestVersion(); | |
| },[]); | |
| //Fetching latest release version from github | |
| useEffect(() => { | |
| const fetchLatestVersion = async () => { | |
| let version="unknown"; | |
| try { | |
| const response = await networkService.fetchGithubLatestRelease(); | |
| if (response.status !== 200) { | |
| throw new Error("Failed to fetch latest version"); | |
| } | |
| version=response.data.tag_name; | |
| } catch (error) { | |
| createToast({ body: error.message || "Error fetching latest version" }); // Set error message | |
| } finally{ | |
| setVersion(version); | |
| } | |
| }; | |
| fetchLatestVersion(); | |
| },[]); |
|
|
||
| /** | ||
| * ************************************ | ||
| * Fetcher github latest release version | ||
| * ************************************ | ||
| * | ||
| * @async | ||
| * @returns {Promise<AxiosResponse>} The response from the axios GET request. | ||
| * | ||
| */ | ||
| async fetchGithubLatestRelease() { | ||
| return this.axiosInstance.get('https://api.github.com/repos/bluewave-labs/bluewave-uptime/releases/latest'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo, this new method's got potential, but it's shakin' like mom's spaghetti!
The fetchGithubLatestRelease method is a solid addition, but it's got a few weak spots that might make your knees buckle:
- The URL's hardcoded like it's set in stone. What if we wanna check other repos? Let's make it flexible, ya feel me?
- There's no error handling, so if something goes wrong, it'll be like vomit on your sweater.
- We're hitting the GitHub API raw, no auth. That's gonna get us rate limited faster than you can say "Lose Yourself".
Here's how we can clean up this spaghetti:
- async fetchGithubLatestRelease() {
- return this.axiosInstance.get('https://api.github.com/repos/bluewave-labs/bluewave-uptime/releases/latest');
- }
+ async fetchGithubLatestRelease(owner, repo, token = null) {
+ const url = `https://api.github.com/repos/${owner}/${repo}/releases/latest`;
+ const headers = token ? { Authorization: `token ${token}` } : {};
+ try {
+ return await this.axiosInstance.get(url, { headers });
+ } catch (error) {
+ logger.error(`Failed to fetch latest release for ${owner}/${repo}:`, error);
+ throw error;
+ }
+ }This way, we're ready for whatever repo comes our way, we're handling errors like a pro, and we've got the option to use a token to avoid getting our palms all sweaty with rate limits. Now that's a recipe even mom would approve!
📝 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.
| /** | |
| * ************************************ | |
| * Fetcher github latest release version | |
| * ************************************ | |
| * | |
| * @async | |
| * @returns {Promise<AxiosResponse>} The response from the axios GET request. | |
| * | |
| */ | |
| async fetchGithubLatestRelease() { | |
| return this.axiosInstance.get('https://api.github.com/repos/bluewave-labs/bluewave-uptime/releases/latest'); | |
| } | |
| /** | |
| * ************************************ | |
| * Fetcher github latest release version | |
| * ************************************ | |
| * | |
| * @async | |
| * @param {string} owner - The owner of the repository | |
| * @param {string} repo - The name of the repository | |
| * @param {string} [token] - Optional GitHub API token for authentication | |
| * @returns {Promise<AxiosResponse>} The response from the axios GET request. | |
| * | |
| */ | |
| async fetchGithubLatestRelease(owner, repo, token = null) { | |
| const url = `https://api.github.com/repos/${owner}/${repo}/releases/latest`; | |
| const headers = token ? { Authorization: `token ${token}` } : {}; | |
| try { | |
| return await this.axiosInstance.get(url, { headers }); | |
| } catch (error) { | |
| logger.error(`Failed to fetch latest release for ${owner}/${repo}:`, error); | |
| throw error; | |
| } | |
| } |
Client/src/Pages/Settings/index.jsx
Outdated
| @@ -300,15 +320,16 @@ const Settings = ({ isAdmin }) => { | |||
| </Box> | |||
| <Box> | |||
| <Typography component="h2">BlueWave Uptime v1.0.0</Typography> | |||
| <Typography component="h2">Latest Version: {version}</Typography> | |||
There was a problem hiding this comment.
We don't need both of these, you can just combine them to
BlueWave Uptime: {version}
| } catch (error) { | ||
| createToast({ body: error.message || "Error fetching latest version" }); // Set error message | ||
| } finally{ | ||
| setVersion(version); |
|
Done! Can you please check |
Fixes issue #967
I have added the tag which will fetch the latest version of the release from the github repo and show it in the about section of settings page.