fix(api): prevent crash from null member_ids on workspace modules endpoint (was mislabeled as feat) - #9541
Conversation
Added an annotation to the WorkspaceModulesEndpoint to aggregate member IDs into an array, ensuring that only active members are included. This change improves the data structure returned by the API, allowing for better handling of member information in the frontend. Updated the corresponding utility function to handle potential null values for member IDs.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
📝 WalkthroughWalkthroughModules now include distinct active member IDs from the API. Utility functions use non-mutating sorting and default missing member IDs to an empty array. ChangesModule membership handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/utils/src/module.ts (1)
33-34: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant array copy before
toSorted.
toSortedalready returns a new array without mutatingmodules. The[...modules]spread before it creates an unneeded extra copy.♻️ Proposed simplification
- if (orderByKey === "name") orderedModules = [...modules].toSorted((a, b) => naturalSort(a.name, b.name)); - if (orderByKey === "-name") orderedModules = [...modules].toSorted((a, b) => naturalSort(b.name, a.name)); + if (orderByKey === "name") orderedModules = modules.toSorted((a, b) => naturalSort(a.name, b.name)); + if (orderByKey === "-name") orderedModules = modules.toSorted((a, b) => naturalSort(b.name, a.name));🤖 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 `@packages/utils/src/module.ts` around lines 33 - 34, Remove the redundant spread copies in the ascending and descending name-sorting branches, calling toSorted directly on modules while preserving the existing naturalSort comparators and ordering behavior.apps/api/plane/app/views/workspace/module.py (1)
115-127: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse a single through-model path for
member_ids.
members__idandmodulemember__deleted_attarget the same through table indirectly. Usemodulemember__member_id__isnull=False, modulemember__deleted_at__isnull=Trueand aggregatemodulemember__member_idso the active membership condition applies to the aggregated member id and join reuse remains clear.🤖 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 `@apps/api/plane/app/views/workspace/module.py` around lines 115 - 127, Update the `member_ids` annotation to use the single `modulemember` through-model path: filter with `modulemember__member_id__isnull=False` and `modulemember__deleted_at__isnull=True`, and aggregate `modulemember__member_id` instead of `members__id`.
🤖 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 `@apps/api/plane/app/views/workspace/module.py`:
- Around line 115-127: Update the `member_ids` annotation to use the single
`modulemember` through-model path: filter with
`modulemember__member_id__isnull=False` and
`modulemember__deleted_at__isnull=True`, and aggregate `modulemember__member_id`
instead of `members__id`.
In `@packages/utils/src/module.ts`:
- Around line 33-34: Remove the redundant spread copies in the ascending and
descending name-sorting branches, calling toSorted directly on modules while
preserving the existing naturalSort comparators and ordering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ec18e1f-6435-4fbe-927c-c8a6eaf6d644
📒 Files selected for processing (2)
apps/api/plane/app/views/workspace/module.pypackages/utils/src/module.ts
Description
WorkspaceModulesEndpoint(GET /api/workspaces/{slug}/modules/) never annotatedmember_idson its queryset, unlike the project-scoped modules endpoint which does. Since the serializer field allows null, every module returned by this endpoint serializedmember_idsasnullinstead of[].That
nullgot written into the shared frontend module cache (viafetchWorkspaceModules/fetchModulesSlim, triggered on workspace-wide pages like "Your Work"). Once poisoned, opening a project's Modules view with an active "Members" filter (which persists inlocalStorage, so it recurs on every visit) crashed the whole page with:Fix:
apps/api/plane/app/views/workspace/module.py: annotatemember_idswithCoalesce(ArrayAgg(...), Value([])), matching the project-scoped endpoint, so it always returns[]instead ofnull.packages/utils/src/module.ts: defensive null-guard inshouldFilterModule(module.member_ids ?? []) so a null value can no longer crash the filter regardless of where it originates.Type of Change
Screenshots and Media (if applicable)
Before:
Before.mov
After:
After.mov
Test Scenarios
Reproduced live end-to-end in a local dev instance:
localStorage).fetchWorkspaceModules, poisoning the shared module cache withmember_ids: null.member_ids: []and the page no longer crashes.References
Reported by multiple users in Slack (CE support thread) — no linked work item.