From c62b2eb88f82dac098de12aa92d3df09aa00eddc Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Sat, 18 Jul 2026 14:53:32 +0200 Subject: [PATCH] fix(theme): share one default-theme source between read and heal Follow-up to ba29dc62ac, from a second multi-agent review. The previous commit fixed two review findings that turned out to interact: it made the on-disk heal id-aware (so TODAY keeps its own theme) and the read side only type-aware. Each was right alone; together they reintroduced the very read/write divergence the type-awareness was meant to remove, relocated from projects onto system entities. For a theme-less TODAY -- the active context at startup, and the entity in the report -- the read side rendered purple/hue-500/tinted while a later repair persisted cornflower/hue-400/untinted. Not a flicker: hydration validates without repairing, so a local-only user saw the wrong theme indefinitely, then it flipped the first time a sync repair ran. - extract getDefaultWorkContextTheme() into a leaf util both sides call, so the two cannot drift again. Net negative production lines. - drop IN_PROGRESS_TAG from the lookup: its theme differs from DEFAULT_TAG's only in backgroundImageDark ('' vs null), which isBackgroundImageSet treats identically, so the row was provably inert. URGENT/IMPORTANT stay -- board.component's createTags() makes them user-reachable and their primaries do differ. - drop 'theme' from RECREATE_FALLBACK requiredKeys: the recreate path already spreads defaults, so it only bought warn text at the cost of a four-clause doc paragraph. Tests: - replace the __proto__ test, which named __proto__ but used entity id 't1' and so never entered the guarded branch -- unfalsifiable, and it survived sabotaging the Map into a plain object literal. - buildCtx defaulted to id 'TODAY', quietly routing the generic-default cases through a system entity. - table-driven system-entity coverage that asserts each row is DISTINGUISHABLE from the generic default, so an inert row fails rather than lingering. - pin the branch's position in the else-if chain via its fix label. - nav-list fixture now omits `icon`, the fall-through its comment claims. Also scoped two over-claiming comments to what they actually guarantee, including a KNOWN RESIDUAL for lwwUpdateMetaReducer's recreate branch -- a third, id-blind writer left alone deliberately as sync-critical. Verified: shared-helper fix sabotage-tested, full suite green both TZ variants (13195/13181), and the real bundle confirmed end-to-end -- no circular import, migration succeeds, TODAY heals to #6495ED/hue-400. --- .../nav-list/nav-list-tree.component.spec.ts | 5 +- .../work-context-default-theme.util.ts | 45 ++++++++++++++++ .../work-context/work-context.service.spec.ts | 53 ++++++++++++++++--- .../work-context/work-context.service.ts | 17 +++--- .../op-log/core/recreate-fallback.const.ts | 13 +++-- .../validation/auto-fix-typia-errors.spec.ts | 31 ++++++----- .../validation/auto-fix-typia-errors.ts | 40 ++------------ 7 files changed, 130 insertions(+), 74 deletions(-) create mode 100644 src/app/features/work-context/work-context-default-theme.util.ts diff --git a/src/app/core-ui/magic-side-nav/nav-list/nav-list-tree.component.spec.ts b/src/app/core-ui/magic-side-nav/nav-list/nav-list-tree.component.spec.ts index 2b6f49f6ee..6c87c3a64f 100644 --- a/src/app/core-ui/magic-side-nav/nav-list/nav-list-tree.component.spec.ts +++ b/src/app/core-ui/magic-side-nav/nav-list/nav-list-tree.component.spec.ts @@ -50,10 +50,11 @@ describe('getProjectVisibilityIconColor', () => { // renders once per project in the side nav on every launch, and because // DEFAULT_PROJECT_ICON is not an emoji the theme branch is the DEFAULT // path — so an unguarded deref crashed the app at startup. - const project = createProject({ icon: 'work' }); + // No `icon` here on purpose: that is the fall-through the comment + // describes, so the fixture exercises the path it claims to. + const project = createProject({}); delete (project as unknown as Record).theme; - expect(() => getProjectVisibilityIconColor(project)).not.toThrow(); expect(getProjectVisibilityIconColor(project)).toBeNull(); }); }); diff --git a/src/app/features/work-context/work-context-default-theme.util.ts b/src/app/features/work-context/work-context-default-theme.util.ts new file mode 100644 index 0000000000..8c374f1dd2 --- /dev/null +++ b/src/app/features/work-context/work-context-default-theme.util.ts @@ -0,0 +1,45 @@ +import { WorkContextThemeCfg } from './work-context.model'; +import { DEFAULT_TAG, IMPORTANT_TAG, TODAY_TAG, URGENT_TAG } from '../tag/tag.const'; +import { DEFAULT_PROJECT, INBOX_PROJECT } from '../project/project.const'; + +/** + * System entities ship a *distinct* theme, so falling back to the generic + * default would silently restyle them — and where `auto-fix-typia-errors` + * persists the value, permanently. + * + * A Map, not an object literal: a hostile entity id of `__proto__` resolves to + * `Object.prototype` on a plain-object lookup, which is non-nullish and would + * therefore be spread into an empty theme instead of falling through. + * + * IN_PROGRESS_TAG is deliberately absent: its theme differs from DEFAULT_TAG's + * only in `backgroundImageDark` ('' vs null), and `isBackgroundImageSet` treats + * both as unset — so a row for it could not change any rendered output. + */ +const SYSTEM_ENTITY_THEMES: ReadonlyMap = new Map( + [TODAY_TAG, URGENT_TAG, IMPORTANT_TAG, INBOX_PROJECT].map( + (e) => [e.id, e.theme] as const, + ), +); + +/** + * The theme a work context should fall back to when it has none. + * + * Shared by the read side (`resolveContextTheme`) and the on-disk heal in + * `auto-fix-typia-errors`. Those two must not diverge: hydration validates + * without repairing, so a local-only user can render the read-side value for + * many sessions before a repair ever runs — and TODAY is the active context at + * startup (#9139). + * + * KNOWN RESIDUAL: `lwwUpdateMetaReducer`'s recreate branch is a third writer + * and does NOT route through here — it spreads `RECREATE_FALLBACK[type].defaults`, + * which is id-blind, so a system entity recreated from a partial LWW update + * still gets the generic theme persisted. Narrow (needs a delete-vs-update race + * on a system entity) and left alone deliberately: that file is sync-critical + * and deserves its own change with its own convergence analysis. + */ +export const getDefaultWorkContextTheme = ( + isTag: boolean, + entityId: string, +): WorkContextThemeCfg => + SYSTEM_ENTITY_THEMES.get(entityId) ?? + (isTag ? DEFAULT_TAG.theme : DEFAULT_PROJECT.theme); diff --git a/src/app/features/work-context/work-context.service.spec.ts b/src/app/features/work-context/work-context.service.spec.ts index 11b2db3572..06f1823295 100644 --- a/src/app/features/work-context/work-context.service.spec.ts +++ b/src/app/features/work-context/work-context.service.spec.ts @@ -2,7 +2,7 @@ import { TestBed } from '@angular/core/testing'; import { of } from 'rxjs'; import { resolveContextTheme, WorkContextService } from './work-context.service'; import { DEFAULT_TAG_COLOR, WORK_CONTEXT_DEFAULT_THEME } from './work-context.const'; -import { DEFAULT_PROJECT } from '../project/project.const'; +import { DEFAULT_PROJECT, INBOX_PROJECT } from '../project/project.const'; import { TaskWithSubTasks } from '../tasks/task.model'; import { MockStore, provideMockStore } from '@ngrx/store/testing'; import { provideMockActions } from '@ngrx/effects/testing'; @@ -13,7 +13,7 @@ import { GlobalTrackingIntervalService } from '../../core/global-tracking-interv import { DateService } from '../../core/date/date.service'; import { TimeTrackingService } from '../time-tracking/time-tracking.service'; import { TaskArchiveService } from '../archive/task-archive.service'; -import { DEFAULT_TAG, TODAY_TAG } from '../tag/tag.const'; +import { DEFAULT_TAG, IMPORTANT_TAG, TODAY_TAG, URGENT_TAG } from '../tag/tag.const'; import { WorkContext, WorkContextType } from './work-context.model'; import { selectActiveContextId, @@ -748,12 +748,15 @@ describe('WorkContextService - isTodayListSignal reactivity', () => { }); describe('resolveContextTheme()', () => { + // NOTE: a plain USER tag id by default. System entities (TODAY, INBOX, …) + // resolve to their own themes, so a system id here would silently stop these + // cases from exercising the generic default. const buildCtx = (over: Record = {}): WorkContext => ({ - id: 'TODAY', - title: 'Today', + id: 'user-tag-1', + title: 'User tag', type: WorkContextType.TAG, - routerLink: 'tag/TODAY', + routerLink: 'tag/user-tag-1', taskIds: [], noteIds: [], color: null, @@ -770,9 +773,10 @@ describe('resolveContextTheme()', () => { const ctx = buildCtx(); delete (ctx as unknown as Record).theme; - // Returns the shared constant by reference — deliberate: both consumers - // only read, and a stable identity helps the downstream - // distinctUntilChanged. `toBe` pins that decision. + // Returns the shared constant by reference. Deliberate, but note the + // reason is only that both consumers are read-only — NOT emission + // dedup: currentTheme$ uses distinctUntilChanged(isShallowEqual), which + // compares key-by-key with no reference short-circuit. expect(resolveContextTheme(ctx)).toBe(DEFAULT_TAG.theme); }); @@ -791,6 +795,39 @@ describe('resolveContextTheme()', () => { expect(resolveContextTheme(ctx)).toBe(DEFAULT_TAG.theme); }); + // Regression: the read side was only type-aware while the on-disk heal was + // id-aware, so a theme-less TODAY — the active context at startup — + // rendered purple/tinted indefinitely (hydration validates but never + // repairs) and then flipped once a sync repair landed. Both sides now + // resolve through getDefaultWorkContextTheme. + // + // Table-driven on purpose: asserting the property that makes each row + // exist (its theme is DISTINGUISHABLE from the generic default) means a + // row that is actually inert fails here rather than sitting unnoticed. + ( + [ + [TODAY_TAG, true], + [URGENT_TAG, true], + [IMPORTANT_TAG, true], + [INBOX_PROJECT, false], + ] as const + ).forEach(([entity, isTag]) => { + it(`gives ${entity.id} its own theme, not the generic default`, () => { + const ctx = buildCtx({ + id: entity.id, + type: isTag ? WorkContextType.TAG : WorkContextType.PROJECT, + }); + delete (ctx as unknown as Record).theme; + + const res = resolveContextTheme(ctx); + const generic = isTag ? DEFAULT_TAG.theme : DEFAULT_PROJECT.theme; + + expect(res).toBe(entity.theme); + // If this fails the row is inert and should be deleted, not kept. + expect(res.primary).not.toBe(generic.primary); + }); + }); + it('yields a COMPLETE theme when the tag-color fallback applies', () => { const ctx = buildCtx({ color: '#abcdef' }); delete (ctx as unknown as Record).theme; diff --git a/src/app/features/work-context/work-context.service.ts b/src/app/features/work-context/work-context.service.ts index 9d1b52142c..f3037cd893 100644 --- a/src/app/features/work-context/work-context.service.ts +++ b/src/app/features/work-context/work-context.service.ts @@ -26,9 +26,10 @@ import { take, withLatestFrom, } from 'rxjs/operators'; -import { DEFAULT_TAG, TODAY_TAG } from '../tag/tag.const'; +import { TODAY_TAG } from '../tag/tag.const'; import { Tag } from '../tag/tag.model'; import { DEFAULT_TAG_COLOR } from './work-context.const'; +import { getDefaultWorkContextTheme } from './work-context-default-theme.util'; import { TagService } from '../tag/tag.service'; import { ArchiveTask, Task, TaskWithSubTasks } from '../tasks/task.model'; import { @@ -70,7 +71,7 @@ import { getTimeSpentForDay } from './get-time-spent-for-day.util'; import { TimeTrackingService } from '../time-tracking/time-tracking.service'; import { updateWorkContextData } from '../time-tracking/store/time-tracking.actions'; import { TaskArchiveService } from '../archive/task-archive.service'; -import { DEFAULT_PROJECT, INBOX_PROJECT } from '../project/project.const'; +import { INBOX_PROJECT } from '../project/project.const'; import { selectProjectById } from '../project/store/project.selectors'; import { Project } from '../project/project.model'; import { Log } from '../../core/log'; @@ -88,19 +89,17 @@ import { LOCAL_ACTIONS } from '../../util/local-actions.token'; * work context. It is NOT an app-wide guarantee — code that iterates over all * projects/tags reads the raw entity and must still guard `theme?.` itself. * - * The default is type-aware so it matches what `auto-fix-typia-errors` would - * later persist for the same entity; otherwise a theme-less project rendered - * tag-purple until a repair ran and then flipped to project-teal. + * The fallback comes from `getDefaultWorkContextTheme`, shared with the on-disk + * heal, so what is rendered now and what a later repair persists cannot differ. */ export const resolveContextTheme = (awc: WorkContext): WorkContextThemeCfg => { - const theme = - awc.theme ?? - (awc.type === WorkContextType.TAG ? DEFAULT_TAG.theme : DEFAULT_PROJECT.theme); + const isTag = awc.type === WorkContextType.TAG; + const theme = awc.theme ?? getDefaultWorkContextTheme(isTag, awc.id); // For tags: theme.primary is the explicit override. If it's still at // the auto-default (or unset) and tag.color is set, fall back to // tag.color so newly created tags drive Material theming with their // randomized color while still letting users override explicitly. - if (awc.type === WorkContextType.TAG) { + if (isTag) { const tagColor = (awc as unknown as Tag).color; const primary = theme.primary; if (tagColor && (!primary || primary === DEFAULT_TAG_COLOR)) { diff --git a/src/app/op-log/core/recreate-fallback.const.ts b/src/app/op-log/core/recreate-fallback.const.ts index 949384afdb..c634b74296 100644 --- a/src/app/op-log/core/recreate-fallback.const.ts +++ b/src/app/op-log/core/recreate-fallback.const.ts @@ -19,11 +19,10 @@ import { EMPTY_SIMPLE_COUNTER } from '../../features/simple-counter/simple-count * `requiredKeys`. * - `requiredKeys` drives only (a) the meta-reducer's diagnostic warn (which * missing schema-required fields to name in the log) and (b) the per-type - * on-disk heal branch in `auto-fix-typia-errors.ts`. TASK and SIMPLE_COUNTER - * have a `requiredKeys`-driven branch. TAG/PROJECT have a narrower one - * covering `theme` only (#9139), which reads its defaults directly rather - * than through this table, so for them `requiredKeys` still feeds only the - * warn — `theme` is listed below so the warn names it. + * on-disk heal branch in `auto-fix-typia-errors.ts`. Only TASK and + * SIMPLE_COUNTER have a `requiredKeys`-driven branch; TAG/PROJECT have a + * `theme`-specific one (#9139) that reads its defaults directly, not through + * this table, so their `requiredKeys` feed only the warn. * List the schema-required fields that are NOT already coerced by an * earlier generic branch in `autoFixTypiaErrors` (booleans → false, * nullable → null) — mirroring TASK's curated list. @@ -75,8 +74,8 @@ export const RECREATE_FALLBACK: Partial> = 'projectId', ], }, - PROJECT: { defaults: DEFAULT_PROJECT, requiredKeys: ['title', 'taskIds', 'theme'] }, - TAG: { defaults: DEFAULT_TAG, requiredKeys: ['title', 'taskIds', 'theme'] }, + PROJECT: { defaults: DEFAULT_PROJECT, requiredKeys: ['title', 'taskIds'] }, + TAG: { defaults: DEFAULT_TAG, requiredKeys: ['title', 'taskIds'] }, SIMPLE_COUNTER: { defaults: EMPTY_SIMPLE_COUNTER, // Curated like TASK: omit `icon` (nullable, healed by the undefined→null diff --git a/src/app/op-log/validation/auto-fix-typia-errors.spec.ts b/src/app/op-log/validation/auto-fix-typia-errors.spec.ts index aa4636e0c1..455f4ded86 100644 --- a/src/app/op-log/validation/auto-fix-typia-errors.spec.ts +++ b/src/app/op-log/validation/auto-fix-typia-errors.spec.ts @@ -578,6 +578,14 @@ describe('autoFixTypiaErrors', () => { const result = autoFixTypiaErrors(mockData, errors); expect((result as any).tag.entities.t1.theme).toEqual(DEFAULT_TAG.theme); + // Pins the branch's POSITION in the else-if chain: one of the earlier + // `expected.includes('null'|'undefined')` branches swallowing this error + // would still leave state changed, but under a different fix label. + expect(errSpy).toHaveBeenCalledWith( + '[auto-fix-typia-errors] Applied validation auto-fix', + undefined, + jasmine.objectContaining({ fix: 'work-context-theme-undefined-to-default' }), + ); }); it('should restore the TODAY tag its own theme, not the generic tag default', () => { @@ -627,25 +635,22 @@ describe('autoFixTypiaErrors', () => { expect(healed.primary).not.toBe(DEFAULT_PROJECT.theme.primary); }); - it('should not treat a hostile "__proto__" entity id as a system theme', () => { - // The system-theme lookup is a Map precisely so this returns undefined - // rather than Object.prototype. + it('should not resolve a hostile "__proto__" entity id to a system theme', () => { + // The lookup is a Map for exactly this: an object literal would return + // Object.prototype here, which is non-nullish, so the `??` would not fall + // through and the entity would be healed to an empty theme. const mockData = createAppDataCompleteMock(); - (mockData as any).tag = { - ids: ['t1'], - entities: { t1: { id: 't1', title: 'x', taskIds: [] } }, - }; + const entities: Record = {}; + // Computed key => a genuine OWN property named __proto__, not a set prototype. + entities['__proto__'] = { id: '__proto__', title: 'x', taskIds: [] }; + (mockData as any).tag = { ids: ['__proto__'], entities }; const errors = [ - createTypiaError('$input.tag.entities.t1.theme', REAL_EXPECTED, undefined), + createTypiaError('$input.tag.entities.__proto__.theme', REAL_EXPECTED, undefined), ]; const result = autoFixTypiaErrors(mockData, errors); - expect((result as any).tag.entities.t1.theme).toEqual(DEFAULT_TAG.theme); - expect( - (result as any).tag.entities.t1.theme instanceof Object && - Object.getPrototypeOf((result as any).tag.entities.t1.theme), - ).toBe(Object.prototype); + expect((result as any).tag.entities['__proto__'].theme).toEqual(DEFAULT_TAG.theme); }); }); }); diff --git a/src/app/op-log/validation/auto-fix-typia-errors.ts b/src/app/op-log/validation/auto-fix-typia-errors.ts index 1c3e290d12..29b4d59f99 100644 --- a/src/app/op-log/validation/auto-fix-typia-errors.ts +++ b/src/app/op-log/validation/auto-fix-typia-errors.ts @@ -2,46 +2,14 @@ import { AppDataComplete } from '../model/model-config'; import { IValidation } from 'typia'; import type { SyncLogMeta } from '@sp/sync-core'; import { DEFAULT_GLOBAL_CONFIG } from '../../features/config/default-global-config.const'; -import { DEFAULT_PROJECT, INBOX_PROJECT } from '../../features/project/project.const'; -import { - DEFAULT_TAG, - IMPORTANT_TAG, - IN_PROGRESS_TAG, - TODAY_TAG, - URGENT_TAG, -} from '../../features/tag/tag.const'; -import { WorkContextThemeCfg } from '../../features/work-context/work-context.model'; +import { INBOX_PROJECT } from '../../features/project/project.const'; +import { getDefaultWorkContextTheme } from '../../features/work-context/work-context-default-theme.util'; import { RECREATE_FALLBACK } from '../core/recreate-fallback.const'; import { OP_LOG_SYNC_LOGGER } from '../core/sync-logger.adapter'; import { devError } from '../../util/dev-error'; const LOG_PREFIX = '[auto-fix-typia-errors]'; -/** - * System tags/projects ship a *distinct* theme, so healing them from the - * generic default would silently restyle them — and because the repair is - * written to disk, permanently. TODAY in particular is the entity from the - * #9139 report: it is cornflower with `huePrimary: '400'` and background tint - * disabled, where DEFAULT_TAG is purple with tint on. - * - * A Map, not an object literal: a hostile entity id of `__proto__` would hit - * `Object.prototype` on a plain-object lookup and be treated as a theme. - */ -const SYSTEM_ENTITY_THEMES: ReadonlyMap = new Map([ - [TODAY_TAG.id, TODAY_TAG.theme], - [URGENT_TAG.id, URGENT_TAG.theme], - [IMPORTANT_TAG.id, IMPORTANT_TAG.theme], - [IN_PROGRESS_TAG.id, IN_PROGRESS_TAG.theme], - [INBOX_PROJECT.id, INBOX_PROJECT.theme], -]); - -const getWorkContextDefaultTheme = ( - pathRoot: string | number, - entityId: string | number, -): WorkContextThemeCfg => - SYSTEM_ENTITY_THEMES.get(String(entityId)) ?? - (pathRoot === 'tag' ? DEFAULT_TAG.theme : DEFAULT_PROJECT.theme); - const getValueType = (value: unknown): string => { if (value === null) return 'null'; if (Array.isArray(value)) return 'array'; @@ -325,7 +293,9 @@ export const autoFixTypiaErrors = ( // the generated name `Readonly<__type>.oNN`, whose ordinal shifts // whenever the type graph changes. Keying on it would make this branch // silently stop firing. Path + nullish value is stable. - const theme = { ...getWorkContextDefaultTheme(keys[0], keys[2]) }; + const theme = { + ...getDefaultWorkContextTheme(keys[0] === 'tag', String(keys[2])), + }; setValueByPath(data, keys, theme); logAutoFixApplied( path,