feat: Permission repo, replace ko observables with zustand store [WPB-20570] #19609
+950
−235
Conversation
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #19609 +/- ##
==========================================
+ Coverage 43.61% 43.67% +0.05%
==========================================
Files 1291 1292 +1
Lines 32266 32312 +46
Branches 7168 7173 +5
==========================================
+ Hits 14072 14111 +39
- Misses 16503 16509 +6
- Partials 1691 1692 +1 🚀 New features to boost your workflow:
|
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
Failed Tests:❌ Calls in channels with device switch and screenshare (tags: TC-8754, crit-flow-web)Location: specs/CriticalFlow/channelsCall-TC-8755.spec.ts:38 Errors: ❌ Channels Management (tags: TC-8752, crit-flow-web)Location: specs/CriticalFlow/channelsManagement-TC-8752.spec.ts:36 Errors: ❌ Planning group call with sending various messages during call (tags: TC-8632, crit-flow-web)Location: specs/CriticalFlow/groupCalls-TC-8632.spec.ts:37 Errors: ❌ Group Video call (tags: TC-8637, crit-flow-web)Location: specs/CriticalFlow/groupVideoCall-TC-8637.spec.ts:39 Errors: ❌ Messages in Channels (tags: TC-8753, crit-flow-web)Location: specs/CriticalFlow/messagesInChannels-TC-8753.spec.ts:44 Errors: ❌ Messages in Groups (tags: TC-8751, crit-flow-web)Location: specs/CriticalFlow/messagesInGroups-TC-8751.spec.ts:42 Errors: Flaky Tests: |
arjita-mitra
reviewed
Oct 1, 2025
| state: PermissionState | PermissionStatusState | NotificationPermission, | ||
| ): UnifiedPermissionState { | ||
| switch (state) { | ||
| case 'default': |
There was a problem hiding this comment.
Could you explain why we need both default and prompt?
There was a problem hiding this comment.
Both default (Notification API) and prompt (Permissions API) represent the same "not yet requested" state but come from different browser APIs, so they're normalized to a unified PROMPT state.
There was a problem hiding this comment.
I agree that the naming is not perfect, since we have case "default" and default in the same switch statement
https://github.com/wireapp/wire-webapp into feat/knockout-removal-permission-repository-WPB-20570
|
e-maad
commented
Oct 2, 2025
| @@ -0,0 +1,141 @@ | |||
| /* | |||
There was a problem hiding this comment.
Permission repository replaced with handlers and pure functions.
aweiss-dev
reviewed
Oct 2, 2025
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see http://www.gnu.org/licenses/. |
There was a problem hiding this comment.
Can we separate the hookless store from the hook, please?
like:
Permissions.store.ts
Permissions.hooks.ts
Permissions.types.ts (for exported common types, not for types that belong to the store for example)
aweiss-dev
reviewed
Oct 2, 2025
| * along with this program. If not, see http://www.gnu.org/licenses/. | ||
| * | ||
| */ | ||
|
|
There was a problem hiding this comment.
since we refactor already, can we move that file into the folder it belongs and call it Permissions.test.ts?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This pull request refactors how permissions are handled across the app, particularly removing direct usage of the
PermissionRepositoryin favor of new permission handler utilities and normalized permission state enums. The changes focus on simplifying permission management for notifications and media streams, and updating related tests and imports to use the new approach.Permission Handling Refactor:
PermissionRepositoryinapp.ts,MediaStreamHandler, andNotificationRepositorywith new permission handler functions (initializePermissions,getPermissionStates,getPermissionState,setPermissionState). This centralizes permission logic and removes the need to pass around repository instances.MediaStreamHandlerandNotificationRepositoryto use the newBrowserPermissionStatusenum and handler functions, replacing previous usage ofPermissionStatusStateand repository methods.Notification Permission State Normalization:
NotificationRepositoryusingnormalizePermissionState, ensuring consistent state management and updating viasetPermissionState.Enum and File Renaming:
PermissionStatusStatetoBrowserPermissionStatusandPermissionStatetoAppPermissionStatefor clarity, updating all references and imports accordingly.Test Updates:
PermissionRepositoryand update permission state checks to the new enums and handler functions. This includes changes in both media and notification repository tests.Code Cleanup:
This refactor streamlines permission management, improves code clarity, and reduces coupling between repositories and permission logic.
Checklist