fix(web): apply stack compatibility atomically - #1130
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughChangesStack compatibility and state resolution
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
🧹 Nitpick comments (9)
apps/web/src/app/(home)/new/_components/utils.ts (2)
743-759: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse display names in these user-facing messages.
Both messages interpolate raw option ids, so users read "tauri and electrobun removed" and "electrobun requires a static React Router export". Nearby messages use display names, for example "Tauri removed (requires compatible frontend)". Map ids to display names for consistent text. The assertions in
apps/web/test/stack-builder-compatibility.test.tsat lines 443-445 must be updated with the same change.Also applies to: 1258-1264
1127-1136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider aligning the analyzer with this stricter rule.
getDisabledReasonnow rejects every ORM when no database is selected.analyzeStackCompatibilityat lines 507-520 still auto-selects a database when an ORM is set without one. The UI path cannot reach that branch becausegetTechSelectionUpdatefilters incompatible options, but URL state and presets can. Keep the branch as a repair path, or make it match the new rule and clear the ORM instead. Add a short comment that states which behavior is intended.apps/web/src/app/(home)/new/_components/stack-builder/use-stack-builder.ts (2)
43-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport non-convergence instead of returning silently.
If the loop exhausts
MAX_COMPATIBILITY_PASSES, the function returns a stack thatanalyzeStackCompatibilitystill wants to change. The caller receives no signal. Add a development-only warning so a future rule cycle is visible outside the invariant test.♻️ Proposed change
const finalAnalysis = analyzeStackCompatibility(currentStack); + if (process.env.NODE_ENV !== "production" && finalAnalysis.adjustedStack) { + console.warn( + `Stack compatibility did not converge within ${MAX_COMPATIBILITY_PASSES} passes`, + finalAnalysis.changes, + ); + } return { stack: currentStack,
73-106: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the duplicated resolution inside the update builders.
applyStackUpdatealready resolves the current stack before it calls the updater.getSelectedTechRemovalUpdateandgetTechSelectionUpdateresolve it again. Each user action therefore runs the compatibility loop three times or more. The result is the same because resolution is idempotent, but the work is duplicated. Document that these builders receive an already-resolved stack, and drop the internalresolveStackCompatibilitycalls.apps/web/src/lib/stack-url-state.client.ts (1)
65-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the parameter type to the query-state shape.
Callers pass the full nuqs state, which also holds
viewModeandselectedFile. The annotation saysStackState. Type the parameter as the inferred nuqs state, for exampleReturnType<typeof useQueryStates<typeof stackParsers>>[0]or a namedStackQueryStatetype. The explicit field list then reads as an intentional projection.apps/web/test/stack-builder-compatibility.test.ts (1)
436-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Tauri counter-case for the exception.
The second test proves the exception holds for Electrobun. The exception in
getDockerDesktopConflictalso requires that Tauri is absent. Add a case withaddons: ["electrobun", "tauri"]on the same Convex + Better Auth + Next.js stack and assert that Docker is rejected. That locks the!selectedDesktopAddons.includes("tauri")condition.apps/web/test/stack-compatibility-invariant.test.ts (3)
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the cross-app relative import into
apps/cli/src.This web test imports CLI internals through
../../cli/src/.... The dependency direction now runs fromapps/webintoapps/cliprivate source.packages/types/src/types.tsalready exportsCLIInputandProjectConfig, so import the types from the shared package. ForvalidateFullConfig, use the CLI package entry point, or move the shared compatibility rules into a package that both apps consume.
148-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the ORM comparison order-insensitive.
toEqualon arrays compares order.enabledOrmsfollows the declaration order ofTECH_OPTIONS.orminapps/web/src/lib/constant.ts. A harmless reorder of those options breaks this test. Sort both sides, or compare sets.♻️ Proposed change
- expect(enabledOrms).toEqual(expectedOrmChoices[database]); + expect([...enabledOrms].sort()).toEqual([...expectedOrmChoices[database]].sort());
167-215: 🚀 Performance & Scalability | 🔵 TrivialMake the iteration counts configurable.
RANDOM_STACK_COUNTis 25,000 and the transition test performs more than 20,000 transitions. Each transition resolves compatibility several times and then runs the full CLI validation. The combined work can add a large fixed cost to every CI run and to local test runs. Read both counts from environment variables with the current values as the default, or gate the exhaustive run behind a scheduled job and keep a smaller default for pull requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ef56496-4622-43d5-844b-f62806f65a99
📒 Files selected for processing (6)
apps/web/src/app/(home)/new/_components/stack-builder/index.tsxapps/web/src/app/(home)/new/_components/stack-builder/use-stack-builder.tsapps/web/src/app/(home)/new/_components/utils.tsapps/web/src/lib/stack-url-state.client.tsapps/web/test/stack-builder-compatibility.test.tsapps/web/test/stack-compatibility-invariant.test.ts
| function showCompatibilityChanges(changes: CompatibilityAnalysis["changes"]) { | ||
| if (changes.length === 1) { | ||
| toast.info(changes[0].message, { duration: 4000 }); | ||
| return; | ||
| } | ||
|
|
||
| if (changes.length > 1) { | ||
| const message = `${changes.length} compatibility adjustments made:\n${changes | ||
| .map((change) => `• ${change.message}`) | ||
| .join("\n")}`; | ||
|
|
||
| toast.info(message, { duration: 5000 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate change messages before you build the toast.
resolveStackCompatibility appends the changes of every pass. A rule that fires in two passes contributes the same message twice. The toast then reports an inflated count, for example "3 compatibility adjustments made" with a repeated bullet. Deduplicate by message before you count.
🐛 Proposed fix
-function showCompatibilityChanges(changes: CompatibilityAnalysis["changes"]) {
- if (changes.length === 1) {
- toast.info(changes[0].message, { duration: 4000 });
+function showCompatibilityChanges(changes: CompatibilityAnalysis["changes"]) {
+ const messages = [...new Set(changes.map((change) => change.message))];
+
+ if (messages.length === 1) {
+ toast.info(messages[0], { duration: 4000 });
return;
}
- if (changes.length > 1) {
- const message = `${changes.length} compatibility adjustments made:\n${changes
- .map((change) => `• ${change.message}`)
- .join("\n")}`;
+ if (messages.length > 1) {
+ const message = `${messages.length} compatibility adjustments made:\n${messages
+ .map((entry) => `• ${entry}`)
+ .join("\n")}`;
toast.info(message, { duration: 5000 });
}
}📝 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.
| function showCompatibilityChanges(changes: CompatibilityAnalysis["changes"]) { | |
| if (changes.length === 1) { | |
| toast.info(changes[0].message, { duration: 4000 }); | |
| return; | |
| } | |
| if (changes.length > 1) { | |
| const message = `${changes.length} compatibility adjustments made:\n${changes | |
| .map((change) => `• ${change.message}`) | |
| .join("\n")}`; | |
| toast.info(message, { duration: 5000 }); | |
| } | |
| } | |
| function showCompatibilityChanges(changes: CompatibilityAnalysis["changes"]) { | |
| const messages = [...new Set(changes.map((change) => change.message))]; | |
| if (messages.length === 1) { | |
| toast.info(messages[0], { duration: 4000 }); | |
| return; | |
| } | |
| if (messages.length > 1) { | |
| const message = `${messages.length} compatibility adjustments made:\n${messages | |
| .map((entry) => `• ${entry}`) | |
| .join("\n")}`; | |
| toast.info(message, { duration: 5000 }); | |
| } | |
| } |
1 participant
Summary
Root cause
The follow-up compatibility write introduced in #1129 still ran in a passive effect. The UI rendered the adjusted stack before that second URL write settled, so a rapid follow-up click could be computed from the adjusted UI and then overwritten by the pending compatibility write. Sidebar counts briefly rendered from different snapshots for the same reason.
This change makes selection plus compatibility resolution one atomic functional update and scopes each query-state setter to only the keys it owns.
Verification
bun run checkcd apps/web && bun run buildbun test apps/web/test/stack-builder-compatibility.test.ts apps/web/test/stack-compatibility-invariant.test.tsbun test apps/cli/test/database-orm.test.ts apps/cli/test/database-setup.test.ts apps/cli/test/backend-runtime.test.tsbun test apps/cli/test/deployment.test.ts apps/cli/test/addons.test.ts apps/cli/test/electrobun-addon.test.tsThe invariant suite resolves 25,000 deterministic randomized stacks, checks every enabled card across more than 20,000 representative transitions, and validates every result with the CLI compatibility validator.
Summary by CodeRabbit
New Features
Bug Fixes