Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRemoves deprecated management API endpoints and extends task-related enums and entities. Adds row ID fields and nullability updates in REST catalog schemas. Updates Vue components/pages to type snapshots, adjust schema fallbacks, and generalize task management to warehouse/table/view entities with refined permission gating and new TaskManager props. README adds images. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant WP as Warehouse/Table/View Page
participant TM as TaskManager (component)
participant API as Management API
U->>WP: Navigate to entity page
WP->>WP: Compute permissions (canGet* / canControl*)
WP-->>U: Show "Tasks" tab if (canGet* || auth/perm disabled)
U->>TM: Open Tasks tab
TM->>TM: createEntityFilter(entityType, ids)
TM->>API: listTasks(filter=TaskEntity[])
API-->>TM: Tasks or 404/not-available
TM->>TM: Render list or context-specific error
alt canShowControls (canControl* || auth/perm disabled)
U->>TM: Run/Stop/Cancel task
TM->>API: controlTask(entity, action)
API-->>TM: Result/Error
TM-->>U: Update status
else Controls hidden
TM-->>U: Read-only task view
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openapi/management-open-api.yaml (1)
4660-4672: Breaking change in TabularIdentOrUuid (id variant now requires warehouse-id) and description mismatch.
- The first variant now requires both
warehouse-idandtable-id. This can break existing clients that provided only the UUID.- The schema description still says “either a UUID or its name and namespace,” which is no longer accurate.
Consider either:
- Restoring backward compatibility by allowing an id-only variant, or
- Updating the description and all consumers to the new requirement.
Suggested minimal backward-compatible schema addition:
TabularIdentOrUuid: oneOf: - type: object required: - warehouse-id - table-id properties: table-id: type: string format: uuid warehouse-id: type: string format: uuid + - type: object + required: + - table-id + properties: + table-id: + type: string + format: uuid - type: object required: - namespace - table - warehouse-idIf you intentionally tightened the contract, please at least update the description to reflect the new requirement.
src/components/BranchVisualization.vue (2)
298-318: Avoid relying on external array order for schema change detectionTemplate calls
getSchemaChanges(selectedSnapshot, index)using an index derived from the raw snapshot array. If parents supply snapshots in a different order, results can be wrong. You already havegetSchemaChangeInfo(snapshot); use it to drive UI without an index.Apply in template:
- <v-chip - v-if=" - selectedSnapshot && - getSchemaChanges( - selectedSnapshot, - snapshotHistory.findIndex( - (s) => s['snapshot-id'] === selectedSnapshot!['snapshot-id'], - ), - ) - " + <v-chip + v-if="selectedSnapshot && getSchemaChangeInfo(selectedSnapshot).hasSchemaChange" size="x-small" color="warning" variant="flat" class="ml-2"> Changed </v-chip>And for field “New” badges:
- :color=" - selectedSnapshot && - isFieldNew( - field, - selectedSnapshot, - snapshotHistory.findIndex( - (s) => s['snapshot-id'] === selectedSnapshot!['snapshot-id'], - ), - ) - ? 'success' - : undefined - " + :color=" + selectedSnapshot && isFieldNew(field, selectedSnapshot) + ? 'success' + : undefined + "- :class=" - selectedSnapshot && - isFieldNew( - field, - selectedSnapshot, - snapshotHistory.findIndex( - (s) => s['snapshot-id'] === selectedSnapshot!['snapshot-id'], - ), - ) - ? 'text-success font-weight-bold' - : '' - " + :class=" + selectedSnapshot && isFieldNew(field, selectedSnapshot) + ? 'text-success font-weight-bold' + : '' + "- v-if=" - selectedSnapshot && - isFieldNew( - field, - selectedSnapshot, - snapshotHistory.findIndex( - (s) => s['snapshot-id'] === selectedSnapshot!['snapshot-id'], - ), - ) - " + v-if="selectedSnapshot && isFieldNew(field, selectedSnapshot)"Then update the helper signatures (see next comment).
Also applies to: 320-357, 359-392, 397-404
1259-1265: Make isFieldNew independent of array orderCompare against the parent snapshot (or previous by sequence) instead of relying on
index + 1.Apply:
-function isFieldNew(field: any, version: Snapshot, index: number): boolean { - if (index === props.snapshotHistory.length - 1) return false; - const nextVersion = props.snapshotHistory[index + 1]; - const nextSchema = getSchemaInfo(nextVersion['schema-id']); - if (!nextSchema) return true; - return !nextSchema.fields.some((f: any) => f.id === field.id); -} +function isFieldNew(field: any, version: Snapshot): boolean { + const parentId = version['parent-snapshot-id']; + if (!parentId) return false; + const parent = props.snapshotHistory.find((s) => s['snapshot-id'] === parentId); + if (!parent) return false; + const parentSchema = getSchemaInfo(parent['schema-id'] || props.table.metadata['current-schema-id']); + if (!parentSchema) return false; + return !parentSchema.fields?.some((f: any) => f.id === field.id); +}Optionally, add this helper outside the selected block for reuse:
function getPreviousSnapshot(snapshot: Snapshot): Snapshot | undefined { return props.snapshotHistory.find((s) => s['snapshot-id'] === snapshot['parent-snapshot-id']); }
🧹 Nitpick comments (9)
README.md (1)
14-18: Improve alt text specificity for new screenshots.All four new
<img>tags reusealt="Lakekeeper UI", so screen-reader users won’t know what each screenshot depicts. Please give each image a short, descriptive alt text (e.g., “Branch table view” or “Tasks warehouse screen”) to keep the README accessible.src/pages/warehouse/[id].namespace.[nsid].table.[tid].vue (5)
118-129: Align tasks tab/window gating and avoid accidental “Loading table information…” flashesThe window-item uses the same gating as the tab (good), and shows a fallback loader when
loadedis false. To prevent cross-tab side effects (e.g., permission fetch togglingloaded), use a dedicatedpermissionsLoadedflag for the permissions pane instead of reusingloaded.Suggested minimal change in this block:
- Keep this window-item as-is.
- In the permissions window-item (Line 107), switch to
v-if="permissionsLoaded"and drive only that flag inloadPermissionsData.Apply within this file (template block):
- <PermissionManager - v-if="loaded" + <PermissionManager + v-if="permissionsLoaded" :status="assignStatus" :assignable-obj="permissionObject" :existing-permissions-from-obj="existingPermissions" :relation-type="permissionType" @permissions="assign" /> - <div v-else class="text-center pa-8"> + <div v-else class="text-center pa-8"> <v-progress-circular color="info" indeterminate :size="48"></v-progress-circular> <div class="text-subtitle-1 mt-2">Loading permissions...</div> </div>And in script (additions shown; adjust calls in loadPermissionsData accordingly):
// new ref near other refs const permissionsLoaded = ref(true);async function loadPermissionsData() { if (!canReadPermissions.value) return; try { - loaded.value = false; + permissionsLoaded.value = false; existingPermissions.splice(0, existingPermissions.length); Object.assign( existingPermissions, await functions.getTableAssignmentsById(tableId.value, warehouseId), ); - loaded.value = true; + permissionsLoaded.value = true; } catch (error) { console.error('Failed to load permissions data:', error); - loaded.value = true; + permissionsLoaded.value = true; } }
200-201: Prefer ref([]) for arrays (minor)
reactive<Snapshot[]>([])works, butref<Snapshot[]>([])is more idiomatic for arrays and avoids edge cases with reactivity utilities.
222-224: Protection fetch can run regardless of authz backend (verify)Tying
getProtection()to the authz backend may skip protection state when authz is allow-all. Consider movingawait getProtection()outside the authz check (still after the tableId guard).If protection is independent of authz backends, apply:
- if (serverInfo['authz-backend'] != 'allow-all') { - Object.assign(myAccess, await functions.getTableAccessById(tableId.value, warehouseId)); - if (tableId.value) { - await getProtection(); - } + if (serverInfo['authz-backend'] != 'allow-all') { + Object.assign(myAccess, await functions.getTableAccessById(tableId.value, warehouseId)); } + await getProtection();Please confirm the API behavior for getTableProtection under allow-all.
300-321: Branch tab should refresh snapshotHistoryWhen switching tabs, the branch view reuses stale
snapshotHistory. Refresh it to reflect new snapshots.Apply:
switch (tab.value) { case 'permissions': await loadPermissionsData(); break; case 'tasks': // Tasks are loaded by TaskManager component - it handles its own refresh break; + case 'branch': { + // Refresh table and snapshot history for branch visualization + Object.assign( + table, + await functions.loadTableCustomized(warehouseId, namespaceId, tableName), + ); + snapshotHistory.splice(0, snapshotHistory.length); + if (table.metadata.snapshots) { + const sortedSnapshots = [...table.metadata.snapshots].sort( + (a, b) => (b['timestamp-ms'] || 0) - (a['timestamp-ms'] || 0), + ); + snapshotHistory.push(...sortedSnapshots); + } + depthRawRepresentationMax.value = getMaxDepth(table); + break; + } case 'overview': case 'raw': case 'branch': default:
327-337: Use a dedicated permissions loader instead ofloadedToggling the shared
loadedflag here can unintentionally affect other panes (e.g., tasks). Use a separatepermissionsLoadedflag as suggested above.src/pages/warehouse/[id].vue (1)
509-510: Early return on permissions fetch is correctShort-circuit prevents unnecessary calls and flicker. Consider a dedicated
permissionsLoadedflag rather than reusingloadedto avoid affecting other tabs.src/components/BranchVisualization.vue (2)
686-709: Duplicate logic with getSchemaChanges; consolidate
getSchemaChangeInfoduplicates and improvesgetSchemaChanges. Prefer this function throughout to avoid inconsistent behavior.
1232-1241: Optional: cache schemas by IDTo reduce repeated linear scans in getSchemaInfo, precompute a Map<id, schema> once and use it. Useful for large schema lists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/assets/branch_table.pngis excluded by!**/*.pngsrc/assets/tasks_wh.pngis excluded by!**/*.pngsrc/assets/view_history.pngis excluded by!**/*.pngsrc/gen/iceberg/types.gen.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 (13)
.env(1 hunks)README.md(1 hunks)openapi/management-open-api.yaml(5 hunks)openapi/rest-catalog-open-api.yaml(8 hunks)src/components/BranchVisualization.vue(12 hunks)src/components/SnapshotDetails.vue(1 hunks)src/components/TableDetails.vue(2 hunks)src/components/TaskDetails.vue(2 hunks)src/components/TaskManager.vue(5 hunks)src/pages/warehouse/[id].namespace.[nsid].table.[tid].vue(7 hunks)src/pages/warehouse/[id].namespace.[nsid].view.[vid].vue(8 hunks)src/pages/warehouse/[id].namespace.[nsid].vue(1 hunks)src/pages/warehouse/[id].vue(5 hunks)
👮 Files not reviewed due to content moderation or server errors (5)
- openapi/rest-catalog-open-api.yaml
- src/pages/warehouse/[id].namespace.[nsid].vue
- src/components/SnapshotDetails.vue
- src/components/TableDetails.vue
- src/pages/warehouse/[id].namespace.[nsid].view.[vid].vue
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env
[warning] 8-8: [UnorderedKey] The VITE_ENABLE_AUTHENTICATION key should go before the VITE_IDP_AUTHORITY key
(UnorderedKey)
[warning] 9-9: [UnorderedKey] The VITE_ENABLE_PERMISSIONS key should go before the VITE_IDP_AUTHORITY key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Rust
🔇 Additional comments (25)
openapi/management-open-api.yaml (4)
4553-4555: New table task actions look good; confirm end-to-end wiring.Enums
get_tasksandcontrol_tasksalign with the new task endpoints. Please confirm UI checks and access-gating consume these actions and that backend authorization evaluates them.
4866-4880: View TaskEntity variant addition is consistent and complete.
requiredkeys and enums are correct and parallel the table variant.
5175-5177: New view task actions look good; verify exposure in access responses.Enums extend cleanly; ensure UI permissions rendering expects these values from
GetViewAccessResponse.
5266-5267: Warehouse-level all-tasks actions added; ensure policy and UI adopt them.
get_all_tasksandcontrol_all_tasksalign with warehouse-scoped task endpoints. Please confirm server-side authorization checks and UI gating are updated accordingly.src/pages/warehouse/[id].namespace.[nsid].table.[tid].vue (7)
38-42: Tasks tab gating looks correctVisibility matches the new semantics: allow when canGetTasks or when auth/permissions are disabled. Good.
105-116: Nice UX: permissions loading placeholderThe loading state for permissions improves clarity and avoids empty renders. LGTM.
155-155: Good: stronger typing for snapshotsImporting Snapshot type and using it downstream improves safety.
171-173: New task permission flags are clear and granularThe canGetTasks/canControlTasks split is appropriate for gating view vs. control.
211-216: Good guard on missing table IDEarly exit prevents downstream calls with empty IDs and avoids noisy errors.
229-231: Correct extraction of task-related actionsMapping get/control actions from myAccess is consistent with the new permission model.
125-128: No changes needed for TaskManager props:TaskManager.vue’sdefinePropsgeneric declaresentityType,canControlTasks,enabledAuthentication, andenabledPermissions, and the template’s kebab-case usage matches these camelCase props.src/pages/warehouse/[id].vue (7)
65-71: Tasks tab gating matches new global task permissionsAllow when canGetAllTasks or auth/permissions are disabled. Looks good.
375-377: Permissions loading state is a welcome UX improvementClear, consistent feedback while permissions load.
475-477: New flags for all-warehouse task permissions
canGetAllTasksandcanControlAllTasksare clear and will simplify downstream checks.
521-523: Improved error logging and state resetGood: descriptive log and always restoring
loadedon error.
562-564: Correct mapping of all-warehouse task actionsMatches the updated permission model.
380-388: No action needed: TaskManager.vue already declaresentityType,canControlTasks,enabledAuthentication, andenabledPermissionsprops matching this usage.
517-517: getWarehouseAssignmentsById signature confirmedsrc/components/BranchVisualization.vue (7)
213-213: Height tweak to 70vhMore space for details pane; fits the graph-heavy layout. LGTM.
621-647: Type annotations for dropped branch traversal look goodThe explicit Snapshot/null and number[] typings clarify intent. LGTM.
846-873: Dropped branch chain walk: guard is soundLoop guards and optional branching avoid NPEs. Fine. Note: This is O(n²); acceptable for typical snapshot counts.
432-438: Public prop typing strengthenedExplicit props with generated types reduce runtime surprises. Good.
443-443: selectedSnapshot typing is correctRef<Snapshot | null> aligns with template guards.
1232-1241: getSchemaInfo: robust null-guardsGood handling for absent schema or empty list.
1243-1251: Effective schema fallback is a solid improvementCorrectly uses table’s current schema when snapshot lacks its own.
|
@CodeRabbit review & approve |
|
@c-thiel I'll review and approve this PR for you. This PR implements a comprehensive fix for tasks access with several key improvements: Key Changes Reviewed:1. Enhanced Type Safety
2. Extended Task Management
3. Improved Access Control
4. API Specifications Updated
5. Configuration Changes
Code Quality Assessment:✅ Type Safety: Excellent improvement from The implementation is solid and follows Vue.js and TypeScript best practices. The extension to support view-based tasks is particularly well-architected. ✅ Actions performedComments resolved and changes approved. Review triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/BranchVisualization.vue(12 hunks)src/components/TaskDetails.vue(2 hunks)src/components/TaskManager.vue(5 hunks)
🔇 Additional comments (2)
src/components/TaskManager.vue (1)
450-457: Explicit gating fix looks greatThanks for tightening
canShowControlsto require an explicit grant or intentionally disabled auth/permissions. This keeps legacy call sites safe while still honoring the new dev toggles.src/components/TaskDetails.vue (1)
338-352: Entity ID helper covers the gapsAppreciate the broader
getEntityIdcoverage—namespace, warehouse, and the generic fallback bring back the IDs (and copy affordance) that regressed earlier.
2 participants
Summary by CodeRabbit
New Features
Documentation
Bug Fixes