fix: preserve intentional gaps when reordering Run of Show items

[cj-vana]

User description

Previously, moving items in the Run of Show would recalculate ALL start times based on cumulative duration, destroying carefully planned gaps between items (stage changes, intermissions, etc.).

Now implements smart gap-preserving logic:

• Items with positive gaps (intentional buffers) → gap size preserved
• Items with no gaps or overlaps → auto-adjusted to follow previous item
• Items without start times → calculated automatically
• Item numbers stay sequential after reordering

This allows users to maintain their production timing intentions while reordering the show flow.

Fixes issue where reordering would mess up all timings.

🤖 Generated with Claude Code

---

PR Type

Bug fix

---

Description

• Preserve intentional gaps when reordering Run of Show items

• Implement smart gap-preserving logic for timing calculations

• Fix issue where reordering destroyed planned intermissions/breaks

• Maintain sequential item numbering after reordering

---

Diagram Walkthrough

flowchart LR
  A["Original Items"] --> B["Swap Items"]
  B --> C["Calculate Gaps"]
  C --> D["Preserve Positive Gaps"]
  C --> E["Auto-adjust No Gaps"]
  D --> F["Updated Items"]
  E --> F

Loading

File Walkthrough

	Relevant files
Bug fix	RunOfShowEditor.tsx Implement gap-preserving reorder logic                                      apps/web/src/pages/RunOfShowEditor.tsx Replace simple cumulative time calculation with gap-preserving logic Add gap detection between existing and expected start times Preserve positive gaps (intentional buffers) when reordering Auto-adjust items with no gaps or overlaps to follow previous item +36/-8

---

[netlify]

✅ Deploy Preview for sounddocsbeta ready!

Name	Link
🔨 Latest commit	9f687a5
🔍 Latest deploy log	https://app.netlify.com/projects/sounddocsbeta/deploys/68e54b94ec17cf0008df36c6
😎 Deploy Preview	https://deploy-preview-108--sounddocsbeta.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
⚪	No custom compliance provided Follow the guide to enable custom compliance check.
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to c44bafa

Category	Suggestion	Impact
Incremental [*]	Fix numbering off-by-one bug Fix an off-by-one error in item numbering logic. The current implementation starts numbering at "2" instead of "1"; the suggestion corrects this and also ensures headers have a consistent, non-conflicting number. apps/web/src/pages/RunOfShowEditor.tsx [581-668] // Step 3: Recalculate item numbers and start times using stored gaps let itemCount = 1; let cumulativeEndTimeSeconds = firstAnchoredStart; const recalculatedItems = itemsWithGaps.map((item) => { if (item.type === "header") { const existingStartTime = parseTimeToSeconds(item.startTime || ""); let newHeaderStart: string; if (existingStartTime !== null) { - // Advance pointer to header time if it's ahead, clamp if behind if (existingStartTime > cumulativeEndTimeSeconds) { cumulativeEndTimeSeconds = existingStartTime; newHeaderStart = item.startTime!; } else { newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); } } else { newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); } - // Return header without calculatedGap property return { id: item.id, type: item.type, - itemNumber: item.itemNumber, + itemNumber: "", // keep headers unnumbered to avoid conflicts startTime: newHeaderStart, highlightColor: item.highlightColor, headerTitle: item.headerTitle, } as RunOfShowItem; } - // Items - extract gap and other properties const itemWithGap = item as RunOfShowItem & { calculatedGap?: number }; const gap = itemWithGap.calculatedGap || 0; const newStartTime = cumulativeEndTimeSeconds + gap; const durationSeconds = parseDurationToSeconds(item.duration || ""); - itemCount++; + const currentItemNumber = itemCount.toString(); + // advance pointers after assigning the current number cumulativeEndTimeSeconds = newStartTime; if (durationSeconds !== null) { cumulativeEndTimeSeconds += durationSeconds; } + itemCount++; - // Return item without calculatedGap property return { id: item.id, type: item.type, - itemNumber: itemCount.toString(), + itemNumber: currentItemNumber, startTime: formatSecondsToTime(newStartTime), highlightColor: item.highlightColor, preset: item.preset, duration: item.duration, privateNotes: item.privateNotes, productionNotes: item.productionNotes, audio: item.audio, video: item.video, lights: item.lights, - // Include custom column values ...Object.keys(item) .filter( (key) => ![ "id", "type", "itemNumber", "startTime", "highlightColor", "preset", "duration", "privateNotes", "productionNotes", "audio", "video", "lights", "calculatedGap", "headerTitle", ].includes(key), ) .reduce( (acc, key) => { acc[key] = item[key as keyof RunOfShowItem]; return acc; }, {} as Record<string, string | number | boolean | undefined>, ), } as RunOfShowItem; }); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies an off-by-one bug in item re-numbering where itemCount is incremented before use, causing numbering to start at "2". The proposed fix resolves this functional error, which is a significant improvement to correctness.	Medium
	Anchor start using items only Modify the logic for finding the initial start time to only consider item types, not header types. This prevents a header with an old start time from incorrectly shifting the entire schedule backward. apps/web/src/pages/RunOfShowEditor.tsx [572-579] -// Establish initial cumulative time from earliest anchored start (header or item) +// Establish initial cumulative time from earliest anchored start based on items only const firstAnchoredStart = (() => { for (const it of itemsWithGaps) { + if (it.type !== "item") continue; const t = parseTimeToSeconds(it.startTime || ""); if (t !== null) return t; } return 0; })(); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that anchoring the start time to a header can cause unintended time shifts if the header's time is earlier than the first item's. By restricting the anchor search to items only, it ensures the schedule recalculation is based on the flow of actual content, improving the logic's correctness.	Medium
	Guard against NaN and negative gaps Add checks to prevent NaN values during gap calculation. The change ensures that invalid time strings do not propagate NaN values, making the time calculations more robust. apps/web/src/pages/RunOfShowEditor.tsx [522-564] -// Step 1: Calculate gaps BEFORE reordering -// This captures the gap each item has relative to its IMMEDIATE previous item const itemsWithGaps = items.map((item, index) => { let gap = 0; if (item.type === "item") { const itemStartTime = parseTimeToSeconds(item.startTime || ""); - - if (itemStartTime === null) { - // Without a valid start time we cannot compute a reliable gap + if (itemStartTime === null || !Number.isFinite(itemStartTime)) { return { ...item, calculatedGap: 0 }; } - // Walk backwards to find the previous item's end time, skipping headers let prevEndTime: number | null = null; for (let i = index - 1; i >= 0; i--) { const prev = items[i]; if (prev.type !== "item") continue; const prevStart = parseTimeToSeconds(prev.startTime || ""); - const prevDuration = parseDurationToSeconds(prev.duration || ""); + const prevDurationParsed = parseDurationToSeconds(prev.duration || ""); + const prevDurationSafe = + prevDurationParsed !== null && Number.isFinite(prevDurationParsed) + ? prevDurationParsed + : 0; - if (prevStart !== null) { - prevEndTime = prevStart + (prevDuration ?? 0); + if (prevStart !== null && Number.isFinite(prevStart)) { + const end = prevStart + prevDurationSafe; + prevEndTime = Number.isFinite(end) ? end : prevStart; // fallback to start if something goes wrong break; } - - // If no start, but we have duration, keep looking further back - // However, don't add duration without a known anchor start. - if (prevStart === null) continue; } if (prevEndTime !== null) { - gap = itemStartTime - prevEndTime; - if (gap < 0) gap = 0; + const rawGap = itemStartTime - prevEndTime; + gap = Number.isFinite(rawGap) && rawGap > 0 ? rawGap : 0; } else { - // No previous item with a valid start; treat as no gap to preserve gap = 0; } } return { ...item, calculatedGap: gap }; }); Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that the existing code does not handle potential NaN values from time parsing, which could lead to invalid calculations. While the original code already clamps negative gaps, adding Number.isFinite checks improves robustness against malformed time string inputs.	Low
Possible issue	Correct numbering and header anchoring Fix an off-by-one error in item numbering where itemCount was incremented before being assigned. Additionally, correct the logic for recalculating header start times to prevent them from being pulled forward unintentionally. apps/web/src/pages/RunOfShowEditor.tsx [585-668] const recalculatedItems = itemsWithGaps.map((item) => { if (item.type === "header") { const existingStartTime = parseTimeToSeconds(item.startTime || ""); let newHeaderStart: string; - if (existingStartTime !== null) { - // Advance pointer to header time if it's ahead, clamp if behind - if (existingStartTime > cumulativeEndTimeSeconds) { - cumulativeEndTimeSeconds = existingStartTime; - newHeaderStart = item.startTime!; - } else { - newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); - } + if (existingStartTime !== null && existingStartTime > cumulativeEndTimeSeconds) { + // Only advance pointer if header explicitly anchors to a future time + cumulativeEndTimeSeconds = existingStartTime; + newHeaderStart = item.startTime!; } else { + // Do not clamp backwards; keep header aligned to current pointer newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); } - // Return header without calculatedGap property return { id: item.id, type: item.type, - itemNumber: item.itemNumber, + itemNumber: "", // headers should not consume or display item numbers here startTime: newHeaderStart, highlightColor: item.highlightColor, headerTitle: item.headerTitle, } as RunOfShowItem; } // Items - extract gap and other properties const itemWithGap = item as RunOfShowItem & { calculatedGap?: number }; const gap = itemWithGap.calculatedGap || 0; const newStartTime = cumulativeEndTimeSeconds + gap; const durationSeconds = parseDurationToSeconds(item.duration || ""); + // Set number before increment to avoid off-by-one + const currentItemNumber = itemCount.toString(); itemCount++; + cumulativeEndTimeSeconds = newStartTime; if (durationSeconds !== null) { cumulativeEndTimeSeconds += durationSeconds; } - // Return item without calculatedGap property return { id: item.id, type: item.type, - itemNumber: itemCount.toString(), + itemNumber: currentItemNumber, startTime: formatSecondsToTime(newStartTime), highlightColor: item.highlightColor, preset: item.preset, duration: item.duration, privateNotes: item.privateNotes, productionNotes: item.productionNotes, audio: item.audio, video: item.video, lights: item.lights, - // Include custom column values ...Object.keys(item) .filter( (key) => ![ "id", "type", "itemNumber", "startTime", "highlightColor", "preset", "duration", "privateNotes", "productionNotes", "audio", "video", "lights", "calculatedGap", "headerTitle", ].includes(key), ) .reduce( (acc, key) => { acc[key] = item[key as keyof RunOfShowItem]; return acc; }, {} as Record<string, string | number | boolean | undefined>, ), } as RunOfShowItem; }); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies and fixes two distinct bugs: an off-by-one error in item numbering and incorrect logic for header time recalculation that could lead to unexpected time shifts.	Medium
General	Accumulate durations to compute gaps Improve the gap calculation logic to correctly handle chains of items where some intermediate items lack a startTime. The current implementation fails to preserve gaps in this scenario, but the suggested change accumulates durations to find a proper anchor and calculate the correct gap. apps/web/src/pages/RunOfShowEditor.tsx [524-564] const itemsWithGaps = items.map((item, index) => { let gap = 0; if (item.type === "item") { const itemStartTime = parseTimeToSeconds(item.startTime || ""); if (itemStartTime === null) { // Without a valid start time we cannot compute a reliable gap return { ...item, calculatedGap: 0 }; } - // Walk backwards to find the previous item's end time, skipping headers + // Walk backwards to find an anchor start, accumulating durations let prevEndTime: number | null = null; + let accumulatedDurations = 0; + for (let i = index - 1; i >= 0; i--) { const prev = items[i]; if (prev.type !== "item") continue; const prevStart = parseTimeToSeconds(prev.startTime || ""); - const prevDuration = parseDurationToSeconds(prev.duration || ""); + const prevDuration = parseDurationToSeconds(prev.duration || "") ?? 0; if (prevStart !== null) { - prevEndTime = prevStart + (prevDuration ?? 0); + prevEndTime = prevStart + prevDuration + accumulatedDurations; break; + } else { + // No start; accumulate duration and continue searching for an earlier anchor + accumulatedDurations += prevDuration; + continue; } - - // If no start, but we have duration, keep looking further back - // However, don't add duration without a known anchor start. - if (prevStart === null) continue; } if (prevEndTime !== null) { gap = itemStartTime - prevEndTime; if (gap < 0) gap = 0; } else { - // No previous item with a valid start; treat as no gap to preserve gap = 0; } } return { ...item, calculatedGap: gap }; }); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a flaw in the gap calculation logic where gaps could be lost if an intermediate item lacks a start time. The proposed fix improves the robustness of the gap preservation feature.	Medium
Update

---

Previous suggestions

✅ Suggestions up to commit 256df11

Category	Suggestion	Impact
Incremental [*]	✅ Correct gap computation with headers Suggestion Impact: The commit implements the suggested backward iteration skipping non-item headers, adds an early return when the current item's start time is null, computes prevEndTime using prevStart + duration, and sets gap based on that with non-negative clamp. code diff: + if (itemStartTime === null) { + // Without a valid start time we cannot compute a reliable gap + return { ...item, calculatedGap: 0 }; + } + + // Walk backwards to find the previous item's end time, skipping headers + let prevEndTime: number | null = null; + for (let i = index - 1; i >= 0; i--) { + const prev = items[i]; + if (prev.type !== "item") continue; + + const prevStart = parseTimeToSeconds(prev.startTime || ""); + const prevDuration = parseDurationToSeconds(prev.duration || ""); + + if (prevStart !== null) { + prevEndTime = prevStart + (prevDuration ?? 0); + break; } - // Gap is the difference between this item's start and when it "should" start + // If no start, but we have duration, keep looking further back + // However, don't add duration without a known anchor start. + if (prevStart === null) continue; + } + + if (prevEndTime !== null) { gap = itemStartTime - prevEndTime; - // Only preserve positive gaps (intentional buffers) if (gap < 0) gap = 0; + } else { + // No previous item with a valid start; treat as no gap to preserve + gap = 0; } Refactor the gap calculation logic to correctly and efficiently find the previous item's end time by iterating backward and skipping headers. This fixes a bug where gaps were miscalculated. apps/web/src/pages/RunOfShowEditor.tsx [524-558] const itemsWithGaps = items.map((item, index) => { let gap = 0; if (item.type === "item") { const itemStartTime = parseTimeToSeconds(item.startTime || ""); + if (itemStartTime === null) { + // Without a valid start time we cannot compute a reliable gap + return { ...item, calculatedGap: 0 }; + } - if (itemStartTime !== null && index > 0) { - // Calculate cumulative end time of all previous items - let prevEndTime = 0; - for (let i = 0; i < index; i++) { - const prevItem = items[i]; - if (prevItem.type === "item") { - const prevStart = parseTimeToSeconds(prevItem.startTime || ""); - const prevDuration = parseDurationToSeconds(prevItem.duration || ""); + // Walk backwards to find the previous item's end time, skipping headers + let prevEndTime: number | null = null; + for (let i = index - 1; i >= 0; i--) { + const prev = items[i]; + if (prev.type !== "item") continue; - if (prevStart !== null) { - prevEndTime = prevStart; - if (prevDuration !== null) { - prevEndTime += prevDuration; - } - } else if (prevDuration !== null) { - prevEndTime += prevDuration; - } - } + const prevStart = parseTimeToSeconds(prev.startTime || ""); + const prevDuration = parseDurationToSeconds(prev.duration || ""); + + if (prevStart !== null) { + prevEndTime = prevStart + (prevDuration ?? 0); + break; } - // Gap is the difference between this item's start and when it "should" start + // If no start, but we have duration, keep looking further back + // However, don't add duration without a known anchor start. + if (prevStart === null) continue; + } + + if (prevEndTime !== null) { gap = itemStartTime - prevEndTime; - // Only preserve positive gaps (intentional buffers) if (gap < 0) gap = 0; + } else { + // No previous item with a valid start; treat as no gap to preserve + gap = 0; } } return { ...item, calculatedGap: gap }; }); [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant logical flaw and inefficiency in the PR's gap calculation logic and provides a robust, correct, and more efficient implementation.	High
Possible issue	✅ Preserve absolute schedule offset Suggestion Impact: The commit added computation of firstAnchoredStart from the first valid start time and set cumulativeEndTimeSeconds to that value, preserving schedule offset during recalculation. It also adjusted header handling accordingly. code diff: + // Establish initial cumulative time from earliest anchored start (header or item) + const firstAnchoredStart = (() => { + for (const it of itemsWithGaps) { + const t = parseTimeToSeconds(it.startTime || ""); + if (t !== null) return t; + } + return 0; + })(); + // Step 3: Recalculate item numbers and start times using stored gaps let itemCount = 1; - let cumulativeEndTimeSeconds = 0; + let cumulativeEndTimeSeconds = firstAnchoredStart; Initialize cumulativeEndTimeSeconds based on the first item's start time instead of hardcoding it to zero. This preserves any absolute schedule offset when recalculating times after moving an item. apps/web/src/pages/RunOfShowEditor.tsx [560-608] // Step 2: Swap items [itemsWithGaps[currentIndex], itemsWithGaps[targetIndex]] = [ itemsWithGaps[targetIndex], itemsWithGaps[currentIndex], ]; +// Establish initial cumulative time from earliest anchored start (header or item) +const firstAnchoredStart = (() => { + for (const it of itemsWithGaps) { + const t = parseTimeToSeconds(it.startTime || ""); + if (t !== null) return t; + } + return 0; +})(); + // Step 3: Recalculate item numbers and start times using stored gaps let itemCount = 1; -let cumulativeEndTimeSeconds = 0; +let cumulativeEndTimeSeconds = firstAnchoredStart; const recalculatedItems = itemsWithGaps.map((item) => { - if (item.type === "item") { - // Update item number - item.itemNumber = itemCount.toString(); - itemCount++; - - // Use the pre-calculated gap from before the move - const gap = item.calculatedGap || 0; - const newStartTime = cumulativeEndTimeSeconds + gap; - - item.startTime = formatSecondsToTime(newStartTime); - cumulativeEndTimeSeconds = newStartTime; - - // Add this item's duration to cumulative end time - const durationSeconds = parseDurationToSeconds(item.duration || ""); - if (durationSeconds !== null) { - cumulativeEndTimeSeconds += durationSeconds; - } - } else if (item.type === "header") { - // Headers use current cumulative time but don't affect it + if (item.type === "header") { const existingStartTime = parseTimeToSeconds(item.startTime || ""); - if (existingStartTime !== null) { - // Header HAS a start time - ensure it's not before the cumulative time - if (existingStartTime < cumulativeEndTimeSeconds) { + // Advance pointer to header time if it's ahead, clamp if behind + if (existingStartTime > cumulativeEndTimeSeconds) { + cumulativeEndTimeSeconds = existingStartTime; + } else { item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); } - // Otherwise, preserve its original time, as it's either on time or creates a gap. } else { - // Header has NO start time - set it to current cumulative time item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); } + // Headers do not change cumulativeEndTimeSeconds beyond ensuring alignment + // Move on without incrementing itemCount + const { calculatedGap, ...withoutGap } = item as any; + return withoutGap as RunOfShowItem; } - // Remove the temporary calculatedGap property - const { calculatedGap, ...itemWithoutGap } = item; - void calculatedGap; // Mark as intentionally unused - return itemWithoutGap as RunOfShowItem; + // Items + item.itemNumber = itemCount.toString(); + itemCount++; + + const gap = item.calculatedGap || 0; + const newStartTime = cumulativeEndTimeSeconds + gap; + item.startTime = formatSecondsToTime(newStartTime); + cumulativeEndTimeSeconds = newStartTime; + + const durationSeconds = parseDurationToSeconds(item.duration || ""); + if (durationSeconds !== null) { + cumulativeEndTimeSeconds += durationSeconds; + } + + const { calculatedGap, ...withoutGap } = item as any; + return withoutGap as RunOfShowItem; }); [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly points out that hardcoding cumulativeEndTimeSeconds to 0 will break schedules that don't start at time zero. The proposed fix to initialize this value from the first item's start time is a critical correction for the feature to work as intended.	High
General	✅ Avoid in-place mutation during mapping Suggestion Impact: The commit stops in-place mutation inside the map by constructing new objects for both headers and items, spreading existing properties and returning copies without the temporary calculatedGap field. code diff: + if (item.type === "header") { + const existingStartTime = parseTimeToSeconds(item.startTime || ""); + let newHeaderStart: string; + + if (existingStartTime !== null) { + // Advance pointer to header time if it's ahead, clamp if behind + if (existingStartTime > cumulativeEndTimeSeconds) { + cumulativeEndTimeSeconds = existingStartTime; + newHeaderStart = item.startTime!; + } else { + newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); + } + } else { + newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); } - } else if (item.type === "header") { - // Headers use current cumulative time but don't affect it - const existingStartTime = parseTimeToSeconds(item.startTime || ""); - - if (existingStartTime !== null) { - // Header HAS a start time - ensure it's not before the cumulative time - if (existingStartTime < cumulativeEndTimeSeconds) { - item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); - } - // Otherwise, preserve its original time, as it's either on time or creates a gap. - } else { - // Header has NO start time - set it to current cumulative time - item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); - } + + const updatedHeader = { ...item, startTime: newHeaderStart } as RunOfShowItem; + const { calculatedGap, ...itemWithoutGap } = updatedHeader as any; + return itemWithoutGap as RunOfShowItem; } - // Remove the temporary calculatedGap property - const { calculatedGap, ...itemWithoutGap } = item; - void calculatedGap; // Mark as intentionally unused + // Items + const gap = (item as any).calculatedGap || 0; + const newStartTime = cumulativeEndTimeSeconds + gap; + const durationSeconds = parseDurationToSeconds(item.duration || ""); + + const updatedItem = { + ...item, + itemNumber: itemCount.toString(), + startTime: formatSecondsToTime(newStartTime), + } as RunOfShowItem; + + itemCount++; + cumulativeEndTimeSeconds = newStartTime; + if (durationSeconds !== null) { + cumulativeEndTimeSeconds += durationSeconds; + } + + const { calculatedGap, ...itemWithoutGap } = updatedItem as any; return itemWithoutGap as RunOfShowItem; Avoid direct mutation of items within the .map() function during time recalculation. Instead, create new item objects with updated properties to ensure immutability and prevent potential React state issues. apps/web/src/pages/RunOfShowEditor.tsx [566-602] // Step 3: Recalculate item numbers and start times using stored gaps let itemCount = 1; let cumulativeEndTimeSeconds = 0; const recalculatedItems = itemsWithGaps.map((item) => { if (item.type === "item") { - // Update item number - item.itemNumber = itemCount.toString(); + const gap = (item as any).calculatedGap || 0; + const newStartTime = cumulativeEndTimeSeconds + gap; + const durationSeconds = parseDurationToSeconds(item.duration || ""); + + const updatedItem = { + ...item, + itemNumber: itemCount.toString(), + startTime: formatSecondsToTime(newStartTime), + } as RunOfShowItem; + itemCount++; - - // Use the pre-calculated gap from before the move - const gap = item.calculatedGap || 0; - const newStartTime = cumulativeEndTimeSeconds + gap; - - item.startTime = formatSecondsToTime(newStartTime); cumulativeEndTimeSeconds = newStartTime; - - // Add this item's duration to cumulative end time - const durationSeconds = parseDurationToSeconds(item.duration || ""); if (durationSeconds !== null) { cumulativeEndTimeSeconds += durationSeconds; } - } else if (item.type === "header") { - // Headers use current cumulative time but don't affect it + + const { calculatedGap, ...itemWithoutGap } = updatedItem as any; + return itemWithoutGap as RunOfShowItem; + } else { const existingStartTime = parseTimeToSeconds(item.startTime || ""); + let newHeaderStart: string; if (existingStartTime !== null) { - // Header HAS a start time - ensure it's not before the cumulative time if (existingStartTime < cumulativeEndTimeSeconds) { - item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); + newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); + } else { + newHeaderStart = item.startTime!; } - // Otherwise, preserve its original time, as it's either on time or creates a gap. } else { - // Header has NO start time - set it to current cumulative time - item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); + newHeaderStart = formatSecondsToTime(cumulativeEndTimeSeconds); } + + const updatedHeader = { ...item, startTime: newHeaderStart } as RunOfShowItem; + const { calculatedGap, ...itemWithoutGap } = updatedHeader as any; + return itemWithoutGap as RunOfShowItem; } +}); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies direct mutation of state-derived objects within a .map() call, which violates React's immutability principles. Adopting the suggested pattern of creating new objects is a best practice that prevents potential state-related bugs.	Medium

✅ Suggestions up to commit e6315c7

Category	Suggestion	Impact
High-level	The reordering logic is flawed The current gap preservation logic is flawed because it compares an item's old absolute start time with the end time of its new preceding item, resulting in incorrect timing calculations. The logic should instead calculate gaps relative to the previous item before reordering and use that stored gap value to set the new start time. Examples: apps/web/src/pages/RunOfShowEditor.tsx [536-556] const existingStartTime = parseTimeToSeconds(item.startTime || ""); const expectedStartTime = cumulativeEndTimeSeconds; if (existingStartTime !== null) { // Item HAS a start time - calculate gap const gap = existingStartTime - expectedStartTime; if (gap > 0) { // Positive gap (intentional buffer/break) - preserve it item.startTime = formatSecondsToTime(expectedStartTime + gap); ... (clipped 11 lines) Solution Walkthrough: Before: function handleMoveItem(itemId, direction) { // ... swap items in the array ... let cumulativeEndTimeSeconds = 0; recalculatedItems = items.map(item => { // This is the item's start time from its OLD position const existingStartTime = parseTimeToSeconds(item.startTime); // This is the end time of the NEW previous item const expectedStartTime = cumulativeEndTimeSeconds; // The gap is calculated incorrectly using values from different contexts const gap = existingStartTime - expectedStartTime; if (gap > 0) { // Preserves an incorrect gap cumulativeEndTimeSeconds = expectedStartTime + gap; } else { cumulativeEndTimeSeconds = expectedStartTime; } // ... update item start time and cumulative end time ... }); } After: function handleMoveItem(itemId, direction) { // 1. Calculate and store gaps BEFORE swapping const itemsWithGaps = calculateGaps(runOfShow.items); const itemToMove = itemsWithGaps.find(i => i.id === itemId); // 2. Swap items in the array // ... swap logic ... // 3. Recalculate times using the stored, stable gap values let cumulativeEndTimeSeconds = 0; recalculatedItems = newOrder.map(item => { // Use the pre-calculated gap relative to its original previous item const gap = item.gapBefore || 0; const newStartTime = cumulativeEndTimeSeconds + gap; item.startTime = formatSecondsToTime(newStartTime); const duration = parseDurationToSeconds(item.duration); cumulativeEndTimeSeconds = newStartTime + duration; return item; }); } Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical flaw in the core logic of the PR, where using an item's absolute startTime after reordering leads to incorrect gap calculations and broken timings.	High
Possible issue	✅ Adjust header start time after reordering Suggestion Impact: The commit added logic to check a header's existing start time against the cumulative end time and update it if earlier, matching the suggested behavior. code diff: @@ -565,14 +590,21 @@ const existingStartTime = parseTimeToSeconds(item.startTime || ""); if (existingStartTime !== null) { - // Header HAS a start time - preserve it as-is - // Don't update cumulative time + // Header HAS a start time - ensure it's not before the cumulative time + if (existingStartTime < cumulativeEndTimeSeconds) { + item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); + } + // Otherwise, preserve its original time, as it's either on time or creates a gap. } else { // Header has NO start time - set it to current cumulative time item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); } When reordering, ensure a header's existing start time is not earlier than the cumulative end time of the preceding item. If it is, update the header's start time to match the cumulative end time. apps/web/src/pages/RunOfShowEditor.tsx [567-573] if (existingStartTime !== null) { - // Header HAS a start time - preserve it as-is - // Don't update cumulative time + // Header HAS a start time - ensure it's not before the cumulative time + if (existingStartTime < cumulativeEndTimeSeconds) { + item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); + } + // Otherwise, preserve its original time, as it's either on time or creates a gap. } else { // Header has NO start time - set it to current cumulative time item.startTime = formatSecondsToTime(cumulativeEndTimeSeconds); } [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an edge case where a header's preserved start time could become invalid after reordering, and proposes a valid fix to ensure it's at least as late as the previous item's end time. This improves the robustness of the time calculation logic.	Medium