feat: add terminal-preview linking functionality #1852
Conversation
Introduces new features for linking preview and terminal blocks: - Added metadata for tracking linked terminals - Implemented menu options to link/unlink terminals - Created sync mechanism to update terminal context when changing directories - Extended type definitions to support new metadata
|
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
/usr/local/bin/yarn: 3: dirname: not found Error: Cannot find module '/yarn.js' Node.js v22.9.0 WalkthroughThe pull request introduces enhancements for synchronizing terminal and preview functionalities within the application. Key changes include the addition of two new functions, Furthermore, the ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/app/view/preview/sync.ts (2)
18-21: Add more descriptive documentation.These lines contain a high-level description, but it might be helpful to provide additional details on prerequisites or usage scenarios for future maintainers unfamiliar with this mechanism. Consider elaborating on how the synchronization is triggered or suspended.
36-41: Handle errors fromgoHistorymore explicitly.Although the
catchblock delegates errors toconsole.error, consider surfacing them at the UI level or employing a global error-handling mechanism to improve reliability. This is especially important if directory changes can fail or be restricted.pkg/waveobj/wtypemeta.go (1)
39-40: Ensure optional field usage is deliberate.
PreviewLinkedTerminalbeing a rawstringinstead of a pointer is fine if you plan on checking for empty strings. However, consider explicitly using a pointer (e.g.,*string) to differentiate missing vs. empty states for more explicit null semantics in code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/view/preview/preview.tsx(6 hunks)frontend/app/view/preview/sync.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)
🔇 Additional comments (6)
frontend/app/view/preview/sync.ts (2)
23-50: Good subscription management inuseTerminalPreviewSync.This React hook correctly sets up and tears down the subscription to the terminal block when the linked terminal ID changes or when the component unmounts, preventing memory leaks. The approach appears robust and consistent with React best practices.
55-59: Verify that terminal objects always exist before unlinking.When unlinking a terminal (lines 59-63), the code assumes the preview block is valid. Consider verifying the terminal block also exists and is in a valid state before clearing the linkage, to avoid edge-case issues or race conditions if the terminal block is missing.
pkg/waveobj/metaconsts.go (1)
40-40: Constant naming is consistent.
MetaKey_PreviewLinkedTerminalfollows the naming pattern of existingMetaKey_*constants. This helps maintain readability and consistency across the codebase.frontend/types/gotypes.d.ts (1)
498-498: LGTM! The new property follows the established conventions.The
"preview:linked_terminal"property is well-defined, following the existing naming pattern and type conventions in the codebase.frontend/app/view/preview/preview.tsx (2)
47-48: LGTM! Clean imports and property initialization.The new imports and property are well-organized and properly initialized.
Also applies to: 177-177
1074-1074: LGTM! Hook placement is correct.The
useTerminalPreviewSynchook is properly placed before any conditional returns in the component.
|
|
||
| menuItems.push({ type: "separator" }); | ||
| menuItems.push({ | ||
| label: "Link to Terminal (from clipboard)", | ||
| click: () => | ||
| fireAndForget(async () => { | ||
| const terminalId = await navigator.clipboard.readText(); | ||
| if (!terminalId) return; | ||
| await toggleTerminalPreviewLink(terminalId, this.blockId); | ||
| }), | ||
| }); | ||
|
|
||
| menuItems.push({ | ||
| label: "Unlink from Terminal", | ||
| click: () => | ||
| fireAndForget(async () => { | ||
| const linkedTerminalId = globalStore.get(this.blockAtom)?.meta?.["preview:linked_terminal"]; | ||
| if (linkedTerminalId) { | ||
| await toggleTerminalPreviewLink(linkedTerminalId, this.blockId); | ||
| } | ||
| }), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the terminal linking UX.
The current implementation has several areas for improvement:
- Using clipboard for terminal ID transfer is error-prone
- No validation of terminal ID format or existence
- Missing user feedback on success/failure of linking/unlinking operations
Consider implementing:
- A terminal selector modal instead of clipboard
- Validation of terminal ID before linking
- User feedback through toast notifications
menuItems.push({
- label: "Link to Terminal (from clipboard)",
+ label: "Link to Terminal",
click: () =>
fireAndForget(async () => {
- const terminalId = await navigator.clipboard.readText();
- if (!terminalId) return;
+ // Show terminal selector modal
+ const terminalId = await showTerminalSelectorModal();
+ if (!terminalId) return;
+
+ try {
await toggleTerminalPreviewLink(terminalId, this.blockId);
+ showToast("Terminal linked successfully");
+ } catch (error) {
+ showToast("Failed to link terminal: " + error.message, "error");
+ }
}),
});Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/view/preview/preview.tsx (1)
535-558: Extract terminal synchronization logic into a helper method.Consider extracting the terminal synchronization logic into a separate method to improve readability and maintainability.
Apply this diff to refactor the code:
+ private async syncTerminalDirectory(terminalId: string, newPath: string) { + try { + const fileInfo = await RpcApi.FileInfoCommand(TabRpcClient, { + info: { + path: await this.formatRemoteUri(newPath, globalStore.get), + }, + }); + if (fileInfo?.isdir) { + const terminalBlockRef = WOS.makeORef("block", terminalId); + await services.ObjectService.UpdateObjectMeta(terminalBlockRef, { + "cmd:cwd": fileInfo.dir + }); + + await RpcApi.ControllerInputCommand(TabRpcClient, { + blockid: terminalId, + inputdata64: btoa(`cd "${fileInfo.dir}"\n`), + }); + } + } catch (error) { + console.error("Failed to sync terminal directory:", error); + // Consider showing a user-friendly error notification + } + } async goHistory(newPath: string) { // ... existing code ... const linkedTerminalId = blockMeta?.["preview:linked_terminal"]; if (linkedTerminalId && !this.isUpdatingFromTerminal) { - try { - const fileInfo = await RpcApi.FileInfoCommand(TabRpcClient, { - info: { - path: await this.formatRemoteUri(newPath, globalStore.get), - }, - }); - if (fileInfo?.isdir) { - const terminalBlockRef = WOS.makeORef("block", linkedTerminalId); - await services.ObjectService.UpdateObjectMeta(terminalBlockRef, { - "cmd:cwd": fileInfo.dir - }); - - await RpcApi.ControllerInputCommand(TabRpcClient, { - blockid: linkedTerminalId, - inputdata64: btoa(`cd "${fileInfo.dir}"\n`), - }); - } - } catch (error) { - console.error("Failed to sync terminal directory:", error); - // Consider showing a user-friendly error notification - } + await this.syncTerminalDirectory(linkedTerminalId, newPath); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/view/preview/preview.tsx(6 hunks)frontend/app/view/preview/sync.ts(1 hunks)
🔇 Additional comments (6)
frontend/app/view/preview/sync.ts (3)
1-10: LGTM!The license and imports are properly defined.
11-16: LGTM!The MetaType interface is correctly extended with the necessary properties for terminal linking functionality.
18-50: 🛠️ Refactor suggestionAdd error handling for terminal synchronization.
While the synchronization logic is sound, consider adding error handling for the
goHistorycall to prevent unhandled promise rejections.Apply this diff to improve error handling:
- previewModel.goHistory(terminalCwd) - .catch(console.error) - .finally(() => { - previewModel.isUpdatingFromTerminal = false; - }); + try { + await previewModel.goHistory(terminalCwd); + } catch (err) { + console.error("Failed to sync preview with terminal directory:", err); + // Consider showing a user-friendly error notification + } finally { + previewModel.isUpdatingFromTerminal = false; + }Likely invalid or redundant comment.
frontend/app/view/preview/preview.tsx (3)
47-48: LGTM!The required functions are properly imported from the sync module.
177-177: LGTM!The
isUpdatingFromTerminalproperty is correctly added and initialized to prevent circular updates during terminal synchronization.Also applies to: 456-456
717-739: Consider enhancing the terminal linking UX.The current implementation has several areas for improvement:
- Using clipboard for terminal ID transfer is error-prone
- No validation of terminal ID format or existence
- Missing user feedback on success/failure of linking/unlinking operations
| /** | ||
| * Links or unlinks a Terminal and Preview pane for directory synchronization | ||
| */ | ||
| export async function toggleTerminalPreviewLink(terminalId: string, previewId: string) { | ||
| const previewBlockRef = WOS.makeORef("block", previewId); | ||
| const previewBlock = globalStore.get(WOS.getWaveObjectAtom(previewBlockRef)); | ||
|
|
||
| if (previewBlock?.meta?.["preview:linked_terminal"] === terminalId) { | ||
| await ObjectService.UpdateObjectMeta(previewBlockRef, { | ||
| "preview:linked_terminal": null | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await ObjectService.UpdateObjectMeta(previewBlockRef, { | ||
| "preview:linked_terminal": terminalId | ||
| }); | ||
| } catch (err) { | ||
| console.error("Failed to update preview block metadata:", err); | ||
| // Optionally display an error notification to the user | ||
| } | ||
|
|
||
| const terminalBlock = globalStore.get(WOS.getWaveObjectAtom(WOS.makeORef("block", terminalId))); | ||
| const terminalCwd = terminalBlock?.meta?.["cmd:cwd"]; | ||
| if (terminalCwd) { | ||
| const previewModel = new PreviewModel(previewId, null); | ||
| previewModel.isUpdatingFromTerminal = true; | ||
| try { | ||
| await previewModel.goHistory(terminalCwd); | ||
| } finally { | ||
| previewModel.isUpdatingFromTerminal = false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for terminal and preview IDs.
The function should validate the input parameters to ensure they are non-empty strings and follow the expected format.
Apply this diff to add input validation:
export async function toggleTerminalPreviewLink(terminalId: string, previewId: string) {
+ if (!terminalId?.trim() || !previewId?.trim()) {
+ throw new Error("Terminal ID and Preview ID must be non-empty strings");
+ }
+
const previewBlockRef = WOS.makeORef("block", previewId);
const previewBlock = globalStore.get(WOS.getWaveObjectAtom(previewBlockRef));📝 Committable suggestion
‼️ 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.
| /** | |
| * Links or unlinks a Terminal and Preview pane for directory synchronization | |
| */ | |
| export async function toggleTerminalPreviewLink(terminalId: string, previewId: string) { | |
| const previewBlockRef = WOS.makeORef("block", previewId); | |
| const previewBlock = globalStore.get(WOS.getWaveObjectAtom(previewBlockRef)); | |
| if (previewBlock?.meta?.["preview:linked_terminal"] === terminalId) { | |
| await ObjectService.UpdateObjectMeta(previewBlockRef, { | |
| "preview:linked_terminal": null | |
| }); | |
| return; | |
| } | |
| try { | |
| await ObjectService.UpdateObjectMeta(previewBlockRef, { | |
| "preview:linked_terminal": terminalId | |
| }); | |
| } catch (err) { | |
| console.error("Failed to update preview block metadata:", err); | |
| // Optionally display an error notification to the user | |
| } | |
| const terminalBlock = globalStore.get(WOS.getWaveObjectAtom(WOS.makeORef("block", terminalId))); | |
| const terminalCwd = terminalBlock?.meta?.["cmd:cwd"]; | |
| if (terminalCwd) { | |
| const previewModel = new PreviewModel(previewId, null); | |
| previewModel.isUpdatingFromTerminal = true; | |
| try { | |
| await previewModel.goHistory(terminalCwd); | |
| } finally { | |
| previewModel.isUpdatingFromTerminal = false; | |
| } | |
| } | |
| } | |
| /** | |
| * Links or unlinks a Terminal and Preview pane for directory synchronization | |
| */ | |
| export async function toggleTerminalPreviewLink(terminalId: string, previewId: string) { | |
| if (!terminalId?.trim() || !previewId?.trim()) { | |
| throw new Error("Terminal ID and Preview ID must be non-empty strings"); | |
| } | |
| const previewBlockRef = WOS.makeORef("block", previewId); | |
| const previewBlock = globalStore.get(WOS.getWaveObjectAtom(previewBlockRef)); | |
| if (previewBlock?.meta?.["preview:linked_terminal"] === terminalId) { | |
| await ObjectService.UpdateObjectMeta(previewBlockRef, { | |
| "preview:linked_terminal": null | |
| }); | |
| return; | |
| } | |
| try { | |
| await ObjectService.UpdateObjectMeta(previewBlockRef, { | |
| "preview:linked_terminal": terminalId | |
| }); | |
| } catch (err) { | |
| console.error("Failed to update preview block metadata:", err); | |
| // Optionally display an error notification to the user | |
| } | |
| const terminalBlock = globalStore.get(WOS.getWaveObjectAtom(WOS.makeORef("block", terminalId))); | |
| const terminalCwd = terminalBlock?.meta?.["cmd:cwd"]; | |
| if (terminalCwd) { | |
| const previewModel = new PreviewModel(previewId, null); | |
| previewModel.isUpdatingFromTerminal = true; | |
| try { | |
| await previewModel.goHistory(terminalCwd); | |
| } finally { | |
| previewModel.isUpdatingFromTerminal = false; | |
| } | |
| } | |
| } |
|
Thanks for submitting! Just checked out the video and it looks cool! Will try to take a look at the implementation on Monday |
Introduces new features for linking preview and terminal blocks:
Great project! I really wanted this feature, so I implemented it. I'm willing to refactor my code in any way necessary -- but only after my functional programming exam :)
Please see the attached video for an explanation on what the PR tries to accomplish.
CleanShot.2025-01-26.at.00.06.49.mp4