Minor changes - Redesign Gallery UI and Add Persistent History#6
Minor changes - Redesign Gallery UI and Add Persistent History#6FlemingJohn wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive documentation (CONTRIBUTING.md, updated README), adds error handling and 404 page components, implements a client-side upload history feature with localStorage persistence, refines CSS styling with custom scrollbar support, and improves TypeScript types in shader components while removing the disableAnimation prop. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Comment |
|
@FlemingJohn "developed with love" footer is overlapping? above |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
components/Dither.tsx (3)
202-205: Fix ref typing to avoid nullability/type errors in TS.
useRef<THREE.Mesh>(null)will fail understrictNullChecks.Proposed fix
- const mesh = useRef<THREE.Mesh>(null); + const mesh = useRef<THREE.Mesh | null>(null);
180-201: HonordisableAnimationto prevent "does nothing" prop behavior.The
disableAnimationprop is accepted by bothDitherPropsandDitheredWavesProps, passed from parent to child, and used by external code (e.g.,app/page.tsx:162), but theDitheredWavesfunction does not destructure it and theuseFramehook unconditionally setsu.time.value = clock.getElapsedTime()with no check.Implement Option A from the original suggestion: destructure
disableAnimationand track time manually so the prop actually controls animation state without phase jumps when toggled:Proposed fix
function DitheredWaves({ waveSpeed, waveFrequency, waveAmplitude, waveColor, colorNum, pixelSize, + disableAnimation, enableMouseInteraction, mouseRadius }: DitheredWavesProps) { const mesh = useRef<THREE.Mesh>(null); const mouseRef = useRef(new THREE.Vector2()); + const timeRef = useRef(0); const { viewport, size, gl } = useThree(); useFrame(({ clock }) => { const u = waveUniformsRef.current; - u.time.value = clock.getElapsedTime(); + // Freeze animation when disableAnimation=true, without jumping phases on re-enable. + if (!disableAnimation) { + timeRef.current += clock.getDelta(); + } + u.time.value = timeRef.current;
218-227: Use primitive values (size.width,size.height) instead of thesizeobject in the dependency array.The
sizeobject fromuseThree()is a reactive reference that may change between renders. Including the wholesizeobject in the dependency array can cause unnecessary effect reruns. Extract the primitive values to ensure the effect only runs when the actual width or height values change.Suggested fix
- }, [size, gl]); + }, [size.width, size.height, gl]);components/Dither.jsx (2)
225-230: Remove one Y-axis inversion to fix mirrored mouse interaction.The JavaScript code inverts Y at line 229:
(rect.height - (e.clientY - rect.top)), and the fragment shader inverts it again at line 89:vec2(1.0, -1.0). This double inversion causes the interaction to be vertically mirrored—when the user moves the mouse down, the effect appears at the top. Remove the shader's Y inversion by changing line 89 tovec2(1.0, 1.0).
203-223:disableAnimationis accepted but has no effect.
Either honor it by freezing time when true, or remove the prop to avoid misleading the API surface.Also applies to: 261-289
🤖 Fix all issues with AI agents
In @app/page.tsx:
- Line 419: Trailing whitespace exists in the closing JSX tag `</div >`; remove
the extra space so the tag is `</div>` and scan nearby JSX for any other closing
tags with a space before `>` (e.g., other `</tag >`) and fix them similarly to
eliminate the formatting artifact.
- Line 376: Remove the stray space inside the JSX tag by changing the opening
tag from "< Toaster" to "<Toaster" in the component render (look for the Toaster
usage in page.tsx); ensure any corresponding closing tag or self-closing syntax
remains unchanged and run formatter to confirm spacing is fixed.
- Around line 137-140: Wrap navigator.clipboard.writeText in a try/catch in
copyHistoryLink (and similarly update copyToClipboard) to handle failures: call
await navigator.clipboard.writeText(url) inside try, show toast.success on
success, and in catch log the error and show toast.error with a helpful message
(e.g., "Failed to copy link") so permission or insecure-context failures surface
to the user; include the caught error when logging for diagnostics.
- Around line 82-92: uploadFile currently reads the captured history variable
and computes updatedHistory directly, which can be stale; change setHistory to
use the functional updater (setHistory(prev => { const updated =
[newHistoryItem, ...prev.filter(item => item.id !== metadata.key)].slice(0,50);
localStorage.setItem('podu_history', JSON.stringify(updated)); return updated;
})) so the update always uses the latest state and ensures localStorage is
updated consistently; reference the uploadFile function, the newHistoryItem
construction, setHistory call and 'podu_history' localStorage key when making
this change.
In @README.md:
- Around line 7-8: The README references an image at public/preview.png that is
missing; either add the actual preview image to that path (create a
public/preview.png file with the intended screenshot/preview) or remove the
markdown image reference "" from
README.md so the broken link is gone; ensure the filename and path exactly match
public/preview.png if you add the file.
🧹 Nitpick comments (7)
CONTRIBUTING.md (1)
1-42: Solid contributor guide; consider linking concrete templates/policies.This reads well. If the repo has issue templates / a CODE_OF_CONDUCT.md, linking them here would make the guidance actionable.
README.md (1)
39-49: Nice setup docs; add an explicit “never commit.env.local” note.
Right now it implies secrets, but doesn’t clearly warn contributors.Also applies to: 60-63
app/globals.css (1)
118-146: Scrollbar styling is webkit-only; consider adding Firefox support (optional).
This will be a no-op on Firefox unless you also setscrollbar-width/scrollbar-color.Proposed addition
.custom-scrollbar::-webkit-scrollbar { width: 6px; } @@ .custom-scrollbar::-webkit-scrollbar-thumb:hover { background: rgba(255, 255, 255, 0.2); } + +/* Firefox */ +.custom-scrollbar { + scrollbar-width: thin; + scrollbar-color: rgba(255, 255, 255, 0.1) transparent; +}app/error.tsx (3)
18-21: Consider integrating an actual error reporting service.The comment mentions logging to an error reporting service, but currently only uses
console.error. For production, consider integrating with a service like Sentry, LogRocket, or a custom error tracking endpoint.
39-50: Navigation approach and handler simplification.Two minor observations:
- Line 40: The callback
() => reset()can be simplified to justreset.- Line 46: Using
window.location.hrefcauses a full page reload. This may be intentional to fully reset application state after an error, but if you want SPA navigation, consider using Next.jsuseRouteror aLinkcomponent.♻️ Suggested simplification
<button - onClick={() => reset()} + onClick={reset} className="flex-1 px-8 py-4 bg-white text-black font-bold rounded-2xl hover:bg-white/90 transition-all duration-200" >
53-58: Avoid exposing raw error messages in development UI.While this block is dev-only,
error.messagecould potentially contain sensitive information (file paths, stack traces, internal details). Consider truncating or sanitizing the message, or displayingerror.stackseparately for more detailed debugging.app/page.tsx (1)
142-151: Remove or gate test helperloadMockHistory.This function appears to be a testing utility but is defined in production code and never invoked in the UI. Consider:
- Removing it entirely if no longer needed
- Moving it to a separate test utility file
- Gating it behind
process.env.NODE_ENV === 'development'if needed for local testing
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
CONTRIBUTING.mdREADME.mdapp/error.tsxapp/globals.cssapp/not-found.tsxapp/page.tsxcomponents/Dither.jsxcomponents/Dither.tsx
🔇 Additional comments (3)
components/Dither.tsx (1)
134-176: Uniform typing +wrapEffectwrapper looks good.
The stricterTHREE.Uniform<number>forRetroEffectImplmatches the actual uniforms used.app/not-found.tsx (1)
1-39: LGTM! Clean 404 page implementation.The component correctly uses the SSR import path for Phosphor icons and leverages Next.js's server component model appropriately for a static 404 page. The UI is well-structured with consistent styling.
app/page.tsx (1)
301-362: Well-implemented history gallery UI.The Recent Uploads section is nicely implemented with:
- Conditional rendering based on history state
- Smooth animations using Tailwind's animate-in utilities
- Proper accessibility with title attributes on buttons
- Good UX with clear visual hierarchy
One minor suggestion: consider adding
aria-labelto the icon-only buttons for better screen reader support.
|
|
||
| // Update history | ||
| const newHistoryItem: HistoryItem = { | ||
| id: metadata.key, | ||
| url: metadata.url, | ||
| timestamp: Date.now(), | ||
| }; | ||
|
|
||
| const updatedHistory = [newHistoryItem, ...history.filter(item => item.id !== metadata.key)].slice(0, 50); | ||
| setHistory(updatedHistory); | ||
| localStorage.setItem('podu_history', JSON.stringify(updatedHistory)); |
There was a problem hiding this comment.
Stale closure issue with history state.
The uploadFile function captures the history state at the time of its creation. If uploadFile is called while a previous update is pending or after multiple renders, history may be stale, leading to lost history items.
Use the functional update pattern for setHistory to ensure you're working with the latest state.
🐛 Proposed fix using functional update
- const updatedHistory = [newHistoryItem, ...history.filter(item => item.id !== metadata.key)].slice(0, 50);
- setHistory(updatedHistory);
- localStorage.setItem('podu_history', JSON.stringify(updatedHistory));
+ setHistory(prevHistory => {
+ const updatedHistory = [newHistoryItem, ...prevHistory.filter(item => item.id !== metadata.key)].slice(0, 50);
+ localStorage.setItem('podu_history', JSON.stringify(updatedHistory));
+ return updatedHistory;
+ });📝 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.
| // Update history | |
| const newHistoryItem: HistoryItem = { | |
| id: metadata.key, | |
| url: metadata.url, | |
| timestamp: Date.now(), | |
| }; | |
| const updatedHistory = [newHistoryItem, ...history.filter(item => item.id !== metadata.key)].slice(0, 50); | |
| setHistory(updatedHistory); | |
| localStorage.setItem('podu_history', JSON.stringify(updatedHistory)); | |
| // Update history | |
| const newHistoryItem: HistoryItem = { | |
| id: metadata.key, | |
| url: metadata.url, | |
| timestamp: Date.now(), | |
| }; | |
| setHistory(prevHistory => { | |
| const updatedHistory = [newHistoryItem, ...prevHistory.filter(item => item.id !== metadata.key)].slice(0, 50); | |
| localStorage.setItem('podu_history', JSON.stringify(updatedHistory)); | |
| return updatedHistory; | |
| }); |
🤖 Prompt for AI Agents
In @app/page.tsx around lines 82 - 92, uploadFile currently reads the captured
history variable and computes updatedHistory directly, which can be stale;
change setHistory to use the functional updater (setHistory(prev => { const
updated = [newHistoryItem, ...prev.filter(item => item.id !==
metadata.key)].slice(0,50); localStorage.setItem('podu_history',
JSON.stringify(updated)); return updated; })) so the update always uses the
latest state and ensures localStorage is updated consistently; reference the
uploadFile function, the newHistoryItem construction, setHistory call and
'podu_history' localStorage key when making this change.
| const copyHistoryLink = async (url: string) => { | ||
| await navigator.clipboard.writeText(url); | ||
| toast.success('Link copied to clipboard'); | ||
| }; |
There was a problem hiding this comment.
Add error handling for clipboard API.
navigator.clipboard.writeText can fail (e.g., due to permissions or non-secure context). The similar copyToClipboard function at line 118 also lacks error handling. Consider wrapping in try/catch and showing an error toast on failure.
🐛 Proposed fix
const copyHistoryLink = async (url: string) => {
- await navigator.clipboard.writeText(url);
- toast.success('Link copied to clipboard');
+ try {
+ await navigator.clipboard.writeText(url);
+ toast.success('Link copied to clipboard');
+ } catch (err) {
+ toast.error('Failed to copy link');
+ }
};📝 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 copyHistoryLink = async (url: string) => { | |
| await navigator.clipboard.writeText(url); | |
| toast.success('Link copied to clipboard'); | |
| }; | |
| const copyHistoryLink = async (url: string) => { | |
| try { | |
| await navigator.clipboard.writeText(url); | |
| toast.success('Link copied to clipboard'); | |
| } catch (err) { | |
| toast.error('Failed to copy link'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In @app/page.tsx around lines 137 - 140, Wrap navigator.clipboard.writeText in a
try/catch in copyHistoryLink (and similarly update copyToClipboard) to handle
failures: call await navigator.clipboard.writeText(url) inside try, show
toast.success on success, and in catch log the error and show toast.error with a
helpful message (e.g., "Failed to copy link") so permission or insecure-context
failures surface to the user; include the caught error when logging for
diagnostics.
|
|
||
| {/* Toast Notifications with Frosted Glass Styling */} | ||
| <Toaster | ||
| < Toaster |
There was a problem hiding this comment.
Fix whitespace in JSX tag.
There's an extra space in < Toaster which, while valid JSX, appears to be a formatting artifact.
✏️ Suggested fix
- < Toaster
+ <Toaster📝 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.
| < Toaster | |
| <Toaster |
🤖 Prompt for AI Agents
In @app/page.tsx at line 376, Remove the stray space inside the JSX tag by
changing the opening tag from "< Toaster" to "<Toaster" in the component render
(look for the Toaster usage in page.tsx); ensure any corresponding closing tag
or self-closing syntax remains unchanged and run formatter to confirm spacing is
fixed.
| } | ||
| /> | ||
| </div> | ||
| </div > |
There was a problem hiding this comment.
Fix trailing whitespace in closing tag.
Similar formatting artifact with </div >.
✏️ Suggested fix
- </div >
+ </div>📝 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.
| </div > | |
| </div> |
🤖 Prompt for AI Agents
In @app/page.tsx at line 419, Trailing whitespace exists in the closing JSX tag
`</div >`; remove the extra space so the tag is `</div>` and scan nearby JSX for
any other closing tags with a space before `>` (e.g., other `</tag >`) and fix
them similarly to eliminate the formatting artifact.
|  | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
test -f public/preview.png && echo "OK: public/preview.png exists" || echo "MISSING: public/preview.png"Repository: FOSSUChennai/podu.pics
Length of output: 93
Add the missing preview image to public/preview.png or remove the reference from the README.
The image referenced at line 7 is not present in the repository, causing a broken media link on GitHub.
🤖 Prompt for AI Agents
In @README.md around lines 7 - 8, The README references an image at
public/preview.png that is missing; either add the actual preview image to that
path (create a public/preview.png file with the intended screenshot/preview) or
remove the markdown image reference ""
from README.md so the broken link is gone; ensure the filename and path exactly
match public/preview.png if you add the file.
okay i will fix it |
Hey, still waiting for this. lmk if you can work on it. |
2 participants
Symmetric Layout: Side-by-side view for upload and history with perfectly matched heights (450px).
History Gallery: LocalStorage persistence, infinite scroll, and a custom frosted glass scrollbar.
Error Boundary Added global error/404 pages and fixed background scrolling issues.
Summary by CodeRabbit
Documentation
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.