refactor: package code organization wave 9#2274
Open
gsxdsm wants to merge 2 commits into
Open
Conversation
Behavior-preserving package organization after #2252: - app/api/global-and-pi-settings.ts and task-content.ts from legacy.ts - types/messages.ts (ParticipantType + Message contracts) - merger-diff-volume-gate: resolveDiffVolumeGateSettings + formatFindings Ratchet legacy/types/merger line-count ceilings downward.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR modularizes core message contracts, adds dashboard clients for settings and task content, preserves legacy API exports, and extracts diff-volume gate utilities from the merger module. ChangesCore message contracts
Dashboard API modules
Engine diff-volume utilities
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR reorganizes several large package modules while preserving their public paths. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Address PR review feedback (#2274)" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/dashboard/app/api/global-and-pi-settings.ts`:
- Around line 53-62: Update testNotification so the explicitly selected
providerId is applied after spreading config in the request body. This prevents
config.providerId from overriding the function argument while preserving all
other configuration fields.
In `@packages/dashboard/app/api/task-content.ts`:
- Around line 34-36: Update deleteAttachment and the affected key-based API
methods in packages/dashboard/app/api/task-content.ts at lines 34-36, 129-131,
133-135, 254-269, and 271-275 to wrap each user-defined filename or key path
segment with encodeURIComponent before constructing the URL; apply the same
encoding consistently at all five sites.
- Around line 29-31: Update the upload response handling around the JSON parsing
in the task-content request to safely handle non-JSON error responses: catch
parse failures, preserve the HTTP failure status, and throw an error using the
parsed API message when available or an HTTP-status-based fallback otherwise.
Follow the existing pattern used by fetchAgentLogsWithMeta.
🪄 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: 406e5db1-b6a4-401c-b53b-271c75ce79c4
📒 Files selected for processing (8)
packages/core/src/types.tspackages/core/src/types/messages.tspackages/dashboard/app/api/global-and-pi-settings.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/api/task-content.tspackages/engine/src/merger-diff-volume-gate.tspackages/engine/src/merger.tsscripts/line-count-baseline.json
- Re-export FetchProjectMarkdownFilesOptions from legacy API barrel - Pin providerId last in testNotification so config cannot override it - Safely parse non-JSON upload error bodies with HTTP status fallback - encodeURIComponent for attachment filename and document key path segments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wave 9 of package code organization (plan:
docs/plans/2026-07-14-001-refactor-package-code-organization-plan.md), after #2252.Peels
app/api/global-and-pi-settings.tslegacy.tsapp/api/task-content.tslegacy.ts(attachments/logs/comments/docs/artifacts)types/messages.tstypes.tsmerger-diff-volume-gatehelpersmerger.tsPublic paths stay stable via re-exports.
LOC
legacy.ts~10561 → ~10269types.ts~6998 → ~6937merger.ts~11223 → ~11208Test plan
Summary by CodeRabbit