Feat/cli replay cached diagnostics - #86
Conversation
📝 WalkthroughWalkthroughAdds an optional ChangesCLI Diagnostic Replay on Cache Hit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/ba/sake/deder/CoreTasks.scala`:
- Around line 714-716: The location formatting in the replay logic differs from
the live diagnostic formatter in CliServerMessage.scala. In the `location`
assignment around the `if diag.range.startLine > 0` condition, the code
unconditionally includes `startChar` in the formatted string, producing
`file:line:0` when the column is zero. Update the logic to only include the
column in the formatted string when `diag.range.startChar > 0`, matching the
live diagnostic formatter's behavior. This means you need to handle two cases:
when both startLine and startChar are greater than zero (include the column),
and when only startLine is greater than zero (omit the column).
In `@TODO.md`:
- Line 4: Remove the completed TODO item from line 4 of TODO.md. The line
"replay compile failures to CLI too!" describes functionality that is now
implemented in this PR through the cliReplayFn hook in CachedTask and the
.withCliReplay() wiring in CoreTasks.compile, as verified by the integration
test in CachedTaskSuite. Delete this line from the TODO list since it is no
longer a pending task.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e02364f9-621b-4ca7-9d02-dd5e1d9df6f1
⛔ Files ignored due to path filters (2)
config/DederProject.pklis excluded by!**/*.pkldeder.pklis excluded by!**/*.pkl
📒 Files selected for processing (5)
TODO.mdintegration/test/src/ba/sake/deder/CachedTaskSuite.scalaintegration/test/src/ba/sake/deder/IntegrationSuite.scalaplugin-api/src/ba/sake/deder/Task.scalaserver/src/ba/sake/deder/CoreTasks.scala
| val location = if diag.range.startLine > 0 then | ||
| Some(s"${fd.file.path}:${diag.range.startLine}:${diag.range.startChar}") | ||
| else None |
There was a problem hiding this comment.
Location formatting inconsistency with live diagnostics when column is zero.
The replay logic always includes startChar when startLine > 0, producing file:line:0 when the column is zero. The live diagnostic formatter (in CliServerMessage.scala:70-75) only includes the column when both line and column are greater than zero, producing file:line in that case. Users comparing first-run and cached-run output may notice the discrepancy.
🔧 Proposed fix to match live diagnostic format
val location = if diag.range.startLine > 0 then
- Some(s"${fd.file.path}:${diag.range.startLine}:${diag.range.startChar}")
+ if diag.range.startChar > 0 then
+ Some(s"${fd.file.path}:${diag.range.startLine}:${diag.range.startChar}")
+ else
+ Some(s"${fd.file.path}:${diag.range.startLine}")
else None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val location = if diag.range.startLine > 0 then | |
| Some(s"${fd.file.path}:${diag.range.startLine}:${diag.range.startChar}") | |
| else None | |
| val location = if diag.range.startLine > 0 then | |
| if diag.range.startChar > 0 then | |
| Some(s"${fd.file.path}:${diag.range.startLine}:${diag.range.startChar}") | |
| else | |
| Some(s"${fd.file.path}:${diag.range.startLine}") | |
| else None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/ba/sake/deder/CoreTasks.scala` around lines 714 - 716, The
location formatting in the replay logic differs from the live diagnostic
formatter in CliServerMessage.scala. In the `location` assignment around the `if
diag.range.startLine > 0` condition, the code unconditionally includes
`startChar` in the formatted string, producing `file:line:0` when the column is
zero. Update the logic to only include the column in the formatted string when
`diag.range.startChar > 0`, matching the live diagnostic formatter's behavior.
This means you need to handle two cases: when both startLine and startChar are
greater than zero (include the column), and when only startLine is greater than
zero (omit the column).
| - modules in stats history empty? | ||
| - init() plugin in new thread? | ||
|
|
||
| - replay compile failures to CLI too! |
There was a problem hiding this comment.
Remove completed TODO: cache-hit CLI replay is now implemented.
Line 4 lists "replay compile failures to CLI too!" as a TODO, but this PR implements exactly this feature via the cliReplayFn hook in CachedTask and the .withCliReplay() wiring in CoreTasks.compile. Verified by the integration test in CachedTaskSuite that asserts cache-hit compile diagnostics are replayed to the CLI. This TODO should be removed rather than left in the backlog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TODO.md` at line 4, Remove the completed TODO item from line 4 of TODO.md.
The line "replay compile failures to CLI too!" describes functionality that is
now implemented in this PR through the cliReplayFn hook in CachedTask and the
.withCliReplay() wiring in CoreTasks.compile, as verified by the integration
test in CachedTaskSuite. Delete this line from the TODO list since it is no
longer a pending task.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores