Don't trim newlines when reading system prompt markdown files#380
Don't trim newlines when reading system prompt markdown files#380
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes modify system prompt markdown parsing to preserve leading and trailing whitespace in content by removing the Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/patches/systemPrompts.ts`:
- Around line 35-40: The stringifyRegex function is losing regex flags because
it trims the string before extracting flags; fix by parsing regex.toString()
once, find the last '/' index (e.g., lastSlash =
regex.toString().lastIndexOf('/')), extract the pattern as substring(1,
lastSlash) and the flags as substring(lastSlash + 1), JSON.stringify both, and
return them so the original flags (like "si") are preserved; update the logic in
stringifyRegex to use these extracted values instead of truncating before
reading flags.
🧹 Nitpick comments (2)
src/systemPromptSync.ts (1)
83-100: Docstring still references trimmed content.
The line-offset comment mentionsparsed.content.trim(), but the function now preserves raw content. Update the comment so the semantics stay accurate for future readers.🔧 Suggested tweak
- // (starting at 1 for the first line of `parsed.content.trim()`) back to + // (starting at 1 for the first line of `parsed.content`) back tosrc/patches/systemPrompts.ts (1)
126-134: Align baseline length calc with untrimmed prompt content.
Now that prompt content preserves leading/trailing newlines, trimming the reconstructed baseline can misreport character deltas (e.g., flagging a newline-only “change”). Consider removing.trim()or trimming both sides consistently.🔧 Suggested tweak
- const originalBaselineContent = reconstructContentFromPieces( - pieces, - identifiers, - identifierMap - ).trim(); + const originalBaselineContent = reconstructContentFromPieces( + pieces, + identifiers, + identifierMap + );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdsrc/patches/systemPrompts.tssrc/systemPromptSync.tssrc/tests/systemPromptSync.test.ts
🔇 Additional comments (4)
CHANGELOG.md (1)
14-14: Changelog entry aligns with the fix.
Captures the behavior change clearly.src/tests/systemPromptSync.test.ts (3)
22-88: Parse tests now capture the preserved leading newline.
Good coverage for the whitespace-preserving change.
483-505: Read-path expectations updated consistently.
Looks aligned with the new parse behavior.
897-1007: Interpolated content assertions align with newline preservation.
These tests now correctly capture the leading newline for both the$$case and the BUILD_TIME placeholder.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Many prompts begin with one or more newlines by default, so we were unintentionally modifying all of those by removing that trailing newline. That's also the cause of this error:
...because there are actually two of those prompts, one with a specific prefix, and one with out it, and when the one without the prefix was updated, as all prompts are automatically, a newline from it was removed, but the replacement was global and so affected the prompt with the prefix since it was strictly a superset.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.