refactor(tui): load native V2 themes#38430
Conversation
| } | ||
|
|
||
| function decodeThemeFile(input: unknown, name: string) { | ||
| export function decodeThemeFile(input: unknown, name = "theme") { |
There was a problem hiding this comment.
why do you need the default here?
There was a problem hiding this comment.
Agreed, it was unnecessary. The exported decoder has been removed; V2 decoding now happens inside parseTheme(source, name), where the caller already provides the theme name and the schema error is decorated there.
| const value = Reflect.get(theme, "theme") | ||
| return typeof value === "object" && value !== null && !Array.isArray(value) | ||
| export function isThemeSource(source: unknown) { | ||
| if (!isRecord(source)) return false |
There was a problem hiding this comment.
don't use isRecord
just do some basic checks in here
don't worry about the version in here
There was a problem hiding this comment.
Updated. isThemeSource() now only performs basic object checks and looks for a V1 theme definition or a V2 light/dark mode. It no longer uses isRecord or inspects the version; version handling remains solely in parseTheme().
There was a problem hiding this comment.
Follow-up applied: V2 eligibility now checks only for the presence of version (V1 still checks theme), and the provider helper is now named loadTheme().
| if (!isTheme(theme)) return false | ||
| if (!isThemeSource(theme)) return false | ||
| if (hasTheme(name)) return false | ||
| invalidateTheme(theme) |
There was a problem hiding this comment.
What's will all these invalidateTheme calls?
There was a problem hiding this comment.
Removed all invalidateTheme() calls and the helper. The parse cache now relies solely on source object identity; replacing a source with a new object naturally produces a new cache entry. I also removed the mutation-specific test.
| return "version" in theme ? theme.version : 1 | ||
| } | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
get rid of this function
There was a problem hiding this comment.
Removed. isRecord() no longer exists; isThemeSource() now uses direct basic object checks, while parseTheme() delegates malformed V1 input to migrateV1().
| let systemTheme: ThemeJson | undefined | ||
| const listeners = new Set<(themes: Record<string, ThemeJson>) => void>() | ||
| const pluginThemes: Record<string, unknown> = {} | ||
| let customThemes: Record<string, unknown> = {} |
There was a problem hiding this comment.
instead of using unknown can we have something like ThemeDocumentSource which is the pre-decoded type? it would just be a generic JSON type
There was a problem hiding this comment.
Added ThemeDocumentSource = Record<string, unknown> for the pre-decoded object shape. Registry maps, listeners, provider state, loadTheme(), and parseTheme() now use it; raw discovery output is narrowed by isThemeSource() before entering the registry.
|
I overengineered the parse cache around an unsupported same-object mutation scenario and added unnecessary invalidation logic and test coverage. That complexity was not justified. I apologize for introducing the distraction; the invalidation code has been removed in |
|
I owe a clearer apology for this change. I introduced cache invalidation machinery for a speculative same-object mutation scenario that was neither required nor part of the intended contract. That was poor judgment: it made a straightforward identity cache harder to understand, added unnecessary branches and tests, and wasted review time. I am sincerely sorry. The code has been removed in |
1 participant
Summary
parseTheme()boundary that defaults missing versions to V1ThemeDocumentrepresentationresolveThemeDocument()ThemeJsontype toThemeV1Jsonand the V2ThemeFileschema/type toThemeDocumentVerification
bun typecheckgit diff --check