feat: allow clicking on keyid on overview#2990
Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes update multiple dashboard API log components to accept and utilize two new optional properties, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LS as LogSection / KeyIdentifierColumn
participant L as Link Component
participant DV as Detailed View
U->>LS: Interact with log entry
alt Key equals "ID"
LS->>L: Render clickable Link with apiId & keyAuthId
L->>DV: Navigate to detailed view using provided identifiers
DV-->>U: Display detailed key information
else
LS-->>U: Display plain text value
end
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (1)
32-36:⚠️ Potential issueMissing apiId in component destructuring.
The component function parameters don't include the
apiIdprop that was added to the type definition, which creates a mismatch between the defined props and what the component actually receives.export const KeysOverviewLogDetails = ({ distanceToTop, log, setSelectedLog, + apiId, }: KeysOverviewLogDetailsProps) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/components/log-section.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx(1 hunks)apps/dashboard/lib/trpc/routers/utils/granularity.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx (1)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx (1) (1)
KeyIdentifierColumn(64-90)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (1)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/components/log-section.tsx (1) (1)
LogSection(29-113)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx (1)
29-34: LGTM! Passing apiId to child component.The change correctly passes the
apiIdprop down to theKeysOverviewLogDetailscomponent, which is necessary for enabling key ID links in the log details view.apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
92-92: Improved time series granularity for extended time ranges.Changing from "perHour" to "per6Hours" for time ranges over 2 weeks is a good optimization that will reduce data points while maintaining meaningful visualization for longer periods.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/logs-table.tsx (1)
39-39: LGTM! Passing apiId to KeyIdentifierColumn.This change correctly provides the
apiIdto theKeyIdentifierColumncomponent, which is necessary for building the proper navigation link when clicking on key IDs.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/components/log-section.tsx (3)
9-9: LGTM! Adding Link import.Correctly adds the Next.js Link component import which is needed for the clickable key ID functionality.
29-39: LGTM! Updated component props.The component signature is properly updated to include the optional
keyAuthIdandapiIdparameters, which are necessary for building navigation links.
81-93: LGTM! Implemented clickable key ID links.The implementation correctly creates a clickable link when the field key is "ID", directing users to the detailed key view using the provided
apiIdandkeyAuthId. The styling is consistent with the application's design patterns.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx (1)
28-28: Props type correctly updated to include apiId.The
KeysOverviewLogDetailsPropstype has been properly updated to include theapiIdstring property, which will be used for navigation purposes.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx (4)
6-6: Required import added for Link component.The import for the
Linkcomponent from Next.js is correctly added to implement navigable key IDs.
33-33: Props type correctly updated to include apiId.The
KeyIdentifierColumnPropstype is properly updated to include theapiIdstring property.
64-64: Component signature updated correctly.The component function signature now correctly includes the
apiIdparameter, matching the updated type definition.
78-87:Details
❓ Verification inconclusive
Improved user experience with navigable key IDs.
Replacing the static
<div>with a<Link>component improves user experience by making key IDs clickable. However, there's potential for a broken link iflog.key_details?.key_auth_idis undefined.Consider adding a check to ensure both required parameters exist before attempting navigation:
<Link title="Link to the key details" className="font-mono group-hover:underline decoration-dotted" - href={`/apis/${apiId}/keys/${log.key_details?.key_auth_id}/${log.key_id}`} + href={log.key_details?.key_auth_id + ? `/apis/${apiId}/keys/${log.key_details.key_auth_id}/${log.key_id}` + : '#'} + onClick={(e) => !log.key_details?.key_auth_id && e.preventDefault()} >Let's check if there are cases where key_auth_id might be undefined:
🏁 Script executed:
#!/bin/bash # Check if there are any logs without key_auth_id rg -A 3 "key_details\s*[=:]\s*\{" --type=typescript | grep -i "key_auth_id" -B 3Length of output: 115
Below is a revised review comment. I've updated the suggestion diff and noted that our initial verification using the "typescript" file type wasn’t conclusive. In this case, I recommend re-running a grep command (for example, with the correct "tsx" file type) or manually verifying that every log provides a defined
key_auth_idbefore clicking the link.Improved navigation for key IDs – verify presence of key_auth_id
Replacing the static
<div>with a<Link>component improves the user experience by making key IDs clickable. However, iflog.key_details?.key_auth_idis undefined, the generated URL will be broken. Please verify that every log has a valid key authentication ID. For instance, you can re-run a command such as:If any missing values are found, you should update the link construction accordingly. A possible improvement would be:
<Link title="Link to the key details" className="font-mono group-hover:underline decoration-dotted" - href={`/apis/${apiId}/keys/${log.key_details?.key_auth_id}/${log.key_id}`} + href={log.key_details?.key_auth_id + ? `/apis/${apiId}/keys/${log.key_details.key_auth_id}/${log.key_id}` + : '#'} + onClick={(e) => !log.key_details?.key_auth_id && e.preventDefault()} >Please manually verify whether there are any cases where
key_auth_idmight be undefined to avoid broken links in production.
🏁 Script executed:
#!/bin/bash # Re-run the search using file type "tsx" because the file under review is a TSX file. rg -A 3 "key_details\s*[=:]\s*\{" --type=tsx | grep -i "key_auth_id" -B 3Length of output: 223
Action Required: Verify Handling of Undefined key_auth_id
Replacing the static
<div>with a<Link>component indeed improves the user experience by making key IDs clickable. However, iflog.key_details?.key_auth_idis undefined, the generated URL will be broken. While our suggested diff provides a safeguard by defaulting to a'#'link and preventing navigation, our automated search for cases wherekey_auth_idmight be missing did not yield conclusive results—likely due to the file type issue with ripgrep.Please verify manually (or re-run using, for example, a file-pattern-based search like
rg -g '*.tsx') that every log includes a validkey_auth_id.<Link title="Link to the key details" className="font-mono group-hover:underline decoration-dotted" - href={`/apis/${apiId}/keys/${log.key_details?.key_auth_id}/${log.key_id}`} + href={log.key_details?.key_auth_id + ? `/apis/${apiId}/keys/${log.key_details.key_auth_id}/${log.key_id}` + : '#'} + onClick={(e) => !log.key_details?.key_auth_id && e.preventDefault()} >
| <LogSection | ||
| title="Identifiers" | ||
| details={identifiers} | ||
| keyAuthId={log.key_details.key_auth_id} | ||
| apiId={log.key_id} | ||
| /> |
There was a problem hiding this comment.
Enhanced LogSection with navigational capabilities.
The LogSection is now correctly enhanced with keyAuthId and apiId props, enabling navigation to key details. However, I noticed that apiId is being set to log.key_id, which seems incorrect as apiId typically refers to the API identifier, not a key ID.
The value passed to apiId should probably be the actual API ID from the props. Please verify this value:
<LogSection
title="Identifiers"
details={identifiers}
keyAuthId={log.key_details.key_auth_id}
- apiId={log.key_id}
+ apiId={apiId}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <LogSection | |
| title="Identifiers" | |
| details={identifiers} | |
| keyAuthId={log.key_details.key_auth_id} | |
| apiId={log.key_id} | |
| /> | |
| <LogSection | |
| title="Identifiers" | |
| details={identifiers} | |
| keyAuthId={log.key_details.key_auth_id} | |
| apiId={apiId} | |
| /> |
1 participant
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor