Feat/render #25
Feat/render #25
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Graph.ProviderTypes and populates it via a new Dix.providerGraphTypes() DOT renderer; replaces several funk/log calls with logger; adds an example stub file; and updates module dependency versions in root and example go.mod files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant API as API.Graph()
participant Dix as Dix
participant Renderer as DotRenderer
Caller->>API: Graph()
API->>Dix: providerGraph() %% existing
Dix-->>API: Providers DOT/structure
API->>Dix: providerGraphTypes() [new]
Dix->>Renderer: init + render type edges
Renderer-->>Dix: DOT (provider types)
Dix-->>API: ProviderTypes DOT
API-->>Caller: Graph{Objects, Providers, ProviderTypes}
sequenceDiagram
autonumber
participant Dix as Dix.providerGraphTypes()
participant Renderer as DotRenderer
participant Providers as x.providers
note over Dix,Renderer: Build DOT of type-level edges
Dix->>Renderer: Init header + cluster "providers"
loop for each provider in Providers
Dix->>Dix: if output empty: continue
alt input is struct
Dix->>Dix: getProvideAllInputs() -> types
Dix->>Dix: lo.Uniq/lo.Map -> unique input types
Dix->>Renderer: RenderEdge(input_type -> output_type)
else non-struct
Dix->>Renderer: RenderEdge(input_type -> output_type)
end
end
Dix->>Renderer: Close subgraph
Renderer-->>Dix: DOT string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Summary of Changes
Hello @kooksee, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the Dix dependency injection framework by adding the ability to render and visualize the relationships between provider types. It provides a structured output in DOT format, making it easier to understand and debug complex dependency graphs. Additionally, the changes include a refactor of the internal logging mechanism for better maintainability and updates to various project dependencies.
Highlights
- New Provider Type Graphing: Introduced a new
providerGraphTypesmethod that generates a DOT language representation of the provider type dependencies within the Dix container. This enables visualization of the dependency injection graph. - Graph Structure Enhancement: The
Graphstruct now includes aProviderTypesfield to expose the newly generated provider type graph data. - Logging Refactor: Transitioned from direct calls to the global
logpackage to using a localloggerinstance across multiple internal Dix files, improving logging consistency and control. - Dependency Updates: Updated various Go module dependencies, including
github.com/pubgo/funkand addinggithub.com/go-echarts/go-echarts/v2to the example module, hinting at future visualization capabilities.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to render a dependency graph of provider types, which is a valuable addition for visualizing dependencies. The implementation also includes a consistent refactoring from a global logger to a package-level logger, improving code structure. My review focuses on the new rendering logic in dixinternal/renderer.go, where I've identified opportunities to reduce code duplication and simplify complexity. Additionally, please note the large number of dependency updates; it's important to ensure these have been thoroughly tested to prevent any regressions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
dixinternal/util.go (2)
65-81: Fix panics from calling IsNil on non-nilable reflect.Values.IsNil panics for non-nilable kinds (e.g., int, struct). Several checks here can panic at runtime.
Apply these diffs to guard IsNil with a kind check:
- val := providerOutTyp.MapIndex(k) - if !val.IsValid() || val.IsNil() { + val := providerOutTyp.MapIndex(k) + if isNilValue(val) { continue } @@ - for i := 0; i < val.Len(); i++ { + for i := 0; i < val.Len(); i++ { vv := val.Index(i) - if !vv.IsValid() || vv.IsNil() { + if isNilValue(vv) { continue } @@ - for i := 0; i < providerOutTyp.Len(); i++ { + for i := 0; i < providerOutTyp.Len(); i++ { val := providerOutTyp.Index(i) - if !val.IsValid() || val.IsNil() { + if isNilValue(val) { continue } @@ - if providerOutTyp.IsValid() && !providerOutTyp.IsNil() { - rr[outType][defaultKey] = []value{providerOutTyp} - } + if providerOutTyp.IsValid() && !(isNilableKind(providerOutTyp.Kind()) && providerOutTyp.IsNil()) { + rr[outType][defaultKey] = []value{providerOutTyp} + }Add these helpers (outside this hunk):
// isNilableKind reports whether k supports IsNil. func isNilableKind(k reflect.Kind) bool { switch k { case reflect.Chan, reflect.Func, reflect.Map, reflect.Pointer, reflect.Slice, reflect.Interface, reflect.UnsafePointer: return true default: return false } } // isNilValue safely checks for nil reflect.Value across kinds. func isNilValue(v reflect.Value) bool { return !v.IsValid() || (isNilableKind(v.Kind()) && v.IsNil()) }Also applies to: 89-96, 110-116
171-177: Correct type check: use inTyp instead of typ.The current guard checks the outer typ, not the field type, which can omit or include unsupported fields incorrectly.
- inTyp := inTye.Field(j).Type - if !isSupportedType(typ) { + inTyp := inTye.Field(j).Type + if !isSupportedType(inTyp) { continue } - input = append(input, getProvideAllInputs(inTyp)...) + input = append(input, getProvideAllInputs(inTyp)...)dixinternal/provider.go (1)
60-61: Avoid potential nil deref when logging output type.n.output or n.output.typ may be nil; String() would panic.
- Any("output_type", n.output.typ.String()). + Any("output_type", safeOutputTypeString(n.output)).Add helper (outside this hunk):
func safeOutputTypeString(p *providerOutputType) string { if p == nil || p.typ == nil { return "<nil>" } return p.typ.String() }
🧹 Nitpick comments (8)
example/_.go (1)
3-3: Consider renaming file and adding a package doc comment.A bare underscore filename can be confusing. For discoverability, prefer doc.go with a short package comment and optionally link to go-callvis usage.
-// https://github.com/ondrajz/go-callvis +// Package example contains demo code. See https://github.com/ondrajz/go-callvis for call graphs.dixinternal/util.go (2)
187-187: Tighten log formatting and variable name in message.Use %v for reflect types/kinds to avoid formatting surprises; also align key with actual var name.
- logger.Error().Msgf("incorrect input type, inTyp=%s kind=%s", inTye, inTye.Kind()) + logger.Error().Msgf("incorrect input type, typ=%v kind=%v", inTye, inTye.Kind())
33-38: Harden reflectValueToString against Interface panics and preallocate.Unexported or non-interfaceable values can panic on Interface(). Also preallocate for fewer allocs.
-func reflectValueToString(values []reflect.Value) []string { - var data []string - for i := range values { - data = append(data, fmt.Sprintf("%#v", values[i].Interface())) - } - return data -} +func reflectValueToString(values []reflect.Value) []string { + data := make([]string, 0, len(values)) + for i := range values { + v := values[i] + if !v.IsValid() { + data = append(data, "<invalid>") + continue + } + if v.CanInterface() { + data = append(data, fmt.Sprintf("%#v", v.Interface())) + } else { + data = append(data, v.Type().String()) + } + } + return data +}example/go.mod (1)
12-14: Let go mod manage indirects; tidy after changes.Explicitly pinning indirect deps in example is usually unnecessary unless required. Recommend a tidy to normalize.
#!/bin/bash # From example/ directory ( cd example && go mod tidy )dixinternal/api.go (1)
34-37: Add a flag to opt out of ProviderTypes graph renderingProviderTypes is correctly wired in the Graph struct (json:"provider_types") and rendered via providerGraphTypes(); consider adding a flag or option to skip ProviderTypes when generating heavy DOT outputs.
go.mod (1)
7-28: Normalize Go directive and tidy modules
- Update the
godirective in go.mod from1.23.0to1.23.- Run
go mod tidyto align go.sum with current deps and drop any unused// indirectentries.dixinternal/renderer.go (2)
73-109: Avoid duplication of the DOT preamble; extract a helper to keep styles in sync.This block duplicates providerGraph(). Factor into a helper to prevent drift and ease tweaks (fonts, colors, arrows).
Example helper (outside this range):
func writeDotPreamble(d *DotRenderer) { d.writef("digraph G {") d.writef(` layout=dot; rankdir=LR; overlap=false; splines=true; nodesep=0.5; ranksep=1.0; concentrate=true; node [shape=box, style=filled, fillcolor="#F9F9F9", color="#666666", fontsize=8, fontname="Arial", width=0.1, height=0.1, fixedsize=false]; edge [arrowhead=vee, arrowsize=0.4, color="#888888", penwidth=0.5]; `) }Then call writeDotPreamble(d) here and in providerGraph().
111-134: Deterministic, de-duplicated edges for reproducible graphs.Map iteration order is random; duplicate edges can inflate output. Collect, dedupe, sort, then render.
Apply:
- for providerOutputType, nodes := range x.providers { + type edge struct{ from, to string } + edges := make([]edge, 0, 256) + seen := make(map[string]struct{}) + for providerOutputType, nodes := range x.providers { if providerOutputType.String() == "" { continue } for _, n := range nodes { for _, in := range n.inputList { if in.typ.Kind() == reflect.Struct { inTypes := lo.Uniq(lo.Map(getProvideAllInputs(in.typ), func(item *providerInputType, index int) reflect.Type { return item.typ })) for _, inType := range inTypes { - if inType.String() == "" { + if inType == nil || inType.String() == "" { continue } - d.RenderEdge(inType.String(), providerOutputType.String(), nil) + from, to := inType.String(), providerOutputType.String() + k := from + "->" + to + if _, ok := seen[k]; ok { + continue + } + seen[k] = struct{}{} + edges = append(edges, edge{from, to}) } } else { - if in.typ.String() == "" { + if in.typ == nil || in.typ.String() == "" { continue } - d.RenderEdge(in.typ.String(), providerOutputType.String(), nil) + from, to := in.typ.String(), providerOutputType.String() + k := from + "->" + to + if _, ok := seen[k]; !ok { + seen[k] = struct{}{} + edges = append(edges, edge{from, to}) + } } } } } - d.EndSubgraph() - d.writef("}") - return d.String() + // Stable order + sort.Slice(edges, func(i, j int) bool { + if edges[i].to == edges[j].to { + return edges[i].from < edges[j].from + } + return edges[i].to < edges[j].to + }) + for _, e := range edges { + d.RenderEdge(e.from, e.to, nil) + } + + d.EndSubgraph() + d.writef("}") + return d.String()Add import (outside this range):
import "sort"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
dixinternal/aaa.go(1 hunks)dixinternal/api.go(1 hunks)dixinternal/dix.go(2 hunks)dixinternal/provider.go(1 hunks)dixinternal/renderer.go(1 hunks)dixinternal/util.go(1 hunks)example/_.go(1 hunks)example/go.mod(1 hunks)go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dixinternal/renderer.go (2)
dixinternal/dix.go (1)
Dix(44-49)dix.go (1)
Dix(16-16)
🔇 Additional comments (5)
dixinternal/util.go (2)
21-29: Guard against empty value slices when building map.values[len(values)-1] panics if values is empty. If empties are possible upstream, add a check.
- // The last value as the default value - val := values[len(values)-1] + // The last value as the default value + if len(values) == 0 { + continue + } + val := values[len(values)-1]If empties are impossible by contract, consider an assert with a clear error to aid debugging instead.
193-210: Preallocation for outer map looks good.Initializing graph[outTyp] upfront reduces rehash during edge insertion. Nice touch.
dixinternal/provider.go (1)
54-62: Switch to central logger looks good.Consistent logging via the shared logger is an improvement.
example/go.mod (1)
3-3: Confirm Go toolchain alignment across modules and CI
Both the root and examplego.modfiles declarego 1.23.0. No GitHub workflows (actions/setup-go) or Dockerfiles pinning a Go image version were found—please verify your CI pipelines and any container images are using Go 1.23.0 to avoid build mismatches.dixinternal/aaa.go (1)
24-27: Graph.ProviderTypes addition looks good.Clear JSON tag; non-breaking for consumers tolerant of extra fields.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/go.mod (1)
3-3: Fixgodirective: use major.minor only (not patch).
go 1.23.0is invalid; thegodirective must bego 1.23. If you want to pin a patch/toolchain, add a separatetoolchaindirective.-go 1.23.0 +go 1.23 +toolchain go1.23.0
🧹 Nitpick comments (1)
example/go.mod (1)
13-30: Dependency refresh LGTM; add tidy/vulncheck to avoid drift.Since these are indirects pulled via the root module, consider:
- Run
go mod tidy -compat=1.23in example/ to prune stale indirects.- Run
govulncheck ./...to catch advisories acrossgrpc,protobuf,zerolog,x/*, etc.golang.org/x/expmoves fast; keep builds reproducible by relying on the pinned pseudo-version and re-tidying regularly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
example/go.mod(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🔇 Additional comments (2)
example/go.mod (2)
5-5: Keep the replace directive dev-only.
replace github.com/pubgo/dix v0.3.20 => ../is great for local iterating, but ensure it doesn’t get published or used in consumers’ builds (CI should build without thisreplace).
9-9: Bump to funk v0.5.68 looks good.No compatibility concerns from this example module’s perspective.
Summary by CodeRabbit