Add block rename and preview follow terminal features#3235
Open
cowhen wants to merge 2 commits intowavetermdev:mainfrom
Open
Add block rename and preview follow terminal features#3235cowhen wants to merge 2 commits intowavetermdev:mainfrom
cowhen wants to merge 2 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
WalkthroughThis PR adds block renaming UI and state (context-menu rename, inline editable header, Jotai atoms, RPC persistence), implements a "Follow Terminal" feature for directory previews (atoms, dropdown UI, shell-aware Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
| this.followTermBidirAtom = atom<boolean>((get) => { | ||
| return (get(this.blockAtom)?.meta?.["preview:followterm:bidir"] as boolean) ?? false; | ||
| }); | ||
| this.followTermMenuDataAtom = atom(null); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript, is PrimitiveAtomfrom jotai'satom(null)assignable to a field typedPrimitiveAtom<SomeObject | null> under strict mode?
💡 Result:
No, PrimitiveAtom<null> from atom(null) is not assignable to PrimitiveAtom<SomeObject | null> under strict mode in TypeScript.
Citations:
- 1: https://jotai.org/docs/guides/typescript
- 2: https://github.com/pmndrs/jotai/blob/main/src/vanilla/atom.ts
- 3: null/undefined initial atom value confuses typescript pmndrs/jotai#550
- 4: Can PrimitiveAtom be covariant? pmndrs/jotai#312
- 5: Type null is not assignable to type object with strictNullChecks microsoft/TypeScript#13438
- 6: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0%2ehtml
🏁 Script executed:
cd frontend/app/view/preview && sed -n '515,520p' preview-model.tsxRepository: wavetermdev/waveterm
Length of output: 281
🏁 Script executed:
cd frontend/app/view/preview && sed -n '189,193p; 197,202p; 441,445p; 467,472p' preview-model.tsxRepository: wavetermdev/waveterm
Length of output: 1038
🏁 Script executed:
cd frontend/app/view/preview && sed -n '524,540p' preview-model.tsxRepository: wavetermdev/waveterm
Length of output: 819
🏁 Script executed:
cd frontend/app/view/preview && sed -n '1,50p' preview-model.tsx | grep -A 5 "followTermMenuDataAtom"Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
cd frontend/app/view/preview && grep -n "followTermMenuDataAtom" preview-model.tsx | head -5Repository: wavetermdev/waveterm
Length of output: 358
🏁 Script executed:
cd frontend/app/view/preview && sed -n '470,520p' preview-model.tsx | grep -B 50 "followTermMenuDataAtom" | head -60Repository: wavetermdev/waveterm
Length of output: 2150
Add explicit type cast for followTermMenuDataAtom, consistent with sibling nullable atoms.
atom(null) infers as PrimitiveAtom<null>, which is not assignable to the declared PrimitiveAtom<{ pos; terms; currentFollowId; bidir } | null> (Jotai's atom type parameter is invariant). The rest of this class uses the explicit cast pattern (e.g. lines 199, 201, 203, 469); this one was missed and will trip strict TS compilation.
🔧 Proposed fix
- this.followTermMenuDataAtom = atom(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,
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).
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