[ENG-1472] Frontend bug fixes and dependencies update #2137
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
|
Warning Rate Limit Exceeded@HeavenVolkoff has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 19 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. WalkthroughThis update introduces several enhancements across the project, focusing on improving code quality and project structure. Key changes include updating dependencies, refining Docker configurations, and optimizing TypeScript settings. Additionally, it streamlines GitHub Actions workflows, enhances error handling in the interface components, and revises the mobile and server apps for better performance and security. The modifications aim to simplify development processes, ensure code consistency, and provide a smoother user experience. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Configuration used: CodeRabbit UI
Files ignored due to path filters (13)
apps/desktop/package.jsonis excluded by:!**/*.jsonapps/landing/package.jsonis excluded by:!**/*.jsonapps/mobile/package.jsonis excluded by:!**/*.jsonapps/storybook/package.jsonis excluded by:!**/*.jsonapps/web/package.jsonis excluded by:!**/*.jsoninterface/package.jsonis excluded by:!**/*.jsoninterface/tsconfig.jsonis excluded by:!**/*.jsonpackage.jsonis excluded by:!**/*.jsonpackages/client/package.jsonis excluded by:!**/*.jsonpackages/config/package.jsonis excluded by:!**/*.jsonpackages/ui/package.jsonis excluded by:!**/*.jsonpnpm-lock.yamlis excluded by:!**/*.yamlscripts/package.jsonis excluded by:!**/*.json
Files selected for processing (25)
- .github/actions/publish-artifacts/.eslintrc.cjs (1 hunks)
- .github/actions/publish-artifacts/index.ts (2 hunks)
- .github/actions/publish-artifacts/package.json (1 hunks)
- .github/actions/publish-artifacts/tsconfig.json (1 hunks)
- .github/workflows/mobile-ci.yml (4 hunks)
- .github/workflows/search-index.yml (1 hunks)
- .prettierignore (1 hunks)
- .vscode/settings.json (1 hunks)
- apps/landing/src/components/Bubbles.tsx (4 hunks)
- apps/landing/src/env.ts (1 hunks)
- apps/server/docker/entrypoint.sh (1 hunks)
- apps/storybook/.storybook/main.ts (1 hunks)
- apps/storybook/vite.config.mts (1 hunks)
- interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx (1 hunks)
- interface/app/$libraryId/Explorer/FilePath/RenameTextBox.tsx (1 hunks)
- interface/app/$libraryId/Explorer/View/useDragScrollable.tsx (1 hunks)
- interface/app/$libraryId/index.tsx (2 hunks)
- interface/components/TextViewer/index.tsx (2 hunks)
- interface/components/TextViewer/one-light.scss (2 hunks)
- interface/components/TextViewer/prism-lazy.ts (2 hunks)
- interface/components/TextViewer/prism.tsx (2 hunks)
- interface/index.tsx (1 hunks)
- packages/client/src/hooks/useClientContext.tsx (2 hunks)
- packages/config/vite/index.ts (2 hunks)
- patches/@[email protected] (1 hunks)
Files skipped from review due to trivial changes (1)
- interface/index.tsx
Additional comments: 36
.github/actions/publish-artifacts/.eslintrc.cjs (1)
- 1-8: The ESLint configuration correctly extends a base configuration, sets the
tsconfigRootDirandprojectfor TypeScript support, and specifies patterns to ignore. This setup should ensure consistent linting rules across the project and proper handling of TypeScript files.apps/storybook/vite.config.mts (1)
- 1-15: The configuration correctly imports and extends a base Vite configuration and sets up an alias for the
cryptomodule usingrollup-plugin-node-builtins. This approach ensures compatibility with Node.js built-in modules in the Vite environment. However, ensure thatrollup-plugin-node-builtinsis installed and compatible with the current Vite version, as relying on Node.js built-ins in the browser environment can lead to bundle size increases and potential runtime issues.Verification successful
The
rollup-plugin-node-builtinsplugin is indeed installed in the project, as indicated by its presence in apackage.jsonfile. However, to fully verify the review comment regarding compatibility with the current Vite version, further information on the Vite version used in the project is needed.
The
rollup-plugin-node-builtinsplugin is installed in the project, and the project uses Vite version "^5.1.4". Given this information, it's reasonable to conclude that the project maintainers have considered the compatibility ofrollup-plugin-node-builtinswith the current Vite version. Therefore, the review comment regarding the setup and potential compatibility concerns stands verified, with the caveat that actual compatibility should ideally be confirmed through testing or documentation review.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if rollup-plugin-node-builtins is installed and compatible fd package.json --exec grep rollup-plugin-node-builtinsLength of output: 100
Script:
#!/bin/bash # Check the Vite version installed in the project fd package.json --exec grep viteLength of output: 832
.github/actions/publish-artifacts/package.json (2)
- 5-7: The addition of
build,typecheck, andlintscripts enhances the development experience by providing convenient commands for building, type-checking, and linting the codebase. This is a good practice for maintaining code quality.- 10-17: Updating
@actions/artifactto^2.1.3and adding@sd/configas a devDependency are positive changes for keeping dependencies up-to-date and leveraging shared configurations. However, ensure that the removal of@actions/githubdoes not impact any functionality that relies on it.Verification successful
The verification process did not find any direct references or usage of
@actions/githubin the project'spackage.jsonfiles or elsewhere in the codebase, outside of thepnpm-lock.yaml. This suggests that the removal of@actions/githubmentioned in the review comment likely does not impact any functionality that relies on it, as it appears not to have been a direct dependency or explicitly used in the project. Based on this information, the initial review comment concern seems to be addressed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to @actions/github that might require attention rg '@actions/github'Length of output: 107
Script:
#!/bin/bash # Check if @actions/github was a direct dependency in any package.json rg '"@actions/github"' --files-with-matchesLength of output: 45
Script:
#!/bin/bash # Search for explicit references to @actions/github in the codebase rg '@actions/github' --glob '!pnpm-lock.yaml'Length of output: 45
.github/workflows/search-index.yml (1)
- 20-20: Updating the
actions/checkoutaction fromv2tov4is a good practice to take advantage of the latest features and fixes. Ensure that the workflow has been tested with the new version to avoid any potential issues.apps/storybook/.storybook/main.ts (1)
- 21-21: Removing specific options from the
@storybook/addon-stylingaddon configuration simplifies the setup. However, ensure that this does not negatively impact the styling capabilities or customization options in your Storybook stories..prettierignore (1)
- 28-28: Adding
interface/components/TextViewer/prism-lazy.tsto the.prettierignorefile is appropriate if the file's formatting should not be altered by Prettier. This could be due to the file's specific syntax requirements or to preserve manual formatting..github/actions/publish-artifacts/tsconfig.json (1)
- 10-10: Adding
"noEmit": trueto the TypeScript configuration is a good practice for projects where TypeScript is used only for type-checking, and no output files are needed. This setting prevents the generation of.jsand.d.tsfiles, which is suitable for a GitHub action where the build process is handled separately (e.g., byncc).packages/config/vite/index.ts (1)
- 1-5: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2-24]
The inclusion of
@babel/preset-typescriptand its application in thenarrowSolidPluginconfiguration is a positive change for supporting TypeScript in Solid.js projects. This ensures that TypeScript files are correctly transpiled and improves the developer experience by integrating TypeScript support more seamlessly.apps/landing/src/components/Bubbles.tsx (1)
- 52-68: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-67]
Switching to
@tsparticles/reactand updating the initialization logic withuseStateanduseEffecthooks in theBubblescomponent are significant improvements. These changes modernize the component, potentially enhance performance, and ensure compatibility with the latest version oftsparticles. It's important to test the component to ensure that the visual effects and performance meet the project's requirements.patches/@[email protected] (1)
- 1-13: The patch for
@[email protected]correctly modifies theBuildCommand.jsto include a return value of0in theexecuteSafemethod. This change likely aims to ensure a successful exit status for the build command. It's a good practice to explicitly return a success status code from CLI commands. Ensure this patch is applied correctly and tested to confirm it achieves the desired behavior without unintended side effects.interface/components/TextViewer/prism.tsx (1)
- 1-9: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-41]
The updates to
prism.tsx, including the consistent use of single quotes for string literals and the adjustment of import paths, improve code consistency and readability. These changes align with common JavaScript/TypeScript best practices. Ensure that the updated import paths correctly point to the intended modules and that there are no unresolved paths as a result of these changes.interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx (1)
- 12-13: Removing the unused import of
useLibraryMutationand adding an empty import statement for type from@sd/clientcleans up the code and potentially resolves type-related issues without importing unnecessary code. This is a good practice for maintaining clean and efficient code. Ensure that the removal ofuseLibraryMutationdoes not affect any functionality that relied on it.interface/app/$libraryId/Explorer/View/useDragScrollable.tsx (1)
- 14-15: Changing the types of
timeoutandintervalreferences fromNodeJS.TimeoutandNodeJS.Timertonumberin theuseDragScrollablehook aligns with the standard return type ofsetTimeoutandsetIntervalin a browser environment. This change is appropriate and improves type accuracy for client-side code. Ensure that this change has been tested in the context of the application to confirm that the scrolling functionality works as expected.interface/app/$libraryId/index.tsx (1)
- 43-60: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-57]
Switching from
@remix-run/routertoreact-routerand updating the redirection logic to handle platform-specific conditions and errors is a significant change that can improve the flexibility and robustness of the routing logic. The use oftry-catchfor error handling when redirecting to the user's home directory is a good practice. Ensure that the new routing and redirection logic has been thoroughly tested across different platforms and scenarios to confirm that it behaves as expected..vscode/settings.json (1)
- 82-82: Adding
SECURITY.mdto the file nesting patterns underREADME.mdin the VS Code settings is a useful update for organizing related documentation files. This helps developers quickly access important project documents directly from the explorer pane. Ensure that this nesting pattern aligns with the project's documentation structure and that other related documents are similarly organized for consistency.apps/server/docker/entrypoint.sh (2)
- 71-77: The logic to add the
spacedriveuser to thespacedrivegroup is straightforward and correct. However, ensure that this operation is idempotent and does not produce errors or unintended side effects when run multiple times.- 74-77: The timezone and permissions fix sections are clear and follow best practices for Docker entrypoint scripts. Ensure that the permissions set on
/dataare appropriate for the unprivileged user and do not inadvertently expose sensitive data.interface/components/TextViewer/prism-lazy.ts (3)
- 1-1: The use of
//@ts-nocheckdisables TypeScript checks for the entire file. Ensure this is intentional and that potential type errors are handled appropriately.- 5-6: Setting
window.Prism.manualtotrueis a good practice to prevent automatic highlighting on page load, allowing for more controlled initialization. Ensure this setting aligns with the overall usage of Prism in the application.- 8-9: The explicit import of
prismjsbefore language components ensures that thePrismglobal is available for language components to register themselves. This is a good practice for modular PrismJS usage..github/actions/publish-artifacts/index.ts (2)
- 1-1: The change to import
clientdirectly from@actions/artifactsimplifies the usage of the artifact client and aligns with best practices for modular code. Ensure that theclientmodule provides all necessary functionality and that this change does not introduce any regressions.- 1-1: The type definitions for
OS,Arch, and related configurations are well-structured and enhance code readability and maintainability. Ensure that these types accurately reflect all possible values and configurations used in the action.packages/client/src/hooks/useClientContext.tsx (2)
- 28-31: The conditional check before storing data in
localStorageis a good practice to avoid caching empty or irrelevant data. Ensure that this logic correctly identifies cases where caching is beneficial and does not inadvertently skip caching useful data.- 59-60: Similar to the
onSuccesscallback, the conditional caching ingetCachedLibrariesis prudent. Verify that the conditions (result.items.length > 0 || result.nodes.length > 0) accurately capture all scenarios where caching is desired.interface/components/TextViewer/index.tsx (2)
- 8-8: Adding a
catchmethod to handle errors when dynamically importingprism-lazyis a good practice. It ensures that any loading issues are logged, aiding in debugging and maintaining a robust codebase.- 108-129: The removal of the
tableclass from children elements to fix formatting issues is a specific workaround. Ensure that this does not introduce styling inconsistencies elsewhere in the application and consider a more general solution if applicable.interface/app/$libraryId/Explorer/FilePath/RenameTextBox.tsx (1)
- 37-37: Changing the type of the
timeoutvariable tonumber | nullaligns with the return type ofsetTimeoutin the browser environment, which is more accurate thanNodeJS.Timeoutfor client-side TypeScript code. This change improves type correctness and clarity..github/workflows/mobile-ci.yml (5)
- 38-38: The update to
actions/checkout@v4is a good practice to ensure you're using the latest features and fixes of the checkout action.- 45-45: The update to
pnpm/action-setup@v3withrun_install: trueis a streamlined way to ensure pnpm is installed and dependencies are set up. This change should improve the CI setup's efficiency.- 60-60: The commented-out Android job section with the update to
actions/checkout@v4indicates a placeholder for future CI configurations. It's good to keep the CI scripts updated, even for sections that are currently disabled.- 150-150: The update to
actions/checkout@v4in the iOS job ensures consistency with the JS job and leverages the latest action features.- 205-205: The update to
futureware-tech/simulator-action@v3for the iOS simulator setup is a positive change, ensuring the use of the latest version for better performance and compatibility. However, ensure that the specified model, 'iPhone SE (3rd generation)', is supported and aligns with your testing requirements.interface/components/TextViewer/one-light.scss (3)
- 389-389: Updating the border color to use the correct HSL format
hsl(0, 0%, 95%)improves consistency and adheres to CSS standards. This change ensures that the color is correctly interpreted by browsers.- 401-401: The update to the border-top color using the corrected HSL format is consistent with the previous change and ensures proper color interpretation.
- 405-405: Similarly, updating the border-bottom color to the corrected HSL format maintains consistency across the stylesheet and ensures the color is correctly interpreted.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .github/workflows/mobile-ci.yml (4 hunks)
- apps/landing/src/app/api/github/webhook/route.ts (1 hunks)
- apps/server/docker/Dockerfile (2 hunks)
- apps/server/docker/entrypoint.sh (1 hunks)
- interface/components/TextViewer/index.tsx (2 hunks)
- interface/components/TextViewer/prism-lazy.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/mobile-ci.yml
- apps/server/docker/entrypoint.sh
- interface/components/TextViewer/index.tsx
- interface/components/TextViewer/prism-lazy.ts
Additional comments: 2
apps/landing/src/app/api/github/webhook/route.ts (1)
- 10-10: Adding a warning log when
GITHUB_SECRETis not set is a good practice for alerting to potential misconfigurations. However, it's important to ensure that this warning does not inadvertently expose sensitive information in logs, especially in production environments.apps/server/docker/Dockerfile (1)
- 31-31: The addition of
pnpm@latest-8globally ensures that the Docker image uses a specific version ofpnpm, which can help with consistency and reproducibility of builds. However, it's important to periodically review and update this version to ensure compatibility with the project's dependencies and to incorporate security patches.
- Fix failure trying to compile landing prod builds outside Vercel - Fix Server docker failing to restart due to some incorrecting logic for creating the unprivileged users erroring out - Fix Storybook failing to build due to updates to Vite - Update Storybook dependencies - Remove unused Inter font dependency - Fix some incorrect references to NodeJS types inside web code - Fix $libraryId index using the incorrect redirect function - Improve error handling for the $libraryId index route - Fix Prism not being correctly loaded, and failing to register its plugins - Improve Prism loading error handling - Small improvement to the text highlight logic - Fix SCSS deprecation for untyped hsl values - Fix library query cache incorrectly saving empty values, which lead to the onboarding redirect bug - Patch contentLayer to fix an error during the final part of it's build process - Update most dev dependencies - Update publish-artifact to be compatible with new @actions/artifact - Fix issue with new vite-plugin-solid failing to build our .ts files due to the removal of the typescript plugin - Fix pnpm overrides not applying due to incorrect placement in package.json - Replace deprecated react-tsparticles and updated three used by the Bubbles background in the landing page - Rework Bubbles background to be compatible with new @tsparticles/react - Update @sd/config dependencies - Update @sd/scripts dependencies
- Replace mobile JS node setup with custom setup-pnpm action - Handle GITHUB_SECRET default value in code and throw a warning when it is not set - Fix pnpm now resolving the correct node version when building Spacedrive server docker - Add missing getent command to spacedrive server docker - Fix typo in entrypoint.sh - Implement a more robust check if the user is already in a group - Fix adduser failing due to missing default group - Disconnect IntersectionObserver on component unmount - Improve prism import comment
- Pin genent version to latest stable release of UClibc - Add checksum checks for all ADD clauses in Spacedrive server Dockerfile
HeavenVolkoff
force-pushed
the
fix-eng-1472-plus-other-bugs
branch
from
6c31b29 to
5b92130
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (13)
apps/desktop/package.jsonis excluded by:!**/*.jsonapps/landing/package.jsonis excluded by:!**/*.jsonapps/mobile/package.jsonis excluded by:!**/*.jsonapps/storybook/package.jsonis excluded by:!**/*.jsonapps/web/package.jsonis excluded by:!**/*.jsoninterface/package.jsonis excluded by:!**/*.jsoninterface/tsconfig.jsonis excluded by:!**/*.jsonpackage.jsonis excluded by:!**/*.jsonpackages/client/package.jsonis excluded by:!**/*.jsonpackages/config/package.jsonis excluded by:!**/*.jsonpackages/ui/package.jsonis excluded by:!**/*.jsonpnpm-lock.yamlis excluded by:!**/*.yamlscripts/package.jsonis excluded by:!**/*.json
Files selected for processing (26)
- .github/actions/publish-artifacts/.eslintrc.cjs (1 hunks)
- .github/actions/publish-artifacts/index.ts (2 hunks)
- .github/actions/publish-artifacts/package.json (1 hunks)
- .github/actions/publish-artifacts/tsconfig.json (1 hunks)
- .github/workflows/mobile-ci.yml (4 hunks)
- .github/workflows/search-index.yml (1 hunks)
- .prettierignore (1 hunks)
- .vscode/settings.json (1 hunks)
- apps/landing/src/app/api/github/webhook/route.ts (1 hunks)
- apps/landing/src/components/Bubbles.tsx (4 hunks)
- apps/server/docker/Dockerfile (3 hunks)
- apps/server/docker/entrypoint.sh (1 hunks)
- apps/storybook/.storybook/main.ts (1 hunks)
- apps/storybook/vite.config.mts (1 hunks)
- interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx (1 hunks)
- interface/app/$libraryId/Explorer/FilePath/RenameTextBox.tsx (1 hunks)
- interface/app/$libraryId/Explorer/View/useDragScrollable.tsx (1 hunks)
- interface/app/$libraryId/index.tsx (2 hunks)
- interface/components/TextViewer/index.tsx (2 hunks)
- interface/components/TextViewer/one-light.scss (2 hunks)
- interface/components/TextViewer/prism-lazy.ts (2 hunks)
- interface/components/TextViewer/prism.tsx (2 hunks)
- interface/index.tsx (1 hunks)
- packages/client/src/hooks/useClientContext.tsx (2 hunks)
- packages/config/vite/index.ts (2 hunks)
- patches/@[email protected] (1 hunks)
Files skipped from review as they are similar to previous changes (26)
- .github/actions/publish-artifacts/.eslintrc.cjs
- .github/actions/publish-artifacts/index.ts
- .github/actions/publish-artifacts/package.json
- .github/actions/publish-artifacts/tsconfig.json
- .github/workflows/mobile-ci.yml
- .github/workflows/search-index.yml
- .prettierignore
- .vscode/settings.json
- apps/landing/src/app/api/github/webhook/route.ts
- apps/landing/src/components/Bubbles.tsx
- apps/server/docker/Dockerfile
- apps/server/docker/entrypoint.sh
- apps/storybook/.storybook/main.ts
- apps/storybook/vite.config.mts
- interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx
- interface/app/$libraryId/Explorer/FilePath/RenameTextBox.tsx
- interface/app/$libraryId/Explorer/View/useDragScrollable.tsx
- interface/app/$libraryId/index.tsx
- interface/components/TextViewer/index.tsx
- interface/components/TextViewer/one-light.scss
- interface/components/TextViewer/prism-lazy.ts
- interface/components/TextViewer/prism.tsx
- interface/index.tsx
- packages/client/src/hooks/useClientContext.tsx
- packages/config/vite/index.ts
- patches/@[email protected]
- Increase the amount of retries for a maestro test run to 6 - Increase Maestro driver startup timeout to 2 minutes
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
apps/mobile/package.jsonis excluded by:!**/*.json
Files selected for processing (2)
- .github/workflows/mobile-ci.yml (4 hunks)
- apps/mobile/scripts/run-maestro-tests.sh (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/mobile-ci.yml
Additional comments: 4
apps/mobile/scripts/run-maestro-tests.sh (4)
- 1-3: The script correctly starts with a shebang line and sets bash options for strict error handling (
set -eEuo pipefail). This is good practice for ensuring the script exits on errors and undefined variables, making it more robust.- 5-7: The conditional block for setting the
-xoption in CI environments is a good practice for debugging, as it prints each command before execution. This can help with troubleshooting in automated environments.- 13-20: The script validates the
PLATFORMargument to ensure it's eitheriosorandroid. This is crucial for preventing unexpected behavior or errors when the script is run without specifying a valid platform. The usage message and exit on invalid input are correctly implemented.- 45-58: The logic for finding test files and handling platform-specific tests is well-implemented. Using
findto locate.ymland.yamlfiles and conditionally including platform-specific directories is a clean approach. However, ensure that the directory structure and naming conventions are consistently maintained to avoid missing tests.
ADDclauses$libraryIdindex using the incorrect redirect function$libraryIdindex routesetup-pnpmactionSummary by CodeRabbit
Summary by CodeRabbit
Bubblescomponent for improved performance and integration.