-
Notifications
You must be signed in to change notification settings - Fork 736
feat: Tab base directory with VS Code style redesign #2789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
sgeraldes
wants to merge
5
commits into
wavetermdev:main
Choose a base branch
from
sgeraldes:feat/tab-base-directory-redesign
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5622d24
feat: Tab base directory with VS Code style redesign
sebastiangeraldes 4421ab3
fix: address code review findings for tab base directory
sebastiangeraldes fb2ab9e
fix: add OSC 7 debounce cleanup to tabbar UI close handler
sebastiangeraldes 53ecce0
fix: address CodeRabbit review findings
sebastiangeraldes 1953327
fix: address additional CodeRabbit review findings
sebastiangeraldes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| # Changes Summary: Race Condition Fixes with Optimistic Locking | ||
|
|
||
| ## Overview | ||
|
|
||
| This implementation addresses race conditions in tab metadata updates (spec-004) by implementing optimistic locking with version checking. The changes prevent TOCTOU (Time-Of-Check-Time-Of-Use) vulnerabilities in concurrent metadata operations. | ||
|
|
||
| ## Files Modified | ||
|
|
||
| ### Backend (Go) | ||
|
|
||
| #### `pkg/wstore/wstore_dbops.go` | ||
| - Added `ErrVersionMismatch` error variable for concurrent modification detection | ||
| - Added `ErrObjectLocked` error variable for lock state rejection | ||
|
|
||
| #### `pkg/wstore/wstore.go` | ||
| - Added `UpdateObjectMetaWithVersion()` function: | ||
| - Performs optimistic locking update with version checking | ||
| - If `expectedVersion > 0` and doesn't match current version, returns `ErrVersionMismatch` | ||
| - If `expectedVersion == 0`, behaves like `UpdateObjectMeta` (no version check) | ||
|
|
||
| - Added `UpdateObjectMetaIfNotLocked()` function: | ||
| - Atomically checks lock and updates metadata | ||
| - Lock is checked INSIDE the transaction, eliminating TOCTOU vulnerability | ||
| - Returns `ErrObjectLocked` (wrapped in `ErrVersionMismatch`) if locked | ||
| - Returns `ErrVersionMismatch` if version doesn't match | ||
|
|
||
| #### `pkg/service/objectservice/objectservice.go` | ||
| - Added `UpdateObjectMetaWithVersion()` RPC service method | ||
| - Added `UpdateObjectMetaIfNotLocked()` RPC service method | ||
| - Both methods include proper metadata annotations for TypeScript binding generation | ||
|
|
||
| ### Frontend (TypeScript) | ||
|
|
||
| #### `frontend/app/view/term/termwrap.ts` | ||
| - Added debounce map (`osc7DebounceMap`) for OSC 7 updates per tab | ||
| - Added `OSC7_DEBOUNCE_MS = 300` constant for debounce delay | ||
| - Added `clearOsc7Debounce()` helper function | ||
| - Added `cleanupOsc7DebounceForTab()` exported function for memory leak prevention | ||
| - Updated `handleOsc7Command()` to: | ||
| - Add null safety check for `tabData?.oid` | ||
| - Use debouncing to reduce race condition window | ||
| - Use atomic lock-aware update (`UpdateObjectMetaIfNotLocked`) instead of regular update | ||
| - Gracefully handle version mismatch and locked state errors | ||
|
|
||
| #### `frontend/app/tab/tab.tsx` | ||
| - Added `getApi` to imports from `@/app/store/global` (fix for pre-existing missing import) | ||
|
|
||
| ### Generated Files | ||
|
|
||
| #### `frontend/app/store/services.ts` | ||
| - Auto-generated new TypeScript methods: | ||
| - `UpdateObjectMetaWithVersion(oref, meta, expectedVersion)` | ||
| - `UpdateObjectMetaIfNotLocked(oref, meta, lockKey, expectedVersion)` | ||
|
|
||
| ## Key Features Implemented | ||
|
|
||
| ### 1. Optimistic Locking | ||
| - Uses existing `version` field in WaveObj types | ||
| - Version checked inside transaction to prevent TOCTOU | ||
| - Atomic increment of version on successful update (already implemented in `DBUpdate`) | ||
|
|
||
| ### 2. Error Types | ||
| - **ErrVersionMismatch**: Indicates concurrent modification detected | ||
| - **ErrObjectLocked**: Indicates update rejected due to lock state | ||
| - Both errors are wrapped appropriately for consistent error handling | ||
|
|
||
| ### 3. OSC 7 Debouncing | ||
| - 300ms debounce window for rapid directory changes | ||
| - Per-tab debounce timers in a Map | ||
| - Cleanup function to prevent memory leaks on tab close | ||
|
|
||
| ### 4. Atomic Lock Checking | ||
| - Lock state checked INSIDE database transaction | ||
| - Eliminates race condition between lock check and update | ||
| - If lock is toggled during update, the update is safely rejected | ||
|
|
||
| ## Acceptance Criteria Status | ||
|
|
||
| - [x] `UpdateObjectMetaWithVersion` added to `wstore.go` | ||
| - [x] RPC endpoints added to `objectservice.go` | ||
| - [x] OSC 7 debounce map with cleanup function | ||
| - [x] Null safety guards in `termwrap.ts` | ||
| - [x] `ErrVersionMismatch` error type created | ||
| - [x] `ErrObjectLocked` error type created | ||
| - [x] TypeScript compilation passes (our files) | ||
| - [x] Go compilation passes | ||
| - [x] Changes committed | ||
|
|
||
| ## Testing Notes | ||
|
|
||
| To test the implementation: | ||
|
|
||
| 1. **Version Mismatch Test**: Open two terminals in the same tab, rapidly change directories in both - the race condition should be handled gracefully | ||
|
|
||
| 2. **Lock Bypass Test**: Toggle the lock while an OSC 7 update is in flight - the update should be rejected if lock is set | ||
|
|
||
| 3. **Debounce Test**: Rapidly `cd` between directories - only the final directory should be set as basedir | ||
|
|
||
| 4. **Memory Leak Test**: Open and close multiple tabs - the debounce map should be cleaned up | ||
|
|
||
| ## Notes | ||
|
|
||
| - The spec mentions retry logic for manual updates (handleSetBaseDir, handleToggleLock) - this was NOT implemented as the spec noted it as optional for Phase 4 and the core race condition fixes are functional without it | ||
| - Pre-existing TypeScript errors in unrelated files (streamdown.tsx, notificationpopover.tsx) remain unfixed as they are not related to this implementation | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo in OSC 7 debouncing bullet.
"directory cha..." should read "directory changes".
✏️ Proposed fix
🧰 Tools
🪛 LanguageTool
[grammar] ~67-~67: Ensure spelling is correct
Context: ... error handling ### 3. OSC 7 Debouncing - 300ms debounce window for rapid directory cha...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents