web bookmarks #1930
web bookmarks #1930
Conversation
…ergonomic by adding a wrapper class
|
Warning Rate limit exceeded@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces changes across both frontend and backend components. In the frontend, updates include the addition of a new prop to a UI component, modifications in suggestion controls, and the removal of an obsolete autocomplete component. Adjustments to keyboard event handling (e.g., adding a new key binding) and refinements in CSS theme variables are also present. Backend modifications encompass updates to dependency versions, enhancements to type definitions for suggestions and bookmarks, and the implementation of a favicon caching mechanism. Additionally, suggestion fetching logic was expanded to handle both file and bookmark suggestions, with accompanying changes to structural data and method signatures. A new server command for processing suggestion requests has been added as well, and metadata configurations have been extended to support web-specific keys. 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 (8)
frontend/app/suggestion/suggestion.tsx (3)
11-21: Consider a default handler for onTab prop.
The interface isOpen, onClose, onSelect, and onTab are clearly defined. However, there's no default or fallback behavior for onTab, which might lead to TypeError if not specified.
84-110: Potential repeated icon handling.
You switch on suggestion.iconsrc, suggestion.icon, and suggestion.type. Refactoring into a single icon resolver function may reduce branching and improve readability.
151-165: Duplicate structure in NoResults and NoData.
These two components share nearly identical markup. Factor them into a single component with parameterization (or one component with variant props) to reduce duplication.pkg/suggestion/suggestion.go (2)
131-139: Add fallback for unrecognized suggestion types.
Currently, the code returns an error for unsupported suggestion types. If the product roadmap expects additional suggestion types in the future, consider a robust extension approach or a default case.
141-303: Bookmark suggestions are well-structured.
The logic for fuzzy matching (title and URL) and sorting according to the highest score is correct. However, watch out for large-scale scenarios with thousands of bookmarks, as iterating over all might impact performance.pkg/faviconcache/faviconcache.go (1)
166-180: Cache stubs can scale later.
The current in-memory cache is straightforward. If you plan to scale to multiple nodes or a persistent store, consider redis or an on-disk approach.pkg/wconfig/settingsconfig.go (1)
128-135: Consider adding URL validation and normalization.The
Urlfield should be validated and normalized to ensure it's a valid URL and consistently formatted. Consider:
- Adding validation in the setter/constructor
- Normalizing URLs (e.g., adding missing schemes, handling relative URLs)
- Documenting URL format requirements
Example validation function:
func (b *WebBookmark) ValidateURL() error { if b.Url == "" { return fmt.Errorf("url is required") } u, err := url.Parse(b.Url) if err != nil { return fmt.Errorf("invalid url: %v", err) } if u.Scheme == "" { u.Scheme = "https" } b.Url = u.String() return nil }frontend/app/view/webview/webview.tsx (1)
483-488: Clean up commented code and clarify intent.The commented out
return falsesuggests uncertainty about the implementation. Either remove the comment or document why it might be needed in the future.- // return false; // commented out for now
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
frontend/app/block/block.tsx(1 hunks)frontend/app/block/blockframe.tsx(1 hunks)frontend/app/store/keymodel.ts(1 hunks)frontend/app/suggestion/suggestion.tsx(1 hunks)frontend/app/typeahead/typeahead.tsx(0 hunks)frontend/app/view/preview/preview.tsx(3 hunks)frontend/app/view/webview/webview.tsx(8 hunks)frontend/app/workspace/workspace.tsx(1 hunks)frontend/tailwindsetup.css(1 hunks)frontend/types/gotypes.d.ts(4 hunks)go.mod(1 hunks)pkg/faviconcache/faviconcache.go(1 hunks)pkg/suggestion/suggestion.go(3 hunks)pkg/util/utilfn/utilfn.go(1 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)pkg/wconfig/settingsconfig.go(2 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/app/typeahead/typeahead.tsx
✅ Files skipped from review due to trivial changes (3)
- frontend/app/block/blockframe.tsx
- frontend/app/store/keymodel.ts
- frontend/app/workspace/workspace.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (23)
frontend/tailwindsetup.css (2)
28-32: New Color Variables Added and Naming Consistency CheckThe additions of the new CSS variables:
• --color-modalbg: #232323
• --color-accentbg: rgba(88, 193, 66, 0.5)
• --color-hoverbg: rgba(255, 255, 255, 0.2)
• --color-accent: rgb(88, 193, 66)
• --color-accenthover: rgb(118, 223, 96)are clear and align with the existing naming convention seen in earlier declarations (e.g., --color-accent-400). Please ensure that these variables are referenced correctly in your React components (especially in styling classes like hover:bg-hoverbg) so that the intended color scheme and hover effects reflect consistently across the application.
26-27: Ensure Consistency with Removed VariablesAccording to the updated design, the removed variable (--color-highlightbg) has been replaced by --color-hoverbg. It is important to confirm that all component code (e.g., in workspace.tsx) referencing the old variable now points to --color-hoverbg. Double-check these references to prevent any broken styling or inconsistencies in the UI.
Also applies to: 28-32
frontend/app/suggestion/suggestion.tsx (4)
28-40: Check early return conditions.
Returning null when isOpen, anchorRef.current, or fetchSuggestions is falsy prevents rendering. Ensure upstream logic always sets these props to valid values, or provide debugging info if suggestions are unexpectedly not shown.
42-67: Highlight function logic looks sound.
The approach handles edge cases (null or empty target) gracefully, and positions are applied correctly.
130-145: Ensure correct React dependency in useEffect.
The effect depends on props.blockRef.current. For clarity and future maintainability, consider using [props.blockRef] as a dependency or verifying that the current usage won't skip updates.
167-315: Comprehensive suggestion logic.
The SuggestionControlInner component correctly handles query changes, fuzzy fetch, keyboard navigation, and merges custom empty states. It looks solid regarding concurrency since fetchSuggestions is invoked in a controlled effect.pkg/suggestion/suggestion.go (1)
421-432: Consistent naming in suggestion fields.
Using Display, FileName, and FileMimeType consistently aligns with other suggestion types. This ensures clarity across the front-end.pkg/faviconcache/faviconcache.go (1)
35-44: Lock usage is appropriate.
The fetchLock and fetching map usage ensures only one fetch per domain. The approach looks correct for avoiding concurrency collisions.pkg/waveobj/metaconsts.go (1)
109-109: LGTM!The new constant follows the established naming convention and is placed appropriately with other web-related constants.
pkg/waveobj/wtypemeta.go (1)
110-112: LGTM!The new
WebPartitionfield is well-placed with other web-related fields and follows the established pattern with appropriate JSON tags and types.frontend/app/block/block.tsx (1)
91-91: LGTM!The
blockRefprop is properly added to theWebViewcomponent, maintaining consistency with other components likePreviewViewthat use block references.pkg/wconfig/settingsconfig.go (2)
128-135: LGTM!The
WebBookmarkstruct is well-designed with appropriate fields and types, following consistent naming conventions and proper use ofomitemptytags.
145-145: LGTM!The
Bookmarksfield inFullConfigTypeis properly defined as a map with string keys andWebBookmarkvalues.pkg/wshrpc/wshrpctypes.go (1)
740-755: LGTM! Well-structured enhancements to SuggestionType.The changes enhance the suggestion system with additional display capabilities while maintaining consistent naming patterns.
pkg/wshrpc/wshserver/wshserver.go (1)
866-868: LGTM! Clean implementation of FetchSuggestionsCommand.The implementation follows the established RPC command pattern and properly handles errors.
frontend/app/view/webview/webview.tsx (2)
606-664: LGTM! Well-implemented BookmarkTypeahead component.The component provides excellent user experience with:
- Clear error states and user guidance
- Proper type safety
- Consistent styling
675-675: Consider adding type validation for webPartition.The partition value is used directly without validation. Consider adding type safety and validation to ensure only valid partition values are used.
frontend/types/gotypes.d.ts (3)
1290-1298: LGTM! Well-structured type definition for web bookmarks.The
WebBookmarktype provides a comprehensive structure for storing bookmark metadata with appropriate optional fields.
783-794: LGTM! Enhanced suggestion type with improved search capabilities.The updated
SuggestionTypeincludes necessary fields for:
- Display formatting (
display,subtext)- Icon handling (
icon,iconcolor,iconsrc)- Search relevance (
matchpos,submatchpos,score)- File and URL metadata
584-584: LGTM! Added web partition support.The
web:partitionproperty inMetaTypeenables isolation between different web contexts.frontend/app/view/preview/preview.tsx (2)
1133-1139: LGTM! Well-implemented tab completion handler.The
handleTabfunction provides intuitive behavior by appending a slash to directory names, improving the user experience.
1151-1159: LGTM! Clean migration to BlockHeaderSuggestionControl.The component replacement maintains existing functionality while adding tab completion support.
go.mod (1)
97-98: Verify compatibility with updated dependencies.The minor version updates to
golang.org/x/syncandgolang.org/x/textshould be checked for breaking changes.✅ Verification successful
🌐 Web query:
What are the changes in golang.org/x/sync v0.11.0 and golang.org/x/text v0.22.0?💡 Result:
The provided search results don't contain specific details about **golang.org/x/sync v0.11.0** or **golang.org/x/text v0.22.0**. However, we can analyze related information from adjacent versions and dependency updates: ### For golang.org/x/sync - Recent updates focus on concurrency primitives like improvements to `errgroup`, `semaphore`, and `singleflight` packages[10] - Version **v0.7.0** appears in multiple dependency manifests ([6][9]) - Version **v0.8.0** was part of a GitLab MR with standard dependency validation checks[1] - No specific release notes found for **v0.11.0** in indexed sources ### For golang.org/x/text - Version **v0.22.0** appears in a GitLab MR as a routine dependency update[8] - The catalog package (part of x/text) added features like: - `Builder` type for programmatic catalog creation[2] - Improved message formatting contexts[2] - Language variant handling improvements[2] - Earlier versions show security-focused updates like: - Unicode standard updates - Localization format fixes ### Key Observations 1. Both packages follow Go's standard versioning where **v1+** indicates stability[7] 2. Dependency updates typically focus on: - Security patches - Concurrency bug fixes - Internationalization improvements - Performance optimizations For exact changes in these specific versions, you should consult: 1. Official Go package documentation 2. GitHub release notes for [golang/sync](https://github.com/golang/sync/releases) and [golang/text](https://github.com/golang/text/releases) 3. Go module proxy details via `go list -m -json golang.org/x/[package]@[version]` Citations: - 1: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/7153 - 2: https://pkg.go.dev/golang.org/x/text/message/catalog - 3: https://cert-manager.io/docs/releases/release-notes/release-notes-1.14/ - 4: https://github.com/golang/tools/releases - 5: https://dev.golang.org/release - 6: https://chromium.googlesource.com/infra/infra/+/main/go/src/infra/go.mod - 7: https://go.dev/ref/mod - 8: https://gitlab.com/gitlab-org/cli/-/merge_requests/1900 - 9: https://catalog.rad.security/images/quay.io/prometheus/alertmanager/digests/sha256:8f627f71d3e34f4c3c5f16940d05251dfaabb64a611e0b5b6c775f162746bdfb?comparisonDigest=sha256%3A45c3a586d4332d710bef92b06e90380c0b1f588968c00100d4b5e4e44c40ca5f - 10: https://pkg.go.dev/golang.org/x/sync
Verify compatibility of dependency updates.
The minor version bumps for both packages (golang.org/x/sync v0.11.0 and golang.org/x/text v0.22.0) suggest that the updates are focused on bug fixes, improvements, and non-breaking enhancements. Although no explicit breaking changes were found in the available information, it's advisable to consult the official release notes if any edge-case issues are observed.
- golang.org/x/sync v0.11.0: Focuses on improvements to concurrency primitives. No breaking changes have been noted.
- golang.org/x/text v0.22.0: Introduces enhancements for internationalization and catalog features, following expected minor update behavior.
pkg/faviconcache/faviconcache.go
Outdated
| // triggerAsyncFetch starts a goroutine to update the favicon cache | ||
| // for the given domain if one isn’t already in progress. | ||
| func triggerAsyncFetch(domain string) { | ||
| fetchLock.Lock() | ||
| if fetching[domain] { | ||
| // Already fetching this domain; nothing to do. | ||
| fetchLock.Unlock() | ||
| return | ||
| } | ||
| // Mark this domain as in-flight. | ||
| fetching[domain] = true | ||
| fetchLock.Unlock() | ||
|
|
||
| go func() { | ||
| // When done, ensure that we clear the “fetching” flag. | ||
| defer func() { | ||
| fetchLock.Lock() | ||
| delete(fetching, domain) | ||
| fetchLock.Unlock() | ||
| }() | ||
|
|
||
| iconStr, err := fetchFavicon(domain) | ||
| if err != nil { | ||
| log.Printf("triggerAsyncFetch: error fetching favicon for %s: %v", domain, err) | ||
| } | ||
| SetInCache(domain, FaviconCacheItem{Data: iconStr, LastFetched: time.Now()}) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider limiting parallel domains in triggerAsyncFetch.
While this approach avoids multiple fetches per domain, a large number of unique domains might spawn many goroutines. A semaphore or controlled concurrency approach could help in high-load environments.
pkg/faviconcache/faviconcache.go
Outdated
| func fetchFavicon(domain string) (string, error) { | ||
| // Create a context that times out after 5 seconds. | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| // Special case for github.com - use their dark favicon from assets domain | ||
| url := "https://" + domain + "/favicon.ico" | ||
| if domain == "github.com" { | ||
| url = "https://github.githubassets.com/favicons/favicon-dark.png" | ||
| } | ||
|
|
||
| // Create a new HTTP request with the context. | ||
| req, err := http.NewRequestWithContext(ctx, "GET", url, nil) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error creating request for %s: %w", url, err) | ||
| } | ||
|
|
||
| // Execute the HTTP request. | ||
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error fetching favicon from %s: %w", url, err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Ensure we got a 200 OK. | ||
| if resp.StatusCode != http.StatusOK { | ||
| return "", fmt.Errorf("non-OK HTTP status: %d fetching %s", resp.StatusCode, url) | ||
| } | ||
|
|
||
| // Read the favicon bytes. | ||
| data, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error reading favicon data from %s: %w", url, err) | ||
| } | ||
|
|
||
| // Encode the image bytes to base64. | ||
| b64Data := base64.StdEncoding.EncodeToString(data) | ||
| if len(b64Data) > maxIconSize { | ||
| return "", fmt.Errorf("favicon too large: %d bytes", len(b64Data)) | ||
| } | ||
|
|
||
| // Try to detect MIME type from Content-Type header first | ||
| mimeType := resp.Header.Get("Content-Type") | ||
| if mimeType == "" { | ||
| // If no Content-Type header, detect from content | ||
| mimeType = http.DetectContentType(data) | ||
| } | ||
|
|
||
| return "data:" + mimeType + ";base64," + b64Data, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Validate Content-Type more strictly.
Currently, non-200 responses or large data are handled. Consider adding checks to handle suspicious or malicious responses (e.g., verifying MIME type is image-based).
+ if !strings.HasPrefix(mimeType, "image/") {
+ return "", fmt.Errorf("unexpected MIME type: %s", mimeType)
+ }
pkg/util/utilfn/utilfn.go
Outdated
| func GetKeys[T any](m map[string]T) []string { | ||
| keys := make([]string, 0, len(m)) | ||
| for key := range m { | ||
| keys = append(keys, key) | ||
| } | ||
| return keys | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant GetKeys function.
This function is redundant with the existing GetMapKeys function at line 552. The GetMapKeys function is more flexible as it works with any comparable key type, while this new function is limited to string keys only. Consider removing this function and using GetMapKeys instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/suggestion/suggestion.go (2)
141-303: Consider breaking down the function into smaller, focused functions.The function is quite long and handles multiple responsibilities. Consider extracting the fuzzy matching logic and scoring into separate functions for better maintainability.
+func fuzzyMatchBookmark(bookmark wconfig.WebBookmark, patternRunes []rune, slab *util.Slab) (score int, titlePos []int, urlPos []int) { + if bookmark.Title != "" { + candidateTitle := strings.ToLower(bookmark.Title) + textTitle := util.ToChars([]byte(candidateTitle)) + resultTitle, titlePositionsPtr := algo.FuzzyMatchV2(false, true, true, &textTitle, patternRunes, true, slab) + if titlePositionsPtr != nil { + titlePos = *titlePositionsPtr + } + score = resultTitle.Score + } + + candidateUrl := strings.ToLower(bookmark.Url) + textUrl := util.ToChars([]byte(candidateUrl)) + resultUrl, urlPositionsPtr := algo.FuzzyMatchV2(false, true, true, &textUrl, patternRunes, true, slab) + urlScore := resultUrl.Score + if urlPositionsPtr != nil { + urlPos = *urlPositionsPtr + } + + if urlScore > score { + score = urlScore + } + return +}
294-296: Extract magic numbers into named constants.The maximum number of suggestions (50) should be defined as a named constant at the package level for better maintainability and reusability.
+const maxSuggestions = 50 - if len(suggestions) >= 50 { + if len(suggestions) >= maxSuggestions {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/suggestion/suggestion.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (3)
pkg/suggestion/suggestion.go (3)
17-21: LGTM!The new imports for faviconcache and wconfig packages are necessary for the bookmark suggestion functionality.
131-139: LGTM!The function cleanly routes requests to type-specific handlers with proper error handling for unsupported types.
305-434: LGTM!The function has been cleanly refactored to handle file-specific suggestions while maintaining the existing functionality and edge case handling.
| UrlUrl: entry.bookmark.Url, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate bookmark URLs before processing.
Consider validating the bookmark URLs to ensure they are well-formed before processing them.
🏁 Script executed:
#!/bin/bash
# Check if URL validation is implemented elsewhere in the codebase
rg -A 5 'func.*ValidateURL|url\.Parse'Length of output: 399
Subject: Implement URL Validation for Bookmarks in pkg/suggestion
The code in pkg/suggestion/suggestion.go is directly using the bookmark URL (entry.bookmark.Url) without any validation. While other parts of the code (e.g., in pkg/faviconcache/faviconcache.go) perform URL parsing and error checking using url.Parse, there is currently no such validation in the suggestion package. It is recommended to add URL validation before processing these bookmark URLs to ensure they are well-formed.
- Insert URL validation (e.g., using url.Parse followed by error checks) in pkg/suggestion/suggestion.go where the bookmark URL is used.
- Align the URL validation logic with the implementation already present in pkg/faviconcache.
No description provided.