Improve profiler artifact diagnostics - #2
Conversation
Add a warnings list to PreparedRegionPresentation and propagate it through prepareRegionPresentation and prepareSessionPresentation. When a re-read CPU profile yields 0 samples but the stored region summary reports a positive count (and no frame filter is active), fall back to the stored summary and emit a warning explaining the mismatch. The presentation JSON now includes an optional 'preparationWarnings' field for comparisons, explanations, inspections, and searches when warnings are present.
- Add enableDtd: bool = true to ProfileAttachRequest so callers can skip the Dart Tooling Daemon when region markers are not needed or when DTD startup times out (common cause of attach failures). - Make DartToolingDaemon? nullable in ProfileSessionContext; registerServices() and postRegionEvent() become no-ops when dtd is null, so whole-session VM-service capture still works without DTD. - profile_runner.attach() conditionally starts the DTD process based on request.enableDtd and records a session warning when DTD is skipped. - Wire --skip-dtd flag into AttachCommand (CLI) and skipDtd boolean into profileAttachTool (MCP), both defaulting to DTD enabled. - Add two integration tests for the prepareRegionPresentation sample-count fallback: one that asserts the stored count and warning text appear when raw CPU samples re-read as 0, and one that asserts no warning is emitted for a healthy profile.
Add the ability to re-read raw memory artifacts and query the full class list, addressing the gap where only the top-N classes from a capture were accessible through the existing CLI/MCP surface. Core (devtools_profiler_core): - memory_profile_summary.dart: add rebuildMemoryProfileFromArtifact and readMemoryClassesFromArtifact, which parse a stored ProfileMemoryArtifact JSON file back into AllocationProfile/ ClassHeapStats objects and call summarizeMemoryProfile with an optional includeClass predicate and configurable topClassCount. - artifacts.dart: add ProfileArtifacts.readMemoryClasses which resolves a session directory, region summary.json, or raw memory_profile.json to the correct raw artifact path, builds an optional class-name / min-live-bytes predicate, and delegates to readMemoryClassesFromArtifact. - profile_runner.dart: expose readMemoryClasses as a public ProfileRunner method. CLI/presentation (devtools_profiler_cli): - models.dart: add PreparedMemoryClassInspection. - preparation.dart: add prepareMemoryClassInspection. - json.dart: add memoryClassInspectionJson. - terminal.dart: add writeMemoryClassInspection with a class table (Class / Library / Live / Live delta / Instances / Inst delta / Alloc delta) and heap delta summary header. - analysis_commands.dart: add InspectClassesCommand (--class, --min-live-bytes, --limit options; examples in help output). - cli.dart: register InspectClassesCommand. - constants.dart: move usageWithExamples here so all command files can share it (removed duplicate from capture_commands.dart). MCP (devtools_profiler_cli): - analysis_tools.dart: add profileInspectClassesTool. - tool_handlers.dart: add profileInspectClasses handler. - server.dart: register profileInspectClassesTool.
…ss-limit The compare command previously only diffed the top-N classes stored in each session summary (default 10, sorted by allocationBytesDelta). Classes with large stable retained size but small allocation delta were invisible to the comparison. Core (devtools_profiler_core): - profile_region_comparison.dart: add baselineMemoryOverride and currentMemoryOverride parameters to compareProfileRegions. When supplied, they replace the stored region.memory for the comparison; otherwise the stored values are used unchanged. CLI/presentation (devtools_profiler_cli): - preparation.dart: add minLiveBytes and memoryClassLimit parameters to prepareProfileComparison. When either is set, attempt to re-read both raw memory artifacts via runner.readMemoryClasses and pass the expanded ProfileMemoryResult objects as overrides; silently fall back to stored classes if the raw artifact is unavailable. Also fix a latent bug where options.frameLimit (CPU frame limit) was used as the memory class limit. - analysis_commands.dart: add --min-live-bytes and --memory-class-limit options to CompareCommand; add usage examples. MCP (devtools_profiler_cli): - analysis_tools.dart: add minLiveBytes and memoryClassLimit to profileCompareTool input schema. - tool_handlers.dart: read and forward both fields in profileCompare.
📝 WalkthroughWalkthroughAdds memory-class inspection (CLI + MCP), memory-class filters/limits for comparisons, an attach-mode option to skip the Dart Tooling Daemon, broader artifact/raw-memory resolution, propagation of preparation warnings into JSON/terminal output, improved memory-summary and reproduction output, better local package-frame detection, and package version bumps to 0.2.0-wip. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/User
participant MCP as MCP Handler
participant Prep as prepareMemoryClassInspection
participant Artifacts as ProfileArtifacts
participant Memory as MemoryProfile
participant JSON as JSON Rendering
CLI->>MCP: Call profileInspectClasses(path, classQuery, minLiveBytes, limit)
MCP->>Prep: prepareMemoryClassInspection(runner, path, classQuery, minLiveBytes, limit)
Prep->>Artifacts: readMemoryClasses(path, classQuery, minLiveBytes, topClassCount)
Artifacts->>Memory: readMemoryClassesFromArtifact(rawPath, predicate, topClassCount)
Memory->>Memory: rebuildMemoryProfileFromArtifact -> filter & top-truncate
Memory-->>Artifacts: ProfileMemoryResult
Artifacts-->>Prep: ProfileMemoryResult
Prep-->>MCP: PreparedMemoryClassInspection
MCP->>JSON: memoryClassInspectionJson(inspection)
JSON-->>MCP: JSON map with cliCommand and warnings
MCP-->>CLI: MCP response with memory-class data
sequenceDiagram
participant CLI as CLI/User
participant AttachCmd as AttachCommand
participant Request as ProfileAttachRequest
participant Runner as ProfileRunner
participant SessionCtrl as ProfileSessionController
participant DTD as DartToolingDaemon
CLI->>AttachCmd: devtools-profiler attach --skip-dtd
AttachCmd->>Runner: runner.attach(ProfileAttachRequest(enableDtd=false))
Runner->>Runner: resolve working/artifact dirs
alt enableDtd true
Runner->>DTD: start DtdProcessSession()
DTD-->>Runner: dtdSession
Runner->>SessionCtrl: Create with dtd=daemon
else enableDtd false
Runner->>SessionCtrl: Create with dtd=null
end
SessionCtrl->>SessionCtrl: registerServices()
alt dtd != null
SessionCtrl->>DTD: register profiler services
else dtd == null
SessionCtrl->>SessionCtrl: return early (no region markers)
end
Runner-->>CLI: Attach session started (whole-session capture)
sequenceDiagram
participant CLI as CLI/User
participant Compare as CompareCommand
participant Prep as prepareProfileComparison
participant Artifacts as ProfileArtifacts
participant CompareImpl as compareProfileRegions
participant Output as Renderer
CLI->>Compare: devtools-profiler compare --min-live-bytes 1000 --memory-class-limit 25
Compare->>Prep: prepareProfileComparison(..., minLiveBytes=1000, memoryClassLimit=25, memoryClassLimitSpecified=true)
Prep->>Artifacts: readMemoryClasses(baselinePath, minLiveBytes=1000, topClassCount=25)
Artifacts-->>Prep: baselineMemoryOverride
Prep->>Artifacts: readMemoryClasses(currentPath, minLiveBytes=1000, topClassCount=25)
Artifacts-->>Prep: currentMemoryOverride
Prep->>CompareImpl: compareProfileRegions(..., baselineMemoryOverride, currentMemoryOverride)
CompareImpl->>CompareImpl: build memory comparison using overrides
CompareImpl-->>Prep: comparison result with memory rows
Prep-->>Compare: PreparedProfileComparison (includes warnings)
Compare->>Output: render comparison, memory-class table, and warnings
Output-->>CLI: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e7f194260
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR improves artifact and diagnostic output across the profiler core + CLI/MCP layers, with a focus on better “what happened / how do I reproduce it” guidance and richer post-run inspection of stored artifacts (including memory-class inspection and additional comparison filtering).
Changes:
- Expanded artifact reading/summarization to accept per-profile artifact directories and added path normalization for artifact outputs.
- Added additional warnings + reproduction/
cliCommandhints to terminal and JSON/MCP outputs; added attach-mode “skip DTD” support. - Added stored-artifact memory class inspection utilities and exposed them via a new CLI command (
inspect-classes) and MCP tool (profile_inspect_classes).
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devtools_profiler_core/test/profile_runner_test.dart | Updates attach warning expectation and adds coverage for summarizing per-profile artifact directories. |
| packages/devtools_profiler_core/test/profile_frames_test.dart | Adds tests for ProfileFrame.packageName resolution across package/file URI variants. |
| packages/devtools_profiler_core/pubspec.yaml | Bumps core package version to 0.2.0-wip. |
| packages/devtools_profiler_core/lib/src/memory/memory_profile_summary.dart | Adds helpers to rebuild/read memory-class summaries from raw artifacts with optional filtering. |
| packages/devtools_profiler_core/lib/src/cpu/profile_frames.dart | Extends packageName detection to handle local package file://.../<pkg>/lib/... paths. |
| packages/devtools_profiler_core/lib/src/capture/runner/profile_session_region_rpc.dart | Guards DTD event posting when DTD is disabled. |
| packages/devtools_profiler_core/lib/src/capture/runner/profile_session_controller.dart | Makes DTD optional and skips service registration when disabled. |
| packages/devtools_profiler_core/lib/src/capture/runner/profile_session_context.dart | Makes DTD optional in session context and documents attach-mode behavior. |
| packages/devtools_profiler_core/lib/src/capture/profile_runner.dart | Normalizes working/artifact dirs, adds attach enableDtd, and adds memory-class artifact reading API. |
| packages/devtools_profiler_core/lib/src/capture/profile_attach_request.dart | Adds enableDtd toggle for attach sessions. |
| packages/devtools_profiler_core/lib/src/capture/artifacts.dart | Supports resolving/summarizing artifacts from directories and adds memory-class artifact resolution/reading. |
| packages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dart | Adds optional memory overrides to comparisons (for re-read/filtered memory comparisons). |
| packages/devtools_profiler_core/CHANGELOG.md | Documents 0.2.0-wip changes for core. |
| packages/devtools_profiler_cli/test/cli_test.dart | Updates attach test expectations and adds coverage for filter-removes-all and sample-count fallback warnings. |
| packages/devtools_profiler_cli/pubspec.yaml | Bumps CLI version to 0.2.0-wip and updates dependency constraint on core. |
| packages/devtools_profiler_cli/lib/src/rendering/terminal.dart | Adds reproduction block, warning surfacing, and memory class inspection rendering; improves memory summary columns. |
| packages/devtools_profiler_cli/lib/src/rendering/methods.dart | Surfaces preparation warnings alongside method inspection/search/comparison output. |
| packages/devtools_profiler_cli/lib/src/rendering/helpers.dart | Adds POSIX-style shellJoin/shellQuote helpers for reproducible commands. |
| packages/devtools_profiler_cli/lib/src/presentation/preparation.dart | Aggregates preparation warnings, adds memory-class inspection preparation, and adds sample-count/filter warnings + fallback. |
| packages/devtools_profiler_cli/lib/src/presentation/options.dart | Adds “active filter” detection + user-facing labels for filters. |
| packages/devtools_profiler_cli/lib/src/presentation/models.dart | Adds prepared models for memory class inspection and per-region preparation warnings. |
| packages/devtools_profiler_cli/lib/src/presentation/json.dart | Adds cliCommand hints and preparation warnings to JSON output; adds JSON shape for memory class inspection. |
| packages/devtools_profiler_cli/lib/src/mcp/tools/capture_tools.dart | Adds skipDtd option to attach tool schema. |
| packages/devtools_profiler_cli/lib/src/mcp/tools/analysis_tools.dart | Adds memory-class compare filters to schema and introduces profile_inspect_classes tool. |
| packages/devtools_profiler_cli/lib/src/mcp/tool_handlers.dart | Wires new MCP arguments (skip DTD, memory filters) and implements profileInspectClasses. |
| packages/devtools_profiler_cli/lib/src/mcp/server.dart | Registers the new MCP tool. |
| packages/devtools_profiler_cli/lib/src/cli/constants.dart | Centralizes usageWithExamples helper. |
| packages/devtools_profiler_cli/lib/src/cli/commands/capture_commands.dart | Adds attach warnings + --skip-dtd flag and examples; uses shared usageWithExamples. |
| packages/devtools_profiler_cli/lib/src/cli/commands/artifact_commands.dart | Propagates preparation warnings into summarize output (terminal + JSON). |
| packages/devtools_profiler_cli/lib/src/cli/commands/analysis_commands.dart | Adds compare memory filters and introduces inspect-classes CLI command. |
| packages/devtools_profiler_cli/lib/src/cli.dart | Registers inspect-classes command. |
| packages/devtools_profiler_cli/CHANGELOG.md | Documents 0.2.0-wip changes for CLI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dart (1)
40-42:⚠️ Potential issue | 🟡 MinorMemory-availability warning ignores the new overrides.
The warning check on line 40 uses only the original
baseline.memoryandcurrent.memoryfields, but the memory comparison logic at lines 79–96 correctly respects the newbaselineMemoryOverrideandcurrentMemoryOverrideparameters via the fallback patternoverride ?? original. This creates an inconsistency: if a caller provides an override to fill missing memory data, the user will still see the "Memory data was only available for one compared profile" warning because the check inspects only the stored memory results.To align the warning with the override semantics, extract the effective memory values once and use them in both the warning check and the switch:
♻️ Proposed adjustment
+ final effectiveBaselineMemory = baselineMemoryOverride ?? baseline.memory; + final effectiveCurrentMemory = currentMemoryOverride ?? current.memory; - if ((baseline.memory == null) != (current.memory == null)) { + if ((effectiveBaselineMemory == null) != (effectiveCurrentMemory == null)) { warnings.add('Memory data was only available for one compared profile.'); }…and use those locals in the switch statement below.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dart` around lines 40 - 42, The warning currently checks raw baseline.memory and current.memory and thus ignores overrides; update the logic in the comparison routine so you compute effectiveMemoryBaseline = baselineMemoryOverride ?? baseline.memory and effectiveMemoryCurrent = currentMemoryOverride ?? current.memory (or similar local names) before the warning and switch, then use those locals both for the initial presence check (the warning that memory is only available for one profile) and inside the existing switch block that compares memory; this keeps the warning consistent with the override semantics used by the memory comparison code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools_profiler_cli/lib/src/mcp/tool_handlers.dart`:
- Around line 622-656: The local variable named `path` inside the
`profileInspectClasses` closure shadows the imported `package:path/path.dart as
path`; rename that local `final path = _requiredStringArgument(...)` to `final
targetPath` (consistent with `prepareMemoryClassInspection` usage elsewhere) and
update its use when calling `prepareMemoryClassInspection` so the code passes
`targetPath` instead of `path`, preventing accidental shadowing of the `path`
library.
- Around line 633-637: The MCP tool profileInspectClasses is using
_treeLimitFromArgument with defaultFrameLimit (12) which diverges from
ProfileArtifacts.readMemoryClasses' default (50); update the call in
profileInspectClasses to use a class-specific default (e.g.,
defaultMemoryClassLimit = 50) instead of defaultFrameLimit so omitted limit
matches the core API, or explicitly pass null/omit the default so
ProfileArtifacts.readMemoryClasses applies its own default; adjust the symbol
_treeLimitFromArgument invocation and introduce/rename the constant
(defaultMemoryClassLimit) near profileInspectClasses to make the intent clear.
In `@packages/devtools_profiler_cli/lib/src/presentation/json.dart`:
- Around line 249-264: _compareCliCommand and _inspectClassesCliCommand
currently omit memory-class flags because PreparedProfileComparison and
PreparedMemoryClassInspection don't carry the memory-class options; update the
preparer types to retain the relevant options (e.g., add memoryClassLimit and
minLiveBytes to PreparedProfileComparison and topClassCount/limit to
PreparedMemoryClassInspection consistent with preparation.dart's topClassCount)
and then modify _compareCliCommand and _inspectClassesCliCommand to emit the
corresponding flags (--memory-class-limit, --min-live-bytes, --limit and any
other memory-class options) when those fields are set so the generated
cliCommand fully reproduces the original invocation (also apply the same change
for the other similar CLI-emitting block around lines 366-380).
In `@packages/devtools_profiler_cli/lib/src/presentation/preparation.dart`:
- Around line 453-472: The PreparedMemoryClassInspection currently drops the
topClassCount used in readMemoryClasses; add a topClassCount (int) property to
the PreparedMemoryClassInspection model, set it in prepareMemoryClassInspection
(pass the topClassCount through when constructing
PreparedMemoryClassInspection), and update the CLI reproduction code paths
(e.g., memoryClassInspectionJson / _inspectClassesCliCommand) to read that
model.topClassCount and emit the corresponding --limit/--topClassCount flag when
it differs from the default so the generated cliCommand faithfully reproduces
the original invocation.
- Around line 516-550: The two warning branches use different checks for "filter
active" (options.hasActiveFrameFilters vs options.framePredicate == null) which
is inconsistent; update the fallback condition that currently checks
options.framePredicate == null to instead use !options.hasActiveFrameFilters (or
the positive/negative of hasActiveFrameFilters you prefer) so both branches use
the same source of truth (options.hasActiveFrameFilters) — change the
conditional guarding the rebuiltRegion.sampleCount==0 && region.sampleCount>0
branch to reference options.hasActiveFrameFilters and keep assigning
regionForSummary = _filterStoredRegion(region, options) when appropriate.
- Around line 107-133: The catch blocks around runner.readMemoryClasses in
preparation.dart currently swallow errors; update them to capture the thrown
exception (and its message) when readMemoryClasses fails for baselineRawPath or
currentRawPath and append a clear, contextual warning string to the prepared
session/comparison warnings collection (the same warnings plumbing used for
preparationWarnings and terminal "Warnings" output) so that
baselineMemoryOverride/currentMemoryOverride fallbacks are reported; ensure you
reference readMemoryClasses, baselineMemoryOverride, currentMemoryOverride,
minLiveBytes and memoryClassLimit in the warning text so consumers know which
override failed.
In `@packages/devtools_profiler_cli/lib/src/rendering/methods.dart`:
- Around line 259-267: The bullet list can show duplicate messages because you
simply concatenate comparison.warnings,
comparison.baseline.presentation.warnings and
comparison.current.presentation.warnings; change the construction of warnings to
deduplicate while preserving order (e.g., iterate those three lists, push into a
Set-backed seen collection and only add unseen entries to the final list) and
then pass the de-duplicated list to
console.section/console.components.bulletList so duplicate warning strings (like
identical preparation/region warnings) are only shown once.
In `@packages/devtools_profiler_cli/lib/src/rendering/terminal.dart`:
- Around line 567-643: The three duplicated helpers (_sessionCliCommand,
_isAttachSession, _durationOptionForSession) and inconsistent shell-joining
should be consolidated into a single shared helper file (e.g., add
cli_command.dart or export them from helpers.dart) and both terminal.dart and
json.dart should import and call those shared functions instead of defining
private copies; update terminal.dart to remove its local
_sessionCliCommand/_isAttachSession/_durationOptionForSession and use the shared
names, ensure the shared helper uses the canonical shellJoin implementation
(replace json.dart’s private _shellJoin if needed), and adjust imports/usages so
the terminal "Profiler command" and JSON cliCommand are produced by the same
code.
In `@packages/devtools_profiler_cli/README.md`:
- Around line 183-194: The README example for the inspect-classes command uses a
CPU/method-table-looking class name ("Parser") which may mislead readers; update
the example invocation of inspect-classes to use a memory/heap-oriented class
name such as "--class _List" or "--class String" (and keep the rest of the flags
like "--json --min-live-bytes 1048576 /path/to/session") so the example clearly
communicates that --class filters heap memory class names; modify the sample
block that shows the inspect-classes usage and replace "Parser" with one of
these heap-oriented names.
In `@packages/devtools_profiler_cli/test/cli_test.dart`:
- Around line 1104-1128: In the test "inspect-classes prints json memory class
output" add an assertion that the JSON includes the emitted CLI command so the
round-trip of flags is verified; specifically assert json['cliCommand'] contains
the reconstructed command with the flags passed to _runJsonCommand (for example
that it includes "devtools-profiler inspect-classes --class Love
--min-live-bytes 512 --limit 1 /tmp/memory/session-1" or at minimum contains the
"--class Love", "--min-live-bytes 512" and "--limit 1" tokens) to ensure the
runner/_runJsonCommand propagation is tested and prevent regressions in the CLI
provenance emitted by the code that builds the cliCommand.
In `@packages/devtools_profiler_core/lib/src/capture/artifacts.dart`:
- Around line 217-272: _resolveRawMemoryPath currently treats a session.json
file as an unsupported artifact because the file-branch only recognizes
ProfileMemoryArtifact and region shapes; detect a session JSON shape by trying
ProfileRunResult.fromJson(map) (or checking for a "regions" key) when the input
is a file and, if found, extract overallProfile?.memory?.rawProfilePath and
return it (throw the same StateError if missing/empty); ensure this uses the
same error message as the directory-branch; alternatively, if you prefer reuse,
convert the file path to its parent directory and call the existing
directory-branch logic (e.g., delegate to readSession or
_sessionFileFor/_summaryFileFor) so session.json paths are resolved consistently
in _resolveRawMemoryPath.
- Around line 198-208: Replace the three-branch predicate construction with a
single closure: keep the same variables (ProfileMemoryClassPredicate? predicate,
classQuery, minLiveBytes) but compute a normalized query (e.g., final query =
classQuery?.toLowerCase().trim()) and then set predicate = (s) => (query == null
|| query.isEmpty || s.className.toLowerCase().contains(query)) && (minLiveBytes
== null || s.liveBytes >= minLiveBytes); this removes duplicated checks while
preserving the original filtering logic in functions/methods that reference
predicate.
- Around line 99-120: summarizeArtifact currently checks _sessionFileFor,
_summaryFileFor, and _rawCpuFileFor but does not mirror readArtifact's fallback
to _rawMemoryFileFor, causing memory-only artifact directories to throw "No
profiler summary found in directory"; update summarizeArtifact to also check
_rawMemoryFileFor(targetPath) and if it exists call
summarizeArtifact(rawMemoryFile.path) so memory-only directories are handled the
same as readArtifact (alternatively, if you prefer to disallow summarizing
memory-only dirs, replace the thrown ArgumentError message with "Memory-only
artifact directories are not summarizable; pass the memory_profile.json path
directly" and keep behavior unchanged).
In `@packages/devtools_profiler_core/lib/src/cpu/profile_frames.dart`:
- Around line 87-108: The local-package fallback in _packageNameFromFilePath
uses lastIndexOf('lib') which can pick a nested lib directory; change it to
locate the first 'lib' segment (e.g., use indexOf('lib') starting from 0 or
search for the first 'lib' whose parent index > 0) and then use
segments[libIndex - 1] as packageDirectoryName, keeping the same validations
(non-empty and not path.separator) and returning
_packageNameFromPubCacheFolder(packageDirectoryName) as before; ensure behavior
mirrors the pub-cache branch that uses indexOf('lib', pubCacheIndex).
In `@packages/devtools_profiler_core/lib/src/memory/memory_profile_summary.dart`:
- Around line 78-83: Add a one-line doc comment to
rebuildMemoryProfileFromArtifact (and mirror in readMemoryClassesFromArtifact if
present) that explains the intentional asymmetry in defaults: these functions
default topClassCount to 50 for deep-dive/inspect-classes artifact-rebuild use,
whereas summarizeMemoryProfile defaults to 10 for brief summaries, so callers
aren’t surprised by the different default behavior.
- Around line 78-108: The parsing in rebuildMemoryProfileFromArtifact
unconditionally casts rawArtifact['start'], rawArtifact['end'], and their nested
'heapSample' which throws on malformed/truncated artifacts; update
rebuildMemoryProfileFromArtifact to validate that rawArtifact contains non-null
Map entries for 'start' and 'end' and that each snapshot contains a non-null Map
'heapSample' (use runtime type checks like `is Map` before calling .cast), and
if any check fails throw a FormatException that includes rawProfilePath (or
return a clear typed error) so callers can handle diagnostics gracefully; keep
using _extractClassStats and HeapSample.fromJson after these guards.
In `@packages/devtools_profiler_core/test/profile_frames_test.dart`:
- Around line 16-81: Tests constructing ProfileFrame instances use path.join and
Uri.file which produce platform-dependent (Windows) paths; update each test that
builds a POSIX-style package layout (the four tests verifying packageName:
"packageName resolves pub-cache file URIs", "packageName resolves local package
file URIs", "packageName strips version suffixes from file package folders", and
"packageName ignores file URIs outside package layouts") to use
path.posix.join(...) and call Uri.file(..., windows: false) for the location
argument so the created URIs are pinned to POSIX semantics and the packageName
parsing remains platform-independent.
---
Outside diff comments:
In
`@packages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dart`:
- Around line 40-42: The warning currently checks raw baseline.memory and
current.memory and thus ignores overrides; update the logic in the comparison
routine so you compute effectiveMemoryBaseline = baselineMemoryOverride ??
baseline.memory and effectiveMemoryCurrent = currentMemoryOverride ??
current.memory (or similar local names) before the warning and switch, then use
those locals both for the initial presence check (the warning that memory is
only available for one profile) and inside the existing switch block that
compares memory; this keeps the warning consistent with the override semantics
used by the memory comparison code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 763f63f9-6630-463a-bc40-faf065be7f3d
📒 Files selected for processing (37)
README.mdpackages/devtools_profiler_cli/CHANGELOG.mdpackages/devtools_profiler_cli/README.mdpackages/devtools_profiler_cli/lib/src/cli.dartpackages/devtools_profiler_cli/lib/src/cli/commands/analysis_commands.dartpackages/devtools_profiler_cli/lib/src/cli/commands/artifact_commands.dartpackages/devtools_profiler_cli/lib/src/cli/commands/capture_commands.dartpackages/devtools_profiler_cli/lib/src/cli/constants.dartpackages/devtools_profiler_cli/lib/src/cli/options.dartpackages/devtools_profiler_cli/lib/src/mcp/server.dartpackages/devtools_profiler_cli/lib/src/mcp/tool_handlers.dartpackages/devtools_profiler_cli/lib/src/mcp/tools/analysis_tools.dartpackages/devtools_profiler_cli/lib/src/mcp/tools/capture_tools.dartpackages/devtools_profiler_cli/lib/src/presentation/json.dartpackages/devtools_profiler_cli/lib/src/presentation/models.dartpackages/devtools_profiler_cli/lib/src/presentation/options.dartpackages/devtools_profiler_cli/lib/src/presentation/preparation.dartpackages/devtools_profiler_cli/lib/src/rendering/helpers.dartpackages/devtools_profiler_cli/lib/src/rendering/methods.dartpackages/devtools_profiler_cli/lib/src/rendering/terminal.dartpackages/devtools_profiler_cli/pubspec.yamlpackages/devtools_profiler_cli/test/cli_test.dartpackages/devtools_profiler_cli/test/mcp_server_test.dartpackages/devtools_profiler_core/CHANGELOG.mdpackages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dartpackages/devtools_profiler_core/lib/src/capture/artifacts.dartpackages/devtools_profiler_core/lib/src/capture/profile_attach_request.dartpackages/devtools_profiler_core/lib/src/capture/profile_runner.dartpackages/devtools_profiler_core/lib/src/capture/runner/profile_session_context.dartpackages/devtools_profiler_core/lib/src/capture/runner/profile_session_controller.dartpackages/devtools_profiler_core/lib/src/capture/runner/profile_session_region_rpc.dartpackages/devtools_profiler_core/lib/src/cpu/profile_frames.dartpackages/devtools_profiler_core/lib/src/memory/memory_profile_summary.dartpackages/devtools_profiler_core/pubspec.yamlpackages/devtools_profiler_core/test/profile_frames_test.dartpackages/devtools_profiler_core/test/profile_runner_test.dartskills/devtools-profiler-local/SKILL.md
📜 Review details
⏰ 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 packages/devtools_profiler_core
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~647-~647: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mory summaries show retained growth. 6. Use profile_compare or `profile_find_regr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (18)
packages/devtools_profiler_cli/lib/src/cli/options.dart (1)
165-179: LGTM.
parseNonNegativeIntcorrectly distinguishes itself fromparseLimit(which collapses0tonull) by preserving0for callers that want unlimited semantics expressed as a literal integer. Error message matchesparseLimitfor consistent UX.packages/devtools_profiler_core/lib/src/capture/runner/profile_session_context.dart (1)
20-25: LGTM.Nullable
dtdwith the contract that whole-session VM-service capture continues to work is consistent with the call sites inprofile_runner.dart(attach branch passesdtdSession?.daemon), the null-guard inprofile_session_controller.registerServices, and the early-return inpostRegionEvent.packages/devtools_profiler_core/lib/src/capture/runner/profile_session_region_rpc.dart (1)
268-295: LGTM — DTD null-guard correctly placed.The early return is before the try block so no spurious "Failed to post … event to DTD" warning is generated for sessions started with
enableDtd: false.postRegionErrorEventstill appends itsRegion "…" failed: …warning via.then, preserving error visibility when DTD is disabled.packages/devtools_profiler_core/lib/src/capture/runner/profile_session_controller.dart (1)
49-71: LGTM.Single early-exit on
context.dtd == nullkeeps the fourregisterServicecalls cleanly guarded; the!accesses are unconditionally safe within this scope.packages/devtools_profiler_cli/lib/src/rendering/helpers.dart (1)
165-183: LGTM — POSIX-shell quoting is correct.
'\''escape pattern is the canonical way to embed a single quote inside a single-quoted string, and the empty-string case correctly emits''. Note that the resulting strings target POSIX shells (bash/zsh);cmd.exedoes not treat'…'as quoting, but this matches the rest of the CLI's reproduction-command UX.packages/devtools_profiler_cli/lib/src/presentation/options.dart (1)
75-122: LGTM.
hasActiveFrameFiltersis the union of all four input sources, and using it to short-circuitframePredicatekeeps behavior identical to the prior locally-computed gate while also giving the renderer/JSON layers a consistent description set (activeFrameFilterDescriptions/activeFrameFilterLabel) for the new reproduction blocks.packages/devtools_profiler_cli/pubspec.yaml (1)
25-25: Verifydevtools_region_profilerconstraint.PR objectives bump changed profiler packages to
0.2.0-wipand the validation list includesdevtools_region_profiler. If itspubspec.yamlwas bumped, this dev_dependency^0.1.0should be updated for workspace consistency. (Verified by the script attached on the core pubspec review.)packages/devtools_profiler_cli/lib/src/mcp/server.dart (1)
68-68: LGTM.Tool registration mirrors the surrounding pattern; both
profileInspectClassesTool(analysis_tools.dart) andhandlers.profileInspectClasses(tool_handlers.dart) are present.packages/devtools_profiler_cli/lib/src/cli.dart (1)
45-45: LGTM.Wiring matches the existing analysis-command pattern.
packages/devtools_profiler_cli/lib/src/cli/commands/artifact_commands.dart (1)
76-87: LGTM.Region-branch wiring of
prepared.warningsmatches the newregionPresentationJson/writeRegionSummarysignatures introduced elsewhere in the PR.packages/devtools_profiler_cli/lib/src/mcp/tools/capture_tools.dart (1)
112-117: LGTM.Schema mirrors the CLI
--skip-dtdflag, and the handler intool_handlers.dartcorrectly inverts it intoenableDtd.packages/devtools_profiler_cli/lib/src/cli/commands/capture_commands.dart (1)
11-14: LGTM.Warning text is accurate for attach mode in general, and the
!(skip-dtd)→enableDtdinversion lines up with theProfileAttachRequestdefault ofenableDtd: true. The non-negatabledefaultsTo: falseflag makes theas boolcast safe.Also applies to: 201-208
packages/devtools_profiler_core/pubspec.yaml (1)
5-23: Thedevtools_profiler_protocolconstraint is correct. Verification shows the protocol package remains at version0.1.0and was not bumped to0.2.0-wip, so the^0.1.0constraint on line 16 is consistent with the workspace dependencies.packages/devtools_profiler_cli/lib/src/cli/constants.dart (1)
17-28: LGTM.The helper cleanly trims and appends a stable examples section. No issues found.
packages/devtools_profiler_core/test/profile_runner_test.dart (1)
440-510: LGTM.The directory-summarization test correctly populates
overall/cpu_profile.jsonandoverall/summary.json, then verifiessummarizeArtifactresolves the directory and round-trips theProfileRegionResult. The expectations onregionId,rawProfilePath, and the top self frame name align with the artifact-resolution branch insummarizeArtifact.packages/devtools_profiler_core/lib/src/capture/profile_attach_request.dart (1)
13-13: LGTM.Adding
enableDtdas an optional parameter withtruedefault keeps existing call sites compatible and gives attach mode a safety valve when DTD startup is problematic.Also applies to: 29-34
packages/devtools_profiler_cli/README.md (1)
138-139: Documentation aligns with the new flags and tools.
cliCommand,--min-live-bytes,--memory-class-limit, andprofile_inspect_classesaccurately reflect the CLI/MCP additions in this PR.Also applies to: 233-236, 273-273
packages/devtools_profiler_core/CHANGELOG.md (1)
3-17: LGTM.Changelog entry accurately summarizes the artifact resolution, attach
--skip-dtd, memory-class inspection/filters, and local package frame detection introduced in this PR.
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools_profiler_cli/lib/src/cli/commands/analysis_commands.dart`:
- Around line 45-53: Update the usage examples returned by formatUsage (the
override that calls usageWithExamples) to include a sample demonstrating the
--memory-class-limit flag; specifically add an entry like 'devtools-profiler
compare --memory-class-limit 0 path/to/baseline path/to/current' to the const
list of examples so the compare command shows the "unlimited" semantics
alongside the existing --min-live-bytes example.
In `@packages/devtools_profiler_cli/lib/src/presentation/models.dart`:
- Around line 38-41: PreparedProfileComparison's constructor currently forces
callers to pass minLiveBytes and memoryClassLimit; make those parameters
nullable (remove required and give them a nullable type/default null) so callers
aren't forced to thread values, and either (a) remove memoryClassLimitSpecified
and derive it as memoryClassLimit != null inside PreparedProfileComparison (if
you accept losing the "explicitly set to null" distinction), or (b) keep
memoryClassLimitSpecified but still make minLiveBytes/memoryClassLimit nullable;
update the constructor signature and any call sites (notably
prepareProfileComparison) to stop passing the removed/optional args and adjust
logic that relied on memoryClassLimitSpecified accordingly.
In `@packages/devtools_profiler_cli/lib/src/presentation/options.dart`:
- Around line 100-108: The getters framePredicate currently builds
excludePrefixes and includePrefixes before checking hasActiveFrameFilters,
causing unnecessary list allocations; move the construction of excludePrefixes
(which uses excludePackages and hideRuntimeHelpers/runtimeHelperPackagePrefixes)
and includePrefixes (includePackages) to after the if (!hasActiveFrameFilters)
return null check so the lists are only allocated when needed, keeping the
early-return fast and avoiding extra allocations.
In `@packages/devtools_profiler_cli/lib/src/presentation/preparation.dart`:
- Around line 165-177: compareProfileRegions is being called with
memoryClassLimit set to options.frameLimit when memoryClassLimitSpecified is
false, which unintentionally couples CPU frame limits and memory class limits;
update the call site to either compute and pass an explicit
defaultMemoryClassLimit (e.g., defaultMemoryClassLimit =
memoryClassLimitSpecified ? memoryClassLimit : someSeparateDefault) or add a
clear code comment where compareProfileRegions is invoked explaining that using
options.frameLimit as the default memoryClassLimit is intentional and mirrors
prepareProfileTrends; refer to the symbols compareProfileRegions,
memoryClassLimitSpecified, memoryClassLimit, options.frameLimit, and
prepareProfileTrends to locate and implement the change.
- Around line 130-138: The fallback warning is ambiguous for CPU-only artifacts
because it only checks region.memory?.rawProfilePath; update the logic in the
readMemoryClasses/baselineMemoryOverride branch to distinguish when
region.memory is entirely missing vs when region.memory exists but
rawProfilePath is null/empty: if region.memory == null push a message saying “no
memory capture was performed (artifact is CPU-only); --min-live-bytes requires a
memory capture” (include minLiveBytes and memoryLimitDescription for context),
else if region.memory != null && (rawProfilePath == null ||
rawProfilePath.isEmpty) keep the existing “no raw memory artifact path was
stored” message. Ensure you update references to memoryWarnings and the existing
message composition to preserve formatting.
In `@packages/devtools_profiler_cli/lib/src/rendering/terminal.dart`:
- Around line 291-298: The merged warnings list is built by concatenating
explanation.hotspots.warnings and explanation.target.presentation.warnings and
can contain duplicates; change the code that constructs or before printing the
warnings (the local variable warnings used in this block that calls
console.section and console.components.bulletList) to pass the combined list
through the existing uniqueWarnings utility (same approach used in
writeComparisonSummary and writeMethodComparison) so duplicate
preparation/hotspot warnings are removed before calling
console.components.bulletList.
In `@packages/devtools_profiler_cli/README.md`:
- Around line 233-236: Update the README flag descriptions to clarify units and
scope: state that `--min-live-bytes <n>` takes a value in bytes (e.g., 1048576 =
1 MiB) and update `--memory-class-limit <n>` to indicate it controls the number
of compared memory class rows for both `compare` and `inspect-classes` (with `0`
meaning unlimited) so the behavior and units are clear for both flags.
In `@packages/devtools_profiler_cli/test/cli_test.dart`:
- Around line 1306-1333: The negative assertion in the test "no warning emitted
when raw CPU profile produces non-zero samples" uses a different substring than
the positive case; update the isNot(contains(...)) call so it uses the same
substring as the positive assertion (e.g. '0 samples when re-read' or the full
phrase used in preparation.dart) to make the positive and negative checks
symmetric; locate the failing expectation in the test (the isNot(contains(...))
at the end of the test) and replace its argument to match the other assertion's
substring.
- Around line 1271-1304: Rename the test group title string to reflect the
end-to-end CLI behavior being tested: change the group invocation that currently
reads group('prepareRegionPresentation sample-count fallback', ...) to something
like group('run sample-count fallback warnings', ...) so the grouping reflects
that the test exercises the run CLI (runCli) with _FakeRunnerWithZeroSamples and
asserts on stdout rather than directly testing prepareRegionPresentation.
- Around line 1192-1198: Update the test 'inspect-classes rejects negative
min-live-bytes' to pass _FakeMemoryProfileRunner instead of relying on the
default runner; specifically, when calling _runCliCommand in that test provide
_FakeMemoryProfileRunner() to mirror other inspect-classes tests and ensure the
runner used implements readMemoryClasses correctly (avoid _FakeProfileRunner) so
future moves of validation won't mask regressions.
In `@packages/devtools_profiler_core/lib/src/capture/artifacts.dart`:
- Around line 41-62: The directory-probing logic for resolving artifacts is
duplicated across readArtifact, summarizeArtifact, and _resolveRawMemoryPath;
extract a single helper (e.g. _resolveDirectoryArtifact) that, given a directory
path plus parameters like missingMessage and a recurse callback, performs the
ordered checks using the existing helpers _sessionFileFor, _summaryFileFor,
_rawCpuFileFor, and _rawMemoryFileFor (check session → summary → rawCpu →
rawMemory) and either returns the resolved result or throws
ArgumentError.value(targetPath, 'targetPath', missingMessage). Replace the
duplicated sequences in readArtifact, summarizeArtifact, and
_resolveRawMemoryPath to call this helper and pass the appropriate recurse
behavior and error text so all resolution logic is centralized.
- Around line 220-237: The current _resolveRawMemoryPath treats a directory that
only contains a CPU-only artifact as "Artifact not found"; update the branch in
_resolveRawMemoryPath so after checking _sessionFileFor, _summaryFileFor, and
_rawMemoryFileFor you also check if _rawCpuFileFor(targetPath).existsSync() and,
if true, throw an ArgumentError.value using the memory-missing diagnostic used
elsewhere (e.g. "No memory profile is available for the session/region at \"…\".
Re-run the target with memory capture enabled."); this preserves the existing
session/summary/raw-memory checks (session via readSession and
_rawMemoryPathForSession) but ensures CPU-only dirs produce the precise
memory-missing message rather than a generic "Artifact not found".
In `@packages/devtools_profiler_core/lib/src/cpu/profile_frames.dart`:
- Around line 87-108: The _packageNameFromFilePath function can misidentify
non-package lib/ paths and Windows drive roots as package names; update the
guard around packageDirectoryName (used after computing libIndex and
packageDirectoryName) to also treat a Windows drive root as invalid by comparing
packageDirectoryName to path.rootPrefix(packageDirectoryName) (i.e., return null
if packageDirectoryName.isEmpty || packageDirectoryName == path.separator ||
packageDirectoryName == path.rootPrefix(packageDirectoryName)); also add a unit
test covering a non-package lib path such as "/usr/local/lib/foo.dart" that
asserts _packageNameFromFilePath returns null to prevent local directories like
"local" or "var" being treated as package names.
In `@packages/devtools_profiler_core/lib/src/memory/memory_profile_summary.dart`:
- Around line 170-171: The current cast of profiles uses (snapshot['profiles']
as List<Object?>? ?? const []).cast<Map<Object?, Object?>>(), which still throws
a TypeError if profiles exists but is not a List; update the extraction to
explicitly check the runtime type (e.g., if (snapshot['profiles'] is List) {
final raw = snapshot['profiles'] as List; ... } else if
(snapshot.containsKey('profiles')) throw FormatException(..., rawProfilePath); )
so it mirrors how start/end are handled and aligns with _requiredArtifactMap
validation; use rawProfilePath in the FormatException message and then
cast/convert the List entries to Map<Object?, Object?> as before.
- Around line 142-144: Replace the unsafe top-level cast of the JSON payload
with an explicit validation step: assign the result of jsonDecode(await
File(rawProfilePath).readAsString()) to a local (e.g., decoded), then pass
decoded and the rawProfilePath into the existing _requiredArtifactMap helper to
obtain the Map<Object?, Object?> (so a malformed array/scalar/null produces the
same FormatException with the artifact path). Update references that used the
previous `json` variable to use the validated map returned by
_requiredArtifactMap.
In `@packages/devtools_profiler_core/test/profile_artifacts_test.dart`:
- Around line 57-73: Add tests to cover the other required nested labels checked
by _requiredArtifactMap when calling rebuildMemoryProfileFromArtifact: assert
that FormatException messages include 'end', 'start.heapSample', and
'end.heapSample' in addition to 'start'. You can parameterize the existing test
or add separate cases that call rebuildMemoryProfileFromArtifact with artifacts
missing each of those keys (and rawProfilePath set like
'/tmp/malformed_memory_profile.json') and use
throwsA(isA<FormatException>().having((e) => e.message, 'message',
contains('<label>'))) to verify the message contains the specific missing label.
In `@packages/devtools_profiler_core/test/profile_comparison_test.dart`:
- Around line 304-322: Add a new test that covers the asymmetric memory-override
case for compareProfileRegions: call compareProfileRegions with baseline:
_region(regionId: 'baseline') and current: _region(regionId: 'current') where
both regions have memory: null, supply only baselineMemoryOverride:
_memoryBaseline() (leave currentMemoryOverride null) and assert that
comparison.memory is still non-null but comparison.warnings contains the "Memory
data was only available for one compared profile." message; this mirrors the
existing test but uses only one override to ensure the warning is emitted when
overrides are not symmetric.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aed0a36d-e318-44d5-9ca0-739df6aa9a4f
📒 Files selected for processing (22)
packages/devtools_profiler_cli/README.mdpackages/devtools_profiler_cli/lib/src/cli/commands/analysis_commands.dartpackages/devtools_profiler_cli/lib/src/mcp/tool_handlers.dartpackages/devtools_profiler_cli/lib/src/presentation.dartpackages/devtools_profiler_cli/lib/src/presentation/cli_command.dartpackages/devtools_profiler_cli/lib/src/presentation/json.dartpackages/devtools_profiler_cli/lib/src/presentation/models.dartpackages/devtools_profiler_cli/lib/src/presentation/options.dartpackages/devtools_profiler_cli/lib/src/presentation/preparation.dartpackages/devtools_profiler_cli/lib/src/rendering/helpers.dartpackages/devtools_profiler_cli/lib/src/rendering/methods.dartpackages/devtools_profiler_cli/lib/src/rendering/terminal.dartpackages/devtools_profiler_cli/test/cli_test.dartpackages/devtools_profiler_cli/test/mcp_server_test.dartpackages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dartpackages/devtools_profiler_core/lib/src/capture/artifacts.dartpackages/devtools_profiler_core/lib/src/cpu/profile_frames.dartpackages/devtools_profiler_core/lib/src/memory/memory_profile_summary.dartpackages/devtools_profiler_core/test/profile_artifacts_test.dartpackages/devtools_profiler_core/test/profile_comparison_test.dartpackages/devtools_profiler_core/test/profile_frames_test.dartskills/devtools-profiler-local/SKILL.md
📜 Review details
⏰ 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 packages/devtools_profiler_core
🔇 Additional comments (19)
packages/devtools_profiler_core/test/profile_frames_test.dart (1)
1-117: Test coverage and platform-pinning look good.POSIX pinning via
path.posix.join+Uri.file(..., windows: false)is applied consistently, and the cases (package URI, pub-cache, local package, nested-libprecedence, version stripping, non-matching layouts) exercise the behavior contract documented for_packageNameFromFilePath. Consider adding one negative case for a "looks like a path but isn't a package" layout (e.g.,/var/lib/foo.dart) to lock in the chosen trade-off — see the related comment onprofile_frames.dart.packages/devtools_profiler_core/lib/src/memory/memory_profile_summary.dart (1)
79-80: Doc note for default-asymmetry resolved.The new doc comment explicitly calling out the 50 vs. 10 default split between
rebuildMemoryProfileFromArtifact/readMemoryClassesFromArtifactandsummarizeMemoryProfileaddresses the prior concern.packages/devtools_profiler_core/lib/src/capture/artifacts.dart (2)
195-218: Predicate composition simplified — past comment addressed.The single-closure form with
normalizedQueryandminLiveBytesshort-circuits cleanly and resolves the prior verbosity concern.
251-274: Session-JSON file target now supported — past comment addressed.The new
'regions'pattern at line 263 routes session JSON file inputs through_rawMemoryPathForSession, eliminating the prior dead-end'Unsupported artifact type'error for session.json file paths.packages/devtools_profiler_cli/README.md (1)
183-194: Heap-class example resolved — past comment addressed.The example now uses
--class String, which clearly reads as a memory-class filter rather than a CPU/method-table query.packages/devtools_profiler_core/lib/src/analysis/profile_region_comparison.dart (1)
18-95: Override-aware effective memory threading is consistent.Both the availability warning at line 42 and the switch at line 81 use
effectiveBaselineMemory/effectiveCurrentMemory, so a caller-provided override correctly drives both branches and the empty-fallback timestamps. Behavior matches the new test andprepareProfileComparisonplumbing.packages/devtools_profiler_cli/lib/src/rendering/helpers.dart (1)
172-178: LGTM.Order-preserving de-dup via
seen.add(...)is idiomatic and O(n).packages/devtools_profiler_cli/lib/src/presentation.dart (1)
1-1: LGTM.Public re-export aligns with the new
cliCommandJSON output surface.packages/devtools_profiler_cli/lib/src/rendering/methods.dart (1)
259-267: LGTM — addresses prior dedup concern.The merged warning render now uses
uniqueWarnings(...)to dedupe acrosscomparison.warnings,baseline.presentation.warnings, andcurrent.presentation.warnings. Single-source renders inwriteMethodInspection/writeMethodSearchcorrectly skip the dedup wrapper since there is nothing to merge.packages/devtools_profiler_cli/lib/src/mcp/tool_handlers.dart (1)
622-656: LGTM — both past review concerns addressed.
profileInspectClassesnow usestargetPath(no library shadowing) and defaults todefaultMemoryClassLimit(50) so MCP behavior matches the CLI/ProfileArtifacts.readMemoryClassesdefault. Thelimit ?? 0translation correctly forwards 0 as "unlimited" totopClassCount, and_optionalNonNegativeIntArgumentrejects negativeminLiveBytes(covered by the new test).packages/devtools_profiler_cli/lib/src/presentation/json.dart (1)
227-250: LGTM — reproducible commands now carry memory-class options.
_compareCliCommandemits--min-live-bytesand--memory-class-limit(usingmemoryClassLimitSpecifiedto distinguish "user passed 0" from "omitted"), and_inspectClassesCliCommandemits--class,--min-live-bytes, and--limitwhenever the value differs fromdefaultMemoryClassLimit. This addresses the prior round-trip concern and matches the documented behavior inSKILL.md.One small note: when the user passes
--limit 50explicitly, the regeneratedcliCommandwill omit--limit(because it equals the default). That is functionally equivalent on re-run; flagging only as a heads-up since it is a minor round-trip lossiness.Also applies to: 352-370
packages/devtools_profiler_cli/test/cli_test.dart (1)
1145-1175:cliCommandround-trip assertion forinspect-classesis now covered.This addresses the previously-flagged gap: the JSON output's
cliCommandis now asserted to round-trip--class,--min-live-bytes, and--limitexactly.packages/devtools_profiler_cli/lib/src/presentation/models.dart (1)
180-205:topClassCountis now persisted on the model.The earlier concern that
PreparedMemoryClassInspectiondiscardedtopClassCount(preventing--limitfrom being reproduced incliCommand) is resolved — the field is stored andcli_test.dartasserts its round-trip into the JSONcliCommand.packages/devtools_profiler_cli/lib/src/presentation/preparation.dart (4)
108-163: Memory-override read failures are now warned, not silently swallowed.The previously-flagged silent fallback is fixed:
readMemoryClassesfailures (and missingrawProfilePath) now emit explicit warnings that propagate intopreparationWarningsand into the JSONcliCommandconsumers.cli_test.dartLines 957–989 lock this in.
543-585: Filter-active checks are now consistent onhasActiveFrameFilters.The earlier inconsistency between
hasActiveFrameFiltersandframePredicate == nullis resolved — all three branches in this block now key offoptions.hasActiveFrameFilters, so they share a single source of truth.
1-2: Thevm_serviceimport is necessary and should be kept.
CpuSamplesis used in the file's own function signature at line 622 (_countCpuSamplesBeforeFilters(CpuSamples cpuSamples)) and is not re-exported fromdevtools_profiler_core's public API. Whiledevtools_profiler_coreinternally usesvm_service, it does not exposeCpuSamplesthrough its exports, so the direct import is required for type annotations in this presentation layer.> Likely an incorrect or invalid review comment.
487-507: MCP entry point properly wrapsprepareMemoryClassInspectionerrors; CLI relies on framework-level handling.Unlike
prepareProfileComparison(Lines 116–129), this helper doesn't try/catch aroundrunner.readMemoryClasses. The MCP tool handler wraps the call via_runTool's try/catch (tool_handlers.dart:658–692), returning a structuredCallToolResultwithisError: trueon failure. The CLI'srunClientry point (cli.dart:46–51) also wraps command execution in a catch-all that prints errors to stderr and returnssoftwareExitCode. Both approaches handle exceptions appropriately for their contexts, though MCP provides more structured error information.packages/devtools_profiler_cli/lib/src/cli/commands/analysis_commands.dart (2)
322-397: No action needed.defaultMemoryClassLimitis properly defined inpackages/devtools_profiler_cli/lib/src/presentation/options.dart(line 13) and is imported via../options.dart. The code will compile without issues.
375-387: The defaults are already aligned and consistent. BothreadMemoryClassesand the CLI's--limitoption use50as the default (defaultMemoryClassLimit = 50), and the help text correctly documents that0means unlimited. There is no divergent behavior to address.> Likely an incorrect or invalid review comment.
Summary
summarizeand resolve run/attach artifact paths before reporting them.cliCommandhints in JSON/MCP output.cliCommandprofile selectors and memory-class flags copyable for artifact targets, region-scoped trends, comparisons, andinspect-classes.inspect-classesin CLI/MCP tests.session.jsontargets, malformed raw memory payloads, and override-aware comparison warnings.0.2.0-wip.Validation
dart format .dart analyze .dart test packages/devtools_profiler_protocoldart test packages/devtools_region_profilerdart test packages/devtools_profiler_coredart test packages/devtools_profiler_cligit diff --checkSummary by CodeRabbit
New Features
Enhancements
Tests / Docs