feat: Adding Timeline Style Registry - #625
Conversation
kyasbal
added
the
area:backend-core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the KHI v6 file format implementation, including container reading/writing, chunk generation with size-based splitting, and a global style registry for timeline metadata. The review identified several critical improvements: converting mutable global variables to constants for the file format header, mitigating potential memory exhaustion (OOM) by limiting decompression size in the reader, and addressing a missing field in the style chunk generation that is necessary for UI rendering.
| var ( | ||
| // MagicBytes are the first 3 bytes of a KHI file ("KHI"). | ||
| MagicBytes = []byte{'K', 'H', 'I'} | ||
| // Version is the supported version of the KHI file format. | ||
| Version = byte(0x06) | ||
|
|
||
| ErrInvalidMagicBytes = errors.New("invalid magic bytes") | ||
| ErrUnsupportedVersion = errors.New("unsupported version") | ||
| ) |
There was a problem hiding this comment.
The MagicBytes and Version are defined as global variables, which makes them mutable. Since these are fundamental constants of the file format, they should be defined as constants to prevent accidental modification at runtime. In Go, MagicBytes can be defined as a string constant and still be indexed like a byte slice.
| var ( | |
| // MagicBytes are the first 3 bytes of a KHI file ("KHI"). | |
| MagicBytes = []byte{'K', 'H', 'I'} | |
| // Version is the supported version of the KHI file format. | |
| Version = byte(0x06) | |
| ErrInvalidMagicBytes = errors.New("invalid magic bytes") | |
| ErrUnsupportedVersion = errors.New("unsupported version") | |
| ) | |
| const ( | |
| // MagicBytes are the first 3 bytes of a KHI file ("KHI"). | |
| MagicBytes = "KHI" | |
| // Version is the supported version of the KHI file format. | |
| Version = byte(0x06) | |
| ) | |
| var ( | |
| ErrInvalidMagicBytes = errors.New("invalid magic bytes") | |
| ErrUnsupportedVersion = errors.New("unsupported version") | |
| ) |
| } | ||
| defer gr.Close() | ||
|
|
||
| uncompressed, err := io.ReadAll(gr) |
There was a problem hiding this comment.
Using io.ReadAll on a gzip.Reader without a size limit can lead to memory exhaustion (OOM) if the input is a "zip bomb" or contains excessively large decompressed data. Since KHI chunks are expected to be within a reasonable size (e.g., around the 64MB Protobuf limit), consider using io.LimitReader to bound the decompression to a safe maximum.
| func GenerateChunk() *pb.TimelineStyleChunk { | ||
| mu.RLock() | ||
| defer mu.RUnlock() | ||
|
|
||
| // Create shallow copies of the slices to avoid concurrent modification issues | ||
| // if someone decides to append while another is iterating over the chunk. | ||
| cSeverities := make([]*pb.Severity, len(severities)) | ||
| copy(cSeverities, severities) | ||
|
|
||
| cVerbs := make([]*pb.Verb, len(verbs)) | ||
| copy(cVerbs, verbs) | ||
|
|
||
| cLogTypes := make([]*pb.LogType, len(logTypes)) | ||
| copy(cLogTypes, logTypes) | ||
|
|
||
| cRevisionStates := make([]*pb.RevisionState, len(revisionStates)) | ||
| copy(cRevisionStates, revisionStates) | ||
|
|
||
| cTimelineTypes := make([]*pb.TimelineType, len(timelineTypes)) | ||
| copy(cTimelineTypes, timelineTypes) | ||
|
|
||
| return &pb.TimelineStyleChunk{ | ||
| Severities: cSeverities, | ||
| Verbs: cVerbs, | ||
| LogTypes: cLogTypes, | ||
| RevisionStates: cRevisionStates, | ||
| TimelineTypes: cTimelineTypes, | ||
| } | ||
| } |
There was a problem hiding this comment.
The GenerateChunk function is missing the IconAtlas field in the returned TimelineStyleChunk. The IconAtlas is essential for rendering icons in the UI (e.g., for revision states). The registry should include a mechanism to register or set the IconAtlas so it can be included in the generated style chunk for global consistency.
542ac24 to
f6fdf8f
Compare
1 participant
Overview
This PR introduces a thread-safe
StyleRegistryfor the next-generation KHI file format (v6), replacing the hardcodedpkg/model/enumapproach. This allows KHI plugins to dynamically register UI styles and ensures global consistency.Key Changes
pkg/model/khifile/v6/style/timeline.gowith thread-safe slice storage and monotonic ID assignment.TimelineType,Severity,Verb,LogType, andRevisionState.GenerateChunk()to collect all registered styles into a singleTimelineStyleChunkready for serialization.Testing