feat: support automated hw probe adoption#178
Conversation
WalkthroughReworks the AdoptProbe modal to add a hardware-adoption path with new guarded steps, timed manual-adoption fallback, probe discovery UI and lifecycle hooks. Adds a Pinia store Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/gp-dialog/content/AdoptProbe.vue (1)
291-291:⚠️ Potential issue | 🟡 MinorBack button navigates to wrong step.
The Back button on step 6 (Verify) navigates to step 4 (Find the probe), but should navigate to step 5 (Send adoption code) since that's the logical previous step in the manual adoption flow.
Fix navigation
- <Button class="mr-2" label="Back" severity="secondary" text `@click`="activateCallback('4')"/> + <Button class="mr-2" label="Back" severity="secondary" text `@click`="activateCallback('5')"/>
🧹 Nitpick comments (6)
app/composables/usePublicIp.ts (1)
10-12: Guard against falsy error values before logging.The watch callback logs even when
error.valueisnullorundefined(e.g., on initial state or if error clears). Add a truthiness check.Suggested fix
watch(error, () => { + if (error.value) { console.error('Error fetching client ip', error.value); + } });app/store/local-adoption.ts (2)
24-24: Inconsistent localStorage default values.Line 24 uses
Object.create(null)as default, but line 123 uses{}. This could lead to subtle inconsistencies. Use the same default throughout.Use consistent default
- ignoredTokens: useLocalStorage<Record<string, string>>(TOKEN_STORAGE_KEY, Object.create(null)).value, + ignoredTokens: useLocalStorage<Record<string, string>>(TOKEN_STORAGE_KEY, {}).value,
138-144: Address static analysis warning: forEach callback should not return a value.The
delete()method returns a boolean, which becomes the implicit return value of the arrow function. Use a block body to avoid this.Suggested fix
- tokensToRemove.forEach(t => this.adoptableProbes.delete(t)); + tokensToRemove.forEach((t) => { this.adoptableProbes.delete(t); });app/components/popup/UnadoptedProbeDetected.vue (2)
73-83: RedundantupdateCurrentProbe()call.
store.onConfirmAdoption()already callsupdateCurrentProbe()internally (seeapp/store/local-adoption.tsline 86). The explicit call on line 77 is redundant.Remove redundant call
try { const newProbe = await store.onConfirmAdoption(tokenToAdopt); newProbes.value = [ newProbe ]; successDialogOpen.value = true; - await store.updateCurrentProbe(); } catch (e) {
85-94: RedundantupdateCurrentProbe()call.
store.onRejectAdoption()already callsupdateCurrentProbe()internally (seeapp/store/local-adoption.tsline 94). The explicit call on line 93 is redundant.Remove redundant call
const onIgnoreClick = async () => { if (!activeProbe.value) { return; } const tokenToIgnore = activeProbe.value.token; store.onRejectAdoption(tokenToIgnore); - - await store.updateCurrentProbe(); };app/components/gp-dialog/content/AdoptProbe.vue (1)
444-460: Add guard for potential nullactiveProbe.While the template guards with
v-if="activeProbe", there's a theoretical race condition whereactiveProbecould become null between render and click handler execution. Using optional chaining with an early return is safer.Add defensive guard
const confirmHardwareAdoption = async () => { + if (!activeProbe.value) { + return; + } + try { - const newProbe = await store.onConfirmAdoption(activeProbe.value!.token); + const newProbe = await store.onConfirmAdoption(activeProbe.value.token);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/store/local-adoption.ts`:
- Around line 137-142: The expiry check on adoptableProbes uses a 60s threshold
that exactly matches the long-poll interval, causing a race where entries can be
removed before the next fetch; update the logic in the cleanup loop (the block
iterating this.adoptableProbes with variables fetchedAt, key and tokensToRemove)
to use a slightly larger threshold (e.g., 90 seconds or pollInterval + buffer)
by introducing a constant (e.g., ADOPTABLE_PROBE_TTL_MS or bufferMs) and
comparing now - fetchedAt >= ADOPTABLE_PROBE_TTL_MS so tokens are not
prematurely removed; adjust any related tests/comments to reflect the new TTL.
🧹 Nitpick comments (2)
app/store/local-adoption.ts (2)
138-144: Fix forEach callback to not return a value.The arrow function implicitly returns the boolean result of
Map.delete(). Per the static analysis hint, use a block body instead.Proposed fix
- tokensToRemove.forEach(t => this.adoptableProbes.delete(t)); + for (const t of tokensToRemove) { + this.adoptableProbes.delete(t); + }
24-24: Consider persisting a single localStorage ref.Extracting
.valueduring state initialization disconnects from the reactive localStorage reference. Methods likeignoreTokenandclearOldTokenscreate newuseLocalStorageinstances for each write, which works but is less efficient.If you want tighter sync, you could keep the ref and derive state from it. However, for this use case the current approach is functional.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/store/local-adoption.ts`:
- Around line 121-134: clearOldTokens prunes localStorage but doesn't update the
in-memory set, so expired tokens remain blocked for the session; after you
assign storage.value = validTokens in clearOldTokens, sync this.ignoredTokens to
match the pruned map (e.g., rebuild the Set from Object.keys(validTokens) or
otherwise remove any tokens not present in validTokens) so the in-memory state
reflects the 1-hour expiry window (refer to TOKEN_STORAGE_KEY, storage.value and
this.ignoredTokens inside clearOldTokens).
- Line 144: The forEach callback currently returns the boolean from Map.delete
(tokensToRemove.forEach(t => this.adoptableProbes.delete(t))) which triggers the
array-callback-return lint rule; change the callback to a block body so it
returns void (e.g., replace the concise arrow callback with a block-bodied arrow
or use a for...of loop) when iterating tokensToRemove and calling
this.adoptableProbes.delete(t) to ensure no value is returned from the callback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/store/local-adoption.ts`:
- Around line 82-95: Both handlers call the async method updateCurrentProbe()
without awaiting, so change onConfirmAdoption and onRejectAdoption to await
updateCurrentProbe(): keep onConfirmAdoption async (it already awaits
adoptProbe) but add await this.updateCurrentProbe() before returning newProbe;
change onRejectAdoption to async and replace this.updateCurrentProbe() with
await this.updateCurrentProbe(); keep the existing adoptableProbes.delete(token)
and this.ignoreToken(token) calls unchanged.
2 participants
Closes #134
Requires that the PRs in the globalping, globalping-probe, and globalping-dash-directus repos be merged first.