feat: mobile-sidbar-sync-with-desktop #2180
Conversation
AkshayBandi027
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@AkshayBandi027 is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@AkshayBandi027 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the sidebar navigation components within the dashboard application. Key changes include the restructuring of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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! 🙏 |
github-actions
bot
added
🕹️ 300 points
Feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/layout.tsx (1)
35-35: Consider adding a comment for clarity.To improve code readability and maintain context for future developers, consider adding a brief comment explaining the purpose of passing the
workspaceprop toMobileSideBar. This can help clarify the intent behind this change, especially in relation to the new Feedback and Discord buttons.Here's a suggested comment you could add above the
MobileSideBarcomponent:+ {/* Passing workspace to MobileSideBar to enable Feedback and Discord buttons */} <MobileSideBar className="lg:hidden" workspace={workspace} />apps/dashboard/app/(app)/mobile-sidebar.tsx (1)
64-64: Includerel="noopener noreferrer"when usingtarget="_blank.When using
target="_blank"in links, it's recommended to includerel="noopener noreferrer"for security reasons. This prevents the new page from accessing thewindow.openerproperty, mitigating security risks like tab-nabbing.Apply this diff:
<Link href={`${item.href}`} target="_blank" key={item.label}> + rel="noopener noreferrer"🧰 Tools
🪛 Biome
[error] 64-64: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/dashboard/app/(app)/desktop-sidebar.tsx (2 hunks)
- apps/dashboard/app/(app)/layout.tsx (1 hunks)
- apps/dashboard/app/(app)/mobile-sidebar.tsx (2 hunks)
- apps/dashboard/lib/constants/workspace-navigations.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/mobile-sidebar.tsx
[error] 49-49: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 64-64: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (4)
apps/dashboard/app/(app)/layout.tsx (1)
35-35: LGTM! Verify MobileSideBar implementation for new buttons.The addition of the
workspaceprop to theMobileSideBarcomponent is consistent with the PR objectives and aligns with how theDesktopSidebaris implemented. This change should enable the addition of Feedback and Discord buttons to the mobile navigation.To ensure the PR objectives are fully met, please verify the implementation of the
MobileSideBarcomponent:apps/dashboard/lib/constants/workspace-navigations.tsx (1)
129-129: Verify the Discord URL for accuracyEnsure that the Discord URL
"https://www.unkey.com/discord"is correct and directs users to the intended destination. If the correct URL is"https://unkey.dev/discord"or another address, please update it accordingly.apps/dashboard/app/(app)/desktop-sidebar.tsx (2)
5-8: Imports navigation constants to centralize definitionsGreat job importing
createWorkspaceNavigationandresourcesNavigationfrom@/lib/constants/workspace-navigations. This promotes maintainability by centralizing navigation definitions and reduces redundancy.
47-47: Dynamically generates workspace navigationUsing
createWorkspaceNavigation(workspace, segments)to generateworkspaceNavigationenhances flexibility and keeps the navigation consistent with the workspace context and current segments.
…grade flow (unkeyed#2179) * fix(billing): add missing subscription fields and audit logging to upgrade flow * fix context properties
* refactor: query audit logs from planetscale * fix: sort logs * [autofix.ci] apply automated fixes * chore: remove csv export * Update apps/dashboard/app/(app)/audit/[bucket]/page.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fmt: add comma * ci: remove wrong lint command --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* chore: Unkey PDF Viewer template * feat: add template --------- Co-authored-by: chronark <[email protected]>
* fix: revalidate /apis after creating new API key * fix: show success message after revalidate suceeds * fix: revalidate cache before routing
…d#2187) Bumps [@types/react-dom](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-dom) from 18.2.25 to 18.3.0. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-dom) --- updated-dependencies: - dependency-name: "@types/react-dom" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (15)
apps/docs/contributing/testing.mdx (1)
16-16: Approved: Improved clarity in API Tests sectionThe addition of "so we also use integration tests" enhances the explanation of why both unit and integration tests are necessary for the core product. This change improves the overall clarity of the testing documentation.
Consider further improving the sentence structure for even better readability. Here's a suggestion:
-`/apps/api` is our core product and needs to be heavily tested. Unit tests are not enough so we also use integration tests: +`/apps/api` is our core product and requires thorough testing. Since unit tests alone are not sufficient, we also employ integration tests:This minor refinement maintains the added context while improving the flow of the sentence.
oss.gg/7_create_a_template.md (4)
6-9: LGTM! Consider adding a contribution guide requirement.The new requirements are excellent additions that promote open-source best practices and ensure accessibility for other developers. They clearly specify the need for open-source licensing, proper documentation, and provide options for where to host the code.
Consider adding a requirement for a basic contribution guide (e.g., CONTRIBUTING.md) to encourage community involvement and maintain consistency across contributions.
31-31: Great addition! Consider adding a brief description.The new example, "Flask RBAC route protection using Unkey," is a valuable addition that demonstrates Unkey's integration with a popular web framework and addresses a common use case.
Consider adding a brief (one-line) description of what the example demonstrates, to give users a quick overview without needing to click through to the repository.
32-33: Excellent example! Consider standardizing the format.The new example, "Time-Sensitive API Keys for Digital Content Access," is a great addition that showcases a specific and relevant use case for Unkey.
To maintain consistency, consider standardizing the format of all entries. For example, you could include the programming language or framework used in brackets after the title, similar to how the Flask example is formatted.
Line range hint
6-33: Overall, excellent updates that align with PR objectives.The changes to this file significantly enhance the template creation guide by adding clear requirements for open-source contributions and providing diverse, practical examples of Unkey integration. These updates align well with the PR objectives of improving documentation and showcasing Unkey's capabilities.
To further improve the document's structure and maintainability:
- Consider creating a separate section for the list of examples, making it easier to add new entries in the future.
- Think about implementing a consistent format for example entries, possibly using a markdown table for better readability and easier parsing.
apps/dashboard/app/(app)/apis/create-api-button.tsx (2)
35-40: LGTM: Async onSuccess with revalidation.The changes to the
onSuccesscallback are well-implemented:
- Making it async allows proper handling of the revalidate function.
- Revalidating the "/apis" route ensures the UI is updated with the new API.
- Moving
router.pushafter revalidation ensures the new API is included in the updated view.These changes align well with the PR objective of synchronizing the sidebar and improving user experience.
Consider adding a try-catch block around the revalidate and router.push calls to handle potential errors gracefully:
async onSuccess(res) { toast.success("Your API has been created"); try { await revalidate("/apis"); router.push(`/apis/${res.id}`); } catch (error) { console.error("Error updating UI:", error); toast.error("Created API, but failed to update UI. Please refresh."); } }
Line range hint
48-48: Move router declaration to avoid potential issues.The
routervariable is currently declared after it's used in theonSuccesscallback. This could lead to runtime errors. To ensurerouteris available when needed, move its declaration to the beginning of the component.Apply this change:
export const CreateApiButton = ({ ...rest }: React.ButtonHTMLAttributes<HTMLButtonElement>) => { + const router = useRouter(); const form = useForm<z.infer<typeof formSchema>>({ resolver: zodResolver(formSchema), }); // ... rest of the component code ... - const router = useRouter(); return ( // ... component JSX ... ); };apps/dashboard/app/(app)/audit/[bucket]/row.tsx (1)
102-103: Approve the update to usetargets, with a suggestion for improvement.The code has been correctly updated to use
auditLog.targetsinstead ofauditLog.resources, maintaining consistency with the type changes.Consider improving type safety by specifying the type of the accumulator in the
reducefunction:auditLog.targets.reduce((acc: Record<string, string>, t) => { acc[t.type] = t.id; return acc; }, {})This change will provide better type inference and catch potential errors earlier in the development process.
internal/db/src/schema/audit_logs.ts (2)
122-122: Consider removing redundant index on auditLogId.The addition of an index on
auditLogIdin theauditLogTargettable may be unnecessary. SinceauditLogIdis already part of the primary key (as seen in thepkdefinition), it is automatically indexed by the database engine.Consider removing this line to avoid redundant indexing:
- auditLog: index("audit_log_id").on(table.auditLogId),This change would simplify the schema without impacting query performance.
Line range hint
1-138: Summary: Schema improvements enhance overall application performance.While these changes to the audit logs schema are not directly related to the mobile sidebar sync mentioned in the PR title, they significantly improve the database structure and query performance for audit logs. This enhancement could indirectly benefit mobile users by providing faster access to audit information.
However, it's worth noting that these changes seem to be outside the scope of the PR objectives as described. Consider splitting these schema improvements into a separate PR for clearer tracking and easier review process.
apps/dashboard/app/(app)/desktop-sidebar.tsx (1)
2-2: LGTM! Consider grouping related imports.The addition of
createWorkspaceNavigationandresourcesNavigationimports aligns well with the PR objective. The removal of unused components (DiscordIconandTag) improves code cleanliness.Consider grouping related imports together for better readability. For example:
import { createWorkspaceNavigation, resourcesNavigation } from "@/app/(app)/workspace-navigations"; import { Feedback } from "@/components/dashboard/feedback-component"; import { Badge } from "@/components/ui/badge"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip";apps/dashboard/app/(app)/workspace-navigations.tsx (2)
49-49: Remove extra whitespace inclassNameThere is an extra space between
py-0.5andfont-monoin theclassNamestring on line 49. Removing the extra space improves code cleanliness.Apply this diff:
- "bg-background border text-content-subtle rounded text-xs px-1 py-0.5 font-mono ", + "bg-background border text-content-subtle rounded text-xs px-1 py-0.5 font-mono ",
58-58: Simplify thePicktypes in function parametersYou can combine the two
Picktypes into a singlePickto simplify the type definition.Apply this diff:
-export const createWorkspaceNavigation = ( - workspace: Pick<Workspace, "features"> & Pick<Workspace, "betaFeatures">, +export const createWorkspaceNavigation = ( + workspace: Pick<Workspace, "features" | "betaFeatures">,apps/dashboard/app/(app)/audit/[bucket]/page.tsx (2)
Line range hint
243-264: Inconsistent query parameter names in 'buildHref' functionThe query parameters used in the
buildHreffunction ("event","user","rootKey") are inconsistent with the ones used elsewhere in the application ("events","users","rootKeys"). This inconsistency may cause the filters to not work as expected when navigating through pagination or resetting filters.Apply this diff to align the query parameter names:
function buildHref(override: Partial<Props["searchParams"]>): string { const searchParams = new URLSearchParams(); const newBefore = override.before ?? before; if (newBefore) { searchParams.set("before", newBefore.toString()); } for (const event of selectedEvents) { - searchParams.append("event", event); + searchParams.append("events", event); } for (const rootKey of selectedRootKeys) { - searchParams.append("rootKey", rootKey); + searchParams.append("rootKeys", rootKey); } for (const user of selectedUsers) { - searchParams.append("user", user); + searchParams.append("users", user); } return `/audit?${searchParams.toString()}`; }
Line range hint
265-272: Consider batching or caching user data retrievalThe code fetches user data individually using
clerkClient.users.getUserfor each user ID. This could lead to performance issues if there are many user IDs. Consider batching the requests if the API supports it or implementing caching to reduce the number of API calls.You might refactor the code as follows if batching is supported:
- const users = ( - await Promise.all(userIds.map((userId) => clerkClient.users.getUser(userId).catch(() => null))) - ).reduce( - (acc, u) => { - if (u) { - acc[u.id] = u; - } - return acc; - }, - {} as Record<string, User>, - ); + const userBatch = await clerkClient.users.getUserList({ userId: userIds }); + const users = userBatch.reduce( + (acc, u) => { + acc[u.id] = u; + return acc; + }, + {} as Record<string, User>, + );Ensure that the
clerkClientsupports batch retrieval of users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
apps/www/public/images/templates/pdf-view.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
- .github/workflows/autofix.ci.yaml (0 hunks)
- apps/billing/package.json (1 hunks)
- apps/dashboard/app/(app)/apis/create-api-button.tsx (2 hunks)
- apps/dashboard/app/(app)/audit/[bucket]/export-csv.tsx (0 hunks)
- apps/dashboard/app/(app)/audit/[bucket]/page.tsx (9 hunks)
- apps/dashboard/app/(app)/audit/[bucket]/row.tsx (2 hunks)
- apps/dashboard/app/(app)/desktop-sidebar.tsx (2 hunks)
- apps/dashboard/app/(app)/mobile-sidebar.tsx (2 hunks)
- apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (3 hunks)
- apps/dashboard/app/(app)/workspace-navigations.tsx (1 hunks)
- apps/dashboard/package.json (1 hunks)
- apps/docs/contributing/testing.mdx (1 hunks)
- apps/workflows/package.json (1 hunks)
- apps/www/app/templates/data.ts (1 hunks)
- apps/www/package.json (1 hunks)
- internal/db/src/schema/audit_logs.ts (3 hunks)
- oss.gg/7_create_a_template.md (2 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/autofix.ci.yaml
- apps/dashboard/app/(app)/audit/[bucket]/export-csv.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/billing/package.json
- apps/workflows/package.json
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (1)
Learnt from: mcstepp PR: unkeyed/unkey#2120 File: apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx:19-19 Timestamp: 2024-09-20T18:32:21.963Z Learning: In the `StripeSuccess` function, TypeScript's type-checking of the `new_plan` parameter ensures that only "free", "pro", or undefined values are accepted, so additional runtime validation is not necessary.
🔇 Additional comments (31)
apps/docs/contributing/testing.mdx (1)
Line range hint
1-33: Verify relevance of testing documentation changes to PR objectivesWhile the improvements to the testing documentation are valuable, they don't seem to directly address the main objectives of this PR, which are to synchronize the mobile sidebar with the desktop version and add Feedback and Discord buttons to the mobile navigation.
Could you clarify how these changes to the testing documentation relate to the mobile sidebar synchronization feature? If they're not directly related, consider:
- Updating this file to include specific testing instructions for the new mobile sidebar features.
- Or, if these changes are unrelated, consider moving them to a separate PR focused on documentation improvements.
🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: Did you mean the adjective “end-to-end” (spelled with hyphens)?
Context: ...gration tests: ###integrationFull end to end tests require a running API either loca...(END_TO_END_HYPHEN)
apps/www/package.json (1)
71-71: Approve the @types/react-dom update, but clarification needed.The update of @types/react-dom from ^18.2.25 to ^18.3.0 is a minor version bump, which is generally safe and follows semantic versioning principles. This change is approved from a technical standpoint.
However, I have a few points to consider:
- This update doesn't seem directly related to the PR's stated objectives of adding Feedback and Discord buttons to the mobile navigation bar.
- It's unclear why this update was included in this specific PR.
Could you please clarify:
- Why was this package update included in this PR?
- Is this update necessary for implementing the mobile navigation changes?
- Have you tested the application with this new version to ensure compatibility?
To verify the impact of this change, you can run the following command:
This will show the installed version and any peer dependencies, helping to ensure everything is compatible.
apps/dashboard/app/(app)/mobile-sidebar.tsx (8)
2-3: LGTM: New imports align with PR objectives.The added imports for workspace navigation, feedback component, and UI elements are appropriate for implementing the synchronization of the mobile sidebar with the desktop version. These additions support the inclusion of Feedback and Discord buttons in the mobile navigation as per the PR objectives.
Also applies to: 5-5
15-21: LGTM: Props type update enables dynamic workspace navigation.The addition of the
workspaceproperty to the Props type is well-structured and allows for passing necessary workspace data to the component. This change enables the rendering of dynamic workspace-specific navigation items, which aligns with the PR objectives of synchronizing the mobile sidebar with the desktop version.
24-25: LGTM: Function signature and hook updates support dynamic navigation.The updated
MobileSideBarfunction signature correctly includes the newworkspaceprop, reflecting the changes in the Props type. The switch touseSelectedLayoutSegmentshook is appropriate for determining the current navigation state. These changes effectively support the implementation of dynamic navigation based on the workspace data, aligning with the PR objectives.
27-27: LGTM: Dynamic workspace navigation creation.The use of
createWorkspaceNavigationto generate navigation items dynamically based on the workspace data and current segments is an excellent approach. This implementation supports the PR objective of synchronizing the mobile sidebar with the desktop version, ensuring consistency across different device types.
45-54: LGTM: Effective dynamic rendering of workspace navigation.The implementation of dynamic rendering for workspace navigation items is well-structured and maintainable. The use of SheetClose ensures proper closing of the mobile sidebar after selection, enhancing the user experience. The addition of the key prop to the Link component addresses a previous review comment, improving React's rendering efficiency.
60-69: LGTM: Consistent and effective resources navigation rendering.The implementation of the resources navigation rendering is consistent with the workspace navigation and well-structured. The use of
target="_blank"for resource links is appropriate for external resources. The dynamic rendering approach allows for easy updates to the resources list, enhancing maintainability. The key prop is correctly added to the Link component, addressing potential React rendering issues.
70-70: LGTM: Feedback component successfully integrated.The addition of the Feedback component at the end of the resources section successfully addresses the PR objective of including a Feedback button in the mobile navigation. Its placement is logical and consistent with typical UI patterns, enhancing the user experience on mobile devices.
Line range hint
1-77: Great job implementing the mobile sidebar synchronization!The changes in this file effectively address the PR objectives:
- The mobile sidebar is now synchronized with the desktop version.
- Feedback and resource navigation items (including Discord) are now accessible on mobile.
- The implementation uses dynamic rendering, enhancing maintainability and flexibility.
- Previous review comments about missing key props have been addressed.
These updates significantly improve the mobile user experience and consistency across devices. Well done!
apps/dashboard/app/(app)/apis/create-api-button.tsx (2)
2-2: LGTM: New import for revalidation.The addition of the
revalidateimport is appropriate for updating the UI after creating a new API. This aligns well with the PR objective of synchronizing the sidebar.
Line range hint
1-96: Summary: Changes align with PR objectives, minor improvements suggested.The modifications to the
CreateApiButtoncomponent effectively contribute to synchronizing the sidebar by ensuring the UI is updated after creating a new API. This aligns well with the PR objectives of improving the mobile navigation experience.Key improvements:
- Added revalidation after creating a new API.
- Made the
onSuccesscallback asynchronous for proper handling of asynchronous operations.These changes enhance the user experience by ensuring that newly created APIs are immediately reflected in the UI.
Suggestions for further improvement:
- Move the
routerdeclaration to the beginning of the component to avoid potential issues.- Consider adding error handling for the revalidation and navigation processes.
While these changes don't directly address the addition of Feedback and Discord buttons mentioned in the PR objectives, they do improve the overall functionality of the API creation process, which is a valuable enhancement to the dashboard.
apps/dashboard/app/(app)/audit/[bucket]/row.tsx (2)
17-17: Verify the intention behind broadening theactor.type.The type of
auditLog.actor.typehas been changed from a specific union type to a more generalstring. While this allows for more flexibility, it also reduces type safety.Could you clarify the reasoning behind this change? If it's to accommodate potential new actor types, consider using a union type with a catch-all string literal:
type: "key" | "user" | "system" | (string & {});This approach maintains flexibility while preserving some type safety for known values.
✅ Verification successful
Verified the change to
actor.typeacross the codebase.The transition from a specific union type to
stringforauditLog.actor.typehas been consistently applied and does not introduce type safety issues based on current usages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of actor.type in the codebase # Expected result: Instances where actor.type is used, to ensure compatibility with the new type rg --type typescript -g '!*.test.ts' -g '!*.spec.ts' 'actor\.type'Length of output: 103
Script:
#!/bin/bash # Description: Check for usage of actor.type in the codebase # Expected result: Instances where actor.type is used, to ensure compatibility with the new type rg 'actor\.type' --glob '!*.test.ts' --glob '!*.spec.ts' -g '*.ts' -g '*.tsx'Length of output: 450
22-25: Approve renamingresourcestotargets, but clarification needed.The renaming of
resourcestotargetshas been implemented correctly. This change appears to be a conceptual shift in how these entities are represented.Could you provide more context on why this rename was necessary? How does it relate to the PR's main objective of synchronizing the mobile sidebar with the desktop version?
apps/dashboard/package.json (3)
Line range hint
3-3: Version bump looks good.The version has been incremented to 0.1.36, which is appropriate for adding a new feature.
127-127: Minor type definition update.The @types/react-dom package has been updated from 18.2.25 to 18.3.0. This is a minor update and shouldn't directly impact the new feature being added.
Line range hint
1-133: Verify if new dependencies are needed for the mobile sidebar feature.The PR objectives mention adding Feedback and Discord buttons to the mobile navigation bar, but there are no new dependencies added in this file. Please confirm if any additional packages are required for implementing this feature.
✅ Verification successful
No new dependencies are needed for the mobile sidebar feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any new dependencies related to mobile navigation or UI components have been added in other package.json files. # Test: Search for changes in package.json files that might be related to the new feature. git diff origin/main -- '**/package.json' | grep -E '^\+.*(@react-navigation|@react-native|react-native-.*|@ui-kitten|@expo)'Length of output: 127
Script:
#!/bin/bash # Description: List all newly added dependencies in package.json files to verify if any are needed for the mobile sidebar feature. # Extract added lines from package.json files in the PR and list the newly added dependencies. git diff origin/main -- '**/package.json' | grep -E '^\+.*"[^"]+":' | awk -F '"' '{print $2}'Length of output: 95
Script:
#!/bin/bash # Description: List all newly added dependencies in package.json files to verify if any are needed for the mobile sidebar feature. # Extract all added dependencies and devDependencies in package.json files from the PR. git diff origin/main -- '**/package.json' | grep -E '^\+.*"(dependencies|devDependencies)": {' -A 100 | grep -E '^\+ *"[^"]+":' | awk -F '"' '{print $2}'Length of output: 155
internal/db/src/schema/audit_logs.ts (2)
43-49: LGTM! Improved relational structure for auditLogBucket.The addition of
auditLogBucketRelationsenhances the schema by explicitly defining the relationships betweenauditLogBucket,workspaces, andauditLog. This improvement will facilitate more efficient queries and maintain data integrity.
81-84: LGTM! Enhanced query performance with new indexes.The addition of indexes for
bucketId,event,actorId, andtimein theauditLogtable will significantly improve read query performance for common audit log operations.To ensure optimal performance, please monitor the impact of these new indexes on write operations. Run the following script to check the table statistics after a period of normal usage:
apps/dashboard/app/(app)/desktop-sidebar.tsx (2)
9-9: LGTM! Please clarify the purpose of the Loader2 import.The addition of the
Loader2import fromlucide-reactis fine. However, it's not immediately clear how this relates to the PR objective of synchronizing the mobile sidebar with the desktop version.Could you please explain the intended use of the
Loader2component in the context of this PR? Is it part of a loading state improvement for the sidebar?
Line range hint
1-164: Overall, the changes align well with the PR objectives.The modifications to the
desktop-sidebar.tsxfile contribute significantly to synchronizing the mobile sidebar with the desktop version. Key improvements include:
- Dynamic generation of workspace navigation items.
- Removal of unused components and imports.
- Potential addition of loading state handling.
These changes enhance maintainability and flexibility, making it easier to keep the mobile and desktop sidebars in sync.
To fully meet the PR objectives:
- Ensure that the Feedback and Discord buttons are properly included in the
createWorkspaceNavigationfunction.- Verify that these buttons are accessible on mobile devices.
- Consider adding a comment explaining the purpose of the
Loader2component in the context of this PR.Please run the verification script provided earlier to confirm the inclusion of Feedback and Discord buttons. Additionally, could you provide a screenshot or description of how these buttons appear on the mobile sidebar?
apps/www/app/templates/data.ts (1)
51-62: Summary of review findings
- The changes introduce a new "pdf-view" template entry, which is structurally correct and consistent with other entries.
- However, these changes appear to be unrelated to the PR objectives of synchronizing the mobile sidebar with the desktop version and adding Feedback and Discord buttons to the mobile navigation bar.
Please clarify the following:
- How do these changes relate to the PR objectives?
- If they are unrelated, should they be moved to a separate PR?
- Are there any missing changes that should be included to address the PR objectives?
This clarification will help ensure that the PR remains focused and traceable to its intended objectives.
apps/dashboard/app/(app)/audit/[bucket]/page.tsx (1)
78-78: Verify the compatibility of date comparison in queryWhen filtering logs based on
createdAtusinggte(table.createdAt, retentionCutoffUnixMilli), ensure thattable.createdAtandretentionCutoffUnixMilliare in compatible formats. Iftable.createdAtis a timestamp or date object, it should be comparable to the Unix timestamp in milliseconds.Run the following script to check the data types of
createdAtfields:If the database is not SQLite, adjust the command accordingly to match your database system.
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (8)
3-3: Import ofinsertAuditLogsis correctly addedThe addition of
insertAuditLogsimport is appropriate and used effectively in the code.
8-8: Import ofingestAuditLogsTinybirdis properly includedIncluding
ingestAuditLogsTinybirdfrom@/lib/tinybirdis necessary for the audit logging functionality.
10-10: Import ofdefaultProSubscriptionsis appropriateThe import of
defaultProSubscriptionsfrom@unkey/billingis correct and used appropriately to set default subscriptions when upgrading to "pro".
11-11: Importingheadersfromnext/headersUsing
headersfromnext/headersallows access to request headers, which is correctly implemented.
25-26: Ensure user authentication before proceedingFetching the current user with
currentUser()and verifying bothtenantIdanduserenhances security by redirecting unauthenticated users.
82-98: Database transaction update is well-structuredThe workspace update within the transaction is correctly implemented, conditionally setting fields when upgrading to "pro".
100-116: Audit logs insertion within transactionInserting audit logs using
insertAuditLogs(tx, {...})within the transaction ensures atomicity, which is appropriate.
Line range hint
136-140: Event tracking with PostHog is properly implementedCapturing the
plan_changedevent withPostHogClientwhen upgrading the plan ensures that analytics are accurately recorded.
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx
Outdated
Show resolved
Hide resolved
|
Awarding AkshayBandi027: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/AkshayBandi027 |
* feat/mobile-sidbar-sync-with-desktop * fix(billing): add missing subscription fields and audit logging to upgrade flow (unkeyed#2179) * fix(billing): add missing subscription fields and audit logging to upgrade flow * fix context properties * refactor: query audit logs from planetscale (unkeyed#2181) * refactor: query audit logs from planetscale * fix: sort logs * [autofix.ci] apply automated fixes * chore: remove csv export * Update apps/dashboard/app/(app)/audit/[bucket]/page.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fmt: add comma * ci: remove wrong lint command --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Unkey PDF Viewer template [SIDE QUEST] (unkeyed#2191) * chore: Unkey PDF Viewer template * feat: add template --------- Co-authored-by: chronark <[email protected]> * perf: add database indices (unkeyed#2192) * fix: add header again * docs: Removing pnpm test:routes (unkeyed#2184) * fix: revalidate /apis after creating new API (unkeyed#2183) * fix: revalidate /apis after creating new API key * fix: show success message after revalidate suceeds * fix: revalidate cache before routing * chore(deps-dev): bump @types/react-dom from 18.2.25 to 18.3.0 (unkeyed#2187) Bumps [@types/react-dom](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-dom) from 18.2.25 to 18.3.0. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-dom) --- updated-dependencies: - dependency-name: "@types/react-dom" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Refactor/workspace-navigation * ran fmt --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Meg Stepp <[email protected]> Co-authored-by: Andreas Thomas <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Nazar Poshtarenko <[email protected]> Co-authored-by: Harsh Shrikant Bhat <[email protected]> Co-authored-by: Gerald Maboshe <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Add default bytes and prefix * adding bytes and prefix columns in harness * fmt * Await * Resolved changes * typo * Capital and Extra bracket * [autofix.ci] apply automated fixes * feat: mobile-sidbar-sync-with-desktop (#2180) * feat/mobile-sidbar-sync-with-desktop * fix(billing): add missing subscription fields and audit logging to upgrade flow (#2179) * fix(billing): add missing subscription fields and audit logging to upgrade flow * fix context properties * refactor: query audit logs from planetscale (#2181) * refactor: query audit logs from planetscale * fix: sort logs * [autofix.ci] apply automated fixes * chore: remove csv export * Update apps/dashboard/app/(app)/audit/[bucket]/page.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fmt: add comma * ci: remove wrong lint command --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: Unkey PDF Viewer template [SIDE QUEST] (#2191) * chore: Unkey PDF Viewer template * feat: add template --------- Co-authored-by: chronark <[email protected]> * perf: add database indices (#2192) * fix: add header again * docs: Removing pnpm test:routes (#2184) * fix: revalidate /apis after creating new API (#2183) * fix: revalidate /apis after creating new API key * fix: show success message after revalidate suceeds * fix: revalidate cache before routing * chore(deps-dev): bump @types/react-dom from 18.2.25 to 18.3.0 (#2187) Bumps [@types/react-dom](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-dom) from 18.2.25 to 18.3.0. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-dom) --- updated-dependencies: - dependency-name: "@types/react-dom" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Refactor/workspace-navigation * ran fmt --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Meg Stepp <[email protected]> Co-authored-by: Andreas Thomas <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Nazar Poshtarenko <[email protected]> Co-authored-by: Harsh Shrikant Bhat <[email protected]> Co-authored-by: Gerald Maboshe <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: update go sdk code examples to the current sdk version (#2200) * fix: update go sdk code examples to the current sdk version * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Akshay <[email protected]> Co-authored-by: Meg Stepp <[email protected]> Co-authored-by: Andreas Thomas <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Nazar Poshtarenko <[email protected]> Co-authored-by: Gerald Maboshe <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
What does this PR do?
Fixes #2171
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
unkey-sidebar.mp4
Summary by CodeRabbit
Feedbackcomponent in the mobile sidebar.MobileSideBarto utilize workspace data for rendering links.