fix: logout bug, React 19 compatibility, and DX improvements #14
Conversation
Frontend:
- Change handleLogout from window.location.href to POST fetch()
- Use credentials: 'include' for session cookie
- Navigate to homepage after logout via React Router
Backend:
- Return JSON { success: true } instead of redirect
- Fixes 'Cannot GET /api/auth/logout' error
- Remove redundant @tiptap/extension-link (already in frontend) - Remove redundant axios (already in backend) - Add concurrently for parallel server startup - Add 'npm start' script to run both frontend and backend
- Switch from react-diff-viewer (React 15/16 only) to react-diff-viewer-continued - Resolves peer dependency conflict with React 19 - npm install now works without --legacy-peer-deps
There was a problem hiding this comment.
Pull request overview
This PR addresses critical bugs and compatibility issues, including fixing the logout functionality (converting from GET to POST request with JSON response), resolving React 19 peer dependency conflicts by replacing react-diff-viewer with react-diff-viewer-continued, and improving developer experience with a concurrent start script and dependency cleanup.
Key Changes:
- Fixed logout bug by changing backend endpoint to return JSON and frontend to use POST method with fetch
- Replaced incompatible
react-diff-viewerpackage with React 19-compatiblereact-diff-viewer-continued - Added
npm startscript using concurrently to run both servers simultaneously
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added concurrent start script, removed redundant dependencies, added concurrently dev dependency |
| frontend/package.json | Replaced react-diff-viewer with react-diff-viewer-continued for React 19 compatibility |
| frontend/src/pages/ReviewPage.jsx | Updated import to use new diff viewer package, fixed indentation |
| frontend/src/pages/DashboardPage.jsx | Converted logout handler to async fetch with POST method, multiple formatting improvements |
| backend/src/server.js | Changed logout endpoint to return JSON instead of redirect, removed unused CORS origin, formatting improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleLogout = async () => { | ||
| try { | ||
| await fetch('/api/auth/logout', { | ||
| method: 'POST', | ||
| credentials: 'include' | ||
| }); | ||
| } catch (error) { | ||
| console.error('Logout error:', error); | ||
| } | ||
| navigate('/'); | ||
| }; |
There was a problem hiding this comment.
The logout handler doesn't check the response status from the logout request. If the request fails (e.g., network error or server error), the user is still navigated to '/' without confirmation that they were logged out. Consider checking the response:
const handleLogout = async () => {
try {
const response = await fetch('/api/auth/logout', {
method: 'POST',
credentials: 'include'
});
if (!response.ok) {
console.error('Logout failed:', await response.text());
// Optionally show an error to the user
}
} catch (error) {
console.error('Logout error:', error);
}
navigate('/');
};| origin: [FRONTEND_URL, 'http://localhost:3000', 'http://localhost:5173'], | ||
| credentials: true | ||
| app.use(cors({ | ||
| origin: [FRONTEND_URL, 'http://localhost:5173'], |
There was a problem hiding this comment.
[nitpick] The CORS origin array may contain a duplicate entry. Since FRONTEND_URL defaults to 'http://localhost:5173' on line 14, the origin array will be ['http://localhost:5173', 'http://localhost:5173'] in the default case. Consider simplifying:
app.use(cors({
origin: FRONTEND_URL === 'http://localhost:5173'
? [FRONTEND_URL]
: [FRONTEND_URL, 'http://localhost:5173'],
credentials: true
}));| origin: [FRONTEND_URL, 'http://localhost:5173'], | |
| origin: FRONTEND_URL === 'http://localhost:5173' | |
| ? [FRONTEND_URL] | |
| : [FRONTEND_URL, 'http://localhost:5173'], |
| app.post('/api/auth/logout', (req, res, next) => { | ||
| req.logout(function(err) { | ||
| req.logout(function (err) { | ||
| if (err) { return next(err); } | ||
| res.redirect('/'); | ||
| res.json({ success: true }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The updated logout endpoint lacks test coverage. Given that the backend has comprehensive test coverage for other features (branch_locking, cache integration, suggestions, etc.), consider adding tests to verify:
- POST method returns JSON with
{success: true} - Session is properly terminated
- Subsequent authenticated requests fail after logout
| req.logout(function (err) { | ||
| if (err) { return next(err); } | ||
| res.redirect('/'); | ||
| res.json({ success: true }); |
There was a problem hiding this comment.
The logout endpoint should destroy the session after logging out to ensure complete cleanup. Consider adding req.session.destroy() after the logout callback:
app.post('/api/auth/logout', (req, res, next) => {
req.logout(function (err) {
if (err) { return next(err); }
req.session.destroy((err) => {
if (err) { return next(err); }
res.json({ success: true });
});
});
});This ensures the session is fully cleared from the store.
| res.json({ success: true }); | |
| req.session.destroy((err) => { | |
| if (err) { return next(err); } | |
| res.json({ success: true }); | |
| }); |
1 participant
Closes #12
Summary
This PR fixes several bugs and improves the developer experience.
Changes
Bug Fixes
Logout Button Fix
Cannot GET /api/auth/logoutfetch()with POST, backend returns JSONReact 19 Peer Dependency Conflict (Fixes #12)
npm installfailed with peer dependency conflictreact-diff-viewer@3.1.1only supports React 15/16react-diff-viewer-continued@4.0.5(React 19 compatible)npm installnow works without--legacy-peer-depsDeveloper Experience
Package Cleanup
Concurrent Start Script
npm startto run both frontend and backend togetherTesting