ical.js (2)

708-708: Consider extracting regex patterns as constants to improve maintainability.

The UNTIL matching regex /UNTIL=(\d{8}T\d{6}Z?)/ (line 708) and replacement regex /UNTIL=\d{8}T\d{6}/ (line 731) are similar but serve different purposes. While this works correctly, extracting them as named constants would improve maintainability and clarify intent:

const UNTIL_DATETIME_PATTERN = /UNTIL=(\d{8}T\d{6}Z?)/;
const UNTIL_DATETIME_NO_UTC_PATTERN = /UNTIL=\d{8}T\d{6}/;

♻️ Optional refactor to use named constants

At the top of the file (near other constants):

+// Regex patterns for RRULE UNTIL normalization
+const UNTIL_DATETIME_PATTERN = /UNTIL=(\d{8}T\d{6}Z?)/;
+const UNTIL_DATETIME_NO_UTC_PATTERN = /UNTIL=\d{8}T\d{6}/;

Then update the usage:

-          const untilMatch = rruleOnly.match(/UNTIL=(\d{8}T\d{6}Z?)/);
+          const untilMatch = rruleOnly.match(UNTIL_DATETIME_PATTERN);
           if (untilMatch && !untilMatch[1].endsWith('Z')) {
             // E.g., "20241231T100000"
             const untilLocal = untilMatch[1];

             try {
               const tzInfo = tzUtil.resolveTZID(curr.start.tz);
               let untilDate;

               // Parse UNTIL in the same timezone as DTSTART
               if (tzInfo.iana && tzUtil.isValidIana(tzInfo.iana)) {
                 untilDate = tzUtil.parseDateTimeInZone(untilLocal, tzInfo.iana);
               } else if (Number.isFinite(tzInfo.offsetMinutes)) {
                 untilDate = tzUtil.parseWithOffset(untilLocal, tzInfo.offset);
               }

               if (untilDate && typeof untilDate.toISOString === 'function') {
                 // Convert to UTC format: 20241231T090000Z
                 const untilUtc = untilDate.toISOString()
                   .replaceAll(/[-:]/g, '')
                   .replace(/\.\d{3}/, '');

                 // Replace in RRULE string
-                rruleOnly = rruleOnly.replace(/UNTIL=\d{8}T\d{6}/, `UNTIL=${untilUtc}`);
+                rruleOnly = rruleOnly.replace(UNTIL_DATETIME_NO_UTC_PATTERN, `UNTIL=${untilUtc}`);
               }
             } catch (error) {
               // If conversion fails, log warning but don't break parsing
               console.warn(`[node-ical] Failed to convert UNTIL to UTC: ${error.message}`);
             }
           }

Also applies to: 731-731

---

704-738: tzUtil.isValidIana exists and is properly exported—remove the verification concern. Consider enhancing error context as an optional improvement.

The UNTIL normalization logic is well-structured with proper guards, timezone resolution, and error handling. One item to address:

The error message on line 735 could optionally include more context for debugging. Rather than:

console.warn(`[node-ical] Failed to convert UNTIL to UTC: ${error.message}`);

Consider:

console.warn(`[node-ical] Failed to convert UNTIL to UTC for TZID="${curr.start.tz}", UNTIL="${untilLocal}": ${error.message}`);

(Note: tzUtil.isValidIana is defined in tz-utils.js:473 and properly exported; no verification needed.)

Configuration used: defaults

Review profile: CHILL

Plan: Pro

ical.js (1)

704-742: Well-implemented UNTIL normalization with proper fallback handling.

The logic correctly:

1. Only processes UNTIL when DTSTART has a timezone
2. Skips already-UTC values (ending with 'Z')
3. Uses the DTSTART timezone to interpret the local UNTIL value
4. Falls back gracefully when timezone conversion fails

One minor observation: UNTIL_DATETIME_NO_UTC_PATTERN on line 710 would technically match the prefix of a UTC timestamp (e.g., matching UNTIL=20241231T100000 within UNTIL=20241231T100000Z). This doesn't cause a bug because line 713 guards against UTC values, but adding a negative lookahead could make the intent clearer:

/UNTIL=\d{8}T\d{6}(?!Z)/

This is optional since the current code works correctly.

Configuration used: defaults

Review profile: CHILL

Plan: Pro