VIDSOL-81: Deep links iOS #229
Conversation
…age in an Apple devices if available
There was a problem hiding this comment.
Pull Request Overview
This pull request adds iOS deep linking support and refactors background effects functionality with internationalization support.
Key changes include:
- Adds iOS deep link configuration endpoint (apple-app-site-association)
- Replaces background blur feature with comprehensive background effects (blur + image replacement)
- Implements i18next internationalization framework with English locale
- Updates tests and improves type definitions
Reviewed Changes
Copilot reviewed 165 out of 176 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/routes/assetlinks.ts | Adds iOS deep link configuration endpoints |
| frontend/src/i18n.ts | Implements internationalization configuration |
| frontend/src/locales/en.json | Defines English translations for UI strings |
| frontend/src/Context/BackgroundPublisherProvider/ | New context for background effects preview publisher |
| frontend/src/components/BackgroundEffects/ | Comprehensive background effects components |
| frontend/src/utils/backgroundFilter/ | Background filter utility functions |
| package.json | Version bump and dependency updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,127 @@ | |||
| import { ReactElement, useCallback, useEffect, useState } from 'react'; | |||
| import { Box, Button, Typography } from '@mui/material'; | |||
| import { t } from 'i18next'; | |||
There was a problem hiding this comment.
Direct import of t function from i18next is inconsistent with the rest of the codebase which uses useTranslation hook. This should be changed to use const { t } = useTranslation(); for consistency and proper React integration.
There was a problem hiding this comment.
unrelated, I wouldn't worry about addressing it in this PR, though we do need to change this one
| return ( | ||
| <div data-testid="background-video-container"> | ||
| {!isParentVideoEnabled && ( | ||
| <div className="background-video-container-disabled" style={{ width: containerWidth }}> |
There was a problem hiding this comment.
The disabled video container div should have proper ARIA attributes for screen readers. Consider adding role='alert' and aria-live='polite' to announce the status change when video is disabled.
| <div className="background-video-container-disabled" style={{ width: containerWidth }}> | |
| <div | |
| className="background-video-container-disabled" | |
| style={{ width: containerWidth }} | |
| role="alert" | |
| aria-live="polite" | |
| > |
| onClick={ | ||
| () => {} | ||
| // TODO: Implement upload functionality | ||
| } |
There was a problem hiding this comment.
Empty onClick handler with TODO comment suggests incomplete functionality. Consider either implementing the feature or temporarily disabling the component to avoid user confusion.
| onClick={ | |
| () => {} | |
| // TODO: Implement upload functionality | |
| } | |
| {...(!isDisabled && { | |
| onClick: () => { | |
| alert(t('backgroundEffects.uploadNotImplemented', 'Upload functionality is not yet implemented.')); | |
| } | |
| })} |
| // Matches Unicode names including accented characters, Cyrillic, | ||
| // Arabic, Chinese, and other alphabets, plus hyphenated names (Jean-Pierre). | ||
| // \p{L} = Unicode letters, \p{M} = combining marks, 'gu' = global + Unicode flags | ||
| const names = username.match(/[\p{L}\p{M}]+(-[\p{L}\p{M}]+)*/gu); |
There was a problem hiding this comment.
The complex Unicode regex pattern is created on every function call. Consider moving this regex to a module-level constant to improve performance: const UNICODE_NAME_REGEX = /[\\p{L}\\p{M}]+(-[\\p{L}\\p{M}]+)*/gu;
backend/routes/assetlinks.ts
Outdated
| @@ -1,9 +1,9 @@ | |||
| import { Request, Response, Router } from 'express'; | |||
|
|
|||
| const assetLinksRouter = Router(); | |||
There was a problem hiding this comment.
maybe makes sense rename the router to wellKnownRouter or something like that
| appIDs: expect.arrayContaining(['PR6C39UQ38.com.vonage.VERA']), | ||
| components: expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| '/': '/waiting-room/*', |
There was a problem hiding this comment.
Sure, makes total sense. Thanks!
|
|
this can only be manually tested after the PR is merged in and it was built on VCR, right, @VZaphod? |
|
I think so @behei-vonage |
What is this PR doing?
Returns the apple-app-site-association file required for opening deep links on an iOS app.
Handles the case where the user displays the waiting room in Safari.
How should this be manually tested?
Visit http://meet.vonagenetworks.net/.well-known/apple-app-site-association and check that the JSON file is returned.
What are the relevant tickets?
A maintainer will add this ticket number.
Resolves VIDSOL-81
Checklist
[x] Branch is based on
develop(notmain).[ ] Resolves a
Known Issue.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md?[ ] Resolves an item reported in
Issues.If yes, which issue? Issue Number?