Design and implement new Security config page and API tab updates#3591
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds token security editing and reinstatement flows, origin-enforcement for clients, token scheduling/restrictions, a System Security admin dashboard (blocked ASN/CIDR and flagged tokens), new service endpoints and types, validators, and UI wiring across client list, client details, and admin pages. ChangesToken Security & System Admin
Sequence Diagram(s)sequenceDiagram
participant AdminUser
participant Dashboard as System Security<br/>Dashboard
participant SWR
participant AdminService
participant API
participant Toast
AdminUser->>Dashboard: view blocked ASNs / flagged tokens
Dashboard->>SWR: fetch paginated datasets
SWR->>AdminService: getBlockedASNs(), getFlaggedTokens()
AdminService->>API: authenticated requests
API-->>AdminService: BlockedAsn[] + FlaggedToken[]
AdminService-->>SWR: data
SWR-->>Dashboard: hydrate tables
AdminUser->>Dashboard: open create/edit ASN dialog
Dashboard->>Dashboard: validate CIDR and ASN format
Dashboard->>AdminService: createBlockedASN(payload)
AdminService->>API: POST blocked-asn
API-->>AdminService: success
AdminService-->>Dashboard: CreateBlockedAsnResponse
Dashboard->>Toast: success notification
Dashboard->>SWR: mutate() refresh
AdminUser->>Dashboard: open resolve flagged token dialog
Dashboard->>AdminService: resolveFlaggedToken(id, note)
AdminService->>API: PUT /flagged-tokens/{id}
API-->>AdminService: success
Dashboard->>SWR: mutate() refresh
sequenceDiagram
participant ClientMgr as API Client Mgr
participant TokenDialog as TokenSecurityDialog
participant ClientService
participant API
participant Toast
ClientMgr->>ClientMgr: select client with suspended token
ClientMgr->>TokenDialog: open(token, clientName)
TokenDialog->>TokenDialog: init state from token props
ClientMgr->>TokenDialog: reinstate button / save changes
TokenDialog->>TokenDialog: validate entries
alt Reinstate
TokenDialog->>ClientService: reinstateToken(token)
ClientService->>API: PATCH /users/tokens/{token} (auto_suspended=false)
else Save Changes
TokenDialog->>ClientService: updateTokenSecurity(token, payload)
ClientService->>API: PATCH /users/tokens/{token} (with restrictions+schedule)
end
API-->>ClientService: success/error
ClientService-->>TokenDialog: response
TokenDialog->>Toast: notify outcome
TokenDialog->>ClientMgr: onSuccess callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Contributor
|
New azure analytics_platform changes available for preview here |
![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: 10
🧹 Nitpick comments (7)
src/platform/src/modules/api-client/ApiClientPage.tsx (1)
333-403: ⚡ Quick winRemove the dead fallback branch below this return.
After the new
returnin this renderer, the older regenerate/edit branch at Lines 378-403 is unreachable. Leaving both versions here makes future action-button changes easy to apply to the wrong path.🤖 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 `@src/platform/src/modules/api-client/ApiClientPage.tsx` around lines 333 - 403, The renderer contains a dead fallback branch after an earlier return — remove the unreachable JSX block starting with the second "if (token && expired && item.isActive)" and the final fallback Button so only the active return remains; update the component to keep the first returned fragment (which uses handleGenerateToken, handleOpenTokenSecurity, handleEditClient and the token/expired/item.isActive checks) and delete the old alternate return path to avoid duplicated/unreachable action-button code.src/platform/src/shared/types/api.ts (1)
1098-1104: ⚡ Quick winEnforce “ASN or CIDR” at the type level for create requests.
CreateBlockedAsnRequestcurrently allows bothasnandcidr_rangesto be omitted, so invalid create payloads compile and fail only at runtime.Proposed type-safe contract
-export interface CreateBlockedAsnRequest { - provider: string; - asn?: string; - cidr_ranges?: string[]; - reason?: string; - active?: boolean; -} +type CreateBlockedAsnBase = { + provider: string; + reason?: string; + active?: boolean; +}; + +export type CreateBlockedAsnRequest = + | (CreateBlockedAsnBase & { asn: string; cidr_ranges?: string[] }) + | (CreateBlockedAsnBase & { asn?: string; cidr_ranges: [string, ...string[]] });🤖 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 `@src/platform/src/shared/types/api.ts` around lines 1098 - 1104, Create a type-level guarantee that at least one of asn or cidr_ranges is present by changing CreateBlockedAsnRequest from a single optional field shape to a union that enforces the presence of one (or both) fields; specifically, keep shared fields (provider, reason, active) and combine them with a union such as: one variant requiring asn (and forbidding cidr_ranges if you want mutual exclusivity) and another variant requiring cidr_ranges (and forbidding asn), or allow both by adding a third variant—update the CreateBlockedAsnRequest declaration accordingly and adjust any callers that rely on the old shape to satisfy the new type.src/platform/src/modules/api-client/components/EditClientDialog.tsx (3)
230-234: 💤 Low valueRedundant error clearing in else clause.
This block clears all origin errors when
enforceOriginis false, but it's unnecessary:
- Errors are already cleared as users type (line 180)
- When enforcement is disabled, the origin inputs aren't rendered (line 380), so errors don't matter
The block can be safely removed to simplify the code.
♻️ Simplification
} } - } else { - newOriginErrors.forEach((_, index) => { - newOriginErrors[index] = ''; - }); } setOriginErrors(newOriginErrors);🤖 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 `@src/platform/src/modules/api-client/components/EditClientDialog.tsx` around lines 230 - 234, In EditClientDialog, remove the redundant else branch that iterates over newOriginErrors to clear them when enforceOrigin is false; the component already clears origin errors on input change (the handler around line ~180) and origin inputs are not rendered when enforceOrigin is false, so delete the else block that sets newOriginErrors[index] = '' to simplify logic and avoid unnecessary state updates.
197-197: 💤 Low valueInconsistent filtering style between create and edit dialogs.
Line 197 uses
origin.trim()(truthy check), while CreateClientDialog line 92 usesorigin.trim() !== ''(explicit check). Both are functionally equivalent, but using the same pattern in both files improves maintainability.🤖 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 `@src/platform/src/modules/api-client/components/EditClientDialog.tsx` at line 197, The filteredOrigins line in EditClientDialog uses a truthy check (origin.trim()) which is inconsistent with CreateClientDialog; update the filter to use an explicit comparison origin.trim() !== '' so the filtering in the filteredOrigins computation matches the pattern used in CreateClientDialog and improves maintainability (locate and change the filteredOrigins declaration inside the EditClientDialog component where originAddresses is filtered).
245-249: ⚡ Quick winInconsistent handling of optional payload fields.
Same issue as CreateClientDialog:
allowed_originsis always included (sent as[]whenenforceOriginis false), whileip_addressesuses conditional spreading. Consider the same fix:const clientData = { name: clientName.trim(), require_secret: requireSecret, - enforce_origin: enforceOrigin, - allowed_origins: enforceOrigin ? filteredOrigins : [], ...(filteredIpAddresses.length > 0 && { ip_addresses: filteredIpAddresses, }), + ...(enforceOrigin && { + enforce_origin: true, + allowed_origins: filteredOrigins, + }), };🤖 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 `@src/platform/src/modules/api-client/components/EditClientDialog.tsx` around lines 245 - 249, The payload always includes allowed_origins as [] when enforceOrigin is false; mirror the conditional pattern used for ip_addresses so allowed_origins is only sent when enforceOrigin is true. In EditClientDialog update the object construction that currently sets enforce_origin: enforceOrigin and allowed_origins: enforceOrigin ? filteredOrigins : [] so it instead spreads ...(enforceOrigin && { allowed_origins: filteredOrigins }) alongside the existing ...(filteredIpAddresses.length > 0 && { ip_addresses: filteredIpAddresses }), keeping enforce_origin: enforceOrigin. This removes the empty array payload when origins are not enforced.src/platform/src/modules/api-client/components/CreateClientDialog.tsx (2)
61-81: 💤 Low valueDuplicated handler logic.
The origin handlers mirror the IP address handlers (lines 38-59) almost exactly. Consider extracting a generic list editor hook (e.g.,
useListEditor) to reduce duplication and simplify maintenance.// Example generic hook function useListEditor(initial = ['']) { const [items, setItems] = useState<string[]>(initial); const [errors, setErrors] = useState<string[]>(initial.map(() => '')); const add = () => { setItems([...items, '']); setErrors([...errors, '']); }; const remove = (index: number) => { if (items.length > 1) { setItems(items.filter((_, i) => i !== index)); setErrors(errors.filter((_, i) => i !== index)); } }; const change = (index: number, value: string) => { const newItems = [...items]; newItems[index] = value; setItems(newItems); const newErrors = [...errors]; newErrors[index] = ''; setErrors(newErrors); }; return { items, errors, add, remove, change, setItems, setErrors }; }🤖 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 `@src/platform/src/modules/api-client/components/CreateClientDialog.tsx` around lines 61 - 81, The origin address handlers (handleAddOriginAddress, handleRemoveOriginAddress, handleOriginAddressChange) duplicate logic from the IP address handlers; extract and replace this repeated behavior with a generic hook (e.g., useListEditor) that manages items and errors and exposes add, remove, change, setItems, setErrors; update CreateClientDialog to call useListEditor for both IPs and origin addresses and wire the returned items/errors and methods in place of the three origin handler functions to remove duplication and centralize list editing logic.
136-140: ⚡ Quick winInconsistent handling of optional payload fields.
The
allowed_originsfield is always included in the payload (sent as[]whenenforceOriginis false), whileip_addressesuses conditional spreading to omit the field when empty. Consider aligning the pattern for consistency:const clientData = { name: clientName.trim(), ...(userId && { user_id: userId }), - enforce_origin: enforceOrigin, - allowed_origins: enforceOrigin ? filteredOrigins : [], ...(filteredIpAddresses.length > 0 && { ip_addresses: filteredIpAddresses, }), + ...(enforceOrigin && { + enforce_origin: true, + allowed_origins: filteredOrigins, + }), };This makes the payload semantics clearer: origin enforcement fields are only present when actively configured.
🤖 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 `@src/platform/src/modules/api-client/components/CreateClientDialog.tsx` around lines 136 - 140, The payload mixes always-present and conditionally-omitted fields; update CreateClientDialog so allowed_origins follows the same conditional-spread pattern as ip_addresses: keep enforce_origin present, but only spread allowed_origins when enforceOrigin is true (and optionally when filteredOrigins.length > 0) using the same ...(condition && { allowed_origins: filteredOrigins }) pattern so the field is omitted when not actively configured; similarly ensure ip_addresses still uses ...(filteredIpAddresses.length > 0 && { ip_addresses: filteredIpAddresses }).
🤖 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 `@src/platform/src/app/`(dashboard)/system/clients/[clientId]/page.tsx:
- Around line 216-219: The catch block currently logs the full error (which can
contain tokens); replace that raw console.error call in the reinstate-token
handler with a sanitized log that only outputs non-sensitive fields such as a
user-facing message or a redacted error code. Specifically, in the catch near
getUserFriendlyErrorMessage(error) remove or change console.error('Reinstate
token error:', error) to log only safe data (for example the result of
getUserFriendlyErrorMessage(error) and/or a redacted error.code) so no full
error payload or `/users/tokens/<token>` transport metadata is written to the
browser console.
- Around line 750-767: The JSX directly dereferences
token.access_schedule.allowed_hours_utc.start/end which can be undefined; update
the rendering in the page component (the block that shows Hours UTC) to first
guard that token.access_schedule.allowed_hours_utc exists and has start/end
before accessing them (e.g., check token.access_schedule?.allowed_hours_utc and
the presence of start/end), and render a safe fallback like "All day" or "No
specific hours" when those fields are absent; keep the call to
describeAllowedDays(token.access_schedule.allowed_days) as-is and only change
the Hours UTC paragraph in the component to use this guarded access.
In `@src/platform/src/app/`(dashboard)/system/security/page.tsx:
- Around line 604-606: The code assumes item.cidr_ranges exists; guard against
undefined by defaulting to an empty array before slicing/reading length. For
example, create a local const cidrRanges = item.cidr_ranges ?? [] (or use
optional chaining (item.cidr_ranges || []) ), then compute const visible =
cidrRanges.slice(0, 2) and const more = Math.max(0, cidrRanges.length -
visible.length); update references to use cidrRanges so ASN-only entries with no
CIDR list don't throw.
- Around line 264-267: The catch block currently logs the raw error object which
may leak sensitive session/auth data; change the console.error call in the
Blocked ASN save handler (the catch in page.tsx around the "Blocked ASN save
error" message) to log only a sanitized message and minimal metadata (e.g., use
getUserFriendlyErrorMessage(error) and/or error?.message and non-sensitive
fields) instead of the full error object, and remove any stack or full object
serialization; ensure toast.error remains but replace console.error('Blocked ASN
save error:', error) with a sanitized log that omits tokens/session info.
In `@src/platform/src/modules/api-client/ApiClientPage.tsx`:
- Around line 153-156: The catch block in ApiClientPage currently logs the full
error from clientService.reinstateToken(), which may contain sensitive token
data; replace the console.error call with a sanitized log that only records the
friendly message and non-sensitive metadata. Specifically, stop dumping the raw
error from clientService.reinstateToken() and instead log
getUserFriendlyErrorMessage(error) and/or a small sanitized object (e.g., {
message: ..., status: ..., requestId: ... }) ensuring any fields named token,
authorization, auth, or headers are omitted or redacted before logging. Update
the catch in the method handling the reinstate flow in ApiClientPage to use that
sanitized log approach and keep toast.error(getUserFriendlyErrorMessage(error))
as-is.
In `@src/platform/src/modules/api-client/components/TokenSecurityDialog.tsx`:
- Around line 155-157: The catch blocks in TokenSecurityDialog.tsx currently log
the raw transport error (which may include sensitive tokens in
error.config.url); create and call a sanitizeErrorForLogging helper (or inline
sanitize logic) that extracts only non-sensitive fields (e.g., message, status,
method) and redacts or omits error.config and error.request.url before logging;
replace console.error('Reinstate token error:', error) and the similar log at
the second catch (lines ~210-212) with console.error('Reinstate token error:',
sanitizeErrorForLogging(error)) or console.error('Delete token error:',
sanitizeErrorForLogging(error)) so only the redacted shape or friendly message
is logged.
In `@src/platform/src/shared/lib/validators.ts`:
- Around line 188-195: isValidOriginUrl is currently allowing full URLs with
paths, queries, fragments, or credentials; tighten it by parsing the value with
URL and then enforcing origin-only semantics: require parsed.protocol to be
'http:' or 'https:', parsed.username and parsed.password to be empty,
parsed.search and parsed.hash to be empty, and parsed.pathname to be either '/'
(no path) and parsed.hostname to be present (port allowed). Update the function
(isValidOriginUrl) to return true only when all these origin-only checks pass,
otherwise return false.
- Around line 183-186: The isValidAsn validator currently accepts any
up-to-10-digit number after "AS" (e.g., AS9999999999); update isValidAsn to
first match /^AS(\d{1,10})$/ against trimmed.toUpperCase(), extract the captured
numeric group, parse it as an integer and then enforce it falls within the valid
32-bit ASN range (1 through 4294967295) before returning true; keep the initial
trim/uppercasing and return false for non-matches or out-of-range values.
In `@src/platform/src/shared/services/adminService.ts`:
- Around line 206-268: The new security endpoint methods (createBlockedASN,
deleteBlockedASN, getFlaggedTokens, resolveFlaggedToken) currently return
response.data without handling logical failures; after each authenticatedClient
call, inspect response.data and if response.data?.success === false throw an
Error (include response.data?.error or response.data?.message or a clear
fallback like "operation failed" in the message) so callers don't treat a
200/false payload as success; otherwise return response.data as before. Ensure
the guard is applied in each of the listed methods immediately after receiving
the response.
In `@src/platform/src/shared/services/clientService.ts`:
- Around line 108-116: The updateTokenSecurity method is placing the raw access
token in the URL path (see updateTokenSecurity and the authenticatedClient.patch
call with `/users/tokens/${encodeURIComponent(token)}`) which risks leaking
credentials; instead, remove the token from the URL and send it via a secure
channel such as the Authorization header or the request body (or use a
non-secret token identifier/ID if the API supports it). Update the call to
authenticatedClient.patch to target a non-sensitive route (e.g., `/users/tokens`
or `/users/tokens/security`) and include the token in payload or set
Authorization: Bearer <token> on the request; ensure any logging/sanitization
avoids printing the raw token and, if the API requires server-side changes,
coordinate to accept token in header/body rather than path.
---
Nitpick comments:
In `@src/platform/src/modules/api-client/ApiClientPage.tsx`:
- Around line 333-403: The renderer contains a dead fallback branch after an
earlier return — remove the unreachable JSX block starting with the second "if
(token && expired && item.isActive)" and the final fallback Button so only the
active return remains; update the component to keep the first returned fragment
(which uses handleGenerateToken, handleOpenTokenSecurity, handleEditClient and
the token/expired/item.isActive checks) and delete the old alternate return path
to avoid duplicated/unreachable action-button code.
In `@src/platform/src/modules/api-client/components/CreateClientDialog.tsx`:
- Around line 61-81: The origin address handlers (handleAddOriginAddress,
handleRemoveOriginAddress, handleOriginAddressChange) duplicate logic from the
IP address handlers; extract and replace this repeated behavior with a generic
hook (e.g., useListEditor) that manages items and errors and exposes add,
remove, change, setItems, setErrors; update CreateClientDialog to call
useListEditor for both IPs and origin addresses and wire the returned
items/errors and methods in place of the three origin handler functions to
remove duplication and centralize list editing logic.
- Around line 136-140: The payload mixes always-present and
conditionally-omitted fields; update CreateClientDialog so allowed_origins
follows the same conditional-spread pattern as ip_addresses: keep enforce_origin
present, but only spread allowed_origins when enforceOrigin is true (and
optionally when filteredOrigins.length > 0) using the same ...(condition && {
allowed_origins: filteredOrigins }) pattern so the field is omitted when not
actively configured; similarly ensure ip_addresses still uses
...(filteredIpAddresses.length > 0 && { ip_addresses: filteredIpAddresses }).
In `@src/platform/src/modules/api-client/components/EditClientDialog.tsx`:
- Around line 230-234: In EditClientDialog, remove the redundant else branch
that iterates over newOriginErrors to clear them when enforceOrigin is false;
the component already clears origin errors on input change (the handler around
line ~180) and origin inputs are not rendered when enforceOrigin is false, so
delete the else block that sets newOriginErrors[index] = '' to simplify logic
and avoid unnecessary state updates.
- Line 197: The filteredOrigins line in EditClientDialog uses a truthy check
(origin.trim()) which is inconsistent with CreateClientDialog; update the filter
to use an explicit comparison origin.trim() !== '' so the filtering in the
filteredOrigins computation matches the pattern used in CreateClientDialog and
improves maintainability (locate and change the filteredOrigins declaration
inside the EditClientDialog component where originAddresses is filtered).
- Around line 245-249: The payload always includes allowed_origins as [] when
enforceOrigin is false; mirror the conditional pattern used for ip_addresses so
allowed_origins is only sent when enforceOrigin is true. In EditClientDialog
update the object construction that currently sets enforce_origin: enforceOrigin
and allowed_origins: enforceOrigin ? filteredOrigins : [] so it instead spreads
...(enforceOrigin && { allowed_origins: filteredOrigins }) alongside the
existing ...(filteredIpAddresses.length > 0 && { ip_addresses:
filteredIpAddresses }), keeping enforce_origin: enforceOrigin. This removes the
empty array payload when origins are not enforced.
In `@src/platform/src/shared/types/api.ts`:
- Around line 1098-1104: Create a type-level guarantee that at least one of asn
or cidr_ranges is present by changing CreateBlockedAsnRequest from a single
optional field shape to a union that enforces the presence of one (or both)
fields; specifically, keep shared fields (provider, reason, active) and combine
them with a union such as: one variant requiring asn (and forbidding cidr_ranges
if you want mutual exclusivity) and another variant requiring cidr_ranges (and
forbidding asn), or allow both by adding a third variant—update the
CreateBlockedAsnRequest declaration accordingly and adjust any callers that rely
on the old shape to satisfy the new type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c99cb06e-ac96-43b3-8cd0-45d54201dcc6
📒 Files selected for processing (17)
src/platform/src/app/(dashboard)/system/clients/[clientId]/page.tsxsrc/platform/src/app/(dashboard)/system/security/page.tsxsrc/platform/src/modules/api-client/ApiClientPage.tsxsrc/platform/src/modules/api-client/components/CreateClientDialog.tsxsrc/platform/src/modules/api-client/components/EditClientDialog.tsxsrc/platform/src/modules/api-client/components/TokenDisplay.tsxsrc/platform/src/modules/api-client/components/TokenSecurityDialog.tsxsrc/platform/src/modules/api-client/components/index.tssrc/platform/src/shared/components/header/config/pageTitles.tssrc/platform/src/shared/components/sidebar/components/SidebarContent.tsxsrc/platform/src/shared/components/sidebar/config/index.tssrc/platform/src/shared/components/ui/dialog.tsxsrc/platform/src/shared/lib/metadata.tssrc/platform/src/shared/lib/validators.tssrc/platform/src/shared/services/adminService.tssrc/platform/src/shared/services/clientService.tssrc/platform/src/shared/types/api.ts
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/src/modules/api-client/ApiClientPage.tsx (1)
337-407:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnreachable code after early return.
Lines 382-406 are dead code. The function returns at line 380, so the subsequent
if (token && expired && item.isActive)block and final return statement can never execute. This looks like leftover code from before the refactor to the stacked layout.</div> ); - - if (token && expired && item.isActive) { - return ( - <Tooltip content="A new token will be generated — copy it when shown to use it"> - <Button - size="sm" - variant="outlined" - onClick={() => handleGenerateToken(item, 'refresh')} - disabled={isGeneratingForThis} - > - {isGeneratingForThis ? 'Regenerating...' : 'Regenerate Token'} - </Button> - </Tooltip> - ); - } - - return ( - <Button - size="sm" - variant="ghost" - onClick={() => handleEditClient(item)} - className="p-1 h-6 w-6" - > - <AqEdit05 className="w-4 h-4" /> - </Button> - ); },🤖 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 `@src/platform/src/modules/api-client/ApiClientPage.tsx` around lines 337 - 407, The JSX after the early return in ApiClientPage.tsx is dead and should be removed: delete the unreachable legacy blocks that start with the duplicated "if (token && expired && item.isActive)" and the final standalone return so only the stacked layout return remains; keep the current stacked layout JSX that uses handleGenerateToken, handleOpenTokenSecurity, handleEditClient and the token/expired/item.isActive/isGeneratingForThis logic (including the Manage Security button and Edit button with its aria-label) and remove the leftover duplicate Tooltip/Button and small ghost Button blocks.
🧹 Nitpick comments (1)
src/platform/src/shared/utils/sanitizeErrorForLogging.ts (1)
1-8: 💤 Low valueConsider exporting the
SanitizedErrorLogtype.The type is used as the return type but isn't exported. If callers want to type variables holding the result, they'd need access to this type.
-type SanitizedErrorLog = { +export type SanitizedErrorLog = { code?: string | number; message?: string; method?: string; name?: string; status?: number; statusText?: string; };🤖 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 `@src/platform/src/shared/utils/sanitizeErrorForLogging.ts` around lines 1 - 8, The type alias SanitizedErrorLog is declared but not exported, which prevents callers from importing it to type variables that receive sanitizeErrorForLogging's result; export the type by adding the export keyword to the SanitizedErrorLog declaration (export type SanitizedErrorLog = { ... }) so callers can import and use the type alongside the sanitizeErrorForLogging function.
🤖 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.
Outside diff comments:
In `@src/platform/src/modules/api-client/ApiClientPage.tsx`:
- Around line 337-407: The JSX after the early return in ApiClientPage.tsx is
dead and should be removed: delete the unreachable legacy blocks that start with
the duplicated "if (token && expired && item.isActive)" and the final standalone
return so only the stacked layout return remains; keep the current stacked
layout JSX that uses handleGenerateToken, handleOpenTokenSecurity,
handleEditClient and the token/expired/item.isActive/isGeneratingForThis logic
(including the Manage Security button and Edit button with its aria-label) and
remove the leftover duplicate Tooltip/Button and small ghost Button blocks.
---
Nitpick comments:
In `@src/platform/src/shared/utils/sanitizeErrorForLogging.ts`:
- Around line 1-8: The type alias SanitizedErrorLog is declared but not
exported, which prevents callers from importing it to type variables that
receive sanitizeErrorForLogging's result; export the type by adding the export
keyword to the SanitizedErrorLog declaration (export type SanitizedErrorLog = {
... }) so callers can import and use the type alongside the
sanitizeErrorForLogging function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74bd3767-bcb1-4152-a7c0-373224632b74
📒 Files selected for processing (9)
src/platform/src/app/(dashboard)/system/clients/[clientId]/page.tsxsrc/platform/src/app/(dashboard)/system/security/page.tsxsrc/platform/src/app/api/users/tokens/security/route.tssrc/platform/src/modules/api-client/ApiClientPage.tsxsrc/platform/src/modules/api-client/components/TokenSecurityDialog.tsxsrc/platform/src/shared/lib/validators.tssrc/platform/src/shared/services/adminService.tssrc/platform/src/shared/services/clientService.tssrc/platform/src/shared/utils/sanitizeErrorForLogging.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/platform/src/shared/lib/validators.ts
- src/platform/src/shared/services/adminService.ts
- src/platform/src/app/(dashboard)/system/security/page.tsx
- src/platform/src/modules/api-client/components/TokenSecurityDialog.tsx
- src/platform/src/app/(dashboard)/system/clients/[clientId]/page.tsx
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
2 participants
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.
Status of maturity (all need to be checked before merging):
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
APIs & Services
Documentation