Feature/song card#27
Conversation
Summary of ChangesHello @aman-wednesdaysol, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundation for a comprehensive music streaming application frontend. It introduces essential features such as a robust music player, a complete user authentication system, and dynamic content display for trending tracks and search results. The changes also include significant UI/UX improvements with theming capabilities, a new responsive layout, and enhanced global styling, all while addressing critical security concerns to ensure a stable and secure user experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of features, effectively building the core of a music streaming application, including authentication, theming, music playback, search functionality, and a new UI design system. A critical security vulnerability was identified where the application globally attaches the user's session token to all API requests, leading to token leakage to external services like GitHub; this must be remediated by ensuring tokens are only sent to authorized internal endpoints. Furthermore, a critical syntax error in package.json will break dependency installation, and there are opportunities to improve maintainability, adhere to best practices, and fully utilize the new theming system by removing hardcoded values.
| "form-data": ">=2.5.4", | ||
| "next": ">=15.0.8", | ||
| "json": ">=10.0.0", | ||
| "braces": ">=3.0.3", | ||
| "cross-spawn": ">=6.0.6", | ||
| "qs": ">=6.14.1" |
There was a problem hiding this comment.
The resolutions object in your package.json is syntactically incorrect due to a missing comma and invalid formatting. This will break package installation with yarn. Each entry in the resolutions object must be a key-value pair, and entries must be separated by commas.
| "form-data": ">=2.5.4", | |
| "next": ">=15.0.8", | |
| "json": ">=10.0.0", | |
| "braces": ">=3.0.3", | |
| "cross-spawn": ">=6.0.6", | |
| "qs": ">=6.14.1" | |
| "form-data": ">=2.5.4", | |
| "next": ">=15.0.8", | |
| "json": ">=10.0.0", | |
| "braces": ">=3.0.3", | |
| "cross-spawn": ">=6.0.6", | |
| "qs": ">=6.14.1" |
app/utils/apiUtils.js
Outdated
| api.addRequestTransform((request) => { | ||
| const token = getStoredToken(); | ||
| if (token) { | ||
| request.headers.Authorization = `Bearer ${token}`; | ||
| } |
There was a problem hiding this comment.
The application's authentication token is leaked to external services. The addRequestTransform function in createApiClientWithTransForm adds the Authorization header containing the user's session token to every request made by any API client created with this utility. This includes the GitHub API client (and any other future external clients), meaning the internal application token is sent to third-party servers. This is a sensitive information leakage vulnerability that could allow external services or attackers positioned to monitor those services to steal user session tokens.
| api.addRequestTransform((request) => { | |
| const token = getStoredToken(); | |
| if (token) { | |
| request.headers.Authorization = `Bearer ${token}`; | |
| } | |
| api.addRequestTransform((request) => { | |
| const token = getStoredToken(); | |
| // Only add the Authorization header if the request is going to our internal music API | |
| if (token && request.baseURL && request.baseURL.includes('localhost:9000')) { | |
| request.headers.Authorization = `Bearer ${token}`; | |
| } | |
| if (request.data) { | |
| request.data = mapKeysDeep(request.data, (key) => snakeCase(key)); | |
| } | |
| return request; | |
| }); |
app/components/Auth/LoginForm.js
Outdated
| {loading ? 'Signing in...' : 'Sign In'} | ||
| </SubmitButton> | ||
| <LinkText> | ||
| {" Don't have an account?"} <a href="/signup">Sign up</a> |
There was a problem hiding this comment.
For internal navigation in a Next.js application, it's a best practice to use the <Link> component from next/link instead of a standard <a> tag. This enables client-side navigation and avoids a full page reload, which provides a faster and smoother user experience.
You will need to add import Link from 'next/link'; at the top of the file.
| {" Don't have an account?"} <a href="/signup">Sign up</a> | |
| {" Don't have an account?"} <Link href="/signup">Sign up</Link> |
app/components/Auth/SignupForm.js
Outdated
| {loading ? 'Creating account...' : 'Sign Up'} | ||
| </SubmitButton> | ||
| <LinkText> | ||
| Already have an account? <a href="/login">Sign in</a> |
There was a problem hiding this comment.
For internal navigation, prefer using the Next.js <Link> component over a standard <a> tag. This leverages client-side routing for a faster, single-page application experience.
You will need to import Link from next/link at the top of the file.
| Already have an account? <a href="/login">Sign in</a> | |
| Already have an account? <Link href="/login">Sign in</Link> |
app/components/SongCard/index.js
Outdated
|
|
||
| &:hover { | ||
| transform: scale(1.1); | ||
| background-color: #0056cc; |
There was a problem hiding this comment.
The hover effect on the play button uses a hardcoded color #0056cc. To ensure theme consistency and maintainability, this color should be defined as a CSS variable in your global styles (e.g., --color-accent-dark) and used here.
| background-color: #0056cc; | |
| background-color: var(--color-accent-dark); |
app/components/SongGrid/index.js
Outdated
| const pick = (...values) => values.find((v) => v !== undefined && v !== null); | ||
|
|
||
| function normalizeSong(song) { | ||
| return { | ||
| id: pick(song.id, song.trackId), | ||
| trackId: pick(song.trackId, song.id), | ||
| title: pick(song.title, song.trackName), | ||
| artist: pick(song.artist, song.artistName), | ||
| album: pick(song.album, song.albumName), | ||
| cover: pick(song.cover, song.artworkUrl, song.artworkUrl100), | ||
| previewUrl: song.previewUrl | ||
| }; | ||
| } |
There was a problem hiding this comment.
There appears to be duplicated and potentially confusing data normalization logic. This file defines pick and normalizeSong, while app/components/SongCard/index.js defines a similar firstNonNull and getSongData. This can make maintenance difficult.
Consider consolidating this logic into a single, reusable utility function (e.g., in a new app/utils/songUtils.js file) that handles all song object normalization for the application. This would improve code clarity and reduce redundancy.
app/components/css/controlscss.js
Outdated
| export const PlaceholderArt = styled.div` | ||
| width: 100%; | ||
| height: 100%; | ||
| background: linear-gradient(135deg, var(--color-accent) 0%, #0056CC 100%); |
There was a problem hiding this comment.
The PlaceholderArt component uses a hardcoded color #0056CC in its gradient. This should be replaced with a CSS variable to ensure it adapts to the application's theme and to improve maintainability.
| background: linear-gradient(135deg, var(--color-accent) 0%, #0056CC 100%); | |
| background: linear-gradient(135deg, var(--color-accent) 0%, var(--color-accent-dark) 100%); |
app/components/css/controlscss.js
Outdated
| font-size: 16px; | ||
|
|
||
| &:hover { | ||
| background-color: #0056CC; |
There was a problem hiding this comment.
The primary control button has a hardcoded hover color (#0056CC). This should be replaced with a CSS variable (e.g., --color-accent-dark) to respect the application's theming and improve maintainability.
| background-color: #0056CC; | |
| background-color: var(--color-accent-dark); |
app/components/css/controlscss.js
Outdated
| export const SearchContainer = styled.div` | ||
| max-width: 1400px; | ||
| margin: 0 auto; | ||
| `; | ||
|
|
||
| export const SearchTitle = styled.h1` | ||
| font-size: 24px; | ||
| font-weight: 600; | ||
| margin-bottom: 24px; | ||
| color: var(--color-text); | ||
| `; | ||
|
|
||
| export const LoadMoreContainer = styled.div` | ||
| display: flex; | ||
| justify-content: center; | ||
| margin: 24px 0 40px; | ||
| `; | ||
|
|
||
| export const LoadMoreButton = styled.button` | ||
| padding: 10px 20px; | ||
| border-radius: 999px; | ||
| border: 1px solid var(--color-border); | ||
| background: var(--color-background-secondary); | ||
| color: var(--color-text); | ||
| font-size: 14px; | ||
| font-family: inherit; | ||
| cursor: pointer; | ||
| transition: all var(--transition-fast); | ||
|
|
||
| &:hover { | ||
| border-color: var(--color-accent); | ||
| background: var(--color-hover); | ||
| } | ||
|
|
||
| &:disabled { | ||
| opacity: 0.6; | ||
| cursor: not-allowed; | ||
| } | ||
| `; No newline at end of file |
There was a problem hiding this comment.
The styles for SearchContainer, SearchTitle, LoadMoreContainer, and LoadMoreButton are related to the search page but are located in controlscss.js, which primarily contains styles for the music player. For better organization and maintainability, these search-related styles should be moved to a more appropriate location, such as a dedicated stylesheet for the search page or a shared component style file.
app/services/musicApi.js
Outdated
| } catch (error) { | ||
| // Fallback: use search endpoint with country parameter | ||
| } |
There was a problem hiding this comment.
The catch block in getTrendingTracks is empty. While the intention is to fall back to another endpoint, swallowing errors silently is a bad practice as it hides potential issues with the primary /music/resources/trending endpoint. At a minimum, the error should be logged to aid in debugging.
| } catch (error) { | |
| // Fallback: use search endpoint with country parameter | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch from trending endpoint, falling back to search:', error); | |
| // Fallback: use search endpoint with country parameter | |
| } |
aman-wednesdaysol
force-pushed
the
feature/song-card
branch
from
41cf3de to
45d14ee
Compare
📝 WalkthroughWalkthroughThis PR introduces two new React components (Layout and Sidebar) with Redux theme integration, updates multiple dependencies to patch critical vulnerabilities, and documents the vulnerability mitigations through an ADR file with Yarn resolutions for secure package pinning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/components/Sidebar/index.js (1)
125-125: Remove the emptySidebar.propTypes = {}.The component accepts no props, so an empty propTypes object adds noise without providing any runtime validation benefit.
✏️ Proposed fix
-Sidebar.propTypes = {}; - export default Sidebar;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Sidebar/index.js` at line 125, Remove the unnecessary empty propTypes declaration on the Sidebar component: delete the Sidebar.propTypes = {} line so there is no empty runtime validation object for the Sidebar component (refer to the Sidebar component and its Sidebar.propTypes symbol in the file).app/components/Layout/index.js (1)
25-38: Hardcodedmargin-leftvalues are tightly coupled toSidebarContainerwidths.The three breakpoint values (
240px,200px,180px) inMainContentmirror the three width values inSidebarContainer. If the sidebar width is ever adjusted, this component must also be updated manually. Consider extracting these as CSS custom properties (e.g.,--sidebar-width) so the two components share a single source of truth.♻️ Suggested refactor (CSS variable approach)
In a global CSS/theme file:
:root { --sidebar-width: 240px; --sidebar-width-md: 200px; --sidebar-width-sm: 180px; }Then in both styled components:
- width: 240px; + width: var(--sidebar-width); - `@media` (max-width: 768px) { - width: 200px; + `@media` (max-width: 768px) { + width: var(--sidebar-width-md);- margin-left: 240px; + margin-left: var(--sidebar-width); - `@media` (max-width: 768px) { - margin-left: 200px; + `@media` (max-width: 768px) { + margin-left: var(--sidebar-width-md);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Layout/index.js` around lines 25 - 38, MainContent currently hardcodes margin-left (240px/200px/180px) which duplicates SidebarContainer widths; define CSS custom properties (e.g., --sidebar-width, --sidebar-width-md, --sidebar-width-sm) in a global CSS or theme root and replace the hardcoded margin-left values in the MainContent styled component with those variables (using var(--sidebar-width) and corresponding media-query variants), and update SidebarContainer to read its width from the same variables so both components share a single source of truth.package.json (1)
124-134: Inconsistent indentation in theresolutionsblock.Lines 125–128 use 4-space indentation while Lines 129–133 use 2-space indentation. Since
package.jsonis parsed as JSON and the inconsistency is purely cosmetic, it won't break anything, but it is visually misleading and should be normalized.✏️ Proposed fix
"resolutions": { "simple-git": ">=3.16.0", "property-expr": ">=2.0.3", "minimist": ">=1.2.6", "form-data": ">=2.5.4", - "next": ">=15.0.8", - "json": ">=10.0.0", - "braces": ">=3.0.3", - "cross-spawn": ">=6.0.6", - "qs": ">=6.14.1" + "json": ">=10.0.0", + "braces": ">=3.0.3", + "cross-spawn": ">=6.0.6", + "qs": ">=6.14.1" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 124 - 134, The `resolutions` block in package.json has mixed indentation; normalize it by making all entries use the same indentation (e.g., change the 4-space indented lines for the first entries to match the 2-space style used for the later entries) so the entire "resolutions" object is consistently indented; update the entries inside the "resolutions" object (the keys like "simple-git", "property-expr", "minimist", "form-data", "next", "json", "braces", "cross-spawn", "qs") to the chosen indentation level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/Layout/index.js`:
- Around line 63-72: The effect in Layout that runs useEffect currently reads
theme and calls initializeTheme but omits them from deps and may call
initializeTheme when it's undefined; fix by separating the DOM write (set
document.documentElement attribute) from the Redux dispatch: inside the
mount-only effect set savedTheme = localStorage.getItem('theme') || 'light' and
always call document.documentElement.setAttribute('data-theme', savedTheme');
then only call initializeTheme(savedTheme) if typeof initializeTheme ===
'function' and savedTheme !== theme; because the DOM write is intentionally
mount-only either add an eslint suppression for react-hooks/exhaustive-deps on
that effect or move the initializeTheme call into a second effect that includes
[theme, initializeTheme] as deps so the exhaustive-deps rule is satisfied; also
remove the redundant typeof window !== 'undefined' guard inside useEffect.
- Around line 11-15: The Layout component currently imports SearchBar,
ThemeToggle, selectTheme and themeActionCreators but those modules don't exist;
either create the missing components/containers (add app/components/SearchBar.js
and app/components/ThemeToggle.js and a Theme container exporting selectTheme
and themeActionCreators) or remove/replace their imports and usages in the
Layout component (search/remove the references to SearchBar and ThemeToggle in
the Layout render and replace selectTheme/themeActionCreators usage with an
existing container like Info or Repos or the correct selector/action names).
Update Layout so all imported symbols (SearchBar, ThemeToggle, selectTheme,
themeActionCreators) resolve to actual exports to fix the build.
- Around line 95-97: The imports for selectTheme and themeActionCreators are
pointing to a non-existent Theme container and will cause a module-not-found
error; either create a Theme container that exports selectTheme and
themeActionCreators (matching the selector and reducer names) or update the
import statements in the Layout component to the correct existing module that
provides those symbols; ensure mapStateToProps uses the imported selectTheme and
any dispatched actions reference themeActionCreators so the Layout component
compiles and runs.
In `@docs/adr/2026-02-17-fix-critical-vulnerabilities.md`:
- Around line 67-79: Close the open JSON fenced code block (add the missing
closing ``` after the resolutions snippet) and expand the "Summary of Actions"
to include all nine packages listed in the resolutions object by adding
vulnerability entries for next, json, braces, cross-spawn, and qs similar to the
existing entries for simple-git/property-expr/minimist/form-data; for each new
entry include the package name, affected versions, CVE or advisory references,
the chosen resolution version, and a brief rationale so the ADR's resolutions
block and narrative are complete and consistent with the "resolutions" JSON.
In `@package.json`:
- Line 57: Update package.json to align eslint-config-next with the installed
Next.js version: change the dependency "eslint-config-next" to a 14.x release
that matches "next": "14.2.35" (e.g., "^14.2.35"), and remove the "next" entry
under "resolutions" entirely (the "next" dependency already contains the CVE fix
and the resolutions entry is unnecessary and mis-indented). Locate the keys
"eslint-config-next" and "resolutions.next" in package.json to make these edits.
---
Nitpick comments:
In `@app/components/Layout/index.js`:
- Around line 25-38: MainContent currently hardcodes margin-left
(240px/200px/180px) which duplicates SidebarContainer widths; define CSS custom
properties (e.g., --sidebar-width, --sidebar-width-md, --sidebar-width-sm) in a
global CSS or theme root and replace the hardcoded margin-left values in the
MainContent styled component with those variables (using var(--sidebar-width)
and corresponding media-query variants), and update SidebarContainer to read its
width from the same variables so both components share a single source of truth.
In `@app/components/Sidebar/index.js`:
- Line 125: Remove the unnecessary empty propTypes declaration on the Sidebar
component: delete the Sidebar.propTypes = {} line so there is no empty runtime
validation object for the Sidebar component (refer to the Sidebar component and
its Sidebar.propTypes symbol in the file).
In `@package.json`:
- Around line 124-134: The `resolutions` block in package.json has mixed
indentation; normalize it by making all entries use the same indentation (e.g.,
change the 4-space indented lines for the first entries to match the 2-space
style used for the later entries) so the entire "resolutions" object is
consistently indented; update the entries inside the "resolutions" object (the
keys like "simple-git", "property-expr", "minimist", "form-data", "next",
"json", "braces", "cross-spawn", "qs") to the chosen indentation level.
| import Sidebar from '@app/components/Sidebar/index'; | ||
| import SearchBar from '@app/components/SearchBar'; | ||
| import ThemeToggle from '@app/components/ThemeToggle'; | ||
| import { selectTheme } from '@app/containers/Theme/selectors'; | ||
| import { themeActionCreators } from '@app/containers/Theme/reducer'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify each imported path resolves in the repository
echo "=== Checking SearchBar component ==="
fd "index\.(js|jsx|ts|tsx)" app/components/SearchBar 2>/dev/null
echo "=== Checking ThemeToggle component ==="
fd "index\.(js|jsx|ts|tsx)" app/components/ThemeToggle 2>/dev/null
echo "=== Checking Theme/selectors ==="
fd "selectors\.(js|ts)" app/containers/Theme 2>/dev/null
echo "=== Checking Theme/reducer ==="
fd "reducer\.(js|ts)" app/containers/Theme 2>/dev/null
echo "=== Checking for themeActionCreators export ==="
rg -n "themeActionCreators" app/containers/Theme/ -C3 2>/dev/null
echo "=== Checking for selectTheme export ==="
rg -n "selectTheme" app/containers/Theme/ -C3 2>/dev/nullRepository: wednesday-solutions/nextjs-template
Length of output: 306
🏁 Script executed:
#!/bin/bash
# First, check if the app directory exists and its structure
echo "=== Repository structure ==="
ls -la | head -20
echo -e "\n=== Contents of app directory ==="
if [ -d "app" ]; then
ls -la app/
else
echo "app directory not found"
fi
echo -e "\n=== Search for SearchBar anywhere in repo ==="
find . -name "*SearchBar*" -o -name "*searchbar*" 2>/dev/null | head -20
echo -e "\n=== Search for ThemeToggle anywhere in repo ==="
find . -name "*ThemeToggle*" -o -name "*themetoggle*" 2>/dev/null | head -20
echo -e "\n=== Search for Theme container ==="
find . -type d -name "Theme" 2>/dev/null | head -20
echo -e "\n=== Layout file location ==="
find . -name "Layout" -type d 2>/dev/null | head -10Repository: wednesday-solutions/nextjs-template
Length of output: 2390
🏁 Script executed:
#!/bin/bash
# Read the Layout component file
echo "=== Contents of Layout/index.js ==="
cat -n app/components/Layout/index.js
echo -e "\n=== App components ==="
ls -la app/components/
echo -e "\n=== App containers ==="
ls -la app/containers/Repository: wednesday-solutions/nextjs-template
Length of output: 4508
Missing component and container imports will cause build failure.
The imports on lines 12-15 reference modules that do not exist in the repository:
SearchBarcomponent (line 12) — does not exist inapp/components/ThemeTogglecomponent (line 13) — does not exist inapp/components/Themecontainer (lines 14-15) — does not exist inapp/containers/(onlyInfoandReposcontainers exist)
These missing modules are actively used in the Layout component (lines 79, 80, 96, 100), so the code will fail to build until they are created or the imports are corrected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/Layout/index.js` around lines 11 - 15, The Layout component
currently imports SearchBar, ThemeToggle, selectTheme and themeActionCreators
but those modules don't exist; either create the missing components/containers
(add app/components/SearchBar.js and app/components/ThemeToggle.js and a Theme
container exporting selectTheme and themeActionCreators) or remove/replace their
imports and usages in the Layout component (search/remove the references to
SearchBar and ThemeToggle in the Layout render and replace
selectTheme/themeActionCreators usage with an existing container like Info or
Repos or the correct selector/action names). Update Layout so all imported
symbols (SearchBar, ThemeToggle, selectTheme, themeActionCreators) resolve to
actual exports to fix the build.
| useEffect(() => { | ||
| // Initialize theme from localStorage on mount | ||
| if (typeof window !== 'undefined') { | ||
| const savedTheme = localStorage.getItem('theme') || 'light'; | ||
| document.documentElement.setAttribute('data-theme', savedTheme); | ||
| if (savedTheme !== theme) { | ||
| initializeTheme(savedTheme); | ||
| } | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
useEffect has two related issues: exhaustive-deps violation and missing null guard on initializeTheme.
1. Exhaustive-deps: theme (Line 68) and initializeTheme (Line 69) are captured from props inside the effect but omitted from the dependency array. React's react-hooks/exhaustive-deps rule will error here. Since mount-only initialization is intentional, add a suppression comment or restructure to separate the DOM write from the Redux dispatch:
2. Null guard: initializeTheme is PropTypes.func (not required). When Layout is used via the named export (without Redux's mapDispatchToProps), initializeTheme is undefined. If savedTheme !== theme at mount, initializeTheme(savedTheme) throws TypeError: initializeTheme is not a function.
🔧 Proposed fix
useEffect(() => {
- // Initialize theme from localStorage on mount
- if (typeof window !== 'undefined') {
- const savedTheme = localStorage.getItem('theme') || 'light';
- document.documentElement.setAttribute('data-theme', savedTheme);
- if (savedTheme !== theme) {
- initializeTheme(savedTheme);
- }
- }
-}, []);
+ // Initialize theme from localStorage on mount only
+ const savedTheme = localStorage.getItem('theme') || 'light';
+ document.documentElement.setAttribute('data-theme', savedTheme);
+ if (savedTheme !== theme && initializeTheme) {
+ initializeTheme(savedTheme);
+ }
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+}, []);The typeof window !== 'undefined' guard is also redundant inside useEffect (which never runs server-side), so it can be removed as shown above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/Layout/index.js` around lines 63 - 72, The effect in Layout
that runs useEffect currently reads theme and calls initializeTheme but omits
them from deps and may call initializeTheme when it's undefined; fix by
separating the DOM write (set document.documentElement attribute) from the Redux
dispatch: inside the mount-only effect set savedTheme =
localStorage.getItem('theme') || 'light' and always call
document.documentElement.setAttribute('data-theme', savedTheme'); then only call
initializeTheme(savedTheme) if typeof initializeTheme === 'function' and
savedTheme !== theme; because the DOM write is intentionally mount-only either
add an eslint suppression for react-hooks/exhaustive-deps on that effect or move
the initializeTheme call into a second effect that includes [theme,
initializeTheme] as deps so the exhaustive-deps rule is satisfied; also remove
the redundant typeof window !== 'undefined' guard inside useEffect.
| const mapStateToProps = createStructuredSelector({ | ||
| theme: selectTheme() | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for selectTheme definition in the theme-related directories
fd selectTheme --type f
echo "=== Looking for selectTheme definition ==="
rg -n "selectTheme\s*=" --type js -C 3
echo "=== Looking for selectTheme export ==="
rg -n "export.*selectTheme" --type js -C 2
echo "=== Looking in Theme container ==="
find . -path "*/Theme/*" -name "*.js" | head -20Repository: wednesday-solutions/nextjs-template
Length of output: 195
🏁 Script executed:
#!/bin/bash
echo "=== Search for selectTheme anywhere in codebase ==="
rg "selectTheme" -n -C 3
echo "=== Check if Layout file exists ==="
find . -name "index.js" -path "*/Layout/*" -type f
echo "=== List app structure ==="
find ./app -type d -name "components" -o -name "containers" | head -20
echo "=== Check for selectors ==="
find . -name "*selector*" -o -name "*selectors*" 2>/dev/null | grep -v node_modulesRepository: wednesday-solutions/nextjs-template
Length of output: 1386
🏁 Script executed:
#!/bin/bash
echo "=== Check if Theme selectors exists ==="
find . -path "*/Theme/selectors*" -type f
echo "=== Read Theme selectors file ==="
cat app/containers/Theme/selectors.jsRepository: wednesday-solutions/nextjs-template
Length of output: 235
🏁 Script executed:
#!/bin/bash
echo "=== List Theme container structure ==="
find ./app/containers/Theme -type f 2>/dev/null
echo "=== Check if Theme directory exists ==="
ls -la ./app/containers/Theme/ 2>&1
echo "=== Broader search for Theme directory ==="
find ./app -type d -name "Theme" 2>/dev/null
echo "=== Search all containers ==="
ls -la ./app/containers/Repository: wednesday-solutions/nextjs-template
Length of output: 521
🏁 Script executed:
#!/bin/bash
echo "=== Read Layout file around import ==="
sed -n '10,20p' app/components/Layout/index.js
echo "=== Search for any selectTheme or Theme selectors ==="
rg -i "selecttheme|theme.*selector" -l
echo "=== Search for createSelector usage ==="
rg "createSelector" -n -C 2Repository: wednesday-solutions/nextjs-template
Length of output: 2568
Remove broken imports and create missing Theme container or import from correct location.
The file imports selectTheme from @app/containers/Theme/selectors and themeActionCreators from @app/containers/Theme/reducer, but the app/containers/Theme/ directory and its files do not exist. This will cause an immediate module not found error at runtime. Either create the Theme container with these exports, or update the imports to reference the correct location where these are actually defined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/Layout/index.js` around lines 95 - 97, The imports for
selectTheme and themeActionCreators are pointing to a non-existent Theme
container and will cause a module-not-found error; either create a Theme
container that exports selectTheme and themeActionCreators (matching the
selector and reducer names) or update the import statements in the Layout
component to the correct existing module that provides those symbols; ensure
mapStateToProps uses the imported selectTheme and any dispatched actions
reference themeActionCreators so the Layout component compiles and runs.
| ## Summary of Actions | ||
|
|
||
| - Added **Yarn resolutions** for all critical packages: | ||
|
|
||
| ```json | ||
| { | ||
| "resolutions": { | ||
| "simple-git": ">=3.16.0", | ||
| "property-expr": ">=2.0.3", | ||
| "minimist": ">=1.2.6", | ||
| "form-data": ">=2.5.4" | ||
| } | ||
| } |
There was a problem hiding this comment.
ADR is incomplete and the code block is missing its closing fence.
Two issues:
- The
resolutionsblock inpackage.jsoncontains nine entries (simple-git,property-expr,minimist,form-data,next,json,braces,cross-spawn,qs), but the Summary here only documents four. The rationale and CVE references fornext,json,braces,cross-spawn, andqsare absent. - The JSON fenced code block opened at Line 71 with
```jsonhas no corresponding closing```, which will break markdown rendering.
📝 Proposed fix
{
"resolutions": {
"simple-git": ">=3.16.0",
"property-expr": ">=2.0.3",
"minimist": ">=1.2.6",
- "form-data": ">=2.5.4"
+ "form-data": ">=2.5.4",
+ "next": ">=15.0.8",
+ "json": ">=10.0.0",
+ "braces": ">=3.0.3",
+ "cross-spawn": ">=6.0.6",
+ "qs": ">=6.14.1"
}
}
+```Add a corresponding vulnerability entry for each missing package following the same template used for the existing five entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/2026-02-17-fix-critical-vulnerabilities.md` around lines 67 - 79,
Close the open JSON fenced code block (add the missing closing ``` after the
resolutions snippet) and expand the "Summary of Actions" to include all nine
packages listed in the resolutions object by adding vulnerability entries for
next, json, braces, cross-spawn, and qs similar to the existing entries for
simple-git/property-expr/minimist/form-data; for each new entry include the
package name, affected versions, CVE or advisory references, the chosen
resolution version, and a brief rationale so the ADR's resolutions block and
narrative are complete and consistent with the "resolutions" JSON.
| "invariant": "^2.2.4", | ||
| "lodash": "^4.17.21", | ||
| "next": "14.2.8", | ||
| "next": "14.2.35", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the full context around the disputed lines in package.json
cd /tmp && if [ -f "package.json" ]; then grep -n -A2 -B2 "next\|eslint-config-next" package.json | head -80; else echo "No package.json in current dir"; fiRepository: wednesday-solutions/nextjs-template
Length of output: 109
🏁 Script executed:
# List files in repository to understand structure
git ls-files | grep -E "(package\.json|yarn\.lock)" | head -20Repository: wednesday-solutions/nextjs-template
Length of output: 101
🏁 Script executed:
# Get full content of package.json to see the exact state
wc -l package.jsonRepository: wednesday-solutions/nextjs-template
Length of output: 95
🏁 Script executed:
# Check the relevant sections
head -140 package.json | tail -90Repository: wednesday-solutions/nextjs-template
Length of output: 3045
🌐 Web query:
CVE-2025-29927 Next.js SSRF vulnerability fixed version
💡 Result:
CVE-2025-29927 (Next.js middleware auth bypass via x-middleware-subrequest) was initially fixed in Next.js 12.3.5, 13.5.9, 14.2.25, and 15.2.3. [1]
Cloudflare later noted additional follow-up patches and recommends upgrading to Next.js 12.3.6, 13.5.10, 14.2.26, or 15.2.4 for a fully patched set. [2]
Sources: NVD CVE record [1], Cloudflare changelog update [2].
🌐 Web query:
eslint-config-next 16.1.6 Next.js version compatibility
💡 Result:
eslint-config-next@16.1.6 is the ESLint config package that ships with Next.js 16.1.6—it’s intended to be used with the Next.js 16.x line, and in practice you should keep it version-matched with your next dependency (i.e., next@16.1.6 + eslint-config-next@16.1.6). [1], [2]
Also note that eslint-config-next@16.1.6 expects ESLint >= 9.0.0 (per its peer dependency), so pairing it with older ESLint can cause install/lint issues. [1]
Sources:
[1] package-lock excerpt showing eslint-config-next@16.1.6 peer dependency (eslint >=9) and dependency on @next/eslint-plugin-next@16.1.6
[2] Next.js 16 LTS “latest” version listed as 16.1.6
🌐 Web query:
Yarn resolutions override direct dependencies behavior
💡 Result:
- Yarn “resolutions” are primarily for overriding transitive (nested) dependencies, not your own direct dependencies. In Yarn Classic (v1), the intended precedence is that
dependencies/devDependencies/optionalDependenciestake precedence overresolutions, soresolutionseffectively only applies to nested deps; if your direct dependency conflicts with a resolution, Yarn will warn. [1] - To change a direct dependency version, change the direct dependency entry (e.g.,
dependencies.foo) rather than relying onresolutions. [1] - In Yarn Berry (v2+),
resolutionsis still the mechanism to force specific versions, declared at the root workspace, and it instructs Yarn to use a specific version “instead of anything the resolver would normally pick” (useful for enforcing a single version or backporting fixes). You can also manage them viayarn set resolution. [2][3]
Sources:
[1] Stack Overflow discussion quoting the resolutions RFC precedence rules and rationale (direct deps win; resolutions for transitives) https://stackoverflow.com/questions/61985738/why-would-i-choose-yarns-resolutions-over-an-explicit-dependency
[2] Yarn (Berry) manifest docs: resolutions field behavior + root-only note https://yarnpkg.com/configuration/manifest
[3] Yarn CLI: yarn set resolution https://yarnpkg.com/cli/set/resolution
🏁 Script executed:
# Check yarn version and packageManager field
grep -E "packageManager|yarn" package.jsonRepository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Check the full resolutions block to see indentation clearly
sed -n '124,134p' package.json | cat -ARepository: wednesday-solutions/nextjs-template
Length of output: 343
Fix eslint-config-next version mismatch and remove unnecessary resolutions.next entry.
The installed next@14.2.35 conflicts with eslint-config-next@16.1.6. The eslint config package is version-matched to the Next.js major version it ships with; 16.1.6 is for Next.js 16.x, not 14.x. Using eslint-config-next v16 with Next.js 14 will cause linting failures and eslint config errors.
Additionally, resolutions.next is unnecessary—dependencies.next at 14.2.35 already contains the fix for CVE-2025-29927 (patched in 14.2.25+). The resolutions entry also has an indentation error (not aligned with other entries).
Recommended action: Downgrade eslint-config-next to ^14.2.35, and remove the next entry from resolutions.
🔧 Proposed fix
- "eslint-config-next": "^16.1.6",
+ "eslint-config-next": "^14.2.35", "resolutions": {
"simple-git": ">=3.16.0",
"property-expr": ">=2.0.3",
"minimist": ">=1.2.6",
"form-data": ">=2.5.4",
- "next": ">=15.0.8",
"json": ">=10.0.0",
"braces": ">=3.0.3",
"cross-spawn": ">=6.0.6",
"qs": ">=6.14.1"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 57, Update package.json to align eslint-config-next
with the installed Next.js version: change the dependency "eslint-config-next"
to a 14.x release that matches "next": "14.2.35" (e.g., "^14.2.35"), and remove
the "next" entry under "resolutions" entirely (the "next" dependency already
contains the CVE fix and the resolutions entry is unnecessary and mis-indented).
Locate the keys "eslint-config-next" and "resolutions.next" in package.json to
make these edits.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores