feat: add toast notifications for github token errors and fetch success #109
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
thissidemayur
changed the title
mehul-m-prajapati
left a comment
mehul-m-prajapati
left a comment
There was a problem hiding this comment.
please remove all indentation and quote changes which are coming from vscode config
backend/server.js
Outdated
| // Middleware | ||
| app.use(bodyParser.json()); | ||
| app.use(session({ | ||
| console.log("process.env.SESSION_SECRET: ", process.env.SESSION_SECRET); |
There was a problem hiding this comment.
remove console.log
backend/package.json
Outdated
| "scripts": { | ||
| "start": "nodemon server.js", | ||
| "start": "nodemon server.js", | ||
| "dev": "nodemon server.js", |
There was a problem hiding this comment.
remove dev command
|
Hi @mehul-m-prajapati sir👋 I noticed that the “Auto Label PRs and Issues with gssoc2025” workflow failed on my PR due to permission issues from a forked repo ( Just wanted to let you know — my PR is still functional, but the label didn’t get added automatically. You might want to check the GitHub Actions permission settings or add the label manually if needed. Thanks! |
|
@thissidemayur : Please resolve conflicts |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes include the removal of the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/pages/Home/Home.tsx (1)
57-57: Restore or RemovedataErrorto Match Updated HookThe
useGitHubDatahook no longer returns anerrorfield (it’s commented out), sodataErrorinHome.tsxwill always beundefined. You can either re-introduceerrorin the hook’s return value or remove the unused destructuring in the component.• In
src/hooks/useGitHubData.ts:
– Re-add theerrorproperty to the returned object (uncomment or includeerrorin thereturn { … }).
• Insrc/pages/Home/Home.tsx:
– Removeerror: dataErrorfrom the hook call, or adjust error handling to use the hook’s new API.Example diff if re-adding in the hook:
return { issues, prs, loading, - // error, + error, fetchData, };Example diff if removing from Home:
- const { issues, prs, loading, error: dataError, fetchData } = useGitHubData(octokit); + const { issues, prs, loading, fetchData } = useGitHubData(octokit);
♻️ Duplicate comments (2)
src/components/Navbar.tsx (1)
62-69: Reconsider the commented code given past feedback.Based on the past review comments, there was specific feedback to "remove comments" which you acknowledged. Adding new commented-out code may contradict that previous guidance.
Consider either:
- Remove the commented code entirely if the signup feature isn't planned for the immediate future
- Create a separate issue/task to track the signup feature implementation
- {/* Uncomment if Signup needed - <Link - to="/signup" - className="text-lg font-medium hover:text-gray-300 transition-all px-2 py-1 border border-transparent hover:border-gray-400 rounded" - > - Signup - </Link> - */}backend/server.js (1)
1-7: Quote-style flip re-introduces inconsistencyThe codebase previously standardised on single quotes for CommonJS imports (see past review comment). Converting these few lines to double quotes breaks that convention and may trigger lint-rule violations.
-const express = require("express"); -const mongoose = require("mongoose"); -const session = require("express-session"); -const passport = require("passport"); -const bodyParser = require("body-parser"); -require("dotenv").config(); -const cors = require("cors"); +const express = require('express'); +const mongoose = require('mongoose'); +const session = require('express-session'); +const passport = require('passport'); +const bodyParser = require('body-parser'); +require('dotenv').config(); +const cors = require('cors');
🧹 Nitpick comments (4)
backend/.env.sample (1)
1-3: Restore trailing newline to satisfy linter & tooling expectations
dotenv-linteris flagging the missing blank line. Re-add a newline so editors and CLI tools that rely on the POSIX convention don’t complain.SESSION_SECRET=your-secret-key +src/hooks/useGitHubAuth.ts (1)
3-3: Remove unused import.The
toastimport is added but not used anywhere in this file. This creates unnecessary bundle size and may cause linting issues.-import toast from 'react-hot-toast';src/hooks/useGitHubData.ts (2)
50-55: Improve toast notification logic and formatting.The success toast logic is good, but the formatting could be improved for better readability.
- if (issuesResponse?.length || prsResponse?.length) toast.success( - `Fetched ${issuesResponse.length} issues and ${prsResponse.length} PRs successfully.` - ); - - else toast("⚠️ No issues or PRs found", { icon: "🕵️♂️" }); + if (issuesResponse?.length || prsResponse?.length) { + toast.success( + `Fetched ${issuesResponse.length} issues and ${prsResponse.length} PRs successfully.` + ); + } else { + toast("⚠️ No issues or PRs found", { icon: "🕵️♂️" }); + }
58-66: Improve error handling structure and messages.The error handling logic is sound, but the structure and some messages could be improved for consistency and clarity.
- } catch (err: any) { - if (err.status === 401 || err.status === 403) { - toast.error("Please provide a valid GitHub token "); - } - else if (err.status === 422) toast.error("User not found") - else { - toast.error("something went wrong while fetching data") - } + } catch (err: any) { + if (err.status === 401 || err.status === 403) { + toast.error("Please provide a valid GitHub token"); + } else if (err.status === 422) { + toast.error("User not found"); + } else { + toast.error("Something went wrong while fetching data"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/.env.sample(1 hunks)backend/package.json(1 hunks)backend/server.js(1 hunks)package.json(1 hunks)src/Routes/Login/Login.tsx(0 hunks)src/components/Navbar.tsx(3 hunks)src/hooks/useGitHubAuth.ts(1 hunks)src/hooks/useGitHubData.ts(4 hunks)src/pages/Home/Home.tsx(6 hunks)
💤 Files with no reviewable changes (1)
- src/Routes/Login/Login.tsx
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
backend/.env.sample
[warning] 3-3: [EndingBlankLine] No blank line at the end of the file
🔇 Additional comments (11)
src/components/Navbar.tsx (2)
25-25: LGTM! Good styling improvement.Adding
items-centerto the desktop links container properly centers the navigation elements vertically, improving the visual alignment.
102-102: Login route and component remain functionalI’ve verified that the Login component and its routes are still present and correctly configured:
- src/pages/Login/Login.tsx defines the Login component
- src/Routes/Router.tsx includes
<Route path="/login" element={<Login />} />- Navbar links (
to="/login") at lines 51 and 130 in src/components/Navbar.tsxNo further action required.
package.json (1)
52-53: Formatting-only change looks goodThe addition of a trailing newline is benign and aligns with POSIX style guidelines for text files.
backend/package.json (1)
6-7: Indentation tweak is harmlessOnly whitespace changed; script definition remains the same. No concerns.
src/hooks/useGitHubAuth.ts (1)
12-12: AllgetOctokitusages safely handleundefinedThe
getOctokitchange to returnundefinedinstead ofnullposes no risk:
- In useGitHubData,
fetchDataimmediately guards on!octokit(and!username) before making any requests.- In Home.tsx, you only invoke
fetchDatavia form submission, and the form’s required fields ensure bothusernameandtokenare present—otherwise, the hook’s guard will show an error toast.No additional changes are needed.
src/hooks/useGitHubData.ts (1)
26-28: Fix syntax error and improve error message.There's a syntax error on line 27 - missing semicolon before return statement. Also, the error message could be more descriptive.
- if (!octokit || !username) { - toast.error("PAT not found"); return - } + if (!octokit || !username) { + toast.error("Please provide a valid GitHub username and Personal Access Token"); + return; + }Likely an incorrect or invalid review comment.
src/pages/Home/Home.tsx (5)
15-15: Good practice: avoiding naming conflicts.Renaming the import to
MuiLinkprevents potential naming conflicts and makes the code more explicit about which Link component is being used.
81-87: Improved filter logic with explicit checks.The change from
Array.includesto explicit equality checks is more readable and potentially more performant for this specific use case.
115-120: Well-implemented helper function.The
getRepoNamehelper function correctly extracts the "owner/repo" format from the repository URL, which is more useful than just the repository name.
302-302: Enhanced pagination options.Expanding the pagination options from just
[5]to[10, 25, 50]provides better user experience for handling different data volumes.
273-281: Consistent use of renamed import.Good consistency in using
MuiLinkthroughout the component after the import rename.
backend/server.js
Outdated
| app.use(cors("*")); | ||
|
|
There was a problem hiding this comment.
cors("*") is an incorrect & unsafe usage
The cors middleware expects an options object or a boolean origin shortcut. Passing a raw string literally forwards "*" to origin, which disables CORS pre-flight handling and can break some browsers. Use the documented form:
-app.use(cors("*"));
+app.use(
+ cors({
+ origin: '*', // or restrict to specific origins
+ credentials: true
+ })
+);📝 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.
| app.use(cors("*")); | |
| app.use( | |
| cors({ | |
| origin: '*', // or restrict to specific origins | |
| credentials: true | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In backend/server.js around lines 15 to 16, the usage of cors("*") is incorrect
and unsafe because it passes a raw string instead of an options object. Replace
cors("*") with cors({ origin: "*" }) to correctly configure the CORS middleware
and ensure proper pre-flight handling.
backend/server.js
Outdated
| console.log("Connected to MongoDB"); | ||
| app.listen(process.env.PORT, () => { | ||
| console.log(`Server running on port ${process.env.PORT}`); | ||
| console.log(`Server running on port ${process.env.PORT}`); | ||
| }); | ||
| }).catch((err) => { | ||
| console.log('MongoDB connection error:', err); | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| console.log("MongoDB connection error:", err); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace ad-hoc console.log with structured logging
These logs were previously requested to be removed. Re-introducing them makes debugging easier during development but pollutes stdout in production. Prefer a logger (winston / pino) or at least guard them behind NODE_ENV !== 'production'.
- console.log("Connected to MongoDB");
+ if (process.env.NODE_ENV !== 'production')
+ console.info('Connected to MongoDB');
@@
- console.log(`Server running on port ${process.env.PORT}`);
+ console.info(`Server running on port ${process.env.PORT}`);
@@
- console.log("MongoDB connection error:", err);
+ console.error('MongoDB connection error:', err);📝 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.
| console.log("Connected to MongoDB"); | |
| app.listen(process.env.PORT, () => { | |
| console.log(`Server running on port ${process.env.PORT}`); | |
| console.log(`Server running on port ${process.env.PORT}`); | |
| }); | |
| }).catch((err) => { | |
| console.log('MongoDB connection error:', err); | |
| }); | |
| }) | |
| .catch((err) => { | |
| console.log("MongoDB connection error:", err); | |
| }); | |
| if (process.env.NODE_ENV !== 'production') | |
| console.info('Connected to MongoDB'); | |
| app.listen(process.env.PORT, () => { | |
| console.info(`Server running on port ${process.env.PORT}`); | |
| }); | |
| }) | |
| .catch((err) => { | |
| console.error('MongoDB connection error:', err); | |
| }); |
🤖 Prompt for AI Agents
In backend/server.js around lines 37 to 44, replace the ad-hoc console.log
statements with structured logging using a logger like winston or pino.
Alternatively, wrap the console.log calls inside a condition that checks if
NODE_ENV is not 'production' to avoid polluting stdout in production
environments. This ensures logs are more manageable and appropriate for
different deployment stages.
|
🎉🎉 Thank you for your contribution! Your PR #109 has been merged! 🎉🎉 |
2 participants
Related Issue: Add toast messages for GitHub token errors and fetch results
Description
This PR adds toast notifications for GitHub token validation and data fetch feedback in the useGitHubData hook.
🔐 Handles invalid/missing PAT tokens
✅ Shows success toast with number of fetched issues and PRs
🧠 Adds proper error handling with status code checks (401, 403, 422, fallback)
How Has This Been Tested?
Video:
Screencast.from.22-07-25.04.35.35.PM.IST.webm
Type of Change
Summary by CodeRabbit
New Features
Chores