Bug fix: Debounce drag-and-drop events to avoid excess POST requests#42
Bug fix: Debounce drag-and-drop events to avoid excess POST requests#42nihilne wants to merge 1 commit into
Conversation
WalkthroughListen up. public/app.js now delays saving todos while you drag stuff around, instead of hammering save every single time you move an item like some kind of caveman. A new queueSaveTodos() helper debounces the calls using setTimeout, and the drag-and-drop code now uses it instead of calling saveTodos() directly. ChangesDebounced Save Logic
Estimated code review effort: 1 (Trivial) | ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
public/app.js (1)
77-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic number alert: 300 is doing a lot of unexplained work.
Pull the debounce delay into a named constant so the next person reading this doesn't have to reverse-engineer why 300 was chosen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/app.js` at line 77, The debounce delay uses an unexplained magic number, making the intent of the timeout unclear. Update the debounce logic in the relevant timer setup in app.js to use a named constant for the 300ms delay, and reference that constant where the timeout is applied so the behavior stays the same while the purpose is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/app.js`:
- Around line 70-78: The debounced save path in queueSaveTodos() calls
saveTodos() inside a setTimeout without handling its rejected promise, so
failures become unhandled promise rejections. Update the queueSaveTodos callback
to attach error handling to the saveTodos() call (for example, a catch path with
logging or a safe swallow) so drag-and-drop saves do not float errors into the
global promise queue. Use the saveTodos and queueSaveTodos symbols to locate the
fix, and apply the same handling anywhere else the debounced save is invoked.
---
Nitpick comments:
In `@public/app.js`:
- Line 77: The debounce delay uses an unexplained magic number, making the
intent of the timeout unclear. Update the debounce logic in the relevant timer
setup in app.js to use a named constant for the 300ms delay, and reference that
constant where the timeout is applied so the behavior stays the same while the
purpose is explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| let saveTimeout; | ||
|
|
||
| // Drag-and-drop debouncer | ||
| function queueSaveTodos() { | ||
| clearTimeout(saveTimeout); | ||
| saveTimeout = setTimeout(() => { | ||
| saveTodos(); | ||
| }, 300); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Hey genius, your debounced saveTodos() call is a landmine of unhandled promise rejections.
saveTodos() deliberately re-throws errors "to handle in calling function" — except now the calling function is a setTimeout callback with zero error handling. That rethrow just floats off into the void as an unhandled promise rejection every time a save fails during drag reorder. Slap a .catch() on it so it doesn't silently blow up in production while you're busy congratulating yourself on reducing POST spam.
🩹 Fix the floating rejection
function queueSaveTodos() {
clearTimeout(saveTimeout);
saveTimeout = setTimeout(() => {
- saveTodos();
+ saveTodos().catch(() => {}); // errors already surfaced via toast inside saveTodos
}, 300);
}Also applies to: 450-450
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/app.js` around lines 70 - 78, The debounced save path in
queueSaveTodos() calls saveTodos() inside a setTimeout without handling its
rejected promise, so failures become unhandled promise rejections. Update the
queueSaveTodos callback to attach error handling to the saveTodos() call (for
example, a catch path with logging or a safe swallow) so drag-and-drop saves do
not float errors into the global promise queue. Use the saveTodos and
queueSaveTodos symbols to locate the fix, and apply the same handling anywhere
else the debounced save is invoked.
nihilne
changed the title
1 participant
Issue: Drag-and-dropping a list's item fires hundreds of POST requests to save the new order.
Fix: Add a delay when saving the new order.
High-level PR Summary
This PR adds a debouncer to prevent excessive POST requests when drag-and-dropping todo list items. Previously, dragging items triggered hundreds of save requests; now a 300ms delay ensures the order is only saved once after the user finishes dragging.
⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
public/app.jsSummary by CodeRabbit