Skip to content

add waveenv to builder app#3225

Merged
sawka merged 1 commit intomainfrom
sawka/fix-builder-window
Apr 16, 2026
Merged

add waveenv to builder app#3225
sawka merged 1 commit intomainfrom
sawka/fix-builder-window

Conversation

@sawka
Copy link
Copy Markdown
Member

@sawka sawka commented Apr 16, 2026

No description provided.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 16, 2026

Walkthrough

The pull request modifies the wave environment initialization pattern. In waveenvimpl.ts, imports are reordered to place the AllServiceImpls import after the global store import block. In builder-app.tsx, the component now instantiates waveEnvImpl once per component lifecycle using useRef(makeWaveEnvImpl()) and provides it through WaveEnvContext.Provider wrapping the inner builder component. React imports are expanded to include useRef, and the copyright year is updated.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to assess whether it relates to the changeset. Add a description explaining the purpose and context of adding waveenv to the builder app, even if brief.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'add waveenv to builder app' directly and clearly describes the main change: adding waveenv integration to the builder app component.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sawka/fix-builder-window

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
frontend/builder/builder-app.tsx (1)

65-65: Use lazy initialization for useRef to avoid unnecessary object recreation.

Line 65 eagerly evaluates makeWaveEnvImpl() on every render. Use the lazy-initialization pattern instead, initializing the ref with null and checking/setting it during render.

Suggested refactor
-    const waveEnvRef = useRef(makeWaveEnvImpl());
+    const waveEnvRef = useRef<ReturnType<typeof makeWaveEnvImpl> | null>(null);
+    if (waveEnvRef.current == null) {
+        waveEnvRef.current = makeWaveEnvImpl();
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/builder/builder-app.tsx` at line 65, The ref initialization eagerly
calls makeWaveEnvImpl() on every render; change waveEnvRef to use lazy init by
creating it with useRef(null) and then, inside the component render (or an
effect if appropriate), check if waveEnvRef.current is null and assign
waveEnvRef.current = makeWaveEnvImpl(); reference waveEnvRef and makeWaveEnvImpl
in your change and ensure subsequent uses read from waveEnvRef.current.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/builder/builder-app.tsx`:
- Line 65: The ref initialization eagerly calls makeWaveEnvImpl() on every
render; change waveEnvRef to use lazy init by creating it with useRef(null) and
then, inside the component render (or an effect if appropriate), check if
waveEnvRef.current is null and assign waveEnvRef.current = makeWaveEnvImpl();
reference waveEnvRef and makeWaveEnvImpl in your change and ensure subsequent
uses read from waveEnvRef.current.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b66bb643-59c6-44b9-bee0-4552884c63fc

📥 Commits

Reviewing files that changed from the base of the PR and between 97e5600 and 79fc942.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • frontend/app/waveenv/waveenvimpl.ts
  • frontend/builder/builder-app.tsx

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying waveterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 79fc942
Status: ✅  Deploy successful!
Preview URL: https://4e1f8705.waveterm.pages.dev
Branch Preview URL: https://sawka-fix-builder-window.waveterm.pages.dev

View logs

@sawka sawka merged commit 80cb181 into main Apr 16, 2026
9 checks passed
@sawka sawka deleted the sawka/fix-builder-window branch April 16, 2026 23:23
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