Resolve build failures, upgrade Next.js and dependencies #102
Conversation
WalkthroughAdds a PWA manifest and a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant Worker as indexedDBWorker
participant Storage as localStorage
rect rgba(135,206,250,0.12)
UI->>Storage: toggleChannel -> persist selectedChannels (try/catch)
Note right of Storage: Persistence may warn on failure
end
rect rgba(144,238,144,0.12)
UI->>Worker: request saveAllDataAsZip
Worker-->>UI: return generated Blob (ZIP)
UI->>UI: show toast success / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Connection.tsx (1)
361-379:setCanvasCountInWorkersends a stale canvasCount value to the workerHere you compute
newCountfromselectedChannels.lengthand only update the parent when it differs, but the value you send to the worker is stillcanvasElementCountRef.current, which lags behindnewCountuntil the separateuseEffectsyncing the ref runs. As a result, the worker can briefly receive an out‑of‑date canvasCount while the parent state has already moved on.You can avoid this desync by basing both the parent update and the worker message on the same
newCount:- const setCanvasCountInWorker = (canvasCount: number) => { + const setCanvasCountInWorker = () => { if (!workerRef.current) { initializeWorker(); } - // Update parent canvasCount only when it differs to avoid unnecessary rerenders - const newCount = selectedChannels.length; - if (typeof setCanvasCount === 'function' && newCount !== canvasCount) { - setCanvasCount(newCount); - } - - // Send canvasCount independently to the worker - workerRef.current?.postMessage({ action: 'setCanvasCount', canvasCount: canvasElementCountRef.current }); + // Update parent canvasCount only when it differs to avoid unnecessary rerenders + const newCount = selectedChannels.length; + if (typeof setCanvasCount === 'function' && newCount !== canvasElementCountRef.current) { + setCanvasCount(newCount); + } + + // Send the same count to the worker to keep it in sync + workerRef.current?.postMessage({ action: 'setCanvasCount', canvasCount: newCount }); }; @@ - useEffect(() => { - setCanvasCountInWorker(canvasElementCountRef.current); + useEffect(() => { + setCanvasCountInWorker(); // eslint-disable-next-line react-hooks/exhaustive-deps }, [selectedChannels.length]);(Adjust any other call sites of
setCanvasCountInWorkeraccordingly if they exist.)
🧹 Nitpick comments (3)
src/components/Connection.tsx (2)
282-318: Avoid side‑effects inside thesetSelectedChannelsupdater; manual flag placement looks goodMoving
setManuallySelected(true)outside the state updater is a good change for clearer semantics of “manual” selection. However, thesetSelectedChannelsfunctional updater now performs side‑effects (localStorage access,portRef.current?.getInfo()) inside the updater callback. React may invoke updater functions more than once (especially under StrictMode/concurrent rendering), so these side‑effects can run multiple times with the same arguments and are harder to reason about.Consider:
- Keeping the updater purely functional, and
- Persisting
sortedChannelsvia auseEffectthat depends on[selectedChannels, devicenameref.current], or by computing the next selection outside the updater and performing the persistence after callingsetSelectedChannels.Also,
portInfois only used as a truthy guard; if you intend to key devices by(usbVendorId, usbProductId)(as in the earlieruseEffect), consider reusing that predicate here for consistency with other saved‑device lookups.
420-440: ZIP toasts are good; consider centralizing worker message handlingThe success/error toasts on ZIP creation are a nice UX improvement and hook into the worker response correctly. One thing to be aware of: this function assigns
workerRef.current.onmessageinsidesaveAllDataAsZip, and other worker operations (e.g.,saveDataByFilename,getFileCountFromIndexedDB, delete functions) also overwriteonmessage. If any of these are triggered around the same time, the last assignment wins and earlier handlers may never see their responses.Longer‑term, consider a pattern like:
- A single
workerRef.current.onmessage(oraddEventListener("message", ...)) registered once, and- Routing based on
event.data.actionto the appropriate handler, or using per‑request IDs.Optionally, you could also surface a generic
toast.errorin the outercatchto cover unexpected exceptions.package.json (1)
15-48: Verify framework and tooling compatibility after major version bumpsThis block upgrades a large set of Radix UI deps plus the core stack (
next→ ^16.0.3,react/react-dom→ 18.3.1). Given the size of the jump:
- Please double‑check the expected React peer version for your targeted Next.js release. The provided Next 15 documentation in this PR’s context notes that Next 15 expects React 19 as a minimum; your config currently uses a newer Next (16.x) with React 18.3.1, so it’s worth confirming that this combo is officially supported for your exact Next version.
- Consider aligning
eslint-config-next(still pinned at 14.2.3) with the Next.js major you’re running, so lint rules match the runtime’s behavior.As per library documentation provided in the PR context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/app/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (11)
package.json(2 hunks)public/manifest.json(1 hunks)src/app/layout.tsx(1 hunks)src/app/npg-lite/page.tsx(1 hunks)src/components/Canvas.tsx(2 hunks)src/components/Connection.tsx(5 hunks)src/components/Contributors.tsx(2 hunks)src/components/DataPass.tsx(2 hunks)src/components/FFT.tsx(3 hunks)tsconfig.json(2 hunks)workers/indexedDBWorker.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-25T08:14:49.164Z
Learnt from: Ritika8081
Repo: upsidedownlabs/Chords-Web PR: 74
File: src/components/Connection.tsx:432-454
Timestamp: 2025-01-25T08:14:49.164Z
Learning: For the Chords-Web project, the user prefers simpler Web Worker implementations without additional type safety or error handling layers.
Applied to files:
workers/indexedDBWorker.ts
🧬 Code graph analysis (2)
src/components/Connection.tsx (1)
src/components/ui/tooltip.tsx (1)
Tooltip(30-30)
src/app/layout.tsx (1)
scripts/generateManifest.mjs (1)
basePath(16-16)
🔇 Additional comments (13)
src/components/Contributors.tsx (3)
16-16: LGTM! DialogTitle import added for accessibility.The addition of
DialogTitleto the imports is appropriate and is correctly used below to provide an accessible title for the dialog, which may be required in Next.js 15 or updated Radix UI versions.
81-81: Excellent accessibility improvement!The addition of
DialogTitlewith thesr-onlyclass properly addresses accessibility requirements, likely introduced in Next.js 15 or updated Radix UI versions. This ensures screen readers can announce the dialog's purpose without affecting the visual layout.
65-79: I need to verify the tooltip and dialog interaction pattern in the actual codebase. Let me gather information:I found critical documentation about this pattern. The correct nesting places the Tooltip wrapper outside, with TooltipTrigger wrapping DialogTrigger, and both using asChild to pass event handlers to the Button element. Let me verify this matches your actual code:
No issues found with the tooltip and dialog interaction pattern.
The code correctly places the outermost wrapper (Tooltip) outside, with the Button element inside both DialogTrigger and TooltipTrigger using asChild. This nesting order follows the documented best practice for combining Tooltip and Dialog components in shadcn/ui. The pattern allows tooltips to appear on hover while dialogs open on click without event propagation conflicts.
tsconfig.json (2)
38-40: LGTM: Include paths expanded appropriately.The addition of
.next/dev/types/**/*.ts,out/types/**/*.ts, andscripts/generateManifest.mjsaligns with Next.js 15's type generation and build artifacts.
18-18: Modern JSX transform is correctly configured.The change to
"react-jsx"is compatible with React 18.3.1 and Next.js 16.0.3, and aligns with modern React best practices. No issues detected.src/app/npg-lite/page.tsx (1)
787-803: LGTM: Manual selection flag updated after state stabilizes.Moving
setManuallySelected(true)outside thesetSelectedChannelsupdater ensures the flag is set after the state update completes, providing clearer sequencing and avoiding potential race conditions.src/components/DataPass.tsx (1)
23-46: LGTM: Counter tracking refactored to use React ref.The refactor from a local
letvariable touseRefis the correct pattern for maintaining mutable counter state across renders without triggering re-renders. All references topreviousCounter.currentare correctly updated.src/components/Canvas.tsx (1)
43-178: LGTM: Counter tracking refactored to use React ref.Consistent with the refactoring pattern applied across other components (DataPass.tsx, FFT.tsx), this change correctly migrates from a local mutable variable to a React ref for loss detection tracking. All references are properly updated to use
.current.workers/indexedDBWorker.ts (2)
258-261: LGTM: Correct separation of worker and UI concerns.The comment correctly identifies that workers must not access UI. Returning the blob to the main thread for user feedback is the proper architecture for web workers.
Based on learnings: The user prefers simpler Web Worker implementations without additional layers.
195-222: Indentation adjustments present but logic unchanged.The indentation changes in the CSV row construction are cosmetic and don't alter the logic.
src/components/FFT.tsx (1)
73-220: LGTM: Counter tracking refactored with overflow protection.The refactor from a local variable to
useRefis consistent with the pattern across the codebase. The addition of overflow protection at Lines 218-219 is good defensive programming, preventing potential integer overflow issues during long-running sessions.public/manifest.json (1)
1-26: All referenced assets exist in the public directory.Verification confirms that all three image assets referenced in the manifest (chords-logo-192x192.png, chords-logo-512x512.png, and screenshot-chords.png) are present in the public directory. The PWA manifest configuration is properly supported by the required asset files.
src/components/Connection.tsx (1)
1557-1665: Tooltip wrapping for the connection control looks correctThe
TooltipProvider→Tooltip→TooltipTrigger asChildstructure around the connectionPopover/Buttonchain is consistent with the shadcn/Radix tooltip pattern, and the dynamic label (“Connect/Disconnect Device”) provides clear affordance without impacting the interaction.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
67-67: Updateeslint-config-nextto match Next.js major version.
eslint-config-nextis pinned to version14.2.3, but Next.js is now at^16.0.3. To ensure consistent linting rules and avoid potential breakage, updateeslint-config-nextto match Next.js 16.x.- "eslint-config-next": "14.2.3", + "eslint-config-next": "^16.0.0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)src/app/layout.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/app/layout.tsx
🔇 Additional comments (1)
package.json (1)
45-45: I need one more clarification before finalizing the review. The web search results indicate Next.js 16 "targets React 19," but I need to confirm whether this is a hard requirement or if React 18 is still compatible.Address React version compatibility concern for Next.js 16 upgrade.
Next.js 16 uses the latest React Canary release, which includes React 19.2 features, and the official upgrade guide recommends
npm install next@latest react@latest react-dom@latest. However, Next.js suggests using, at a minimum, [email protected] and [email protected], and official documentation does not explicitly state that React 18 is unsupported in Next.js 16.The concern is that pinning React to 18.3.1 (instead of upgrading to React 19.x) may cause compatibility issues with Next.js 16's new features, particularly those relying on React 19.2 APIs. Additionally, clarify the rationale for exact version pinning without caret (
18.3.1vs^18.3.1), especially since other dependencies use caret versioning.Review the async-only API changes in Next.js 16 to ensure your codebase handles the required
awaitpatterns correctly regardless of React version.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Connection.tsx (1)
361-373: Clarify the canvas count logic.The function uses three different count values which creates confusion:
- The
canvasCountparameter (used for comparison)newCountfromselectedChannels.length(used for parent update)canvasElementCountRef.current(sent to worker)This makes it unclear which value is the source of truth. Additionally, the parameter name
canvasCountshadows the component prop, reducing readability.Consider refactoring to make the relationship between these values explicit:
-const setCanvasCountInWorker = (canvasCount: number) => { +const setCanvasCountInWorker = (currentCount: number) => { if (!workerRef.current) { initializeWorker(); } - // Update parent canvasCount only when it differs to avoid unnecessary rerenders const newCount = selectedChannels.length; - if (typeof setCanvasCount === 'function' && newCount !== canvasCount) { + if (typeof setCanvasCount === 'function' && newCount !== currentCount) { setCanvasCount(newCount); } - // Send canvasCount independently to the worker - workerRef.current?.postMessage({ action: 'setCanvasCount', canvasCount: canvasElementCountRef.current }); + // Send the updated count to the worker + workerRef.current?.postMessage({ action: 'setCanvasCount', canvasCount: newCount }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Connection.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Connection.tsx (2)
282-318: LGTM! Channel toggle with localStorage persistence.The implementation correctly persists channel selections to localStorage within the state updater callback and marks the selection as manual. The try-catch wrapper ensures graceful degradation if localStorage fails.
420-440: LGTM! User feedback for ZIP operations.The addition of toast notifications provides clear user feedback for both success and error cases when creating ZIP files, improving the user experience.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package.json (1)
12-13: Remove duplicate script entry and movedistDirto Next.js config.You have two scripts that execute the identical command:
- Line 7:
"prebuild"runs automatically beforebuild- Line 13:
"generate-manifest"is redundantAlso,
distDir(line 12) is a Next.js configuration property and should not be in thescriptssection. Move it tonext.config.jsornext.config.ts.Apply this diff to fix both issues:
"scripts": { "dev": "next dev", "prebuild": "ts-node scripts/generateManifest.mjs", "build": "next build", "start": "next start", "lint": "next lint", "export": "next export", - "distDir": "out", - "generate-manifest": "ts-node scripts/generateManifest.mjs" },Then ensure your
next.config.jsornext.config.tscontains:/** @type {import('next').NextConfig} */ const nextConfig = { distDir: 'out', // ... other config }; export default nextConfig;
🧹 Nitpick comments (1)
package.json (1)
67-67: Updateeslint-config-nextversion to align with Next.js 16.You upgraded Next.js to
^16.0.3buteslint-config-nextis pinned to14.2.3. This version mismatch may cause ESLint configuration issues or missing rules for the newer Next.js features.Update
eslint-config-nextto match your Next.js version:- "eslint-config-next": "14.2.3", + "eslint-config-next": "^16.0.3",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(2 hunks)
🔇 Additional comments (1)
package.json (1)
45-45: Verify Next.js 16 compatibility and breaking changes.Next.js 16 is a major version upgrade. Per the Next.js 15 release notes, there are breaking changes including async Request APIs (e.g.,
cookies(),headers(),params,searchParamsnow requireawait). Ensure your codebase has been updated to handle these breaking changes.Please verify:
- All dynamic API calls (
cookies(),headers(), etc.) are properly awaited throughout the codebase.- GET route handlers have appropriate caching behavior configured (they no longer default to cached in Next.js 15+).
- All Radix UI dependencies (lines 16–32) are compatible with Next.js 16 and React 18.3.1.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
68-68: Updateeslint-config-nextto match Next.js 16 major version.
eslint-config-nextis pinned at 14.2.3, but Next.js is upgraded to 16.0.3. This major version mismatch will cause ESLint configuration compatibility issues and likely build failures during the lint step.Update
eslint-config-nextto version 16.x to align with the Next.js major version.Apply this diff:
- "eslint-config-next": "14.2.3", + "eslint-config-next": "^16.0.0",
🧹 Nitpick comments (1)
package.json (1)
49-50: Consider using caret ranges for React and React-dom.React and React-dom are pinned to exact versions (18.3.1) rather than caret ranges (e.g.,
^18.3.1). While exact pinning ensures reproducibility, it prevents automatic security updates within the minor/patch range.Evaluate whether reproducibility is prioritized over automatic security patch adoption. If security updates are important, consider using caret ranges to allow patch-level updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(2 hunks)
🔇 Additional comments (2)
package.json (2)
46-46: Verify Next.js 16.0.3 compatibility with the entire codebase.Next.js was upgraded from 15.3.3 to 16.0.3 (a major version jump). While the PR description mentions that build failures were resolved, Next.js 16 is a very recent release and may introduce additional breaking changes beyond those covered by Next.js 15.
Confirm that:
- All route handlers, middleware, and dynamic APIs follow Next.js 16 patterns
- No deprecated APIs from Next.js 15 are being used
- The build passes with
next buildand there are no runtime errorsRefer to the Next.js migration guide for any breaking changes specific to version 16.
7-7: No issues found with the prebuild script change.
generateManifest.mjsis properly implemented as an ES module with no TypeScript-specific syntax. The change fromts-nodetonodeis correct and appropriate for.mjsfiles. The requireddotenvdependency is declared inpackage.json, and the script will execute correctly in normal CI/CD flows wherenpm installprecedes the prebuild step.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/layout.tsx (1)
51-51: Remove redundant manual manifest link tag (previously flagged).This issue was previously identified and marked as addressed in commit 7ad1c24, but the manual
<link rel="manifest">tag is still present at Line 51. In Next.js 15's App Router, themetadata.manifestproperty at Line 23 automatically generates the appropriate link tag, making this manual tag redundant and potentially conflicting.Apply this diff to remove the redundant link:
- <head><link rel="manifest" href={manifestHref} /></head> + <head></head>Or remove the entire
<head>element if it serves no other purpose.
🧹 Nitpick comments (2)
src/components/LandingComp/HeadSection.tsx (1)
105-117: SVG translate transform change is standards‑compliant; base transform is optionalSwitching to
transform="translate(0,0)"aligns with SVG’s 2Dtransformsyntax and matches theanimateTransformtype oftranslate, so this is correct. Sincefrom/toalready drive the motion, the initialtransform="translate(0,0)"is a no‑op and could be removed for slightly cleaner markup if you want, but it’s not required.src/app/layout.tsx (1)
15-15: Simplify manifestHref to use consistent absolute paths.The current ternary produces inconsistent path styles:
- When
basePath = ""(empty string is falsy) →"manifest.json"(relative)- When
basePath = "/Chords-Web"→"/Chords-Web/manifest.json"(absolute)Apply this diff for consistent absolute paths:
-const manifestHref = basePath ? `${basePath}/manifest.json` : "manifest.json"; +const manifestHref = `${basePath}/manifest.json`;This produces:
- When
basePath = ""→"/manifest.json"- When
basePath = "/Chords-Web"→"/Chords-Web/manifest.json"Both are now absolute paths from the root, ensuring consistent resolution behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/deploy.yml(1 hunks)public/manifest.json(1 hunks)scripts/generateManifest.mjs(1 hunks)src/app/layout.tsx(3 hunks)src/components/LandingComp/HeadSection.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/manifest.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/layout.tsx (1)
scripts/generateManifest.mjs (1)
basePath(16-16)
🔇 Additional comments (2)
scripts/generateManifest.mjs (1)
36-43: LGTM! Robust error handling for the build script.The try-catch block with logging and
process.exitCode = 1ensures that manifest generation failures are visible and properly propagate to CI workflows..github/workflows/deploy.yml (1)
80-106: LGTM! Helpful CI diagnostics for troubleshooting.These read-only debug steps provide valuable visibility into the manifest generation and build process, which directly supports the PR's objective to resolve build failures. The
|| truefallbacks prevent diagnostic failures from breaking the build.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/manifest.json (1)
1-13: Consider adding theme_color and background_color for improved UX.The manifest defines the core PWA structure. However, adding
theme_colorandbackground_colorproperties would improve the user experience when the app is installed on a device—these colors will be displayed while the app is loading. This is optional but recommended for PWAs.If desired, add the following properties:
{ "name": "Chords", "short_name": "Chords", "start_url": ".", "display": "standalone", + "theme_color": "#000000", + "background_color": "#ffffff", "icons": [ { "src": "chords-logo-192x192.png", "sizes": "192x192", "type": "image/png" }, { "src": "chords-logo-512x512.png", "sizes": "512x512", "type": "image/png" } ], "screenshots": [ { "src": "screenshot-chords.png", "sizes": "1280x720", "type": "image/png", "form_factor": "wide" } ] }Adjust the color values (
#000000and#ffffff) to match your app's branding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/manifest.json(1 hunks)src/app/layout.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/layout.tsx
🔇 Additional comments (1)
public/manifest.json (1)
6-12: Assets verified—no issues found.All three image files referenced in the manifest (
chords-logo-192x192.png,chords-logo-512x512.png, andscreenshot-chords.png) exist in thepublic/directory. The PWA manifest is properly configured.
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
|
I have updated version of glob to 10.5.0 and eslint-config-next to 16.0.3. fixed #103 |
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/npg-lite/page.tsx (1)
598-611: Fix invalid else syntax at line 607 insrc/app/npg-lite/page.tsx.The invalid syntax
} else (error: any) => {breaks the build. Replace with valid conditional:if (blob) { saveAs(blob, filename); // FileSaver.js toast.success("File downloaded successfully."); - } else (error: any) => { + } else if (error) { console.error("Worker error:", error); - toast.error(`Error during file download: ${error.message}`); + toast.error(`Error during file download: ${String(error)}`); }This restores valid syntax and ensures worker-reported failures reach the UI.
src/components/Connection.tsx (1)
442-454: Fix invalidelsesyntax at line 451—this is a TypeScript compilation error.The code has invalid syntax mixing
elsewith arrow function notation:} else (error: any) => { ... }Change line 451 from:
} else (error: any) => {to:
} else if (error) {This restores valid TypeScript syntax and properly checks for the error condition before attempting to access its message property.
♻️ Duplicate comments (1)
src/components/Connection.tsx (1)
361-378: Worker canvas-count sync can send a stale value; also resolves prior exhaustive-deps concern.Wrapping
setCanvasCountInWorkerinuseCallbacktied toselectedChannelsfixes the earlier stale-closure /exhaustive-depsissue, but the worker message still usescanvasElementCountRef.current, which can lag behindselectedChannels.lengthfor the render where the change occurs.To keep React state, the ref, and the worker in lockstep, consider:
const setCanvasCountInWorker = useCallback((canvasCount: number) => { if (!workerRef.current) { initializeWorker(); } - // Update parent canvasCount only when it differs to avoid unnecessary rerenders - const newCount = selectedChannels.length; - if (typeof setCanvasCount === 'function' && newCount !== canvasCount) { - setCanvasCount(newCount); - } - - // Send canvasCount independently to the worker - workerRef.current?.postMessage({ action: 'setCanvasCount', canvasCount: canvasElementCountRef.current }); + // Derive the new count from the latest selectedChannels + const newCount = selectedChannels.length; + if (typeof setCanvasCount === 'function' && newCount !== canvasCount) { + setCanvasCount(newCount); + } + // Keep the ref and worker in sync with the same value + canvasElementCountRef.current = newCount; + workerRef.current?.postMessage({ action: 'setCanvasCount', canvasCount: newCount }); }, [selectedChannels, setCanvasCount]);This avoids the worker ever seeing an outdated
canvasCountwhile still preventing unnecessary parent re-renders.
🧹 Nitpick comments (1)
src/app/npg-lite/page.tsx (1)
59-59: Sampling-rate–driven point counts and filter params are now consistently derived.Using
samplingrateref.currentto computedpCount,dataPointCountRef.current, grid spacing, and all filter configurations (Notch,EXGFilter,HighPassFilter) keeps the rendered window and DSP behavior tied to a single sampling-rate source. If you ever makesamplingraterefdynamic, you may also want to replace the hard-coded500 * 4innumGridLineswith the same ref for full consistency.Also applies to: 108-111, 145-145, 195-199, 321-322, 345-352
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/muscle-strength/page.tsx(3 hunks)src/app/npg-lite/page.tsx(9 hunks)src/components/Connection.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/npg-lite/page.tsx (1)
src/components/Colors.ts (1)
getLineColor(103-122)
🔇 Additional comments (6)
src/app/muscle-strength/page.tsx (1)
39-39: Sampling-rate ref wiring is consistent and behavior-preserving.Initializing
samplingraterefonce and reusing it fordataPointCountRefand the Notch/EXG filter configuration keeps the sampling-rate–derived values in sync without changing behavior for the current defaults.Also applies to: 647-648, 678-682
src/components/Connection.tsx (3)
282-318: Channel toggle + persistence behavior looks sound.Moving
setManuallySelected(true)after thesetSelectedChannelsupdater and persistingsortedChannelsintosavedDevices(with a guarded try/catch) gives you deterministic channel ordering, prevents an empty selection, and keeps the “Select All / RESET” logic aligned with true manual interaction.
419-433: ZIP export toasts improve user feedback without changing behavior.Adding
toast.successon a successful ZIP download andtoast.erroron worker-reported failures makes the ZIP export path much clearer to end users, while keeping the underlying worker messaging intact.
578-579: Centralizing sampling-rate intosamplingraterefkeeps filter configuration coherent.Hooking
samplingrateref.currentto the board’ssampling_rate(serial path) and the BLE default (500 Hz), then feeding the same ref intoNotch,HighPassFilter, andEXGFilter.setbitsin bothreadDataand the BLE processing path, ensures all filters see a consistent sampling rate instead of scattered literals.Also applies to: 1174-1175, 1306-1313
src/app/npg-lite/page.tsx (2)
791-807: Manual-selection flag is now correctly decoupled from programmatic “Select All”.Moving
setManuallySelected(true)out of thesetSelectedChannelsupdater ensures that only explicit channel toggles mark the selection as manual, so the “Select All / RESET” button logic reflects actual user intent rather than programmatic changes.
1517-1517: Explicittype="button"on time-base controls is a safe clarification.Adding
type="button"avoids accidental form submission semantics and makes these slider-adjacent controls behave purely as UI buttons, which is future-proof even if the layout later ends up inside a<form>.Also applies to: 1536-1536
Ritika8081
had a problem deploying
to
github-pages
GitHub Actions
Failure
|
@Ritika8081 Thank you so much for your contributions. I am merging this now! great work! |
2 participants
@lorforlinux please review my pull request this includes following changes :
Summary by CodeRabbit
New Features
Bug Fixes
Updates