Skip to content

Commit e3c6fae

Browse files
committed
fix(tasty): injector optimizations * 2
1 parent 781e245 commit e3c6fae

File tree

4 files changed

+102
-43
lines changed

4 files changed

+102
-43
lines changed

src/tasty/injector/sheet-manager.test.ts

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ describe('SheetManager', () => {
341341
expect(registry.rules.has(className)).toBe(true);
342342
});
343343

344-
it('should schedule bulk cleanup when threshold is exceeded', () => {
344+
it('should schedule bulk cleanup when threshold is exceeded', (done) => {
345345
const manager = new SheetManager({ unusedStylesThreshold: 2 });
346346
const registry = manager.getRegistry(document);
347347

@@ -363,8 +363,16 @@ describe('SheetManager', () => {
363363
// Check if cleanup should be scheduled
364364
manager.checkCleanupNeeded(registry);
365365

366-
// Should have scheduled bulk cleanup
367-
expect(registry.bulkCleanupTimeout).toBeTruthy();
366+
// Should have scheduled cleanup check timeout immediately
367+
expect(registry.cleanupCheckTimeout).toBeTruthy();
368+
369+
// Wait for async cleanup check to complete
370+
setTimeout(() => {
371+
// Should have scheduled bulk cleanup after the check
372+
expect(registry.bulkCleanupTimeout).toBeTruthy();
373+
expect(registry.cleanupCheckTimeout).toBe(null);
374+
done();
375+
}, 10);
368376
});
369377
});
370378

@@ -441,7 +449,7 @@ describe('SheetManager', () => {
441449
});
442450

443451
describe('bulk cleanup scheduling', () => {
444-
it('should use requestIdleCallback when idleCleanup is enabled', () => {
452+
it('should use requestIdleCallback when idleCleanup is enabled', (done) => {
445453
// Mock requestIdleCallback
446454
const mockRequestIdleCallback = jest.fn((callback) => {
447455
return 123; // mock handle - don't execute callback immediately
@@ -471,15 +479,23 @@ describe('SheetManager', () => {
471479
registry.refCounts.set('test-class', 0);
472480
manager.checkCleanupNeeded(registry);
473481

474-
expect(mockRequestIdleCallback).toHaveBeenCalled();
475-
expect(registry.bulkCleanupTimeout).toBe(123);
482+
// Should have scheduled cleanup check timeout immediately
483+
expect(registry.cleanupCheckTimeout).toBeTruthy();
476484

477-
// Cleanup mocks
478-
delete (global as any).requestIdleCallback;
479-
delete (global as any).cancelIdleCallback;
485+
// Wait for async cleanup check to complete
486+
setTimeout(() => {
487+
expect(mockRequestIdleCallback).toHaveBeenCalled();
488+
expect(registry.bulkCleanupTimeout).toBe(123);
489+
expect(registry.cleanupCheckTimeout).toBe(null);
490+
491+
// Cleanup mocks
492+
delete (global as any).requestIdleCallback;
493+
delete (global as any).cancelIdleCallback;
494+
done();
495+
}, 10);
480496
});
481497

482-
it('should fallback to setTimeout when requestIdleCallback is not available', () => {
498+
it('should fallback to setTimeout when requestIdleCallback is not available', (done) => {
483499
// Ensure requestIdleCallback is not available
484500
delete (global as any).requestIdleCallback;
485501

@@ -506,9 +522,20 @@ describe('SheetManager', () => {
506522
registry.refCounts.set('test-class', 0);
507523
manager.checkCleanupNeeded(registry);
508524

509-
expect(mockSetTimeout).toHaveBeenCalledWith(expect.any(Function), 100);
525+
// Should have called setTimeout for cleanup check (0ms) immediately
526+
expect(mockSetTimeout).toHaveBeenCalledWith(expect.any(Function), 0);
527+
expect(registry.cleanupCheckTimeout).toBeTruthy();
528+
529+
// Wait for async cleanup check to complete
530+
setTimeout(() => {
531+
// Should have called setTimeout again for bulk cleanup (100ms)
532+
expect(mockSetTimeout).toHaveBeenCalledWith(expect.any(Function), 100);
533+
expect(registry.bulkCleanupTimeout).toBeTruthy();
534+
expect(registry.cleanupCheckTimeout).toBe(null);
510535

511-
mockSetTimeout.mockRestore();
536+
mockSetTimeout.mockRestore();
537+
done();
538+
}, 10);
512539
});
513540
});
514541
});

src/tasty/injector/sheet-manager.ts

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export class SheetManager {
4949
rules: new Map(),
5050
ruleTextSet: new Set<string>(),
5151
bulkCleanupTimeout: null,
52+
cleanupCheckTimeout: null,
5253
metrics,
5354
classCounter: 0,
5455
keyframesCache: new Map(),
@@ -596,48 +597,52 @@ export class SheetManager {
596597
rulesBySheet.get(sheetIndex)!.push({ className, ruleInfo });
597598
}
598599

599-
// Resolve root node for DOM checks (Document or ShadowRoot)
600-
const resolveRoot = (): Document | ShadowRoot | null => {
601-
const firstSheet = registry.sheets[0]?.sheet;
602-
if (!firstSheet) return null;
603-
const rootNode = (firstSheet as any).getRootNode
604-
? (firstSheet as any).getRootNode()
605-
: (firstSheet.ownerDocument as Document | null);
606-
// Prefer ShadowRoot if available, else Document
607-
return (rootNode as ShadowRoot | Document) || null;
608-
};
609-
610-
const rootNode = resolveRoot();
611-
612600
// Delete rules from each sheet (in reverse order to preserve indices)
613601
for (const [sheetIndex, rulesInSheet] of rulesBySheet) {
614602
// Sort by rule index in descending order for safe deletion
615603
rulesInSheet.sort((a, b) => b.ruleInfo.ruleIndex - a.ruleInfo.ruleIndex);
616604

617605
for (const { className, ruleInfo } of rulesInSheet) {
618-
// SAFETY: Re-check that the rule is still unused at deletion time.
619-
// Between scheduling and execution a class may have been restored
620-
// (refCount set to > 0). Skip such entries.
606+
// SAFETY 1: Double-check refCount is still 0
621607
const currentRefCount = registry.refCounts.get(className) || 0;
622608
if (currentRefCount > 0) {
623609
// Class became active again; do not delete
624610
continue;
625611
}
626612

627-
// Ensure we delete the same RuleInfo we marked earlier to avoid
628-
// accidentally deleting updated rules for the same class.
613+
// SAFETY 2: Ensure rule wasn't replaced
614+
// Between scheduling and execution a class may have been replaced with a new RuleInfo
629615
const currentInfo = registry.rules.get(className);
630-
if (currentInfo && currentInfo !== ruleInfo) {
616+
if (currentInfo !== ruleInfo) {
631617
// Rule was replaced; skip deletion of the old reference
632618
continue;
633619
}
634620

635-
// Optional last-resort safety: ensure the sheet element still exists
621+
// SAFETY 3: Verify the sheet element is still valid and accessible
636622
const sheetInfo = registry.sheets[ruleInfo.sheetIndex];
637623
if (!sheetInfo || !sheetInfo.sheet) {
624+
// Sheet was removed or corrupted; skip this rule
625+
continue;
626+
}
627+
628+
// SAFETY 4: Verify the stylesheet itself is accessible
629+
const styleSheet = sheetInfo.sheet.sheet;
630+
if (!styleSheet) {
631+
// Stylesheet not available; skip this rule
632+
continue;
633+
}
634+
635+
// SAFETY 5: Verify rule index is still within valid range
636+
const maxRuleIndex = styleSheet.cssRules.length - 1;
637+
const startIdx = ruleInfo.ruleIndex;
638+
const endIdx = ruleInfo.endRuleIndex ?? ruleInfo.ruleIndex;
639+
640+
if (startIdx < 0 || endIdx > maxRuleIndex || startIdx > endIdx) {
641+
// Rule indices are out of bounds; skip this rule
638642
continue;
639643
}
640644

645+
// All safety checks passed - proceed with deletion
641646
this.deleteRule(registry, ruleInfo);
642647
registry.rules.delete(className);
643648
registry.refCounts.delete(className);
@@ -890,9 +895,26 @@ export class SheetManager {
890895
}
891896

892897
/**
893-
* Check if cleanup should be scheduled (async, non-stacking)
898+
* Schedule async cleanup check (non-stacking)
894899
*/
895900
public checkCleanupNeeded(registry: RootRegistry): void {
901+
// Clear any existing check timeout to prevent stacking
902+
if (registry.cleanupCheckTimeout) {
903+
clearTimeout(registry.cleanupCheckTimeout);
904+
registry.cleanupCheckTimeout = null;
905+
}
906+
907+
// Schedule the actual check with setTimeout(..., 0)
908+
registry.cleanupCheckTimeout = setTimeout(() => {
909+
this.performCleanupCheck(registry);
910+
registry.cleanupCheckTimeout = null;
911+
}, 0);
912+
}
913+
914+
/**
915+
* Perform the actual cleanup check (called asynchronously)
916+
*/
917+
private performCleanupCheck(registry: RootRegistry): void {
896918
// Count unused rules (refCount = 0) - keyframes are disposed immediately
897919
const unusedRulesCount = Array.from(registry.refCounts.values()).filter(
898920
(count) => count === 0,
@@ -927,6 +949,12 @@ export class SheetManager {
927949
registry.bulkCleanupTimeout = null;
928950
}
929951

952+
// Cancel any scheduled cleanup check
953+
if (registry.cleanupCheckTimeout) {
954+
clearTimeout(registry.cleanupCheckTimeout);
955+
registry.cleanupCheckTimeout = null;
956+
}
957+
930958
// Remove all sheets
931959
for (const sheet of registry.sheets) {
932960
try {

src/tasty/injector/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ export interface RootRegistry {
7979
| ReturnType<typeof requestIdleCallback>
8080
| ReturnType<typeof setTimeout>
8181
| null;
82+
/** Scheduled cleanup check timeout */
83+
cleanupCheckTimeout: ReturnType<typeof setTimeout> | null;
8284
/** Performance metrics (optional) */
8385
metrics?: CacheMetrics;
8486
/** Counter for generating sequential class names like t0, t1, t2... */

src/tasty/tasty.tsx

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -382,28 +382,30 @@ function tastyElement<K extends StyleList, V extends VariantMap>(
382382

383383
const disposeRef = useRef<(() => void) | null>(null);
384384

385-
// Compute the className synchronously
386-
const injectedClassName = useMemo(() => {
387-
if (!directResult.rules.length) return '';
388-
// This will either reuse existing or allocate new class
389-
const { className } = inject(directResult.rules, { cacheKey });
390-
return className;
385+
// Single injection pattern - inject once and get both className and dispose
386+
const injectionResult = useMemo(() => {
387+
if (!directResult.rules.length) return null;
388+
return inject(directResult.rules, { cacheKey });
391389
}, [directResult.rules, cacheKey]);
392390

391+
const injectedClassName = injectionResult?.className || '';
392+
393393
// Handle disposal in useInsertionEffect
394394
useInsertionEffect(() => {
395+
// Cleanup previous disposal reference
395396
disposeRef.current?.();
396-
if (directResult.rules.length) {
397-
const { dispose } = inject(directResult.rules, { cacheKey });
398-
disposeRef.current = dispose;
397+
398+
if (injectionResult) {
399+
disposeRef.current = injectionResult.dispose;
399400
} else {
400401
disposeRef.current = null;
401402
}
403+
402404
return () => {
403405
disposeRef.current?.();
404406
disposeRef.current = null;
405407
};
406-
}, [directResult.rules, cacheKey]);
408+
}, [injectionResult]);
407409

408410
let modProps: Record<string, unknown> | undefined;
409411
if (mods) {

0 commit comments

Comments
 (0)