mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-30 11:10:04 +00:00
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.
This commit is contained in:
Johannes Millan
parent
ba29dc62ac
commit
c62b2eb88f
7 changed files with 130 additions and 74 deletions
|
|
@ -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<string, unknown>).theme;
|
||||
|
||||
expect(() => getProjectVisibilityIconColor(project)).not.toThrow();
|
||||
expect(getProjectVisibilityIconColor(project)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, WorkContextThemeCfg> = 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);
|
||||
|
|
@ -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<string, unknown> = {}): 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<string, unknown>).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<string, unknown>).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<string, unknown>).theme;
|
||||
|
|
|
|||
|
|
@ -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)) {
|
||||
|
|
|
|||
|
|
@ -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<Record<EntityType, RecreateFallback>> =
|
|||
'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
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> = {};
|
||||
// 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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, WorkContextThemeCfg> = 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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue