fix(theme): surface legacy syntax translation#574
Conversation
Greptile SummaryThis PR surfaces a transient footer warning when deprecated
Confidence Score: 3/5Safe to merge for the legacy-notice feature itself, but the empty-array allocation in config.ts will silently erase any in-progress async update notice whenever a session reloads. The core notice queue, dedup, and sequencing logic in useStartupNotices is well-tested. The issue is in config.ts: startupNotices is always a freshly allocated array (even the empty case returns []), so every reloadSession call in AppHost gives the hook a new notices reference, tearing down the current effect, calling setNoticeText(null), and marking the in-flight update notice key in shownKeysRef as already seen — permanently suppressing it for the rest of the session. On a busy workflow where diffs refresh frequently, users could consistently lose the update-available banner mid-display and never see it again until they restart the process. src/core/config.ts line 570 — the startupNotices return value; src/ui/hooks/useStartupNotices.ts — the persistent shownKeysRef interaction with effect restarts. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Config as config.ts
participant Startup as startup.ts
participant AppHost as AppHost.tsx
participant Hook as useStartupNotices
participant Footer as App Footer
Config->>Config: resolveConfiguredCliInput()
Config->>Config: readCustomTheme() → usesLegacySyntax flag
Config-->>Startup: "HunkConfigResolution { startupNotices: [] or [LEGACY_NOTICE] }"
Startup->>Startup: prepareStartupPlan()
Startup-->>AppHost: "AppBootstrap { startupNotices }"
AppHost->>Hook: "useStartupNotices({ notices: activeBootstrap.startupNotices, resolver })"
Hook->>Hook: enqueue local notices immediately
Hook->>Hook: "setTimeout(resolver, delayMs=1200ms)"
Hook-->>Footer: noticeText (legacy notice or null)
Note over Hook,Footer: After 1200ms
Hook->>Hook: resolver() → UpdateNotice?
Hook->>Hook: enqueue if key not in shownKeysRef
Hook-->>Footer: noticeText (update notice)
Note over AppHost,Hook: On session reload
AppHost->>AppHost: reloadSession() → setActiveBootstrap(nextBootstrap)
AppHost->>Hook: notices changes reference (new [] each time)
Hook->>Hook: effect cleanup + restart
Hook->>Hook: setNoticeText(null) — clears any visible notice
Hook->>Hook: shownKeysRef keeps all previously seen keys
Hook->>Hook: enqueue local notices (skipped if key in shownKeysRef)
Hook->>Hook: setTimeout(resolver, delayMs) — restarts delay
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Config as config.ts
participant Startup as startup.ts
participant AppHost as AppHost.tsx
participant Hook as useStartupNotices
participant Footer as App Footer
Config->>Config: resolveConfiguredCliInput()
Config->>Config: readCustomTheme() → usesLegacySyntax flag
Config-->>Startup: "HunkConfigResolution { startupNotices: [] or [LEGACY_NOTICE] }"
Startup->>Startup: prepareStartupPlan()
Startup-->>AppHost: "AppBootstrap { startupNotices }"
AppHost->>Hook: "useStartupNotices({ notices: activeBootstrap.startupNotices, resolver })"
Hook->>Hook: enqueue local notices immediately
Hook->>Hook: "setTimeout(resolver, delayMs=1200ms)"
Hook-->>Footer: noticeText (legacy notice or null)
Note over Hook,Footer: After 1200ms
Hook->>Hook: resolver() → UpdateNotice?
Hook->>Hook: enqueue if key not in shownKeysRef
Hook-->>Footer: noticeText (update notice)
Note over AppHost,Hook: On session reload
AppHost->>AppHost: reloadSession() → setActiveBootstrap(nextBootstrap)
AppHost->>Hook: notices changes reference (new [] each time)
Hook->>Hook: effect cleanup + restart
Hook->>Hook: setNoticeText(null) — clears any visible notice
Hook->>Hook: shownKeysRef keeps all previously seen keys
Hook->>Hook: enqueue local notices (skipped if key in shownKeysRef)
Hook->>Hook: setTimeout(resolver, delayMs) — restarts delay
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/core/config.ts:570
**New array reference on every config resolution breaks notice dedup after reload**
`resolveConfiguredCliInput` always returns a freshly-allocated array for `startupNotices` — either `[LEGACY_CUSTOM_SYNTAX_NOTICE]` or `[]`. `AppHost` passes this directly as the `notices` prop to `useStartupNotices`. Because the reference changes on every `reloadSession` call (even when the array is logically empty), the hook's `[notices]` dependency fires on each reload: the current effect is torn down, `setNoticeText(null)` immediately erases any visible async update notice, and `shownKeysRef` prevents it from being re-queued for the rest of the session.
Use `undefined` for the no-notice case so the reference is stable across reloads. `AppBootstrap.startupNotices` is already typed as optional, and `useStartupNotices` defaults to `EMPTY_STARTUP_NOTICES` when the prop is `undefined`, so no other changes are needed.
### Issue 2 of 2
src/ui/hooks/useStartupNotices.ts:28
**`shownKeysRef` is never cleared on effect restart — one-shot per process lifetime**
`shownKeysRef` is a `useRef` that accumulates shown keys for the lifetime of the `AppHost` component. When `notices` or `resolver` changes and the effect restarts (e.g., on a session reload that keeps the legacy syntax), any key already in `shownKeysRef.current` is permanently suppressed. For the update-check path this matches the old `lastShownKeyRef` behavior. For the new local `notices` path it means the deprecated-syntax warning is shown at most once per process lifetime — worth confirming that is the intended UX.
Reviews (1): Last reviewed commit: "fix(theme): surface legacy syntax transl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| options: resolvedOptions, | ||
| }, | ||
| customTheme: resolvedCustomTheme, | ||
| startupNotices: usesLegacyCustomSyntax ? [LEGACY_CUSTOM_SYNTAX_NOTICE] : [], |
There was a problem hiding this comment.
resolveConfiguredCliInput always returns a freshly-allocated array for startupNotices — either [LEGACY_CUSTOM_SYNTAX_NOTICE] or []. AppHost passes this directly as the notices prop to useStartupNotices. Because the reference changes on every reloadSession call (even when the array is logically empty), the hook's [notices] dependency fires on each reload: the current effect is torn down, setNoticeText(null) immediately erases any visible async update notice, and shownKeysRef prevents it from being re-queued for the rest of the session.
Use undefined for the no-notice case so the reference is stable across reloads. AppBootstrap.startupNotices is already typed as optional, and useStartupNotices defaults to EMPTY_STARTUP_NOTICES when the prop is undefined, so no other changes are needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/config.ts
Line: 570
Comment:
**New array reference on every config resolution breaks notice dedup after reload**
`resolveConfiguredCliInput` always returns a freshly-allocated array for `startupNotices` — either `[LEGACY_CUSTOM_SYNTAX_NOTICE]` or `[]`. `AppHost` passes this directly as the `notices` prop to `useStartupNotices`. Because the reference changes on every `reloadSession` call (even when the array is logically empty), the hook's `[notices]` dependency fires on each reload: the current effect is torn down, `setNoticeText(null)` immediately erases any visible async update notice, and `shownKeysRef` prevents it from being re-queued for the rest of the session.
Use `undefined` for the no-notice case so the reference is stable across reloads. `AppBootstrap.startupNotices` is already typed as optional, and `useStartupNotices` defaults to `EMPTY_STARTUP_NOTICES` when the prop is `undefined`, so no other changes are needed.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 37b30ab. Config resolution now returns undefined when there is no local notice and reuses a singleton array when legacy syntax is present, so unchanged reloads preserve the notice queue in both cases. The config tests now assert those identity semantics.
Responded by Pi using OpenAI GPT-5.3.
This comment was generated by Pi using OpenAI GPT-5.3
| resolver, | ||
| }: StartupNoticeOptions) { | ||
| const [noticeText, setNoticeText] = useState<string | null>(null); | ||
| const shownKeysRef = useRef(new Set<string>()); |
There was a problem hiding this comment.
shownKeysRef is never cleared on effect restart — one-shot per process lifetime
shownKeysRef is a useRef that accumulates shown keys for the lifetime of the AppHost component. When notices or resolver changes and the effect restarts (e.g., on a session reload that keeps the legacy syntax), any key already in shownKeysRef.current is permanently suppressed. For the update-check path this matches the old lastShownKeyRef behavior. For the new local notices path it means the deprecated-syntax warning is shown at most once per process lifetime — worth confirming that is the intended UX.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/hooks/useStartupNotices.ts
Line: 28
Comment:
**`shownKeysRef` is never cleared on effect restart — one-shot per process lifetime**
`shownKeysRef` is a `useRef` that accumulates shown keys for the lifetime of the `AppHost` component. When `notices` or `resolver` changes and the effect restarts (e.g., on a session reload that keeps the legacy syntax), any key already in `shownKeysRef.current` is permanently suppressed. For the update-check path this matches the old `lastShownKeyRef` behavior. For the new local `notices` path it means the deprecated-syntax warning is shown at most once per process lifetime — worth confirming that is the intended UX.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Confirmed as intentional: startup notices are one-shot for the lifetime of AppHost, matching the existing update-notice policy, so config/session reloads do not replay a warning the user already saw. I documented that policy beside shownKeysRef and tightened the restart test to assert the local warning appears exactly once.
Responded by Pi using OpenAI GPT-5.3.
This comment was generated by Pi using OpenAI GPT-5.3
7830670 to
e37a77e
Compare
1 participant
Summary
[custom_theme.syntax]colors are translated into approximate Shiki scopesNotice
Testing
bun run typecheckbun run lintbun run format:checkbun run test:tty-smoke(9 passed)bun test: 1,226 passed, 14 skipped; only the existing direct-file atomic-save watch test failed, matching the known failure documented on feat(theme): support raw Shiki syntax scopes #570bun run test:integration: 54 passed; the same direct-file atomic-save watch test failedDependency
#570 introduced the legacy syntax translation this notice reports and is now merged.
This PR description was generated by Pi using OpenAI GPT-5.3