Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
public/duckdb-worker-wrapper.js (1)
5-8: Subdirectory deployment issue already flagged in previous review.A previous review comment correctly identified that using
self.location.originwill break subdirectory deployments since it doesn't include the configured base path prefix.Please address the suggested fixes from the previous review:
- Use a relative path:
importScripts('./duckdb/duckdb-browser-eh.worker.js')- Or move the wrapper to
src/so Vite can process it and substituteimport.meta.env.BASE_URLsrc/components/Home.vue (1)
525-532: Correctly awaits the Promise before accessing.bootstrapped.The previous review issue has been properly addressed by using
(await functions.getServerInfo()).bootstrapped.package.json (1)
6-6: Port consistency is now correct.Both
dev:test(line 6) andtest:codegen(line 19) now correctly use port 5001.Also applies to: 19-19
🧹 Nitpick comments (7)
src/components/NavigationBar.vue (1)
53-56: Consider consolidating imports from the same module.The
useVisualStore(line 53) andType(line 55) are both imported from@lakekeeper/console-componentsbut in separate import statements.🔎 Proposed consolidation
-import { useVisualStore } from '@lakekeeper/console-components'; -import { useRouter } from 'vue-router'; -import { Type } from '@lakekeeper/console-components'; +import { Type, useVisualStore } from '@lakekeeper/console-components'; +import { useRouter } from 'vue-router';scripts/update-dependencies-page.js (1)
73-75: Regex may miss crate names containing dots.The pattern
/^(\w[\w-]*)\s*=\s*(.+)/only matches word characters and hyphens. Some Rust crates use dots in their names (e.g.,foo.bar). Consider extending the pattern if such crates may appear in the workspace.🔎 Proposed fix if needed
- const match = line.match(/^(\w[\w-]*)\s*=\s*(.+)/); + const match = line.match(/^([\w][\w.-]*)\s*=\s*(.+)/);src/data/dependencies.json (1)
1-775: Consider adding a generation timestamp or header comment.Since this file is auto-generated by
scripts/update-dependencies-page.js, adding metadata indicating when it was last generated would help identify stale data. However, JSON doesn't support comments natively, so this could be a top-level_generatedfield.src/components/Home.vue (3)
303-303: Consider defining a proper type for the injected auth provider.Using
anytype loses type safety. If the auth provider interface is known, define it explicitly or import from the library.-const auth = inject<any>('auth', null); +interface AuthProvider { + signOut(): void; +} +const auth = inject<AuthProvider | null>('auth', null);
480-516: GitHub API calls may hit rate limits.The unauthenticated GitHub API has a rate limit of 60 requests per hour. If this page is frequently loaded, users may encounter failures. Consider:
- Caching the response in local/session storage with a TTL
- Handling 403 rate-limit responses gracefully
🔎 Example caching approach
async function fetchGitHub() { const cacheKey = 'github-stats'; const cacheTTL = 5 * 60 * 1000; // 5 minutes const cached = sessionStorage.getItem(cacheKey); if (cached) { const { data, timestamp } = JSON.parse(cached); if (Date.now() - timestamp < cacheTTL) { starCount.value = data.stars; forksCount.value = data.forks; version.value = data.version; return; } } try { // ... existing fetch logic ... sessionStorage.setItem(cacheKey, JSON.stringify({ data: { stars: starCount.value, forks: forksCount.value, version: version.value }, timestamp: Date.now() })); } catch (error) { console.error('Error fetching GitHub data:', error); } }
544-556: Duplicate logic with NavigationBar.vue.This
routeToRolesfunction is nearly identical to the one inNavigationBar.vue. Consider extracting this into a shared composable to maintain DRY principles.🔎 Example composable extraction
Create a new composable
src/composables/useNavigation.ts:import { Type, useVisualStore } from '@lakekeeper/console-components'; import { useRouter } from 'vue-router'; import { enabledAuthentication, enabledPermissions } from '@/app.config'; export function useNavigation() { const router = useRouter(); const visual = useVisualStore(); function routeToRoles() { if (enabledAuthentication && enabledPermissions) { router.push('/roles'); } else { visual.setSnackbarMsg({ function: 'routeToRoles', text: 'Authorization is disabled', ttl: 3000, ts: Date.now(), type: Type.INFO, }); } } return { routeToRoles }; }SHARED_COMPONENTS_SETUP.md (1)
328-353: Specify language identifier for the .gitignore code block.The fenced code block for
.gitignoreis missing a language identifier. While.gitignoresyntax highlighting is limited, adding the identifier improves consistency.🔎 Proposed fix
-#### .gitignore - -``` +#### .gitignore + +```gitignore # Logs logs
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (23)
openapi-ts-error-1757869919490.logis excluded by!**/*.logpackage-lock.jsonis excluded by!**/package-lock.jsonpublic/duckdb/duckdb-coi.wasmis excluded by!**/*.wasmpublic/duckdb/duckdb-eh.wasmis excluded by!**/*.wasmpublic/duckdb/duckdb-mvp.wasmis excluded by!**/*.wasmsrc/assets/LAKEKEEPER_IMAGE_TEXT.svgis excluded by!**/*.svgsrc/assets/LAKEKEEPER_IMAGE_TEXT_WHITE.svgis excluded by!**/*.svgsrc/assets/cf.svgis excluded by!**/*.svgsrc/assets/iceberg.pngis excluded by!**/*.pngsrc/assets/logo.pngis excluded by!**/*.pngsrc/assets/logo.svgis excluded by!**/*.svgsrc/assets/python-icon.svgis excluded by!**/*.svgsrc/assets/s3.svgis excluded by!**/*.svgsrc/assets/spark-icon.svgis excluded by!**/*.svgsrc/assets/trino-icon.svgis excluded by!**/*.svgsrc/gen/iceberg/client.gen.tsis excluded by!**/gen/**src/gen/iceberg/index.tsis excluded by!**/gen/**src/gen/iceberg/sdk.gen.tsis excluded by!**/gen/**src/gen/iceberg/types.gen.tsis excluded by!**/gen/**src/gen/management/client.gen.tsis excluded by!**/gen/**src/gen/management/index.tsis excluded by!**/gen/**src/gen/management/sdk.gen.tsis excluded by!**/gen/**src/gen/management/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (66)
.env(1 hunks).env.test(1 hunks).github/workflows/checks.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks).prettierignore(1 hunks)SHARED_COMPONENTS_SETUP.md(1 hunks)console-rs/build.rs(1 hunks)eslint.config.mjs(1 hunks)justfile(0 hunks)openapi-ts.config.ts(0 hunks)package.json(1 hunks)playwright.config.ts(1 hunks)public/duckdb-worker-wrapper.js(1 hunks)scripts/update-dependencies-page.js(1 hunks)src/App.vue(1 hunks)src/app.config.ts(1 hunks)src/auto-imports.d copy.ts(0 hunks)src/common/enums.ts(0 hunks)src/common/globals.ts(0 hunks)src/common/interfaces.ts(0 hunks)src/common/queueConfig.ts(0 hunks)src/common/queueOptions.ts(0 hunks)src/common/schemaUtils.ts(0 hunks)src/common/taskUtils.ts(0 hunks)src/components.d copy.ts(0 hunks)src/components.d.ts(0 hunks)src/components/AddOrEditProjectNameDialog.vue(0 hunks)src/components/AppBar.vue(0 hunks)src/components/AppFooter.vue(0 hunks)src/components/AuthenticationDisabledWarningBanner.vue(0 hunks)src/components/BranchVisualization.vue(0 hunks)src/components/BreadcrumbsFromUrl.vue(0 hunks)src/components/ComputeConnectDialog.vue(0 hunks)src/components/DialogDelete.vue(0 hunks)src/components/Home.vue(5 hunks)src/components/NavigationBar.vue(1 hunks)src/components/PermissionManager.vue(0 hunks)src/components/ProjectManager.vue(0 hunks)src/components/ProjectStatistics.vue(0 hunks)src/components/RenameWarehouseDialog.vue(0 hunks)src/components/SearchTabular.vue(0 hunks)src/components/SnackbarMessage.vue(0 hunks)src/components/SnapshotDetails.vue(0 hunks)src/components/StatisticsDialog.vue(0 hunks)src/components/TableDetails.vue(0 hunks)src/components/TaskConfigDialog.vue(0 hunks)src/components/TaskDetails.vue(0 hunks)src/components/TaskManager.vue(0 hunks)src/components/UserManager.vue(0 hunks)src/components/UserRenameDialog.vue(0 hunks)src/components/ViewDetails.vue(0 hunks)src/components/ViewHistory.vue(0 hunks)src/components/WarehouseActionsMenu.vue(0 hunks)src/components/WarehouseAzure.vue(0 hunks)src/components/WarehouseGCS.vue(0 hunks)src/components/WarehouseJSON.vue(0 hunks)src/components/WarehouseS3.vue(0 hunks)src/components/addNamespaceDialog.vue(0 hunks)src/components/addWarehouseDialog.vue(0 hunks)src/components/assignToRoleDialogSingle.vue(0 hunks)src/components/deletingDialog.vue(0 hunks)src/components/dialogDeleteConfirm.vue(0 hunks)src/components/roleDialog.vue(0 hunks)src/console-components.d.ts(1 hunks)src/data/dependencies.json(1 hunks)
💤 Files with no reviewable changes (47)
- src/components/addWarehouseDialog.vue
- src/components/deletingDialog.vue
- src/components/TaskManager.vue
- src/common/queueOptions.ts
- src/components/SnackbarMessage.vue
- openapi-ts.config.ts
- src/components/roleDialog.vue
- src/components/TaskDetails.vue
- src/components/TableDetails.vue
- src/components/DialogDelete.vue
- src/components/BreadcrumbsFromUrl.vue
- src/common/schemaUtils.ts
- src/common/taskUtils.ts
- src/components/BranchVisualization.vue
- src/components/ViewDetails.vue
- src/components/RenameWarehouseDialog.vue
- src/components/SearchTabular.vue
- src/components/AppFooter.vue
- src/components/TaskConfigDialog.vue
- src/common/queueConfig.ts
- src/common/enums.ts
- src/components/SnapshotDetails.vue
- justfile
- src/components/UserManager.vue
- src/components/addNamespaceDialog.vue
- src/components/WarehouseGCS.vue
- src/components/PermissionManager.vue
- src/components/WarehouseAzure.vue
- src/components/UserRenameDialog.vue
- src/components/dialogDeleteConfirm.vue
- src/components/ViewHistory.vue
- src/components/AuthenticationDisabledWarningBanner.vue
- src/components.d copy.ts
- src/common/globals.ts
- src/components/AppBar.vue
- src/components/WarehouseJSON.vue
- src/components/assignToRoleDialogSingle.vue
- src/components/ComputeConnectDialog.vue
- src/components/WarehouseS3.vue
- src/components/ProjectManager.vue
- src/common/interfaces.ts
- src/auto-imports.d copy.ts
- src/components/AddOrEditProjectNameDialog.vue
- src/components/ProjectStatistics.vue
- src/components/WarehouseActionsMenu.vue
- src/components/StatisticsDialog.vue
- src/components.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
playwright.config.ts (1)
scripts/update-dependencies-page.js (1)
__dirname(16-16)
🪛 dotenv-linter (4.0.0)
.env.test
[warning] 7-7: [UnorderedKey] The VITE_ICEBERG_REST_API_URL key should go before the VITE_MANAGEMENT_API_URL key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The TEST_PASSWORD key should go before the TEST_USERNAME key
(UnorderedKey)
[warning] 22-22: [UnorderedKey] The TEST_ADMIN_PASSWORD key should go before the TEST_ADMIN_USERNAME key
(UnorderedKey)
[warning] 23-23: [UnorderedKey] The TEST_ADMIN_EMAIL key should go before the TEST_ADMIN_PASSWORD key
(UnorderedKey)
🪛 LanguageTool
SHARED_COMPONENTS_SETUP.md
[uncategorized] ~55-~55: The official name of this software platform is spelled with a capital “H”.
Context: ...Create GitHub Actions Workflows Create .github/workflows/release.yml: ```yaml name: ...
(GITHUB)
[uncategorized] ~126-~126: The official name of this software platform is spelled with a capital “H”.
Context: ..._please.outputs.tag_name }} Create `.github/workflows/checks.yml`: yaml name: C...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
SHARED_COMPONENTS_SETUP.md
8-8: Bare URL used
(MD034, no-bare-urls)
328-328: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (15)
.gitignore (1)
68-72: LGTM! Standard Playwright ignore patterns added.The ignore patterns for Playwright test artifacts are standard and appropriate for the new test setup.
eslint.config.mjs (1)
70-73: LGTM! Appropriate ESLint ignore patterns for generated assets.Excluding
public/**and DuckDB-related files from linting is correct since these are third-party or generated assets that shouldn't be linted..github/workflows/release.yml (1)
60-61: Same Rollup binary workaround as in checks.yml—verify necessity.The explicit Rollup binary installation with
--forceis repeated here from the checks workflow. While consistency is good, the concerns about the--forceflag bypassing peer dependency checks apply here as well.Refer to the review comment on
.github/workflows/checks.ymlfor detailed analysis and verification steps..prettierignore (1)
7-16: LGTM! Appropriate Prettier ignore patterns.The ignore patterns correctly exclude generated artifacts, third-party libraries, and build outputs from formatting checks.
src/app.config.ts (1)
1-1: TokenType import from @lakekeeper/console-components is valid and verified.The import is confirmed to be valid based on successful TypeScript compilation. The build script includes type checking (
vue-tsc --noEmit) which would fail ifTokenTypewas not exported by version 0.2.11. The code actively usesTokenType.ACCESS_TOKEN, demonstrating the export is available and properly imported.src/components/NavigationBar.vue (1)
62-74: LGTM!The
routeToRolesfunction correctly checks authentication and permission flags before navigating, and provides appropriate user feedback via the visual store's snackbar when authorization is disabled.scripts/update-dependencies-page.js (1)
102-120: LGTM!The dependency data structure is well-organized with clear separation between console, components, and Rust dependencies. The output includes all relevant metadata (versions, peer dependencies, features).
src/components/Home.vue (1)
323-456: Tic-Tac-Toe game logic is well implemented.The game includes proper win detection, draw handling, simple AI with win/block/center/random strategy, and clean state management. Nice addition for the "Access Pending" view.
package.json (3)
24-24: GitHub reference dependency uses a specific version tag.Using
github:lakekeeper/console-components#v0.2.11pins to a specific release, which is good for reproducibility. Ensure this tag is updated when new versions of the library are released.
15-21: Playwright test scripts are well-organized.Good coverage of test modes: headless, headed, UI mode, debug, and codegen for test generation. The
test:manualscript withSKIP_WEBSERVER=trueis useful for CI environments where the server runs separately.
46-46: Pinia is not used in this project's runtime code. The stores cited (useVisualStore,useUserStore) are imported from@lakekeeper/console-components, an external package in dependencies. The local Pinia setup insrc/stores/index.tsis initialized but never used anywhere in the application. No placement change is needed.Likely an incorrect or invalid review comment.
src/data/dependencies.json (1)
2-3: Version mismatch between dependencies.json and package.json.The console version here is
0.10.2, butpackage.jsonshows version0.10.3. This suggests theupdate-deps-pagescript may not have been run after the version bump, or the file is out of sync.Consider regenerating this file by running:
npm run update-deps-pageOr if this file should be auto-generated during builds, consider adding it to
.gitignoreto avoid version drift.src/App.vue (1)
12-23: LGTM! Clean migration to shared component library.The refactor successfully migrates from local modules to
@lakekeeper/console-componentsand adds explicit theme initialization. The theme is properly synchronized with the visual store state..env.test (1)
1-23: LGTM! Placeholder credentials properly implemented.The previous issue with real test credentials has been correctly resolved. The file now uses appropriate placeholders (
your-test-user,your-admin-user, etc.) and provides clear instructions for users to create their local configuration.playwright.config.ts (1)
1-8: This concern does not apply to this project.The review comment raises a valid concern about
__dirnamein ES modules, but unless you use '.mjs' or '.mts' extensions, or specify type: "module" in your package.json, Playwright Test will treat all TypeScript files as CommonJS. This project has no"type": "module"in package.json, so__dirnameis available and works correctly in CommonJS context. The code at line 8 is safe as-is and requires no changes.Likely an incorrect or invalid review comment.
|
@CodeRabbit review. If all resolved approve |
🧩 Analysis chain🏁 Script executed: Length of output: 163840 🏁 Script executed: Length of output: 42964 🏁 Script executed: Length of output: 1147 🏁 Script executed: Length of output: 136 @c-thiel Understood. I attempted to verify the remaining items (duckdb worker wrapper path, Playwright port, EULA import, server-offline button prop, roles import, .env.test placeholders), but the sandbox only exposed package.json and didn’t include the other files, so I couldn’t conclusively confirm fixes from the repo snapshot. I’m triggering a fresh review now so we can get an up-to-date pass on all outstanding comments. If everything comes back clean, I’ll proceed to approve immediately. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
package.json (1)
6-6: Port inconsistency resolved.Both
dev:testandtest:codegennow use port 5001, fixing the previously reported issue.Also applies to: 19-19
🧹 Nitpick comments (1)
scripts/update-dependencies-page.js (1)
30-44: Existence checks implemented as previously suggested.The checks for missing package.json files are now in place. One remaining concern:
JSON.parse(lines 43-44) can still throw on malformed JSON content. Consider wrapping in try-catch for robustness.🔎 Optional enhancement for malformed JSON handling
-const consolePackage = JSON.parse(fs.readFileSync(consolePackagePath, 'utf8')); -const componentsPackage = JSON.parse(fs.readFileSync(componentsPackagePath, 'utf8')); +let consolePackage, componentsPackage; +try { + consolePackage = JSON.parse(fs.readFileSync(consolePackagePath, 'utf8')); +} catch (e) { + console.error(`❌ Error: Failed to parse ${consolePackagePath}`); + console.error(` ${e.message}`); + process.exit(1); +} +try { + componentsPackage = JSON.parse(fs.readFileSync(componentsPackagePath, 'utf8')); +} catch (e) { + console.error(`❌ Error: Failed to parse ${componentsPackagePath}`); + console.error(` ${e.message}`); + process.exit(1); +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/checks.yml(1 hunks).github/workflows/release.yml(1 hunks)console-rs/build.rs(1 hunks)eslint.config.mjs(2 hunks)package.json(1 hunks)scripts/update-dependencies-page.js(1 hunks)src/assets/dependencies.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/assets/dependencies.json
🚧 Files skipped from review as they are similar to previous changes (3)
- console-rs/build.rs
- eslint.config.mjs
- .github/workflows/checks.yml
🔇 Additional comments (7)
scripts/update-dependencies-page.js (3)
46-52: Clean helper function for dependency parsing.The
parseDependenciesfunction is well-implemented with null safety and consistent sorting.
70-112: Cargo.toml parsing gracefully handles missing file.The optional handling for Cargo.toml is appropriate given it may not exist in all environments. The regex-based TOML parsing is adequate for extracting workspace dependencies, though note it won't handle multi-line dependency declarations or comments within the section.
134-148: Output handling is robust.Directory creation with
{ recursive: true }and clear summary logging are good practices.package.json (4)
15-21: Comprehensive Playwright test scripts added.Good coverage of test modes: headless, headed, UI, debug, and codegen. The
test:manualscript withSKIP_WEBSERVER=trueprovides flexibility for running against an already-running server.
27-31: Dependencies updated to modern versions.The updates to
oidc-client-ts,pinia-plugin-persistedstate-2,vue,vue-json-pretty, andvuetifyalign with the Vue 3 ecosystem modernization. Ensure peer dependency compatibility with the new@lakekeeper/console-componentslibrary.
24-24: Verify GitHub dependency tag exists.The dependency references a specific tag
v0.2.12in the console-components repository. Ensure this tag exists by checking the repository's releases/tags page on GitHub.
46-48: Dependencies are stable releases and compatible with Vue 3.5.x.TypeScript 5.9.3 is the latest version, and TypeScript 5.9.3 is marked as Stable. Pinia 3.x only supports Vue 3 and requires TypeScript 5 or newer, and TypeScript 5.9.3 is successfully used with Vue 3.5.18 in real projects. Vue 3.5 includes improved TypeScript integration and enhanced type inference.
| - name: Install Packages and Build Application | ||
| run: | | ||
| npm ci | ||
| npm ci --include=optional | ||
| # Workaround for npm bug where platform-specific optional dependencies | ||
| # may not install correctly in CI (https://github.com/npm/cli/issues/4828) | ||
| npm install --no-save --legacy-peer-deps @rollup/rollup-linux-x64-gnu | ||
| npm run build |
There was a problem hiding this comment.
Consider a more maintainable approach to the optional dependency workaround.
The npm bug with optional dependencies (npm/cli#4828) still exists in npm 10.9.2 and 11.4.1, so the workaround is addressing a real issue. However, the current approach has maintenance concerns:
- The manual install with
--no-savelacks version pinning, risking drift between the main rollup package and the platform-specific binary - Using
--legacy-peer-depsrequires the same flags when runningnpm cior errors may occur, and it may mask legitimate peer dependency conflicts - The hardcoded platform binary (
@rollup/rollup-linux-x64-gnu) assumes a fixed architecture
A better long-term approach would be to add the platform-specific packages directly to optionalDependencies in package.json with pinned versions. This way, npm ci --include=optional alone would install them correctly, eliminating the need for manual installation and --legacy-peer-deps. Alternatively, consider regenerating the package-lock.json from scratch on a clean environment to ensure all platform variants are captured, which is the recommended workaround for this npm bug.
🤖 Prompt for AI Agents
.github/workflows/release.yml lines 58-64: the current workaround manually
installs a platform-specific Rollup binary with --no-save and
--legacy-peer-deps, which is unpinned, architecture-specific, and brittle;
instead add the platform-specific rollup binaries to package.json
optionalDependencies with explicit pinned versions (matching the rollup
package), remove the manual npm install and the --legacy-peer-deps flag from the
workflow so CI uses only npm ci --include=optional, and regenerate
package-lock.json on a clean, representative environment so all platform
variants are recorded (or document the specific platform pins if regenerating
lock is not feasible).
- Check both local dev path (../console-components/public) and node_modules path - When installed as npm package, files are in dist/, not public/ - Fixes Docker build where console-components is installed from GitHub
This reverts commit b1af7b6.
3 participants
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.