[issues/280] Hide Bookmarks feature behind setting (beta)#367
Merged
Conversation
Foundation for hiding Bookmarks behind an opt-in beta flag. The setting defaults to `false` so bookmarks are invisible to users in 1.1.0 — subsequent commits will wire the `when` clauses and runtime guards that read this value.
…ses (#280) When `features.bookmarks.enabled` is false (the default), bookmark commands vanish from the Command Palette, keybindings are inert, and the editor context menu "Save Selection as Bookmark" entry is hidden. Uses the standard VSCode `config.rangelink.features.bookmarks.enabled` when-clause pattern so the UI layer handles visibility declaratively — no runtime code needed for these touchpoints.
…#280) BookmarkService now carries a `visible` flag (from the `features.bookmarks.enabled` setting). The status bar checks `isVisible()` to decide whether to render bookmark items in R-M. All bookmark code still initializes — commands are always registered (package.json `when` clauses handle Command Palette / keybinding visibility). This keeps SCM noise low and avoids optional params. TODO: #366 markers added at all gate points for easy cleanup when bookmarks graduates from beta in 2.0.0.
5 locations wrapped in `<!-- -->` so bookmark documentation is invisible on GitHub/npmjs but preserved in git for #366: - Bookmarks bullet in the R-M menu description - Full Bookmarks section (Save/List instructions) - Two command table rows (R-B-S, R-B-L) - Editor context menu row (Save Selection as Bookmark) - Smart Padding pasteBookmark setting row
3 locations wrapped in `<!-- -->` in the [Unreleased] section: - "List Bookmarks / Manage Bookmarks" menu item bullet - "Save Selection as Bookmark" context menu entry - Full "Bookmarks System" feature entry (R-B-S, R-B-L) All tagged with TODO: #366 for easy cleanup in 2.0.0.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughA bookmarks feature flag is introduced with default value false, gating all bookmark UI elements, commands, and menus behind the configuration Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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 |
![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 (1)
packages/rangelink-vscode-extension/src/extension.ts (1)
138-138: Debug log doesn't distinguish default vs. user-configured path.-logger.debug({ fn: 'activate', bookmarksEnabled }, 'Bookmarks feature flag read'); +logger.debug( + { fn: 'activate', bookmarksEnabled, isDefault: bookmarksEnabled === DEFAULT_FEATURES_BOOKMARKS_ENABLED }, + 'Bookmarks feature flag read', +);As per coding guidelines, "Log at DEBUG level when using default vs provided values to indicate which path was taken."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/extension.ts` at line 138, The debug log at logger.debug({ fn: 'activate', bookmarksEnabled }, 'Bookmarks feature flag read') doesn't indicate whether a default path was used or a user-specified one; update the log to include the resolved bookmarks path and a flag (e.g., bookmarksPath and bookmarksPathIsDefault) so the activate flow shows which branch was taken. Locate the code that resolves the bookmarks path (in the activate function) and augment the logger.debug call to include those symbols so the log clearly distinguishes default vs user-configured path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rangelink-vscode-extension/CHANGELOG.md`:
- Line 63: The "Editor Content" line currently has multiple sub-items condensed
onto one line; split that single run-on line into separate markdown list entries
so each command is its own bullet like the other blocks (e.g., break the text
into separate indented lines for "RangeLink: Copy Range Link", "RangeLink: Copy
Range Link (Absolute)", "RangeLink: Copy Portable Link", "RangeLink: Copy
Portable Link (Absolute)", and "RangeLink: Paste Selected Text"), matching the
indentation/style used on lines 67–71 and preserving the surrounding
TODO/context.
In `@packages/rangelink-vscode-extension/README.md`:
- Around line 271-278: Remove the blank lines immediately before and after the
HTML comment blocks that hide unreleased rows in the README table so the
Markdown table remains contiguous; specifically, edit the commands table block
containing the comment around the "Save Selection as Bookmark" / "List
Bookmarks" rows (the <!-- TODO: `#366` ... --> block) and the similar comment
blocks near the "Editor Content" and "Smart Padding" tables so there are no
empty lines adjacent to the HTML comments, preserving the header/separator + row
sequence and ensuring subsequent rows like "Go to Link <sup>Unreleased</sup>"
render as table rows.
In `@packages/rangelink-vscode-extension/src/extension.ts`:
- Around line 133-138: The bookmarksEnabled flag is captured once at activation
and never updated, causing BookmarkService.isVisible() to stay stale; update the
implementation so visibility is read dynamically: change
BookmarkService.isVisible() to query
configReader.getBoolean(SETTING_FEATURES_BOOKMARKS_ENABLED,
DEFAULT_FEATURES_BOOKMARKS_ENABLED) each call instead of relying on the
activation-time bookmarksEnabled value, or alternatively add an
onDidChangeConfiguration listener in extension.ts that updates the
BookmarkService visibility state (and prompts reload if needed); reference the
bookmarksEnabled variable in extension.ts and the BookmarkService.isVisible()
method to locate where to remove the frozen flag and perform the dynamic read or
the config change wiring.
---
Nitpick comments:
In `@packages/rangelink-vscode-extension/src/extension.ts`:
- Line 138: The debug log at logger.debug({ fn: 'activate', bookmarksEnabled },
'Bookmarks feature flag read') doesn't indicate whether a default path was used
or a user-specified one; update the log to include the resolved bookmarks path
and a flag (e.g., bookmarksPath and bookmarksPathIsDefault) so the activate flow
shows which branch was taken. Locate the code that resolves the bookmarks path
(in the activate function) and augment the logger.debug call to include those
symbols so the log clearly distinguishes default vs user-configured path.
Comment on lines
133
to
138
| // TODO: #366 remove feature flag when bookmarks graduates from beta | ||
| const bookmarksEnabled = configReader.getBoolean( | ||
| SETTING_FEATURES_BOOKMARKS_ENABLED, | ||
| DEFAULT_FEATURES_BOOKMARKS_ENABLED, | ||
| ); | ||
| logger.debug({ fn: 'activate', bookmarksEnabled }, 'Bookmarks feature flag read'); |
There was a problem hiding this comment.
bookmarksEnabled is captured once at activation; no config-change listener is registered.
The when clauses in package.json are re-evaluated by VS Code dynamically — toggling rangelink.features.bookmarks.enabled in settings will immediately show/hide bookmark commands in the palette, keybindings, and context menu. However, bookmarkService.isVisible() always returns the activation-time value, so the status-bar bookmark section stays out of sync until a window reload. The disable path is worse: commands disappear from all UI surfaces, but the status bar still renders bookmark items.
Preferred fix — make isVisible() read from configReader dynamically inside BookmarkService instead of storing a frozen flag:
🔧 Option A — dynamic read in BookmarkService.isVisible() (no listener needed)
In BookmarkService.ts:
constructor(
private readonly bookmarksStore: BookmarksStore,
private readonly ideAdapter: VscodeAdapter,
private readonly configReader: ConfigReader,
private readonly destinationManager: PasteDestinationManager,
private readonly logger: Logger,
- private readonly visible: boolean = true,
) { ... }
isVisible(): boolean {
- return this.visible;
+ return this.configReader.getBoolean(
+ SETTING_FEATURES_BOOKMARKS_ENABLED,
+ DEFAULT_FEATURES_BOOKMARKS_ENABLED,
+ );
}In extension.ts:
-const bookmarksEnabled = configReader.getBoolean(
- SETTING_FEATURES_BOOKMARKS_ENABLED,
- DEFAULT_FEATURES_BOOKMARKS_ENABLED,
-);
-logger.debug({ fn: 'activate', bookmarksEnabled }, 'Bookmarks feature flag read');
-
const bookmarksStore = new BookmarksStore(context.globalState, logger);
const bookmarkService = new BookmarkService(
bookmarksStore,
ideAdapter,
configReader,
destinationManager,
logger,
- bookmarksEnabled,
);🔧 Option B — onDidChangeConfiguration listener with reload prompt (minimal invasiveness)
+context.subscriptions.push(
+ vscode.workspace.onDidChangeConfiguration((e) => {
+ if (e.affectsConfiguration(`rangelink.${SETTING_FEATURES_BOOKMARKS_ENABLED}`)) {
+ // TODO: `#366` remove when bookmarks graduates from beta
+ void ideAdapter.showInformationMessage(
+ 'Reload the window to apply Bookmarks visibility changes.',
+ 'Reload Window',
+ ).then((action) => {
+ if (action === 'Reload Window') {
+ void vscode.commands.executeCommand('workbench.action.reloadWindow');
+ }
+ });
+ }
+ }),
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rangelink-vscode-extension/src/extension.ts` around lines 133 - 138,
The bookmarksEnabled flag is captured once at activation and never updated,
causing BookmarkService.isVisible() to stay stale; update the implementation so
visibility is read dynamically: change BookmarkService.isVisible() to query
configReader.getBoolean(SETTING_FEATURES_BOOKMARKS_ENABLED,
DEFAULT_FEATURES_BOOKMARKS_ENABLED) each call instead of relying on the
activation-time bookmarksEnabled value, or alternatively add an
onDidChangeConfiguration listener in extension.ts that updates the
BookmarkService visibility state (and prompts reload if needed); reference the
bookmarksEnabled variable in extension.ts and the BookmarkService.isVisible()
method to locate where to remove the frozen flag and perform the dynamic read or
the config change wiring.
Addresses CodeRabbit review on PR #367: - Split CHANGELOG line 63 back into individual bullets (was a collapsed run-on line that broke markdown rendering) - Remove blank lines around HTML comment blocks in README tables (blank lines terminate GFM tables, causing rows to render as plain text) - Make BookmarkService.isVisible() read from configReader dynamically instead of storing a frozen activation-time flag — the R-M menu now stays in sync when users toggle the setting without reload Ignored Feedback: - Debug log isDefault flag: ConfigReader.getWithDefault() already logs "using default" vs "using configured value" at DEBUG level; adding isDefault to the caller would duplicate this information Ref: #367 (review)
HTML comments inside GFM tables terminate the table — rows after the comment render as plain text on GitHub. Moved all hidden bookmark rows out of their tables into standalone `<!-- -->` blocks below each section, with instructions for where to re-insert them. Also removes `prettier-ignore-start/end` directives that were working around the same issue less cleanly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rangelink-vscode-extension/CHANGELOG.md`:
- Around line 84-94: Update the TODO comment that currently reads "TODO: `#366`
unhide when bookmarks graduates from beta" to use correct subject-verb agreement
by changing "graduates" to "graduate" (i.e., "TODO: `#366` unhide when bookmarks
graduate from beta"); locate the phrase in the existing commented block
containing the bookmarks TODO and replace only that word to preserve the rest of
the comment.
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
1 participant
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.
Summary
Hides the Bookmarks feature behind an opt-in
features.bookmarks.enabledsetting (default:false) so it ships as dormant code in 1.1.0. A two-layer gate — declarativewhenclauses inpackage.jsonfor UI visibility, plusBookmarkService.isVisible()for the R-M status bar menu — ensures bookmarks are completely invisible to users unless they manually enable the setting. All bookmark code remains in git and initializes normally to minimize SCM noise.Changes
rangelink.features.bookmarks.enabled(boolean, defaultfalse) with constants insettingKeys.tsandsettingDefaults.tswhenclauses: Bookmark keybindings (R-B-S, R-B-L), editor context menu (Save Selection as Bookmark), and Command Palette entries gated byconfig.rangelink.features.bookmarks.enabledvisibleflag onBookmarkServiceconstructor, read from the setting at activation. Status bar menu checksisVisible()to conditionally render the bookmark section in R-M<!-- -->HTML comments (Bookmarks section, command table rows, context menu row, Smart Padding row, R-M menu bullet)<!-- -->HTML commentsTest Plan
features.bookmarks.enabledcontract test inpackageJsonContracts.test.ts, bookmarkwhenclause tests for keybindings/commandPalette/context menusBookmarkServicemock helper updated withisVisible()— all StatusBar tests passRelated
Summary by CodeRabbit
New Features
Documentation