File explorer bookmarks (folder / file / document-position)#3443
File explorer bookmarks (folder / file / document-position)#3443dandacompany wants to merge 12 commits into
Conversation
Inline the markdown content so OverlayScrollbars no longer remounts every render, track viewport scrollTop via the scroll event, and restore it in a layout effect after content updates so refreshing an edited document keeps the reader's position instead of jumping to the top. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the preview mock's FullConfigType literal after the new required filebookmarks field (and pre-existing version/buildtime gaps) so the mock type-checks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThe change adds persisted file bookmarks with default configuration, Go configuration helpers, RPC handlers, generated clients, and frontend RPC wrappers. The frontend adds sorted bookmark state, CRUD/reorder operations, context menus, an edit modal, directory actions, and preview integration. Markdown and editor previews capture and restore document locations, while Markdown gains imperative anchor scrolling, current-anchor detection, and scroll preservation. Unit tests cover bookmark ordering, menu helpers, configuration updates, and anchor selection. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 Checkov (3.3.8)pkg/wconfig/defaultconfig/filebookmarks.jsonTraceback (most recent call last): 🔧 Biome (2.5.3)frontend/types/gotypes.d.tsFile contains syntax errors that prevent linting: Line 1898: Expected a property, or a signature but instead found '#'.; Line 1898: Expected an expression, or an assignment but instead found ':'.; Line 1898: Expected an expression but instead found ']'.; Line 2211: Expected a statement but instead found '}'. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/store/bookmarksmodel.ts`:
- Around line 79-95: Update reorder in the bookmark model to persist the entire
ordering through a single transactional or bulk reorder RPC instead of issuing
one SetFileBookmarkCommand per bookmark. Preserve the orderedKeys mapping and
display:order values, and ensure a failed operation leaves all bookmark orders
unchanged.
In `@frontend/app/view/preview/preview-model.tsx`:
- Around line 604-614: Update openBookmark so cross-connection bookmark opens
use the same goHistory navigation path as same-connection opens, while still
applying the required connection and file metadata updates. Ensure the forward
history is reset or updated consistently so both bookmark-opening branches
participate in back/forward navigation.
In `@pkg/wconfig/settingsconfig.go`:
- Around line 938-956: Serialize the full read-modify-write transaction in
SetFileBookmarkConfigValue and DeleteFileBookmarkConfigValue, rather than
locking only through WriteWaveHomeConfigFile. Add or reuse an atomic
fileBookmark update helper that holds the shared lock across
ReadWaveHomeConfigFile, UpsertFileBookmarkInMap or RemoveFileBookmarkInMap, and
the write, then route both functions through it.
🪄 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 Plus
Run ID: 87be89a8-9a5f-4cdd-a13e-614371a65814
📒 Files selected for processing (22)
frontend/app/element/markdown-anchor.test.tsfrontend/app/element/markdown-anchor.tsfrontend/app/element/markdown.tsxfrontend/app/modals/modalregistry.tsxfrontend/app/store/bookmarksmodel.test.tsfrontend/app/store/bookmarksmodel.tsfrontend/app/store/wshclientapi.tsfrontend/app/view/preview/bookmarks-edit-modal.tsxfrontend/app/view/preview/bookmarks-menu.test.tsfrontend/app/view/preview/bookmarks-menu.tsfrontend/app/view/preview/preview-directory.tsxfrontend/app/view/preview/preview-edit.tsxfrontend/app/view/preview/preview-markdown.tsxfrontend/app/view/preview/preview-model.tsxfrontend/preview/mock/defaultconfig.tsfrontend/types/gotypes.d.tspkg/wconfig/defaultconfig/filebookmarks.jsonpkg/wconfig/filebookmarks_test.gopkg/wconfig/settingsconfig.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.go
| async reorder(orderedKeys: string[]): Promise<void> { | ||
| const byKey = new Map(this.getBookmarks().map((b) => [b.key, b])); | ||
| for (let i = 0; i < orderedKeys.length; i++) { | ||
| const b = byKey.get(orderedKeys[i]); | ||
| if (b == null) { | ||
| continue; | ||
| } | ||
| const newOrder = i + 1; | ||
| if ((b["display:order"] ?? 0) == newOrder) { | ||
| continue; | ||
| } | ||
| const { key: _omit, ...rest } = b; | ||
| await RpcApi.SetFileBookmarkCommand(TabRpcClient, { | ||
| key: b.key, | ||
| bookmark: { ...rest, "display:order": newOrder }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make reorder persistence atomic.
This loop issues one full bookmark update per item. If a later RPC fails, earlier order changes remain persisted, leaving a partially reordered list. Use a transactional/bulk reorder operation rather than independent SetFileBookmarkCommand calls.
🤖 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/store/bookmarksmodel.ts` around lines 79 - 95, Update reorder in
the bookmark model to persist the entire ordering through a single transactional
or bulk reorder RPC instead of issuing one SetFileBookmarkCommand per bookmark.
Preserve the orderedKeys mapping and display:order values, and ensure a failed
operation leaves all bookmark orders unchanged.
| async openBookmark(bm: BookmarkEntry) { | ||
| const currentConn = globalStore.get(this.blockAtom)?.meta?.connection ?? ""; | ||
| const bmConn = bm.connection ?? ""; | ||
| if (bmConn != currentConn) { | ||
| const blockOref = WOS.makeORef("block", this.blockId); | ||
| await this.env.services.object.UpdateObjectMeta(blockOref, { connection: bmConn, file: bm.path }); | ||
| globalStore.set(this.fileContentSaved, null); | ||
| globalStore.set(this.newFileContent, null); | ||
| } else { | ||
| await this.goHistory(bm.path); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect goHistory/goHistoryBack/goHistoryForward implementations to see what meta fields they manage.
rg -n -A 20 'function goHistory\b' --type=ts
rg -n -A 20 'function goHistoryBack\b' --type=tsRepository: wavetermdev/waveterm
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files matching preview-model:"
git ls-files | rg 'preview-model\.tsx$' || true
echo
echo "Search history-related identifiers:"
rg -n 'goHistory|history|goHistoryBack|goHistoryForward|Cmd.*Arrow|ArrowLeft|ArrowRight|OpenBookmark|openBookmark' --type ts --type tsx .Repository: wavetermdev/waveterm
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -u
echo "Locate preview-model.tsx:"
git ls-files | rg 'frontend/app/view/preview/preview-model\.tsx$' || true
echo
echo "Search history-related identifiers:"
rg -n 'goHistory|history|goHistoryBack|goHistoryForward|Cmd.*Arrow|ArrowLeft|ArrowRight|OpenBookmark|openBookmark' frontend/app/view/preview/preview-model.tsx frontend/app --glob '*.ts' --glob '*.tsx' || true
echo
echo "Show surrounding preview-model section:"
sed -n '560,640p' frontend/app/view/preview/preview-model.tsxRepository: wavetermdev/waveterm
Length of output: 9658
🏁 Script executed:
#!/bin/bash
set -u
echo "Find historyutil source:"
git ls-files | rg 'historyutil\.(ts|tsx)$' || true
echo
echo "historyutil implementation:"
sed -n '1,260p' frontend/app/util/historyutil.ts
echo
echo "Preview model top and relevant history helpers:"
sed -n '1,80p' frontend/app/view/preview/preview-model.tsx
sed -n '860,915p' frontend/app/view/preview/preview-model.tsxRepository: wavetermdev/waveterm
Length of output: 5222
🏁 Script executed:
#!/bin/bash
set -u
echo "Locate historyutil:"
git ls-files | rg '(^|/)historyutil\.(ts|tsx)$' || true
echo
FOUND=$(git ls-files | rg 'historyutil\.(ts|tsx)$' | head -n 1)
if [ -n "$FOUND" ]; then
echo "historyutil implementation:"
sed -n '1,260p' "$FOUND"
echo
echo "bookmark connection/path metadata usages:"
rg -n 'bookmark(type|type)?|connection|bookmarks|OpenBookmark|openBookmark|Cmd:ArrowLeft|Cmd:ArrowRight' frontend --glob '*.ts' --glob '*.tsx' || true
fiRepository: wavetermdev/waveterm
Length of output: 29625
Treat cross-connection bookmark opens the same way as normal navigation
Same-connection bookmark opens are pushed onto /forward history by goHistory, but cross-connection opens update block meta with only { connection, file }. Update the connection/path via the same goHistory path (or reset history:forward explicitly) so back/forward navigation doesn’t ignore cross-connection bookmark opens.
🤖 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/view/preview/preview-model.tsx` around lines 604 - 614, Update
openBookmark so cross-connection bookmark opens use the same goHistory
navigation path as same-connection opens, while still applying the required
connection and file metadata updates. Ensure the forward history is reset or
updated consistently so both bookmark-opening branches participate in
back/forward navigation.
| func SetFileBookmarkConfigValue(key string, bookmark FileBookmark) error { | ||
| m, cerrs := ReadWaveHomeConfigFile(FileBookmarksFile) | ||
| if len(cerrs) > 0 { | ||
| return fmt.Errorf("error reading config file: %v", cerrs[0]) | ||
| } | ||
| m, err := UpsertFileBookmarkInMap(m, key, bookmark) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return WriteWaveHomeConfigFile(FileBookmarksFile, m) | ||
| } | ||
|
|
||
| func DeleteFileBookmarkConfigValue(key string) error { | ||
| m, cerrs := ReadWaveHomeConfigFile(FileBookmarksFile) | ||
| if len(cerrs) > 0 { | ||
| return fmt.Errorf("error reading config file: %v", cerrs[0]) | ||
| } | ||
| m = RemoveFileBookmarkInMap(m, key) | ||
| return WriteWaveHomeConfigFile(FileBookmarksFile, m) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether bookmark persistence has transaction-level serialization.
rg -n -C 8 'func (ReadWaveHomeConfigFile|WriteWaveHomeConfigFile|SetFileBookmarkConfigValue|DeleteFileBookmarkConfigValue)' pkg/wconfig/settingsconfig.go
rg -n -C 5 'SetFileBookmarkCommand|DeleteFileBookmarkCommand' pkg/wshrpc/wshserver/wshserver.go
rg -n -i 'mutex|rwmutex|flock|lock' pkg/wconfig/settingsconfig.goRepository: wavetermdev/waveterm
Length of output: 3628
🏁 Script executed:
#!/bin/bash
# Inspect the full read/write/filebookmarks helpers and call graph/lock types.
set -eu
sed -n '1,80p' pkg/wconfig/settingsconfig.go
sed -n '520,615p' pkg/wconfig/settingsconfig.go
sed -n '900,960p' pkg/wconfig/settingsconfig.go
rg -n 'type .*File.*Bookmark|var .*Bookmark|func .*FileBookmark|DeleteFileBookmark|SetFileBookmark|UpsertFileBookmarkInMap|RemoveFileBookmarkInMap|FileBookmarksFile' pkgRepository: wavetermdev/waveterm
Length of output: 10888
Serialize the full fileBookmark RMW transaction.
WriteWaveHomeConfigFile locks only the write step, while SetFileBookmarkConfigValue/DeleteFileBookmarkConfigValue read, mutate, and then write filebookmarks.json separately. Concurrent add/reorder/delete RPCs can interleave and overwrite each other; hold a shared lock/read-modify-write guard around the entire operation with an atomic update helper.
🤖 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 `@pkg/wconfig/settingsconfig.go` around lines 938 - 956, Serialize the full
read-modify-write transaction in SetFileBookmarkConfigValue and
DeleteFileBookmarkConfigValue, rather than locking only through
WriteWaveHomeConfigFile. Add or reuse an atomic fileBookmark update helper that
holds the shared lock across ReadWaveHomeConfigFile, UpsertFileBookmarkInMap or
RemoveFileBookmarkInMap, and the write, then route both functions through it.
1 participant
What
Adds a bookmarks feature to the file preview/explorer: save and quickly jump to folders, files, or a specific position inside a document.
Bookmarks are stored globally in a new
filebookmarks.jsonconfig collection, following the existingWebBookmark/map + display:orderconvention. This also replaces the hard-codedBOOKMARKSarray inpreview-model.tsx(the// TODO drive this using config) with config-driven defaults.Why
The preview file explorer had a hard-coded folder-shortcut list and no way for users to save their own locations. This puts frequently-used folders, files, and document positions one click away.
How
pkg/wconfig,pkg/wshrpc):FileBookmarktype +filebookmarks.jsoncollection; RPCsSetFileBookmarkCommand/DeleteFileBookmarkCommand.BookmarksModelsingleton + derivedfileBookmarksAtom;PreviewModelgainsopenBookmark/addCurrentLocationBookmark/showBookmarkMenuand a star button inendIconButtons; document-position capture/restore via an imperativeMarkdownHandle(heading slug) and the Monaco editor line; an edit modal registered in the modal registry.Also included is a small related fix: preserve Markdown preview scroll position across refresh. The preview was defining
ScrollableMarkdown/NonScrollableMarkdowninside render, which remounted theOverlayScrollbarscontainer every render and reset scroll to the top; this inlines them and restores the viewport scroll after content updates.Testing
pkg/wconfig).BookmarksModelordering,pickCurrentAnchor, and the menu builder.Notes
task generate(no manual edits to generated files).