Skip to content

fix issue with an undefined this in service code#3048

Merged
sawka merged 2 commits intomainfrom
sawka/emergency-onboarding-fix
Mar 12, 2026
Merged

fix issue with an undefined this in service code#3048
sawka merged 2 commits intomainfrom
sawka/emergency-onboarding-fix

Conversation

@sawka
Copy link
Member

@sawka sawka commented Mar 12, 2026

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a3687444-7e03-4a3f-9f3f-2e6119892d43

📥 Commits

Reviewing files that changed from the base of the PR and between 5facc78 and 654a38e.

📒 Files selected for processing (3)
  • frontend/app/onboarding/onboarding-common.tsx
  • frontend/app/onboarding/onboarding-upgrade-patch.tsx
  • frontend/app/onboarding/onboarding-upgrade-v0142.tsx
✅ Files skipped from review due to trivial changes (1)
  • frontend/app/onboarding/onboarding-upgrade-v0142.tsx

Walkthrough

The PR makes small, focused updates across the frontend and codegen: it changes two onboarding call sites so AgreeTos is invoked via a fireAndForget wrapper and telemetryUpdateFn is wrapped inline; bumps the onboarding CURRENT version string from v0.14.2 to v0.14.3 and updates the upgrade sequence and modal text to note a v0.14.3 patch/bugfix; and adds optional chaining for waveEnv access (this?.waveEnv) in multiple frontend service implementations and in the tsgen code generator output. No public API signatures were modified.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No pull request description was provided by the author. Add a description explaining the issue being fixed, why undefined 'this' was problematic, and how the optional chaining approach resolves it.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix issue with an undefined this in service code' accurately reflects the main changes, which involve adding optional chaining (this?.waveEnv) to handle potentially undefined context throughout the service files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawka/emergency-onboarding-fix
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 12, 2026

Deploying waveterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 654a38e
Status: ✅  Deploy successful!
Preview URL: https://fa4f279a.waveterm.pages.dev
Branch Preview URL: https://sawka-emergency-onboarding-f.waveterm.pages.dev

View logs

@kilo-code-bot
Copy link
Contributor

kilo-code-bot bot commented Mar 12, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Overview

The PR addresses legitimate this binding issues that would cause runtime errors when service methods are passed as callbacks without proper context binding.

Changes Analysis

File Change Assessment
frontend/app/onboarding/onboarding.tsx Fixed fireAndForget(services.ClientService.AgreeTos)fireAndForget(() => services.ClientService.AgreeTos()) Correct fix - Original passed method reference which loses this binding
frontend/app/onboarding/onboarding.tsx Fixed telemetryUpdateFn={services.ClientService.TelemetryUpdate}telemetryUpdateFn={(value) => ...} Correct fix - Prop callbacks need bound methods
frontend/app/store/services.ts Changed this.waveEnvthis?.waveEnv across all services Correct defensive change - Handles edge cases where methods are extracted as callbacks
pkg/tsgen/tsgen.go Updated code generator to output this?.waveEnv Correct - Ensures generated code is consistent

Technical Details

The callBackendService function already handles null/undefined waveEnv by falling back to WOS.callBackendService, so the this?.waveEnv change provides defense-in-depth for cases where this might be undefined when methods are called as callbacks.

Files Reviewed (3 files)
  • frontend/app/onboarding/onboarding.tsx - 2 changes
  • frontend/app/store/services.ts - 25 changes
  • pkg/tsgen/tsgen.go - 1 change

@sawka sawka merged commit cdb300a into main Mar 12, 2026
8 checks passed
@sawka sawka deleted the sawka/emergency-onboarding-fix branch March 12, 2026 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant