Skip to content

Commit 80b5734

Browse files
anandgupta42anandgupta42
committed
fix: address code review feedback on new user detection and telemetry
- use `~/.altimate/machine-id` existence for robust `is_upgrade` flag - fix 3-state logic in `isFirstTimeUser` memo to prevent suppressed beginner UI - prevent tip re-randomization on prop change in `tips.tsx` - add missing `first_launch` event to telemetry tests - remove unused import
1 parent e664d78 commit 80b5734

4 files changed

Lines changed: 54 additions & 42 deletions

File tree

‎packages/opencode/src/cli/cmd/tui/component/tips.tsx‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createMemo, createSignal, For } from "solid-js"
1+
import { createMemo, For } from "solid-js"
22
import { DEFAULT_THEMES, useTheme } from "@tui/context/theme"
33

44
const themeCount = Object.keys(DEFAULT_THEMES).length
@@ -50,9 +50,12 @@ const BEGINNER_TIPS = [
5050
// altimate_change start — first-time user beginner tips with reactive pool
5151
export function Tips(props: { isFirstTime?: boolean }) {
5252
const theme = useTheme().theme
53+
// Pick random tip index once on mount instead of recalculating randomly when props change
54+
// Use useMemo without dependencies so it only evaluates once
55+
const tipIndex = Math.random()
5356
const tip = createMemo(() => {
5457
const pool = props.isFirstTime ? BEGINNER_TIPS : TIPS
55-
return parse(pool[Math.floor(Math.random() * pool.length)])
58+
return parse(pool[Math.floor(tipIndex * pool.length)])
5659
})
5760

5861
return (

‎packages/opencode/src/cli/cmd/tui/routes/home.tsx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ export function Home() {
4141
// altimate_change start — fix race condition: don't show beginner UI until sessions loaded
4242
const isFirstTimeUser = createMemo(() => {
4343
// Don't evaluate until sessions have actually loaded (avoid flash of beginner UI)
44-
if (sync.status === "loading" || sync.status === "partial") return false
44+
// Return undefined to represent "loading" state
45+
if (sync.status === "loading" || sync.status === "partial") return undefined
4546
return sync.data.session.length === 0
4647
})
4748
// altimate_change end
@@ -133,7 +134,7 @@ export function Home() {
133134
/>
134135
</box>
135136
{/* altimate_change start — first-time onboarding hint */}
136-
<Show when={isFirstTimeUser()}>
137+
<Show when={isFirstTimeUser() === true}>
137138
<box width="100%" maxWidth={75} paddingTop={1} flexShrink={0}>
138139
<text>
139140
<span style={{ fg: theme.textMuted }}>Get started: </span>
@@ -152,7 +153,7 @@ export function Home() {
152153
<box height={4} minHeight={0} width="100%" maxWidth={75} alignItems="center" paddingTop={3} flexShrink={1}>
153154
<Show when={showTips()}>
154155
{/* altimate_change start — pass first-time flag for beginner tips */}
155-
<Tips isFirstTime={isFirstTimeUser()} />
156+
<Tips isFirstTime={isFirstTimeUser() === true} />
156157
{/* altimate_change end */}
157158
</Show>
158159
</box>

‎packages/opencode/src/cli/welcome.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,13 @@ export function showWelcomeBannerIfNeeded(): void {
3939
// Remove marker first to avoid showing twice even if display fails
4040
fs.unlinkSync(markerPath)
4141

42-
// altimate_change start — VERSION is already normalized (no "v" prefix)
43-
const currentVersion = Installation.VERSION
42+
// altimate_change start — use ~/.altimate/machine-id existence as a proxy for upgrade vs fresh install
43+
// Since postinstall.mjs always writes the current version to the marker file, we can't reliably
44+
// use installedVersion !== currentVersion for release builds. Instead, if machine-id exists,
45+
// they've run the CLI before.
46+
const machineIdPath = path.join(os.homedir(), ".altimate", "machine-id")
47+
const isUpgrade = fs.existsSync(machineIdPath)
4448
// altimate_change end
45-
const isUpgrade = installedVersion === currentVersion && installedVersion !== "local"
4649

4750
// altimate_change start — track first launch for new user counting (privacy-safe: only version + machine_id)
4851
Telemetry.track({
@@ -64,7 +67,7 @@ export function showWelcomeBannerIfNeeded(): void {
6467
const reset = "\x1b[0m"
6568
const bold = "\x1b[1m"
6669

67-
const v = `altimate-code v${currentVersion} installed`
70+
const v = `altimate-code v${installedVersion} installed`
6871
const lines = [
6972
"",
7073
" Get started:",

‎packages/opencode/test/telemetry/telemetry.test.ts‎

Lines changed: 38 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -231,8 +231,12 @@ describe("telemetry.event-types", () => {
231231
"warehouse_discovery",
232232
"warehouse_census",
233233
"core_failure",
234+
"first_launch",
235+
"skill_created",
236+
"skill_installed",
237+
"skill_removed",
234238
]
235-
expect(eventTypes.length).toBe(33)
239+
expect(eventTypes.length).toBe(37)
236240
})
237241
})
238242

@@ -352,6 +356,10 @@ describe("telemetry.naming-convention", () => {
352356
"warehouse_discovery",
353357
"warehouse_census",
354358
"core_failure",
359+
"first_launch",
360+
"skill_created",
361+
"skill_installed",
362+
"skill_removed",
355363
]
356364
for (const t of types) {
357365
expect(t).toMatch(/^[a-z][a-z0-9_]*$/)
@@ -418,8 +426,7 @@ describe("telemetry.parseConnectionString (indirect)", () => {
418426

419427
try {
420428
delete process.env.ALTIMATE_TELEMETRY_DISABLED
421-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
422-
"IngestionEndpoint=https://example.com"
429+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "IngestionEndpoint=https://example.com"
423430
await Telemetry.init()
424431

425432
Telemetry.track({
@@ -734,8 +741,7 @@ describe("telemetry.flush", () => {
734741

735742
try {
736743
delete process.env.ALTIMATE_TELEMETRY_DISABLED
737-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
738-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
744+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
739745
await Telemetry.init()
740746

741747
Telemetry.track({
@@ -770,8 +776,7 @@ describe("telemetry.flush", () => {
770776

771777
try {
772778
delete process.env.ALTIMATE_TELEMETRY_DISABLED
773-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
774-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
779+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
775780
await Telemetry.init()
776781

777782
Telemetry.track({
@@ -810,8 +815,7 @@ describe("telemetry.flush", () => {
810815

811816
try {
812817
delete process.env.ALTIMATE_TELEMETRY_DISABLED
813-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
814-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
818+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
815819
await Telemetry.init()
816820

817821
Telemetry.track({
@@ -858,8 +862,7 @@ describe("telemetry.flush", () => {
858862

859863
try {
860864
delete process.env.ALTIMATE_TELEMETRY_DISABLED
861-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
862-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
865+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
863866
await Telemetry.init()
864867

865868
Telemetry.track({
@@ -894,8 +897,7 @@ describe("telemetry.flush", () => {
894897

895898
try {
896899
delete process.env.ALTIMATE_TELEMETRY_DISABLED
897-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
898-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
900+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
899901
await Telemetry.init()
900902

901903
// Fill buffer beyond MAX_BUFFER_SIZE (200) to trigger drops
@@ -917,8 +919,8 @@ describe("telemetry.flush", () => {
917919
const envelopes = JSON.parse(fetchBodies[0])
918920
// Should include a TelemetryBufferOverflow error event
919921
const overflowEvent = envelopes.find(
920-
(e: any) => e.data?.baseData?.name === "error" &&
921-
e.data?.baseData?.properties?.error_name === "TelemetryBufferOverflow",
922+
(e: any) =>
923+
e.data?.baseData?.name === "error" && e.data?.baseData?.properties?.error_name === "TelemetryBufferOverflow",
922924
)
923925
expect(overflowEvent).toBeDefined()
924926
expect(overflowEvent.data.baseData.properties.error_message).toContain("10 events dropped")
@@ -970,8 +972,7 @@ describe("telemetry.shutdown", () => {
970972

971973
try {
972974
delete process.env.ALTIMATE_TELEMETRY_DISABLED
973-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
974-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
975+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
975976
await Telemetry.init()
976977

977978
Telemetry.track({
@@ -1005,8 +1006,7 @@ describe("telemetry.shutdown", () => {
10051006

10061007
try {
10071008
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1008-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1009-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1009+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
10101010
await Telemetry.init()
10111011

10121012
Telemetry.setContext({ sessionId: "sess-1", projectId: "proj-1" })
@@ -1060,8 +1060,7 @@ describe("telemetry.shutdown", () => {
10601060

10611061
try {
10621062
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1063-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1064-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1063+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
10651064
await Telemetry.init()
10661065
await Telemetry.shutdown()
10671066

@@ -1101,8 +1100,7 @@ describe("telemetry.buffer overflow", () => {
11011100

11021101
try {
11031102
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1104-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1105-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1103+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
11061104
await Telemetry.init()
11071105

11081106
// Track 250 events — first 50 should be dropped
@@ -1158,8 +1156,7 @@ describe("telemetry.buffer overflow", () => {
11581156

11591157
try {
11601158
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1161-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1162-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1159+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
11631160
await Telemetry.init()
11641161

11651162
// Exactly 205 events — 5 should be dropped
@@ -1211,8 +1208,7 @@ describe("telemetry.init with enabled telemetry", () => {
12111208

12121209
try {
12131210
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1214-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1215-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1211+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
12161212
await Telemetry.init()
12171213

12181214
// If flush timer is set up, tracking + waiting should eventually trigger flush
@@ -1316,8 +1312,7 @@ describe("telemetry.init with enabled telemetry", () => {
13161312

13171313
try {
13181314
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1319-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1320-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1315+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
13211316

13221317
const p1 = Telemetry.init()
13231318
const p2 = Telemetry.init()
@@ -1450,6 +1445,18 @@ describe("telemetry.memory", () => {
14501445
})
14511446
}).not.toThrow()
14521447
})
1448+
1449+
test("track accepts first_launch event without throwing", () => {
1450+
expect(() => {
1451+
Telemetry.track({
1452+
type: "first_launch",
1453+
timestamp: Date.now(),
1454+
session_id: "",
1455+
version: "0.5.9",
1456+
is_upgrade: false,
1457+
})
1458+
}).not.toThrow()
1459+
})
14531460
})
14541461

14551462
// ---------------------------------------------------------------------------
@@ -1483,8 +1490,7 @@ describe("Telemetry.isEnabled()", () => {
14831490
spyOn(global, "fetch").mockImplementation(async () => new Response("", { status: 200 }))
14841491
try {
14851492
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1486-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1487-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1493+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
14881494
await Telemetry.init()
14891495
expect(Telemetry.isEnabled()).toBe(true)
14901496
} finally {
@@ -1501,8 +1507,7 @@ describe("Telemetry.isEnabled()", () => {
15011507
spyOn(global, "fetch").mockImplementation(async () => new Response("", { status: 200 }))
15021508
try {
15031509
delete process.env.ALTIMATE_TELEMETRY_DISABLED
1504-
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
1505-
"InstrumentationKey=k;IngestionEndpoint=https://e.com"
1510+
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://e.com"
15061511
await Telemetry.init()
15071512
expect(Telemetry.isEnabled()).toBe(true)
15081513
await Telemetry.shutdown()

0 commit comments

Comments
 (0)