perf(planner): drop per-CD getters/allocations in planner-task and kb (#8808)

planner-task took a non-signal @Input task and exposed per-change-
detection getters (titleHasLinks ran a regex scan, timeEstimate, and
the isDone/isDragReady/isCurrent HostBinding getters) that re-executed
on every CD pass, per row. Convert task to a required signal input,
replace the getters with computed()s, and move the class bindings to
host metadata (mirrors schedule-event). The kb getters in task and
main-header allocated a fresh {} on the no-shortcuts path every CD
pass; return a shared frozen EMPTY_KEYBOARD_CONFIG via a pure
keyboardConfigOrEmpty helper so the value is referentially stable.
Behavior (current-task highlight, drag-ready, done styling, shortcut
hints, sub-task count) is unchanged.

SPAP-28
This commit is contained in:
aakhter 2026-07-07 05:49:46 -04:00 committed by GitHub
parent e29cd4cd11
commit 0842ffe1fa
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 212 additions and 57 deletions

View file

@ -59,6 +59,14 @@ export type KeyboardConfig = Readonly<{
[key: `plugin_${string}`]: string | null;
}>;
/** Shared frozen empty keyboard config so the "no shortcuts" path returns a
* referentially-stable value instead of allocating a fresh {} every CD pass. */
export const EMPTY_KEYBOARD_CONFIG: KeyboardConfig = Object.freeze({}) as KeyboardConfig;
export const keyboardConfigOrEmpty = (
keyboard: KeyboardConfig | undefined,
): KeyboardConfig => keyboard ?? EMPTY_KEYBOARD_CONFIG;
export const GLOBAL_KEY_CFG_KEYS: (keyof KeyboardConfig)[] = [
'globalShowHide',
'globalToggleTaskStart',

View file

@ -23,7 +23,7 @@ import { SyncWrapperService } from '../../imex/sync/sync-wrapper.service';
import { SnackService } from '../../core/snack/snack.service';
import { NavigationEnd, Router } from '@angular/router';
import { GlobalConfigService } from '../../features/config/global-config.service';
import { KeyboardConfig } from '@sp/keyboard-config';
import { KeyboardConfig, keyboardConfigOrEmpty } from '@sp/keyboard-config';
import { MatIconButton } from '@angular/material/button';
import { MatIcon } from '@angular/material/icon';
import { MatTooltip } from '@angular/material/tooltip';
@ -342,6 +342,6 @@ export class MainHeaderComponent implements OnDestroy {
}
get kb(): KeyboardConfig {
return (this._configService.cfg()?.keyboard as KeyboardConfig) || {};
return keyboardConfigOrEmpty(this._configService.cfg()?.keyboard as KeyboardConfig);
}
}

View file

@ -0,0 +1,24 @@
import {
EMPTY_KEYBOARD_CONFIG,
KeyboardConfig,
keyboardConfigOrEmpty,
} from '@sp/keyboard-config';
describe('keyboardConfigOrEmpty', () => {
it('returns EMPTY_KEYBOARD_CONFIG for undefined', () => {
expect(keyboardConfigOrEmpty(undefined)).toBe(EMPTY_KEYBOARD_CONFIG);
});
it('returns a referentially-stable empty config across calls', () => {
expect(keyboardConfigOrEmpty(undefined)).toBe(keyboardConfigOrEmpty(undefined));
});
it('returns the same reference for a provided config', () => {
const someKb: KeyboardConfig = { addNewTask: 'n' };
expect(keyboardConfigOrEmpty(someKb)).toBe(someKb);
});
it('EMPTY_KEYBOARD_CONFIG is frozen', () => {
expect(Object.isFrozen(EMPTY_KEYBOARD_CONFIG)).toBe(true);
});
});

View file

@ -1,7 +1,7 @@
<ng-content></ng-content>
<swipe-block
[isDone]="task.isDone"
[isDone]="task().isDone"
(swipeStart)="onSwipeStart()"
(swipeRight)="toggleTaskDone()"
(swipeRightTriggered)="onSwipeRightTriggered($event)"
@ -9,8 +9,8 @@
>
<div class="task-content">
<done-toggle
[isDone]="task.isDone"
[isCurrent]="isCurrent"
[isDone]="task().isDone"
[isCurrent]="isCurrent()"
[showDoneAnimation]="showDoneAnimation()"
[showUndoneAnimation]="showUndoneAnimation()"
[attr.aria-label]="T.F.TASK.CMP.TOGGLE_DONE | translate"
@ -23,38 +23,38 @@
<div class="parent-title">{{ parentTitle }}</div>
}
@if (titleHasLinks) {
@if (titleHasLinks()) {
<div
class="title"
[innerHTML]="task.title | renderLinks"
[innerHTML]="task().title | renderLinks"
></div>
} @else {
<div class="title">{{ task.title }}</div>
<div class="title">{{ task().title }}</div>
}
<tag-list
[isShowProjectTagAlways]="true"
[tagsToHide]="tagsToHide()"
[task]="task"
[task]="task()"
></tag-list>
</div>
<div
(click)="estimateTimeClick($event)"
[class.hasNoTimeSpentOrEstimate]="!task.timeSpent && !task.timeEstimate"
[style.pointer-events]="task.subTaskIds?.length && 'none'"
[class.hasNoTimeSpentOrEstimate]="!task().timeSpent && !task().timeEstimate"
[style.pointer-events]="task().subTaskIds?.length && 'none'"
class="planner-time-remaining-shared"
>
<inline-input
(changed)="updateTimeEstimate($event)"
[displayValue]="timeEstimate | msToString"
[displayValue]="timeEstimate() | msToString"
[type]="'duration'"
[value]="timeEstimate"
[value]="timeEstimate()"
>
</inline-input>
@if (task.subTaskIds?.length) {
@if (task().subTaskIds?.length) {
<span class="sub-task-count">
<mat-icon inline="true">account_tree</mat-icon>{{ task.subTaskIds.length }}
<mat-icon inline="true">account_tree</mat-icon>{{ task().subTaskIds?.length }}
</span>
}
</div>
@ -65,6 +65,6 @@
@if (isContextMenuLoaded()) {
<task-context-menu
#taskContextMenu
[task]="task"
[task]="task()"
></task-context-menu>
}

View file

@ -0,0 +1,126 @@
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { NO_ERRORS_SCHEMA, signal, WritableSignal } from '@angular/core';
import { TranslateModule } from '@ngx-translate/core';
import { of } from 'rxjs';
import { PlannerTaskComponent } from './planner-task.component';
import { TaskService } from '../../tasks/task.service';
import { DEFAULT_TASK, TaskCopy } from '../../tasks/task.model';
import { MsToStringPipe } from '../../../ui/duration/ms-to-string.pipe';
import { RenderLinksPipe } from '../../../ui/pipes/render-links.pipe';
import { TranslatePipe } from '@ngx-translate/core';
const makeTask = (overrides: Partial<TaskCopy> = {}): TaskCopy =>
({
...DEFAULT_TASK,
projectId: 'p1',
id: 't1',
parentId: null,
...overrides,
}) as TaskCopy;
describe('PlannerTaskComponent', () => {
let currentTaskId: WritableSignal<string | null>;
const create = (
task: TaskCopy,
): {
fixture: ComponentFixture<PlannerTaskComponent>;
component: PlannerTaskComponent;
} => {
const fixture = TestBed.createComponent(PlannerTaskComponent);
fixture.componentRef.setInput('task', task);
fixture.detectChanges();
return { fixture, component: fixture.componentInstance };
};
beforeEach(() => {
currentTaskId = signal<string | null>(null);
const taskServiceMock = {
...jasmine.createSpyObj('TaskService', [
'setSelectedId',
'toggleDoneWithAnimation',
'update',
]),
currentTaskId,
getByIdLive$: () => of(null),
};
TestBed.configureTestingModule({
imports: [PlannerTaskComponent, TranslateModule.forRoot()],
schemas: [NO_ERRORS_SCHEMA],
providers: [{ provide: TaskService, useValue: taskServiceMock }],
});
// Isolate the component from its heavyweight child components (tag-list etc.)
// so the spec exercises PlannerTaskComponent's OWN template bindings without
// needing the full Store/service graph. Child elements become unknown tags
// (ignored via NO_ERRORS_SCHEMA); the pipes used in the template are kept.
TestBed.overrideComponent(PlannerTaskComponent, {
set: {
imports: [MsToStringPipe, RenderLinksPipe, TranslatePipe],
schemas: [NO_ERRORS_SCHEMA],
},
});
});
it('renders the template without throwing', () => {
expect(() => create(makeTask({ title: 'plain title' }))).not.toThrow();
});
describe('titleHasLinks', () => {
it('is false for a plain title', () => {
const { component } = create(makeTask({ title: 'plain title' }));
expect(component.titleHasLinks()).toBe(false);
});
it('is true for a title containing a URL', () => {
const { component } = create(makeTask({ title: 'see https://example.com' }));
expect(component.titleHasLinks()).toBe(true);
});
});
describe('timeEstimate', () => {
it('returns the raw estimate when the task has subTaskIds', () => {
const { component } = create(
makeTask({ timeEstimate: 5000, timeSpent: 2000, subTaskIds: ['s1'] }),
);
expect(component.timeEstimate()).toBe(5000);
});
});
describe('isCurrent', () => {
it('is true when the current task id equals the task id and reflects on the host', () => {
currentTaskId.set('t1');
const { fixture, component } = create(makeTask({ id: 't1' }));
expect(component.isCurrent()).toBe(true);
expect(fixture.debugElement.nativeElement.classList.contains('isCurrent')).toBe(
true,
);
});
it('flips to false when the current task id changes', () => {
currentTaskId.set('t1');
const { fixture, component } = create(makeTask({ id: 't1' }));
expect(component.isCurrent()).toBe(true);
currentTaskId.set('other');
fixture.detectChanges();
expect(component.isCurrent()).toBe(false);
expect(fixture.debugElement.nativeElement.classList.contains('isCurrent')).toBe(
false,
);
});
});
describe('isDone host class', () => {
it('carries the isDone class when the task is done', () => {
const { fixture } = create(makeTask({ isDone: true }));
expect(fixture.debugElement.nativeElement.classList.contains('isDone')).toBe(true);
});
it('omits the isDone class when the task is not done', () => {
const { fixture } = create(makeTask({ isDone: false }));
expect(fixture.debugElement.nativeElement.classList.contains('isDone')).toBe(false);
});
});
});

View file

@ -3,12 +3,11 @@ import {
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
computed,
DestroyRef,
ElementRef,
HostBinding,
HostListener,
inject,
Input,
input,
OnDestroy,
OnInit,
@ -49,21 +48,26 @@ import { TranslatePipe } from '@ngx-translate/core';
SwipeBlockComponent,
TranslatePipe,
],
/* eslint-disable @typescript-eslint/naming-convention */
host: {
'[class.isDone]': 'task().isDone',
'[class.isDragReady]': 'isDragReady()',
'[class.isCurrent]': 'isCurrent()',
},
/* eslint-enable @typescript-eslint/naming-convention */
})
export class PlannerTaskComponent implements OnInit, OnDestroy, AfterViewInit {
private _taskService = inject(TaskService);
private _cd = inject(ChangeDetectorRef);
private _destroyRef = inject(DestroyRef);
private _elementRef = inject(ElementRef);
get titleHasLinks(): boolean {
const title = this.task?.title;
return !!title && hasLinkHints(title);
}
// TODO: Skipped for migration because:
// This input is used in a control flow expression (e.g. `@if` or `*ngIf`)
// and migrating would break narrowing currently.
@Input({ required: true }) task!: TaskCopy;
readonly task = input.required<TaskCopy>();
readonly titleHasLinks = computed<boolean>(() => {
const title = this.task().title;
return !!title && hasLinkHints(title);
});
// TODO remove
readonly day = input<string | undefined>();
@ -84,20 +88,9 @@ export class PlannerTaskComponent implements OnInit, OnDestroy, AfterViewInit {
read: TaskContextMenuComponent,
});
@HostBinding('class.isDone')
get isDone(): boolean {
return this.task.isDone;
}
@HostBinding('class.isDragReady')
get isDragReadyClass(): boolean {
return this.isDragReady();
}
@HostBinding('class.isCurrent')
get isCurrent(): boolean {
return this.task.id === this._taskService.currentTaskId();
}
readonly isCurrent = computed<boolean>(
() => this.task().id === this._taskService.currentTaskId(),
);
@HostListener('contextmenu', ['$event'])
onContextMenu(event: MouseEvent): void {
@ -114,25 +107,24 @@ export class PlannerTaskComponent implements OnInit, OnDestroy, AfterViewInit {
if (target?.tagName === 'A' || target?.closest('a')) {
return;
}
if (this.task) {
// Use bottom panel on mobile, dialog on desktop
this._taskService.setSelectedId(this.task.id);
}
// Use bottom panel on mobile, dialog on desktop
this._taskService.setSelectedId(this.task().id);
}
get timeEstimate(): number {
const t = this.task;
return this.task.subTaskIds
readonly timeEstimate = computed<number>(() => {
const t = this.task();
return t.subTaskIds
? t.timeEstimate
: t.timeEstimate - t.timeSpent > 0
? t.timeEstimate - t.timeSpent
: 0;
}
});
ngOnInit(): void {
if (this.task.parentId) {
const parentId = this.task().parentId;
if (parentId) {
this._taskService
.getByIdLive$(this.task.parentId)
.getByIdLive$(parentId)
.pipe(takeUntilDestroyed(this._destroyRef))
.subscribe((parentTask) => {
this.parentTitle = parentTask && parentTask.title;
@ -179,7 +171,7 @@ export class PlannerTaskComponent implements OnInit, OnDestroy, AfterViewInit {
}
onSwipeRightTriggered(isTriggered: boolean): void {
if (this.task.isDone) {
if (this.task().isDone) {
this.showUndoneAnimation.set(isTriggered);
} else {
this.showDoneAnimation.set(isTriggered);
@ -188,9 +180,10 @@ export class PlannerTaskComponent implements OnInit, OnDestroy, AfterViewInit {
toggleTaskDone(): void {
window.clearTimeout(this._doneAnimationTimeout);
const t = this.task();
this._doneAnimationTimeout = this._taskService.toggleDoneWithAnimation(
this.task.id,
this.task.isDone,
t.id,
t.isDone,
(v) => this.showDoneAnimation.set(v),
);
}
@ -216,7 +209,7 @@ export class PlannerTaskComponent implements OnInit, OnDestroy, AfterViewInit {
}
updateTimeEstimate(val: number): void {
this._taskService.update(this.task.id, {
this._taskService.update(this.task().id, {
timeEstimate: val,
});
}

View file

@ -71,7 +71,11 @@ import { DateService } from '../../../core/date/date.service';
import { isTouchActive } from '../../../util/input-intent';
import { IS_HYBRID_DEVICE } from '../../../util/is-mouse-primary';
import { DRAG_DELAY_FOR_TOUCH } from '../../../app.constants';
import { KeyboardConfig } from '@sp/keyboard-config';
import {
EMPTY_KEYBOARD_CONFIG,
KeyboardConfig,
keyboardConfigOrEmpty,
} from '@sp/keyboard-config';
import { DialogScheduleTaskComponent } from '../../planner/dialog-schedule-task/dialog-schedule-task.component';
import { PlannerActions } from '../../planner/store/planner.actions';
import { PlannerService } from '../../planner/planner.service';
@ -1394,9 +1398,9 @@ export class TaskComponent implements OnDestroy, AfterViewInit {
get kb(): KeyboardConfig {
if (isTouchActive()) {
return {} as KeyboardConfig;
return EMPTY_KEYBOARD_CONFIG;
}
return (this._configService.cfg()?.keyboard as KeyboardConfig) || {};
return keyboardConfigOrEmpty(this._configService.cfg()?.keyboard as KeyboardConfig);
}
protected readonly ICAL_TYPE = ICAL_TYPE;