Feat/details(tract details)#30
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive music streaming application featuring user authentication (login/signup), music search and playback, library management for liked songs, and track detail viewing. It includes a full Redux-based state management system with sagas, custom React hooks for playback control, a styled component library with theme support, API service layer, and corresponding pages with authentication protection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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 introduces significant new features to the application, transforming it into a music streaming-like platform. It includes core functionalities such as user authentication (login and signup), a music search page, a user's liked songs library, and a detailed track view. A global audio player is implemented for seamless playback, along with UI components for interacting with songs (play/pause, like/unlike). The application now supports theme toggling and uses Redux for state management across these new features. 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
Activity
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 comprehensive music player feature, including search functionality, a favorites library, and detailed track views, leveraging modern React patterns and unit tests. However, a security audit revealed critical vulnerabilities, including hardcoded API URLs that could leak into production, a potential XSS vulnerability through unsanitized external URLs, and insecure storage of authentication tokens in localStorage. Additionally, the authentication header initialization logic has a bug preventing token application, several files exceed the 100-line limit and require refactoring, and playback state synchronization in the audio hook needs refinement.
| const client = apiClients[type]; | ||
| if (client) { | ||
| client.setHeader('Authorization', `Bearer ${token}`); | ||
| } |
There was a problem hiding this comment.
The setAuthHeader function directly accesses apiClients[type]. If this function is called before the client has been initialized (which occurs in _app.js on mount), the header will not be set because the client is still null. Using generateApiClient(type) ensures the client instance is created before the header is applied.
| const client = apiClients[type]; | |
| if (client) { | |
| client.setHeader('Authorization', `Bearer ${token}`); | |
| } | |
| const client = generateApiClient(type); | |
| if (client) { | |
| client.setHeader('Authorization', `Bearer ${token}`); | |
| } |
| case 'github': | ||
| apiClients[type] = createApiClientWithTransForm(process.env.NEXT_PUBLIC_GITHUB_URL); | ||
| case 'auth': | ||
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000'); |
There was a problem hiding this comment.
The API base URL for the 'auth' client is hardcoded to http://localhost:9000. This poses a security risk by potentially leaking internal network details and making secure configuration across environments difficult. If deployed, it could lead to exploitation. Please move this to an environment variable (e.g., process.env.NEXT_PUBLIC_AUTH_URL) for secure and correct functionality across all environments.
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000'); | ||
| return apiClients[type]; | ||
| case 'music': | ||
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000', { skipRequestTransform: true }); |
There was a problem hiding this comment.
The API base URL for the 'music' service is hardcoded to http://localhost:9000. Similar to the 'auth' service, this should be moved to an environment variable to ensure secure and flexible configuration across different deployment environments.
| )} | ||
| </TagRow> | ||
| {track.trackUrl && ( | ||
| <StoreLink href={track.trackUrl} target="_blank" rel="noopener noreferrer" data-testid="store-link"> |
There was a problem hiding this comment.
The track.trackUrl property, which originates from an external API response, is directly assigned to the href attribute of the StoreLink component. If the API returns a malicious URL using the javascript: protocol, clicking the link will execute arbitrary JavaScript in the context of the user's session. It is recommended to validate that the URL uses an allowed protocol (e.g., http: or https:).
| if (!isBrowser()) { | ||
| return; | ||
| } | ||
| localStorage.setItem(TOKEN_KEY, token); |
There was a problem hiding this comment.
The application stores the authentication accessToken in localStorage. Data in localStorage is accessible to any JavaScript running on the same origin, making it vulnerable to theft via Cross-Site Scripting (XSS). For better security, consider storing sensitive session tokens in secure, HttpOnly cookies, which are inaccessible to client-side scripts.
| audioRef.current.play().catch(() => {}); | ||
| setIsPlaying(true); | ||
| if (onPlayStateChange) { | ||
| onPlayStateChange(true); | ||
| } |
There was a problem hiding this comment.
The setIsPlaying(true) state update and the onPlayStateChange callback are executed immediately after calling play(), without waiting for the promise to resolve. If the browser blocks playback (e.g., due to autoplay policies), the UI will incorrectly show that the song is playing. These updates should be moved inside the .then() block of the play() call, and a corresponding reset should be added to the .catch() block. This pattern should also be applied to the togglePlay function.
audioRef.current.play()
.then(() => {
setIsPlaying(true);
if (onPlayStateChange) {
onPlayStateChange(true);
}
})
.catch(() => {
setIsPlaying(false);
if (onPlayStateChange) {
onPlayStateChange(false);
}
});| import useToggleLike from './useToggleLike'; | ||
| import usePlayToggle from './usePlayToggle'; | ||
|
|
||
| export function Music(props) { |
There was a problem hiding this comment.
This file is 139 lines long, which violates the repository style guide rule (Max file length: 100 lines). Consider breaking this container into smaller sub-components or moving the Redux connect logic and prop types to a separate file to improve readability and maintainability.
References
- Max file length: 100 lines — break into smaller files if exceeded (link)
| 100% { transform: translateY(-280px) rotate(40deg); opacity: 0; } | ||
| `; | ||
|
|
||
| export const VinylRecord = styled.div` |
There was a problem hiding this comment.
This styled-components file is 131 lines long, exceeding the 100-line limit specified in the style guide. It is recommended to split these styles into smaller, logically grouped files (e.g., equalizerStyles.js, vinylStyles.js) to adhere to the project's coding standards.
References
- Max file length: 100 lines — break into smaller files if exceeded (link)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
environments/.env.development (1)
2-3: Same issue asenvironments/.env.qa.The identical hard-coded
NEXT_PUBLIC_POSTHOG_KEYvalue flags the same Gitleaks warning and shares the same PostHog project across environments. See the comment onenvironments/.env.qafor the full analysis and remediation steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@environments/.env.development` around lines 2 - 3, The file contains a hard-coded public PostHog API key (NEXT_PUBLIC_POSTHOG_KEY) and host (NEXT_PUBLIC_POSTHOG_HOST) which duplicates the QA env and triggers Gitleaks; replace the literal key with an environment-driven value and avoid committing secrets by: remove the hard-coded NEXT_PUBLIC_POSTHOG_KEY from environments/.env.development, reference a secure deploy-time secret or a different per-environment key (e.g., NEXT_PUBLIC_POSTHOG_KEY set via CI/hosting secrets), keep NEXT_PUBLIC_POSTHOG_HOST configurable if needed, and update any docs or CI to populate NEXT_PUBLIC_POSTHOG_KEY for development so each environment uses its own key rather than the duplicated value.environments/.env.production (1)
2-3: Same issue asenvironments/.env.qa; production environment is highest-impact.The hard-coded production key is now permanently embedded in git history. Refer to the
environments/.env.qacomment for remediation. The production key in particular should be rotated after removal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@environments/.env.production` around lines 2 - 3, Replace the hard-coded PostHog key and host by removing the literal NEXT_PUBLIC_POSTHOG_KEY value and switching to a runtime secret (e.g., read NEXT_PUBLIC_POSTHOG_KEY from your secret manager or CI/CD environment) and ensure NEXT_PUBLIC_POSTHOG_HOST is also sourced from config rather than committed plaintext; remove the sensitive value from the repository, add your environment files or patterns (e.g., .env.*) to .gitignore, purge the leaked key from git history using a history-cleaning tool (git filter-repo or BFG), and rotate the exposed PostHog API key immediately after removal.
🟡 Minor comments (24)
CLAUDE.md-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorSelf-contradicting convention: constant defined in this same file.
Line 8 states "Keep constants in a separate constants file," yet
DISTILLED_AESTHETICS_PROMPTis defined as a constant directly in this file (lines 13–33). Either move the prompt to a dedicated constants file and reference it here, or relax/remove this specific rule if CLAUDE.md is intentionally the home for AI-context constants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 8, The CLAUDE.md rule "Keep constants in a separate constants file" conflicts with the in-file constant DISTILLED_AESTHETICS_PROMPT; fix by either moving DISTILLED_AESTHETICS_PROMPT into the shared constants module and replacing the in-file definition with an import/reference, or remove/adjust the rule text so it no longer mandates externalizing AI-context constants; update any references to DISTILLED_AESTHETICS_PROMPT (the symbol defined in this file) to use the new exported constant name/location if you choose the move approach.CLAUDE.md-13-33 (1)
13-33:⚠️ Potential issue | 🟡 MinorPython triple-quote syntax in a Markdown file is unformatted and structurally ambiguous.
DISTILLED_AESTHETICS_PROMPT = """..."""is raw Python-style assignment syntax with no Markdown structure. In a.mdfile it renders as plain paragraph text with literal"""delimiters, which looks broken in any Markdown viewer. Since CLAUDE.md is read by both humans and the Claude AI toolchain, the intent should be explicit. Consider one of:
- If it's documentation only — add a heading and use a fenced code block:
📝 Proposed fix: fenced code block with section heading
+## Design Aesthetics Prompt + +```python DISTILLED_AESTHETICS_PROMPT = """ <frontend_aesthetics> ... </frontend_aesthetics> """ +```
- If it's a live instruction to the Claude CLI — place it as plain prose under a heading (no variable assignment needed; Claude reads the whole file as context):
📝 Proposed fix: plain prose section
-DISTILLED_AESTHETICS_PROMPT = """ -<frontend_aesthetics> +## Frontend Aesthetics Guidelines + +<frontend_aesthetics> ... -</frontend_aesthetics> -""" +</frontend_aesthetics>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 13 - 33, The block using DISTILLED_AESTHETICS_PROMPT = """...""" is Python syntax embedded in a Markdown doc and should be converted to a proper Markdown structure: either (A) treat it as documentation—replace the assignment with a heading and a fenced code block (use ```xml or ```python) containing the <frontend_aesthetics> ... </frontend_aesthetics> content so the triple-quotes disappear and it renders correctly, or (B) treat it as live instructions for Claude—remove the variable assignment and triple-quotes and place the inner content as plain prose under a clear heading so the tool reads it as context; locate the block by the DISTILLED_AESTHETICS_PROMPT identifier and the <frontend_aesthetics> tags when making the change.app/utils/tests/withAuth.test.js-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorFragile spy cleanup — move
mockRestore()toafterEach.
mockRestore()is called inline after assertions. If anyexpect(...)call throws before it, the spy is never restored, leaving stale mock state for the remaining tests.🔧 Proposed fix
beforeEach(() => { mockReplace.mockClear() + jest.restoreAllMocks() }) it('should render the wrapped component when token exists', () => { jest.spyOn(authStorage, 'getStoredToken').mockReturnValue('valid-token') const { getByTestId } = render(<WrappedComponent />) expect(getByTestId('protected')).toBeTruthy() - authStorage.getStoredToken.mockRestore() }) it('should redirect to /login when no token exists', () => { jest.spyOn(authStorage, 'getStoredToken').mockReturnValue(null) const { queryByTestId } = render(<WrappedComponent />) expect(queryByTestId('protected')).toBeNull() expect(mockReplace).toHaveBeenCalledWith('/login') - authStorage.getStoredToken.mockRestore() })Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/tests/withAuth.test.js` at line 26, The test calls authStorage.getStoredToken.mockRestore() inline after assertions which can leave the spy restored only when assertions pass; move the mockRestore() calls into a common afterEach cleanup so mocks are always restored even if a test fails — update the test suite in withAuth.test.js to call authStorage.getStoredToken.mockRestore() (and any other inline mockRestore at the other occurrence) from an afterEach block that runs after each test.app/containers/Music/tests/usePlaybackNav.test.js-39-61 (1)
39-61:⚠️ Potential issue | 🟡 MinorMissing test:
currentSongnot found insongs(idx === −1).The four existing boundary tests all use a
currentSongthat is in thesongsarray. There is no case wherefindIdx()returns-1, which is the exact scenario that triggers the dispatch-of-songs[0]bug described above. Adding this test would both document the expected no-op behaviour and catch the regression.it('should not dispatch when currentSong is not in songs', () => { const { result } = renderHook(() => usePlaybackNav({ songs, currentSong: { trackId: 99, trackName: 'Unknown' }, dispatchSetSong: mockSetSong }) ) act(() => result.current.handleNext()) act(() => result.current.handlePrev()) expect(mockSetSong).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Music/tests/usePlaybackNav.test.js` around lines 39 - 61, Add a test covering the case where currentSong is not present in songs (findIdx === -1) to ensure usePlaybackNav does not call dispatchSetSong; specifically, renderHook calling usePlaybackNav with a currentSong object not in the songs array and then call result.current.handleNext() and result.current.handlePrev(), asserting mockSetSong was not called. This targets the functions usePlaybackNav, findIdx, handleNext, handlePrev and the callback dispatchSetSong so the regression that dispatches songs[0] when idx === -1 is caught.app/global-styles.js-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorCSS custom properties lack fallback values — FOUC risk on SSR/hydration.
var(--musica-text)andvar(--musica-bg)have no fallback. IfThemeContexthasn't injected the CSS variables yet (e.g., initial SSR render before client hydration), text renders with an undefined color and the background may flash. Providing a safe default (matching the default theme) prevents any visible flicker.🔧 Proposed fix (using dark-theme defaults as an example)
- color: var(--musica-text); + color: var(--musica-text, `#e8e8e8`); ... - color: var(--musica-text); + color: var(--musica-text, `#e8e8e8`); ... - background-color: var(--musica-bg); + background-color: var(--musica-bg, `#0e0e0e`);Replace the fallback values with whatever the actual default palette tokens evaluate to for the initial theme.
Also applies to: 27-27, 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/global-styles.js` at line 16, The CSS custom properties used in global-styles.js (color: var(--musica-text) and background: var(--musica-bg)) need safe fallbacks to avoid FOUC on SSR/hydration; update every occurrence of var(--musica-text) and var(--musica-bg) to include explicit fallback values that match the default theme palette (the initial theme tokens you use on first render) so the text and background have deterministic colors before ThemeContext injects variables.app/components/ArtworkPlayButton/tests/index.test.js-37-46 (1)
37-46:⚠️ Potential issue | 🟡 Minor"Stop propagation" is never actually asserted.
The test sets up
parentClickand fires a click, but only checks thatdefaultProps.onClickwas called. TheparentClickmock is never asserted, so the test does not verify thate.stopPropagation()worked. The test name is misleading without the missing assertion.🐛 Proposed fix
fireEvent.click(wrapper) expect(defaultProps.onClick).toHaveBeenCalledTimes(1) + expect(parentClick).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ArtworkPlayButton/tests/index.test.js` around lines 37 - 46, The test named "should call onClick and stop propagation" currently never asserts propagation was stopped: after setting up parentClick on wrapper.parentElement and firing fireEvent.click(wrapper), add an assertion that parentClick was not called (e.g. expect(parentClick).not.toHaveBeenCalled()) to verify e.stopPropagation() worked; keep the existing assertion that defaultProps.onClick was called so both behaviors (ArtworkPlayButton onClick handler and propagation stopping) are validated.app/components/BackButton/index.js-6-10 (1)
6-10:⚠️ Potential issue | 🟡 MinorAdd
aria-hidden="true"to the decorative icon.The button has an explicit
aria-label, soArrowLeftOutlinedis purely decorative. Withoutaria-hidden, some screen reader / SVG combinations may announce the icon's internal title or description in addition to the button label. Marking it hidden is defensive and idiomatic for icon-only buttons.♿ Proposed fix
<BackBtn onClick={onClick} data-testid="back-button" aria-label="Go back"> - <ArrowLeftOutlined /> + <ArrowLeftOutlined aria-hidden="true" /> </BackBtn>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/BackButton/index.js` around lines 6 - 10, The ArrowLeftOutlined icon inside the BackButton is decorative and should be hidden from assistive tech; update the BackButton render to add aria-hidden="true" (or role="presentation") to the ArrowLeftOutlined element so screen readers only announce the BackBtn's aria-label; locate the BackButton component and the ArrowLeftOutlined usage and add the aria-hidden attribute to the icon element.app/components/SearchBar/index.js-18-22 (1)
18-22:⚠️ Potential issue | 🟡 Minor
valueshould be required or have a default to avoid the uncontrolled→controlled warning.
valueis not marked.isRequiredand has nodefaultProps. Whenvalue={undefined}, React treats the input as uncontrolled; if the parent later provides a string, React will warn about the mode switch. SinceonChangeis required, the component is intended to be fully controlled.🛡️ Proposed fix
SearchBar.propTypes = { - value: PropTypes.string, + value: PropTypes.string.isRequired, onChange: PropTypes.func.isRequired, loading: PropTypes.bool };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SearchBar/index.js` around lines 18 - 22, The prop "value" on SearchBar is currently optional which can cause an uncontrolled→controlled React warning; update SearchBar to either mark value as required in SearchBar.propTypes (make value: PropTypes.string.isRequired) or add a default prop (e.g., SearchBar.defaultProps = { value: '' }) so the input is always controlled; change the propTypes/defaultProps near the SearchBar.propTypes declaration to implement the chosen fix.app/containers/Music/usePlayToggle.js-10-21 (1)
10-21:⚠️ Potential issue | 🟡 MinorGuard against
null/undefinedsonginhandlePlayToggle.
currentSong?.trackIduses optional chaining whilesong.trackIddoes not. IfhandlePlayToggleis ever called with a falsysong, the access on line 12 will throw aTypeError.🛡️ Proposed fix
const handlePlayToggle = useCallback( (song) => { + if (!song) return; if (currentSong?.trackId === song.trackId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Music/usePlayToggle.js` around lines 10 - 21, The handler handlePlayToggle can throw when called with a falsy song because it accesses song.trackId without a guard; update handlePlayToggle to first check that song is truthy (e.g., if (!song) return) before comparing song.trackId to currentSong?.trackId, and keep using togglePlayRef.current() and dispatchSetSong(song) as before so existing behavior is preserved.app/utils/withAuth.js-19-21 (1)
19-21:⚠️ Potential issue | 🟡 MinorReturning
nullduring the auth check causes a blank flash on every protected route.Since
isReadystarts asfalse, every page wrapped withwithAuthrenders a blank screen until theuseEffectfires and the token check completes. A loading skeleton or spinner is a straightforward improvement.♻️ Proposed fix
if (!isReady) { - return null; + return <YourLoadingSpinner />; // or a page-level skeleton }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/withAuth.js` around lines 19 - 21, withAuth currently returns null while isReady is false which causes a blank flash on protected routes; update the withAuth HOC (the withAuth function and its internal isReady check) to render a loading fallback instead of null — either use an existing LoadingSpinner/LoadingSkeleton component or accept a loadingFallback prop and render that while isReady is false, then proceed with the normal auth rendering once token check completes.app/utils/authStorage.js-5-24 (1)
5-24:⚠️ Potential issue | 🟡 Minor
localStorageaccess is not wrapped intry/catch.
localStorage.setItemthrows aQuotaExceededErrorwhen storage is full. In some restricted browser contextsgetItem/setItem/removeItemcan also throw aSecurityError. An unhandled exception here would propagate up to the auth flow.🛡️ Proposed fix
export const getStoredToken = () => { if (!isBrowser()) return null; - return localStorage.getItem(TOKEN_KEY); + try { + return localStorage.getItem(TOKEN_KEY); + } catch { + return null; + } }; export const setStoredToken = (token) => { if (!isBrowser()) return; - localStorage.setItem(TOKEN_KEY, token); + try { + localStorage.setItem(TOKEN_KEY, token); + } catch { + // Storage unavailable or quota exceeded — fail silently + } }; export const clearStoredToken = () => { if (!isBrowser()) return; - localStorage.removeItem(TOKEN_KEY); + try { + localStorage.removeItem(TOKEN_KEY); + } catch { + // Ignore + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/authStorage.js` around lines 5 - 24, Wrap all direct localStorage accesses in try/catch inside getStoredToken, setStoredToken, and clearStoredToken so any SecurityError/QuotaExceededError is caught; in getStoredToken return null on error, in setStoredToken/clearStoredToken swallow the error (or log it via a logger) and avoid throwing, and still guard with isBrowser() first; reference TOKEN_KEY and isBrowser() when adding the try/catch blocks so the functions consistently handle storage exceptions without leaking them into the auth flow.app/containers/Music/useToggleLike.js-4-13 (1)
4-13:⚠️ Potential issue | 🟡 MinorNo null guard on
songbefore accessingsong.trackId.If
handleToggleLikeis ever invoked without an argument (e.g., a stale event handler during unmount, or an incorrect call site), it will throw aTypeError. A minimal guard prevents an unhandled runtime error:🛡️ Proposed defensive fix
(song) => { + if (!song?.trackId) return; if (likedTrackIds[song.trackId]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Music/useToggleLike.js` around lines 4 - 13, handleToggleLike accesses song.trackId without guarding song; add a defensive null/undefined check at the top of the handleToggleLike callback (e.g., return early if song is falsy) so it never attempts to read song.trackId, then proceed to check likedTrackIds and call dispatchLike/dispatchUnlike as before; reference the handleToggleLike function and the likedTrackIds, dispatchLike, dispatchUnlike symbols when making the change.app/themes/palettes.js-10-21 (1)
10-21:⚠️ Potential issue | 🟡 Minor
darkPalettehas potentially inconsistentinputBg,placeholder, andbordervalues.Three values look suspect for a dark theme:
Token Dark value Light value Concern inputBg#d2d2db(light-ish grey)#eeeef4(very light)Dark-theme inputs typically have a dark background; #d2d2dbis brighter than expectedplaceholder#070708(near-black)#b0b0c0(muted grey)Near-black placeholder on a #d2d2dbinput is barely-legible and inverts the usual contrast expectation; may have been swapped with#0d0d0dbgborder#000000(pure black)#d8d8e4A pure-black border against a #0d0d0dbackground is indistinguishableIf the
inputBg/placeholdervalues were accidentally swapped compared to intent, verify against design specs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/themes/palettes.js` around lines 10 - 21, darkPalette has likely swapped/mischosen contrast tokens: inputBg ('#d2d2db') and placeholder ('#070708') are inverted for a dark theme and border ('#000000') is too indistinguishable against bg; update the darkPalette object so inputBg is a dark/low‑light color (near bg or surface), placeholder is a muted lighter grey for readability, and border uses a slightly lighter dark grey (not pure black) to create subtle separation—change the values in the darkPalette definition for inputBg, placeholder, and border to match the light/dark contrast expectations.app/components/styled/musicSearch.js-30-30 (1)
30-30:⚠️ Potential issue | 🟡 MinorHardcoded
#3d3d54breaks the theming system.Every other color in this file uses the
C.*CSS variable tokens to respond to dark/light theme switching. The hover border color at line 30 is hardcoded, so it will render incorrectly in the light theme (a dark-purple border on a light input).♻️ Proposed fix using a theme token
&:hover:not(:focus) { - border-color: `#3d3d54`; + border-color: ${C.surface}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/styled/musicSearch.js` at line 30, Replace the hardcoded border-color "#3d3d54" in the styled component's hover rule with the appropriate theme CSS variable (a C.* token used elsewhere in this file) so the input responds to dark/light themes; locate the border-color in the musicSearch styled component's hover rule and swap the literal value for the matching token (e.g., C.inputBorder or the token the file uses for input borders) so theming works consistently.app/components/ArtworkPlayButton/index.js-13-19 (1)
13-19:⚠️ Potential issue | 🟡 Minor
ArtworkWrapper(div) has noroleoraria-label, breaking keyboard/screen-reader accessibility.A
divwith anonClickis not focusable or announced as interactive by default. Withoutrole="button"and a descriptivearia-label, keyboard users can't reach it and screen readers won't announce the play/pause intent.♿ Proposed fix
- <ArtworkWrapper onClick={handleClick} data-testid="artwork-play-btn"> + <ArtworkWrapper + onClick={handleClick} + role="button" + tabIndex={0} + aria-label={isActive && isPlaying ? `Pause ${alt}` : `Play ${alt}`} + onKeyDown={(e) => e.key === 'Enter' && handleClick(e)} + data-testid="artwork-play-btn" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ArtworkPlayButton/index.js` around lines 13 - 19, The ArtworkWrapper div is not accessible: make it an interactive button by adding role="button", tabIndex={0}, and a descriptive aria-label that reflects the current action (e.g., aria-label={`Play ${alt}`} or aria-label={isPlaying ? 'Pause' : 'Play'}) and consider adding aria-pressed={isPlaying} for toggle state; also wire keyboard activation by handling keyDown (Enter/Space) to call handleClick. Update the ArtworkWrapper element and related handlers (handleClick and any parent component state using isPlaying) so screen readers announce the control and keyboard users can activate it.app/components/styled/authForm.js-55-57 (1)
55-57:⚠️ Potential issue | 🟡 MinorHardcoded
#3d3d54breaks the theme abstraction.Every other color in this file uses a
C.*token, making this raw hex value a maintenance hazard — it won't respond to theme changes. Either add a dedicated token (e.g.,C.inputHoverBorder) tocolors.js, or alias it to the nearest existing token.🐛 Proposed fix
In
app/components/styled/colors.js:export const C = { ... border: 'var(--musica-border)', + inputHoverBorder: 'var(--musica-inputHoverBorder)', ... };In
authForm.js:&:hover:not(:focus) { - border-color: `#3d3d54`; + border-color: ${C.inputHoverBorder}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/styled/authForm.js` around lines 55 - 57, The hover border color in the styled auth form is a hardcoded hex (`#3d3d54`) which breaks theme tokens; add a new color token (for example C.inputHoverBorder) to colors.js or map it to the closest existing token (e.g., C.borderMuted) and then replace the raw hex in the &:hover:not(:focus) rule inside authForm.js with that token (use C.inputHoverBorder or the chosen existing C.* token) so the component respects theme changes.app/containers/Auth/saga.js-25-26 (1)
25-26:⚠️ Potential issue | 🟡 Minor
failureAuth(response.data)dispatchesnullon network/timeout errors.When the request fails at the network layer (timeout, no connection), apisauce sets
response.ok = falseandresponse.dataisnullorundefined. DispatchingfailureAuth(null)means the reducer receives no error message, which may silently swallow the failure in the UI. Prefer a normalised fallback:🛡️ Proposed fix
- yield put(failureAuth(response.data)); + yield put(failureAuth(response.data ?? { message: 'Something went wrong. Please try again.' }));Apply this pattern in both
handleLoginandhandleSignup.Also applies to: 37-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Auth/saga.js` around lines 25 - 26, The failureAuth dispatch currently passes response.data which can be null for network/timeouts; update both handleLogin and handleSignup to normalize the error before dispatching: derive an errorMessage from response.data (if present) else fallback to response.problem/response.statusText/or a static string like "Network error" (or response.originalError if available), then call yield put(failureAuth(errorMessage)); replace the direct failureAuth(response.data) usages with this normalized error variable so reducers always receive a meaningful error string/object.app/utils/formatDuration.js-1-6 (1)
1-6:⚠️ Potential issue | 🟡 MinorAdd a guard for
undefined/NaNinput.If
msisundefined(e.g.,track.durationMsabsent from an API response),Math.floor(undefined / 1000)evaluates toNaNand the function returns the visible string"NaN:NaN".🛡️ Proposed fix
export const formatDuration = (ms) => { + if (ms == null || isNaN(ms) || ms < 0) return '0:00'; const totalSeconds = Math.floor(ms / 1000); const minutes = Math.floor(totalSeconds / 60); const seconds = totalSeconds % 60; return `${minutes}:${seconds.toString().padStart(2, '0')}`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/formatDuration.js` around lines 1 - 6, The formatDuration function should guard against undefined/NaN inputs: inside formatDuration check that the incoming ms is a finite number (e.g., via Number.isFinite(ms) or Number.isNaN) and if not, set ms to 0 (or return a safe fallback like "0:00"); then proceed to compute totalSeconds/minutes/seconds as before so the function never returns "NaN:NaN". Ensure you update the ms validation in the formatDuration function.app/components/TrackInfo/index.js-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorGenre tag always renders — inconsistent with other optional tags
track.genrehas no guard, so an empty<Tag>is emitted when genre is absent. All other metadata tags (durationMs,releaseDate,trackPrice) are conditionally rendered. Apply the same guard for consistency:🐛 Proposed fix
- {<Tag data-testid="tag-genre">{track.genre}</Tag>} + {track.genre && <Tag data-testid="tag-genre">{track.genre}</Tag>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/TrackInfo/index.js` at line 25, The Genre Tag always renders an empty Tag when track.genre is falsy; update the TrackInfo component to conditionally render the Tag only when track.genre exists. Locate the JSX using Tag and track.genre (the Genre Tag line) and wrap it with the same guard pattern used for durationMs/releaseDate/trackPrice so the Tag is only emitted when track.genre is truthy. Ensure you keep the existing data-testid ("tag-genre") on the Tag when it is rendered.app/contexts/tests/ThemeContext.test.js-59-63 (1)
59-63:⚠️ Potential issue | 🟡 MinorHardcoded storage key creates silent coupling
'musica_theme'is the privateTHEME_KEYfromthemeStorage.js. If it ever changes, these tests will continue to pass (localStorage will simply be empty) while the actual persistence feature is broken. ExportingTHEME_KEYfromthemeStorage.jsand importing it here removes the coupling.🛠 Suggested fix
In
app/utils/themeStorage.js:-const THEME_KEY = 'musica_theme'; +export const THEME_KEY = 'musica_theme';In
app/contexts/tests/ThemeContext.test.js:+import { THEME_KEY } from '../../utils/themeStorage' ... - expect(window.localStorage.getItem('musica_theme')).toBe('light') + expect(window.localStorage.getItem(THEME_KEY)).toBe('light') ... - window.localStorage.setItem('musica_theme', 'light') + window.localStorage.setItem(THEME_KEY, 'light')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/contexts/tests/ThemeContext.test.js` around lines 59 - 63, Export the private THEME_KEY constant from app/utils/themeStorage.js and update the tests in ThemeContext.test.js to import THEME_KEY (e.g., import { THEME_KEY } from 'app/utils/themeStorage') and use that constant in calls to window.localStorage.setItem/getItem instead of the hardcoded 'musica_theme' string; this ensures the test references the same key as the production code (look for THEME_KEY in themeStorage.js and replace string usages in ThemeContext.test.js).app/containers/Library/reducer.js-63-72 (1)
63-72:⚠️ Potential issue | 🟡 MinorNo handlers for
REQUEST_LIKE_SONG/REQUEST_UNLIKE_SONG— like/unlike failures callstopLoadingwithout a priorstartLoading.The
handleFetchFailurehandler (reused for like/unlike failures) callsstopLoading, but since no request handler setsloading = truefor like/unlike, thestopLoadingis a harmless no-op. However, the error will be set on failure without any mechanism to clear it on the next like/unlike attempt (no request handler resetserrortonull). This could leave a stale error message in the UI after a failed like/unlike, even if a subsequent attempt succeeds.♻️ Proposed fix — add lightweight request handlers for like/unlike to clear errors
+const handleLikeRequest = (draft) => { + draft[PAYLOAD.ERROR] = null; +}; + const handlers = { [libraryTypes.REQUEST_FETCH_LIBRARY]: handleFetchRequest, [libraryTypes.SUCCESS_FETCH_LIBRARY]: handleFetchSuccess, [libraryTypes.FAILURE_FETCH_LIBRARY]: handleFetchFailure, + [libraryTypes.REQUEST_LIKE_SONG]: handleLikeRequest, [libraryTypes.SUCCESS_LIKE_SONG]: handleLikeSuccess, [libraryTypes.FAILURE_LIKE_SONG]: handleFetchFailure, + [libraryTypes.REQUEST_UNLIKE_SONG]: handleLikeRequest, [libraryTypes.SUCCESS_UNLIKE_SONG]: handleUnlikeSuccess, [libraryTypes.FAILURE_UNLIKE_SONG]: handleFetchFailure, [libraryTypes.CLEAR_LIBRARY]: () => initialState };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Library/reducer.js` around lines 63 - 72, Add request handlers for libraryTypes.REQUEST_LIKE_SONG and libraryTypes.REQUEST_UNLIKE_SONG in the handlers map so like/unlike requests reset error and set loading state; either create small functions (e.g., handleLikeRequest / handleUnlikeRequest) or use inline handlers that return { ...state, loading: true, error: null } so subsequent failures/successes correctly clear stale errors and pair with existing handleLikeSuccess/handleUnlikeSuccess and handleFetchFailure behavior.app/containers/TrackDetail/index.js-54-62 (1)
54-62:⚠️ Potential issue | 🟡 MinorConditional blocks are not mutually exclusive — overlapping states can render simultaneously.
Since each
<If>block is independent, if bothloadinganderrorare truthy at the same time (e.g., a fetch failure that doesn't clearloading), both the spinner and the error message will render. Consider usingelse-ifsemantics or a single conditional branch.♻️ Proposed fix — use early returns or ternary for exclusive rendering
- <If condition={loading}> - <LoadingSpinner data-testid="loading-spinner" /> - </If> - <If condition={!!error}> - <EmptyState data-testid="error-state">Failed to load track details.</EmptyState> - </If> - <If condition={!loading && !error && !!trackData}> - <TrackInfo track={trackData || {}} onPlay={handlePlay} /> - </If> + <If condition={loading} otherwise={ + <If condition={!!error} otherwise={ + <If condition={!!trackData}> + <TrackInfo track={trackData} onPlay={handlePlay} /> + </If> + }> + <EmptyState data-testid="error-state">Failed to load track details.</EmptyState> + </If> + }> + <LoadingSpinner data-testid="loading-spinner" /> + </If>Alternatively, a simpler approach with plain ternaries may be more readable:
{loading ? ( <LoadingSpinner data-testid="loading-spinner" /> ) : error ? ( <EmptyState data-testid="error-state">Failed to load track details.</EmptyState> ) : trackData ? ( <TrackInfo track={trackData} onPlay={handlePlay} /> ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/TrackDetail/index.js` around lines 54 - 62, The three independent <If> blocks in the TrackDetail render allow overlapping states (e.g., loading and error) to display simultaneously; change the rendering to an exclusive branch (early return or chained conditional) so only one state renders: use loading first, then error, then trackData — replace the three <If> blocks with a single conditional expression or early-return in the TrackDetail component that renders LoadingSpinner when loading is true, EmptyState when error is truthy, TrackInfo (passing trackData and handlePlay) when trackData exists, otherwise null.app/components/AudioPlayer/useAudioPlayer.js-67-71 (1)
67-71:⚠️ Potential issue | 🟡 Minor
seekdoes not update thecurrentTimestate.Setting
audioRef.current.currentTimewill trigger thetimeupdateevent asynchronously, so the UI slider value won't immediately reflect the seek position, causing a brief visual snap-back on the progress slider. Consider also callingsetCurrentTime(time)for immediate feedback.♻️ Proposed fix
const seek = useCallback((time) => { if (audioRef.current) { audioRef.current.currentTime = time; + setCurrentTime(time); } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/AudioPlayer/useAudioPlayer.js` around lines 67 - 71, The seek function only sets audioRef.current.currentTime which updates asynchronously; update UI immediately by also calling setCurrentTime(time) inside useCallback so the slider reflects the new position instantly. Modify the seek callback (function seek) to call setCurrentTime(time) after verifying audioRef.current exists, keeping the existing audioRef.current.currentTime = time assignment so the actual media position still changes. Ensure you import/use the same setCurrentTime state updater that the component uses for tracking currentTime so the UI state and media position stay in sync.app/components/AudioPlayer/index.js-65-65 (1)
65-65:⚠️ Potential issue | 🟡 MinorUse CSS variable for accent color to maintain theme consistency.
The hardcoded color
#ff6b35bypasses the theme system. All theme colors should usevar(--musica-accent)which is dynamically set based on the active palette.♻️ Proposed fix
- <SoundFilled data-testid="volume-icon" style={{ color: '#ff6b35', fontSize: '1rem' }} /> + <SoundFilled data-testid="volume-icon" style={{ color: 'var(--musica-accent)', fontSize: '1rem' }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/AudioPlayer/index.js` at line 65, The volume icon uses a hardcoded color '#ff6b35' in the SoundFilled element (data-testid="volume-icon"), which bypasses theme variables; update the inline style in the SoundFilled component to use the CSS variable var(--musica-accent) for color (e.g., style={{ color: 'var(--musica-accent)', fontSize: '1rem' }}) so the icon follows the active palette and theme system.
| useEffect(() => { | ||
| const audio = new Audio(); | ||
| audio.volume = DEFAULT_VOLUME; | ||
| audioRef.current = audio; | ||
|
|
||
| const onTime = () => setCurrentTime(audio.currentTime); | ||
| const onLoaded = () => setDuration(audio.duration); | ||
| const onEnded = () => { | ||
| setIsPlaying(false); | ||
| if (onPlayStateChange) { | ||
| onPlayStateChange(false); | ||
| } | ||
| if (onSongEnd) { | ||
| onSongEnd(); | ||
| } | ||
| }; | ||
|
|
||
| audio.addEventListener('timeupdate', onTime); | ||
| audio.addEventListener('loadedmetadata', onLoaded); | ||
| audio.addEventListener('ended', onEnded); | ||
|
|
||
| return () => { | ||
| audio.removeEventListener('timeupdate', onTime); | ||
| audio.removeEventListener('loadedmetadata', onLoaded); | ||
| audio.removeEventListener('ended', onEnded); | ||
| audio.pause(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Stale closure: onSongEnd and onPlayStateChange are captured once at mount and never updated.
The first useEffect has an empty dependency array, so the onEnded handler permanently captures the initial onSongEnd and onPlayStateChange references from the first render. If these callbacks change (e.g., parent re-renders with new props), the audio ended event will invoke the stale versions.
Store callbacks in refs to keep them current:
🐛 Proposed fix
+ const onSongEndRef = useRef(onSongEnd);
+ const onPlayStateChangeRef = useRef(onPlayStateChange);
+
+ useEffect(() => { onSongEndRef.current = onSongEnd; }, [onSongEnd]);
+ useEffect(() => { onPlayStateChangeRef.current = onPlayStateChange; }, [onPlayStateChange]);
+
useEffect(() => {
const audio = new Audio();
audio.volume = DEFAULT_VOLUME;
audioRef.current = audio;
const onTime = () => setCurrentTime(audio.currentTime);
const onLoaded = () => setDuration(audio.duration);
const onEnded = () => {
setIsPlaying(false);
- if (onPlayStateChange) {
- onPlayStateChange(false);
+ if (onPlayStateChangeRef.current) {
+ onPlayStateChangeRef.current(false);
}
- if (onSongEnd) {
- onSongEnd();
+ if (onSongEndRef.current) {
+ onSongEndRef.current();
}
};Apply the same pattern for onPlayStateChange used inside the song-change effect on line 46.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/AudioPlayer/useAudioPlayer.js` around lines 11 - 38, The audio
ended handler inside the useEffect mounts with stale references to onSongEnd and
onPlayStateChange; fix it by storing those callbacks in refs (e.g., onSongEndRef
and onPlayStateChangeRef), update those refs whenever the props change, and have
the onEnded closure call the current ref values (onSongEndRef.current,
onPlayStateChangeRef.current) instead of the captured variables; update the
effect that sets up listeners (the useEffect that creates audio and adds
'ended') to read from the refs so the latest callbacks run while keeping the
same mount/unmount listener logic.
| it('should navigate to track detail when card clicked', () => { | ||
| const { getByTestId } = renderProvider(<SongList {...defaultProps} />) | ||
| fireEvent.click(getByTestId('song-1')) | ||
| expect(mockPush).toHaveBeenCalledWith('/track/1') | ||
| }) | ||
|
|
||
| it('should call onPlayToggle when artwork clicked', () => { | ||
| const { getAllByTestId } = renderProvider(<SongList {...defaultProps} />) | ||
| fireEvent.click(getAllByTestId('artwork-play-btn')[0]) | ||
| expect(mockPlayToggle).toHaveBeenCalledWith(mockSongs[0]) | ||
| }) |
There was a problem hiding this comment.
Event bubbling: clicking play button will also trigger card navigation.
Looking at the SongList component (app/components/SongList/index.js), ArtworkPlayButton's onClick is nested inside SongCard's onClick. Without e.stopPropagation(), clicking the play button will also fire handleCardClick, navigating the user away. The same applies to the HeartButton. These tests pass in isolation because the mock doesn't simulate bubbling fully, but the real behavior would trigger both handlers.
Consider adding e.stopPropagation() in the ArtworkPlayButton and HeartButton onClick wrappers inside SongList.
#!/bin/bash
# Check SongList component for stopPropagation usage
ast-grep --pattern $'onClick={($_) => {
$$$
stopPropagation
$$$
}}'
rg -n 'stopPropagation' --type=js -g '**/SongList/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/SongList/tests/index.test.js` around lines 58 - 68, The
SongList component currently nests ArtworkPlayButton and HeartButton click
handlers inside SongCard's onClick (handleCardClick), causing clicks on those
buttons to bubble and trigger navigation; update the onClick wrappers for
ArtworkPlayButton and HeartButton in SongList (the arrow functions passed as
onClick for ArtworkPlayButton and HeartButton) to accept the event param and
call e.stopPropagation() before invoking onPlayToggle/onHeartToggle so the
button clicks do not propagate to handleCardClick and trigger navigation.
| required | ||
| /> | ||
| </InputWrapper> | ||
| {error && <ErrorMessage data-testid="login-error">{error}</ErrorMessage>} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the login page file
fd -type f -name "*.js" -path "*/pages/*" | grep -i login | head -20Repository: wednesday-solutions/nextjs-template
Length of output: 249
🏁 Script executed:
# Also search for LoginForm usage in pages
rg "LoginForm" --type=js -A 3 -B 1 pages/ 2>/dev/null | head -50Repository: wednesday-solutions/nextjs-template
Length of output: 699
🏁 Script executed:
# Search for selectAuthError usage
rg "selectAuthError" --type=js -A 2 -B 2 2>/dev/null | head -80Repository: wednesday-solutions/nextjs-template
Length of output: 2662
🏁 Script executed:
# Look at the LoginForm component to see PropTypes and error handling
rg "PropTypes\|error" app/containers/Auth/LoginForm.js -A 1 -B 1 | head -40Repository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Find and examine the auth saga
rg "failureAuth\|PAYLOAD" app/containers/Auth/saga.js -A 2 -B 1 | head -80Repository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Check the auth reducer for PAYLOAD constant and how error is stored
rg "PAYLOAD\|ERROR" app/containers/Auth/reducer.js -A 2 -B 1 | head -100Repository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Look at LoginForm.js PropTypes declaration (lines 70-74 approximately)
sed -n '65,80p' app/containers/Auth/LoginForm.jsRepository: wednesday-solutions/nextjs-template
Length of output: 269
🏁 Script executed:
# Also get the full picture of what happens in saga with error handling
rg "failureAuth|catch|response" app/containers/Auth/saga.js -A 3 -B 1 | head -100Repository: wednesday-solutions/nextjs-template
Length of output: 779
🏁 Script executed:
# Search for authCreators definition and failureAuth action
rg "authCreators\|failureAuth" app/containers/Auth/ --type-list | head -5Repository: wednesday-solutions/nextjs-template
Length of output: 468
🏁 Script executed:
# Find the reducer file and see how it handles failureAuth
fd -name "reducer.js" app/containers/Auth/Repository: wednesday-solutions/nextjs-template
Length of output: 309
🏁 Script executed:
# Check the Auth container structure
ls -la app/containers/Auth/Repository: wednesday-solutions/nextjs-template
Length of output: 549
🏁 Script executed:
# Search for how authCreators is defined
rg -n "const authCreators\|export.*failureAuth\|failureAuth.*=" app/containers/Auth/ -A 3 -B 1 | head -100Repository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Read the full reducer file
cat app/containers/Auth/reducer.jsRepository: wednesday-solutions/nextjs-template
Length of output: 1352
🏁 Script executed:
# Read the full saga file
cat app/containers/Auth/saga.jsRepository: wednesday-solutions/nextjs-template
Length of output: 1410
🏁 Script executed:
# Search for where action creators are defined
rg "successAuth\|failureAuth" app/containers/Auth/reducer.js -A 2 -B 2Repository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Check the utils/reducer file to understand setError
rg "setError\|PAYLOAD" app/utils/reducer.js -A 3 -B 1Repository: wednesday-solutions/nextjs-template
Length of output: 61
🏁 Script executed:
# Also check the full utils/reducer.js file if short enough
wc -l app/utils/reducer.jsRepository: wednesday-solutions/nextjs-template
Length of output: 102
🏁 Script executed:
# Read the full utils/reducer.js
cat app/utils/reducer.jsRepository: wednesday-solutions/nextjs-template
Length of output: 597
Verify that error is always a string before being passed to LoginForm.
The component renders {error} directly and declares PropTypes.string. However, the auth saga dispatches failureAuth(response.data) where response.data is the raw API error payload — typically an object (e.g., { message: '...' }). This object gets stored in the state and passed directly to LoginForm without extraction. If the API returns an error object instead of a string, React will throw "Objects are not valid as a React child" at runtime.
The chain is: saga → failureAuth(response.data) → state.error = response.data → selectAuthError() → error prop → rendered as {error}.
Either extract the error message string in the saga (e.g., failureAuth(response.data?.message || 'something_went_wrong')) before dispatching, or ensure the API response shape is normalized before reaching this component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/containers/Auth/LoginForm.js` at line 55, The LoginForm is rendering the
error prop directly (LoginForm -> {error}) but the saga dispatches
failureAuth(response.data) where response.data can be an object, causing
"Objects are not valid as a React child"; update the saga that calls failureAuth
to pass a string instead (e.g., extract response.data.message or fallback to a
default like 'Something went wrong') before dispatching, or normalize
state.error in the reducer/selector (selectAuthError) to return a string, so
LoginForm always receives a string error prop.
| it('should set error when FAILURE_AUTH is dispatched', () => { | ||
| const expectedResult = { | ||
| ...state, | ||
| [PAYLOAD.ERROR]: 'something_went_wrong', | ||
| loading: false | ||
| } | ||
| expect(authReducer(state, { type: authTypes.FAILURE_AUTH })).toEqual( | ||
| expectedResult | ||
| ) | ||
| }) |
There was a problem hiding this comment.
FAILURE_AUTH test will fail — error is never supplied in the dispatched action.
The action { type: authTypes.FAILURE_AUTH } carries no error field, so setError(draft, action) sets draft[PAYLOAD.ERROR] to undefined. The expected result asserts [PAYLOAD.ERROR]: 'something_went_wrong', which will never match.
🐛 Proposed fix — supply the error in the action
it('should set error when FAILURE_AUTH is dispatched', () => {
+ const errorMessage = 'something_went_wrong'
const expectedResult = {
...state,
- [PAYLOAD.ERROR]: 'something_went_wrong',
+ [PAYLOAD.ERROR]: errorMessage,
loading: false
}
- expect(authReducer(state, { type: authTypes.FAILURE_AUTH })).toEqual(
+ expect(authReducer(state, { type: authTypes.FAILURE_AUTH, [PAYLOAD.ERROR]: errorMessage })).toEqual(
expectedResult
)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/containers/Auth/tests/reducer.test.js` around lines 45 - 54, The test for
FAILURE_AUTH dispatches an action without an error so authReducer (and its
helper setError) sets PAYLOAD.ERROR to undefined; update the test to dispatch
the error payload (include an error property equal to 'something_went_wrong' on
the action passed to authReducer for authTypes.FAILURE_AUTH) so the resulting
state matches expectedResult that checks [PAYLOAD.ERROR] and loading: false.
| <If condition={loading}> | ||
| <LoadingSpinner data-testid="loading-spinner" /> | ||
| </If> | ||
| <If condition={!loading && likedSongs.length > 0}> | ||
| <SongList | ||
| songs={likedSongs} | ||
| currentSong={currentSong} | ||
| isPlaying={isPlaying} | ||
| onPlayToggle={handlePlayToggle} | ||
| likedTrackIds={likedTrackIds} | ||
| onToggleLike={handleToggleLike} | ||
| /> | ||
| </If> | ||
| <If condition={!loading && likedSongs.length === 0}> | ||
| <EmptyState data-testid="empty-library"> | ||
| No liked songs yet. Search and like songs to build your library. | ||
| </EmptyState> | ||
| </If> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for selectLibraryError in the codebase
rg -n 'selectLibraryError' --type=jsRepository: wednesday-solutions/nextjs-template
Length of output: 508
🏁 Script executed:
#!/bin/bash
# Read the Library component file to see the complete mapStateToProps and imports
wc -l app/containers/Library/index.jsRepository: wednesday-solutions/nextjs-template
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check if the file exists and get its structure
head -100 app/containers/Library/index.jsRepository: wednesday-solutions/nextjs-template
Length of output: 3731
🏁 Script executed:
#!/bin/bash
# Read the remaining part of the file to see the mapStateToProps
tail -30 app/containers/Library/index.jsRepository: wednesday-solutions/nextjs-template
Length of output: 1109
Add error state handling to prevent misleading empty state on fetch failure.
selectLibraryError exists in selectors.js but is not imported or used in mapStateToProps. If fetchLibrary fails, users see the "No liked songs yet" empty state instead of an error message, which is misleading. Import the selector and render an error state when a fetch fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/containers/Library/index.js` around lines 59 - 76, Import
selectLibraryError and include it in mapStateToProps (e.g., mapStateToProps ->
error: selectLibraryError(state)), pass the error prop into the Library
component, and add an early render branch in the JSX to display an error state
when error is truthy (e.g., render an ErrorState/ErrorMessage component or
element) before the empty-state branch so a fetch failure shows an error instead
of "No liked songs yet"; update any tests or data-testid usage as needed.
| const handleNext = useCallback(() => { | ||
| const idx = findIdx(); | ||
| if (idx < songs.length - 1) { | ||
| dispatchSetSong(songs[idx + 1]); | ||
| } | ||
| }, [songs, findIdx, dispatchSetSong]); |
There was a problem hiding this comment.
handleNext dispatches the wrong song when currentSong is absent from songs.
When findIdx() returns -1 (e.g., when the Library container passes likedSongs but currentSong is from a global search result), the guard idx < songs.length - 1 evaluates to true for any non-empty list, causing songs[0] to be dispatched silently.
🐛 Proposed fix
const handleNext = useCallback(() => {
const idx = findIdx();
- if (idx < songs.length - 1) {
+ if (idx !== -1 && idx < songs.length - 1) {
dispatchSetSong(songs[idx + 1]);
}
}, [songs, findIdx, dispatchSetSong]);📝 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.
| const handleNext = useCallback(() => { | |
| const idx = findIdx(); | |
| if (idx < songs.length - 1) { | |
| dispatchSetSong(songs[idx + 1]); | |
| } | |
| }, [songs, findIdx, dispatchSetSong]); | |
| const handleNext = useCallback(() => { | |
| const idx = findIdx(); | |
| if (idx !== -1 && idx < songs.length - 1) { | |
| dispatchSetSong(songs[idx + 1]); | |
| } | |
| }, [songs, findIdx, dispatchSetSong]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/containers/Music/usePlaybackNav.js` around lines 6 - 11, The handleNext
logic dispatches an incorrect song when findIdx() returns -1; change the guard
in handleNext (function handleNext) to ensure idx is a valid index before
advancing (i.e., require idx >= 0 && idx < songs.length - 1) and do nothing (or
return early) when idx === -1 so dispatchSetSong(songs[idx + 1]) is never called
for an absent currentSong; update any related callers that assume a next song
only when findIdx() yields a valid index.
| case 'auth': | ||
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000'); | ||
| return apiClients[type]; | ||
| case 'music': | ||
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000', { skipRequestTransform: true }); |
There was a problem hiding this comment.
Hardcoded localhost:9000 will break in any non-local deployment.
Both 'auth' and 'music' clients use 'http://localhost:9000' as baseURL. This needs to be driven by environment variables so staging and production can be configured correctly.
🔧 Proposed fix
- case 'auth':
- apiClients[type] = createApiClientWithTransForm('http://localhost:9000');
+ case 'auth':
+ apiClients[type] = createApiClientWithTransForm(process.env.NEXT_PUBLIC_AUTH_URL);
return apiClients[type];
- case 'music':
- apiClients[type] = createApiClientWithTransForm('http://localhost:9000', { skipRequestTransform: true });
+ case 'music':
+ apiClients[type] = createApiClientWithTransForm(process.env.NEXT_PUBLIC_MUSIC_URL, { skipRequestTransform: true });
return apiClients[type];Add NEXT_PUBLIC_AUTH_URL and NEXT_PUBLIC_MUSIC_URL to .env.local, .env.staging, and .env.production.
📝 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.
| case 'auth': | |
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000'); | |
| return apiClients[type]; | |
| case 'music': | |
| apiClients[type] = createApiClientWithTransForm('http://localhost:9000', { skipRequestTransform: true }); | |
| case 'auth': | |
| apiClients[type] = createApiClientWithTransForm(process.env.NEXT_PUBLIC_AUTH_URL); | |
| return apiClients[type]; | |
| case 'music': | |
| apiClients[type] = createApiClientWithTransForm(process.env.NEXT_PUBLIC_MUSIC_URL, { skipRequestTransform: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/utils/apiUtils.js` around lines 19 - 23, Replace the hardcoded
'http://localhost:9000' in the switch cases for 'auth' and 'music' by reading
their base URLs from environment variables; update the 'auth' case and the
'music' case where apiClients[type] is assigned via createApiClientWithTransForm
to use process.env.NEXT_PUBLIC_AUTH_URL and process.env.NEXT_PUBLIC_MUSIC_URL
respectively (optionally with a local fallback), and ensure those env vars are
documented/added to .env.local/.env.staging/.env.production so
staging/production use the correct endpoints.
| const TOKEN_KEY = 'musica_access_token'; | ||
|
|
||
| const isBrowser = () => typeof window !== 'undefined'; | ||
|
|
||
| export const getStoredToken = () => { | ||
| if (!isBrowser()) { | ||
| return null; | ||
| } | ||
| return localStorage.getItem(TOKEN_KEY); | ||
| }; | ||
|
|
||
| export const setStoredToken = (token) => { | ||
| if (!isBrowser()) { | ||
| return; | ||
| } | ||
| localStorage.setItem(TOKEN_KEY, token); | ||
| }; | ||
|
|
||
| export const clearStoredToken = () => { | ||
| if (!isBrowser()) { | ||
| return; | ||
| } | ||
| localStorage.removeItem(TOKEN_KEY); | ||
| }; |
There was a problem hiding this comment.
Storing the auth token in localStorage exposes it to XSS.
Any script running on the page (including injected third-party or XSS payloads) can read localStorage and exfiltrate the token. HttpOnly cookies are immune to this class of attack because they are never accessible to JavaScript.
If migrating to HttpOnly cookies is out of scope for this PR, the risk should at minimum be documented, and an issue should be created to revisit this before the app handles real user credentials.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/utils/authStorage.js` around lines 1 - 24, The helpers TOKEN_KEY,
isBrowser, getStoredToken, setStoredToken, and clearStoredToken currently
persist the auth token to localStorage which exposes it to XSS; replace this by
moving token storage to HttpOnly, Secure cookies set by the server (use
Set-Cookie on login/refresh and rely on cookie sending rather than reading via
JS) and remove client-side reads/writes to localStorage, or if server-side
cookie migration is out of scope: update these functions to no longer store
sensitive tokens, add an explicit code comment documenting the XSS risk, and
open a ticket to implement HttpOnly cookie-based auth (include steps: issue
should require server endpoints to set/clear Secure, HttpOnly cookies on auth
and refresh and update client to stop reading localStorage).
| NEXT_PUBLIC_POSTHOG_KEY=phc_s5dpMW7DvBDzwFpo0dWrj59N2iFi6aj59yn4bW8gmP8 | ||
| NEXT_PUBLIC_POSTHOG_HOST=https://us.i.posthog.com No newline at end of file |
There was a problem hiding this comment.
Same PostHog key across all three environments will mix production and non-production analytics data.
The PostHog project API key (phc_…) is a write-only key designed to be safe for public client-side use. However, two separate problems exist here:
-
Environment data pollution: The identical key
phc_s5dpMW7DvBDzwFpo0dWrj59N2iFi6aj59yn4bW8gmP8appears verbatim in.env.qa,.env.development, and.env.production. All three environments send events to the same PostHog project, making it impossible to isolate production user behaviour from dev/QA noise. PostHog recommends creating multiple projects for sandbox and production environments — the sandbox API key should point to a separate PostHog project used for testing. -
Gitleaks CI failure: Gitleaks (8.30.0) flags line 2 as a high-severity Generic API Key, which will fail any CI pipeline that runs secret scanning. Even though the key is write-only, it should be injected at build time via CI/CD secret variables rather than hard-coded in source files.
🛠️ Suggested remediation
Remove the hard-coded values from all three env files and inject them via your CI/CD platform (e.g., GitHub Actions secrets, AWS SSM):
-NEXT_PUBLIC_POSTHOG_KEY=phc_s5dpMW7DvBDzwFpo0dWrj59N2iFi6aj59yn4bW8gmP8
-NEXT_PUBLIC_POSTHOG_HOST=https://us.i.posthog.com
+NEXT_PUBLIC_POSTHOG_KEY=
+NEXT_PUBLIC_POSTHOG_HOST=Use distinct PostHog projects (and therefore distinct keys) for development, QA, and production.
📝 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.
| NEXT_PUBLIC_POSTHOG_KEY=phc_s5dpMW7DvBDzwFpo0dWrj59N2iFi6aj59yn4bW8gmP8 | |
| NEXT_PUBLIC_POSTHOG_HOST=https://us.i.posthog.com | |
| NEXT_PUBLIC_POSTHOG_KEY= | |
| NEXT_PUBLIC_POSTHOG_HOST= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 3-3: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 3-3: [UnorderedKey] The NEXT_PUBLIC_POSTHOG_HOST key should go before the NEXT_PUBLIC_POSTHOG_KEY key
(UnorderedKey)
🪛 Gitleaks (8.30.0)
[high] 2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@environments/.env.qa` around lines 2 - 3, Remove the hard-coded PostHog API
key from environments/.env.qa (and the other .env files) and replace with a
reference to an injected secret; specifically stop committing
NEXT_PUBLIC_POSTHOG_KEY and ensure NEXT_PUBLIC_POSTHOG_HOST remains configurable
if needed. Provision separate PostHog projects/keys for production vs
QA/development and configure your CI/CD (e.g., GitHub Actions secrets or AWS
SSM) to inject the appropriate NEXT_PUBLIC_POSTHOG_KEY per environment at
build/deploy time to avoid mixing telemetry and to satisfy secret-scanning
(gitleaks) requirements.
| "npm run lint:eslint:fix", | ||
| "git add --force", | ||
| "jest --findRelatedTests --passWithNoTests $STAGED_FILES" | ||
| "jest --findRelatedTests -u --passWithNoTests $STAGED_FILES" |
There was a problem hiding this comment.
-u in pre-commit silently auto-updates snapshots, masking regressions; $STAGED_FILES is a no-op.
Two problems on this line:
-
-u/--updateSnapshotin the pre-commit hook — any component whose rendered output has changed (intentionally or not) will have its snapshot automatically overwritten and committed without any developer warning. This defeats the purpose of snapshot tests as a regression guard. Snapshot updates should only happen when a developer runsjest -umanually and reviews the diff. Remove-ufrom the pre-commit command. -
$STAGED_FILES— lint-staged does not set this environment variable. lint-staged takes care of passing the file paths of the staged files itself; your linter-command should just be written to run for a given file path. The variable expands to an empty string in the shell; staged files are still correctly appended by lint-staged as positional arguments, so the test run is functionally unaffected, but the token is misleading and should be removed.
🛠️ Proposed fix
- "jest --findRelatedTests -u --passWithNoTests $STAGED_FILES"
+ "jest --findRelatedTests --passWithNoTests"📝 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.
| "jest --findRelatedTests -u --passWithNoTests $STAGED_FILES" | |
| "jest --findRelatedTests --passWithNoTests" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 34, Remove the snapshot auto-update flag and the
misleading env var from the pre-commit jest command: in package.json update the
lint-staged / pre-commit entry that currently runs "jest --findRelatedTests -u
--passWithNoTests $STAGED_FILES" by deleting the "-u" / "--updateSnapshot" token
and removing "$STAGED_FILES" so the command becomes "jest --findRelatedTests
--passWithNoTests" (lint-staged will supply file paths).
1 participant
Summary by CodeRabbit
Release Notes