Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughThis PR introduces a startup-wide window-type module at frontend/app/store/windowtype.ts (getWaveWindowType, isBuilderWindow, isTabWindow, isPreviewWindow, setWaveWindowType) and replaces in-file and atom-based window-type checks with these helper predicates across the frontend. It sets the preview window type during preview initialization, removes the prior waveWindowType atom and related exports, and updates key event/refocus and AI panel code to use the new predicates. UI changes add Tailwind layout classes to main containers, remove public/style.scss, and adjust link styling classes in multiple components. Several documentation files (.kilocode/.roo) and overview docs are added or expanded. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
.kilocode/rules/rules.md
Outdated
| - Use string constants for status values, packet types, and other string-based enumerations. | ||
| - in Go code, prefer using Printf() vs Println() | ||
| - use "Make" as opposed to "New" for struct initialization func names | ||
| - in general const decls go at the top fo the file (before types and functions) |
Contributor
There was a problem hiding this comment.
WARNING: Typo in documentation
"fo the file" should be "of the file"
Suggested change
| - in general const decls go at the top fo the file (before types and functions) | |
| - in general const decls go at the top of the file (before types and functions) |
Contributor
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Review NotesThis PR refactors the window type detection system by:
The refactoring improves code organization and makes window type checks more explicit and maintainable. All changes follow the project's coding guidelines. Note: There are existing inline comments on this PR that were posted previously. No new issues were found during this review. Files Reviewed (13 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/app/store/windowtype.ts (1)
5-25: Enforce the “set once at startup” contract in code.Right now
waveWindowTypedefaults to"tab"and can be overwritten later. A small guard would make init-order mistakes fail fast instead of silently behaving as tab.♻️ Proposed hardening
-// waveWindowType is set once at startup and never changes. -let waveWindowType: "tab" | "builder" | "preview" = "tab"; +// waveWindowType is set once at startup and never changes. +let waveWindowType: "tab" | "builder" | "preview" | null = null; function getWaveWindowType(): "tab" | "builder" | "preview" { + if (waveWindowType == null) { + throw new Error("waveWindowType accessed before initialization"); + } return waveWindowType; } function isBuilderWindow(): boolean { - return waveWindowType === "builder"; + return getWaveWindowType() === "builder"; } function isTabWindow(): boolean { - return waveWindowType === "tab"; + return getWaveWindowType() === "tab"; } function isPreviewWindow(): boolean { - return waveWindowType === "preview"; + return getWaveWindowType() === "preview"; } function setWaveWindowType(windowType: "tab" | "builder" | "preview") { + if (waveWindowType != null && waveWindowType !== windowType) { + throw new Error(`waveWindowType already initialized as "${waveWindowType}"`); + } waveWindowType = windowType; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/store/windowtype.ts` around lines 5 - 25, Change waveWindowType to be initially unset (e.g., let waveWindowType: "tab" | "builder" | "preview" | undefined = undefined) and enforce a one-time initialization: update setWaveWindowType to throw an error if waveWindowType is already set (preventing subsequent overwrites), and update getWaveWindowType, isBuilderWindow, isTabWindow, and isPreviewWindow to throw if waveWindowType is still undefined (failing fast on incorrect init order) so callers must call setWaveWindowType exactly once at startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.kilocode/rules/rules.md:
- Around line 65-69: The fenced code block containing the snippet with getApi()
and getIsDev() should include a language identifier to satisfy markdown linting
and improve readability; update the triple-backtick fence for the block that
wraps "import { getApi } from \"@/store/global\";" and "getApi().getIsDev()" to
use a TypeScript tag (```ts) so the code is highlighted properly.
- Line 1: Fix the typos and grammar in the contributor rules text by updating
the sentence "Wave Terminal is a modern terminal which provides graphical
blocks, dynamic layout, workspaces, and SSH connection management. It is cross
platform and built on electron." to use "cross-platform" and capitalize
"Electron", and scan the document for the reported misspellings and casing
issues—replace "fo" with "for", lowercase "react" with "React", and change
"overn" to the intended word (e.g., "overnight" or "over") wherever they appear
(notably near the shown sentence and the other instances referenced).
---
Nitpick comments:
In `@frontend/app/store/windowtype.ts`:
- Around line 5-25: Change waveWindowType to be initially unset (e.g., let
waveWindowType: "tab" | "builder" | "preview" | undefined = undefined) and
enforce a one-time initialization: update setWaveWindowType to throw an error if
waveWindowType is already set (preventing subsequent overwrites), and update
getWaveWindowType, isBuilderWindow, isTabWindow, and isPreviewWindow to throw if
waveWindowType is still undefined (failing fast on incorrect init order) so
callers must call setWaveWindowType exactly once at startup.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.kilocode/rules/rules.mdfrontend/app/aipanel/aipanel.tsxfrontend/app/aipanel/waveai-model.tsxfrontend/app/app.scssfrontend/app/element/markdown.tsxfrontend/app/element/streamdown.tsxfrontend/app/onboarding/onboarding.tsxfrontend/app/store/global-atoms.tsfrontend/app/store/keymodel.tsfrontend/app/store/windowtype.tsfrontend/preview/index.htmlfrontend/preview/preview.tsxfrontend/types/custom.d.tsindex.htmlpublic/style.scsstsunami/frontend/src/element/markdown.tsx
💤 Files with no reviewable changes (2)
- frontend/app/app.scss
- public/style.scss
| @@ -0,0 +1,198 @@ | |||
| Wave Terminal is a modern terminal which provides graphical blocks, dynamic layout, workspaces, and SSH connection management. It is cross platform and built on electron. | |||
Contributor
There was a problem hiding this comment.
Fix typo/grammar issues in contributor rules text.
There are a few wording mistakes that can confuse readers (e.g., “cross platform”, “fo”, “react”, “overn”).
✏️ Suggested edits
-Wave Terminal is a modern terminal which provides graphical blocks, dynamic layout, workspaces, and SSH connection management. It is cross platform and built on electron.
+Wave Terminal is a modern terminal which provides graphical blocks, dynamic layout, workspaces, and SSH connection management. It is cross-platform and built on Electron.
...
- - in general const decls go at the top fo the file (before types and functions)
+ - in general const decls go at the top of the file (before types and functions)
...
-- **CRITICAL** - useAtomValue and useAtom are React HOOKS. They cannot be used inline in JSX code, they must appear at the top of a component in the hooks area of the react code.
+- **CRITICAL** - useAtomValue and useAtom are React HOOKS. They cannot be used inline in JSX code, they must appear at the top of a component in the hooks area of the React code.
-- for simple functions, we prefer `if (!cond) { return }; functionality;` pattern overn `if (cond) { functionality }` because it produces less indentation and is easier to follow.
+- for simple functions, we prefer `if (!cond) { return }; functionality;` pattern over `if (cond) { functionality }` because it produces less indentation and is easier to follow.Also applies to: 14-14, 93-93, 94-94
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Use a hyphen to join words.
Context: ...d SSH connection management. It is cross platform and built on electron. ### Pro...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.kilocode/rules/rules.md at line 1, Fix the typos and grammar in the
contributor rules text by updating the sentence "Wave Terminal is a modern
terminal which provides graphical blocks, dynamic layout, workspaces, and SSH
connection management. It is cross platform and built on electron." to use
"cross-platform" and capitalize "Electron", and scan the document for the
reported misspellings and casing issues—replace "fo" with "for", lowercase
"react" with "React", and change "overn" to the intended word (e.g., "overnight"
or "over") wherever they appear (notably near the shown sentence and the other
instances referenced).
| <br /> | ||
| <a | ||
| className="plain-link" | ||
| className="text-secondary! hover:underline!" |
Contributor
There was a problem hiding this comment.
WARNING: Invalid Tailwind syntax - exclamation mark should come before the class name
The ! important modifier in Tailwind v4 should be placed at the beginning of the class name, not at the end.
Suggested change
| className="text-secondary! hover:underline!" | |
| className="!text-secondary hover:!underline" |
| @@ -15,10 +16,7 @@ function initGlobalAtoms(initOpts: GlobalInitOptions) { | |||
| const windowIdAtom = atom(initOpts.windowId) as PrimitiveAtom<string>; | |||
| const builderIdAtom = atom(initOpts.builderId) as PrimitiveAtom<string>; | |||
Contributor
There was a problem hiding this comment.
WARNING: Type mismatch with GlobalAtomsType definition
The builderIdAtom is typed as PrimitiveAtom<string> here, but in custom.d.ts line 10 it's defined as jotai.Atom<string> (readonly). Since this atom is initialized with a constant value and never written to, it should be typed as Atom<string> to match the type definition.
Suggested change
| const builderIdAtom = atom(initOpts.builderId) as PrimitiveAtom<string>; | |
| const builderIdAtom = atom(initOpts.builderId) as Atom<string>; |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.kilocode/rules/rules.md (2)
65-69:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced code block.
The code block starting on Line 65 is still missing a language identifier and will continue to fail MD040.
Suggested fix
-``` +```ts import { getApi } from "@/store/global"; getApi().getIsDev()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.kilocode/rules/rules.md around lines 65 - 69, The fenced code block containing the snippet that calls getApi().getIsDev() lacks a language tag; update that code fence to include the TypeScript language identifier (e.g., change ``` to ```ts) so the block is marked as TypeScript and the MD040 lint rule will pass, keeping the code exactly as shown (including the import and the getApi().getIsDev() call).
1-1:⚠️ Potential issue | 🟡 MinorAddress remaining wording/casing typos in the rules text.
Previously reported issues are still present: Line 1 (
cross platform,electron), Line 93 (react), and Line 94 (overn). Please normalize these for clarity and consistency.Suggested edits
-Wave Terminal is a modern terminal which provides graphical blocks, dynamic layout, workspaces, and SSH connection management. It is cross platform and built on electron. +Wave Terminal is a modern terminal which provides graphical blocks, dynamic layout, workspaces, and SSH connection management. It is cross-platform and built on Electron. ... -- **CRITICAL** - useAtomValue and useAtom are React HOOKS. They cannot be used inline in JSX code, they must appear at the top of a component in the hooks area of the react code. +- **CRITICAL** - useAtomValue and useAtom are React HOOKS. They cannot be used inline in JSX code, they must appear at the top of a component in the hooks area of the React code. ... -- for simple functions, we prefer `if (!cond) { return }; functionality;` pattern overn `if (cond) { functionality }` because it produces less indentation and is easier to follow. +- for simple functions, we prefer `if (!cond) { return }; functionality;` pattern over `if (cond) { functionality }` because it produces less indentation and is easier to follow.Also applies to: 93-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.kilocode/rules/rules.md at line 1, Fix wording and casing typos in the rules text: change "cross platform" to "cross-platform", capitalize "electron" to "Electron", capitalize "react" to "React", and correct the misspelling "overn" to the intended word (e.g., "owner") wherever it appears; update the sentence starting "Wave Terminal is a modern terminal..." and the occurrences of "react" and "overn" so the casing and spelling are normalized and consistent across the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.kilocode/rules/overview.md:
- Around line 9-23: The unlabeled fenced code blocks containing the repository
trees (the block starting with "waveterm/" and the block starting with
"frontend/") need language identifiers to satisfy markdownlint MD040; edit the
opening triple-backtick for those blocks (and any similar block in the 50-77
range) to include a language tag such as text (e.g., change ``` to ```text) so
the examples are properly labeled; locate the blocks by the unique lines
"waveterm/" and "frontend/" to update their opening fences accordingly.
- Around line 33-43: The listed markdown links in .kilocode/rules/overview.md
for files like emain.ts, emain-window.ts, emain-tabview.ts, emain-wavesrv.ts,
emain-wsh.ts, emain-ipc.ts, emain-menu.ts, updater.ts, preload.ts, and
preload-webview.ts are using paths relative to .kilocode/rules/ and will not
resolve; update those link targets to correct repo-relative paths (e.g.,
../../emain/emain.ts or the canonical repository URLs) so they point to the
actual files, and apply the same fix to the other broken entries referenced
later in the file (the entry around the single-file link at line ~95 and the
block around lines ~140-145) to ensure all links resolve from this document.
In @.roo/rules/overview.md:
- Around line 50-77: The fenced code block containing the project tree that
starts with "frontend/" is missing a language identifier (MD040); update the
opening triple-backtick for that block to include a language (e.g., change ```
to ```text or ```txt) so markdownlint stops reporting MD040—locate the block
that lists frontend/, app/, builder/, etc., and add the language token to the
opening fence.
---
Duplicate comments:
In @.kilocode/rules/rules.md:
- Around line 65-69: The fenced code block containing the snippet that calls
getApi().getIsDev() lacks a language tag; update that code fence to include the
TypeScript language identifier (e.g., change ``` to ```ts) so the block is
marked as TypeScript and the MD040 lint rule will pass, keeping the code exactly
as shown (including the import and the getApi().getIsDev() call).
- Line 1: Fix wording and casing typos in the rules text: change "cross
platform" to "cross-platform", capitalize "electron" to "Electron", capitalize
"react" to "React", and correct the misspelling "overn" to the intended word
(e.g., "owner") wherever it appears; update the sentence starting "Wave Terminal
is a modern terminal..." and the occurrences of "react" and "overn" so the
casing and spelling are normalized and consistent across the document.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.kilocode/rules/overview.md.kilocode/rules/rules.md.roo/rules/overview.md.roo/rules/rules.md
Comment on lines
+9
to
+23
| ``` | ||
| waveterm/ | ||
| ├── emain/ # Electron main process code | ||
| ├── frontend/ # React application (renderer process) | ||
| ├── cmd/ # Go command-line applications | ||
| ├── pkg/ # Go packages/modules | ||
| ├── db/ # Database migrations | ||
| ├── docs/ # Documentation (Docusaurus) | ||
| ├── build/ # Build configuration and assets | ||
| ├── assets/ # Application assets (icons, images) | ||
| ├── public/ # Static public assets | ||
| ├── tests/ # Test files | ||
| ├── .github/ # GitHub workflows and configuration | ||
| └── Configuration files (package.json, tsconfig.json, etc.) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Line 9 and Line 50 use unlabeled triple-backtick fences, which will keep triggering MD040 in markdownlint. Please label them (for example, text).
Suggested fix
-```
+```text
waveterm/
├── emain/ # Electron main process code
...
└── Configuration files (package.json, tsconfig.json, etc.)- +text
frontend/
├── app/ # Main application code
...
└── util/ # Utility functions
</details>
Also applies to: 50-77
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.kilocode/rules/overview.md around lines 9 - 23, The unlabeled fenced code
blocks containing the repository trees (the block starting with "waveterm/" and
the block starting with "frontend/") need language identifiers to satisfy
markdownlint MD040; edit the opening triple-backtick for those blocks (and any
similar block in the 50-77 range) to include a language tag such as text (e.g.,
change totext) so the examples are properly labeled; locate the blocks
by the unique lines "waveterm/" and "frontend/" to update their opening fences
accordingly.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+33
to
+43
| - [`emain.ts`](emain/emain.ts) - Main entry point, application lifecycle management | ||
| - [`emain-window.ts`](emain/emain-window.ts) - Window management (`WaveBrowserWindow` class) | ||
| - [`emain-tabview.ts`](emain/emain-tabview.ts) - Tab view management (`WaveTabView` class) | ||
| - [`emain-wavesrv.ts`](emain/emain-wavesrv.ts) - Go backend server integration | ||
| - [`emain-wsh.ts`](emain/emain-wsh.ts) - WSH (Wave Shell) client integration | ||
| - [`emain-ipc.ts`](emain/emain-ipc.ts) - IPC handlers for frontend ↔ main process communication | ||
| - [`emain-menu.ts`](emain/emain-menu.ts) - Application menu system | ||
| - [`updater.ts`](emain/updater.ts) - Auto-update functionality | ||
| - [`preload.ts`](emain/preload.ts) - Preload script for renderer security | ||
| - [`preload-webview.ts`](emain/preload-webview.ts) - Webview preload script | ||
|
|
Contributor
There was a problem hiding this comment.
Fix likely broken relative links in the architecture doc.
The links on Line 33–Line 43, Line 95, and Line 140–Line 145 appear to be relative to .kilocode/rules/, so they likely won’t resolve to repo files. Please update to correct relative paths (or stable absolute repo links) so navigation works from this file.
Also applies to: 95-95, 140-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.kilocode/rules/overview.md around lines 33 - 43, The listed markdown links
in .kilocode/rules/overview.md for files like emain.ts, emain-window.ts,
emain-tabview.ts, emain-wavesrv.ts, emain-wsh.ts, emain-ipc.ts, emain-menu.ts,
updater.ts, preload.ts, and preload-webview.ts are using paths relative to
.kilocode/rules/ and will not resolve; update those link targets to correct
repo-relative paths (e.g., ../../emain/emain.ts or the canonical repository
URLs) so they point to the actual files, and apply the same fix to the other
broken entries referenced later in the file (the entry around the single-file
link at line ~95 and the block around lines ~140-145) to ensure all links
resolve from this document.
Comment on lines
50
to
77
| ``` | ||
| frontend/ | ||
| ├── app/ # Main application code | ||
| │ ├── app.tsx # Root App component | ||
| │ ├── aipanel/ # AI panel UI | ||
| │ ├── block/ # Block-based UI components | ||
| │ ├── element/ # Reusable UI elements | ||
| │ ├── hook/ # Custom React hooks | ||
| │ ├── modals/ # Modal components | ||
| │ ├── store/ # State management (Jotai) | ||
| │ ├── tab/ # Tab components | ||
| │ ├── view/ # Different view types | ||
| │ │ ├── chat/ # Chat interface | ||
| │ │ ├── codeeditor/ # Code editor (Monaco) | ||
| │ │ ├── preview/ # File preview | ||
| │ │ ├── sysinfo/ # System info view | ||
| │ │ ├── term/ # Terminal view | ||
| │ │ ├── webview/ # Web view | ||
| │ │ └── waveai/ # AI integration | ||
| │ │ ├── tsunami/ # Tsunami builder view | ||
| │ │ ├── vdom/ # Virtual DOM view | ||
| │ │ ├── waveai/ # AI chat integration | ||
| │ │ ├── waveconfig/ # Config editor view | ||
| │ │ └── webview/ # Web view | ||
| │ └── workspace/ # Workspace management | ||
| ├── builder/ # Builder app entry | ||
| ├── layout/ # Layout system | ||
| ├── preview/ # Standalone preview renderer | ||
| ├── types/ # TypeScript type definitions | ||
| └── util/ # Utility functions | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced code block (MD040).
The code fence starting at Line 50 is missing a language identifier, which triggers markdownlint MD040.
Suggested fix
-```
+```text
frontend/
├── app/ # Main application code
│ ├── app.tsx # Root App component
...
└── util/ # Utility functions</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.roo/rules/overview.md around lines 50 - 77, The fenced code block
containing the project tree that starts with "frontend/" is missing a language
identifier (MD040); update the opening triple-backtick for that block to include
a language (e.g., change ``` to ```text or ```txt) so markdownlint stops
reporting MD040—locate the block that lists frontend/, app/, builder/, etc., and
add the language token to the opening fence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
also duplicates rules.md for kilocode which was not picking it up from the .roo directory