TypeScript Breathing Part2 🗡️🗡️ #329
Conversation
WalkthroughThis pull request involves a transition of several JavaScript files to TypeScript, enhancing type safety and improving the structure of the codebase. Key changes include making the Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
js/sidebar.ts (1)
Line range hint
142-155: Add event listener cleanup and improve type safety.Consider adding a cleanup mechanism for event listeners and improve type safety with proper event typing.
+type EventCleanup = () => void; + +const addSafeEventListener = <K extends keyof WindowEventMap>( + target: EventTarget, + type: K, + listener: (ev: WindowEventMap[K]) => void, + options?: boolean | AddEventListenerOptions +): EventCleanup => { + target.addEventListener(type, listener, options); + return () => target.removeEventListener(type, listener, options); +}; - document.addEventListener('keyup', ev => toggleHandler(ev.key), { once: false, passive: true }); + const cleanups: EventCleanup[] = []; + + cleanups.push( + addSafeEventListener(document, 'keyup', ev => toggleHandler(ev.key), + { passive: true }) + ); const toggleButton = document.getElementById(ID_TOGGLE_BUTTON); if (toggleButton) { - toggleButton.addEventListener('click', () => toggleSidebar(), - { once: false, passive: true }); + cleanups.push( + addSafeEventListener(toggleButton, 'click', () => toggleSidebar(), + { passive: true }) + ); } + // Return cleanup function + return () => cleanups.forEach(cleanup => cleanup());
🧹 Nitpick comments (10)
js/table-of-contents.ts (3)
8-17: Consider encapsulating state in a class or module.The current implementation uses global variables for state management, which could lead to maintainability issues and make testing more difficult. Consider encapsulating these variables within a class or module:
class TableOfContents { private readonly tocMap: Map<HTMLElement, HTMLAnchorElement> = new Map(); private observer?: IntersectionObserver; private mobileMaxWidth: number; private environment: number; private onlyActive: HTMLElement | null = null; private currentInlineCenter: HTMLElement | null = null; constructor(mobileMaxWidth: number) { this.mobileMaxWidth = mobileMaxWidth; this.environment = window.innerWidth >= mobileMaxWidth ? ENV_PC : ENV_MOBILE; } // ... rest of the methods }
27-43: Enhance accessibility and type safety.The active state management could be improved with better accessibility support and type safety:
- Consider using
aria-selectedinstead of a custom 'active' class- Add proper ARIA roles for navigation items
- Add type guard for target element
const isHTMLElement = (element: unknown): element is HTMLElement => { return element instanceof HTMLElement; }; const addActive = (entry: IntersectionObserverEntry): void => { const target = entry.target; if (!isHTMLElement(target)) { console.error('Invalid target element'); return; } // ... rest of the function }
71-75: Consider using built-in array methods.While the generator function works, consider using array methods for simpler code:
-function* reverseItr(array: IntersectionObserverEntry[]): Generator<IntersectionObserverEntry> { - for (let i = array.length - 1; i >= 0; i--) { - yield array[i]; - } -} +const reverseItr = (array: IntersectionObserverEntry[]): IntersectionObserverEntry[] => { + return [...array].reverse(); +};js/finder.ts (1)
65-71: Ensure consistency when accessing titles in search resultsAfter refactoring the title storage, make sure that any functions accessing titles, such as within
searchresults, use the updatedtitlesByKeymapping to prevent errors.js/replace-dom.ts (4)
3-4: Consider increasing the timeout duration.The
TIMEOUT_MSvalue of 50ms seems quite low for waiting on style changes, especially on slower devices or networks. Consider increasing this to a more reasonable value (e.g., 500ms or 1000ms).
6-15: Add JSDoc comments to interfaces.Consider adding documentation to explain the purpose and usage of these interfaces. This will improve code maintainability and help other developers understand the expected structure.
+/** + * Represents light and dark mode image sources + */ interface ImageSrc { light: string; dark: string; } +/** + * Configuration object for replaceable image elements + */ interface ReplaceImageObject { id: string; src: ImageSrc; alt: string; }
17-37: Consider using requestAnimationFrame for style polling.The current implementation uses
setTimeoutfor polling, which might not sync well with the browser's rendering cycle. Consider usingrequestAnimationFramefor better performance and browser compatibility.const waitForStyle = (property: string): Promise<string> => { const start = Date.now(); return new Promise((resolve, reject) => { const checkStyle = () => { const value = getRootVariable(property); if (value !== '') { resolve(value); return; } if (Date.now() - start >= TIMEOUT_MS) { reject(new Error(`Timeout: ${property}`)); return; } - setTimeout(checkStyle, INTERVAL_MS); + requestAnimationFrame(checkStyle); }; checkStyle(); }); };
39-58: Add error handling for setAttribute operations.The function should handle potential errors when setting attributes and consider batching DOM operations for better performance when dealing with multiple images.
const replaceProc = async (replaceImageObjectArray: ReplaceImageObject[]): Promise<void> => { const scheme = await waitForStyle('--color-scheme'); + const fragment = document.createDocumentFragment(); for (const x of replaceImageObjectArray) { const elm = document.getElementById(x.id); if (elm === null) { console.warn(`id: ${x.id} element does not exist`); continue; } const img = document.createElement('img'); + try { img.setAttribute('id', x.id); img.setAttribute('src', scheme === 'light' ? x.src.light : x.src.dark); img.setAttribute('alt', x.alt); img.setAttribute('decoding', 'lazy'); + fragment.appendChild(img); + elm.replaceWith(fragment.firstChild!); + } catch (error) { + console.error(`Failed to replace image ${x.id}:`, error); + } - elm.replaceWith(img); } };js/sidebar.ts (2)
16-19: Consider encapsulating global variables in a class or module.The global variables could be better organized by encapsulating them within a class or module structure. This would improve maintainability and prevent potential naming conflicts.
-let rootPath: string; -let isInitialized = false; -let searchPop: HTMLElement; +class SidebarManager { + private static rootPath: string; + private static isInitialized = false; + private static searchPop: HTMLElement; + + public static initialize(root: string): void { + this.rootPath = root; + // ... rest of initialization logic + } +}
50-57: Enhance error recovery with retry mechanism.Consider adding a retry mechanism for transient failures when loading the sidebar content. This would improve reliability on unstable networks.
+const retry = async <T>( + fn: () => Promise<T>, + retries: number = 3, + delay: number = 1000 +): Promise<T> => { + try { + return await fn(); + } catch (error) { + if (retries === 0) throw error; + await new Promise(resolve => setTimeout(resolve, delay)); + return retry(fn, retries - 1, delay); + } +}; try { - await loadStyleSheet(`${rootPath}${STYLE_CHAPTER}`); + await retry(() => loadStyleSheet(`${rootPath}${STYLE_CHAPTER}`)); sidebar.insertAdjacentHTML('afterbegin', await loadSitemap()); } catch (err: unknown) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
js/book.ts(1 hunks)js/build.js(1 hunks)js/finder.js(0 hunks)js/finder.ts(1 hunks)js/replace-dom.js(0 hunks)js/replace-dom.ts(1 hunks)js/sidebar.ts(6 hunks)js/table-of-contents.js(0 hunks)js/table-of-contents.ts(1 hunks)
💤 Files with no reviewable changes (3)
- js/finder.js
- js/replace-dom.js
- js/table-of-contents.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build JS
🔇 Additional comments (2)
js/book.ts (1)
10-10: Verify thatpathtorootis provided in allDataSetusagesBy making
pathtoroota required property in theDataSetinterface, ensure that all elements using this interface include thepathtorootattribute. Missing this attribute may lead to runtime errors.js/build.js (1)
7-7: Updated entry point to TypeScript fileThe
ENTRY_POINTSarray now includes'replace-dom.ts'instead of'replace-dom.js'. Confirm that'replace-dom.ts'exists and that the build process is correctly configured to handle TypeScript files.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
js/replace-dom.ts (3)
3-4: Consider increasing timeout values for network operations.The current values (INTERVAL_MS = 8ms, TIMEOUT_MS = 50ms) might be too aggressive for network operations or slow devices. Consider increasing these values to more reasonable defaults:
- INTERVAL_MS: ~100ms
- TIMEOUT_MS: ~1000ms
-const INTERVAL_MS = 8; -const TIMEOUT_MS = 50; +const INTERVAL_MS = 100; +const TIMEOUT_MS = 1000;
20-40: Enhance error message with current value.Consider including the current value in the error message to help with debugging.
- reject(new Error(`Timeout: ${property}`)); + reject(new Error(`Timeout waiting for ${property}. Current value: '${value}'`));
72-91: Add error handling for observer setup.Consider adding error handling for the initial
replaceProccall to prevent potential errors from stopping the observer setup.export const replaceId = (imageObjectArray: ImageObject[]): (() => void) => { - replaceProc(imageObjectArray); + replaceProc(imageObjectArray).catch(error => { + console.error('Failed to perform initial image replacement:', error); + });js/sidebar.ts (2)
19-19: Consider using a more specific type for searchPop.Instead of using
HTMLElement, consider using a more specific type that matches the actual element type.-let searchPop: HTMLElement; +let searchPop: HTMLDivElement;
50-57: Consider extracting error messages to constants.Error messages should be extracted to constants for better maintainability and consistency.
+const ERROR_MESSAGES = { + SIDEBAR_LOAD: 'Error loading sidebar content.', + UNKNOWN_ERROR: 'An unknown error occurred.', +} as const; - sidebar.insertAdjacentHTML('afterbegin', '<p>Error loading sidebar content.</p>'); + sidebar.insertAdjacentHTML('afterbegin', `<p>${ERROR_MESSAGES.SIDEBAR_LOAD}</p>`); - sidebar.insertAdjacentHTML('afterbegin', '<p>An unknown error occurred.</p>'); + sidebar.insertAdjacentHTML('afterbegin', `<p>${ERROR_MESSAGES.UNKNOWN_ERROR}</p>`);js/table-of-contents.ts (6)
3-7: Consider using TypeScript enums for better type safety.The environment and element constants could be better typed using TypeScript enums:
-const ENV_PC: number = 0; -const ENV_MOBILE: number = 1; +enum Environment { + PC, + MOBILE +} -const ELEMENT_TOC: string[] = ['righttoc', 'bottomtoc']; +const ELEMENT_TOC = ['righttoc', 'bottomtoc'] as const; +type TocElement = typeof ELEMENT_TOC[number];
8-16: Add explicit type annotations for global state variables.The state variables would benefit from more specific TypeScript types:
-let environment: number; +let environment: Environment; -let onlyActive: HTMLElement | null = null; +let onlyActive: HTMLAnchorElement | null = null; -let currentInlineCenter: HTMLElement | null = null; +let currentInlineCenter: HTMLAnchorElement | null = null;
27-43: Improve type safety in addActive function.Replace type assertion with type guard and enhance error handling:
-const target = tocMap.get(entry.target as HTMLElement); +if (!(entry.target instanceof HTMLElement)) { + console.error('Invalid target type in intersection entry'); + return; +} +const target = tocMap.get(entry.target); if (target === undefined) { - console.error(`addActive: ${entry.target} does not exist`); + console.error(`TOC mapping not found for element: ${entry.target.id || entry.target.tagName}`); return; }
88-89: Consider using window.matchMedia instead of window.innerWidth.Using innerWidth for environment detection is less reliable than matchMedia:
- environment = window.innerWidth >= mobileMaxWidth ? ENV_PC : ENV_MOBILE; + environment = window.matchMedia(`(min-width: ${mobileMaxWidth}px)`).matches ? + Environment.PC : Environment.MOBILE;
111-119: Improve type safety in link creation.The link creation and class assignment could be more robust:
const link: HTMLAnchorElement = document.createElement('a'); +const text = el.textContent || el.innerText || ''; -link.appendChild(document.createTextNode(el.text)); +link.appendChild(document.createTextNode(text)); link.href = el.href; -link.classList.add(el.parentElement?.tagName ?? ''); +if (el.parentElement?.tagName) { + link.classList.add(el.parentElement.tagName.toLowerCase()); +}
1-163: Add JSDoc comments and consider unit testing.The code would benefit from:
- JSDoc comments for exported functions to document parameters, return values, and behavior
- Unit tests to verify the table of contents behavior in different environments
Example JSDoc for exported functions:
/** * Resets the table of contents by removing existing elements and reinitializing * @throws {Error} If required DOM elements are not found */ export const tocReset = (): void => { // ... }; /** * Initializes the table of contents with mobile/desktop responsive behavior * @returns {() => void} Cleanup function to remove event listeners and observers * @throws {Error} If mobile max width is invalid or media query is not supported */ export const initTableOfContents = (): (() => void) => { // ... };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/replace-dom.ts(1 hunks)js/sidebar.ts(6 hunks)js/table-of-contents.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
js/replace-dom.ts (2)
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:23:08.280Z
Learning: The replaceId function in replace-dom.ts returns a cleanup function to disconnect the MutationObserver, allowing for explicit cleanup when needed in the future.
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:01:41.027Z
Learning: MutationObservers attached to document.documentElement in replace-dom.ts don't require explicit cleanup as they are automatically cleaned up when the page unloads, and the observer exists throughout the page's lifetime.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build JS
🔇 Additional comments (9)
js/replace-dom.ts (2)
6-18: LGTM! Well-structured interface hierarchy.The interface design with
BaseObjectas the base interface andImageObjectextending it with additional properties is clean and maintainable.
63-70: LGTM! Clean and type-safe debounce implementation.The generic implementation with
BaseObjectconstraint is well-designed and maintains type safety.js/sidebar.ts (3)
Line range hint
21-33: LGTM! Well-implemented URL handling with proper types.The URL handling functions are well-typed and include proper error handling.
84-115: LGTM! Type-safe sidebar visibility management.The sidebar visibility functions make good use of TypeScript features like optional chaining and type assertions where necessary.
Line range hint
117-156: LGTM! Well-implemented event handlers.The event handlers are properly typed and make good use of passive listeners for better performance.
js/table-of-contents.ts (4)
18-25: Specify setTimeout duration and handle cleanup.The setTimeout call lacks a duration parameter and doesn't handle cleanup, which could lead to unexpected behavior. The previous review comment's suggestion remains valid:
- setTimeout(() => { + const timeoutId = setTimeout(() => { currentInlineCenter?.scrollIntoView({ inline: 'center' }); - }); + }, 0); + + // Consider cleanup in a destructor or reset function + return () => clearTimeout(timeoutId);
71-75: LGTM! Well-typed generator implementation.Good use of TypeScript's generator syntax with proper type annotations.
132-153: LGTM! Good error handling and cleanup.The implementation includes proper error handling and cleanup of resources.
155-163: Add cleanup for event listeners and error handling.The initialization still needs proper cleanup and error handling as suggested in the previous review:
export const initTableOfContents = (): void => { mobileMaxWidth = getRootVariableNum('--mobile-max-width'); + if (mobileMaxWidth <= 0) { + console.error('Invalid mobile max width'); + return; + } initialize(); - window - .matchMedia(`(min-width: ${mobileMaxWidth}px)`) - .addEventListener('change', tocReset, { once: false, passive: true }); + const mediaQuery = window.matchMedia(`(min-width: ${mobileMaxWidth}px)`); + if (!mediaQuery?.matches) { + console.error('Media query not supported'); + return; + } + + mediaQuery.addEventListener('change', tocReset, { passive: true }); + + // Return cleanup function + return () => { + mediaQuery.removeEventListener('change', tocReset); + observer.disconnect(); + tocMap.clear(); + }; };
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
js/replace-dom.ts (4)
3-4: Consider increasing timeout values for network operations.The current values (INTERVAL_MS: 8ms, TIMEOUT_MS: 50ms) might be too aggressive for slower networks or when the system is under load.
-const INTERVAL_MS = 8; -const TIMEOUT_MS = 50; +const INTERVAL_MS = 50; +const TIMEOUT_MS = 1000;
20-40: Enhance error message with more context.The error message could be more descriptive to aid in debugging.
- reject(new Error(`Timeout: ${property}`)); + reject(new Error(`Timeout waiting for CSS variable "${property}" after ${TIMEOUT_MS}ms`));
42-67: Consider batch processing for large arrays.For better performance with large arrays, consider processing images in batches and using
requestAnimationFrameto avoid blocking the main thread.const processBatch = (batch: ImageObject[], scheme: string) => { for (const x of batch) { // ... existing replacement logic } }; const replaceProc = async (imageObjectArray: ImageObject[]): Promise<void> => { let scheme: string; try { scheme = await waitForStyle('--color-scheme'); } catch (error) { console.error('Failed to get color scheme:', error); scheme = 'light'; } const BATCH_SIZE = 10; for (let i = 0; i < imageObjectArray.length; i += BATCH_SIZE) { const batch = imageObjectArray.slice(i, i + BATCH_SIZE); requestAnimationFrame(() => processBatch(batch, scheme)); } };
78-97: Add JSDoc comment to document the cleanup function.Consider adding documentation to clarify the purpose and usage of the returned cleanup function.
+/** + * Replaces images based on color scheme and observes class changes. + * @param imageObjectArray - Array of image objects to process + * @returns A cleanup function that disconnects the observer when called + */ export const replaceId = (imageObjectArray: ImageObject[]): (() => void) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/replace-dom.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
js/replace-dom.ts (2)
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:23:08.280Z
Learning: The replaceId function in replace-dom.ts returns a cleanup function to disconnect the MutationObserver, allowing for explicit cleanup when needed in the future.
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:01:41.027Z
Learning: MutationObservers attached to document.documentElement in replace-dom.ts don't require explicit cleanup as they are automatically cleaned up when the page unloads, and the observer exists throughout the page's lifetime.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
js/replace-dom.ts (2)
6-18: LGTM! Well-structured interface hierarchy.The interfaces are well-organized with
BaseObjectas the base interface, promoting code reuse and type safety.
69-76: LGTM! Clean and type-safe debounce implementation.The debounce utility is well-implemented with proper TypeScript generics and type constraints.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
js/replace-dom.ts (4)
3-6: Document the purpose and rationale of timing constants.Add JSDoc comments explaining the purpose of these timing constants and why these specific values were chosen.
+/** + * Interval between style checks in milliseconds. + */ const INTERVAL_MS = 50; + +/** + * Maximum time to wait for style to be available in milliseconds. + */ const TIMEOUT_MS = 1000;
22-42: Add return type annotation for the checkStyle function.For better type safety, explicitly declare the return type of the inner function.
- const checkStyle = () => { + const checkStyle = (): void => {
53-70: Add error handling for image loading failures.Consider handling potential image loading failures and adding loading state attributes for better user experience.
const img = document.createElement('img'); img.setAttribute('id', x.id); img.setAttribute('src', scheme === 'light' ? x.src.light : x.src.dark); img.setAttribute('alt', x.alt); img.setAttribute('decoding', 'lazy'); + img.setAttribute('loading', 'lazy'); + + img.onerror = () => { + console.error(`Failed to load image for id: ${x.id}`); + // Optionally set a fallback image or add error class + }; elm.replaceWith(img);
92-111: Add error logging for observer setup failures.Consider adding try-catch block around observer setup to handle potential errors.
- observer.observe(document.documentElement, { attributes: true, attributeFilter: ['class'] }); + try { + observer.observe(document.documentElement, { attributes: true, attributeFilter: ['class'] }); + } catch (error) { + console.error('Failed to setup observer:', error); + // Return a no-op cleanup function + return () => {}; + }js/table-of-contents.ts (3)
3-16: Consider using TypeScript enums for environment types.The environment constants could be better represented using TypeScript's enum feature for improved type safety and maintainability.
-const ENV_PC: number = 0; -const ENV_MOBILE: number = 1; - -const ELEMENT_TOC: string[] = ['righttoc', 'bottomtoc']; - -let environment: number; +enum Environment { + PC, + MOBILE +} + +const ELEMENT_TOC: Record<Environment, string> = { + [Environment.PC]: 'righttoc', + [Environment.MOBILE]: 'bottomtoc' +}; + +let environment: Environment;
27-69: Add type guard for IntersectionObserverEntry target.The type casting of entry.target could be replaced with a type guard for better type safety.
+function isHTMLElement(element: Element): element is HTMLElement { + return element instanceof HTMLElement; +} + const addActive = (entry: IntersectionObserverEntry): void => { if (onlyActive !== null) { onlyActive.classList.remove('active'); onlyActive = null; } - const target = tocMap.get(entry.target as HTMLElement); + if (!isHTMLElement(entry.target)) { + console.error('Target is not an HTMLElement'); + return; + } + const target = tocMap.get(entry.target);
77-132: Extract magic values as named constants.Consider extracting the intersection observer threshold and ARIA label as named constants for better maintainability.
+const INTERSECTION_THRESHOLD = 1.0; +const ARIA_LABEL = 'Table of Contents'; + const initialize = (): void => { observer = new IntersectionObserver( (entries: IntersectionObserverEntry[]) => { for (const x of reverseItr(entries)) { x.isIntersecting ? addActive(x) : removeActive(x); } scrollCenter(); }, - { threshold: 1.0 }, + { threshold: INTERSECTION_THRESHOLD }, ); const nav: HTMLElement = document.createElement('nav'); nav.setAttribute('id', 'pagetoc'); nav.setAttribute('role', 'navigation'); - nav.setAttribute('aria-label', 'Table of Contents'); + nav.setAttribute('aria-label', ARIA_LABEL);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/replace-dom.ts(1 hunks)js/table-of-contents.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
js/table-of-contents.ts (2)
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/table-of-contents.ts:155-163
Timestamp: 2025-01-23T06:05:02.932Z
Learning: When handling CSS variable retrieval errors in TypeScript, prefer using try-catch with a sensible fallback value instead of early returns, as demonstrated in the table of contents component where `--mobile-max-width` falls back to 999 when the variable cannot be loaded.
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/table-of-contents.ts:18-25
Timestamp: 2025-01-23T05:45:30.531Z
Learning: The currentInlineCenter variable in table-of-contents.ts acts as a guard to prevent duplicate scroll operations, making explicit timeout cleanup unnecessary.
js/replace-dom.ts (2)
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:23:08.280Z
Learning: The replaceId function in replace-dom.ts returns a cleanup function to disconnect the MutationObserver, allowing for explicit cleanup when needed in the future.
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:01:41.027Z
Learning: MutationObservers attached to document.documentElement in replace-dom.ts don't require explicit cleanup as they are automatically cleaned up when the page unloads, and the observer exists throughout the page's lifetime.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
js/replace-dom.ts (4)
6-6: Document the rationale behind BATCH_SIZE value.The batch size of 2 seems arbitrary. Consider documenting why this specific value was chosen or if it should be configurable based on performance testing.
Would you like me to help create a performance test script to determine the optimal batch size?
8-20: LGTM! Well-structured type hierarchy.The interfaces are well-organized with a clear hierarchy.
BaseObjectprovides a common foundation, whileImageSrcandImageObjectadd specific properties needed for image handling.
44-51: LGTM! Type-safe debounce implementation.The implementation correctly uses generics with the
BaseObjectconstraint, providing type safety while maintaining flexibility.
72-85: LGTM! Well-implemented error handling and batch processing.The function correctly implements error handling for color scheme retrieval and uses
requestAnimationFramefor efficient batch processing.js/table-of-contents.ts (4)
18-25: LGTM! Scroll handling implementation is correct.The implementation correctly uses currentInlineCenter as a guard and setTimeout without an explicit delay.
71-75: LGTM! Well-implemented generator function.The reverse iterator is correctly typed and efficiently implemented.
134-156: LGTM! Thorough cleanup implementation.The reset functionality properly handles cleanup of both the observer and the mapping, with appropriate error handling.
158-171: LGTM! Robust initialization with proper error handling.The implementation correctly follows the established pattern of using try-catch with a fallback value for CSS variable retrieval.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
js/replace-dom.ts (4)
3-6: Document timing constants and consider configuration options.The timing constants could benefit from documentation explaining their purpose and impact. Consider making these configurable for different environments.
+/** Interval between style checks in milliseconds */ const INTERVAL_MS = 50; +/** Maximum time to wait for style in milliseconds */ const TIMEOUT_MS = 1000;
6-6: Document batch size constant and consider performance implications.The batch size affects DOM manipulation performance. Document why 2 was chosen and consider making it configurable based on device capabilities.
+/** Number of images to process in each animation frame for optimal performance */ const BATCH_SIZE = 2;
22-42: Consider additional error cases in waitForStyle.The function handles timeouts well, but consider adding validation for invalid property names and handling potential getRootVariable errors.
const waitForStyle = (property: string): Promise<string> => { + if (!property || !property.startsWith('--')) { + return Promise.reject(new Error('Invalid CSS variable name')); + } const start = Date.now(); return new Promise((resolve, reject) => { const checkStyle = (): void => { - const value = getRootVariable(property); + let value: string; + try { + value = getRootVariable(property); + } catch (error) { + reject(new Error(`Failed to get CSS variable: ${error.message}`)); + return; + }
53-74: Enhance error handling and add memory management.Consider cleaning up old event listeners and adding more comprehensive error handling.
const processBatch = (batch: ImageObject[], scheme: string) => { for (const x of batch) { const elm = document.getElementById(x.id); if (elm === null) { console.warn(`id: ${x.id} element does not exist`); continue; } const img = document.createElement('img'); img.setAttribute('id', x.id); img.setAttribute('src', scheme === 'light' ? x.src.light : x.src.dark); img.setAttribute('alt', x.alt); img.setAttribute('loading', 'lazy'); + // Clean up old error listeners if element is an image + if (elm instanceof HTMLImageElement) { + elm.onerror = null; + } img.onerror = () => { console.error(`Failed to load image for id: ${x.id}`); + // Optionally retry with the other theme's URL as fallback + const fallbackSrc = scheme === 'light' ? x.src.dark : x.src.light; + img.src = fallbackSrc; }; elm.replaceWith(img); } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/replace-dom.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
js/replace-dom.ts (2)
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:23:08.280Z
Learning: The replaceId function in replace-dom.ts returns a cleanup function to disconnect the MutationObserver, allowing for explicit cleanup when needed in the future.
Learnt from: CoralPink
PR: CoralPink/commentary#329
File: js/replace-dom.ts:60-72
Timestamp: 2025-01-23T04:01:41.027Z
Learning: MutationObservers attached to document.documentElement in replace-dom.ts don't require explicit cleanup as they are automatically cleaned up when the page unloads, and the observer exists throughout the page's lifetime.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build JS
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
js/replace-dom.ts (3)
8-20: LGTM! Well-structured interface hierarchy.The interfaces are well-organized with a clear hierarchy. The
BaseObjectinterface provides a good foundation for extension.
44-51: LGTM! Type-safe debounce implementation.The implementation follows the agreed-upon approach using
BaseObjectconstraint instead ofany, providing good type safety while satisfying linting rules.
91-115: LGTM! Well-documented and properly implemented.The function is well-documented with JSDoc, implements proper cleanup, and uses debouncing effectively. The implementation aligns with the previously discussed approach.
|
I will review the rest of rabbit comments later and would like to merge them so far. By the way, the idea of “returning a cleanup function” is very interesting. |
Continuing from #320, I'm rewriting
JavaScripttoTypeScript.I'm getting a little more comfortable with it as I go along,
but I’m going to the movies to see "The Apprentice" tomorrow 🃏
So I decided to break up the work here.
I'll be gone all day tomorrow, so if there are any problems I'll fix them the day after tomorrow😅
Summary by CodeRabbit
Release Notes
New Features
Finderclass for refined search functionality.Changes
DataSetinterface to enforce required properties.Finder,replace-dom, andtable-of-contents.