Add block rename and preview follow terminal features#3235
Open
cowhen wants to merge 4 commits intowavetermdev:mainfrom
Open
Add block rename and preview follow terminal features#3235cowhen wants to merge 4 commits intowavetermdev:mainfrom
cowhen wants to merge 4 commits intowavetermdev:mainfrom
Conversation
- Add inline block renaming via context menu with frame:title metadata - Add preview follow terminal functionality to sync directory navigation - Configure dev build as separate Wave Dev app to avoid conflicts - Add launch_wave_dev.command helper script for dev builds Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
|
Contributor
WalkthroughAdds block-rename UI and state (Jotai atom, context-menu entry, inline editable header, RPC persistence). Implements a "follow terminal" feature for directory previews (new preview atoms, menu/dropdown UI, shell-aware cd dispatch, optional bidirectional sync, metadata updates). Extends TypeScript MetaType and Go metadata constants/struct fields to support preview follow-term keys. Updates Electron dev packaging identifiers and adds a macOS dev launch script. Documents a macOS dev build & packaging workflow in CLAUDE.md. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: 8
🧹 Nitpick comments (5)
frontend/app/block/blockframe-header.tsx (1)
86-91: Minor: every block header re-renders on any rename start/stop.
renamingBlockIdAtomis a single global atom, so every mountedHeaderTextElems(one per block) re-subscribes and re-renders whenever any block enters/exits rename mode. For typical layouts this is negligible, but a derived per-block atom keeps the subscription scoped:♻️ Optional refactor
// in blockrenamestate.ts +export const isRenamingBlockAtomFamily = (blockId: string) => + jotai.atom((get) => get(renamingBlockIdAtom) === blockId);Then consume
useAtomValue(isRenamingBlockAtomFamily(blockId))instead of comparing the raw id. (Wrap in a memoizedatomFamilyif you'd rather not re-create the derived atom per render.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/blockframe-header.tsx` around lines 86 - 91, The header components subscribe to the global renamingBlockIdAtom so every HeaderTextElems re-renders on any rename toggle; fix this by introducing a per-block derived atom (e.g., create an atomFamily or a memoized function like isRenamingBlockAtom(blockId) that returns jotai.atom(get => get(renamingBlockIdAtom) === blockId)) and then have HeaderTextElems consume useAtomValue of that per-block atom (replace the renamingBlockId === blockId check with util.useAtomValueSafe(isRenamingBlockAtom(blockId))). If you create the derived atom inside a factory, memoize it (or use an atomFamily helper) to avoid recreating it each render.frontend/app/view/preview/preview.tsx (2)
143-151: Consider also clearing bidir when switching to a different terminal vialinkTerm.
linkTermonly writespreview:followtermid;preview:followterm:bidiris preserved. If a user had bidir enabled on terminal A and then switches to terminal B through this menu, the new link silently inherits bidir — which will immediately push acdinto terminal B on the next directory change. If that's intentional, fine; otherwise consider resetting bidir on link to match the unlink semantics at L143–151.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview.tsx` around lines 143 - 151, linkTerm currently only updates "preview:followtermid" and leaves "preview:followterm:bidir" intact, causing a new link to inherit bidir state; update the linkTerm implementation (the same area interacting with model.env.services.object.UpdateObjectMeta / WOS.makeORef and model.blockId) to also clear or explicitly set "preview:followterm:bidir" (e.g., null/false) when writing the new followterm id so its behavior matches unlink which clears both keys; ensure any client-side atoms like model.followTermMenuDataAtom are updated consistently after the UpdateObjectMeta call.
7-28: Move top-level helpers below the import block.
shellEscapePathandsendCdToTerminalare declared between two groups ofimportstatements (lines 7–20 vs 21–28). ES module import hoisting makes this work at runtime, but it breaks the conventional top-of-file import layout and trips most linters/formatters. Group all imports first, then define the helpers — or move them into a small helper module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview.tsx` around lines 7 - 28, Move the top-level helper functions out of the middle of the import section: relocate shellEscapePath and sendCdToTerminal so they appear after all import statements (or extract them into a new helper module and import them), ensuring you preserve their signatures and update any imports/exports if you extract them; this restores a single contiguous import block and keeps the functions (shellEscapePath, sendCdToTerminal) in their original file or a small helper module referenced by preview.tsx.frontend/app/view/preview/preview-model.tsx (2)
173-176: Extract the dropdown menu data type for readability.The inline anonymous type on
followTermMenuDataAtomis long and is duplicated conceptually inpreview.tsxwhen destructuringmenuData. Naming it (e.g.FollowTermMenuData) and exporting it simplifies future consumers and keeps the shape in one place.♻️ Proposed extraction
+type FollowTermMenuData = { + pos: { x: number; y: number }; + terms: { blockId: string; title: string }[]; + currentFollowId: string | null; + bidir: boolean; +}; + ... - followTermMenuDataAtom: PrimitiveAtom<{ pos: { x: number; y: number }; terms: { blockId: string; title: string }[]; currentFollowId: string | null; bidir: boolean } | null>; + followTermMenuDataAtom: PrimitiveAtom<FollowTermMenuData | null>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-model.tsx` around lines 173 - 176, Extract the long anonymous menu-data shape into a named exported type (e.g. FollowTermMenuData) and replace the inline type on followTermMenuDataAtom with that alias so the shape is defined once; update any consumers (like preview.tsx where menuData is destructured) to import and use FollowTermMenuData instead of repeating the inline type. Ensure the new type is exported from the module that defines followTermMenuDataAtom and keep the exact field names/structures (pos, terms array with blockId/title, currentFollowId, bidir) to maintain compatibility.
520-544: Minor: menu fallback title index is based on the filtered list, not tab order.The
iin.map(..., i) => ... || \Terminal ${i + 1}`is the index *after* filtering out non-terminal blocks and the current preview block. Users with mixed blocks will see "Terminal 1"/"Terminal 2" numbers that don't align with tab position. Givenframe:title` is usually present, this is a cosmetic fallback only — flagging as nitpick.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-model.tsx` around lines 520 - 544, The fallback "Terminal N" index is being taken from the filtered/map iteration index in showFollowTermMenu, which reflects the post-filtered order rather than the tab order; to fix, build a termBlockIds array from the original tabData.blockids in the same order (filtering out this.blockId and non-term blocks using WOS.getObjectValue and block.meta.view === "term"), then when constructing terms use that ordered termBlockIds to derive the fallback index (e.g., Terminal ${termBlockIds.indexOf(bid) + 1}) instead of the map's i; update the logic that sets followTermMenuDataAtom accordingly so titles use the tab-order-based index while keeping frame:title when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 26-30: Update the CLAUDE.md launch docs to explicitly document the
config/data env overrides used by the launcher: replace the single WAVETERM_HOME
example with a multi-line example that sets WAVETERM_HOME, WAVETERM_CONFIG_HOME,
and WAVETERM_DATA_HOME (e.g. WAVETERM_HOME=~/.waveterm-dev,
WAVETERM_CONFIG_HOME=~/.waveterm-dev/config,
WAVETERM_DATA_HOME=~/.waveterm-dev/data) and change the explanatory sentence to
state these variables create isolated config and data dirs for the dev build;
also mention that getWaveHomeDir() only honors WAVETERM_HOME after wave.lock
exists so the explicit CONFIG/DATA overrides are needed for clean
installs/launched dev instances.
In `@frontend/app/block/blockframe-header.tsx`:
- Around line 46-49: The "Rename Block" menu item currently always calls
startBlockRename(blockId); update the generation/handler in
handleHeaderContextMenu (or where the menu array with label "Rename Block" is
built) to either omit the menu item when preview is true or to guard the click
with a check like if (!ephemeral && !preview) then call
startBlockRename(blockId); reference startBlockRename, handleHeaderContextMenu,
the "Rename Block" menu entry, and the ephemeral/preview flags when making the
change so renames are disabled for preview/ephemeral blocks.
- Around line 131-142: The "Rename Block" context-menu item is shown
unconditionally while the inline rename badge only renders when useTermHeader &&
frameTitle; update the context-menu entry for "Rename Block" to respect the same
condition by adding a visible or enabled property driven by useTermHeader (or
the same terminal-block check used to render the badge) so the menu item is
hidden or disabled for non-terminal/ephemeral blocks; locate the context-menu
items array or builder that defines the "Rename Block" entry (the item with
label "Rename Block") and gate it on useTermHeader (or the terminal/frame-type
predicate) to match the badge rendering logic.
- Around line 95-127: The input rename UX should guard against Escape/blur
races, auto-select text on focus, and surface RPC errors: add a ref (e.g.,
cancelRef) that you set when Escape is pressed (inside the onKeyDown handler)
and check in the onBlur handler and saveRename to no-op if cancellation is set;
change saveRename to await the waveEnv.rpc.SetMetaCommand call (or attach a
.catch) and log errors instead of fire-and-forget; in the input's onFocus call
select() on the target to auto-select the current frameTitle; ensure
stopBlockRename still clears the ref when appropriate so subsequent opens are
not cancelled. Reference saveRename, stopBlockRename,
waveEnv.rpc.SetMetaCommand, and the input's onBlur/onKeyDown/onFocus handlers
when making the changes.
In `@frontend/app/view/preview/preview.tsx`:
- Around line 116-218: FollowTermDropdown currently only closes on backdrop
mousedown and lacks keyboard/accessibility features; update the component
(FollowTermDropdown) to add an Escape key listener (in a useEffect) that calls
closeMenu and is cleaned up on unmount, assign role="menu" to the portal inner
container and role="menuitem" plus tabIndex={0} and descriptive aria-labels to
each clickable row (the term rows, the bidir toggle in toggleBidir, and Stop
Following via unlink), add onKeyDown handlers on those rows to trigger the same
actions on Enter/Space, and implement focus management by capturing
document.activeElement before opening, moving focus to the first menu item on
mount (use refs or a ref array for the mapped terms), and restoring focus to the
original opener when closeMenu runs; ensure all event listeners and refs are
properly cleaned up.
- Around line 249-265: Add the missing effect dependencies and make the bidir
suppression robust: update the first useEffect to include followTermId and model
(it currently references them but only depends on followTermCwd) and update the
second useEffect to include followTermId, followTermBidir, and env in its
dependency array (keep loadableFileInfo too) so toggling bidir immediately
triggers a cd; change the suppressBidirRef logic around model.goHistory so you
only set suppressBidirRef.current = true when model.goHistory(followTermCwd)
actually produces an update (i.e., returns a non-null/changed value) or
alternatively clear the suppressor on a short timer or in a cleanup to avoid
suppressing the next genuine navigation; references: followTermId,
followTermCwd, model.goHistory, suppressBidirRef, loadableFileInfo,
followTermBidir, sendCdToTerminal, env.
- Around line 8-20: The current sendCdToTerminal and shellEscapePath assume
POSIX shells and send a raw Ctrl-U + single-quoted cd payload which breaks on
PowerShell, cmd.exe, and when a foreground program is running; update the logic
to first obtain the terminal's shell type via PreviewEnv (extend PreviewEnv to
expose a method like getTerminalMeta or getShellType or add term metadata
access) and skip or use a shell-aware code path if the shell is unknown/unsafe;
implement per-shell quoting helpers (e.g. posixQuote in shellEscapePath,
powershellQuote, cmdQuote) and route sendCdToTerminal to use the appropriate
quoter or to avoid sending Ctrl-U (remove the leading "\x15" for non-POSIX
shells) and to avoid sending keystrokes into foreground processes by only
sending when term metadata indicates a normal shell prompt; also rename or
document shellEscapePath to posixEscapePath if you keep POSIX-only behavior and
update references (shellEscapePath, sendCdToTerminal, PreviewEnv, TabRpcClient)
accordingly.
In `@launch_wave_dev.command`:
- Line 4: Line 4 hard-codes the app binary and only sets WAVETERM_HOME (which
can be ignored on a clean install); change the launcher to compute its own
directory and use that to set portable dev paths and explicit overrides: derive
SCRIPT_DIR from the script location (e.g., dirname of $0 or ${BASH_SOURCE[0]}),
set WAVETERM_HOME to "$SCRIPT_DIR/.waveterm-dev" and also set explicit
WAVETERM_CONFIG and WAVETERM_DATA to "$WAVETERM_HOME/config" and
"$WAVETERM_HOME/data" (or similar) so clean installs are isolated, then exec the
Wave Dev binary using the computed SCRIPT_DIR path instead of the hard-coded
absolute path.
---
Nitpick comments:
In `@frontend/app/block/blockframe-header.tsx`:
- Around line 86-91: The header components subscribe to the global
renamingBlockIdAtom so every HeaderTextElems re-renders on any rename toggle;
fix this by introducing a per-block derived atom (e.g., create an atomFamily or
a memoized function like isRenamingBlockAtom(blockId) that returns
jotai.atom(get => get(renamingBlockIdAtom) === blockId)) and then have
HeaderTextElems consume useAtomValue of that per-block atom (replace the
renamingBlockId === blockId check with
util.useAtomValueSafe(isRenamingBlockAtom(blockId))). If you create the derived
atom inside a factory, memoize it (or use an atomFamily helper) to avoid
recreating it each render.
In `@frontend/app/view/preview/preview-model.tsx`:
- Around line 173-176: Extract the long anonymous menu-data shape into a named
exported type (e.g. FollowTermMenuData) and replace the inline type on
followTermMenuDataAtom with that alias so the shape is defined once; update any
consumers (like preview.tsx where menuData is destructured) to import and use
FollowTermMenuData instead of repeating the inline type. Ensure the new type is
exported from the module that defines followTermMenuDataAtom and keep the exact
field names/structures (pos, terms array with blockId/title, currentFollowId,
bidir) to maintain compatibility.
- Around line 520-544: The fallback "Terminal N" index is being taken from the
filtered/map iteration index in showFollowTermMenu, which reflects the
post-filtered order rather than the tab order; to fix, build a termBlockIds
array from the original tabData.blockids in the same order (filtering out
this.blockId and non-term blocks using WOS.getObjectValue and block.meta.view
=== "term"), then when constructing terms use that ordered termBlockIds to
derive the fallback index (e.g., Terminal ${termBlockIds.indexOf(bid) + 1})
instead of the map's i; update the logic that sets followTermMenuDataAtom
accordingly so titles use the tab-order-based index while keeping frame:title
when present.
In `@frontend/app/view/preview/preview.tsx`:
- Around line 143-151: linkTerm currently only updates "preview:followtermid"
and leaves "preview:followterm:bidir" intact, causing a new link to inherit
bidir state; update the linkTerm implementation (the same area interacting with
model.env.services.object.UpdateObjectMeta / WOS.makeORef and model.blockId) to
also clear or explicitly set "preview:followterm:bidir" (e.g., null/false) when
writing the new followterm id so its behavior matches unlink which clears both
keys; ensure any client-side atoms like model.followTermMenuDataAtom are updated
consistently after the UpdateObjectMeta call.
- Around line 7-28: Move the top-level helper functions out of the middle of the
import section: relocate shellEscapePath and sendCdToTerminal so they appear
after all import statements (or extract them into a new helper module and import
them), ensuring you preserve their signatures and update any imports/exports if
you extract them; this restores a single contiguous import block and keeps the
functions (shellEscapePath, sendCdToTerminal) in their original file or a small
helper module referenced by preview.tsx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6113d99a-90b4-49d7-b8cd-b91bf62604df
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CLAUDE.mdfrontend/app/block/blockframe-header.tsxfrontend/app/block/blockrenamestate.tsfrontend/app/view/preview/preview-model.tsxfrontend/app/view/preview/preview.tsxfrontend/types/gotypes.d.tslaunch_wave_dev.commandpackage.jsonpkg/waveobj/metaconsts.gopkg/waveobj/wtypemeta.go
Block rename improvements: - Restrict rename menu to terminal blocks only (match badge visibility) - Disable rename for preview/ephemeral blocks - Add race condition protection for Escape/blur handling - Auto-select text on focus for better UX - Await RPC calls and log errors instead of fire-and-forget Preview follow terminal enhancements: - Add keyboard accessibility (Escape, Enter, Space navigation) - Add ARIA roles and labels for screen readers - Implement focus management (capture and restore) - Fix effect dependencies to prevent stale closures - Improve bidir suppression with timeout cleanup - Make sendCdToTerminal shell-aware (POSIX, PowerShell, cmd.exe) - Clear bidir state when linking new terminal - Fix terminal numbering to use tab order instead of filtered index - Move helper functions after imports for better organization Dev build improvements: - Update CLAUDE.md with explicit config/data env variables - Make launch_wave_dev.command portable with dynamic paths - Document why explicit overrides needed for clean installs Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/app/block/blockframe-header.tsx (1)
158-168:cursor-pointeron a non-clickable badge is misleading.The badge sets
cursor-pointerbut has noonClickhandler; the tooltip instructs the user to right-click. This mismatch suggests left-click should do something. Either dropcursor-pointer(use default/contextmenu cursor), or wire aonDoubleClick={() => startBlockRename(blockId)}for a natural rename-entry UX that matches the cursor affordance.♻️ Proposed tweak
- <div - key="frame-title" - className="block-frame-text shrink-0 opacity-70 cursor-pointer" - title="Right-click header to rename" - > + <div + key="frame-title" + className="block-frame-text shrink-0 opacity-70 cursor-pointer" + title="Double-click to rename (or right-click header)" + onDoubleClick={(e) => { + e.stopPropagation(); + startBlockRename(blockId); + }} + > {frameTitle} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/blockframe-header.tsx` around lines 158 - 168, The badge element rendered when useTermHeader && frameTitle is true shows a misleading cursor via className "cursor-pointer" but has no left-click behavior; either remove "cursor-pointer" from headerTextElems' div className or add a left-click/double-click handler to match the affordance—e.g., wire onDoubleClick={() => startBlockRename(blockId)} on the same div (referencing startBlockRename and blockId) so the visual cue matches actual behavior; update the JSX in blockframe-header.tsx accordingly.frontend/app/view/preview/preview-model.tsx (1)
524-538: Use.mapindex instead ofindexOffor terminal numbering.
termBlockIds.indexOf(bid)inside.map(bid => …)over the same array is O(n²) and redundant — the map callback already receives the index. Block IDs are unique so the result is identical. Also worth collapsing the two iterations (filter + map) into a single pass if you want, but the indexOf fix alone is the main win.♻️ Proposed refactor
- const termBlockIds = blockIds - .filter((bid) => { - if (bid === this.blockId) return false; - const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get); - return block?.meta?.view === "term"; - }); - - const terms = termBlockIds.map((bid) => { - const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get); - const termIndex = termBlockIds.indexOf(bid) + 1; - return { - blockId: bid, - title: (block?.meta?.["frame:title"] as string) || `Terminal ${termIndex}`, - }; - }); + const terms: { blockId: string; title: string }[] = []; + for (const bid of blockIds) { + if (bid === this.blockId) continue; + const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get); + if (block?.meta?.view !== "term") continue; + terms.push({ + blockId: bid, + title: (block?.meta?.["frame:title"] as string) || `Terminal ${terms.length + 1}`, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-model.tsx` around lines 524 - 538, The current code uses termBlockIds.indexOf(bid) inside the map callback which makes numbering O(n²); change the map to use its index parameter (e.g., termBlockIds.map((bid, i) => { const termIndex = i + 1; ... })) to compute Terminal N, and optionally collapse the filter+map into a single reduce/map over blockIds while still skipping this.blockId and checking block?.meta?.view === "term" (use WOS.getObjectValue(WOS.makeORef("block", bid), globalStore.get) as before) so numbering is computed from the iteration index instead of indexOf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/block/blockframe-header.tsx`:
- Around line 111-119: The RPC failure is currently only logged but
stopBlockRename() is always called, closing the input and hiding the failed
save; change the flow so stopBlockRename() is only invoked after a successful
waveEnv.rpc.SetMetaCommand(TabRpcClient, ...) and on catch keep the input open
and surface an error notification to the user (use the app's existing
notification/toast utility or add a showToast/errorToast call) so they can
retry; ensure you reference the same parameters (oref via WOS.makeORef("block",
blockId) and meta "frame:title": val) and do not swallow the caught error — pass
its message into the toast.
In `@frontend/app/view/preview/preview-model.tsx`:
- Line 517: followTermMenuDataAtom is initialized with atom(null) which infers
PrimitiveAtom<null> and fails assignment to the declared PrimitiveAtom<{ pos;
terms; currentFollowId; bidir } | null>; change the initialization to use the
same explicit cast pattern as siblings by casting atom(null) to PrimitiveAtom<{
pos: any; terms: any; currentFollowId: any; bidir: any } | null> (i.e., replace
the current atom(null) expression for followTermMenuDataAtom with an explicit
type cast matching the declared atom type).
---
Nitpick comments:
In `@frontend/app/block/blockframe-header.tsx`:
- Around line 158-168: The badge element rendered when useTermHeader &&
frameTitle is true shows a misleading cursor via className "cursor-pointer" but
has no left-click behavior; either remove "cursor-pointer" from headerTextElems'
div className or add a left-click/double-click handler to match the
affordance—e.g., wire onDoubleClick={() => startBlockRename(blockId)} on the
same div (referencing startBlockRename and blockId) so the visual cue matches
actual behavior; update the JSX in blockframe-header.tsx accordingly.
In `@frontend/app/view/preview/preview-model.tsx`:
- Around line 524-538: The current code uses termBlockIds.indexOf(bid) inside
the map callback which makes numbering O(n²); change the map to use its index
parameter (e.g., termBlockIds.map((bid, i) => { const termIndex = i + 1; ... }))
to compute Terminal N, and optionally collapse the filter+map into a single
reduce/map over blockIds while still skipping this.blockId and checking
block?.meta?.view === "term" (use WOS.getObjectValue(WOS.makeORef("block", bid),
globalStore.get) as before) so numbering is computed from the iteration index
instead of indexOf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85e68495-48c3-45f1-8a71-6f19eb3e074d
📒 Files selected for processing (5)
CLAUDE.mdfrontend/app/block/blockframe-header.tsxfrontend/app/view/preview/preview-model.tsxfrontend/app/view/preview/preview.tsxlaunch_wave_dev.command
✅ Files skipped from review due to trivial changes (2)
- launch_wave_dev.command
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/view/preview/preview.tsx
Comment on lines
+111
to
+119
| try { | ||
| await waveEnv.rpc.SetMetaCommand(TabRpcClient, { | ||
| oref: WOS.makeORef("block", blockId), | ||
| meta: { "frame:title": val }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to save block rename:", error); | ||
| } | ||
| stopBlockRename(); |
Contributor
There was a problem hiding this comment.
Silent RPC failure leaves user thinking rename succeeded.
On SetMetaCommand failure, the error is logged but stopBlockRename() still runs, so the input closes and the pre-existing frame:title stays — the user has no indication their edit was lost. Consider surfacing a toast/notification, or keeping the input open on failure so the user can retry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/block/blockframe-header.tsx` around lines 111 - 119, The RPC
failure is currently only logged but stopBlockRename() is always called, closing
the input and hiding the failed save; change the flow so stopBlockRename() is
only invoked after a successful waveEnv.rpc.SetMetaCommand(TabRpcClient, ...)
and on catch keep the input open and surface an error notification to the user
(use the app's existing notification/toast utility or add a showToast/errorToast
call) so they can retry; ensure you reference the same parameters (oref via
WOS.makeORef("block", blockId) and meta "frame:title": val) and do not swallow
the caught error — pass its message into the toast.
- Move stopBlockRename() to only execute after successful RPC - Keep rename input open on error so user can retry - Add explicit type cast for followTermMenuDataAtom to fix type inference Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/app/view/preview/preview-model.tsx (2)
517-517: Match the declared field type instead ofanyin the cast.The field on line 176 is declared with a fully-typed shape, but this initializer weakens it with
anyon every property. FutureglobalStore.set(followTermMenuDataAtom, …)calls inside this class won't be checked against the intended shape. Either drop the cast (rely on the field type) or mirror the declared type explicitly.♻️ Proposed refactor
- this.followTermMenuDataAtom = atom(null) as PrimitiveAtom<{ pos: any; terms: any; currentFollowId: any; bidir: any } | null>; + this.followTermMenuDataAtom = atom(null) as PrimitiveAtom<{ + pos: { x: number; y: number }; + terms: { blockId: string; title: string }[]; + currentFollowId: string | null; + bidir: boolean; + } | null>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-model.tsx` at line 517, The initializer for followTermMenuDataAtom uses a cast to PrimitiveAtom<{ pos: any; terms: any; currentFollowId: any; bidir: any } | null> which weakens type safety; update the cast to match the declared field shape (use the exact typed shape used on the field declaration instead of any) or remove the cast entirely and let TypeScript infer the type from the declared field; ensure subsequent calls like globalStore.set(followTermMenuDataAtom, ...) will be type-checked against the correct { pos: ..., terms: ..., currentFollowId: ..., bidir: ... } | null shape.
524-538: Fold filter + map into one pass; use the map index.A couple of small issues in this block:
- Each candidate block is looked up twice via
WOS.getObjectValue(once infilter, again inmap).termBlockIds.indexOf(bid)inside.mapmakes terminal numbering O(n²); the index is already available as the second argument of the callback.Combining them is both clearer and avoids the redundant lookups:
♻️ Proposed refactor
- const termBlockIds = blockIds - .filter((bid) => { - if (bid === this.blockId) return false; - const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get); - return block?.meta?.view === "term"; - }); - - const terms = termBlockIds.map((bid) => { - const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get); - const termIndex = termBlockIds.indexOf(bid) + 1; - return { - blockId: bid, - title: (block?.meta?.["frame:title"] as string) || `Terminal ${termIndex}`, - }; - }); + const terms = blockIds + .filter((bid) => bid !== this.blockId) + .map((bid) => ({ + bid, + block: WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get), + })) + .filter(({ block }) => block?.meta?.view === "term") + .map(({ bid, block }, idx) => ({ + blockId: bid, + title: (block?.meta?.["frame:title"] as string) || `Terminal ${idx + 1}`, + }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-model.tsx` around lines 524 - 538, Replace the two-pass filter+map over blockIds with a single pass that looks up each block once using WOS.getObjectValue(WOS.makeORef("block", bid), globalStore.get), skips this.blockId and only collects blocks whose meta.view === "term", and uses the map callback's index (second argument) or a local counter to derive the terminal number instead of calling termBlockIds.indexOf(bid); produce an array of objects with blockId and title = block?.meta?.["frame:title"] || `Terminal ${index+1}`.
🤖 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/app/view/preview/preview-model.tsx`:
- Line 517: The initializer for followTermMenuDataAtom uses a cast to
PrimitiveAtom<{ pos: any; terms: any; currentFollowId: any; bidir: any } | null>
which weakens type safety; update the cast to match the declared field shape
(use the exact typed shape used on the field declaration instead of any) or
remove the cast entirely and let TypeScript infer the type from the declared
field; ensure subsequent calls like globalStore.set(followTermMenuDataAtom, ...)
will be type-checked against the correct { pos: ..., terms: ...,
currentFollowId: ..., bidir: ... } | null shape.
- Around line 524-538: Replace the two-pass filter+map over blockIds with a
single pass that looks up each block once using
WOS.getObjectValue(WOS.makeORef("block", bid), globalStore.get), skips
this.blockId and only collects blocks whose meta.view === "term", and uses the
map callback's index (second argument) or a local counter to derive the terminal
number instead of calling termBlockIds.indexOf(bid); produce an array of objects
with blockId and title = block?.meta?.["frame:title"] || `Terminal ${index+1}`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b4954884-925b-4236-81a3-0548b47c5535
📒 Files selected for processing (2)
frontend/app/block/blockframe-header.tsxfrontend/app/view/preview/preview-model.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/app/block/blockframe-header.tsx
**P0 Fixes:** - Fix React hooks violation: move useEffect before conditional return in FollowTermDropdown - Remove broken shell type detection (cmd:shell is boolean, not shell path) **P1 Fixes:** - Fix command injection by removing vulnerable cmdEscapePath/powershellEscapePath - Revert to POSIX-only shell escaping (safe for bash/zsh/sh) - Fix race condition: remove timeout-based suppressBidir reset **P2 Fixes:** - Fix stale closure: make closeMenu a useCallback with proper dependencies - Fix linkTerm behavior: only reset bidir when linking to different terminal - Improve type safety: use proper types instead of any in followTermMenuDataAtom Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/app/view/preview/preview.tsx (2)
24-35:⚠️ Potential issue | 🟠 MajorRoute
cdthrough a shell-aware and foreground-safe path before sending raw input.This still builds a POSIX/readline payload:
\x15cd '<path>'\r. It will not quote correctly for PowerShell/cmd.exe and can be sent into foreground apps like vim, ssh, REPLs, etc. Gate this on terminal shell/prompt metadata or use a backend/controller helper that can safely perform shell-aware directory changes.#!/bin/bash # Verify whether preview cd dispatch is still POSIX-only and whether PreviewEnv exposes shell metadata. set -u rg -nP -C3 'function shellEscapePath|async function sendCdToTerminal|ControllerInputCommand' frontend/app/view/preview/preview.tsx || true rg -nP -C3 'shell:type|term:localshellpath|GetMetaCommand|getShellType|foreground|prompt' frontend/app/view/preview/previewenv.ts frontend/app/view/preview/preview.tsx || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview.tsx` around lines 24 - 35, The current sendCdToTerminal builds and injects a POSIX/readline payload via shellEscapePath which is unsafe for non-POSIX shells and can be injected into foreground programs; update sendCdToTerminal to first consult PreviewEnv for shell/prompt/foreground metadata (PreviewEnv) and, if available, branch by shell type: for bash/zsh use a safely quoted POSIX cd, for PowerShell use a Set-Location -LiteralPath-safe call, for cmd.exe use cd /d with proper quoting; if the terminal is in a foreground app or shell metadata is absent, instead call a backend/controller helper RPC (e.g. ControllerChangeDirectory or a new ControllerSafeCd) via env.rpc to perform the directory change server-side rather than sending raw input with ControllerInputCommand/TabRpcClient; keep shellEscapePath only for POSIX branch and add a safe literal-quoting function per-shell or delegate quoting to the backend.
309-325:⚠️ Potential issue | 🟡 MinorArm bidirectional suppression only when preview navigation will actually change.
model.goHistory(followTermCwd)can no-op for the current path, but Line 311 still leavessuppressBidirRef.current = true. The next genuine preview navigation then gets suppressed and does notcdthe terminal.🛠️ Proposed guard
useEffect(() => { if (!followTermId || !followTermCwd) return; + const curPath = globalStore.get(model.metaFilePath); + if (curPath === followTermCwd) return; suppressBidirRef.current = true; fireAndForget(() => model.goHistory(followTermCwd)); }, [followTermCwd, followTermId, model]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview.tsx` around lines 309 - 325, The suppression flag is being set unconditionally before calling model.goHistory, so a no-op navigation still arms suppression and blocks the next real navigation; change the first useEffect so you only set suppressBidirRef.current = true when the preview navigation will actually change the path: either compare followTermCwd to the model's current cwd (e.g. model.cwd or model.getCwd()) and only set the flag if they differ, or call model.goHistory(followTermCwd) and set suppressBidirRef.current = true only if that call returns/indicates a real change; keep the rest of the effect (fireAndForget) the same and reference suppressBidirRef, model.goHistory, followTermCwd, and followTermId when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/preview/preview.tsx`:
- Around line 120-146: The effect is recapturing document.activeElement whenever
menuData updates (e.g., when toggleBidir updates menuData) which causes focus
jumps; change the logic so we only capture the opener focus when the menu
actually opens by setting previousActiveElement.current = document.activeElement
only when previousActiveElement.current is null (or when menuData transitions
from null to non-null), leaving subsequent menuData updates alone; keep existing
behavior of focusing the first item via menuRef but do not overwrite the saved
opener, and apply the same change to the similar effect referenced around the
165-172 region (use the same guard before assigning
previousActiveElement.current).
---
Duplicate comments:
In `@frontend/app/view/preview/preview.tsx`:
- Around line 24-35: The current sendCdToTerminal builds and injects a
POSIX/readline payload via shellEscapePath which is unsafe for non-POSIX shells
and can be injected into foreground programs; update sendCdToTerminal to first
consult PreviewEnv for shell/prompt/foreground metadata (PreviewEnv) and, if
available, branch by shell type: for bash/zsh use a safely quoted POSIX cd, for
PowerShell use a Set-Location -LiteralPath-safe call, for cmd.exe use cd /d with
proper quoting; if the terminal is in a foreground app or shell metadata is
absent, instead call a backend/controller helper RPC (e.g.
ControllerChangeDirectory or a new ControllerSafeCd) via env.rpc to perform the
directory change server-side rather than sending raw input with
ControllerInputCommand/TabRpcClient; keep shellEscapePath only for POSIX branch
and add a safe literal-quoting function per-shell or delegate quoting to the
backend.
- Around line 309-325: The suppression flag is being set unconditionally before
calling model.goHistory, so a no-op navigation still arms suppression and blocks
the next real navigation; change the first useEffect so you only set
suppressBidirRef.current = true when the preview navigation will actually change
the path: either compare followTermCwd to the model's current cwd (e.g.
model.cwd or model.getCwd()) and only set the flag if they differ, or call
model.goHistory(followTermCwd) and set suppressBidirRef.current = true only if
that call returns/indicates a real change; keep the rest of the effect
(fireAndForget) the same and reference suppressBidirRef, model.goHistory,
followTermCwd, and followTermId when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1fc3433-f353-48aa-b81a-cb2b5b68ab4e
📒 Files selected for processing (2)
frontend/app/view/preview/preview-model.tsxfrontend/app/view/preview/preview.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/app/view/preview/preview-model.tsx
Comment on lines
+120
to
+146
| const closeMenu = React.useCallback(() => { | ||
| BlockModel.getInstance().setBlockHighlight(null); | ||
| globalStore.set(model.followTermMenuDataAtom, null); | ||
| if (previousActiveElement.current instanceof HTMLElement) { | ||
| previousActiveElement.current.focus(); | ||
| } | ||
| }, [model.followTermMenuDataAtom]); | ||
|
|
||
| useEffect(() => { | ||
| if (!menuData) return; | ||
| previousActiveElement.current = document.activeElement; | ||
| const handleEscape = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| closeMenu(); | ||
| } | ||
| }; | ||
| document.addEventListener("keydown", handleEscape); | ||
| if (menuRef.current) { | ||
| const firstItem = menuRef.current.querySelector('[role="menuitem"]'); | ||
| if (firstItem instanceof HTMLElement) { | ||
| firstItem.focus(); | ||
| } | ||
| } | ||
| return () => { | ||
| document.removeEventListener("keydown", handleEscape); | ||
| }; | ||
| }, [menuData, closeMenu]); |
Contributor
There was a problem hiding this comment.
Only capture/restore opener focus when the menu opens.
toggleBidir replaces menuData while the menu remains open, so this effect recaptures document.activeElement from inside the dropdown and focuses the first item again. That breaks focus restore and causes keyboard focus to jump after toggling.
♿ Proposed focus-management adjustment
+ const isMenuOpen = menuData != null;
+
const closeMenu = React.useCallback(() => {
BlockModel.getInstance().setBlockHighlight(null);
globalStore.set(model.followTermMenuDataAtom, null);
- if (previousActiveElement.current instanceof HTMLElement) {
- previousActiveElement.current.focus();
+ const elementToRestore = previousActiveElement.current;
+ previousActiveElement.current = null;
+ if (elementToRestore instanceof HTMLElement && document.contains(elementToRestore)) {
+ elementToRestore.focus();
}
}, [model.followTermMenuDataAtom]);
useEffect(() => {
- if (!menuData) return;
- previousActiveElement.current = document.activeElement;
+ if (!isMenuOpen) return;
+ previousActiveElement.current = document.activeElement;
const handleEscape = (e: KeyboardEvent) => {
if (e.key === "Escape") {
closeMenu();
}
};
@@
return () => {
document.removeEventListener("keydown", handleEscape);
};
- }, [menuData, closeMenu]);
+ }, [isMenuOpen, closeMenu]);Also applies to: 165-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/preview/preview.tsx` around lines 120 - 146, The effect is
recapturing document.activeElement whenever menuData updates (e.g., when
toggleBidir updates menuData) which causes focus jumps; change the logic so we
only capture the opener focus when the menu actually opens by setting
previousActiveElement.current = document.activeElement only when
previousActiveElement.current is null (or when menuData transitions from null to
non-null), leaving subsequent menuData updates alone; keep existing behavior of
focusing the first item via menuRef but do not overwrite the saved opener, and
apply the same change to the similar effect referenced around the 165-172 region
(use the same guard before assigning previousActiveElement.current).
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
2 participants
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.
Summary
Detailed Changes
Block Rename Improvements
Preview Follow Terminal Enhancements
sendCdToTerminalshell-aware (POSIX, PowerShell, cmd.exe)Dev Build Improvements
WAVETERM_CONFIG_HOMEandWAVETERM_DATA_HOMEenv variableslaunch_wave_dev.commandportable with dynamic script directory detectionTest plan
🤖 Generated with Claude Code