feat(ai): score fast apply code quality#3118
Open
RitwijParmar wants to merge 3 commits into
Open
Conversation
Signed-off-by: Ritwij Aryan Parmar <ritwij.aryan.parmar@gmail.com>
|
@RitwijParmar is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new quality assessment module (assessCodeChange, shouldBlockApply), integrates preflight/result assessments into applyCodeChangeWithQuality with optional gating, tweaks provider response handling and apply error retry behavior, re-exports quality APIs, and adds tests covering assessment signals and gating rules. ChangesCode Quality Assessment Integration
Sequence Diagram(s)sequenceDiagram
participant caller
participant applyCodeChangeWithQuality
participant assessCodeChange
participant shouldBlockApply
participant applyCodeChange
participant resultAssessment
caller->>applyCodeChangeWithQuality: request apply with quality
applyCodeChangeWithQuality->>assessCodeChange: preflight assessment
assessCodeChange-->>applyCodeChangeWithQuality: preflight result
applyCodeChangeWithQuality->>shouldBlockApply: check gate conditions
alt blocked by gate
applyCodeChangeWithQuality-->>caller: return blocked result
else proceed
applyCodeChangeWithQuality->>applyCodeChange: apply code change
applyCodeChange-->>applyCodeChangeWithQuality: applied code
applyCodeChangeWithQuality->>resultAssessment: assess applied output
resultAssessment-->>applyCodeChangeWithQuality: result assessment
applyCodeChangeWithQuality-->>caller: return success with assessments
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai/src/apply/client.ts (1)
143-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback provider is never attempted after first failure.
At Line 145, throwing inside the loop exits immediately, so the second provider in
providerAttemptsis never tried.Suggested fix
+ let lastError: unknown = null; // Run provider attempts in order of preference for (const { provider, applyFn } of providerAttempts) { try { const result = provider === FastApplyProvider.MORPH ? await (applyFn as typeof applyCodeChangeWithMorph)( originalCode, updateSnippet, instruction, ) : await applyFn(originalCode, updateSnippet, instruction, metadata); if (result) return result; } catch (error) { console.warn(`Code application failed with provider ${provider}:`, error); - throw error; + lastError = error; + continue; } } + if (lastError) throw lastError; return null;🤖 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 `@packages/ai/src/apply/client.ts` around lines 143 - 146, The catch inside the providerAttempts loop currently rethrows the error (catch { console.warn(...); throw error; }) which stops the loop and prevents trying fallback providers; change the catch to log the error and continue to the next provider (do not rethrow inside the loop), and after the loop finishes without a successful result, throw a new aggregated error (or the last error) to surface failure; update the catch block that references provider and providerAttempts so it only logs the failure for that provider and allows the loop to try the next one.
🤖 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/ai/src/apply/quality.ts`:
- Around line 101-103: The current logic computes placeholders and risky
patterns from codeToAssess only (using findPlaceholders and findRiskyPatterns)
and then treats them as generated concerns; instead compute placeholders and
riskyPatterns for both codeToAssess and originalCode, then derive the newly
introduced items by set-difference (newPlaceholders = placeholders(codeToAssess)
- placeholders(originalCode), likewise for riskyPatterns) and use those
newPlaceholders/newRiskyPatterns where the code treats generated concerns (the
sections around scoreSyntaxBalance and the handling at lines ~163-166). For
syntax balance (scoreSyntaxBalance) consider using the delta or only penalize if
the balance worsened relative to originalCode. Ensure you update any variable
names used later (e.g., placeholders, riskyPatterns) to reference the "new" sets
so only newly introduced issues are counted.
- Around line 267-273: The current extractDeclaredSymbols function is too broad
(it picks up local vars and text in comments/strings) and causes false positives
for missingOriginalSymbols; restrict extraction to top-level exported
declarations or switch to an AST-based approach: update extractDeclaredSymbols
to only capture exported/top-level identifiers (e.g., look only for "export"
declarations or parse the file with a JS/TS parser like `@babel/parser` /
TypeScript compiler API to collect top-level exported function/class/const/type
names) and then use that narrower set where missingOriginalSymbols or the
blocking logic (the code that defaults to block on missingOriginalSymbols) is
consulted so local/inline declarations and commented text are ignored. Ensure
references to extractDeclaredSymbols and the blocking check that uses
missingOriginalSymbols are updated accordingly.
---
Outside diff comments:
In `@packages/ai/src/apply/client.ts`:
- Around line 143-146: The catch inside the providerAttempts loop currently
rethrows the error (catch { console.warn(...); throw error; }) which stops the
loop and prevents trying fallback providers; change the catch to log the error
and continue to the next provider (do not rethrow inside the loop), and after
the loop finishes without a successful result, throw a new aggregated error (or
the last error) to surface failure; update the catch block that references
provider and providerAttempts so it only logs the failure for that provider and
allows the loop to try the next one.
🪄 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
Run ID: 1421f9f3-0e0d-4bda-b434-b6ee3537d1a5
📒 Files selected for processing (4)
packages/ai/src/apply/client.tspackages/ai/src/apply/index.tspackages/ai/src/apply/quality.tspackages/ai/test/apply-quality.test.ts
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 `@packages/ai/src/apply/quality.ts`:
- Around line 279-280: The export-symbol extraction treats "export default"
declarations as stable named symbols causing false missing-symbol reports;
update the symbol extraction logic that uses the regexes
(/^export\s+(?:default\s+)?(?:async\s+)?(?:function|class|interface|type|enum|const|let|var)\s+([A-Za-z_$][\w$]*)/gm
and the export list regex) to special-case default exports: do not treat "export
default <identifier|expression>" as a named exported symbol for missing-symbol
checks, instead detect "export default" forms and either skip generating a
missing-symbol block for them or map them to a canonical default marker (e.g.,
"default") so renames of internal identifiers (e.g., local function/class names)
do not trigger dropped-symbol warnings; adjust the extraction and downstream
comparison code paths that reference the regex match group (the captured
identifier) to handle this special-case.
- Around line 214-216: The current check uses assessment.blockingConcerns.length
to decide blocking, which ignores gate.blockOnMissingSymbols and therefore
blocks when the only concern is missing-symbols; update the condition so you
first filter assessment.blockingConcerns to remove missing-symbols concerns when
gate.blockOnMissingSymbols is false (e.g. const relevant =
assessment.blockingConcerns.filter(c => !(c.type === 'missing_symbols' &&
!gate.blockOnMissingSymbols'))) and then use relevant.length > 0 in the if that
references gate.blockHighRisk and assessment.blockingConcerns (look for the if
using gate.blockHighRisk and assessment.blockingConcerns in quality.ts).
🪄 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
Run ID: 8d34a9af-3e21-46bd-bc03-12caca4ebc78
📒 Files selected for processing (3)
packages/ai/src/apply/client.tspackages/ai/src/apply/quality.tspackages/ai/test/apply-quality.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ai/test/apply-quality.test.ts
- packages/ai/src/apply/client.ts
Author
|
Checked this again. The CodeRabbit threads are already addressed in a567cec and f4b3fd. Focused Bun test passes with 8 tests. The remaining red status is Vercel auth only. |
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
Closes #3114. Adds an opt-in quality assessment layer for fast apply code changes. The scorer can run before the provider call to estimate risk from the update snippet, then run again on the merged output returned by Morph or Relace.
It checks for:
The API exposes
assessCodeChange,shouldBlockApply, andapplyCodeChangeWithQuality, so the UI can show score/confidence/risk before applying changes without forcing one hard product policy.Notes
I also typed the Relace response shape and changed existing empty-value fallbacks from
||to??in the touched apply client.Testing
bun test packages/ai/test/apply-quality.test.tsbunx eslint packages/ai/src/apply/client.ts packages/ai/src/apply/index.ts packages/ai/src/apply/quality.ts packages/ai/test/apply-quality.test.tsbunx tsc --noEmit --pretty false --moduleResolution bundler --module ESNext --target ES2022 --strict --skipLibCheck packages/ai/src/apply/client.ts packages/ai/src/apply/quality.ts packages/ai/test/apply-quality.test.tsI also tried
bun --filter @onlook/ai typecheck, but it fails on existing unrelated repo issues in web-client path aliases and older tests, not in this change.Summary by CodeRabbit
New Features
Improvements
Tests