Repository navigation
feat(kb): entity-based permissions with share service layer and exten… - #1092
Merged
Merged
Conversation
Contributor
|
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 entity_type/entity_id to ResourceMember with an Alembic migration, converts backend services and raw SQL to use entity-based membership, introduces external entity resolvers (namespace), refactors KB permission merging and sources, updates share APIs and schemas, extends frontend permission UI and group search, and adapts tests and docs. ChangesEntity-based membership and knowledge permissions
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend
participant API as FastAPI
participant Share as UnifiedShareService
participant Resolver as NamespaceEntityResolver
participant DB as Database
Frontend->>API: POST /KnowledgeBase (members=[{entity_type,entity_id,...}])
API->>Share: batch_add_members(resource_id, members)
Share->>Resolver: batch_get_display_names(entity_ids)
Resolver-->>Share: display names map
Share->>DB: INSERT ResourceMember (entity_type, entity_id, snapshot)
DB-->>Share: OK
Share-->>API: BatchResourceMemberResponse
API-->>Frontend: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx (1)
313-321:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
publicKB routes fall through the wrong navigation branch.Organization detail pages intentionally have
initialKbNamespace === undefined, so this check treats/knowledge/public/...as query-param mode and keeps users on the detail route when they navigate back to group/all views. Gate this oninitialKbNameor an explicit “virtual path mode” flag instead.🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx` around lines 313 - 321, The navigation branch is using initialKbNamespace to detect virtual-URL mode, which is wrong for public KBs; change the gate to check initialKbName (or a dedicated boolean like isVirtualPathMode) instead of initialKbNamespace so that organization detail pages (where initialKbNamespace is undefined) do not fall into the virtual URL logic; update the condition around the router.push block (the code referencing initialKbNamespace, params.group, and router.push(`/knowledge?...`)) to use initialKbName !== undefined or isVirtualPathMode, and ensure any callers/props that set the new flag are adjusted accordingly.backend/app/schemas/share.py (1)
147-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd cross-field validation to ensure valid member identity combinations.
The schema currently allows
entity_type='namespace'withoutentity_id, which the service accepts by falling back touser_id. This violates the documented expectation (shown inbatch_add_membersdocstring) that non-user entity members must provideentity_id. Add schema-level validation to enforce this.Schema-level validation example
-from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator class ResourceMemberCreate(BaseModel): @@ `@field_validator`("entity_type") `@classmethod` def validate_entity_type(cls, v: Optional[str]) -> Optional[str]: """Normalize entity_type to lowercase.""" if v is not None: return v.lower() return v + + `@model_validator`(mode="after") + def validate_member_identity(self) -> "ResourceMemberCreate": + entity_type = self.entity_type or "user" + if entity_type != "user" and not self.entity_id: + raise ValueError("entity_id is required for non-user entity members") + return self🤖 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 `@backend/app/schemas/share.py` around lines 147 - 183, Add a Pydantic model-level validator to enforce that when entity_type is set and not 'user' the entity_id must be provided (and optionally user_id must be 0); locate the same model that contains the existing `@field_validator` methods (validate_role and validate_entity_type) and add a `@model_validator` (or root_validator for older Pydantic) method (e.g., validate_identity or validate_cross_fields) that checks if entity_type is not None and entity_type.lower() != "user" then if not entity_id raise ValueError("entity_id is required when entity_type is not 'user'") (and optionally ensure user_id == 0 or raise) so invalid member combinations are rejected at schema validation.backend/app/services/share/knowledge_share_service.py (2)
883-891:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompare against
BaseRolehere, notResourceRole.
KnowledgeService._get_user_kb_permission()now returnsBaseRole | None, so this membership test is always false for shared Maintainers/Owners. As written, non-creator users with sufficient KB role can lose permission-management access.Suggested fix
if is_creator: return True - return has_access and role in ( - ResourceRole.Owner, - ResourceRole.Maintainer, - ) + return has_access and role is not None and has_permission( + role, BaseRole.Maintainer + )🤖 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 `@backend/app/services/share/knowledge_share_service.py` around lines 883 - 891, The membership check is using ResourceRole constants but KnowledgeService._get_user_kb_permission returns BaseRole | None, so change the role comparison to use BaseRole values: update the return logic in the permission check (the block using has_access, role, is_creator) to return True when is_creator is True, otherwise return has_access and role in (BaseRole.Owner, BaseRole.Maintainer); reference KnowledgeService._get_user_kb_permission, the local variables has_access/role/is_creator, and replace ResourceRole.Owner/Maintainer with BaseRole.Owner/BaseRole.Maintainer so shared maintainer/owner users retain permission management access.
184-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the normalized member variants in
_get_resource, too.The rest of this service now queries
ResourceMemberthrough_resource_type_variants/_approved_status_variants, but_get_resourcestill requires exact"KnowledgeBase"and"approved"matches here. That can deny access for legacy member rows even thoughget_user_kb_permission()says the user has permission.Suggested fix
+ resource_type_variants = self._resource_type_variants + approved_status_variants = self._approved_status_variants + member = ( db.query(ResourceMember) .filter( - ResourceMember.resource_type == ResourceType.KNOWLEDGE_BASE.value, + ResourceMember.resource_type.in_(resource_type_variants), ResourceMember.resource_id == resource_id, ResourceMember.entity_type == "user", ResourceMember.entity_id == str(user_id), - ResourceMember.status == MemberStatus.APPROVED.value, + ResourceMember.status.in_(approved_status_variants), ) .first() ) @@ entity_members = ( db.query(ResourceMember) .filter( - ResourceMember.resource_type == ResourceType.KNOWLEDGE_BASE.value, + ResourceMember.resource_type.in_(resource_type_variants), ResourceMember.resource_id == resource_id, ResourceMember.entity_type != "user", ResourceMember.entity_type != "", ResourceMember.entity_id.isnot(None), - ResourceMember.status == MemberStatus.APPROVED.value, + ResourceMember.status.in_(approved_status_variants), ) .all() )🤖 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 `@backend/app/services/share/knowledge_share_service.py` around lines 184 - 220, The _get_resource function currently queries ResourceMember using exact matches to ResourceType.KNOWLEDGE_BASE.value and MemberStatus.APPROVED.value which excludes normalized/legacy variants; update both the user-specific query (the one assigning `member`) and the entity_members query to use the normalization helper variants instead: replace equality checks on `ResourceMember.resource_type` and `ResourceMember.status` with membership checks using `_resource_type_variants(ResourceType.KNOWLEDGE_BASE)` and `_approved_status_variants()` (e.g., `.filter(ResourceMember.resource_type.in_(_resource_type_variants(...)), ResourceMember.status.in_(_approved_status_variants()), ...)`) so legacy/normalized rows are considered while keeping the existing `member.get_effective_role()` and other entity_type/entity_id filters intact.
🟡 Minor comments (9)
backend/tests/services/share/test_erp_entity_resolver_cache.py-76-90 (1)
76-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name and asserted TTL are inconsistent.
Line 76 says “one hour”, but Line 90 asserts
expire == 900(15 minutes). Rename the test or update the asserted value so intent matches behavior.Suggested fix
- def test_cache_ttl_is_one_hour(self): + def test_cache_ttl_is_fifteen_minutes(self):🤖 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 `@backend/tests/services/share/test_erp_entity_resolver_cache.py` around lines 76 - 90, The test name test_cache_ttl_is_one_hour is inconsistent with the asserted TTL (expire == 900); update the test to reflect actual behavior by renaming the test to test_cache_ttl_is_15_minutes (or change the asserted value to 3600 if you intend one hour) and ensure the assertion inspects mock_cache.set_sync.call_args[1].get("expire") after calling resolver.match_entity_bindings so the name and the expected expire value (900 for 15 minutes or 3600 for one hour) match.frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx-152-157 (1)
152-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRole selector doesn't meet 44px minimum height.
Line 156 uses
h-10(40px) for the SelectTrigger, but the coding guidelines require all interactive elements to be at least 44px for mobile touch-friendliness. Useh-11instead.🐛 Proposed fix
<Select value={role} onValueChange={v => setRoleForType(section.type, v as MemberRole)} > - <SelectTrigger className="w-28 h-10 min-w-[44px] flex-shrink-0"> + <SelectTrigger className="w-28 h-11 min-w-[44px] flex-shrink-0"> <span className="truncate">{t(`document.permission.role.${role}`)}</span> </SelectTrigger>As per coding guidelines: Touch-friendly mobile requirements: all interactive elements must be at least 44px × 44px; use
h-11 min-w-[44px]for buttons.🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx` around lines 152 - 157, The SelectTrigger for the role selector is currently using h-10 (40px) and should be increased to meet the 44px touch-minimum; update the SelectTrigger className in the KnowledgeBaseAuthSection component (the Select/SelectTrigger used alongside value={role} and onValueChange calling setRoleForType with MemberRole) to use h-11 (e.g., "w-28 h-11 min-w-[44px] flex-shrink-0") so the interactive element meets the 44×44px guideline.frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx-69-83 (1)
69-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing dependencies in useEffect hooks.
Both effect hooks at lines 69-74 and 77-83 call
performSearchbut don't include it in their dependency arrays. This violates the exhaustive-deps rule and could cause stale closures.🔧 Proposed fix
// Load initial groups when dialog opens useEffect(() => { if (open) { setSearchQuery('') performSearch('') } - }, [open]) + }, [open, performSearch]) // Debounced search when query changes useEffect(() => { if (!open) return const timer = setTimeout(() => { performSearch(searchQuery) }, 300) return () => clearTimeout(timer) - }, [searchQuery, open]) + }, [searchQuery, open, performSearch])Alternatively, wrap
performSearchwithuseCallbackto stabilize its identity.🤖 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 `@frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx` around lines 69 - 83, The two useEffect hooks in add-namespace-dialog.tsx reference performSearch but don't list it as a dependency, which can cause stale closures; fix by either adding performSearch to both effect dependency arrays or (preferably) wrap performSearch in useCallback so its identity is stable and then include that memoized function in the dependency arrays for the effects that call it (the effects that read open, searchQuery, and call setSearchQuery/performSearch).frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx-665-672 (1)
665-672:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType assertion may be unsafe.
Line 669 uses
as KnowledgeBaseWithGroupInfoto castkb. Ifkbdoesn't have the expected properties (likeshare_sourceorshared_by_user_name),getKbShareSourceTextmight fail or return unexpected results. Consider adding a type guard or handling the union type properly.🛡️ Suggested improvement
{showFromInfo && ( <td className="px-6 py-3 text-text-secondary overflow-hidden"> <span className="truncate block"> - {getKbShareSourceText(kb as KnowledgeBaseWithGroupInfo)} + {'share_source' in kb ? getKbShareSourceText(kb as KnowledgeBaseWithGroupInfo) : '--'} </span> </td> )}Or verify the type at the call site level.
🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx` around lines 665 - 672, The code unsafely casts kb to KnowledgeBaseWithGroupInfo before calling getKbShareSourceText; instead add a type guard (e.g., isKnowledgeBaseWithGroupInfo(kb)) or check required properties (like share_source or shared_by_user_name) on kb and only call getKbShareSourceText(kb) when the guard passes, otherwise render a safe fallback (empty string or "Unknown") — implement an exported type guard function isKnowledgeBaseWithGroupInfo(value): value is KnowledgeBaseWithGroupInfo that checks the presence/types of share_source/shared_by_user_name and use it at the call site to avoid the unsafe `as` cast.backend/app/services/group_service.py-280-283 (1)
280-283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse implicit boolean check for
is_active.Line 281 uses
Namespace.is_active == Truewhich is flagged by Ruff E712 and discouraged by PEP 8. Use implicit boolean evaluation instead.🐛 Proposed fix
query = db.query(Namespace).filter( - Namespace.is_active == True, + Namespace.is_active.is_(True), Namespace.level == GroupLevel.group.value, )As per coding guidelines: follow PEP 8 standard.
🤖 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 `@backend/app/services/group_service.py` around lines 280 - 283, The filter currently uses an explicit boolean comparison (Namespace.is_active == True) which violates E712; update the filter in group_service.py where query = db.query(Namespace).filter(...) to use implicit truthiness by replacing Namespace.is_active == True with Namespace.is_active (i.e., pass Namespace.is_active, Namespace.level == GroupLevel.group.value) so the ORM boolean check follows PEP 8 and Ruff rules.frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx-269-279 (1)
269-279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing dependencies in useEffect hooks.
Similar to the AddNamespaceDialog, the useEffect hooks at lines 269-271 and 274-279 call
performSearchbut don't include it in their dependency arrays. This violates exhaustive-deps.🔧 Proposed fix
Wrap
performSearchwithuseCallback:+ const performSearch = useCallback(async (query: string) => { - const performSearch = async (query: string) => { setSearching(true) try { const result = await searchGroups({ q: query, limit: 20 }) setGroups(result.items || []) } catch (_err) { // silently fail } finally { setSearching(false) } - } + }, [])Then the existing dependency arrays will be correct.
🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx` around lines 269 - 279, The useEffect hooks call performSearch without listing it as a dependency (lines using useEffect that call performSearch), which violates exhaustive-deps; wrap performSearch in useCallback (e.g., define performSearch = useCallback(..., [its internal deps])) so it has a stable identity, then include performSearch in both useEffect dependency arrays (the initial empty-run effect and the debounced [searchQuery] effect should become [performSearch] and [searchQuery, performSearch] respectively) to satisfy linting and ensure correct behavior—mirror the approach used in AddNamespaceDialog.backend/tests/services/share/test_namespace_entity_resolver.py-120-123 (1)
120-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t bake DB ordering into this resolver test.
If
match_entity_bindings()does not promise output order, this assertion can flap across backends or query plans. Compare sets or sort both sides first.🤖 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 `@backend/tests/services/share/test_namespace_entity_resolver.py` around lines 120 - 123, The test currently asserts ordering for resolver.match_entity_bindings(test_db, user.id, "namespace", [str(ns1.id), str(ns2.id)]) which may be non-deterministic; modify the assertion to not rely on order by comparing unordered collections (e.g., compare set(result) to {str(ns1.id), str(ns2.id)}) or by sorting both sides before asserting, so the test no longer bakes DB-specific ordering into the expectation.frontend/src/features/knowledge/permission/components/PermissionManagementTab.tsx-53-56 (1)
53-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate the new source comments to English.
These added comments mix Chinese into the codebase, which breaks the repository’s comment-language rule.
As per coding guidelines, "
**/*.{py,ts,tsx,js,jsx}: All code comments MUST be written in English".Also applies to: 334-334, 520-520, 622-628
🤖 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 `@frontend/src/features/knowledge/permission/components/PermissionManagementTab.tsx` around lines 53 - 56, Update the Chinese inline comments in PermissionManagementTab.tsx to English: replace "个人/群组" and surrounding Chinese text with an English equivalent (e.g., "personal/group") so the top comment reads something like "Extension tabs rendered alongside standard tabs (Personal/Groups). Used by internal deployments to add entity-type permission panels." Also review and translate the other occurrences referenced (lines ~334, ~520, and ~622-628) to English, ensuring all comments in functions/components (e.g., PermissionManagementTab and any helper methods) follow the repository rule that all comments are in English.backend/app/services/share/base_service.py-1511-1524 (1)
1511-1524:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
share_link_idno longer distinguishes “joined by link” from “direct add.”
add_member()andbatch_add_members()now create a backingshare_link_idfor ordinary direct grants as well, so this helper will label those members asshare_link. That leaks intosource_typein member lists and permission-source responses.Suggested fix
if member.entity_type and member.entity_type != "user": return "entity_permission" - if member.share_link_id and member.share_link_id > 0: + if ( + member.share_link_id + and member.share_link_id > 0 + and member.invited_by_user_id == 0 + ): return "share_link" return "direct"🤖 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 `@backend/app/services/share/base_service.py` around lines 1511 - 1524, _member.share_link_id is now set for direct grants too, so _resolve_source_type mislabels those as "share_link"; change logic to rely on an explicit boolean flag (e.g. member.joined_via_share_link or member.invited_via_share_link) instead of just share_link_id. Update _resolve_source_type to return "share_link" only when that new flag is true (and member.share_link_id present), keep entity_type check as-is, and default to "direct". Also update add_member() and batch_add_members() to set that boolean only when the member was actually added via a share link (not for ordinary direct grants).
🧹 Nitpick comments (4)
backend/app/services/share/namespace_entity_resolver.py (1)
79-89: 💤 Low valueHardcoded resource type string.
Line 82 uses the hardcoded string
"Namespace". The codebase has aNAMESPACE_RESOURCE_TYPEconstant defined in other services (e.g.,group_service.pyline 33). Consider importing and using the constant for consistency.♻️ Suggested improvement
+ from app.services.group_member_helper import NAMESPACE_RESOURCE_TYPE + matched_rows = ( db.query(ResourceMember.resource_id) .filter( - ResourceMember.resource_type == "Namespace", + ResourceMember.resource_type == NAMESPACE_RESOURCE_TYPE, ResourceMember.resource_id.in_(namespace_ids),🤖 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 `@backend/app/services/share/namespace_entity_resolver.py` around lines 79 - 89, Replace the hardcoded string "Namespace" used in the ResourceMember.resource_type filter with the shared constant (e.g., NAMESPACE_RESOURCE_TYPE) to ensure consistency; import NAMESPACE_RESOURCE_TYPE from the module where it is defined (the same constant used in group_service.py) and update the query filter in namespace_entity_resolver.py to use ResourceMember.resource_type == NAMESPACE_RESOURCE_TYPE.frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx (1)
146-152: ⚡ Quick winSequential permission additions may be slow for multiple groups.
The loop at lines 147-149 makes sequential API calls. If a user selects many groups, this could be slow and provide poor feedback. Consider using
Promise.allorPromise.allSettledto parallelize, or provide per-group feedback.⚡ Proposed parallel approach
setLoading(true) try { - for (const group of selectedGroups) { - await knowledgePermissionApi.addNamespacePermission(kbId, group.id, role) - } + const results = await Promise.allSettled( + selectedGroups.map(group => + knowledgePermissionApi.addNamespacePermission(kbId, group.id, role) + ) + ) + const failed = results.filter(r => r.status === 'rejected') + if (failed.length > 0) { + setError(`Failed to add ${failed.length} group(s)`) + return + } resetForm() onSuccess?.() onOpenChange(false) } catch (err) {This provides faster execution and better error reporting.
🤖 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 `@frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx` around lines 146 - 152, The current try block iterates selectedGroups and awaits knowledgePermissionApi.addNamespacePermission(kbId, group.id, role) sequentially which is slow; instead map selectedGroups to promises and run them in parallel with Promise.allSettled (or Promise.all) to await completion, then handle results: collect failures to surface an error/notification, and only after settling call resetForm(), onSuccess?.(), and onOpenChange(false). Update the code where the loop is (the for...of over selectedGroups) to use selectedGroups.map(g => knowledgePermissionApi.addNamespacePermission(kbId, g.id, role)) and await Promise.allSettled(...), then process the returned statuses to determine success vs. failed group IDs.frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx (1)
63-67: 💤 Low valueSilent error suppression may hide extension loading issues.
Lines 64-65 dynamically import the extension loader and catch errors with an empty catch block. This silently suppresses any loading errors, making debugging difficult if extensions fail to load.
📝 Consider logging extension errors
useEffect(() => { import('../extension-loader').then(({ loadKBExtensions }) => { - loadKBExtensions().catch(() => {}) + loadKBExtensions().catch((err) => { + console.warn('Failed to load KB extensions:', err) + }) }) }, [])This helps developers identify issues during development.
🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx` around lines 63 - 67, The current useEffect silently swallows errors from the dynamic import/load sequence; update the promise rejection handler for import('../extension-loader').then(({ loadKBExtensions }) => loadKBExtensions()).catch(...) to log the error (e.g., console.error or your app logger) with a clear message like "Failed to load KB extensions" and include the error object so extension loading failures are visible; keep the same import and loadKBExtensions call but replace the empty catch handler with one that reports the error.backend/app/services/knowledge/knowledge_service.py (1)
2062-2564: 🏗️ Heavy liftSplit
get_all_knowledge_bases_groupedbefore this grows further.This method now owns querying, permission merging, metadata batching, response shaping, and summary calculation in one block. It is already hard to validate and exceeds the repo’s preferred size limits for both functions and files.
As per coding guidelines: "File size limit: if a file exceeds 1000 lines, split it into multiple sub-modules" and "Maximum function length of 50 lines preferred for all functions".
🤖 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 `@backend/app/services/knowledge/knowledge_service.py` around lines 2062 - 2564, get_all_knowledge_bases_grouped has grown too large and must be split into focused helper functions; extract the query logic, permission/role merging, metadata batching, and response-shaping into separate private helpers or small service modules and keep the top-level method as an orchestrator that calls them. Specifically: move the DB fetch blocks (personal_created/shared_kbs/group_kbs/org_kbs and namespace queries) into a new helper like _fetch_accessible_kbs(db, user_id) that returns those lists and maps; move the entity resolver usage into a thin wrapper that calls KnowledgeService._collect_entity_authorized_kbs; move the metadata batch logic into KnowledgeService._batch_fetch_kb_metadata (already exists) but call it from a new prepare_metadata helper; move the KB-to-response and merge_roles inline closures into private methods (e.g., _kb_to_response_wrapper calling _kb_to_response and _merge_roles); and move the aggregation of kb_sources and building of groups/organization/summary into a separate builder like _build_grouped_response that accepts the fetched data and metadata and returns AllGroupedKnowledgeResponse; update get_all_knowledge_bases_grouped to call these helpers in sequence, preserve existing symbols (_collect_entity_authorized_kbs, _batch_fetch_kb_metadata, _kb_to_response, _merge_roles) and ensure unit tests cover behavior to validate no logic changes.
🤖 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
`@backend/alembic/versions/20260506_d1e2f3a4b5c6_extend_resource_members_for_entity.py`:
- Around line 94-100: The migration only makes resource_members.user_id nullable
for MySQL; update the migration to also make user_id nullable on SQLite by using
Alembic's batch mode instead of raw SQL: inside the branch where dialect !=
"mysql" (or explicitly for "sqlite"), call
op.batch_alter_table('resource_members') and use alter_column('user_id',
existing_type=sa.Integer(), nullable=True, existing_comment="Member user ID
(kept for backward compatibility, use entity_type+entity_id for new code)") so
SQLite schema is updated correctly; reference op.execute (current MySQL branch),
op.batch_alter_table, alter_column, user_id and resource_members to locate and
apply the change.
In `@backend/app/api/endpoints/adapter/attachments.py`:
- Around line 824-825: The attachments.py module has grown too large; split
related attachment access/share logic into new submodules (e.g.,
attachments/access.py and attachments/share.py) by moving the functions and
routes that handle ResourceMember checks (references to
ResourceMember.entity_type / ResourceMember.entity_id and usages of
current_user.id), along with any helpers they use, into those files; keep shared
models/schemas imported from the original module (or a new package-level
__init__.py), update imports in the original attachments.py to re-export or
delegate to the new submodules, adjust relative imports for ResourceMember and
current_user, and run/update any tests or route registrations to point to the
new modules so functionality and names remain unchanged.
In `@backend/app/api/endpoints/knowledge.py`:
- Around line 290-318: The _add_initial_kb_members function and its logic
(including the InitialMemberCreate type usage and calls to
knowledge_share_service.add_member and db.commit()) should be moved out of the
oversized endpoints module into a new sub-module (e.g.,
app.api.endpoints.knowledge_initial_members or app.services.kb_members) to keep
the original file under 1000 lines; create the new module, paste
_add_initial_kb_members there, export it, update the original knowledge.py to
import _add_initial_kb_members from the new module, and ensure any references to
InitialMemberCreate and knowledge_share_service are either imported or passed in
(adjust imports at top of the new module and update callers to use the relocated
function).
In `@backend/app/models/resource_member.py`:
- Around line 255-297: The factory is silently normalizing missing/invalid
entity identities (setting entity_id to "" or leaving user_id None), which
allows creation of members that permission resolution cannot handle; update
ResourceMember.create and the related block around lines 305-313 to validate
entity_id instead of normalizing it: require a non-empty, well-formed entity_id
for all entity_type values (and keep the existing stricter check for entity_type
== "user"), raise ValueError when the identity is missing or malformed, and stop
calling code that replaces entity_id with "" or lets _sync_user_id_from_entity
silently produce user_id=None — alternatively add a helper like
_validate_entity_id(entity_type, entity_id) and call it from create() before
instantiating the member so invalid entities are rejected early.
In `@backend/app/schemas/knowledge.py`:
- Around line 75-82: The schema currently allows invalid member payloads because
entity_type is an unconstrained str and entity_id can be empty; update the
Pydantic fields in knowledge.py to restrict entity_type to only "user" or
"namespace" (e.g., use typing.Literal["user","namespace"] or an Enum for
entity_type) and require a non-empty entity_id by adding a min_length=1
constraint (or use pydantic.constr(min_length=1)) on the Field so payloads with
missing/invalid member identity are rejected at the schema boundary.
In `@backend/app/services/adapters/task_kinds/query_utils.py`:
- Around line 30-31: The SQL uses the stringified :entity_id for owner checks
against TaskResource.user_id (k.user_id and user_id predicates), causing a type
mismatch; change those predicates to compare against a new numeric bind
parameter :user_id (e.g., replace "k.user_id = :entity_id" and "user_id =
:entity_id" with "k.user_id = :user_id"/"user_id = :user_id"), keep :entity_id
for resource_members filtering, and update all bind payloads used by the
query-building code to include both keys (entity_id as string and user_id as
integer) so the DB can use integer indexes for owner lookups (apply this change
to the occurrences noted around k.user_id/user_id comparisons).
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 1987-2011: The code builds all_kb_ids and owner_user_ids but omits
remaining_shared_group_kbs, causing missing document counts and owner names;
update the all_kb_ids construction (the list passed to
KnowledgeService.get_active_document_counts) to include ids from
remaining_shared_group_kbs and also add a loop that adds each kb.user_id from
remaining_shared_group_kbs into owner_user_ids so those KBs are included in
metadata lookups and owner resolution.
- Around line 1907-1928: The current loop pulls all ResourceMember rows for
resolved_kb_ids and entity_type_str, which returns every external-entity record
on those KBs instead of only the binding(s) that actually matched the user;
change the logic so ResourceMember rows are filtered to the specific entity IDs
that matched per KB from get_resource_ids_by_entity() (use the matched-entity-id
mapping returned by get_resource_ids_by_entity(), then restrict the
query/filters so ResourceMember.entity_id is limited to those matched ids for
each resource_id). Update the query that constructs entity_type_members (and/or
post-filter the results) to include only rows where (ResourceMember.resource_id
== kb_id AND ResourceMember.entity_id IN matched_entity_ids_for_that_kb) while
keeping the existing filters (ResourceType.KNOWLEDGE_BASE, entity_type_str,
resolved_kb_ids, MemberStatus.APPROVED) so inherited roles/metadata come only
from bindings that actually matched the user.
In `@backend/app/services/share/base_service.py`:
- Around line 927-933: The logger.info call in the add_member flow currently
logs target_user.email which exposes PII; update the log in the block that
checks target_user (referencing target_user, target_user_id, and logger.info) to
remove the email field and only include non-PII such as user_id and user_name
(e.g., "Target user found: user_id=..., user_name=..."); ensure no other
references to target_user.email remain in that logging statement.
In `@backend/tests/api/endpoints/test_groups_api.py`:
- Around line 54-55: Tests are inconsistent with the migrated ResourceMember
schema: the helper _add_member now creates members with entity_type="user" and
entity_id=str(user.id) but assertions still use
ResourceMember.filter_by(user_id=...), so update all assertions (e.g., the
checks around lines referenced in the review) to query ResourceMember by
.filter_by(entity_type="user", entity_id=str(user.id)) or equivalent; locate the
assertions that use ResourceMember.filter_by(user_id=...) and replace them to
filter on entity_type/entity_id (and cast IDs to strings where helper uses
str(user.id)) so tests align with the new _add_member creation pattern.
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Around line 145-153: sharingKb state in KnowledgeDocumentPageDesktop is never
set so the share dialog has no opener; to fix, wire up setSharingKb to the UI
actions that should open the share flow (e.g., the share button/menu item for a
KB) so that selecting "share" assigns the selected KnowledgeBase to sharingKb
and triggers the dialog (same pattern used for editingKb, deletingKb,
migratingKb). Search for UI elements that open edit/delete/migrate dialogs and
add a handler that calls setSharingKb(kb) (and ensure the prop for closing the
dialog clears sharingKb). Apply the same fix for the other occurrences mentioned
(around the other ranges) so the share-link flow is restored.
- Around line 571-591: After deleting or migrating a KB the code (in
handleDelete and the corresponding migrate handler around lines 594-620) clears
the sidebar selection but does not update the browser route, leaving the app on
a stale KB URL; update both handlers to, after calling sidebar.clearSelection()
(and after sidebar.refreshAll()), use the router to navigate away from the
deleted/moved KB (e.g., router.push or navigate to the app's default/KB-list
route or sidebar.getDefaultRoute()) when deletingKb.id === sidebar.selectedKbId
so the URL reflects the new selection state and avoids pointing to a removed
resource. Ensure you reference the same async flow and keep
setIsDeleting/setIsMigrating semantics intact.
In `@frontend/src/features/knowledge/document/extension-loader.ts`:
- Around line 17-22: loadKBExtensions currently lets a failed dynamic import of
EXTENSION_MODULE reject and break callers; change it to treat import failures as
non-fatal by catching any error from import(EXTENSION_MODULE) and returning a
resolved Promise instead. Specifically, update loadKBExtensions to perform the
dynamic import and in the then() chain or a try/catch (if you convert it to
async) swallow the error, log a warning with the error and EXTENSION_MODULE name
(so you can debug) and always resolve (never rethrow) so callers of
loadKBExtensions are not disrupted.
In
`@frontend/src/features/knowledge/permission/components/PermissionManagementTab.tsx`:
- Around line 249-330: The tab buttons rendered in PermissionManagementTab (the
onClick handlers that call setActiveTab for the built-in tabs, the extensionTabs
map and the externalTabs map using externalRoleGroups) are not touch-accessible
and some row actions are hover-only; update the button classNames to include
touch-friendly sizing (add "h-11 min-w-[44px]") for the built-in, extension and
external tab buttons (the elements created in the extensionTabs.map and
externalTabs.map blocks) and change the row action visibility CSS from
hover-only to a focus-inclusive rule (e.g., expose edit/delete also on focus or
via a parent :focus-within state) so touch and keyboard users can access them.
Ensure the same changes are applied to the other listed UI areas noted in the
comment (the other button rows and row-action containers).
In `@frontend/src/types/knowledge.ts`:
- Around line 611-625: The PermissionUserInfo type requires updating so
entity/namespace permission rows can be represented: change the user_id property
on PermissionUserInfo to allow missing/null values (e.g. make it optional and
accept null or number) so namespace/external-entity rows without a concrete user
id conform to the type; update any related usages that assume a non-null number
to handle the optional/null user_id (refer to PermissionUserInfo.user_id in
frontend/src/types/knowledge.ts).
---
Outside diff comments:
In `@backend/app/schemas/share.py`:
- Around line 147-183: Add a Pydantic model-level validator to enforce that when
entity_type is set and not 'user' the entity_id must be provided (and optionally
user_id must be 0); locate the same model that contains the existing
`@field_validator` methods (validate_role and validate_entity_type) and add a
`@model_validator` (or root_validator for older Pydantic) method (e.g.,
validate_identity or validate_cross_fields) that checks if entity_type is not
None and entity_type.lower() != "user" then if not entity_id raise
ValueError("entity_id is required when entity_type is not 'user'") (and
optionally ensure user_id == 0 or raise) so invalid member combinations are
rejected at schema validation.
In `@backend/app/services/share/knowledge_share_service.py`:
- Around line 883-891: The membership check is using ResourceRole constants but
KnowledgeService._get_user_kb_permission returns BaseRole | None, so change the
role comparison to use BaseRole values: update the return logic in the
permission check (the block using has_access, role, is_creator) to return True
when is_creator is True, otherwise return has_access and role in
(BaseRole.Owner, BaseRole.Maintainer); reference
KnowledgeService._get_user_kb_permission, the local variables
has_access/role/is_creator, and replace ResourceRole.Owner/Maintainer with
BaseRole.Owner/BaseRole.Maintainer so shared maintainer/owner users retain
permission management access.
- Around line 184-220: The _get_resource function currently queries
ResourceMember using exact matches to ResourceType.KNOWLEDGE_BASE.value and
MemberStatus.APPROVED.value which excludes normalized/legacy variants; update
both the user-specific query (the one assigning `member`) and the entity_members
query to use the normalization helper variants instead: replace equality checks
on `ResourceMember.resource_type` and `ResourceMember.status` with membership
checks using `_resource_type_variants(ResourceType.KNOWLEDGE_BASE)` and
`_approved_status_variants()` (e.g.,
`.filter(ResourceMember.resource_type.in_(_resource_type_variants(...)),
ResourceMember.status.in_(_approved_status_variants()), ...)`) so
legacy/normalized rows are considered while keeping the existing
`member.get_effective_role()` and other entity_type/entity_id filters intact.
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Around line 313-321: The navigation branch is using initialKbNamespace to
detect virtual-URL mode, which is wrong for public KBs; change the gate to check
initialKbName (or a dedicated boolean like isVirtualPathMode) instead of
initialKbNamespace so that organization detail pages (where initialKbNamespace
is undefined) do not fall into the virtual URL logic; update the condition
around the router.push block (the code referencing initialKbNamespace,
params.group, and router.push(`/knowledge?...`)) to use initialKbName !==
undefined or isVirtualPathMode, and ensure any callers/props that set the new
flag are adjusted accordingly.
---
Minor comments:
In `@backend/app/services/group_service.py`:
- Around line 280-283: The filter currently uses an explicit boolean comparison
(Namespace.is_active == True) which violates E712; update the filter in
group_service.py where query = db.query(Namespace).filter(...) to use implicit
truthiness by replacing Namespace.is_active == True with Namespace.is_active
(i.e., pass Namespace.is_active, Namespace.level == GroupLevel.group.value) so
the ORM boolean check follows PEP 8 and Ruff rules.
In `@backend/app/services/share/base_service.py`:
- Around line 1511-1524: _member.share_link_id is now set for direct grants too,
so _resolve_source_type mislabels those as "share_link"; change logic to rely on
an explicit boolean flag (e.g. member.joined_via_share_link or
member.invited_via_share_link) instead of just share_link_id. Update
_resolve_source_type to return "share_link" only when that new flag is true (and
member.share_link_id present), keep entity_type check as-is, and default to
"direct". Also update add_member() and batch_add_members() to set that boolean
only when the member was actually added via a share link (not for ordinary
direct grants).
In `@backend/tests/services/share/test_erp_entity_resolver_cache.py`:
- Around line 76-90: The test name test_cache_ttl_is_one_hour is inconsistent
with the asserted TTL (expire == 900); update the test to reflect actual
behavior by renaming the test to test_cache_ttl_is_15_minutes (or change the
asserted value to 3600 if you intend one hour) and ensure the assertion inspects
mock_cache.set_sync.call_args[1].get("expire") after calling
resolver.match_entity_bindings so the name and the expected expire value (900
for 15 minutes or 3600 for one hour) match.
In `@backend/tests/services/share/test_namespace_entity_resolver.py`:
- Around line 120-123: The test currently asserts ordering for
resolver.match_entity_bindings(test_db, user.id, "namespace", [str(ns1.id),
str(ns2.id)]) which may be non-deterministic; modify the assertion to not rely
on order by comparing unordered collections (e.g., compare set(result) to
{str(ns1.id), str(ns2.id)}) or by sorting both sides before asserting, so the
test no longer bakes DB-specific ordering into the expectation.
In
`@frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx`:
- Around line 152-157: The SelectTrigger for the role selector is currently
using h-10 (40px) and should be increased to meet the 44px touch-minimum; update
the SelectTrigger className in the KnowledgeBaseAuthSection component (the
Select/SelectTrigger used alongside value={role} and onValueChange calling
setRoleForType with MemberRole) to use h-11 (e.g., "w-28 h-11 min-w-[44px]
flex-shrink-0") so the interactive element meets the 44×44px guideline.
- Around line 269-279: The useEffect hooks call performSearch without listing it
as a dependency (lines using useEffect that call performSearch), which violates
exhaustive-deps; wrap performSearch in useCallback (e.g., define performSearch =
useCallback(..., [its internal deps])) so it has a stable identity, then include
performSearch in both useEffect dependency arrays (the initial empty-run effect
and the debounced [searchQuery] effect should become [performSearch] and
[searchQuery, performSearch] respectively) to satisfy linting and ensure correct
behavior—mirror the approach used in AddNamespaceDialog.
In
`@frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx`:
- Around line 665-672: The code unsafely casts kb to KnowledgeBaseWithGroupInfo
before calling getKbShareSourceText; instead add a type guard (e.g.,
isKnowledgeBaseWithGroupInfo(kb)) or check required properties (like
share_source or shared_by_user_name) on kb and only call
getKbShareSourceText(kb) when the guard passes, otherwise render a safe fallback
(empty string or "Unknown") — implement an exported type guard function
isKnowledgeBaseWithGroupInfo(value): value is KnowledgeBaseWithGroupInfo that
checks the presence/types of share_source/shared_by_user_name and use it at the
call site to avoid the unsafe `as` cast.
In
`@frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx`:
- Around line 69-83: The two useEffect hooks in add-namespace-dialog.tsx
reference performSearch but don't list it as a dependency, which can cause stale
closures; fix by either adding performSearch to both effect dependency arrays or
(preferably) wrap performSearch in useCallback so its identity is stable and
then include that memoized function in the dependency arrays for the effects
that call it (the effects that read open, searchQuery, and call
setSearchQuery/performSearch).
In
`@frontend/src/features/knowledge/permission/components/PermissionManagementTab.tsx`:
- Around line 53-56: Update the Chinese inline comments in
PermissionManagementTab.tsx to English: replace "个人/群组" and surrounding Chinese
text with an English equivalent (e.g., "personal/group") so the top comment
reads something like "Extension tabs rendered alongside standard tabs
(Personal/Groups). Used by internal deployments to add entity-type permission
panels." Also review and translate the other occurrences referenced (lines ~334,
~520, and ~622-628) to English, ensuring all comments in functions/components
(e.g., PermissionManagementTab and any helper methods) follow the repository
rule that all comments are in English.
---
Nitpick comments:
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 2062-2564: get_all_knowledge_bases_grouped has grown too large and
must be split into focused helper functions; extract the query logic,
permission/role merging, metadata batching, and response-shaping into separate
private helpers or small service modules and keep the top-level method as an
orchestrator that calls them. Specifically: move the DB fetch blocks
(personal_created/shared_kbs/group_kbs/org_kbs and namespace queries) into a new
helper like _fetch_accessible_kbs(db, user_id) that returns those lists and
maps; move the entity resolver usage into a thin wrapper that calls
KnowledgeService._collect_entity_authorized_kbs; move the metadata batch logic
into KnowledgeService._batch_fetch_kb_metadata (already exists) but call it from
a new prepare_metadata helper; move the KB-to-response and merge_roles inline
closures into private methods (e.g., _kb_to_response_wrapper calling
_kb_to_response and _merge_roles); and move the aggregation of kb_sources and
building of groups/organization/summary into a separate builder like
_build_grouped_response that accepts the fetched data and metadata and returns
AllGroupedKnowledgeResponse; update get_all_knowledge_bases_grouped to call
these helpers in sequence, preserve existing symbols
(_collect_entity_authorized_kbs, _batch_fetch_kb_metadata, _kb_to_response,
_merge_roles) and ensure unit tests cover behavior to validate no logic changes.
In `@backend/app/services/share/namespace_entity_resolver.py`:
- Around line 79-89: Replace the hardcoded string "Namespace" used in the
ResourceMember.resource_type filter with the shared constant (e.g.,
NAMESPACE_RESOURCE_TYPE) to ensure consistency; import NAMESPACE_RESOURCE_TYPE
from the module where it is defined (the same constant used in group_service.py)
and update the query filter in namespace_entity_resolver.py to use
ResourceMember.resource_type == NAMESPACE_RESOURCE_TYPE.
In
`@frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx`:
- Around line 63-67: The current useEffect silently swallows errors from the
dynamic import/load sequence; update the promise rejection handler for
import('../extension-loader').then(({ loadKBExtensions }) =>
loadKBExtensions()).catch(...) to log the error (e.g., console.error or your app
logger) with a clear message like "Failed to load KB extensions" and include the
error object so extension loading failures are visible; keep the same import and
loadKBExtensions call but replace the empty catch handler with one that reports
the error.
In
`@frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx`:
- Around line 146-152: The current try block iterates selectedGroups and awaits
knowledgePermissionApi.addNamespacePermission(kbId, group.id, role) sequentially
which is slow; instead map selectedGroups to promises and run them in parallel
with Promise.allSettled (or Promise.all) to await completion, then handle
results: collect failures to surface an error/notification, and only after
settling call resetForm(), onSuccess?.(), and onOpenChange(false). Update the
code where the loop is (the for...of over selectedGroups) to use
selectedGroups.map(g => knowledgePermissionApi.addNamespacePermission(kbId,
g.id, role)) and await Promise.allSettled(...), then process the returned
statuses to determine success vs. failed group IDs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dfec83a-7b86-4485-9d02-f02465ddd53b
📒 Files selected for processing (71)
backend/alembic/versions/20260506_d1e2f3a4b5c6_extend_resource_members_for_entity.pybackend/app/api/endpoints/adapter/attachments.pybackend/app/api/endpoints/adapter/chat.pybackend/app/api/endpoints/groups.pybackend/app/api/endpoints/knowledge.pybackend/app/api/endpoints/share.pybackend/app/core/cache.pybackend/app/models/resource_member.pybackend/app/models/user.pybackend/app/schemas/base_role.pybackend/app/schemas/knowledge.pybackend/app/schemas/share.pybackend/app/services/adapters/task_kinds/operations.pybackend/app/services/adapters/task_kinds/query_utils.pybackend/app/services/adapters/task_kinds/running_tasks.pybackend/app/services/adapters/team_kinds.pybackend/app/services/chat/access/permissions.pybackend/app/services/chat/storage/task_manager.pybackend/app/services/group_member_helper.pybackend/app/services/group_service.pybackend/app/services/knowledge/knowledge_service.pybackend/app/services/knowledge/task_knowledge_base_service.pybackend/app/services/readers/group_members.pybackend/app/services/readers/shared_teams.pybackend/app/services/share/__init__.pybackend/app/services/share/base_service.pybackend/app/services/share/external_entity_resolver.pybackend/app/services/share/knowledge_share_service.pybackend/app/services/share/namespace_entity_resolver.pybackend/app/services/share/team_share_service.pybackend/app/services/shared_task.pybackend/app/services/shared_team.pybackend/app/services/task_member_service.pybackend/tests/api/endpoints/test_group_namespace_permissions.pybackend/tests/api/endpoints/test_groups_api.pybackend/tests/api/test_batch_add_members.pybackend/tests/api/test_skills_api.pybackend/tests/services/knowledge/test_external_entity_permissions.pybackend/tests/services/knowledge/test_namespace_knowledge_permissions.pybackend/tests/services/knowledge/test_permission_policy.pybackend/tests/services/share/test_check_entity_permission.pybackend/tests/services/share/test_erp_entity_resolver_cache.pybackend/tests/services/share/test_namespace_entity_resolver.pybackend/tests/services/test_task_member_service_group_chat.pydocs/en/user-guide/knowledge/knowledge-base-guide.mddocs/zh/user-guide/knowledge/knowledge-base-guide.mdfrontend/src/apis/groups.tsfrontend/src/apis/knowledge-permission.tsfrontend/src/features/knowledge/document/auth-section-registry.tsfrontend/src/features/knowledge/document/components/CommandPalette.tsxfrontend/src/features/knowledge/document/components/CreateKnowledgeBaseDialog.tsxfrontend/src/features/knowledge/document/components/DocumentPanel.tsxfrontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsxfrontend/src/features/knowledge/document/components/KnowledgeBaseForm.tsxfrontend/src/features/knowledge/document/components/KnowledgeDetailPanel.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageMobile.tsxfrontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsxfrontend/src/features/knowledge/document/components/KnowledgeSidebar/index.tsxfrontend/src/features/knowledge/document/extension-loader.tsfrontend/src/features/knowledge/document/hooks/useGroupKbs.tsfrontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.tsfrontend/src/features/knowledge/document/pages/KnowledgeBaseClassicPageDesktop.tsxfrontend/src/features/knowledge/document/pages/KnowledgeBaseClassicPageMobile.tsxfrontend/src/features/knowledge/permission/components/PermissionManagementTab.tsxfrontend/src/features/knowledge/permission/components/add-namespace-dialog.tsxfrontend/src/features/knowledge/permission/permission-tab-registry.tsfrontend/src/features/knowledge/utils/share-source.tsfrontend/src/i18n/locales/en/knowledge.jsonfrontend/src/i18n/locales/zh-CN/knowledge.jsonfrontend/src/types/knowledge.ts
💤 Files with no reviewable changes (1)
- backend/app/models/user.py
cocowh
force-pushed
the
wegent/kb-entity-permission
branch
from
May 11, 2026 23:55
2010849 to
bd7b9f9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx (1)
64-1055: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this module — file is over the 1000-line limit and the component/handlers exceed the function-length budget.
The file is now 1055 lines and the
KnowledgeDocumentPageDesktopcomponent body spans ~990 lines, withhandleCreatealone at ~76 lines (lines 468-543). The component inlines URL-sync, dialog state, CRUD handlers, migration logic, and rendering — all of which were previously consolidated inuseKnowledgeDialogs. Inlining made the previously-separated dialog opener for share/migrate easy to drop on the floor (see past comments onsharingKband stale URLs).Recommended decomposition:
- Extract dialog state + handlers (create/update/delete/share/migrate) back into a hook (e.g.
useKnowledgeBaseDialogs) returning state, openers, and a<Dialogs />element.- Extract the URL-sync logic (the initial-load effect +
updateUrlParams+navigateToKb) into auseKnowledgeUrlSynchook.- Extract
renderMainContent's per-view branches (groups,dingtalk,all, group-detail) into small presentational components.This also restores a single source of truth for opening the share dialog (currently unreachable from this screen) and gives delete/migrate a single place to push the route after clearing selection.
As per coding guidelines: "File size limit: if a file exceeds 1000 lines, split it into multiple sub-modules" and "Maximum function length of 50 lines preferred for all functions".
🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx` around lines 64 - 1055, The component KnowledgeDocumentPageDesktop is too large (>1000 lines) with inlined dialog and URL sync logic (e.g., handleCreate, renderMainContent), violating the file/function size rules; extract responsibilities into hooks and smaller components. Fix by extracting dialog state and CRUD handlers (create/update/delete/share/migrate and their state like showCreateDialog, editingKb, deletingKb, migratingKb, and functions such as handleCreate, handleUpdate, handleDelete, handleMigrate) into a new hook useKnowledgeBaseDialogs that returns state, opener functions and a Dialogs JSX element; extract URL-sync logic (the initial-load useEffect, updateUrlParams, navigateToKb and initialUrlSyncDone) into a useKnowledgeUrlSync hook that exposes sync helpers; and split renderMainContent’s per-view branches (groups, dingtalk, all, group-detail) into small presentational components (e.g., KnowledgeAllView, KnowledgeGroupsView, KnowledgeGroupDetailView, DingtalkDocsView) that accept necessary props (sidebar, handlers, flags) so KnowledgeDocumentPageDesktop becomes a thin orchestrator. Ensure the new hooks/components preserve behavior and single source of truth for opening share/migrate dialogs and call sidebar.refreshAll / sidebar.clearSelection where previously done.
♻️ Duplicate comments (2)
frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx (2)
152-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
sharingKbstill has no opener — share dialog remains unreachable from this screen.
setSharingKbis only invoked from<ShareLinkDialog onOpenChange>to clear state. No KB card, menu item, or detail-panel action wiressetSharingKb(kb), so the dialog at lines 1038-1043 can never be opened fromKnowledgeDocumentPageDesktop. Either pass anonShareKbcallback down toKnowledgeGroupListPage/KnowledgeDetailPanel, or remove the dialog andsharingKbstate until it is wired up.Also applies to: 1038-1043
🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx` at line 152, The sharingKb state and ShareLinkDialog are never opened because setSharingKb is never called; either wire an opener or remove them. Add an onShareKb callback prop (e.g., onShareKb: (kb: KnowledgeBase) => void) to the components rendered inside KnowledgeDocumentPageDesktop such as KnowledgeGroupListPage and KnowledgeDetailPanel and pass setSharingKb as that callback so clicking a KB card/menu/action calls setSharingKb(kb) and opens the ShareLinkDialog, or if you prefer not to expose sharing yet, remove sharingKb, setSharingKb and the ShareLinkDialog usage (lines with ShareLinkDialog and the sharingKb state) until a caller is implemented.
571-621:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuccessful delete/migrate still leaves the browser on the deleted KB's URL.
handleDelete(lines 571-591) andhandleMigrate(lines 594-621) both callsidebar.clearSelection()when the affected KB is selected, but neither updates the route. In virtual-URL mode (/knowledge/{namespace}/{name}) this leaves the address bar pointing at a KB that no longer exists or has moved; on next navigation/refresh the initial-URL-sync effect will try to re-select it and fall through to the not-found state. Push back to/knowledge?type=document(or to the new group route for migrate) after clearing selection.🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx` around lines 571 - 621, After clearing the sidebar selection in handleDelete and handleMigrate, also navigate the browser away from the deleted/moved KB so the URL no longer points at a removed resource: when deleting (inside handleDelete, after sidebar.clearSelection() and when deletingKb.id === sidebar.selectedKbId) call the router push to /knowledge?type=document; when migrating (inside handleMigrate, after sidebar.clearSelection() and when migratingKb.id === sidebar.selectedKbId) push to the appropriate group route (e.g. the target group's /knowledge route or its namespace/name) instead of leaving the current URL; use the same router instance used elsewhere in this component (e.g., Next.js router or useNavigate) so route changes occur after successful delete/migrate and before setDeleting/setMigrating clears.
🧹 Nitpick comments (2)
backend/app/services/group_service.py (1)
281-281: ⚡ Quick winSimplify boolean comparison.
Static analysis correctly flags
== Trueas unnecessary. Use the truthy value directly or.is_(True)for explicit SQL generation.♻️ Proposed fix
- Namespace.is_active == True, + Namespace.is_active.is_(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 `@backend/app/services/group_service.py` at line 281, Replace the unnecessary boolean comparison "Namespace.is_active == True" with a direct truthy check or explicit SQLAlchemy predicate; locate usages of the Namespace.is_active symbol (e.g., in the query/filter within the GroupService functions) and change to either "Namespace.is_active" or "Namespace.is_active.is_(True)" to produce clearer, idiomatic SQLAlchemy code.backend/app/services/share/external_entity_resolver.py (1)
44-56: ⚡ Quick winConsider caching resolver instances for better performance.
get_entity_resolvercreates a new resolver instance on every call (line 55). If resolvers are stateless, this repeated instantiation is wasteful. Consider caching instances in a module-level dictionary or using a singleton pattern to avoid redundant object creation.♻️ Proposed fix to cache resolver instances
# Module-level resolver registry _external_entity_resolvers: dict[str, type["IExternalEntityResolver"]] = {} +_resolver_instances: dict[str, "IExternalEntityResolver"] = {} def register_entity_resolver( entity_type: str, resolver_cls: type["IExternalEntityResolver"] ) -> None: """Register an external entity resolver for a specific entity type. Args: entity_type: The entity type to handle (e.g., 'namespace') resolver_cls: The resolver class implementing IExternalEntityResolver """ _external_entity_resolvers[entity_type] = resolver_cls + _resolver_instances.pop(entity_type, None) # Clear cached instance logger.info( f"Registered external entity resolver for type='{entity_type}': " f"{resolver_cls.__name__}" ) def get_entity_resolver(entity_type: str) -> Optional["IExternalEntityResolver"]: """Get a resolver instance for the given entity type. Args: entity_type: The entity type to get a resolver for Returns: An IExternalEntityResolver instance, or None if no resolver is registered """ + if entity_type in _resolver_instances: + return _resolver_instances[entity_type] + cls = _external_entity_resolvers.get(entity_type) if cls: - return cls() + instance = cls() + _resolver_instances[entity_type] = instance + return instance return None🤖 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 `@backend/app/services/share/external_entity_resolver.py` around lines 44 - 56, get_entity_resolver currently constructs a new resolver for every call by doing cls() from _external_entity_resolvers; change it to reuse instances by introducing a module-level cache (e.g., _external_entity_resolver_instances: Dict[str, IExternalEntityResolver]) and on get_entity_resolver look up the cache first, instantiate and store the instance only if missing, then return the cached instance; ensure you still return None when no class is registered and preserve types by returning Optional["IExternalEntityResolver"].
🤖 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 `@backend/app/core/cache.py`:
- Around line 143-145: In set_sync (cache.py) narrow the broad except: replace
"except Exception as e" with a targeted catch for Redis and serialization errors
(for example except (redis.exceptions.RedisError,
redis.exceptions.ConnectionError, TypeError, ValueError) as e) so only
cache-related failures are handled, keep the existing logger.error message and
return False fallback, and add an appropriate import for redis.exceptions if
missing; leave other exceptions to propagate.
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 1943-1957: The same KBs are being appended to both
entity_shared_to_me_kbs and shared_into_group_kbs; fix by ensuring KBs that are
added to shared_into_group_kbs are excluded from entity_shared_to_me_kbs. After
you build shared_into_group_kbs (using entity_kbs, entity_member_group_map,
accessible_groups), create a set of their ids and filter entity_shared_to_me_kbs
to remove any ekb whose id is in that set (or alternatively check membership
before appending to entity_shared_to_me_kbs in the earlier loop). Use the
existing symbols entity_shared_to_me_kbs, shared_into_group_kbs, entity_kbs,
entity_member_group_map, and accessible_groups to locate and implement this
change.
- Around line 2543-2551: The summary currently omits inaccessible-group direct
shares (remaining_shared_group_kbs) from the AllGroupedSummary counts; update
the computation of total_count and personal_count in the AllGroupedSummary
construction to include len(remaining_shared_group_kbs) alongside the existing
lists (personal_created, personal_shared, entity_shared_to_me_kbs, group_kbs,
org_kbs) so that remaining_shared_group_kbs is accounted for in both total_count
and personal_count rendered by personal.shared_with_me.
- Around line 2398-2423: The aggregation is non-deterministic due to iterating
over dict/set insertion order and using next(iter(...)); make it deterministic
by explicitly sorting the collections before picking or deduplicating: for
kb_sources loop, sort info.inviter_ids (e.g., by inviter id or by
inviter_user_map.get(uid) fallback) before building source_names and before
selecting primary_inviter/shared_from, sort info.source_groups
(lexicographically) before deduplicating and choosing primary_source_group, and
sort info.shared_vias before choosing primary_via; update the logic around
merged_role, source_names, primary_inviter/shared_from, primary_source_group,
and primary_via to use these sorted lists so output order and chosen primary
values are stable across runs.
In `@backend/app/services/share/knowledge_share_service.py`:
- Line 207: In the _get_resource method of KnowledgeShareService (look for the
logger.info call that logs "[_get_resource] User has NO explicit shared
access"), remove the unnecessary f-string prefix on the literal since there are
no placeholders; replace f"[_get_resource] User has NO explicit shared access"
with a plain string "[_get_resource] User has NO explicit shared access".
- Around line 412-433: In the members_data iteration where you unpack
(target_user_id, role, ent_type, ent_id, ent_display_name), change the unused
variables to be prefixed with an underscore (e.g., _target_user_id, _role,
_ent_display_name) so intent is clear; update the tuple unpack in the for loop
inside the function/method that processes members_data (the loop that checks
ent_type == "namespace" and uses db, kb, Namespace, and FailedMemberResponse)
and leave ent_type and ent_id unchanged.
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Line 131: Replace the Chinese characters in the inline comment inside the
KnowledgeDocumentPageDesktop component with English; specifically update the
comment that references TaskSidebar and the `"知识"` button to use an English
phrase like `"Knowledge"` so the comment reads e.g. "// Listen for clear
selection event from TaskSidebar (when user clicks 'Knowledge' button while
already on knowledge page)". Ensure the updated comment is placed near the
existing code that handles the clear selection event in
KnowledgeDocumentPageDesktop and mentions TaskSidebar.
---
Outside diff comments:
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Around line 64-1055: The component KnowledgeDocumentPageDesktop is too large
(>1000 lines) with inlined dialog and URL sync logic (e.g., handleCreate,
renderMainContent), violating the file/function size rules; extract
responsibilities into hooks and smaller components. Fix by extracting dialog
state and CRUD handlers (create/update/delete/share/migrate and their state like
showCreateDialog, editingKb, deletingKb, migratingKb, and functions such as
handleCreate, handleUpdate, handleDelete, handleMigrate) into a new hook
useKnowledgeBaseDialogs that returns state, opener functions and a Dialogs JSX
element; extract URL-sync logic (the initial-load useEffect, updateUrlParams,
navigateToKb and initialUrlSyncDone) into a useKnowledgeUrlSync hook that
exposes sync helpers; and split renderMainContent’s per-view branches (groups,
dingtalk, all, group-detail) into small presentational components (e.g.,
KnowledgeAllView, KnowledgeGroupsView, KnowledgeGroupDetailView,
DingtalkDocsView) that accept necessary props (sidebar, handlers, flags) so
KnowledgeDocumentPageDesktop becomes a thin orchestrator. Ensure the new
hooks/components preserve behavior and single source of truth for opening
share/migrate dialogs and call sidebar.refreshAll / sidebar.clearSelection where
previously done.
---
Duplicate comments:
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Line 152: The sharingKb state and ShareLinkDialog are never opened because
setSharingKb is never called; either wire an opener or remove them. Add an
onShareKb callback prop (e.g., onShareKb: (kb: KnowledgeBase) => void) to the
components rendered inside KnowledgeDocumentPageDesktop such as
KnowledgeGroupListPage and KnowledgeDetailPanel and pass setSharingKb as that
callback so clicking a KB card/menu/action calls setSharingKb(kb) and opens the
ShareLinkDialog, or if you prefer not to expose sharing yet, remove sharingKb,
setSharingKb and the ShareLinkDialog usage (lines with ShareLinkDialog and the
sharingKb state) until a caller is implemented.
- Around line 571-621: After clearing the sidebar selection in handleDelete and
handleMigrate, also navigate the browser away from the deleted/moved KB so the
URL no longer points at a removed resource: when deleting (inside handleDelete,
after sidebar.clearSelection() and when deletingKb.id === sidebar.selectedKbId)
call the router push to /knowledge?type=document; when migrating (inside
handleMigrate, after sidebar.clearSelection() and when migratingKb.id ===
sidebar.selectedKbId) push to the appropriate group route (e.g. the target
group's /knowledge route or its namespace/name) instead of leaving the current
URL; use the same router instance used elsewhere in this component (e.g.,
Next.js router or useNavigate) so route changes occur after successful
delete/migrate and before setDeleting/setMigrating clears.
---
Nitpick comments:
In `@backend/app/services/group_service.py`:
- Line 281: Replace the unnecessary boolean comparison "Namespace.is_active ==
True" with a direct truthy check or explicit SQLAlchemy predicate; locate usages
of the Namespace.is_active symbol (e.g., in the query/filter within the
GroupService functions) and change to either "Namespace.is_active" or
"Namespace.is_active.is_(True)" to produce clearer, idiomatic SQLAlchemy code.
In `@backend/app/services/share/external_entity_resolver.py`:
- Around line 44-56: get_entity_resolver currently constructs a new resolver for
every call by doing cls() from _external_entity_resolvers; change it to reuse
instances by introducing a module-level cache (e.g.,
_external_entity_resolver_instances: Dict[str, IExternalEntityResolver]) and on
get_entity_resolver look up the cache first, instantiate and store the instance
only if missing, then return the cached instance; ensure you still return None
when no class is registered and preserve types by returning
Optional["IExternalEntityResolver"].
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ad9ddf7-5e90-417d-be54-0747942219bc
📒 Files selected for processing (71)
backend/alembic/versions/20260506_d1e2f3a4b5c6_extend_resource_members_for_entity.pybackend/app/api/endpoints/adapter/attachments.pybackend/app/api/endpoints/adapter/chat.pybackend/app/api/endpoints/groups.pybackend/app/api/endpoints/knowledge.pybackend/app/api/endpoints/share.pybackend/app/core/cache.pybackend/app/models/resource_member.pybackend/app/models/user.pybackend/app/schemas/base_role.pybackend/app/schemas/knowledge.pybackend/app/schemas/share.pybackend/app/services/adapters/task_kinds/operations.pybackend/app/services/adapters/task_kinds/query_utils.pybackend/app/services/adapters/task_kinds/running_tasks.pybackend/app/services/adapters/team_kinds.pybackend/app/services/chat/access/permissions.pybackend/app/services/chat/storage/task_manager.pybackend/app/services/group_member_helper.pybackend/app/services/group_service.pybackend/app/services/knowledge/knowledge_service.pybackend/app/services/knowledge/task_knowledge_base_service.pybackend/app/services/readers/group_members.pybackend/app/services/readers/shared_teams.pybackend/app/services/share/__init__.pybackend/app/services/share/base_service.pybackend/app/services/share/external_entity_resolver.pybackend/app/services/share/knowledge_share_service.pybackend/app/services/share/namespace_entity_resolver.pybackend/app/services/share/team_share_service.pybackend/app/services/shared_task.pybackend/app/services/shared_team.pybackend/app/services/task_member_service.pybackend/tests/api/endpoints/test_group_namespace_permissions.pybackend/tests/api/endpoints/test_groups_api.pybackend/tests/api/test_batch_add_members.pybackend/tests/api/test_skills_api.pybackend/tests/services/adapters/test_team_kinds_copy.pybackend/tests/services/knowledge/test_external_entity_permissions.pybackend/tests/services/knowledge/test_namespace_knowledge_permissions.pybackend/tests/services/knowledge/test_permission_policy.pybackend/tests/services/share/test_check_entity_permission.pybackend/tests/services/share/test_namespace_entity_resolver.pybackend/tests/services/test_task_member_service_group_chat.pydocs/en/user-guide/knowledge/knowledge-base-guide.mddocs/zh/user-guide/knowledge/knowledge-base-guide.mdfrontend/src/apis/groups.tsfrontend/src/apis/knowledge-permission.tsfrontend/src/features/knowledge/document/auth-section-registry.tsfrontend/src/features/knowledge/document/components/CommandPalette.tsxfrontend/src/features/knowledge/document/components/CreateKnowledgeBaseDialog.tsxfrontend/src/features/knowledge/document/components/DocumentPanel.tsxfrontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsxfrontend/src/features/knowledge/document/components/KnowledgeBaseForm.tsxfrontend/src/features/knowledge/document/components/KnowledgeDetailPanel.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageMobile.tsxfrontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsxfrontend/src/features/knowledge/document/components/KnowledgeSidebar/index.tsxfrontend/src/features/knowledge/document/extension-loader.tsfrontend/src/features/knowledge/document/hooks/useGroupKbs.tsfrontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.tsfrontend/src/features/knowledge/document/pages/KnowledgeBaseClassicPageDesktop.tsxfrontend/src/features/knowledge/document/pages/KnowledgeBaseClassicPageMobile.tsxfrontend/src/features/knowledge/permission/components/PermissionManagementTab.tsxfrontend/src/features/knowledge/permission/components/add-namespace-dialog.tsxfrontend/src/features/knowledge/permission/permission-tab-registry.tsfrontend/src/features/knowledge/utils/share-source.tsfrontend/src/i18n/locales/en/knowledge.jsonfrontend/src/i18n/locales/zh-CN/knowledge.jsonfrontend/src/types/knowledge.ts
💤 Files with no reviewable changes (1)
- backend/app/models/user.py
✅ Files skipped from review due to trivial changes (2)
- docs/en/user-guide/knowledge/knowledge-base-guide.md
- backend/tests/services/test_task_member_service_group_chat.py
🚧 Files skipped from review as they are similar to previous changes (59)
- frontend/src/features/knowledge/document/pages/KnowledgeBaseClassicPageDesktop.tsx
- frontend/src/features/knowledge/document/components/KnowledgeDetailPanel.tsx
- frontend/src/features/knowledge/document/components/CommandPalette.tsx
- frontend/src/features/knowledge/document/components/DocumentPanel.tsx
- frontend/src/features/knowledge/permission/permission-tab-registry.ts
- backend/app/services/knowledge/task_knowledge_base_service.py
- backend/app/services/task_member_service.py
- frontend/src/features/knowledge/document/components/KnowledgeSidebar/index.tsx
- frontend/src/apis/knowledge-permission.ts
- backend/app/services/group_member_helper.py
- frontend/src/features/knowledge/document/components/KnowledgeDocumentPageMobile.tsx
- backend/app/services/readers/shared_teams.py
- frontend/src/i18n/locales/en/knowledge.json
- frontend/src/features/knowledge/document/hooks/useGroupKbs.ts
- backend/app/services/shared_task.py
- frontend/src/features/knowledge/document/pages/KnowledgeBaseClassicPageMobile.tsx
- frontend/src/features/knowledge/document/components/KnowledgeBaseForm.tsx
- backend/app/services/adapters/team_kinds.py
- backend/tests/services/knowledge/test_permission_policy.py
- backend/tests/api/endpoints/test_group_namespace_permissions.py
- frontend/src/features/knowledge/document/auth-section-registry.ts
- backend/app/services/readers/group_members.py
- frontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.ts
- backend/app/api/endpoints/adapter/attachments.py
- backend/app/api/endpoints/adapter/chat.py
- frontend/src/features/knowledge/utils/share-source.ts
- docs/zh/user-guide/knowledge/knowledge-base-guide.md
- backend/app/services/adapters/task_kinds/operations.py
- backend/app/schemas/base_role.py
- backend/app/services/share/team_share_service.py
- backend/tests/api/endpoints/test_groups_api.py
- frontend/src/types/knowledge.ts
- backend/app/services/share/namespace_entity_resolver.py
- backend/app/schemas/knowledge.py
- frontend/src/features/knowledge/document/extension-loader.ts
- frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx
- backend/app/api/endpoints/knowledge.py
- backend/app/services/adapters/task_kinds/query_utils.py
- backend/app/services/shared_team.py
- frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx
- backend/tests/api/test_skills_api.py
- backend/tests/services/knowledge/test_namespace_knowledge_permissions.py
- backend/tests/api/test_batch_add_members.py
- frontend/src/i18n/locales/zh-CN/knowledge.json
- backend/app/services/chat/storage/task_manager.py
- backend/app/services/share/init.py
- backend/app/schemas/share.py
- frontend/src/apis/groups.ts
- frontend/src/features/knowledge/document/components/CreateKnowledgeBaseDialog.tsx
- backend/app/models/resource_member.py
- backend/app/services/chat/access/permissions.py
- backend/tests/services/share/test_check_entity_permission.py
- backend/tests/services/knowledge/test_external_entity_permissions.py
- backend/tests/services/share/test_namespace_entity_resolver.py
- frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx
- backend/alembic/versions/20260506_d1e2f3a4b5c6_extend_resource_members_for_entity.py
- frontend/src/features/knowledge/permission/components/PermissionManagementTab.tsx
- backend/app/services/share/base_service.py
- backend/app/services/adapters/task_kinds/running_tasks.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/services/knowledge/knowledge_service.py (1)
2062-2584: 🏗️ Heavy liftSplit
get_all_knowledge_bases_grouped(and_collect_entity_authorized_kbs) into smaller helpers.
get_all_knowledge_bases_groupednow spans ~520 lines and_collect_entity_authorized_kbs~140 lines, well over the preferred 50‑line ceiling. The function mixes data-loading, classification (personal/group/org/entity), source aggregation, and response building, which makes the multi-source merging bugs already flagged on this PR easy to introduce and hard to test. Consider extracting cohesive helpers, e.g.:
_load_direct_shares(db, user_id) -> (personal_shared, shared_group_kbs, shared_kb_roles, shared_kb_inviter_map, remaining_shared_group_kbs)_classify_entity_kbs(...)/ split the namespace branch and the external-resolver branch of_collect_entity_authorized_kbs_build_shared_with_me(...)around theKbSourceInfoaggregation (Lines 2343-2456)_build_groups_section(...)for Lines 2458-2531That also lets
KbSourceInfo(currently redefined per call inside the method) live at module scope.As per coding guidelines: "Maximum function length of 50 lines preferred for all functions".
🤖 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 `@backend/app/services/knowledge/knowledge_service.py` around lines 2062 - 2584, The get_all_knowledge_bases_grouped function is far too large and mixes loading, classification, aggregation and response building; split it into focused helper functions and move KbSourceInfo to module scope: extract a loader _load_direct_shares(db, user_id) that returns (personal_shared, shared_group_kbs, shared_kb_roles, shared_kb_inviter_map, remaining_shared_group_kbs), split _collect_entity_authorized_kbs into a namespace branch and an external-resolver branch (e.g. _classify_entity_kbs(...)), extract the KbSourceInfo aggregation and response construction into _build_shared_with_me(kb_sources, document_counts, owner_user_map, namespace_display_names, inviter_user_map, knowledge_share_service, merge_roles, kb_to_response) and extract the groups assembly into _build_groups_section(group_kbs, shared_into_group_kbs, entity_member_group_map, entity_member_role_map, entity_member_inviter_map, namespace_display_names, group_roles, grouped_namespace_names, merge_roles, kb_to_response), then have get_all_knowledge_bases_grouped orchestrate these helpers and call KnowledgeService._kb_to_response/_merge_roles as needed.
🤖 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.
Nitpick comments:
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 2062-2584: The get_all_knowledge_bases_grouped function is far too
large and mixes loading, classification, aggregation and response building;
split it into focused helper functions and move KbSourceInfo to module scope:
extract a loader _load_direct_shares(db, user_id) that returns (personal_shared,
shared_group_kbs, shared_kb_roles, shared_kb_inviter_map,
remaining_shared_group_kbs), split _collect_entity_authorized_kbs into a
namespace branch and an external-resolver branch (e.g.
_classify_entity_kbs(...)), extract the KbSourceInfo aggregation and response
construction into _build_shared_with_me(kb_sources, document_counts,
owner_user_map, namespace_display_names, inviter_user_map,
knowledge_share_service, merge_roles, kb_to_response) and extract the groups
assembly into _build_groups_section(group_kbs, shared_into_group_kbs,
entity_member_group_map, entity_member_role_map, entity_member_inviter_map,
namespace_display_names, group_roles, grouped_namespace_names, merge_roles,
kb_to_response), then have get_all_knowledge_bases_grouped orchestrate these
helpers and call KnowledgeService._kb_to_response/_merge_roles as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f9de05a-5ec7-40e7-a9d7-371c8ad3ae0f
📒 Files selected for processing (3)
backend/app/api/endpoints/adapter/attachments.pybackend/app/services/knowledge/knowledge_service.pybackend/tests/services/knowledge/test_namespace_knowledge_permissions.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/services/knowledge/test_namespace_knowledge_permissions.py
- backend/app/api/endpoints/adapter/attachments.py
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 12, 2026
- Add Pydantic model_validator to enforce entity_id for non-user entity members - Fix BaseRole comparison in can_manage_permissions (was using ResourceRole) - Fix _get_resource to use _resource_type_variants/_approved_status_variants - Fix virtual URL mode detection to use initialKbName instead of initialKbNamespace - Remove unreachable ShareLinkDialog and sharingKb state - Navigate away from deleted/migrated KB URL after clearSelection Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 12, 2026
- Fix _resolve_source_type to distinguish share_link from direct add - Fix E712 boolean comparison in group_service.py - Narrow cache.py exception handling to TypeError/ValueError - Fix test_namespace_entity_resolver to not rely on DB ordering - Fix knowledge_service: - Deduplicate entity_shared_to_me_kbs vs shared_into_group_kbs - Include remaining_shared_group_kbs in all_kb_ids, owner_user_ids, summary - Make aggregation deterministic by sorting collections Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 12, 2026
- Fix SelectTrigger height to meet 44px touch minimum (h-10 → h-11) - Fix useEffect exhaustive-deps in KnowledgeBaseAuthSection and AddNamespaceDialog - Parallelize namespace permission additions with Promise.allSettled - Replace unsafe type assertion in KnowledgeGroupListPage with type guard - Translate Chinese comments to English in PermissionManagementTab and KnowledgeDocumentPageDesktop - Add console.warn for extension loading failures Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 12, 2026
- Replace hardcoded "Namespace" string with NAMESPACE_RESOURCE_TYPE constant - Cache external entity resolver instances to avoid repeated instantiation Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx (1)
114-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClearing selection leaves the old URL behind.
This listener only clears sidebar state. If the user is on a KB path or a
?group=URL, refresh/back navigation will restore the stale selection because the route never changes. Clear the URL alongsidesidebar.clearSelection().🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx` around lines 114 - 122, The handler registered for the 'knowledge-clear-selection' event only calls clearSelection(), leaving the URL state (KB path or ?group=) intact so navigation can restore the stale selection; update handleClearSelection in KnowledgeDocumentPageDesktop to both call clearSelection() and programmatically update the router history (e.g., via navigate or history.replace) to remove the KB-specific path and any ?group= query param so the route reflects the cleared state. Locate handleClearSelection and the window.addEventListener('knowledge-clear-selection', ...) and ensure the URL update runs before/after clearSelection() (but in the same handler) to prevent the stale selection from reappearing on refresh/back.
♻️ Duplicate comments (1)
backend/app/core/cache.py (1)
126-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle Redis failures in
set_syncto preserve theFalsefallback contract.At Line 143, only serialization errors are caught.
SyncRedis.from_url(...)andclient.set(...)can raise Redis exceptions, which currently escape and can break callers expecting a boolean result.Suggested patch
from typing import Any, Dict, List, Optional import orjson from redis import Redis as SyncRedis +from redis.exceptions import RedisError from redis.asyncio import Redis @@ - except (TypeError, ValueError) as e: - logger.error(f"Error setting cache key {key} (sync): {str(e)}") + except (RedisError, TypeError, ValueError) as e: + logger.error(f"Error setting cache key {key} (sync): {e!s}") return False#!/bin/bash # Verify callsites rely on boolean return (not exception handling) and may be impacted. rg -nP --type=py -C3 '\bset_sync\s*\('🤖 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 `@backend/app/core/cache.py` around lines 126 - 145, The set_sync implementation currently only catches serialization errors and lets Redis connection/operation exceptions from SyncRedis.from_url or client.set propagate; update set_sync to catch redis-related exceptions (e.g., those raised by SyncRedis.from_url and client.set) around the client creation and set logic in the set_sync function and return False on failure, while still closing the client in the finally block; reference SyncRedis.from_url, client.set, and the set_sync function to find where to add a broader except (catching RedisError or the Redis client base exception) and log the error with context before returning False.
🧹 Nitpick comments (1)
frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx (1)
370-550: 🏗️ Heavy liftSplit
renderMainContentinto view-specific components/helpers.This function now mixes mode routing, data reshaping, and action wiring for KB detail, groups, dingtalk, all-KB, and selected-group views in one place. Extracting each branch into a focused component/helper would make the permission/share flows much easier to extend safely.
As per coding guidelines: "Maximum function length of 50 lines preferred for all functions".
🤖 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 `@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx` around lines 370 - 550, renderMainContent is doing mode routing, data mapping, and action wiring for multiple views (detail, groups, dingtalk, all, selectedGroup); extract each branch into its own small component or helper (e.g., KnowledgeDetailView, GroupsView, DingtalkView, AllView, SelectedGroupView) and move the data reshaping logic (teamGroupKbs mapping, groupKbsWithInfo/groupNativeKbs/groupSharedKbs) and action handlers (onEditKb/onDeleteKb/onCreateKb/onMigrateKb) into those components so renderMainContent only selects and renders the appropriate view component; keep existing props and callbacks (selectedKb, isSidebarCollapsed, updateSidebarCollapsed, dialogs.*, sidebar.* handlers, handleSelectKb, handleToggleFavorite, isFavorite, availableGroups, selectedGroup, etc.) when moving logic to preserve behavior and tests, and ensure each new component returns the same KnowledgeDetailPanel, KnowledgeGroupListPage, or DingtalkDocsPage usages with the already-computed props.
🤖 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 `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 2552-2556: The summary counters under-count because KBs added via
shared_into_group_kbs are appended to groups_map but not included in the
group_count and total_count calculations; update the counting logic that
currently only counts group_kbs (and the analogous block at lines ~2613-2625) to
also include shared_into_group_kbs entries—e.g., when you iterate target_groups
to append to groups_map, increment group_count and total_count (or add them into
the same set used for unique counts) for each unique kb.id so that
shared_into_group_kbs contributions are reflected in the summary.
- Around line 2209-2222: The code in _build_groups_section (and the similar
blocks at the other ranges) uses owner_user_map when constructing the
shared_from field for shared_into_group_kbs, but the inviter may not be an
owner; replace the lookup that uses owner_user_map with the inviter_user_map
(the map populated by _batch_fetch_kb_metadata) when setting shared_from for
shared_into_group_kbs (and any other place where shared_from is derived from
owner_user_map for shared KBs). Ensure you reference the inviter_user_map key
lookups instead of owner_user_map when handling
shared_into_group_kbs/shared_from in _build_groups_section and the equivalent
code paths at the other mentioned locations.
In `@frontend/src/features/knowledge/document/hooks/useKnowledgeBaseDialogs.ts`:
- Around line 121-137: The namespace fallback silently puts KBs in the wrong
scope; change the logic in useKnowledgeBaseDialogs (where namespace,
data.selectedGroupId, sidebar.groups, createScope and createGroupName are used)
to validate resolution and fail fast instead of defaulting: if
data.selectedGroupId is set but sidebar.groups.find(g => g.id ===
data.selectedGroupId) returns no match, throw or return an error (or surface a
user validation message) rather than using 'default'; similarly, when
createScope === 'organization' ensure sidebar.groups.find(g => g.type ===
'organization') exists and if not fail/surface an error instead of using
'organization'; only allow falling back to createGroupName when explicitly
provided and valid.
In
`@frontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx`:
- Around line 68-83: Duplicate useEffect hooks are causing doubled searches;
consolidate them by removing the redundant effect and keeping a single pair of
behaviors: when open becomes true reset the query (call setSearchQuery('') and
immediately call performSearch('')) and have one debounced effect that watches
[searchQuery, open] to call performSearch(searchQuery) after a 300ms timeout
(clearing the timeout on cleanup). Update the code around the existing useEffect
usages referencing performSearch, setSearchQuery, searchQuery and open so only
one effect handles the open/reset and one handles debounced searching (with
proper timeout cleanup) to prevent duplicate requests.
- Around line 164-173: The current Promise.allSettled call can leave a mix of
succeeded and failed group additions but keeps all groups selected, causing
retries to resubmit already-successful groups; update the logic in
add-namespace-dialog.tsx (around selectedGroups,
knowledgePermissionApi.addNamespacePermission, results and setError) to detect
which entries succeeded vs rejected, remove succeeded groups from selectedGroups
(or replace selectedGroups with only the failed group IDs) and surface which
groups succeeded/finally failed in the UI before returning so a retry only
re-attempts the failed ones; also ensure any success state is reflected (e.g.,
disable already-added groups) so retries are safe if addNamespacePermission is
not idempotent.
- Around line 310-318: The icon-only remove button is inaccessible and too
small; update the Button (the component rendering the XCircle) to provide an
accessible name (aria-label or visually hidden text like aria-label="Remove
group" or include screen-reader-only text) and make it touch-friendly by using
the repo’s sizing classes (replace "h-4 w-4" with "h-11 min-w-[44px]" or
equivalent and ensure className includes size/spacing like "p-0" adjusted if
necessary). Ensure the onClick still calls handleRemoveGroup(group.id) and keep
the XCircle icon for visual affordance.
---
Outside diff comments:
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Around line 114-122: The handler registered for the
'knowledge-clear-selection' event only calls clearSelection(), leaving the URL
state (KB path or ?group=) intact so navigation can restore the stale selection;
update handleClearSelection in KnowledgeDocumentPageDesktop to both call
clearSelection() and programmatically update the router history (e.g., via
navigate or history.replace) to remove the KB-specific path and any ?group=
query param so the route reflects the cleared state. Locate handleClearSelection
and the window.addEventListener('knowledge-clear-selection', ...) and ensure the
URL update runs before/after clearSelection() (but in the same handler) to
prevent the stale selection from reappearing on refresh/back.
---
Duplicate comments:
In `@backend/app/core/cache.py`:
- Around line 126-145: The set_sync implementation currently only catches
serialization errors and lets Redis connection/operation exceptions from
SyncRedis.from_url or client.set propagate; update set_sync to catch
redis-related exceptions (e.g., those raised by SyncRedis.from_url and
client.set) around the client creation and set logic in the set_sync function
and return False on failure, while still closing the client in the finally
block; reference SyncRedis.from_url, client.set, and the set_sync function to
find where to add a broader except (catching RedisError or the Redis client base
exception) and log the error with context before returning False.
---
Nitpick comments:
In
`@frontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsx`:
- Around line 370-550: renderMainContent is doing mode routing, data mapping,
and action wiring for multiple views (detail, groups, dingtalk, all,
selectedGroup); extract each branch into its own small component or helper
(e.g., KnowledgeDetailView, GroupsView, DingtalkView, AllView,
SelectedGroupView) and move the data reshaping logic (teamGroupKbs mapping,
groupKbsWithInfo/groupNativeKbs/groupSharedKbs) and action handlers
(onEditKb/onDeleteKb/onCreateKb/onMigrateKb) into those components so
renderMainContent only selects and renders the appropriate view component; keep
existing props and callbacks (selectedKb, isSidebarCollapsed,
updateSidebarCollapsed, dialogs.*, sidebar.* handlers, handleSelectKb,
handleToggleFavorite, isFavorite, availableGroups, selectedGroup, etc.) when
moving logic to preserve behavior and tests, and ensure each new component
returns the same KnowledgeDetailPanel, KnowledgeGroupListPage, or
DingtalkDocsPage usages with the already-computed props.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd252c41-96e6-413c-98d1-c4446a79f7eb
📒 Files selected for processing (16)
backend/app/core/cache.pybackend/app/schemas/share.pybackend/app/services/group_service.pybackend/app/services/knowledge/knowledge_service.pybackend/app/services/share/base_service.pybackend/app/services/share/external_entity_resolver.pybackend/app/services/share/knowledge_share_service.pybackend/app/services/share/namespace_entity_resolver.pybackend/tests/services/share/test_namespace_entity_resolver.pyfrontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsxfrontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsxfrontend/src/features/knowledge/document/hooks/useKnowledgeBaseDialogs.tsfrontend/src/features/knowledge/document/hooks/useKnowledgeUrlSync.tsfrontend/src/features/knowledge/permission/components/PermissionManagementTab.tsxfrontend/src/features/knowledge/permission/components/add-namespace-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/app/services/share/namespace_entity_resolver.py
- frontend/src/features/knowledge/document/components/KnowledgeBaseAuthSection.tsx
- backend/app/schemas/share.py
- frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx
- backend/app/services/share/external_entity_resolver.py
- backend/app/services/group_service.py
- backend/tests/services/share/test_namespace_entity_resolver.py
- frontend/src/features/knowledge/permission/components/PermissionManagementTab.tsx
- backend/app/services/share/knowledge_share_service.py
- backend/app/services/share/base_service.py
cocowh
added a commit
to cocowh/Wegent
that referenced
this pull request
May 12, 2026
Backend: - cache.py: Replace SyncRedis with asyncio.run() pattern (ref PR wecode-ai#1011) - knowledge_service.py: Fix shared_from to use inviter_user_map instead of owner_user_map for correct inviter display when KB is shared via external entity Frontend: - KnowledgeDocumentPageDesktop.tsx: Add URL navigation on clear selection event - useKnowledgeBaseDialogs.ts: Add namespace validation with explicit error handling - add-namespace-dialog.tsx: - Remove duplicate useEffect hooks - Improve Promise.allSettled error handling to track failed groups for retry - Fix button accessibility with aria-label and touch-friendly sizing Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cocowh
added a commit
to cocowh/Wegent
that referenced
this pull request
May 12, 2026
Fix regression from dad8d0e where _build_groups_section was using owner_user_map instead of inviter_user_map to look up inviter names. This caused shared_from to be None (showing "--") for KBs shared into groups. Changes: - Add inviter_user_map parameter to _build_groups_section - Use inviter_user_map.get(inviter_id) instead of owner_user_map - Update caller to pass inviter_user_map Refs: PR wecode-ai#1092
cocowh
force-pushed
the
wegent/kb-entity-permission
branch
5 times, most recently
from
May 12, 2026 09:48
f013cf9 to
78f2245
Compare
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 13, 2026
- Validate unknown entity_type (non-user, non-namespace) returns 400 - Preserve user-provided entity_display_name for reliably resolvable types - Remove duplicate inline resource_type_variants logic (use property) - Sanitize logs: remove share_token partial output from create_share_link - Optimize N+1 queries in _source_entity_permission via batch_get_display_names - Add tests for unknown entity rejection, permission downgrade, batch dedup Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cocowh
force-pushed
the
wegent/kb-entity-permission
branch
from
May 13, 2026 07:06
b22caa3 to
685c537
Compare
Collaborator
|
LGTM |
…sions - Add entity-based permission system via ResourceMember with entity_type + entity_id - Implement knowledge share service with batch_add_members, add_member, remove_member, update_member_role, and list_members - Refactor _add_initial_kb_members to use batch_add_members for single-transaction safety - Relax InitialMemberCreate.entity_type from Literal[\"user\", \"namespace\"] to str to support custom entity types (e.g., org_department) via registerAuthSection - Fix summary counts: deduplicate total_count and include shared_into_group_kbs in group_count - Frontend: use backend summary data for sidebar counts (total, personal, group, organization) - Frontend: hide \"from\" column in all/groups list views, show only in specific group view - Fix hidden invalid character (U+00B7) in PermissionManagementTab.tsx import - Add comprehensive tests for batch_add_members and initial KB member creation Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cocowh
force-pushed
the
wegent/kb-entity-permission
branch
from
May 13, 2026 16:51
f423c4b to
31763c0
Compare
qdaxb
approved these changes
May 14, 2026
This was referenced May 14, 2026
This was referenced Jun 12, 2026
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
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.
…sions
Summary by CodeRabbit
New Features
Enhancements
Documentation