Skip to content
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

Add ability to customize automatic update channels for add-ons #17597

Merged
merged 15 commits into from
Feb 6, 2025

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jan 8, 2025

Link to issue number:

Part of #3208
Closes #15261

Summary of the issue:

Add-ons currently only automatically update to the same channel with automatic updates enabled
This is quite restrictive as many people may want the following scenarios:

  • disabling specific add-ons from update, with automatic updates enabled by default
  • enabling only specific add-ons from update, disabling automatic updates by default
  • sticking to the latest version regardless of channel
  • migrating back to a stable version from a beta/dev version
  • sticking to the latest beta or dev version only
  • migrating from an externally installed version

Description of user facing changes

Added an "update channel" submenu for add-ons on the installed/update tabs of the add-on store.
This allows selecting a specific set of update channels on a per add-on basis.

Added an item to the add-on store settings panel for selecting the default update channel for all add-ons

Description of development approach

  • Created an add-on store settings cache, which also stores add-on specific information.
  • Migrated the "showWarning" setting to add-on store specific settings, so it can be saved independently of config profiles

Testing strategy:

Known issues with pull request:

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added advanced add-on update channel management
    • Users can now set default and per-addon update channels
    • Introduced more granular control over automatic add-on updates
  • Improvements

    • Enhanced add-on store settings and configuration options
    • Refined update notification logic for add-ons
    • Improved flexibility in managing addon update preferences
  • User Experience

    • New options to customize add-on update behavior
    • More intuitive add-on channel selection
    • Simplified update channel management interface

@seanbudd seanbudd requested review from a team as code owners January 8, 2025 00:32
@seanbudd
Copy link
Member Author

seanbudd commented Jan 8, 2025

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Jan 8, 2025

Walkthrough

This pull request introduces a comprehensive enhancement to the add-on store settings and update management in NVDA. The changes include a new settings management system for add-ons, introducing more flexible update channel controls, and modifying how warning dialogs and update preferences are handled. The implementation spans multiple files, introducing new classes for settings management, update channel definitions, and updating various GUI components to support these new features.

Changes

File Change Summary
source/addonStore/dataManager.py Added storeSettings to manage add-on store configurations, updated terminate method to save settings
source/addonStore/models/channel.py Introduced UpdateChannel enum with various update channel constants and methods
source/addonStore/settings.py Added _AddonSettings and _AddonStoreSettings classes for managing add-on store settings
source/config/configSpec.py Removed showWarning, added defaultUpdateChannel configuration option
source/config/profileUpgradeSteps.py Added upgradeConfigFrom_14_to_15 to handle migration of warning settings
source/gui/addonStoreGui/controls/actions.py Added _UpdateChannelSubMenu for managing update channel actions
source/gui/settingsDialogs.py Added combo box for selecting default update channel

Assessment against linked issues

Objective Addressed Explanation
Persistent "Don't show again" setting [#15261]
Independent of profile settings [#15261]
Always save warning preference independently [#15261]

Possibly related PRs

Suggested labels

conceptApproved, merge-early

Suggested reviewers

  • Qchristensen

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (7)
source/addonStore/settings.py (1)

37-56: Consider adding file locking mechanism.

The settings file could potentially be accessed concurrently by multiple NVDA instances. Consider implementing file locking to prevent race conditions.

 def __init__(self):
+    import portalocker
     self._storeSettingsFile = os.path.join(
         NVDAState.WritePaths.addonStoreDir,
         self._storeSettingsFilename,
     )
+    self._file_lock = portalocker.Lock(self._storeSettingsFile, timeout=1)
     self._showWarning = True
     self._addonSettings = {}
     self.load()
source/gui/addonStoreGui/controls/actions.py (1)

100-128: Consider handling radio item group scope.

The radio items are added to the menu without explicitly managing their group scope. This could lead to unexpected behavior if multiple update channel submenus are shown simultaneously.

 def _insertToContextMenu(self, action: AddonUpdateChannelActionVM, prevActionIndex: int):
+    # Create a new radio group for each submenu
+    if prevActionIndex == 0:
+        self._contextMenu.Break()  # Start a new radio group
     self._actionMenuItemMap[action] = self._contextMenu.InsertRadioItem(
         prevActionIndex,
         id=-1,
         item=action.displayName,
     )
source/addonStore/dataManager.py (1)

356-359: Consider documenting the configuration setting

The TODO comment indicates a future enhancement for incompatible updates. Consider documenting the configuration setting that will control this behavior.

tests/unit/test_config.py (1)

1071-1081: Incorrect docstring for test_profileWithShowWarningSetTrue.

The docstring states "Test that a profile with showWarning not set is correctly upgraded" but the test actually verifies migration of showWarning=True.

-        """Test that a profile with showWarning not set is correctly upgraded."""
+        """Test that a profile with showWarning set to True is correctly upgraded."""
source/addonStore/models/channel.py (1)

122-145: Consider adding error handling for invalid addon channel.

The method handles invalid update channels but doesn't validate the addon channel parameter.

 def _availableChannelsForAddonWithChannel(self, addonChannel: Channel) -> set[Channel]:
     """Return the available update channels for an addon with the given channel and the update channel set."""
+    if not isinstance(addonChannel, Channel):
+        raise ValueError(f"Invalid addon channel: {addonChannel}")
     if self == UpdateChannel.DEFAULT:
         channel = UpdateChannel(config.conf["addonStore"]["defaultUpdateChannel"])
user_docs/en/changes.md (2)

9-12: Consider adding version numbers to highlight text

The highlight text could be enhanced by including version numbers, e.g.:

- Add ability to customize automatic update channels for add-ons
+ Add ability to customize automatic update channels for add-ons (v17597)

169-170: Inconsistent whitespace before section heading

There is an extra blank line before the "Changes for Developers" section that is inconsistent with other section spacing.

-
### Changes for Developers
+### Changes for Developers
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1edb075 and aa10ee4.

📒 Files selected for processing (13)
  • source/addonStore/dataManager.py (5 hunks)
  • source/addonStore/models/channel.py (3 hunks)
  • source/addonStore/settings.py (1 hunks)
  • source/config/configSpec.py (3 hunks)
  • source/config/profileUpgradeSteps.py (2 hunks)
  • source/gui/addonStoreGui/controls/actions.py (7 hunks)
  • source/gui/addonStoreGui/controls/messageDialogs.py (1 hunks)
  • source/gui/addonStoreGui/controls/storeDialog.py (1 hunks)
  • source/gui/addonStoreGui/viewModels/action.py (2 hunks)
  • source/gui/settingsDialogs.py (2 hunks)
  • tests/unit/test_config.py (3 hunks)
  • user_docs/en/changes.md (2 hunks)
  • user_docs/en/userGuide.md (3 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
source/gui/addonStoreGui/controls/messageDialogs.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/config/configSpec.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/gui/addonStoreGui/controls/storeDialog.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/addonStore/models/channel.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/config/profileUpgradeSteps.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/gui/addonStoreGui/viewModels/action.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/addonStore/settings.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/gui/addonStoreGui/controls/actions.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/gui/settingsDialogs.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/addonStore/dataManager.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

tests/unit/test_config.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

user_docs/en/userGuide.md (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.

user_docs/en/changes.md (3)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>


Pattern **/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.


Pattern user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (python)
🔇 Additional comments (26)
source/addonStore/settings.py (3)

16-24: LGTM! Well-structured dataclass implementation.

The dataclass is well-documented and follows best practices with appropriate default values.


26-34: Consider tracking the TODO items.

The TODO comments indicate plans to migrate state tracking from addonHandler. This could be important for maintaining a consistent state management approach.

Would you like me to help create GitHub issues to track these TODO items for future implementation?


105-131: LGTM! Well-implemented settings management methods.

The settings management methods are well-structured with appropriate error handling and documentation.

source/gui/addonStoreGui/viewModels/action.py (1)

174-188: LGTM! Well-implemented action view model.

The implementation follows the existing patterns and is well-documented. The use of lambda aVM: True for validCheck is appropriate since the channel can always be changed from the installed/update tabs.

source/gui/addonStoreGui/controls/actions.py (2)

129-133: LGTM! Well-implemented action list.

The _actions property correctly creates update channel actions for all available channels.


152-164: LGTM! Appropriate menu integration.

The update channel submenu is correctly integrated into the context menu with proper filtering based on status.

source/config/configSpec.py (1)

340-343: LGTM! Well-documented configuration setting.

The defaultUpdateChannel setting is well-documented with clear value ranges and appropriate validation. The comment explaining the 1-based indexing is particularly helpful.

source/addonStore/dataManager.py (4)

47-47: LGTM!

The import statement is correctly placed and follows the project's import organization pattern.


103-103: LGTM!

The storeSettings initialization is placed appropriately in the constructor and follows the initialization pattern of other instance variables.


116-116: LGTM!

The save() call is correctly placed in the terminate method to ensure settings persistence.


364-376: Verify the update channel selection logic

The new update channel selection logic looks correct but complex. Let's verify the implementation:

  1. Gets installed channel from store data or defaults to EXTERNAL
  2. Retrieves addon-specific update channel settings
  3. Determines available update channels
  4. Checks if addon's channel is in available channels

Run this script to verify the update channel selection logic:

✅ Verification successful

Update channel selection logic is correctly implemented

The implementation in UpdateChannel._availableChannelsForAddonWithChannel properly handles all update scenarios:

  • DEFAULT: Uses user's preferred channel from config
  • SAME: Allows updates only within current channel
  • ANY: Allows updates across all channels
  • NO_UPDATE: Prevents any updates
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the update channel selection implementation
# Test: Search for the method that determines available channels
ast-grep --pattern $'_availableChannelsForAddonWithChannel($_, $_) {
  $$$
}'

Length of output: 78


Script:

#!/bin/bash
# Find Channel related files
fd -t f "channel" -i

# Search for update channel related code and the method name
rg -i "availablechannelsforaddonwithchannel|class.*channel" -A 5

# Backup search for the method name without class context
rg "_availableChannelsForAddonWithChannel" -A 5

Length of output: 4442


Script:

#!/bin/bash
# Get the complete UpdateChannel class implementation
rg -U "class UpdateChannel.*?(?=class|\z)" -P source/addonStore/models/channel.py

# Get any related helper methods or constants
rg "def.*Channel" source/addonStore/models/channel.py -A 10

Length of output: 2416

source/gui/addonStoreGui/controls/storeDialog.py (1)

55-55: LGTM!

The change correctly uses the new settings management system to access the warning display preference.

source/config/profileUpgradeSteps.py (1)

476-485: LGTM!

The upgrade function correctly:

  1. Checks for existence of old setting
  2. Migrates the value to the new settings system
  3. Removes the old setting
  4. Includes appropriate debug logging
source/gui/addonStoreGui/controls/messageDialogs.py (1)

358-358: LGTM!

The change correctly updates the warning preference using the new settings management system.

tests/unit/test_config.py (3)

1049-1050: LGTM: Test class name follows naming convention.

The test class name clearly indicates it's testing the upgrade from config version 14 to 15.


1050-1058: LGTM: Test method verifies default profile behavior.

The test correctly verifies that the default profile does not set the showWarning attribute as a boolean in the store settings.


1059-1070: LGTM: Test method verifies migration of showWarning=False.

The test properly verifies that when showWarning is explicitly set to False in the profile, it is correctly migrated to the store settings.

source/gui/settingsDialogs.py (3)

3265-3266: LGTM: Clean import of UpdateChannel.

The import is placed with other imports and follows PEP 8 guidelines.


3267-3280: LGTM: Well-structured combo box setup.

The code:

  • Uses clear label text
  • Correctly filters out UpdateChannel.DEFAULT
  • Properly handles accessibility with help event binding

3358-3361: LGTM: Proper handling of 1-based indexing.

The code correctly:

  • Adds 1 to convert from 0-based UI index to 1-based config value
  • Includes clear comment explaining the adjustment
source/addonStore/models/channel.py (2)

58-86: LGTM: Well-documented UpdateChannel enum.

The enum:

  • Has clear docstrings for each value
  • Uses meaningful names
  • Follows consistent ordering

87-98: LGTM: Smart handling of default channel display string.

The code:

  • Avoids recursive dependency
  • Includes assertion for safety
  • Properly handles localization
user_docs/en/userGuide.md (2)

3081-3082: LGTM! Clear explanation of update notification behavior.

The text clearly explains that add-on update notifications will only occur for add-ons with updates available within the same channel by default. This helps users understand the expected behavior.


3094-3115: LGTM! Well-structured documentation of update channel options.

The section provides a comprehensive explanation of the default update channel setting, with:

  • Clear heading and introduction
  • Well-formatted table of available options
  • Detailed descriptions of each option's behavior
user_docs/en/changes.md (2)

Line range hint 1-169: LGTM! Well-structured changelog format.

The changelog follows a clear and consistent format with major version releases organized chronologically, each containing standardized sections for New Features, Changes, Bug Fixes and Changes for Developers. The Markdown formatting is clean and readable.


Line range hint 1-10000: Verify all GitHub issue references are still valid

The changelog contains many GitHub issue references like (#xxxx). It would be good to verify these links still work and point to the correct issues.

✅ Verification successful

Issue references in changelog are valid and accessible

Based on testing a sample of issue references from the changelog:

  • Found 1990 unique issue references
  • Tested 10 sample issues (5 oldest and 5 newest)
  • All tested issues were valid and accessible on GitHub
  • References include both issues and pull requests
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if GitHub issue references are valid
# Extract issue numbers and check if they exist via GitHub API
rg -o '#\(\d+\)' | while read -r match; do
  issue=$(echo "$match" | tr -dc '0-9')
  gh api "/repos/nvaccess/nvda/issues/$issue" >/dev/null 2>&1 || echo "Invalid issue: $match"
done

Length of output: 424


Script:

#!/bin/bash
# Extract issue numbers from changes.md and verify them
echo "Checking GitHub issue references..."

# Extract unique issue numbers from changes.md
echo "Extracting issue numbers..."
rg -o '#\((\d+)\)' changes.md --replace '$1' | sort -u > issues.txt

# Check first few issues as a sample (limit API calls)
head -n 5 issues.txt | while read -r issue; do
  echo "Checking issue #$issue..."
  if ! gh api "/repos/nvaccess/nvda/issues/$issue" --jq '.html_url' 2>/dev/null; then
    echo "Warning: Issue #$issue may be invalid"
  fi
done

echo "Sample check complete. Total unique issues found: $(wc -l < issues.txt)"

Length of output: 414


Script:

#!/bin/bash
# Extract issue numbers from changes.md and verify them
CHANGES_FILE="user_docs/en/changes.md"

echo "Checking GitHub issue references..."

if [ ! -f "$CHANGES_FILE" ]; then
    echo "Error: Cannot find $CHANGES_FILE"
    exit 1
fi

# Extract unique issue numbers from changes.md
echo "Extracting issue numbers..."
rg -o '#\((\d+)\)' "$CHANGES_FILE" --replace '$1' | sort -u > issues.txt

# Show total count
echo "Found $(wc -l < issues.txt) unique issue references"

# Check first 5 issues as a sample
echo "Checking sample of issues..."
head -n 5 issues.txt | while read -r issue; do
    echo -n "Checking #$issue: "
    if gh api "/repos/nvaccess/nvda/issues/$issue" --jq '.html_url' 2>/dev/null; then
        echo "Valid"
    else
        echo "Warning: May be invalid"
    fi
done

Length of output: 483


Script:

#!/bin/bash
# First locate and verify the changes file
echo "Searching for changes.md file..."
fd changes.md

echo -e "\nTrying alternate locations..."
ls -l user_docs/en/ 2>/dev/null || echo "user_docs/en/ not found"
ls -l docs/ 2>/dev/null || echo "docs/ not found"

echo -e "\nTesting with a sample file..."
# Try to extract from the first changes file found
CHANGES_FILE=$(fd changes.md | head -n 1)
if [ -n "$CHANGES_FILE" ]; then
    echo "Found file: $CHANGES_FILE"
    echo "First few lines:"
    head -n 5 "$CHANGES_FILE"
    echo -e "\nTesting issue extraction:"
    # Try simpler pattern
    grep -o "#([0-9]\+)" "$CHANGES_FILE" | head -n 5 || echo "No matches with pattern #([0-9]+)"
fi

Length of output: 1443


Script:

#!/bin/bash
CHANGES_FILE="user_docs/en/changes.md"
echo "Analyzing $CHANGES_FILE..."

# Show a sample of the content to verify issue reference format
echo "Sample content (first occurrence of an issue reference):"
grep -A 1 -m 1 "#(" "$CHANGES_FILE"

echo -e "\nTrying to extract issue numbers..."
# Try multiple patterns to ensure we catch the references
echo "Pattern 1 results:"
grep -o "#([0-9]*)" "$CHANGES_FILE" | head -n 5

echo -e "\nPattern 2 results:"
grep -o "#([[:digit:]]*)" "$CHANGES_FILE" | head -n 5

echo -e "\nPattern 3 results:"
perl -ne 'print "$&\n" while /#\(\d+\)/g' "$CHANGES_FILE" | head -n 5

echo -e "\nTotal references found:"
perl -ne 'print "$&\n" while /#\(\d+\)/g' "$CHANGES_FILE" | wc -l

Length of output: 871


Script:

#!/bin/bash
CHANGES_FILE="user_docs/en/changes.md"

echo "File info:"
ls -l "$CHANGES_FILE"

echo -e "\nFirst 10 lines of content:"
head -n 10 "$CHANGES_FILE"

echo -e "\nSearching for lines containing '#':"
grep -n "#" "$CHANGES_FILE" | head -n 5

echo -e "\nSearching for lines containing numbers in parentheses:"
grep -n "([0-9]" "$CHANGES_FILE" | head -n 5

echo -e "\nSearching for specific issue reference format:"
grep -n "(\#[0-9]" "$CHANGES_FILE" | head -n 5

Length of output: 2147


Script:

#!/bin/bash
CHANGES_FILE="user_docs/en/changes.md"

echo "Extracting all unique issue numbers..."
# Extract numbers between (# and )
perl -ne 'print "$1\n" while /\(#(\d+)\)/g' "$CHANGES_FILE" | sort -un > issue_numbers.txt

echo "Total unique issues found: $(wc -l < issue_numbers.txt)"

echo -e "\nChecking random sample of 5 issues..."
# Check 5 random issues
shuf -n 5 issue_numbers.txt | while read -r issue; do
    echo -n "Issue #$issue: "
    if gh api "/repos/nvaccess/nvda/issues/$issue" --jq '.html_url' 2>/dev/null; then
        echo "Valid"
    else
        echo "Not found or inaccessible"
    fi
done

Length of output: 521


Script:

#!/bin/bash
CHANGES_FILE="user_docs/en/changes.md"

echo "Extracting all unique issue numbers..."
perl -ne 'print "$1\n" while /\(#(\d+)\)/g' "$CHANGES_FILE" | sort -un > issue_numbers.txt
TOTAL=$(wc -l < issue_numbers.txt)
echo "Total unique issues found: $TOTAL"

echo -e "\nChecking sample of first 5 issues..."
head -n 5 issue_numbers.txt | while read -r issue; do
    echo -n "Issue #$issue: "
    if gh api "/repos/nvaccess/nvda/issues/$issue" --jq '.html_url' 2>/dev/null; then
        echo "Valid"
    else
        echo "Not found or inaccessible"
    fi
done

echo -e "\nChecking sample of last 5 issues..."
tail -n 5 issue_numbers.txt | while read -r issue; do
    echo -n "Issue #$issue: "
    if gh api "/repos/nvaccess/nvda/issues/$issue" --jq '.html_url' 2>/dev/null; then
        echo "Valid"
    else
        echo "Not found or inaccessible"
    fi
done

Length of output: 2299

@seanbudd seanbudd requested a review from SaschaCowley January 16, 2025 03:21
@SaschaCowley SaschaCowley marked this pull request as draft January 16, 2025 05:29
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 20, 2025
@seanbudd seanbudd marked this pull request as ready for review January 23, 2025 03:53
@seanbudd seanbudd requested a review from SaschaCowley January 23, 2025 03:53
@seanbudd
Copy link
Member Author

Note that migrating the showWarning setting cannot be done, as there is a circular dependency between loading the add-on store settings and loading the config.

Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @seanbudd

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work

@seanbudd seanbudd merged commit 8d96f9f into master Feb 6, 2025
5 checks passed
@seanbudd seanbudd deleted the updateChannelAddons branch February 6, 2025 03:40
@github-actions github-actions bot added this to the 2025.1 milestone Feb 6, 2025
@coderabbitai coderabbitai bot mentioned this pull request Feb 7, 2025
5 tasks
SaschaCowley pushed a commit that referenced this pull request Feb 21, 2025
Closes #3208 
Closes #17162

Summary of the issue:
To update add-ons automatically, users must click-through a dialog to confirm they wish to update all add-ons.
Instead, this can be done in the background, and the user just needs to confirm whether to restart NVDA once add-on updates are complete.

Additionally:
- Users couldn't automatically update to incompatible add-ons. this is useful for alpha/beta testers testing add-on compatibility in breaking releases
- The update notification was annoying - it queued too late and users should get a ding when it appeared.
- If the add-on store server failed, users didn't receive displayable error messages when using automatic update notification
- Since #17597, the update notification panel will list all available updates for all channels. The update all button would update the add-on for each channel. Instead only the latest add-on version for any channel should be used/listed.
 
Description of user facing changes
- Added the ability to automatically update add-ons in the background.
This can be changed in the "automatic updates" section.
- Added the ability to update to incompatible add-ons. users can enable this via a new setting in the add-on store panel.
- The update notification is now queued sooner (1 minute not 3 minutes) and provides a ding before focusing the panel
- the update notification panel now only lists the most recent version of an add-on, rather than showing all add-ons that are newer across channels
- Server errors now log an error message if the update notification fails to fetch data

Description of development approach
- Refactored update status tracking so devs can compare add-ons to see which is newer more easily
- Re-designed message box tracking to make it work for pseudo-modals, e.g. the background update process
- use a shorter starting offset for daily scheduled jobs

Testing strategy:
- Installed an old add-on
- Tested automatic update notifications
- Tested background automatic updates
- Tested both of these without a server connection

Known issues with pull request:
None
gexgd0419 pushed a commit to gexgd0419/nvda that referenced this pull request Feb 22, 2025
Closes nvaccess#3208 
Closes nvaccess#17162

Summary of the issue:
To update add-ons automatically, users must click-through a dialog to confirm they wish to update all add-ons.
Instead, this can be done in the background, and the user just needs to confirm whether to restart NVDA once add-on updates are complete.

Additionally:
- Users couldn't automatically update to incompatible add-ons. this is useful for alpha/beta testers testing add-on compatibility in breaking releases
- The update notification was annoying - it queued too late and users should get a ding when it appeared.
- If the add-on store server failed, users didn't receive displayable error messages when using automatic update notification
- Since nvaccess#17597, the update notification panel will list all available updates for all channels. The update all button would update the add-on for each channel. Instead only the latest add-on version for any channel should be used/listed.
 
Description of user facing changes
- Added the ability to automatically update add-ons in the background.
This can be changed in the "automatic updates" section.
- Added the ability to update to incompatible add-ons. users can enable this via a new setting in the add-on store panel.
- The update notification is now queued sooner (1 minute not 3 minutes) and provides a ding before focusing the panel
- the update notification panel now only lists the most recent version of an add-on, rather than showing all add-ons that are newer across channels
- Server errors now log an error message if the update notification fails to fetch data

Description of development approach
- Refactored update status tracking so devs can compare add-ons to see which is newer more easily
- Re-designed message box tracking to make it work for pseudo-modals, e.g. the background update process
- use a shorter starting offset for daily scheduled jobs

Testing strategy:
- Installed an old add-on
- Tested automatic update notifications
- Tested background automatic updates
- Tested both of these without a server connection

Known issues with pull request:
None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning dialog of Add-on Store is shown again although "Don't show again" checkbox has been checked
3 participants