Conversation
WalkthroughRenamed and extended FlakeHub login telemetry events in src/index.ts and instrumented the flakehubLogin flow to emit start, success, and end events across all paths, replacing the prior single event. Adjusted control flow to ensure end telemetry is consistently recorded on success, fork, misconfiguration, and failure. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as Action (src/index.ts)
participant FH as FlakeHub
participant T as Telemetry
U->>A: Trigger flakehubLogin
A->>T: record EVENT_LOGIN_START
A->>FH: Attempt login
alt Success
FH-->>A: Auth OK
A->>T: record EVENT_LOGIN_SUCCESS
A->>T: record EVENT_LOGIN_END
A-->>U: Return success
else Fork or Not Configured
FH-->>A: Fork/Misconfig path
A->>T: record failure (existing) + EVENT_LOGIN_END
A-->>U: Return handled path
else Failure
FH-->>A: Error
A->>T: record failure (existing)
A->>T: record EVENT_LOGIN_END
A-->>U: Return error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 (3)
src/index.ts (3)
23-23: Telemetry bug: install success constant is misnamed.
EVENT_INSTALL_NIX_SUCCESSequals"install_nix_start"causing start and success to be indistinguishable in analytics.Apply this diff:
-const EVENT_INSTALL_NIX_SUCCESS = "install_nix_start"; +const EVENT_INSTALL_NIX_SUCCESS = "install_nix_success";
287-289: Wrong env value: writes user ID base from count.
NIX_INSTALLER_NIX_BUILD_USER_ID_BASEshould usenixBuildUserBase, notnixBuildUserCount.Apply this diff:
- executionEnv.NIX_INSTALLER_NIX_BUILD_USER_ID_BASE = `${this.nixBuildUserCount}`; + executionEnv.NIX_INSTALLER_NIX_BUILD_USER_ID_BASE = `${this.nixBuildUserBase}`;
143-148: Start time recorded after install/login — events may be excluded from summary.
STATE_START_DATETIMEis saved afterinstall()finishes. Any events emitted during install (including the FlakeHub funnel) won’t be included ingetRecentEvents(startDate).Apply this diff to capture all events:
async main(): Promise<void> { - await this.scienceDebugFly(); - await this.detectAndForceNoSystemd(); - await this.install(); - actionsCore.saveState(STATE_START_DATETIME, new Date().toISOString()); + actionsCore.saveState(STATE_START_DATETIME, new Date().toISOString()); + await this.scienceDebugFly(); + await this.detectAndForceNoSystemd(); + await this.install(); }
🧹 Nitpick comments (1)
src/index.ts (1)
766-767: Option: use finally for end/group.A
finallyguard makes the end event andendGroup()robust to any early throws.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/index.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
dist/index.js (3)
head(100506-100506)base(100505-100505)e(92944-92947)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test: macos-14-large with determinate
- GitHub Check: Test: macos-13-large with determinate
- GitHub Check: Test: macos-14-xlarge with determinate
- GitHub Check: Test: macos-14-large
- GitHub Check: Test: macos-13-large
- GitHub Check: Test: macos-14-xlarge
- GitHub Check: Test: namespace-profile-default-arm64
- GitHub Check: Test: ubuntu-latest
- GitHub Check: Test: namespace-profile-default-arm64 with determinate
- GitHub Check: Test: nscloud-ubuntu-22.04-amd64-4x16
🔇 Additional comments (5)
src/index.ts (5)
28-31: Event naming for FlakeHub funnel looks good.start/success/end are consistently declared and scoped to FlakeHub.
721-722: Emit start at entry — LGTM.Recording
flakehub-login:startat the top aligns with the funnel.
734-735: Ensure end on fork path — LGTM.Failure + end and early return prevent double-end later.
743-744: Ensure end on misconfiguration path — LGTM.Failure + end before return is correct.
757-757: Don’t assume @actions/exec throws on non-zero exit — record failure on exitCode ≠ 0.
@actions/exec.execreturns an exit code; it won’t throw on failure by default. Current code will emit “success” even if the login command exits non-zero. Handle both non-zero exit and exceptions.Apply this diff to make success/failure accurate and guarantee end/group closure:
- actionsCore.startGroup("Logging in to FlakeHub"); - try { - await actionsExec.exec(`determinate-nixd`, ["login", "github-action"]); - this.recordEvent(EVENT_LOGIN_SUCCESS); - } catch (e: unknown) { - actionsCore.warning(`FlakeHub Login failure: ${stringifyError(e)}`); - this.recordEvent(EVENT_LOGIN_FAILURE, { - reason: "failed", - exception: stringifyError(e), - }); - } - - this.recordEvent(EVENT_LOGIN_END); - actionsCore.endGroup(); + actionsCore.startGroup("Logging in to FlakeHub"); + try { + const exitCode = await actionsExec.exec("determinate-nixd", [ + "login", + "github-action", + ]); + if (exitCode === 0) { + this.recordEvent(EVENT_LOGIN_SUCCESS); + } else { + this.recordEvent(EVENT_LOGIN_FAILURE, { + reason: "exit-code", + exitCode, + }); + } + } catch (e: unknown) { + actionsCore.warning(`FlakeHub Login failure: ${stringifyError(e)}`); + this.recordEvent(EVENT_LOGIN_FAILURE, { + reason: "exception", + exception: stringifyError(e), + }); + } finally { + this.recordEvent(EVENT_LOGIN_END); + actionsCore.endGroup(); + }Likely an incorrect or invalid review comment.
Description
This PR restructures FlakeHub login events:
startevent.successorfailure.endis recorded.As a future maintenance note: we need to make sure that
endis recorded in all code paths.Checklist
Summary by CodeRabbit
Refactor
Chores