feat: add layer visualization table #179
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new LayerVisualization component to display the size distribution of container image layers. It also adds a Updated class diagram for TagModelclassDiagram
class TagModel {
+static create(imageId: number, name: string, digest: string): number
+static getById(id: number): Tag | null
+static getByImageId(imageId: number): Tag[]
+static findByName(imageId: number, name: string): Tag | undefined
+static saveMetadata(tagId: number, metadata: TagMetadata): void
+static getWithMetadata(tagId: number): TagWithMetadata | undefined
+static delete(id: number): void
+static deleteByDigest(digest: string): void
+static countByImage(imageId: number): number
+static clear(): void
}
class Tag {
id: number
imageId: number
name: string
digest: string
createdAt: Date
}
class TagMetadata {
created: string
os: string
architecture: string
author: string
dockerFile: string
exposedPorts: string[]
totalSize: string
workDir: string
command: string
description: string
contentDigest: string
entrypoint: string
indexDigest: string
isOCI: boolean
layers: ImageLayer[]
}
class ImageLayer {
size: number
digest: string
}
class TagWithMetadata extends Tag {
metadata: TagMetadata
}
TagModel -- Tag : returns
TagModel -- TagWithMetadata : returns
TagWithMetadata *-- TagMetadata : has
TagMetadata *-- ImageLayer : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kmendell - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a database migration to populate the
layerscolumn for existing tags. - The
parseJSONandparseCommandOrEntrypointfunctions are used in multiple places; consider creating a dedicated utility module for them.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!result) return undefined; | ||
|
|
||
| // Parse JSON fields and format data | ||
| static getWithMetadata(tagId: number): TagWithMetadata | undefined { |
There was a problem hiding this comment.
issue (complexity): Consider using a single join query to fetch tag and metadata, which simplifies the code and reduces database interactions, unless fine-grained logging or separate handling of missing metadata is required, in which case, consider using a debug flag for development-only logging to keep the production path cleaner..
The refactor’s decomposition of the metadata lookup into two separate queries (versus the original single join) and the numerous debug logs do add extra branching. If you don’t require the fine‑grained logging or separate handling of missing metadata, you could simplify by reverting to a single join query—while retaining the extra layers field parsing. For example:
static getWithMetadata(tagId: number): TagWithMetadata | undefined {
const result = db.prepare(
`SELECT
t.id,
t.image_id,
t.name,
t.digest,
t.created_at,
tm.created_at as meta_created,
tm.os as meta_os,
tm.architecture as meta_architecture,
tm.author as meta_author,
tm.dockerFile as meta_dockerFile,
tm.exposedPorts as meta_exposedPorts,
tm.totalSize as meta_totalSize,
tm.workDir as meta_workDir,
tm.command as meta_command,
tm.description as meta_description,
tm.contentDigest as meta_contentDigest,
tm.entrypoint as meta_entrypoint,
tm.isOCI as meta_isOCI,
tm.indexDigest as meta_indexDigest,
tm.layers as meta_layers
FROM tags t
LEFT JOIN tag_metadata tm ON tm.tag_id = t.id
WHERE t.id = ?`
).get(tagId) as any;
if (!result) return undefined;
return {
id: result.id,
imageId: result.image_id,
name: result.name,
digest: result.digest,
createdAt: new Date(result.created_at),
metadata: result.meta_created ? {
created: result.meta_created,
os: result.meta_os,
architecture: result.meta_architecture,
author: result.meta_author,
dockerFile: result.meta_dockerFile,
exposedPorts: parseJSON(result.meta_exposedPorts, []),
totalSize: result.meta_totalSize,
workDir: result.meta_workDir,
command: result.meta_command,
description: result.meta_description,
contentDigest: result.meta_contentDigest,
entrypoint: result.meta_entrypoint,
isOCI: result.meta_isOCI === 1,
indexDigest: result.meta_indexDigest,
layers: parseJSON(result.meta_layers, [])
} : undefined
};
}This approach avoids a second database round-trip and reduces extra logging unless needed. If the additional logging is useful only during development, consider wrapping it in a debug flag to keep the production path cleaner.
| export const TagModel = { | ||
| // Helper function to parse JSON safely - moved to the top to avoid circular imports | ||
| export function parseJSON<T>(str: string | null, defaultValue: T): T { | ||
| if (!str) return defaultValue; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!str) return defaultValue; | |
| if (!str) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| // Helper to parse command/entrypoint | ||
| export function parseCommandOrEntrypoint(value: string | null): string | string[] | null { | ||
| if (!value) return null; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!value) return null; | |
| if (!value) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| static getWithMetadata(tagId: number): TagWithMetadata | undefined { | ||
| const tag = db.prepare('SELECT * FROM tags WHERE id = ?').get(tagId) as TagRecord | undefined; | ||
|
|
||
| if (!tag) return undefined; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!tag) return undefined; | |
| if (!tag) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new layer visualization feature for Docker image metadata, including database schema updates and UI components. Key changes include adding tests for layer visualization, updating API types and database models to support layer data, and refactoring the TagModel to static methods with additional helper functions.
Reviewed Changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/registry.spec.ts | Adds end-to-end tests for layer visualization rendering and error handling when layer data is missing |
| src/routes/details/[repo]/[image]/[tag]/+page.server.ts | Updates the load function to include parsing of layer JSON data from the database |
| src/lib/utils/api/manifest.ts | Extracts and maps layer information from Docker manifest responses |
| src/lib/types/db/records.ts | Renames metadata fields for consistency and adds a new "layers" field |
| src/lib/types/db/models.ts | Updates the TagMetadata interface to include required fields and the new layer information |
| src/lib/types/api/manifest.ts | Defines the ImageLayer interface and enhances TagMetadata with optional layer data |
| src/lib/services/database/models/tag.ts | Refactors TagModel into a class with static methods and adds JSON parsing for layer data |
| src/lib/services/database/migrations.ts | Adds a migration to include layer information in the tag_metadata table |
| src/lib/services/database/index.ts | Provides default metadata values including an empty layers array when none is present |
| src/lib/components/ui/progress/index.ts | Introduces a new progress UI component |
| .github/workflows/linter.yml & e2e-tests.yml | Changes the CI configuration to use self-hosted runners and adjusts dependency cache paths |
Files not reviewed (3)
- src/lib/components/docker-metadata/LayerVisualization.svelte: Language not supported
- src/lib/components/ui/progress/progress.svelte: Language not supported
- src/routes/details/[repo]/[image]/[tag]/+page.svelte: Language not supported
Comments suppressed due to low confidence (2)
src/lib/services/database/models/tag.ts:155
- The assignment of configDigest is sourced from metadata.contentDigest. If the intention is to use a dedicated configuration digest (as done in other parts of the code), verify whether this should instead reference a distinct field (e.g. metadata.configDigest) or derive from tag.digest.
configDigest: metadata.contentDigest || ''
src/lib/types/db/models.ts:29
- The change making all TagMetadata fields non-optional could lead to runtime issues if the database returns null values for any of these fields. Consider ensuring that defaults are provided or updating the type definitions to reflect possible nulls.
export interface TagMetadata {
Summary by Sourcery
Add layer visualization feature for Docker image metadata, including database schema updates and UI components to display layer composition
New Features:
Enhancements:
Build:
Tests: