Skip to content

Conversation

urmauur
Copy link
Member

@urmauur urmauur commented Jun 4, 2025

Describe Your Changes

This pull request refactors the state management for the app updater functionality in the web-app project. The changes introduce a new local state in the AppUpdater component to enhance clarity and ensure better separation of concerns. Additionally, the useAppUpdater hook is updated to reset the remindMeLater flag when an update becomes available.

Refactor of state management in AppUpdater component:

  • web-app/src/containers/dialogs/AppUpdater.tsx: Introduced a local state appUpdateState using useState to manage remindMeLater and isUpdateAvailable. This state is synchronized with the global updateState using a useEffect hook. Updated conditional rendering to use appUpdateState instead of updateState.

Update to useAppUpdater hook:

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Fixes and synchronizes 'remind me later' functionality in app updater, ensuring consistent update state across components.

  • Behavior:
    • Fixes 'remind me later' functionality in DialogAppUpdater by using appUpdateState to manage update visibility.
    • Synchronizes update state across instances using events in useAppUpdater.
    • Handles update checks and state resets in checkForUpdate() in useAppUpdater.
  • State Management:
    • Adds appUpdateState in DialogAppUpdater to track remindMeLater and isUpdateAvailable.
    • Uses useEffect to sync appUpdateState with updateState in DialogAppUpdater.
    • Implements syncStateToOtherInstances() in useAppUpdater to emit state changes.
  • Misc:
    • Adds useCallback for handleCheckForUpdate in general.tsx to optimize performance.
    • Minor refactoring in useAppUpdater for better error handling and state synchronization.

This description was created by Ellipsis for 83fc4e0. You can customize this summary. It will automatically update as commits are pushed.

@urmauur urmauur force-pushed the fix/remind-me-later-updater branch from 1b09ca2 to 3b4fefb Compare June 4, 2025 05:46
@urmauur urmauur requested review from Minh141120 and louis-menlo June 4, 2025 06:31
@urmauur urmauur marked this pull request as ready for review June 4, 2025 06:32
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 83fc4e0 in 1 minute and 49 seconds. Click for details.
  • Reviewed 267 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/settings/general.tsx:11
  • Draft comment:
    Confirm removal of setRemindMeLater from useAppUpdater is intentional and that 'Remind me later' logic relies solely on the updater dialog.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment violates our rules in multiple ways: 1) It asks for confirmation/verification which we explicitly disallow 2) It's speculative about whether the change might affect other logic 3) It doesn't point out a clear issue that needs fixing 4) Understanding if this is actually an issue would require seeing the updater dialog code which we don't have access to. Maybe removing setRemindMeLater could cause bugs in the reminder functionality? Maybe this is a critical feature that needs verification? Even if there could be bugs, we should trust the author's intention in removing this. If there are bugs, they'll be caught in testing. We shouldn't ask for confirmations or make speculative comments. This comment should be deleted as it violates our rules by asking for confirmation and making speculative statements without pointing out a clear issue that needs fixing.

Workflow ID: wflow_uUe396EfsT0jwXSK

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@urmauur urmauur merged commit da10502 into release/v0.5.18 Jun 4, 2025
48 checks passed
@urmauur urmauur deleted the fix/remind-me-later-updater branch June 4, 2025 08:35
@github-project-automation github-project-automation bot moved this to QA in Jan Jun 4, 2025
@github-actions github-actions bot added this to the v0.5.19 milestone Jun 4, 2025
@david-menloai david-menloai moved this from QA to Done in Jan Jun 20, 2025
@LazyYuuki LazyYuuki removed this from the v0.7.2 milestone Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants