Conversation
- Centralizes dismissible UI and onboarding logic into a new, versioned service - Replaces disjointed storage keys with a type-safe registry that supports automatic schema migrations and item re-showing - Simplifies onboarding state management across core logic and webview components - Removes legacy per-item storage flags in favor of a unified `onboarding:state` storage structure
eamodio
force-pushed
the
feature/onboarding-service
branch
from
d0428b3 to
1fb369a
Compare
🤖 Augment PR SummarySummary: This PR introduces a centralized onboarding/dismissible-state service to replace scattered “dismissed” storage keys and command-based storage writes. Changes:
Technical Notes: Adds a small 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if (args.id in onboardingDefinitions) { | ||
| void this.dismiss(args.id); | ||
| } else { | ||
| debugger; |
There was a problem hiding this comment.
debugger; is left in the gitlens.onboarding.dismiss command handler; if a webview (or a bad link) sends an unknown id, this will pause execution for end users and can break automated environments.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| }), | ||
| ); | ||
|
|
||
| void this.migrateLegacyState().then(() => this._ready.fulfill(undefined)); |
There was a problem hiding this comment.
migrateLegacyState() is launched without any rejection handling; if it throws, the rejection can be unhandled and ready will never resolve, leaving onboarding state potentially stuck for the session.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| /** Get item state, running migrations if needed */ | ||
| getItemState<T extends OnboardingKeys>(key: T): OnboardingItemState<T> | undefined { | ||
| const item = this.getItem(key); | ||
| if (!item?.state) return undefined; |
There was a problem hiding this comment.
if (!item?.state) return undefined; treats valid falsy states (e.g. 0, false, '') as “missing”, which could break onboarding items whose state isn’t an object.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| // Persist migrated data so we don't re-migrate next time | ||
| void this.setItemStateCore(key, state, scope, currentSchema); |
There was a problem hiding this comment.
void this.setItemStateCore(...) can reject and become an unhandled promise rejection (no await/.catch()), which is especially risky since it runs during reads (getItemState) and could surface as noisy runtime errors if storage fails.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| export function fromVersion(v: Version, includePre: false): `${number}.${number}.${number}`; | ||
| export function fromVersion(v: Version, includePre?: boolean): `${number}.${number}.${number}${string | undefined}`; |
There was a problem hiding this comment.
The fromVersion overload return type includes ${string | undefined}, but the implementation always appends either '' or -${v.pre}—never undefined. This can make downstream typing misleading (e.g. suggesting an undefined suffix needs handling when it can’t occur).
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
No description provided.