feat: Introduce ability to view conflict files and track them live#511
Conversation
ENG-647 Add Error and Conflict Visualization
Enhance application cards to display detailed error information and conflict files. Tasks:
Acceptance Criteria:
|
🦋 Changeset detectedLatest commit: 3e6c860 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update introduces several new features and improvements across the project builder ecosystem. The server and web application now support user configuration, including an optional editor command, which enables users to open conflict files directly from the web UI. Conflict file management is enhanced with new server-side API methods for opening, deleting, and keeping conflict files, and a new interactive table-based UI for viewing and managing these conflicts. The sync metadata structure and schema are updated to include a package ordering field, ensuring consistent display order. The CLI gains a configuration command for managing user settings, with validation and persistence. Additional updates include new tests, TypeScript strictness, and integration of Vitest for testing. Changes
Sequence Diagram(s)Conflict File Management: Web UI to ServersequenceDiagram
participant User
participant WebApp
participant ServerAPI
participant BuilderService
User->>WebApp: Clicks "View", "Keep", or "Delete" on conflict file
WebApp->>ServerAPI: Calls openEditor / keepConflictFile / deleteConflictFile (with packageId, relativePath)
ServerAPI->>BuilderService: Invokes corresponding method (openEditor, keepConflictFile, deleteConflictFile)
BuilderService-->>ServerAPI: Performs action (opens editor, updates metadata, deletes file)
ServerAPI-->>WebApp: Returns success or error
WebApp-->>User: Shows toast notification
CLI User Config ManagementsequenceDiagram
participant User
participant CLI
participant UserConfigService
participant ZodSchema
User->>CLI: Runs `config set <path> <value>`
CLI->>UserConfigService: Reads current config
CLI->>ZodSchema: Validates updated config
alt Valid
CLI->>UserConfigService: Writes updated config
CLI-->>User: Prints updated config
else Invalid
ZodSchema-->>CLI: Returns validation error
CLI-->>User: Prints error message
end
User->>CLI: Runs `config get`
CLI->>UserConfigService: Reads current config
CLI-->>User: Prints config
Sync Metadata Initialization and ValidationsequenceDiagram
participant BuilderService
participant SyncMetadataController
BuilderService->>SyncMetadataController: Reads sync metadata on startup
alt Status is "in-progress"
BuilderService->>SyncMetadataController: Updates status to "cancelled"
else Invalid metadata
BuilderService->>SyncMetadataController: Deletes sync metadata file
end
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
packages/project-builder-server/src/sync/sync-metadata.ts (1)
63-70: 💡 Verification agent🧩 Analysis chain
Added required order field to package schema
The addition of the
orderfield as a required number in the package schema is a good enhancement. This provides a consistent way to maintain display order of packages across the application.Let's verify this change is applied consistently:
🏁 Script executed:
#!/bin/bash # Check if all places creating PackageSyncInfo objects include the order property rg -t ts "packageSyncInfoSchema" -C 5 # Look for instances where packages are created to verify they include order rg -t ts "packages\s*[:=]\s*[{]" -A 10Length of output: 11264
Add missing
orderproperty to test fixturesWe verified that every runtime use of
PackageSyncInfoincludesorder, except in the conflict-file-monitor tests. The fixtures inconflict-file-monitor.test.tsneed to be updated to include anordervalue (e.g.0) so they comply withpackageSyncInfoSchema.• File:
packages/project-builder-server/src/sync/conflict-file-monitor.test.ts
Update each package fixture underpackages: { … }to include:'test-package': { name: 'test-package', path: testPackagePath, + order: 0, status: 'conflicts', result: { filesWithConflicts: [ /* … */ ],After adding
orderto these fixtures, all schema validations will pass.
🧹 Nitpick comments (8)
.changeset/tiny-fans-matter.md (1)
1-6: Consider mentioning editor integration in the changesetThe changeset accurately describes the new conflict file table view, but could be enhanced by explicitly mentioning the editor integration feature, which appears to be a key part of the new conflict file management capabilities.
--- '@halfdomelabs/project-builder-web': patch --- -Introduce ability to view conflict files in a table and act on them accordingly +Introduce ability to view conflict files in a table and act on them accordingly, including opening in editor, deleting, or keeping filespackages/project-builder-server/src/api/sync.ts (1)
102-136: Consider adding error handling and return valuesAll three mutations would benefit from explicit error handling and meaningful return values to communicate success or failure to the client.
Consider enhancing the mutations with try/catch blocks and return values:
openEditor: privateProcedure .input( z.object({ id: z.string(), packageId: z.string(), relativePath: z.string(), }), ) - .mutation(async ({ input: { id, packageId, relativePath }, ctx }) => { - await ctx.getApi(id).openEditor(packageId, relativePath); - }), + .mutation(async ({ input: { id, packageId, relativePath }, ctx }) => { + try { + await ctx.getApi(id).openEditor(packageId, relativePath); + return { success: true }; + } catch (error) { + ctx.logger.error(error); + return { + success: false, + error: error instanceof Error ? error.message : String(error) + }; + } + }),Apply similar enhancements to the other two mutations.
packages/project-builder-cli/src/commands/config.ts (3)
12-31:setConfigValuedoes not support array indices or dot‐escaped keysThe helper works for plain nested objects but:
- A path like
foo.0.barwill create{ foo: { "0": { bar: ... }}}, not an array.- Keys containing dots (
"editor.command") cannot be set.If either case is expected, extend the routine (or use a library such as
lodash.set) to recognise numeric segments and an escape syntax (e.g.path.split(/\.(?!\\)/)).
45-52: Boolean conversion is narrow – numbers & JSON values ignoredOnly the literals
"true"/"false"are coerced.
Users might reasonably type:project-builder config set sync.timeout 300 project-builder config set feature.enabled true ✅ project-builder config set feature.enabled FALSE ❌ (string)A more flexible strategy:
const parsedValue = (() => { try { return JSON.parse(value); } // handles numbers, booleans, objects, arrays catch { return value; } // fallback to raw string })();This keeps the CLI ergonomic without sacrificing validation.
54-68: Zod unknown‑key detection fragile
err.message.includes('Unrecognized key')relies on Zod’s English wording and singular form; future releases/locales may change.
Instead, inspect the error issues array:const isUnknownKey = err.issues.some( (i) => i.code === 'unrecognized_keys', );This is version‑stable and avoids false positives.
packages/project-builder-web/src/app/ProjectSyncModal/ApplicationCard.tsx (3)
93-108: Duplicate logic – consider consolidating conflict labels
getConflictTypeLabelandgetConflictTypeTooltiprepeat the same switch; adding a new conflict type requires edits in two places.A tiny map object eliminates duplication:
const conflictMeta = { 'merge-conflict': { label: 'Merge Conflict', tooltip: 'This file was modified in both …', }, 'working-deleted': { … }, 'generated-deleted': { … }, } as const;Then:
const { label, tooltip } = conflictMeta[type] ?? { label: 'Unknown', tooltip: 'Unknown conflict type' };
181-194: Section title is misleading
<h4>Merge Conflicts</h4>precedes a list that can also contain deletion conflicts.
Rename to something neutral like “Files with Conflicts” to avoid confusion.
314-330: Prop drilling – consider memoising FilesWithConflictsView
FilesWithConflictsViewis re‑created on every render; with large tables this may become costly.
Wrap the component inReact.memoor move handlers intouseCallbackto prevent unnecessary re‑renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
.changeset/tiny-fans-matter.md(1 hunks)packages/project-builder-cli/package.json(3 hunks)packages/project-builder-cli/src/commands/config.ts(1 hunks)packages/project-builder-cli/src/commands/config.unit.test.ts(1 hunks)packages/project-builder-cli/src/commands/server.ts(1 hunks)packages/project-builder-cli/src/index.ts(2 hunks)packages/project-builder-cli/src/services/user-config.ts(2 hunks)packages/project-builder-cli/tsconfig.json(1 hunks)packages/project-builder-cli/vitest.config.ts(1 hunks)packages/project-builder-server/package.json(1 hunks)packages/project-builder-server/src/api/sync.ts(1 hunks)packages/project-builder-server/src/api/types.ts(1 hunks)packages/project-builder-server/src/api/version.ts(1 hunks)packages/project-builder-server/src/server/plugin.ts(3 hunks)packages/project-builder-server/src/server/server.ts(4 hunks)packages/project-builder-server/src/service/builder-service.ts(4 hunks)packages/project-builder-server/src/sync/build-project.ts(1 hunks)packages/project-builder-server/src/sync/conflict-file-monitor.test.ts(4 hunks)packages/project-builder-server/src/sync/sync-metadata-controller.ts(2 hunks)packages/project-builder-server/src/sync/sync-metadata-controller.unit.test.ts(2 hunks)packages/project-builder-server/src/sync/sync-metadata-service.ts(2 hunks)packages/project-builder-server/src/sync/sync-metadata.ts(1 hunks)packages/project-builder-server/src/user-config/user-config-schema.ts(1 hunks)packages/project-builder-test/src/commands/serve.ts(1 hunks)packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.test.tsx(1 hunks)packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.tsx(1 hunks)packages/project-builder-web/src/app/ProjectSyncModal/ApplicationCard.tsx(4 hunks)packages/project-builder-web/src/app/ProjectSyncModal/PackageSyncStatus.tsx(2 hunks)packages/project-builder-web/src/hooks/useClientVersion.ts(1 hunks)packages/project-builder-web/src/hooks/useSyncMetadata.ts(1 hunks)packages/project-builder-web/src/services/api/sync.ts(1 hunks)packages/project-builder-web/src/services/api/version.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
packages/project-builder-web/src/hooks/useSyncMetadata.ts (2)
packages/project-builder-web/src/services/trpc.ts (1)
trpc(179-201)packages/ui-components/src/index.ts (1)
toast(7-7)
packages/project-builder-cli/src/index.ts (1)
packages/project-builder-cli/src/commands/config.ts (1)
addConfigCommand(33-78)
packages/project-builder-web/src/hooks/useClientVersion.ts (2)
packages/project-builder-lib/src/feature-flags/flags.ts (1)
FeatureFlag(3-3)packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(32-32)
packages/project-builder-server/src/api/version.ts (3)
packages/project-builder-lib/src/feature-flags/flags.ts (1)
FeatureFlag(3-3)packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(32-32)packages/project-builder-server/src/api/trpc.ts (1)
publicProcedure(24-24)
packages/project-builder-cli/src/services/user-config.ts (2)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(32-32)packages/utils/src/fs/write-stable-pretty-json.ts (1)
writeStablePrettyJson(13-19)
packages/project-builder-server/src/api/types.ts (4)
packages/project-builder-server/src/server/builder-service-manager.ts (1)
BuilderServiceManager(9-67)packages/sync/src/utils/evented-logger.ts (1)
Logger(3-8)packages/project-builder-lib/src/feature-flags/flags.ts (1)
FeatureFlag(3-3)packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(32-32)
packages/project-builder-web/src/app/ProjectSyncModal/PackageSyncStatus.tsx (1)
packages/project-builder-web/src/app/ProjectSyncModal/ApplicationCard.tsx (1)
ApplicationCard(272-347)
packages/project-builder-server/src/server/server.ts (1)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(32-32)
packages/project-builder-cli/src/commands/config.unit.test.ts (1)
packages/project-builder-cli/src/commands/config.ts (1)
setConfigValue(12-31)
packages/project-builder-server/src/service/builder-service.ts (1)
packages/project-builder-server/src/sync/sync-metadata-service.ts (1)
deleteSyncMetadata(40-45)
packages/project-builder-server/src/server/plugin.ts (1)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(32-32)
🔇 Additional comments (48)
packages/project-builder-server/package.json (1)
51-51: LGTM: appropriate dependency for editor integrationAdding the
execapackage makes sense for launching external editor processes, which aligns with the PR objective of enabling users to open conflict files directly from the UI.packages/project-builder-web/src/services/api/version.ts (1)
11-11: LGTM: Consistent interface for preview modeAdding the
userConfigproperty to the preview mode response ensures consistency with the actual API, which is good practice.packages/project-builder-server/src/sync/build-project.ts (1)
114-124: LGTM: Good addition for consistent package orderingAdding the
orderproperty to package metadata is a good improvement that ensures consistent display order of packages in the UI. Using the index of the application in theappsarray is a sensible approach.packages/project-builder-web/src/services/api/sync.ts (1)
35-35: Added order property to match updated schemaAdding the
orderproperty to the mock package object in preview mode ensures consistency with the updatedpackageSyncInfoSchemawhich now requires this field.packages/project-builder-cli/src/index.ts (2)
6-6: Added import for new config command moduleCorrectly imports the new configuration command module that will allow users to manage settings through the CLI.
25-25: Integrated new config command into CLI programThe configuration command is properly integrated into the command program structure, enhancing the CLI with user configuration management capabilities.
packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.test.tsx (1)
17-17: Added userConfig property to test fixtureAdding the
userConfigproperty to themockVersionInfofixture correctly aligns the test data with the expandedClientVersionInfointerface that now includes user configuration data.packages/project-builder-test/src/commands/serve.ts (1)
99-99: Added userConfig to startWebServer optionsThis change ensures user configuration is consistently passed to both the
BuilderServiceManagerconstructor and thestartWebServerfunction. This maintains consistency with similar changes in other parts of the codebase.packages/project-builder-cli/tsconfig.json (1)
3-6: Strong TypeScript configuration upgrade!Adding
"noEmit": trueand"strict": truecompiler options is an excellent improvement that will enhance code quality. The strict mode enables all type checking features, which helps catch potential runtime errors during development. This change aligns well with the introduction of new TypeScript files and testing infrastructure mentioned in the summary.packages/project-builder-cli/src/commands/server.ts (1)
66-66: Good addition for consistent user config propagationAdding
userConfigto thestartWebServeroptions ensures that user configuration is consistently available throughout the system. Previously, it was only passed to theBuilderServiceManagerconstructor but not to the web server. This change completes the propagation path for user configuration data.packages/project-builder-cli/vitest.config.ts (1)
1-1: Good approach for test configuration reuseRe-exporting the default Vitest configuration from a shared package promotes consistency across the project and reduces configuration duplication. This is a clean pattern for monorepos to ensure all packages follow the same testing standards.
packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.tsx (1)
66-66: Correctly exposes user config to client componentsAdding
userConfigto theclientVersionResultobject ensures that the user configuration is available through the React context API. This change properly aligns the client-side context with the server-side API changes, allowing React components to access user configuration data.packages/project-builder-server/src/sync/sync-metadata-controller.unit.test.ts (1)
95-95: Added order property to align with updated schemaThe changes correctly add an
orderproperty to the package metadata in test cases, ensuring consistency with the updated schema forPackageSyncInfo.Also applies to: 118-118
packages/project-builder-server/src/api/types.ts (1)
5-5: User configuration support added to API contextThe addition of
userConfigto the API context enables API endpoints to access user preferences, which supports features like opening conflict files in an external editor.Also applies to: 12-12
packages/project-builder-web/src/hooks/useClientVersion.ts (1)
2-2: Client-side access to user configurationGood addition of user configuration to the client version context, enabling UI components to access user preferences for features like editor integration.
Also applies to: 9-9
packages/project-builder-server/src/sync/conflict-file-monitor.test.ts (4)
32-40: Parameter name update improves readabilityThe parameter name change from
throttleWrites: falsetodisableThrottling: trueis a good improvement as it makes the intent clearer. The new name better expresses that you're disabling a feature rather than configuring a property.
46-71: Package metadata now includes order propertyThe addition of
order: 0to the package metadata aligns with the schema update. This ensures consistent ordering of packages in the UI.Make sure this
orderproperty is consistently set throughout the codebase whenever package metadata is created or updated.
90-110: Consistent schema update appliedThe
order: 0field has been properly added to this test case as well, keeping it consistent with the schema changes.
171-191: Order field consistently appliedThe
order: 0property has been appropriately added to all test cases, ensuring consistent test coverage for the updated schema.packages/project-builder-cli/src/commands/config.unit.test.ts (1)
1-48: Well-structured tests for immutable config updatesThese tests are well-designed and thoroughly validate the
setConfigValuefunction's behavior:
- They follow a clear Arrange-Act-Assert pattern
- They verify both the correctness of the returned value and the immutability of the input
- They cover important scenarios: top-level keys, nested keys, and creating intermediate objects
The tests demonstrate that the function correctly implements immutable updates to configuration objects at arbitrary depths.
packages/project-builder-server/src/api/version.ts (3)
1-4: Added import for user configuration typeThe import of
BaseplateUserConfigis appropriate for extending the version API to include user configuration.
7-11: Extended ClientVersionInfo to include user configurationAdding
userConfigto theClientVersionInfointerface is a good way to ensure the client has access to user-specific settings. This enables features like custom editor integration.
13-19:Details
✅ Verification successful
User config now exposed through version API
The version router correctly returns the user configuration from the context. This ensures configuration is available to clients without requiring additional API calls.
Let's verify the API context properly includes the user configuration:
🏁 Script executed:
#!/bin/bash # Check if the API context definition includes userConfig rg -t ts "interface BaseplateApiContext" -A 10 # Check where userConfig is passed to the context rg -t ts "userConfig.*context" -C 5Length of output: 617
Confirmed: BaseplateApiContext includes
userConfig
TheBaseplateApiContextinterface inpackages/project-builder-server/src/api/types.tsdefinesuserConfig: BaseplateUserConfig, soctx.userConfigis available to theversionRouter. No further changes needed.packages/project-builder-cli/package.json (3)
34-34: Good addition of test framework.Adding Vitest as the testing framework will enable proper unit testing for the new configuration command functionality.
47-48: Great choice adding Zod for schema validation.Zod is an excellent choice for runtime validation of user configuration, providing type safety and validation in a single package.
60-61: Appropriate dev dependency for testing.Adding Vitest as a dev dependency complements the new test script and enables modern testing capabilities.
packages/project-builder-web/src/app/ProjectSyncModal/PackageSyncStatus.tsx (3)
5-5: Good utility import for sorting.The
sortByutility from es-toolkit is a good choice for implementing consistent package sorting.
45-47: Nice implementation of package sorting.Sorting packages by their
orderproperty ensures consistent display order in the UI, improving user experience.
51-56: Good prop passing to support conflict file management.Passing both
packageIdandpackageInfoto theApplicationCardcomponent properly supports the new conflict file management functionality, allowing the component to identify which package the conflicts belong to and display appropriate actions.packages/project-builder-server/src/user-config/user-config-schema.ts (1)
7-30: Well-structured and documented schema with editor support.The schema is well-organized with proper documentation for each property. The addition of the optional
editorproperty enables the new feature to open conflict files directly from the UI. Using.strict()for both the root and nested objects enforces schema integrity.packages/project-builder-web/src/hooks/useSyncMetadata.ts (2)
59-79: Good implementation of real-time sync status notifications.The new subscription to
trpc.sync.onSyncCompletedproperly detects sync completion events and provides appropriate toast notifications based on the sync status. The conflict detection logic is well-implemented, checking if any packages have a 'conflicts' status.
82-82: Proper cleanup of subscription resources.Correctly unsubscribing from the new subscription in the cleanup function prevents memory leaks and ensures proper resource management.
packages/project-builder-cli/src/services/user-config.ts (2)
36-43: Well-structured function for user configuration persistenceThe
writeUserConfigfunction is well-implemented with proper error handling through async/await and follows good practices by:
- Reusing the existing
getConfigPathfunction- Creating the directory structure if it doesn't exist
- Using the appropriate utility for stable JSON serialization
7-9: Proper import additionsThe new imports support the added functionality and follow the codebase's import organization pattern.
packages/project-builder-server/src/server/plugin.ts (4)
11-11: Clean type import for user configurationThe import is correctly placed and follows the established import ordering pattern.
24-24: Properly typed plugin optionThe
userConfigparameter is correctly added to the plugin options with the appropriate type.
27-28: Clean function signature updateThe destructuring pattern is updated consistently with the new parameter.
48-48: Context builder integrationThe
userConfigis properly passed to the context builder, making it available throughout the tRPC router system.packages/project-builder-server/src/server/server.ts (4)
14-14: Clean type importThe import follows the project's import organization pattern.
27-27: Interface correctly extendedThe
WebServerOptionsinterface is properly extended with theuserConfigproperty.
36-36: Function signature updateThe function parameter destructuring pattern is updated consistently.
65-65: Plugin registration updateThe
userConfigis correctly passed to thebaseplatePluginregistration.packages/project-builder-server/src/api/sync.ts (3)
102-113: Well-structuredopenEditormutationThe mutation follows the established pattern in the router with proper input validation and API method invocation.
114-125: Well-structureddeleteConflictFilemutationThe mutation is correctly implemented with proper input validation and the appropriate API call.
126-136: Well-structuredkeepConflictFilemutationThe mutation is well-implemented with consistent structure and proper input validation.
packages/project-builder-server/src/sync/sync-metadata-controller.ts (1)
31-46: Potential breaking‑change: renamed option may leave callers silently throttledThe constructor now expects
options.disableThrottling(inverse semantics to the removedthrottleWrites).
Any older call‑sites that still pass{ throttleWrites: true }will compile (excess property is ignored) but throttling will stay enabled, altering behaviour in subtle ways.Consider one of the following to avoid silent regressions:
- protected options: { disableThrottling?: boolean } = {}, + protected options: { disableThrottling?: boolean; /** @deprecated */ throttleWrites?: boolean } = {},and then map the deprecated flag with a runtime warning:
const disable = options.disableThrottling ?? (options as { throttleWrites?: boolean }).throttleWrites ?? false; this.throttledWrite = disable ? writeCallback : throttle(writeCallback, 100);This preserves backwards compatibility while nudging consumers to migrate.
Run a quick grep for
new SyncMetadataController(to ensure every instantiation passes the new flag (or none at all).packages/project-builder-web/src/app/ProjectSyncModal/ApplicationCard.tsx (1)
143-154: Open‑editor handler ignores missing editor config
handleOpenEditorexecutes even when no editor is configured (the caller guards most cases, but defensive coding here avoids silent failures):- if (!currentProjectId) return; + if (!currentProjectId || !clientVersion.userConfig.sync?.editor) return;packages/project-builder-server/src/service/builder-service.ts (1)
503-546:Details
❌ Incorrect review comment
keepConflictFileshouldawaitthe metadata update
updateMetadataForPackageis asynchronous (based on other controller methods) yet its promise is ignored; failures will be swallowed.- this.syncMetadataController.updateMetadataForPackage( + await this.syncMetadataController.updateMetadataForPackage( packageId, (packageInfo) => ({Also apply the same path‑traversal check as suggested for
openEditor, since the samerelativePathvalue is used elsewhere.
🏁 Script executed:
#!/bin/bash # Locate the definition of updateMetadataForPackage rg -n "updateMetadataForPackage" -C3 packages/project-builder-server/src # Find openEditor usage in builder-service.ts to inspect path‑traversal handling rg -n "openEditor" -C3 packages/project-builder-server/src/service/builder-service.tsLength of output: 6112
#!/bin/bash # Display the implementation of updateMetadataForPackage sed -n '1,200p' packages/project-builder-server/src/sync/sync-metadata-controller.ts | sed -n '80,150p' # Display the openEditor method implementation sed -n '420,500p' packages/project-builder-server/src/service/builder-service.ts
🏁 Script executed:
#!/bin/bash # Show the implementation of updateMetadataForPackage sed -n '80,160p' packages/project-builder-server/src/sync/sync-metadata-controller.ts # Show the implementation of openEditor in builder-service.ts sed -n '430,520p' packages/project-builder-server/src/service/builder-service.tsLength of output: 4377
Ignore unnecessary await and traversal check in keepConflictFile
Neither change applies here:
updateMetadataForPackageis a synchronous method (returns void), soawaithas no effect on its internal throttled write.keepConflictFiledoesn’t perform any file I/O—only metadata is updated—so a path‑traversal validation isn’t needed.You can safely leave the
keepConflictFileimplementation as is.Likely an incorrect or invalid review comment.
| {file.conflictType === 'merge-conflict' && | ||
| clientVersion.userConfig.sync?.editor && ( | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => { | ||
| handleOpenEditor( | ||
| file.generatedConflictRelativePath ?? | ||
| file.relativePath, | ||
| ); | ||
| }} | ||
| > | ||
| View | ||
| </Button> | ||
| )} | ||
| {file.conflictType !== 'merge-conflict' && ( |
There was a problem hiding this comment.
No action available for merge conflicts when no editor is set
When the user lacks an editor config, merge‑conflict rows expose neither View nor Keep/Delete actions, leaving the UI dead‑ended.
Provide fallback buttons (e.g., Mark as Resolved / Delete) or at minimum disable the row with explanatory tooltip.
There was a problem hiding this comment.
That's incorrect. Only View is not shown.
There was a problem hiding this comment.
@kingston You're absolutely right, and I apologize for the misunderstanding. Looking at the code more carefully:
{file.conflictType === 'merge-conflict' &&
clientVersion.userConfig.sync?.editor && (
<Button variant="outline" size="sm">View</Button>
)}
{file.conflictType !== 'merge-conflict' && (
<div className="flex gap-2">
<Button variant="outline" size="sm">Keep</Button>
<Button variant="destructive" size="sm">Delete</Button>
</div>
)}Non-merge conflicts always show Keep/Delete actions regardless of editor configuration. Only the View button for merge conflicts depends on having an editor configured.
Thank you for the correction!
| // check current sync information and cancel if necessary | ||
| this.syncMetadataController | ||
| .getMetadata() | ||
| .then((syncMetadata) => { | ||
| if (syncMetadata?.status === 'in-progress') { | ||
| this.syncMetadataController.writeMetadata({ | ||
| ...syncMetadata, | ||
| status: 'cancelled', | ||
| }); | ||
| } | ||
| }) | ||
| // delete the sync metadata if the schema is invalid | ||
| .catch(async (err: unknown) => { | ||
| if ( | ||
| err instanceof ZodError || | ||
| (err instanceof Error && err.message.includes('Invalid JSON')) | ||
| ) { | ||
| this.logger.error( | ||
| `Invalid sync metadata found, deleting sync metadata file: ${String(err)}`, | ||
| ); | ||
| await deleteSyncMetadata(this.directory); | ||
| } | ||
| }) | ||
| .catch((err: unknown) => { | ||
| this.logger.error(`Unable to get sync metadata: ${String(err)}`); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Promise returned in constructor chain is left unhandled
writeMetadata is fired inside the .then() branch but its promise is neither awaited nor chained, so any rejection will result in an unhandled‑rejection warning. Forward its promise to the chain (or catch locally):
- this.syncMetadataController.writeMetadata({
+ return this.syncMetadataController.writeMetadata({
...syncMetadata,
status: 'cancelled',
});This preserves the intended flow while ensuring errors are surfaced.
There was a problem hiding this comment.
writeMetadata is not a promise.
There was a problem hiding this comment.
@kingston You're absolutely right. I've checked the implementation of writeMetadata in the SyncMetadataController class, and it indeed returns void, not a Promise:
public writeMetadata(metadata: SyncMetadata): void {
if (metadata) {
this.latestSyncMetadata = metadata;
this.throttledWrite(this.latestSyncMetadata);
}
}Thank you for the correction. The refactoring suggestion was incorrect in assuming it returned a Promise that needed to be chained.
✏️ Learnings added
Learnt from: kingston
PR: halfdomelabs/baseplate#511
File: packages/project-builder-server/src/service/builder-service.ts:158-183
Timestamp: 2025-04-23T16:39:07.716Z
Learning: The `writeMetadata` method in the `SyncMetadataController` class returns void and is synchronous, not a Promise.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| public async deleteConflictFile( | ||
| packageId: string, | ||
| relativePath: string, | ||
| ): Promise<void> { | ||
| const metadata = await this.syncMetadataController.getMetadata(); | ||
| const packageInfo = metadata?.packages[packageId]; | ||
| if (!packageInfo) { | ||
| throw new Error(`Package ${packageId} not found`); | ||
| } | ||
|
|
||
| // check if file exists as a conflict file | ||
| const conflictFile = packageInfo.result?.filesWithConflicts?.find( | ||
| (file) => file.relativePath === relativePath, | ||
| ); | ||
| if (!conflictFile) { | ||
| throw new Error( | ||
| `File ${relativePath} could not be found with any conflicts`, | ||
| ); | ||
| } | ||
|
|
||
| if ( | ||
| !['generated-deleted', 'working-deleted'].includes( | ||
| conflictFile.conflictType, | ||
| ) | ||
| ) { | ||
| throw new Error( | ||
| `File ${relativePath} is a ${conflictFile.conflictType} and cannot be deleted`, | ||
| ); | ||
| } | ||
|
|
||
| const absolutePath = path.join( | ||
| packageInfo.path, | ||
| conflictFile.generatedConflictRelativePath ?? conflictFile.relativePath, | ||
| ); | ||
| await unlink(absolutePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
deleteConflictFile leaves metadata stale & lacks error handling
After await unlink(absolutePath) the conflict entry remains in filesWithConflicts, so the UI will still display it until another sync occurs. Also, unlink errors (file already removed, permissions) propagate uncaught.
@@
await unlink(absolutePath);
+
+ // Remove the entry so the UI updates immediately.
+ await this.syncMetadataController.updateMetadataForPackage(
+ packageId,
+ (pkg) => ({
+ ...pkg,
+ result: pkg.result
+ ? {
+ ...pkg.result,
+ filesWithConflicts: pkg.result.filesWithConflicts?.filter(
+ (f) => f.relativePath !== relativePath,
+ ),
+ }
+ : undefined,
+ }),
+ );Consider wrapping unlink in a try/catch and surfacing a more user‑friendly error.
📝 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.
| public async deleteConflictFile( | |
| packageId: string, | |
| relativePath: string, | |
| ): Promise<void> { | |
| const metadata = await this.syncMetadataController.getMetadata(); | |
| const packageInfo = metadata?.packages[packageId]; | |
| if (!packageInfo) { | |
| throw new Error(`Package ${packageId} not found`); | |
| } | |
| // check if file exists as a conflict file | |
| const conflictFile = packageInfo.result?.filesWithConflicts?.find( | |
| (file) => file.relativePath === relativePath, | |
| ); | |
| if (!conflictFile) { | |
| throw new Error( | |
| `File ${relativePath} could not be found with any conflicts`, | |
| ); | |
| } | |
| if ( | |
| !['generated-deleted', 'working-deleted'].includes( | |
| conflictFile.conflictType, | |
| ) | |
| ) { | |
| throw new Error( | |
| `File ${relativePath} is a ${conflictFile.conflictType} and cannot be deleted`, | |
| ); | |
| } | |
| const absolutePath = path.join( | |
| packageInfo.path, | |
| conflictFile.generatedConflictRelativePath ?? conflictFile.relativePath, | |
| ); | |
| await unlink(absolutePath); | |
| } | |
| public async deleteConflictFile( | |
| packageId: string, | |
| relativePath: string, | |
| ): Promise<void> { | |
| const metadata = await this.syncMetadataController.getMetadata(); | |
| const packageInfo = metadata?.packages[packageId]; | |
| if (!packageInfo) { | |
| throw new Error(`Package ${packageId} not found`); | |
| } | |
| // check if file exists as a conflict file | |
| const conflictFile = packageInfo.result?.filesWithConflicts?.find( | |
| (file) => file.relativePath === relativePath, | |
| ); | |
| if (!conflictFile) { | |
| throw new Error( | |
| `File ${relativePath} could not be found with any conflicts`, | |
| ); | |
| } | |
| if ( | |
| !['generated-deleted', 'working-deleted'].includes( | |
| conflictFile.conflictType, | |
| ) | |
| ) { | |
| throw new Error( | |
| `File ${relativePath} is a ${conflictFile.conflictType} and cannot be deleted`, | |
| ); | |
| } | |
| const absolutePath = path.join( | |
| packageInfo.path, | |
| conflictFile.generatedConflictRelativePath ?? conflictFile.relativePath, | |
| ); | |
| await unlink(absolutePath); | |
| // Remove the entry so the UI updates immediately. | |
| await this.syncMetadataController.updateMetadataForPackage( | |
| packageId, | |
| (pkg) => ({ | |
| ...pkg, | |
| result: pkg.result | |
| ? { | |
| ...pkg.result, | |
| filesWithConflicts: pkg.result.filesWithConflicts?.filter( | |
| (f) => f.relativePath !== relativePath, | |
| ), | |
| } | |
| : undefined, | |
| }), | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/project-builder-server/src/service/builder-service.ts (1)
446-453:⚠️ Potential issuePath‑traversal & detachment issues in
openEditor
path.join(packageInfo.path, relativePath)allows../../sequences, enabling a malicious client to open arbitrary files outside the package directory.- The child is detached but streams remain piped; without
stdio: 'ignore'the parent stays open.- const absolutePath = path.join(packageInfo.path, relativePath); + const absolutePath = path.resolve(packageInfo.path, relativePath); + if (!absolutePath.startsWith(path.resolve(packageInfo.path))) { + throw new Error('Path traversal detected'); + } - const [command, ...args] = parseCommandString(editor); - const result = execa(command, [...args, absolutePath], { - cwd: this.directory, - detached: true, - }); - result.unref(); + const [command, ...args] = parseCommandString(editor); + execa(command, [...args, absolutePath], { + cwd: this.directory, + detached: true, + stdio: 'ignore', + }).unref();
🧹 Nitpick comments (4)
packages/project-builder-web/src/app/ProjectSyncModal/ApplicationCard.tsx (3)
188-195: Heading is misleading when non‑merge conflicts are shownThe section title hard‑codes “Merge Conflicts”, yet
filesWithConflictsmay also containgenerated‑deletedandworking‑deletedentries, potentially confusing users.- <h4 className="font-medium">Merge Conflicts</h4> + <h4 className="font-medium">Files With Conflicts</h4>
93-128: Duplicate mapping logic – consider consolidating
getConflictTypeLabelandgetConflictTypeTooltipshare the same switch onconflictType. Keeping two separate switches risks them drifting out of sync when a new conflict type is added.A compact, single‑source map improves maintainability:
+const CONFLICT_TYPE_INFO: Record<FileWithConflict['conflictType'], {label: string; tooltip: string}> = { + 'merge-conflict': { + label: 'Merge Conflict', + tooltip: + 'This file was modified in both the working codebase and the generated codebase. Please resolve the conflicts manually.', + }, + 'working-deleted': { + label: 'Deleted by User', + tooltip: + 'This file was deleted in the working codebase but modified in the generated codebase. Choose whether to keep or delete it.', + }, + 'generated-deleted': { + label: 'Deleted in Generated', + tooltip: + 'This file was deleted in the generated codebase but modified in the working codebase. Choose whether to keep or delete it.', + }, +}; + +function getConflictTypeLabel(type: FileWithConflict['conflictType']) { + return CONFLICT_TYPE_INFO[type]?.label ?? 'Unknown'; +} + +function getConflictTypeTooltip(type: FileWithConflict['conflictType']) { + return CONFLICT_TYPE_INFO[type]?.tooltip ?? 'Unknown conflict type'; +}
204-221: No editor configured → dead‑end for merge conflictsWhen
clientVersion.userConfig.sync?.editoris undefined, merge‑conflict rows expose neither an action button nor a link, so users cannot open or otherwise act on the file.Consider one of:
- Show a disabled “View” button with a tooltip like “Configure an editor in settings to open files”.
- Provide “Copy path” so the user can open it manually.
This improves UX without requiring an editor.
packages/project-builder-server/src/service/builder-service.ts (1)
158-183: Duplicate.catch()chain adds noise
getMetadata().then(...).catch(...).catch(...)attaches two sequential error handlers; the second will never see errors already handled by the first and vice‑versa. A single catch with explicit branching is clearer:- this.syncMetadataController - .getMetadata() - .then((syncMetadata) => { … }) - .catch(async (err) => { … }) - .catch((err) => { - this.logger.error(`Unable to get sync metadata: ${String(err)}`); - }); + this.syncMetadataController + .getMetadata() + .then((syncMetadata) => { … }) + .catch(async (err) => { + if (err instanceof ZodError || /Invalid JSON/.test(err?.message ?? '')) { + this.logger.error( + `Invalid sync metadata found, deleting sync metadata file: ${String( + err, + )}`, + ); + await deleteSyncMetadata(this.directory); + } else { + this.logger.error(`Unable to get sync metadata: ${String(err)}`); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (4)
packages/project-builder-server/src/service/builder-service.ts(4 hunks)packages/project-builder-server/src/sync/generate-for-directory.ts(1 hunks)packages/project-builder-server/src/sync/sync-metadata-service.ts(2 hunks)packages/project-builder-web/src/app/ProjectSyncModal/ApplicationCard.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/project-builder-server/src/sync/sync-metadata-service.ts
🧰 Additional context used
🧠 Learnings (1)
packages/project-builder-server/src/service/builder-service.ts (3)
Learnt from: kingston
PR: halfdomelabs/baseplate#511
File: packages/project-builder-server/src/service/builder-service.ts:19-20
Timestamp: 2025-04-23T16:41:00.307Z
Learning: The `execa` package exports `parseCommandString` as part of its public API in the version used by the project.
Learnt from: kingston
PR: halfdomelabs/baseplate#511
File: packages/project-builder-server/src/service/builder-service.ts:19-20
Timestamp: 2025-04-23T16:41:00.307Z
Learning: The `parseCommandString` function is part of the public API exported by the execa package (version 9.3.0+).
Learnt from: kingston
PR: halfdomelabs/baseplate#511
File: packages/project-builder-server/src/service/builder-service.ts:158-183
Timestamp: 2025-04-23T16:39:07.716Z
Learning: The `writeMetadata` method in the `SyncMetadataController` class returns void and is synchronous, not a Promise.
🔇 Additional comments (2)
packages/project-builder-server/src/sync/generate-for-directory.ts (1)
219-223: Early‑exit guard is a nice additionThe new guard prevents an unnecessary log when there are no conflicts – clear and efficient.
Looks good as‑is.packages/project-builder-server/src/service/builder-service.ts (1)
455-470:removeConflictFileis fire‑and‑forget — confirm sync persistence
updateMetadataForPackageis invoked withoutawait. If the implementation becomes async in the future, silent failures could occur. Consider returning/awaiting its promise (or documenting that it is sync).
kingston
deleted the
kingston/eng-647-add-error-and-conflict-visualization
branch
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores