Conversation
|
Independent latest-head review at c696f5c found no actionable findings. Reviewer checked correlation, privacy bounds/clear semantics, transport operation guards, teardown attribution and preferred-focus cancellation ownership; 106 focused tests passed independently. Full local validation passed 594 tests and Expo Doctor21/21. Native CI remains in progress; no hardware reliability claim. |
Greptile SummarySummaryTwo diagnostic attribution issues remain:
These issues affect diagnostic accuracy only and do not block merging. Confidence Score: 4/5Safe to merge: the confirmed issues affect support diagnostics rather than connection behavior, authorization, or stored data. Focused executable checks reproduced two independent diagnostic attribution failures. Files Needing Attention: src/diagnostics/DiagnosticLog.ts and src/connection/ConnectionManager.ts
What T-Rex did
|
| if (desktopId.length <= 128) { | ||
| if (this.#pcs.size >= 128) this.#pcs.delete(this.#pcs.keys().next().value!); | ||
| this.#pcs.set(desktopId, pc); |
There was a problem hiding this comment.
Desktop IDs longer than 128 characters are never retained in the session alias map. Each later observation of the same peer therefore receives a new anonymous PC label, so exported diagnostics cannot correlate that peer's events. This is a non-blocking diagnostic-quality concern that makes support investigations less reliable.
Artifacts
- Authored Jest test that invokes the real DiagnosticLog path twice with one 129-character desktop ID and asserts a stable anonymous label; it is the executed reproducer.
- Captured Jest output from the focused reproducer shows the two observations received PC-2 and PC-3, proving correlation is broken.
Ran code and verified through T-Rex
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/diagnostics/DiagnosticLog.ts
Line: 74-76
Comment:
**Keep long IDs correlated**
Desktop IDs longer than 128 characters are never retained in the session alias map. Each later observation of the same peer therefore receives a new anonymous PC label, so exported diagnostics cannot correlate that peer's events. This is a non-blocking diagnostic-quality concern that makes support investigations less reliable.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return true; | ||
| } catch { | ||
| this.diagnostics.add('unpair_failed', 'warning'); | ||
| this.#record('unpair_failed', 'warning'); |
There was a problem hiding this comment.
A failed saved-PC removal is logged only after the asynchronous removal finishes, using the manager's current connection attempt. If another PC connection begins while removal is pending, the unpair failure is labeled as part of that unrelated connection. This is a non-blocking diagnostic-quality concern that can mislead support investigations.
Artifacts
- This focused test starts removal for the first saved PC, starts a connection to the second PC while removal is pending, rejects removal, and asserts the diagnostic record; it demonstrates the attribution race.
- This captured Jest run logs the observed `unpair_failed` record with the later connection attempt prefix and exits successfully; it confirms the misattribution.
Ran code and verified through T-Rex
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/connection/ConnectionManager.ts
Line: 229
Comment:
**Keep unpair context separate**
A failed saved-PC removal is logged only after the asynchronous removal finishes, using the manager's current connection attempt. If another PC connection begins while removal is pending, the unpair failure is labeled as part of that unrelated connection. This is a non-blocking diagnostic-quality concern that can mislead support investigations.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Closes #164
Summary
Validation