refactor(config): lean config load + setup/scaffold path - #366
Conversation
|
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
codeant-ai
Bot
added
the
size:L
PR Summary by QodoRefactor config loading and setup scaffolding helpers
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const pathExists = ( | ||
| fileSystem: FileSystem.FileSystem, | ||
| candidatePath: string, | ||
| ): Effect.Effect<boolean> => | ||
| fileSystem.exists(candidatePath).pipe(Effect.orElseSucceed(() => false)); |
There was a problem hiding this comment.
Suggestion: pathExists turns every filesystem failure into false, not only a genuine missing-path result. A permission error or transient filesystem failure therefore makes findLaunchConfig silently return null, causing callers to treat an inaccessible existing configuration as absent instead of surfacing the load/access failure. Only suppress a confirmed not-found condition and preserve other errors. [api mismatch]
Severity Level: Major ⚠️
- ❌ Inaccessible launch configs appear to be missing.
- ⚠️ App discovery silently skips filesystem failures.
- ⚠️ Users receive misleading initialization guidance.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/config/config.ts
**Line:** 35:39
**Comment:**
*Api Mismatch: `pathExists` turns every filesystem failure into `false`, not only a genuine missing-path result. A permission error or transient filesystem failure therefore makes `findLaunchConfig` silently return `null`, causing callers to treat an inaccessible existing configuration as absent instead of surfacing the load/access failure. Only suppress a confirmed not-found condition and preserve other errors.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const relativeAppPath = pathService.relative(workingDirectory, app.dir); | ||
| // An app at the repo root means scan the root - no shared monorepo subdir. | ||
| if (relativeAppPath === '') return null; | ||
| const [firstSegment] = relativeAppPath.split(/[/\\]/); | ||
| if (firstSegment !== undefined && firstSegment.length > 0) { | ||
| topLevelSegments.add(firstSegment); |
There was a problem hiding this comment.
Suggestion: When an app directory is outside workingDirectory, relativeAppPath begins with .., so this code emits appRoots: ["./.."]. Loading that scaffold then resolves the root to the parent directory and recursively discovers unrelated sibling projects instead of limiting discovery to the intended repository. Reject outside-root apps or return null rather than converting .. into an app root. [possible bug]
Severity Level: Major ⚠️
- ❌ Generated appRoots can include the repository parent.
- ⚠️ Discovery may include unrelated sibling projects.
- ⚠️ Build and app selection can operate on extra apps.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/config/configScaffold.ts
**Line:** 10:15
**Comment:**
*Possible Bug: When an app directory is outside `workingDirectory`, `relativeAppPath` begins with `..`, so this code emits `appRoots: ["./.."]`. Loading that scaffold then resolves the root to the parent directory and recursively discovers unrelated sibling projects instead of limiting discovery to the intended repository. Reject outside-root apps or return `null` rather than converting `..` into an app root.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const appCheck = yield* storeAppProbe.checkStoreApp(appId).pipe(Effect.either); | ||
| if (appCheck._tag === 'Left') { | ||
| storeAppProbe.readinessEntries.push( | ||
| makeReadinessRow(storeAppProbe.okLabel(appId), 'todo', storeAppProbe.missingDetail), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Suggestion: The helper converts every failed store probe into a missing-app todo. checkAppleApp and checkPlayApp can fail because of authentication, authorization, rate limiting, or transport errors, in which case telling the user to create the app is incorrect and hides the actual readiness failure. Preserve the underlying error classification or report an indeterminate probe separately from a confirmed missing app. [api mismatch]
Severity Level: Major ⚠️
- ❌ Store readiness reports API failures as missing apps.
- ⚠️ Users receive incorrect creation instructions.
- ⚠️ Authentication and transport failures lose diagnostics.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/config/setup.ts
**Line:** 197:203
**Comment:**
*Api Mismatch: The helper converts every failed store probe into a missing-app todo. `checkAppleApp` and `checkPlayApp` can fail because of authentication, authorization, rate limiting, or transport errors, in which case telling the user to create the app is incorrect and hides the actual readiness failure. Preserve the underlying error classification or report an indeterminate probe separately from a confirmed missing app.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review by Qodo
1. Sidecar probe hides FS errors
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const fileSystem = yield* FileSystem.FileSystem; | ||
| if (yield* fileSystem.exists(params.configPath)) return yield* params.load(params.configPath); | ||
| const sidecarExists = yield* pathExists(fileSystem, sidecarRequest.configPath); | ||
| if (sidecarExists) return yield* sidecarRequest.load(sidecarRequest.configPath); | ||
| return undefined; |
There was a problem hiding this comment.
1. Sidecar probe hides fs errors 🐞 Bug ☼ Reliability
resolveSidecarConfig now uses pathExists(), which converts any FileSystem.exists() failure into false and returns undefined instead of failing. This can silently skip a present-but-unreadable sidecar (e.g., permission/IO error) and make planners treat the surface as “nothing declared”.
Agent Prompt
### Issue description
`resolveSidecarConfig()` uses `pathExists()` for the implicit sidecar path check. `pathExists()` swallows *all* `FileSystem.exists()` failures and returns `false`, which changes behavior from “surface discovery fails with the underlying FS error” to “surface treated as absent / undefined”. This can hide permission/IO issues and lead to incorrect “nothing declared” outcomes.
### Issue Context
This only affects the implicit fallback sidecar lookup (when `explicitPath` is false and `typed` is undefined). It is used by multiple planners during `launch plan` / `launch drift` to locate default sidecars.
### Fix Focus Areas
- src/core/config/config.ts[34-39]
- src/core/config/config.ts[83-100]
### Suggested fix
- In `resolveSidecarConfig`, prefer `yield* fileSystem.exists(sidecarRequest.configPath)` (no blanket suppression) so unexpected FS errors propagate.
- If you still want best-effort behavior elsewhere, keep `pathExists()` for directory scanning and other non-critical probes, but avoid it for configuration discovery where errors should be surfaced.
- Add a unit test that simulates `FileSystem.exists()` failing and asserts `resolveSidecarConfig` fails (rather than returning `undefined`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/config/configScaffold.ts">
<violation number="1" location="src/core/config/configScaffold.ts:53">
P2: An empty `appRoot` now produces `appRoots: [""]` instead of the root-scan starter, changing the prior truthiness behavior and generating an ambiguous config. Retaining a non-empty check preserves the documented fallback for an empty path.</violation>
</file>
<file name="src/core/config/config.ts">
<violation number="1" location="src/core/config/config.ts:97">
P2: Unreadable sidecars can silently disappear from `launch plan`/`drift`, yielding an incomplete plan; the new best-effort existence wrapper masks filesystem errors in `resolveSidecarConfig`. Preserving the resolver's previous error propagation here would distinguish a missing sidecar from one that cannot be inspected.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ): string => { | ||
| let appRootsLine = ` // appRoots: ["./apps"], // uncomment if your apps live in a subfolder`; | ||
| if (appRoot) appRootsLine = ` appRoots: ["${appRoot}"], // every app.json lives under here`; | ||
| if (appRoot !== null) { |
There was a problem hiding this comment.
P2: An empty appRoot now produces appRoots: [""] instead of the root-scan starter, changing the prior truthiness behavior and generating an ambiguous config. Retaining a non-empty check preserves the documented fallback for an empty path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/config/configScaffold.ts, line 53:
<comment>An empty `appRoot` now produces `appRoots: [""]` instead of the root-scan starter, changing the prior truthiness behavior and generating an ambiguous config. Retaining a non-empty check preserves the documented fallback for an empty path.</comment>
<file context>
@@ -46,13 +50,17 @@ export const configTemplate = (
): string => {
let appRootsLine = ` // appRoots: ["./apps"], // uncomment if your apps live in a subfolder`;
- if (appRoot) appRootsLine = ` appRoots: ["${appRoot}"], // every app.json lives under here`;
+ if (appRoot !== null) {
+ appRootsLine = ` appRoots: ["${appRoot}"], // every app.json lives under here`;
+ }
</file context>
| if (appRoot !== null) { | |
| if (appRoot !== null && appRoot.length > 0) { |
| const sidecarExists = yield* pathExists(fileSystem, sidecarRequest.configPath); | ||
| if (sidecarExists) return yield* sidecarRequest.load(sidecarRequest.configPath); |
There was a problem hiding this comment.
P2: Unreadable sidecars can silently disappear from launch plan/drift, yielding an incomplete plan; the new best-effort existence wrapper masks filesystem errors in resolveSidecarConfig. Preserving the resolver's previous error propagation here would distinguish a missing sidecar from one that cannot be inspected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/config/config.ts, line 97:
<comment>Unreadable sidecars can silently disappear from `launch plan`/`drift`, yielding an incomplete plan; the new best-effort existence wrapper masks filesystem errors in `resolveSidecarConfig`. Preserving the resolver's previous error propagation here would distinguish a missing sidecar from one that cannot be inspected.</comment>
<file context>
@@ -74,41 +80,47 @@ export const defineConfig = (input: LaunchConfigInput): LaunchConfig => {
+ if (sidecarRequest.typed !== undefined) return sidecarRequest.typed;
const fileSystem = yield* FileSystem.FileSystem;
- if (yield* fileSystem.exists(params.configPath)) return yield* params.load(params.configPath);
+ const sidecarExists = yield* pathExists(fileSystem, sidecarRequest.configPath);
+ if (sidecarExists) return yield* sidecarRequest.load(sidecarRequest.configPath);
return undefined;
</file context>
| const sidecarExists = yield* pathExists(fileSystem, sidecarRequest.configPath); | |
| if (sidecarExists) return yield* sidecarRequest.load(sidecarRequest.configPath); | |
| if (yield* fileSystem.exists(sidecarRequest.configPath)) | |
| return yield* sidecarRequest.load(sidecarRequest.configPath); |
b055e31 to
3680a28
Compare
Deduplicate static app.json read/write and store-app readiness probes, wire DEFAULT_* into the fallback Launch config, rename private bad-stem helpers, simplify toolchain consent, and export pure setup helpers with business tests. Behavior preserved. Fixes #341.
3680a28 to
ce14d11
Compare
1 participant
User description
Summary
Lean remediations for config load + setup/scaffold (path-only deslop for #341).
pathExists/ static app.json read+write;DEFAULT_*for fallback config; rename privatetoDescriptor/resolveConfigstems; explicit iOS field guards; readonlyFoundConfig/LoadedConfigshapesmayInstallToolchain+assumeYes: yes); export purereadinessMark/formatPendingTodoLine/mayInstallToolchaindetectAppRootBehavior preserved. No live ASC/Play/AWS.
Gate
pnpm typecheck && pnpm lint && pnpm lint:style && pnpm docs:check && pnpm test && pnpm build— all green (2091 tests).Fixes #341
CodeAnt-AI Description
Streamline configuration discovery and setup readiness checks
What Changed
--yesis provided, avoiding unattended installation prompts.Impact
✅ Safer non-interactive setup✅ Clearer store setup steps✅ Safer static app config updates💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by cubic
Refactors config loading and setup/scaffold paths for leaner, predictable behavior (fixes #341). Deduplicates static
app.jsonI/O, unifies App Store/Play readiness checks, and exports small setup helpers while keeping behavior the same.LaunchConfigusesDEFAULT_*; sharedpathExists; newloadAppConfigDocument; static JSON helpersreadStaticAppJson/writeStaticAppJson; renamedappDescriptorFromConfig; stricter iOS/Android guards; readonlyFoundConfig/LoadedConfig.readinessMark,formatPendingTodoLine,mayInstallToolchain; installs gated by TTY/--yesviaensureToolchain({ assumeYes: yes }); inlinerunBuildrehearsal; simplerpendingTodos.detectAppRoot(readonly input) and deterministic template handling with optionalartifactDir.Written for commit ce14d11. Summary will update on new commits.