Skip to content

Conversation

@MartinSchoeler
Copy link
Member

@MartinSchoeler MartinSchoeler commented Nov 14, 2025

Proposed changes (including videos or screenshots)

Issue(s)

ABAC-57

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes

    • Updated ABAC help link to the correct documentation endpoint (/i/abac).
    • Removed an outdated license-renewal link to avoid user confusion.
    • ABAC admin warning now directs users to the updated documentation.
  • New Links

    • Added a direct ABAC LDAP documentation link (/i/abac-ldap).

✏️ Tip: You can customize this high-level summary in your review settings.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 14, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2025

⚠️ No Changeset found

Latest commit: ed0f0b4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025

Walkthrough

Replaced placeholder ABAC links in client exports with concrete endpoints (/i/abac and /i/abac-ldap), removed the obsolete abacLicenseRenewalUrl, and updated the Admin ABAC page to reference the new abacDocs link.

Changes

Cohort / File(s) Summary
Client links update
apps/meteor/client/lib/links.ts
Replace placeholder abacDocs with https://go.rocket.chat/i/abac, add abacLDAPDocs as https://go.rocket.chat/i/abac-ldap, and remove exported abacLicenseRenewalUrl.
Admin ABAC page update
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
Update the ABAC warning description anchor to use links.abacDocs instead of the removed abacLicenseRenewalUrl.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check apps/meteor/client/lib/links.ts for consistent URL formatting and any other consumers of abacLicenseRenewalUrl.
  • Verify AdminABACPage.tsx imports and uses links.abacDocs and that no runtime references to the removed property remain.

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • aleksandernsilva
  • tassoevan

Poem

🐰 I hopped through links and cleared the maze,
TODOs swapped for tidy ways.
ABAC paths now point where they should,
A brief clean hop — the code feels good.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore: update link values for ABAC' accurately summarizes the main change of updating ABAC-related documentation links in the codebase.
Linked Issues check ✅ Passed The pull request implements the documentation links specified in ABAC-57, updating abacDocs to /i/abac and adding abacLDAPDocs pointing to /i/abac-ldap.
Out of Scope Changes check ✅ Passed All changes are directly related to the linked issue ABAC-57; the PR updates links as specified and also corrects a reference in AdminABACPage.tsx to maintain consistency.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/implement-links

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a984f0a and ed0f0b4.

📒 Files selected for processing (1)
  • apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.25%. Comparing base (3475101) to head (ed0f0b4).
⚠️ Report is 4 commits behind head on feat/abac.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##           feat/abac   #37525      +/-   ##
=============================================
- Coverage      54.27%   54.25%   -0.02%     
=============================================
  Files           2637     2637              
  Lines          50050    50050              
  Branches       11202    11202              
=============================================
- Hits           27163    27155       -8     
- Misses         20720    20722       +2     
- Partials        2167     2173       +6     
Flag Coverage Δ
e2e 57.23% <ø> (-0.01%) ⬇️
e2e-api 43.64% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MartinSchoeler MartinSchoeler marked this pull request as ready for review November 19, 2025 13:50
@MartinSchoeler MartinSchoeler requested a review from a team as a code owner November 19, 2025 13:50
@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.2GiB 1.2GiB +12MiB
rocketchat 359MiB 347MiB +12MiB
omnichannel-transcript-service 132MiB 132MiB +13KiB
queue-worker-service 132MiB 132MiB +9.0KiB
ddp-streamer-service 126MiB 126MiB +8.5KiB
account-service 113MiB 113MiB +11KiB
authorization-service 111MiB 111MiB +66KiB
stream-hub-service 111MiB 111MiB +11KiB
presence-service 111MiB 111MiB +12KiB

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 16:24", "12/03 17:34 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
  line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Loading

Statistics (last 12 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.2GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37525
  • Baseline: develop
  • Timestamp: 2025-12-03 17:34:19 UTC
  • Historical data points: 12

Updated: Wed, 03 Dec 2025 17:34:20 GMT

@KevLehman KevLehman requested review from a team as code owners November 24, 2025 15:17
@MartinSchoeler MartinSchoeler force-pushed the chore/implement-links branch 2 times, most recently from 0a5ce81 to e4b4b89 Compare November 27, 2025 20:03
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/meteor/client/lib/links.ts (1)

34-35: Consider using distinct URL paths for future flexibility.

Based on past review feedback, both abacDocs and abacLicenseRenewalUrl currently point to the same URL path. Using different paths (e.g., /i/abac-docs and /i/abac-license-renewal) would allow the docs team to redirect both to the same page initially while preserving the ability to point them to different destinations later without requiring a code change.

-		abacDocs: `${GO_ROCKET_CHAT_PREFIX}/i/abac`,
-		abacLicenseRenewalUrl: `${GO_ROCKET_CHAT_PREFIX}/i/abac`,
+		abacDocs: `${GO_ROCKET_CHAT_PREFIX}/i/abac-docs`,
+		abacLicenseRenewalUrl: `${GO_ROCKET_CHAT_PREFIX}/i/abac-license-renewal`,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8eb23b8 and 0a5ce81.

📒 Files selected for processing (1)
  • apps/meteor/client/lib/links.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • apps/meteor/client/lib/links.ts
🔇 Additional comments (1)
apps/meteor/client/lib/links.ts (1)

36-36: Based on my verification, I found that:

  1. Line number is incorrect: The review states line 36, but the URL is actually on line 35
  2. File was newly created: The commit e4b4b89d6e shows this file was newly created with the URL already using lowercase abac-ldap
  3. No reference to ABAC-57 found: I searched the entire codebase and found no references to "ABAC-57"
  4. Consistent lowercase pattern: All other URL paths in the file use lowercase (e.g., ldap-docs, e2ee-guide, fingerprint-changed-faq)

The review comment references an issue "ABAC-57" that supposedly specifies uppercase "LDAP" in the URL, but I cannot verify this claim exists in the codebase or if it's the correct specification.

Verify the correct case for the LDAP URL path with the docs team and issue tracker. The implementation uses /i/abac-ldap (lowercase), but confirm whether this matches the intended specification from ABAC-57 or related documentation. Also note the line number should be 35, not 36.

@MartinSchoeler MartinSchoeler merged commit 3b7804f into feat/abac Dec 5, 2025
88 of 91 checks passed
@MartinSchoeler MartinSchoeler deleted the chore/implement-links branch December 5, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants