MM-61199: Replace ChannelBookmarks feature flag with min server version check#9911
MM-61199: Replace ChannelBookmarks feature flag with min server version check#9911calebroseland wants to merge 3 commits into
Conversation
…rsion
ChannelBookmarks FF was removed from the server (GA in v10.1). Replace the
observeConfigBooleanValue('FeatureFlagChannelBookmarks') check with an
isMinimumServerVersion(v, 10, 1, 0) check so bookmarks work on v10.1+
servers regardless of the FF field being present.
MM-61199
📝 WalkthroughWalkthroughThis PR replaces the ChangesChannel Bookmarks Version Gating
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant fetchChannelBookmarks
participant ServerDB
ServerDB->>fetchChannelBookmarks: Version, License
fetchChannelBookmarks->>fetchChannelBookmarks: isMinimumServerVersion(Version, CHANNEL_BOOKMARKS_VERSION)
alt version too old or license inactive
fetchChannelBookmarks-->>Caller: {bookmarks: []}
else version ok and licensed
fetchChannelBookmarks->>Caller: getChannelBookmarksForChannel result
end
Related issues: None specified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/actions/remote/channel_bookmark.ts (1)
21-24: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the independent DB reads.
getConfigValueandgetLicenseare independent lookups but are awaited sequentially, adding unnecessary latency to a fetch action called on channel load.♻️ Proposed fix
- const serverVersion = await getConfigValue(database, 'Version'); - const isLicensed = (await getLicense(database))?.IsLicensed === 'true'; + const [serverVersion, license] = await Promise.all([ + getConfigValue(database, 'Version'), + getLicense(database), + ]); + const isLicensed = license?.IsLicensed === 'true';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/actions/remote/channel_bookmark.ts` around lines 21 - 24, The channel bookmark fetch action is doing two independent database reads sequentially, which adds avoidable latency on load. In the action that reads server version and license status, fetch both values concurrently instead of awaiting one before starting the other, then keep the existing version/license gate logic unchanged. Use the existing getConfigValue and getLicense calls in the channel bookmark flow to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/queries/servers/features.ts`:
- Around line 19-24: `observeIsChannelBookmarksEnabled` currently gates
bookmarks only on `Version`, which can make the UI show bookmark features even
when `fetchChannelBookmarks` would later return nothing on unlicensed servers.
Update this observable to also check `getLicense` from the same `Database`, and
combine that license gate with the existing `isMinimumServerVersion` check in
the `observeIsChannelBookmarksEnabled` pipeline so the header/info screens stay
consistent with the fetch path.
---
Nitpick comments:
In `@app/actions/remote/channel_bookmark.ts`:
- Around line 21-24: The channel bookmark fetch action is doing two independent
database reads sequentially, which adds avoidable latency on load. In the action
that reads server version and license status, fetch both values concurrently
instead of awaiting one before starting the other, then keep the existing
version/license gate logic unchanged. Use the existing getConfigValue and
getLicense calls in the channel bookmark flow to locate the code.
🪄 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: bcb3f847-f064-40d7-8366-60520ce4a37b
📒 Files selected for processing (7)
app/actions/remote/channel_bookmark.test.tsapp/actions/remote/channel_bookmark.tsapp/constants/versions.tsapp/queries/servers/features.tsapp/screens/channel/header/index.tsapp/screens/channel/index.tsxapp/screens/channel_info/index.ts
Coverage Comparison Report |
…arksEnabled Ensures the observable is consistent with the fetchChannelBookmarks guard — unlicensed servers correctly suppress bookmark UI, not just the network call. MM-69705
|
Hi @calebroseland - I built the apps for this PR but when trying to download the .apk on my Android, it goes through the motions of downloading but then disappears from my downloads folder. Not sure if I'm having an issue because there's a failing check on the PR? Could you take a look please? The .apk built is here on community but I just can't get it to download on my phone. I removed the build apps for PR label and then just built the Android .apk but I can't get it downloaded onto my phone either. I haven't seen this behavior before :( |
lindy65
left a comment
There was a problem hiding this comment.
Thanks @calebroseland - finally managed to sort out the .apk install issues!
Tested as per Jira tickets and both are working as expected 👍
|
Before merging, @calebroseland can you look into #9916 ? seems to be addressing the same problem but a little bit different |
|
Ah, didn't realize you were looking at this as well, @calebroseland! (Or more likely, I just forgot ;p) My original approach also used the version it first defaulted true, but then I realized that a later server might also override the feature flag back off. So instead I'm anchoring it to the server version in which the feature flag was removed. I've also added a framework to help us manage this going forward, as I need to do the same for another flag. Thoughts? |
@lieut-data all good, I was actually wondering if having a pre/post version boundary was the right direction, so I'm happy to defer to https://github.com/mattermost/mattermost-mobile/pull/9916—it's the better fix! My only ask is if you might be able to bring over the quick-fix for https://mattermost.atlassian.net/browse/MM-69705. |
Absolutely! I've pushed these changes to #9911, with an other addition to make the aysnc fetch and observable both inline the license checks. Are we ok to close this PR in favour of #9916? |
Summary
ChannelBookmarksfeature flag was removed, mobile was reading this key viaobserveConfigBooleanValue— when the key is absent, WatermelonDB returnsfalse, hiding bookmarks on all servers running mattermost/mattermost#37120.mattermost/mattermost#37368 restores channel bookmarks functionality on mobile for versions before this PR.
Replace the feature flag dependency with
isMinimumServerVersion(v, 10, 1, 0), which reflects when the feature was first enabled by default. The check lives in a newobserveIsChannelBookmarksEnabledobservable infeatures.ts(alongside the existingobserveHasGMasDMFeaturepattern), and also gates on license so the UI stays consistent with the fetch path.See also MM-69705 for a pre-existing related gap surfaced during review.
Ticket Link
https://mattermost.atlassian.net/browse/MM-61199
https://mattermost.atlassian.net/browse/MM-69705
Checklist
Device Information
N/A — logic change only, no UI changes.
Screenshots
N/A
Release Note