Add Tab Lock feature: prevent close, lock icon and tab color#3353
Open
norarat-devsmith wants to merge 4 commits into
Open
Add Tab Lock feature: prevent close, lock icon and tab color#3353norarat-devsmith wants to merge 4 commits into
norarat-devsmith wants to merge 4 commits into
Conversation
Adds a per-tab "Tab Lock" toggle accessible from the tab context menu. When a tab is locked: - it cannot be closed via the close button, the context-menu Close Tab item, or the Cmd/Ctrl+W shortcut (all three entry points are guarded, plus a backend guard in WorkspaceService.CloseTab) - the close button is replaced by an amber lock icon - the tab gets a subtle amber tint Lock state is stored in tab meta under the new "tab:locked" key, mirroring the existing "tab:flagcolor" pattern, so no new RPC is needed (uses SetMetaCommand). Works in both the top and left tab bars. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
|
Contributor
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds end-to-end tab locking: a new "tab:locked" metadata key (types and backend constant/struct), a TabLockedColor and isTabLocked() helper, CSS for .tab.locked, TabV and VTab changes to render a lock icon and hide the close control when locked, context-menu lock/unlock checkbox (disables Close when locked), UI close-path guards (tabbar, vtabbar, keymodel), a backend guard in WorkspaceService.CloseTab that errors for locked tabs, and a preview component demonstrating locked tabs. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
![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: 1
🧹 Nitpick comments (2)
frontend/app/tab/vtab.tsx (1)
180-182: ⚡ Quick winUse the TabLockedColor constant instead of hardcoding the color.
The tint overlay hardcodes
#FFB400in Tailwind's arbitrary value syntax, butTabLockedColoris already imported. This duplicates the color definition: ifTabLockedColoris updated, this line must also be changed manually.♻️ Refactor to use the constant with inline styles
- {locked && ( - <div className="pointer-events-none absolute inset-x-1 inset-y-[4px] rounded-sm bg-[`#FFB400`]/10 ring-1 ring-inset ring-[`#FFB400`]/40" /> - )} + {locked && ( + <div + className="pointer-events-none absolute inset-x-1 inset-y-[4px] rounded-sm ring-1 ring-inset" + style={{ + backgroundColor: `${TabLockedColor}1a`, + borderColor: `${TabLockedColor}66`, + }} + /> + )}(Hex opacity:
1a≈ 10%,66≈ 40%)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/tab/vtab.tsx` around lines 180 - 182, Replace the hardcoded `#FFB400` in the overlay div with the imported `TabLockedColor`: locate the conditional div (the element rendered when `locked` is true) and remove the Tailwind arbitrary hex usages in the className; instead apply inline styles that derive from `TabLockedColor` with the proper opacity hex suffixes (10% -> `1a` for background, 40% -> `66` for ring/border color) or set `backgroundColor: TabLockedColor + '1a'` and the ring/border color to `TabLockedColor + '66'`, keeping the positioning and rounded classes intact so the visual and layout behavior (pointer-events-none, absolute, inset-x/inset-y, rounded-sm) remain unchanged.frontend/app/tab/tab.scss (1)
97-98: ⚡ Quick winConsider defining the locked tab color as a CSS variable.
The amber color
rgb(255 180 0 / ...)is hardcoded here and duplicates theTabLockedColor = "#FFB400"constant fromtablock.ts. If the locked color changes, both locations need updating.♻️ Suggested refactor using a CSS custom property
Define a CSS variable in a global scope (e.g., in your root styles or theme file):
:root { --tab-locked-color: 255 180 0; /* `#FFB400` as RGB components */ }Then reference it here:
&.locked { .tab-inner { - box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45); - background: rgb(255 180 0 / 0.08); + box-shadow: inset 0 0 0 1px rgb(var(--tab-locked-color) / 0.45); + background: rgb(var(--tab-locked-color) / 0.08); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/tab/tab.scss` around lines 97 - 98, The amber color is hardcoded in tab.scss and duplicates the TabLockedColor constant in tablock.ts; add a CSS custom property (e.g., --tab-locked-color as RGB components) in your global/root styles, replace the hardcoded rgb(255 180 0 / ...) usages in tab.scss (both box-shadow and background) to use that CSS variable with the appropriate alpha, and then update tablock.ts to either read from the stylesheet (if runtime is needed) or remove/derive the duplicate TabLockedColor constant so there is a single source of truth for the locked-tab color.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/app/tab/tab.scss`:
- Around line 95-114: The hover rule body:not(.nohover) .tab:hover .tab-inner
overrides the locked styling in .tab.locked .tab-inner; modify the hover rule so
it does not apply to locked tabs (e.g., add :not(.locked) to the hover selector)
so that .tab.locked .tab-inner keeps its amber border/background on hover;
update the selector used for hover overrides (body:not(.nohover) .tab:hover
.tab-inner) to exclude .locked tabs (or increase specificity to prefer
.tab.locked .tab-inner) and verify .lock-icon and .close behaviors remain
unchanged.
---
Nitpick comments:
In `@frontend/app/tab/tab.scss`:
- Around line 97-98: The amber color is hardcoded in tab.scss and duplicates the
TabLockedColor constant in tablock.ts; add a CSS custom property (e.g.,
--tab-locked-color as RGB components) in your global/root styles, replace the
hardcoded rgb(255 180 0 / ...) usages in tab.scss (both box-shadow and
background) to use that CSS variable with the appropriate alpha, and then update
tablock.ts to either read from the stylesheet (if runtime is needed) or
remove/derive the duplicate TabLockedColor constant so there is a single source
of truth for the locked-tab color.
In `@frontend/app/tab/vtab.tsx`:
- Around line 180-182: Replace the hardcoded `#FFB400` in the overlay div with
the imported `TabLockedColor`: locate the conditional div (the element rendered
when `locked` is true) and remove the Tailwind arbitrary hex usages in the
className; instead apply inline styles that derive from `TabLockedColor` with
the proper opacity hex suffixes (10% -> `1a` for background, 40% -> `66` for
ring/border color) or set `backgroundColor: TabLockedColor + '1a'` and the
ring/border color to `TabLockedColor + '66'`, keeping the positioning and
rounded classes intact so the visual and layout behavior (pointer-events-none,
absolute, inset-x/inset-y, rounded-sm) remain unchanged.
🪄 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: cf9cc350-b76e-44c7-831b-29aa74f16515
📒 Files selected for processing (13)
frontend/app/store/keymodel.tsfrontend/app/tab/tab.scssfrontend/app/tab/tab.tsxfrontend/app/tab/tabbar.tsxfrontend/app/tab/tabcontextmenu.tsfrontend/app/tab/tablock.tsfrontend/app/tab/vtab.test.tsxfrontend/app/tab/vtab.tsxfrontend/app/tab/vtabbar.tsxfrontend/types/gotypes.d.tspkg/service/workspaceservice/workspaceservice.gopkg/waveobj/metaconsts.gopkg/waveobj/wtypemeta.go
Comment on lines
+95
to
+114
| &.locked { | ||
| .tab-inner { | ||
| box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45); | ||
| background: rgb(255 180 0 / 0.08); | ||
| } | ||
|
|
||
| .close { | ||
| display: none; | ||
| } | ||
|
|
||
| .lock-icon { | ||
| position: absolute; | ||
| top: 50%; | ||
| right: 6px; | ||
| transform: translate3d(0, -50%, 0); | ||
| z-index: var(--zindex-tab-name); | ||
| font-size: 11px; | ||
| pointer-events: none; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Hover state will override the locked tab styling.
The locked tab background and border styling will be overridden when the user hovers over a locked tab because body:not(.nohover) .tab:hover .tab-inner (specificity 0,4,1) has higher specificity than .tab.locked .tab-inner (specificity 0,3,0). This means hovering over a locked tab will hide its amber visual indicator, degrading the UX.
🎨 Proposed fix to preserve locked styling on hover
&.locked {
.tab-inner {
box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45);
background: rgb(255 180 0 / 0.08);
}
.close {
display: none;
}
.lock-icon {
position: absolute;
top: 50%;
right: 6px;
transform: translate3d(0, -50%, 0);
z-index: var(--zindex-tab-name);
font-size: 11px;
pointer-events: none;
}
}
+
+ &.locked:hover .tab-inner,
+ &.locked.dragging .tab-inner {
+ box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45);
+ background: rgb(255 180 0 / 0.08);
+ }
}Alternatively, wrap the hover-specific overrides with :not(.locked):
body:not(.nohover) .tab:hover:not(.locked) {
.tab-inner {
border-color: transparent;
background: rgb(from var(--main-text-color) r g b / 0.1);
}
}📝 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.
Suggested change
| &.locked { | |
| .tab-inner { | |
| box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45); | |
| background: rgb(255 180 0 / 0.08); | |
| } | |
| .close { | |
| display: none; | |
| } | |
| .lock-icon { | |
| position: absolute; | |
| top: 50%; | |
| right: 6px; | |
| transform: translate3d(0, -50%, 0); | |
| z-index: var(--zindex-tab-name); | |
| font-size: 11px; | |
| pointer-events: none; | |
| } | |
| } | |
| &.locked { | |
| .tab-inner { | |
| box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45); | |
| background: rgb(255 180 0 / 0.08); | |
| } | |
| .close { | |
| display: none; | |
| } | |
| .lock-icon { | |
| position: absolute; | |
| top: 50%; | |
| right: 6px; | |
| transform: translate3d(0, -50%, 0); | |
| z-index: var(--zindex-tab-name); | |
| font-size: 11px; | |
| pointer-events: none; | |
| } | |
| } | |
| &.locked:hover .tab-inner, | |
| &.locked.dragging .tab-inner { | |
| box-shadow: inset 0 0 0 1px rgb(255 180 0 / 0.45); | |
| background: rgb(255 180 0 / 0.08); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/app/tab/tab.scss` around lines 95 - 114, The hover rule
body:not(.nohover) .tab:hover .tab-inner overrides the locked styling in
.tab.locked .tab-inner; modify the hover rule so it does not apply to locked
tabs (e.g., add :not(.locked) to the hover selector) so that .tab.locked
.tab-inner keeps its amber border/background on hover; update the selector used
for hover overrides (body:not(.nohover) .tab:hover .tab-inner) to exclude
.locked tabs (or increase specificity to prefer .tab.locked .tab-inner) and
verify .lock-icon and .close behaviors remain unchanged.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/preview/previews/tablock.preview.tsx (1)
32-72: ⚡ Quick winConsider declarative positioning instead of imperative DOM manipulation.
The current approach uses
useEffectto mutate DOM styles after render. This can be simplified by computing the transform during render and passing it as an inline style prop, which is more idiomatic in React and avoids potential layout shifts.♻️ Suggested refactor
function TopBar() { - const tabRefs = useRef<Record<string, HTMLDivElement | null>>({}); - useEffect(() => { - topTabs.forEach((tab, index) => { - const el = tabRefs.current[tab.tabId]; - if (el) { - el.style.opacity = "1"; - el.style.transform = `translate3d(${index * TAB_WIDTH}px, 0, 0)`; - } - }); - }, []); return ( <div style={{ position: "relative", width: TAB_WIDTH * topTabs.length, height: TAB_HEIGHT }}> {topTabs.map((tab, index) => { const activeIndex = topTabs.findIndex((t) => t.active); const showDivider = index !== 0 && !tab.active && index !== activeIndex + 1; return ( <TabV key={tab.tabId} - ref={(el) => { - tabRefs.current[tab.tabId] = el; - }} + style={{ + transform: `translate3d(${index * TAB_WIDTH}px, 0, 0)`, + opacity: 1 + }} tabId={tab.tabId} tabName={tab.tabName} active={tab.active} showDivider={showDivider} isDragging={false} tabWidth={TAB_WIDTH} isNew={false} locked={tab.locked} onClick={noop} onClose={noop} onDragStart={noop} onContextMenu={noop} onRename={noop} /> ); })} </div> ); }Additionally, you can optimize
activeIndexcalculation by hoisting it outside the map:function TopBar() { + const activeIndex = topTabs.findIndex((t) => t.active); return ( <div style={{ position: "relative", width: TAB_WIDTH * topTabs.length, height: TAB_HEIGHT }}> {topTabs.map((tab, index) => { - const activeIndex = topTabs.findIndex((t) => t.active); const showDivider = index !== 0 && !tab.active && index !== activeIndex + 1; return ( <TabV🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/preview/previews/tablock.preview.tsx` around lines 32 - 72, The TopBar component currently mutates DOM in useEffect via tabRefs to set opacity and translate3d; instead compute each tab's transform and opacity declaratively during render and pass them as style props to TabV (remove the imperative tabRefs use and the useEffect), and hoist the activeIndex = topTabs.findIndex(...) above the topTabs.map to avoid recomputing. Ensure you still pass the same props (tabId, tabName, active, showDivider, isDragging, tabWidth, etc.) but add a style prop like { transform: `translate3d(${index * TAB_WIDTH}px,0,0)`, opacity: 1 } for each TabV and remove tabRefs and related handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/preview/previews/tablock.preview.tsx`:
- Around line 32-72: The TopBar component currently mutates DOM in useEffect via
tabRefs to set opacity and translate3d; instead compute each tab's transform and
opacity declaratively during render and pass them as style props to TabV (remove
the imperative tabRefs use and the useEffect), and hoist the activeIndex =
topTabs.findIndex(...) above the topTabs.map to avoid recomputing. Ensure you
still pass the same props (tabId, tabName, active, showDivider, isDragging,
tabWidth, etc.) but add a style prop like { transform: `translate3d(${index *
TAB_WIDTH}px,0,0)`, opacity: 1 } for each TabV and remove tabRefs and related
handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 423c84bd-e24f-40f8-9dd7-bdbcc97e07dc
📒 Files selected for processing (1)
frontend/preview/previews/tablock.preview.tsx
Document the new lock helper, close-guards, CloseTab's lock behavior, and preview helpers to satisfy docstring coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/tab/vtab.test.tsx (1)
68-83: 💤 Low valueConsider adding tests for withClose=false to verify lock icon visibility without onClose.
The current tests verify locked and unlocked tabs when
withClose=true. For completeness, consider adding tests to verify:
- Locked tab with
withClose=falseshould still render the lock icon (since lock rendering is independent ofonClose).- Unlocked tab with
withClose=falseshould render neither lock nor close button.This would ensure the lock icon's independence from the
onClosehandler is explicitly tested.📋 Suggested additional test cases
+ + it("shows lock icon even without onClose handler", () => { + const markup = renderVTab({ id: "tab-5", name: "Locked", locked: true }, false); + + expect(markup).toContain("fa-lock"); + expect(markup).toContain(TabLockedColor); + expect(markup).not.toContain("fa-xmark"); + }); + + it("shows neither lock nor close when unlocked without onClose", () => { + const markup = renderVTab({ id: "tab-6", name: "Normal" }, false); + + expect(markup).not.toContain("fa-lock"); + expect(markup).not.toContain("fa-xmark"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/tab/vtab.test.tsx` around lines 68 - 83, Add unit tests for renderVTab/VTab when withClose is false to assert lock icon rendering independent of onClose: create a test that renders a locked tab via renderVTab({ id: "tab-x", name: "...", locked: true }, false) and assert markup contains "fa-lock" and TabLockedColor; add another test rendering an unlocked tab with withClose=false and assert markup contains neither "fa-lock" nor "fa-xmark". Ensure tests reference renderVTab, withClose (second arg), fa-lock, fa-xmark and TabLockedColor so the behavior of lock rendering without an onClose handler is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/app/tab/vtab.test.tsx`:
- Around line 68-83: Add unit tests for renderVTab/VTab when withClose is false
to assert lock icon rendering independent of onClose: create a test that renders
a locked tab via renderVTab({ id: "tab-x", name: "...", locked: true }, false)
and assert markup contains "fa-lock" and TabLockedColor; add another test
rendering an unlocked tab with withClose=false and assert markup contains
neither "fa-lock" nor "fa-xmark". Ensure tests reference renderVTab, withClose
(second arg), fa-lock, fa-xmark and TabLockedColor so the behavior of lock
rendering without an onClose handler is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0645282-6da8-449f-a664-245ce713c964
📒 Files selected for processing (6)
frontend/app/store/keymodel.tsfrontend/app/tab/tabbar.tsxfrontend/app/tab/tablock.tsfrontend/app/tab/vtab.test.tsxfrontend/preview/previews/tablock.preview.tsxpkg/service/workspaceservice/workspaceservice.go
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/app/tab/tabbar.tsx
- frontend/app/store/keymodel.ts
- frontend/preview/previews/tablock.preview.tsx
- frontend/app/tab/tablock.ts
Document TabV, TabInner, VTab, VTabWrapper, and VTabBar to clear the remaining docstring-coverage gap. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.
Preview
Lock / unlock interaction (right-click → Lock Tab). A locked tab shows an amber padlock in place of the close button, gets an amber tint, and can no longer be closed:
In both the top and left tab bars:
Summary
Adds a per-tab Tab Lock toggle, accessible from the tab right-click menu. Locking a tab protects it from being closed accidentally and gives it a clear visual indicator.
When a tab is locked:
Cmd/Ctrl+Wshortcut. All three frontend entry points are guarded, plus a backend guard inWorkspaceService.CloseTabas defense in depth.Works in both the top (horizontal) and left (vertical) tab bars.
Implementation
Lock state is stored in tab
metaunder a newtab:lockedkey, mirroring the existingtab:flagcolorpattern — so no new RPC is required (it uses the existingSetMetaCommand), and it persists/syncs like other tab meta.Backend (Go)
pkg/waveobj/metaconsts.go—MetaKey_TabLocked = "tab:locked"pkg/waveobj/wtypemeta.go—TabLocked boolmeta fieldpkg/service/workspaceservice/workspaceservice.go—CloseTabrefuses a locked tabfrontend/types/gotypes.d.ts— regenerated viatask generateFrontend (TS/React)
frontend/app/tab/tablock.ts(new) —isTabLocked()helper +TabLockedColorfrontend/app/tab/tabcontextmenu.ts— Lock/Unlock toggle; disabled Close Tab when lockedfrontend/app/tab/tab.tsx+tab.scss— top tab bar visualsfrontend/app/tab/vtab.tsx/vtabbar.tsx— left tab bar visualsfrontend/app/tab/tabbar.tsx,vtabbar.tsx,store/keymodel.ts— guard close entry pointsNote: window-close still closes locked tabs by design — locking only blocks per-tab close, not quitting.
Testing
go vet ./pkg/service/workspaceservice/ ./pkg/waveobj/— cleantsc --noEmit— no errors in changed filesvitest run frontend/app/tab/vtab.test.tsx— 4/4 pass (added 2 tests covering locked rendering)🤖 Generated with Claude Code