UI Refinements #445
UI Refinements #445
Conversation
WalkthroughThis pull request standardizes import paths for entity types and updates various UI components. It simplifies component interfaces by removing obsolete props and functions, introduces sorting mechanisms and dynamic linking via a new property in the header, and renames several variables to reflect a shift from "distributions" to "summary". Overall, the changes adjust module organization and enhance component clarity without altering core functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageHeader
participant EntityConfig
User->>PageHeader: Load page
PageHeader->>EntityConfig: Retrieve learnHref based on entity type
EntityConfig-->>PageHeader: Return URL
User->>PageHeader: Click "Learn" button
PageHeader->>User: Navigate to URL
sequenceDiagram
participant User
participant DriftMetricToggle
participant sortFunc
User->>DriftMetricToggle: Open metrics view
DriftMetricToggle->>sortFunc: Request sorted metrics
sortFunc-->>DriftMetricToggle: Return sorted list (PSI prioritized)
DriftMetricToggle->>User: Render sorted metric buttons
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 🪧 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 (
|
sean-zlai
force-pushed
the
sean/ui-simplifications
branch
from
30dcc86 to
2208ba3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (23)
frontend/src/lib/api/utils.ts(1 hunks)frontend/src/lib/components/ChartControls.svelte(2 hunks)frontend/src/lib/components/DriftMetricToggle.svelte(1 hunks)frontend/src/lib/components/EntityTable.svelte(2 hunks)frontend/src/lib/components/ExpandableCell.svelte(1 hunks)frontend/src/lib/components/NavigationBar.svelte(1 hunks)frontend/src/lib/components/PageHeader.svelte(2 hunks)frontend/src/lib/components/SortButton.svelte(2 hunks)frontend/src/lib/job/tree-builder/tree-builder.ts(1 hunks)frontend/src/lib/types/Entity.ts(1 hunks)frontend/src/lib/types/LogicalNode.ts(1 hunks)frontend/src/lib/util/sort.ts(1 hunks)frontend/src/routes/+layout.svelte(1 hunks)frontend/src/routes/[conf]/+page.server.ts(1 hunks)frontend/src/routes/[conf]/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/+layout.svelte(2 hunks)frontend/src/routes/[conf]/[name]/+layout.ts(1 hunks)frontend/src/routes/[conf]/[name]/observability/ObservabilityNavTabs.svelte(1 hunks)frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte(4 hunks)frontend/src/routes/[conf]/[name]/observability/summary/+page.svelte(3 hunks)frontend/src/routes/[conf]/[name]/observability/summary/+page.ts(2 hunks)frontend/src/routes/[conf]/[name]/overview/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/overview/EntityProperties.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- frontend/src/routes/+layout.svelte
- frontend/src/lib/types/LogicalNode.ts
- frontend/src/lib/components/ExpandableCell.svelte
- frontend/src/routes/[conf]/[name]/+layout.ts
- frontend/src/routes/[conf]/[name]/overview/EntityProperties.svelte
- frontend/src/routes/[conf]/[name]/observability/summary/+page.ts
- frontend/src/routes/[conf]/+page.server.ts
- frontend/src/lib/job/tree-builder/tree-builder.ts
- frontend/src/lib/components/NavigationBar.svelte
- frontend/src/lib/api/utils.ts
- frontend/src/routes/[conf]/[name]/overview/+page.svelte
🔇 Additional comments (39)
frontend/src/routes/[conf]/[name]/observability/ObservabilityNavTabs.svelte (1)
14-16: Tab name change matches PR objectives.The renaming from "Distributions" to "Summary" aligns with the PR description.
frontend/src/lib/util/sort.ts (1)
6-6: Type update consistent with tab renaming.SortContext type updated to reflect "summary" instead of "distributions".
frontend/src/routes/[conf]/[name]/+layout.svelte (2)
3-3: Import addition for entity context.New import enables dynamic learn link functionality.
17-20: Dynamic learn link implementation.PageHeader now uses entity configuration to set appropriate learning resources.
frontend/src/lib/components/EntityTable.svelte (3)
11-12: Entity imports added.Enables dynamic label generation from entity config.
23-24: Dynamic entity label.Uses entity configuration to determine appropriate label based on entity type.
39-39: Message uses dynamic label.Replaces hardcoded message with entity-appropriate term.
frontend/src/lib/components/PageHeader.svelte (2)
6-7: Flexible learning links.The new
learnHrefproperty improves component versatility.
16-22: Conditional button rendering with dynamic linking.Cleanly replaces old static function with dynamic link.
frontend/src/routes/[conf]/+page.svelte (3)
5-8: Imports align with component restructuring.Appropriate imports for the restructured components.
19-22: Clean header and separator implementation.The page structure is now properly organized.
23-23: Simplified EntityTable props.Title prop correctly removed since it's now in PageHeader.
frontend/src/lib/components/SortButton.svelte (2)
10-13: Simplified props handling.Good use of rest parameters for flexibility.
27-30: Clean button implementation.Direct rendering with spread props improves maintainability.
frontend/src/lib/components/DriftMetricToggle.svelte (4)
6-6: Added sortFunc import.Necessary import for the new sorting functionality.
9-14: Separated param config from initialization.Cleaner structure for reusing config in sorting.
15-18: PSI metric prioritization.Well-implemented sorting that fulfills PR objective.
22-22: Updated iteration to use sorted metrics.Properly utilizes the new sorted metrics array.
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte (4)
157-158: ChartControls property update looks good.The
showSortproperty reflects the component's UI refinement.
198-200: Consistent event handler implementation.Changed from arrow function to block statement for better readability.
288-290: Block statement for onbrushend - good consistency.Maintains the same pattern as other event handlers.
306-308: Consistent onbrushend implementation.Block statement matches other similar event handlers in the file.
frontend/src/lib/components/ChartControls.svelte (2)
5-5: Import updated to SortButton component.Aligns with PR objective of renaming ActionButtons to SortButton.
43-45: SortButton implementation looks good.Clean conditional rendering based on the showSort prop.
frontend/src/lib/types/Entity.ts (7)
41-43: Model entity learnHref added.Good addition though TODO suggests more specific documentation might be needed.
50-52: Join entity learnHref added.Appropriate documentation link for Joins.
59-61: GroupBy entity learnHref added.Appropriate documentation link for GroupBys.
68-70: StagingQuery entity learnHref added.Proper documentation link for Staging Queries.
77-79: EntitySource learnHref added.Correct documentation link for Entity Sources.
86-88: EventSource learnHref added.Reuses the Source documentation link appropriately.
95-97: JoinSource learnHref added.Uses the ChainingFeatures documentation link.
frontend/src/routes/[conf]/[name]/observability/summary/+page.svelte (8)
17-18: Variable names updated for clarity.Renamed from distributions-specific to more general columnSummaries.
20-23: Promise handling updated.Now correctly uses columnSummariesPromise.
25-26: Error message updated.Reflects the new terminology.
28-31: Sort context and function updated.Now uses 'summary' context and sorts columnSummaries.
50-52: ChartControls context updated.Context changed from 'distributions' to 'summary'.
60-62: Loading message updated.Now references column summaries.
64-65: No data message updated.Message now correctly references column summaries.
67-73: Iteration variables updated.Now properly uses columnSummary variables throughout.
| sort(data.items, (d) => (d.metaData?.name === 'risk.user_transactions.txn_join' ? -1 : 1)) | ||
| ); | ||
| const learnHref = entityConfig[getEntityType(data.items[0])].learnHref; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add null check for empty data arrays.
No check if data.items is empty before accessing first element.
🏁 Script executed:
#!/bin/bash
# Find if there are any defensive checks elsewhere for empty data.items
rg -A 3 "data.items\[0\]" --glob "*.svelte" --glob "*.ts"Length of output: 349
Null check required on data.items access
- The code in
frontend/src/routes/[conf]/+page.svelte(line 16) directly accessesdata.items[0]without checking if the array is empty. - Please add a condition to verify that
data.itemsis non-empty before proceeding.
|
Looks good overall, but a few issues:
Screen.Recording.2025-02-27.at.8.21.15.AM.mov
|
|
@ken-zlai Good catch! Yeah, I was deriving the type from the first item, but that won't work when there are no results (the navigation issues are due to thrown errors). Let me rework how this is setup (probably just pass the type explicitly since it's known upstream). Thanks for catching this! |
|
@ken-zlai I added |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/routes/[conf]/[name]/+layout.ts (1)
4-4: Remove unused import.The entityConfig import is defined but never used in this file.
-import { entityConfig, getEntityConfigFromPath } from '$src/lib/types/Entity'; +import { getEntityConfigFromPath } from '$src/lib/types/Entity';🧰 Tools
🪛 GitHub Check: frontend_tests
[failure] 4-4:
'entityConfig' is defined but never usedfrontend/src/routes/[conf]/+page.svelte (1)
7-7: Remove unused import.The getEntityType import is defined but never used in this file.
-import { getEntityConfigFromPath, getEntityType } from '$lib/types/Entity'; +import { getEntityConfigFromPath } from '$lib/types/Entity';🧰 Tools
🪛 GitHub Check: frontend_tests
[failure] 7-7:
'getEntityType' is defined but never used. Allowed unused vars must match /^$$(Props|Events|Slots|Generic)$/ufrontend/src/lib/types/Entity.ts (2)
42-42: Address TODO comment.The TODO comment should be addressed by finding a specific documentation link for models.
140-143: Add input validation to getEntityConfigFromPath.The function should handle edge cases like null/undefined paths.
export function getEntityConfigFromPath(path: string) { + if (!path) return undefined; const config = Object.values(entityConfig).find((c) => c.path != null && path.startsWith(c.path)); return config; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
frontend/src/lib/components/EntityTable.svelte(2 hunks)frontend/src/lib/types/Entity.ts(2 hunks)frontend/src/routes/[conf]/+page.server.ts(2 hunks)frontend/src/routes/[conf]/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/+layout.svelte(2 hunks)frontend/src/routes/[conf]/[name]/+layout.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/routes/[conf]/[name]/+layout.svelte
- frontend/src/routes/[conf]/+page.server.ts
🧰 Additional context used
🪛 GitHub Check: frontend_tests
frontend/src/routes/[conf]/+page.svelte
[failure] 7-7:
'getEntityType' is defined but never used. Allowed unused vars must match /^$$(Props|Events|Slots|Generic)$/u
frontend/src/routes/[conf]/[name]/+layout.ts
[failure] 4-4:
'entityConfig' is defined but never used
🔇 Additional comments (5)
frontend/src/lib/components/EntityTable.svelte (2)
11-12: Component now uses EntityConfig instead of title prop.Component now leverages a structured configuration object rather than a plain string, enabling more consistent behavior.
Also applies to: 16-20
39-39: Dynamic label generation is more maintainable.Using entityConfig.label.toLowerCase() creates more consistent messaging across the application.
frontend/src/routes/[conf]/[name]/+layout.ts (1)
35-35: Simplified config retrieval.Using getEntityConfigFromPath simplifies the code by deriving confType directly from the URL path.
frontend/src/routes/[conf]/+page.svelte (2)
18-18: Good use of optional chaining.The code properly handles potential null/undefined values with optional chaining.
21-28: Improved conditional rendering with fallback.The conditional rendering provides a clear fallback for unknown entity types, improving user experience.
…stead of first item to determine entityConfig (or pass explicitly)
sean-zlai
force-pushed
the
sean/ui-simplifications
branch
from
8622cff to
17c2a24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/routes/[conf]/+page.server.ts (1)
35-37: Consider preserving original error.Current implementation might lose original error context.
- throw error; + throw new Error(`Failed to load ${entityConfig.label.toLowerCase()}`, { cause: error });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (23)
frontend/src/lib/api/utils.ts(1 hunks)frontend/src/lib/components/ChartControls.svelte(2 hunks)frontend/src/lib/components/DriftMetricToggle.svelte(1 hunks)frontend/src/lib/components/EntityTable.svelte(2 hunks)frontend/src/lib/components/ExpandableCell.svelte(1 hunks)frontend/src/lib/components/NavigationBar.svelte(1 hunks)frontend/src/lib/components/PageHeader.svelte(2 hunks)frontend/src/lib/components/SortButton.svelte(2 hunks)frontend/src/lib/job/tree-builder/tree-builder.ts(1 hunks)frontend/src/lib/types/Entity.ts(2 hunks)frontend/src/lib/types/LogicalNode.ts(1 hunks)frontend/src/lib/util/sort.ts(1 hunks)frontend/src/routes/+layout.svelte(1 hunks)frontend/src/routes/[conf]/+page.server.ts(2 hunks)frontend/src/routes/[conf]/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/+layout.svelte(2 hunks)frontend/src/routes/[conf]/[name]/+layout.ts(2 hunks)frontend/src/routes/[conf]/[name]/observability/ObservabilityNavTabs.svelte(1 hunks)frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte(4 hunks)frontend/src/routes/[conf]/[name]/observability/summary/+page.svelte(3 hunks)frontend/src/routes/[conf]/[name]/observability/summary/+page.ts(2 hunks)frontend/src/routes/[conf]/[name]/overview/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/overview/EntityProperties.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- frontend/src/routes/[conf]/[name]/observability/ObservabilityNavTabs.svelte
- frontend/src/routes/[conf]/[name]/observability/summary/+page.ts
- frontend/src/lib/util/sort.ts
- frontend/src/lib/components/ExpandableCell.svelte
- frontend/src/routes/[conf]/[name]/+layout.ts
- frontend/src/lib/components/NavigationBar.svelte
- frontend/src/lib/types/LogicalNode.ts
- frontend/src/routes/[conf]/[name]/overview/+page.svelte
- frontend/src/routes/[conf]/[name]/overview/EntityProperties.svelte
- frontend/src/lib/components/EntityTable.svelte
- frontend/src/lib/components/ChartControls.svelte
- frontend/src/lib/job/tree-builder/tree-builder.ts
- frontend/src/routes/[conf]/+page.svelte
- frontend/src/lib/api/utils.ts
- frontend/src/routes/[conf]/[name]/+layout.svelte
- frontend/src/lib/components/DriftMetricToggle.svelte
- frontend/src/lib/types/Entity.ts
- frontend/src/routes/+layout.svelte
- frontend/src/lib/components/SortButton.svelte
- frontend/src/routes/[conf]/[name]/observability/summary/+page.svelte
- frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (5)
frontend/src/lib/components/PageHeader.svelte (2)
6-7: Props signature updated to include learnHref.Added optional learnHref parameter makes component more flexible.
16-22: Button now uses dynamic linking instead of fixed function.Learn button conditionally renders based on learnHref and opens URL in new tab.
frontend/src/routes/[conf]/+page.server.ts (3)
14-16: Improved approach for config retrieval.Using direct path-based config lookup instead of param matching simplifies the code.
18-20: Better error handling.Clear 404 error when no valid config found.
5-5:✅ Verification successful
Check import path alias.
The
$srcalias is unusual in SvelteKit. Standard is$lib. Verify this alias is properly configured.
🏁 Script executed:
#!/bin/bash # Check for $src alias configuration grep -r '\$src' --include="*.js" --include="*.json" --include="*.ts" --include="*.svelte" .Length of output: 4616
Alias configuration verified – no changes needed.
The
$srcalias is explicitly defined infrontend/svelte.config.js(mapping$srctosrc) and is used consistently across the codebase. There’s no need to change it to$libunless a project-wide standardization is desired.
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
Looks great! Once merged, I'll move the ` into page.svelte
…ight and improved LineChart perf (up to 33%) (#451) ## Summary Fixes close/overlapping series points not properly triggering highlight. Video after update: https://github.com/user-attachments/assets/fc4d8631-1525-4196-81b7-2092e47c58f1 Release also improves performance of Spline/LineCharts by up to 33% from my [testing](techniq/layerchart#418). More work to make the summary/distributions page initial render much faster, but moving in the right direction. This PR also fixes distribution zoom resetting on chart click (similar to drift/feature chart change in #445) ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- av pr metadata This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated the charting component dependency to incorporate minor bug fixes and improvements for enhanced stability. - **Bug Fixes** - Modified the event handling logic for the `PercentileLineChart` component to ensure `xDomain` is assigned directly from event details. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Sean Lynch <[email protected]>
Summary
ActionButtonstoSortButton(only remaining use case)PageHeaderout ofEntityTableand into pageEntity.tsnestingLearnbutton href based on conf/entity type (ex.Joingoes to applicable page for both list and individual entity)DistributionstoSummarytab/route (feedback from meeting, and usesgetColumnSummary()API)Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation