Management panel visibility (TS-2914) #1667
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change updates the package management page to expand moderator access and refactor its data loading pipeline. The Manage Package UI block now displays for users with either manager or moderator permissions. The package edit page's clientLoader function is refactored to fetch listing data via Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
- Show "Manage Package" button on package page - Allow access to package "edit" page
- team was unused - communityFilters was a duplicate for filters and unused - community was used only for identifier, which is available via URL params and listing object
Sentry logging isn't needed in UI component, it's handled by entry.server.tsx on server and RouteErrorBoundary on client.
anttimaki
force-pushed
the
management-panel-visibility
branch
from
4ce0896 to
dc065a2
Compare
anttimaki
changed the base branch from
moderation-tools-feature-branch
to
version-dependency-merge
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/app/p/packageEdit.tsx (1)
82-93: Rules of Hooks violation: hooks called after conditional return.Hooks (
useOutletContext,useToast,useRevalidator) are called after the early return on line 86-87. This violates React's Rules of Hooks—hooks must be called unconditionally and in the same order on every render.Move all hooks above the early return:
Proposed fix
export default function PackageListing() { const loaderData = useLoaderData<typeof loader | typeof clientLoader>(); + const outletContext = useOutletContext() as OutletContextShape; + const config = outletContext.requestConfig; + const toast = useToast(); + const revalidator = useRevalidator(); if (!loaderData) { return <SkeletonBox className="package-edit__skeleton" />; } const { listing, filters, permissions } = loaderData; - const outletContext = useOutletContext() as OutletContextShape; - const config = outletContext.requestConfig; - const toast = useToast(); - const revalidator = useRevalidator();
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/p/packageEdit.tsx (1)
41-45: Module-level Response object.The
package404Response is created once at module load time and reused. While this works in React Router (the thrown Response is used for status/headers), consider creating it inline within the throw statements for clarity and to avoid potential issues if the Response body is ever consumed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cyberstorm-remix/app/p/components/PackageListing/ManagementTools.tsx(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.css(1 hunks)apps/cyberstorm-remix/app/p/packageEdit.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
apps/cyberstorm-remix/app/p/components/PackageListing/ManagementTools.tsx (1)
84-100: LGTM!The expanded permission check aligns with the authorization logic in
packageEdit.tsxclientLoader. Moderators can now access the management panel as intended.apps/cyberstorm-remix/app/p/packageEdit.css (1)
88-91: LGTM!Clean skeleton style addition for the loading state.
apps/cyberstorm-remix/app/p/packageEdit.tsx (2)
204-244: LGTM!Permission checks for individual actions (
can_unlist,can_manage_deprecation) are appropriately granular, separate from page access permissions.
54-77: Incorrect error handling approach:ApiErrordoesn't have astatusproperty.The suggested fix references
error.status, butApiErrorhas aresponseproperty instead. Checkerror.response.statusto access the HTTP status code.That said, the underlying concern has merit: if
getPrivateListingor other API calls fail with 401/403, they're masked as 404. The explicit auth checks at lines 65–72 work correctly since they throwResponseobjects directly, but auth errors from the API calls would lose their status codes.If preserving auth error responses is important, use
error.response.statusto distinguish:} catch (error) { if (error instanceof ApiError) { + if (error.response.status === 401 || error.response.status === 403) { + throw new Response(error.message, { status: error.response.status }); + } throw package404; } throw error; }Note: The current behavior might be intentional for security reasons.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## version-dependency-merge #1667 +/- ##
============================================================
+ Coverage 11.73% 11.76% +0.03%
============================================================
Files 320 320
Lines 22796 22754 -42
Branches 509 510 +1
============================================================
+ Hits 2676 2678 +2
+ Misses 20120 20076 -44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The page requires authentication and authorization, and the SSR rendering half a form that's shown to user while the client side checks for permissions is just bad UX. Instead, return undefined so the component renders immediately and show a skeleton component to user.
Attempt to fetch package listing data first as unauthenticated user in hopes of hitting cache. If this results in 404, retry as authenticated user in hopes of having permissions to view a rejected package listing. Since we block rendering be awaiting permissions and listing, we might as well trigger filter loading before we know whether user can access the page or not. A little bit on overfetch is preferred to awaiting in phases.
anttimaki
force-pushed
the
management-panel-visibility
branch
from
dc065a2 to
6e5f19e
Compare
Roffenlund
left a comment
Roffenlund
left a comment
There was a problem hiding this comment.
Approved. Several changes are similar to what has been merged in #1670.
No description provided.