MM-68274 - Adding watermark capability in mobile#9682
MM-68274 - Adding watermark capability in mobile#9682asaadmahmood wants to merge 9 commits intomainfrom
Conversation
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR introduces a watermark feature for the mobile app that displays user information (username, domain, date, time) as a diagonal overlay. The feature is controlled by a new server configuration option Documentation Impact Details
Recommended Actions
ConfidenceHigh — The new configuration option clearly requires documentation for system administrators, and the watermark feature fits the pattern of other mobile security features that are documented in the security guides. The feature meets the materiality test as administrators and security officers would need documentation to understand and configure this compliance/audit feature. |
Coverage Comparison ReportGenerated on April 10, 2026 at 15:11:00 UTC |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Watermark feature: new screen constant and transparent-background flag, Watermark screen component with DB-observed current user and periodic timestamp, config flag, navigation overlay tracking and show/dismiss APIs, ChannelList observable integration and tests, plus related react-native-navigation Android patch changes. Changes
Sequence DiagramsequenceDiagram
participant Config as Config System
participant ChannelList as ChannelListScreen
participant Navigation as Navigation Manager
participant Watermark as WatermarkScreen
Config->>ChannelList: ExperimentalEnableWatermark change
activate ChannelList
ChannelList->>ChannelList: update isWatermarkEnabled prop
ChannelList->>Navigation: showWatermarkOverlay() or dismissWatermarkOverlay()
alt show
Navigation->>Navigation: set watermarkShouldBeShown = true
Navigation->>Watermark: register/show watermark overlay
Watermark->>Watermark: render tiled diagonal watermark (user@server + time)
else dismiss
Navigation->>Navigation: set watermarkShouldBeShown = false
Navigation->>Watermark: dismiss watermark overlay
end
deactivate ChannelList
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/screens/navigation.ts (1)
844-865:⚠️ Potential issue | 🟠 MajorTrack the actual overlay instance ID here.
overlayIdfalls back toname, but Line 861 still passes rawidinto the layout, soshownNonWatermarkOverlayIdsis not tracking the exact value used to create the overlay. Even if that mismatch is fixed,id ?? namestill collapses every overlay of the same screen into one set entry;showSnackBar()inapp/utils/snack_bar/index.ts:15-24already callsshowOverlay(screen, passProps)without an explicit ID.dismissAllOverlays()can therefore miss older overlays and leave them visible after navigation resets. Please generate a per-instance ID when one is not supplied, pass that same ID into the overlay layout, and track/remove that exact value.Also applies to: 879-884
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/navigation.ts` around lines 844 - 865, The code tracks overlay instances using overlayId = id ?? name but then passes the raw id into Navigation.showOverlay, causing mismatches and collapsing instances; change the logic in the overlay-showing code (the block that computes overlayId, updates shownNonWatermarkOverlayIds, and calls Navigation.showOverlay) to generate a unique per-instance id when id is undefined (e.g., using a UUID/timestamp + name), assign that generated value to overlayId, pass overlayId into component.id in Navigation.showOverlay, and use that exact overlayId when adding/removing entries from shownNonWatermarkOverlayIds (apply the same fix to the second occurrence of this pattern in the file); ensure showSnackBar/dismissAllOverlays end up using these per-instance ids so overlays are tracked and dismissed correctly.
🧹 Nitpick comments (6)
app/screens/home/channel_list/channel_list.tsx (1)
178-184: Consider initial render behaviour.The
useEffectwill calldismissWatermarkOverlay()on the initial render whenisWatermarkEnabledisfalse. While this ensures a clean state, it may be unnecessary if the overlay was never shown. Consider whether this is intentional for handling edge cases (e.g., app restart with changed config) or if the initial dismiss call could be avoided.♻️ Optional: Skip dismiss on initial false state
+import {useRef} from 'react'; // Inside component: +const hasShownWatermark = useRef(false); useEffect(() => { if (props.isWatermarkEnabled) { + hasShownWatermark.current = true; showWatermarkOverlay(); - } else { + } else if (hasShownWatermark.current) { dismissWatermarkOverlay(); } }, [props.isWatermarkEnabled]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/home/channel_list/channel_list.tsx` around lines 178 - 184, The effect on useEffect watching props.isWatermarkEnabled currently calls dismissWatermarkOverlay() on initial render when the prop is false; to avoid unnecessary initial dismiss calls, add an isInitialMount ref (e.g., const isInitialMount = useRef(true)) and in the useEffect skip the dismiss/show logic on the very first render (set isInitialMount.current = false after first run), so showWatermarkOverlay() or dismissWatermarkOverlay() only run on subsequent prop changes; update the effect that references props.isWatermarkEnabled, showWatermarkOverlay, and dismissWatermarkOverlay to use this initial-mount guard.app/screens/watermark/index.test.tsx (1)
15-55: Consider adding database cleanup.The test suite sets up a server database in
beforeAllbut doesn't clean it up. Following the testing guidelines, consider adding cleanup to prevent potential memory leaks.♻️ Proposed fix to add cleanup
+import DatabaseManager from '@database/manager'; + describe('WatermarkScreen', () => { let database: Database; const serverUrl = 'http://www.someserver.com'; beforeAll(async () => { const server = await TestHelper.setupServerDatabase(serverUrl); database = server.database; }); + afterAll(async () => { + await DatabaseManager.destroyServerDatabase(serverUrl); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/watermark/index.test.tsx` around lines 15 - 55, The tests call TestHelper.setupServerDatabase in beforeAll but never clean it up, so add teardown logic: lift the local server variable to the describe scope (alongside database), assign it in beforeAll (server = await TestHelper.setupServerDatabase(...)), and add an afterAll that calls the appropriate cleanup on that server (e.g., server.close() or TestHelper.teardownServerDatabase(server)) and/or closes database to avoid leaks; update references to server/database accordingly.app/screens/watermark/index.tsx (4)
87-106: Grid rendering creates new arrays on every render.The
renderGridfunction usesArray.fromwhich creates new arrays on every render. Consider memoizing the grid withuseMemosince it depends onnumRows,numCols,colWidth,ROW_HEIGHT,styles, andwatermarkText.♻️ Proposed fix using useMemo
-import React, {useEffect, useState} from 'react'; +import React, {useEffect, useMemo, useState} from 'react'; - const renderGrid = () => + const grid = useMemo(() => Array.from({length: numRows}, (_, row) => { const staggerOffset = row % 2 === 0 ? 0 : colWidth / 2; return ( <View key={row} style={[styles.row, {top: row * ROW_HEIGHT, left: -staggerOffset}]} > {Array.from({length: numCols + 1}, (_, col) => ( <Text key={col} numberOfLines={1} - style={textStyle} + style={styles.watermarkText} > {watermarkText} </Text> ))} </View> ); - }); + }), [numRows, numCols, colWidth, styles, watermarkText]); // In render: - {renderGrid()} + {grid}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/watermark/index.tsx` around lines 87 - 106, renderGrid currently recreates the grid arrays on every render causing unnecessary allocations; wrap the grid generation in a useMemo that returns the same JSX unless dependencies change. Replace the renderGrid function body with a memoized value via useMemo(() => { ...generate rows/cols JSX... }, [numRows, numCols, colWidth, ROW_HEIGHT, styles, watermarkText, textStyle]) so the grid only recomputes when those values change; keep the same keys (row, col) and component structure when moving the logic into the memoized callback.
81-85: Inline style object recreated on every render.The
textStyleobject is created inside the render function, causing a new reference on every render. Per coding guidelines, define non-memoized inline styles in the stylesheet instead to avoid render stress.♻️ Proposed fix to move to stylesheet
const getStyleSheet = makeStyleSheetFromTheme((theme: Theme) => ({ container: { ...StyleSheet.absoluteFillObject, overflow: 'hidden', }, diagonalLayer: { position: 'absolute', }, row: { position: 'absolute', flexDirection: 'row', }, + watermarkText: { + ...typography('Body', 75, 'Regular'), + color: WATERMARK_COLOR, + marginRight: TEXT_GAP, + }, })); // In component: - const textStyle = { - ...typography('Body', 75, 'Regular'), - color: WATERMARK_COLOR, - marginRight: TEXT_GAP, - }; // In renderGrid: - style={textStyle} + style={styles.watermarkText}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/watermark/index.tsx` around lines 81 - 85, The inline style object textStyle is recreated on every render; move it into a static stylesheet by creating a StyleSheet (e.g., const styles = StyleSheet.create({...})) outside the component and define the style using typography('Body', 75, 'Regular') plus color: WATERMARK_COLOR and marginRight: TEXT_GAP so the component uses styles.text instead of textStyle; update any usages to reference styles.text to avoid allocating a new object each render.
20-21: UnusedcomponentIdprop.The
componentIdprop is declared in thePropstype but never used in the component. Either remove it from the type or use it if it's needed for navigation purposes.♻️ Proposed fix to remove unused prop
type Props = { currentUser: UserModel | undefined; - componentId: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/watermark/index.tsx` around lines 20 - 21, The Props type declares componentId but it isn't used by the Watermark component; remove componentId from the Props type and update the Watermark component signature (and any places instantiating it) to stop expecting that prop, or alternatively, if componentId is required for navigation, use the passed componentId inside the Watermark component where navigation is performed (e.g., reference componentId in the component's props and pass it to the navigation call). Ensure you update the Props type and the Watermark component signature consistently to eliminate the unused prop warning.
76-77: Consider extracting magic number for character width.The hardcoded value
9for character width estimation could be extracted as a named constant for clarity.♻️ Proposed fix
const WATERMARK_COLOR = 'rgba(128, 128, 128, 0.45)'; const ROW_HEIGHT = 130; const TEXT_GAP = 50; +// Approximate character width for Body/75 typography (~9px per char at 11sp) +const CHAR_WIDTH_ESTIMATE = 9; // In component: - // Approximate character width for Body/75 typography (~9px per char at 11sp) - const estimatedTextWidth = watermarkText.length * 9; + const estimatedTextWidth = watermarkText.length * CHAR_WIDTH_ESTIMATE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/screens/watermark/index.tsx` around lines 76 - 77, Extract the magic number 9 used in computing estimatedTextWidth into a named constant (e.g., AVERAGE_CHAR_WIDTH_PX or ESTIMATED_CHAR_WIDTH_BODY75) and replace the literal in the calculation (const estimatedTextWidth = watermarkText.length * 9) with that constant; ensure the constant is defined near the top of the file or next to the comment about Body/75 typography to improve clarity and maintainability while keeping the existing calculation using watermarkText.length * AVERAGE_CHAR_WIDTH_PX.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/screens/home/channel_list/channel_list.test.tsx`:
- Around line 98-105: The test name and behavior are mismatched: it currently
renders ChannelListScreen initially with isWatermarkEnabled: false (duplicating
the prior test) instead of exercising a prop change; update the test to
initialize with isWatermarkEnabled: true, render using renderWithEverything,
then call rerender (or re-render the component) with props from getBaseProps()
modified to isWatermarkEnabled: false and assert dismissWatermarkOverlay was
called once—use the ChannelListScreen component, the dismissWatermarkOverlay
mock, and the existing renderWithEverything / getBaseProps helpers to locate and
implement the change.
In `@app/screens/navigation.ts`:
- Around line 138-142: The guard using watermarkCurrentlyShown in the safety-net
block is unreliable because that flag is only toggled inside
showWatermarkOverlay() and dismissWatermarkOverlay(), so if the overlay is
removed by any other path the flag remains true and recovery won't run; add a
watermark-specific dismiss/disappear listener (hook into the overlay system used
by showOverlay or the Screens.WATERMARK overlay lifecycle) that sets
watermarkCurrentlyShown = false when the watermark overlay is dismissed or
disappears, then rely on that cleared flag in the existing recovery check (also
update the equivalent logic around the other occurrence that mirrors this
pattern between lines ~990-1002) so the watermark can be re-shown correctly.
In `@app/screens/watermark/index.test.tsx`:
- Around line 32-34: The empty act(async () => { }) calls in
app/screens/watermark/index.test.tsx (used around the Watermark tests) are
no-ops and won’t wait for WatermelonDB observables to emit; replace those empty
act blocks with a proper wait strategy (e.g., import and use waitFor from
`@testing-library/react-native` or await the actual async operation) so the test
waits for the observable resolution before asserting; update the usages of
act(...) at the spots around the Watermark test (the existing empty act calls)
to use waitFor(...) or an awaited async call that checks for the expected
UI/state change.
---
Outside diff comments:
In `@app/screens/navigation.ts`:
- Around line 844-865: The code tracks overlay instances using overlayId = id ??
name but then passes the raw id into Navigation.showOverlay, causing mismatches
and collapsing instances; change the logic in the overlay-showing code (the
block that computes overlayId, updates shownNonWatermarkOverlayIds, and calls
Navigation.showOverlay) to generate a unique per-instance id when id is
undefined (e.g., using a UUID/timestamp + name), assign that generated value to
overlayId, pass overlayId into component.id in Navigation.showOverlay, and use
that exact overlayId when adding/removing entries from
shownNonWatermarkOverlayIds (apply the same fix to the second occurrence of this
pattern in the file); ensure showSnackBar/dismissAllOverlays end up using these
per-instance ids so overlays are tracked and dismissed correctly.
---
Nitpick comments:
In `@app/screens/home/channel_list/channel_list.tsx`:
- Around line 178-184: The effect on useEffect watching props.isWatermarkEnabled
currently calls dismissWatermarkOverlay() on initial render when the prop is
false; to avoid unnecessary initial dismiss calls, add an isInitialMount ref
(e.g., const isInitialMount = useRef(true)) and in the useEffect skip the
dismiss/show logic on the very first render (set isInitialMount.current = false
after first run), so showWatermarkOverlay() or dismissWatermarkOverlay() only
run on subsequent prop changes; update the effect that references
props.isWatermarkEnabled, showWatermarkOverlay, and dismissWatermarkOverlay to
use this initial-mount guard.
In `@app/screens/watermark/index.test.tsx`:
- Around line 15-55: The tests call TestHelper.setupServerDatabase in beforeAll
but never clean it up, so add teardown logic: lift the local server variable to
the describe scope (alongside database), assign it in beforeAll (server = await
TestHelper.setupServerDatabase(...)), and add an afterAll that calls the
appropriate cleanup on that server (e.g., server.close() or
TestHelper.teardownServerDatabase(server)) and/or closes database to avoid
leaks; update references to server/database accordingly.
In `@app/screens/watermark/index.tsx`:
- Around line 87-106: renderGrid currently recreates the grid arrays on every
render causing unnecessary allocations; wrap the grid generation in a useMemo
that returns the same JSX unless dependencies change. Replace the renderGrid
function body with a memoized value via useMemo(() => { ...generate rows/cols
JSX... }, [numRows, numCols, colWidth, ROW_HEIGHT, styles, watermarkText,
textStyle]) so the grid only recomputes when those values change; keep the same
keys (row, col) and component structure when moving the logic into the memoized
callback.
- Around line 81-85: The inline style object textStyle is recreated on every
render; move it into a static stylesheet by creating a StyleSheet (e.g., const
styles = StyleSheet.create({...})) outside the component and define the style
using typography('Body', 75, 'Regular') plus color: WATERMARK_COLOR and
marginRight: TEXT_GAP so the component uses styles.text instead of textStyle;
update any usages to reference styles.text to avoid allocating a new object each
render.
- Around line 20-21: The Props type declares componentId but it isn't used by
the Watermark component; remove componentId from the Props type and update the
Watermark component signature (and any places instantiating it) to stop
expecting that prop, or alternatively, if componentId is required for
navigation, use the passed componentId inside the Watermark component where
navigation is performed (e.g., reference componentId in the component's props
and pass it to the navigation call). Ensure you update the Props type and the
Watermark component signature consistently to eliminate the unused prop warning.
- Around line 76-77: Extract the magic number 9 used in computing
estimatedTextWidth into a named constant (e.g., AVERAGE_CHAR_WIDTH_PX or
ESTIMATED_CHAR_WIDTH_BODY75) and replace the literal in the calculation (const
estimatedTextWidth = watermarkText.length * 9) with that constant; ensure the
constant is defined near the top of the file or next to the comment about
Body/75 typography to improve clarity and maintainability while keeping the
existing calculation using watermarkText.length * AVERAGE_CHAR_WIDTH_PX.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 405c836b-0d4a-4960-8b76-87e3a7d6930d
📒 Files selected for processing (9)
app/constants/screens.tsapp/screens/home/channel_list/channel_list.test.tsxapp/screens/home/channel_list/channel_list.tsxapp/screens/home/channel_list/index.tsapp/screens/index.tsxapp/screens/navigation.tsapp/screens/watermark/index.test.tsxapp/screens/watermark/index.tsxtypes/api/config.d.ts
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
Several changes to be addressed. Also, this seems to break completely the android app (the user cannot interact with anything). May not be related to your PR, though.
| componentId: string; | ||
| }; | ||
|
|
||
| const WATERMARK_COLOR = 'rgba(128, 128, 128, 0.45)'; |
There was a problem hiding this comment.
Since we are using a hardcoded color, I can change my theme channel background to #808080 and the watermark blends with the background.
There was a problem hiding this comment.
Yes, that is correct, unless we use a theme color, then if we use a theme color we run into a few dependencies with the sidebar color, and center channel text color.
I previously used a different color for the sidebar and the center color, but it was looking a bit weird with the transition.
|
@enahum two things on this:
|
For expor router we will need to migrate this to a Portal, the same way the other overlays were migrated As for the overlay problem, is probably a missing property that allows it to be receive touches outside, not the problem here is that it takes the whole scren, so that indeed seems to be capturing the gestures, so probably the wrapped view should have something like pointerEvents='none' or something |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
patches/react-native-navigation+7.45.0.patch (5)
461-486:⚠️ Potential issue | 🟠 MajorStill unsafe: force unwraps in callbacks will crash despite nullable return type.
getEventDispatcher()is now nullable but force unwrapped via!!inonChildStartedNativeGestureandonChildEndedNativeGesture, causing NPEs in the same lifecycle window where other callbacks now safely no-op. Additionally, the helper method itself still force unwrapsgetNativeModule, so null safety is incomplete.Apply the early-return pattern to all three callbacks:
Suggested fix pattern
override fun onChildStartedNativeGesture(child: View, androidEvent: MotionEvent?) { - androidEvent?.let { - mJSTouchDispatcher.onChildStartedNativeGesture(it, this.getEventDispatcher()!!) - } + val dispatcher = getEventDispatcher() ?: return + androidEvent?.let { mJSTouchDispatcher.onChildStartedNativeGesture(it, dispatcher) } }private fun getEventDispatcher(): EventDispatcher? { val reactContext: ReactContext = this.getReactContext() - return reactContext.getNativeModule(UIManagerModule::class.java)!!.eventDispatcher + return reactContext.getNativeModule(UIManagerModule::class.java)?.eventDispatcher }Apply the same
dispatcher ?: returnpattern toonChildEndedNativeGestureas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 461 - 486, The callbacks onChildStartedNativeGesture (both variants) and onChildEndedNativeGesture are force-unwrapping a nullable EventDispatcher via !! and getNativeModule is also force-unwrapped in getEventDispatcher(); change getEventDispatcher() to safely return EventDispatcher? by using a safe call on getNativeModule (e.g., getNativeModule(UIManagerModule::class.java)?.eventDispatcher) and update each callback to early-return when the dispatcher is null (e.g., val dispatcher = getEventDispatcher() ?: return) before calling mJSTouchDispatcher, removing all !! usages so no NPEs occur.
338-344:⚠️ Potential issue | 🟡 MinorMask
softInputModebefore comparing adjust flags.
softInputModeis a bitmask with both adjustment and state bits. Direct comparison (==) only matches when the entire value matches exactly, so it fails when state bits are also set (e.g.,stateVisible | adjustNothing). UseSOFT_INPUT_MASK_ADJUSTto isolate the adjustment bits before checking.Suggested fix
- if (softInputMode == WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING && !isRoot()) { + if ((softInputMode & WindowManager.LayoutParams.SOFT_INPUT_MASK_ADJUST) == + WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING && !isRoot()) { view.setPadding(ime.left, ime.top, ime.right, 0); return WindowInsetsCompat.CONSUMED; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 338 - 344, The comparison of softInputMode is incorrect because softInputMode contains state and adjust bits; in the block where you call getActivity() and compute Insets ime from insets.getInsets(WindowInsetsCompat.Type.ime()), mask softInputMode with WindowManager.LayoutParams.SOFT_INPUT_MASK_ADJUST before comparing to WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING (i.e. use (softInputMode & SOFT_INPUT_MASK_ADJUST) == SOFT_INPUT_ADJUST_NOTHING) so the check in the isRoot() guard correctly detects adjustNothing and then calls view.setPadding(...) and returns WindowInsetsCompat.CONSUMED.
287-295:⚠️ Potential issue | 🟠 MajorGuard the cast in
invalidate().The
activity()method signature changed to returnActivityinstead ofNavigationActivity, allowing it to return any Activity type. Other methods in the file properly handle this—navigator()uses instanceof checks, andhandle()catches ClassCastException. However,invalidate()performs an unguarded cast that will throw ClassCastException ifactivity()returns a non-NavigationActivity instance, occurring beforesuper.invalidate()runs and potentially leaving cleanup incomplete.Suggested fix
`@Override` public void invalidate() { - final NavigationActivity navigationActivity = (NavigationActivity)activity(); + final Activity currentActivity = activity(); + final NavigationActivity navigationActivity = + currentActivity instanceof NavigationActivity ? (NavigationActivity) currentActivity : null; if (navigationActivity != null) { navigationActivity.onCatalystInstanceDestroy(); } super.invalidate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 287 - 295, The invalidate() method currently performs an unchecked cast of activity() to NavigationActivity which can throw ClassCastException before super.invalidate() runs; change it to first get Activity a = activity() and check "instanceof NavigationActivity" (or safely catch ClassCastException), then only call navigationActivity.onCatalystInstanceDestroy() when the cast is valid, and ensure super.invalidate() is always invoked afterward so cleanup always runs; reference invalidate(), activity(), NavigationActivity, onCatalystInstanceDestroy(), and super.invalidate() when making the change.
254-277:⚠️ Potential issue | 🟠 MajorReject promises explicitly when NavigationActivity is unavailable.
When the current activity is not a
NavigationActivity,navigator()returns an emptyOptional, causingnavigator().ifPresent(...)to silently skip execution. This leaves promises unresolved indefinitely, so JS callers wait forever onsetRoot,push,pop,popTo,popToRoot,showModal,dismissModal,dismissAllModals,showOverlay,dismissOverlay,dismissAllOverlays, and other promise-backed commands.Instead of silently skipping, these methods should reject the promise with an explicit error when the activity is not a
NavigationActivity.
35-55:⚠️ Potential issue | 🔴 CriticalRestore
navigator.handleBack()delegation in the Android back-press path.Changing from
AppCompatActivitytoReactActivityrequires adapting back-press handling, but removing thenavigator.handleBack()call ininvokeDefaultOnBackPressed()bypasses RNN's stack-aware back handling. This will cause the hardware back button to skip overlays and nested screens, jumping directly to the activity default instead of properly popping the RNN controller stack.Suggested fix
- // setBackPressedCallback(); + setBackPressedCallback();`@Override` public void invokeDefaultOnBackPressed() { - super.invokeDefaultOnBackPressed(); + if (!navigator.handleBack(new CommandListenerAdapter())) { + callback.setEnabled(false); + super.invokeDefaultOnBackPressed(); + callback.setEnabled(true); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 35 - 55, The Android back-press delegation was removed causing RNN’s navigator stack to be bypassed; in NavigationActivity.invokeDefaultOnBackPressed() restore the original delegation by calling navigator.handleBack(new CommandListenerAdapter()) and only falling back to the activity default when that returns false: if navigator.handleBack(...) is false, set callback.setEnabled(false), call super.invokeDefaultOnBackPressed(), then set callback.setEnabled(true). Ensure you reference the existing navigator, CommandListenerAdapter and callback variables so overlays and nested controllers are popped correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patches/react-native-navigation`+7.45.0.patch:
- Around line 358-362: The onViewDisappear handler currently pops the top child
then only skips calling onViewBroughtToFront if the new top is destroyed,
leaving destroyed controllers in children; modify onViewDisappear (used with
isTopChild, children.pop, children.peek, isDestroyed, onViewBroughtToFront) to,
after popping the top, repeatedly pop and discard while children is not empty
and children.peek().isDestroyed() so that the first non-destroyed child (if any)
receives onViewBroughtToFront(); ensure you still guard onViewBroughtToFront()
with an existence check (children.isEmpty()).
---
Outside diff comments:
In `@patches/react-native-navigation`+7.45.0.patch:
- Around line 461-486: The callbacks onChildStartedNativeGesture (both variants)
and onChildEndedNativeGesture are force-unwrapping a nullable EventDispatcher
via !! and getNativeModule is also force-unwrapped in getEventDispatcher();
change getEventDispatcher() to safely return EventDispatcher? by using a safe
call on getNativeModule (e.g.,
getNativeModule(UIManagerModule::class.java)?.eventDispatcher) and update each
callback to early-return when the dispatcher is null (e.g., val dispatcher =
getEventDispatcher() ?: return) before calling mJSTouchDispatcher, removing all
!! usages so no NPEs occur.
- Around line 338-344: The comparison of softInputMode is incorrect because
softInputMode contains state and adjust bits; in the block where you call
getActivity() and compute Insets ime from
insets.getInsets(WindowInsetsCompat.Type.ime()), mask softInputMode with
WindowManager.LayoutParams.SOFT_INPUT_MASK_ADJUST before comparing to
WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING (i.e. use (softInputMode &
SOFT_INPUT_MASK_ADJUST) == SOFT_INPUT_ADJUST_NOTHING) so the check in the
isRoot() guard correctly detects adjustNothing and then calls
view.setPadding(...) and returns WindowInsetsCompat.CONSUMED.
- Around line 287-295: The invalidate() method currently performs an unchecked
cast of activity() to NavigationActivity which can throw ClassCastException
before super.invalidate() runs; change it to first get Activity a = activity()
and check "instanceof NavigationActivity" (or safely catch ClassCastException),
then only call navigationActivity.onCatalystInstanceDestroy() when the cast is
valid, and ensure super.invalidate() is always invoked afterward so cleanup
always runs; reference invalidate(), activity(), NavigationActivity,
onCatalystInstanceDestroy(), and super.invalidate() when making the change.
- Around line 35-55: The Android back-press delegation was removed causing RNN’s
navigator stack to be bypassed; in
NavigationActivity.invokeDefaultOnBackPressed() restore the original delegation
by calling navigator.handleBack(new CommandListenerAdapter()) and only falling
back to the activity default when that returns false: if
navigator.handleBack(...) is false, set callback.setEnabled(false), call
super.invokeDefaultOnBackPressed(), then set callback.setEnabled(true). Ensure
you reference the existing navigator, CommandListenerAdapter and callback
variables so overlays and nested controllers are popped correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ec8dcc6-e41d-4d63-8e1a-d856a1522423
📒 Files selected for processing (2)
app/screens/navigation.tspatches/react-native-navigation+7.45.0.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- app/screens/navigation.ts
…t-mobile into echo40-watermarking
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/screens/watermark/watermark.tsx`:
- Around line 61-64: Replace the mount-only useEffect timer with the useDidMount
hook: import useDidMount from '@hooks/did_mount', remove the useEffect(() => {
... }, []), and call useDidMount(() => { const interval = setInterval(() =>
setNow(new Date()), toMilliseconds({minutes: 1})); return () =>
clearInterval(interval); }); — update the import list and ensure the referenced
setNow and toMilliseconds usage remains unchanged inside the new useDidMount
callback.
- Around line 110-117: The watermark overlay is currently exposing many visual
Text nodes from renderGrid() to screen readers; mark the overlay container and
its diagonalLayer as hidden from accessibility services by adding
accessibilityElementsHidden={true} and
importantForAccessibility="no-hide-descendants" (or accessible={false} where
appropriate) to the outer View(s) that wrap renderGrid() (symbols: renderGrid,
styles.container, styles.diagonalLayer, pointerEvents='none') so the overlay
remains visually present but is excluded from VoiceOver/TalkBack.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5703552f-20d2-4105-9b27-70b523d7d09c
📒 Files selected for processing (3)
app/screens/watermark/index.test.tsxapp/screens/watermark/index.tsxapp/screens/watermark/watermark.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/screens/watermark/index.tsx
- app/screens/watermark/index.test.tsx
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
Looks good to me. Two really minor things to look into before merging, though.
If you take a stab to the test, let me know and I will review the result.
Co-authored-by: Daniel Espino García <larkox@gmail.com>
|
This pull request registers the Watermark screen using an early return that bypasses the withManagedConfig HOC, which may let sensitive watermark information remain visible or not be masked when the app is locked or backgrounded, risking non‑compliance with EMM/security requirements. The issue is marked medium severity and risky but non‑blocking.
🟡 Security Policy Bypass in
|
| Vulnerability | Security Policy Bypass |
|---|---|
| Description | The Watermark screen is registered using an early return that explicitly bypasses the withManagedConfig HOC. withManagedConfig is typically responsible for enforcing enterprise mobility management (EMM) policies and app-level security constraints (e.g., app lock, screen masking when backgrounded). By skipping this wrapper, the watermark overlay, which intentionally displays sensitive user and server information, might remain visible or fail to be correctly obscured in scenarios where the application is locked or backgrounded, potentially violating compliance requirements. |
mattermost-mobile/app/screens/index.tsx
Lines 308 to 311 in 02ff0f9
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
amyblais
added
CherryPick/Approved
|
@copilot resolve the merge conflicts in this pull request |
5 participants
Summary
MM-68274 - Adding watermark capability in mobile
Ticket Link
Jira https://mattermost.atlassian.net/browse/MM-68274
Checklist
E2E/Run(orE2E/Run-iOS/E2E/Run-Androidfor platform-specific runs).Device Information
This PR was tested on: iPhone 15 simulator, iOS 17.5
Screenshots
Release Note